feat: add contributes.panelTabs app-manifest contribution - #7975
Conversation
github-actions
Bot
added
fork
UX Review (Fable 5.1, fork) — 🟡 CONCERNSUX-level review of I have enough to review. This is a fork-lane UX review: no blind read, no fork checkout, and the PR explicitly ships Reconcile — user-visible controls the diff adds (all app-data-driven, none present in the base repo, none in any screenshot, no blind reader has seen them):
No BLOCK exits fire: no blind read (so the primary-control exit is disabled by lens 12); the orphaned-tab hide/restore is app-lifecycle (lens 4), not a hard swap of a persistent identified element (lens 13); the error notice carries a real UX-Verdict: CONCERNS No first-time reader has seen any of the contributed-tab surfaces, and none can appear in a screenshot without a demo app declaring Evidence gaps
Suggestions
[UX-REVIEWED] e934f8f |
Design Review (Fable 5.1, fork) — ✅ PASSDesign-level review of Design assessment complete. This adds The load-bearing design decisions are all sound:
Failure modes (app disabled mid-session, prune-on-read vs. delete, cross-slot mount survival) are handled and pinned by tests. Nothing rises to a design concern; remaining items are line-level and out of scope. Design-Verdict: PASS Sound, proportionate reuse of the existing in-process app host; the one real risk (repointing a signed, executed [DESIGN-REVIEWED] e934f8f |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed Review detailsI've verified the full chain from the diff and the surrounding code. The working tree sits on Re-deriving the candidate against the diff:
The mechanism is solid and reachable. Severity is low: the body is null so nothing runs inside, no crash/data-loss/security — advisory only. This is the sole candidate; it does not block. No blocking issues; one advisory finding. FINDING — website/src/hooks/usePanelTabs.ts:312 — [OPUS-REVIEWED] e934f8f |
First Principles Review (Fable 5.1, fork) — 🟡 CONCERNSPremise-level review of I have enough to complete the review. Base facts confirmed: First-Principles-Verdict: CONCERNS Large permanent public surface (manifest field + SDK What this change shipsIntent: let an installed app own a persistent chat side-panel tab that mounts its own ESM bundle, without forking core tab machinery. This is an ADDITION.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] e934f8f |
GPT 5.6 Review (fork) — ✅ no blocking findingsReviewed Review detailsNo findings. |
github-actions
Bot
added
readiness: passed
bolichen97
left a comment
bolichen97
left a comment
There was a problem hiding this comment.
Thanks — verified the headline claim: ViewKind (website/src/hooks/usePanelTabs.ts:9) is untouched and all four exhaustive Record<ViewKind,…> tables (VIEW_TITLE_KEY, NEW_MENU_LABEL_KEY, NEW_MENU_DESC_KEY, NEW_MENU_GROUPS) stay exhaustive. Body-owning TabKind is the right call. Diff is free of product-specific naming. Requesting changes for the typing decision, one real persistence bug, and a direction change on my side.
Direction
Pippin will ship as a standalone app end to end (own repo + catalog entry), not "app backend + edition thin frontend". So the seam has to be usable by an installed app. As written it is edition-only: the descriptor holds a live ComponentType + ReactNode icon, registered by a build-time ESM side effect through virtual:kirocrew-edition, and read non-reactively at module load — an app bundle loaded at runtime cannot register into it. The runtime app TabKind (McpAppTabBody) does not cover this either: it is a per-tool-call MCP App render keyed by toolCallId, ephemeral and never persisted.
Please make the seam declarative, in the style of contributes.commands / publishProviders: contributes.panelTabs[] in app.json with id, title, menuLabel, icon (lucide name), and route/entry; core renders the tab as an app iframe (the same host ui.pages already use) and persists {kind: 'app-tab', appName, tabId}. The edition then registers nothing — it just installs the app.
Findings on 76a40b4
usePanelTabs.ts:21—(string & {})collapseskeyof TabKindtostring, soKIND_ICON: Record<TabKind, ReactNode>(SidePanel.tsx:42) degrades to an index signature: a future built-in kind that forgets an icon compiles and renders nothing, andtab.kind === 'termnal'-class typos stop being errors. Use a template-literal member instead —type RegisteredTabKind = `edition:${string}`(orapp:${string}per the above) — which keeps every built-in literal required in mapped types while admitting registered keys; typePanelTabDescriptor.kindandopenPanelTab()with it and keepPANEL_TAB_KIND_REonly for persisted/untrusted input.usePanelTabs.ts:214—let store = loadPersisted()runs the descriptor prune at module evaluation, not "at render" aspanelTabRegistry.tssays. ESM evaluates an edition module's imports before its top-levelregisterPanelTab(), so a tab component that transitively importsusePanelTabs/SidePanelgets its persisted tab pruned on every reload — silent tab loss. Move the prune to the read path (project it in the hook'stabsmemo next tolocaliseTitles); that also handles a contributor removed mid-session.SidePanel.tsx:1006—<Body projectDir={projectDir} />drops theactiveflagTabBodyalready receives (:817). A registered tab stays mounted while hidden, so its body cannot stop polling or release Escape/Cmd+S. One-line additive fix now; a migration later.panelTabRegistry.ts:36—menuDescriptionhas no read site (the+rows render only icon + label; the empty-state launcher that renders descriptions is untouched). Wire the launcher or drop the field.SidePanel.tsx:725— the empty-panel launcher iterates built-ins only, so a registered tab is reachable only from the+dropdown while the launcher presents itself as the full set.usePanelTabs.ts:418-420— theactiveIdrepair now runs for every bucket, not just pruned ones; declare it or scope it, since the PR claims byte-identical behavior with an empty registry.website/docs/extension-seams.md:15-17— "exercises each one except the source-provider seam" is now false; the PR edits this file three lines below.- Tests:
panelTabRegistry.test.tscovers register/lookup/collision. Missing: a registered kind takes theTabBodyintercept (notActivityViewer), the+row opens it and dedupes to one instance, and register → persist → reload-without-descriptor prunes correctly. - CI: Screenshot Evidence is red — add the
<!-- no-visual-delta -->+**Why no screenshot:**markers.
|
Thanks @bolichen97 — adopting the direction. Reshaping from the edition-only registry (a live Plan for this PR
Two grounding notes vs. the review wording
Defects I'll fix
Will push the reshaped revision shortly. |
76a40b4 to
793f716
Compare
suhasaitham22
changed the title
|
Pushed the declarative reshape (
Three TS interactions I couldn't build locally are called out in the description for CI to confirm. The "autolink rules" seam in |
bolichen97
left a comment
bolichen97
left a comment
There was a problem hiding this comment.
Thanks — the direction is now right. The seam is genuinely declarative and app-reachable (app.json contributes.panelTabs → PanelTabConfig → persisted manifest → /api/apps → resolvePanelTabs → TabBody intercept → AppHost), persistence as {appName, appTabId} checks out (survives serializeBucket's kind !== 'app' filter since 'app:pippin:browser' !== 'app', MCP app kind stays excluded, disabled/uninstalled apps' tabs disappear cleanly and return on re-enable), and the code-level findings from last round are addressed except one. Three things block.
Blocking
1. contributes is not net-new — this PR redefines a class that already exists on main, and a naive conflict resolution silently deletes contributes.commands.
Your claim was true when the commit was authored (2026-09-02 18:36Z) and became false ~3h later when #7423 (147010ce) merged class Contributes with commands: list[CommandContribution] at src/kiro_crew/apps/manifest.py:1556 (+ _KNOWN_FIELDS :1675, AppManifest field :1744, discovery.py:64-68 persisting it, types.ts contributes?: { commands?: unknown }). This PR's class Contributes declares panelTabs as the only field. I reproduced the merge: hard conflicts in manifest.py (4 regions), test/test_app_manifest.py, website/src/components/appstore/types.ts. If the PR side wins any of them, Contributes.to_dict() stops emitting commands, discovery.py writes that into the persisted app.json, and every builtin's Command Bar rows vanish with no error.
Fix: rebase onto main and add panelTabs as a second field on the existing Contributes, keeping commands in to_dict/from_dict/validate. The exposure path you'd otherwise need to build already exists, so this is mostly deletion.
2. contributes.panelTabs[].icon is not app-reachable — an installed app can never supply one.
SidePanel.tsx iconForKind resolves the manifest icon through getBuiltinIcon(d.icon), a hardcoded 15-entry allowlist (website/src/apps/builtinIcons.tsx:42) populated only by registerBuiltinIcons(), which its own header documents as "expected at module-load time (edition composition) … not reactive". A runtime-installed app cannot register into it — the same edition-only dependency the last review rejected, relocated to the icon dimension. Your own documented example and test fixture use icon: "BookMarked", which is not in the registry, so the canonical example silently renders the generic <PanelRight> fallback.
Fix: resolve the name against lucide-react's icon map directly (with an allowlist/fallback), or restrict the set and correct the doc/fixture.
3. mergeable_state: dirty, and no CI lane has run.
Head 793f716: combined status pending, total_count: 0; 4 queued + 1 skipped fork-gate checks only. None of build / tsc / vitest / pytest / eslint ceiling / coverage, none of the 5 review bots. The three TypeScript interactions you flagged for CI to confirm (Record<BuiltinTabKind, ReactNode> with the app:${string} arm, AppHost's inline prop type, the suite-wide usePanelTabDescriptors mock) are therefore unverified by anyone. Rebase, push, get a green run.
Should fix
4. Contributes.validate() omits the malformed-input handling its sibling field requires, and a test codifies the omission. Non-object contributes, non-list panelTabs, non-dict entries all degrade silently to [] in from_dict. Main's Contributes carries bad_block / bad_commands / dropped_commands for exactly this (rationale at manifest.py:1567-1580). test_null_paneltabs_degrades_to_empty asserts the silent path as correct — mirror the three flags and invert that test.
5. types.ts types third-party manifest data as concrete and required. The new block declares id/title/menuLabel/entry: string required. Main types the sibling as commands?: unknown with the reason inline ("the only code allowed to decide what a contribution IS is the module that checks it"), and this PR's own AppPanelTabDecl (panelTabRegistry.ts:20-27) makes every field optional for that reason. Use panelTabs?: unknown here; keep the shape in panelTabRegistry.ts.
6. activeId repair is still unscoped — prior finding #7, moved not fixed. usePanelTabs.ts effectiveActiveId runs unconditionally and is now what the hook returns. On main a stored activeId naming a nonexistent tab yields activeTab === null; with this change it silently focuses the last tab, reachable from a drifted localStorage bucket. Gate the fallback on the prune having removed something (tabs.length !== localised.length).
7. Backend cap not mirrored on the read path. MAX_PANEL_TABS = 8 is enforced in validate(), but resolvePanelTabs iterates unbounded; #7423's own comment calls a one-sided cap "silent truncation". Slice to the cap in resolvePanelTabs.
8. Two component-level tests still missing. panelTabRegistry.test.ts (8 cases) and usePanelTabs.test.ts cover the resolver and hook well. Not covered: that an app: kind takes the TabBody intercept rather than falling through to ActivityViewer's closed ViewKind multiplexer, and that the + row / launcher button actually opens it.
On iframe vs ESM: keep ESM.
Your correction is right and my earlier wording was wrong — ui.pages does not use an iframe. AppHost.tsx on main is a bare lazy(() => import(bundlePath)) in the same realm, same origin, same document ("no iframes, no Web Components, no Shadow DOM"); it provides an error boundary, Suspense, dev reload, and AppApiProvider context — no sandbox, CSP, module allowlist, or import map. So on capability the side panel is not materially different from an app's own page; exposure is already total in both, and an iframe here alone would be isolation theater. Two properties are genuinely new and worth one line in the PR body rather than a mechanism change: (a) a panel tab stays mounted while hidden and is restored on reload — durable background execution a page does not get; (b) it is co-mounted with the chat composer. Both point at a separate cross-cutting decision about app isolation (real sandboxing for ui.pages and panel tabs alike, or an explicit "installed apps are trusted code" tenet). Land ESM; file that question separately.
Nits
9. MAX_PANEL_TABS is public; every sibling bound is module-private (_MAX_COMMANDS_PER_APP, …). Rename unless the frontend imports it.
10. iconForKind needs KIND_ICON[kind as BuiltinTabKind] because isPanelTabKind returns boolean; make it a type predicate and the cast disappears.
11. PR body describes deltas against the previous revision, not main ("panelTabRegistry.ts rewritten from in-code registerPanelTab" — the file is added; "removed the extensions.ts register line", "extension-seams.md row dropped" — neither file is in the diff, main has no such row).
Prior comments
Direction, (string & {}) → template literal, module-eval prune → read path, active flag plumbed to AppInfo, menuDescription read site, launcher iterating descriptors, Screenshot Evidence markers — addressed. activeId repair — not addressed (#6). extension-seams.md — moot. Tests — partial (#8).
Not verified by me
Nothing compiled or run; all type-level claims are read-only inference. The four exhaustive ViewKind Records are untouched and ViewKind unchanged, so exhaustive by inspection only. No app on main declares contributes.panelTabs, so runtime mount/unmount/hidden behaviour is untested by anyone.
github-actions
Bot
added
readiness: checking
793f716 to
d9793bc
Compare
|
Rebased onto Blocking
Should fix Nits — 9: On ESM — agreed, keeping ESM, and thanks for the detailed read. The two genuinely-new properties are now named in the body: a panel tab stays mounted while hidden and is restored on reload (durable background execution a page doesn't get), and it's co-mounted with the chat composer. I'm treating real app isolation as a separate cross-cutting decision to file rather than a mechanism change here. |
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
1 similar comment
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
|
Description updated for the template gate — added a Why it matters section, and renamed That description gate is also the answer to the "no CI lane has run" finding — the real lanes (build / tsc / vitest / pytest / review bots) were never approved to start, rather than having run and been skipped. Once they execute I'll drive whatever they report to green. |
github-actions
Bot
added
readiness: action required
d9793bc to
f0c007a
Compare
github-actions
Bot
added
the
readiness: action required
f99cc8b to
fd07f7a
Compare
github-actions
Bot
added
readiness: checking
bolichen97
left a comment
bolichen97
left a comment
There was a problem hiding this comment.
Approving. The seam is the right shape and every finding from the earlier rounds is addressed in code.
On your second flag — ESM AppHost vs iframe: keep the ESM host. This is a deliberate decision, not an omission. ui.pages already render through the same in-process host, so an app can already run a full page in the dashboard's origin; iframe-ing only the panel tab would take no capability away from a malicious app while making every future app pay for a versioned postMessage contract plus manual theme, height and focus plumbing, and would leave the platform forked (pages in-process, tabs isolated). If we want isolation, the place to add it is AppHost itself as a sandboxed mode covering both surfaces, driven by whether registry apps are ever installable without review — not one surface in one PR.
On your first flag — contributes being net-new: mirroring the top-level publishProvider pattern, including the /api/apps/<app>/ endpoint allowlist, was the right call.
Changes folded in since your last revision, all with regression tests and verified negative controls:
- Endpoint allowlist hardened. Being inside
/api/apps/<self>/was not sufficient —routes.pymounts core-owned lifecycle handlers in that same namespace, so a manifest could aim a row at its ownuninstalland a reader's click would run it with their session. Reserved segments are now derived from the actual registrations (which also turned upmanifest,config,migrate-cleanup,tokenand_jobs), with a drift test that fails when a new core route appears unlisted. Three parser-level bypasses closed along the way — a query string, a stripped control character, and\as a path separator — which is why the validator now takes a character allowlist rather than a blocklist. - Cross-slot hosting for contributed tabs. They render from the cross-slot
allAppTabslist alongside MCP frames, andopenPanelTabstamps the owning slot, so the React key is stable and a chat switch no longer remountsAppHostand discards in-body state. setOrderpreserves hidden orphans. It was writing the visible list over the stored bucket, so one drag while an app was disabled deleted its tab — defeating the "hidden, not deleted" contract the code documents.- Contributed rows are attributed to their app, following
contributedCommands.ts'sappLabelprecedent (degrade toname, then clipped, never empty), so a row labelledDownloadcannot pass as the core action. appIconuses an own-property check —APP_ICONS['toString']was a truthy function and React throws on it, andiconis unvalidated manifest data.isPanelTabKindguardstypeof, since the persisted bucket is parsed from localStorage and cast; a non-stringkindthrew upstream of the whole chat render.entryisui/-relative, not app-root-relative. The code was right and the prose wrong; corrected here and in the two sibling rows that said the same thing, so the reference table no longer contradicts itself.
fd07f7a to
da811fc
Compare
github-actions
Bot
added
readiness: action required
da811fc to
fb66095
Compare
github-actions
Bot
added
readiness: action required
Add panelTabs as a second field on the existing Contributes block,
alongside commands, so an installed app can own a chat side-panel tab.
Core resolves the declarations off the shared /api/apps query, mounts
the body through the ESM host ui.pages already use, and persists the tab
as {appName, appTabId} metadata that re-mounts on load. Caps and
malformed-input reporting mirror the commands precedent; app icons
resolve through an app-facing icon set rather than the edition-only
builtin registry an installed app cannot reach.
5 participants
Problem / Motivation
An installed app has no way to add a tab to the chat side panel.
contributeson main carriescommands, which a manifest-only app can already use to reach the Command Bar — but nothing lets an app own a panel surface, and the runtimeappTabKind is not it: that is a per-tool-call MCP App render keyed bytoolCallId, ephemeral and never persisted.What changed
contributes.panelTabs[]— a second field on the existingContributes, alongsidecommands:The tab kind is
app:<app_name>:<id>; at most 8 tabs per app.apps/manifest.py) —PanelTabConfig+panelTabsonContributes, threaded through itsto_dict/from_dict/validate.commandsis untouched: both fields parse, serialize and validate side by side. Caps and malformed-input reporting mirror thecommandsprecedent (bad_panel_tabs/dropped_panel_tabs, and_MAX_PANEL_TABS_PER_APPenforced on both sides). Entry paths go through the existing_path_escapes_app_root.hooks/panelTabRegistry.ts, added) —resolvePanelTabs(apps)over a locally pinned subset ofGET /api/apps, the same posture asoverlaySlots.ts;usePanelTabDescriptors()reuses the shared['apps']query, so no extra request. Enabled apps only, ordered by app name, malformed or duplicate declarations warned and skipped.components/AppHost.tsx) — an optionalentryoverride mounts a panel-tab bundle through the hostui.pagesalready use;activeflows toAppApiProviderso a hidden body can pause. No iframe (see below).hooks/usePanelTabs.ts) —TabKindgains`app:${string}`; app tabs persist as{ kind, appName, appTabId }metadata and re-mount on load; an orphaned tab (app disabled/uninstalled) is hidden at READ and returns when the app does.pages/chat/SidePanel.tsx) — the app kind is intercepted before theVIEW_KINDSbranch; declared tabs appear in the+menu and the empty-panel launcher (which rendersmenuDescription).apps/appIcons.tsx, added) — an app-facing name→glyph set. Deliberately notbuiltinIcons.tsx: that registry is only populated byregisterBuiltinIcons()at edition-composition module load, so a runtime-installed app can never register into it and every app icon would silently fall back. Bounded rather than a lookup over lucide's full export, which would bundle ~1k components; the allowed names are documented.Why it matters
An installed app can own a full side-panel experience — a document browser, a review queue, a live log — next to the chat it belongs to, declared in its manifest and rendered through the same ESM host
ui.pagesalready use. Before this, a panel surface was reachable only by editing the core tab machinery (usePanelTabs/SidePanel), which a downstream fork then re-applies on every sync; the runtimeappTabKind is not an alternative, being an ephemeral per-tool-call MCP render keyed bytoolCallIdand never persisted. A declared tab persists as{appName, appTabId}metadata, re-mounts on reload, and disappears cleanly when its app is disabled — so the edition registers nothing and installing the app is the whole integration. With no app declaringpanelTabs, the registry resolves empty and the stock build is byte-identical to main.Addressing the review
contributesalready exists on main — correct, and the earlier revision was wrong to add a second class.panelTabsis now a field on main'sContributes;commandskeeps parsing, serializing and validating (test_commands_survive_alongside_paneltabspins that both survive a round trip). Rebased ontomain.BookOpen);BookMarkedis not in the pinned lucide version's verified set here, so it is out of the doc.bad_panel_tabs/dropped_panel_tabsmirrorbad_commands/dropped_commandsand are reported fromvalidate(). The test that codified the silent path is inverted; non-objectcontributesand non-object entries are covered too.types.ts—panelTabs?: unknown, alongsidecommands?: unknown. The shape stays inpanelTabRegistry.ts.activeIdrepair — now scoped to the case where the prune actually hid the active tab. A storedactiveIdnaming no tab still yields no active tab, as on main.resolvePanelTabsslices toMAX_PANEL_TABS_PER_APPand warns.app:kind takes theAppHostintercept and never reachesActivityViewer; the+row and the launcher each open it; dedupe to one instance; the per-app cap; the scoped-vs-unscopedactiveIdcases._MAX_PANEL_TABS_PER_APPis module-private on the Python side;isPanelTabKindis a type predicate, so theKIND_ICONcast is gone.ESM, not an iframe
Keeping ESM, per the review. Two properties of a panel tab are genuinely new versus a page and worth naming: it stays mounted while hidden and is restored on reload (durable background execution a page does not get), and it is co-mounted with the chat composer. Neither is addressed by an iframe here —
AppHoston main is same-realm, same-origin with no sandbox, CSP or module allowlist, so exposure is already total forui.pages. Real app isolation is a separate cross-cutting decision (sandboxing for pages and panel tabs alike, or an explicit "installed apps are trusted code" tenet); filing that rather than changing the mechanism in this PR.Tests
Backend:
flake8clean,mypyclean on the changed file, the repoblackgate passes,docs-lintpasses, brand gate passes, andscrub-lint's working-tree scan is clean (its 2 failures are the pre-existing git-history item it defers itself).Frontend
tsc/vitestcould not run on the authoring host — no npm egress — so CI is the gate for them. Verified by inspection:Exclude<TabKind, \app:${string}`>leaves'app'inBuiltinTabKindand the type predicate narrows the else branch to exactly that;InstalledAppis structurally assignable toAppHost'sapp` prop.Screenshots / video
Why no screenshot: No user-visible change in the stock build — no app on main declares
contributes.panelTabs, so the registry resolves empty and every render site is inert. A contributed tab appears only when an installed app declares one, which is out of scope for this PR.