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 ｜ 𝕏
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 ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found. The delta cleanly fixes both informational items from the prior reviews: the subagent live region now lives outside any surface that can become inert, and the stale specimen snippet now reflects the two-surface model.
Reviewed changes
- Fixed subagent aria-live containment —
SubagentLiveAnnouncerno longer portals into the Environment surface; it mounts at provider level outside both surfaces (environment-panel.tsx:1056-1065), and the now-unusedsubagentAnnouncerHoststate andsetSubagentAnnouncerHostplumbing were removed from the provider and surface. - Rewrote the Environment/Subagents contract assertions to pin the stable top-level region (no
createPortal/portalHostin the announcer, nosubagentAnnouncerHostin the environment stack, region as the direct child of the provider guarded bysubagentsEnabled, singlerole="status"). - Extended the mounted-DOM test to assert the live DOM node is preserved and stays a direct child of the harness container across floating/covered transitions, with announcements still updating while the sibling surface is
inert+aria-hidden. - Extended the macOS chat-shell E2E: asserts exactly one live announcer, never inside an
[inert]/[aria-hidden]ancestor, same DOM node across the stacked narrow layout (tools surfaceinertat 900px), Escape close, and surface close — an assertion that genuinely fails on the pre-fix portaled behavior. - Corrected the mock editor snippet in
docs/chatgpt-ui-element-specimen.htmlto the Quick View/Environment two-surface model (retiring theEnvironmentSummaryCard/"overview" example flagged in the initial review) and recorded the containment-test papercut note.
I verified the implementation (region is rendered ahead of {children} inside EnvironmentPanelContext.Provider at root-view.tsx:37, and the covered tools aside is the only surface that becomes inert), confirmed no portalHost/subagentAnnouncerHost/EnvironmentSummaryCard references remain repo-wide, and ran the focused Environment/Subagents/layout suites — 82 tests pass. The chat-shell E2E scenario itself runs only on the macOS CI job, which cannot be launched on this Linux runner.
DeepSeek Flash ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found. The fixture-timestamp fix is correct and makes the rejection tests date-independent.
Reviewed changes
- Made the forged-record rejection fixtures date-independent — the fixed
2026-08-27T12:00:00.000Zatinmain/services/diagnostic-support.test.tsaged past the 7-day general-journal retention window on later CI runs, sopruneRetentionAtdeleted the log file before projection could reject the forged record and the/unknown record/assertion silently never fired. Both fixtures (general forged record, subagent open-string record) now stampat: new Date().toISOString(), which always survives the prune. - Recorded the fixture-aging lesson in
.papercuts/troubleshooting.md.
I traced the failure mechanism in source (pruneRetentionAt removes a whole journal file when its oldest record's at predates the cutoff — diagnostic-journal.ts:343-377) and confirmed the rejection triggers are content-based, not at-based: the general record fails projection on its non-allowlisted attackerContent field key and the subagent record on its off-enum failure value (diagnostic-support.ts:204-295), so swapping only the timestamp preserves the original semantics. Ran npx tsx --test main/services/diagnostic-support.test.ts — 16/16 pass, including both modified tests.
DeepSeek Flash ｜ 𝕏
…ility-fix # Conflicts: # .papercuts/troubleshooting.md # renderer/components/environment-panel.tsx # renderer/components/terminal-drawer.tsx
There was a problem hiding this comment.
ℹ️ The merge of current main is clean — conflict resolutions are consistent and both sides' test/contract coverage still passes on the merged tree. One token-consistency nit inline.
Reviewed changes
- Merged current main into the branch (f2e5017), integrating the borderless-UI token rework (plus PR #84 and catalog refreshes) with manual resolutions in the three conflicted files —
.papercuts/troubleshooting.md,renderer/components/environment-panel.tsx, andrenderer/components/terminal-drawer.tsx. - Applied main's new radius tokens during conflict resolution — the Environment tab pill and the Quick View card moved to
rounded-menu/rounded-sheet, matching the repo-wide migration. - Terminal-drawer active-pane styling resolves to main's
bg-list-selectionfill, superseding the branch's outright ring removal while preserving the same borderless direction (and the active-session affordance). - Auto-merged shared tests and the chat-shell E2E spec retain both sides' assertions; verified
npm run type-checkand 193 focused renderer tests (sidebar source contracts, Environment/Subagents contracts, layout, motion, interface-polish, text-entry focus, composer, slash commands, keybindings) all pass on the merged tree, with the feature's net delta vs main unchanged at exactly the 30 reviewed files.
DeepSeek Flash ｜ 𝕏
There was a problem hiding this comment.
Important
Commit 2beaf1a5 fixed the token-consistency nit but left the identical source-contract assertion in renderer/components/chat-sidebar.test.tsx stale — npm run test:sidebar (chained into pretest, so npm test/CI) now fails.
Reviewed changes
- Applied the shared
rounded-sheettoken to the floating Environment surface (environment-panel.tsx:1201), retiring the last rawrounded-[24px]inrenderer/and addressing the prior review's nit (thread resolved). - Synced the twin contract regex in
environment-subagents-contract.test.ts— but missed the identicalassert.matchinchat-sidebar.test.tsx:344, which the commit's source change now breaks.
I verified the failure empirically: npx tsx --test renderer/components/chat-sidebar.test.tsx → 28 pass / 1 fail (environment inline handoff uses the same animated spacer pattern), and confirmed test:sidebar runs inside pretest, so the PR's test job fails until the assertion is updated.
DeepSeek Flash ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found. The delta resolves the prior finding: the stale radius assertion in the sidebar handoff contract is synced to the
rounded-sheettoken.
Reviewed changes
- Synced the twin source-contract regex in
renderer/components/chat-sidebar.test.tsx— the "environment inline handoff" assertion now expects"bottom-3 right-3 top-3 rounded-sheet ...", matchingenvironment-panel.tsx:1201after2beaf1a5renamed the floating-surface class. This was the exact stale assertion that broketest:sidebar(chained intopretest/CI).
I verified the source and tests agree: environment-panel.tsx:1201 holds the rounded-sheet class, chat-sidebar.test.tsx passes 29/29 and environment-subagents-contract.test.ts 24/24, and no rounded-[24px] references remain anywhere in renderer/ (the appearance.test.ts occurrence is a doesNotMatch, still valid). The prior review thread is resolved.
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.Accessibility follow-up
Keep the single Subagent live region mounted at provider level, outside the non-modal Environment and Quick View surfaces. Covering Environment on narrow layouts no longer hides announcements inside an inert subtree, and panel transitions preserve the live DOM node. Removed obsolete portal ownership and corrected the specimen's retired overview-tab example.
Validation: 71 focused Environment/Subagent tests and all 3 chat-shell Electron scenarios passed, including narrow-layout inert containment and stable live-region identity. TypeScript, E2E type checking, production build, and lint passed. React Doctor reported only existing complexity/state-organization warnings. No announcement payload, server, or native-client contracts changed. The accessibility update passed hosted Electron E2E and Pullfrog. The verify job exposed an existing diagnostics fixture aged beyond journal retention; rejection fixtures now use current timestamps. All 72 diagnostics tests and focused lint passed locally. Hosted checks rerun on the follow-up commit.