Skip to content

fix(ui): work over plain http, and catch render crashes at the boundaries - #2865

Merged
Charlesthebird merged 9 commits into
mainfrom
charlesthebird/randomIdNoCrypto
Sep 17, 2026
Merged

Charlesthebird merged 9 commits into
mainfrom
charlesthebird/randomIdNoCrypto

Conversation

@Charlesthebird

@Charlesthebird Charlesthebird commented Sep 17, 2026 •

Copy link
Copy Markdown
Collaborator

🤖 written by Claude (start)

Changelog

The UI now works when served over plain http from anything but localhost, and one component throwing no longer blanks the page.

Testing

  1. cd ui && yarn dev --host, then open the printed Network URL — a LAN IP, not localhost, so not a secure context.
  2. /schedules/new loads and takes input. On main it throws crypto.randomUUID is not a function at mount and stays blank.
  3. Share a conversation and press Copy link. The button reports whether it worked; on main it silently does nothing here.
  4. Put crypto.randomUUID() back in ScheduledRunForm.tsx and reload. The error renders inside the shell, nav usable, stack behind Error details.

Additional Notes

This reinstates #1868, which fixed the same crash in May. That fix lived in ui/src/lib/utils.ts and went with the interface rewrite. The lint rule is the part meant to outlast the next one.

randomId() returns a v4 UUID because the control plane validates some of these ids as UUIDs. It uses crypto.getRandomValues, which carries no secure-context condition.

The clipboard fallback answers the document's own copy event, as antd does. antd's copyable UI is not used: it reports a copy whatever copy() returns.

Not covered: a module that throws while being evaluated. main.tsx never runs, so no boundary exists yet.


🤖 written by Claude (end)

`crypto.randomUUID` is only defined in a secure context, so every page that
minted an id with it threw when the app was served over plain http from
anything but localhost — the new-schedule page did it at mount, which blanked
the page. One `randomId()` helper now serves every call site; it returns a v4
UUID because some of these ids are validated as UUIDs server-side.

Signed-off-by: Nicholas Bucher <[email protected]>
…sion slot

A thrown component used to take the whole page with it. Three boundaries now
contain one: react-router's `errorElement` on a pathless route nested inside
the shell, so a page crash keeps the nav; a theme-free class boundary above the
providers for anything that fails before the app can style itself; and one per
extension contribution, so a broken extension drops out instead of the page.

Signed-off-by: Nicholas Bucher <[email protected]>
@github-actions github-actions Bot added the bug Something isn't working label Sep 17, 2026
@Charlesthebird

Charlesthebird commented Sep 17, 2026 •

Copy link
Copy Markdown
Collaborator Author

Note

Outdated. Superseded by the full reel.

Schedules over plain http, and a crashed page keeping the shell
reel.mp4

🤖 written by Claude

@blacksmith-sh

This comment has been minimized.

Nothing has rendered when the stylesheet, branding or mock-backend step throws,
so the boundary above cannot see it and the user gets the blank page this
branch exists to remove. The fallback moves out of the boundary so both can
render it, and the route boundary gains the test for its own claim: a page that
throws is replaced, the shell around it is not.

Signed-off-by: Nicholas Bucher <[email protected]>
…t fails

`navigator.clipboard` is undefined over plain http, so the optional call was a
silent no-op under an alert reading "Copy this link now — it cannot be shown
again": the button did nothing and the token was gone. `copyText` falls back to
`execCommand`, and the dialog reports a failure instead of implying a copy.

The textarea it borrows goes inside the open dialog, not on the body. A modal
traps focus, so one outside it takes no selection and `execCommand` then returns
true for having copied nothing.

Signed-off-by: Nicholas Bucher <[email protected]>
The reload button said what the browser already offers and told the reader
nothing. In its place the message and stack sit behind a disclosure, as raw
text, so whoever hits this can read or copy what actually happened.

The fallback renders above the theme provider, so it carries its own palette
and reads the mode the user chose from storage, falling back to the system
preference. Both themes.

Signed-off-by: Nicholas Bucher <[email protected]>
@Charlesthebird

Charlesthebird commented Sep 17, 2026 •

Copy link
Copy Markdown
Collaborator Author

Note

Outdated. Superseded by the full reel, which covers this and the other three fixes.

The root error boundary, dark and light
reel2.mp4

🤖 written by Claude

@Charlesthebird Charlesthebird left a comment •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This automated review is a first pass. A manual review will follow once the blockers are addressed.


Nothing blocks merge. Each fix was checked on a LAN IP against a live cluster — a non-secure context — rather than reasoned about.

Two findings are worth taking first, both verified in a browser. crypto.getRandomValues is not secure-context gated, so randomId can use real randomness. antd already ships the copy technique this reimplements, though its copyable UI reports success unconditionally and cannot be used as-is.

✅ What I checked
  • ✅ On http://<LAN-IP>: crypto.randomUUID undefined, getRandomValues returns bytes.
  • ✅ New-schedule page mounts and takes input there; on main it throws.
  • ✅ Share link reaches the clipboard on both origins — clipboard poisoned first, read back from a secure page.
  • ✅ No crypto.randomUUID or bare navigator.clipboard left in src/.
  • ✅ No randomId caller needs unguessable randomness; share tokens are minted by the controller.
  • ✅ RouteErrorBoundary has its Emotion and antd context on every route.
  • ✅ Reverting either boundary fix fails its test.
  • ✅ 165 e2e, 524 unit.
  • ✅ The reported TestRuntimeRevisionLifecycle failure is a Go cluster test; this diff is all ui/.

