Repository navigation
Conversation
The Sidebar suite fakes `@/store` with a hand-written partial state object. PR #22552 added `LocalGitToolchainScanBanner` to the sidebar, which reads `detectedWorktreesByRepo` and indexes it per repo. The fixture never set that key, so the one test that renders with a non-empty `repos` list crashed in `findLocalToolchainBlock` with "Cannot read properties of undefined". The real store seeds the record to `{}` at creation and has no persist middleware, so the product code cannot see it undefined; only the fixture could. Seed it in the fixture rather than adding a dead guard to the product path. No assertion changed.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Seed
detectedWorktreesByRepoin the Sidebar fixture —setSidebarStatenow setsdetectedWorktreesByRepo: {}with a// Why:comment atsrc/renderer/src/components/sidebar/Sidebar.test.tsx:110-111, restoring the hand-written partial store state thatLocalGitToolchainScanBanner(added in #22552) indexes per repo viafindLocalToolchainBlock.
I independently verified the claims in the description:
- The key is load-bearing — without it, the hydration test's non-empty
reposreachesdetectedByRepo[repo.id]inrepo-scan-failure.ts:54and throwsTypeError: Cannot read properties of undefined. The other five tests passrepos: [], soflatMapnever dereferences it. - The product store always seeds the key:
worktreeSliceInitialStatesetsdetectedWorktreesByRepo: {}(worktree-slice-initial-state.ts:25) and the slice types it as a requiredRecord<string, DetectedWorktreeListResult>(worktree-helpers.ts:98), so no runtime consumer can hit this. - Sibling suites already seed the same key (
WorktreeVisibilityDialog.test.tsx:27,project-added-default-checkout.test.ts:23), and the banner retains direct coverage inLocalGitToolchainScanBanner.test.tsxrather than being mocked out. npx vitest run --config config/vitest.config.ts src/renderer/src/components/sidebar/Sidebar.test.tsx→ 6 passed locally.
Scoping the fix to the fixture rather than adding a ?? {} guard to findLocalToolchainBlock is the right call: the guard would be unreachable in production and would mask the next fixture drift. The pre-existing Record<string, unknown> fixture typing that let this slip past tsc is worth revisiting separately, but not in this fix.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe sidebar test fixture now initializes Merge Risk: ⚪ Minimal · up to This test-only change aligns the sidebar fixture with the real store; it does not change production behavior, and no actionable merge risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
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 |
|
Superseded by #22563, which landed the identical fix ( |
ELI5
A test that checks the project sidebar started crashing on
main, which turned the test gate red for every open PR. The test builds a pretend version of the app's data store, and a recently added sidebar banner needed one piece of data the pretend store had never been told to include. This adds that one piece.What Changed
src/renderer/src/components/sidebar/Sidebar.test.tsxfakes the app store (@/store) with a hand-written partial state object.setSidebarStatenow also seedsdetectedWorktreesByRepo: {}.Before:
npx vitest run --config config/vitest.config.ts src/renderer/src/components/sidebar/Sidebar.test.tsxon cleanmainfailed withAfter: 6 passed.
The mechanism: #22552 added
<LocalGitToolchainScanBanner />intosrc/renderer/src/components/sidebar/index.tsx. That banner readsuseAppStore((s) => s.detectedWorktreesByRepo)and passes it tofindLocalToolchainBlock, which doesdetectedByRepo[repo.id]for every repo. The fixture never defined that key, so the value wasundefined. Five of the six tests survived because they render withrepos: [], and theflatMapover an empty array never dereferences it. The sixth — the hydration test — is the only one that sets a non-emptyrepos, so it was the only one that indexed intoundefined.No assertion, name, or behaviour in the test changed. The "don't scan during hydration" invariant is asserted exactly as before, and the banner still renders for real (it returns
nullwith an empty record) rather than being mocked away.Why
mainis red, which reds theverifygate on every unrelated PR. In CI this shows up astest / tests node 24 1/8.The defect is in the test fixture, not the product code, and I checked rather than assumed:
createWorktreeSlicespreadsworktreeSliceInitialState, which setsdetectedWorktreesByRepo: {}at store creation (src/renderer/src/store/slices/worktrees.ts:78,.../worktrees/session/worktree-slice-initial-state.ts:25).WorktreeSlicetypes it as a requiredRecord<string, DetectedWorktreeListResult>— only the values are allowed to be absent, andresolveRepoScanFailurealready handles a missing value via its!detectedearly return.useAppStorehas nopersistmiddleware, so there is no rehydration path that can drop the key.useAppStore.setState(next, true); the only replacingsetStateis in a test harness.So the real sidebar could never have hit this at runtime — no users were affected, and this is not a latent product crash. That is why I did not add a
?? {}guard tofindLocalToolchainBlock: with the key guaranteed by the slice's initial state and by the type, a guard would be unreachable code that also hides the next fixture drift instead of surfacing it. MockingLocalGitToolchainScanBannerout of the suite (the pattern used forWorktreeList,SidebarHeader, etc.) would also have gone green, but it would quietly drop the banner from Sidebar's render coverage — strictly less testing than we have now. Seeding the fixture keeps coverage and matches what sibling suites already do (WorktreeVisibilityDialog.test.tsx,project-added-default-checkout.test.tsboth seed this key in their base state).Root cause class worth noting for reviewers: the fixture is typed
Record<string, unknown>, so TypeScript cannot catch a new store read that the fixture does not satisfy. That is a pre-existing shape in this file and I kept the fix scoped rather than retyping it.Linked Issue
No tracking issue exists for this breakage;
mainwas red and blocking merges. Filed as a direct fix.Fixes #
Visual Proof
N/A— test-fixture-only change. No product file is touched, so there is no user-visible before/after to capture. As established in Why, the crash was not reachable in the shipped sidebar.Testing
macOS (arm64). Reproduced on clean
origin/mainat7f203ad5aabwith zero local changes before fixing.The whole
src/renderer/src/components/sidebardirectory was run to confirm no sibling suite regressed.The change is the test fix; the existing hydration test is the regression test and it now exercises the real banner.
AI Disclosure
Claude Opus 4.5 (Claude Code) investigated and wrote this change.
Review
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
No security, cross-platform, SSH/remote, mobile, backwards-compatibility, or performance impact — this is a one-line addition to a renderer test fixture and ships no product code.
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)