code-quality

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: 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: 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: 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. 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: // 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: 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. // 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" > ) { /* ... */ } # 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. // 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" }); # 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: 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: # 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. 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: // 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. # 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: // 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 : 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. # 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. # 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 # 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: # 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 # 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