# Code Quality

> MUST be used whenever reviewing a Flows app for code quality, maintainability, or clean code issues — before a PR review, after a feature is complete, or when the user asks for a code review. Do NOT skip linting steps. Triggers: code quality, code review, clean code, refactor, maintainability, technical debt, any type, naming, dead code, duplication, DRY, single responsibility, component size, li…

- **Type:** Skill
- **Install:** `agentstack add skill-cognitedata-builder-skills-code-quality`
- **Verified:** Yes — security-reviewed for prompt injection and unsafe behavior
- **Seller:** [cognitedata](https://agentstack.voostack.com/s/cognitedata)
- **Installs:** 0
- **Category:** [Agent Skills](https://agentstack.voostack.com/c/agent-skills)
- **Latest version:** 0.1.0
- **License:** Apache-2.0
- **Upstream author:** [cognitedata](https://github.com/cognitedata)
- **Source:** https://github.com/cognitedata/builder-skills/tree/main/skills/code-quality

## Install

```sh
agentstack add skill-cognitedata-builder-skills-code-quality
```

Requires the [AgentStack CLI](https://agentstack.voostack.com/docs/cli). Works with Claude Code, Cursor, and any MCP-compatible agent.

## About

# Code Quality Review

Review **$ARGUMENTS** (or the whole app if no argument is given) for code quality issues. Work through every step below in order and report all findings with file paths and line numbers.

---

## Step 1 — Run the linter first

Before reading any code manually, get a baseline from the automated tools:

```bash
pnpm run lint
```

List every error and warning. Fix all errors before proceeding — lint errors are not negotiable. Warnings should be reviewed and resolved unless there is a documented exception.

Also run the TypeScript compiler in strict mode to surface any hidden type issues:

```bash
pnpm exec tsc --noEmit
```

List every type error. These must be fixed.

---

## Step 2 — TypeScript type safety

### 2a — Eliminate `any` types

Search for `any` usage across the codebase:

```bash
grep -rn --include="*.ts" --include="*.tsx" -E ": any|as any|" src/
```

For each hit, replace with the correct type. Common substitutions:

| Instead of | Use |
|------------|-----|
| `any` for unknown external data | `unknown` + type guard or Zod parse |
| `any` for event handlers | `React.ChangeEvent`, `React.MouseEvent`, etc. |
| `any` for CDF responses | The SDK's own response types (import from `@cognite/sdk`) |
| `any[]` for arrays | `T[]` with the correct generic |
| `as any` casts | Proper type narrowing or explicit overloaded function signature |

The goal is zero `any` in `src/`. If a third-party library forces it, wrap the call in a typed adapter function so `any` does not leak into the app.

### 2b — Make impossible states unrepresentable

Use the type system to make invalid states fail at compile time. Fewer reachable states = easier code to read and change.

**Branded types** — brand primitives so they can't be mixed up. Validate once at the boundary; downstream code trusts the type.

```ts
type PhoneNumber = string & { __brand: "PhoneNumber" };

function parsePhone(input: string): PhoneNumber {
  if (!/^\+?\d{10,15}$/.test(input)) throw new Error(`Invalid: ${input}`);
  return input as PhoneNumber;
}
```

If the project uses a library with native branded-type support (e.g. Effect), use their primitives instead of rolling your own.

**Discriminated unions over flag bags** — replace boolean/optional combos with an exhaustive union:

```ts
// Don't — invalid combos representable
type State = { loading: boolean; user?: User; error?: string };

// Do — only valid states exist
type State =
  | { status: "loading" }
  | { status: "success"; user: User }
  | { status: "error"; error: string };
```

Search for flag-bag patterns:

```bash
grep -rn --include="*.ts" --include="*.tsx" -E "loading\?|isLoading.*isError|isSuccess.*isError" src/
```

Flag every type that combines boolean flags where only certain combos are valid. These should be discriminated unions.

### 2c — Let types flow end-to-end

DB schema → server → client should share types without manual duplication. Don't restate types you can derive — reach for `Pick`, `Omit`, `Parameters`, `ReturnType`, `Awaited`, `typeof` before writing a new interface.

```ts
// Don't — duplicate shape, drifts when the row changes
type UserSummary = { id: string; email: Email };
function renderUser(u: UserSummary) { /* ... */ }

// Do — derive from the source of truth
type User = Awaited>;
function renderUser(u: Pick) { /* ... */ }
```

```bash
# Find manually duplicated type shapes
grep -rn --include="*.ts" --include="*.tsx" -E "^(export )?type \w+Summary|^(export )?interface \w+DTO" src/
```

Flag interfaces that manually restate fields already present on an SDK or DB type — these should use `Pick`/`Omit` instead.

### 2d — Pass objects, not positional arguments

Functions with two or more parameters of the same primitive type should receive a named-property object so callers can't silently swap arguments.

```ts
// Don't — swap two args, still compiles
sendEmail("Welcome!", "Hi there");

// Do — order-independent, self-documenting
sendEmail({ to: "alice@x.com", subject: "Welcome!", body: "Hi there" });
```

```bash
# Find functions with multiple string/number parameters (potential swap bugs)
grep -rn --include="*.ts" --include="*.tsx" -E "^\s*(export\s+)?(function|const)\s+\w+\s*\([^)]*string[^)]*string" src/
```

---

## Step 3 — Check component size and single responsibility

List all `.tsx` files with their line counts:

```bash
node -e "const fs=require('fs'),path=require('path');function walk(d){return fs.readdirSync(d,{withFileTypes:true}).flatMap(e=>{const p=path.join(d,e.name);return e.isDirectory()?walk(p):p.endsWith('.tsx')?[p]:[]})}walk('src').map(p=>({p,l:fs.readFileSync(p,'utf8').split('\n').length})).sort((a,b)=>b.l-a.l).forEach(({l,p})=>console.log(l,p))"
```

Flag every component file over **150 lines**. For each, read it and check:

- Does it do more than one thing? (fetch data AND render UI AND handle form state)
- Can the fetch logic move to a custom hook (`useAssetData`)?
- Can sub-sections be extracted as named sub-components?

Apply the split only when it creates a genuinely cleaner separation — do not split for the sake of line count alone. A well-named 200-line component is better than three poorly-named 60-line ones.

---

## Step 4 — Find and remove duplicate logic (DRY)

Search for copy-pasted patterns across hooks, utilities, and components:

```bash
# Find repeated fetch patterns
grep -rn --include="*.ts" --include="*.tsx" -E "sdk\.(assets|timeseries|events|files)\.(list|retrieve)" src/

# Find repeated formatting functions
grep -rn --include="*.ts" --include="*.tsx" -E "toLocaleDateString|toLocaleString|new Date\(" src/

# Find repeated className strings longer than 40 chars
grep -rn --include="*.tsx" -E 'className="[^"]{40,}"' src/
```

For each set of duplicates:
- Extract to `src/utils/` if it is a pure function
- Extract to `src/hooks/` if it contains React state or effects
- Extract to a shared component if it is JSX

---

## Step 5 — Enforce dependency injection for external calls

Components and hooks must not import the CDF client directly. The SDK client must be obtained from context (via `useCogniteClient()` or a prop) so the component is testable in isolation.

```bash
grep -rn --include="*.ts" --include="*.tsx" -E "new CogniteClient|createCogniteClient" src/
```

Flag any direct client construction outside of the app's bootstrap / auth setup file. The pattern should always be:

```ts
// GOOD — client comes from context
export function useMyData() {
  const sdk = useCogniteClient(); // from Flows auth context
  // ...
}

// BAD — direct construction inside a hook or component
const sdk = new CogniteClient({ project: "my-project", ... });
```

Similarly, Atlas tools should receive their dependencies via `execute`'s closure over a hook-provided ref, not by importing a global singleton.

---

## Step 6 — Verify coding patterns and testability

Check that the codebase follows the three core patterns required by the Flows app review process. These patterns keep code testable, maintainable, and consistent.

### 6a — Dependency injection via React context

Hooks must declare their dependencies through a context type and consume them via `useContext`, not by importing them directly. This enables testing without module-level mocks.

```bash
# Find hooks that import other hooks/services directly (potential DI violation)
grep -rn --include="*.ts" --include="*.tsx" -E "^import.*from\s+['\"]\.\./" src/hooks/

# Find hooks that use useContext for dependency injection (good pattern)
grep -rn --include="*.ts" --include="*.tsx" "useContext" src/hooks/
```

The preferred pattern:

```typescript
// GOOD — injectable via context
const defaultDependencies = { useDataSource, useAnalytics };
export type UseMyHookContextType = typeof defaultDependencies;
export const UseMyHookContext = createContext(defaultDependencies);
export function useMyHook() {
  const { useDataSource } = useContext(UseMyHookContext);
}

// BAD — hard-coded import, requires vi.mock to test
import { useDataSource } from '../data/useDataSource';
export function useMyHook() { const data = useDataSource(); }
```

For non-React code (utilities, services), use **factory functions with partial dependency overrides**:

```typescript
type Deps = { serviceFactory: () => SomeService };
const defaultDeps: Deps = { serviceFactory: () => new SomeServiceImpl() };
export const doSomething = async (props: Props, depOverrides?: Partial) => {
  const deps = { ...defaultDeps, ...depOverrides };
  const service = deps.serviceFactory();
};
```

Flag every hook that imports dependencies directly instead of receiving them through context. These are testability concerns even if tests exist today.

### 6b — Interface-based services

Service classes must implement explicit TypeScript interfaces. This keeps production code substitutable and test doubles type-safe.

```bash
# Find service/class definitions and check for interface implementations
grep -rn --include="*.ts" --include="*.tsx" -E "class\s+\w+(Service|Client|Repository|Manager)" src/

# Find unsafe casts in production AND test code
grep -rn --include="*.ts" --include="*.tsx" "as unknown as" src/
```

Flag:
- Service classes that do not implement an explicit interface
- `as unknown as T` casts in either production or test code — this signals poor interface design

### 6c — ViewModel pattern

Page-level hooks (`useSomethingViewModel`) must separate business logic from presentation. UI components receive data and callbacks only; they contain no data-fetching, side-effect logic, or direct SDK calls.

```bash
# Find page/view components
grep -rn --include="*.tsx" --include="*.ts" -l "useQuery\|useMutation\|sdk\.\|client\." src/pages/ src/views/ 2>/dev/null

# Find ViewModel hooks
grep -rn --include="*.ts" --include="*.tsx" -l "ViewModel" src/hooks/ 2>/dev/null
```

Flag:
- Page components that contain `useQuery`, `useMutation`, or direct SDK calls — this logic should be in a ViewModel hook
- Missing ViewModel hooks for pages with non-trivial data logic

### 6d — Test mock quality

```bash
# Find vi.mock usage — each should have a comment justifying why context injection wasn't used
grep -rn --include="*.ts" --include="*.tsx" "vi\.mock" src/

# Find unsafe test casts
grep -rn --include="*.ts" --include="*.tsx" "as unknown as" src/ | grep -E "\.test\.|\.spec\."
```

Flag:
- `vi.mock` usage without a justification comment explaining why context injection was not possible
- `as unknown as T` casts in test files — signals poor interface design in the production code

---

## Step 7 — Check naming conventions

Read a representative sample of files and verify:

| Artifact | Convention | Examples |
|----------|-----------|---------|
| Files & directories | `kebab-case` | `asset-panel.tsx`, `use-asset-data.ts` |
| React components | `PascalCase` | `AssetPanel`, `NavigationBar` |
| Variables, functions, hooks | `camelCase` | `isLoading`, `fetchAssets`, `useAssetData` |
| Constants (module-level) | `SCREAMING_SNAKE_CASE` | `MAX_ITEMS`, `AGENT_EXTERNAL_ID` |
| TypeScript types & interfaces | `PascalCase` | `AssetNode`, `ChartConfig` |
| Boolean variables | Auxiliary verb prefix | `isLoading`, `hasError`, `canEdit` |

Search for common violations:

```bash
# TSX components not in PascalCase (filename starts with lowercase)
node -e "const fs=require('fs'),path=require('path');function walk(d){return fs.readdirSync(d,{withFileTypes:true}).flatMap(e=>{const p=path.join(d,e.name);return e.isDirectory()?walk(p):p.endsWith('.tsx')?[p]:[]})}walk('src').filter(p=>/^[a-z]/.test(path.basename(p))).forEach(p=>console.log(p))"

# Hook files not prefixed with "use"
node -e "const fs=require('fs');fs.readdirSync('src/hooks').filter(f=>f.endsWith('.ts')&&!f.startsWith('use')).forEach(f=>console.log('src/hooks/'+f))"
```

---

## Step 8 — Remove dead code

```powershell
# Find commented-out code blocks (3+ consecutive commented lines)
Get-ChildItem -Recurse -Include "*.ts","*.tsx" src | ForEach-Object {
    $file = $_; $lines = Get-Content $file.FullName
    $count = 0; $startLine = 0
    for ($i = 0; $i -lt $lines.Count; $i++) {
        if ($lines[$i] -match '^\s*//') {
            if ($count -eq 0) { $startLine = $i + 1 }
            $count++
        } else {
            if ($count -ge 3) { "$($file.FullName):$startLine — $count consecutive comment lines" }
            $count = 0
        }
    }
    if ($count -ge 3) { "$($file.FullName):$startLine — $count consecutive comment lines" }
}

# Find console.log/debug statements
grep -rn --include="*.tsx" --include="*.ts" -E "console\.(log|debug|warn|error|info)" src/

# Find TODO/FIXME/HACK comments
grep -rn --include="*.tsx" --include="*.ts" -E "(TODO|FIXME|HACK|XXX):" src/
```

Search for unreachable pages (routes defined in the router but whose component is never imported or rendered) and entirely unused files:

```bash
# Find all .ts/.tsx files and check if they are imported anywhere
for file in $(find src -name "*.ts" -o -name "*.tsx" | grep -v ".test." | grep -v ".spec." | grep -v "node_modules"); do
  basename=$(basename "$file" | sed 's/\.[^.]*$//')
  imports=$(grep -rn --include="*.ts" --include="*.tsx" "$basename" src/ | grep -v "$file" | wc -l)
  if [ "$imports" -eq 0 ]; then
    echo "UNUSED: $file"
  fi
done

# Find route definitions and verify their components are imported
grep -rn --include="*.tsx" --include="*.ts" -E "path:\s*['\"]|/
│       ├── .tsx
│       └── index.ts    # re-exports the public API
├── hooks/              # Custom hooks (each file = one hook)
├── utils/              # Pure utility functions (no React)
├── contexts/           # React context providers
├── pages/ or views/    # Route-level components
└── types/              # Shared TypeScript types
```

Flag:
- Business logic sitting directly in page components (should be in hooks)
- Utility functions living inside component files (should be in `utils/`)
- Types defined inline in component files when they are used across multiple files (should be in `types/`)
- Missing `index.ts` barrel files for component directories (makes imports verbose)

---

## Step 10 — Report findings

Produce a structured report grouped by category:

| Category | File | Line | Issue | Recommendation |
|----------|------|------|-------|----------------|
| TypeScript | `src/hooks/useData.ts` | 18 | `response as any` cast | Import and use `NodeItem` type from `@cognite/sdk` |
| Size | `src/components/Dashboard.tsx` | — | 340 lines, mixes fetch and render logic | Extract `useDashboardData` hook (~120 lines) |
| DRY | `src/components/A.tsx`, `src/components/B.tsx` | 45, 62 | Identical date formatter | Extract to `src/utils/formatDate.ts` |
| Naming | `src/hooks/data.ts` | — | File name does not start with `use` | Rename to `useData.ts` |
| Dead code | `src/App.tsx` | 88 | `console.log("debug response", data)` | Remove |

If no issues are found in a step, state "No issues found" for that step. Do not skip steps silently.

---

## Done

Summarize the total number of findings by category and list the highest-impact items to address first. Any `any` type and lint error must be treated as blocking — list these separately.

## Source & license

This open-source skill is cataloged on AgentStack and links to its original source — we do not rehost the code.

- **Author:** [cognitedata](https://github.com/cognitedata)
- **Source:** [cognitedata/builder-skills](https://github.com/cognitedata/builder-skills)
- **License:** Apache-2.0

Install and usage instructions live in the source repository linked above.

## Pricing

- **Free** — Free

## Security capabilities

Automated source analysis of v0.1.0 — what this tool can access:

- **Network access:** no
- **Filesystem access:** yes
- **Shell / process execution:** no
- **Environment & secrets:** no
- **Dynamic code execution:** no

*"Yes" means the capability is present in the source — more access means more to trust, not that it is unsafe.*


## Versions

- **0.1.0** — security scan: passed — Imported from the upstream source.

## Links

- Listing page: https://agentstack.voostack.com/l/skill-cognitedata-builder-skills-code-quality
- Seller: https://agentstack.voostack.com/s/cognitedata
- Browse the marketplace: https://agentstack.voostack.com/browse

---
Listed on AgentStack — the marketplace for AI agent skills and MCP servers. Every listing is security-reviewed. Creators keep 70%.