Repository navigation
Conversation
The New tab dialog shows the same editable Directory field, Browse button and recent folders as New workspace. The field starts at the tab target's folder (a worktree's checkout, else the pane in front) and a ?new=tab&cwd= link still fills it. The chosen folder goes to createTab as cwd and applies to the new tab only; a tab made in a chosen folder is remembered in recent folders. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 11 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 13, 2026
devswha
added a commit
that referenced
this pull request
Oct 13, 2026
## Problem A render error outside the few places that catch one (chat turns, the chat lens, the file viewer, the release notes) unmounted the whole React tree and left an empty page. In an installed home-screen app on iOS or Android there is no browser reload button and no pull-to-refresh on the terminal, so the only way back was to force-quit the app. Nothing was logged under a name a bug report could point to. ## Cause `src/main.tsx` rendered `<SettingsProvider><App/></SettingsProvider>` straight into `createRoot` with no boundary. `RenderBoundary` was used only around chat turns, the chat lens, FileViewer and the release notes. App, the sidebar, the tab strip, the pane canvas, the terminal's own JSX, Settings and the dialogs had no boundary, and React 18 unmounts the root on an uncaught render error. ## Fix - `main.tsx` wraps the whole tree, `SettingsProvider` included, in `RenderBoundary`. Its fallback, `AppCrash`, uses the module-level `t` and no context, because a context may be what threw. It offers **Reload app**, **Copy details** (via `lib/clipboard.ts` `copyText`, which falls back to selecting the text on a plain-http LAN address) and the error text in a fold. - The sidebar (`MachineSidebar`), each pane slot (tab strip plus canvas, reset when another pane is picked) and the Settings dialog get boundaries of their own, so one broken region leaves the rest usable. Settings' fallback is a small dialog with Close that keeps what Settings gives a modal: the focus moves to Close and Tab stays inside (`useFocusTrap`), Escape closes it, and on a phone the sheet's swipe down closes it (`useSheetSwipe`). - When the active pane slot falls back to its notice, the notice reports the connection lost (`RegionCrash` `onShown`), since the unmounted terminal cannot; the header no longer claims live with no terminal attached. - `RenderBoundary` now passes the caught error to its fallback as a second argument. Existing callers are unchanged. - `window` `error` and `unhandledrejection` are logged to the console with a `herdr-web-ui:` prefix. Console only, no telemetry. - Test hook: `src/lib/crashProbe.ts` (`?crash=root|sidebar|pane|settings`) throws while rendering only when the build-time constant `__HERDR_TEST_HOOKS__` is true. `vite.config.ts` sets it from `HERDR_TEST_HOOKS=1`, which only `scripts/demo-build.ts` passes. `bun run build` and the published demo (`build-site.ts`) build with it off, so the check compiles out of production. - New strings have entries in ko, ja, zh and tr. Changelog fragment `changelog.d/render-error-boundaries.md`. `src/AGENTS.md` and `scripts/AGENTS.md` each get one line about the boundaries and the hook. ## Tests - New browser check `scripts/render-crash-demo-regression.ts`, registered in `ci-browser.sh` (lane count 45 to 46 in `ci-browser.test.ts`). On the demo build it checks five cases: - Without `crash`, no notice appears and there are no page errors. - `root`: AppCrash shows; Copy details puts the error on the clipboard; Reload app reloads the page. - `sidebar` and `pane`: only that region shows its notice, the rest is drawn, and Try again recovers once the region stops throwing. - `settings`: the dialog shows its notice, the app is drawn behind it, the focus is inside it and Tab/Shift+Tab stay there, Escape dismisses it, and in a second opening Close does. - A pane that throws after its terminal was live (switched to with `crash=pane` set): the header goes from live to reconnecting. - On a 390px touch phone (`isMobile`, `hasTouch`): `root` with the clipboard API absent and the copy command refused, where Copy details opens the details with the error text selected and Reload app is on screen; `pane`, where the workspace drawer still opens and Try again recovers. - `bun run check run bun scripts/render-crash-demo-regression.ts`: PASS. - Negative runs: root boundary removed from `main.tsx` FAILS (timeout waiting for the AppCrash heading); the `App.tsx` connection report reverted FAILS (timeout waiting for `.conn-reconnecting`); the Settings fallback's focus trap/Escape reverted FAILS (focus never enters the dialog). - `bun run typecheck`: pass. The new script, which is outside `tsconfig.tools.json`, was also typechecked with a temporary config: pass. - `bun run test:unit`: 3069 pass, 0 fail, 8 skip (rerun after the review fixes). - `bun run build`, then grep: `crash probe` is absent from `dist/`. - Not run: `check full` and `check concurrent`. The phone sheet swipe on the Settings fallback uses the same `useSheetSwipe` hook as Settings and is not exercised by the script. ## Notes - Overlaps with open PRs: - #951 touches App.tsx, SettingsDialog.tsx, src/AGENTS.md, scripts/AGENTS.md, ci-browser.sh, ci-browser.test.ts and demo-build.ts. It is not a duplicate: it adds `RenderBoundary` only inside chat and diff rendering. - #917 and #822 also add browser scripts, so the `toHaveLength` lane count in `ci-browser.test.ts` will conflict with whichever merges second. - #909, #890, #917 and #822 touch App.tsx near the sidebar and slot JSX. - The `crashProbe` check compiles out, but the `<CrashProbe>` elements stay in production as components that render null. - The pane boundary covers a split half (tab strip plus canvas), not each herdr pane, so a sibling in a herdr split inside the same half goes down with it. A per-pane boundary in `PaneCanvas.renderPane` would be the finer follow-up. - The browser console already prints uncaught errors. The listeners add a greppable `herdr-web-ui:` prefix. - The lane's shared demo build now has the inert probe compiled in, so every demo script runs on that build. - Left out: reporting crashes to telemetry, and other dialogs and the palette, which are still caught by the root boundary only. ## Merge-session review Fixed in 5ff418f: - The Settings crash fallback ignored `overPreview`: with a file preview open (Settings opened by its shortcut), the notice was drawn under the preview while its focus trap held the keyboard. `SettingsCrash` now takes `overPreview` and uses the `settings-over-preview` layer, as Settings does. - The fallback recorded no Settings entry in the history: if Settings threw on its first render, the system Back button left the page or the installed app; if it threw after a phone opened a page, the first Back did nothing visible. `SettingsCrash` now records one Settings level (`recordSettings([{ page: null, keyBar: false }])`), so one Back closes it. - `render-crash-demo-regression` adds three cases, each confirmed to fail without its fix: the hit test on Close over a file preview, Back on a desktop and a phone after a first-render crash, and one Back after a phone crash on an opened page. Deferred: none. Checks: `bun run check fast` passed; `render-crash-demo-regression` passed (run directly, since `check run` admission was busy). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
devswha
added a commit
that referenced
this pull request
Oct 13, 2026
…PCs (#958) ## Problem - On Android Chrome or a desktop browser, tapping an agent alert could bring forward a Markdown reader tab (`/?read=…`) or a raw file tab opened earlier from the file viewer. The pane named in the alert never opened. - An alert (still in the notification center) or a remembered selection (reload, new window) that names a PC removed since left an empty pane area with no explanation. An alert for a pane that has closed switched to herdr's focused pane without saying anything. - Review follow-ups (second commit): the worker's second delivery of one tap could strand a closed pane's lookup; a tab resumed with an old roster dropped a tap for a PC set up since; a just-added PC could be reported missing by a roster rebuilt from a pane status; shipped files such as `/social-preview.png` still counted as app windows; and a closed pane's tap always said its workspace had closed. ## Cause - `public/sw.js` `notificationclick` picked `windows.find(focused) || windows[0]` without checking `client.url`. `matchAll` lists the most recently focused window first, so the reader tab got the `select-pane` message. It renders `FileReader`, not `App`, so nothing consumed the message, and `focus()` succeeded, so `openWindow` never ran. - `src/App.tsx` handled "PC missing" and "pane closed" only for address links (`pendingLink`, `resolveLink`, `setHeaderNote`). Notification targets went straight to `selectTarget`, which also cleared `pendingLink`. That includes the worker's `selectLatest` repeat on a cold start, so a cold start's link could lose its machine-missing handling as well. A stored selection never set `pendingLink`. - Review follow-ups: the link resolver only re-ran on snapshot or selection changes, and dropped a lookup whose `pendingLink` object had been replaced, so the worker's second identical delivery stranded it. The tap handler decided "PC missing" from whatever roster the tab held, and the selection check ran on any `machines` change, including lists rebuilt from a pane status before `load()` brought a just-added PC. The alert carried no workspace, so `resolveLink` could only answer workspace-closed for a closed pane. ## Fix - `sw.js`: new `isAppWindow(url)`. Only the app shell's paths count: it rejects `?read=` readers, `/api/` responses (raw files and HTML pages), `/ws`, and any path whose last part has an extension other than `index.html` (icons, `/social-preview.png`, the manifest, `/sw.js`, notices). `notificationclick` only considers app windows, and opens the app when none is left. `CACHE_NAME` is unchanged because no icons changed. - Push payload: `PushPayload.workspace_id` (optional, additive). `server/push.ts` records each pane's workspace in `seed()` beside its title and sends it with status and ended alerts. `sw.js` keeps it in the notification data, posts it with `select-pane`, and puts it in the cold-start URL as `ws=`. - `src/lib/notificationTarget.ts`: `machineMissing`, `machineGone(roster, rosterSeq, since, id)` (a PC is gone only by a roster the server sent after it was named), `notificationLanding(roster, target)` (select, or a link carrying the alert's workspace, marked `stale` when the cached roster lacks the PC) and `sameLink`. - `App.tsx`, notification handler: - A pane the roster lists is selected as before. - Anything else is set as `pendingLink`, the same way popstate does, so it settles through `resolveLink` with its notes. With the alert's workspace, a closed pane opens its workspace's pane with the pane-closed note. - A second identical delivery keeps the pending link (and its lookup); a new link bumps `linkTick`, which the resolver effect watches, so it also settles when the pane is already selected. - A PC missing from the cached roster no longer settles at once: the link waits and `load()` asks for a fresh roster. - `App.tsx`, machine-missing effect: counts server rosters (`rosterSeq`, bumped in `load()` and the `machines` event, never for pane-status rebuilds) and decides only from one sent after the link or selection was made. A remembered selection (reload, new window) is checked against the first roster, and the selection keeps being checked against each newer one (a PC removed elsewhere while selected); "Open PC" before `load()` is checked against the roster `load()` brings. A `rosterTick` state re-runs the check when a roster comes in while a link waits, even if the list is unchanged (no re-render per poll otherwise). ## Tests - `src/pwa-notification.test.ts`: - `isAppWindow` cases, now including `/social-preview.png`, `/THIRD_PARTY_NOTICES.md`, the manifest, `/sw.js` and `/favicon.ico`. - The push handler keeps `workspace_id` in the notification data; a tap posts it and a cold start opens `/?machine=…&ws=…&pane=…&view=chat`. Both fail on the previous `sw.js`. - A reader tab used last never gets focused or messaged, and the app window takes the pane. - With only reader or raw tabs open, the app opens on the pane. - Existing tests now give clients a URL. All 17 fail without the `sw.js` change. - `src/lib/notificationTarget.test.ts`: `machineMissing`, `machineGone` (a roster no newer than the naming never settles; one after it does), `notificationLanding` (a PC the cached roster lacks waits as a stale link, known pane, unknown roster/panes/pane, the alert's workspace with `resolveLink` giving `pane-closed`), `sameLink`, and `workspace_id` parsing in `notificationTargets`. - `server/push.test.ts`: status and ended alerts carry the seeded `workspace_id`. - `scripts/notification-view-regression.ts`: an ended pane's alert delivered twice while the session lookup is held (`page.route` on `/api/session`, released after the second tap) lands on its workspace's pane with "That agent has closed: its workspace is open". Run alone through `bun run check run`: passes with the fix, times out on the note with the dedupe and `linkTick` taken out. - First commit: `bun run build` then `bun run check run bun scripts/ui-contexts-regression.ts`: pass. After the review follow-ups: `bun run build` then `bun run check run bun scripts/ui-contexts-regression.ts` (includes `checkNotificationStartup`, which pins the `select-pane` message shape, and `checkNotificationView`): pass. - `bun run check fast` (workflow syntax, generated types, typecheck, build, unit tests: 3080 pass, 0 fail): pass. ## Notes - #957 (service worker cache per build) also edits `public/sw.js`, in the cache and fetch code; this PR edits the push and notificationclick handlers and no longer calls `isCacheFirst`, so the two should merge with at most a textual conflict. Otherwise no open PR touches `public/sw.js`, `src/lib/notificationTarget.ts` or `src/lib/deepLink.ts`. Several open PRs edit `src/App.tsx` (#951, #917, #909, #890, #822), but none in the notification or machine-missing code. - Wording: the notes reuse the existing link strings, so no new i18n entries. A reload of a removed PC also shows "That link's PC is not set up here"; a separate non-link wording is left out. - An alert from a server before this change, or for a pane that alerts before the server's first `seed()` saw it, has no `workspace_id`: its closed pane still gets the workspace-closed note and herdr's fallback pane. - A tap for a PC the tab's roster lacks selects that PC while a fresh roster is fetched, so the pane area is empty until it arrives (the same state as a reload remembering a removed PC); a failed fetch is retried by the next poll. - Left out: the push handler's `watching` check still counts any visible window, a reader tab included; a split view's other half on a removed PC is not handled here. ## Merge-session review Fixed (2be0fd0, 3b87dfc): - **Ended pane still in the cached roster** (medium): a tap on a pane the resumed tab still listed selected it and dropped the alert's `workspace_id`; when the next roster removed the pane, the app fell back to herdr's focused pane with no note. The tap now keeps the alert's workspace as a hint with the selection (cleared by any other pick); once the selected pane is confirmed gone against `fetchSession`, the hint resolves through `resolveLink`: the workspace's pane opens with "That agent has closed: its workspace is open". Regression in `scripts/notification-view-regression.ts`: the pane is listed at the tap, closed through herdr afterwards, with herdr's focus in another workspace; it times out without the fix. - **Tab alerts without `workspace_id`** (medium): with push off and alerts on in this tab, `showPaneStatusNotification` / `showPaneEndedNotification` now put the pane's `workspace_id` in the notification data, and the worker-less fallback click goes through the same landing as a worker's tap. Unit test in `src/lib/notifications.test.ts` (fails without the fix). - **Remove PC on the selected PC** (low): the server tells the roster before it answers the DELETE, so the selection check showed "That link's PC is not set up here" for the user's own removal. `MachineSidebar` now selects this PC before the request and restores the selection if the request fails. `scripts/machine-conflict-regression.ts` gains an event stream and a DELETE that tells the roster first; it fails without the fix. - **Unbounded wait in the browser check** (low): the `/api/session` lookup wait races a 4 s timeout, and `release()` / `unroute` run in a `finally`. Deferred: - An old server's push (or a pane that alerts before the server's first `seed()`) with no `workspace_id`, whose pane alone is gone still shows "That link's workspace is closed"; settling such a tap silently is a wording/policy choice, left as in the Notes above. Checks: `bun run check fast` (3081 pass, 0 fail); `CI=1 bun run check run --build bun scripts/ui-contexts-regression.ts`: pass; `CI=1 bun run check run --build bun scripts/machine-conflict-regression.ts`: pass. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Owner
|
Thanks for the work on this. Closing it: the maintainer decided not to take this UI change for now. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #881
Change
New tab always used the workspace's folder and showed it as a fixed line, so starting an agent in a
subfolder or a sibling project needed a new workspace or a
cdafter the fact.The New tab dialog now shows the same editable Directory field, Browse button and recent folders as
New workspace.
NewTabTarget.cwdthatAppalready computes). A?new=tab&cwd=link still fills the field, andnow takes effect (before, the tab ignored it).
createTabascwdand applies to the new tab only. The panesalready open keep their folders.
(ko, ja, zh, tr) are removed.
CHANGELOG.mdgets a line under Unreleased, Added.Validation
bun install: exit 0HERDR_TEST_MODE=unit bun test ./src/lib/i18n.test.ts: 20 pass, 0 fail, exit 0bun run typecheck: exit 0bun run build: exit 0scripts/ui-regression.tsnow expects an editable#new-session-cwdseededwith the workspace folder and one Browse button, then creates a tab in another folder and checks
that the new pane's cwd is that folder, that the first pane's cwd is unchanged, that the folder
is stored in the recent folders and that a recent folder fills the field. I could not run the
whole 2000-line script on Windows (it stops early on this machine, before the New tab step). A
focused Playwright script with the same flow passed against a real herdr test session, exit 0:
field seeded with the workspace folder, Browse opens the folder browser, the new tab's pane opens
in the chosen folder while the first pane stays put, the chosen folder shows as a recent folder
chip that fills the field, and
?new=tab&cwd=prefills the field.NewSessionDialog.tsxfails (no#new-session-cwdin the dialog), exit 1.
No HTTP or WebSocket contract changed:
/api/tab/createalready acceptedcwd.🤖 Generated with Claude Code