Plan: UI discoverability pass (Experiments panel)#2
Merged
Conversation
Follow-up plan for the RuLake-inspired roadmap addressing the single largest UX weakness: three features (warm-restart/share, cross-tab, and the absent quantization UI) ship with zero discoverability because their UI is gated on URL flags. Proposes a single 🧪 Experiments disclosure panel consolidating all feature toggles, with URL flags demoted from UI-gate to initial-state-preset (the pattern consistency-modes and federation already use correctly). No default-on flips; plain URL still yields plain behaviour. Scope is deliberately small: ~250 lines, primarily in uiPanels.js, with a new agent-browser smoke harness. No feature module touched. See the plan for the 10-task checklist, default-state table, and design trade-offs.
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe PR introduces a new "🧪 Experiments" collapsible disclosure panel that consolidates previously URL-flag-dependent feature toggles (snapshots, cross-tab training, etc.). Changes include CSS styling for the panel component, DOM construction and event wiring logic, confirmation dialogs for destructive archive imports, preset handling for URL flags, and a smoke test harness to validate panel behavior. Changes
Sequence DiagramsequenceDiagram
actor User
participant Panel as Experiments Panel
participant DOM as DOM/State
participant Bridge as Bridge API
participant Import as Import Handler
User->>Panel: Opens experiments disclosure
Panel->>DOM: Reveal 6 toggle rows
User->>Panel: Click snapshots checkbox
Panel->>DOM: Toggle snapshots subbody visibility
DOM-->>User: Show/hide export & import controls
User->>Panel: Click crosstab checkbox
Panel->>Bridge: setCrosstabEnabled(state)
Bridge-->>Panel: Acknowledge state change
User->>Import: Click archive import button
Import->>User: Show confirm dialog (destructive warning)
alt User confirms
Import->>DOM: Load & replace archive
DOM-->>User: Archive updated
else User cancels
Import-->>User: Import aborted
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
…s panel Implements the plan from docs/plan/ui-discoverability-pass.md, in the same branch / PR #2 the plan landed in. Surfaces the previously URL-flag-gated Save/Share-archives row and Cross-tab row in a single collapsible disclosure panel, alongside the already-visible Federation and Consistency rows. What changed: - uiPanels.js — new buildExperimentsPanel() at the bottom of the IIFE. Creates a <details class="rv-experiments"> + summary + body, then re-parents the existing consistency/federation/crosstab DOM nodes into the disclosure body via appendChild. Existing event listeners and el.X references survive untouched (wrap-don't-rebuild). The snapshots row and the share row are now ALWAYS created (the if (_snapshotsFlagOn) gate is removed); the experiments-level "📦 Save & share archives" checkbox controls subbody visibility. Same pattern for the crosstab "🔗 Cross-tab live training" toggle, which now drives bridge.setCrosstabEnabled() directly. URL flags become PRESETS that pre-toggle the corresponding control; if any flag is set the disclosure auto-opens so the user can see what their share-link enabled. Default state matches the plan's table: observability ON, consistency Fresh, federation OFF, snapshots OFF, crosstab OFF, quantization DISABLED. - uiPanels.js — confirm() on destructive Import paths. Both the file-Import in the snapshots row and the URL-Import in the share row now ask "REPLACE your current N brains?" before proceeding. Previously the ?snapshots=1 flag was the only friction; with the UI unhidden, an explicit confirm replaces that. - uiPanels.js — crosstab listeners now wired unconditionally (previously gated by ?crosstab=1). The bridge state, not the listener subscription, decides whether to broadcast. - style.css — adds .rv-experiments, .rv-experiments-summary, .rv-experiments-body, .rv-experiments-row, .rv-experiments-toggle, .rv-experiments-subbody, .rv-experiments-row-disabled, .rv-experiments-badge classes. Native <details>/<summary>; no custom widget, no JS animation. Triangle marker rotates 90° on open. - tests/experiments-panel-smoke.html (new) — 7-claim iframe-based harness. Loads the main page in a hidden iframe (no flags) and a second iframe with ?snapshots=1, then asserts: 1. Disclosure exists, defaults closed. 2. 6 rows visible after expand. 3. Quantization row is disabled. 4. [Learn] eli15 badges wired on new rows. 5. Snapshots toggle gates the inner controls + both snapshots row and share row are migrated inside the subbody. 6. Crosstab toggle drives bridge.setCrosstabEnabled() (false→true). 7. ?snapshots=1 presets the toggle ON + reveals controls + opens the disclosure. - docs/plan/ui-discoverability-pass.md — A.1–A.10 status flipped to ✅ with implementation notes (wrap-don't-rebuild strategy, auto-open on flag, listener-always-wired refactor). Validated via agent-browser: - Default boot clean, no new console errors. - Smoke harness 7/7 PASS, including bridge-state coupling (crosstab checkbox flips bridge.isCrosstabEnabled() live). - ?consistency=frozen&federation=1&crosstab=1 multi-flag URL still works — every UI control reflects its flag, every bridge getter reports the active state, disclosure auto-opens. Out of scope (explicit non-changes): - No default-on flips. Federation, Cross-tab, Snapshots, Quantization all remain OFF by default. - No new URL flags. - No feature module changes (archive/, consistency/, federation/, crosstab/, quantization/, observability/, share/, lineage/ all untouched). - No quantization integration into archiveBrain — disabled row says so explicitly with a tooltip pointing at the chapter. Net diff: ~310 lines added across 4 files (close to the plan's ~250-line estimate; came in higher because the smoke harness imitates a two-iframe test runner).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
docs/plan/ui-discoverability-pass.md (1)
71-87: Fenced code block missing language hint.Markdownlint flags this block as
MD040, fenced-code-language. Since it's an ASCII mock-up,textis appropriate.📝 Proposed fix
-``` +```text 🧪 Experiments ▼🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plan/ui-discoverability-pass.md` around lines 71 - 87, Add a fenced-code language hint "text" to the ASCII mock-up code block that begins with the line "🧪 Experiments ▼" so markdownlint MD040 is satisfied; change the opening fence from ``` to ```text (leave the block contents unchanged) to mark it as plain text.tests/experiments-panel-smoke.html (1)
47-47: Potential HTML injection in test reporter (low risk, but trivial to fix).The
recordhelper interpolateslabelanddetailintoinnerHTMLdirectly. The current callers pass static strings, so this is safe today, but if a future claim'sdetailever contains user/iframe-derived content (e.g. an error message stack from the iframe), an<could break the table or worse. UsetextContentfor the cell payloads:♻️ Suggested fix
- tr.innerHTML = `<td>${claims.length}</td><td>${label}</td><td class="${ok ? 'ok' : 'fail'}">${ok ? 'PASS' : 'FAIL'}</td><td><pre>${typeof detail === 'string' ? detail : JSON.stringify(detail, null, 2)}</pre></td>`; + const idx = document.createElement('td'); idx.textContent = String(claims.length); + const lbl = document.createElement('td'); lbl.textContent = label; + const verdict = document.createElement('td'); verdict.className = ok ? 'ok' : 'fail'; verdict.textContent = ok ? 'PASS' : 'FAIL'; + const det = document.createElement('td'); + const pre = document.createElement('pre'); + pre.textContent = typeof detail === 'string' ? detail : JSON.stringify(detail, null, 2); + det.appendChild(pre); + tr.append(idx, lbl, verdict, det);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/experiments-panel-smoke.html` at line 47, The test reporter's record helper currently builds a row via tr.innerHTML (see the tr.innerHTML assignment) which interpolates label and detail directly and risks HTML injection; instead construct the cells with createElement('td') and set their textContent (for label and the detail <pre> content) and set the status cell's className to ok ? 'ok' : 'fail', then append the cells to tr so no unescaped HTML is inserted.AI-Car-Racer/uiPanels.js (3)
252-259: Duplicate URL parsing — single source of truth would be cleaner.
_snapshotsFlagOnparsesURLSearchParamsat line 252-259, thenbuildExperimentsPanel's IIFE parses it again at line 1617-1619 with its ownflag()/flagEq()helpers. Trivial duplication, but if a third caller is added later (e.g. another flag preset), it would be cleaner to compute oneurlFlagsobject up top and reference it from both places.Also applies to: 1617-1621
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AI-Car-Racer/uiPanels.js` around lines 252 - 259, Duplicate URL query parsing occurs: remove repeated parsing and create a single parsed flags object at module initialization (using URLSearchParams on window.location.search) and reference it everywhere; replace the top-level _snapshotsFlagOn IIFE with a lookup into that shared urlFlags object and update buildExperimentsPanel’s IIFE helpers (flag and flagEq) to read from the same urlFlags instead of re-parsing URLSearchParams, ensuring all consumers (including any future flag readers) use the single source of truth.
1732-1742:?crosstab=1preset relies on a 100 mssetTimeoutto race the bridge.If the bridge isn't ready at +100 ms,
applyCrosstabState(true)callsb.setCrosstabEnabledagainst an undefined or partially-loaded bridge, the try/catch swallows it, and the only thing left is the existing__applyUrlCrosstabFlagpoll inmain.js(which up-to 20×100 ms polls forsetCrosstabEnabled). That works today, but:
- The checkbox is now
checkedon the page even if the actual enable hasn't landed yet — see the desync issue raised above.- If main.js's poll path is removed in a future cleanup ("the experiments panel handles this now"), the preset will silently no-op on slow loads.
Consider replacing the
setTimeoutwith the samefor (let i = 0; i < 20 && …) await sleep(100)pattern used inensureCrosstabWiring, or at minimum keep a comment inmain.jsflagging this dependency so the poller isn't deleted accidentally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AI-Car-Racer/uiPanels.js` around lines 1732 - 1742, The current ?crosstab=1 flow sets expCrosstabCb.checked and uses a 100ms setTimeout to call applyCrosstabState(true), which can race the bridge and leave the checkbox desynced; replace the single setTimeout retry with the same polling pattern used in ensureCrosstabWiring (for loop with await sleep(100) up to ~20 attempts) to wait for the bridge before calling applyCrosstabState, referencing expCrosstabCb and applyCrosstabState to keep behavior consistent; if you prefer not to add polling here, add a clear TODO/comment in __applyUrlCrosstabFlag/main.js that this code depends on the poller so future removals won’t break the preset.
1685-1709: Add a clarifying comment explaining whycrosstabElrelies on subbody visibility control.The codebase confirms there is no other code path re-asserting
crosstabEl.hiddenafter line 1701 sets it tofalse. The old polling-based behavior that managed thehiddenattribute based on?crosstab=1gating no longer applies, so visibility is now entirely controlled byexpCrosstabSub.hidden. Since this is an intentional design change from the refactor, adding a comment alongside line 1701 explaining that subbody visibility is the sole control mechanism will help prevent future changes from mistakenly restoring the old pattern.Also confirm the consistency tick strip animation, federation viewer mount, and crosstab pulse animation all trigger correctly after the re-parenting (cached references like
el.crosstabPilldo survive the move, but a smoke-check ensures no other behavioral assumptions were broken).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AI-Car-Racer/uiPanels.js` around lines 1685 - 1709, Add a clarifying inline comment at the line setting crosstabEl.hidden = false explaining that visibility is now solely driven by expCrosstabSub.hidden (the old ?crosstab=1 polling no longer applies) so the pill itself must be left unhidden when re-parented; after appending, add a brief smoke-check that cached references survived the move and behaviors still run (e.g., ensure crosstabEl.crosstabPill exists and trigger its pulse/animation, verify the federation viewer mount runs for federationEl, and ensure the consistency tick-strip animation still triggers for consistencyEl) so any regressions from re-parenting are caught.AI-Car-Racer/style.css (1)
2121-2219: Theming inconsistency: hardcoded dark palette in a token-driven, themeable stylesheet.The new
.rv-experiments*rules use hardcoded dark navy backgrounds (rgba(20, 28, 48, …)) and light text (#cbd8ff,#e6ebfa,#8a98be) regardless ofprefers-color-scheme. Everywhere else in this file uses--surface-*/--ink-*/--ml-*tokens with light/dark variants. Since#rv-panelhas a light paper background in light mode (var(--ml-50)with a violet rim), this disclosure will appear as a dark navy island inside an otherwise warm-paper panel in light mode, and the badge/hint colors won't shift in dark mode either.For consistency with the rest of the panel, prefer the existing tokens (
--surface-2/3,--ink-700/800,--line/--line-control,--ml-100,--amber-100for the badge) and add a@media (prefers-color-scheme: dark)override only where needed. This also avoids re-introducing the WCAG 1.4.11 issues the rest of the file has been carefully tuned for.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AI-Car-Racer/style.css` around lines 2121 - 2219, Replace the hardcoded dark/navy and light text colors in the .rv-experiments rules with the site's CSS tokens (e.g., use --surface-2/--surface-3 for backgrounds, --ink-700/--ink-800 for text, --line or --line-control for borders, --ml-100 for subtle highlights and --amber-100 for .rv-experiments-badge) and remove literal rgba/# values in selectors such as .rv-experiments, .rv-experiments[open], .rv-experiments-summary, .rv-experiments-hint, .rv-experiments-subbody and .rv-experiments-badge; then add a `@media` (prefers-color-scheme: dark) block only for overrides that need darker surfaces or lighter ink so the component follows the theme tokens in both light and dark modes while keeping existing structure and class names intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AI-Car-Racer/uiPanels.js`:
- Around line 1744-1752: The checkbox only hides .rv-obs-panel but does not stop
its background polling (getIndexStats) because the panel is mounted
unconditionally; update expObsCb and applyObsState so toggling actually
enables/disables the observability module (either by unmounting the panel or
calling a pause/stop method on its polling). Specifically, change applyObsState
to create/destroy or start/stop the obs instance (e.g., call
obs.startPolling()/obs.stopPolling() or remove the node and clear its timers)
instead of only setting hidden, update the change listener on expObsCb to invoke
those lifecycle calls, and move the created .rv-obs-panel into the Experiments
<details> container (where other panels live) so the visual toggle and the panel
are colocated; alternatively, if you prefer the minimal change, rename the
checkbox label to “Show per-stage timings” to reflect it only controls
visibility.
- Around line 1723-1742: The crosstab checkbox can drift because
applyCrosstabState only pushes UI→bridge; add a small renderer (e.g.
renderCrosstab) called from the existing tick loop (or inside tick()) that reads
the bridge state and updates the UI: get window.__rvBridge, check typeof
b.isCrosstabEnabled === 'function', call b.isCrosstabEnabled() and set
expCrosstabCb.checked accordingly and expCrosstabSub.hidden = !checked
(mirroring applyCrosstabState), guarding for missing bridge so it stays stable
until the bridge is ready.
In `@docs/plan/ui-discoverability-pass.md`:
- Line 147: Summary: The exit criterion text incorrectly states "PASS on all 6
claims" but the harness and tests/experiments-panel-smoke.html record 7 claims.
Fix: Update exit criterion `#8` to say "PASS on all 7 claims" (and any nearby
explanatory text if present) so it matches the harness and the test file; verify
the status tracker wording referencing the claim count (the harness PASS message
for tests/experiments-panel-smoke.html) remains consistent.
In `@tests/experiments-panel-smoke.html`:
- Around line 119-129: The C6 test can race against bridge loading or fail on
environments without BroadcastChannel; update the test so before toggling it
waits for the bridge to be ready and skips when BroadcastChannel is unavailable:
use the existing waitForIframe logic but extend it to poll
w.__rvBridge?.info?.().ready === true (or add a small readyForTest helper that
awaits that ready flag), and in the C6 block check typeof BroadcastChannel ===
'undefined' and record a SKIP-style result instead of asserting; reference the
symbols crosstabCb, w.__rvBridge, __rvBridge.info().ready, setCrosstabEnabled,
crosstabIsStarted, and record to locate and modify the code.
---
Nitpick comments:
In `@AI-Car-Racer/style.css`:
- Around line 2121-2219: Replace the hardcoded dark/navy and light text colors
in the .rv-experiments rules with the site's CSS tokens (e.g., use
--surface-2/--surface-3 for backgrounds, --ink-700/--ink-800 for text, --line or
--line-control for borders, --ml-100 for subtle highlights and --amber-100 for
.rv-experiments-badge) and remove literal rgba/# values in selectors such as
.rv-experiments, .rv-experiments[open], .rv-experiments-summary,
.rv-experiments-hint, .rv-experiments-subbody and .rv-experiments-badge; then
add a `@media` (prefers-color-scheme: dark) block only for overrides that need
darker surfaces or lighter ink so the component follows the theme tokens in both
light and dark modes while keeping existing structure and class names intact.
In `@AI-Car-Racer/uiPanels.js`:
- Around line 252-259: Duplicate URL query parsing occurs: remove repeated
parsing and create a single parsed flags object at module initialization (using
URLSearchParams on window.location.search) and reference it everywhere; replace
the top-level _snapshotsFlagOn IIFE with a lookup into that shared urlFlags
object and update buildExperimentsPanel’s IIFE helpers (flag and flagEq) to read
from the same urlFlags instead of re-parsing URLSearchParams, ensuring all
consumers (including any future flag readers) use the single source of truth.
- Around line 1732-1742: The current ?crosstab=1 flow sets expCrosstabCb.checked
and uses a 100ms setTimeout to call applyCrosstabState(true), which can race the
bridge and leave the checkbox desynced; replace the single setTimeout retry with
the same polling pattern used in ensureCrosstabWiring (for loop with await
sleep(100) up to ~20 attempts) to wait for the bridge before calling
applyCrosstabState, referencing expCrosstabCb and applyCrosstabState to keep
behavior consistent; if you prefer not to add polling here, add a clear
TODO/comment in __applyUrlCrosstabFlag/main.js that this code depends on the
poller so future removals won’t break the preset.
- Around line 1685-1709: Add a clarifying inline comment at the line setting
crosstabEl.hidden = false explaining that visibility is now solely driven by
expCrosstabSub.hidden (the old ?crosstab=1 polling no longer applies) so the
pill itself must be left unhidden when re-parented; after appending, add a brief
smoke-check that cached references survived the move and behaviors still run
(e.g., ensure crosstabEl.crosstabPill exists and trigger its pulse/animation,
verify the federation viewer mount runs for federationEl, and ensure the
consistency tick-strip animation still triggers for consistencyEl) so any
regressions from re-parenting are caught.
In `@docs/plan/ui-discoverability-pass.md`:
- Around line 71-87: Add a fenced-code language hint "text" to the ASCII mock-up
code block that begins with the line "🧪 Experiments ▼" so markdownlint MD040 is
satisfied; change the opening fence from ``` to ```text (leave the block
contents unchanged) to mark it as plain text.
In `@tests/experiments-panel-smoke.html`:
- Line 47: The test reporter's record helper currently builds a row via
tr.innerHTML (see the tr.innerHTML assignment) which interpolates label and
detail directly and risks HTML injection; instead construct the cells with
createElement('td') and set their textContent (for label and the detail <pre>
content) and set the status cell's className to ok ? 'ok' : 'fail', then append
the cells to tr so no unescaped HTML is inserted.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 797dceb7-6aa3-4bf7-a83f-1414ad400cf1
📒 Files selected for processing (4)
AI-Car-Racer/style.cssAI-Car-Racer/uiPanels.jsdocs/plan/ui-discoverability-pass.mdtests/experiments-panel-smoke.html
Comment on lines
+1723
to
+1742
| // Crosstab toggle — flip both the bridge state AND the pill visibility. | ||
| function applyCrosstabState(on) { | ||
| if (expCrosstabSub) expCrosstabSub.hidden = !on; | ||
| try { | ||
| const b = window.__rvBridge; | ||
| if (b && typeof b.setCrosstabEnabled === 'function') b.setCrosstabEnabled(!!on); | ||
| } catch (e) { console.warn('[rv-experiments] setCrosstabEnabled failed', e); } | ||
| } | ||
| expCrosstabCb.addEventListener('change', () => applyCrosstabState(expCrosstabCb.checked)); | ||
| // ?crosstab=1 preset — but the bridge may not be ready yet. The | ||
| // existing __applyUrlCrosstabFlag in main.js polls for bridge | ||
| // readiness; here we just sync the checkbox state. The bridge's | ||
| // setCrosstabEnabled will then be called once it's ready. | ||
| if (flagEq('crosstab', '1')) { | ||
| expCrosstabCb.checked = true; | ||
| // Apply with a short delay to give the bridge time to load. If the | ||
| // bridge isn't ready, applyCrosstabState's try/catch swallows it | ||
| // and main.js's poll will pick up the slack. | ||
| setTimeout(() => applyCrosstabState(true), 100); | ||
| } |
There was a problem hiding this comment.
Crosstab checkbox can drift out of sync with bridge state.
applyCrosstabState only flows in one direction (UI → bridge). Unlike renderFederation and renderConsistency, which reflect the current bridge state back into the UI on every tick, there's no equivalent for the crosstab checkbox. If bridge.setCrosstabEnabled(false) is called from elsewhere (e.g. a future console call, an error path inside the bridge that flips _crosstabEnabled back to false when BroadcastChannel is unavailable, or the ?crosstab=1 preset's race with main.js's polling applier), the checkbox will stay checked while the bridge is actually off, and the smoke-test invariant (isCrosstabEnabled() reflecting the toggle) silently breaks.
Cheap fix: in tick() (or a small renderCrosstab() like the others), reflect bridge.isCrosstabEnabled() back into expCrosstabCb.checked and the subbody's hidden state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AI-Car-Racer/uiPanels.js` around lines 1723 - 1742, The crosstab checkbox can
drift because applyCrosstabState only pushes UI→bridge; add a small renderer
(e.g. renderCrosstab) called from the existing tick loop (or inside tick()) that
reads the bridge state and updates the UI: get window.__rvBridge, check typeof
b.isCrosstabEnabled === 'function', call b.isCrosstabEnabled() and set
expCrosstabCb.checked accordingly and expCrosstabSub.hidden = !checked
(mirroring applyCrosstabState), guarding for missing bridge so it stays stable
until the bridge is ready.
Comment on lines
+1744
to
+1752
| // Observability toggle — show/hide the obs panel. The panel itself is | ||
| // mounted by the existing 3A code below; we just toggle its CSS. | ||
| function applyObsState(on) { | ||
| const obs = root.querySelector('.rv-obs-panel'); | ||
| if (obs) obs.hidden = !on; | ||
| } | ||
| expObsCb.addEventListener('change', () => applyObsState(expObsCb.checked)); | ||
| // Apply after a tick so the obs panel is mounted by then. | ||
| setTimeout(() => applyObsState(expObsCb.checked), 250); |
There was a problem hiding this comment.
"Per-stage timings panel" toggle only hides the UI; it doesn't disable the work.
applyObsState flips .rv-obs-panel's hidden attribute, but the observability panel is mounted unconditionally at line 1449 and continues polling getIndexStats regardless of the checkbox. A user who unchecks expecting to "turn off telemetry" still pays for it. Either:
- Rename/relabel the toggle to "Show per-stage timings" to match the actual behaviour, or
- Wire the checkbox to actually mount/unmount (or pause) the obs panel module.
Also note the panel itself remains a sibling of the <details> rather than being moved inside it like consistency/federation/crosstab/snapshots, so visually the "row" inside Experiments and the actual panel below it are disconnected — clicking the toggle changes a card sitting outside the disclosure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AI-Car-Racer/uiPanels.js` around lines 1744 - 1752, The checkbox only hides
.rv-obs-panel but does not stop its background polling (getIndexStats) because
the panel is mounted unconditionally; update expObsCb and applyObsState so
toggling actually enables/disables the observability module (either by
unmounting the panel or calling a pause/stop method on its polling).
Specifically, change applyObsState to create/destroy or start/stop the obs
instance (e.g., call obs.startPolling()/obs.stopPolling() or remove the node and
clear its timers) instead of only setting hidden, update the change listener on
expObsCb to invoke those lifecycle calls, and move the created .rv-obs-panel
into the Experiments <details> container (where other panels live) so the visual
toggle and the panel are colocated; alternatively, if you prefer the minimal
change, rename the checkbox label to “Show per-stage timings” to reflect it only
controls visibility.
| 5. URL flags still work as presets — at boot, every flag that was set pre-selects its UI control. | ||
| 6. Import button shows a `confirm()` that mentions the live archive count. | ||
| 7. Quantization row renders disabled with a tooltip explaining the limitation. | ||
| 8. `tests/experiments-panel-smoke.html` PASS on all 6 claims. |
There was a problem hiding this comment.
Claim count mismatch — harness has 7 claims, exit criterion says 6.
The status tracker (line 37) correctly notes "7/7 harness PASS", but exit criterion #8 still says "PASS on all 6 claims." tests/experiments-panel-smoke.html actually records 7 claims (the trailing ?snapshots=1 preset check). Update the exit criterion to 7 to match the harness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plan/ui-discoverability-pass.md` at line 147, Summary: The exit
criterion text incorrectly states "PASS on all 6 claims" but the harness and
tests/experiments-panel-smoke.html record 7 claims. Fix: Update exit criterion
`#8` to say "PASS on all 7 claims" (and any nearby explanatory text if present) so
it matches the harness and the test file; verify the status tracker wording
referencing the claim count (the harness PASS message for
tests/experiments-panel-smoke.html) remains consistent.
Comment on lines
+119
to
+129
| // C6 — crosstab toggle drives the bridge state | ||
| const crosstabCb = details.querySelector('[data-rv="exp-crosstab"]'); | ||
| const bridge = wDef.__rvBridge; | ||
| const beforeEnabled = bridge && bridge.isCrosstabEnabled ? bridge.isCrosstabEnabled() : null; | ||
| crosstabCb.checked = true; | ||
| crosstabCb.dispatchEvent(new Event('change')); | ||
| await new Promise(r => setTimeout(r, 100)); | ||
| const afterEnabled = bridge && bridge.isCrosstabEnabled ? bridge.isCrosstabEnabled() : null; | ||
| record('6. Crosstab toggle drives bridge.setCrosstabEnabled', beforeEnabled === false && afterEnabled === true, { | ||
| beforeEnabled, afterEnabled, | ||
| }); |
There was a problem hiding this comment.
Claim 6 may flake when the bridge isn't ready or BroadcastChannel is unavailable.
waitForIframe only waits for the experiments-panel DOM, not for __rvBridge.info().ready. If the bridge module is still loading when C6 runs:
bridgemay be undefined →beforeEnabled === null, and the assertionbeforeEnabled === false && afterEnabled === truefails.- Even when defined,
setCrosstabEnabled(true)inruvectorBridge.jsreturnscrosstabIsStarted(), which isfalseifBroadcastChannelisn't available (some CI/test runners, file:// origins, or restricted contexts) — soafterEnabledwould befalseand C6 reports FAIL despite the wiring being correct.
Two cheap defenses:
- Extend
waitForIframeto also poll forw.__rvBridge?.info?.().ready === true, or expose a smallawait readyForTest()helper. - In C6, short-circuit to a SKIP-style record if
typeof BroadcastChannel === 'undefined'so the harness doesn't fail on environments that legitimately can't run it.
♻️ Suggested helper
- async function waitForIframe(ifr, ms = 6000) {
+ async function waitForIframe(ifr, ms = 6000, requireBridge = true) {
const start = Date.now();
while (Date.now() - start < ms) {
try {
const w = ifr.contentWindow;
const d = ifr.contentDocument;
- if (w && d && d.querySelector('[data-rv="experiments"]')) return { w, d };
+ if (w && d && d.querySelector('[data-rv="experiments"]')) {
+ const bridgeReady = !requireBridge ||
+ (w.__rvBridge && w.__rvBridge.info && w.__rvBridge.info().ready);
+ if (bridgeReady) return { w, d };
+ }
} catch (_) {}
await new Promise(r => setTimeout(r, 100));
}
throw new Error('iframe did not load experiments panel within ' + ms + 'ms');
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/experiments-panel-smoke.html` around lines 119 - 129, The C6 test can
race against bridge loading or fail on environments without BroadcastChannel;
update the test so before toggling it waits for the bridge to be ready and skips
when BroadcastChannel is unavailable: use the existing waitForIframe logic but
extend it to poll w.__rvBridge?.info?.().ready === true (or add a small
readyForTest helper that awaits that ready flag), and in the C6 block check
typeof BroadcastChannel === 'undefined' and record a SKIP-style result instead
of asserting; reference the symbols crosstabCb, w.__rvBridge,
__rvBridge.info().ready, setCrosstabEnabled, crosstabIsStarted, and record to
locate and modify the code.
Adds a "🧠 Brain saves" disclosure section directly below the existing
"More actions" block (which contains the legacy single-slot Save
Best+Restart / Restore Old Brain). The new row gives users:
┌──────────────────────────────────────┐
│ ▼ (no saves yet) │ ← <select> dropdown
└──────────────────────────────────────┘
[💾 Save current as…] [📂 Load] [🗑 Delete]
[🌱 Start with empty brain]
Storage: localStorage keys `vv_brainsave_<name>` namespace, JSON-encoded
slot of `{name, savedAt, fitness, trackId, brain: serializeBrain(...)}`.
Load reuses the existing `restoreOldBrain` pathway (write
`localStorage.bestBrain` + `restartBatch()`), so the seeding loop in
main.js picks up the saved brain on next gen. Start Fresh wipes
`bestBrain`/`oldBestBrain`/`fastLap`/`progress`/`rvAnnotations` plus
calls `bridge._debugReset()` plus reloads the page — true gen-0 boot.
**Named saves are preserved** across Start Fresh, so the user's
curated slots survive a reset.
Both destructive paths (Load replacing the seed, Delete dropping a
slot, Start Fresh wiping everything) are gated by `window.confirm()`
with messages that surface the live brain count where relevant.
What this ships:
- buttonResponse.js — brainSaveAs / brainSaveLoad / brainSaveDelete /
brainStartFresh / refreshBrainSavesDropdown / _brainSavesList
helpers. Default-name suggestion is `<ISO-timestamp>-fit<N>` so a
user can hammer "Save" repeatedly without naming friction.
- utils.js — new <details id='brainSaves'> section appended to the
side-column rendering, repositioned post-render to sit directly
under #moreActions for visual continuity with the legacy buttons.
refreshBrainSavesDropdown() called once on render to populate from
any pre-existing localStorage slots.
- style.css — .brain-saves disclosure styling matching the
Experiments panel aesthetic; .brain-saves-fresh button has an amber
tint so users don't confuse Start Fresh with the cheaper Reset
Brain.
- tests/brain-saves-smoke.html — 6-claim harness covering the
storage primitives that don't require a `confirm()` dialog
(round-trip byte-identity, dropdown enumeration with fitness
decoration, multi-slot coexistence, Load primitive, Delete
primitive, handler-presence sanity check). The confirm-bearing
wrappers are simple enough to verify by code review.
Validated via agent-browser:
- Harness PASS 6/6, including the load-bearing claim 1 (a saved brain
reads back JSON-byte-identical to the original).
- Live UI: section renders open with 4 buttons + dropdown, default
option "(no saves yet)", Start Fresh visually distinct (amber).
- Default boot: no new console errors. Existing legacy buttons (Save
Best + Restart / Restore Old Brain) untouched and functional.
Out of scope (explicit non-changes):
- No track-mismatch warning when loading a brain trained on a
different track shape — allowed today, deferred to a future slice.
- No per-slot export to .vvarchive — the Phase 1A whole-archive
bundle remains the path for shareable archives.
- No bridge changes — every primitive reuses an existing
bridge/localStorage/serializeBrain pathway. Pure UI + handler code.
The global localStorage.fastLap key was track-confused: train on Rect,
get an 11s record, switch to a triangle, the panel still shows "11.0"
for a track where 11s is impossible. Phase A makes Fast Lap track-aware
by content-addressing tracks the same way Phase 1D content-addresses
brains.
What changed user-visible:
Before: "Fast Lap: 11.0" (one global, unaware of track)
After: "Fast Lap: 11.00s (this track)"
"Last: 13.42s"
"all-time best: 9.10s" (subscript; suppressed when this
track IS the all-time-record holder)
Storage shape: localStorage.vv_fastlap_<trackHash> = JSON of
{timeS, recordedAt, generation}. trackHash = xxHash32 over the 512-d
CNN track embedding (window.currentTrackVec) via the new hashVec alias
in archive/hash.js — the same xxHash32 helper Phase 1D already uses
to dedup brains. Symmetric naming: hashBrain at the F5 site,
hashVec at the fast-lap site, one implementation.
What this ships:
- archive/hash.js — hashVec exported as an alias of hashBrain. Same
function, two domain-honest names. Documented in a comment.
- main.js — fastLap stays a global cache (so brainExport.js, grapher.js
and other classic-script readers continue to work) but now reflects
the CURRENT track's record. New globals: lastLap (in-memory,
per-session) and allTimeBest (cached min over vv_fastlap_*).
Helper bridge under window.__vvFastLap exposes syncFromStore /
write / read / trackKey / allTimeBest / setLastLap / getLastLap so
the classic-script files (buttonResponse.js) can call them without
a module bridge. Hash util loaded async via dynamic import; bounded
poll _bootSyncFastLap waits up to 15s for both hash util and
currentTrackVec, then syncs once. New-record path writes per-track
+ recomputes allTimeBest. The render becomes 3-4 lines: "Fast Lap"
+ "Last" + (sometimes) "all-time best", with opacity hierarchy
Fast Lap > Last > all-time-best.
- main.js — legacy localStorage.fastLap key silently retired at boot.
The old value was attributed to no track; migrating it would mislead.
Clean cut: user gets a fresh per-track record from their next lap.
- buttonResponse.js — resetFastLap() now scopes to the CURRENT track
via __vvFastLap.trackKey + syncFromStore. clearAllFastLaps() is the
bulk option, removes every vv_fastlap_* key (preserving
vv_brainsave_* — confirmation dialog mentions this explicitly).
destroyBrain() routes through resetFastLap() so the post-reset
state is consistent.
- utils.js — new "🗑 Clear all fast laps" row in the 🧠 Brain saves
disclosure, directly below "🌱 Start with empty brain". Same
destructive-but-cheap energy; same confirm()-friction shape.
- tests/fastlap-track-aware-smoke.html — 7-claim harness covering
hashVec aliasing, hash determinism, distinct-vec hash distinctness,
per-track segregation, all-time-best math, legacy-key retirement
not disturbing per-track records, clear-all-fastlaps not touching
vv_brainsave_* keys.
Validated via agent-browser:
- Harness PASS 7/7.
- Default boot clean: timer renders "Fast Lap: -- (this track) / Last: —"
with the all-time-best subscript correctly suppressed (no records yet).
- Live-state injection (current track 15.5s, different track 9.1s,
last lap 17.3s): timer renders all 3 lines + subscript, opacity
hierarchy as designed. Screenshot at /tmp/vv-review/10-*.png.
- window.__vvFastLap API exposed and reachable from buttonResponse.js.
Out of scope (intentional non-changes):
- Track-change hot re-sync — the cache picks up new currentTrackVec
references via _trackHash's identity check, but the global fastLap
doesn't auto-resync on every track switch. The first lap on a new
track triggers the sync via the new-record handler. Acceptable
given how rarely tracks change mid-session.
- Lap-history sparkline — deferred per the plan; the 3-line layout
is already the meaningful upgrade.
- Track-preview thumbnails — deferred; would require a proper
"your tracks" feature, not an inline addition.
shaal
added a commit
that referenced
this pull request
Apr 27, 2026
…s panel Implements the plan from docs/plan/ui-discoverability-pass.md, in the same branch / PR #2 the plan landed in. Surfaces the previously URL-flag-gated Save/Share-archives row and Cross-tab row in a single collapsible disclosure panel, alongside the already-visible Federation and Consistency rows. What changed: - uiPanels.js — new buildExperimentsPanel() at the bottom of the IIFE. Creates a <details class="rv-experiments"> + summary + body, then re-parents the existing consistency/federation/crosstab DOM nodes into the disclosure body via appendChild. Existing event listeners and el.X references survive untouched (wrap-don't-rebuild). The snapshots row and the share row are now ALWAYS created (the if (_snapshotsFlagOn) gate is removed); the experiments-level "📦 Save & share archives" checkbox controls subbody visibility. Same pattern for the crosstab "🔗 Cross-tab live training" toggle, which now drives bridge.setCrosstabEnabled() directly. URL flags become PRESETS that pre-toggle the corresponding control; if any flag is set the disclosure auto-opens so the user can see what their share-link enabled. Default state matches the plan's table: observability ON, consistency Fresh, federation OFF, snapshots OFF, crosstab OFF, quantization DISABLED. - uiPanels.js — confirm() on destructive Import paths. Both the file-Import in the snapshots row and the URL-Import in the share row now ask "REPLACE your current N brains?" before proceeding. Previously the ?snapshots=1 flag was the only friction; with the UI unhidden, an explicit confirm replaces that. - uiPanels.js — crosstab listeners now wired unconditionally (previously gated by ?crosstab=1). The bridge state, not the listener subscription, decides whether to broadcast. - style.css — adds .rv-experiments, .rv-experiments-summary, .rv-experiments-body, .rv-experiments-row, .rv-experiments-toggle, .rv-experiments-subbody, .rv-experiments-row-disabled, .rv-experiments-badge classes. Native <details>/<summary>; no custom widget, no JS animation. Triangle marker rotates 90° on open. - tests/experiments-panel-smoke.html (new) — 7-claim iframe-based harness. Loads the main page in a hidden iframe (no flags) and a second iframe with ?snapshots=1, then asserts: 1. Disclosure exists, defaults closed. 2. 6 rows visible after expand. 3. Quantization row is disabled. 4. [Learn] eli15 badges wired on new rows. 5. Snapshots toggle gates the inner controls + both snapshots row and share row are migrated inside the subbody. 6. Crosstab toggle drives bridge.setCrosstabEnabled() (false→true). 7. ?snapshots=1 presets the toggle ON + reveals controls + opens the disclosure. - docs/plan/ui-discoverability-pass.md — A.1–A.10 status flipped to ✅ with implementation notes (wrap-don't-rebuild strategy, auto-open on flag, listener-always-wired refactor). Validated via agent-browser: - Default boot clean, no new console errors. - Smoke harness 7/7 PASS, including bridge-state coupling (crosstab checkbox flips bridge.isCrosstabEnabled() live). - ?consistency=frozen&federation=1&crosstab=1 multi-flag URL still works — every UI control reflects its flag, every bridge getter reports the active state, disclosure auto-opens. Out of scope (explicit non-changes): - No default-on flips. Federation, Cross-tab, Snapshots, Quantization all remain OFF by default. - No new URL flags. - No feature module changes (archive/, consistency/, federation/, crosstab/, quantization/, observability/, share/, lineage/ all untouched). - No quantization integration into archiveBrain — disabled row says so explicitly with a tooltip pointing at the chapter. Net diff: ~310 lines added across 4 files (close to the plan's ~250-line estimate; came in higher because the smoke harness imitates a two-iframe test runner).
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.
Summary
This PR contains a plan document only — no feature code. It
proposes a UI discoverability pass for the features landed in PR #1
(RuLake-inspired roadmap).
The core issue: three features (`?snapshots=1` save/share,
`?crosstab=1` cross-tab sync, and the library-only 1-bit
quantization) ship with zero UI — only discoverable by reading
docs or chapter text. Two other features (federation, consistency
modes) already use the right pattern: URL flag presets the initial
state of an always-visible UI control.
This plan proposes a single `🧪 Experiments` disclosure panel that:
Scope is small: ~250 lines, one PR, primarily in `AI-Car-Racer/uiPanels.js`. No feature module is touched.
See `docs/plan/ui-discoverability-pass.md` for:
Details
` over custom widgets, one consolidated panel over scattered toggles, no default-on flips)Why plan-only first
Per the working discipline on this project, non-trivial UX
changes get a reviewable plan document before code. This PR is
the reviewable plan; the implementation PR will follow once the
plan is approved.
What's NOT in this plan
Test plan
After approval, an implementation PR lands the actual ~250 lines + the `tests/experiments-panel-smoke.html` harness.
Summary by CodeRabbit
New Features
Bug Fixes