fix(desktop): profile-scope cron/messaging REST calls; contain message render errors - #58110
Conversation
…rors - Add profileScoped() to all cron and messaging REST wrappers so global remote multi-profile setups stop reading/mutating the default profile (backend already accepts ?profile= on these routes). - MessageRenderBoundary now contains all render errors with an inline fallback instead of re-throwing to the root boundary. - createRoot gains onRecoverableError/onCaughtError/onUncaughtError logging of error.cause + componentStack (decodes minified React NousResearch#520 reports). - Clamp @assistant-ui/store tapClientLookup out-of-bounds lookups via a postinstall node_modules patcher (survives npm ci; loud on drift). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved (read-only token - formal approval deferred)
Profile-scope cron/messaging REST calls so they don't leak across profile boundaries; contain message render errors to prevent one bad message from crashing the whole render pipeline. Clean multi-concern fix. No concerns.
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved (LGTM)
Profile-scopes cron and messaging REST calls; contains message render errors. Multiple desktop improvements bundled in one PR. Well-scoped for the scope.
What Looks Good
- Multiple related desktop improvements
- No security concerns
- No debug artifacts
Reviewed by Hermes Agent
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment (moderate surface area)
Fix (+406/-20, 15 files) making profile-scoped cron/messaging REST calls and containing message render errors. Desktop-focused changes.
Note
console.logstatements inmessage-render-boundary.test.tsxappear to be development/test output — appropriate in test context
Looks Good
- Profile-scoped design is sound
- Error containment is appropriate
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying the profile-routing gap: it is real on current main. apps/desktop/src/hermes.ts:727-813 omits profileScoped() for the messaging and cron wrappers, while apps/desktop/electron/connection-config.ts:200-232 converts request.profile into ?profile= for global-remote calls.
Problems
- The dependency patch is stale. Current
package-lock.json:280resolves Desktop's@assistant-ui/storeto0.2.19underapps/desktop/node_modules, whilepatches/apply-patches.cjstargets a rootnode_modules0.2.13 file. A clean current install will not patch the Desktop dependency. - Current
MessageRenderBoundarydeliberately recognizesuseClientLookupas well as legacytapClient*errors (apps/desktop/src/components/assistant-ui/message-render-boundary.tsx:12-13, added by673a61edc). The proposed matcher dropsuseClientLookup, regressing current containment.
Suggested changes
- Salvage the
profileScoped()wrapper changes and tests. - Drop or redesign the stale postinstall patch against the actual locked dependency, and retain the current matcher.
Automated hermes-sweeper review.
| // render entirely (store notification tick), blanking the desktop app. | ||
| // Production signature: "tapClientLookup: Index N out of bounds | ||
| // (length: 0)" in ~/.hermes/logs/desktop.log. Upstream: | ||
| // https://github.com/assistant-ui/assistant-ui/issues/4051 |
There was a problem hiding this comment.
Current package-lock.json resolves Desktop's store package as apps/desktop/node_modules/@assistant-ui/store version 0.2.19, not this root 0.2.13 target. A clean current install will skip this patch while Desktop continues using the unpatched dependency; please remove or retarget it based on a current-version repro.
| // The throw is transient and self-heals on the next consistent snapshot, so it | ||
| // is swallowed silently (no fallback flash, no log spam). | ||
| // Upstream-tracked: assistant-ui/assistant-ui#4051, #3652. | ||
| const isTransientLookupError = (error: unknown): boolean => |
There was a problem hiding this comment.
Current main's matcher includes useClientLookup as well as legacy tapClient* signatures (commit 673a61edc after this PR base). Keep that branch when salvaging this change or the current dependency's transient race will again escape this boundary.
|
Status update: the cron/messaging profileScoped() half of this PR landed on main via #67493 + #67602 (contract test) + #67615 (endpoint-level list filtering) — that half is now superseded. The message-render error-boundary half (message-render-boundary.tsx + react-root-error-logging) is NOT covered by any of those and still stands on its own; if you can split it into a focused PR against current main it can be reviewed independently. Leaving this open for that half. |
Problem
src/hermes.tsomitprofileScoped(). In global remote mode (shared backend, multiple profiles disambiguated by?profile=) the cron sidebar/CronView and messaging settings silently read and mutate the default profile while the rail shows another profile — same bug class as fix(desktop): route global remote profile REST calls #47011. The backend already accepts?profile=on every one of these routes, and the web dashboard already sends it.MessageRenderBoundarydeliberately re-throws anything that isn't the known transient tapClientLookup race, escalating a single bad markdown/tool-payload render to the root boundary (all UI state lost).@assistant-ui/storetapClientLookup throws on transient index races during session-switch/reconnect list replacement (tapClientLookup: Index N out of bounds), which is exactly the error class the boundary special-cases; production logs show it still firing on 0.2.13.cause, so the real culprit is invisible in packaged builds.Fix
profileScoped()added to all 9 cron wrappers and 3 messaging wrappers. Primary profile stays unscoped ({}), preserving existing single-profile behavior.MessageRenderBoundaryis now a catch-all: transient races still render null + auto-reset; any other error renders a compact i18n'd "This message failed to render." fallback (role=alert) and logs the cause with a[message-render-boundary]tag. Strings added for en/ja/zh/zh-hant.patches/apply-patches.cjs(dependency-free, idempotent, loud-on-drift) rewrites the 0.2.13tapClientLookupout-of-bounds throw into clamp-to-last/undefined-when-empty, wired into rootpostinstallso it survivesnpm ci.createRootgainsonRecoverableError/onCaughtError/onUncaughtErrorhandlers logging the wrapper,error.cause, and component stack through the console pipeline the main process already persists todesktop.log.Tests
hermes.test.ts: every cron/messaging wrapper carriesprofileunder a non-primary active profile, and stays unscoped for the primary (10 tests).message-render-boundary.test.tsx: fallback rendering, tagged console.error, transient-race silence, resetKey recovery (6 tests).patches/apply-patches.test.cjs: apply/idempotent/drift-detection + behavioral bounds checks (7 tests).react-root-error-logging.test.ts(2 tests). Desktop typecheck + lint clean.🤖 Generated with Claude Code