A module that throws while being evaluated is still uncovered: main.tsx never runs, so no boundary exists yet.


🤖 written by Claude

Comment thread ui/src/components/common/copyText.ts Resolved
Comment thread ui/src/api/randomId.ts Resolved
Comment thread ui/src/components/chat/ShareDialog.tsx Resolved
Comment thread ui/src/components/common/copyText.ts Resolved
Comment thread ui/src/router/RouteErrorBoundary.tsx Resolved
Comment thread ui/src/main.tsx Resolved
Comment thread ui/src/components/chat/ShareDialog.tsx Resolved
…tform

`getRandomValues` carries no secure-context condition — only `randomUUID` and
`subtle` do — so the ids are properly random for the same line count.

The copy fallback now answers the document's own `copy` event rather than
selecting a borrowed textarea, which is what antd does and what removes the
need to know where the focus trap is. Success is the event having fired, not
what `execCommand` returns, since it returns true for copying nothing.

Also: a rejected copy reports failure rather than leaving the button silent, a
missing root element no longer throws inside the handler that renders the
failure, and the retry button's comment says what it does.

Signed-off-by: Nicholas Bucher <[email protected]>
@Charlesthebird

Charlesthebird commented Sep 17, 2026 •

Copy link
Copy Markdown
Collaborator Author

Note

Outdated. Superseded by the current reel; the route boundary gained its disclosure after this was recorded.

All four fixes, before the route boundary disclosure
reel3.mp4

🤖 written by Claude

…IdNoCrypto

Signed-off-by: Nicholas Bucher <[email protected]>

# Conflicts:
#	ui/src/mocks/transport.ts
The page boundary showed one line and dropped the rest, so a crash mid-session
told the reader less than the console did. It now carries the same disclosure
the root fallback has, in theme tokens, with the formatter shared between them.

`storedMode` moves to its own module so reading the theme costs no import of
the provider the root fallback renders above.

Signed-off-by: Nicholas Bucher <[email protected]>
@Charlesthebird

Charlesthebird commented Sep 17, 2026 •

Copy link
Copy Markdown
Collaborator Author

The route boundary carries the same disclosure as the root fallback: message visible, stack behind Error details. Dark and light.

Route error boundary, dark theme, with the stack expanded

Route error boundary, light theme, with the stack expanded


🤖 written by Claude

`crypto.randomUUID` was fixed once in #1868 and came back when the interface was
rewritten and the file holding the helper went with it. A note in a file is lost
the same way twice, so the rule states it where a rewrite cannot drop it, and
names the helper to use instead. `navigator.clipboard` is listed for the same
reason: bare, it fails silently rather than loudly.

Signed-off-by: Nicholas Bucher <[email protected]>
@Charlesthebird Charlesthebird changed the title fix(ui): drop crypto.randomUUID, and catch render crashes at the boundaries fix(ui): work over plain http, and catch render crashes at the boundaries Sep 17, 2026
@github-actions github-actions Bot added bug Something isn't working and removed bug Something isn't working labels Sep 17, 2026
@Charlesthebird
Charlesthebird marked this pull request as ready for review September 17, 2026 20:56
@Charlesthebird

Copy link
Copy Markdown
Collaborator Author

All four fixes at fd0cc798, recorded against the live cluster from a LAN IP — an origin where neither crypto.randomUUID nor navigator.clipboard exists.

reel4.mp4

🤖 written by Claude

@EItanya
EItanya added this pull request to the merge queue Sep 17, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 17, 2026
@Charlesthebird
Charlesthebird added this pull request to the merge queue Sep 17, 2026
Merged via the queue into main with commit 56b6bea Sep 17, 2026
31 checks passed
@Charlesthebird
Charlesthebird deleted the charlesthebird/randomIdNoCrypto branch September 17, 2026 21:20
alexdarbyshire pushed a commit to alexdarbyshire/kagent that referenced this pull request Oct 5, 2026
…ries (kagent-dev#2865)

Signed-off-by: alexdarbyshire <[email protected]>

---
*🤖 written by Claude (start)*

## Changelog

The UI now works when served over plain http from anything but
localhost, and one component throwing no longer blanks the page.

## Testing

1. `cd ui && yarn dev --host`, then open the printed **Network** URL — a
LAN IP, not localhost, so not a secure context.
2. `/schedules/new` loads and takes input. On `main` it throws
`crypto.randomUUID is not a function` at mount and stays blank.
3. Share a conversation and press **Copy link**. The button reports
whether it worked; on `main` it silently does nothing here.
4. Put `crypto.randomUUID()` back in `ScheduledRunForm.tsx` and reload.
The error renders inside the shell, nav usable, stack behind **Error
details**.

## Additional Notes

This reinstates kagent-dev#1868, which fixed the same crash in May. That fix lived
in `ui/src/lib/utils.ts` and went with the interface rewrite. The lint
rule is the part meant to outlast the next one.

`randomId()` returns a v4 UUID because the control plane validates some
of these ids as UUIDs. It uses `crypto.getRandomValues`, which carries
no secure-context condition.

The clipboard fallback answers the document's own `copy` event, as antd
does. antd's `copyable` UI is not used: it reports a copy whatever
`copy()` returns.

Not covered: a module that throws while being evaluated. `main.tsx`
never runs, so no boundary exists yet.

---
*🤖 written by Claude (end)*

---------

Signed-off-by: Nicholas Bucher <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants