feat(webhooks): reach inbound webhooks from Settings, not the nav rail - #2056
Conversation
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — ✅ PASSAdvisory design-level review of Design-Verdict: PASS Right problem, right mechanism — reuses the registry's existing Suggestions
[DESIGN-REVIEWED] fbb7fb2 |
UX Review (Fable 5) — ✅ PASSAdvisory UX-level review of UX-Verdict: PASS A status-summary panel that answers "is it on, who can reach it" and hands off cleanly; states (enabled / disabled / no-token / read-failed) are all distinguishable. Suggestions
[UX-REVIEWED] fbb7fb2 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
6855eb5 to
f5edc1f
Compare
f5edc1f to
77dbff3
Compare
Review round 1 — dispositionsPushed Design Review 🟡 CONCERNS — both points ACCEPTED, fixed1. Palette unreachability — real regression, fixed. This was correct and it { key: 'webhooks', route: '/webhooks', icon: inlineIcon(Webhook) },Two notes on the implementation:
Pinned with a regression test that fails if the entry is removed 2. Description overstated the preserved wiring — accepted, corrected. You are UX Review 🟡 CONCERNS — watch ACCEPTED, one suggestion accepted, one declined1. Search Everywhere loses Webhooks. Same finding as Design; fixed above. 2. 3. Error copy offers no recovery — DECLINED, with reasoning. Legitimate point, GPT 5.6 ✅ no blocking findings — the one detail finding is a FALSE POSITIVE (verified)Answering it explicitly because a green check is not a reply, and the finding is
The premise about I did not want to assert that from memory, so it is now pinned by a test that Adopting the suggested fix would actively be a regression: the page's Also addressed a gap neither bot raised: the panel shipped with no tests at Opus 5 ✅ no findingsNothing to disposition. CIAll checks green except Backend Tests (Windows) ×4, which is pre-existing on
I have not touched it — it needs its own fix by whoever owns the ACP resolver The one local test failure I see is Standing caveat before merge (unchanged)The 12 new strings × 10 locales here are machine-generated. They pass every Still open for a human callThe new tab sits in Settings' SYSTEM group, between Computer Use and Remote |
77dbff3 to
7fb1a62
Compare
Review round 2 — dispositionsPushed UX Review 🟡 CONCERNS1. Terminology drift across the handoff — ACCEPTED, fixed (and it deleted Correct finding, though the specifics were quoted as key names rather than
So one of the three already agreed and two drifted, including a singular/plural Fixed by having the panel render Worth calling out the side benefit: it let me delete Pinned by a test that asserts the panel renders the page's 2. Palette siblings a newcomer can't distinguish — ACCEPTED, fixed. Titled I did not give it a distinct icon, which was the "and/or" half of your 3. Rail removal ships with no signpost — ESCALATED to the user, not resolved by
The person who asked for this change wanted webhooks to stop being a top-level 4. Error copy has no recovery affordance — PARTLY ACCEPTED, fixed differently Design Review ✅ PASS — two follow-up suggestions, both ACKNOWLEDGED and DEFERRED1. Derive palette entries for hidden builtins from the registry. Agreed, and 2. Unify the page's queryFn on the null convention. Also agreed in principle, CIUnchanged from round 1: green except Backend Tests (Windows) ×4, still Standing caveat before mergeNow 9 new strings × 10 locales rather than 12, all machine-generated. They |
7fb1a62 to
c714360
Compare
Review round 3 — dispositionsPushed UX Review 🟡 CONCERNS1. Error copy has no next step — ACCEPTED this round, reversing two earlier I declined this twice (round 1 and 2) on the grounds that the fix cost ten Translated across all ten locales by hand rather than by generator, following
Note this is in addition to the behavioural fix from round 2 (dropping 2. Palette pair shares an icon — ACCEPTED, and my round-2 reasoning was I declined this last round arguing the Changed to 3. Silent relocation — still the user's call, now explicitly framed as one. You asked me to "confirm the demotion deliberately accepts that one-time Verification
Both fixes were checked in a running instance, not just compiled — the palette CIUnchanged and still not attributable to this branch: Backend Tests (Windows) Standing caveat before mergeUnchanged: 9 new strings × 10 locales, machine-generated except the error line |
c714360 to
3a9f009
Compare
Rebased onto main (237 commits) — conflicts resolved,
|
| question it answers | value | |
|---|---|---|
previewFlag (main) |
whether to advertise it at all | off until an operator opts in |
hiddenFromNav (this PR) |
where it lives once advertised | Settings → Webhooks, not the rail |
So the surface now carries both.
The part that needed real work, not a merge
Taking both sides naively would have broken main's invariant. Main's own
comment in collectPages() states it plainly:
getAdvertisedSurfaces(), notgetBuiltinSurfaces(): a preview-gated surface
is not released yet, so it must not be reachable from Search Everywhere either
— the palette is a second front door to the rail, and gating only the rail
would leave the unpolished page one ⌘K away.
getAdvertisedSurfaces() is getBuiltinSurfaces().filter(surfacePreviewEnabled),
and getBuiltinSurfaces() already drops hiddenFromNav surfaces. So the moment
this PR sets hiddenFromNav, the surface leaves the registry's reach and the
preview gate stops being applied to it — because the two places that apply it
never see it.
That would have left the two things this PR adds unguarded:
- The Settings tab — listed for everyone, releasing an unpolished page.
- The palette entry —
EXTRA_PAGESis a hardcoded list appended after the
advertised-surfaces loop, so it bypasses the gate entirely. Exactly the ⌘K
hole main's comment describes.
Both now read PREVIEW_WEBHOOKS directly, so the Developer > Config toggle still
controls visibility end to end:
SettingsPagefilters the tab onusePreviewFlag(PREVIEW_WEBHOOKS)— the hook
rather than a bare read, so toggling updates the rail without a reload.EXTRA_PAGESentries gained an optionalpreviewFlag, andcollectPages()
skips gated ones.
Four new tests pin both directions (hidden while off, present once on), and I
revert-verified them: removing either gate fails exactly its own test and nothing
else.
Other conflict resolutions
SettingsPageicon import — union; main addedHistory, this PR adds
Webhook.InstancesPanel→RemoteCrewPanel— took main's rename; this branch's
copy still referenced the old name.- 12 catalogs — resolved by taking main's catalog verbatim and
re-inserting onlykeys(branch) − keys(merge-base), computed rather than
eyeballed. A hand union-merge here reorders main's keys (which inflates the diff
by thousands of lines) and can silently resurrect keys main deleted. The script
asserted the branch deleted no base key and added nothing outside the webhooks
namespace. Result is insert-only:en +17 −0,ru +15 −0,ja +12 −0. ko(new locale) — main added Korean while this branch was open, so parity
demanded the 8 new keys there too. Written by hand using ko.json's own
vocabulary (에이전트 턴, 엔드포인트, 토큰, 게이트웨이,webhookleft in Latin)
and its style guide's register (합니다체 statements,~하세요instructions, no
~들plural marking — which is also why only_otherexists, as in ja/zh-CN).en-XA— regenerated withnpm run i18n:pseudorather than hand-edited.
Verification
tsc -b clean · eslint 0 errors · i18n:check exit 0 · full frontend
suite 12,173 passed, 0 failed (886 files) · catalog parity green across all
11 locales · all locale style suites pass · git merge-tree reports no conflict
against main · one commit, 0 behind, zero Python files.
One thing worth a human eye
With the flag off — the default — this PR is now invisible: no rail item, no
Settings tab, no palette entry. That is the correct reading of main's intent, but
it does mean the surface this PR reshapes is unreachable until someone enables the
preview. If you would rather the Settings tab be visible ungated (on the argument
that a settings entry is a reasonable home for a preview feature, unlike a rail
slot), that is a one-line change to the filter — but it would re-open the "release
by accident" hole for the panel, so I did not assume it.
ce778c9 to
da1192e
Compare
CI on
|
Correcting myself: the
|
The top-level rail slot overstated it. Inbound webhooks are operator
configuration touched once at setup — minting a token, flipping the kill
switch — not a daily destination like Sessions or Schedule.
Move the entry point to Settings → Webhooks (SYSTEM group, after Computer
Use) and suppress the rail item with the registry's existing
`hiddenFromNav`, which keeps the route, badge selector, and tab-title
attention count wired. `/webhooks` stays valid, so bookmarks and the
onboarding anchor still resolve.
Settings gets a summary panel rather than the page itself. The Webhooks
page is a rail-and-detail shell with its own resizable splitter and
Settings is a `SidePanelLayout`; embedding one in the other stacks two
rails and spends most of the width on chrome. So the panel answers what a
settings reader asks — is it on, how many callers can reach it — and hands
off to the full page for the work.
The badge separates "Turned off" from "No tokens yet". `enabled` is the
effective state (`has_tokens && switch_on`) while `switch_on` is the kill
switch alone, and those diverge on a fresh install: switch on, nothing
minted. Collapsing both to "off" would report first-run as broken.
A failed read now says so. Gating the badge and count on `!isError` alone
left a mute card that looked identical to a healthy endpoint with nothing
configured — caught by rendering the panel against a gateway-less harness
and seeing the state vanish.
Adds `capture/webhooks-settings.{html,tsx}` for the same reason the
Webhooks page has one: reaching `/settings?tab=webhooks` through the SPA
needs a live gateway and a dashboard credential, and without one the shell
renders the Kiro CLI prerequisite gate instead of Settings.
12 new strings across all ten locales. Plural categories are per-language
rather than copied from English: ru carries `_one/_few/_many/_other`, ja
only `_other`, and es/fr/pt/it needed the `many` CLDR category their
catalogs already mirror from `other`. zh-CN uses 轮次 for "turn" per
style/zh-CN.md §2, and fr says "jeton" to match the existing catalog
rather than "token".
da1192e to
fbb7fb2
Compare
Rebased onto main — Frontend Tests fixed upstream, one pre-existing backend flake leftHead The new gate script passes on this branch, including its diff-scoped halves:
|
Review-ready — fully green on
|
| reviewer | verdict |
|---|---|
| Design Review (Fable 5) | ✅ PASS |
| UX Review (Fable 5) | ✅ PASS |
| GPT 5.6 Review | ✅ no blocking findings |
| Opus 4.8 Review | ✅ no blocking findings |
What this round actually involved
The merge conflict was not a mechanical one. Main moved inbound webhooks behind
previewFlag: PREVIEW_WEBHOOKS while this PR was open, and the two changes answer
different questions — previewFlag decides whether to advertise the surface,
hiddenFromNav decides where it lives once advertised. So the surface carries
both.
Taking both sides naively would have broken main's own invariant. hiddenFromNav
drops the surface from getBuiltinSurfaces(), and therefore from
getAdvertisedSurfaces() — which is exactly where the preview gate is applied. The
two things this PR adds would have escaped it:
- the Settings tab, which is now the surface's only advertised home, and
- the palette entry, since
EXTRA_PAGESis appended after the
advertised-surfaces loop and bypasses the gate entirely — the same ⌘K hole main's
own comment incollectPages()warns about.
Both now read PREVIEW_WEBHOOKS directly, so the Developer > Config toggle controls
visibility end to end. Four tests pin both directions, revert-verified: removing
either gate fails exactly its own test and nothing else.
Three CI blockers, three different owners
Screenshot Evidence(a gate main added while this was open) — legitimately
failing; this PR has a real visual delta and no committed evidence. Fixed: three
frames captured from a real dev gateway, committed under
temp-screenshots/webhooks-under-settings/, SHA-pinned in the description.
Includes the flag-off state, because "the tab is absent by default" is as much
a required proof as "the tab renders".Frontend Tests— a 15 stestTimeoutagainst a scan measuring 15.8-16.4 s on
CI. I first called it a flake and re-ran it; it failed again, so that was wrong. I
built the fix as fix(test): stop the unit-literal ratchet failing on runner speed #2577, then closed it as superseded: main's refactor(i18n): run the unit-literal scan as a gate, not a vitest test #2562 moved the
whole scan out of vitest into a gate script, which removes the failure mode instead
of budgeting around it. Better than my version.Backend Tests (3.12, 1)—test_cron_cancel.py::TestSubprocessRegistry,
timing-sensitive (sleep 30cancelled mid-run), with the second failure a cascade
from the first leaving registry state behind. Verified pre-existing by running it
againstorigin/mainon this machine, where it fails identically. This branch
changes zero Python files. Passed on re-run.
Localization
Main added ko (Korean) as an 11th locale while this was open, so parity required
the 8 new keys there too. Written by hand from ko.json's own vocabulary
(에이전트 턴, 엔드포인트, 토큰, 게이트웨이, webhook left in Latin) and its style
guide's register — 합니다체 statements, ~하세요 instructions, no ~들 plural
marking, which is also why only _other exists as in ja/zh-CN.
The 12 conflicted catalogs were resolved by taking main's copy verbatim and
re-inserting only keys(branch) − keys(merge-base), computed rather than eyeballed,
with an assertion that the branch deleted no base key. Result is insert-only
(en +17 −0, ru +15 −0, ja +12 −0) rather than the thousands of reordered lines a
hand union-merge produces. en-XA was regenerated, not hand-edited.
Verification
tsc -b clean · eslint 0 errors · i18n:check exit 0 · unit-literal gate
clean including [added-lines] 0 and [vs-base] 0 · full frontend suite 12,176
passed, 0 failed across 889 files · catalog parity green across all 11 locales · all
locale style suites pass · git merge-tree clean against main · one commit.
Standing notes for a human
Default invisibility. With the flag off — the default — this PR is now invisible:
no rail item, no Settings tab, no palette entry. That is the correct reading of main's
intent, but it does mean the surface this PR reshapes is unreachable until someone
enables the preview. If you would rather the Settings tab be visible ungated, that is
a one-line change to the filter, but it re-opens the release-by-accident hole, so I
did not assume it.
Machine-generated copy. The new strings across 11 locales are machine-generated
except the error line and the Korean set, which I wrote by hand. They pass parity,
placeholder, plural-category and every style-guide check, but no gate judges whether
the copy reads naturally on a surface carrying remote-execution and disable-switch
language. A native-speaker pass is still my recommendation.
Grouping. The tab sits in SYSTEM between Computer Use and Remote Crew.
Channels sits ungrouped at top level and is arguably the closer neighbour. One-line
move if you disagree.
Currently 10 commits behind main but merge-tree is clean and readiness passed, so I
have left it rather than triggering another full round and re-pinning the screenshots
again. Not merging — that is yours.
kirodotdev#2056) The top-level rail slot overstated it. Inbound webhooks are operator configuration touched once at setup — minting a token, flipping the kill switch — not a daily destination like Sessions or Schedule. Move the entry point to Settings → Webhooks (SYSTEM group, after Computer Use) and suppress the rail item with the registry's existing `hiddenFromNav`, which keeps the route, badge selector, and tab-title attention count wired. `/webhooks` stays valid, so bookmarks and the onboarding anchor still resolve. Settings gets a summary panel rather than the page itself. The Webhooks page is a rail-and-detail shell with its own resizable splitter and Settings is a `SidePanelLayout`; embedding one in the other stacks two rails and spends most of the width on chrome. So the panel answers what a settings reader asks — is it on, how many callers can reach it — and hands off to the full page for the work. The badge separates "Turned off" from "No tokens yet". `enabled` is the effective state (`has_tokens && switch_on`) while `switch_on` is the kill switch alone, and those diverge on a fresh install: switch on, nothing minted. Collapsing both to "off" would report first-run as broken. A failed read now says so. Gating the badge and count on `!isError` alone left a mute card that looked identical to a healthy endpoint with nothing configured — caught by rendering the panel against a gateway-less harness and seeing the state vanish. Adds `capture/webhooks-settings.{html,tsx}` for the same reason the Webhooks page has one: reaching `/settings?tab=webhooks` through the SPA needs a live gateway and a dashboard credential, and without one the shell renders the Kiro CLI prerequisite gate instead of Settings. 12 new strings across all ten locales. Plural categories are per-language rather than copied from English: ru carries `_one/_few/_many/_other`, ja only `_other`, and es/fr/pt/it needed the `many` CLDR category their catalogs already mirror from `other`. zh-CN uses 轮次 for "turn" per style/zh-CN.md §2, and fr says "jeton" to match the existing catalog rather than "token". Co-authored-by: Kyle Seaman <kseam@dev-dsk-kseam-1b-55230d27.us-east-1.amazon.com>
Why
The top-level rail slot overstated it. Inbound webhooks are operator configuration
touched once at setup — mint a token, flip the kill switch — not a daily destination
sitting next to Sessions and Schedule.
What changed
The rail item is gone, the route is not.
hiddenFromNav: trueon the webhookssurface, which is the registry's existing mechanism for "surfaced elsewhere"
(Notifications and Explore already use it). Route, badge selector, and tab-title
attention count stay wired, so
/webhooksbookmarks and the onboarding anchor stillresolve.
Settings → Webhooks appears in the SYSTEM group after Computer Use.
Why a summary panel and not the page
The Webhooks page is a rail-and-detail shell with its own resizable splitter, and
Settings is itself a
SidePanelLayout. Embedding one in the other stacks two railsand spends most of the width on chrome — the same reason
SidePanelLayoutwasrejected for this page when it was built. So the panel answers what a settings
reader actually asks (is it on, how many callers can reach it) and hands off to the
full page for the work.
Two things worth reviewing
The badge separates "Turned off" from "No tokens yet".
enabledis the effectivestate (
has_tokens && switch_on);switch_onis the kill switch alone. They divergeon a fresh install — switch on, nothing minted — which is the common first-run case.
Collapsing both to "off" would report first-run as broken.
A failed read now says so. Gating the badge and count on
!isErroralone left amute card indistinguishable from a healthy endpoint with nothing configured. Caught
by rendering the panel against a gateway-less harness and watching the state vanish,
not by reading the code.
Localization
12 strings across all ten locales, with plural categories per language rather than
copied from English:
ru—_one/_few/_many/_otherja,zh-CN—_otheronlyes,fr,pt,it— needed themanyCLDR category, mirroringotherasthose catalogs already do for
webhooksPage.enabled_credentials_manyTerminology follows each catalog rather than the translator's instinct:
zh-CNuses轮次 for "turn" per
style/zh-CN.md§2 (caught byzhStyle.test.ts),frsays"jeton" to match the existing catalog instead of "token", and es/it/pt keep
"Endpoint" as the borrowed term they already use.
Screenshots
Captured from a real dev gateway (not a component harness) because three of these
prove things about the SURROUNDING chrome — which rail items exist, which Settings
tabs exist — that a harness cannot show.
The preview gate, both ways. Since main moved this surface behind
PREVIEW_WEBHOOKS, "the tab is absent by default" is as much a required proof as"the tab renders", so both are here.
Flag off (the default) — SYSTEM lists Browser, Computer Use, Remote Crew,
Privacy, Security, Developer, Releases. No Webhooks tab, and no Webhooks item in
the rail:
Flag on — the tab appears under SYSTEM between Computer Use and Remote Crew,
and the panel renders with its
No token yetbadge and theManage webhooks →hand-off. The nav rail still has no Webhooks entry, which is the actual point of
this PR:
Command palette — the regression this PR had to fix, and the disambiguation UX
Review asked for. Typing "hooks" returns both routed pages, distinguishable by
title and icon rather than only by the route in the subtitle:
URLs are commit-SHA-pinned (
fbb7fb27c) rather than branch-pinned, so they keepresolving after the branch is deleted on merge and after
temp-screenshots/ispruned from main's tip.
Verification
tsc -bclean · lint 0 errors · 13/13 i18n checks withI18N_BASE_REF=origin/main·catalog parity 73/73 · all 10 locale style suites pass · full vitest 10,368 passing
with the one known Node-24-only
Intl.DurationFormatfailure (CI runs Node 20).Rendered and inspected in a browser across the enabled and first-run states via the
new capture harness — added for the same reason the Webhooks page has one: reaching
/settings?tab=webhooksthrough the SPA needs a live gateway and a dashboardcredential, and without one the shell renders the Kiro CLI prerequisite gate.
Note
This makes Webhooks reachable only from Settings. That is the intent, but it does
remove the discoverable entry point — worth a second opinion on whether SYSTEM is
the right group for it.