Lanes Snapshot Invalidation#711
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
@copilot review but do not make fixes |
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (16)
📝 WalkthroughWalkthroughThis PR expands the ChangesLane lifecycle events, invalidation, and consumers
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
arul28
force-pushed
the
ade/start-by-running-skill-keywords-9923fd90
branch
from
433ef71 to
9e47ded
Compare
|
@codex review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
apps/desktop/src/main/services/lanes/laneService.ts (1)
4937-4948: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider guarding against no-op unarchive calls.
Unlike
rename()(which early-returns whentrimmed === lane.name),unarchive()broadcastslane-unarchivedunconditionally, even ifrow.statusis already"active". Since this event now drives a user-facing toast ("Lane unarchived"inuseLaneEventToasts.ts) and triggers cache invalidation/refresh inuseLaneListInvalidation, a redundant call would produce a spurious toast and unnecessary refresh cascade.archive()has the same gap, so this isn't a new issue, but the newly-wired event/toast wiring makes it user-visible for the first time.🩹 Proposed fix
unarchive({ laneId }: { laneId: string }): void { const row = getLaneRow(laneId); if (!row) throw new Error(`Lane not found: ${laneId}`); + if (row.status !== "archived") return; db.run("update lanes set status = 'active', archived_at = null where id = ? and project_id = ?", [laneId, projectId]);🤖 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/main/services/lanes/laneService.ts` around lines 4937 - 4948, Guard the no-op path in laneService.ts for unarchive() (and likewise archive()) so lifecycle events are only emitted when the lane status actually changes. In unarchive(), check the current row.status before updating; if it is already active, return early without calling db.run, invalidateLaneListCache, or broadcastLifecycleEvent("lane-unarchived"). Mirror the same status check pattern in archive() to avoid redundant writes, refreshes, and toasts.apps/desktop/src/renderer/lib/laneReadCache.ts (1)
92-103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate cache-clear logic between test helper and production invalidation.
clearLaneReadInFlightForTestandinvalidateLaneReadCacheshare nearly identical bodies (clear in-flight maps + bump generation), differing only by thekeybindingsInFlight.clear()call. Consider having the test helper callinvalidateLaneReadCache()plus its ownkeybindingsInFlight.clear()to avoid future drift between the two.♻️ Proposed refactor
export function clearLaneReadInFlightForTest(): void { - laneListInFlight.clear(); - laneSnapshotInFlight.clear(); keybindingsInFlight.clear(); - laneReadGeneration += 1; + invalidateLaneReadCache(); }🤖 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/lib/laneReadCache.ts` around lines 92 - 103, The cache-clearing logic in clearLaneReadInFlightForTest and invalidateLaneReadCache is duplicated and likely to drift. Refactor clearLaneReadInFlightForTest to call invalidateLaneReadCache() for the shared laneListInFlight, laneSnapshotInFlight, and laneReadGeneration reset behavior, then keep only the test-specific keybindingsInFlight.clear() in the test helper. Use the existing function names as the single source of truth so future changes to invalidation behavior stay consistent.apps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsx (1)
183-183: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winVerify stale-focus refresh options don't duplicate this page's dedicated refresh effects.
useLaneListInvalidationis wired with the raw storerefreshLanes(not the page'srefreshGraphLaneswrapper, which deliberately skips heavier phases per the comment at Lines 174-180). Per the hook's contract (apps/desktop/src/renderer/hooks/useLaneListInvalidation.ts), its stale-focus path callsrunRefresh({ includeStatus: false, includeSnapshots: true, includeConflictStatus: true, includeRebaseSuggestions: true, includeAutoRebaseStatus: true })on refocus afterstaleMs. This page already has dedicated focus/visibility-triggered refreshes for auto-rebase status (refreshAutoRebaseStatuses) and sync status (refreshLaneSyncStatuses) at Lines 1248-1258, plus its own risk-batch refresh cadence. The new hook's heavier stale-refresh could duplicate that work on refocus after the tab has been hidden a while.🔍 Suggested verification
Confirm whether
laneLifecycleRefreshOptions(used for lifecycle-driven refreshes) and the fixed stale-focus option set inuseLaneListInvalidationare expected to overlap with this page's existing per-concern refresh effects, or whether a lighter-weight customrefreshLaneswrapper (mirroringrefreshGraphLanes) should be passed instead.🤖 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/components/graph/WorkspaceGraphPage.tsx` at line 183, The stale-focus refresh wired through useLaneListInvalidation is using the raw refreshLanes path, which can duplicate this page’s existing dedicated refresh effects. Review WorkspaceGraphPage’s refreshGraphLanes wrapper and the related focus/visibility refresh handlers (including refreshAutoRebaseStatuses and refreshLaneSyncStatuses), then either pass a lighter custom wrapper into useLaneListInvalidation or align laneLifecycleRefreshOptions so the stale-focus path does not re-run phases this page already refreshes separately.apps/desktop/src/renderer/components/prs/state/PrsContext.tsx (1)
895-925: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffDuplicated debounce/visibility-defer pattern vs.
useLaneListInvalidation.This effect reimplements essentially the same "debounce lifecycle event, defer while hidden, replay on focus/visibility" pattern already encapsulated in
useLaneListInvalidation(used byWorkspaceGraphPage,LanesPage,RunPageper the stack context). SincePrsContextrefreshes viarefreshCore()rather than aRefreshLanes-shaped function, direct reuse isn't a drop-in, but consider extracting the shared "debounce + hidden-defer + focus/visibility replay" logic into a small reusable utility/hook to avoid two independently-maintained copies of this timing logic (and two magic debounce constants:PRS_LANE_LIFECYCLE_REFRESH_DEBOUNCE_MShere vs. the hook's own debounce constant).🤖 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/components/prs/state/PrsContext.tsx` around lines 895 - 925, The `useEffect` in `PrsContext` duplicates the same debounce/hidden-defer/focus-replay lifecycle handling already centralized in `useLaneListInvalidation`. Extract that timing behavior into a shared utility or hook that can accept `refreshCore()` here and the `RefreshLanes` callback used elsewhere, then have `PrsContext` use it instead of maintaining its own `scheduleRefresh`, `refreshPendingIfVisible`, and local debounce constant.
🤖 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.
Nitpick comments:
In `@apps/desktop/src/main/services/lanes/laneService.ts`:
- Around line 4937-4948: Guard the no-op path in laneService.ts for unarchive()
(and likewise archive()) so lifecycle events are only emitted when the lane
status actually changes. In unarchive(), check the current row.status before
updating; if it is already active, return early without calling db.run,
invalidateLaneListCache, or broadcastLifecycleEvent("lane-unarchived"). Mirror
the same status check pattern in archive() to avoid redundant writes, refreshes,
and toasts.
In `@apps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsx`:
- Line 183: The stale-focus refresh wired through useLaneListInvalidation is
using the raw refreshLanes path, which can duplicate this page’s existing
dedicated refresh effects. Review WorkspaceGraphPage’s refreshGraphLanes wrapper
and the related focus/visibility refresh handlers (including
refreshAutoRebaseStatuses and refreshLaneSyncStatuses), then either pass a
lighter custom wrapper into useLaneListInvalidation or align
laneLifecycleRefreshOptions so the stale-focus path does not re-run phases this
page already refreshes separately.
In `@apps/desktop/src/renderer/components/prs/state/PrsContext.tsx`:
- Around line 895-925: The `useEffect` in `PrsContext` duplicates the same
debounce/hidden-defer/focus-replay lifecycle handling already centralized in
`useLaneListInvalidation`. Extract that timing behavior into a shared utility or
hook that can accept `refreshCore()` here and the `RefreshLanes` callback used
elsewhere, then have `PrsContext` use it instead of maintaining its own
`scheduleRefresh`, `refreshPendingIfVisible`, and local debounce constant.
In `@apps/desktop/src/renderer/lib/laneReadCache.ts`:
- Around line 92-103: The cache-clearing logic in clearLaneReadInFlightForTest
and invalidateLaneReadCache is duplicated and likely to drift. Refactor
clearLaneReadInFlightForTest to call invalidateLaneReadCache() for the shared
laneListInFlight, laneSnapshotInFlight, and laneReadGeneration reset behavior,
then keep only the test-specific keybindingsInFlight.clear() in the test helper.
Use the existing function names as the single source of truth so future changes
to invalidation behavior stay consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 13fa9c6d-a4d7-4c57-9a99-6dae46eba357
⛔ Files ignored due to path filters (2)
docs/features/lanes/README.mdis excluded by!docs/**docs/features/pull-requests/README.mdis excluded by!docs/**
📒 Files selected for processing (16)
apps/desktop/src/main/services/lanes/laneService.test.tsapps/desktop/src/main/services/lanes/laneService.tsapps/desktop/src/preload/preload.test.tsapps/desktop/src/renderer/components/app/toast/useLaneEventToasts.tsapps/desktop/src/renderer/components/graph/WorkspaceGraphPage.tsxapps/desktop/src/renderer/components/lanes/LanesPage.tsxapps/desktop/src/renderer/components/prs/state/PrsContext.test.tsxapps/desktop/src/renderer/components/prs/state/PrsContext.tsxapps/desktop/src/renderer/components/prs/state/PrsContextWarmCache.test.tsxapps/desktop/src/renderer/components/run/RunPage.test.tsxapps/desktop/src/renderer/components/run/RunPage.tsxapps/desktop/src/renderer/hooks/useLaneListInvalidation.test.tsxapps/desktop/src/renderer/hooks/useLaneListInvalidation.tsapps/desktop/src/renderer/lib/laneReadCache.test.tsapps/desktop/src/renderer/lib/laneReadCache.tsapps/desktop/src/shared/types/lanes.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e47dede2a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| type RefreshLanes = AppState["refreshLanes"]; | ||
|
|
||
| export function laneLifecycleRefreshOptions(event: LaneLifecycleEvent): Parameters<RefreshLanes>[0] { | ||
| const includeStatus = event.type === "lane-created"; |
There was a problem hiding this comment.
When a lane-unarchived event arrives, the lane was previously absent from the active lane list/snapshots, so refreshLanes has no previous status to preserve. This branch requests includeStatus: false, which makes the service return the restored lane with the default clean/0-ahead/0-behind status, leaving its dirty/ahead/behind badges wrong until an unrelated full-status refresh. Treat lane-unarchived like lane-created here so newly reintroduced lanes are hydrated with real status.
Useful? React with 👍 / 👎.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| type RefreshLanes = AppState["refreshLanes"]; | ||
|
|
||
| export function laneLifecycleRefreshOptions(event: LaneLifecycleEvent): Parameters<RefreshLanes>[0] { | ||
| const includeStatus = event.type === "lane-created"; |
There was a problem hiding this comment.
When lane-unarchived reaches this refresh path, the hook requests snapshots without status recomputation because only lane-created sets includeStatus. A restored lane can reappear in the active list with stale or missing git status until a later full refresh.
Artifacts
Repro: focused Vitest test source for lane-unarchived refresh options
- Contains supporting evidence from the run (text/typescript; charset=utf-8).
Repro: verbose Vitest output showing lane-unarchived includeStatus false and assertion failure
- Keeps the command output available without making the summary code-heavy.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/hooks/useLaneListInvalidation.ts
Line: 14
Comment:
**Unarchived Lanes Skip Status**
When `lane-unarchived` reaches this refresh path, the hook requests snapshots without status recomputation because only `lane-created` sets `includeStatus`. A restored lane can reappear in the active list with stale or missing git status until a later full refresh.
How can I resolve this? If you propose a fix, please make it concise.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc6f0e7f9e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| type RefreshLanes = AppState["refreshLanes"]; | ||
|
|
||
| export function laneLifecycleRefreshOptions(event: LaneLifecycleEvent): Parameters<RefreshLanes>[0] { | ||
| const includeStatus = event.type === "lane-created"; |
There was a problem hiding this comment.
When a lane-unarchived event arrives, this refresh runs with includeStatus: false. Unarchived lanes were previously absent from the active lane list (includeArchived: false), so the app store has no prior status to preserve; the lightweight list path returns the default lane status, leaving dirty/ahead/behind/rebase badges wrong until an unrelated full status refresh happens. Treat lane-unarchived like lane-created here so the lane re-enters the UI with a computed status.
Useful? React with 👍 / 👎.
| type RefreshLanes = AppState["refreshLanes"]; | ||
|
|
||
| export function laneLifecycleRefreshOptions(event: LaneLifecycleEvent): Parameters<RefreshLanes>[0] { | ||
| const includeStatus = event.type === "lane-created"; |
There was a problem hiding this comment.
When a lane is restored, lane-unarchived reaches this path with includeStatus: false. The restored lane was not in the previous active-lane status maps, so it can reappear with missing or stale git status until a later full refresh.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/hooks/useLaneListInvalidation.ts
Line: 14
Comment:
**Unarchived Status Skips Refresh**
When a lane is restored, `lane-unarchived` reaches this path with `includeStatus: false`. The restored lane was not in the previous active-lane status maps, so it can reappear with missing or stale git status until a later full refresh.
How can I resolve this? If you propose a fix, please make it concise.
arul28
force-pushed
the
ade/start-by-running-skill-keywords-9923fd90
branch
from
fc6f0e7 to
53015b4
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53015b435b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | null>(null); | ||
| const laneSnapshots = useAppStore((s) => s.laneSnapshots); | ||
| const laneListFreshnessKey = useMemo(() => ({ lanes, laneSnapshots }), [lanes, laneSnapshots]); | ||
| useLaneListInvalidation({ active: active && Boolean(activeProjectRoot), refreshLanes, freshnessKey: laneListFreshnessKey }); |
There was a problem hiding this comment.
When App hides the Lanes surface it keeps LanesPage mounted with active=false, so this disables the only lifecycle invalidation subscription for the lane store. If a daemon/remote lane-created, rename, archive, or delete event arrives while the user is on another tab such as Work, the global toast can fire but the lane list cache is not invalidated/refreshed; switching back to /lanes also does not force a refresh because the hook treats activation as fresh. Keep the lifecycle listener active for the mounted lane surface or force a refresh when re-activating after missed events.
Useful? React with 👍 / 👎.
| type RefreshLanes = AppState["refreshLanes"]; | ||
|
|
||
| export function laneLifecycleRefreshOptions(event: LaneLifecycleEvent): Parameters<RefreshLanes>[0] { | ||
| const includeStatus = event.type === "lane-created"; |
There was a problem hiding this comment.
For a lane-unarchived lifecycle event this sets includeStatus to false, but archived lanes are excluded from the active lane list, so the unarchived lane has no previous status for refreshLanes to preserve. The statusless service response seeds it with the default clean/zero status, causing dirty/ahead/behind badges and graph/run summaries to be wrong until a later full status refresh. Treat unarchive like create here and request status.
Useful? React with 👍 / 👎.
| type RefreshLanes = AppState["refreshLanes"]; | ||
|
|
||
| export function laneLifecycleRefreshOptions(event: LaneLifecycleEvent): Parameters<RefreshLanes>[0] { | ||
| const includeStatus = event.type === "lane-created"; |
There was a problem hiding this comment.
When lane-unarchived reaches this helper, it still gets includeStatus: false. The restored lane can be fetched without fresh dirty/ahead/behind data, so it may reappear in active lane views with missing or stale git status until an unrelated full status refresh runs.
Artifacts
Repro: focused Vitest test exercising lane-unarchived helper and hook refresh options
- Contains supporting evidence from the run (text/tsx; charset=utf-8).
Repro: verbose Vitest output showing lane-unarchived stale-status assertions passed
- Keeps the command output available without making the summary code-heavy.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src/renderer/hooks/useLaneListInvalidation.ts
Line: 14
Comment:
**Unarchived Status Still Skips**
When `lane-unarchived` reaches this helper, it still gets `includeStatus: false`. The restored lane can be fetched without fresh dirty/ahead/behind data, so it may reappear in active lane views with missing or stale git status until an unrelated full status refresh runs.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Summary
Describe the change.
What Changed
Key files and behaviors.
Validation
How you tested.
Risks
Anything to watch.
Summary by CodeRabbit
New Features
Bug Fixes
Greptile Summary
This PR improves lane lifecycle refreshes across the desktop app. The main changes are:
Confidence Score: 4/5
The unarchive refresh path still needs a small fix before merging.
lane-unarchivednow reaches the renderer, but the refresh options still skip git status recomputation for restored lanes. The immediate refresh and delayed follow-up use the same stale-status option.apps/desktop/src/renderer/hooks/useLaneListInvalidation.ts
What T-Rex did
Important Files Changed
Prompt To Fix All With AI
Reviews (3): Last reviewed commit: "review: address lane lifecycle refresh f..." | Re-trigger Greptile