refactor(frontend): extract extractErrorCode() helper to eliminate repeated as-unknown-as type assertions #113

New Issue

marcel · 2026-03-27T17:53:17+01:00

marcel commented

2026-03-27 17:53:17 +01:00

Problem

The pattern (result.error as unknown as { code?: string })?.code appears 10+ times across load functions. Double-casting through unknown means TypeScript cannot catch shape mismatches — it is the equivalent of any with extra steps.

Fix

Define the backend error shape once in src/lib/api.server.ts:

export interface ApiError {
  code?: string;
  message?: string;
}

export function extractErrorCode(error: unknown): string | undefined {
  return (error as ApiError | undefined)?.code;
}

Then replace all call sites:

// Before
const code = (docResult.error as unknown as { code?: string })?.code;

// After
const code = extractErrorCode(docResult.error);

Acceptance Criteria

ApiError interface and extractErrorCode() defined in api.server.ts
All as unknown as { code?: string } occurrences replaced across all load functions
No TypeScript errors introduced
npm run check passes cleanly

## Problem The pattern `(result.error as unknown as { code?: string })?.code` appears 10+ times across load functions. Double-casting through `unknown` means TypeScript cannot catch shape mismatches — it is the equivalent of `any` with extra steps. ## Fix Define the backend error shape once in `src/lib/api.server.ts`: ```typescript export interface ApiError { code?: string; message?: string; } export function extractErrorCode(error: unknown): string | undefined { return (error as ApiError | undefined)?.code; } ``` Then replace all call sites: ```typescript // Before const code = (docResult.error as unknown as { code?: string })?.code; // After const code = extractErrorCode(docResult.error); ``` ## Acceptance Criteria - `ApiError` interface and `extractErrorCode()` defined in `api.server.ts` - All `as unknown as { code?: string }` occurrences replaced across all load functions - No TypeScript errors introduced - `npm run check` passes cleanly

marcel added the feature label 2026-03-31 20:49:49 +02:00

marcel added refactor and removed feature labels 2026-03-31 20:51:47 +02:00

marcel commented

2026-03-31 20:58:48 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Questions & Observations

This is exactly the right time to extract: 10+ identical patterns, a clear stable name, and a genuinely simpler call site. The KISS/DRY balance is correct here.
The placement in api.server.ts is good — it's the boundary file where errors originate. Make sure it's exported so any load function can import it directly without going through a barrel file.
The ApiError interface should be exported too. Even if it's only used internally now, it gives TypeScript something to check against if the backend error shape ever changes.

Suggestions

Write a Vitest unit test before the refactor:

describe('extractErrorCode', () => {
  it('returns the code from a backend error', () => {
    expect(extractErrorCode({ code: 'DOC_NOT_FOUND' })).toBe('DOC_NOT_FOUND');
  });
  it('returns undefined when error is undefined', () => {
    expect(extractErrorCode(undefined)).toBeUndefined();
  });
  it('returns undefined when code field is absent', () => {
    expect(extractErrorCode({ message: 'oops' })).toBeUndefined();
  });
});

After the refactor: run grep -r "as unknown as" frontend/src — the result should be empty. If not, you missed a call site.
npm run check must pass cleanly as the acceptance criterion states — add it to the PR checklist.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Questions & Observations - This is exactly the right time to extract: 10+ identical patterns, a clear stable name, and a genuinely simpler call site. The KISS/DRY balance is correct here. - The placement in `api.server.ts` is good — it's the boundary file where errors originate. Make sure it's exported so any load function can import it directly without going through a barrel file. - The `ApiError` interface should be exported too. Even if it's only used internally now, it gives TypeScript something to check against if the backend error shape ever changes. ### Suggestions - Write a Vitest unit test before the refactor: ```typescript describe('extractErrorCode', () => { it('returns the code from a backend error', () => { expect(extractErrorCode({ code: 'DOC_NOT_FOUND' })).toBe('DOC_NOT_FOUND'); }); it('returns undefined when error is undefined', () => { expect(extractErrorCode(undefined)).toBeUndefined(); }); it('returns undefined when code field is absent', () => { expect(extractErrorCode({ message: 'oops' })).toBeUndefined(); }); }); ``` - After the refactor: run `grep -r "as unknown as" frontend/src` — the result should be empty. If not, you missed a call site. - `npm run check` must pass cleanly as the acceptance criterion states — add it to the PR checklist.

