Add independent Quick View and Environment tools - #82
Conversation
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ Minor suggestions only.
Reviewed changes
- Independent surface state — Quick View and Environment get separate open bits, front-surface tracking, legacy storage migration, and a reducer with transition coverage in
environment-panel-state.ts. - Measured side-by-side layout —
resolveQuickViewLayoutkeeps both surfaces beside each other when the workbench fits them and auto-stacks on narrower allocations, driven by aResizeObservermeasurement of the actual workbench. - Non-modal floating tools — the compact-modal machinery (backdrop, focus trap,
applicationModal,interactionBlocked) is removed; the floating surface has no backdrop, blur, or app-wide inert state, with per-surface trigger focus restored only while focus is still inside the closing surface. - Command and toolbar routes —
quick-view.togglecommand (no default binding) and/quick-viewslash command;⌘⇧E//environmentnow toggle the full tools destination directly; the toolbar gains a Quick View toggle and hides the open-in-editor picker below 460px of toolbar width. - Dock placement — the assistant dock shifts left by the inline tools width via
rightInset, and stays layered at the chat edge when tools float. - Terminal viewport outline removed, plus plan, inspiration-guide, and specimen updates with the compact-modal regressions rewritten for the non-modal contract.
I verified the focused Environment/layout/command/keybinding suites (92 tests) plus the Subagents panel and sidebar suites (76 tests), type-check, and lint on the changed files — all pass.
ℹ️ Stale code snippet in the design specimen
The mock editor snippet at docs/chatgpt-ui-element-specimen.html:690-701 still shows EnvironmentSummaryCard and the old "overview" tab model — the only leftover of the pre-split surface naming after the rest of the specimen was updated. The design reference docs are treated as load-bearing for future UI work, so this snippet could mislead; update it to the Quick View/Environment two-surface model (or the tab-based EnvironmentSurfaces shell with toolsOpen/quickViewOpen).
DeepSeek Flash ｜ 𝕏
| runs={subagents.liveSnapshots} | ||
| detailRequest={subagentDetailAnnouncement} | ||
| portalHost={open && tab !== "overview" ? subagentAnnouncerHost : null} | ||
| portalHost={surfaceState.toolsOpen ? subagentAnnouncerHost : null} |
There was a problem hiding this comment.
While the tools surface is covered (stacked narrow layout with Quick View in front), portalHost still targets the surface's host node, so the aria-live region renders inside an inert + aria-hidden subtree and subagent lifecycle announcements are silently suppressed. They are also permanently lost: the region text has already changed by the time the surface is re-presented, so nothing re-announces.
| portalHost={surfaceState.toolsOpen ? subagentAnnouncerHost : null} | |
| portalHost={surfaceState.toolsOpen ? subagentAnnouncerHost : null} |
There was a problem hiding this comment.
ℹ️ No new issues in this delta — the E2E alignment is accurate and would genuinely fail on a shared-state regression.
Reviewed changes
- Rewrote the deterministic chat-shell E2E to drive the two-surface UI — the
Show Environmenttoolbar toggle opens the "Environment work surface" (Review tab selected with its empty-state text), Quick View opens from the surface header, and the test asserts independentaria-pressed/aria-labelstate on both toolbar toggles plus Escape closing only the front surface while Environment stays mounted. - Recorded the E2E-alignment papercut in
.papercuts/troubleshooting.md.
I verified each locator and assertion against the implementation — the EnvironmentPanelToggle/QuickViewToggle accessible names and data-quick-view-toggle, the aria-labels on both <aside> surfaces, the tabpanel's aria-labelledby name chain, the reducer's frontSurface: "quick-view" transition, and the Escape handler's focus-based surface resolution (the open-quick-view focus effect moves focus into the card, so Escape closes Quick View rather than Environment). npm run type-check:e2e passes.
The two informational items from the prior review remain open and unchanged in this delta (the portalHost aria-live thread and the stale specimen snippet), so this run leaves them for the author's judgment rather than approving.
DeepSeek Flash ｜ 𝕏
There was a problem hiding this comment.
ℹ️ The surface-close E2E fix is accurate and verified — no new issues in this delta. The two informational items from earlier reviews remain open, so this run stays non-approving.
Reviewed changes
- Closed the floating Environment surface via its surface-local "Close environment panel" header button instead of the toolbar toggle — a floating surface can cover the toolbar trigger at the window edge, so the smoke test now drives the visible in-surface control.
- Asserted the toolbar toggle returns to
aria-pressed="false"behind the closing surface, so the E2E genuinely fails if the surface-local close stops syncing the toolbar state. - Updated the
.papercuts/troubleshooting.mdnote to record the covered-trigger lesson.
I verified every new locator and assertion against the implementation: the header close button (renderer/components/environment-panel.tsx:1321) calls closeTools → close-tools reducer, and EnvironmentPanelToggle's aria-pressed derives directly from toolsOpen (environment-panel.tsx:1658), so the state-sync assertion is a real behavioral check. npm run type-check:e2e passes. Note: CI runs test:e2e on macos-26 only; this Linux runner launches Electron but the app fails startup ("Failed to start Aiden Agent"), so I could not execute the spec end-to-end here.
The two informational items from prior reviews remain open and unchanged in this delta (the portalHost aria-live thread at environment-panel.tsx:1067 and the stale EnvironmentSummaryCard/"overview" snippet in docs/chatgpt-ui-element-specimen.html).
DeepSeek Flash ｜ 𝕏
1 participant
Summary
Testing
npm run type-checknpm run type-check:e2enpm run lintnpm run test:e2e(11 passing, 1 intentionally skipped)npm run test:sidebarnpm run test:slash-commandsnpm run buildgit diff --checkKnown baseline
npm run test:command-systempasses 63/64 locally; the remainingrenderer-readiness-coresource-contract assertion predates and is unrelated to this renderer-only change.