Skip to content

Coalesce repos:changed refetch so stale fetches can't leave removed projects as ghost rows (#7020)#7083

Closed
xianjianlf2 wants to merge 1 commit into
stablyai:mainfrom
xianjianlf2:fix/coalesce-repos-changed-refetch
Closed

Coalesce repos:changed refetch so stale fetches can't leave removed projects as ghost rows (#7020)#7083
xianjianlf2 wants to merge 1 commit into
stablyai:mainfrom
xianjianlf2:fix/coalesce-repos-changed-refetch

Conversation

@xianjianlf2

@xianjianlf2 xianjianlf2 commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes #7020.

Deleting a Project Group with "Remove contained projects" could leave the removed projects as stale, unusable rows in the sidebar until the app was restarted.

Root cause

repos:changed is broadcast once per persisted mutation, so a group delete that also removes N contained projects emits N+1 events. The renderer's repos.onChanged handler reacted to every event by firing three independent, unsequenced fetches (fetchProjectGroups / fetchFolderWorkspaces / fetchRepos, or their all-hosts variants).

Because the fetches were unsequenced, an early fetch — dispatched for the group-delete event, before the per-project removals had persisted — could resolve last and overwrite the store with a snapshot that still contained the projects, reintroducing them in memory even though persistence had already dropped them. A restart "fixed" it only because startup reloads the final persisted state.

Fix

Route the repos:changed refresh through a serial coalescing runner (createReposChangedRefetchRunner, built on the existing createCoalescedPollRunner):

  • Refetches never overlap — while one is in flight, further events collapse into a single pending rerun.
  • A trailing run always executes after the final event, so the store settles on the final persisted state.

The last event therefore always drives a refetch whose read happens at-or-after the final mutation, and serialization guarantees that refetch is the one that applies last. As a bonus, an N+1-event burst now performs ~2 refetch cycles instead of N+1.

The generic coalescing primitive is promoted from the git-status domain to src/renderer/src/lib/coalesced-poll-runner.ts (with its test), since it now has two consumers; useGitStatusPolling is updated to import from the new location.

Testing

  • New repos-changed-refetch.test.ts covers: burst coalescing into a single trailing refetch, immediate run for an isolated event, the runtime-active all-hosts path, and dispose dropping a queued trailing run.
  • Existing coalesced-poll-runner, useGitStatusPolling, and useIpcEvents suites pass unchanged.
  • oxlint (incl. react-doctor) and web tsgo typecheck pass.

🤖 Generated with Claude Code

X: @mark86202384100

