Install to Claude Code
npx -y skills add https://github.com/cognitedata/builder-skills --skill code-qualityDescription
code quality
SKILL.md
---
name: code-quality
description: "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, lint, linting, TypeScript strict, dependency injection, file structure."
allowed-tools: Read, Glob, Grep, Shell, Write
metadata:
argument-hint: "[file, directory, or PR branch to review — e.g. 'src/components/AssetPanel.tsx']"
---
# 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: "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<UseMyHookContextType>(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<Deps>) => {
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*['\"]|<Route" src/
```
Rules:
- `console.log` and `console.debug` must be removed before shipping (use proper error logging for `console.error`).
- Commented-out code blocks must be removed — version control preserves history.
- `TODO` and `FIXME` comments older than the current sprint should be resolved or converted to tracked issues.
- Unused imports are caught by the linter (Step 1); confirm they are gone.
**Hard gate:** Unreachable pages, entirely unused files, and significant dead code blocks **must** be removed before approval. These are blocking findings.
---
## Step 9 — Verify file and export structure
Every feature area should follow a consistent structure. Check that the app's layout matches this pattern:
```
src/
├── components/ # Shared presentational components
│ └── <name>/
│ ├── <name>.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.