code-quality

What this skill does

# 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|<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<HTMLInputElement>`, `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<ReturnType<typeof db.query.users.findFirst>>;
function renderUser(u: Pick<User, "id" | "email">) { /* ... */ }
```

```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: "[email protected]", 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<UseMyHookContextType>(defaultDependencies);
export function useMyHook() {
  const { useD