…ed projects (stablyai#7020)

Deleting a project group with "Remove contained projects" could leave the
removed projects as stale, unusable sidebar rows until restart.

`repos:changed` is broadcast once per persisted mutation, so the group
delete plus each contained-project removal emit N+1 events. The renderer
handled every event by firing three independent, unsequenced fetches
(`fetchProjectGroups` / `fetchFolderWorkspaces` / `fetchRepos`). An early
fetch — dispatched for the group-delete event before the project removals
had persisted — could resolve last and overwrite the store, reintroducing
the just-removed projects in memory even though persistence had dropped
them. Restart "fixed" it only because startup reloads the final state.

Route the repos:changed refresh through a serial coalescing runner
(`createReposChangedRefetchRunner`) so refetches never overlap and a
trailing run always executes after the final event, letting the store
settle on the final persisted state. The generic coalescing primitive is
promoted from the git-status domain to `lib/coalesced-poll-runner` since
it now has two consumers.

Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Changes

This pull request introduces a coalescing runner for handling repos:changed IPC events. A new module exports createReposChangedRefetchRunner, which batches rapid refetch triggers into a single trailing execution, refetching repos, project groups, and folder workspaces either per-host or via all-host variants depending on runtime environment state. useIpcEvents.ts is updated to replace its previous inline refetch handler with this runner, registering it for disposal. A new test suite validates the runner's coalescing, immediate-run, runtime-active, and disposal behaviors. Separately, useGitStatusPolling.ts updates an import path for CoalescedPollRunner and createCoalescedPollRunner to a shared module location.

Sequence Diagram(s)

Included above in the hidden review stack artifact.

Related PRs: None identified.

Suggested labels: refactor, hooks, tests

Suggested reviewers: None identified.

Poem
A rabbit hops where refetches once flew wild,
Coalescing bursts, gentle and mild.
One trailing call where many once ran,
All-hosts or one-host, per the runtime plan.
Tests stand watch, dispose keeps it clean —
The tidiest burrow this codebase has seen. 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main fix: coalescing repos:changed refetches to prevent stale project rows.
Linked Issues check ✅ Passed The changes address #7020 by serializing repos:changed refetches and adding coverage for the race and runtime paths.
Out of Scope Changes check ✅ Passed The shared coalescing utility move and new tests are directly tied to the refetch race fix and not unrelated scope.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description check ✅ Passed The PR description is mostly complete and covers summary, root cause, fix, and testing, though the screenshots and audit sections are missing.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/renderer/src/hooks/repos-changed-refetch.ts (1)

27-47: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Sequential awaits vs Promise.all: inconsistent and fragile on partial failure.

The runtime-active branch awaits fetchReposForAllHostsfetchProjectGroupsForAllHostsfetchFolderWorkspacesForAllHosts sequentially, while the default branch runs the equivalent three fetches concurrently via Promise.all. These three calls appear independent (no data passed between them), so the sequential version adds unnecessary latency (sum vs. max) and, if the first call rejects, the other two never even start in this cycle — leaving those slices stale until the next repos:changed event. If there's no ordering dependency, unify both branches to run concurrently.

♻️ Proposed fix
     if (options.isRuntimeEnvironmentActive()) {
       // Why: the all-host sidebar still shows local repos while a runtime is
       // focused, so refresh every host's slice without dropping the
       // runtime-owned slices already shown.
-      await actions.fetchReposForAllHosts()
-      await actions.fetchProjectGroupsForAllHosts()
-      await actions.fetchFolderWorkspacesForAllHosts()
+      await Promise.all([
+        actions.fetchReposForAllHosts(),
+        actions.fetchProjectGroupsForAllHosts(),
+        actions.fetchFolderWorkspacesForAllHosts()
+      ])
       return
     }

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 0e900c86-018b-4eb8-a020-395f8a6b4925

📥 Commits

Reviewing files that changed from the base of the PR and between c29bf6f and b0d5b57.

📒 Files selected for processing (6)
  • src/renderer/src/components/right-sidebar/useGitStatusPolling.ts
  • src/renderer/src/hooks/repos-changed-refetch.test.ts
  • src/renderer/src/hooks/repos-changed-refetch.ts
  • src/renderer/src/hooks/useIpcEvents.ts
  • src/renderer/src/lib/coalesced-poll-runner.test.ts
  • src/renderer/src/lib/coalesced-poll-runner.ts

@nwparker

nwparker commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Thanks for this, @xianjianlf2 — really thoughtful work. Your root-cause writeup is spot-on, and the coalescing-runner approach is genuinely nice: it serializes the whole repos:changed refetch (repos + project groups + folder workspaces, plus the all-host path) through one runner and de-dupes the N+1 burst, which is strictly broader coverage than a per-slice guard.

After evaluating both approaches for the same issue (#7020), we went with #7024, which has now merged and closed the issue. The deciding factors were scope and blast radius: #7024 is a surgical, store-level generation guard on fetchRepos (the exact path in the reported repro), with a deterministic regression test, and no changes to shared infrastructure. Your PR is the more complete fix but also touches more surface (moving coalesced-poll-runner into lib, the git-status polling import, and refetch timing semantics), which is more than the reported symptom needs right now.

That said, the broader-path coverage your PR provides — the runtime-active fetchReposForAllHosts path and coalescing the burst — is real and not addressed by the merged fix; it's called out as a follow-up in #7024's description with credit to your framing here. If you're up for it, a focused follow-up that closes that all-host gap would be very welcome. Superseded by #7024, but this was a close call and the analysis was appreciated. 🙏

@xianjianlf2

Copy link
Copy Markdown
Contributor Author

Closing in favor of #7024 (merged, covers the default fetchRepos path) and #7288 (covers the all-host slice). Thanks for evaluating both approaches!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Deleting a project group can leave removed contained projects as stale sidebar rows until restart

3 participants