Skip to content

feat(apps): pop out an app page into its own window - #22926

Merged
iamwhatever merged 1 commit into
kirodotdev:mainfrom
cixuuz:feat/app-popout
Nov 1, 2026
Merged

iamwhatever merged 1 commit into
kirodotdev:mainfrom
cixuuz:feat/app-popout

Conversation

@cixuuz

@cixuuz cixuuz commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Problem / Motivation

Goal: Pop an app page (Dev Fleet, AWS Control, an installed app) out into its own window, so the app stays on screen beside the main window's chat.

Chat sessions, artifacts and the terminal each pop out into their own OS window (/popout/chat, /popout/artifact, /popout/terminal). App pages cannot. To watch a Dev Fleet build or a pod boot while asking the agent about it, you flip between the two pages, or open a second full dashboard tab.

Why it matters

Apps are where the work shows (builds, pods, reviews); chat is where you drive it. In one window every glance at an app costs the chat view. A second dashboard tab duplicates the shell, the sidebar and every connection, and has no way back to the main window.

Not a goal

  • No change to the chat, artifact or terminal pop-outs' behaviour, with one shared seam named: they share the engine, and this adds optional fields to its navigation intent. The engine's no-main fallback (navFallbackUrl) now carries a prompt that names no session as ?new=1&prefill= instead of dropping it, which the artifact pop-out's fallback inherits.
  • No rewrite of app pages or the App SDK. Apps keep their absolute paths and their exits as they are.
  • No window management: no tiling, no remembered window positions.
  • No new chat-page URL contract. The no-main fallback uses the ?sid=&prefill= and ?new=1&prefill= forms the chat page already reads.

What changed (motivation → approach → change)

An app page and the chat share one window, and a popped-out app is a second JS context with its own store and socket. The existing pop-out engine (utils/popoutController.ts, used by chat and artifacts) already solves the cross-window part: one BroadcastChannel per feature, heartbeat liveness, window.open dedupe by window name, and a hand-off for navigation that must happen in the main window. So this reuses it instead of adding a split view.

Part What it does
Entry point (shell/nav/appRowPopoutAction.tsx, shell/nav/navRail.tsx) The pop-out lives on the sidebar, not on the app page. Each Apps-group row of the expanded desktop rail carries a pop-out action beside it, shown while the row is hovered or the action has keyboard focus, and always on a touch screen (where hover: never matches), at a 32px target. On the row of the app on screen it opens the window at the page and query the user is on (/aws-control/usage?tab=x); on any other row, at the app's route; keyed by the app's route for dedupe. While the app is out the row shows an always-visible accent mark instead, which focuses that window; the row itself still navigates this window to the app. The mark has its own glyph (a window, AppWindow), not the pop-out arrow, so the state reads from its shape and not from colour alone. The action is a sibling of the row's role="button" element, never a child, and swallows the press events dnd-kit's sensors start a reorder from, so pressing it is never a pick-up. Nothing is drawn over an app page, so an app's own chrome (File Explorer's tab strip, a page's top-right controls) is never covered, and third-party bundles need no change. The collapsed rail, the phone drawer and the drag ghost carry no action; a row whose route the window would refuse as no app gets none either.
Title bar (components/AppPopoutTitleBar.tsx) The pop-out window has a real 32px title bar above the app's <main>, with the app's name and Return, which hands the app back to the main window. A row in the layout, not an overlay, so the app lays out in the space that is left; its trailing group is indented past the focus-mode caption controls (focus-caption-reserve) as the chat title bar's is.
Window (pages/AppPopoutFrame.tsx at /popout/app/<route>) Renders the app as the dashboard does (BuiltinAppRoute or AppPage), registers as the live pop-out, titles the OS window after the app. On the /apps/<name> arm the app list is consulted first: a native builtin (which AppPage would redirect to its own route, enabled or not) or an unknown name gets a not-found notice with Return rather than a redirect loop, and a list that failed to load gets a Retry notice instead of AppPage. While the list is still loading, the window shows the same "Loading app…" line AppPage shows for its own load, never a blank area.
Router (main.tsx, utils/appPopoutRoute.ts) The window's router mounts at /popout/app, so the app's absolute paths resolve unchanged and a copied URL reopens as a pop-out. The frame compares its pinned route with the router's params in the router's own decoded form (routerParamForm), so a percent-encoded link such as /popout/app/apps/a%20b settles on its not-found notice instead of handing off to main on every render.
Exit hand-off (pages/AppPopoutFrame.tsx, utils/navIntent.ts, utils/popoutController.ts, utils/errorReport.ts, shell/appShell.tsx) A navigation that leaves the app is performed by the main window, not the pop-out. The app subtree navigates through a navigator scoped to the pinned app (UNSAFE_NavigationContext), so an exit is handed off before the router commits it: this window's location never changes and the app keeps its state. An exit <Link>'s rendered href is the main dashboard's URL for that page (build base plus app-space path), so a middle or modifier click, "open in new tab" and copy-link reach it there, not in a second pop-out pinned to a page that is not an app. The hand-off carries the path and query, and the four things an app's chat exit stages in its own window before navigating: the Redux pendingInput, the session it activated (a switch, a creation or a fork), the App SDK's window.__mc_chat_launch intent, and the error prompts an "Ask the agent" control staged. Main publishes the forwarded pendingInput to its store only once the navigation it was staged for has committed: Redux re-renders synchronously while BrowserRouter commits in a transition, so an immediate publish would let a chat page already on screen consume the prompt under the outgoing query, overwrite that session's draft and lose the ?autoSend=1&newSession=1 launch. It publishes only on that navigation's own commit: the committed path and query equal the staged destination, or, for a prompt bound to a session, the commit is the chat page's own /chat[/<slug>]?sid= rewrite for that session, with that session active. Any other commit that lands inside the pending transition, whether a later forwarded intent, a sidebar click or a session switch, drops the staged prompt instead of publishing it under the other navigation's query or into another session's composer. Forwarded error prompts are re-staged in main's own sessionStorage for the chat page's hand-off intake; when that storage refuses, main navigates to the chat page's draft-only fresh-session launch (/chat?new=1&prefill=) instead, so the diagnostic seeds the session that launch creates and never a retained session's unsent draft (a stale prompt paired with a non-chat destination is dropped). The shell installs its soft navigator in an app pop-out too; that control navigates above the frame, so its exit is handed off after the router commits it and the window steps back. A hard "Ask the agent" (the card an ErrorBoundary shows for a crashed app, or for a crashed frame) never loads /chat in the pop-out: the window installs a hard exit (installHardExit in utils/errorReport.ts, installed by AppPopoutHardExit above the route boundary in App.tsx), so sendErrorToChat stages the prompt and hands it to main through the same exit hand-off, with no navigation in this window; the full page load remains the fallback where no hard exit is installed (the main window, the other pop-outs).
No-main fallback (utils/popoutController.ts) With no main window open, the destination opens in a new tab instead, under the build base (/crew/chat on a KIROCREW_BASE_PATH=/crew build). The prompt rides as ?sid=&prefill= for a named session, or ?new=1&prefill= for a new one; an error prompt always rides as ?new=1&prefill=, a fresh session, even when the exit also named one, since a keyed prefill replaces that session's composer.
Coordination (utils/appPopout.ts, hooks/useAppPopouts.ts) The shared engine on its own channel, keyed by the app's dashboard route.
Inherited storage (utils/errorReport.ts) A window window.open creates starts with a copy of its opener's sessionStorage, so a new app pop-out inherits main's hand-off queue and the FIFO main is delivering. On its first document the pop-out drops both (dropInheritedChatHandoffs, before recovery runs), so a diagnostic main is delivering is never forwarded back as a second one; a storage marker tells a reload of that window apart, so its own entries still recover. Any other document clears that marker, so a pop-out tab whose Return fell back to loading the dashboard in place cannot pass a stale marker into its next pop-out.

