From 805dd88a4552d5ddef8310319ffdbe5dd63cf9d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ant=C3=98n=20Ustin=C3=98ff?= Date: Mon, 26 Oct 2026 13:18:03 +0400 Subject: [PATCH 1/2] Add Viewed and next-unviewed file navigation --- CHANGELOG.md | 1 + README.md | 2 + docs/ARCHITECTURE.md | 1 + src/content/sidebar.scss | 76 ++++++++++++++++++++++--- src/content/sidebar/render.js | 1 + src/content/sidebar/tree.js | 37 ++---------- src/content/state.js | 2 + src/content/viewed.js | 94 +++++++++++++++++++++++++++++++ src/content/viewer/file-review.js | 92 ++++++++++++++++++++++++++++++ src/content/viewer/file-search.js | 12 ++-- src/content/viewer/file-tabs.js | 46 ++++++++++++--- src/content/viewer/viewer.js | 20 ++++++- 12 files changed, 329 insertions(+), 55 deletions(-) create mode 100644 src/content/viewed.js create mode 100644 src/content/viewer/file-review.js diff --git a/CHANGELOG.md b/CHANGELOG.md index d93148d..219e36b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,7 @@ - **ADDED**: Unified / Side-by-side full-file diffs in file tabs and View full, with synchronized panes and preserved source-line positions - **ADDED**: Ctrl/Cmd+F literal search in open files and full-file diffs, with match highlights, counts and previous/next navigation across virtualized rows - **ADDED**: previous/next change-block navigation with a scroll-aware counter in file tabs and View full, for Unified and Side-by-side diffs +- **ADDED**: Viewed & next in PR/MR file tabs and View full saves the current mark before opening the next listed unviewed file - **CHANGED**: switching branches from a file page preserves its resolved file path instead of opening the branch root - **CHANGED**: enable GitHub device-flow and GitLab PKCE sign-in with Codetree's registered public client IDs - **CHANGED**: minimum Chrome version to 140 for trusted-only local and Sync storage access diff --git a/README.md b/README.md index 0a0ccc6..be0eff8 100644 --- a/README.md +++ b/README.md @@ -63,6 +63,8 @@ Use **Ctrl/Cmd+F** or the viewer's search button to find literal text without ca In diffs, the **Changes** counter and its arrow buttons cycle through blocks of additions/removals separated by unchanged lines. They work in file tabs and **View full**, in both layouts, revealing off-screen blocks without reloading the file. The counter follows scrolling and keeps the selected block current while it remains visible. Diffs without changed lines show `0 / 0` with disabled arrows. +In PR/MR file tabs and **View full**, **Viewed & next** saves the current file's Viewed mark, then opens the next unviewed file in the same viewer. It follows the full listed changed-file order, wraps to earlier files and ignores sidebar search and **Only unviewed**. An already viewed file offers **Next unviewed** without another write. Failed marks keep the file open; failed navigation keeps a successful mark and offers a retry. When all listed files are viewed, the current file stays open with a completion message. GitHub uses its available Viewed mode; GitLab marks remain local. + Workspace metadata belongs to the current browser tab, selected account, host and repository. It resets on extension/browser restart and is excluded from Sync. Limits are 32 open files, 256 saved positions and 50 workspaces, with 64 KiB per entry; file text is never stored with this metadata. In-page tabs are new beta behavior; installed-browser smoke verification is pending. | Shortcut | Action | diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 65fb8f9..0c3789e 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -77,6 +77,7 @@ Repository requests are dispatched to `providers/github.js` or `providers/gitlab | `navigation.js` | Page loads, header, tabs, Refresh. | | `sidebar/view.js`, `layout.js`, `render.js` | Shadow DOM elements, placement and page padding, tab rendering. | | `sidebar/files.js`, `tree.js` | Tree loading and lazy folders; virtualized rows, keyboard navigation, Viewed marks. | +| `viewed.js`, `viewer/file-review.js` | Revision-bound Viewed publication and owned PR/MR Viewed-and-next controls shared by the tree, file tabs and View full. | | `sidebar/file-icons.js` | file-icons rule matching and glyph rendering. | | `sidebar/branches.js`, `pulls.js`, `bookmarks.js` | Branch popover, request list and filters, bookmarks. | | `viewer/` | Full-file diff and comments dialog; lexical highlighting. | diff --git a/src/content/sidebar.scss b/src/content/sidebar.scss index 552bd2e..d482595 100644 --- a/src/content/sidebar.scss +++ b/src/content/sidebar.scss @@ -906,11 +906,11 @@ button:disabled { flex-direction: column; } .viewer-header { - padding: 13px 16px; + padding: 12px 16px; border-bottom: 1px solid var(--border); display: flex; flex-wrap: wrap; - gap: 10px; + gap: 12px; align-items: center; } .viewer-heading { @@ -928,18 +928,53 @@ button:disabled { display: block; font-size: 11px; color: var(--muted); - margin-top: 5px; + margin-top: 4px; } .viewer-actions { display: flex; flex-wrap: wrap; align-items: center; - gap: 5px; + justify-content: flex-end; + gap: 8px; + margin-left: auto; + min-width: 0; max-width: 100%; } +.viewer-tools { + display: flex; + align-items: center; + gap: 8px; + min-width: 0; + max-width: 100%; +} +.viewer-actions > .icon-button, +.viewer-tools > .icon-button, +.viewer-find > .icon-button { + width: 30px; + height: 30px; + flex-shrink: 0; +} +.viewer-actions > .icon-button, +.viewer-tools > .icon-button { + margin: 0; + padding: 0; +} .viewer-actions a { + height: 30px; + margin: 0; + padding: 0 8px; + border-radius: 6px; + background: var(--muted-bg); font-size: 11px; - margin-right: 9px; + font-weight: 600; + text-decoration: none; + white-space: nowrap; +} +.viewer-actions a.viewer-native-accent { + color: var(--accent); +} +.viewer-actions a:hover { + background: var(--hover); } .diff-layout { display: inline-flex; @@ -1005,7 +1040,6 @@ button:disabled { } .viewer-find { display: flex; - flex-wrap: wrap; align-items: center; gap: 4px; max-width: 100%; @@ -1203,7 +1237,7 @@ button:disabled { word-break: break-word; } .viewer-bottom { - padding: 7px 14px; + padding: 7px 16px; border-top: 1px solid var(--border); font-size: 11px; color: var(--muted); @@ -1214,6 +1248,34 @@ button:disabled { .viewer-bottom .flex { flex: 1; } +.viewer-bottom:has(> .file-review) { + flex-wrap: wrap; +} +.file-review { + display: flex; + flex-wrap: wrap; + align-items: center; + gap: 8px; + min-width: 0; + margin-left: auto; +} +.file-review[hidden] { + display: none; +} +.file-review .small-button { + margin: 0; + flex-shrink: 0; +} +.file-review-status { + max-width: 36ch; + overflow-wrap: anywhere; +} +.file-review-status:empty { + display: none; +} +.file-review-status.error { + color: var(--red); +} .file-workspace { margin: 16px; background: var(--bg); diff --git a/src/content/sidebar/render.js b/src/content/sidebar/render.js index 5917b09..2b22045 100644 --- a/src/content/sidebar/render.js +++ b/src/content/sidebar/render.js @@ -227,6 +227,7 @@ export function createRender(app) { }), ); if (state.mode === 'changes' && state.diff) { + void state.viewedVersion; if (state.diff.viewedMode === 'local') notice.append(document.createTextNode('Viewed marks are local to this browser. ')); for (const warning of state.diff.warnings) notice.append(el('div', {text: warning})); diff --git a/src/content/sidebar/tree.js b/src/content/sidebar/tree.js index 9deef71..6b30c67 100644 --- a/src/content/sidebar/tree.js +++ b/src/content/sidebar/tree.js @@ -15,8 +15,9 @@ import {blobURL, pullURL, repoURL} from '../../shared/routes.js'; import {fileKind, flatten} from '../../shared/tree.js'; import {button, el, highlight} from '../dom.js'; import {fileIconElement, matchIcon} from './file-icons.js'; -import {batch, createMemo, createRenderEffect, createSignal, on, onCleanup, withOwner} from '../reactive.js'; +import {createMemo, createRenderEffect, createSignal, on, onCleanup, withOwner} from '../reactive.js'; import {requestFrame} from '../reactive-dom.js'; +import {createViewedAction} from '../viewed.js'; /** Fixed tree-row height in pixels; virtualization depends on it. */ const rowHeight = 29; @@ -155,11 +156,7 @@ export function createTree(app) { ); row.append(full); if (state.diff?.viewedMode !== 'none') { - const context = state.context; - const headSHA = state.diff.head.sha; - const epoch = state.epoch; - const generation = state.filesGeneration; - const alive = app.pageAlive || (() => true); + const action = createViewedAction(app); const checkbox = el('input', { type: 'checkbox', class: 'viewed-checkbox', @@ -174,33 +171,9 @@ export function createTree(app) { const viewed = checkbox.checked; checkbox.disabled = true; try { - const result = await app.rpc('VIEWED', {context, headSHA, path: node.path, viewed}); - if ( - !alive() || - epoch !== state.epoch || - generation !== state.filesGeneration || - state.diff?.head.sha !== headSHA - ) - return; - batch(() => { - node.viewed = result.state === 'VIEWED'; - // Publish file marks for progress/count bindings; pending or failed writes never reach them. - state.entries = state.entries.map(entry => - entry.path === node.path ? {...entry, viewed: node.viewed} : entry, - ); - // Only ancestors can change; search and collapsed rows do not affect the full tree. - let path = node.path; - state.tree.nodes.get(path).viewed = node.viewed; - while (path.includes('/')) { - path = path.slice(0, path.lastIndexOf('/')); - const parent = state.tree.nodes.get(path); - parent.viewed = - parent.loaded && parent.children.length > 0 && parent.children.every(child => child.viewed); - } - updateTree(); - }); + await action.set(node.path, viewed); } catch (error) { - checkbox.checked = Boolean(node.viewed); + checkbox.checked = action.viewed(node.path); throw error; } finally { checkbox.disabled = false; diff --git a/src/content/state.js b/src/content/state.js index a967e9d..a1958c7 100644 --- a/src/content/state.js +++ b/src/content/state.js @@ -44,6 +44,7 @@ const viewFields = [ 'viewError', 'diff', 'diffLayout', + 'viewedVersion', 'pulls', 'loadingAll', 'selected', @@ -85,6 +86,7 @@ export function createState() { viewError: '', diff: null, diffLayout: 'unified', + viewedVersion: 0, branches: null, pulls: [], loadingAll: false, diff --git a/src/content/viewed.js b/src/content/viewed.js new file mode 100644 index 0000000..cb468fa --- /dev/null +++ b/src/content/viewed.js @@ -0,0 +1,94 @@ +/* + * https://github.com/ziqq/Codetree + * Copyright (C) 2026 Anton Ustinoff + * https://github.com/ziqq/Codetree/blob/main/LICENSE + * + * Revision-bound Viewed writes shared by the changed-file tree and file viewers. + */ + +import {batch} from './reactive.js'; + +/** Pending writes reject duplicate updates from controls sharing one loaded diff. */ +const pendingPaths = new WeakMap(); + +/** Captures a loaded review so delayed controls cannot mark a different page or revision. */ +export function createViewedAction(app) { + const {state} = app; + const context = state.context; + const diff = state.diff; + const headSHA = diff?.head?.sha; + const baseSHA = diff?.base?.sha; + const epoch = state.epoch; + const generation = state.filesGeneration; + const alive = app.pageAlive || (() => true); + + /** Requires the captured page, loaded diff and exact source revisions to remain current. */ + function current() { + return Boolean( + alive() && + context && + state.context === context && + state.diff === diff && + headSHA && + diff.head?.sha === headSHA && + diff.base?.sha === baseSHA && + ['github', 'local'].includes(diff.viewedMode) && + epoch === state.epoch && + generation === state.filesGeneration, + ); + } + + /** Reads a confirmed mark and subscribes viewer controls to marks published elsewhere. */ + function viewed(path) { + void state.viewedVersion; + return diff?.viewed?.[path] === 'VIEWED'; + } + + /** Confirms one listed file's mark before publishing it to viewer and sidebar bindings. */ + async function set(path, value) { + if (!current()) return false; + if (typeof value !== 'boolean' || !diff.files.some(file => file.filename === path)) + throw new Error('This file is not part of the loaded review.'); + let pending = pendingPaths.get(diff); + if (!pending) { + pending = new Set(); + pendingPaths.set(diff, pending); + } + if (pending.has(path)) throw new Error('This file already has a Viewed update in progress.'); + pending.add(path); + try { + const result = await app.rpc('VIEWED', {context, headSHA, path, viewed: value}); + if (!current()) return false; + const expected = value ? 'VIEWED' : 'UNVIEWED'; + if (result?.state !== expected || (result.mode !== undefined && !['github', 'local'].includes(result.mode))) + throw new Error('The Viewed update was not confirmed. Try again.'); + batch(() => { + diff.viewed = {...diff.viewed, [path]: expected}; + if (result.mode !== undefined) diff.viewedMode = result.mode; + if (state.mode === 'changes') { + state.entries = state.entries.map(entry => (entry.path === path ? {...entry, viewed: value} : entry)); + const node = state.tree.nodes.get(path); + if (node) { + node.viewed = value; + let parentPath = path; + while (parentPath.includes('/')) { + parentPath = parentPath.slice(0, parentPath.lastIndexOf('/')); + const parent = state.tree.nodes.get(parentPath); + if (!parent) break; + parent.viewed = + parent.loaded && parent.children.length > 0 && parent.children.every(child => child.viewed); + } + } + app.updateTree?.(); + } + // Keep the diff identity: replacing it would reload the current file and discard its position. + state.viewedVersion++; + }); + return true; + } finally { + pending.delete(path); + } + } + + return {current, viewed, set}; +} diff --git a/src/content/viewer/file-review.js b/src/content/viewer/file-review.js new file mode 100644 index 0000000..93e7773 --- /dev/null +++ b/src/content/viewer/file-review.js @@ -0,0 +1,92 @@ +/* + * https://github.com/ziqq/Codetree + * Copyright (C) 2026 Anton Ustinoff + * https://github.com/ziqq/Codetree/blob/main/LICENSE + * + * Owned Viewed-and-next control shared by file tabs and the full-file dialog. + */ + +import {icon} from '../../shared/icons.js'; +import {el} from '../dom.js'; +import {createAlive, createRenderEffect, createSignal, onCleanup, withOwner} from '../reactive.js'; +import {listen} from '../reactive-dom.js'; +import {createViewedAction} from '../viewed.js'; + +/** + * Adds a review action for one loaded PR/MR file. Navigation follows the full + * listed file order, wrapping once, and waits for a confirmed Viewed write. + * Closing or replacing the viewer never cancels the write; it only prevents + * that viewer from navigating when the reply arrives. + * + * @param {object} app The content feature context. + * @param {{bottom: Element, owner: object}} shell The owned viewer footer. + * @param {{path: string}} node The current changed file. + * @param {object} diff The captured PR/MR diff and its listed files. + * @param {() => boolean} current Whether this render still owns the viewer. + * @param {(path: string) => Promise} open Opens another file in the same viewer. + * @param {() => number} navigation Reads navigation intent before metadata saves finish. + */ +export function createFileReview(app, shell, node, diff, current, open, navigation = () => 0) { + if (app.state.context?.kind !== 'pull' || app.state.diff !== diff) return; + const action = createViewedAction(app); + const index = diff.files.findIndex(file => file.filename === node.path); + if (index === -1 || !action.current()) return; + const alive = createAlive(); + const [pending, setPending] = createSignal(false); + const [failure, setFailure] = createSignal(''); + const label = el('span'); + const control = el('button', {type: 'button', class: 'small-button'}, [icon('check'), label]); + const status = el('span', {class: 'file-review-status', role: 'status', 'aria-live': 'polite'}); + const group = el('div', {class: 'file-review', role: 'group', 'aria-label': 'File review'}, [control, status]); + const openFile = withOwner(open); + shell.bottom.append(group); + onCleanup(() => group.remove()); + + /** Whether this control may still act on its captured page and viewer. */ + function available() { + return alive() && current() && action.current(); + } + + /** Finds the next unviewed listed file without consulting sidebar filters. */ + function nextUnviewed() { + for (let offset = 1; offset < diff.files.length; offset++) { + const path = diff.files[(index + offset) % diff.files.length].filename; + if (!action.viewed(path)) return path; + } + return null; + } + + /** Saves Viewed once, then advances only while this viewer is still current. */ + async function advance() { + if (!available() || pending()) return; + const request = navigation(); + setFailure(''); + setPending(true); + try { + if (!action.viewed(node.path) && !(await action.set(node.path, true))) return; + if (!available() || request !== navigation()) return; + const path = nextUnviewed(); + if (path) await openFile(path); + } catch (error) { + if (available()) setFailure(error?.message || 'Could not save Viewed. Try again.'); + } finally { + if (alive()) setPending(false); + } + } + + listen(control, 'click', advance); + createRenderEffect(() => { + const supported = available(); + group.hidden = !supported; + if (!supported) { + control.disabled = true; + return; + } + const viewed = action.viewed(node.path); + const complete = viewed && !nextUnviewed(); + control.disabled = pending() || complete; + label.textContent = pending() ? 'Saving…' : viewed ? 'Next unviewed' : 'Viewed & next'; + status.textContent = failure() || (complete ? 'All listed files viewed' : ''); + status.classList.toggle('error', Boolean(failure())); + }); +} diff --git a/src/content/viewer/file-search.js b/src/content/viewer/file-search.js index 5cffb5b..92b9a76 100644 --- a/src/content/viewer/file-search.js +++ b/src/content/viewer/file-search.js @@ -44,8 +44,13 @@ export function createFileSearch(app, shell, rows, viewport, current) { const close = button('close', 'Close file search', closeSearch); const field = el('div', {class: 'searchbar viewer-find-field'}, [icon('search'), input, count]); const bar = el('div', {class: 'viewer-find', role: 'search', hidden: ''}, [field, previous, next, close]); - const toggle = button('search', 'Find in file (Ctrl/Cmd+F)', openSearch, 'viewer-find-toggle'); - shell.actions.append(toggle, bar); + const toggle = button('search', 'Find in file (Ctrl/Cmd+F)', openSearch, 'viewer-find-toggle small-button'); + const closeFocused = shell.closeButton?.matches(':focus'); + const tools = el('div', {class: 'viewer-tools'}, [toggle, bar]); + // Wrap find and Close together, retaining DOM/tab order and the dialog's initial focus. + if (shell.closeButton) tools.append(shell.closeButton); + shell.actions.append(tools); + if (closeFocused) shell.closeButton.focus(); shell.body.setAttribute('tabindex', '0'); let ranges = new Map(); let matches = []; @@ -203,8 +208,7 @@ export function createFileSearch(app, shell, rows, viewport, current) { onCleanup(() => { live = false; reset(); - bar.remove(); - toggle.remove(); + tools.remove(); }); update(); return {ranges: row => ranges.get(row) || [], active: () => matches[active]}; diff --git a/src/content/viewer/file-tabs.js b/src/content/viewer/file-tabs.js index b12bd21..33cc92a 100644 --- a/src/content/viewer/file-tabs.js +++ b/src/content/viewer/file-tabs.js @@ -17,6 +17,7 @@ import {createRoot, getOwner, onCleanup, runWithOwner, withOwner} from '../react import {listen} from '../reactive-dom.js'; import {diffNode} from './viewer.js'; import {renderDiffFile, renderTextFile} from './file-content.js'; +import {createFileReview} from './file-review.js'; /** Creates the inline workspace and its navigation, persistence and theme actions. */ export function createFileTabs(app) { @@ -27,7 +28,7 @@ export function createFileTabs(app) { const tabs = el('div', {class: 'file-tabs', role: 'tablist', 'aria-label': 'Open files'}); const heading = el('strong'); const revision = el('small'); - const native = el('a', {target: '_blank', rel: 'noopener noreferrer'}); + const native = el('a', {class: 'small-button', target: '_blank', rel: 'noopener noreferrer'}); const actions = el('div', {class: 'viewer-actions'}, [native]); const frame = el('section', {class: 'file-workspace', 'aria-label': 'Codetree file workspace'}, [ tabs, @@ -41,6 +42,7 @@ export function createFileTabs(app) { let shell; let disposeContent; let generation = 0; + let opening = 0; let contentReady = false; let waitingForMount = false; let saveTimer; @@ -247,16 +249,29 @@ export function createFileTabs(app) { if (epoch !== state.epoch) return; if (!snapshot || !state.info) throw new Error('Wait for the repository to finish loading.'); const currentSnapshot = snapshot; + const request = ++opening; + const render = generation; capturePosition(); - if (!snapshot.value.paths.includes(node.path)) { - if (snapshot.value.paths.length >= maxFileTabs) - throw new Error(`Close a file tab before opening another (maximum ${maxFileTabs}).`); - snapshot.value.paths.push(node.path); - } - snapshot.value.active = node.path; + const previous = snapshot.value; + const paths = previous.paths; + const existing = paths.includes(node.path); + if (!existing && paths.length >= maxFileTabs) + throw new Error(`Close a file tab before opening another (maximum ${maxFileTabs}).`); + const value = {...previous, paths: existing ? paths : [...paths, node.path], active: node.path}; + clearTimeout(saveTimer); + // Keep the rendered tab current if saving fails, so its review action can report and retry navigation. + await app.rpc('FILE_TABS', {...currentSnapshot, value: structuredClone(value)}); + if ( + snapshot !== currentSnapshot || + epoch !== state.epoch || + request !== opening || + render !== generation || + snapshot.value !== previous || + previous.paths !== paths + ) + return; + snapshot.value = {...value, positions: previous.positions}; state.selected = node.path; - await rememberFileTabs(); - if (snapshot !== currentSnapshot || epoch !== state.epoch || snapshot.value.active !== node.path) return; renderTabs(); await renderActiveFile(); } @@ -370,6 +385,7 @@ export function createFileTabs(app) { ? `${diff.base.sha?.slice(0, 7) || 'empty'} → ${diff.head.sha.slice(0, 7)} · full-file diff` : `${info.ref} · ${info.commitSHA?.slice(0, 7) || 'empty'} · read only`; native.textContent = `View on ${providerName(context)}`; + native.classList.toggle('viewer-native-accent', Boolean(diff) && context.provider === 'github'); native.removeAttribute('href'); shell.body.replaceChildren(empty('Loading file', 'Loading this file at the selected revision…', 'refresh')); let styles; @@ -396,6 +412,18 @@ export function createFileTabs(app) { await renderTextFile(app, shell, path, text, current); } if (!current()) return; + if (file) + runWithOwner(shell.owner, () => + createFileReview( + app, + shell, + diffNode(file), + diff, + current, + path => openFileTab({path}), + () => opening, + ), + ); unavailable.delete(path); contentReady = true; const position = snapshot.value.positions.find( diff --git a/src/content/viewer/viewer.js b/src/content/viewer/viewer.js index ac0496d..bb08325 100644 --- a/src/content/viewer/viewer.js +++ b/src/content/viewer/viewer.js @@ -12,6 +12,7 @@ import {button, el, empty} from '../dom.js'; import {providerName} from '../page.js'; import {renderDiffFile} from './file-content.js'; +import {createFileReview} from './file-review.js'; import {createAlive, createRoot, getOwner, onCleanup, runWithOwner} from '../reactive.js'; import {listen} from '../reactive-dom.js'; @@ -51,7 +52,7 @@ export function createViewer(app) { /** * Opens the dialog with an empty layout for a new view. * - * @returns {{actions: Element, body: Element, bottom: Element, generation: number}} + * @returns {{actions: Element, closeButton: HTMLButtonElement, body: Element, bottom: Element, generation: number}} */ function viewerShell(title, subtitle, label = 'Full-file diff') { disposeViewer?.(); @@ -64,7 +65,8 @@ export function createViewer(app) { el('strong', {text: title}), el('small', {text: subtitle}), ]); - const actions = el('div', {class: 'viewer-actions'}, [button('close', 'Close viewer', () => viewer.close())]); + const closeButton = button('close', 'Close viewer', () => viewer.close(), 'small-button'); + const actions = el('div', {class: 'viewer-actions'}, [closeButton]); const viewerBody = el('div', {class: 'viewer-body'}); const bottom = el('div', {class: 'viewer-bottom'}); viewer.replaceChildren( @@ -75,7 +77,7 @@ export function createViewer(app) { ]), ); if (!viewer.open) viewer.showModal(); - return {actions, body: viewerBody, bottom, generation, owner: getOwner(), alive: createAlive()}; + return {actions, closeButton, body: viewerBody, bottom, generation, owner: getOwner(), alive: createAlive()}; }), ); } @@ -102,6 +104,7 @@ export function createViewer(app) { if (!isCurrent(shell)) return; shell.actions.prepend( el('a', { + class: `small-button${state.context.provider === 'github' ? ' viewer-native-accent' : ''}`, href: originalURL, text: `${providerName(state.context)} diff`, target: '_blank', @@ -131,6 +134,17 @@ export function createViewer(app) { ); try { await renderDiffFile(app, shell, node, diff, () => isCurrent(shell)); + if (isCurrent(shell)) + runWithOwner(shell.owner, () => + createFileReview( + app, + shell, + node, + diff, + () => isCurrent(shell), + path => showDiff(diffNode(diff.files.find(file => file.filename === path))), + ), + ); } catch (error) { if (isCurrent(shell)) shell.body.replaceChildren(empty('Text preview unavailable', error.message, 'diff')); } From 64a4c591e87e7a1d8672d1bfc6cdd95855f9cc19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ant=C3=98n=20Ustin=C3=98ff?= Date: Mon, 26 Oct 2026 13:37:14 +0400 Subject: [PATCH 2/2] Test Viewed-and-next navigation and fix pending-view races --- docs/ARCHITECTURE.md | 3 +- src/content/viewer/file-tabs.js | 42 ++-- src/content/viewer/viewer.js | 2 +- tests/file-tabs.test.mjs | 281 ++++++++++++++++++++++- tests/sidebar-reactive.test.mjs | 84 +++++++ tests/viewer-review.test.mjs | 383 ++++++++++++++++++++++++++++++++ 6 files changed, 773 insertions(+), 22 deletions(-) create mode 100644 tests/viewer-review.test.mjs diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 0c3789e..177f57f 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -117,7 +117,8 @@ See [Checks and releases](RELEASING.md) for CI and publication. - `split-diff.test.mjs`: View full layout controls, aligned old/new rows, synchronized/clamped scrolling, source-line positions, font/viewport changes and disposal through the production viewer with synthetic geometry. - `viewer-search.test.mjs`: file-search controls and shortcuts, literal/Unicode ranges across syntax tokens, complete virtualized-file navigation, result limits and stale-scan/disposal checks through the production viewer. - `viewer-changes.test.mjs`: change-block counts, cyclic navigation, virtual row reveal, manual scrolling, split projection, clamped positions and owned controls through the production View full dialog. -- `file-tabs.test.mjs`: file navigation and session restoration, including Split diff positions and change-block counters across file/review switches, Refresh and page reload, plus independent inline/dialog scrolling and navigation with a shared layout. +- `viewer-review.test.mjs`: Viewed-and-next acknowledgement ordering, listed-file traversal, completion, retries and stale-view disposal through View full in both layouts and provider modes. +- `file-tabs.test.mjs`: file navigation and session restoration, including Split diff positions and change-block counters across file/review switches, Refresh and page reload, plus independent inline/dialog scrolling and navigation with a shared layout. Viewed-and-next coverage checks confirmed marks, metadata-save retries, manual navigation intent and stale replies. - `hosts.test.mjs`: request contexts built like the content script's, and rejected branch/path segments. - `hide-rules.test.mjs`: regex and wildcard URL exclusions, validation, persisted rules, URL query/fragment boundaries and code-only scope. - `viewed.test.mjs`: Viewed marks through the bundled service worker in a VM with mocked APIs. diff --git a/src/content/viewer/file-tabs.js b/src/content/viewer/file-tabs.js index 33cc92a..a6e0c74 100644 --- a/src/content/viewer/file-tabs.js +++ b/src/content/viewer/file-tabs.js @@ -43,6 +43,7 @@ export function createFileTabs(app) { let disposeContent; let generation = 0; let opening = 0; + let pendingOpen; let contentReady = false; let waitingForMount = false; let saveTimer; @@ -130,8 +131,11 @@ export function createFileTabs(app) { async function rememberFileTabs() { clearTimeout(saveTimer); capturePosition(); - if (!snapshot) return; - const {context, scope, accountID, value} = snapshot; + const saved = snapshot; + if (!saved) return; + // A scroll save must follow an in-flight open's publication, not overwrite it with the old active path. + while (pendingOpen && snapshot === saved) await pendingOpen; + const {context, scope, accountID, value} = saved; await app.rpc('FILE_TABS', {context, scope, accountID, value: structuredClone(value)}); } @@ -259,18 +263,28 @@ export function createFileTabs(app) { throw new Error(`Close a file tab before opening another (maximum ${maxFileTabs}).`); const value = {...previous, paths: existing ? paths : [...paths, node.path], active: node.path}; clearTimeout(saveTimer); - // Keep the rendered tab current if saving fails, so its review action can report and retry navigation. - await app.rpc('FILE_TABS', {...currentSnapshot, value: structuredClone(value)}); - if ( - snapshot !== currentSnapshot || - epoch !== state.epoch || - request !== opening || - render !== generation || - snapshot.value !== previous || - previous.paths !== paths - ) - return; - snapshot.value = {...value, positions: previous.positions}; + let completeOpen; + const pending = new Promise(resolve => { + completeOpen = resolve; + }); + pendingOpen = pending; + try { + // Keep the rendered tab current if saving fails, so its review action can report and retry navigation. + await app.rpc('FILE_TABS', {...currentSnapshot, value: structuredClone(value)}); + if ( + snapshot !== currentSnapshot || + epoch !== state.epoch || + request !== opening || + render !== generation || + snapshot.value !== previous || + previous.paths !== paths + ) + return; + snapshot.value = {...value, positions: previous.positions}; + } finally { + if (pendingOpen === pending) pendingOpen = null; + completeOpen(); + } state.selected = node.path; renderTabs(); await renderActiveFile(); diff --git a/src/content/viewer/viewer.js b/src/content/viewer/viewer.js index bb08325..6a449f5 100644 --- a/src/content/viewer/viewer.js +++ b/src/content/viewer/viewer.js @@ -46,7 +46,7 @@ export function createViewer(app) { /** Whether [shell] is still the open view. */ function isCurrent(shell) { - return shell.alive() && shell.generation === generation; + return viewer.open && shell.alive() && shell.generation === generation; } /** diff --git a/tests/file-tabs.test.mjs b/tests/file-tabs.test.mjs index a8eca55..15b38ec 100644 --- a/tests/file-tabs.test.mjs +++ b/tests/file-tabs.test.mjs @@ -55,7 +55,7 @@ async function turn() { } /** Composes real navigation, tree, branch and workspace factories around a metadata-only RPC fixture. */ -function fixture(t, provider, reviewDiff, {search = false} = {}) { +function fixture(t, provider, reviewDiff, {search = false, review = {}} = {}) { const {document, window} = dom(); if (reviewDiff || search) installDiffViewport(document); if (search) installViewerSearchDOM(document); @@ -69,6 +69,7 @@ function fixture(t, provider, reviewDiff, {search = false} = {}) { const assigned = []; const opened = []; const files = []; + const viewedRequests = []; const errors = []; let app; t.after(() => app?.dispose()); @@ -180,18 +181,23 @@ function fixture(t, provider, reviewDiff, {search = false} = {}) { if (type === 'DIFF') return { base: source(baseSHA), - head: source(reviewSHA), - files: paths.slice(0, 2).map(filename => ({ + head: source(review.headSHA || reviewSHA), + files: (review.paths || paths.slice(0, 2)).map(filename => ({ filename, ...(reviewDiff?.file || {status: 'added', additions: 220, deletions: 0, patch: ''}), })), comments: [], - viewed: {}, - viewedMode: 'none', + viewed: {...review.viewed}, + viewedMode: review.mode || 'none', warnings: [], }; if (type === 'BRANCHES') return [{name: 'main'}, {name: 'feature'}]; if (type === 'REFRESH') return null; + if (type === 'VIEWED') { + viewedRequests.push({...message}); + const controlled = app.viewedReply?.(message); + return controlled === undefined ? {state: message.viewed ? 'VIEWED' : 'UNVIEWED', mode: review.mode} : controlled; + } if (type === 'FILE_TABS') { const key = JSON.stringify([ context.origin, @@ -200,7 +206,10 @@ function fixture(t, provider, reviewDiff, {search = false} = {}) { message.scope, message.scope === 'review' ? context.number : '', ]); - if (message.value) metadata.set(key, structuredClone(message.value)); + if (message.value) { + if (app.fileTabsReply) await app.fileTabsReply(message, context); + metadata.set(key, structuredClone(message.value)); + } return {...structuredClone(metadata.get(key) || {paths: [], active: '', positions: [], page: ''}), accountID: ''}; } if (type === 'FILE') { @@ -407,6 +416,7 @@ function fixture(t, provider, reviewDiff, {search = false} = {}) { window, clock, files, + viewedRequests, errors, metadata, assigned, @@ -1218,6 +1228,265 @@ function searchRevision() { }; } +/** Reads the inline review action through its visible label and accessible status. */ +function fileReviewControls(ui) { + const group = ui.workspace().querySelector('.file-review'); + assert.ok(group, 'A loaded review file exposes Viewed and next-unviewed navigation.'); + assert.equal(group.getAttribute('aria-label'), 'File review'); + const button = group.querySelector('button'); + const status = group.querySelector('[role="status"]'); + assert.ok(button?.classList.contains('small-button'), 'Review navigation uses the shared button component.'); + assert.ok(button.querySelector('svg'), 'The action retains its original check icon.'); + assert.equal(status?.getAttribute('aria-live'), 'polite'); + return {group, button, status}; +} + +for (const provider of ['github', 'gitlab']) { + const mode = provider === 'github' ? 'github' : 'local'; + + test(`${provider} Viewed waits for confirmation and wraps through the full list despite sidebar filters`, async t => { + const reviewPaths = [paths[0], 'src/viewed.js', paths[1], 'src/also-viewed.js']; + const ui = fixture(t, provider, splitRevision(), { + review: {mode, paths: reviewPaths, viewed: {[reviewPaths[1]]: 'VIEWED', [reviewPaths[3]]: 'VIEWED'}}, + }); + await ui.navigate(ui.review(41)); + await ui.open(paths[1]); + await diffLayout(ui, ui.workspace(), 'Side-by-side'); + ui.app.state.query = 'also-viewed.js'; + ui.app.state.unviewedOnly = true; + await ui.drain(1); + assert.equal(ui.app.state.flat.filter(node => node.type === 'blob').length, 0); + const controls = fileReviewControls(ui); + const pending = deferred(); + ui.app.viewedReply = () => pending.promise; + assert.equal(controls.button.textContent, 'Viewed & next'); + const saving = controls.button.fire('click'); + await ui.drain(1); + assert.equal(controls.button.disabled, true); + assert.equal(controls.button.textContent, 'Saving…'); + assert.equal(ui.selection().active, paths[1]); + assert.equal( + ui.app.state.diff.viewed[paths[1]], + undefined, + 'An unconfirmed write is not published optimistically.', + ); + assert.equal(ui.viewedRequests.length, 1); + await controls.button.fire('click'); + assert.equal(ui.viewedRequests.length, 1, 'A pending action cannot send a duplicate mark.'); + assert.deepEqual(ui.viewedRequests[0], { + context: ui.app.state.context, + headSHA: reviewSHA, + path: paths[1], + viewed: true, + }); + pending.resolve({state: 'VIEWED', mode}); + await ui.settle(saving); + assert.equal( + ui.selection().active, + paths[0], + 'Navigation skips viewed files and wraps to the earlier unviewed file.', + ); + assert.equal(ui.app.state.diff.viewed[paths[1]], 'VIEWED'); + assert.equal( + ui.body().querySelectorAll('.split-diff-pane').length, + 2, + 'The chosen diff layout survives navigation.', + ); + assert.equal(controls.group.isConnected, false, 'The previous file releases its review control.'); + assert.equal(fileReviewControls(ui).button.textContent, 'Viewed & next'); + assert.deepEqual(ui.errors, []); + }); + + test(`${provider} a failed Viewed write stays on the file, retries and completes the listed review`, async t => { + const ui = fixture(t, provider, splitRevision(), {review: {mode}}); + await ui.navigate(ui.review(41)); + const controls = fileReviewControls(ui); + ui.app.viewedReply = () => Promise.reject(new Error('Viewed could not be saved.')); + await ui.settle(controls.button.fire('click')); + assert.equal(ui.selection().active, paths[0]); + assert.equal(ui.app.state.diff.viewed[paths[0]], undefined); + assert.equal(controls.button.disabled, false); + assert.equal(controls.button.textContent, 'Viewed & next'); + assert.equal(controls.status.textContent, 'Viewed could not be saved.'); + assert.ok(controls.status.classList.contains('error')); + ui.app.viewedReply = undefined; + await ui.settle(controls.button.fire('click')); + assert.equal(ui.selection().active, paths[1]); + assert.equal(ui.viewedRequests.length, 2); + const final = fileReviewControls(ui); + await ui.settle(final.button.fire('click')); + assert.equal(ui.selection().active, paths[1], 'Completing the list keeps the final file open.'); + assert.equal(final.button.disabled, true); + assert.equal(final.status.textContent, 'All listed files viewed'); + assert.equal(ui.viewedRequests.length, 3); + assert.deepEqual(ui.app.state.diff.viewed, {[paths[0]]: 'VIEWED', [paths[1]]: 'VIEWED'}); + assert.deepEqual(ui.errors, []); + }); + + test(`${provider} failed next-tab metadata preserves a confirmed mark and retries without another Viewed write`, async t => { + const ui = fixture(t, provider, splitRevision(), {review: {mode}}); + await ui.navigate(ui.review(41)); + const controls = fileReviewControls(ui); + ui.app.fileTabsReply = message => { + if (message.value.active === paths[1]) throw new Error('File tabs could not be saved.'); + }; + await ui.settle(controls.button.fire('click')); + assert.deepEqual(ui.selection(), {paths: [paths[0]], active: paths[0]}); + assert.equal(ui.app.state.diff.viewed[paths[0]], 'VIEWED', 'The successful mark is not rolled back.'); + assert.equal(ui.viewedRequests.length, 1); + assert.equal(controls.button.textContent, 'Next unviewed'); + assert.equal(controls.status.textContent, 'File tabs could not be saved.'); + assert.equal(controls.button.disabled, false); + ui.app.fileTabsReply = undefined; + await ui.settle(controls.button.fire('click')); + assert.deepEqual(ui.selection(), {paths: paths.slice(0, 2), active: paths[1]}); + assert.equal(ui.viewedRequests.length, 1, 'A navigation retry does not repeat the confirmed mark.'); + assert.deepEqual(ui.errors, []); + }); + + test(`${provider} scrolling during the next-file save preserves the new active tab and outgoing position`, async t => { + const ui = fixture(t, provider, splitRevision(), {review: {mode}}); + await ui.navigate(ui.review(41)); + const nextSave = deferred(); + // The trusted broker serializes writes: later scroll saves complete after + // the delayed next-file save, even while the old file remains visible. + ui.app.fileTabsReply = () => nextSave.promise; + const saving = fileReviewControls(ui).button.fire('click'); + await ui.drain(1); + assert.equal(ui.selection().active, paths[0]); + t.mock.timers.enable({apis: ['setTimeout']}); + try { + const body = ui.body(); + body.scrollTop = 440; + body.scrollLeft = 55; + await body.fire('scroll'); + t.mock.timers.tick(150); + await Promise.resolve(); + } finally { + t.mock.timers.reset(); + } + await ui.drain(1); + nextSave.resolve(); + await ui.settle(saving); + await ui.drain(); + assert.deepEqual(ui.selection(), {paths: paths.slice(0, 2), active: paths[1]}); + assert.equal(ui.metadata.size, 1); + const saved = [...ui.metadata.values()][0]; + assert.deepEqual(saved.paths, paths.slice(0, 2), 'A delayed old-file scroll cannot drop the newly opened tab.'); + assert.equal(saved.active, paths[1], 'The next file is the persisted active tab.'); + assert.deepEqual( + saved.positions.find(position => position.path === paths[0]), + {ref: `${baseSHA}:${reviewSHA}`, path: paths[0], top: 440, left: 55}, + 'The outgoing file still retains its latest scroll position.', + ); + assert.equal(ui.viewedRequests.length, 1); + assert.deepEqual(ui.errors, []); + }); + + test(`${provider} sidebar Viewed updates the open file action without refetching or resetting its viewport`, async t => { + const ui = fixture(t, provider, splitRevision(), {review: {mode}}); + await ui.navigate(ui.review(41)); + await ui.scroll(440, 55); + const controls = fileReviewControls(ui); + const body = ui.body(); + const requests = ui.files.length; + const checkbox = ui.app.view.spacer + .querySelectorAll('.viewed-checkbox') + .find(node => node.getAttribute('aria-label') === `Mark ${paths[0]} as viewed`); + assert.ok(checkbox, 'The current review file has its real sidebar Viewed checkbox.'); + checkbox.checked = true; + await ui.settle(checkbox.fire('change')); + assert.equal(controls.button.textContent, 'Next unviewed'); + assert.equal(ui.body(), body, 'Publishing the mark retains the currently rendered file.'); + assert.equal(ui.body().scrollTop, 440); + assert.equal(ui.body().scrollLeft, 55); + assert.equal(ui.files.length, requests, 'The shared mark must not reload the file text.'); + assert.equal(ui.selection().active, paths[0]); + await ui.settle(controls.button.fire('click')); + assert.equal(ui.selection().active, paths[1]); + assert.equal(ui.viewedRequests.length, 1, 'Next unviewed skips the mark already confirmed by the checkbox.'); + assert.deepEqual(ui.errors, []); + }); + + test(`${provider} manual tab intent wins while Viewed and the manual metadata save are pending`, async t => { + const ui = fixture(t, provider, splitRevision(), {review: {mode, paths}}); + await ui.navigate(ui.review(41)); + const pendingViewed = deferred(); + const pendingTab = deferred(); + ui.app.viewedReply = () => pendingViewed.promise; + const saving = fileReviewControls(ui).button.fire('click'); + await ui.drain(1); + ui.app.fileTabsReply = message => (message.value.active === paths[2] ? pendingTab.promise : undefined); + const opening = ui.app.openFileTab({path: paths[2]}); + await ui.drain(1); + assert.equal(ui.selection().active, paths[0], 'The manual file has not published before its metadata save.'); + pendingViewed.resolve({state: 'VIEWED', mode}); + await ui.settle(saving); + assert.equal(ui.app.state.diff.viewed[paths[0]], 'VIEWED'); + assert.equal(ui.selection().active, paths[0], 'The late mark must not start automatic navigation.'); + pendingTab.resolve(); + await ui.settle(opening); + assert.deepEqual(ui.selection(), {paths: [paths[0], paths[2]], active: paths[2]}); + assert.equal(ui.viewedRequests.length, 1); + assert.deepEqual(ui.errors, []); + }); + + for (const replacement of ['review', 'revision', 'disposal']) { + test(`${provider} a Viewed reply after ${replacement} cannot update or advance a stale inline viewer`, async t => { + const review = {mode}; + const ui = fixture(t, provider, splitRevision(), {review}); + await ui.navigate(ui.review(41)); + const oldDiff = ui.app.state.diff; + const controls = fileReviewControls(ui); + const pending = deferred(); + ui.app.viewedReply = () => pending.promise; + const saving = controls.button.fire('click'); + await ui.drain(1); + if (replacement === 'review') await ui.navigate(ui.review(42)); + else if (replacement === 'revision') { + review.headSHA = featureSHA; + await ui.settle(ui.app.refresh()); + assert.equal(ui.app.state.diff.head.sha, featureSHA); + } else ui.app.dispose(); + await ui.drain(1); + const fileRequests = ui.files.length; + const stored = structuredClone([...ui.metadata]); + pending.resolve({state: 'VIEWED', mode}); + await ui.settle(saving); + assert.deepEqual(oldDiff.viewed, {}, 'A reply cannot publish a mark after its review or page is obsolete.'); + assert.equal(controls.group.isConnected, false); + assert.equal(ui.files.length, fileRequests, 'A stale completion cannot load another file.'); + assert.deepEqual([...ui.metadata], stored, 'A stale completion cannot persist a different active file.'); + if (replacement !== 'disposal') { + assert.equal(ui.selection().active, paths[0]); + assert.deepEqual(ui.app.state.diff.viewed, {}); + assert.equal(fileReviewControls(ui).button.textContent, 'Viewed & next'); + } + assert.deepEqual(ui.errors, []); + }); + } + + test(`${provider} Viewed navigation is absent in Code, commits and unsupported review mode`, async t => { + const revision = splitRevision(); + const review = {mode: 'none'}; + const ui = fixture(t, provider, revision, {review}); + await ui.navigate(ui.root); + await ui.open(paths[0]); + assert.equal(ui.workspace().querySelector('.file-review'), null); + await ui.navigate(ui.review(41)); + assert.ok(ui.body().querySelector('.code-row'), 'The unsupported review still renders its diff.'); + assert.equal(ui.workspace().querySelector('.file-review'), null); + review.mode = mode; + ui.app.fileReply = (message, context) => + context.kind === 'commit' ? (message.source.sha === baseSHA ? revision.before : revision.after) : undefined; + await ui.navigate(`${ui.root}/${provider === 'gitlab' ? '-/' : ''}commit/${reviewSHA}`); + assert.ok(ui.body().querySelector('.code-row'), 'The commit still renders its diff.'); + assert.equal(ui.workspace().querySelector('.file-review'), null); + assert.deepEqual(ui.viewedRequests, []); + assert.deepEqual(ui.errors, []); + }); +} + for (const provider of ['github', 'gitlab']) { test(`${provider} file search finds literal offscreen text and retains its revealed position across tabs`, async t => { const ui = fixture(t, provider, undefined, {search: true}); diff --git a/tests/sidebar-reactive.test.mjs b/tests/sidebar-reactive.test.mjs index ebf9100..322a3de 100644 --- a/tests/sidebar-reactive.test.mjs +++ b/tests/sidebar-reactive.test.mjs @@ -916,6 +916,90 @@ for (const mode of ['github', 'local']) { }); } +for (const mode of ['github', 'local']) { + test(`tree Viewed writes publish confirmed ${mode} marks to the shared diff without replacing it`, async t => { + let reply = deferred(); + const ui = sidebar(t, () => reply.promise); + ui.state.mode = 'changes'; + const diff = reviewDiff(mode, {}, ['__proto__', 'src/a.js']); + ui.applyDiff(diff); + ui.clock.flush(); + const prototype = Object.getPrototypeOf(diff.viewed); + let checkbox = treeRow(ui, '__proto__').querySelector('.viewed-checkbox'); + checkbox.checked = true; + const mark = checkbox.fire('change'); + await Promise.resolve(); + assert.equal(Object.hasOwn(diff.viewed, '__proto__'), false, 'a pending write is not a confirmed shared mark'); + reply.resolve({state: 'VIEWED', mode}); + await mark; + ui.clock.flush(); + assert.equal(ui.state.diff, diff, 'publishing a mark must not reload an open viewer'); + assert.equal(Object.hasOwn(diff.viewed, '__proto__'), true, 'literal file paths become own map entries'); + assert.equal(diff.viewed.__proto__, 'VIEWED'); + assert.equal(Object.getPrototypeOf(diff.viewed), prototype); + assert.equal(reviewControls(ui).progress.textContent, '1/2 viewed'); + + reply = deferred(); + checkbox = treeRow(ui, '__proto__').querySelector('.viewed-checkbox'); + checkbox.checked = false; + const unmark = checkbox.fire('change'); + await Promise.resolve(); + assert.equal(diff.viewed.__proto__, 'VIEWED'); + reply.resolve({state: 'UNVIEWED', mode}); + await unmark; + ui.clock.flush(); + assert.equal(ui.state.diff, diff); + assert.equal(diff.viewed.__proto__, 'UNVIEWED', 'other review controls see confirmed unmarks too'); + assert.equal(reviewControls(ui).progress.textContent, '0/2 viewed'); + assert.deepEqual(ui.errors, []); + }); +} + +test('a confirmed GitHub local fallback updates the shared mode, sidebar notice and progress wording', async t => { + const ui = sidebar(t, async () => ({state: 'VIEWED', mode: 'local'})); + ui.state.mode = 'changes'; + const diff = reviewDiff('github', {}, ['src/a.js']); + ui.applyDiff(diff); + ui.state.expanded = new Set(['src']); + ui.clock.flush(); + assert.doesNotMatch(ui.view.notice.textContent, /Viewed marks are local/); + const checkbox = treeRow(ui, 'src/a.js').querySelector('.viewed-checkbox'); + checkbox.checked = true; + await checkbox.fire('change'); + ui.clock.flush(); + assert.equal(ui.state.diff, diff); + assert.equal(diff.viewedMode, 'local'); + assert.equal(diff.viewed['src/a.js'], 'VIEWED'); + assert.match(ui.view.notice.textContent, /Viewed marks are local to this browser/); + assert.equal(reviewControls(ui).progress.getAttribute('aria-label'), '1 of 1 listed changed files viewed locally'); + assert.equal(treeRow(ui, 'src/a.js').querySelector('.viewed-checkbox').title, 'Viewed locally'); + assert.deepEqual(ui.errors, []); +}); + +for (const [label, result] of [ + ['opposite mark', {state: 'UNVIEWED', mode: 'github'}], + ['unsupported mode', {state: 'VIEWED', mode: 'none'}], +]) { + test(`tree Viewed controls reject an acknowledgement with an ${label}`, async t => { + const ui = sidebar(t, async () => result); + ui.state.mode = 'changes'; + const diff = reviewDiff('github', {}, ['src/a.js']); + ui.applyDiff(diff); + ui.state.expanded = new Set(['src']); + ui.clock.flush(); + const checkbox = treeRow(ui, 'src/a.js').querySelector('.viewed-checkbox'); + checkbox.checked = true; + await checkbox.fire('change'); + ui.clock.flush(); + assert.deepEqual(diff.viewed, {}); + assert.equal(diff.viewedMode, 'github'); + assert.equal(checkbox.checked, false); + assert.equal(checkbox.disabled, false); + assert.equal(reviewControls(ui).progress.textContent, '0/1 viewed'); + assert.deepEqual(ui.errors, ['The Viewed update was not confirmed. Try again.']); + }); +} + test('folder Viewed aggregation does not mark empty or partially loaded folders', () => { const tree = makeTree([ {path: 'empty', type: 'tree', viewed: true}, diff --git a/tests/viewer-review.test.mjs b/tests/viewer-review.test.mjs new file mode 100644 index 0000000..093c762 --- /dev/null +++ b/tests/viewer-review.test.mjs @@ -0,0 +1,383 @@ +/* + * https://github.com/ziqq/Codetree + * Copyright (C) 2026 Anton Ustinoff + * https://github.com/ziqq/Codetree/blob/main/LICENSE + * + * Viewed-and-next regressions through the production View full modal. + * Controlled broker replies verify ordering and stale-view ownership; fixture + * geometry does not prove installed-browser or authenticated provider behavior. + */ + +import assert from 'node:assert/strict'; +import test from 'node:test'; +import {mkdtemp, rm, writeFile} from 'node:fs/promises'; +import {tmpdir} from 'node:os'; +import {join} from 'node:path'; +import {pathToFileURL} from 'node:url'; +import {bundle} from './helpers/content.mjs'; +import {dom, frames, install} from './helpers/dom.mjs'; +import {installDiffViewport} from './helpers/diff-viewport.mjs'; +import {installViewerSearchDOM} from './helpers/viewer-search.mjs'; + +const directory = await mkdtemp(join(tmpdir(), 'codetree-viewer-review-')); +const modulePath = join(directory, 'content.mjs'); +await writeFile(modulePath, await bundle()); +const content = await import(pathToFileURL(modulePath)); +await rm(directory, {recursive: true, force: true}); +const {createRoot, createState, getOwner, untrackActions, withOwner} = content; +const baseSHA = 'a'.repeat(40); +const headSHA = 'b'.repeat(40); +const paths = ['src/zebra.txt', 'docs/alpha.txt', 'src/middle.txt']; + +/** Creates exact complete revisions with a valid replacement patch for each listed file. */ +function revision(path) { + return { + before: `old ${path}\nshared context\n`, + after: `new ${path}\nshared context\n`, + node: { + path, + type: 'blob', + status: 'modified', + adds: 1, + dels: 1, + patch: `@@ -1,2 +1,2 @@\n-old ${path}\n+new ${path}\n shared context`, + }, + }; +} + +/** Owns one deferred broker acknowledgement without replacing production handlers. */ +function deferred() { + let resolve; + let reject; + const promise = new Promise((done, fail) => { + resolve = done; + reject = fail; + }); + return {promise, resolve, reject}; +} + +/** Installs the real modal and captures exact FILE and VIEWED requests. */ +function fixture( + t, + {provider = 'github', mode = provider === 'github' ? 'github' : 'local', viewed = {}, delayedClose = false} = {}, +) { + const {document, window} = dom(); + installDiffViewport(document); + installViewerSearchDOM(document); + const clock = frames(); + install(t, { + document, + window, + requestAnimationFrame: clock.requestAnimationFrame, + cancelAnimationFrame: clock.cancelAnimationFrame, + }); + const viewer = document.createElement('dialog'); + viewer.showModal = () => { + viewer.open = true; + }; + const closeEvents = []; + viewer.close = () => { + viewer.open = false; + if (delayedClose) closeEvents.push(() => viewer.fire('close')); + else void viewer.fire('close'); + }; + /** Delivers the queued native close event after intervening broker microtasks. */ + async function flushClose() { + for (const event of closeEvents.splice(0)) await event(); + } + document.body.append(viewer); + const files = paths.map(revision); + const requests = []; + const writes = []; + let pageAlive = true; + const app = createRoot(dispose => { + const state = createState(); + const origin = provider === 'github' ? 'https://github.com' : 'https://gitlab.com'; + state.context = {origin, provider, owner: 'sample', repo: 'repo', kind: 'pull', number: 1}; + const source = sha => ({origin, provider, owner: 'sample', repo: 'repo', sha}); + state.diff = { + base: source(baseSHA), + head: source(headSHA), + files: files.map(({node}) => ({...node, filename: node.path, additions: node.adds, deletions: node.dels})), + viewedMode: mode, + viewed: {...viewed}, + }; + const app = { + owner: getOwner(), + state, + dispose, + pageAlive: () => pageAlive, + view: {viewer}, + run: callback => withOwner(callback), + diffURL: async () => + `${origin}/sample/repo/${provider === 'github' ? 'pull/1/files' : '-/merge_requests/1/diffs'}`, + async rpc(type, message) { + requests.push({type, message}); + if (type === 'VIEWED') { + const response = deferred(); + writes.push({...response, message}); + return response.promise; + } + assert.equal(type, 'FILE'); + const file = files.find(({node}) => node.path === message.path); + assert.ok(file, `the requested file is listed: ${message.path}`); + return message.source.sha === baseSHA ? file.before : file.after; + }, + }; + Object.assign(app, untrackActions(content.createViewer(app))); + return app; + }); + t.after(app.dispose); + return {app, files, viewer, writes, requests, clock, flushClose, expire: () => (pageAlive = false)}; +} + +/** Requires the public review UI, so the pre-feature version fails on visible behavior. */ +function controls(ui) { + const group = ui.viewer.querySelector('[aria-label="File review"]'); + assert.ok(group, 'View full exposes Viewed and next-unviewed review controls'); + assert.equal(group.hidden, false); + const control = group.querySelector('button'); + assert.ok(control, 'the review action is a button'); + return {group, control, status: group.querySelector('[role="status"]')}; +} + +/** Opens one complete file and checks its heading and usable public review control. */ +async function open(ui, index = 0, layout = 'Unified') { + await ui.app.showDiff(ui.files[index].node); + assert.equal(ui.viewer.open, true); + assert.equal(heading(ui), paths[index]); + assert.ok(ui.viewer.querySelector('.code-spacer'), 'View full renders the complete validated file'); + if (layout === 'Side-by-side') { + const control = ui.viewer.querySelectorAll('button').find(node => node.textContent === layout); + assert.ok(control, 'View full exposes Side-by-side'); + await control.fire('click'); + ui.clock.flush(); + assert.ok(ui.viewer.querySelector('[aria-label="Old revision"]')); + assert.ok(ui.viewer.querySelector('[aria-label="New revision"]')); + } + return controls(ui); +} + +/** Reads the current public file heading without relying on viewer generation internals. */ +function heading(ui) { + return ui.viewer.querySelector('.viewer-heading')?.querySelector('strong')?.textContent; +} + +/** Returns file reads only, making unintended navigation observable. */ +function reads(ui) { + return ui.requests.filter(request => request.type === 'FILE').map(request => request.message.path); +} + +/** Starts the public action while withholding its broker acknowledgement. */ +function start(ui) { + const {control} = controls(ui); + assert.equal(control.disabled, false); + const task = control.fire('click'); + assert.equal(control.disabled, true, 'the write disables repeat activation'); + assert.equal(control.textContent, 'Saving…'); + return task; +} + +/** Confirms the latest captured write and waits for any resulting full-file navigation. */ +async function confirm(ui, task, result = {state: 'VIEWED'}) { + ui.writes.at(-1).resolve(result); + await task; + ui.clock.flush(); +} + +for (const {provider, mode, layout} of [ + {provider: 'github', mode: 'github', layout: 'Unified'}, + {provider: 'github', mode: 'local', layout: 'Side-by-side'}, + {provider: 'gitlab', mode: 'local', layout: 'Unified'}, + {provider: 'gitlab', mode: 'local', layout: 'Side-by-side'}, +]) { + test(`View full ${provider}/${mode} ${layout} waits for Viewed acknowledgement before advancing in listed order`, async t => { + const ui = fixture(t, {provider, mode}); + const {control} = await open(ui, 0, layout); + assert.equal(control.classList.contains('small-button'), true); + assert.equal(control.textContent, 'Viewed & next'); + const context = ui.app.state.context; + const diff = ui.app.state.diff; + const task = start(ui); + assert.equal(ui.writes.length, 1); + assert.deepEqual(ui.writes[0].message, {context, headSHA, path: paths[0], viewed: true}); + assert.equal(heading(ui), paths[0]); + assert.deepEqual(reads(ui), [paths[0], paths[0]], 'unconfirmed Viewed cannot start the next file fetch'); + assert.equal(diff.viewed[paths[0]], undefined); + await confirm(ui, task, {state: 'VIEWED', mode}); + assert.equal(diff.viewed[paths[0]], 'VIEWED'); + assert.equal(ui.app.state.diff, diff, 'publishing Viewed preserves the loaded review identity'); + assert.equal(heading(ui), paths[1], 'the API list order wins over alphabetical path sorting'); + assert.deepEqual(reads(ui), [paths[0], paths[0], paths[1], paths[1]]); + assert.equal(controls(ui).control.textContent, 'Viewed & next'); + }); +} + +test('View full skips viewed files and wraps to an earlier unviewed file despite sidebar filters', async t => { + const ui = fixture(t, {viewed: {[paths[1]]: 'VIEWED'}}); + ui.app.state.searchText = 'only-matching-something-else'; + ui.app.state.query = 'only-matching-something-else'; + ui.app.state.filter = 'removed'; + ui.app.state.unviewedOnly = true; + ui.app.state.entries = [{path: paths[2], viewed: false}]; + await open(ui, 2, 'Side-by-side'); + const task = start(ui); + await confirm(ui, task); + assert.equal(heading(ui), paths[0]); + assert.equal(ui.app.state.diff.viewed[paths[1]], 'VIEWED'); + assert.deepEqual(reads(ui), [paths[2], paths[2], paths[0], paths[0]]); +}); + +test('View full Next unviewed skips an already viewed current file without another write', async t => { + const ui = fixture(t, {viewed: {[paths[0]]: 'VIEWED', [paths[1]]: 'VIEWED'}}); + const {control} = await open(ui); + assert.equal(control.textContent, 'Next unviewed'); + await control.fire('click'); + assert.equal(heading(ui), paths[2]); + assert.equal(ui.writes.length, 0); + assert.deepEqual(reads(ui), [paths[0], paths[0], paths[2], paths[2]]); +}); + +test('View full keeps the final file open and reports completion of the listed files only', async t => { + const ui = fixture(t, {viewed: {[paths[0]]: 'VIEWED', [paths[1]]: 'VIEWED'}}); + ui.app.state.diff.warnings = ['GitHub returned 3 of 4 changed files. Its PR files API is limited to 3,000 files.']; + await open(ui, 2); + const task = start(ui); + await confirm(ui, task); + const {control, status} = controls(ui); + assert.equal(heading(ui), paths[2]); + assert.equal(ui.viewer.open, true); + assert.equal(control.disabled, true); + assert.equal(control.textContent, 'Next unviewed'); + assert.equal(status.textContent, 'All listed files viewed'); + assert.equal(status.getAttribute('aria-live'), 'polite'); + assert.deepEqual(reads(ui), [paths[2], paths[2]]); + await control.fire('click'); + assert.equal(ui.writes.length, 1, 'even a synthetic activation cannot repeat a completed mark'); +}); + +for (const failure of ['rejected', 'mismatched state', 'invalid mode']) { + test(`View full ${failure} acknowledgement keeps the file unviewed and offers a safe retry`, async t => { + const ui = fixture(t); + await open(ui); + const task = start(ui); + if (failure === 'rejected') ui.writes[0].reject(new Error('Provider could not save Viewed')); + else + ui.writes[0].resolve({ + state: failure === 'mismatched state' ? 'UNVIEWED' : 'VIEWED', + mode: failure === 'invalid mode' ? 'remote' : 'github', + }); + await task; + const {control, status} = controls(ui); + assert.equal(heading(ui), paths[0]); + assert.equal(ui.app.state.diff.viewed[paths[0]], undefined); + assert.deepEqual(reads(ui), [paths[0], paths[0]]); + assert.equal(control.disabled, false); + assert.equal(control.textContent, 'Viewed & next'); + assert.match(status.textContent, /could not save|not confirmed/i); + assert.equal(status.classList.contains('error'), true); + const retry = start(ui); + assert.equal(status.textContent, '', 'retry clears the previous provider error'); + assert.equal(ui.writes.length, 2); + await confirm(ui, retry); + assert.equal(heading(ui), paths[1]); + assert.equal(ui.app.state.diff.viewed[paths[0]], 'VIEWED'); + }); +} + +test('View full ignores duplicate activation while the same Viewed acknowledgement is pending', async t => { + const ui = fixture(t); + const {control} = await open(ui); + const task = start(ui); + await control.fire('click'); + await control.fire('click'); + assert.equal(ui.writes.length, 1); + assert.equal(heading(ui), paths[0]); + await confirm(ui, task); + assert.equal(heading(ui), paths[1]); + assert.equal(ui.writes.length, 1); +}); + +test('View full accepts GitHub local fallback before advancing and retains the returned mode', async t => { + const ui = fixture(t); + await open(ui); + const task = start(ui); + await confirm(ui, task, {state: 'VIEWED', mode: 'local'}); + assert.equal(ui.app.state.diff.viewedMode, 'local'); + assert.equal(ui.app.state.diff.viewed[paths[0]], 'VIEWED'); + assert.equal(heading(ui), paths[1]); +}); + +test('closing View full permits its same-review acknowledgement to publish without reopening the viewer', async t => { + const ui = fixture(t); + const {group} = await open(ui); + const task = start(ui); + const close = ui.viewer.querySelector('[aria-label="Close viewer"]'); + assert.ok(close); + await close.fire('click'); + assert.equal(ui.viewer.open, false); + assert.equal(group.isConnected, false, 'closing disposes the review control'); + await confirm(ui, task); + assert.equal(ui.app.state.diff.viewed[paths[0]], 'VIEWED'); + assert.equal(ui.viewer.open, false); + assert.deepEqual(reads(ui), [paths[0], paths[0]], 'a late acknowledgement cannot fetch or reopen another file'); +}); + +test('View full cannot reopen from a Viewed acknowledgement while its native close event is still queued', async t => { + const ui = fixture(t, {delayedClose: true}); + const {group} = await open(ui); + const task = start(ui); + const close = ui.viewer.querySelector('[aria-label="Close viewer"]'); + assert.ok(close); + await close.fire('click'); + assert.equal(ui.viewer.open, false, 'dialog.close clears open before dispatching the native close event'); + assert.equal(group.isConnected, true, 'the fixture withholds close cleanup until its queued event runs'); + await confirm(ui, task); + assert.equal(ui.app.state.diff.viewed[paths[0]], 'VIEWED', 'the confirmed same-review mark still publishes'); + assert.deepEqual(reads(ui), [paths[0], paths[0]], 'closed dialog cannot fetch another file before its close event'); + assert.equal(ui.viewer.open, false, 'the acknowledgement cannot reopen the dismissed dialog'); + assert.equal(heading(ui), paths[0]); + await ui.flushClose(); + assert.equal(group.isConnected, false, 'the queued close event disposes the old review controls'); + assert.equal(ui.viewer.open, false); +}); + +test('manual View full navigation wins over an earlier pending Viewed-and-next action', async t => { + const ui = fixture(t); + const {group} = await open(ui); + const task = start(ui); + const next = ui.viewer.querySelector('[aria-label="Next changed file"]'); + assert.ok(next); + await next.fire('click'); + assert.equal(heading(ui), paths[1]); + assert.equal(group.isConnected, false); + await confirm(ui, task); + assert.equal(ui.app.state.diff.viewed[paths[0]], 'VIEWED'); + assert.equal(heading(ui), paths[1]); + assert.deepEqual(reads(ui), [paths[0], paths[0], paths[1], paths[1]]); + assert.equal(ui.viewer.querySelectorAll('[aria-label="File review"]').length, 1); +}); + +for (const invalidation of [ + {name: 'page lifetime', change: ui => ui.expire()}, + {name: 'page epoch', change: ui => ui.app.state.epoch++}, + {name: 'file generation', change: ui => ui.app.state.filesGeneration++}, + {name: 'review context', change: ui => (ui.app.state.context = {...ui.app.state.context, number: 2})}, + {name: 'loaded review', change: ui => (ui.app.state.diff = {...ui.app.state.diff, viewed: {}})}, + {name: 'head revision', change: ui => (ui.app.state.diff.head.sha = 'c'.repeat(40))}, + {name: 'base revision', change: ui => (ui.app.state.diff.base.sha = 'd'.repeat(40))}, +]) { + test(`View full ignores a Viewed acknowledgement after its ${invalidation.name} changes`, async t => { + const ui = fixture(t); + await open(ui); + const diff = ui.app.state.diff; + const task = start(ui); + invalidation.change(ui); + await confirm(ui, task); + assert.equal(diff.viewed[paths[0]], undefined, 'a stale write is not published into the captured review'); + assert.equal(ui.app.state.diff.viewed[paths[0]], undefined, 'the current review is not marked by stale work'); + assert.equal(heading(ui), paths[0]); + assert.deepEqual(reads(ui), [paths[0], paths[0]], 'stale work cannot fetch the next changed file'); + assert.equal(ui.writes.length, 1); + }); +}