Repository navigation
feat(desktop): add indexed document preview - #117
Conversation
|
The indexed document preview is a useful addition to the desktop application. The implementation is reasonably scoped, and I appreciate the inclusion of search highlighting, match navigation, and regression tests. After reviewing the available changes, I found a few issues that should be addressed before merging. 1. [P1] Prevent stale asynchronous responses from overwriting the current previewFile: The preview request does not currently handle cancellation or stale responses. For example:
The response from A can overwrite B's content, even though B is the currently selected document. A similar issue can occur when the user closes a preview while its request is still pending. Suggested fix: Use an Please also add regression tests covering rapid document switching and closing the dialog during an active request. 2. [P2] Scroll the active search match into viewFile: The Previous and Next buttons update For long documents, the active match may be outside the visible preview area. As a result, clicking Next or Previous can appear to have no effect. Suggested fix: Track the active match element and call Please add a test verifying that navigating to a match outside the visible area brings it into view. 3. [P2] Implement complete keyboard and focus management for the preview dialogFile: The dialog declares In particular:
These behaviors are important for keyboard navigation and accessibility. Suggested fix: Consider using an existing accessible dialog component, if one is available in the project, or implementing the necessary focus and keyboard behavior. Please include keyboard interaction tests. 4. [P2] Avoid excessive processing when searching large documentsFile: The implementation uses For large indexed documents containing many matches, this can produce substantial intermediate allocations and a large number of React elements. Because the search operation runs synchronously, it may also cause noticeable UI delays while typing. Suggested fix: Consider limiting the number of rendered matches, using incremental processing, or introducing virtualization for large documents. At minimum, please add a regression or performance test with a large document containing many repeated matches. 5. [P3] Improve search input focus visibilityFile: The new search input uses This makes keyboard navigation less discoverable. Suggested fix: Add a visible Overall assessmentThe feature is useful and the implementation provides a good foundation. However, I recommend addressing the asynchronous request handling, search navigation, and modal accessibility issues before merging. The large-document performance concern should also be evaluated to ensure that previewing indexed content remains responsive. Once these issues are addressed and the relevant regression tests are added, I would be happy to review the changes again. Thanks for your work on this feature! |
|
Approved — the indexed document preview is a useful, well-scoped addition to the desktop application. The follow-up changes address the earlier review concerns: stale aborted preview requests no longer update the active dialog, search match navigation scrolls the active result into view, and the dialog now includes focus handling, keyboard navigation, Escape-to-close behavior, and focus restoration. The search input also has a visible focus indicator. I also fixed an additional long-document issue discovered during review: indexed chunks intentionally overlap for retrieval, so reconstructing a preview by concatenating chunks duplicated text at every chunk boundary. The original normalized document content is now persisted and used for previewing, with a migration for existing databases. Regression coverage now includes stale preview errors and multi-chunk document fidelity. Targeted frontend, API, and storage tests pass. |
Summary
Verification
Scope
Five files changed. No unrelated hardware changes included.