Repository navigation
Stop translated portals taking the whole app down - #4704
nachocossio wants to merge 1 commit into
Conversation
A browser translator rewrites text nodes in place. React then holds references to nodes that are no longer children of their parent, and when it tears down a portaled surface the removal throws NotFoundError. React Router catches it at the global errorElement, so the user loses the entire app to the "Something went wrong" screen — mid-chat, on the Playground. Sentry INSPECTOR-CLIENT-27X is the alerted case: Safari on an English UI, "The object can not be found here." (DOMException code 8), fired 7s after a Radix dialog opened, with a stack ending in commitDeletionEffectsOnFiber -> removeChild against a HostPortal container. 54 events over 30 days across Chrome, Safari and Edge; not one from Firefox, the only browser of those without translation offered by default. f4009ba diagnosed this in August and fixed one dialog. Move the opt-out to the shared primitives so all 89 DialogContent call sites and every other portaled surface are covered: dialog, alert-dialog, sheet, popover, dropdown-menu, select, tooltip, hover-card, context-menu and menubar, plus the three sub-menu contents, which portal separately and so do not inherit the parent surface's opt-out. ServerDetailModal's local copy goes with it — the primitive now supplies it, and keeping both emitted `notranslate` twice. Its August test still passes, and now proves the primitive covers a component that passes its own className. This does not cover the insertBefore variant (INSPECTOR-CLIENT-27Y, 289, 25Y), where React places a conditional child into the #root container. Closing that needs a global `<html translate="no">`, which turns off translation for the whole app and is a product call.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Internal previewPreview URL: https://mcp-inspector-pr-4704.up.railway.app |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (12)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. WalkthroughDesign-system portal surfaces now set Merge Risk: ⚪ Minimal · up to Portal-based design-system surfaces now opt out of browser translation, including nested menus, while preserving the ServerDetailModal behavior through the shared dialog primitive. The change is covered by targeted rendering tests and is ready to merge. 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="mcpjam-inspector/client/src/components/__tests__/design-system-portal-translate.test.tsx">
<violation number="1" location="mcpjam-inspector/client/src/components/__tests__/design-system-portal-translate.test.tsx:204">
P2: These three sub-content tests open the submenu with `fireEvent.click`, but Radix submenus open on pointer-enter/hover or keyboard, not click, and `fireEvent.click` fires no pointer events in jsdom. The sub content never renders, so `expectOptedOutOfTranslation` fails at its `not.toBeNull()` check, making the tests unreliable rather than testing the opt-out. Use `userEvent.hover` (await user.hover(screen.getByText("More"))) to open each submenu, matching the repo's existing pattern in model-selector.test.tsx.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| </DropdownMenuContent> | ||
| </DropdownMenu>, | ||
| ); | ||
| fireEvent.click(screen.getByText("More")); |
There was a problem hiding this comment.
P2: These three sub-content tests open the submenu with fireEvent.click, but Radix submenus open on pointer-enter/hover or keyboard, not click, and fireEvent.click fires no pointer events in jsdom. The sub content never renders, so expectOptedOutOfTranslation fails at its not.toBeNull() check, making the tests unreliable rather than testing the opt-out. Use userEvent.hover (await user.hover(screen.getByText("More"))) to open each submenu, matching the repo's existing pattern in model-selector.test.tsx.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At mcpjam-inspector/client/src/components/__tests__/design-system-portal-translate.test.tsx, line 204:
<comment>These three sub-content tests open the submenu with `fireEvent.click`, but Radix submenus open on pointer-enter/hover or keyboard, not click, and `fireEvent.click` fires no pointer events in jsdom. The sub content never renders, so `expectOptedOutOfTranslation` fails at its `not.toBeNull()` check, making the tests unreliable rather than testing the opt-out. Use `userEvent.hover` (await user.hover(screen.getByText("More"))) to open each submenu, matching the repo's existing pattern in model-selector.test.tsx.</comment>
<file context>
@@ -0,0 +1,248 @@
+ </DropdownMenuContent>
+ </DropdownMenu>,
+ );
+ fireEvent.click(screen.getByText("More"));
+
+ expectOptedOutOfTranslation("dropdown-menu-sub-content");
</file context>
| fireEvent.click(screen.getByText("More")); | |
| await user.hover(screen.getByText("More")); |
Code Review Finding: PR #4704Found 1 issue with test reliability: Unreliable Submenu Tests (P2)File: Problem: Three submenu tests use Why tests are unreliable:
Solution: Use // Instead of:
fireEvent.click(screen.getByText("More"));
// Use:
await user.hover(screen.getByText("More"));Affected tests:
This makes the test coverage for submenu translation protection unreliable. The fix is to use the correct event type that Radix actually responds to. |
This branch was successfully deployed
3 participants
What broke
A browser translator rewrites text nodes in place. React then holds references to nodes that are no longer children of their parent, and tearing down a portaled surface throws
NotFoundError. React Router catches it at the globalerrorElement(router.tsx:508-515), so the user loses the whole app to the "Something went wrong" screen — mid-chat, on the Playground.Evidence
INSPECTOR-CLIENT-27X is the alerted case: Safari 26.6.2 on an English UI,
NotFoundError: The object can not be found here.(DOMException code 8),release: 3.3.5.Breadcrumbs from that session:
The stack is entirely React internals, ending in
commitDeletionEffectsOnFiber→recursivelyTraverseDeletionEffects→ nativeremoveChild, with the frame that enterscase 4/containerInfo. Fiber tag 4 isHostPortal: React was deleting a subtree containing a portal and the node was gone from the container.54 prod events over 30 days, ~17 users, releases 2.35.1 → 3.3.6 — chronic, not a regression. Browsers: Chrome, Chrome Mobile, Chrome Mobile iOS, Safari, Mobile Safari, Edge. Not one Firefox, the only browser of those without translation offered by default.
f4009ba17("Prevent translated server dialog teardown crash", #4056) already diagnosed this in August and named the mechanism in a code comment — but fixed exactly one dialog.The change
Move the opt-out to the shared primitives, so all 89
DialogContentcall sites and every other portaled surface are covered in one place: dialog, alert-dialog, sheet, popover, dropdown-menu, select, tooltip, hover-card, context-menu, menubar — plus the three sub-menu contents, which portal separately and so do not inherit the parent surface's opt-out.ServerDetailModal's local copy goes with it: the primitive supplies it now, and keeping both emittednotranslatetwice in the DOM. Its August test still passes, and now proves the primitive covers a component that passes its ownclassName.What this does not fix
The
insertBeforevariant (27Y, 289, 25Y), where React places a conditional child (Toaster,MCPJamLimitDialog,PlanLimitDialog,SessionRefreshBanner) into the#rootcontainer. Closing that needs<html translate="no">, which turns off translation for the whole app — a product call, so it is deliberately not in this PR.Also out of scope, tracked separately: sonner's
Toaster(accepts classes only, and a barenotranslatedoes not protect Safari), and three structural defects that throw the same exception with no translator involved —widget-surface-hostreparenting a portal container outside React,HostsTab'sAnimatePresence mode="sync"wrapping both a portal container and its portal source, andkey={i}over the streaming message-parts list.Testing
13 new tests in
design-system-portal-translate.test.tsx, written first and watched fail for the right reason (element renders, attribute isnull) before the fix.Those 3 failures are pre-existing:
score-run-resume,scenario-chat-transcriptandeval-live-chat-panelfail identically on a stashed clean tree. All storage/filesystem tests, unrelated to this diff.Summary by cubic
Stops browser translators from crashing the app when portaled surfaces unmount by moving the
translate="no"opt-out to every portaled primitive in@mcpjam/design-system.ServerDetailModaland its duplicatenotranslateclass.translate="no"andnotranslate.Written for commit 45e3be1. Summary will update on new commits.