refactor(tui): extract PrefetchEngine and LazyMounter into fnd/tui/preview - #51
Conversation
Move the pre-FNDApp module region (~1,100 lines) into dedicated modules: - fnd/tui/widgets/markdown.py: the FNDMarkdown widget family, highlight span helpers, and the legacy-block markdown fallback - fnd/tui/widgets/results_tree.py: ResultsTree - fnd/tui/widgets/preview_container.py: PreviewContainer, PreviewCache, the open-with Hit adapter, and the preview-cache tunables - fnd/tui/results_labels.py: row-label and score formatting Pure relocation; every moved name keeps its spelling and app.py re-exports the externally-imported surface, so sibling modules and the test suite are unaffected. The lazy-mount/prune tunables that are read at call time stay defined in app.py.
Move search-scope state (collections / sources / filters), the sidebar panel rendering and toggle handlers, and UI-state persistence into ScopeController (fnd/tui/scope_panel.py). The filter presentation tables move with their only consumer. FNDApp keeps its existing surface: the seven scope fields are exposed as read/write properties delegating to the controller, the persistence and panel-refresh entry points stay as one-line delegators, and the @on tree handlers remain bound to the app class as thin forwards. Cross-concern calls from scope code (query rerun, status refresh, ranking-profile resolution) route back through the app.
Move the background-indexer lifecycle — the async task, cancel event, event queue, run-generation counter, and the update-all chain bookkeeping — into IndexerService (fnd/tui/indexer_service.py), together with the start/resume/reindex entry points. FNDApp keeps its existing surface: start_indexer remains a real app method with an identical signature (chain continuations re-enter through it, so a patched app method keeps intercepting starts), the 17 _indexer_* fields become read/write properties, and the resume / reindex helpers stay as one-line delegators. indexer_modal.py and the settings screens are untouched — they keep reading and writing the app's _indexer_* accessors.
SearchController (fnd/tui/search_controller.py) owns the searcher handle, active query + match spec, result groups, search trace, synonyms, and ranking profile; run() is the single query entry point and _PrefixingSearcher moves with it. The preview-cache invalidation inside run()/clear_results() still reaches through the app until the preview subsystem is extracted. ResultsView (fnd/tui/results_view.py) renders and relabels the results tree; it owns no state. FNDApp keeps its existing surface: the nine search fields become read/write properties, query entry points and helpers used by sibling modules stay as one-line delegators, and @on handlers / actions remain bound to the app class.
FlatBufferView (fnd/tui/preview/flat_view.py) owns the flat preview path: the shared LineBufferPreview widget, the per-file rendered- document cache, the installed-key bookkeeping, and the install / activate / reset lifecycle. First module of the fnd/tui/preview package; the structural mount machinery follows separately. FNDApp keeps its surface: the four flat-buffer fields become read/write properties and the six entry points stay as one-line delegators (the dispatch/prefetch code that calls them is still on the app until its own extraction).
…eview PrefetchEngine (fnd/tui/preview/prefetch.py) owns the top-N warming pipeline: the decode worker fan-out, the main-thread record sinks, the hidden pre-mount jobs, and the single-consumer sink queue + drainer. The drainer task is still created from the app's on_mount (via PrefetchEngine.start) so task timing matches the app lifecycle, and worker closures keep calling through the app so user-side state always wins. LazyMounter (fnd/tui/preview/lazy_mount.py) owns the scroll-driven mount: the debounced boundary check, the batch mounts in both directions, and the cancel path invoked on file switch / query change. Mount-window tunables stay defined in app.py and are read off the app module at call time, preserving their existing override point. FNDApp keeps one-line delegators for every entry point still used by the remaining mount machinery, plus a read/write property for the in-flight lazy task.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 refactors the preview subsystem by extracting scroll-driven lazy mounting and background prefetching logic from the main FNDApp class into two new dedicated classes: LazyMounter and PrefetchEngine. This significantly simplifies fnd/tui/app.py. The reviewer feedback focuses on improving type safety and code cleanliness in the newly created files. Specifically, the reviewer suggests moving standard library imports (like asyncio and contextlib) to the top of the files, which allows for more precise type annotations (e.g., using asyncio.Task and Timer instead of object or Any) and the removal of redundant local imports and # type: ignore comments.
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.
| from __future__ import annotations | ||
|
|
||
| from typing import TYPE_CHECKING | ||
|
|
||
| from textual.containers import VerticalScroll | ||
| from textual.widget import Widget | ||
|
|
||
| from fnd.tui.widgets.markdown import FNDMarkdown | ||
|
|
||
| if TYPE_CHECKING: | ||
| from fnd.query import FileChunk | ||
| from fnd.tui.app import FNDApp | ||
| from fnd.tui.widgets.preview_container import PreviewContainer | ||
|
|
||
| __all__ = ["LazyMounter"] |
There was a problem hiding this comment.
To adhere to PEP 8 and improve type safety, standard library modules like asyncio and contextlib should be imported at the top of the file. Additionally, we can import Any from typing and Timer from textual.timer to type the class attributes more precisely.
from __future__ import annotations
import asyncio
import contextlib
from typing import TYPE_CHECKING, Any
from textual.containers import VerticalScroll
from textual.timer import Timer
from textual.widget import Widget
from fnd.tui.widgets.markdown import FNDMarkdown
if TYPE_CHECKING:
from fnd.query import FileChunk
from fnd.tui.app import FNDApp
from fnd.tui.widgets.preview_container import PreviewContainer
__all__ = ["LazyMounter"]| self.task: object | None = None | ||
| # Monotonic-time gate. Programmatic scrolls (navigation anchor, | ||
| # finalize reveal) push this forward so the watcher doesn't | ||
| # interpret their own scroll changes as user intent and fire a | ||
| # competing mount that yanks the focused chunk off-screen. | ||
| # Debounce timer so rapid scroll bursts collapse to a single | ||
| # check at the tail end — protects programmatic intermediate | ||
| # scrolls AND smooths user wheel/key scroll bursts. | ||
| self.check_timer: object | None = None |
There was a problem hiding this comment.
With asyncio and Timer imported at the top of the file, we can type self.task as asyncio.Task[Any] | None and self.check_timer as Timer | None. This avoids using the overly generic object type and eliminates the need for several # type: ignore comments throughout the class.
| self.task: object | None = None | |
| # Monotonic-time gate. Programmatic scrolls (navigation anchor, | |
| # finalize reveal) push this forward so the watcher doesn't | |
| # interpret their own scroll changes as user intent and fire a | |
| # competing mount that yanks the focused chunk off-screen. | |
| # Debounce timer so rapid scroll bursts collapse to a single | |
| # check at the tail end — protects programmatic intermediate | |
| # scrolls AND smooths user wheel/key scroll bursts. | |
| self.check_timer: object | None = None | |
| # In-flight lazy-mount task (driven by scroll). One at a time; | |
| # cleared on file switch alongside ``_preview_mount_task``. | |
| self.task: asyncio.Task[Any] | None = None | |
| # Monotonic-time gate. Programmatic scrolls (navigation anchor, | |
| # finalize reveal) push this forward so the watcher doesn't | |
| # interpret their own scroll changes as user intent and fire a | |
| # competing mount that yanks the focused chunk off-screen. | |
| # Debounce timer so rapid scroll bursts collapse to a single | |
| # check at the tail end — protects programmatic intermediate | |
| # scrolls AND smooths user wheel/key scroll bursts. | |
| self.check_timer: Timer | None = None |
| import contextlib | ||
|
|
||
| if self.check_timer is not None: | ||
| with contextlib.suppress(Exception): | ||
| self.check_timer.stop() # type: ignore[attr-defined] | ||
| self.check_timer = None | ||
| task = self.task | ||
| if task is None: | ||
| return | ||
| try: | ||
| done = task.done() # type: ignore[attr-defined] | ||
| except Exception: | ||
| done = True | ||
| if not done: | ||
| with contextlib.suppress(Exception): | ||
| task.cancel() # type: ignore[attr-defined] |
There was a problem hiding this comment.
Since contextlib is now imported at the top of the file and self.task and self.check_timer are properly typed, we can remove the redundant local import and the # type: ignore[attr-defined] comments.
| import contextlib | |
| if self.check_timer is not None: | |
| with contextlib.suppress(Exception): | |
| self.check_timer.stop() # type: ignore[attr-defined] | |
| self.check_timer = None | |
| task = self.task | |
| if task is None: | |
| return | |
| try: | |
| done = task.done() # type: ignore[attr-defined] | |
| except Exception: | |
| done = True | |
| if not done: | |
| with contextlib.suppress(Exception): | |
| task.cancel() # type: ignore[attr-defined] | |
| if self.check_timer is not None: | |
| with contextlib.suppress(Exception): | |
| self.check_timer.stop() | |
| self.check_timer = None | |
| task = self.task | |
| if task is None: | |
| return | |
| try: | |
| done = task.done() | |
| except Exception: | |
| done = True | |
| if not done: | |
| with contextlib.suppress(Exception): | |
| task.cancel() |
| if self.check_timer is not None: | ||
| with contextlib.suppress(Exception): | ||
| self.check_timer.stop() # type: ignore[attr-defined] |
There was a problem hiding this comment.
Since contextlib is imported at the top of the file and self.check_timer is properly typed as Timer, we can remove the redundant local import and the # type: ignore comment.
| if self.check_timer is not None: | |
| with contextlib.suppress(Exception): | |
| self.check_timer.stop() # type: ignore[attr-defined] | |
| if self.check_timer is not None: | |
| with contextlib.suppress(Exception): | |
| self.check_timer.stop() |
| task = self.task | ||
| if task is not None: | ||
| try: | ||
| if not task.done(): # type: ignore[attr-defined] | ||
| return | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Since self.task is properly typed as asyncio.Task[Any], we can remove the # type: ignore[attr-defined] comment.
| task = self.task | |
| if task is not None: | |
| try: | |
| if not task.done(): # type: ignore[attr-defined] | |
| return | |
| except Exception: | |
| pass | |
| task = self.task | |
| if task is not None: | |
| try: | |
| if not task.done(): | |
| return | |
| except Exception: | |
| pass |
| import asyncio | ||
| import contextlib |
There was a problem hiding this comment.
The local imports of asyncio and contextlib are redundant because they are already imported at the top of the file.
| # busy across navigation. | ||
| q = self.sink_queue | ||
| if q is not None: | ||
| import contextlib as _contextlib |
There was a problem hiding this comment.
The local import of contextlib as _contextlib is redundant because contextlib is already imported at the top of the file. We can use contextlib.suppress directly.
| with _contextlib.suppress(Exception): | ||
| q.task_done() |
There was a problem hiding this comment.
Use contextlib.suppress directly instead of the local alias _contextlib.suppress.
| with _contextlib.suppress(Exception): | |
| q.task_done() | |
| with contextlib.suppress(Exception): | |
| q.task_done() |
| import asyncio | ||
| import contextlib |
There was a problem hiding this comment.
The local imports of asyncio and contextlib are redundant because they are already imported at the top of the file.
| @property | ||
| def _lazy_mount_task(self) -> object | None: | ||
| return self._lazy.task | ||
|
|
||
| async def _cancel_prefetch_task_on(self, container: PreviewContainer) -> None: | ||
| """Cancel + await any background prefetch task on ``container`` | ||
| so the user-side mount doesn't race it and trip MountError.""" | ||
| import asyncio | ||
| import contextlib | ||
| @_lazy_mount_task.setter | ||
| def _lazy_mount_task(self, value: object | None) -> None: | ||
| self._lazy.task = value |
There was a problem hiding this comment.
We can type _lazy_mount_task more precisely as asyncio.Task[Any] | None instead of object | None to improve type safety and match the type of LazyMounter.task.
| @property | |
| def _lazy_mount_task(self) -> object | None: | |
| return self._lazy.task | |
| async def _cancel_prefetch_task_on(self, container: PreviewContainer) -> None: | |
| """Cancel + await any background prefetch task on ``container`` | |
| so the user-side mount doesn't race it and trip MountError.""" | |
| import asyncio | |
| import contextlib | |
| @_lazy_mount_task.setter | |
| def _lazy_mount_task(self, value: object | None) -> None: | |
| self._lazy.task = value | |
| @property | |
| def _lazy_mount_task(self) -> asyncio.Task[Any] | None: | |
| return self._lazy.task | |
| @_lazy_mount_task.setter | |
| def _lazy_mount_task(self, value: asyncio.Task[Any] | None) -> None: | |
| self._lazy.task = value |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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/indexer_service.py`:
- Around line 331-340: The resume logic uses the hard-coded default index root;
change the saved-state lookup and the resumed start to use the app’s configured
index root (self._app._index_dir) instead of default_index_dir() or
state_file_for("default"). In practice, pass self._app._index_dir into the state
file lookup (replace state_file_for("default") usage) and call
self._app.start_indexer(...) with index_dir=self._app._index_dir (replace
index_dir=default_index_dir()), so both is_state_resumable/load_state and
start_indexer operate on the same index root.
- Around line 127-128: When re-opening the running modal the code path that
pushes IndexerScreen uses only the collection name and loses multi-collection
progress; update the branch that runs when open_modal and _bump_seq to pass the
existing chain context (chain_total and chain_index) into IndexerScreen (the
same way the fresh-start flow does), ensuring you read the current chain_total
and chain_index variables and forward them to IndexerScreen(self.collection or
collection, chain_total=..., chain_index=...).
In `@fnd/tui/preview/prefetch.py`:
- Around line 123-138: The prefetch filter only checks self._app._preview_cache
so files that already have flat renders in self._app._flat_buffer_cache still
get re-targeted; update the same conditional that computes in_preview to also
check the flat buffer (e.g., treat warmed_flat =
self._app._flat_buffer_cache.get(g.parent_id, query_sig_for_filter) is not None)
and consider a result warmed if either warmed_flat or in_preview is true before
appending to already_cached/continuing, so build_rendered_document isn't re-run
for already-prefetched flat results; adjust the logic around
in_preview/is_active/targets to use this combined check.
- Around line 467-492: The finally block may insert a stale partially-mounted
container into _preview_cache; before calling
self._app._preview_cache.put(container, ...), verify the container still belongs
to the active query by comparing its query signature to the current signature
(i.e., only cache when container.query_sig (or container.sig if that is the
existing attribute you use to store the query signature) equals the current
signature from the mounting context such as parent_id/current query signature),
and skip caching (and call container.remove()) when they differ; update
_mount_chunk_loop()/container creation to set/propagate the signature if it does
not already exist so the check can be performed.
In `@fnd/tui/scope_panel.py`:
- Around line 331-350: The pruning logic in the currently_full/remove-collection
branch uses only self.active_sources and cannot tell why an id is active
(collection inheritance vs explicit user selection), so replace the flat-id
removal with provenance-aware logic: introduce a per-source provenance structure
(e.g. self.source_provenance: mapping from source_id -> set of owners or a
refcount plus a flag for explicit user selection) and update it whenever
collections are added/removed and when the user explicitly toggles sources; then
in the block around currently_full / collection_source_ids use that provenance
to remove only those source ids whose provenance no longer contains any owner
(and are not explicitly selected), updating self.active_sources accordingly;
update collection_source_ids, collection add/remove paths, and any
explicit-toggle handlers to maintain the provenance mapping.
In `@fnd/tui/search_controller.py`:
- Around line 156-162: The error-handling paths after QueryPlan.from_user_text
failure currently only clear self.groups and refresh the tree, leaving stale
preview/cache and trace; update both error blocks (the one around
QueryPlan.from_user_text and the other at lines 215-219) to call the
controller's clear_results() method and set self.latest_trace = None (or reset
latest_trace) before returning, while still invoking self._show_query_notice(e)
so the UI fully clears previews/traces and rebuilds clean state.
In `@fnd/tui/widgets/markdown.py`:
- Around line 667-689: The update method must reset per-render state so callers
can't observe a stale build_done or first_match_block; at the start of
Markdown.update (the override in this class) reinitialize self.build_done to a
new _asyncio.Event() and set self._first_match_block = None (so the
first_match_block property is cleared) before calling super().update(markdown),
then keep the existing aw._future.add_done_callback(...) behavior to set the new
build_done when the parse+mount completes.
- Around line 416-422: The current compose path stores header-only matches
returned by _find_first_match_coord_in_table() (which returns (0, col) for TH
hits) into dt._fnd_match_coord, causing DataTable scrolling to target a body
row; change the logic so after calling _find_first_match_coord_in_table(headers,
rows, spec) you still set md._first_match_block = self for header hits but only
assign dt._fnd_match_coord = Coordinate(*match_coord) when match_coord is not
None and match_coord[0] > 0 (i.e., an actual body row); apply the same fix in
the other compose location (the block around lines 571-580) and add a regression
test that runs a query matching only a table header to assert the table wrapper
(not a body cell) is the scroll target.
🪄 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: 49aabcd9-12e3-4e7a-bc34-923c8f8991cf
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
fnd/tui/app.pyfnd/tui/indexer_service.pyfnd/tui/preview/__init__.pyfnd/tui/preview/flat_view.pyfnd/tui/preview/lazy_mount.pyfnd/tui/preview/prefetch.pyfnd/tui/results_labels.pyfnd/tui/results_view.pyfnd/tui/scope_panel.pyfnd/tui/search_controller.pyfnd/tui/widgets/markdown.pyfnd/tui/widgets/preview_container.pyfnd/tui/widgets/results_tree.py
| if open_modal and _bump_seq: | ||
| self._app.push_screen(IndexerScreen(self.collection or collection)) |
There was a problem hiding this comment.
Preserve chain progress when re-opening the running modal.
Lines 127-128 rebuild IndexerScreen with only the collection name, but the fresh-start path on Lines 221-229 also passes chain_total and chain_index. During an update-all run, re-opening progress will therefore lose the current (N of M) context and render as a single-collection screen.
Proposed fix
if open_modal and _bump_seq:
- self._app.push_screen(IndexerScreen(self.collection or collection))
+ chain_total = getattr(self, "chain_total", 1) or 1
+ chain_pending = getattr(self, "chain_remaining", None) or []
+ chain_index = max(1, chain_total - len(chain_pending))
+ self._app.push_screen(
+ IndexerScreen(
+ self.collection or collection,
+ chain_total=chain_total,
+ chain_index=chain_index,
+ )
+ )
return False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if open_modal and _bump_seq: | |
| self._app.push_screen(IndexerScreen(self.collection or collection)) | |
| if open_modal and _bump_seq: | |
| chain_total = getattr(self, "chain_total", 1) or 1 | |
| chain_pending = getattr(self, "chain_remaining", None) or [] | |
| chain_index = max(1, chain_total - len(chain_pending)) | |
| self._app.push_screen( | |
| IndexerScreen( | |
| self.collection or collection, | |
| chain_total=chain_total, | |
| chain_index=chain_index, | |
| ) | |
| ) | |
| return False |
🤖 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/indexer_service.py` around lines 127 - 128, When re-opening the
running modal the code path that pushes IndexerScreen uses only the collection
name and loses multi-collection progress; update the branch that runs when
open_modal and _bump_seq to pass the existing chain context (chain_total and
chain_index) into IndexerScreen (the same way the fresh-start flow does),
ensuring you read the current chain_total and chain_index variables and forward
them to IndexerScreen(self.collection or collection, chain_total=...,
chain_index=...).
| state = load_state(state_file_for("default")) | ||
| if not is_state_resumable( | ||
| state, known_collections=set(cfg.collections), now=_dt.datetime.now(tz=_dt.UTC) | ||
| ): | ||
| return | ||
| assert state is not None # narrowed by is_state_resumable | ||
| try: | ||
| self._app.start_indexer( | ||
| collection="default", index_dir=default_index_dir(), open_modal=False | ||
| ) |
There was a problem hiding this comment.
Use the app’s configured index directory for auto-resume.
Lines 331-340 hard-code the default index root, but this service otherwise treats self._app._index_dir as the source of truth for the active session. On an app constructed with a non-default index directory, maybe_resume() can resume work in one directory while on_reindex_complete() reopens the searcher from another, so the resumed rebuild never becomes visible in the live session.
Please thread self._app._index_dir through both the saved-state lookup and the resumed start_indexer() call so resume uses the same index root as the rest of the app.
🤖 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/indexer_service.py` around lines 331 - 340, The resume logic uses the
hard-coded default index root; change the saved-state lookup and the resumed
start to use the app’s configured index root (self._app._index_dir) instead of
default_index_dir() or state_file_for("default"). In practice, pass
self._app._index_dir into the state file lookup (replace
state_file_for("default") usage) and call self._app.start_indexer(...) with
index_dir=self._app._index_dir (replace index_dir=default_index_dir()), so both
is_state_resumable/load_state and start_indexer operate on the same index root.
| # Filter by preview_cache (widget tree ready), not chunk_cache: | ||
| # a file whose chunks are cached but whose mount got drained | ||
| # by a prior cursor move must be re-queued. Also skip if it's | ||
| # the active preview — that one's owned by the user-side path. | ||
| in_preview = self._app._preview_cache.get(g.parent_id, query_sig_for_filter) is not None | ||
| is_active = ( | ||
| self._app._active_preview is not None | ||
| and self._app._active_preview.parent_doc_id == g.parent_id | ||
| and self._app._active_preview.query_signature == query_sig_for_filter | ||
| ) | ||
| if in_preview or is_active: | ||
| already_cached.append(g.parent_id[:8]) | ||
| continue | ||
| focus = g.hits[0].chunk_seq if g.hits else 0 | ||
| targets.append((g.parent_id, focus)) | ||
| if len(targets) >= n: |
There was a problem hiding this comment.
Skip flat previews that are already warmed.
This filter only consults self._app._preview_cache, so a flat result already present in self._app._flat_buffer_cache is still re-targeted on every anchor move. That forces build_rendered_document(...) to run again for files that are already prefetched, which burns CPU on the hot navigation path for no gain.
💡 Suggested fix
- in_preview = self._app._preview_cache.get(g.parent_id, query_sig_for_filter) is not None
+ in_preview = self._app._preview_cache.get(g.parent_id, query_sig_for_filter) is not None
+ in_flat_cache = (
+ (g.parent_id, query_sig_for_filter) in self._app._flat_buffer_cache
+ )
is_active = (
self._app._active_preview is not None
and self._app._active_preview.parent_doc_id == g.parent_id
and self._app._active_preview.query_signature == query_sig_for_filter
)
- if in_preview or is_active:
+ if in_preview or in_flat_cache or is_active:
already_cached.append(g.parent_id[:8])
continue🤖 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/preview/prefetch.py` around lines 123 - 138, The prefetch filter only
checks self._app._preview_cache so files that already have flat renders in
self._app._flat_buffer_cache still get re-targeted; update the same conditional
that computes in_preview to also check the flat buffer (e.g., treat warmed_flat
= self._app._flat_buffer_cache.get(g.parent_id, query_sig_for_filter) is not
None) and consider a result warmed if either warmed_flat or in_preview is true
before appending to already_cached/continuing, so build_rendered_document isn't
re-run for already-prefetched flat results; adjust the logic around
in_preview/is_active/targets to use this combined check.
| finally: | ||
| _perf.mark( | ||
| "prefetch_loop_end", | ||
| parent_id=parent_id, | ||
| n_mounted=n_mounted, | ||
| mounted_indices_size=len(container.mounted_indices), | ||
| is_complete=container.is_complete, | ||
| ) | ||
| self._app._diag_log( | ||
| f"prefetch_loop_end parent={parent_id[:8]} n_mounted={n_mounted} " | ||
| f"mounted_size={len(container.mounted_indices)} " | ||
| f"is_complete={container.is_complete}" | ||
| ) | ||
| if container.mounted_indices: | ||
| evicted = self._app._preview_cache.put(container, protect=self._app._active_preview) | ||
| for old in evicted: | ||
| with contextlib.suppress(Exception): | ||
| old.remove() | ||
| else: | ||
| # Loop bailed on user-mount-in-flight (or every mount raised) | ||
| # before any chunk landed. Caching the empty container would | ||
| # block the next prefetch attempt for this (parent_id, sig) | ||
| # via the already-cached short-circuit; instead, drop it so a | ||
| # later trigger (cursor move, second query) can retry cleanly. | ||
| with contextlib.suppress(Exception): | ||
| container.remove() |
There was a problem hiding this comment.
Do not cache a prefetch container after its query has gone stale.
_mount_chunk_loop() returns as soon as the query signature changes, but this finally block still inserts any partially mounted container into _preview_cache. With the cache capped to a single file, that late stale insert can evict the current query’s warmed preview and repopulate hidden DOM that _run_query has just cleared.
💡 Suggested fix
finally:
_perf.mark(
"prefetch_loop_end",
parent_id=parent_id,
n_mounted=n_mounted,
mounted_indices_size=len(container.mounted_indices),
is_complete=container.is_complete,
)
self._app._diag_log(
f"prefetch_loop_end parent={parent_id[:8]} n_mounted={n_mounted} "
f"mounted_size={len(container.mounted_indices)} "
f"is_complete={container.is_complete}"
)
- if container.mounted_indices:
+ stale = query_sig != self._app._current_query_signature()
+ if stale:
+ with contextlib.suppress(Exception):
+ container.remove()
+ elif container.mounted_indices:
evicted = self._app._preview_cache.put(container, protect=self._app._active_preview)
for old in evicted:
with contextlib.suppress(Exception):
old.remove()
else:
# Loop bailed on user-mount-in-flight (or every mount raised)
# before any chunk landed. Caching the empty container would
# block the next prefetch attempt for this (parent_id, sig)
# via the already-cached short-circuit; instead, drop it so a
# later trigger (cursor move, second query) can retry cleanly.
with contextlib.suppress(Exception):
container.remove()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| finally: | |
| _perf.mark( | |
| "prefetch_loop_end", | |
| parent_id=parent_id, | |
| n_mounted=n_mounted, | |
| mounted_indices_size=len(container.mounted_indices), | |
| is_complete=container.is_complete, | |
| ) | |
| self._app._diag_log( | |
| f"prefetch_loop_end parent={parent_id[:8]} n_mounted={n_mounted} " | |
| f"mounted_size={len(container.mounted_indices)} " | |
| f"is_complete={container.is_complete}" | |
| ) | |
| if container.mounted_indices: | |
| evicted = self._app._preview_cache.put(container, protect=self._app._active_preview) | |
| for old in evicted: | |
| with contextlib.suppress(Exception): | |
| old.remove() | |
| else: | |
| # Loop bailed on user-mount-in-flight (or every mount raised) | |
| # before any chunk landed. Caching the empty container would | |
| # block the next prefetch attempt for this (parent_id, sig) | |
| # via the already-cached short-circuit; instead, drop it so a | |
| # later trigger (cursor move, second query) can retry cleanly. | |
| with contextlib.suppress(Exception): | |
| container.remove() | |
| finally: | |
| _perf.mark( | |
| "prefetch_loop_end", | |
| parent_id=parent_id, | |
| n_mounted=n_mounted, | |
| mounted_indices_size=len(container.mounted_indices), | |
| is_complete=container.is_complete, | |
| ) | |
| self._app._diag_log( | |
| f"prefetch_loop_end parent={parent_id[:8]} n_mounted={n_mounted} " | |
| f"mounted_size={len(container.mounted_indices)} " | |
| f"is_complete={container.is_complete}" | |
| ) | |
| stale = query_sig != self._app._current_query_signature() | |
| if stale: | |
| with contextlib.suppress(Exception): | |
| container.remove() | |
| elif container.mounted_indices: | |
| evicted = self._app._preview_cache.put(container, protect=self._app._active_preview) | |
| for old in evicted: | |
| with contextlib.suppress(Exception): | |
| old.remove() | |
| else: | |
| # Loop bailed on user-mount-in-flight (or every mount raised) | |
| # before any chunk landed. Caching the empty container would | |
| # block the next prefetch attempt for this (parent_id, sig) | |
| # via the already-cached short-circuit; instead, drop it so a | |
| # later trigger (cursor move, second query) can retry cleanly. | |
| with contextlib.suppress(Exception): | |
| container.remove() |
🤖 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/preview/prefetch.py` around lines 467 - 492, The finally block may
insert a stale partially-mounted container into _preview_cache; before calling
self._app._preview_cache.put(container, ...), verify the container still belongs
to the active query by comparing its query signature to the current signature
(i.e., only cache when container.query_sig (or container.sig if that is the
existing attribute you use to store the query signature) equals the current
signature from the mounting context such as parent_id/current query signature),
and skip caching (and call container.remove()) when they differ; update
_mount_chunk_loop()/container creation to set/propagate the signature if it does
not already exist so the check can be performed.
| currently_full = name in self.collections or ( | ||
| bool(source_ids) and all(sid in self.active_sources for sid in source_ids) | ||
| ) | ||
| if currently_full: | ||
| if name in self.collections: | ||
| self.collections.remove(name) | ||
| if source_ids: | ||
| # A source shared with a still-active collection stays | ||
| # on — only drop ids no remaining collection claims. | ||
| still_claimed = { | ||
| sid | ||
| for other in self.collections | ||
| for sid in self.collection_source_ids(other) | ||
| } | ||
| keep = (set(self.active_sources) - set(source_ids)) | ( | ||
| set(self.active_sources) & still_claimed | ||
| ) | ||
| # Preserve the user's relative ordering of the kept | ||
| # sources (set difference loses it). | ||
| self.active_sources = [s for s in self.active_sources if s in keep] |
There was a problem hiding this comment.
Track shared-source selection with provenance, not just a flat id list.
Lines 331-350 prune active_sources after removing the collection, but the state at that point only knows that a source id is “on”, not why it is on. For a source shared by collections A and B, turning A off cannot distinguish “this id was inherited from A being fully on” from “this id is still explicitly selected through B”. The prune can therefore silently clear a row the user still has selected in B, which narrows subsequent searches unexpectedly.
This needs per-collection source provenance, ref-counting, or an equivalent ownership model before the toggle-off path can be made correct.
🤖 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/scope_panel.py` around lines 331 - 350, The pruning logic in the
currently_full/remove-collection branch uses only self.active_sources and cannot
tell why an id is active (collection inheritance vs explicit user selection), so
replace the flat-id removal with provenance-aware logic: introduce a per-source
provenance structure (e.g. self.source_provenance: mapping from source_id -> set
of owners or a refcount plus a flag for explicit user selection) and update it
whenever collections are added/removed and when the user explicitly toggles
sources; then in the block around currently_full / collection_source_ids use
that provenance to remove only those source ids whose provenance no longer
contains any owner (and are not explicitly selected), updating
self.active_sources accordingly; update collection_source_ids, collection
add/remove paths, and any explicit-toggle handlers to maintain the provenance
mapping.
| try: | ||
| plan = QueryPlan.from_user_text(query) | ||
| except QueryError as e: | ||
| self._show_query_notice(e) | ||
| self.groups = [] | ||
| self._app._refresh_results_tree() | ||
| return |
There was a problem hiding this comment.
Clear preview state on invalid queries.
Lines 156-162 and 215-219 only empty groups and rebuild the tree. After a successful search, a malformed query or filter leaves the old preview mounted, the old preview/cache state intact, and the previous trace still hanging around, so the UI can show an empty results pane beside stale content. Route both error paths through the same cleanup as clear_results() and reset latest_trace before returning.
Possible fix
try:
plan = QueryPlan.from_user_text(query)
except QueryError as e:
self._show_query_notice(e)
- self.groups = []
- self._app._refresh_results_tree()
+ self.latest_trace = None
+ self.clear_results()
return
@@
try:
self.groups = self._search_layered(
lexical=lexical,
filter_prefix=filter_prefix,
limit=50,
@@
)
except (QueryError, FilterError) as e:
self._show_query_notice(e)
- self.groups = []
- self._app._refresh_results_tree()
+ self.latest_trace = None
+ self.clear_results()
returnAlso applies to: 215-219
🤖 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 156 - 162, The error-handling
paths after QueryPlan.from_user_text failure currently only clear self.groups
and refresh the tree, leaving stale preview/cache and trace; update both error
blocks (the one around QueryPlan.from_user_text and the other at lines 215-219)
to call the controller's clear_results() method and set self.latest_trace = None
(or reset latest_trace) before returning, while still invoking
self._show_query_notice(e) so the UI fully clears previews/traces and rebuilds
clean state.
| match_coord = _find_first_match_coord_in_table(headers, rows, spec) | ||
| if match_coord is not None: | ||
| dt._fnd_match_coord = Coordinate(*match_coord) # type: ignore[attr-defined] | ||
| # Register self as parent's first_match_block — TH/TD | ||
| # widgets are bypassed so _record_first_match never fires. | ||
| if isinstance(md, FNDMarkdown) and md._first_match_block is None: | ||
| md._first_match_block = self |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Textual 8.0.0, do DataTablecoordinates used byget_cell_at(...)and_get_cell_region(...) address header cells, or only body rows?
💡 Result:
In Textual 8.0.0, the DataTable coordinates used by get_cell_at(...) and _get_cell_region(...) refer to body-row data cells, not header cells. get_cell_at(coordinate) converts the provided Coordinate(row_index, column_index) into a row_key and column_key via coordinate_to_cell_key(...), then returns get_cell(row_key, column_key) (which is backed by self._data[row_key][column_key]). This means header coordinates are not mapped to a “header cell” value lookup; they map to actual row/column cell data keys instead. [1] _get_cell_region(coordinate) similarly treats coordinate.row as an index into self.ordered_rows and uses it to look up row_key = self._row_locations.get_key(row_index) and then row = self.rows[row_key]. It does have a if self.show_header: y += self.header_height offset when computing the y-position, but that offset is only to shift where the data-row regions are drawn vertically when the header is shown—not to select header rows/cells as part of the coordinate system. [2] Bottom line: the coordinate system for these methods addresses the data grid (body rows) cells; header visuals affect rendering offsets, but header cells are not addressed via get_cell_at(...)/_get_cell_region(...) coordinates. [1][2]
Citations:
- 1: https://github.com/Textualize/textual/blob/f8b42bd7/src/textual/widgets/_data_table.py
- 2: https://github.com/Textualize/textual/blob/813aeeac/src/textual/widgets/_data_table.py
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant file(s)
ls -la fnd/tui/widgets || true
sed -n '350,650p' fnd/tui/widgets/markdown.py | nl -ba | sed -n '380,620p'Repository: ben-dev-au/fnd
Length of output: 560
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show where match coord is defined/used
rg -n "_find_first_match_coord_in_table|_fnd_match_coord|first_match_block|match_coord" fnd/tui/widgets/markdown.py
# Print the relevant sections (avoid relying on nl)
sed -n '380,470p' fnd/tui/widgets/markdown.py
echo "----"
sed -n '520,620p' fnd/tui/widgets/markdown.pyRepository: ben-dev-au/fnd
Length of output: 10060
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "_fnd_match_coord" -S .
rg -n "_fnd_match_coord|first_match_block|match_coord" -S fnd/tui/widgets/markdown.pyRepository: ben-dev-au/fnd
Length of output: 1933
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '430,590p' fnd/tui/preview_scroll.py
echo "----"
sed -n '240,340p' tests/test_preview_scrolls_to_match.py
echo "----"
sed -n '1,220p' tests/test_preview_scroll_characterization.py
echo "----"
sed -n '150,260p' fnd/tui/_md_hybrid.pyRepository: ben-dev-au/fnd
Length of output: 25539
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show where _scroll_proxy_for / _scroll_pane_to_table_cell are used
rg -n "_scroll_proxy_for\\(|_scroll_pane_to_table_cell\\(|_fallback_match_target\\(" fnd/tui/preview_scroll.py
# Inspect surrounding scroll-to-match logic
sed -n '1,260p' fnd/tui/preview_scroll.py
echo "----"
sed -n '260,430p' fnd/tui/preview_scroll.py
echo "----"
sed -n '430,720p' fnd/tui/preview_scroll.pyRepository: ben-dev-au/fnd
Length of output: 30668
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "header|TH/TD|tbody|thead|Term0|Term1|Deterministic|tall_table|match in.*header|header-only" tests/test_preview_scroll*.py tests -S
rg -n "_find_first_match_coord_in_table" tests -SRepository: ben-dev-au/fnd
Length of output: 13668
Don’t encode header-only matches as DataTable body-row (row 0) coordinates.
_find_first_match_coord_in_table() returns (0, col) for header hits, and compose() stores it in dt._fnd_match_coord. The scroll controller then uses that coordinate with DataTable._get_cell_region(coord) to compute the scroll target; DataTable row/column coordinates address body/data cells (not TH/header cells), so a header-only match will scroll to the first body row in that column instead of the highlighted header.
Keep md._first_match_block = self for header matches, but only set _fnd_match_coord when the match is found in an actual body cell (or otherwise leave _fnd_match_coord unset so the controller falls back to scrolling the table wrapper). Add a regression test for a query that matches only within table headers.
File: fnd/tui/widgets/markdown.py (416-422)
Also applies to: 571-580
🤖 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/widgets/markdown.py` around lines 416 - 422, The current compose path
stores header-only matches returned by _find_first_match_coord_in_table() (which
returns (0, col) for TH hits) into dt._fnd_match_coord, causing DataTable
scrolling to target a body row; change the logic so after calling
_find_first_match_coord_in_table(headers, rows, spec) you still set
md._first_match_block = self for header hits but only assign dt._fnd_match_coord
= Coordinate(*match_coord) when match_coord is not None and match_coord[0] > 0
(i.e., an actual body row); apply the same fix in the other compose location
(the block around lines 571-580) and add a regression test that runs a query
matching only a table header to assert the table wrapper (not a body cell) is
the scroll target.
| self._first_match_block: MarkdownBlock | None = None | ||
| # Set by ``_on_mount`` after ``super()._on_mount`` (which awaits | ||
| # ``Markdown.update``) returns. Lets the scroll path event-trigger | ||
| # on build completion instead of polling. | ||
| self.build_done: _asyncio.Event = _asyncio.Event() | ||
|
|
||
| @property | ||
| def first_match_block(self) -> MarkdownBlock | None: | ||
| """The first highlighted block in document order, or ``None`` | ||
| when the source has no matches. Set by the highlight-aware | ||
| block subclasses during ``build_from_token``.""" | ||
| return self._first_match_block | ||
|
|
||
| def update(self, markdown): # type: ignore[no-untyped-def, override] | ||
| # Textual's dispatcher walks the MRO and invokes every class's | ||
| # _on_mount — overriding _on_mount and calling super() ran | ||
| # Markdown._on_mount twice; the second pass saw _initial_markdown | ||
| # already consumed and called update("") which removed all | ||
| # blocks. Hook into update() instead: AwaitComplete's future | ||
| # fires when parse+mount completes — set build_done from there. | ||
| aw = super().update(markdown) | ||
| aw._future.add_done_callback(lambda _: self.build_done.set()) # type: ignore[attr-defined] | ||
| return aw |
There was a problem hiding this comment.
Reset per-render match state before each update().
update() currently reuses the previous render's build_done event and first_match_block. On a second FNDMarkdown.update(...), callers can observe the widget as "done" immediately and still see a stale match anchor until the new parse/mount completes.
Suggested fix
class FNDMarkdown(Markdown):
@@
self.render_mermaid: bool = render_mermaid
self._first_match_block: MarkdownBlock | None = None
+ self._build_generation = 0
# Set by ``_on_mount`` after ``super()._on_mount`` (which awaits
# ``Markdown.update``) returns. Lets the scroll path event-trigger
# on build completion instead of polling.
self.build_done: _asyncio.Event = _asyncio.Event()
@@
def update(self, markdown): # type: ignore[no-untyped-def, override]
+ self._build_generation += 1
+ generation = self._build_generation
+ self._first_match_block = None
+ self.build_done.clear()
# Textual's dispatcher walks the MRO and invokes every class's
# _on_mount — overriding _on_mount and calling super() ran
# Markdown._on_mount twice; the second pass saw _initial_markdown
# already consumed and called update("") which removed all
# blocks. Hook into update() instead: AwaitComplete's future
# fires when parse+mount completes — set build_done from there.
aw = super().update(markdown)
- aw._future.add_done_callback(lambda _: self.build_done.set()) # type: ignore[attr-defined]
+ aw._future.add_done_callback( # type: ignore[attr-defined]
+ lambda _: generation == self._build_generation and self.build_done.set()
+ )
return aw🤖 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/widgets/markdown.py` around lines 667 - 689, The update method must
reset per-render state so callers can't observe a stale build_done or
first_match_block; at the start of Markdown.update (the override in this class)
reinitialize self.build_done to a new _asyncio.Event() and set
self._first_match_block = None (so the first_match_block property is cleared)
before calling super().update(markdown), then keep the existing
aw._future.add_done_callback(...) behavior to set the new build_done when the
parse+mount completes.
1 participant
Sixth step of the FNDApp decomposition (stacks on #50).
PrefetchEngine(fnd/tui/preview/prefetch.py) owns the top-N warming pipeline: the decode worker fan-out, the main-thread record sinks, the hidden pre-mount jobs, and the single-consumer sink queue + drainer. The drainer task is still created from the app'son_mount(viaPrefetchEngine.start) so task timing matches the app lifecycle; worker closures keep calling through the app so user-side state always wins, and the exclusive worker grouppreview-prefetchis unchanged.LazyMounter(fnd/tui/preview/lazy_mount.py) owns the scroll-driven mount: the debounced boundary check, batch mounts in both directions (including the settle-then-compensate upward path), and the cancel invoked on file switch / query change.Mount-window tunables stay defined in
app.pyand are read off the app module at call time, preserving the existing monkeypatch point. FNDApp keeps one-line delegators for every entry point the remaining mount machinery uses, plus a read/write property for the in-flight lazy task.Verification: ruff format/check, pyright strict, full suite run twice (1660 passed, 4 skipped, both runs) — test suite byte-unchanged. Perf harnesses re-run: medians within noise of the baseline.