code-quality

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:

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:

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
    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:

# 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:

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.