feat(chrome): make conversation panel horizontally resizable - #164
feat(chrome): make conversation panel horizontally resizable#164gfiorav wants to merge 5 commits into
Conversation
f334348 to
d258fca
Compare
|
Thanks for the PR! This branch currently has a merge conflict with the base branch. When you get a chance, please rebase onto (or merge) the latest base branch, resolve the conflict, and push. After that, checks will re-run and the PR will get looked at again. Noted for lavish-axi#164 at |
d258fca to
6aa26d5
Compare
|
Automated reminder: thanks for the PR! This branch currently has a merge conflict with the base branch. When you get a chance, please rebase onto (or merge) the latest base branch, resolve the conflict, and push. After that, checks will re-run and the PR will get looked at again. Noted for lavish-axi#164 at |
6aa26d5 to
7698685
Compare
|
Automated reminder: thanks for the PR! This branch currently has a merge conflict with the base branch. When you get a chance, please rebase onto (or merge) the latest base branch, resolve the conflict, and push. After that, checks will re-run and the PR will get looked at again. Noted for lavish-axi#164 at |
The conversation panel was a fixed 360px column, often too narrow to read agent replies comfortably. Add a draggable splitter on its left edge: - New `src/panel-width.js` exposes pure clamp/persistence helpers (`clampPanelWidth`, `loadStoredPanelWidth`, `savePanelWidth`) that the chrome and unit tests share via a single source of truth. - `src/server.js` injects those helpers into the chrome page through an inline script (same pattern as the artifact SDK), adds a `<div class="splitter">` between `.frame` and `.panel`, and updates the grid template to `minmax(0, 1fr) auto var(--panel-w)`. - `src/chrome.css` styles the splitter with a col-resize cursor, a subtle vertical line + center grip that appear on hover/focus/drag, and hides it in the stacked mobile breakpoint (already in place at 860px). - `src/chrome-client.js` wires pointer events on `window` (so the drag survives the cursor crossing the artifact iframe), disables iframe pointer events via a `dragging-splitter` body class, re-clamps the stored width on `window.resize`, and supports dblclick to reset and arrow keys for keyboard nudging. Behavior: - Width clamps to [280, 60% of viewport]; max floored at min so a tiny viewport can never collapse the panel. - Stored in localStorage as `lavish-axi:panel-w`; corrupt or out-of-range values fall back to the 360px default and self-heal on next load. - 19 unit tests cover the clamp/persistence rules and 11 chrome-client tests cover the drag flow, reset, persistence, and resize re-clamp. Artifacts stay portable: the splitter lives entirely in the chrome page and the injected panel-width helper is stripped by the export pipeline.
…-sync
Per the review gate, the keyboard support and slider-ARIA values were
out of scope (the original ask was just a draggable divider). The
splitter is now pointer-drag + double-click only.
Removed:
- Splitter `keydown` handler (Enter/Space/Arrow keys) and the
preceding comment that advertised PageUp/PageDown/Home/End/Escape.
- `tabindex="0"` and the slider value attributes
`aria-valuemin` / `aria-valuemax` / `aria-valuenow` from the
splitter element. role=separator with no value semantics is the
correct accessibility shape here.
- The `aria-valuenow` write inside `applyPanelWidth`.
Kept:
- Pointer drag to resize (clamped to [280, 60% of viewport]).
- Double-click to reset to the 360px default.
- Width persistence across reloads in localStorage.
Also fixed: the window-resize listener was clamping the stored width
against the (small) mobile viewport and silently overwriting
localStorage when the splitter was actually hidden under the 860px
breakpoint - a brief mobile-width visit would permanently shrink a
desktop-chosen panel. The handler now short-circuits when
`matchMedia('(max-width: 860px)')` matches, so resync only runs
in the layout where the splitter is visible. Two new chrome-client
tests cover the guard.
7698685 to
afed7f3
Compare
|
Automated reminder: thanks for the PR! This branch currently has a merge conflict with the base branch. When you get a chance, please rebase onto (or merge) the latest base branch, resolve the conflict, and push. After that, checks will re-run and the PR will get looked at again. Noted for lavish-axi#164 at |
|
Speaking as Kun's firstmate: reviewed. Default panel width stays 360px and the splitter is chrome-only (export strips it). I'm not auto-merging because it still changes default chrome — always-on drag handle, localStorage |
|
Speaking as Kun's firstmate: restamping hold. This remains a captain-decision hold (new-default chrome: always-on splitter + localStorage |
What Changed
src/panel-width.jsmodule that clamps, persists, and exposes the conversation panel width; the server inlines it into the chrome page so the panel can resize without a round trip.src/chrome.css.test/panel-width.test.jsand extended chrome-client queue tests, and document the resizable panel inREADME.mdand the lavish-design skill.Risk Assessment
✅ Low: The implementation is well-bounded, well-tested, and works correctly in the browser (verified via vm simulation of the generated IIFE); the only finding is a misleading comment that does not affect behavior.
Testing
Verified the user intent (horizontally resizable conversation panel) by running the full unit test suite (all 538 pass) and then driving a real Lavish session in Chrome: I dragged the splitter, reloaded to confirm persistence, double-clicked to confirm reset, shrank the viewport to confirm the 60%-of-viewport clamp commits the new value, shrank further into the mobile breakpoint to confirm the splitter hides and the stored width is preserved, and corrupted localStorage to confirm self-heal. Seven PNG screenshots were captured to the evidence directory to make the user-facing behavior reviewer-visible.
/var/folders/y8/wy53333j43ncsk_r9djvh5mh0000gn/T/no-mistakes-evidence/01KX71PHRXZN5N5J53CBA7PFMS/01-baseline-default-360px.png)/var/folders/y8/wy53333j43ncsk_r9djvh5mh0000gn/T/no-mistakes-evidence/01KX71PHRXZN5N5J53CBA7PFMS/02-after-drag-840px.png)/var/folders/y8/wy53333j43ncsk_r9djvh5mh0000gn/T/no-mistakes-evidence/01KX71PHRXZN5N5J53CBA7PFMS/03-after-reload-persists-840px.png)/var/folders/y8/wy53333j43ncsk_r9djvh5mh0000gn/T/no-mistakes-evidence/01KX71PHRXZN5N5J53CBA7PFMS/04-after-dblclick-reset-360px.png)/var/folders/y8/wy53333j43ncsk_r9djvh5mh0000gn/T/no-mistakes-evidence/01KX71PHRXZN5N5J53CBA7PFMS/05-after-window-resize-clamp-540px.png)/var/folders/y8/wy53333j43ncsk_r9djvh5mh0000gn/T/no-mistakes-evidence/01KX71PHRXZN5N5J53CBA7PFMS/06-mobile-breakpoint-splitter-hidden.png)/var/folders/y8/wy53333j43ncsk_r9djvh5mh0000gn/T/no-mistakes-evidence/01KX71PHRXZN5N5J53CBA7PFMS/07-splitter-hover-state.png)Pipeline
Updates from git push no-mistakes
⏭️ **intent** - skipped
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
src/chrome-client.js:866- endSplitterDrag calls event.preventDefault() on every pointerup, which Chrome/Firefox/Safari interpret as cancelling the default action of the pointerup - that action is what fires the compatibility click event. Without a click event firing, the dblclick event will not be raised at all, so the "double-click to reset" affordance registered on the splitter (chrome-client.js:891) is effectively dead in real-world usage. The test at test/chrome-client-queue.test.js:1159 only fires a synthetic dblclick via fireSplitterEvent, so it never exercises the pointerdown -> pointerup -> click -> pointerdown -> pointerup -> click -> dblclick chain and misses the regression. The same preventDefault also suppresses click after a real drag, which the cursor user did not ask for. Fix: drop the preventDefault on pointerup (the body class already sets user-select: none to block text selection, which was likely the original motivation), or only preventDefault when the user actually moved the pointer between down and up.src/chrome.css:656- The :focus-visible::after, :focus-visible::before selectors and the .splitter:focus-visible { outline: none; } rule are dead code. Commit da6d18e removed tabindex="0" from the splitter (src/server.js:955) and dropped the splitter keydown handler, so the element is no longer focusable by keyboard and these rules can never match. Either restore keyboard focus (e.g. tabindex="0") if focus styling is wanted, or strip the unreachable selectors to keep the stylesheet honest.test/chrome-client-queue.test.js:18- The createChromeHarness helper accepts a panelWidthHelpers parameter and wires it into context.LavishPanelWidth when set, but no test in the suite (or anywhere in test/) ever passes it. The non-null branch is dead and can be removed, or one test should use it to prove the override path works.test/chrome-client-queue.test.js:340- The literal "lavish-axi:panel-w" is duplicated across 8 call sites in this file (lines 340, 1087, 1097, 1109, 1162, 1174, 1189, 1208). The single source of truth is PANEL_STORAGE_KEY in src/panel-width.js:20 and it is already imported by other tests. Re-importing it here (or re-exporting from a shared test helper) would make the constant change in one place.src/chrome-client.js:892- The dblclick handler calls event.preventDefault() but the dblclick event has no default action, so the call is a no-op. Harmless, just dead.src/chrome-client.js:905- The mobile breakpoint string "(max-width: 860px)" is duplicated against the CSS @media (max-width: 860px) in src/chrome.css:995 and the .splitter { display: none; } rule on src/chrome.css:1004. If the breakpoint ever changes in CSS, this matchMedia query has to be updated by hand or the desktop-chosen width will silently get re-clamped on a mobile resize. Consider deriving the value from a shared constant (similar to PANEL_DEFAULTS) so the JS and CSS stay in lockstep.src/chrome-client.js:888- initializePanelWidth unconditionally calls commitPanelWidth on every page load, even when the loaded stored value already satisfied the clamp and was written back unchanged. The comment justifies this as self-healing for corrupt or out-of-range values, which is the only case where the write matters. For the common in-range case this is a wasted localStorage.setItem on every chrome page open. Consider guarding the commit withif (clamped !== rawStored)to skip the no-op writes.🔧 Fix: fix splitter pointerup, dead focus rules, no-op writes
1 warning still open:
src/chrome-client.js:883- initializePanelWidth calls storage.getItem(PANEL_STORAGE_KEY) on the newrawStoredline introduced by the f7 fix, but unlike loadStoredPanelWidth (src/panel-width.js:61) which wraps getItem in try/catch, this direct call is unprotected. The adjacent safeLocalStorage() (line 882) only guards access to window.localStorage itself, not method calls on the returned object. In rare cases (e.g. localStorage disabled by user policy, third-party-cookie blocking surfacing a SecurityError on getItem, or a corrupted storage entry), getItem can throw and crash the chrome before applyPanelWidth runs - even though the f6-era loadStoredPanelWidth would have returned a safe default. The f7 commit intentionally added this comparison so init skips no-op writes; the fix should wrap the getItem in try/catch (treat throw as 'no key stored' and fall through to the commit path) so a broken storage cannot regress the chrome.🔧 Fix: guard rawStored getItem with try/catch in initializePanelWidth
1 info still open:
src/panel-width.js:27- The comment aboveexport { resolveDefaults }claims the re-export is needed so the inlined browser copy inserializePanelWidthForBrowsercan see it, but the IIFE defines its own localresolveDefaultsfrom.toString()and does not depend on the ESM export. The actual reasonresolveDefaultsneeds to be exported is thatsrc/server.js:37imports it to call.toString()when building the inlined script. Either drop the export and inline the function source via a direct import inserver.js(matching howclampPanelWidthHelper/etc. are aliased), or rewrite the comment to point at the real reason. The current comment will mislead a future reader who tries to inline another module and looks here for the pattern.✅ **Test** - passed
✅ No issues found.
node --test test/panel-width.test.js(19/19 pass)node --test test/chrome-client-queue.test.js(43/43 pass)node --test test/server.test.js(143/143 pass)node --testfull suite (538/538 pass)End-to-end:chrome-devtools-axi openof a real session, baseline panel width measured at 360pxEnd-to-end:chrome-devtools-axi dragof the splitter to the artifact heading moved--panel-wto 840px (60% of 1400px viewport cap) and persistedlocalStorage['lavish-axi:panel-w']= "840"End-to-end:window.location.reload()re-applied the 840px width from localStorageEnd-to-end:splitter.dispatchEvent(new MouseEvent('dblclick'))reset--panel-wto 360px and persisted "360"End-to-end:chrome-devtools-axi resize 900 800re-clamped the stored 600px width to 540px (60% of 900) and committed "540" to localStorageEnd-to-end:chrome-devtools-axi resize 600 800(mobile breakpoint) hid the splitter (display: none) and left the stored width at "600" without overwritingEnd-to-end: corruptlocalStorage['lavish-axi:panel-w']value self-heals to the default 360px on next loadEnd-to-end:getComputedStyle(splitter, '::before').opacityis 1 on hover (visual hint), width 3px, height 28px, using--border-strong✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.