marcel commented

2026-03-31 21:00:42 +02:00

🔒 Nora "NullX" Steiner — Application Security Engineer

No direct security concerns here — this is a type-safety refactor with no security-sensitive behaviour change.

One observation worth noting: the as unknown as { code?: string } pattern is type-system evasion. It doesn't create a vulnerability today, but it does mean TypeScript cannot catch a future case where the backend changes its error shape and the frontend silently passes undefined to getErrorMessage() — potentially hiding an error that should be surfaced to the user. The ApiError interface makes this one step more robust, though it's still a runtime shape assumption. If the backend ever adds a formal problem+JSON response body (RFC 7807), a stricter validation at the boundary would be worth considering then.

No security findings to add — this is the right thing to do for code health.

## 🔒 Nora "NullX" Steiner — Application Security Engineer No direct security concerns here — this is a type-safety refactor with no security-sensitive behaviour change. One observation worth noting: the `as unknown as { code?: string }` pattern is type-system evasion. It doesn't create a vulnerability today, but it does mean TypeScript cannot catch a future case where the backend changes its error shape and the frontend silently passes `undefined` to `getErrorMessage()` — potentially hiding an error that should be surfaced to the user. The `ApiError` interface makes this one step more robust, though it's still a runtime shape assumption. If the backend ever adds a formal problem+JSON response body (RFC 7807), a stricter validation at the boundary would be worth considering then. No security findings to add — this is the right thing to do for code health.

marcel commented

2026-03-31 21:02:28 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Test Strategy

This is a pure unit test target — a small, pure function with no side effects.

Vitest unit tests (extractErrorCode.test.ts):

import { describe, it, expect } from 'vitest';
import { extractErrorCode } from '$lib/api.server';

describe('extractErrorCode', () => {
  it('extracts code from a standard API error', () => {
    expect(extractErrorCode({ code: 'DOC_NOT_FOUND' })).toBe('DOC_NOT_FOUND');
  });
  it('returns undefined when error is null', () => {
    expect(extractErrorCode(null)).toBeUndefined();
  });
  it('returns undefined when error is undefined', () => {
    expect(extractErrorCode(undefined)).toBeUndefined();
  });
  it('returns undefined when code field is missing', () => {
    expect(extractErrorCode({ message: 'something went wrong' })).toBeUndefined();
  });
  it('returns undefined when code is not a string', () => {
    expect(extractErrorCode({ code: 42 })).toBeUndefined();
  });
});

Observations

The last case (non-string code) is worth adding given the as unknown origin — we know nothing about the runtime shape.
No integration or E2E coverage needed. This function has no I/O.
After the refactor, the existing load function tests should still pass unchanged — that's the regression signal.

## 🧪 Sara Holt — QA Engineer & Test Strategist ### Test Strategy This is a pure unit test target — a small, pure function with no side effects. **Vitest unit tests (`extractErrorCode.test.ts`):** ```typescript import { describe, it, expect } from 'vitest'; import { extractErrorCode } from '$lib/api.server'; describe('extractErrorCode', () => { it('extracts code from a standard API error', () => { expect(extractErrorCode({ code: 'DOC_NOT_FOUND' })).toBe('DOC_NOT_FOUND'); }); it('returns undefined when error is null', () => { expect(extractErrorCode(null)).toBeUndefined(); }); it('returns undefined when error is undefined', () => { expect(extractErrorCode(undefined)).toBeUndefined(); }); it('returns undefined when code field is missing', () => { expect(extractErrorCode({ message: 'something went wrong' })).toBeUndefined(); }); it('returns undefined when code is not a string', () => { expect(extractErrorCode({ code: 42 })).toBeUndefined(); }); }); ``` ### Observations - The last case (non-string code) is worth adding given the `as unknown` origin — we know nothing about the runtime shape. - No integration or E2E coverage needed. This function has no I/O. - After the refactor, the existing load function tests should still pass unchanged — that's the regression signal.

