fix(studio): stop side panels closing twice after react-router v8 - #1731
Open
aray12 wants to merge 3 commits into
Open
fix(studio): stop side panels closing twice after react-router v8#1731aray12 wants to merge 3 commits into
aray12 wants to merge 3 commits into
Conversation
URL-driven side panels animate out, snap back open, then animate out again. Two deferrals stack on the close path. KUI's SidePanel passes animateOutDuration=200 into useDialog, so onOpenChange(false) - and therefore the consumer's navigate() - fires from inside a setTimeout after the exit animation. react-router 8 then wraps that location update in React.startTransition by default, which v6 did not do. React flushes the urgent setIsClosing(false) first, producing a commit where the open prop is still true but the native <dialog> is already closed. useDialog's layout effect takes its `if (isOpen)` branch and calls showNativeDialog(), re-opening the panel; when the transition finally lands it closes a second time. Pass flushSync: true on the navigations that close a URL-driven panel so the param clear commits alongside the dialog close. This requires RouterProvider from react-router/dom - only that variant injects ReactDOM.flushSync. a6904bd moved both createBrowserRouter and RouterProvider onto the bare react-router entry point during the v8 upgrade, which silently made the flushSync navigate option a no-op that only logs a warning. Also covers the create-deployment panel, where a deferred setSearchParams let the createPrefill effect observe stale params and reopen the panel. The regression test asserts showModal() is not called after dismissal; it fails without the fix with exactly one spurious re-show. It spies on showModal rather than watching data-state because the shared test setup stubs MutationObserver to a no-op suite-wide. Signed-off-by: Alex Ray <alray@nvidia.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (4)
🚧 Files skipped from review as they are similar to previous changes (2)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 WalkthroughWalkthroughThe change enables DOM-backed ChangesSide-panel navigation
Merge Risk: ⚪ Minimal · up to This PR applies localized navigation fixes with targeted validation, and no actionable merge-blocking risk remains beyond normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 8 files. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
|
FilesetNewRoute is a whole route rendered as a side panel, mounted with a
literal `open` rather than a state-derived one. The prop can never go
false, so the navigation that unmounts the route is the only thing that
closes the panel.
That makes it the most pronounced instance of the same stale-prop race:
KUI's useDialog emits onOpenChange from inside its 200ms animate-out
timeout, react-router v8 defers the resulting navigation into a
transition, and on the urgent commit the layout effect sees a permanently
true `open` against an already-closed <dialog> and calls
showNativeDialog(). The panel flashes back up and only disappears when
the transition finally unmounts the route.
The earlier sweep missed this because it looked for `open={...}`
expressions derived from route params; a bare `open` attribute does not
match that shape. Re-swept for the bare-`open` form: three other panels
use it, but all three close via local setState, which is urgent and
leaves no window for the re-show.
The regression test asserts showModal() is not called after dismissal and
fails without the fix with exactly one spurious re-show. It restores the
real router hooks, since the existing suite in this file mocks
useNavigate and so never runs a genuine navigation.
Signed-off-by: Alex Ray <alray@nvidia.com>
The navigate options read clearly enough on their own, and the rationale lives in the commit history and the regression tests. Restores the pre-existing comment on PanelManagement's open flags verbatim, so that file is now a pure three-line change. Keeps one line in the test harness: that the declarative MemoryRouter fallback silently ignores flushSync is not inferable from the code, and a test written against that path would pass vacuously. Signed-off-by: Alex Ray <alray@nvidia.com>
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
2 participants
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.
Summary
Side panels close twice: the panel animates out, snaps back open, then animates out again — visible as a flash. This regressed with the react-router v6 → v8 upgrade (a6904bd). Passing
flushSync: trueon the navigations that close these panels restores the pre-upgrade commit ordering, and switchingRouterProviderto thereact-router/domentry point makes that option actually take effect.Before
Screen.Recording.2026-09-02.at.1.58.06.PM.mov
After
Screen.Recording.2026-09-02.at.2.00.03.PM.mov
Changes
Root cause. Two deferrals stack on the close path:
SidePanelpassesanimateOutDuration = 200intouseDialog, soonOpenChange(false)— and therefore the consumer'snavigate()— fires from inside asetTimeoutafter the exit animation.react-router@8.3.0wraps every router state update inReact.startTransitionby default (lib/components.js:348). v6 was synchronous.React flushes the urgent
setIsClosing(false)first, producing an intermediate commit where theopenprop is stilltruebut the native<dialog>is already closed.useDialog's layout effect takes itsif (isOpen)branch and callsshowNativeDialog(), re-opening the panel; when the transition finally lands, it closes a second time.Fixes
App.tsx— importRouterProviderfromreact-router/dom, keepingcreateBrowserRouteronreact-router. Only the DOM variant injectsReactDOM.flushSync.a6904bd83moved both onto the bare entry point during the v8 upgrade, which silently turned theflushSyncnavigate option into a no-op that only logs a warning. This is a prerequisite for everything below.DeploymentsListRoute—flushSync: trueon the details-panel close and on the post-delete close.DeploymentsListRoute—flushSync: trueon the create-panel deep-link param cleanup. Same shape viasetSearchParams: a deferred param clear let thecreatePrefilleffect observe stale params and reopen the panel.FilesetListRoute/PanelManagement—flushSync: trueon the dataset-panel close, file-panel close, and file-panel outside-click. The existing mirrored open flags are kept; the comment explaining them is corrected to say the two mechanisms are complementary.MetricRunSidePanel— same treatment on its post-submit navigation. This component currently has no consumer outside its own directory and stories, so nothing exercises it; hardened so it is correct when wired up.tests/util/render.tsx— shared harness uses thereact-router/domRouterProvider, so tests can exercise the flushed path. Note this only applies to thecreateMemoryRouterbranch; the declarativeMemoryRouterfallback ignoresflushSyncentirely.FilesetNewRoute—flushSync: trueon the close navigation. This is the create-fileset panel, and the most pronounced instance: the route is the panel and is mounted with a literalopen, so the prop can never go false and the navigation unmounting the route is the only thing that closes it. The re-show is therefore guaranteed rather than merely likely, and the panel flashes back up until the transition lands.routes/DeploymentsListRoute/index.test.tsxand inroutes/FilesetNewRoute/index.test.tsx.Navigations that move between panel states rather than closing to a panel-less route were deliberately left alone, as were panels whose
openis plain local state.The
flushSync: trueoptions carry no explainer comments — the rationale lives here and in the commit history, with the regression tests as the executable record. One line is kept in the test harness: that the declarativeMemoryRouterfallback silently ignoresflushSyncis not inferable from the code, and a test written against that path would pass vacuously.Sweep coverage. The first pass looked for
open={...}expressions derived from route params, which does not match a bareopenattribute — that is howFilesetNewRoutewas missed initially. Re-swept for the bare-openform: three other panels use it (SafeSynthesizerFilesetPreview/FilePreview,customizer/CustomizationFilesetSelect,PromptTuningForm/InContextLearningSection), but all three close via localsetState, which is urgent and leaves no window for the re-show.Type of Change
Quality Gates
Verification
Signed-off-by:traileruv run pre-commit run -apasses, or any blocked checks are identified belowTargeted validation:
pnpm --filter nemo-studio-ui typecheckpnpm --filter nemo-studio-ui lint(--max-warnings 0)pnpm --filter nemo-studio-ui formatpnpm --filter nemo-studio-ui test(full suite)pnpm --filter nemo-studio-ui test src/routes/DeploymentsListRoute src/routes/FilesetListRoutepnpm --filter nemo-studio-ui test src/routes/FilesetNewRouteuv run pre-commit run -aFull-suite flakes. Two runs of the full suite on an identical tree produced different failure sets (4 files / 49 skipped, then 2 files / 0 skipped), and an earlier run of the same tree was fully green. The affected tests —
components/Layouts/GlobalNavandroutes/SafeSynthesizerNewRoute— pass in isolation, take ~11.5s when they fail (timeouts under load), build their own routers directly fromreact-router, and do not touch the shared test harness or any route changed here. Treating these as pre-existing load flakiness rather than a regression; CI is the arbiter.Red/green check on the regression tests. With
flushSync: truereverted fromhandleCloseDetailsPanel, the deployment test fails withexpected "showModal" to not be called at all, but actually been called 1 times— exactly the predicted spurious re-show. It passes with the fix restored. TheFilesetNewRoutetest behaves identically — one spurious re-show without the fix, none with it. An earlierMutationObserver-based version of these tests passed against both the fixed and broken code; the shared test setup (packages/testing/src/react/setup.ts:50) stubsMutationObserverto a no-op suite-wide, so the test now spies onHTMLDialogElement.prototype.showModalinstead. Worth knowing more broadly: that stub also makes KUI's own top-layer-healMutationObserverinert under test.uv run pre-commit run -a— two failures, both environment-only and unrelated to this change:Helm Docs—helm-docsis not installed locally. This PR changes no Helm files.Run uv lock with platform uv— local uv is 0.9.28, the hook requires 0.9.14. This PR changes nopyproject.toml; the separateCheck for uv.lock drifthook passed.Both hooks reported
(no files to check) Skippedon the actual commit, confirming they do not apply here. Every other hook passed.Not verified. The
FilesetNewRouteflash was reported from manual browser use and the fix is verified by test, not yet re-confirmed in a browser. Regression tests cover the deployment details panel's close button and the create-fileset panel's close button; Escape, outside click, the fileset list panels, the create-deployment deep link, and the focus-restoration behavior remain unverified.MetricRunSidePanelis unverifiable by construction since it has no consumer.Summary by CodeRabbit