Skip to content

Commit 54ef5eb

Browse files
Brian WestphalBrian Westphal
authored andcommitted
Serve demo SVG bytes so the live-rendered view stops 404ing the E2E suite (GB-947)
GB-932 renders SVGs as live `<img src="/api/image/:fileId/:side">`, but the synthetic demo file `src/assets/icons.min.svg` exists nowhere on disk or in git, so both sides returned 404. `svg_view_mode` is a server-persisted preference and the E2E suite shares one demo server, so once a spec toggled an SVG to the Rendered view, every later load of the giant SVG (navigation, stability, themes, large-file-perf) emitted a console 404 — which the new failOnPageError fixture then correctly failed on. The fixture didn't cause the breakage; it exposed a real 404. The difftool blob store already solves the same shape of problem — image bytes for a DB-seeded review with no git refs or working tree to re-read. Generalize it from difftool-only to that whole class: - Rename `src/difftool/blob-store.ts` to `src/git/image-blobs.ts` with neutral names (`writeImageBlob` / `readImageBlob` / `clearImageBlobs`), since it now backs demo reviews too, not just difftool sessions. - Add `resolveImageSide()` in the image route: read git / disk first, then fall back to the blob store (used outright for difftool, as a fallback for demo). Both the `:side` and `metadata` routes go through it. - Seed each demo SVG's old/new bytes into the blob store at setup time, reconstructed from the diff hunks (old drops adds, new drops removes). Then fix a second failure the 404 had been masking: the large-file-perf specs assert the SVG *code* view (giant-line truncation), but relied on the 'code' default that no longer holds once an earlier spec persists 'rendered' on the shared server. Add `ensureCodeView()` to pin the text view regardless of prior state. Verified with the CI-parity Docker E2E suite: 149 passed, 0 failed (was 16 failed). Unit tests and the difftool integration suite still pass after the rename.
1 parent 484d4f1 commit 54ef5eb

12 files changed

Lines changed: 222 additions & 141 deletions

File tree

docs/ai/code-summary.md

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -99,7 +99,7 @@ How the layer is used:
9999
| `api/outline.ts` | `/outline/:fileId` and `/symbol-definition` go-to-definition repo scan. |
100100
| `api/context.ts` | `/context/:fileId` line-range fetch for hunk expansion. |
101101
| `api/project-settings.ts` | `.glassbox/settings.json` read/write (per-repo `appName`). |
102-
| `api/image.ts` | Image diff: `/image/:fileId/metadata` and `/image/:fileId/:side` with SVG rasterization. For a difftool review (doc 19) the bytes come from the persisted blob store (`src/difftool/blob-store.ts`) instead of git refs / disk — a difftool session has neither (GB-863). |
102+
| `api/image.ts` | Image diff: `/image/:fileId/metadata` and `/image/:fileId/:side`; SVGs served as raw `image/svg+xml` for live `<img>` render. `resolveImageSide()` reads from git refs / disk, then falls back to the persisted blob store (`src/git/image-blobs.ts`) — used outright for a difftool review (doc 19, no git/working tree, GB-863) and as a fallback for demo SVGs seeded into the DB with no on-disk file (GB-947). |
103103
| `api/share-prompt.ts` | Share prompt state / dismiss / tick (uses `global-config.ts`). |
104104
| `api/system.ts` | `POST /open-external` — opens a validated http(s) URL via `openOS` (same OS-open path as file reveal). |
105105
| `ai-api.ts` | Router that mounts ai-config + ai-analysis handlers under `/api/ai/*`. |
@@ -332,10 +332,11 @@ path), `GET /poll` (live file list + `active` flag for the client sidebar),
332332
attached; resolves on session end), `POST /end` ("Done" / tab-close
333333
`sendBeacon`). Session state + lifecycle live in `src/difftool/session.ts`;
334334
the detached server is started by `glassbox --difftool-serve`. On append, an
335-
image/SVG file's raw old/new bytes are persisted by `src/difftool/blob-store.ts`
336-
(content under `<dataDir>/difftool-blobs/`, keyed by `fileId`+side, cleared on
335+
image/SVG file's raw old/new bytes are persisted by `src/git/image-blobs.ts`
336+
(content under `<dataDir>/image-blobs/`, keyed by `fileId`+side, cleared on
337337
session start + teardown) so the `/image` route can serve them — a difftool
338-
review has no git refs / working tree to re-read (GB-863).
338+
review has no git refs / working tree to re-read (GB-863). The same store also
339+
backs demo SVGs, which are seeded into the DB with no on-disk file (GB-947).
339340

340341
## 6. Database schema
341342

docs/ai/requirements-summary.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -568,7 +568,7 @@ launcher shim in `--difftool-serve` mode and later invocations append to the
568568
running session; closing the window kills the sidecar and ends the session
569569
(`src-tauri/src/lib.rs`). Image/SVG visual comparison works too: the append
570570
endpoint persists each binary/SVG file's raw bytes to an on-disk blob store
571-
(`src/difftool/blob-store.ts`, under the session data dir, cleared on teardown)
571+
(`src/git/image-blobs.ts`, under the session data dir, cleared on teardown)
572572
and the `/image` route reads from there for a difftool review (GB-863). Internal
573573
git subprocesses run with `git difftool`'s leaked `GIT_EXTERNAL_DIFF` /
574574
`GIT_DIFF_PATH_*` scrubbed (`scrubbedGitEnv()` in `src/git/repo.ts`) so Glassbox's

src/cli.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -291,15 +291,15 @@ async function main() {
291291
if (difftoolServe) {
292292
const { initDifftoolSession } = await import("./difftool/session.js");
293293
const { writeDiscovery, clearDiscovery, releaseStartingLock } = await import("./git/difftool-discovery.js");
294-
const { clearDifftoolBlobs } = await import("./difftool/blob-store.js");
294+
const { clearImageBlobs } = await import("./git/image-blobs.js");
295295
mkdirSync(dataDir, { recursive: true });
296296
setDataDir(dataDir);
297297
// Capture as a const so the shutdown closure below sees a non-null string
298298
// (TS won't narrow the captured `let dataDir`).
299299
const sessionDataDir = dataDir;
300300
// Clear any image blobs left by a previous session that was hard-killed
301301
// (e.g. desktop window force-close) before it could run teardown (GB-863).
302-
clearDifftoolBlobs(sessionDataDir);
302+
clearImageBlobs(sessionDataDir);
303303
const repoRoot = process.cwd();
304304
const review = await createReview(repoRoot, "git difftool", "difftool");
305305
const { port: actualPort, server } = await startServer(port, review.id, repoRoot, { noOpen, strictPort });
@@ -308,7 +308,7 @@ async function main() {
308308
repoRoot,
309309
shutdown: () => {
310310
try { server.close(); } catch { /* already closing */ }
311-
clearDifftoolBlobs(sessionDataDir);
311+
clearImageBlobs(sessionDataDir);
312312
clearDiscovery();
313313
releaseStartingLock();
314314
process.exit(0);

src/demo.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,31 @@
11
import { saveGuidedReviewConfig } from './ai/config.js';
22
import { appendFileScores, createAnalysis, saveUserPreferences, updateAnalysisStatus } from './db/ai-queries.js';
3+
import { getDataDir } from './db/connection.js';
34
import { addAnnotation, addReviewFile, createReview } from './db/queries.js';
45
import type { FileDiff } from './git/diff.js';
6+
import { writeImageBlob } from './git/image-blobs.js';
57
import type { ReviewNoteView } from './review-notes/view.js';
68

9+
/**
10+
* The SVG demo files are seeded into the DB, not committed to disk, so the
11+
* live-rendered `<img>` view (GB-932) — which fetches `/api/image/:fileId/:side`
12+
* — would 404 on the synthetic giant `src/assets/icons.min.svg` (GB-947). The
13+
* full bytes of each side live in the diff hunks, so we persist them to the
14+
* image-blob store, which the image route serves as a fallback. SVGs are
15+
* single-hunk full-file diffs here, so each side is the join of the lines it
16+
* keeps (old drops adds; new drops removes).
17+
*/
18+
function seedSvgBlobs(fileId: string, diff: FileDiff): void {
19+
if (!diff.filePath.toLowerCase().endsWith('.svg')) return;
20+
const dataDir = getDataDir();
21+
if (dataDir === null) return;
22+
const lines = diff.hunks.flatMap(h => h.lines);
23+
const sideBytes = (keep: (type: string) => boolean): Buffer =>
24+
Buffer.from(lines.filter(l => keep(l.type)).map(l => l.content).join('\n'), 'utf8');
25+
writeImageBlob(dataDir, fileId, 'old', sideBytes(t => t !== 'add'));
26+
writeImageBlob(dataDir, fileId, 'new', sideBytes(t => t !== 'remove'));
27+
}
28+
729
/**
830
* Illustrative AI-authored review notes for the demo (docs/20 P2). Demo runs
931
* against synthetic, DB-seeded diffs with no on-disk `.pr-notes/`, so the diff
@@ -495,6 +517,7 @@ export async function setupDemoReview(scenario: number): Promise<{ reviewId: str
495517
};
496518
const rf = await addReviewFile(review.id, file.path, JSON.stringify(diff));
497519
fileIdMap.set(file.path, rf.id);
520+
seedSvgBlobs(rf.id, diff);
498521
}
499522

500523
// Common: mark some files as reviewed

src/difftool/blob-store.ts

Lines changed: 0 additions & 58 deletions
This file was deleted.

src/git/image-blobs.ts

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs';
2+
import { join } from 'node:path';
3+
4+
/**
5+
* On-disk storage for the raw image/SVG bytes of a review whose files have no
6+
* git refs and no working tree to re-read from.
7+
*
8+
* The image-comparison routes (`src/routes/api/image.ts`) normally re-fetch a
9+
* file's bytes from git or the working tree (`getOldImage`/`getNewImage`). Two
10+
* review kinds have neither:
11+
*
12+
* - an accumulating `git difftool` session (doc 19 / GB-863), whose files are
13+
* appended with their raw content and no backing refs, and
14+
* - demo mode (GB-947), whose synthetic SVGs (e.g. the giant
15+
* `src/assets/icons.min.svg`) are seeded into the DB with no real file on
16+
* disk — so once an SVG is shown in the live-rendered `<img>` view (GB-932),
17+
* `/api/image/:fileId/:side` would 404.
18+
*
19+
* For both, we persist the raw old/new bytes here when the file is created, then
20+
* read them back to serve the `/image` route. Files live under the review's data
21+
* dir and are cleared on difftool-session start + teardown; demo runs use a
22+
* fresh per-run tmpdir, so they need no explicit clear.
23+
*
24+
* Keyed by `fileId`+side rather than content hash: the filename is itself the
25+
* lookup key, which avoids a separate fileId→hash mapping. A re-created file
26+
* reuses its fileId, so its blobs are overwritten in place.
27+
*/
28+
29+
function blobDir(dataDir: string): string {
30+
return join(dataDir, 'image-blobs');
31+
}
32+
33+
/** fileIds are base36 (`[a-z0-9]`); strip anything else as belt-and-suspenders
34+
* so the key can never escape the blob directory. */
35+
function blobName(fileId: string, side: 'old' | 'new'): string {
36+
return `${fileId.replace(/[^a-z0-9]/gi, '')}-${side}`;
37+
}
38+
39+
/** Persist one side's raw bytes for a file. No-op for empty content (the absent
40+
* side of an add/delete). */
41+
export function writeImageBlob(dataDir: string, fileId: string, side: 'old' | 'new', bytes: Buffer): void {
42+
if (bytes.length === 0) return;
43+
const dir = blobDir(dataDir);
44+
mkdirSync(dir, { recursive: true });
45+
writeFileSync(join(dir, blobName(fileId, side)), bytes);
46+
}
47+
48+
/** Read back a side's bytes, or null if none were stored. */
49+
export function readImageBlob(dataDir: string, fileId: string, side: 'old' | 'new'): Buffer | null {
50+
const path = join(blobDir(dataDir), blobName(fileId, side));
51+
if (!existsSync(path)) return null;
52+
try {
53+
return readFileSync(path);
54+
} catch {
55+
return null;
56+
}
57+
}
58+
59+
/** Remove every stored blob. Called when a difftool session starts (clear a
60+
* previous session's leftovers, e.g. after a hard kill) and when it ends. */
61+
export function clearImageBlobs(dataDir: string): void {
62+
try {
63+
rmSync(blobDir(dataDir), { recursive: true, force: true });
64+
} catch {
65+
/* best-effort */
66+
}
67+
}

src/routes/api/image.ts

Lines changed: 43 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -2,27 +2,57 @@ import { Hono } from 'hono';
22

33
import { getDataDir } from '../../db/connection.js';
44
import { getReview, getReviewFile } from '../../db/queries.js';
5-
import { readDifftoolBlob } from '../../difftool/blob-store.js';
5+
import type { ReviewMode } from '../../git/diff.js';
66
import { parseDiffData, parseModeString } from '../../git/diff.js';
77
import type { ImageSide } from '../../git/image.js';
88
import { extractMetadata, formatMetadataLines, getContentType, getNewImage, getOldImage } from '../../git/image.js';
9+
import { readImageBlob } from '../../git/image-blobs.js';
910
import type { AppEnv } from '../../types.js';
1011
import { requirePathParam } from '../../utils/parseBody.js';
1112

1213
export const imageRoutes = new Hono<AppEnv>();
1314

1415
/**
15-
* A difftool review (doc 19) has no git refs or working tree, so its image bytes
16-
* come from the blobs persisted at append time (GB-863) rather than
17-
* `getOldImage`/`getNewImage`. Returns null if nothing was stored for this side.
16+
* Read a side's bytes from the on-disk blob store (`src/git/image-blobs.ts`).
17+
* Returns null if nothing was stored for this side / there's no data dir.
1818
*/
19-
function difftoolImageSide(fileId: string, side: 'old' | 'new'): ImageSide | null {
19+
function blobImageSide(fileId: string, side: 'old' | 'new'): ImageSide | null {
2020
const dataDir = getDataDir();
2121
if (dataDir === null) return null;
22-
const bytes = readDifftoolBlob(dataDir, fileId, side);
22+
const bytes = readImageBlob(dataDir, fileId, side);
2323
return bytes !== null ? { data: bytes, size: bytes.length } : null;
2424
}
2525

26+
/**
27+
* Resolve one side's image bytes for any review mode, honoring add/delete (the
28+
* absent side has no bytes).
29+
*
30+
* - A difftool review (doc 19) has no git refs or working tree, so its bytes
31+
* come only from the blobs persisted at append time (GB-863).
32+
* - Every other mode reads from git / the working tree, but falls back to the
33+
* blob store when that yields nothing — demo mode (GB-947) seeds synthetic
34+
* SVGs that exist nowhere on disk, so the live-rendered `<img>` view (GB-932)
35+
* would otherwise 404.
36+
*/
37+
function resolveImageSide(
38+
fileId: string,
39+
side: 'old' | 'new',
40+
status: string,
41+
mode: ReviewMode,
42+
filePath: string,
43+
oldPath: string | null,
44+
repoRoot: string,
45+
isDifftool: boolean,
46+
): ImageSide | null {
47+
if (side === 'old' && status === 'added') return null;
48+
if (side === 'new' && status === 'deleted') return null;
49+
if (isDifftool) return blobImageSide(fileId, side);
50+
const fromGit = side === 'old'
51+
? getOldImage(mode, filePath, oldPath, repoRoot)
52+
: getNewImage(mode, filePath, repoRoot);
53+
return fromGit ?? blobImageSide(fileId, side);
54+
}
55+
2656
// Metadata route must come before the :side wildcard route
2757
imageRoutes.get('/image/:fileId/metadata', async (c) => {
2858
const fileIdParam = requirePathParam(c, 'fileId');
@@ -40,12 +70,8 @@ imageRoutes.get('/image/:fileId/metadata', async (c) => {
4070
const status = diff?.status ?? 'modified';
4171
const isDifftool = review.mode === 'difftool';
4272

43-
const oldImage = status === 'added' ? null
44-
: isDifftool ? difftoolImageSide(fileIdParam.data, 'old')
45-
: getOldImage(mode, file.file_path, oldPath, repoRoot);
46-
const newImage = status === 'deleted' ? null
47-
: isDifftool ? difftoolImageSide(fileIdParam.data, 'new')
48-
: getNewImage(mode, file.file_path, repoRoot);
73+
const oldImage = resolveImageSide(fileIdParam.data, 'old', status, mode, file.file_path, oldPath, repoRoot, isDifftool);
74+
const newImage = resolveImageSide(fileIdParam.data, 'new', status, mode, file.file_path, oldPath, repoRoot, isDifftool);
4975

5076
const oldMeta = oldImage !== null ? extractMetadata(oldImage.data, oldPath ?? file.file_path) : null;
5177
const newMeta = newImage !== null ? extractMetadata(newImage.data, file.file_path) : null;
@@ -72,12 +98,12 @@ imageRoutes.get('/image/:fileId/:side', async (c) => {
7298
const mode = parseModeString(review.mode);
7399
const diff = parseDiffData(file.diff_data);
74100
const oldPath: string | null = diff?.oldPath ?? null;
101+
const status = diff?.status ?? 'modified';
75102

76-
const image = review.mode === 'difftool'
77-
? difftoolImageSide(fileIdParam.data, side)
78-
: side === 'old'
79-
? getOldImage(mode, file.file_path, oldPath, repoRoot)
80-
: getNewImage(mode, file.file_path, repoRoot);
103+
const image = resolveImageSide(
104+
fileIdParam.data, side, status, mode, file.file_path, oldPath, repoRoot,
105+
review.mode === 'difftool',
106+
);
81107

82108
if (!image) return c.text('Image not available', 404);
83109

src/routes/difftool-api.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ import { Hono } from 'hono';
33
import { AppendDifftoolFileReqSchema, RegisterDifftoolReqSchema } from '../api/index.js';
44
import { getDataDir } from '../db/connection.js';
55
import { addReviewFile, getAnnotationCountsForReview, getReviewFiles, updateFileDiff } from '../db/queries.js';
6-
import { writeDifftoolBlob } from '../difftool/blob-store.js';
76
import {
87
addDifftoolHold,
98
endDifftoolSession,
@@ -13,6 +12,7 @@ import {
1312
import { diffRawContent } from '../git/diff.js';
1413
import { getDifftoolStatus, registerDifftool, unregisterDifftool } from '../git/difftool.js';
1514
import { isSvgFile } from '../git/image.js';
15+
import { writeImageBlob } from '../git/image-blobs.js';
1616
import type { AppEnv } from '../types.js';
1717
import { errorResponse, parseBody } from '../utils/parseBody.js';
1818

@@ -86,8 +86,8 @@ difftoolApiRoutes.post('/append', async (c) => {
8686
if (diff.isBinary || isSvgFile(diff.filePath)) {
8787
const dataDir = getDataDir();
8888
if (dataDir !== null) {
89-
writeDifftoolBlob(dataDir, fileId, 'old', oldContent);
90-
writeDifftoolBlob(dataDir, fileId, 'new', newContent);
89+
writeImageBlob(dataDir, fileId, 'old', oldContent);
90+
writeImageBlob(dataDir, fileId, 'new', newContent);
9191
}
9292
}
9393

tests/e2e/large-file-perf.test.ts

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,21 @@ async function openFileList(page: import('@playwright/test').Page) {
3030
await expect(page.locator('#progress-summary')).toHaveText(/files reviewed/, { timeout: 5000 });
3131
}
3232

33+
/**
34+
* Force the SVG *code* (text) view, the subject of these tests. `svg_view_mode`
35+
* is a server-persisted preference (see `app.tsx`), and the suite shares one
36+
* demo server — so an earlier spec that toggled an SVG to the Rendered view
37+
* (e.g. `image-diff.test.ts`) leaves the giant SVG opening as a live `<img>`
38+
* with no `.diff-line` rows. Click Code to pin the text view regardless of what
39+
* ran before, then wait for the diff rows to (re)render.
40+
*/
41+
async function ensureCodeView(page: import('@playwright/test').Page) {
42+
const codeToggle = page.locator('[data-svg-mode="code"]');
43+
await expect(codeToggle).toBeVisible({ timeout: 5000 });
44+
await codeToggle.click();
45+
await expect(page.locator('.diff-line').first()).toBeVisible({ timeout: 5000 });
46+
}
47+
3348
/**
3449
* Install a Long Tasks observer that accumulates total main-thread blocking
3550
* time (the sum of every task longer than 50 ms — the browser only reports
@@ -57,7 +72,7 @@ test.describe('Large file performance (GB-821)', () => {
5772
await page.locator('.file-item .file-name', { hasText: SVG_FILE }).click();
5873
await expect(page.locator('.diff-view')).toHaveAttribute(
5974
'data-file-path', new RegExp(SVG_FILE), { timeout: 5000 });
60-
await expect(page.locator('.diff-line').first()).toBeVisible({ timeout: 5000 });
75+
await ensureCodeView(page);
6176

6277
// Every `.code` cell's rendered text must be bounded — the giant line is
6378
// never put into the DOM in full. (The fixture builds ~850 KB lines; after
@@ -78,6 +93,12 @@ test.describe('Large file performance (GB-821)', () => {
7893
test('selecting a large minified SVG does not freeze the main thread', async ({ page }) => {
7994
await openFileList(page);
8095

96+
// Pin the SVG code view (server-persisted; a prior spec may have left it on
97+
// Rendered) so the timed selection below opens the text diff directly and
98+
// the recorder measures code-view layout, not a live-rendered `<img>`.
99+
await page.locator('.file-item .file-name', { hasText: SVG_FILE }).click();
100+
await ensureCodeView(page);
101+
81102
// Land on a normal file first, so the recorder measures *only* the work of
82103
// selecting the large SVG — independent of which file the app auto-selects
83104
// on load.

0 commit comments

Comments
 (0)