Repository navigation
feat(apps): pop out an app page into its own window - #22926
Conversation
Design Review (Opus 5.5, fork) — ✅ PASSDesign-level review of Design-Verdict: PASS Nothing blocks: the change reuses the existing pop-out engine, mounts the window's router at Notes
[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 |
UX Review (Opus 5.5, fork) — ✅ PASSUX-level review of 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
[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 |
First Principles Review (Opus 5.5, fork) — ✅ PASSPremise-level review of 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
What this change shipsInventory (10 items) — 9 justifiedIntent: keep an app page on screen in its own OS window beside the main window's chat. This is an ADDITION.
[FIRST-PRINCIPLES-REVIEWED] 8ded073 Description read[DESCRIPTION-READ] 529f42fc3696ee8980984385e2c16552c72042a5e07038598559e6105f184760 That sha256 names the PR description this verdict read. Recompute it with |
GPT 6.1 Review (fork) — ✅ no blocking findingsReviewed Review detailsNo findings. |
f85fa25 to
a33dc06
Compare
|
span=0b589d9ea58f
|
|
span=c500a5e06d3c
|
Opus 5.5 Review (fork) — ✅ no blocking findingsReviewed |
a33dc06 to
dac2761
Compare
|
span=0b589d9ea58f
|
|
span=0b589d9ea58f
|
|
span=0b589d9ea58f
|
|
span=381c15af5376
|
dac2761 to
d87b23b
Compare
|
span=381c15af5376
|
|
span=96a7c8d01039
|
|
span=434384425704
|
|
span=839826d8e0d5
|
|
Rebased: main moved in the i18n catalogs ( |
|
Rebased: main moved in the i18n catalogs ( |
|
Re: HOLD (crew-pr-reviewer r120, head d604a96), following up on the earlier reply. The current head is 6d88689 (rebased onto 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 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. |
|
Triage of the red on |
|
Rebased onto |
|
span=bebb7e9e4da7
|
|
span=622d6b39011e
|
|
Design change by the author, pushed as 5ba24cc (rebased onto 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
Tests: The description is rewritten for the new design and the five screenshots are recaptured from this head's |
|
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 |
|
span=d78a420f7f6e
|
|
span=18e45ae36123
|
|
Red fix, pushed as a5140a6 (rebased onto
Also on ad77724, |
|
span=839826d8e0d5
|
|
Triage for the other reds on a5140a6, cleared by 6a7d999 (rebased onto
GPT 6.1's blocking finding on that head has its own disposition above (fixed in this push). |
|
span=03186cb4505e
|
|
span=0b589d9ea58f
|
|
Red fix, pushed as c949bbd (same base,
|
|
span=e0b133c1fa02
|
iamwhatever
left a comment
There was a problem hiding this comment.
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:
- Follow-ups without an issue. The Opus disposition of 2026-10-25 says the hard
location.assignin 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 carryingautoSendthrough 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. - UX notes on this head with no reply, since the sidebar redesign: the popped-out mark uses the same
ExternalLinkglyph 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 ondeferredPendingInputRefwould also close it.
— crew-pr-reviewer (r126)
|
Reply to the request for changes at b786c56 (crew-pr-reviewer r126). Everything below is on the new head 0d1e13f, rebased onto current 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:
Design note on Rebase fallout: two tests failed because
|
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
left a comment
There was a problem hiding this comment.
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_NavigationContexthas no stability promise.AppPopoutFrame.test.tsxdrives realLink/useNavigatethrough 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.pngstill 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)
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
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.?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.opendedupe 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.shell/nav/appRowPopoutAction.tsx,shell/nav/navRail.tsx)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'srole="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.components/AppPopoutTitleBar.tsx)<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.pages/AppPopoutFrame.tsxat/popout/app/<route>)BuiltinAppRouteorAppPage), 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 (whichAppPagewould 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 ofAppPage. While the list is still loading, the window shows the same "Loading app…" lineAppPageshows for its own load, never a blank area.main.tsx,utils/appPopoutRoute.ts)/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%20bsettles on its not-found notice instead of handing off to main on every render.pages/AppPopoutFrame.tsx,utils/navIntent.ts,utils/popoutController.ts,utils/errorReport.ts,shell/appShell.tsx)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 ReduxpendingInput, the session it activated (a switch, a creation or a fork), the App SDK'swindow.__mc_chat_launchintent, and the error prompts an "Ask the agent" control staged. Main publishes the forwardedpendingInputto its store only once the navigation it was staged for has committed: Redux re-renders synchronously whileBrowserRoutercommits 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=1launch. 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 anErrorBoundaryshows for a crashed app, or for a crashed frame) never loads/chatin the pop-out: the window installs a hard exit (installHardExitinutils/errorReport.ts, installed byAppPopoutHardExitabove the route boundary inApp.tsx), sosendErrorToChatstages 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).utils/popoutController.ts)/crew/chaton aKIROCREW_BASE_PATH=/crewbuild). 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.utils/appPopout.ts,hooks/useAppPopouts.ts)utils/errorReport.ts)window.opencreates 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.tsxmounts the window's routes.hooks/panelTabRegistry.tsreports 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 ownChatLaunchOptions. The new strings are ini18n/locales/en.jsonand the thirteen machine catalogs.docs/feature-map/README.mdnames the route.Backwards compatibility
Compatible: every existing route, pop-out and chat URL form behaves as before; the new
NavIntentfields are optional, anduseInstalledAppsgains 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 nameddetailormigrate(the App Store's own routes for those words need a third segment, so only/apps/libraryand 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;openPopoutopens that URL under the route's window name;routerParamFormleaves a plain route alone, decodes each segment, turns an encoded slash into a slash, leaves a malformed escape undecoded, and agrees with the paramsmatchRoutesreads 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)statehoverVariantPolicy.test.tsrequires 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 seespointerdown,mousedown,touchstartorkeydown, 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 andnavigate(-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 anavigate; 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/appbasename; 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/appbasename; 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'ssid; 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 throughAppPage; 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 neverAppPage; 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 oneerrorHandoffsentry on a/chatintent, empties this window's staged queue, never callslocation.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__resetNavSeamForTestsrestore 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/crewbuild 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=1sentinel 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);applyNavIntentInMainstagespendingInput, 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 nopendingInputand no slot switch, keeps the destination's other query parameters while dropping itssid, and drops a prompt paired with a non-chat destination without publishing it;claimChatLaunchfresh, at the ten-second edge, past it, and absent, on a pinned clock;takeDeferredPendingInputwith nothing staged, an unchanged router key, an equal committed location, the inclusive ten-second edge against one past it, the user's/chatcommitting 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 forwardedpendingInputreaches the store only once the destination query has committed (the publication is observed with/chat?autoSend=1&newSession=1already 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=1prompt, 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 (seeds7311,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 throughwindow.openat/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 bypopoutNavForwarding.test.ts).Screenshots / video
Committed under
temp-screenshots/app-popout/rather than attached: this account has read access only, andgh --attachanswers that with a 404 (cli/cli#14302). All six were rendered from a build of this PR'swebsite/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.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=1dropped by ChatPage's?sid=rewrite, shared with the Command Bar).Checklist
feat|fix|docs|style|refactor|perf|test|chore|ci|build|revert: ...)