Skip to content

feat: save and restore reading progress per file#10

Open
pushkarchoudhari wants to merge 1 commit into
vaibhavuk-dev:mainfrom
pushkarchoudhari:feat/save-reading-progress
Open

feat: save and restore reading progress per file#10
pushkarchoudhari wants to merge 1 commit into
vaibhavuk-dev:mainfrom
pushkarchoudhari:feat/save-reading-progress

Conversation

@pushkarchoudhari
Copy link
Copy Markdown
Contributor

Summary

  • Adds a new readingProgress store (src/lib/stores/readingProgress.ts) with save/get/clear/clearAll API, LRU eviction at 500 entries, schema-versioned localStorage persistence, and safe JSON parse
  • Wires debounced scroll save (500ms), restore on tab activation, and flush on beforeunload + visibilitychange in +page.svelte
  • Uses data-source-line anchoring so saved positions survive font/width/reflow changes between sessions
  • Handles edge cases: file-shrunk (closest valid line), tiny-file (skip if fits viewport), top-of-file cleanup, paste:// skip, url:// key normalization, isRestoring guard with scrollend + 1s timeout fallback

Closes #7

Test plan

  • Open a long markdown file, scroll halfway, close and reopen — should smooth-scroll to saved position
  • Open a short file that fits in viewport — no progress should be saved
  • Scroll back to top of a file, reopen — should open at top (no scroll animation)
  • Switch between tabs rapidly — positions should be preserved per tab
  • Open same content via file path and URL — tracked as separate entries
  • Paste markdown via paste:// — no progress tracking
  • Edit mode should not trigger save or restore
  • Truncate a file between sessions — should restore to closest valid line

Implements silent save/restore of scroll position using data-source-line
anchoring. Includes debounced scroll save, visibilitychange + beforeunload
flush, LRU-capped localStorage persistence, and scrollend-based restore
guard with timeout fallback.

Closes vaibhavuk-dev#7
Copy link
Copy Markdown
Owner

@vaibhavuk-dev vaibhavuk-dev left a comment

Choose a reason for hiding this comment

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

Thanks @pushkarchoudhari — nice work on this. The schema versioning, LRU eviction, isRestoring / scrollend guard, and defensive paste:// checks all match what I was hoping to see. A few things to address before this can land:

1. Merge conflict. Branch was opened before #8 merged; both PRs touch the imports + onMount listener wiring in +page.svelte. A rebase against main should sort it.

2. Tab close doesn't save progress. handleCloseTab calls tabStore.closeTab(id) directly with no save first. If a user scrolls and immediately closes within the 500ms debounce window, the position is lost. Could you call saveProgressNow() (or inline equivalent) for the closing tab before tabStore.closeTab(id)?

3. Tab-switch save: timing concern on the DOM read. In the tab-leave block, getCurrentSourceLine("viewer") queries the current article DOM. Depending on $effect ordering, the article HTML may already have been swapped to the new tab's content by the time this runs, which would mean we're computing the line from the new tab's elements at the old tab's scroll position. Two safer options: (a) use the outgoing tab's captured scroll position from the tab store (tabs.ts already calls saveScrollPosition on switch) and convert that to a line, or (b) confirm via logging on your end that the DOM read happens before the swap. Worth verifying either way.

4. File-shrunk comment doesn't match the code. In restoreProgress the comment says "closest valid line ≤ savedLine" but the loop finds the first element with elLine >= savedLine and falls back to the last element. The behavior is actually correct in all cases the design doc enumerates, but the comment will confuse readers — could you reword to something like "find the saved line if it still exists, else fall back to the last element (handles file-shrunk case)"?

Out of scope, will file as a follow-up: the close-on-ESC path (quit_app Tauri command, added in #4) calls AppHandle::exit(0) and bypasses beforeunload, so the final position is lost when the user quits via ESC. Separate from this PR — fix is to saveProgressNow() from the close-on-ESC handler before invoke("quit_app").

CI hasn't run yet — once you push the rebase, I'll approve the workflow. Thanks again for picking this one up!

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.

Save reading progress per file

2 participants