From bab61f33b3504b227a776c53abbd45154000c9e9 Mon Sep 17 00:00:00 2001 From: Pham Thanh Trung Date: Sat, 15 Aug 2026 18:19:51 +0700 Subject: [PATCH] feat: persist viewed files across page reloads Marking a file as viewed collapsed it, but the state lived only in React state, so a browser refresh expanded every file again. It was also lost on any live diff refetch, since the collapse set is rebuilt from scratch whenever the diff object changes. Viewed paths are now stored in localStorage keyed by repo root and ref, capped to the ten most recently used refs so the store can't grow without bound. Each mark is stored alongside a fingerprint of the file's diff. On load the fingerprint is recomputed and the mark is kept only if it still matches, so a file you edited since reviewing it expands again while untouched files stay collapsed. The fingerprint covers file identity, status, and every add/delete line, excluding context lines, hunk headers and line numbers so an unrelated edit elsewhere in the file doesn't unmark a reviewed hunk. Content is trimmed so toggling the hide-whitespace filter doesn't churn marks. Viewed now also takes precedence over a file having comments, which previously force-expanded it. --- packages/ui/src/components/diff/diff-page.tsx | 32 ++- packages/ui/src/lib/viewed-storage.ts | 180 +++++++++++++ packages/ui/tests/viewed-storage.test.ts | 248 ++++++++++++++++++ 3 files changed, 458 insertions(+), 2 deletions(-) create mode 100644 packages/ui/src/lib/viewed-storage.ts create mode 100644 packages/ui/tests/viewed-storage.test.ts diff --git a/packages/ui/src/components/diff/diff-page.tsx b/packages/ui/src/components/diff/diff-page.tsx index 9027ff9..ffacd57 100644 --- a/packages/ui/src/components/diff/diff-page.tsx +++ b/packages/ui/src/components/diff/diff-page.tsx @@ -18,6 +18,13 @@ import { useDiffStaleness } from '../../hooks/use-diff-staleness'; import { type ViewMode, getFilePath, getAutoCollapsedPaths } from '../../lib/diff-utils'; import { buildFirstOpenThreadByFile, buildThreadCountsByFile } from '../../lib/comment-navigation'; import { getHunkHeaders, scrollToElement } from '../../lib/dom-utils'; +import { + fingerprintFiles, + loadViewedFiles, + pickFingerprints, + reconcileViewed, + saveViewedFiles, +} from '../../lib/viewed-storage'; import { fetchGitHubDetails, type GitHubDetails } from '../../lib/api'; import type { LineSelection } from '../comments/types'; import { isThreadResolved } from '../comments/types'; @@ -79,16 +86,27 @@ export function DiffPage() { setPendingSelection(null); }, [commentActions]); + const repoRoot = info?.root ?? null; + const fileFingerprints = useMemo(() => (diff ? fingerprintFiles(diff.files) : {}), [diff]); + useEffect(() => { if (!diff || diff === initializedDiffRef.current) { return; } initializedDiffRef.current = diff; + const restoredViewed = repoRoot + ? reconcileViewed(loadViewedFiles(repoRoot, refParam), fileFingerprints) + : new Set(); + setReviewedFiles(restoredViewed); + const autoCollapsed = getAutoCollapsedPaths(diff.files); for (const path of filesWithComments) { autoCollapsed.delete(path); } + for (const path of restoredViewed) { + autoCollapsed.add(path); + } for (const path of manuallyToggledRef.current) { if (autoCollapsed.has(path)) { autoCollapsed.delete(path); @@ -97,7 +115,14 @@ export function DiffPage() { } } setCollapsedFiles(autoCollapsed); - }, [diff]); + }, [diff, fileFingerprints, repoRoot, refParam]); + + useEffect(() => { + if (!repoRoot || !initializedDiffRef.current) { + return; + } + saveViewedFiles(repoRoot, refParam, pickFingerprints(fileFingerprints, reviewedFiles)); + }, [reviewedFiles, fileFingerprints, repoRoot, refParam]); useEffect(() => { if (filesWithComments.size === 0) { @@ -107,6 +132,9 @@ export function DiffPage() { let changed = false; const next = new Set(prev); for (const path of filesWithComments) { + if (reviewedFiles.has(path)) { + continue; + } if (next.has(path)) { next.delete(path); changed = true; @@ -114,7 +142,7 @@ export function DiffPage() { } return changed ? next : prev; }); - }, [filesWithComments]); + }, [filesWithComments, reviewedFiles]); const handleToggleCollapse = useCallback((path: string) => { const toggled = manuallyToggledRef.current; diff --git a/packages/ui/src/lib/viewed-storage.ts b/packages/ui/src/lib/viewed-storage.ts new file mode 100644 index 0000000..9de43c2 --- /dev/null +++ b/packages/ui/src/lib/viewed-storage.ts @@ -0,0 +1,180 @@ +import type { DiffFile } from '@diffity/parser'; +import { getFilePath } from './diff-utils'; + +const STORAGE_KEY = 'diffity-viewed'; +const STORAGE_VERSION = 1; +const MAX_ENTRIES = 10; + +/** Map of file path -> fingerprint of the file's diff at the time it was marked viewed. */ +export type ViewedFiles = Record; + +interface ViewedEntry { + updatedAt: number; + /** + * Monotonic write counter used to order entries for eviction. Date.now() is too coarse — + * several writes can land in the same millisecond and tie, which makes eviction arbitrary. + */ + seq: number; + files: ViewedFiles; +} + +interface ViewedStore { + version: number; + seq: number; + entries: Record; +} + +function emptyStore(): ViewedStore { + return { version: STORAGE_VERSION, seq: 0, entries: {} }; +} + +export function buildEntryKey(repoRoot: string, ref: string): string { + return `${repoRoot}|${ref}`; +} + +/** + * Fingerprints a file's diff so we can tell whether it changed since it was marked viewed. + * + * Two files with the same fingerprint are treated as "the same change", and a viewed mark + * survives. A different fingerprint drops the mark and the file expands again. + * + * Deliberately excluded: context lines, hunk headers, and line numbers, so an unrelated edit + * elsewhere in the file doesn't unmark a hunk you already reviewed. Content is trimmed so + * toggling the hide-whitespace filter doesn't churn every mark. + */ +export function fingerprintFile(file: DiffFile): string { + const parts: string[] = [file.status, file.oldPath, file.newPath]; + + for (const hunk of file.hunks) { + for (const line of hunk.lines) { + if (line.type === 'context') { + continue; + } + parts.push(`${line.type}:${line.content.trim()}`); + } + } + + return hashString(parts.join('\n')); +} + +/** FNV-1a. Non-cryptographic and synchronous, so restore happens in a single pass. */ +export function hashString(input: string): string { + let hash = 0x811c9dc5; + for (let i = 0; i < input.length; i++) { + hash ^= input.charCodeAt(i); + hash = Math.imul(hash, 0x01000193); + } + return (hash >>> 0).toString(36); +} + +export function fingerprintFiles(files: DiffFile[]): ViewedFiles { + const fingerprints: ViewedFiles = {}; + + for (const file of files) { + fingerprints[getFilePath(file)] = fingerprintFile(file); + } + + return fingerprints; +} + +/** Keeps a stored viewed mark only when the file's fingerprint still matches. */ +export function reconcileViewed( + stored: ViewedFiles, + current: ViewedFiles, +): Set { + const viewed = new Set(); + + for (const [path, fingerprint] of Object.entries(stored)) { + if (current[path] === fingerprint) { + viewed.add(path); + } + } + + return viewed; +} + +/** Narrows a full fingerprint map down to the paths that are currently marked viewed. */ +export function pickFingerprints( + fingerprints: ViewedFiles, + paths: Set, +): ViewedFiles { + const picked: ViewedFiles = {}; + + for (const path of paths) { + if (fingerprints[path] !== undefined) { + picked[path] = fingerprints[path]; + } + } + + return picked; +} + +function readStore(): ViewedStore { + try { + const raw = localStorage.getItem(STORAGE_KEY); + if (!raw) { + return emptyStore(); + } + + const parsed = JSON.parse(raw) as ViewedStore; + if ( + parsed?.version !== STORAGE_VERSION || + typeof parsed.entries !== 'object' + ) { + return emptyStore(); + } + + return { ...parsed, seq: parsed.seq ?? 0 }; + } catch { + return emptyStore(); + } +} + +/** Drops the least recently used entries so the store can't grow without bound. */ +export function evictOldest( + entries: Record, + max = MAX_ENTRIES, +): Record { + const keys = Object.keys(entries); + if (keys.length <= max) { + return entries; + } + + const kept = keys + .sort((a, b) => entries[b].seq - entries[a].seq) + .slice(0, max); + + const next: Record = {}; + for (const key of kept) { + next[key] = entries[key]; + } + + return next; +} + +export function loadViewedFiles(repoRoot: string, ref: string): ViewedFiles { + return readStore().entries[buildEntryKey(repoRoot, ref)]?.files ?? {}; +} + +export function saveViewedFiles( + repoRoot: string, + ref: string, + files: ViewedFiles, +): void { + try { + const store = readStore(); + const key = buildEntryKey(repoRoot, ref); + + if (Object.keys(files).length === 0) { + delete store.entries[key]; + } else { + store.seq += 1; + store.entries[key] = { updatedAt: Date.now(), seq: store.seq, files }; + } + + store.entries = evictOldest(store.entries); + localStorage.setItem(STORAGE_KEY, JSON.stringify(store)); + } catch { + // Storage full, disabled, or unavailable — viewed state just won't persist. + } +} diff --git a/packages/ui/tests/viewed-storage.test.ts b/packages/ui/tests/viewed-storage.test.ts new file mode 100644 index 0000000..89a6162 --- /dev/null +++ b/packages/ui/tests/viewed-storage.test.ts @@ -0,0 +1,248 @@ +import { describe, it, expect, beforeEach } from 'vitest'; +import type { DiffFile, DiffLine } from '@diffity/parser'; +import { + buildEntryKey, + evictOldest, + fingerprintFile, + fingerprintFiles, + hashString, + loadViewedFiles, + reconcileViewed, + saveViewedFiles, +} from '../src/lib/viewed-storage'; + +function makeLine(type: DiffLine['type'], content: string, oldLineNumber: number | null, newLineNumber: number | null): DiffLine { + return { type, content, oldLineNumber, newLineNumber }; +} + +function makeFile(path: string, lines: DiffLine[], status: DiffFile['status'] = 'modified'): DiffFile { + return { + oldPath: status === 'added' ? '' : path, + newPath: status === 'deleted' ? '' : path, + status, + additions: lines.filter(line => line.type === 'add').length, + deletions: lines.filter(line => line.type === 'delete').length, + isBinary: false, + hunks: [ + { + header: '@@ -1,3 +1,3 @@', + oldStart: 1, + oldCount: 3, + newStart: 1, + newCount: 3, + lines, + }, + ], + }; +} + +// The baseline file every fingerprint case is compared against. +function baseFile(path = 'src/app.ts'): DiffFile { + return makeFile(path, [ + makeLine('context', 'const a = 1;', 1, 1), + makeLine('delete', 'const b = 2;', 2, null), + makeLine('add', 'const b = 3;', null, 2), + makeLine('context', 'export { a, b };', 3, 3), + ]); +} + +describe('hashString', () => { + it('is stable for the same input', () => { + expect(hashString('hello')).toBe(hashString('hello')); + }); + + it('differs for different input', () => { + expect(hashString('hello')).not.toBe(hashString('world')); + }); + + it('handles the empty string', () => { + expect(typeof hashString('')).toBe('string'); + }); +}); + +describe('fingerprintFile', () => { + it('is stable across calls for an identical file', () => { + expect(fingerprintFile(baseFile())).toBe(fingerprintFile(baseFile())); + }); + + it('changes when a changed line is edited', () => { + const edited = makeFile('src/app.ts', [ + makeLine('context', 'const a = 1;', 1, 1), + makeLine('delete', 'const b = 2;', 2, null), + makeLine('add', 'const b = 4;', null, 2), + makeLine('context', 'export { a, b };', 3, 3), + ]); + + expect(fingerprintFile(edited)).not.toBe(fingerprintFile(baseFile())); + }); + + it('ignores indentation-only differences so the whitespace filter does not churn marks', () => { + const reindented = makeFile('src/app.ts', [ + makeLine('context', ' const a = 1;', 1, 1), + makeLine('delete', ' const b = 2;', 2, null), + makeLine('add', ' const b = 3;', null, 2), + makeLine('context', ' export { a, b };', 3, 3), + ]); + + expect(fingerprintFile(reindented)).toBe(fingerprintFile(baseFile())); + }); + + it('survives an unrelated edit elsewhere in the file shifting context and line numbers', () => { + const shifted = makeFile('src/app.ts', [ + makeLine('context', 'const zero = 0;', 40, 41), + makeLine('delete', 'const b = 2;', 41, null), + makeLine('add', 'const b = 3;', null, 42), + makeLine('context', 'const later = 9;', 42, 43), + ]); + + expect(fingerprintFile(shifted)).toBe(fingerprintFile(baseFile())); + }); + + it('changes when the file is renamed', () => { + const renamed = baseFile('src/renamed.ts'); + + expect(fingerprintFile(renamed)).not.toBe(fingerprintFile(baseFile())); + }); + + it('changes when the status changes', () => { + const deleted = makeFile('src/app.ts', baseFile().hunks[0].lines, 'deleted'); + + expect(fingerprintFile(deleted)).not.toBe(fingerprintFile(baseFile())); + }); +}); + +describe('fingerprintFiles', () => { + it('keys fingerprints by display path', () => { + const fingerprints = fingerprintFiles([baseFile('src/a.ts'), baseFile('src/b.ts')]); + + expect(Object.keys(fingerprints).sort()).toEqual(['src/a.ts', 'src/b.ts']); + }); +}); + +describe('reconcileViewed', () => { + it('keeps paths whose fingerprint is unchanged', () => { + const viewed = reconcileViewed({ 'src/a.ts': 'abc' }, { 'src/a.ts': 'abc' }); + + expect(viewed.has('src/a.ts')).toBe(true); + }); + + it('drops paths whose fingerprint changed', () => { + const viewed = reconcileViewed({ 'src/a.ts': 'abc' }, { 'src/a.ts': 'xyz' }); + + expect(viewed.has('src/a.ts')).toBe(false); + }); + + it('drops paths that are no longer in the diff', () => { + const viewed = reconcileViewed({ 'src/gone.ts': 'abc' }, { 'src/a.ts': 'abc' }); + + expect(viewed.has('src/gone.ts')).toBe(false); + }); + + it('keeps unchanged files while dropping the one that was edited', () => { + const viewed = reconcileViewed( + { 'src/a.ts': 'abc', 'src/b.ts': 'def' }, + { 'src/a.ts': 'abc', 'src/b.ts': 'CHANGED' }, + ); + + expect([...viewed]).toEqual(['src/a.ts']); + }); +}); + +describe('evictOldest', () => { + it('leaves the store alone when under the cap', () => { + const entries = { + a: { updatedAt: 1, seq: 1, files: {} }, + b: { updatedAt: 2, seq: 2, files: {} }, + }; + + expect(Object.keys(evictOldest(entries, 10))).toHaveLength(2); + }); + + it('drops the least recently used entries past the cap', () => { + const entries = { + oldest: { updatedAt: 1, seq: 1, files: {} }, + middle: { updatedAt: 2, seq: 2, files: {} }, + newest: { updatedAt: 3, seq: 3, files: {} }, + }; + + expect(Object.keys(evictOldest(entries, 2)).sort()).toEqual(['middle', 'newest']); + }); + + it('orders by write sequence, not wall clock, so same-millisecond writes still evict correctly', () => { + const entries = { + oldest: { updatedAt: 1000, seq: 1, files: {} }, + middle: { updatedAt: 1000, seq: 2, files: {} }, + newest: { updatedAt: 1000, seq: 3, files: {} }, + }; + + expect(Object.keys(evictOldest(entries, 2)).sort()).toEqual(['middle', 'newest']); + }); +}); + +describe('localStorage round-trip', () => { + beforeEach(() => { + const store = new Map(); + // vitest runs these in the node environment, so stand up a minimal localStorage. + (globalThis as { localStorage?: unknown }).localStorage = { + getItem: (key: string) => store.get(key) ?? null, + setItem: (key: string, value: string) => void store.set(key, value), + removeItem: (key: string) => void store.delete(key), + clear: () => store.clear(), + }; + }); + + it('returns an empty map when nothing is stored', () => { + expect(loadViewedFiles('/repo', 'work')).toEqual({}); + }); + + it('round-trips viewed files for a repo and ref', () => { + saveViewedFiles('/repo', 'work', { 'src/a.ts': 'abc' }); + + expect(loadViewedFiles('/repo', 'work')).toEqual({ 'src/a.ts': 'abc' }); + }); + + it('keeps refs in the same repo isolated', () => { + saveViewedFiles('/repo', 'work', { 'src/a.ts': 'abc' }); + saveViewedFiles('/repo', 'main', { 'src/b.ts': 'def' }); + + expect(loadViewedFiles('/repo', 'work')).toEqual({ 'src/a.ts': 'abc' }); + expect(loadViewedFiles('/repo', 'main')).toEqual({ 'src/b.ts': 'def' }); + }); + + it('keeps repos isolated so a recycled port cannot leak state', () => { + saveViewedFiles('/repo-one', 'work', { 'src/a.ts': 'abc' }); + + expect(loadViewedFiles('/repo-two', 'work')).toEqual({}); + }); + + it('clears the entry when the last file is unmarked', () => { + saveViewedFiles('/repo', 'work', { 'src/a.ts': 'abc' }); + saveViewedFiles('/repo', 'work', {}); + + expect(loadViewedFiles('/repo', 'work')).toEqual({}); + }); + + it('evicts the oldest ref once more than ten are stored', () => { + for (let i = 0; i < 11; i++) { + saveViewedFiles('/repo', `ref-${i}`, { 'src/a.ts': `fp-${i}` }); + } + + expect(loadViewedFiles('/repo', 'ref-0')).toEqual({}); + expect(loadViewedFiles('/repo', 'ref-10')).toEqual({ 'src/a.ts': 'fp-10' }); + }); + + it('recovers from corrupt stored JSON instead of throwing', () => { + localStorage.setItem('diffity-viewed', '{not json'); + + expect(loadViewedFiles('/repo', 'work')).toEqual({}); + }); + + it('ignores a store written by a future version', () => { + localStorage.setItem('diffity-viewed', JSON.stringify({ + version: 99, + entries: { [buildEntryKey('/repo', 'work')]: { updatedAt: 1, files: { 'src/a.ts': 'abc' } } }, + })); + + expect(loadViewedFiles('/repo', 'work')).toEqual({}); + }); +});