Skip to content

Refactor IndexedDB document persistence to reuse data structures of desktop persistence#4020

Open
Keavon wants to merge 5 commits intomasterfrom
unify-doc-persistence
Open

Refactor IndexedDB document persistence to reuse data structures of desktop persistence#4020
Keavon wants to merge 5 commits intomasterfrom
unify-doc-persistence

Conversation

@Keavon
Copy link
Copy Markdown
Member

@Keavon Keavon commented Apr 9, 2026

No description provided.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4 issues found across 12 files

Confidence score: 3/5

  • Migration in frontend/src/utility-functions/persistence.ts drops the active document and ignores saved tab order, so users can lose current focus and see tabs reordered after upgrade
  • Restore path in desktop/src/app.rs can reverse document order because of push_front, compounding tab reorder risk
  • Score reflects concrete user-facing state/order regressions rather than minor refactors; issues are fixable but impact is visible
  • Pay close attention to frontend/src/utility-functions/persistence.ts, desktop/src/app.rs - migration and restore ordering behavior.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="desktop/src/persist.rs">

<violation number="1" location="desktop/src/persist.rs:86">
P3: `force_order` only reads `desired_order` by reference, so accepting `&[DocumentId]` instead of `Vec<DocumentId>` avoids unnecessary clones at both call sites.</violation>
</file>

<file name="desktop/src/app.rs">

<violation number="1" location="desktop/src/app.rs:323">
P2: Documents loaded before the current one will be inserted with `to_front = true`, which reverses their order because `load_document` uses `push_front`. This will reorder tabs on restore. Since `all_documents()` is already in persisted order, keep `to_front` false to preserve ordering.</violation>
</file>

<file name="frontend/src/utility-functions/persistence.ts">

<violation number="1" location="frontend/src/utility-functions/persistence.ts:227">
P2: Migration ignores the saved tab order. `oldTabOrder` is checked for existence but its values are never used to order `newDocumentInfos`. Documents end up in arbitrary `Object.values()` enumeration order instead of the user's tab arrangement. Iterate over `oldTabOrder` entries (looking each up in `oldDocuments`) to preserve ordering.</violation>

<violation number="2" location="frontend/src/utility-functions/persistence.ts:261">
P2: Migration drops the user's active document. `current_document_id` is deleted but never read, so `newState.current_document` stays `undefined` after migration. Read it before deleting and carry it forward.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request refactors the document persistence system to use a unified state-based format, replacing the previous separate stores for document tab order and current document ID. It introduces a new 'state' store for metadata and ordering, implements a migration path from the old format, and adds garbage collection for orphaned document content files. Additionally, the desktop-specific persistence logic has been updated to handle file I/O for document content separately from metadata, and the frontend message handling has been consolidated. I have no feedback to provide as the changes are comprehensive and well-structured.

@Keavon Keavon requested a review from timon-schelling April 9, 2026 11:51
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.

1 participant