fix(ts-client): prevent Overview crash from single-resource cache seeding#3048
Conversation
…ding
updateSingleResource seeded the TanStack cache with { files: [...] }
when oldData was null (buffer replay race). This list-shaped fallback
broke single-resource queries like libraries.info where the response
is a plain object, causing TypeError on statistics.total_capacity.
Return undefined instead so the queryFn delivers the correct shape.
Batch seeding in updateBatchResources is preserved for file listings.
WalkthroughModified the cache seeding fallback in Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ts-client/src/hooks/useNormalizedQuery.ts (1)
509-513: Please add a regression test for this cold-cache branch.Given this behavioral change, add coverage that
updateSingleResourceleaves cache unset whenoldDatais missing, whileupdateBatchResourcesstill seeds list-shaped cache. This will prevent reintroducing the Overview crash pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ts-client/src/hooks/useNormalizedQuery.ts` around lines 509 - 513, Add a regression test that exercises the cold-cache branch in useNormalizedQuery by calling updateSingleResource with oldData === undefined and asserting it does not set the cache, and a separate test that calls updateBatchResources when oldData is undefined and asserts it still seeds a list-shaped cache; specifically instantiate the hook (or call the exported helpers) so updateSingleResource and updateBatchResources are invoked with a missing oldData, mock or spy on the cache setter to verify no write occurs for updateSingleResource and that a list-shaped value is written for updateBatchResources, and include assertions to prevent reintroduction of the Overview crash pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/ts-client/src/hooks/useNormalizedQuery.ts`:
- Around line 509-513: Add a regression test that exercises the cold-cache
branch in useNormalizedQuery by calling updateSingleResource with oldData ===
undefined and asserting it does not set the cache, and a separate test that
calls updateBatchResources when oldData is undefined and asserts it still seeds
a list-shaped cache; specifically instantiate the hook (or call the exported
helpers) so updateSingleResource and updateBatchResources are invoked with a
missing oldData, mock or spy on the cache setter to verify no write occurs for
updateSingleResource and that a list-shaped value is written for
updateBatchResources, and include assertions to prevent reintroduction of the
Overview crash pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c0a8fa58-d873-4b49-b6ee-c52ee2faf3f8
📒 Files selected for processing (1)
packages/ts-client/src/hooks/useNormalizedQuery.ts
Summary
updateSingleResourceseeded the TanStack cache with{ files: [...], total_count, has_more }whenoldDatawas null (buffer replay race). This list-shaped fallback broke single-resource queries likelibraries.infowhere the response is a plainLibraryobject, causingTypeError: Cannot read properties of undefined (reading 'total_capacity')on the Overview page.undefinedinstead (TanStack no-op) so thequeryFndelivers the correct shape. Batch seeding inupdateBatchResourcesis preserved for file listings.Root Cause
The subscription manager pre-registers the listener before
transport.subscribe()(correct for capturing buffer replay). When the daemon has a recentResourceChangedforlibrary(fromrecalculate_statistics()), the buffer replay delivers it before thequeryFnresolves. WitholdDatanull, the fallback creates{ files: [Library] }which has nostatisticskey, crashing the Overview component.Why only
updateSingleResourceupdateBatchResourceskeeps its{ files: [...] }seeding becauseResourceChangedBatchevents are only emitted by the file indexing pipeline, where the list shape is correct. SingleResourceChangedevents affect all resource types (library, device, volume, location, tag), most of which don't use list-shaped responses.