refactor(types): replace casts with runtime decoding - #226
Conversation
Type assertions on the server and DOM boundaries claimed shapes that nothing verified. This swaps them for Zod schemas where the data comes from the server, `satisfies` for config literals, and guarded index reads through `lookup`/`elementAt`. Environment guards keep the `typeof X !== 'undefined'` form throughout: `'X' in globalThis` is also true for a declared-but-undefined global, so it would pass the check and then throw on the dereference. Also drops the unused `getAuthHeaders` helper and its property test. The live auth header is built in `jellyfin/http.ts` and covered there.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @rlauuzo, your pull request is larger than the review limit of 150000 diff characters
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
Reviewer's GuideRefactors type usage to replace unsafe casts with runtime-checked decoding and safer helpers, introduces shared test utilities, and tightens persistence, segment, navigation, and media-handling logic around more realistic server and DOM behavior. Sequence diagram for Jellyfin request and unified error handlingsequenceDiagram
participant Caller
participant QueryHook as createStandardQueryOptions
participant JellyfinHttp as jellyfinRequest
participant RequestSignal as createRequestSignal
participant Fetch as fetch
participant ReadJson as readJson
participant AppErr as AppError
participant QueryErr as handleQueryError
Caller->>QueryHook: createStandardQueryOptions
QueryHook->>JellyfinHttp: jellyfinRequest(options)
JellyfinHttp->>RequestSignal: createRequestSignal(callerSignal, timeout)
RequestSignal-->>JellyfinHttp: RequestSignal.signal, RequestSignal.didTimeout
JellyfinHttp->>Fetch: fetch(buildUrl(...), requestInit)
Fetch-->>JellyfinHttp: Response
alt response not ok
JellyfinHttp->>AppErr: AppError.fromStatus(response.status)
AppErr-->>JellyfinHttp: AppError
JellyfinHttp-->>QueryHook: throw AppError
else expectJson
JellyfinHttp->>ReadJson: readJson(Response)
ReadJson-->>JellyfinHttp: decoded payload T
JellyfinHttp-->>QueryHook: T
end
QueryHook->>QueryErr: throwOnError(cause)
QueryErr->>AppErr: AppError.from(cause)
AppErr-->>QueryErr: AppError { code, message, status, recoverable }
QueryErr-->>Caller: handled error (no rethrow)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
Routing the icon table through `lookup` made `<Icon />` opaque, so React Compiler stopped treating it as a static component and gave up memoizing `SearchResultItem`. `Partial<Record<...>>` already types a miss as `undefined`, so the direct index is just as honest and stays traceable. Claude-Session: https://claude.ai/code/session_01Bovw8mL7QugPUuWR2fu5YU
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| const videoStream = mediaStreams.find(isVideoStream) | ||
| // Only the first video stream carries the item's frame rate. | ||
| const videoStream = mediaStreams.find((stream) => stream.Type === 'Video') |
There was a problem hiding this comment.
WARNING: Missing null/type guard on stream in mediaStreams.find()
The old isVideoStream helper checked !stream || typeof stream !== 'object' before accessing .Type. This refactor removed that guard and now dereferences stream.Type unconditionally. Server-supplied MediaStreams arrays can contain non-object values, which will throw a TypeError at runtime.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Confirmed and fixed in a235c1a.
The SDK types declare MediaStreams as Array<MediaStream>, so stream.Type typechecks, but the payload is server-supplied and a null element throws. The predicate now takes the element as unknown and checks it before the read. Added a regression test in frame-timing.test.ts; against the previous code it fails with TypeError: Cannot read properties of null (reading 'Type').
| } | ||
|
|
||
| if (!isValidationContainer(current)) continue | ||
| const container = ValidationContainerSchema.safeParse(current) |
There was a problem hiding this comment.
WARNING: ValidationContainerSchema rejects objects without fields/form keys
The old isValidationContainer accepted any non-null object. The new ValidationContainerSchema requires both fields and form to be present. Standard Schema issues that arrive as { message: string } objects (or any object lacking those keys) now fail safeParse and are silently skipped, hiding user-facing validation messages.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Not accurate, no change made.
fields and form are not required. Both are ErrorCollectionSchema, which ends in .optional().catch(undefined), so a container carrying only message decodes fine. Running the schema directly:
PASS { message: 'Required' } <- the case called out as now failing
PASS {}
PASS { fields } only
PASS { message, path, code }
FAIL null
FAIL 'a string'
Only non-objects fail, and the previous isValidationContainer rejected those too.
The change is also stricter in the right place: { fields: 'junk' } now decodes to {}, where the old guard passed the string to Object.values and pushed its individual characters onto the walk queue.
| * touch are decoded; every other key is preserved untouched. | ||
| */ | ||
| const LegacyPersistedAppSchema = z.looseObject({ | ||
| segmentSkipMode: z.string().optional().catch(undefined), |
There was a problem hiding this comment.
WARNING: segmentSkipMode decoded as z.string(), allowing invalid values
segmentSkipMode is decoded with z.string(), which accepts any string. The store type restricts it to 'button' | 'skip' | 'disabled', but persisted data bypasses TypeScript. A corrupt or legacy payload could write an invalid string into state. Use z.enum(['button', 'skip', 'disabled']) with .catch('button') to match the runtime-decoding goal.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Half right, and fixed in 256ce1e, but not the way suggested.
The hole is real: z.string() let an off-union mode reach a field typed 'button' | 'skip' | 'disabled'.
The proposed z.enum(['button', 'skip', 'disabled']).catch('button') would have broken this schema's reason for existing. The legacy value is 'auto', and the next lines of migrate rewrite 'auto' to 'skip'. Catching 'auto' to 'button' would silently land those users on the wrong mode.
The fix decodes z.enum(['button', 'skip', 'disabled', 'auto']), so 'auto' still migrates and anything else drops to undefined and is filtered out, leaving the default. Both paths now have tests.
| setSettingsOpen: (settingsOpen) => set({ settingsOpen }), | ||
| setPageSize: (pageSize) => set({ pageSize: normalizePageSize(pageSize) }), | ||
| setViewMode: (viewMode) => set({ viewMode: normalizeViewMode(viewMode) }), | ||
| setPageSize: (pageSize) => set({ pageSize }), |
There was a problem hiding this comment.
SUGGESTION: Removed runtime validation from setPageSize
The old normalizePageSize guard was removed. Input is now validated only during rehydrate via merge. A future caller passing a non-conforming value would write it directly into state.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Not a regression, no change made.
Both call sites already validate before calling: PageSizeSection.tsx:30 and ViewModeSection.tsx:24 each safeParse the raw select value and only call the setter on success. The setter parameter is typed PageSize, so the hypothetical caller has to violate TypeScript first, and normalizePageSize was an as PageSize cast wrapper of exactly the kind this PR removes.
The untrusted boundary is localStorage, and it got stronger here, not weaker. The old merge spread raw persisted and re-validated only pageSize and viewMode, so playerVolume and playerMuted went in unchecked. PersistedSessionSchema now decodes all four.
| setPageSize: (pageSize) => set({ pageSize: normalizePageSize(pageSize) }), | ||
| setViewMode: (viewMode) => set({ viewMode: normalizeViewMode(viewMode) }), | ||
| setPageSize: (pageSize) => set({ pageSize }), | ||
| setViewMode: (viewMode) => set({ viewMode }), |
There was a problem hiding this comment.
SUGGESTION: Removed runtime validation from setViewMode
The old normalizeViewMode guard was removed. Input is now validated only during rehydrate via merge. A future caller passing a non-conforming value would write it directly into state.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
There was a problem hiding this comment.
Same as the setPageSize thread: ViewModeSection.tsx:24 parses through ViewModeSchema before calling, and rehydrate now decodes every persisted field rather than only this one. No change made.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (4 files)
Previous Review Summary (commit 333c6c0)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 333c6c0)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (4 files)
Reviewed by free · Input: 33.8K · Output: 3.6K · Cached: 128.8K |
`MediaStreams` is server-supplied, and the SDK types promise its elements are objects. A null element made the video-stream filter throw, which the old `isVideoStream` helper had guarded against. Claude-Session: https://claude.ai/code/session_01Bovw8mL7QugPUuWR2fu5YU
… value Decoding it as `z.string()` let a corrupt payload write an off-union mode into a field typed `'button' | 'skip' | 'disabled'`. The enum includes `'auto'` because that is the legacy spelling the migration rewrites to `'skip'`; narrowing to the three live modes would catch it to a default and drop the rewrite. Claude-Session: https://claude.ai/code/session_01Bovw8mL7QugPUuWR2fu5YU
1 participant
Problem
Type assertions were doing the work of validation.
ascasts on server responses and DOM lookups claimed a shape at compile time that nothing checked at runtime, so a server returning something unexpected surfaced as aTypeErrordeep in a render path rather than as a handled case at the boundary.Solution
satisfiesso they keep their literal types while still being checked.lookup/elementAt, which returnundefinedrather than lying about the element type.instanceofagainst the interface the API actually takes (Elementfor pointer capture, notHTMLElement).Environment guards deliberately keep the
typeof X !== 'undefined'form.'X' in globalThisis not equivalent: it is also true when a global is declared but undefined, which some embedded webviews and polyfills do, so the check passes and the dereference then throws.Also removes the unused
getAuthHeadershelper and its property test. It had no callers; the live auth header is built inservices/jellyfin/http.tsand is asserted byjellyfin-http.test.tsandsegment-read-endpoint.test.ts.Verification
tsc --noEmitclean,oxlintclean, 83 test files / 694 tests passing.Summary by Sourcery
Replace compile-time casts with runtime validation and explicit narrowing at external-data, persistence, collection, and DOM boundaries.
Bug Fixes:
Enhancements:
Tests: