Reproducible README demo GIF — Playwright drives, ffmpeg encodes - #520
Merged
Conversation
The demo GIF was previously hand-recorded, so refreshing it after a UI change meant redoing it by hand. This makes it a scripted artifact: `scripts/record-demo.sh` drives the real app through a scripted walkthrough, records WebM, and encodes an optimized GIF. Recording is opt-in. `playwright test` runs every project by default and --project alone can't express "exclude from the default run", so the demo project is only registered when DEMO=1 (`npm run demo:record`), with testIgnore keeping demo.spec.ts out of the chromium project. A plain e2e run stays at 3 tests and doesn't pay the walkthrough's deliberate pauses. Viewport is 1280x720 — 16:9 at 2x the README's render width, so the GIF downscale stays sharp. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0165VsVLYm3G8DnE5N9vXToS
|
Warning Review limit reached
Next review available in: 55 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds an opt-in Playwright demo that seeds an Office Audit scenario, records a complete README walkthrough, captures timeline spans, exposes Sigma for graph interaction, and encodes the resulting video into a tuned GIF. ChangesREADME demo recording
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Sequence Diagram(s)sequenceDiagram
participant Playwright as demo.spec.ts
participant Fixture as demo-fixture.ts
participant API as Backend API
participant UI as Application UI
participant Encoder as record-demo.sh
Playwright->>Fixture: Seed weekly captures and network
Fixture->>API: Upload files and attach snapshots
Playwright->>UI: Execute the README walkthrough
UI->>API: Analyze capture and load insights
Playwright->>Encoder: Write timeline and recorded video
Encoder->>Encoder: Fast-forward waits and encode GIF
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an automated demo recording walkthrough using Playwright to capture the "Office Audit" scenario and generate an optimized GIF for the README. It includes the walkthrough spec, seeding fixtures, a timeline helper to fast-forward LLM waits, and a shell script to orchestrate the recording and encoding process. The review feedback focuses on improving the portability of the shell script on macOS (addressing GNU-specific mktemp options and xargs behavior) and adding defensive assertions in the Playwright spec to prevent potential runtime TypeErrors.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+56
to
+58
| PALETTE="$(mktemp --suffix=.png)" | ||
| TRIMMED="$(mktemp --suffix=.mp4)" | ||
| trap 'rm -f "$PALETTE" "$TRIMMED"' EXIT |
There was a problem hiding this comment.
The --suffix option for mktemp is a GNU extension and is not supported on macOS (BSD mktemp), which will cause the script to fail immediately for macOS users.
To make this portable, we can use mktemp to generate a unique base path and append the extension, then clean up both the base and the extended paths in the trap handler.
Suggested change
| PALETTE="$(mktemp --suffix=.png)" | |
| TRIMMED="$(mktemp --suffix=.mp4)" | |
| trap 'rm -f "$PALETTE" "$TRIMMED"' EXIT | |
| PALETTE="$(mktemp).png" | |
| TRIMMED="$(mktemp).mp4" | |
| trap 'rm -f "${PALETTE%.png}" "$PALETTE" "${TRIMMED%.mp4}" "$TRIMMED"' EXIT |
| rm -rf "$VIDEO_DIR" | ||
| (cd "$REPO_ROOT/frontend" && npm run demo:record) | ||
|
|
||
| WEBM="$(find "$VIDEO_DIR" -name '*.webm' -type f -print0 2>/dev/null | xargs -0 ls -t 2>/dev/null | head -1 || true)" |
There was a problem hiding this comment.
If no .webm file is produced (e.g., if the Playwright run fails), xargs -0 ls -t on macOS/BSD will execute ls -t with no arguments. This lists the contents of the current working directory, setting WEBM to a directory name (like frontend). This bypasses the empty check on the next line and causes a cryptic failure in ffprobe later.
Since $VIDEO_DIR is cleared before the recording run, there will only be a single .webm file produced. We can safely simplify this to a direct find and head -1 without xargs or ls -t to make it robust and portable.
Suggested change
| WEBM="$(find "$VIDEO_DIR" -name '*.webm' -type f -print0 2>/dev/null | xargs -0 ls -t 2>/dev/null | head -1 || true)" | |
| WEBM="$(find "$VIDEO_DIR" -name '*.webm' -type f 2>/dev/null | head -1)" |
Comment on lines
+147
to
+153
| const networkId = await (async () => { | ||
| const list = await request.get('/api/v1/monitor/networks'); | ||
| const nets = await list.json(); | ||
| return (Array.isArray(nets) ? nets : (nets.data ?? [])).find( | ||
| (n: { name: string }) => n.name === NETWORK_NAME | ||
| ).id as string; | ||
| })(); |
There was a problem hiding this comment.
If the network is not found in the list, .find(...) will return undefined, causing a TypeError: Cannot read properties of undefined (reading 'id') when trying to access .id.
We should add explicit assertions to ensure the network list request succeeded and the target network exists, which will provide a much clearer error message if the setup fails.
Suggested change
| const networkId = await (async () => { | |
| const list = await request.get('/api/v1/monitor/networks'); | |
| const nets = await list.json(); | |
| return (Array.isArray(nets) ? nets : (nets.data ?? [])).find( | |
| (n: { name: string }) => n.name === NETWORK_NAME | |
| ).id as string; | |
| })(); | |
| const networkId = await (async () => { | |
| const list = await request.get('/api/v1/monitor/networks'); | |
| expect(list.ok(), `Failed to fetch networks: ${list.status()}`).toBeTruthy(); | |
| const nets = await list.json(); | |
| const networks = Array.isArray(nets) ? nets : (nets.data ?? []); | |
| const targetNetwork = networks.find((n: { name: string }) => n.name === NETWORK_NAME); | |
| expect(targetNetwork, `Network "${NETWORK_NAME}" not found`).toBeDefined(); | |
| return targetNetwork.id as string; | |
| })(); |
The recorded walkthrough stopped at the analysis tabs, so the README GIF never showed Monitor, which is half of what TracePcap does. This extends it to eleven sections: one capture's life (upload with Suricata on, the overview charts, a Telegram conversation down to its reassembled session and host-identity evidence, an LLM story, a generated filter, extracted files, topology, geography) and then eight weekly captures of a different network becoming change detection. Setup is not story, so beforeAll seeds the eight snapshots, the network, and the role labels via the API, and asserts network insights already exist — a ~20s LLM call at the end of a long recording is not worth filming, but an empty insights panel is worth failing on. The LLM waits are real and long (story generation alone ran 115s), so Timeline now races them at 40x rather than 10x: the spinner still shows, briefly, because the point is that the work is real, not that you watch it. The demo project's timeout goes to 20min to bound a hang without asserting performance — a recording that dies at minute eight wastes everything before it. Encoder settings corrected against the finished walkthrough rather than a single frame. The old comment called MAX_COLORS an expensive lever based on the topology frame's ~15k colours, but that frame is a few seconds of a 75s tour; across the whole thing 256->128 is ~40dB PSNR on the worst frames and saves 2.5MB. Per-segment cost is near-uniform, so there is no hot section to trim — the levers are global, and colour is the cheapest. Result: 6.9MB for the full product tour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Story tab is seven stacked panels and the recording scrolled past all of them with one blind scrollBy, so the tab that explains how the LLM is grounded showed none of that. It now stops on each in page order — the generation settings, full-dataset traffic intelligence, the deterministic findings the narrative is accountable to, the LLM's own investigation steps, then the narrative with its Key Events rail — and asks a suggested question in the chat, which is the interactive half of the tab and never appeared before. Monitor's snapshot modal was filmed mid-load: the tab loop skipped the Network Diagram tab it actually opens on, and the wait keyed on loading copy that Insights doesn't render (a bare Spinner), so the assertion passed instantly. It now waits for the spinner to clear *and* for Sigma to mount and paint its canvas, then holds — the blank grey panel it used to dwell on is gone, and each tab gets a real hold rather than a glance. Network Visualization gains the node-detail modal, after the hierarchical layout and before the close. Sigma renders to WebGL, so a node has no DOM element, no addressable pixel and no selector — the old hover sweep was guessing at fractions of the plot area and mostly hovering empty space. NetworkGraph now exposes its Sigma instance on window.__sigma so the recording can ask where nodes actually are; both the hover sweep and the click use it. Coordinates are read after the fit-view camera settles, because positions sampled mid-animation are stale by the time the click lands, and the click is asserted rather than best-effort so a miss fails the recording instead of silently leaving a gap. Three selectors were pointing at UI that no longer exists — the nginx image had been serving a frontend build from an older commit, so the walkthrough was written against it. "Evidence weighed" and "Inspect Hardware" are in no build (the destination badge opens the Classification popup, which is what this now films); the edge-colour <select> and the Node-to-Node Volume heatmap are gone from the product. Every remaining selector was re-verified against the running app. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rding # Conflicts: # frontend/src/components/network/NetworkGraph/NetworkGraph.tsx
Eight pacing/framing refinements to the walkthrough, plus repairs to steps the merge from main had broken. Framing: - Conversation modal re-frames after each tab's content loads. The session panel is taller with its spinner up than with text rendered, so the browser clamps scrollTop when it shrinks; a single scroll gets undone a frame later. Scroll twice with a settle between. - Topology diagram anchors its card header under the sticky navbar, re-applied after layout and fit-view since both change the card's height. The card is the graph body's grandparent — xpath=.. lands inside it and excludes the header this is meant to keep on screen. - Extracted files stops on the table instead of scrolling to footer whitespace. - Filter generator holds on the generated BPF expression before executing. - Story Q&A waits for the answer to stop streaming, not just for the spinner to clear, so the reply is not filmed half-written. Inside the fast-forward span: still waiting on the model. - Snapshot modal tabs and the IP drift modal hold long enough to read. Repairs: - Restore the edge-colour cycle (transport/application/volume). It was absent from the spec entirely. - Snapshot tabs matched nothing: decorative <i> icons pad the accessible name, so /^Changes/ never fired and the loop `continue`d past all five, filming a 2s flash of the diagram. Match loosely, scope to the pill strip, and assert rather than skip. - Conversation host details and graph node details followed markup that #575 and #578 replaced (title="Click for details" and the "Node Details" title). Anchor on the aria-label and on #entity-detail-title. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/e2e/demo-fixture.ts`:
- Around line 238-249: The unused resetNetwork helper conflicts with
seedNetwork’s intentional network reuse and would delete insights required by
assertNetworkInsights. Remove resetNetwork and its clean-slate docstring from
the demo fixture, leaving the existing seeding and assertion flow unchanged.
In `@frontend/e2e/demo-timeline.ts`:
- Line 34: Add a shared positiveEnvNumber(name, fallback) helper in
demo-timeline.ts that parses environment overrides and rejects non-finite or
non-positive values, then use it for DEMO_FF_SPEED before assigning the span
speed. Import and use the same helper in demo.spec.ts for DEMO_PACE before
passing the value to ms(); update both frontend/e2e/demo-timeline.ts:34-34 and
frontend/e2e/demo.spec.ts:36-37, preserving the existing fallback values.
- Line 34: Validate the DEMO_FF_SPEED value when initializing SPEED in
frontend/e2e/demo-timeline.ts: reject non-numeric, zero, and negative values
with a clear fail-fast error, while preserving the default when the variable is
unset. Update the nearby wait-speed comment to reflect the current default
multiplier of 40x.
In `@frontend/e2e/demo.spec.ts`:
- Around line 544-549: Update the Execute Filter fast-forward span around
showClick and timeline.fastForward so it waits for an actual post-click state
transition, such as the button becoming disabled before re-enabled, or the
results heading completion signal used later in the test. Ensure the span cannot
resolve against the button’s pre-click enabled state.
- Line 162: Update the point-generation bounds in the demo test to derive the
maximum x and y values from the live Playwright viewport, preserving the
existing 20px inset on every edge. Remove the hardcoded 1260/700 limits so
generated points and the subsequent click remain within the actual viewport when
DEMO_SIZE changes.
- Around line 310-315: Replace the specific 192.168.1.77 assertion in the
firstRow verification with a generic assertion that validates the rendered row
contains an address-shaped value, without depending on Telegram-filtered
ordering or a particular host.
- Around line 917-918: Ensure the timeline is written regardless of walkthrough
success by moving timeline.write() from the success-only path into an afterAll
hook or try/finally surrounding the walkthrough in the demo test. Preserve all
spans accumulated before any failure so demo-timeline.json is still generated.
In `@frontend/e2e/README.md`:
- Around line 71-75: Update the recording guidance in the README to match the
current defaults and size estimate used by scripts/record-demo.sh: document
DEMO_WIDTH as 640, DEMO_FPS as 6, DEMO_COLORS as 128, and approximately 7MB.
Keep the existing pacing and file-size guidance unchanged.
In `@frontend/src/components/network/NetworkGraph/NetworkGraph.tsx`:
- Around line 687-694: Gate the __sigma assignment in the NetworkGraph setup
with import.meta.env.DEV, update the adjacent test-seam comment to reference
framedGraphToViewport, and make teardown delete window.__sigma only when it
still references the current sigma instance. Use the existing setup and cleanup
paths around the sigma assignment and unmount cleanup.
In `@scripts/build-ff-filter.mjs`:
- Around line 35-41: Update the loop over sorted spans so spans whose end is at
or before the current cursor are skipped entirely, preventing fully covered
spans from being appended. For partially overlapping spans, clamp the emitted
start to cursor and update cursor monotonically using the maximum of its current
value and span.end.
In `@scripts/record-demo.sh`:
- Around line 17-18: Update the BASE_URL configuration in the recording script
to prevent arbitrary remote hosts: remove the E2E_BASE_URL override and use the
local loopback demo endpoint exclusively, or validate the override and reject
any non-loopback URL before curl is invoked. Apply the same restriction to the
related recording flow covering the referenced lines.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3095408e-f9b0-4c99-9ff7-1d6d908799d9
⛔ Files ignored due to path filters (1)
sample-files/TracePcap-Demo.gifis excluded by!**/*.gif
📒 Files selected for processing (9)
frontend/e2e/README.mdfrontend/e2e/demo-fixture.tsfrontend/e2e/demo-timeline.tsfrontend/e2e/demo.spec.tsfrontend/package.jsonfrontend/playwright.config.tsfrontend/src/components/network/NetworkGraph/NetworkGraph.tsxscripts/build-ff-filter.mjsscripts/record-demo.sh
| * The wait is still shown, just briefly: the point is that the work is real, not | ||
| * that you watch it. Override with DEMO_FF_SPEED to slow it back down. | ||
| */ | ||
| const SPEED = Number(process.env.DEMO_FF_SPEED ?? 40); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Numeric demo env overrides are coerced with bare Number() and never validated. Both tuning knobs accept any string; a typo yields NaN and 0 yields a degenerate value, and neither is caught — the recording just comes out wrong with no error.
frontend/e2e/demo-timeline.ts#L34-L34: reject non-finite or non-positiveDEMO_FF_SPEEDbefore it reaches the spanspeedfield, which is serialised tonulland handed toscripts/build-ff-filter.mjs.frontend/e2e/demo.spec.ts#L36-L37: reject non-finite or non-positiveDEMO_PACEbeforems()turns every hold intoNaN(no pacing at all) orInfinity(hangs to the 20-minute timeout).
A shared positiveEnvNumber(name, fallback) helper in demo-timeline.ts, imported by the spec, covers both.
📍 Affects 2 files
frontend/e2e/demo-timeline.ts#L34-L34(this comment)frontend/e2e/demo.spec.ts#L36-L37
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/e2e/demo-timeline.ts` at line 34, Add a shared
positiveEnvNumber(name, fallback) helper in demo-timeline.ts that parses
environment overrides and rejects non-finite or non-positive values, then use it
for DEMO_FF_SPEED before assigning the span speed. Import and use the same
helper in demo.spec.ts for DEMO_PACE before passing the value to ms(); update
both frontend/e2e/demo-timeline.ts:34-34 and frontend/e2e/demo.spec.ts:36-37,
preserving the existing fallback values.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Guard DEMO_FF_SPEED against non-numeric and zero values.
A malformed override makes SPEED NaN, which JSON.stringify serialises as null at line 66 and hands to scripts/build-ff-filter.mjs as the span speed; DEMO_FF_SPEED=0 is equally bad for a playback multiplier. Fail fast instead of producing a corrupt timeline that only surfaces as a broken ffmpeg filter.
Separately, the comment above still says the waits run at 10x while the default is now 40.
♻️ Proposed guard
-const SPEED = Number(process.env.DEMO_FF_SPEED ?? 40);
+const SPEED = Number(process.env.DEMO_FF_SPEED ?? 40);
+if (!Number.isFinite(SPEED) || SPEED <= 0) {
+ throw new Error(`DEMO_FF_SPEED must be a positive number, got "${process.env.DEMO_FF_SPEED}"`);
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const SPEED = Number(process.env.DEMO_FF_SPEED ?? 40); | |
| const SPEED = Number(process.env.DEMO_FF_SPEED ?? 40); | |
| if (!Number.isFinite(SPEED) || SPEED <= 0) { | |
| throw new Error(`DEMO_FF_SPEED must be a positive number, got "${process.env.DEMO_FF_SPEED}"`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/e2e/demo-timeline.ts` at line 34, Validate the DEMO_FF_SPEED value
when initializing SPEED in frontend/e2e/demo-timeline.ts: reject non-numeric,
zero, and negative values with a clear fail-fast error, while preserving the
default when the variable is unset. Update the nearby wait-speed comment to
reflect the current default multiplier of 40x.
Comment on lines
+310
to
+315
| // First row of the filtered set. Targeted positionally, not by address: the | ||
| // demo capture's default sort order is data, not contract, and hardcoding | ||
| // "91.108.16.1:527" broke the moment the ordering shifted. | ||
| const firstRow = page.locator('tbody tr').first(); | ||
| await expect(firstRow).toBeVisible({ timeout: 15_000 }); | ||
| await expect(firstRow).toContainText('192.168.1.77'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The 192.168.1.77 assertion reintroduces the fragility the comment above warns about.
The row is targeted positionally precisely because "the demo capture's default sort order is data, not contract", but line 315 then hard-asserts a specific address from that same unordered set. Any change to the Telegram-filtered ordering breaks the recording exactly the way the hardcoded 91.108.16.1:527 did. If the intent is only "the row rendered and has an address", assert a generic shape instead.
♻️ Assert shape rather than a specific host
- await expect(firstRow).toContainText('192.168.1.77');
+ // Shape, not a specific host — the ordering of this filtered set is data, not contract.
+ await expect(firstRow).toContainText(/\d+\.\d+\.\d+\.\d+/);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // First row of the filtered set. Targeted positionally, not by address: the | |
| // demo capture's default sort order is data, not contract, and hardcoding | |
| // "91.108.16.1:527" broke the moment the ordering shifted. | |
| const firstRow = page.locator('tbody tr').first(); | |
| await expect(firstRow).toBeVisible({ timeout: 15_000 }); | |
| await expect(firstRow).toContainText('192.168.1.77'); | |
| // First row of the filtered set. Targeted positionally, not by address: the | |
| // demo capture's default sort order is data, not contract, and hardcoding | |
| // "91.108.16.1:527" broke the moment the ordering shifted. | |
| const firstRow = page.locator('tbody tr').first(); | |
| await expect(firstRow).toBeVisible({ timeout: 15_000 }); | |
| // Shape, not a specific host — the ordering of this filtered set is data, not contract. | |
| await expect(firstRow).toContainText(/\d+\.\d+\.\d+\.\d+/); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/e2e/demo.spec.ts` around lines 310 - 315, Replace the specific
192.168.1.77 assertion in the firstRow verification with a generic assertion
that validates the rendered row contains an address-shaped value, without
depending on Telegram-filtered ordering or a particular host.
Comment on lines
+917
to
+918
| timeline.write(); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Write the timeline even when the walkthrough fails.
timeline.write() only runs on the success path. Any failure late in this ~10-minute test leaves Playwright's video on disk with no demo-timeline.json, so scripts/record-demo.sh has no dead-time spans to fast-forward and every LLM wait plays at 1x. Move the write into an afterAll (or try/finally) so the spans accumulated before the failure survive.
♻️ Write unconditionally
- timeline.write();
});
+
+test.afterAll(() => {
+ timeline.write();
+});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| timeline.write(); | |
| }); | |
| }); | |
| test.afterAll(() => { | |
| timeline.write(); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/e2e/demo.spec.ts` around lines 917 - 918, Ensure the timeline is
written regardless of walkthrough success by moving timeline.write() from the
success-only path into an afterAll hook or try/finally surrounding the
walkthrough in the demo test. Preserve all spans accumulated before any failure
so demo-timeline.json is still generated.
Comment on lines
+71
to
+75
| Tuning knobs on `record-demo.sh`: `DEMO_WIDTH` (default 800), `DEMO_FPS` (10), | ||
| `DEMO_COLORS` (96) — these produce ~3MB, roughly GitHub's comfortable ceiling. | ||
| Pacing lives in the `BEAT`/`READ` constants in the spec, and it dominates size: | ||
| the GIF costs ~100KB per second of runtime, so trimming holds beats tuning the | ||
| encoder. Raising width/fps/colors grows the file fast — check the printed size. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the documented encoding defaults.
Lines 71-75 describe 800/10/96 and ~3MB, but scripts/record-demo.sh currently defaults to 640/6/128 and estimates ~7MB. Align the README with the script so recordings are reproducible.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/e2e/README.md` around lines 71 - 75, Update the recording guidance
in the README to match the current defaults and size estimate used by
scripts/record-demo.sh: document DEMO_WIDTH as 640, DEMO_FPS as 6, DEMO_COLORS
as 128, and approximately 7MB. Keep the existing pacing and file-size guidance
unchanged.
Comment on lines
+687
to
+694
| // Test seam for the README demo recording (frontend/e2e/demo.spec.ts). | ||
| // | ||
| // The graph is WebGL, so a node has no DOM element and no addressable pixel: | ||
| // there is nothing for a test to hover or click by selector, and Sigma does | ||
| // its hit-testing internally. Exposing the instance lets the recording ask | ||
| // where a node actually is on screen (viewportForNode) and click that point, | ||
| // instead of guessing coordinates and silently filming a miss. | ||
| (window as unknown as { __sigma?: Sigma }).__sigma = sigma; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Gate the seam, and make the teardown instance-aware.
Two things on this global:
- It ships in production builds. Wrap it in
import.meta.env.DEV(or a dedicated flag) so the live Sigma instance and its full graph aren't published onwindowfor every user. - The cleanup at line 920 deletes
window.__sigmaunconditionally. When twoNetworkGraphs are mounted at once — the page topology plus the snapshot-modal diagram the demo opens atfrontend/e2e/demo.spec.tsline 826 — the modal's unmount wipes the global while the page graph is still alive, andgraphNodePointsthen silently returns[]. Only delete if the global still points at this instance.
Also, the comment on line 692 names viewportForNode, but the consumer (SigmaLike, frontend/e2e/demo.spec.ts lines 53-58) uses framedGraphToViewport.
♻️ Gated assignment + instance-checked cleanup
- (window as unknown as { __sigma?: Sigma }).__sigma = sigma;
+ if (import.meta.env.DEV) {
+ (window as unknown as { __sigma?: Sigma }).__sigma = sigma;
+ }And in the teardown at line 920:
- delete (window as unknown as { __sigma?: Sigma }).__sigma;
+ const w = window as unknown as { __sigma?: Sigma };
+ if (w.__sigma === sigma) delete w.__sigma;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/components/network/NetworkGraph/NetworkGraph.tsx` around lines
687 - 694, Gate the __sigma assignment in the NetworkGraph setup with
import.meta.env.DEV, update the adjacent test-seam comment to reference
framedGraphToViewport, and make teardown delete window.__sigma only when it
still references the current sigma instance. Use the existing setup and cleanup
paths around the sigma assignment and unmount cleanup.
Comment on lines
+35
to
+41
| for (const span of sorted) { | ||
| // Overlapping/rounding artefacts would produce a negative-length trim. | ||
| if (span.start > cursor + 0.05) { | ||
| parts.push({ from: cursor, to: span.start, speed: 1 }); | ||
| } | ||
| parts.push({ from: Math.max(span.start, cursor), to: span.end, speed: span.speed }); | ||
| cursor = span.end; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Skip spans already covered by an earlier span.
Line 40 only clamps from. For spans 10-20 then 15-18, it emits trim=20.000:18.000, which makes ffmpeg fail. Do not append fully-covered spans, and keep cursor monotonic.
Proposed fix
for (const span of sorted) {
// Overlapping/rounding artefacts would produce a negative-length trim.
if (span.start > cursor + 0.05) {
parts.push({ from: cursor, to: span.start, speed: 1 });
}
- parts.push({ from: Math.max(span.start, cursor), to: span.end, speed: span.speed });
- cursor = span.end;
+ const from = Math.max(span.start, cursor);
+ if (span.end > from + 0.05) {
+ parts.push({ from, to: span.end, speed: span.speed });
+ }
+ cursor = Math.max(cursor, span.end);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for (const span of sorted) { | |
| // Overlapping/rounding artefacts would produce a negative-length trim. | |
| if (span.start > cursor + 0.05) { | |
| parts.push({ from: cursor, to: span.start, speed: 1 }); | |
| } | |
| parts.push({ from: Math.max(span.start, cursor), to: span.end, speed: span.speed }); | |
| cursor = span.end; | |
| for (const span of sorted) { | |
| // Overlapping/rounding artefacts would produce a negative-length trim. | |
| if (span.start > cursor + 0.05) { | |
| parts.push({ from: cursor, to: span.start, speed: 1 }); | |
| } | |
| const from = Math.max(span.start, cursor); | |
| if (span.end > from + 0.05) { | |
| parts.push({ from, to: span.end, speed: span.speed }); | |
| } | |
| cursor = Math.max(cursor, span.end); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/build-ff-filter.mjs` around lines 35 - 41, Update the loop over
sorted spans so spans whose end is at or before the current cursor are skipped
entirely, preventing fully covered spans from being appended. For partially
overlapping spans, clamp the emitted start to cursor and update cursor
monotonically using the maximum of its current value and span.end.
Comment on lines
+17
to
+18
| BASE_URL="${E2E_BASE_URL:-http://localhost:8888}" | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Do not permit recording against arbitrary remote hosts.
E2E_BASE_URL is passed directly to curl, so this workflow can make external API calls. The demo only requires the local stack; remove the remote override or reject non-loopback URLs.
As per coding guidelines, “The application must function fully offline at runtime and must not make external API calls.”
Also applies to: 47-58
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/record-demo.sh` around lines 17 - 18, Update the BASE_URL
configuration in the recording script to prevent arbitrary remote hosts: remove
the E2E_BASE_URL override and use the local loopback demo endpoint exclusively,
or validate the override and reject any non-loopback URL before curl is invoked.
Apply the same restriction to the related recording flow covering the referenced
lines.
Source: Coding guidelines
The merge from main brought in #575/#578, which collapsed the conversation device-type badge and the graph's "Node Details" modal into the shared EntityDetailModal. Both selectors were dead: the recording failed outright on the badge, and would have failed on the node modal a section later. Re-anchored on the host card's aria-label and on #entity-detail-title, since the new modal's visible title is the host's own display name. The edge-colour cycle had been lost entirely — a `select` filtered by hasText matches against its options, so it silently found nothing and skipped all three modes. Anchored on the option values instead, and asserted rather than guarded, because the silent skip is what hid it. Framing fixes, each measured against the running app rather than guessed: - Conversation packet/session tabs: the session panel keeps growing after its payload first renders, so scrollHeight is stale at that moment and scrolling to it stops ~54px short. Anchor on the tab strip's own offset instead. - Topology: the card is the graph body's grandparent, not its parent, so the previous anchor excluded the very header it was meant to keep on screen. It also needs re-framing after layout and fit-view, both of which resize the card. - Story Q&A: "Thinking..." disappearing means the first token landed, not the last — wait for the reply to stop growing, inside the fast-forward span so the wait still races. - Filter generator: hold on the generated BPF expression before executing it. - Extracted files: stop on the table rather than scrolling on to page bottom. - Snapshot tabs and the IP drift modal: hold long enough to read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Restructures the Story section to follow the page's own order: the chat card
first (click a suggested question, wait for the full answer, hold on it), then
each panel in turn with its header at the top of the screen.
The framing needed a new helper. reveal() centres its target, which for a card
taller than the fold pushes the header off the top entirely — so the recording
showed panel bodies with no indication of which panel they belonged to.
frameCardTop anchors the card's top edge just under the sticky navbar instead,
and it takes the card rather than the heading, since scrolling the heading
itself clips the card's own header padding.
The real cause of the misses, though, was that html { scroll-behavior: smooth }
is set globally: every scroll animates for ~1s whether or not the call asks it
to, and the fixed 120–200ms waits were sampling mid-flight. Two measurements
that looked like "the card cannot reach the top" (page-bottom clamping, cards
taller than the viewport) were both this. settleScroll polls scrollY until it
stops — three consecutive equal samples, because a smooth scroll eases in and
out and two reads 80ms apart can match while it is still moving. reveal(),
scrollBy() and frameCardTop() all wait on it now, which also fixes holds
elsewhere in the tour that were starting before their scroll had landed.
Also sets MAX_COLORS to 256: the colour-coded views (edge-colour modes, the
volume gradient, the country choropleth) are the feature being demonstrated, so
fidelity is worth the bytes. DEMO_COLORS=128 restores the smaller encode.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ation
The Story tab felt slow, but the holds were not the cause: of ~26s on that tab,
only 8.2s was deliberate pauses and ~17.6s was scroll animation. The app sets
html { scroll-behavior: smooth } globally, so every jump animates for ~1s.
disableSmoothScroll turns that off for the recording. It is also the single
biggest size lever found so far — a pan repaints the whole frame on every frame,
which is the worst case for GIF inter-frame compression. Combined with the
trims below: 112s/11.6MB -> 91s/7.1MB, at 256 colours either way.
Pauses are now one constant. Per-step timings had drifted to fifteen different
values across sixty-odd beat() calls, which reads as arbitrary rather than
deliberate; HOLD replaces them all, with READ_HOLD (2x) kept for the one thing
that has to be read rather than recognised — the LLM's answer. DEMO_HOLD retimes
the whole demo in one number.
Framing, all anchored so the section header sits at the top of the screen:
- Overview opens on the Analysis Summary, and both distribution charts keep
their headers in view (previously centred, which pushed the header off-screen
and left the pies unlabelled).
- Network Intelligence frames the Network Cluster View header before switching
Group by to Country.
- Network Visualization ends on the node-to-node volume heatmap, expanded from
its collapsed default.
Also drops the fullscreen topology step, and ends the Monitor section on the
Network Insights components rather than scrolling on to the page footer.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…eader The pauses were not uniform, despite every beat() call passing HOLD. Six raw waitForTimeout calls sat immediately before a beat() — in openTab, gotoSection, the filter results, the edge-colour cycle and two monitor modals — so those stops ran HOLD plus 400-1200ms. Because the padding was outside beat(), it did not show up as a different hold value; it just made some sections feel slower. Where the wait was real (a canvas paint, a tab-swap relayout) it is now a poll on the condition instead of a fixed delay, so the visible pause is one HOLD. Verified by instrumenting beat(): 50 of 51 pauses land at 600ms with a max overshoot of 4ms, the 51st being the deliberate 1200ms READ_HOLD. cardOf matched card-header. contains(@Class,"card") is a substring test, so it also matches "card-header" and "card-body" — it was returning the 57px header rather than the card. For the heatmap that meant framing against an element whose top only coincidentally matched the card's and whose height never changes when the card expands, leaving the shot ~130px low with the topology's tail still in it, and breaking a height check that assumed the real card. Now matches the class token exactly: 59px collapsed, 1237px expanded, framed at y=110. The heatmap also needed the same settle the session panel did — the grid keeps laying out after .tp-heatmap-body turns visible, so framing on visibility alone used stale geometry. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…he top
The section now runs in the order the flow actually reads: open the tab, frame
the filter panel at the top of the screen, expand it, pick the Telegram badge,
open the first conversation, inspect the destination host, return to the
conversation, then Packets and Session.
Previously the packet/session tabs came first and the host modal last, so the
section ended inside a nested modal and doubled back on itself. Framing the
filter panel before expanding it also means the panel unfolds into an empty
viewport rather than off the bottom edge.
On the Session tab "auto scroll": there is no autoscroll to remove. The only
scrollIntoView in ConversationDetail is the deep-link packet jump, which this
recording never triggers. What actually happens is a clamp — measured, the modal
body sits at scrollTop 212, clicking Session swaps in the shorter spinner panel
so the browser clamps to the new max of 180, and when the payload renders the
panel grows back and the browser restores 212. It self-corrects by ~800ms, and
the existing settle loop already waits for it.
Also fixes an assertion that would have failed for the wrong reason: hostModal
is getByRole('dialog').last(), which re-resolves to the conversation modal once
the host modal closes, so expect(hostModal).toBeHidden() was checking the wrong
element. Asserts on #entity-detail-title and the dialog count instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…step Two tiers instead of one. BEAT (600ms) for transitions — opening a filter, switching a tab, moving between panels. FEATURE (1100ms) for landing on the thing a section exists to show: a chart, a diagram, a modal, a result. 20 stops are now FEATURE, the rest BEAT, and the LLM answer keeps 2x FEATURE. Both are plain milliseconds of finished GIF, deliberately not divided by PACE. PACE shortens waits *while recording*, so what is filmed is what plays back — there is no speed-up pass on top, which meant the previous 600ms uniform hold was 0.6s on screen for every feature, not 1.5s. Also fixes a step that was never running. The generated-filter pause matched 'code, pre', but the expression renders into an editable Form.Control (#filter) — so the locator found nothing, and because it sat behind an isVisible() guard the whole step was skipped without failing. It is asserted now, and the section frames "Ask in Natural Language" at the top before typing so the prompt, the button and the generated filter stay in one shot. Extracted Files frames the Files card header and stops there rather than scrolling on. The heatmap zooms out until the whole 50x50 matrix fits: measured on the grid element, not the card, since the card keeps full width at any cell size — a height-only test overshot to 6px/cell and left half the panel empty. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The drift panels — and the whole Monitor section — were still on the 600ms BEAT. The earlier tier split promoted stops that follow frameCardTop/reveal, but the Monitor page navigates with gotoSection, so none of its landings matched: six drift panel stops and the closing Network Insights all cut away in 0.6s. Every gotoSection landing is now FEATURE, and the shadow-host modal (192.0.2.99) gets READ_HOLD — it is the payoff of the drift section and the panel is dense enough to need reading, not just recognising. Sweeping the rest of the file the same way found eleven more feature landings still on BEAT: the node-label modal and its preview, the fitted hierarchical layout, the node-details identity panel, the country map switch, the Italy and Pistoia drill-downs, the conversation packet/session framing, the evidence panel, the hardware axis, and the matched packets. Final split: 37 FEATURE, 24 BEAT, 2 READ_HOLD. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Replaces the old Monitor-only recording with the eleven-section tour, encoded at 640px/6fps/128c — 5.4MB for 118s. 128 rather than 256: measured on this walkthrough it costs ~40dB PSNR on the worst frames (topology, world map, pie charts), which leaves the colour-coded views reading correctly, and saves ~2MB. The tuning comment now carries the measured ladder for this recording rather than the older one's numbers. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ewport Two findings from reviewing PR #520. resetNetwork was exported but never called, and it contradicts seedNetwork's documented reuse: deleting the network would discard the operator-generated insights the recording films in step 11, which assertNetworkInsights then fails loudly about. Removing it rather than wiring it up. The heatmap zoom loop compared against a literal 720. The recording viewport is defined in playwright.config.ts, so the two could drift apart silently and the matrix would stop fitting. Reads page.viewportSize() instead — same result (stops at 8px/cell, 408px grid), no coupling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/record-demo.sh (2)
79-82: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winA non-numeric
ffprobeduration silently disables the whole fast-forward pass.Chromium's Playwright
.webmoutput routinely carries no duration in the container header, soffprobe -show_entries format=durationreturnsN/Aor an empty string.build-ff-filter.mjsthen hits itsNumber.isFinite(duration)guard andprocess.exit(0),FF_FILTERis empty, and the script encodes the full un-raced source with no warning — the LLM waits play at 1x and the GIF blows past the size budget for reasons the operator cannot see.Fall back to a decode-based probe and fail loudly if neither works.
🛠️ Fall back to a packet-level duration and warn
TIMELINE="$VIDEO_DIR/demo-timeline.json" DURATION="$(ffprobe -v error -show_entries format=duration -of csv=p=0 "$WEBM")" +case "$DURATION" in + ''|N/A) + # Chromium's webm often has no duration in the container header. + DURATION="$(ffprobe -v error -select_streams v:0 -count_packets \ + -show_entries stream=duration -of csv=p=0 "$WEBM")" + ;; +esac +case "$DURATION" in + ''|N/A) echo "warning: could not determine duration of $WEBM — encoding without fast-forward." >&2 ;; +esac FF_FILTER="$(node "$REPO_ROOT/scripts/build-ff-filter.mjs" "$TIMELINE" "$DURATION" || true)" +if [ -z "$FF_FILTER" ] && [ -s "$TIMELINE" ]; then + echo "warning: $TIMELINE exists but produced no fast-forward filter." >&2 +fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/record-demo.sh` around lines 79 - 82, The duration handling around DURATION and build-ff-filter.mjs must not silently disable fast-forwarding when ffprobe returns N/A or an empty value. Validate that the format duration is numeric, fall back to a decode/packet-level ffprobe duration probe when it is not, and emit a clear warning or failure if neither probe yields a usable duration before the encoding path continues.
90-91: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe intermediate
.mp4is a lossy generation the palette then has to model.
ffmpeg ... "$TRIMMED"with an.mp4suffix picks libx264 at default CRF, sopalettegenat Line 109 is sampling x264 artefacts rather than the source frames — which works directly against the colour-accuracy tuning documented at Lines 102-107. Use a visually-lossless intermediate; it is a temp file, so size does not matter.♻️ Near-lossless intermediate
-TRIMMED="$(mktemp --suffix=.mp4)" +TRIMMED="$(mktemp --suffix=.mkv)" @@ - ffmpeg -v error -y -i "$WEBM" -filter_complex "$FF_FILTER" -map "[ff]" "$TRIMMED" + # Lossless intermediate: palettegen below must see the source colours, not + # x264's approximation of them. + ffmpeg -v error -y -i "$WEBM" -filter_complex "$FF_FILTER" -map "[ff]" \ + -c:v ffv1 -level 3 "$TRIMMED"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/record-demo.sh` around lines 90 - 91, Update the intermediate conversion in the recording flow before palette generation to use a visually lossless codec and explicitly lossless-quality settings instead of the default lossy `.mp4` encoding. Keep the existing filtering, output assignment, and subsequent palettegen flow unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/e2e/demo.spec.ts`:
- Around line 1054-1066: Update the polling callback in the canvas readiness
check to inspect rendered pixel data rather than multiplying the canvas
dimensions. Use the existing canvas element to obtain image data and return a
value indicating whether any pixel is non-transparent, while preserving the
current timeout and polling interval and requiring a painted pixel before
continuing.
- Around line 617-633: Update the Story Q&A wait flow around chatCard and the
“Thinking...” locator so it first waits for the thinking bubble to become
visible or otherwise appear, then waits for it to disappear before starting the
response-growth polling. Ensure the fast-forward span cannot resolve while the
answer has not started, while preserving the existing completion wait and
timeout behavior.
- Around line 186-190: Update the framing helpers around cardOf and frameCardTop
to centralize the missing-card fallback in a new frameEnclosingCard helper:
return the enclosing card when present, otherwise use the appropriate centered
fallback element. Replace the inline card count guard at the first call site and
use frameEnclosingCard for prompt, generated, and filesHeading at the other
three call sites.
- Around line 941-944: Update the groupBy locator in the group-by test to
identify the select by its option values or other stable select-specific
attributes instead of filtering with hasText on the select element. Preserve the
visibility assertion and selectOption('country') behavior; retain showClick only
if the pointer interaction is intentional.
---
Outside diff comments:
In `@scripts/record-demo.sh`:
- Around line 79-82: The duration handling around DURATION and
build-ff-filter.mjs must not silently disable fast-forwarding when ffprobe
returns N/A or an empty value. Validate that the format duration is numeric,
fall back to a decode/packet-level ffprobe duration probe when it is not, and
emit a clear warning or failure if neither probe yields a usable duration before
the encoding path continues.
- Around line 90-91: Update the intermediate conversion in the recording flow
before palette generation to use a visually lossless codec and explicitly
lossless-quality settings instead of the default lossy `.mp4` encoding. Keep the
existing filtering, output assignment, and subsequent palettegen flow unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9e7878cc-6be9-457a-b92f-2f73e02281df
⛔ Files ignored due to path filters (1)
sample-files/TracePcap-Demo.gifis excluded by!**/*.gif
📒 Files selected for processing (2)
frontend/e2e/demo.spec.tsscripts/record-demo.sh
Comment on lines
+186
to
+190
| function cardOf(target: Locator) { | ||
| return target.locator( | ||
| 'xpath=ancestor::div[contains(concat(" ", normalize-space(@class), " "), " card ")][1]', | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
cardOf() has no fallback at three of its four call sites.
Line 669 guards with (await card.count()) ? card.first() : panelHeading, but Lines 690, 717 and 756 pass cardOf(...) straight into frameCardTop. If the enclosing .card wrapper disappears in a UI refactor, target.evaluate waits out the action timeout and kills a ~10-minute recording instead of degrading to a centred element. Fold the fallback into a single helper.
♻️ Single framing helper with the fallback built in
function cardOf(target: Locator) {
return target.locator(
'xpath=ancestor::div[contains(concat(" ", normalize-space(`@class`), " "), " card ")][1]',
);
}
+
+/** frameCardTop on the enclosing .card, falling back to the element itself. */
+async function frameEnclosingCard(target: Locator) {
+ const card = cardOf(target);
+ await frameCardTop((await card.count()) ? card.first() : target);
+}Then use frameEnclosingCard(prompt) / frameEnclosingCard(generated) / frameEnclosingCard(filesHeading) at Lines 690, 717 and 756, and replace the inline guard at Line 669.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/e2e/demo.spec.ts` around lines 186 - 190, Update the framing helpers
around cardOf and frameCardTop to centralize the missing-card fallback in a new
frameEnclosingCard helper: return the enclosing card when present, otherwise use
the appropriate centered fallback element. Replace the inline card count guard
at the first call site and use frameEnclosingCard for prompt, generated, and
filesHeading at the other three call sites.
Comment on lines
+617
to
+633
| await beat(page, HOLD); | ||
| await timeline.fastForward('Story Q&A (LLM)', 2, async () => { | ||
| await expect(chatCard.getByText(/Thinking\.\.\./)).toBeHidden({ timeout: 300_000 }); | ||
|
|
||
| // "Thinking..." disappearing means the first token landed, not that the | ||
| // answer is done — the reply streams in, so re-framing on that signal filmed | ||
| // a half-written sentence scrolling away. Hold until the text stops growing | ||
| // (two equal samples). Inside the fast-forward span: this is still waiting | ||
| // on the model, so it should race like the rest of it. | ||
| let previous = -1; | ||
| for (let i = 0; i < 40; i++) { | ||
| const length = (await chatCard.innerText()).length; | ||
| if (length === previous) break; | ||
| previous = length; | ||
| await page.waitForTimeout(500); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The Story Q&A span can resolve before the model ever starts.
toBeHidden passes for an element that does not exist yet, so if the "Thinking..." bubble hasn't mounted within the 600ms beat, Line 619 returns immediately. The growth poll behind it does not save you either: two consecutive equal innerText lengths on a card with no answer yet breaks out on the second sample (500ms). The span then measures ~0s, frameCardTop + READ_HOLD film an empty card, and the reply streams in afterwards at 1x — the opposite of the intent.
Wait for the bubble to appear first, then for it to go.
♻️ Gate on the bubble appearing
await beat(page, HOLD);
+ const thinking = chatCard.getByText(/Thinking\.\.\./);
await timeline.fastForward('Story Q&A (LLM)', 2, async () => {
- await expect(chatCard.getByText(/Thinking\.\.\./)).toBeHidden({ timeout: 300_000 });
+ // Must appear before it can meaningfully disappear — otherwise toBeHidden
+ // matches the pre-request state and the span measures zero.
+ await expect(thinking, 'the LLM request never started').toBeVisible({ timeout: 30_000 });
+ await expect(thinking).toBeHidden({ timeout: 300_000 });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await beat(page, HOLD); | |
| await timeline.fastForward('Story Q&A (LLM)', 2, async () => { | |
| await expect(chatCard.getByText(/Thinking\.\.\./)).toBeHidden({ timeout: 300_000 }); | |
| // "Thinking..." disappearing means the first token landed, not that the | |
| // answer is done — the reply streams in, so re-framing on that signal filmed | |
| // a half-written sentence scrolling away. Hold until the text stops growing | |
| // (two equal samples). Inside the fast-forward span: this is still waiting | |
| // on the model, so it should race like the rest of it. | |
| let previous = -1; | |
| for (let i = 0; i < 40; i++) { | |
| const length = (await chatCard.innerText()).length; | |
| if (length === previous) break; | |
| previous = length; | |
| await page.waitForTimeout(500); | |
| } | |
| }); | |
| await beat(page, HOLD); | |
| const thinking = chatCard.getByText(/Thinking\.\.\./); | |
| await timeline.fastForward('Story Q&A (LLM)', 2, async () => { | |
| // Must appear before it can meaningfully disappear — otherwise toBeHidden | |
| // matches the pre-request state and the span measures zero. | |
| await expect(thinking, 'the LLM request never started').toBeVisible({ timeout: 30_000 }); | |
| await expect(thinking).toBeHidden({ timeout: 300_000 }); | |
| // "Thinking..." disappearing means the first token landed, not that the | |
| // answer is done — the reply streams in, so re-framing on that signal filmed | |
| // a half-written sentence scrolling away. Hold until the text stops growing | |
| // (two equal samples). Inside the fast-forward span: this is still waiting | |
| // on the model, so it should race like the rest of it. | |
| let previous = -1; | |
| for (let i = 0; i < 40; i++) { | |
| const length = (await chatCard.innerText()).length; | |
| if (length === previous) break; | |
| previous = length; | |
| await page.waitForTimeout(500); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/e2e/demo.spec.ts` around lines 617 - 633, Update the Story Q&A wait
flow around chatCard and the “Thinking...” locator so it first waits for the
thinking bubble to become visible or otherwise appear, then waits for it to
disappear before starting the response-growth polling. Ensure the fast-forward
span cannot resolve while the answer has not started, while preserving the
existing completion wait and timeout behavior.
Comment on lines
+941
to
+944
| const groupBy = page.locator('select').filter({ hasText: /ASN \/ Organization/ }).first(); | ||
| await expect(groupBy, 'group-by control not found').toBeVisible({ timeout: 20_000 }); | ||
| await showClick(page, groupBy); | ||
| await groupBy.selectOption('country'); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
This is the same hasText-on-<select> pattern the edge-colour comment calls unreliable.
Lines 799-802 document that filtering a <select> by hasText "silently matched nothing" and skipped a whole section. Here it is used again for the group-by control. The toBeVisible assertion at Line 942 at least makes a miss loud, but anchor on the option values for consistency with the fix above.
Also, showClick on a <select> at Line 943 opens the native dropdown; selectOption then works regardless, so the click only adds a pointer flourish — keep it if that is intentional.
♻️ Anchor on option values
- const groupBy = page.locator('select').filter({ hasText: /ASN \/ Organization/ }).first();
+ const groupBy = page.locator('select:has(option[value="country"])').first();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const groupBy = page.locator('select').filter({ hasText: /ASN \/ Organization/ }).first(); | |
| await expect(groupBy, 'group-by control not found').toBeVisible({ timeout: 20_000 }); | |
| await showClick(page, groupBy); | |
| await groupBy.selectOption('country'); | |
| const groupBy = page.locator('select:has(option[value="country"])').first(); | |
| await expect(groupBy, 'group-by control not found').toBeVisible({ timeout: 20_000 }); | |
| await showClick(page, groupBy); | |
| await groupBy.selectOption('country'); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/e2e/demo.spec.ts` around lines 941 - 944, Update the groupBy locator
in the group-by test to identify the select by its option values or other stable
select-specific attributes instead of filtering with hasText on the select
element. Preserve the visibility assertion and selectOption('country') behavior;
retain showClick only if the pointer interaction is intentional.
Comment on lines
+1054
to
+1066
| // still be the blank canvas it mounted with. Waiting on the paint is not a | ||
| // pause — poll for it so this stop lasts one HOLD like every other, instead of | ||
| // HOLD plus a fixed 1.2s. | ||
| await expect | ||
| .poll( | ||
| () => | ||
| page.evaluate(() => { | ||
| const c = document.querySelector('canvas.sigma-nodes') as HTMLCanvasElement | null; | ||
| return c ? c.width * c.height : 0; | ||
| }), | ||
| { timeout: 30_000, intervals: [100] }, | ||
| ) | ||
| .toBeGreaterThan(0); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
c.width * c.height measures the canvas backing size, not that it painted.
The comment says this waits for the paint, but a Sigma canvas is sized at mount and passes this poll while still blank — exactly the case it is meant to exclude. Sample pixels instead.
♻️ Poll for non-transparent pixels
page.evaluate(() => {
const c = document.querySelector('canvas.sigma-nodes') as HTMLCanvasElement | null;
- return c ? c.width * c.height : 0;
+ if (!c || !c.width || !c.height) return 0;
+ const ctx = c.getContext('2d');
+ if (!ctx) return 0;
+ const { data } = ctx.getImageData(0, 0, c.width, c.height);
+ let painted = 0;
+ for (let i = 3; i < data.length; i += 4 * 97) if (data[i] !== 0) painted++;
+ return painted;
}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // still be the blank canvas it mounted with. Waiting on the paint is not a | |
| // pause — poll for it so this stop lasts one HOLD like every other, instead of | |
| // HOLD plus a fixed 1.2s. | |
| await expect | |
| .poll( | |
| () => | |
| page.evaluate(() => { | |
| const c = document.querySelector('canvas.sigma-nodes') as HTMLCanvasElement | null; | |
| return c ? c.width * c.height : 0; | |
| }), | |
| { timeout: 30_000, intervals: [100] }, | |
| ) | |
| .toBeGreaterThan(0); | |
| // still be the blank canvas it mounted with. Waiting on the paint is not a | |
| // pause — poll for it so this stop lasts one HOLD like every other, instead of | |
| // HOLD plus a fixed 1.2s. | |
| await expect | |
| .poll( | |
| () => | |
| page.evaluate(() => { | |
| const c = document.querySelector('canvas.sigma-nodes') as HTMLCanvasElement | null; | |
| if (!c || !c.width || !c.height) return 0; | |
| const ctx = c.getContext('2d'); | |
| if (!ctx) return 0; | |
| const { data } = ctx.getImageData(0, 0, c.width, c.height); | |
| let painted = 0; | |
| for (let i = 3; i < data.length; i += 4 * 97) if (data[i] !== 0) painted++; | |
| return painted; | |
| }), | |
| { timeout: 30_000, intervals: [100] }, | |
| ) | |
| .toBeGreaterThan(0); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/e2e/demo.spec.ts` around lines 1054 - 1066, Update the polling
callback in the canvas readiness check to inspect rendered pixel data rather
than multiplying the canvas dimensions. Use the existing canvas element to
obtain image data and return a value indicating whether any pixel is
non-transparent, while preserving the current timeout and polling interval and
requiring a painted pixel before continuing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The README demo GIF was hand-recorded, so refreshing it after a UI change meant redoing it by hand. This makes it a scripted artifact.
scripts/record-demo.shdrives the real app through a scripted walkthrough, records WebM, and encodes an optimized GIF via ffmpeg.Recording is opt-in
Playwright runs every project by default, and
--projectalone can't express "exclude from the default run". So the demo project only registers whenDEMO=1(npm run demo:record), withtestIgnorekeepingdemo.spec.tsout of the chromium project.Verified both modes:
npx playwright test --list→ 3 tests in 2 files (demo excluded, no ~40s of deliberate pauses on a normal e2e run)DEMO=1 npx playwright test --list→ 4 tests in 3 files ([demo] › demo.spec.tsregisters)npx tsc --noEmitclean.Viewport is 1280x720 — 16:9 at 2x the README's ~640px render width, so the GIF downscale stays sharp.
Prerequisites (documented in
frontend/e2e/README.md)docker compose up -d --build(cd frontend && npx playwright install chromium)ffmpegon PATH🤖 Generated with Claude Code
https://claude.ai/code/session_0165VsVLYm3G8DnE5N9vXToS
Summary by CodeRabbit
Documentation
New Features
demo:recordto create recordings on demand.Tests