diff --git a/README.md b/README.md index 55a944f..06a1d41 100644 --- a/README.md +++ b/README.md @@ -53,7 +53,7 @@ Security boundaries: - Added and removed HTML must use one full-width pane. Never render an empty opposite pane. - Edited HTML must render both revisions without visible **Before**, **After**, or **Ready** labels. - Successful previews must not show resource panels or persistent diagnostics. Resource failures must remain actionable. -- Network failures and HTTP `408`, `425`, `429`, and `5xx` responses retry automatically three times with bounded exponential backoff. After exhaustion—or immediately for permanent failures—a failure-only **Retry** action clears cached metadata and refetches without reloading GitHub. +- Network failures and HTTP `408`, `425`, `429`, and `5xx` responses retry automatically three times with bounded exponential backoff. After exhaustion—or immediately for permanent failures—a failure-only **Retry** action refetches the selected HTML and its supported repository resource graph with fresh HTTP requests, without reloading GitHub or clearing unrelated caches. PR Retry keeps the same base/head commits and leaves other file previews alone. External resources and arbitrary runtime requests keep their normal browser behavior. - Review comments and GitHub collapse/expand behavior must continue working in Preview. - GitHub native and extension fallback cards must never produce duplicate previews. - Long HTML must remain scrollable. Edited panes must synchronize semantically when stable anchors exist and proportionally otherwise. diff --git a/e2e/extension.spec.ts b/e2e/extension.spec.ts index 9b0c9b4..a306415 100644 --- a/e2e/extension.spec.ts +++ b/e2e/extension.spec.ts @@ -2096,3 +2096,152 @@ function delay(milliseconds: number): Promise { setTimeout(resolve, milliseconds); return promise; } + + +test('Retry freshens only its blob source and transitive graph', async () => { + const { context, page, extensionId } = await launchWithExtension(); + const unrelated = await context.newPage(); + const selectedUrl = 'https://github.com/acme/reports/blob/main/selected/index.html'; + const otherUrl = 'https://github.com/acme/reports/blob/main/other/index.html'; + const requests: string[] = []; + let retrying = false; + const html = (version: string) => '

' + version + '

' + + '' + + 'waiting' + + '' + + '' + + '' + + '' + + '' + + ''; + try { + // Routing disables Chromium's HTTP cache. The unit regression proves + // cache-mode behavior; this checks fresh graph rendering and request scope. + await context.route('**/*', async route => { + const url = route.request().url(); + if (url === selectedUrl || url === otherUrl) { + const path = url === selectedUrl ? 'selected/index.html' : 'other/index.html'; + await route.fulfill({ status: 200, contentType: 'text/html', body: githubBlobFixture(html('cached'), commit, path) }); + return; + } + if (url.startsWith(rawBase + '/')) { + const path = url.slice(rawBase.length + 1); + requests.push(path); + const reload = retrying && path.startsWith('selected/'); + const name = path.split('/').pop(); + const version = reload ? 'fresh' : 'cached'; + if (path === 'selected/recover.png' && !reload) { + await route.fulfill({ status: 400, body: 'resource unavailable' }); return; + } + const body = name === 'index.html' ? html(version) : + name === 'diagram.svg' ? '' : + name === 'theme.css' ? '@import "./nested.css"; @font-face { font-family: report; src: url("./font.woff2") }' : + name === 'nested.css' ? 'h1 { color: ' + (reload ? 'rgb(4, 5, 6)' : 'rgb(1, 2, 3)') + '; background: url("./background.png") }' : + name === 'main.js' ? 'import { value } from "./child.js"; import("./lazy.js").then(() => document.querySelector("#module").textContent = value);' : + name === 'child.js' || name === 'lazy.js' ? 'export const value = "' + version + '";' : version; + await route.fulfill({ status: 200, contentType: name?.endsWith('.css') ? 'text/css' : name?.endsWith('.js') ? 'application/javascript' : name?.endsWith('.html') ? 'text/html' : 'application/octet-stream', headers: { 'cache-control': 'public, max-age=3600' }, body }); + return; + } + if (url.startsWith('chrome-extension:')) { await route.continue(); return; } + await route.abort(); + }); + await page.goto(selectedUrl); + const selected = page.locator('.gh-html-preview-container'); + await selected.getByText('View resource issues', { exact: true }).click(); + await expect(selected.getByRole('button', { name: 'Retry' })).toBeVisible(); + const initialFrame = selected.locator('iframe[title="Executable HTML preview"]').contentFrame(); + await expect(initialFrame.locator('#modern-loaded')).toHaveText('loaded'); + await expect(initialFrame.locator('#legacy-loaded')).toHaveText('loaded'); + const cachedModernPixels = await initialFrame.locator('#modern-image').screenshot(); + const cachedLegacyPixels = await initialFrame.locator('#legacy-image').screenshot(); + await unrelated.goto(otherUrl); + const other = unrelated.locator('.gh-html-preview-container'); + await expect(other.locator('iframe[title="Executable HTML preview"]').contentFrame().locator('#module')).toHaveText('cached'); + // Observe RequestInit in the extension isolated world without changing it. + // Interception occurs before Chromium adds HTTP cache directives. + const cdp = await context.newCDPSession(page); + const worlds: number[] = []; + cdp.on('Runtime.executionContextCreated', event => worlds.push(event.context.id)); + await cdp.send('Runtime.enable'); + let extensionWorld: number | undefined; + for (const contextId of worlds) { + const identity = await cdp.send('Runtime.evaluate', { contextId, + expression: 'globalThis.chrome?.runtime?.id', returnByValue: true }); + if (identity.result.value === extensionId) extensionWorld = contextId; + } + expect(extensionWorld).toBeDefined(); + await cdp.send('Runtime.evaluate', { contextId: extensionWorld, expression: 'globalThis.retryRequests = []; const originalFetch = globalThis.fetch; globalThis.fetch = function(input, init) { retryRequests.push({ url: String(input), cache: init?.cache }); return originalFetch.call(this, input, init); }' }); + const start = requests.length; + retrying = true; + await selected.getByRole('button', { name: 'Retry' }).click(); + const frame = selected.locator('iframe[title="Executable HTML preview"]').contentFrame(); + await expect(frame.locator('#version')).toHaveText('fresh'); + await expect(frame.locator('#module')).toHaveText('fresh'); + await expect(frame.locator('h1')).toHaveCSS('color', 'rgb(4, 5, 6)'); + await expect(frame.locator('#chart')).toHaveAttribute('src', 'data:image/png;base64,' + Buffer.from('fresh').toString('base64')); + await expect(frame.locator('#modern-loaded')).toHaveText('loaded'); + await expect(frame.locator('#legacy-loaded')).toHaveText('loaded'); + await expect(frame.locator('#modern-image')).toHaveAttribute('href', /^data:image\/svg\+xml;base64,/); + const svgState = await frame.locator('svg').evaluate(element => ({ + modern: element.querySelector('#modern-image')!.getAttribute('href'), + legacy: element.querySelector('#legacy-image')!.getAttributeNS('http://www.w3.org/1999/xlink', 'href'), + localWidth: (element.querySelector('#local-use') as SVGGraphicsElement).getBBox().width, + })); + expect(svgState.legacy).toBe(svgState.modern); + expect(Buffer.from(svgState.modern!.split(',')[1], 'base64').toString()).toContain('fill="lime"'); + expect(svgState.localWidth).toBe(12); + // Same element bounds, red before Retry and lime after: prove the fresh + // SVG bytes were painted, not merely loaded or embedded in the document. + expect(await frame.locator('#modern-image').screenshot()).not.toEqual(cachedModernPixels); + expect(await frame.locator('#legacy-image').screenshot()).not.toEqual(cachedLegacyPixels); + await expect(selected.getByRole('button', { name: 'Retry' })).toHaveCount(0); + const observed = await cdp.send('Runtime.evaluate', { contextId: extensionWorld, + expression: 'retryRequests', returnByValue: true }); + expect(observed.result.value).toHaveLength(11); + expect(observed.result.value.every((request: { url: string; cache: string }) => + request.cache === 'reload' && request.url.startsWith(rawBase + '/selected/'))).toBe(true); + expect(requests.slice(start).every(path => path.startsWith('selected/'))).toBe(true); + expect(requests.slice(start).sort()).toEqual([ + 'index.html', 'theme.css', 'nested.css', 'font.woff2', 'background.png', 'chart.png', 'recover.png', 'diagram.svg', 'main.js', 'child.js', 'lazy.js', + ].map(name => 'selected/' + name).sort()); + await expect(other.locator('iframe[title="Executable HTML preview"]').contentFrame().locator('#version')).toHaveText('cached'); + const normalStart = requests.length; + await unrelated.reload(); + await expect(other.locator('iframe[title="Executable HTML preview"]').contentFrame().locator('#module')).toHaveText('cached'); + expect(requests.slice(normalStart).every(path => path.startsWith('other/'))).toBe(true); + } finally { + await context.close(); + } +}); + + +test('Retry diagnoses repository external SVG use instead of emitting non-rendering data URLs', async () => { + const { context, page } = await launchWithExtension(); + const sprite = ''; + const dataUse = 'data:image/svg+xml;base64,' + Buffer.from(sprite).toString('base64') + '#box'; + const html = '' + + '' + + '' + + '' + + '' + + '' + + ''; + try { + const requested = await routeProductFixtures(context, html); + await page.goto(blobUrl); + const container = page.locator('.gh-html-preview-container'); + await container.getByText('View resource issues', { exact: true }).click(); + await container.getByRole('button', { name: 'Retry' }).click(); + await expect(container.getByText(/Embed the SVG symbols in this HTML/)).toHaveCount(2); + const frame = container.locator('iframe[title="Executable HTML preview"]').contentFrame(); + await expect(frame.locator('#repo')).not.toHaveAttribute('href'); + await expect(frame.locator('#repo-legacy')).not.toHaveAttribute('xlink:href'); + await expect(frame.locator('#data-use')).toHaveAttribute('href', dataUse); + await expect(frame.locator('#external')).toHaveAttribute('href', 'https://external.example/symbols.svg#box'); + const widths = await frame.locator('svg').evaluate(svg => Array.from(svg.querySelectorAll('use')).map(use => [use.id, use.getBBox().width])); + expect(Object.fromEntries(widths)).toEqual({ local: 24, 'legacy-local': 24, repo: 0, 'repo-legacy': 0, 'data-use': 0, external: 0 }); + expect(requested).not.toContain(rawBase + '/reports/weekly/symbols.svg'); + } finally { + await context.close(); + } +}); diff --git a/entrypoints/content.ts b/entrypoints/content.ts index 4d4fed8..d43f85f 100644 --- a/entrypoints/content.ts +++ b/entrypoints/content.ts @@ -353,7 +353,7 @@ export default defineContentScript({ }, }); -async function ensureResolved(route: RouteState): Promise { +async function ensureResolved(route: RouteState, refresh = false): Promise { if (!route.active || route.controller.signal.aborted) return; if (route.render || route.resolving) return route.resolving ?? Promise.resolve(); const controller = route.controller; @@ -371,10 +371,11 @@ async function ensureResolved(route: RouteState): Promise { privateRepo: route.isPrivate, }); const sourceHtml = - route.sourceHtml ?? + (!refresh ? route.sourceHtml : null) ?? ( await fetchRepositoryFile(route.repoRef, controller.signal, { privateRepo: route.isPrivate, + refresh, }) ).text; controller.signal.throwIfAborted(); @@ -382,6 +383,7 @@ async function ensureResolved(route: RouteState): Promise { target: 'sandbox-private', repoRef: route.repoRef, privateRepo: route.isPrivate, + refresh, signal: controller.signal, }); controller.signal.throwIfAborted(); @@ -516,7 +518,7 @@ function retryResolution(route: RouteState): void { route.details.replaceChildren(); route.details.style.display = 'none'; route.previewArea.replaceChildren(message('Retrying preview resources…')); - void ensureResolved(route); + void ensureResolved(route, true); } function createActionButton( diff --git a/entrypoints/pr.content.ts b/entrypoints/pr.content.ts index 942d68a..17a4d07 100644 --- a/entrypoints/pr.content.ts +++ b/entrypoints/pr.content.ts @@ -1422,10 +1422,8 @@ async function renderRichComparison( state.headRender?.destroy(); state.baseRender = null; state.headRender = null; - state.comparison = null; - routeState.metadata = null; - routeState.fileMetadata = null; - routeState.sessionPullData = null; + // Keep this card pinned to its comparison commits and leave shared route + // metadata/in-flight work alone. Failed metadata promises clear themselves. } state.retryableFailures = 0; const controller = new AbortController(); @@ -1474,6 +1472,7 @@ async function renderRichComparison( routeState, controller.signal, info, + force, ), ); } @@ -1487,6 +1486,7 @@ async function renderRichComparison( routeState, controller.signal, info, + force, ), ); } @@ -1539,6 +1539,7 @@ async function renderComparisonSide( routeState: DiffRouteState, signal: AbortSignal, knownInfo: DiffFileInfo | null, + refresh: boolean, ): Promise { const area = sideName === 'base' ? state.baseArea : state.headArea; try { @@ -1551,6 +1552,7 @@ async function renderComparisonSide( try { file = await fetchRepositoryFile(repoRef, signal, { privateRepo: side.privateRepo, + refresh, }); } catch (initialError) { const info = await getDiffFileInfo( @@ -1567,6 +1569,7 @@ async function renderComparisonSide( repoRef = sideRepoRef(side, path); file = await fetchRepositoryFile(repoRef, signal, { privateRepo: side.privateRepo, + refresh, }); } else if (sideName === 'base' && info?.status === 'added') { throw new ExpectedMissingSideError( @@ -1585,6 +1588,7 @@ async function renderComparisonSide( target: 'sandbox-private', repoRef, privateRepo, + refresh, signal, }); signal.throwIfAborted(); diff --git a/package.json b/package.json index 536af2d..63331e7 100644 --- a/package.json +++ b/package.json @@ -2,7 +2,7 @@ "name": "github-html-preview", "description": "Safely preview public HTML files and pull-request changes on GitHub", "private": true, - "version": "0.6.6", + "version": "0.6.7", "type": "module", "scripts": { "dev": "wxt", diff --git a/tests/blobViewChoice.test.ts b/tests/blobViewChoice.test.ts index d271578..535d3d6 100644 --- a/tests/blobViewChoice.test.ts +++ b/tests/blobViewChoice.test.ts @@ -184,7 +184,7 @@ describe('blob view choice', () => { await navigate('second.html', false); expect(mocks.fetch).toHaveBeenCalledWith( { owner: 'acme', repo: 'reports', ref: 'main', path: 'second.html' }, - expect.any(AbortSignal), { privateRepo: false }, + expect.any(AbortSignal), { privateRepo: false, refresh: false }, ); expect(mocks.resolve).toHaveBeenLastCalledWith('

Fetched current file

', expect.objectContaining({ repoRef: expect.objectContaining({ path: 'second.html' }) })); @@ -251,3 +251,18 @@ describe('blob view choice', () => { expect(mocks.resolve).not.toHaveBeenCalled(); }); }); + + +it('Retry fetches fresh HTML at the pinned commit instead of reusing embedded source', async () => { + mocks.resolve.mockResolvedValueOnce({ ...result, resources: { ...result.resources, failed: 1 }, + diagnostics: [{ level: 'error', code: 'resource-fetch-failed', message: 'Failed image' }] }); + start(); await settle(); + expect(mocks.fetch).not.toHaveBeenCalled(); + expect(mocks.resolve.mock.calls[0][0]).toBe('

first.html

'); + const retry = Array.from(document.querySelectorAll('button')).find(b => b.textContent === 'Retry')!; + retry.click(); await vi.advanceTimersByTimeAsync(1); await settle(); + expect(mocks.fetch).toHaveBeenCalledWith({ owner: 'acme', repo: 'reports', ref: oid, path: 'first.html' }, + expect.any(AbortSignal), { privateRepo: false, refresh: true }); + expect(mocks.resolve).toHaveBeenLastCalledWith('

Fetched current file

', expect.objectContaining({ refresh: true, repoRef: expect.objectContaining({ ref: oid }) })); + expect(Array.from(document.querySelectorAll('button')).some(b => b.textContent === 'Retry')).toBe(false); +}); diff --git a/tests/github.test.ts b/tests/github.test.ts index 980ef84..faf8c44 100644 --- a/tests/github.test.ts +++ b/tests/github.test.ts @@ -270,3 +270,21 @@ describe('repository fetching', () => { expect(cancel).toHaveBeenCalledTimes(1); }); }); + + +it('private Retry reloads only the exact session URL without public transport or credentials leakage', async () => { + vi.stubGlobal('location', new URL('https://github.com/private-owner/private-repo/blob/main/report.html')); + const fetchMock = vi.fn(async (input: RequestInfo | URL, _init?: RequestInit) => { + const response = new Response('private HTML', { headers: { 'content-type': 'text/plain' } }); + Object.defineProperty(response, 'url', { value: String(input) }); + return response; + }); + globalThis.fetch = fetchMock as typeof fetch; + await fetchRepositoryFile(repoRef, new AbortController().signal, { privateRepo: true, refresh: true }); + expect(fetchMock).toHaveBeenCalledTimes(1); + expect(fetchMock.mock.calls[0]).toEqual([buildGitHubSessionRawUrl(repoRef), expect.objectContaining({ cache: 'reload', credentials: 'same-origin', referrerPolicy: 'same-origin' })]); + expect(fetchMock.mock.calls[0][1]?.headers).toBeUndefined(); + fetchMock.mockClear(); + await fetchRepositoryFile(repoRef, new AbortController().signal, { privateRepo: true }); + expect(fetchMock.mock.calls[0][1]?.cache).toBeUndefined(); +}); diff --git a/tests/prViewLoading.test.ts b/tests/prViewLoading.test.ts index be05cfd..e3517f8 100644 --- a/tests/prViewLoading.test.ts +++ b/tests/prViewLoading.test.ts @@ -4,7 +4,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; const mocks = vi.hoisted(() => ({ getPreferences: vi.fn(), savePreferences: vi.fn(), watchPreferences: vi.fn(), - fetchFile: vi.fn(), fetchMetadata: vi.fn(), + fetchFile: vi.fn(), fetchMetadata: vi.fn(), resolve: vi.fn(), })); vi.mock('../utils/storage', () => ({ enabledStorage: { getValue: async () => true, watch: () => () => {} }, @@ -24,7 +24,7 @@ vi.mock('../utils/github', () => ({ vi.mock('../utils/debug', () => ({ debugLog: () => {}, debugError: () => {} })); vi.mock('../utils/fetchWithRetry', () => ({ fetchWithRetry: mocks.fetchMetadata })); vi.mock('../utils/resolveHtml', () => ({ - resolveHtml: async () => ({ resources: { failed: 0, skipped: 0 } }), + resolveHtml: mocks.resolve, })); vi.mock('../utils/renderer', () => ({ renderExecutablePreview: (area: HTMLElement) => { @@ -95,6 +95,7 @@ beforeEach(() => { mocks.savePreferences.mockResolvedValue(undefined); mocks.watchPreferences.mockImplementation((callback) => { preferenceWatcher = callback; return () => {}; }); mocks.fetchFile.mockResolvedValue({ text: '

Hello

', authenticated: false }); + mocks.resolve.mockResolvedValue({ resources: { failed: 0, skipped: 0 } }); page(); }); afterEach(() => { @@ -210,3 +211,33 @@ describe('PR HTML view loading', () => { expect(mocks.savePreferences.mock.calls.map(([value]) => value.mode)).toEqual(['source', 'split']); }); }); + + +it('Retry is card-local, pins base/head commits, and leaves another preview in flight', async () => { + mocks.resolve.mockResolvedValue({ resources: { failed: 1, skipped: 0 } }); + const files = document.querySelector('#files')!; + const second = files.firstElementChild!.cloneNode(true) as HTMLElement; + second.dataset.path = 'other.html'; files.append(second); + const pending = deferred<{ text: string; authenticated: boolean }>(); + mocks.fetchFile.mockImplementation((ref) => ref.path === 'other.html' ? pending.promise : Promise.resolve({ text: '

Hello

', authenticated: false })); + await mount(); + expect(mocks.fetchFile.mock.calls).toHaveLength(4); + const firstPanel = document.querySelector('.gh-html-preview-pr-rich')!; + // A changed page payload must not move the retried card to new commits. + const data = document.querySelector('script[data-target="react-app.embeddedData"]')!; + data.textContent = data.textContent!.replaceAll(baseSha, 'c'.repeat(40)).replaceAll(headSha, 'd'.repeat(40)); + mocks.resolve.mockResolvedValue({ resources: { failed: 0, skipped: 0 } }); + const retry = Array.from(firstPanel.querySelectorAll('button')).find(b => b.textContent === 'Retry')!; + retry.click(); await settle(); + const freshCalls = mocks.fetchFile.mock.calls.slice(4); + expect(freshCalls.map(call => call[0])).toEqual([ + expect.objectContaining({ path: 'index.html', ref: baseSha }), + expect.objectContaining({ path: 'index.html', ref: headSha }), + ]); + expect(freshCalls.every(call => call[2].refresh === true)).toBe(true); + expect(mocks.resolve.mock.calls.slice(2).every(call => call[1].refresh === true)).toBe(true); + expect(mocks.fetchFile.mock.calls.filter(call => call[0].path === 'other.html').every(call => call[1].aborted === false && !call[2].refresh)).toBe(true); + pending.resolve({ text: '

Other

', authenticated: false }); await settle(); + expect(document.querySelectorAll('.gh-html-preview-pr-rich iframe')).toHaveLength(4); + expect(mocks.fetchMetadata).not.toHaveBeenCalled(); +}); diff --git a/tests/resolveHtml.test.ts b/tests/resolveHtml.test.ts index 814937c..396103f 100644 --- a/tests/resolveHtml.test.ts +++ b/tests/resolveHtml.test.ts @@ -6,6 +6,7 @@ import { resolveRepositoryUrl, transformSrcset, } from '../utils/resolveHtml'; +import { fetchRepositoryFile } from '../utils/github'; import type { RepoRef } from '../utils/types'; const repoRef: RepoRef = { @@ -621,3 +622,161 @@ function decodeDataUrl(url: string): string { const encoded = url.split(',')[1] ?? ''; return atob(encoded); } + + +describe('scoped document Retry', () => { + it('reloads only the selected source and its packaged public graph; normal documents keep caching', async () => { + const selected = { ...repoRef, path: 'selected/index.html' }; + const other = { ...repoRef, path: 'other/index.html' }; + let version = 'old'; + const cache = new Map(); + const requests: Array<{ path: string; init?: RequestInit }> = []; + const html = (value: string) => '

' + value + '

' + + '' + + '' + + ''; + globalThis.fetch = vi.fn(async (input: RequestInfo | URL, init?: RequestInit) => { + const url = String(input); + const path = new URL(url).pathname.split('/').slice(4).join('/'); + requests.push({ path, init }); + const name = path.split('/').pop(); + const fresh = name === 'index.html' ? html(version) : + name === 'theme.css' ? '@import "./nested.css"; @font-face { font-family: report; src: url("./font.woff2") }' : + name === 'nested.css' ? 'h1 { --version: ' + version + '; background: url("./background.png") }' : + name === 'main.js' ? 'import { value } from "./child.js"; import("./lazy.js");' : + name === 'child.js' || name === 'lazy.js' ? 'export const value = "' + version + '";' : version; + if (!cache.has(url) || init?.cache === 'reload') cache.set(url, fresh); + return new Response(cache.get(url), { headers: { 'content-type': name?.endsWith('.css') ? 'text/css' : 'application/octet-stream' } }); + }) as typeof fetch; + // Prime both documents' repository assets using the packaging path. + for (const ref of [selected, other]) { + const source = await fetchRepositoryFile(ref, new AbortController().signal); + await resolveHtml(source.text, { target: 'sandbox-private', privateRepo: false, repoRef: ref }); + } + version = 'fresh'; + const start = requests.length; + const source = await fetchRepositoryFile(selected, new AbortController().signal, { refresh: true }); + const refreshed = await resolveHtml(source.text, { target: 'sandbox', repoRef: selected, refresh: true }); + const freshDoc = new DOMParser().parseFromString(refreshed.html, 'text/html'); + expect(freshDoc.querySelector('h1')?.textContent).toBe('fresh'); + expect(freshDoc.querySelector('style')?.textContent).toContain('--version: fresh'); + expect(freshDoc.querySelector('img')?.getAttribute('src')).toBe('data:image/png;base64,' + btoa('fresh')); + const imports = JSON.parse(freshDoc.querySelector('script[type="importmap"]')!.textContent!).imports; + expect(atob(imports['https://private-preview.invalid/selected/child.js'].split(',')[1])).toContain('fresh'); + expect(atob(imports['https://private-preview.invalid/selected/lazy.js'].split(',')[1])).toContain('fresh'); + expect(atob(imports['https://private-preview.invalid/selected/main.js'].split(',')[1])).toContain('import("https://private-preview.invalid/selected/lazy.js")'); + expect(refreshed.diagnostics).toEqual([]); + const retryRequests = requests.slice(start); + expect(retryRequests.map(r => r.path).sort()).toEqual([ + 'index.html', 'theme.css', 'nested.css', 'font.woff2', 'background.png', 'chart.png', 'main.js', 'child.js', 'lazy.js', + ].map(name => 'selected/' + name).sort()); + expect(retryRequests.every(r => r.init?.cache === 'reload' && r.init.credentials === 'omit')).toBe(true); + const normalStart = requests.length; + const normalSource = await fetchRepositoryFile(other, new AbortController().signal); + const normal = await resolveHtml(normalSource.text, { target: 'sandbox', repoRef: other }); + expect(normal.html).toContain('

old

'); + expect(normal.html).toContain('cdn.jsdelivr.net/gh/acme/reports@' + repoRef.ref + '/other/chart.png'); + expect(requests.slice(normalStart).every(r => r.path.startsWith('other/') && r.init?.cache === undefined)).toBe(true); + }); +}); + + +describe('packaged SVG repository references', () => { + const xlink = 'http://www.w3.org/1999/xlink'; + const svg = 'http://www.w3.org/2000/svg'; + + it('keeps normal public SVG images browser-loaded without a refresh fetch', async () => { + globalThis.fetch = vi.fn() as typeof fetch; + const result = await resolveHtml('' + + '' + + '', { + repoRef, target: 'sandbox', + }); + expect(globalThis.fetch).not.toHaveBeenCalled(); + const doc = new DOMParser().parseFromString(result.html, 'text/html'); + const expected = 'https://cdn.jsdelivr.net/gh/acme/reports@' + repoRef.ref + '/reports/weekly/diagram.svg#view'; + expect(doc.querySelector('#modern')!.getAttribute('href')).toBe(expected); + expect(doc.querySelector('#legacy')!.getAttributeNS(xlink, 'href')).toBe(expected); + }); + + it.each([false, true])('reloads and deduplicates SVG images with private transport=%s', async (privateRepo) => { + vi.stubGlobal('location', new URL('https://github.com/acme/reports/blob/main/index.html')); + let version = 'cached'; + const requests: Array<{ url: string; init?: RequestInit }> = []; + globalThis.fetch = vi.fn(async (input: RequestInfo | URL, init?: RequestInit) => { + const url = String(input); + requests.push({ url, init }); + const response = new Response('' + version + '', { + headers: { 'content-type': 'image/svg+xml' }, + }); + Object.defineProperty(response, 'url', { value: url }); + return response; + }) as typeof fetch; + const html = '' + + '' + + '' + + '' + + '' + + ''; + const options = { target: 'sandbox-private' as const, privateRepo, repoRef }; + await resolveHtml(html, options); + expect(requests).toHaveLength(1); + expect(requests[0].init?.cache).toBeUndefined(); + version = 'fresh'; + const result = await resolveHtml(html, { ...options, target: privateRepo ? 'sandbox-private' : 'sandbox', refresh: true }); + expect(requests).toHaveLength(2); + const retry = requests[1]; + expect(retry.init?.cache).toBe('reload'); + expect(retry.init?.credentials).toBe(privateRepo ? 'same-origin' : 'omit'); + expect(retry.url).toBe((privateRepo ? 'https://github.com/acme/reports/raw/' : 'https://raw.githubusercontent.com/acme/reports/') + repoRef.ref + '/reports/weekly/diagram.svg'); + expect(result.resources.fetched).toBe(1); + expect(result.diagnostics).toEqual([]); + const doc = new DOMParser().parseFromString(result.html, 'text/html'); + const modern = doc.querySelector('#modern')!; + const legacy = doc.querySelector('#legacy')!; + expect(modern.namespaceURI).toBe(svg); + expect(modern.getAttribute('href')).toMatch(/^data:image\/svg\+xml;base64,.*#view$/); + expect(atob(modern.getAttribute('href')!.split(',')[1].split('#')[0])).toContain('fresh'); + expect(legacy.getAttributeNodeNS(xlink, 'href')?.name).toBe('xlink:href'); + expect(legacy.getAttributeNS(xlink, 'href')).toMatch(/^data:image\/svg\+xml;base64,.*#other-view$/); + expect(doc.querySelector('#both')!.getAttribute('href')).toBe(doc.querySelector('#both')!.getAttributeNS(xlink, 'href')); + expect(doc.querySelector('#local')!.getAttribute('href')).toBe('#local-image'); + expect(doc.querySelector('#external')!.getAttributeNS(xlink, 'href')).toBe('https://external.example/diagram.svg#view'); + expect(doc.querySelector('svg')!.getAttribute('xmlns:xlink')).toBe(xlink); + }); + + it.each([false, true])('diagnoses repository external use without fetching or leaking it (private=%s)', async (privateRepo) => { + globalThis.fetch = vi.fn() as typeof fetch; + const result = await resolveHtml('' + + '' + + '' + + '' + + '' + + '', { + repoRef, target: privateRepo ? 'sandbox-private' : 'sandbox', privateRepo, refresh: true, + }); + expect(globalThis.fetch).not.toHaveBeenCalled(); + expect(result.resources.skipped).toBe(2); + expect(result.diagnostics).toEqual([ + expect.objectContaining({ code: 'svg-use-not-packaged', url: './symbols.svg#box', message: expect.stringContaining('Embed the SVG symbols') }), + expect.objectContaining({ code: 'svg-use-not-packaged', url: '/assets/symbols.svg#box' }), + ]); + const doc = new DOMParser().parseFromString(result.html, 'text/html'); + expect(doc.querySelector('#local')!.getAttribute('href')).toBe('#box'); + expect(doc.querySelector('#legacy-local')!.getAttributeNS(xlink, 'href')).toBe('#box'); + expect(doc.querySelector('#repo')!.hasAttribute('href')).toBe(false); + expect(doc.querySelector('#repo-legacy')!.hasAttributeNS(xlink, 'href')).toBe(false); + expect(doc.querySelector('#external')!.getAttribute('href')).toBe('https://external.example/symbols.svg#box'); + }); + + it('applies resource byte limits to SVG images without leaving browser-loaded references', async () => { + globalThis.fetch = vi.fn(async () => new Response('too big', { headers: { 'content-type': 'image/svg+xml' } })) as typeof fetch; + const result = await resolveHtml('', { + repoRef, target: 'sandbox', refresh: true, limits: { maxResourceBytes: 4 }, + }); + const doc = new DOMParser().parseFromString(result.html, 'text/html'); + expect(doc.querySelector('image')!.attributes).toHaveLength(0); + expect(result.diagnostics.every(issue => issue.code === 'resource-fetch-failed')).toBe(true); + expect(result.diagnostics).toHaveLength(2); + }); +}); diff --git a/utils/github.ts b/utils/github.ts index 6f7acea..d106e9b 100644 --- a/utils/github.ts +++ b/utils/github.ts @@ -43,6 +43,7 @@ export interface RepositoryBytes { export interface RepositoryFetchOptions { privateRepo?: boolean; maxBytes?: number; + refresh?: boolean; } export function parseBlobUrl(url: string | URL): RepoRef | null { @@ -283,6 +284,7 @@ export async function fetchRepositoryBytes( buildRawUrl({ ...repoRef, path }), { signal, + ...(options.refresh ? { cache: 'reload' as const } : {}), credentials: 'omit', redirect: 'error', referrerPolicy: 'no-referrer', @@ -320,6 +322,7 @@ export async function fetchRepositoryBytes( path, maxBytes, signal, + options.refresh ?? false, ); if (sessionResource) return sessionResource; throw new Error( @@ -334,6 +337,7 @@ async function fetchViaGitHubSession( path: string, maxBytes: number, signal: AbortSignal, + refresh: boolean, ): Promise { if ( typeof location === 'undefined' || @@ -352,6 +356,7 @@ async function fetchViaGitHubSession( buildGitHubSessionRawUrl(repoRef, path), { signal, + ...(refresh ? { cache: 'reload' as const } : {}), credentials: 'same-origin', redirect: 'follow', referrerPolicy: 'same-origin', diff --git a/utils/resolveHtml.ts b/utils/resolveHtml.ts index c994468..099df3c 100644 --- a/utils/resolveHtml.ts +++ b/utils/resolveHtml.ts @@ -86,6 +86,7 @@ class ResourceLoader { readonly limits: ResolveLimits, readonly signal: AbortSignal, readonly privateRepo: boolean, + readonly refresh: boolean, ) {} addDiagnostic( @@ -110,6 +111,7 @@ class ResourceLoader { this.signal, { privateRepo: this.privateRepo, + refresh: this.refresh, maxBytes: this.limits.maxResourceBytes, }, ); @@ -161,6 +163,7 @@ export async function resolveHtml( limits, signal, options.privateRepo ?? options.target === 'sandbox-private', + options.refresh ?? false, ); debugLog('resolver', 'start', { target: options.target, @@ -171,7 +174,10 @@ export async function resolveHtml( const doc = new DOMParser().parseFromString(html, 'text/html'); const sourcePath = resolveBasePath(doc, options.repoRef, loader); - if (options.target === 'sandbox') { + // Retry packages repository assets, including normally browser-loaded public + // URLs, so CSS/module descendants use the same per-request reload policy. + // This does not change the transport's public/private credential decision. + if (options.target === 'sandbox' && !options.refresh) { await resolveSandboxDocument(doc, sourcePath, loader); } else { await resolvePrivateSandboxDocument(doc, sourcePath, loader); @@ -480,6 +486,37 @@ async function resolvePrivateSandboxDocument( queueAttribute('link[href]:not([rel~="stylesheet"])', 'href'); queueAttribute('iframe[src], embed[src]', 'src'); queueAttribute('object[data]', 'data'); + // HTML's SVG parser supplies the authored SVG/xlink namespaces. Work on + // attribute nodes, not HTML property setters or recreated attributes. + for (const element of Array.from(doc.querySelectorAll('image, use'))) { + if (element.namespaceURI !== 'http://www.w3.org/2000/svg') continue; + for (const attribute of Array.from(element.attributes)) { + if ( + attribute.localName !== 'href' || + (attribute.namespaceURI !== null && + attribute.namespaceURI !== 'http://www.w3.org/1999/xlink') + ) continue; + if (element.localName === 'image') { + attributeJobs.push( + inlinePrivateAttribute(element, attribute.name, sourcePath, loader), + ); + } else { + const resolved = resolveRepositoryUrl( + attribute.value, loader.repoRef, sourcePath, + ); + if (resolved.kind !== 'repo') continue; + // Chromium does not render data-URL in our opaque-origin sandbox. + // Do not pretend a fetch/embedding would refresh a working diagram. + loader.stats.skipped += 1; + loader.addDiagnostic( + 'svg-use-not-packaged', + 'Repository SVG references cannot be packaged in this sandbox and were omitted. Embed the SVG symbols in this HTML and use href="#symbol", or use for a standalone SVG.', + attribute.value, + ); + element.removeAttributeNode(attribute); + } + } + } await Promise.all(attributeJobs); await Promise.all( @@ -706,11 +743,17 @@ async function inlinePrivateAttribute( sourcePath: string, loader: ResourceLoader, ): Promise { - const value = element.getAttribute(attribute); - if (!value) return; - const inlined = await privateResourceValue(value, sourcePath, loader); - if (inlined) element.setAttribute(attribute, inlined); - else element.removeAttribute(attribute); + const node = element.getAttributeNode(attribute); + if (!node?.value) return; + const inlined = await privateResourceValue(node.value, sourcePath, loader); + if (inlined) { + const resolved = resolveRepositoryUrl(node.value, loader.repoRef, sourcePath); + const svgImage = + element.namespaceURI === 'http://www.w3.org/2000/svg' && + element.localName === 'image'; + node.value = inlined + + (svgImage && resolved.kind === 'repo' ? resolved.hash ?? '' : ''); + } else element.removeAttributeNode(node); } async function privateResourceValue( @@ -810,7 +853,7 @@ async function rewriteModuleSource( if (record.d >= 0) { loader.addDiagnostic( 'dynamic-module-expression', - 'Dynamic module expression could not be preloaded for private preview.', + 'Dynamic module expression could not be preloaded for packaged preview.', sourcePath, ); } @@ -820,7 +863,12 @@ async function rewriteModuleSource( const resolved = resolveRepositoryUrl(record.n, loader.repoRef, sourcePath); if (resolved.kind !== 'repo' || !resolved.path) continue; const virtualUrl = privateModuleUrl(resolved.path); - replacements.push({ start: record.s, end: record.e, value: virtualUrl }); + // The lexer includes quotes in dynamic-import spans, but excludes them + // from static-import spans. Keep literal dynamic imports executable. + replacements.push({ + start: record.s, end: record.e, + value: record.d >= 0 ? JSON.stringify(virtualUrl) : virtualUrl, + }); await collectPrivateModule(resolved.path, loader, imports, modules, depth + 1); } let transformed = source; diff --git a/utils/types.ts b/utils/types.ts index 51b5341..0089585 100644 --- a/utils/types.ts +++ b/utils/types.ts @@ -46,6 +46,8 @@ export interface ResolveOptions { target: 'sandbox' | 'sandbox-private'; repoRef: RepoRef; privateRepo?: boolean; + /** Explicit document Retry: bypass HTTP cache for this resolution only. */ + refresh?: boolean; signal?: AbortSignal; limits?: Partial; } diff --git a/wisdom/product-review.md b/wisdom/product-review.md new file mode 100644 index 0000000..e53582b --- /dev/null +++ b/wisdom/product-review.md @@ -0,0 +1,23 @@ +# Product direction — 2026-10-07 + +The user wants user experience first. The job is reading HTML docs on GitHub without cloning the repo. HTML-only support is enough. + +Do not lead with mobile GitHub use, responsive controls, or CSS/JS-only PR support. Those were review ideas the user rejected or deprioritized. A demo link is not a priority if the reading flow is already clear. + +Check real documents for readable authored styles, heading links, reading space, and scrolling. These are checks, not confirmed bugs. + +## Chosen next step + +The user asked to improve Retry. Refresh only the selected HTML document and resources it uses. Leave unrelated documents and browser caches alone. Keep the existing failure-only action; add no cache-clear button or browser-wide cache permission. + +Open a PR, merge it after checks, then publish a release. Planned stable version: 0.6.7. + +## Implementation and validation + +The scoped Retry change and SVG review fix are integrated on t3/product-improvements in /home/tnfssc/.t3/worktrees/gh-html-iframe/t3-7864b6d9. Worker paths, commits, design, and limits are in [scoped HTML Retry](scoped-html-retry.md). + +Parent validation passed after the SVG fix: type-check, 108 unit tests, 26 extension E2E tests, and 3 live GitHub smoke tests. Stable and debug ZIPs built. Both exact ZIPs passed the blob-preview installation smoke. Version checks passed for package and both manifests at 0.6.7. No new permissions or controls were added. + +Next: final review, push the PR, wait for CI, merge, then tag the merged commit as v0.6.7. The tag workflow publishes the tested stable archive. No release has happened yet. Record later merge/release facts on the PR and release, not by editing this shipped worktree. + +Values say to put user experience first. No extra value was added for the cache implementation; its details belong with the feature. diff --git a/wisdom/scoped-html-retry.md b/wisdom/scoped-html-retry.md new file mode 100644 index 0000000..efcf0de --- /dev/null +++ b/wisdom/scoped-html-retry.md @@ -0,0 +1,54 @@ +# Scoped HTML Retry + +## Design + +HTML documents are the product, not cache management. Keep successful previews quiet and the existing failure-only Retry; no extra visible controls or permissions. + +Explicit document Retry passes refresh: true through source fetching and one resolver invocation. Repository fetches use native cache: 'reload': bypass the HTTP cache for these requests and let the browser update those entries. Normal loads omit the cache override. No browser/origin/repository clearing, browsingData, CacheStorage sweep, persisted registry, or URL query cache-buster is used. The existing resolution-local request deduplication, concurrency, aborts, and byte/depth/output limits remain. + +Blob Retry must ignore embedded sourceHtml and fetch the selected HTML again at its existing exact ref. Failed fresh requests do not fall back to old source. The previous render/snapshot is replaced only for that route. + +PR Retry destroys only that card's base/head renders and refreshes their HTML and dependencies (including renamed-base fallback). Keep the card's comparison commits and shared route metadata/in-flight promises; do not reset them to refresh content. Metadata failure promises already clear themselves. Another card can finish while Retry runs. + +The normal public resolver rewrites some assets to browser-loaded CDN URLs. A Retry cannot set their request cache mode through HTML attributes, and a query on an entry URL would not propagate to CSS/module descendants. For a public sandbox Retry, reuse the existing packaging traversal, fetching repository CSS imports, CSS images/fonts, attributes/srcset and script/module descendants with the same reload policy and embedding the results. Public/private transport selection is independent of packaging: public requests still omit credentials; private requests still use only the signed-in GitHub-tab session transport. Exact refs are unchanged. A literal dynamic-import regression exposed dropped quotes in lexer replacements; preserve quotes for those spans so packaged dynamic descendants actually execute. + +## Limits and full preview + +- This refreshes the statically discoverable, loader-supported repository graph, within existing resource/depth/byte limits. Literal module imports are packaged; computed dynamic imports are diagnosed, not guessed. Runtime fetch/XHR, runtime-created URLs, external authored resources and their descendants are not under this per-resolution repository-fetch policy. Do not claim arbitrary external/dynamic traffic is fully refreshed. Nested iframe documents are not recursively resolved as HTML graphs. +- Shared dependency URLs are legitimately refreshed when used by the selected document. Unrelated URLs retain their normal cache policy; this is not a separate cache partition for each HTML file. Upstream servers/CDNs may still return old content even after a fresh browser fetch. +- Full preview loads a session-stored resolved snapshot, rather than repository source. Its existing error-only Retry reloads that package, not a document network Retry. Missing/expired snapshots already direct the reader back to GitHub. Keep this behavior: fetching private source from an extension-origin page would break the GitHub-session boundary. Use the GitHub document's Retry and reopen full preview for freshly packaged content; opening/reloading an old snapshot is not a freshness guarantee. No new cross-tab broker is needed. + +## Checks + +- corepack pnpm test: extension build and **102 unit tests passed**. +- corepack pnpm exec tsc --noEmit and git diff --check: passed. +- corepack pnpm test:e2e: extension build and **25 Chromium E2E tests passed**, including existing public/private full-preview and PR Retry tests. +- Focused cache-model regression primes two documents, changes server content, then verifies only the selected source/CSS imports/images/fonts/static and literal-dynamic modules use reload and contain fresh bytes. An unrelated document retains old cached source and default fetch policy; normal public URL rewriting remains. +- Blob controller regression rejects stale embedded source on Retry. PR regression changes embedded commit metadata during Retry and holds another card in flight: original base/head refs remain pinned and unrelated work is not aborted/re-fetched. Private transport regression verifies reload without public requests or credential leakage. +- Browser regression uses existing routed fixtures, executes both static/literal-dynamic module descendants, checks fresh source/CSS/image output, exact requested graph, failure-only Retry disappearance, and an unrelated document. CDP observes the extension isolated world's native fetch init without changing its policy: every selected Retry request uses reload. Playwright routing disables Chromium HTTP caching, so this is a request-policy/rendering integration test, **not** a real disk-cache eviction test; the deterministic unit cache model supplies the stale/fresh proof. +- Earlier unnecessary xvfb wrapper runs reported successful Playwright checks but failed during wrapper cleanup; the harness already uses Chromium's new headless mode. The final direct run above exited zero. The existing es-module-lexer asm.js warning is nonfatal; unit assertions pass. + +## Provenance + +- Inherited commit: 1ccd8786c8fc2e14651f36c8189b1929aff5f68c. +- Worktree: /home/tnfssc/.bruv/worktrees/t3-7864b6d9-d4d936df1f38-task_1e568cb6. +- Branch: fix/scoped-html-retry. +- Parent owns integration/release. No push, PR, merge, release-version change, or release. Existing advice/wisdom files are untouched. + + +## SVG packaging review fix (0.6.7) + +- The Retry/private packaging traversal now includes SVG `` and ``. Repository images use the existing bounded, per-resolution deduplicated loader, so explicit Retry requests use `cache: 'reload'` with the original exact ref and public/private transport. The normal public browser-loaded path is unchanged. Updating attribute nodes retains authored SVG/xlink namespaces; local fragments and external URLs are retained, including SVG image view fragments after embedding. +- Do **not** embed external `` targets as data URLs: Chromium in the actual opaque-origin preview sandbox renders no geometry for ``, while inline `#box` targets render. Repository-file `use` references (both href spellings) are explicitly omitted by the packaging path with `svg-use-not-packaged` diagnostics, not fetched or claimed refreshed. The diagnostic tells authors to embed symbols in the HTML and use `href="#symbol"`, or use an `` with a standalone SVG. Arbitrary external-host URLs and authored data URLs remain unchanged and outside Retry freshness; cross-origin external `use` also did not render in Chromium. No sprite-import engine, new permission/control, public private-resource fallback, or cache-wide clearing was added. +- Focused checks: `corepack pnpm exec vitest run tests/resolveHtml.test.ts` (**23 passed**), `corepack pnpm exec tsc --noEmit`, and `git diff --check` passed. New unit regressions cover both SVG image attributes, namespace retention, view/local fragments, external URL preservation, normal public behavior, reload/deduplication/exact refs with both transports, byte-limit failures, and actionable external-use diagnostics. +- `corepack pnpm test:e2e --grep "Retry freshens|Retry diagnoses"`: extension build and **2 Chromium tests passed**. The selected-document graph now includes a valid SVG diagram, checks actual image load events for href/xlink:href, fresh SVG bytes, local-use geometry, one reload fetch for the shared SVG, and untouched unrelated work. The external-use regression checks the displayed author guidance, omitted repository references, unchanged authored external/data URLs, and actual zero-vs-nonzero browser geometry. Routed requests remain policy/rendering evidence, not real disk-cache eviction proof. +- Final full validation: `corepack pnpm test` (extension build, **108 unit tests passed**), `corepack pnpm test:e2e` (extension build, **26 Chromium tests passed**), `corepack pnpm exec tsc --noEmit`, and `git diff --check` all exited zero. The existing es-module-lexer asm.js warning remains nonfatal. +- Review-fix base: `d6f231c` (parent's integrated scoped Retry commit). Branch: `fix/scoped-html-retry-svg`. Worktree: `/home/tnfssc/.bruv/worktrees/t3-7864b6d9-d4d936df1f38-task_36737353`. Package remains **0.6.7**; parent owns integration. No push, PR, merge, or release. + +## Parent integration proof + +Parent integrated both worker commits and repeated type-check, all 108 unit tests, 26 extension E2E tests, and 3 live GitHub smoke tests successfully. Stable and debug 0.6.7 ZIPs built, their package/manifest versions match, and both exact archives passed the blob-preview installation smoke. The SVG browser test checks load events, fresh decoded SVG bytes, and changed screenshots of both modern/legacy SVG image elements. The final two focused Retry tests passed with these rendered-pixel comparisons. They compare before/after in the same run; no platform-specific golden images or new image dependency is needed. + +The current environment's normal /home/tnfssc/.local/bin/gh wrapper works. Forcing the older headless config path from github-cli.md reported no login here. Do not print credentials while troubleshooting this difference. + +Remaining release steps belong to the parent: final review, PR/CI, merge, then push v0.6.7 at the merged commit. No repository edits after shipping. Release facts go on the PR/release. diff --git a/wisdom/values.md b/wisdom/values.md index 906de63..26012a8 100644 --- a/wisdom/values.md +++ b/wisdom/values.md @@ -11,3 +11,7 @@ Read traces before changing a smoke test. Fix a real product fault without weake ## Remember choices, not startup effects Read the saved view before mounting. Save explicit user choices, not the automatic default. Keep the local choice current while storage writes finish. This helps views that remount on SPA routes; it does not mean another tab must switch its active view. See [HTML view startup](html-view-startup.md). + +## Put user experience first + +Start with what people actually do, then remove the friction in that job. Rank work by how much it helps users, not by how many controls we can expose. Keep the successful preview path simple. This helps product priorities and UI choices; it is not a reason to weaken credential isolation. See [product review](product-review.md).