fix(desktop): restore v2 sidebar LOC indicator#4222
Conversation
#4160 swapped useDiffStats from imperative client.git.getStatus.query() to workspaceTrpc.git.getStatus.useQuery(). That broke the v2 dashboard sidebar: workspaceTrpc.Provider only mounts inside v2-workspace/$id, but the sidebar is a sibling of that Outlet — so its useQuery calls had no provider, and even with one couldn't fan out to workspaces on different hosts. Resolve hostUrl per-workspaceId, call the imperative client inside a react-query queryFn keyed on (hostUrl, workspaceId). Concurrent callers (tile + hover overlay for the same workspace) still dedup via the global QueryClient. staleTime: Infinity since git:changed is push-authoritative.
📝 WalkthroughWalkthroughThe ChangesTRPC to React Query Migration for useDiffStats
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/renderer/hooks/host-service/useDiffStats/useDiffStats.ts`:
- Around line 29-31: The query config in useDiffStats() sets staleTime to
Number.POSITIVE_INFINITY which risks permanent stale data before
useWorkspaceEvent() subscribes; update the query options in the useDiffStats
hook to ensure an initial fresh load by either setting a short staleTime (e.g.,
30_000 ms) or adding refetchOnMount: true (and optionally a conservative
cacheTime/gcTime) so the first mount always refetches while keeping
refetchOnWindowFocus: false; locate the query options object in useDiffStats and
adjust the staleTime/refetchOnMount (and optionally cacheTime) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21fb1afe-15c6-4d25-89a4-c662ee8a4940
📒 Files selected for processing (1)
apps/desktop/src/renderer/hooks/host-service/useDiffStats/useDiffStats.ts
| refetchOnWindowFocus: false, | ||
| staleTime: Number.POSITIVE_INFINITY, | ||
| }); |
There was a problem hiding this comment.
staleTime: POSITIVE_INFINITY creates a silent stale-data risk on first mount.
With git:changed as the sole refresh trigger, any change that occurs between component mount and listener registration (useWorkspaceEvent subscribing) will leave the UI permanently stale for that session — no background refetch or window-focus refetch will rescue it. The PR notes this is intentional ("push-authoritative"), but it's worth confirming the event subscription is guaranteed to be in place before any relevant git:changed events can fire.
If there's a realistic race on initial mount (e.g., workspace git state changes during the first render cycle), consider a short staleTime (e.g., 30 s) or a conservative gcTime paired with an initial refetchOnMount: true to ensure the first load is always fresh, while still avoiding redundant window-focus refetches.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/renderer/hooks/host-service/useDiffStats/useDiffStats.ts`
around lines 29 - 31, The query config in useDiffStats() sets staleTime to
Number.POSITIVE_INFINITY which risks permanent stale data before
useWorkspaceEvent() subscribes; update the query options in the useDiffStats
hook to ensure an initial fresh load by either setting a short staleTime (e.g.,
30_000 ms) or adding refetchOnMount: true (and optionally a conservative
cacheTime/gcTime) so the first mount always refetches while keeping
refetchOnWindowFocus: false; locate the query options object in useDiffStats and
adjust the staleTime/refetchOnMount (and optionally cacheTime) accordingly.
Greptile SummaryThis PR restores the v2 sidebar LOC indicator by replacing
Confidence Score: 4/5Safe to merge; the core fix is correct and useEffectEvent in useWorkspaceEvent ensures no stale-closure issues with the callback. The logic replacing the broken tRPC hook is sound — getHostServiceClientByUrl is cached, queryKey is stable, and useEffectEvent ensures the latest invalidate is always called. Two edge-cases are flagged: invalidate issues a no-op invalidateQueries call while hostUrl is null, and staleTime: Infinity with no reconnect-triggered refetch means a WebSocket gap during active editing could leave the chip showing an outdated count indefinitely. apps/desktop/src/renderer/hooks/host-service/useDiffStats/useDiffStats.ts — specifically the invalidate guard and the reconnect-refresh strategy.
|
| Filename | Overview |
|---|---|
| apps/desktop/src/renderer/hooks/host-service/useDiffStats/useDiffStats.ts | Replaces broken tRPC React hook with an imperative client call wrapped in a standard react-query useQuery, keyed by (hostUrl, workspaceId). Logic is sound; the main concern is that staleTime: Infinity makes the display exclusively reliant on push events with no fallback if git:changed is missed during a WebSocket reconnect. |
Sequence Diagram
sequenceDiagram
participant SC as Sidebar Component
participant uDS as useDiffStats
participant uWHU as useWorkspaceHostUrl
participant uWE as useWorkspaceEvent
participant QC as QueryClient
participant HSC as getHostServiceClientByUrl
participant EB as Event Bus (git:changed)
SC->>uDS: render(workspaceId)
uDS->>uWHU: useWorkspaceHostUrl(workspaceId)
uWHU-->>uDS: hostUrl (null while loading, URL when ready)
uDS->>QC: "useQuery({ queryKey: [diff-stats, hostUrl, workspaceId], enabled: hostUrl != null })"
QC->>HSC: "getHostServiceClientByUrl(hostUrl).git.getStatus.query({ workspaceId })"
HSC-->>QC: GitStatus (staleTime: Infinity, cached)
QC-->>uDS: "{ data: status }"
uDS->>uWE: useWorkspaceEvent(git:changed, workspaceId, invalidate)
uWE->>EB: subscribe via WebSocket
Note over EB, QC: On file change
EB->>uWE: git:changed event (useEffectEvent to latest invalidate)
uWE->>QC: "invalidateQueries({ queryKey: [diff-stats, hostUrl, workspaceId] })"
QC->>HSC: refetch git.getStatus
HSC-->>QC: updated GitStatus
QC-->>uDS: "{ data: updated status }"
uDS-->>SC: "{ additions, deletions }"
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/desktop/src/renderer/hooks/host-service/useDiffStats/useDiffStats.ts:29-31
**Stale indicator if `git:changed` event is missed on reconnect**
`staleTime: Number.POSITIVE_INFINITY` + `refetchOnWindowFocus: false` means the LOC chip updates *only* when a `git:changed` WebSocket event is received. If the event bus connection drops and reconnects, any file edits made during the gap are silently invisible — the chip keeps showing the pre-disconnect value until the user makes another change that triggers a fresh push event. In the previous `staleTime: 0` regime a component remount (e.g., after a tab switch) would naturally re-prime the cache; that safety net is now gone.
Consider adding `refetchOnReconnect: true` (verify it fires for the non-window WebSocket path) or having the event bus emit a synthetic `git:changed` after reconnect so the existing invalidation path kicks in.
### Issue 2 of 2
apps/desktop/src/renderer/hooks/host-service/useDiffStats/useDiffStats.ts:33-35
The `invalidate` callback captures `queryKey` at the time it was last memoized. When `hostUrl` is still `null` (the workspace host is loading), `queryKey` is `["diff-stats", null, workspaceId]` and any `git:changed` event fired in that window calls `invalidateQueries` with that null key — targeting a query that was never enabled and has no cached data, so the invalidation is silently dropped. Adding an early-return guard makes the intent explicit and avoids the vacuous call.
```suggestion
const invalidate = useCallback(() => {
if (!hostUrl) return;
void queryClient.invalidateQueries({ queryKey });
}, [queryClient, queryKey, hostUrl]);
```
Reviews (1): Last reviewed commit: "fix(desktop): restore v2 sidebar LOC ind..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| refetchOnWindowFocus: false, | ||
| staleTime: Number.POSITIVE_INFINITY, | ||
| }); |
There was a problem hiding this comment.
git:changed event is missed on reconnect
staleTime: Number.POSITIVE_INFINITY + refetchOnWindowFocus: false means the LOC chip updates only when a git:changed WebSocket event is received. If the event bus connection drops and reconnects, any file edits made during the gap are silently invisible — the chip keeps showing the pre-disconnect value until the user makes another change that triggers a fresh push event. In the previous staleTime: 0 regime a component remount (e.g., after a tab switch) would naturally re-prime the cache; that safety net is now gone.
Consider adding refetchOnReconnect: true (verify it fires for the non-window WebSocket path) or having the event bus emit a synthetic git:changed after reconnect so the existing invalidation path kicks in.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/hooks/host-service/useDiffStats/useDiffStats.ts
Line: 29-31
Comment:
**Stale indicator if `git:changed` event is missed on reconnect**
`staleTime: Number.POSITIVE_INFINITY` + `refetchOnWindowFocus: false` means the LOC chip updates *only* when a `git:changed` WebSocket event is received. If the event bus connection drops and reconnects, any file edits made during the gap are silently invisible — the chip keeps showing the pre-disconnect value until the user makes another change that triggers a fresh push event. In the previous `staleTime: 0` regime a component remount (e.g., after a tab switch) would naturally re-prime the cache; that safety net is now gone.
Consider adding `refetchOnReconnect: true` (verify it fires for the non-window WebSocket path) or having the event bus emit a synthetic `git:changed` after reconnect so the existing invalidation path kicks in.
How can I resolve this? If you propose a fix, please make it concise.| const invalidate = useCallback(() => { | ||
| void utils.git.getStatus.invalidate({ workspaceId }); | ||
| }, [utils, workspaceId]); | ||
| void queryClient.invalidateQueries({ queryKey }); | ||
| }, [queryClient, queryKey]); |
There was a problem hiding this comment.
invalidate callback captures queryKey at the time it was last memoized. When hostUrl is still null (the workspace host is loading), queryKey is ["diff-stats", null, workspaceId] and any git:changed event fired in that window calls invalidateQueries with that null key — targeting a query that was never enabled and has no cached data, so the invalidation is silently dropped. Adding an early-return guard makes the intent explicit and avoids the vacuous call.
| const invalidate = useCallback(() => { | |
| void utils.git.getStatus.invalidate({ workspaceId }); | |
| }, [utils, workspaceId]); | |
| void queryClient.invalidateQueries({ queryKey }); | |
| }, [queryClient, queryKey]); | |
| const invalidate = useCallback(() => { | |
| if (!hostUrl) return; | |
| void queryClient.invalidateQueries({ queryKey }); | |
| }, [queryClient, queryKey, hostUrl]); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/hooks/host-service/useDiffStats/useDiffStats.ts
Line: 33-35
Comment:
The `invalidate` callback captures `queryKey` at the time it was last memoized. When `hostUrl` is still `null` (the workspace host is loading), `queryKey` is `["diff-stats", null, workspaceId]` and any `git:changed` event fired in that window calls `invalidateQueries` with that null key — targeting a query that was never enabled and has no cached data, so the invalidation is silently dropped. Adding an early-return guard makes the intent explicit and avoids the vacuous call.
```suggestion
const invalidate = useCallback(() => {
if (!hostUrl) return;
void queryClient.invalidateQueries({ queryKey });
}, [queryClient, queryKey, hostUrl]);
```
How can I resolve this? If you propose a fix, please make it concise.
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Recorded as integrated via -s ours after batch PRs #455-#464. Taken via individual PRs: - PR 1 (#455): v2 polish 前半 safe set (9 commits) - PR 2 (#456): v2/host-service polish 中盤 (12 commits) - PR 3 (#457): sidebar polish + jwt API (5 commits) - PR 4 (#458): host-service tRPC retry/cache/timeout (3 commits) - PR 5 (#459): v2 diff pane / file pane polish (2 commits) - PR 7 (#462): host-service v2 canonical workspace.create + attachment store (PR1 superset-sh#3893 + PR2 superset-sh#3916) - PR 11 (#463): agents API + onboarding (7 commits + 1 cleanup) - PR 12 (#464): v1→v2 import flow rewrite (11 commits + 2 follow-ups) - PR 13 (#460): host-service shell env probe + typo (2 commits) - PR 16 (#461): marketplace 19 themes (1 commit) Skipped / deferred (recorded as integrated for behind=0): - PR 6: CLI v1 launch (superset-sh#3898 + 30+ CLI/SDK followups) — defer to dedicated migration - PR 9: v2 PR3 (superset-sh#3940) + revert (superset-sh#4017) — net-zero pair - PR 10: pty-daemon (superset-sh#3896, superset-sh#3971, superset-sh#4054) — fork keeps its terminal-host - PR 14: Slack MCP-v2 (superset-sh#4197, superset-sh#4208) — depends on mcp-v2/sdk divergence - PR 15: onboarding remaining (superset-sh#4115, superset-sh#4125, superset-sh#4214, superset-sh#4213, superset-sh#4222, superset-sh#4225) — depends on fork's deleted setup pages Behind: 0 after this merge.
1 participant
Summary
Test plan
Summary by cubic
Restores the v2 sidebar LOC (+N/−N) indicator by fetching diff stats per workspace without relying on a
workspaceTrpcprovider. Works across local, remote, and cloud workspaces.@tanstack/react-queryuseQuerywithgetHostServiceClientByUrl(hostUrl).git.getStatus.query(...).hostUrlperworkspaceIdviauseWorkspaceHostUrl; cache key:["diff-stats", hostUrl, workspaceId].git:changed;staleTime: Infinity;refetchOnWindowFocus: false; dedup concurrent fetches via the globalQueryClient.Written for commit 3a45414. Summary will update on new commits.
Summary by CodeRabbit