Skip to content

feat(desktop): add indexed document preview - #117

Merged
petertzy merged 3 commits into
mainfrom
issue-83-document-preview
Oct 31, 2026
Merged

petertzy merged 3 commits into
mainfrom
issue-83-document-preview

Conversation

@snehassneha4578-collab

Copy link
Copy Markdown
Collaborator

Summary

  • Add a read-only preview for indexed documents.
  • Add an API endpoint to retrieve reconstructed document content.
  • Support literal text search, highlighted matches, and previous/next navigation.
  • Add backend and frontend regression tests.

Verification

  • Storage tests: 5 passed
  • API tests: 41 passed
  • Frontend tests: 27 passed
  • Production build: passed
  • Rust formatting and Git whitespace checks: passed

Scope

Five files changed. No unrelated hardware changes included.

@petertzy

Copy link
Copy Markdown
Owner

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 preview

File: apps/desktop/src/main.tsx
Location: openDocumentPreview()

The preview request does not currently handle cancellation or stale responses.

For example:

  1. The user opens document A.
  2. Before A finishes loading, the user opens document B.
  3. Document B finishes loading first.
  4. Document A finishes loading afterward.

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 AbortController or a request-generation identifier to ensure that only the latest active request can update the preview state.

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 view

File: apps/desktop/src/main.tsx
Location: movePreviewMatch()

The Previous and Next buttons update previewMatchIndex, which changes the highlighted match. However, the implementation does not scroll the selected match into view.

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 scrollIntoView() whenever the selected match changes.

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 dialog

File: apps/desktop/src/main.tsx
Location: Document preview dialog

The dialog declares role="dialog" and aria-modal="true", but I could not find corresponding focus management or keyboard handling in the added implementation.

In particular:

  • Focus is not explicitly moved into the dialog when it opens.
  • Keyboard focus is not trapped inside the modal.
  • The Escape key does not appear to close the dialog.
  • Focus is not explicitly restored to the triggering button when the dialog closes.

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 documents

File: apps/desktop/src/main.tsx
Location: previewMatches and preview rendering

The implementation uses matchAll() to collect every occurrence of the search query and then uses split() to reconstruct the highlighted content.

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 visibility

File: apps/desktop/src/styles.css
Location: .documentPreviewSearchRow input

The new search input uses outline: none, but the added styles do not provide an equivalent visible focus indicator.

This makes keyboard navigation less discoverable.

Suggested fix:

Add a visible :focus-visible or :focus-within style that is consistent with the application's existing design system.

Overall assessment

The 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!

@petertzy

Copy link
Copy Markdown
Owner

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.

@petertzy
petertzy merged commit e93081e into main Oct 31, 2026
4 checks passed
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.

3 participants