marcel commented

2026-03-31 21:04:28 +02:00

🏗️ Markus Keller — Application Architect

Questions & Observations

Correct placement. api.server.ts is the right home — it's the module that wraps all backend communication, so error shape knowledge belongs there. Don't put this in a separate utils.ts barrel; keep it co-located with createApiClient.
Export both the function and the interface. The ApiError interface should be exported. Load functions in separate route files will need to import it if they ever want to type a caught error explicitly — and keeping it importable from one canonical location is cleaner than each route re-declaring the shape.
This is exactly KISS winning over DRY. 10+ occurrences with a stable, meaningful name — the abstraction is justified. This is not premature generalisation; it's removing noise that's already there.

Suggestions

No architectural concerns. One-file change with mechanical call-site replacements. The scope is exactly right — don't be tempted to do more in this PR (e.g. adding error boundary improvements or changing how getErrorMessage works). Keep it atomic.
After the PR lands, add a note to the project's CODESTYLE.md or the API client section in CLAUDE.md that extractErrorCode is the canonical way to read the error code — so future developers don't re-introduce the pattern.

## 🏗️ Markus Keller — Application Architect ### Questions & Observations - **Correct placement.** `api.server.ts` is the right home — it's the module that wraps all backend communication, so error shape knowledge belongs there. Don't put this in a separate `utils.ts` barrel; keep it co-located with `createApiClient`. - **Export both the function and the interface.** The `ApiError` interface should be exported. Load functions in separate route files will need to import it if they ever want to type a caught error explicitly — and keeping it importable from one canonical location is cleaner than each route re-declaring the shape. - **This is exactly KISS winning over DRY.** 10+ occurrences with a stable, meaningful name — the abstraction is justified. This is not premature generalisation; it's removing noise that's already there. ### Suggestions - No architectural concerns. One-file change with mechanical call-site replacements. The scope is exactly right — don't be tempted to do more in this PR (e.g. adding error boundary improvements or changing how `getErrorMessage` works). Keep it atomic. - After the PR lands, add a note to the project's `CODESTYLE.md` or the API client section in `CLAUDE.md` that `extractErrorCode` is the canonical way to read the error code — so future developers don't re-introduce the pattern.

marcel commented

2026-03-31 21:11:55 +02:00

🎨 Leonie Voss — UI/UX Designer & Accessibility Strategist

No UI/UX concerns from my angle — this is a backend TypeScript utility refactor with no user-facing rendering changes. No visual components, no interaction patterns, and no i18n strings are affected.

I verified that extractErrorCode is a server-side helper in api.server.ts and does not touch any Svelte components or rendered output. Clean from my perspective.

## 🎨 Leonie Voss — UI/UX Designer & Accessibility Strategist No UI/UX concerns from my angle — this is a backend TypeScript utility refactor with no user-facing rendering changes. No visual components, no interaction patterns, and no i18n strings are affected. I verified that `extractErrorCode` is a server-side helper in `api.server.ts` and does not touch any Svelte components or rendered output. Clean from my perspective.

marcel commented

2026-03-31 21:13:33 +02:00

🚀 Tobias Wendt — DevOps & Platform Engineer

No DevOps or infrastructure concerns here — this is a pure frontend TypeScript refactor with no deployment, config, or infrastructure impact.

npm run check passing is the only gate needed. CI will catch it automatically.

## 🚀 Tobias Wendt — DevOps & Platform Engineer No DevOps or infrastructure concerns here — this is a pure frontend TypeScript refactor with no deployment, config, or infrastructure impact. `npm run check` passing is the only gate needed. CI will catch it automatically.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#113