App.tsx mounts the window's routes. hooks/panelTabRegistry.ts reports whether the apps list has loaded (isPending), whether its first load failed (isLoadingError, so a failed refresh of a list already on screen changes nothing) and how to ask again (refetch); the window waits on the first before mounting an /apps/<name> page and offers Retry on the second. The launch intent is typed with the App SDK's own ChatLaunchOptions. The new strings are in i18n/locales/en.json and the thirteen machine catalogs. docs/feature-map/README.md names the route.

Backwards compatibility

Compatible: every existing route, pop-out and chat URL form behaves as before; the new NavIntent fields are optional, and useInstalledApps gains a field without changing the existing ones.

Removes nothing: the diff adds a route, a rail action, a title bar and fields; nothing main accepts is rejected, renamed or hidden. The Apps-group rows gain trailing padding for the action on the expanded desktop rail, and nothing else in the sidebar moves.

Tests

  • test/appPopout.test.ts: which dashboard paths count as an app page, including an installed app named detail or migrate (the App Store's own routes for those words need a third segment, so only /apps/library and the /apps/-/ prefix are the store's); pop-out document path to app route; router basename, including a sub-path build and a path outside the build base; URL and window-name shape, and no collision with the other pop-outs' names; the URL opens at the initial page and query, drops a hash, and falls back to the app root for a path the window would not pin to the route; openPopout opens that URL under the route's window name; routerParamForm leaves a plain route alone, decodes each segment, turns an encoded slash into a slash, leaves a malformed escape undecoded, and agrees with the params matchRoutes reads for the frame's two arms across ten encoded paths.
  • test/AppPopoutEntryPoints.test.tsx: the rail action on a row that is not the app on screen opens the app at its route, on the active row at the page and query on screen, and an installed app by its /apps/<name> route; it shows only on row hover or keyboard focus while the app is in this window, and carries the (hover: none) state hoverVariantPolicy.test.ts requires of every hidden-until-hover control; while the app is out it is an always-shown accent mark, with its own glyph rather than the pop-out arrow, that focuses the window; it reads only its own route out of the live set; a press and its keys stop at the action (the sortable wrapper never sees pointerdown, mousedown, touchstart or keydown, while the same events on the row do reach it) and a click never reaches the row; it is a sibling of the row, not inside it. The title bar names the app and offers Return, which hands the app back to main; it is a 32px row whose trailing group clears the focus-mode caption controls; a long name is truncated with the full name as its title.
  • test/navRailAppPopoutAction.test.tsx: through the real rail, the action sits beside each Apps-group row element on the expanded rail, not inside it, and the collapsed rail renders none.
  • test/AppPopoutFrame.test.tsx: mounting a builtin and an installed app under a title bar carrying its name and Return; the title bar sits above the app's scroll container, not inside it; register/unregister; window title; in-place sub-path navigation and navigate(-1) through the app's own history; an exit keeps the app mounted with its state (the location never changes); a <Link> out of the app is intercepted like a navigate; an exit link's href is the main dashboard's URL and an in-app link's is the router's, with and without the window's /popout/app basename; a modifier-click on an exit link is left to the browser with no hand-off and no navigation here; app-space paths are judged under the window's /popout/app basename; a navigation from outside the frame (the shell's soft navigator) is still handed off and the window steps back; a chat exit hands off flags and the staged input and consumes it, including a prompt staged in the same handler as the exit; a deep link's sid; the activated session is carried once, spent, and carried again on a same-session re-open; a session activated without a switch (a creation) is carried; a non-chat exit spends it; an SDK launch is claimed off the window and carried; staged error prompts are forwarded and the pop-out's queue spent; the shell installs its soft navigator for an app pop-out and skips the other pop-outs and embeds; a replace-exit never changes this window's location; a bogus deep link, /apps/<native builtin> (enabled or disabled) and an unlisted name are refused with Return; a builtin that ships a UI bundle and a disabled installed app mount through AppPage; a pending list mounts nothing; a list whose first load failed shows the Retry notice with its "Ask the agent" hand-off (Retry asks for the list again) and never AppPage; a failed refresh of a loaded list keeps the app on screen; a builtin route ignores the list's state; an encoded route (/apps/a%20b, /apps/foo%2Fbar, /dev%2Dfleet) settles with one registration, no hand-off and no navigation; a hard "Ask the agent" from a crashed builtin app, and from a crashed frame (the route-level boundary), hands the prompt to main as one errorHandoffs entry on a /chat intent, empties this window's staged queue, never calls location.assign, and leaves this window's location untouched.
  • test/errorToAgent.test.tsx: the hard-exit seam: a hard hand-off stages the prompt and then calls the installed sink instead of loading a page, as does one with no soft navigator; a refused staging calls no sink and reports the hand-off as not made; a soft hand-off keeps the soft navigator and never the sink; installHardExit(null) and __resetNavSeamForTests restore the full-load path.
  • test/errorToAgent.test.tsx (inherited storage): the first document of an app pop-out drops the copied queue and claim and marks the storage as its own, and recovery then promotes nothing; a reload of that pop-out keeps and recovers its own claim; a main-window document keeps its hand-offs and writes no marker; a former pop-out tab that loads the dashboard clears the marker, so its next pop-out still drops what it inherits; the drop runs before module-load recovery can promote an inherited claim.
  • test/popoutNavForwarding.basePath.test.ts: under a /crew build base, the no-main fallback tab opens the destination under the base with its query intact, and a bare path and the root get the base.
  • test/popoutNavForwarding.test.ts: fallback URL shapes (?new=1&prefill=, the ?prefill=1 sentinel replaced, a launch's message, a launch into an existing session, a forwarded error prompt, an error prompt beside a named slot or a replayed ?sid= opening a fresh session with the other parameters kept); applyNavIntentInMain stages pendingInput, the launch and the error prompts (notifying a mounted chat page) before navigating, and stages nothing on a refused leave; when this window's storage refuses the error prompts it navigates to /chat?new=1&prefill=<prompts> with no pendingInput and no slot switch, keeps the destination's other query parameters while dropping its sid, and drops a prompt paired with a non-chat destination without publishing it; claimChatLaunch fresh, at the ten-second edge, past it, and absent, on a pinned clock; takeDeferredPendingInput with nothing staged, an unchanged router key, an equal committed location, the inclusive ten-second edge against one past it, the user's /chat committing before a session-less launch, the chat page's ?sid= and legacy ?slot= rewrites for the named session, a rewrite naming another session, another session active at an equal location, no rewrite allowance for a non-chat destination, /chat? normalised to /chat, and a malformed committed href; the hand-off stages the prompt with its destination and session before switching and navigating.
  • test/App.navLeaveGuard.test.tsx: through the real shell and router, a forwarded pendingInput reaches the store only once the destination query has committed (the publication is observed with /chat?autoSend=1&newSession=1 already rendered, never under the outgoing route); a second forwarded intent to another destination, or one that switches to another session, landing in the same transition drops the staged prompt; an intent's own session switch, and the chat page's own ?sid= rewrite of the intent's URL, still deliver it; a hand-off whose navigation never committed is dropped, not published under an unrelated navigation; the user's own sidebar navigation to /chat, or to another session's /chat?sid=, landing inside the pending transition drops a forwarded ?autoSend=1&newSession=1 prompt, while the chat page's own session rewrite of the next session-bound hand-off still publishes it exactly once, observed with that session's /chat?sid= rendered; the leave-guard cases drive both the artifact and the app channel.
  • test/App.topbarAndShortcuts.test.tsx: the import-outcome notice and main's file-menu result toast both still cover the new frame arm.

Determinism: the six frontend test files of the sidebar entry (AppPopoutEntryPoints, navRailAppPopoutAction, AppPopoutFrame, appPopout, popoutNavForwarding, App.navLeaveGuard; 146 tests) ran three times plain and three times shuffled (--sequence.shuffle --sequence.seed=7311, =48219, =9137), all green. The earlier heads' five files ran 33 times plain and twelve times shuffled across four rounds (seeds 7311, 48219, 9137, 60412, 31337, 8086), green. The repo's scoped frontend suite (485 files, 13,279 tests) and the i18n render gate ran green on this head.

Manual verification

In a real browser against this head's website/dist (the capture run below, Chromium through Playwright, every /api/** answered from fixtures): hovering the Dev Fleet row showed the action; clicking it opened the window through window.open at /popout/app/dev-fleet, titled "Dev Fleet — Kiro Crew", with the title bar reading "Dev Fleet / Return"; the window registered over the channel and the main window's row switched to the accent mark; the mark stayed through the main window's own navigation to a chat session. On an earlier head's dev build, with the entry point still on the page: Return in the pop-out focused main and closed the window; the engine behind it is unchanged here. Not checked by hand: the no-main fallback tab (covered by popoutNavForwarding.test.ts).

Screenshots / video

Committed under temp-screenshots/app-popout/ rather than attached: this account has read access only, and gh --attach answers that with a 404 (cli/cli#14302). All six were rendered from a build of this PR's website/dist (the popped-out mark and the loading window from this head; the first frame predates the mark's new glyph, so its sidebar row still shows the earlier arrow icon) with the repo's capture-harness pattern (scripts/lib/serve-dist.mjs + stubDashboardApi, every /api/** answered from fixtures: three builtin apps in the list, a two-worktree Dev Fleet), so they show the exact copy and layout that ship. The first is two browser windows from one run laid side by side; the rest are single windows.

Dev Fleet popped out beside the main window, which is on a chat session; the Dev Fleet row in its sidebar carries the accent mark, and the pop-out window has the title bar with the app's name and Return

The Dev Fleet row of the sidebar, hovered, with nothing popped out: the pop-out action appears at the row's trailing end, with its tip "Pop out app to its own window"

The Dev Fleet row while Dev Fleet is out: the always-visible accent mark with its window glyph, with its tip "This app is open in its own window — focus it"

A pop-out window opened for /apps/does-not-exist: the title bar with the generic name and Return, and the "No app at" notice

An /apps/my-tool pop-out window while the app list is still loading: the title bar with the generic name and Return, and the "Loading app…" line

A pop-out window whose app list failed to load: the title bar with Return, and the "Could not load the app list" notice with its error, Ask the agent, and Retry

Related Issues

Closes #22886

Follow-ups left out of this PR on purpose, each behaviour already on main: #26430 (IncidentChat's hard exit inside an app pop-out) and #26431 (?autoSend=1 dropped by ChatPage's ?sid= rewrite, shared with the Command Bar).

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|style|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

@cixuuz
cixuuz requested review from a team and KiroCrewCaptain as code owners October 21, 2026 22:20
@cixuuz
cixuuz requested a review from dodgeblaster October 21, 2026 22:20
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: checking Automated validation is still running labels Oct 21, 2026
@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Design Review (Opus 5.5, fork) — ✅ PASS

Design-level review of 8ded073d1c12ca20e09ca0fec472ff461e665841 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS does not.

Design-Verdict: PASS

Nothing blocks: the change reuses the existing pop-out engine, mounts the window's router at /popout/app so apps run unchanged, and includes six committed screenshots of this revision.

Notes

  • Every app exit is intercepted by wrapping react-router's UNSAFE_NavigationContext navigator. The router's maintainers make no stability promise for that API. If an upgrade changes how <Link>, useNavigate or <Navigate> reach the navigator, exits would quietly mount pages inside the pop-out. Pin it with a test that fails on such an upgrade, not only on today's behaviour.
  • The ## Backwards compatibility section falls after the description's 8000-byte cut, so I couldn't check its reader list. I found no reader outside the patch that breaks. The artifact pop-out's no-main fallback now opens ?new=1&prefill= and adds the build base to the path; the description already declares that change.

[DESIGN-REVIEWED] 8ded073

Description read

[DESCRIPTION-READ] 3f15908b0b3000a3915f825e96fa1ae948a804f9494f2e267ee7283d76a51d4f

That sha256 names everything this verdict read: the PR description, plus the 1 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with .github/scripts/pr-description-capture.sh over the same inputs; if it differs, the description or that evidence changed after this verdict was formed and any finding drawn from either is unproven.

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

UX Review (Opus 5.5, fork) — ✅ PASS

UX-level review of 8ded073d1c12ca20e09ca0fec472ff461e665841 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS does not.

UX-Verdict: PASS

Nothing blocks. The screenshots show every new control and state: the row pop-out action with its tooltip, the popped-out mark, the title bar with Return, and the loading, list-error and not-found windows. Nobody has done a first-time read, because this lane reviews forks in one pass; pushing the branch to the repository runs the blind-read lane.

Notes

  • dev-fleet-popout-beside-chat.png shows the Dev Fleet row with the accent pop-out arrow (ExternalLink) while the app is out. The code renders the AppWindow glyph in that state, as app-row-popped-out-mark.png shows. Retake that composite so the two images agree.
  • pages.appPopoutFrame.no_app_at_route puts the raw route in the heading ("No app at /apps/does-not-exist"). Name the app instead, "{{name}} isn't installed", and keep the path for the lead line.

[UX-REVIEWED] 8ded073

Description read

[DESCRIPTION-READ] 214bec3a60700f96bd8a7556a25831d07bd976305ac80d00eb3ef1b6847b110e

That sha256 names everything this verdict read: the PR description, plus the 9 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with .github/scripts/pr-description-capture.sh over the same inputs; if it differs, the description or that evidence changed after this verdict was formed and any finding drawn from either is unproven.

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

First Principles Review (Opus 5.5, fork) — ✅ PASS

Premise-level review of 8ded073d1c12ca20e09ca0fec472ff461e665841 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS does not.

First-Principles-Verdict: PASS

Nothing blocks. Pop-out is a new, opt-in capability added beside the existing app page, and no default changes. Every rider is declared in the description.

Notes

  • Item 8 — oversized: the hunk else if (intent.path === '/chat' …) { … params.set('new', '1'); params.set('prefill', …) } adds a URL fallback for when main's sessionStorage refuses. On base, sendErrorToChat simply does not navigate when staging fails ("Navigating is conditional on the staging having stuck"). Subtraction: delete the else if branch and drop the prompt when handoffToChat fails, as base already does. Clears when: the mechanism is removed, or a human amends the Goal.

What this change ships

Inventory (10 items) — 9 justified

Intent: keep an app page on screen in its own OS window beside the main window's chat. This is an ADDITION.

  1. Each Apps row in the expanded desktop rail gets a pop-out action, shown on hover or focus (always shown on touch) — justified
  2. While an app is popped out, its row shows an accent window mark that focuses that window — justified
  3. New /popout/app/<route> window renders the app, with its router mounted under that prefix — justified
  4. The pop-out window has a title bar with the app's name and Return — justified
  5. Not-found, list-loading and list-error notices (with Retry) in the pop-out — justified
  6. A navigation that leaves the app (link, SDK openChat, staged prompt, slot switch) is performed in the main window — justified
  7. Main publishes a forwarded pendingInput only when its own navigation commits — justified
  8. A forwarded error prompt that main cannot store opens /chat?new=1&prefill= — oversized
  9. The shared no-main fallback now sends a session-less prompt as ?new=1&prefill= and adds the build base (artifact pop-out inherits this) — justified
  10. A hard "Ask the agent" in a pop-out hands off to main; sessionStorage copied from the opener is dropped on first load — justified

[FIRST-PRINCIPLES-REVIEWED] 8ded073

Description read

[DESCRIPTION-READ] 529f42fc3696ee8980984385e2c16552c72042a5e07038598559e6105f184760

That sha256 names the PR description this verdict read. Recompute it with .github/scripts/pr-description-capture.sh; if it differs, the description changed after this verdict was formed and any finding drawn from the description is unproven.

@github-actions

github-actions Bot commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

GPT 6.1 Review (fork) — ✅ no blocking findings

Reviewed 8ded073d1c12ca20e09ca0fec472ff461e665841 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 8ded073

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Oct 21, 2026
@cixuuz

cixuuz commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor Author

span=0b589d9ea58f
self-added: yes
mechanism: the /apps/<name> arm consults the installed-apps list before mounting AppPage

  • A failed app list permits a native-app redirect loop — fixed in a33dc06

Legitimate: with /api/apps failing and /api/apps/<name> succeeding, misrouted excluded appsError, so AppPage mounted and its native-builtin redirect became an exit this window hands to main and steps back from, forever.
Fix: on that arm a list whose FIRST load failed renders an ErrorNotice with Retry (useInstalledApps().refetch) and the Return control; AppPage never mounts while the list is pending or errored. Gated on isLoadingError, not isError, so a failed background refetch of a list already on screen keeps the running app (react-query keeps the cached data; isLoadingError is status === 'error' && data === undefined).
Tests: AppPopoutFrame.test.tsx — a first-load failure shows the notice, Retry calls refetch, AppPage is absent; a failed refresh of a loaded list keeps the app; a builtin route ignores the list's state. The first fails on f85fa25.

@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Oct 21, 2026
@cixuuz

cixuuz commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor Author

span=c500a5e06d3c
self-added: yes
mechanism: claimChatLaunch freshness cases in popoutNavForwarding.test.ts

  • Pin the clock for launch-freshness assertions — fixed in a33dc06

Legitimate: the stale case compared Date.now() - 10_001 written into the fixture against a second Date.now() read inside claimChatLaunch, so the verdict depended on the wall clock between two reads.
Fix: the claimChatLaunch describe pins the clock (vi.useFakeTimers({ toFake: ['Date'] }) + vi.setSystemTime(NOW)); fixtures use NOW, NOW - 10_000 (kept, the edge of the > 10_000 rule) and NOW - 10_001 (dropped). The file-level afterEach restores real timers; the other describes fake timers only inside their own cases.

@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Oct 21, 2026
@cixuuz

cixuuz commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

Rebased: main moved in the i18n catalogs and website/src/components since this head went green (4533b1e -> dde25d8); CI re-running on a33dc06 together with the two GPT fixes above.

@github-actions

github-actions Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Opus 5.5 Review (fork) — ✅ no blocking findings

Reviewed 8ded073d1c12ca20e09ca0fec472ff461e665841 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 8ded073

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Oct 22, 2026
@cixuuz

cixuuz commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor Author

span=0b589d9ea58f
self-added: yes
mechanism: the pop-out's uncarriedSlotSwitch ledger

  • Newly created app sessions are not carried to main — fixed in dac2761

Legitimate: Personal Shopper activates its advisor session through createSlot.fulfilled and never dispatches switchSlot, so a ledger keyed on slotSwitchRequestId alone recorded nothing and main kept its previous conversation.
Fix: the store subscription also records a change of chat.activeSlot to a non-null key (creation, fork, direct selection); a same-key re-open is still recorded through the request id. The next /chat exit without a sid carries the recorded key and spends it.
Test: AppPopoutFrame.test.tsx 'carries a newly activated session before a plain /chat exit' (fails on a33dc06).

@cixuuz

cixuuz commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor Author

span=0b589d9ea58f
self-added: yes
mechanism: the shell's soft-navigator seam and the hand-off's errorHandoffs

  • Error-to-chat exits replace the app window instead of forwarding — fixed in dac2761

Legitimate: the shell skipped installSoftNavigate for every pop-out, so an "Ask the agent" control inside an app pop-out fell to window.location.assign('/chat') and the window became a second dashboard.
Fix: the shell installs the soft navigator for an app pop-out (chat and artifact pop-outs, and embeds, still skip it), so sendErrorToChat routes through the router to AppPopoutHandoff; the hand-off drains the window's sessionStorage queue (consumeChatHandoff) into NavIntent.errorHandoffs, and main re-stages them with handoffToChat and notifyChatHandoffs() before navigating, falling back to the store hand-off if its storage refuses. With no main window the first prompt rides ?new=1&prefill=.
Residual: askAgentHard (the root ErrorBoundary) keeps the hard navigation by design; a crashed pop-out has no router to route through.
Tests: 'forwards staged error prompts to main and spends the pop-out queue'; 're-stages error hand-offs and notifies mounted chat before navigating'; the storage-refusal fallback; the shell rule read from source.

@cixuuz

cixuuz commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor Author

span=0b589d9ea58f
self-added: yes
mechanism: the failed-app-list ErrorNotice in the pop-out frame

  • Failed app-list loading omits the required hand-off decision — fixed in dac2761

Legitimate: a load failure's notice must carry askAgent when nothing can be lost and the agent can run; this one carried neither the prop nor a sanctioned opt-out.
Fix: askAgent added. With the soft navigator now installed in an app pop-out, the hand-off forwards to the main window's chat instead of replacing the pop-out.
Test: the failed-list case asserts the "Ask the agent" control.

@cixuuz

cixuuz commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor Author

span=381c15af5376
self-added: yes
mechanism: the failed-app-list ErrorNotice in the pop-out frame

  • Failed app-list ErrorNotice has no agent hand-off (errors-use-error-notice) — fixed in dac2761

Legitimate: a read/list/load failure's notice must turn on askAgent; nothing editable lives in that window, and the agent can run.
Fix: askAgent added, and the shell now installs its soft navigator in an app pop-out so the hand-off reaches the main window's chat (the pop-out drains its staged prompt into the forwarded intent) rather than turning the pop-out into a dashboard.
Test: the failed-list case asserts the "Ask the agent" control; the hand-off path is covered in AppPopoutFrame.test.tsx and popoutNavForwarding.test.ts.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Oct 22, 2026
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Oct 24, 2026
@cixuuz

cixuuz commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor Author

span=381c15af5376
self-added: yes
mechanism: the frame compares here (rebuilt from useParams(), decoded) with route (the raw document path) to decide whether a location is an exit to hand to main

  • A pop-out URL with a percent-encoded app segment sends the pop-out into an endless replace-and-forward loop — fixed in 7ade642

Confirmed: react-router 7.18.2 decodes each segment for matching and turns a param's %2F back into /, while route keeps the document's escapes, so /popout/app/apps/a%20b gave here = /apps/a b against route = /apps/a%20b, mounted the hand-off, and its replace to the same URL fired it again. The regression tests ran out of heap on the unfixed code.
The comparison now reads route the way the router reads its params (routerParamForm in utils/appPopoutRoute.ts: per-segment decodeURIComponent, a decoded / written back as %2F, a malformed escape left undecoded, then %2F → /), so the location the hand-off would replace to is a fixed point on first render. route itself is unchanged everywhere else: the registration key, the pinned navigator's judge, the installed-app match and the Return target.
The suggested guard in AppPopoutHandoff was not added: with the comparison fixed the hand-off only mounts for a location whose params differ from the route's form, and its replace lands where they agree, so a route === location.pathname branch is unreachable.
An encoded route settles on the not-found notice rather than an app: backend APP_NAME_RE is [a-z0-9][a-z0-9_-]*, so no installed app's route carries an escape, and for every valid route routerParamForm(route) === route, which keeps the hand-off decision byte-identical for real apps.
Tests: three frame cases (/apps/a%20b, /apps/foo%2Fbar, /dev%2Dfleet: location unchanged, no forwardToMain, one registration) and five helper cases, one of which compares ten encoded paths against matchRoutes for the frame's two arms so a router upgrade that changes param decoding fails there first.
Span 381c15af5376 is this file's Opus span; this is its fourth finding across rounds 3, 4, 5 and 10, each a different defect (error-notice hand-off, exit teardown, exit hrefs, now encoding). pr_findings.py --rounds reports no recurring-span trigger for this PR.

@cixuuz

cixuuz commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor Author

span=96a7c8d01039
self-added: yes
mechanism: deferred pendingInput publish in shell/appShell.tsx + utils/navIntent.ts; supersession is now judged at the router commit, and the issue-time wrappers navigateIntent / switchSlotIntent are removed

  • Superseding sidebar navigation publishes the prompt into the wrong draft — fixed in 6dde9c1

Third round on this span (rounds 8, 9, 11). A read-only retrospective over the whole origin/main...HEAD diff ran before this repair and chose a restructure, not a fourth guard: rounds 8 and 9 judged supersession where a navigation is issued (navigateIntent, switchSlotIntent), and a sidebar NavItem click never passes those seams, which is exactly this finding.
Both wrappers are removed; the pop-out and native-menu nav-intent suppliers are back to main's plain navigate / dispatch(switchSlot(...)), and the switchSlotCallsiteClassification pin is back to main's value.
The deferral itself stays (Redux renders synchronously while BrowserRouter commits in a transition), but takeDeferredPendingInput now publishes only on the intent's own commit: the committed path and query equal the staged destination, or, for a session-bound /chat destination, the commit is ChatPage's /chat[/<slug>]?sid=<slotKey> rewrite; and when the intent names a session it must be the store's activeSlot at that commit. Any other commit inside the pending transition, forwarded or user-made, drops the prompt.
The finding's scenario: the QA hand-off stages /chat?autoSend=1&newSession=1 with no session; the user's navigate('/chat') commits inside the transition; /chat is not the staged destination, so nothing is published, the active session's draft is untouched, and no launch runs without its flags.
Pinned by App.navLeaveGuard.test.tsx "drops the prompt on a user navigation and publishes it on ChatPage’s own session rewrite" (real shell and router, no fake clock: step 1 fails on the key-only predicate, step 2 fails on a literal href match), the /chat?sid= sidebar variant, and ten pure cases in popoutNavForwarding.test.ts; 143 tests in the five pop-out files, 3 plain + 3 shuffled runs green.
Left out on purpose: launch flags are not carried in the store (13 setPendingInput seeders); a session-bound hand-off still loses ?autoSend=1 to ChatPage's own rewrite and lands unsent in the named session's composer; no toast for a dropped hand-off.

@cixuuz

cixuuz commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor Author

span=434384425704
self-added: yes
mechanism: APP_STORE_SEGMENTS in utils/appPopoutRoute.ts, the set that tells the App Store's own /apps/<segment> pages apart from an installed app's page

  • Valid installed-app names are excluded from pop-outs — fixed in 73dcd6e

Confirmed against the code: the backend reserves only library as a route name (src/kiro_crew/apps/manifest_names.py:33-39 says detail and migrate need no entry because their routes carry a mandatory second segment), and App.tsx registers /apps/detail/:name and /apps/migrate/:name, so a bare /apps/detail falls through to /apps/:name and is an installed app named detail.
APP_STORE_SEGMENTS is now ['library', '-']; /apps/detail/<name> and /apps/migrate/<name> are still not app pages through the existing two-segment check, and the doc comment says why the two words are not listed.
test/appPopout.test.ts pins /apps/detail → /apps/detail and /apps/migrate → /apps/migrate beside the kept null cases for /apps/library, /apps/detail/my-tool, /apps/migrate/my-tool and /apps/-/updates.

@cixuuz

cixuuz commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor Author

span=839826d8e0d5
self-added: no
mechanism: ChatPage's activeSlot → ?sid= rewrite strips autoSend from a session-bound destination; the forwarded prompt is still delivered to that session

  • Session rewrites lose a forwarded command’s auto-send flag — rebutted: not a defect this PR introduces, and the remedy is out of its goal

Second round on this span (8, 13); adjudication downgraded it as a disproportionate remedy, and this is the case the restructure deliberately left out: the prompt reaches the session the hand-off named and sits visible in its composer, unsent. Nothing is lost or misdirected.
The pop-out path matches main's own in-window path for the same action: the Command Bar awaits switchSlot, dispatches setPendingInput synchronously and then navigates to /chat?autoSend=1 (apps/command-bar/useCommandBarOverlayActivation.ts), so with ChatPage already on screen the intake consumes the prompt under a query that has no autoSend there too. With ChatPage not yet mounted, both paths commit /chat?autoSend=1 itself and the launch fires.
Carrying launch flags in the store would touch the 13 setPendingInput seeders and add a second intake channel plus an ordering rule between the stash and the intake; that is a change to how every chat hand-off works, not part of popping an app page out. A follow-up can do it for the in-window and pop-out paths together.

@cixuuz

cixuuz commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

Rebased: main moved in the i18n catalogs (website/src/i18n/en.context.json, website/src/i18n/locales/*.json) and docs/feature-map/README.md since this head went green (12419547 -> 1ccd3f4f); the merge was clean and the local gates (build, lint, i18n check and render, scoped frontend tests) are green on 7938a97b8ce6. This push also re-triggers the two fork lanes that never judged 73dcd6ee: the Fork Opus 5.5 Review run failed at "Resolve and validate PR" and the Fork Internal Content Scan run hung at "Checkout base (trusted)", both at 10:11Z, leaving that head with no verdict from either. CI re-running.

@cixuuz

cixuuz commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

Rebased: main moved in the i18n catalogs (website/src/i18n/en.context.json, website/src/i18n/locales/*.json), docs/feature-map/README.md and nearby website/src/components/* and website/src/hooks/* files since this head went green (1ccd3f4f -> 0dab1986, 48 commits); the merge was clean and the local gates (build with the React Compiler toolchain from #23634, lint, i18n check and render, 13,134 scoped frontend tests) are green on 6d8868990aec. This push also re-triggers the fork Internal Content Scan, which never opened a check-run on 7938a97b: its run failed at "Resolve and validate PR" because the runner could not reach api.github.com (connection refused, three attempts), and PR Readiness has been waiting on that lane since 13:04Z. CI re-running.

@cixuuz

cixuuz commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

Re: HOLD (crew-pr-reviewer r120, head d604a96), following up on the earlier reply.

The current head is 6d88689 (rebased onto main 0dab198). All three held items remain addressed there exactly as described in that reply: the appPopout.ts re-exports and unused hook fields are gone, the Not a goal line names the shared navFallbackUrl seam, and the five screenshots (icon, Popped out chip, Return, no-app notice, list-error notice) are committed under temp-screenshots/app-popout/ and shown in the description.

Since that reply, three AI-review rounds have run and each finding has a disposition on this thread: GPT 6.1's third block on the forwarded-prompt hand-off was answered by restructuring the mechanism (supersession is now judged at the router commit; the two issue-time wrappers were removed) rather than a fourth guard, after a retrospective over the whole diff; a GPT finding on APP_STORE_SEGMENTS (installed apps named detail or migrate) was fixed; and GPT's downgraded note on the autoSend flag surviving ChatPage's own URL rewrite was rebutted as pre-existing behaviour shared with the in-window Command Bar path. GPT 6.1 and Opus 5.5 were both clean on 7938a97b, which differs from this head only by the rebase. No item became a follow-up.

The one lane still without a verdict is the fork Internal Content Scan, which failed at PR resolution on the previous head (runner could not reach api.github.com); this push re-triggers it.

@cixuuz

cixuuz commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

Triage of the red on 6d8868990aec: Bundle Size Gate fails on assets/ModelEffortDropdown-*.js (505.0 KB against the 500.0 KB default budget, over by 5.0 KB). The red is main's, not this PR's: a local vite build --mode analyze of pristine main at this head's base 0dab19863294 fails scripts/check-bundle-size.mjs with the identical chunk (519.85 kB, the same 5.0 KB over), and this branch's build reports the same byte count for it, so the PR adds nothing to that chunk. Tracked in #23866, fix in #23867. No change here; the next rebase picks the fix up once it lands.

@cixuuz

cixuuz commented Oct 25, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main c4ce1efa5d77, which carries #23867 (the ModelEffortDropdown bundle-size ceiling, the red main had at this PR's previous base; see #23866). Main also moved in website/src/App.tsx, docs/feature-map/README.md and the i18n catalogs since 6d886899; the merge was clean and the local gates (build, lint, bundle-size gate: 926 chunks within budget, i18n check and render, 13,135 scoped frontend tests) are green on 7ebd1d2483a1. The diff is unchanged from 6d886899, which GPT 6.1 and Opus 5.5 judged clean. CI re-running.

@cixuuz

cixuuz commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor Author

span=bebb7e9e4da7
self-added: yes
mechanism: the host's zero-height overlay row in components/AppPopoutControl.tsx, whose only hit target is the 28px pop-out control in an app page's top-trailing corner

  • Pop-out overlay intercepts File Explorer’s new-workspace button — fixed in 7181251

Confirmed in the code: the control spans 12–40px from the page's right edge and 6–34px from the top (mt-1.5 mr-3, w-7 h-7); File Explorer's .mc-fe-tabs has padding: 6px 8px 0 with the "+" button (~33×25px) as its last child, so once the strip overflows and is scrolled right the "+" ends 8px from the edge, under the control.
Fix: TabStrip.tsx adds a trailing non-interactive spacer (.mc-fe-tab-gutter, flex: 0 0 36px, aria-hidden) after the "+" button, so it ends 44px from the edge, clear of the control; the spacer also carries focus-caption-reserve, so in focus mode on Windows and frameless Linux it widens by the same caption inset the control itself dodges.
The adjudication named "a trailing gutter on the tab strip" as an acceptable fix; the overlay placement itself is unchanged (Design and UX passed it with the five screenshots), and the control's doc already states the tradeoff that an app header putting something in that corner loses at most the pixels under the button.
Test: fileExplorerComponents.test.tsx "keeps a gutter after the new-workspace button so the host pop-out control cannot cover it" (last child of .mc-fe-tabs is the gutter, aria-hidden, and it follows the mc-fe-tab-new button). tsc, eslint, the bundle-size gate and 13,154 scoped frontend tests are green on 7181251.

@cixuuz

cixuuz commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor Author

span=622d6b39011e
self-added: yes
mechanism: the fixed 36px trailing gutter in File Explorer's tab strip, which fit only the host control's 28px icon state

  • The gutter does not clear the labeled pop-out control — fixed in 5cce72d

Confirmed: the Popped out chip (icon plus a localized label) and the pop-out window's Return chip are wider than the 28px icon the 36px gutter was sized for, so with the strip overflowed and scrolled right the chip still covered the "+" button.
Fix: the gutter no longer carries a number. AppPopoutControl measures its rendered wrapper (getBoundingClientRect once, then a ResizeObserver, so a state change from icon to chip is followed) and publishes --mc-app-popout-reserve = width + 16px (its 12px margin plus 4px clearance) on the document, clearing it on unmount; .mc-fe-tab-gutter is calc(var(--mc-app-popout-reserve, 44px) - 8px) (the strip's own 8px padding counts toward the footprint; the fallback keeps 36px when no control is mounted). The control's three visual states are unchanged.
Tests: AppPopoutControl.test.tsx "publishes its rendered footprint as --mc-app-popout-reserve and clears it on unmount" (a 90px wrapper publishes 106px, a resize to 120px publishes 136px, unmount removes the property and disconnects); the tab-strip gutter test from the previous head is kept. tsc, eslint, the bundle-size gate and 13,155 scoped frontend tests are green on 5cce72d.

@cixuuz

cixuuz commented Oct 25, 2026

Copy link
Copy Markdown
Contributor Author

Design change by the author, pushed as 5ba24cc (rebased onto main af11e17).

The pop-out entry point moves from the app page to the sidebar. The previous head overlaid a control on every app page's top-trailing corner, and two review rounds in a row found an app's own chrome under it (File Explorer's "+" button, then the wider Popped out chip), each answered with a reserve (a fixed gutter, then a measured --mc-app-popout-reserve). A third finding of that class would have meant the design was wrong, not the padding, so instead of waiting for it the author chose the sidebar:

  • Each Apps-group row of the expanded desktop rail carries a pop-out action beside it (shell/nav/appRowPopoutAction.tsx), shown on row hover or keyboard focus. On the active app's row it opens the window at the page and query on screen; on any other row, at the app's route. While the app is out, the row shows an always-visible accent mark that focuses its window. The action is a sibling of the row's role="button", not a child, and stops the press events dnd-kit's sensors start a reorder from.
  • The pop-out window gets a real 32px title bar with the app's name and Return (components/AppPopoutTitleBar.tsx), a row in the layout rather than an overlay.
  • Removed: components/AppPopoutHost.tsx, components/AppPopoutControl.tsx, the File Explorer tab-strip gutter and --mc-app-popout-reserve, and the popped_out string (icon-only mark). Nothing is drawn over an app page any more, so no app needs to reserve space. The collapsed rail, the phone drawer and the drag ghost carry no action.

Tests: AppPopoutControl.test.tsx is replaced by AppPopoutEntryPoints.test.tsx (the action's two states and what each click opens, hover/focus-only visibility, press events stopped at the action, sibling placement; the title bar's Return, height and truncation) and navRailAppPopoutAction.test.tsx (through the real rail: beside each Apps-group row on the expanded rail, none on the collapsed one); AppPopoutFrame.test.tsx gains the title bar cases. On this head: tsc, eslint, the six pop-out test files three times plain and three times shuffled (146 tests), the scoped frontend suite (483 files, 13,138 tests), i18n check and render, feature map, brand and bundle-size gates all green.

The description is rewritten for the new design and the five screenshots are recaptured from this head's website/dist (the entry-point and mark shots are new; the side-by-side is now two windows from one browser run). The Design, First-Principles and UX verdicts from the previous head were about a different UI and need a fresh read.

@cixuuz

cixuuz commented Oct 25, 2026

Copy link
Copy Markdown
Contributor Author

Re: HOLD (crew-pr-reviewer r120, head d604a96), following up on the previous reply.

The current head is 5ba24cc. It carries a design change (details in the comment above): the pop-out entry point moved from an overlay on the app page to a hover action on the sidebar's app row, and the pop-out window got a title bar with Return. Of the three held items, two stand as before (the appPopout.ts re-exports and unused hook fields are gone; the Not a goal line names the shared navFallbackUrl seam). The third, the screenshots, is re-done for the new design: five frames under temp-screenshots/app-popout/ (row action, popped-out mark, pop-out beside chat, no-app notice, list-error notice), all rendered from this head's website/dist, shown in the description.

@cixuuz

cixuuz commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor Author

span=d78a420f7f6e
self-added: no
mechanism: the engine's no-main fallback tab (navFallbackUrl), which this PR extends with the prompt query but which built its URL from the app-space path alone

  • The no-main fallback opens a URL outside the dashboard on a sub-path build — fixed in ad77724

Confirmed: navFallbackUrl returned ${origin}${intent.path}, and intent.path is app space (/chat) for every caller (the frame's handOffToMain strips the router basename; the artifact companion passes the path it also hands to main's navigate), so a KIROCREW_BASE_PATH=/crew build opened https://host/chat?… outside the mount, with the prompt already consumed from the pop-out.
Fix: the URL is ${origin}${withBase(intent.path)}${qs}, as navigate in the same file already does; withBase leaves a path already under the base unchanged, so nothing doubles.
Tests: new popoutNavForwarding.basePath.test.ts mocks lib/basePath with base /crew and asserts the fallback tab URL …/crew/chat?autoSend=1&prefill=…&new=1, plus /crew/artifacts and /crew/ for a bare path and the root; the root-base cases in popoutNavForwarding.test.ts are unchanged and green. tsc, eslint, the scoped frontend suite (484 files, 13,148 tests) and the i18n render gate are green on ad77724.

@cixuuz

cixuuz commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor Author

span=18e45ae36123
self-added: no
mechanism: sendErrorToChat's hard path, a full location.assign('/chat') meant for a boundary card whose React tree may be what threw, which the pop-out window inherited unchanged

  • A hard "Ask the agent" in an app pop-out turns the window into a second dashboard — fixed in ad77724

Confirmed: BuiltinAppRoute's boundary card and the route-level ErrorBoundary both render AskAgentButton … hard, whose sendErrorToChat did window.location.assign(withBase('/chat')) in the pop-out, bypassing the pinned navigator and AppPopoutHandoff: the pop-out loaded /chat, dropped its registration, and main received nothing.
Fix: utils/errorReport.ts gains a hard-exit seam, installHardExit(fn | null). On the non-soft path sendErrorToChat stages the prompt as before and then, when a hard exit is installed, calls it and returns; the page load stays the fallback where none is installed (the main window, the chat/artifact/terminal pop-outs). AppPopoutFrame.tsx exports AppPopoutHardExit, which installs the frame's existing handOffToMain(store, '/chat', ''): it drains the staged prompt into errorHandoffs and forwards the /chat intent over the pop-out channel, exactly as a soft exit to /chat does, with no navigation in the pop-out. App.tsx renders it above the pop-out <Routes>, outside every route-level boundary, so a crash of the frame itself does not unmount the sink. Both sides are plain JS, so a crashed React tree cannot take the hand-off down, which is what hard is for.
Tests: AppPopoutFrame.test.tsx mounts the frame as App.tsx does with a builtin that throws during render, and with a frame whose registry lookup throws: clicking "Ask the agent" on either card calls forwardToMain once with path: '/chat' and one errorHandoffs entry carrying the thrown message, empties the pop-out's staged queue, never calls location.assign, and leaves the router location and key unchanged (both fail with the sink removed). errorToAgent.test.tsx covers the seam: staging before the sink, a refused staging calls no sink, the soft path never uses it, installHardExit(null) and __resetNavSeamForTests restore the full-load path. A read-only Opus 5.5 verifier confirmed both boundaries closed and no new Critical/High; the one remaining hard location.assign reachable from a pop-out is IncidentChat.tsx's embed-navigation exit, an in-app path listed as a follow-up.

@cixuuz

cixuuz commented Oct 25, 2026

Copy link
Copy Markdown
Contributor Author

Red fix, pushed as a5140a6 (rebased onto main 2e0b123).

Frontend Tests (2) on ad77724 failed one test, hoverVariantPolicy.test.ts › every hidden-until-hover site has a (hover: none) state or is listed as decorative: the sidebar row's pop-out action was opacity-0 group-hover/approw:opacity-100 focus-visible:opacity-100, and with hover: gated on (hover: hover) a touch screen could never reach it. It is a control, not decoration, so it now carries the policy's touch state: [@media(hover:none)]:opacity-100 with a 32px target ([@media(hover:none)]:w-8 h-8 right-1), always shown where there is no hover. Desktop rendering is unchanged, so the screenshots stand. AppPopoutEntryPoints.test.tsx pins the touch class; hoverVariantPolicy.test.ts is green locally (the scoped runner does not select that repo-wide scan, which is why it was first seen here).

Also on ad77724, Cross-Platform Portability was cancelled at actions/checkout after the 5-minute job limit; nothing of ours ran there, and this push re-queues it.

@cixuuz

cixuuz commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor Author

span=839826d8e0d5
self-added: yes
mechanism: the storage-refused fallback in applyNavIntentInMain, added in an earlier round so a forwarded error prompt main's sessionStorage refuses to re-stage would not be lost; it published the prompt through Redux pendingInput

  • Storage-failure fallback overwrites an existing chat draft — fixed in 6a7d999

Confirmed: with main on another page and a retained session holding an unsent draft, a refused handoffToChat made the fallback publish the diagnostic as pendingInput; usePendingInputIntake (no autoSend flag) then ran setDraft/setInput on the active slot and persisted over that draft, with no fresh session created.
Fix: the fallback is now the chat page's own draft-only fresh-session launch. For a chat destination main navigates to ${path}?new=1&prefill=<prompts> (the destination's sid/slot dropped, its other parameters kept), the same form the engine's no-main fallback tab uses: the new=1 effect creates a fresh slot and seeds the prefill into it, and even its own refused-storage branch publishes only after that created slot is active, so a retained session's draft is never written. setPendingInput is no longer called for error prompts. A prompt paired with a non-chat destination is dropped rather than published: every "Ask the agent" targets /chat (sendErrorToChat navigates there after staging; the pop-out's hard exit hands off /chat), so such a prompt is a stale leftover, and losing it is the lesser harm than overwriting a draft.
Tests (popoutNavForwarding.test.ts): a refused re-stage navigates once to /chat?new=1&prefill=… (two prompts joined), never calls setPendingInput or switchSlot, and leaves no kirocrew_error_handoff in storage; a destination with its own query and a named session keeps autoSend=1, drops sid, still switches to the intent's slot; a /settings destination navigates unchanged with nothing published. The accepted-storage path (stage + notifyChatHandoffs, no pendingInput) is unchanged. tsc, eslint, the scoped frontend suite (485 files, 13,273 tests) and the i18n render gate are green on 6a7d999.

@cixuuz

cixuuz commented Oct 25, 2026

Copy link
Copy Markdown
Contributor Author

Triage for the other reds on a5140a6, cleared by 6a7d999 (rebased onto main 0b9a470).

  • Backend Tests (3.12, 4) failed one test, test/test_governance_policy_viewer.py::TestNoPolicyDefault::test_enterprise_unnamed_scope_reports_the_baseline (PlatformCompositionError … could not fetch the ceiling). This PR changes no Python; the test fails the same way on the base alone, and main fixed it in test: build the governance viewer context before switching to enterprise #23933 (663147f, after this PR's previous base). The rebase carries that fix. Coverage Gate failed only because that shard did (backend-test=failure -- failing closed).
  • Opus 5.5 Review ended as "review incomplete" with no verdict for a5140a6; this push re-runs the lane.

GPT 6.1's blocking finding on that head has its own disposition above (fixed in this push).

@cixuuz

cixuuz commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor Author

span=03186cb4505e
self-added: no
mechanism: navFallbackUrl, the engine's no-main fallback tab, which named the intent's session (?sid=) and carried the first error prompt as ?prefill=

  • No-main error fallback overwrites an existing session's draft — fixed in 246101d

Confirmed: an exit carrying both a session (slotKey, or a replayed ?sid=) and errorHandoffs, with no main window to claim it, opened /chat?sid=<existing>&prefill=<diagnostic>; ChatPage's keyed prefill then replaced that session's composer and persisted over its unsent draft. Same class as the in-main fallback fixed on this PR's previous head, at its second and last site: every error-prompt path (ChatPage's own hand-off intake, the in-main re-stage fallback, this tab) now opens a fresh session.
Fix: when the intent carries error prompts the fallback URL names no session: sid is not set from slotKey, any sid/slot in the replayed query is dropped, and the existing rule adds new=1, so the tab is /chat?…&prefill=<first diagnostic>&new=1, the chat page's draft-only fresh-session launch. Non-error intents keep their session and ?new=1 only when none is named.
Tests (popoutNavForwarding.test.ts): an error hand-off beside slotKey: 'chat-42' opens …/chat?prefill=Diag&new=1 with no sid; one beside a replayed ?sid=chat-9&autoSend=1 opens …/chat?autoSend=1&prefill=Diag&new=1; the non-error sid + prefill, pendingInput, sentinel and launch cases are unchanged. tsc, eslint, the scoped frontend suite (485 files, 13,279 tests) and the i18n render gate are green on 246101d.

@cixuuz

cixuuz commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor Author

span=0b589d9ea58f
self-added: no
mechanism: handOffToMain's drain of this window's kirocrew_error_handoff queue, which forwards whatever the queue holds, and recoverClaimedChatHandoffs's module-load promotion of a persisted claim

  • Inherited recovery entries replay already-delivered diagnostics — fixed in 246101d

Confirmed: a window window.open creates starts with a copy of its opener's sessionStorage, so a pop-out opened while main was delivering an error hand-off inherited main's persisted FIFO (kirocrew_error_handoff_claimed) and any staged queue; module-load recovery promoted the copied claim into the pop-out's own queue, and a later exit from the pop-out forwarded it with an unrelated intent, so main re-staged it and useErrorHandoffIntake opened a second diagnostic session.
Fix, at the source rather than at the drain: utils/errorReport.ts gains dropInheritedChatHandoffs(), run at module load before recoverClaimedChatHandoffs(). In an app pop-out document (appPopoutRouteOf(appPathname())) whose storage carries no kirocrew_app_popout_storage_owned marker, it removes kirocrew_error_handoff and kirocrew_error_handoff_claimed and writes the marker. Main never writes the marker, so the first document of a new window (storage copied from main) is told apart from a reload of that window (storage the window itself wrote), whose own claim still recovers. Main and the other pop-outs are untouched.
Tests (errorToAgent.test.tsx): the first document of an app pop-out drops both copied keys, marks the storage as its own, and recovery then promotes nothing; a reload with the marker keeps and recovers the window's own claim; a main-window document is untouched and writes no marker; after vi.resetModules() and a fresh import under /popout/app/dev-fleet with a seeded claim, consumeChatHandoff() is null and the marker is set, pinning the order against module-load recovery.

@cixuuz

cixuuz commented Oct 25, 2026

Copy link
Copy Markdown
Contributor Author

Red fix, pushed as c949bbd (same base, main 695ec6a).

Flake Pattern Ratchet on 246101d failed on website/src/test/errorToAgent.test.tsx: J3-unpinned-date 5 > 3: the two new inherited-storage cases hand-wrote queue entries with ts: Date.now(). They now stage the entry through handoffToChat, the production path that stamps it, so the file carries no new unpinned clock read; the ratchet (test/test_flake_pattern_ratchet.py, 60 tests) is green locally against this base. Test-only change: the two fixes dispositioned above for 6a7d999 are carried unchanged in this head.

@cixuuz

cixuuz commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor Author

span=e0b133c1fa02
self-added: no
mechanism: dropInheritedChatHandoffs's kirocrew_app_popout_storage_owned marker, which outlived the pop-out document that wrote it

  • Clear pop-out ownership when the window becomes a dashboard — fixed in b786c56

Confirmed: when Return falls back to loading the dashboard in the pop-out's own tab, that tab is now a main window but its sessionStorage still holds the ownership marker. A pop-out it opens later inherits the marker along with main's pending diagnostic, so the guard at errorReport.ts:576 reads it as a reload and skips the drop, and the copied claim can be forwarded back as a duplicate session.
Fix, as the adjudicator named it: on any document that is not an app pop-out, dropInheritedChatHandoffs() now removes the marker before returning. App pop-out reloads keep it, so their own claims still recover. Main's hand-off keys are not touched.
Test (errorToAgent.test.tsx): a tab carrying the marker loads /chat and the marker is cleared; a pop-out it then opens at /popout/app/file-explorer with an inherited claim drops it, and recovery promotes nothing. The existing reload-keeps-its-claim test still passes.

@iamwhatever iamwhatever left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes. Reviewed head b786c56.

The pop-out design is sound, every GPT and Opus finding has a fix or a reasoned rebuttal, the items from my earlier hold are addressed, and every check on this head is green. Two kinds of item are still open:

  1. Follow-ups without an issue. The Opus disposition of 2026-10-25 says the hard location.assign in IncidentChat.tsx#L70-L73 is still reachable from a pop-out and is "listed as a follow-up", and the GPT rebuttal of 2026-10-24 says carrying autoSend through ChatPage's ?sid= rewrite is for "a follow-up". I found no issue for either. Please fix them here, or open one issue per item and link both in the description.
  2. UX notes on this head with no reply, since the sidebar redesign: the popped-out mark uses the same ExternalLink glyph as Pop out, so the state is shown mainly by colour (appRowPopoutAction.tsx#L54-L72), and an /apps/<name> pop-out shows a blank area while the app list loads (AppPopoutFrame.tsx#L194-L201). Fix each or reply with a reason. A one-line answer to the Design note on deferredPendingInputRef would also close it.

— crew-pr-reviewer (r126)

@cixuuz

cixuuz commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Reply to the request for changes at b786c56 (crew-pr-reviewer r126). Everything below is on the new head 0d1e13f, rebased onto current main.

1. Follow-ups without an issue. Both now have one, linked from the description's Related Issues:

2. UX notes since the sidebar redesign. Both are fixed:

  • Popped-out mark shown only by colour: the mark now uses its own glyph (AppWindow) instead of the ExternalLink arrow, so its shape tells the state apart as well as its colour (shell/nav/appRowPopoutAction.tsx). AppPopoutEntryPoints.test.tsx pins which glyph each state renders. The app-row-popped-out-mark.png screenshot is recaptured.
  • Blank /apps/<name> window while the list loads: the frame now shows the same "Loading app…" line AppPage uses (pages/AppPopoutFrame.tsx), with role="status". AppPopoutFrame.test.tsx asserts it, and a new screenshot popout-app-list-loading.png shows it.

Design note on deferredPendingInputRef: agreed. The shell holds the forwarded prompt until its own navigation commits because ChatPage takes pendingInput under whatever query is current. Keying ChatPage's intake to its destination would let the shell drop that matching logic. That is a ChatPage change with its own callers, so it is out of scope here; the shell-side hold is the contained fix.

Rebase fallout: two tests failed because main changed under them. I updated both, and the behaviour they check is unchanged:

Every app page (a builtin like Dev Fleet, or an installed app at
/apps/<name>) gets the chat title bar's pop-out control in its
top-trailing corner. Pop out opens the app under /popout/app/<route>
in its own OS window, at the page and query the user is on; the main
window shows a Popped out chip that focuses it; the pop-out carries a
Return control in the same spot.

The pop-out window mounts its router at /popout/app, so the app's own
absolute paths resolve inside it unchanged. A navigation that leaves
the app (a "go to chat" exit, a link to Settings) is handed to the
main window before it commits, with its query string, the session the
app activated and any staged pendingInput, so the pop-out stays on its
app with its state intact; an exit link's href is the main dashboard's
URL, so a middle or modifier click opens that page, not a second
pop-out.

Closes kirodotdev#22886

@iamwhatever iamwhatever left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at 8ded073.

Thank you for working through my r126 request. Both follow-ups now have open issues linked in the description (#26430, #26431). The popped-out mark uses its own AppWindow glyph (appRowPopoutAction.tsx L74-L76), and an /apps/<name> window shows a loading line while the list loads (AppPopoutFrame.tsx L264-L274). The pop-out reuses the existing same-origin pop-out engine on its own BroadcastChannel. Every lane passes, every check is green, and the branch merges cleanly with main.

AI findings judged non-blocking

  • Design: UNSAFE_NavigationContext has no stability promise. AppPopoutFrame.test.tsx drives real Link/useNavigate through the wrapped navigator, so an upgrade that breaks it fails those tests (L117-L130).
  • Design: the Backwards compatibility section fell past the description cut. The lane found no breaking reader.
  • UX: dev-fleet-popout-beside-chat.png still shows the old glyph. This affects the evidence image only, not the code.
  • UX: the not-found heading shows the raw route (L292). Wording nit.
  • First Principles: the /chat?new=1&prefill= error-handoff fallback is called oversized. The pop-out has already spent its queue, so without it the prompt is lost, and it opens a fresh session rather than overwriting a draft (navIntent.ts L192-L208).

— crew-pr-reviewer (r138)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pop out an app page (Dev Fleet, installed apps) into its own window

2 participants