fix(preview): drop stale mount container on new query instead of re-caching it - #80
Conversation
…aching it
A new query (run), scope clear (clear_results) and highlight rerender
(rerender_current) clear the chunk/preview caches and DOM, then
cancel_mount_task() on the assumption the cancel is synchronous. It
isn't: the in-flight _mount_chunks_async finally runs a tick later and
unconditionally preview_cache.put()s its container back AND leaves it
mounted — re-polluting the just-cleared pane with the previous query's
half-built container ("stuck mid-mount after a new query").
Add a reset_generation counter, bumped by all three reset paths and
snapshotted at create_task time (eager coroutine-arg eval closes the
create-then-bump window). When the finally sees the generation moved,
it removes its container and skips the re-cache instead of resurrecting
the now-stale state. Non-reset cancel callers (overshoot/resume/cold
swap) don't bump, so their partial container is still kept for resume.
Regression test drives a new query onto a parked structural mount and
asserts the stale container is purged from cache, DOM and active.
|
Warning Review limit reached
More reviews will be available in 26 minutes and 7 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?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 credits. 🚦 How do rate 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 see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughPreview mounts now carry a generation token so in-flight async cleanup can detect when preview state was reset. Reset flows in the search controller and rerender path advance that token before clearing caches, and the tests cover cancellation plus a new-query regression. ChangesPreview mount invalidation
Sequence Diagram(s)sequenceDiagram
participant SearchController
participant PreviewPresenter
participant MountChunksAsync as "_mount_chunks_async"
participant PreviewCache as "preview.preview_cache"
SearchController->>PreviewPresenter: bump_reset_generation()
PreviewPresenter->>MountChunksAsync: start mount with reset_generation snapshot
MountChunksAsync->>PreviewPresenter: compare my_generation with reset_generation
alt generation changed
MountChunksAsync->>PreviewCache: remove stale PreviewContainer
MountChunksAsync->>PreviewPresenter: clear active if it matches
else generation unchanged
MountChunksAsync->>PreviewCache: cache partial container
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a reset_generation counter to track preview resets (such as new queries, scope clears, or highlight rerenders) and prevent stale in-flight mount tasks from re-polluting the DOM or cache. A regression test has also been added to verify this behavior. The reviewer suggested adding a diagnostic log message when a mount task is superseded to facilitate future debugging of race conditions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if superseded: | ||
| # A new query / scope clear / rerender cleared the caches AND the | ||
| # DOM while this mount was in flight, then cancelled us. Re- | ||
| # caching or leaving this container mounted would re-pollute the | ||
| # just-cleared pane with the previous query's half-built widget | ||
| # tree — the "stuck mid-mount after a new query" bug. Drop it. | ||
| with contextlib.suppress(Exception): | ||
| old.remove() | ||
| if container.is_complete: | ||
| container.remove() | ||
| if self.active is container: | ||
| self.active = None |
There was a problem hiding this comment.
To aid in debugging potential future race conditions, it would be beneficial to add a diagnostic log message within this if superseded: block. This would make it explicit when a mount task is correctly identified as stale and its container is dropped, which is valuable for maintaining this complex asynchronous logic.
| if superseded: | |
| # A new query / scope clear / rerender cleared the caches AND the | |
| # DOM while this mount was in flight, then cancelled us. Re- | |
| # caching or leaving this container mounted would re-pollute the | |
| # just-cleared pane with the previous query's half-built widget | |
| # tree — the "stuck mid-mount after a new query" bug. Drop it. | |
| with contextlib.suppress(Exception): | |
| old.remove() | |
| if container.is_complete: | |
| container.remove() | |
| if self.active is container: | |
| self.active = None | |
| if superseded: | |
| self.diag_log( | |
| f"mount task for {container.parent_doc_id} superseded; dropping container" | |
| ) | |
| # A new query / scope clear / rerender cleared the caches AND the | |
| # DOM while this mount was in flight, then cancelled us. Re- | |
| # caching or leaving this container mounted would re-pollute the | |
| # just-cleared pane with the previous query's half-built widget | |
| # tree — the "stuck mid-mount after a new query" bug. Drop it. | |
| with contextlib.suppress(Exception): | |
| container.remove() | |
| if self.active is container: | |
| self.active = None |
There was a problem hiding this comment.
Added a diag_log in the superseded branch recording the generation transition and the dropped container's parent id (295ed27).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fnd/tui/search_controller.py (1)
236-245: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCancel stale debounced preview loads during new-query reset.
clear_results()cancels pending preview loads, butrun()does not. A previous query’sload_timercan fire after this reset and dispatch its old(parent_id, focus_seq)under the newquery_signature, bypassing the generation guard because it starts after Line 239.Proposed fix
# Invalidate any in-flight mount before clearing: its deferred finally # must drop its (now-stale) container instead of re-caching it back into # the cache we clear just below. self._app._preview.bump_reset_generation() + self._app._preview.cancel_pending_load() self._app._preview.chunk_cache.clear()🤖 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 `@fnd/tui/search_controller.py` around lines 236 - 245, The new-query reset in run() leaves an old debounced preview load alive, so a stale load_timer can still dispatch under the new query_signature after clear_results()-style invalidation. Update run() to cancel any pending preview load before starting the new query, using the same preview-reset path as clear_results() and the existing identifiers load_timer, query_signature, and the preview cancellation/reset helpers in SearchController. Ensure the stale timer cannot fire after the generation bump and re-issue its old (parent_id, focus_seq) for the new query.
🤖 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 `@fnd/tui/preview/presenter.py`:
- Around line 1516-1527: The superseded branch in presenter.py’s mount flow only
removes the stale container, but any previously spawned container._finalize_task
can still run and affect the next mount. In the self.reset_generation !=
my_generation path, cancel the detached finaliser before container.remove(),
then clear or suppress the task reference so it cannot later hide a successor’s
progress bar or reset inflight_target; keep the change localized around the
superseded handling in the mount finalization logic.
In `@tests/test_preview_new_query_strand.py`:
- Around line 78-85: The cleanup assertion is using
preview.user_mount_in_flight() to decide when the cancelled mount is done, but
run() can clear preview.mount_task before this specific task’s finally block has
finished. Update the test to await the captured task variable directly in this
scenario, then run the stale-cache/DOM assertions after that await so they
execute only once the finaliser has fully drained.
---
Outside diff comments:
In `@fnd/tui/search_controller.py`:
- Around line 236-245: The new-query reset in run() leaves an old debounced
preview load alive, so a stale load_timer can still dispatch under the new
query_signature after clear_results()-style invalidation. Update run() to cancel
any pending preview load before starting the new query, using the same
preview-reset path as clear_results() and the existing identifiers load_timer,
query_signature, and the preview cancellation/reset helpers in SearchController.
Ensure the stale timer cannot fire after the generation bump and re-issue its
old (parent_id, focus_seq) for the new query.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9038b437-d70e-4d54-980e-40204560ba2e
📒 Files selected for processing (4)
fnd/tui/preview/presenter.pyfnd/tui/search_controller.pytests/test_preview_mount_cancel_strand.pytests/test_preview_new_query_strand.py
…mount Address review of the reset_generation fix: - The cold mount spawns a detached `_finalize_via_lock` task that, on completion, unconditionally hides the progress bar and clears `inflight_target`. Cancelling the mount task does not cancel it, so a superseded mount's finaliser would later clobber the SUCCESSOR query's bar + latch. Cancel `container._finalize_task` in the superseded branch, and clear a dangling `outgoing` reference to the dropped widget. Adds a regression test (fails if the finaliser isn't cancelled). - run(): cancel any pending debounced load inside the reset block so it is self-contained, mirroring clear_results() (the later _results.refresh() also cancels it, but don't rely on that side effect). - Diagnostic log when a mount is dropped as superseded. - Test: await the captured mount task directly instead of polling user_mount_in_flight() — run() nulls mount_task during cancellation, so the poll could return before this task's finally drained.
|
Re: the outside-diff comment on The described "stale That said, I added the explicit |
1 participant
Problem
Files getting stuck mid-mount when a new query is run — a residual of the earlier preview-strand fixes, which only covered the progress bar and the in-flight coalescing latch, not the container itself.
The three preview-reset paths —
search_controller.run()(new query),clear_results()(scope clear), andpresenter.rerender_current()(highlight toggle) — clear the chunk/preview caches, drop containers from the DOM, and callcancel_mount_task()assuming the cancel is synchronous. It isn't:cancel()only requests cancellation. The in-flight_mount_chunks_async'sfinallyruns a tick later and unconditionallypreview_cache.put()s its container back and leaves it mounted — re-polluting the just-cleared pane with the previous query's half-built container.Fix
A
reset_generationcounter onPreviewPresenter:bump_reset_generation()is called by all three reset paths before they clear the caches.create_tasktime (coroutine args evaluate eagerly, closing the create-then-bump race window).finally, if the generation moved (superseded), the task removes its container and skips the re-cache instead of resurrecting stale state.Verification
test_preview_new_query_strand.py: parks a structural cold mount mid-flight, fires a new query, asserts the stale container is purged from cache, DOM, and active.Note: the strand is a sub-perceptual timing race — the settled TUI always shows the last query's file because that mount completes; the leak is a stale container co-residing in DOM/cache. It is reproduced deterministically at the component level.