fix(web): preserve dialog state through close transitions - #5199

💭 I want to dig into the PR itself, focusing on dialog close transitions, the ExpandedImageDialog rewrite to Base UI Dialog, the RightPanelSheet keepMounted removal, and CommandPalette content mounting. Let me check the actual files involved. I'm checking the shared ui/dialog component to see if ExpandedImageDialog should be using those primitives instead of reconstructing Dialog.Root/Portal/Backdrop/Popup manually — though the full-screen image dialog's unique geometry might justify the divergence. I'm noticing ExpandedImageDialog rebuilds its own Base UI dialog primitives rather than reusing the shared DialogPortal/Backdrop/Viewport components, likely because its full-viewport image lightbox styling intentionally diverges from the standard card-chrome dialog look. I'm also checking that the import path matches the one used in ui/dialog for consistency, and noting that the Escape key handling now relies on Base UI's built-in behavior instead of a manual stopPropagation call. There's also a subtlety around the conditional rendering of the dialog content tied to the open/preview state that I want to trace through on close. Next I want to verify how the generation key remounts the dialog when a new image opens mid-close, then check CommandPalette's content-mounting gate, GitActionsControl's onOpenChange dependency array, the ConnectionsSettings diff around the open-change handlers, and the removal of keepMounted in RightPanelSheet. Since the outer conditional was removed, keepMounted actually mattered before but is now moot since the sheet stays mounted regardless—removing it just means children unmount on close, same as prior behavior but now with an exit animation. I'm double-checking whether the right panel content still gets created even while closed, since React might still evaluate those children eagerly. Now I want to check the SshPasswordPromptDialog's open state logic—it starts closed, opens via an effect, and closes on submit before triggering removal, so I should verify there's no race if the dialog never actually opens. I also want to compare the two image-closing handlers in ChatView, since one seems to animate out while the other unmounts immediately without animation—worth looking at both definitions directly. I'm checking whether ExpandedImageDialog, keyed by generation and mounted only when open, will still play its entry transition given it mounts already open—wondering if Base UI applies the starting-style state on initial mount the way Radix does. Opening a new image increments generation, forcing a remount rather than a smooth transition between images, which is a minor cosmetic tradeoff. The thread-change effect intentionally clears the dialog without animation, and I'm confirming the popup stays rendered during the exit transition since the item and preview values remain truthy. I'm also checking that the dialog's accessibility semantics—role, aria-modal, and the sr-only title—carry over correctly now that it's built with Base UI's Viewport and Popup components instead of a plain div. I'm also tracing through the drag-region and z-index behavior to make sure the fullscreen overlay still blocks window dragging and stacks correctly, and now I'm digging into whether Escape key presses get properly stopped from propagating to other global handlers like the composer or panel-closing logic, since Base UI's dismiss behavior may attach its own document-level listener that behaves differently than the previous manual handler. I'm checking the CommandPalette's dialog mounting logic for edge cases, like whether toggling modes while closing could leave the content mounted state out of sync, but it seems the open-change-complete handler covers that case fine. Now I'm moving on to review GitActionsControl, ConnectionsSettings, and SshPasswordPromptDialog. I realize there's a timing issue: since open now starts false and only becomes true inside the rAF callback, the manual focus call on the input ref runs before the dialog actually renders its content, so inputRef.current is still null and the focus/select calls become no-ops. Previously this worked because open was always true from mount, so the input already existed when focus was called. Confirming the ref is definitely null at that point since the popup isn't mounted while closed, so focus is genuinely lost until Base UI applies its own default focus behavior. This is worth flagging as an inline comment suggesting initialFocus={inputRef} on the DialogPopup instead of the imperative focus call — I should check the Base UI version and whether other dialogs in the repo already use that pattern. I don't see any initialFocus usage, so I should check the Base UI version and docs to confirm whether Dialog Popup defaults to focusing the first focusable element automatically. If that's the case, the input would get focused anyway since it's the first focusable element in the popup (no close button, no focusables in header), which would weaken my finding about the dead focus+select code. Still, I think it's worth flagging as a moderate accessibility/interaction-semantics issue since the explicit select() behavior is lost, with a suggested fix of using initialFocus on the DialogPopup — but I want to verify Base UI's actual default behavior by checking the package version in package.json before finalizing this. I'm trying to recall Base UI's default focus behavior for Dialog—whether it focuses the first focusable element or the popup itself—but without access to the source or web search, I can't verify this definitively from memory alone. Given that uncertainty, I'll phrase the comment carefully: the imperative focus/select call in the rAF now runs while the popup is unmounted, so the input ref is null and the explicit focus+select no longer happens. I'll recommend setting initialFocus on the DialogPopup instead, noting that even if Base UI's default happens to focus the input, the select() behavior is now lost—framing this as "no longer runs; relies on default focus" rather than overclaiming that focus is lost entirely. Now I want to check the GitActionsControl's handleOpenChange dependencies more closely, though the destructured props dependency looks fine for the exhaustive-deps lint rule. The bigger thing to verify is the commit dialog flow—previously the close paths reset state directly, but now reset happens on onOpenChangeComplete, and the success handlers that removed their reset calls need to still properly close the dialog for that completion handler to fire. I'm worried about a race condition: if the user reopens the commit dialog while it's still animating closed, the reset logic tied to onOpenChangeComplete(false) might not fire correctly, leaving stale state like the commit message uncleared. This is a low-value edge case though. The bigger thing to check is whether onOpenChange={setIsCommitDialogOpen} behaves correctly — it should only fire for user-initiated changes, so that seems fine. Now I want to look at the ConnectionsSettings diff around lines 3100-3180, since there's an insertion there that looks suspicious and worth examining more closely. The WSL dialog keeps referencing pendingWslChange's kind during the close animation so the text doesn't flash, which is the intent of the PR. I'm also checking that the desktopServerExposure dialog's use of onOpenChangeComplete is pre-existing and unrelated to this diff, then looking at how handleConfirmWslChange updates pendingWslChange's open state to false. If pendingWslChange is null at that point, spreading it would produce an object missing the kind field, which could cause a type error—I need to verify this. I'm noticing the AddProviderInstanceDialog is now always mounted rather than conditionally rendered, controlled via open state instead. That changes when its internal state resets—previously unmounting handled reset naturally, but now I need to check whether reset happens properly on close since the component stays alive. I'm noticing that ExpandedImageDialog rebuilds the dialog shell by hand instead of using the existing DialogPortal/DialogBackdrop/DialogViewport primitives, duplicating backdrop classes that should come from the shared dialog component. That seems to violate the guidance to prefer the core UI primitives over locally reconstructed ones, so I want to check the dialog implementation further. I'm noticing that dialog-backdrop is a global class tied to nested-dialog stacking behavior, but the new ExpandedImageDialog backdrop uses raw classes instead, so it likely won't participate in that parent-scaling effect. I should check how Base UI tracks nesting context to confirm. The image lightbox deliberately keeps an opaque dark backdrop rather than the themed translucent blur, preserving its prior look, so using raw Base UI Dialog parts here feels justified given the differing geometry rather than flagging it as a violation. Checking for actual regressions in ExpandedImageDialog: the popup has its own close button, the backdrop uses forceRender consistently with the shared dialog, and the default modal behavior now adds focus trapping and scroll lock that the old plain overlay lacked, which is an improvement. One concern is that the full-screen close button inside the popup is the first focusable element, so Base UI's initial focus may land there rather than somewhere more meaningful. I'm also tracing whether mounting the dialog already open (since ChatView conditionally renders it based on whether an image is expanded) triggers Base UI's starting-style transition state correctly, since that depends on how useTransitionStatus handles an initial open=true case. I notice CommandDialogBackdrop doesn't set forceRender unlike the other backdrops, which seems pre-existing rather than part of this diff. That raises a question about CommandPalette gating: CommandPaletteDialog was always rendered before and used a Portal that unmounts while closed, so the gating might exist because CommandPaletteDialog itself runs hooks like data fetching even when closed. Actually those hooks ran regardless of mount state before too, so this change seems to delay mounting the dialog content until it's actually opening, likely for performance and to keep data alive during the exit animation. I'm now worried about a potential bug: if dialogContentMounted starts false and something opens the dialog without going through the proper setOpen/toggleMode path — like a CommandDialogTrigger elsewhere in children — the dialog could open with empty content. I want to check whether any CommandDialogTrigger usages exist that could bypass this state. Checking further, it looks like CommandDialog's onOpenChange always routes through setOpen, which correctly sets dialogContentMounted when opening, so any trigger-driven open should be fine unless something sets open imperatively outside that path. Now I want to look at how ChatView handles its PullRequestDialogState.open and closePullRequestDialog logic. I'm checking whether switching the ExpandedImageDialog key from ${src}:${index} to a generation counter still triggers a remount correctly when a different image opens while the dialog is active — it seems equivalent since both force a reset. Now I'm turning to the RightPanelSheet's keepMounted removal, examining the conditional that renders it only when the sheet mode is active, the panel is open, and there's an active thread reference. Thinking through why keepMounted mattered: it likely preserved DOM state for embedded previews like terminals or browser webviews inside RightPanelTabs when the sheet closed, since unmounting would kill that session. With the new always-rendered approach using open={rightPanelOpen} but without keepMounted, closing the panel now fully unmounts that content instead of just hiding it, which could drop live preview sessions. I'm now checking whether removing keepMounted actually regresses anything, since previously the parent's conditional rendering meant keepMounted never had a chance to preserve state anyway — so there's likely no real behavior change, just cleanup to avoid a permanently mounted hidden panel. I want to verify this by looking at how ChatView handles rightPanelContent and related props. I also need to confirm that PullRequestThreadDialog's newly required onOpenChangeComplete prop is updated everywhere it's used, otherwise other call sites would fail typechecking. Good, single call sites confirmed. Now I should check tests referencing the ExpandedImageDialog or image lightbox behavior, searching for things like "Expanded image preview" and "Close image preview" to see what assertions exist around dialog role and escape handling. The keepMounted prop on SheetPopup has no consumers left, but removing it isn't a required fix. I'm more interested in whether ExpandedImageDialog bypassing the shared ui/dialog wrapper in favor of the raw base-ui dialog loses any functionality the primitive provides, like backdrop styling. I want to pin down whether Base UI's default initial focus behavior actually causes a regression here, since the rAF now fires while the popup is unmounted and inputRef.current is null, meaning the explicit focus/select call never runs. Without vendored docs or offline package source to confirm Base UI's fallback focus target, I'll phrase the comment carefully: the password field likely loses its auto-focus on open and falls back to whatever's first focusable in the popup, so suggesting initialFocus={inputRef} as the fix. Checking the DialogPopup setup, since showCloseButton={false}, the Input is probably the first tabbable element, so default focus behavior might actually preserve the original UX — but the code is still dead regardless, silently dropping both the focus and select calls. I'm confirming this is genuinely dead code given React 18's batching behavior inside rAF callbacks, and that the file is in scope for review. I'll also flag a second candidate issue around the ExpandedImageDialog's close button being the first tabbable element in its popup. Now I'm checking whether Base UI's Escape dismissal behavior differs from the old handler, which used to call preventDefault and stopPropagation — I want to see if removing that could let a window-level Escape listener in ChatView also fire and cause unintended side effects like closing the right panel simultaneously with the lightbox. The global keydown handler checks if the command palette is open before letting Escape clear thread selection, so that's unrelated to the image dialog. I found one moderate SSH focus regression so far, and now I'm checking whether switching between images in the timeline while the expanded dialog is open could cause a similar remount issue. For the GitActionsControl commit dialog, passing setState directly to onOpenChange is fine, but the excludedFiles reset now happens after the close animation instead of immediately, which could leave the "Commit N files" label stale for a brief moment — negligible though. I also want to check the ConnectionsSettings AuthorizedClients section around lines 985-1045, since a reset handler there seems to have been removed in favor of resetting only after the close animation completes. I'm checking whether there's a genuine regression worth flagging, looking closely at the ExpandedImageDialog's handling of the preview prop now being nullable and whether the null case renders safely, plus reviewing the dialog's positioning classes. The fixed inset-0 styling on the Popup overlaps with the Viewport's own fixed positioning, which is redundant but not broken, and the backdrop opacity bypassing the shared treatment seems intentional for a lightbox. The scale animation on the full-screen container also looks harmless. I want to check whether the PR includes any test changes for these dialog behaviors, since the project's rules call for focused tests when primitive composition, prop forwarding, or keyboard handling changes — and so far I don't see any test updates accompanying these changes. Since ExpandedImageDialog now relies on Base UI for Escape handling instead of custom logic, that's a real behavior change worth flagging if there's no test coverage for it. I should look for existing dialog-related test files in the web app to see if testing these components is already an established pattern here. I should check whether ProviderSettingsPanel.environment.test.tsx or AddProviderInstanceDialog.environment.test.tsx assert that the dialog isn't rendered when closed, since always rendering it now could break those assumptions. These tests use a shallow element-tree visitor, and the dialog component is now always present with open=false. The read-only test checks that the "Add provider instance" aria-label is null, which refers to the trigger button, not the dialog itself—so I need to check whether the dialog is also unconditionally rendered in read-only mode by looking at ProviderSettingsPanel around lines 880-910. I'm checking whether read-only mode hides the "Add provider instance" button while still rendering the dialog itself, just kept closed since nothing triggers setIsAddInstanceDialogOpen. That should satisfy the test since the add form never becomes visible without the button to open it. Now I'm considering whether always mounting AddProviderInstanceDialog adds overhead from its settings hooks subscribing per environment panel, but that seems like an acceptable tradeoff rather than a real bug. I'm also double-checking the RightPanelSheet change removing keepMounted to make sure there's no close-animation regression when the sheet unmounts immediately instead of animating out. I confirm Base UI's Dialog.Portal with keepMounted={false} still keeps the popup mounted through the exit transition before unmounting, so that's fine. I'm also checking whether rightPanelContent and panelToggleControls get constructed even when the sheet is closed in ChatView, but creating those React elements unconditionally is cheap and not a real issue. Now I want to verify the open field added to PullRequestDialogState in ChatView.logic.ts doesn't break any test that constructs that state object without including open, which would fail typechecking. I want to check how handlePreparedPullRequestThread closes the dialog after the PR is prepared—whether it sets state to null immediately or uses an open:false flag. Let me check the PullRequestThreadDialog component itself, around lines 190-210, to see how it handles closing after the onPrepared callback fires. PullRequestThreadDialog correctly gates its rAF focus effect on the open state, so by the time it runs the popup is mounted and the ref exists. SshPasswordPromptDialog instead runs its rAF-focus effect on mount with empty deps and sets open inside that same rAF, meaning the ref is still null at that point — this asymmetry is the bug. The fix is to split the logic: keep a mount-time rAF that only sets open, then add a separate effect keyed on open that focuses and selects the input, mirroring PullRequestThreadDialog's pattern. I need to figure out which diff line to attach the inline comment to, since the effect spans several lines but only specific lines are part of the changed diff region. I'm checking the cancel/respond flow in SshPasswordPromptDialog—cancelPrompt closes the dialog and calls respond(null), which guards against duplicate responses via isRespondingRef, and onOpenChangeComplete safely triggers onRemove even if the dialog never fully opened. Now I'm considering what happens if respond(null) itself fails. The error path in respond keeps the dialog open when nextPassword isn't null, which seems fine, and the queue logic correctly renders only the first request, removing it after its close animation before mounting the next one with a fresh key and its own open animation. I also verify that GitActionsControl's handleOpenChange dependency array is valid under the React Compiler and won't trigger exhaustive-deps lint errors. I'm settling on my final review comments, focusing on the SSH focus regression as a genuine interaction-semantics issue, and now considering whether ExpandedImageDialog's bypass of the shared dialog pattern is worth flagging too. Looking closer, I think the lightbox's heavy overrides of DialogPopup's card chrome justify building from lower-level Base UI primitives rather than the full Dialog stack, so this isn't a real violation and I'll leave it out. I also rule out the memoized key prop in ChatView, the unused keepMounted prop on SheetPopup, and start examining how Dialog.Popup is rendered in ExpandedImageDialog to see if anything there warrants a comment. One thing worth flagging: the full-screen close button sits as the first focusable element inside the popup, so Base UI's default focus-on-open behavior could land focus there, making an immediate Enter/Space press close the dialog unexpectedly — a subtle accessibility quirk, though not clearly a regression since focus management wasn't really handled before either. I'm also checking CommandPalette's mount state and initial open logic, but nothing stands out there. Given the one moderate focus-related finding, I'm weighing whether this warrants a failure verdict or falls within acceptable risk, leaning toward treating it as a real but modest regression worth flagging rather than a blocking issue. Since public repos are accessible, I'll try searching the actual mui/base-ui repository for "initialFocus" in the dialog documentation to settle this definitively. I'm tracing through Base UI's default focus behavior for the dialog, checking whether the ScrollArea viewport inside DialogPanel would be the first tabbable element since it may receive tabIndex=0 when scrollable. If so, focus would land on the scroll container rather than the password input, which would be a real regression worth verifying against the actual ScrollArea implementation. Since the ScrollArea viewport likely doesn't overflow in the SSH dialog, focus probably defaults to the password input anyway, so the "focus is lost" finding may actually be a false positive—the real issues are just the irrelevant select() call and dead code. I should be cautious about flagging that as a genuine regression. I'm weighing whether to note it as a minor cleanup item (the imperative focus/select logic running before the popup mounts is now dead code, since Base UI's default focus handles it), but that feels too trivial to report given the instruction to avoid commenting on correct code. Let me scan the rest of the PR again for something more substantive—checking CommandPalette's focus handling, the ExpandedImageDialog remount behavior, and whether expandedImage in ChatView is referenced elsewhere, like suppressing other keyboard handlers while the lightbox is open. I notice the expandedImage alias is only used once, which is a minor unnecessary indirection — I could suggest using expandedImageDialog.preview directly and dropping the alias. This also highlights that the preview prop was widened to allow null, forcing null-guards internally even though the only call site always passes a non-null value. Stepping back, I want to think about what a real UI reviewer would actually flag here. One potentially significant issue: in GitActionsControl, the commit dialog reset moved to onOpenChangeComplete, but if the dialog is reopened quickly within the close transition, that callback may never fire with a closed state, letting the previous commit message leak into the new dialog session — a real regression compared to the old synchronous clear on close. I'm also noticing the same pattern could apply to AddProviderInstanceDialog's wizard state and the AuthorizedClients pairing dialog if reopened fast. I want to verify whether Base UI's onOpenChangeComplete actually skips firing when the close animation gets interrupted by a reopen, so I should check how useOpenChangeComplete and useAnimationsFinished handle that interrupted case in the base-ui source. Weighing it though, this reopen-within-200ms scenario seems like a rare edge case and not a severe one even if it happens, so I'm considering whether there's a more impactful issue — like how GitActionsControl wires onOpenChange to setIsCommitDialogOpen directly. I traced through that the extra eventDetails argument passed by Base UI's onOpenChange gets silently ignored by React's state setter since it's called with a boolean, not a function, so there's no risk of it being misinterpreted as an updater. The same reasoning applies to the other dialogs using raw setters like setDialogOpen and setIsAddInstanceDialogOpen — all fine. Now I want to circle back to the RightPanelSheet change, since it has real layout implications: previously the sheet only mounted when open, but now it's always rendered whenever shouldUseRightPanelSheet && activeThreadRef is true, with open controlled via the rightPanelOpen prop — so I need to think through what Base UI's Dialog.Root does when open=false but the component stays mounted. Since the portal isn't using keepMounted, it renders nothing while closed, so there's no visual change in the closed state — but now the exit transition (translate and fade) plays properly on close, which is exactly what this PR is trying to fix. I also note the sheet is modal by default, locking scroll and adding inert to the rest of the app, but only while open, so that's fine too. I'm also checking how children mount and unmount: when the sheet opens, RightPanelTabs mounts in sheet mode, and on close the children now stay mounted through the 200ms exit animation before unmounting — previously they vanished instantly. This is actually beneficial since it keeps things like a terminal or browser preview surface visible during the transition instead of flashing away. Then I move to checking PullRequestThreadDialog in ChatView, which uses a key derived from Date.now() at open time — so reopening generates a new key, forcing a remount that resets internal state, while closing preserves state until the animation finishes. That seems like the right behavior, though I want to consider the edge case where openPullRequestDialog fires while a dialog is mid-close, producing a new key and open: true simultaneously — I need to trace through what the component does in that scenario. The component remounts fresh with open=true, so it plays the mount animation from scratch, which seems fine. I also check the dialog's focus effect tied to the open dependency — since the component mounts with open=true, the requestAnimationFrame call runs after mounting, so focusing the input should work correctly. Now I'm comparing the ExpandedImageDialog transition classes against the shared DIALOG_POPUP_CLASS conventions — the lightbox popup's scale/opacity transition matches the shared class well enough, and the backdrop's opacity-only transition versus the shared "transition-all" is close enough not to matter. I also verify the backdrop is actually visible given the z-index layering and DOM order between Dialog.Backdrop and Dialog.Viewport — the viewport paints on top as expected, so that's fine. One more thing worth confirming: whether Dialog.Viewport is a valid export in this version of the dialog library — yes, it maps to the primitive's Viewport component. I'm reconsidering whether forceRender on the backdrop causes any issue with nested dialogs — it shouldn't. I also check whether the arrow-key window listener still works properly when focus is trapped inside the modal popup, which it does. The real question I want to dig into is a potential regression: removing preventDefault/stopPropagation from the Escape handling in ExpandedImageDialog. I need to figure out whether there's a global Escape handler elsewhere in the app that would now also fire when closing the lightbox, and whether the dismiss-on-Escape behavior calls preventDefault() internally or not — I don't think it does by default. So I want to check whether the global handler checks event.defaultPrevented before acting, which means I need to look at how that handler begins. That edge case with escape during thread selection and lightbox is too rare to flag. Scanning the diff for CSS ownership issues, the only new styling is in ExpandedImageDialog using Tailwind at the correct ownership level, so no global CSS concerns there. I'm also checking whether the new dialog markup follows the rule about preferring core UI primitives over locally reconstructed ones, noting it now imports from the base component library. Looking closer, the file hand-rolls Backdrop/Viewport/Popup directly rather than using the existing Dialog/DialogPortal/DialogBackdrop/DialogViewport exports, but given how the dialog-backdrop utility's CSS declarations could conflict with Tailwind override classes depending on layer ordering, composing the base parts directly here is actually the cleaner approach since it avoids inheriting the themed glass backdrop the lightbox shouldn't have. I'm comfortable not flagging this. Before concluding everything's clear, I want to revisit the SSH dialog once more since there's a real behavioral difference beyond focus handling worth checking. Tracing through cancelPrompt and respond, the expired-prompt dismissal path, and the queue rendering for multiple SSH prompts, everything seems to resolve correctly through the animation and removal lifecycle. I'm checking whether onOpenChangeComplete could fail to fire in edge cases, but that seems unreachable without direct UI interaction, and the mount/rAF sequencing for ActiveSshPasswordPrompt also checks out fine. Now I'm looking more closely at the CommandPalette's mounted-state gating logic, specifically the edge case where toggling the same mode while the palette is already open triggers a close — I want to verify the ordering of the mounted=true and open=false state updates when they land in the same batch. Actually, since open transitions true→false, onOpenChangeComplete does fire and mounted becomes false — so only a dispatch that's a no-op on open could leave it stuck. I should check the reducer to see if any action leaves open unchanged. I'm tracing through the state transitions to confirm there's no stuck state — every path that sets mounted to true also sets open to true, except the no-op close-when-already-closed case. The key fix is that the dialog content stays mounted during the exit animation rather than unmounting immediately, and I'm checking that React batches the mounted/open updates together when the palette opens so the dialog renders correctly in the same commit. Now I'm digging into ExpandedImageDialog for a real edge case: if preview.images is empty, the computed index resolves to an undefined item, so nothing renders inside the Dialog.Root even though open is still true. That's concerning because with no popup mounted, onOpenChangeComplete might never fire, which could leave ChatView's state stuck open indefinitely with nothing visible and no way to close it — unlike the previous version which returned null outright in that case. But I realize previews always have at least one image in practice, so this is a pre-existing edge case that behaves equivalently to before — not a new bug introduced by this PR. I should move on and check whether RightPanelSheet's exit animation and its class name still work correctly now that keepMounted was removed, since something might rely on the component staying mounted. I'm deciding the unused keepMounted prop is unrelated cleanup and not worth flagging, per the smallest-durable-contract guidance. That leaves the SSH focus point as the one finding worth pursuing, especially now that I've confirmed Base UI's default focus behavior targets the first tabbable element. Tracing through what that first tabbable element actually is, I realize it depends on whether the ScrollArea viewport gets a tabIndex when content overflows — if so, focus could land on the scrollable container instead of the password input, both before and after the change, since the dialog was already open on mount in the old behavior too. So the real question is whether the rAF focus call was actually correcting a focus mismatch in the overflow case, meaning removing it introduces a genuine (if narrow) regression. That seems worth flagging as a precise, low-severity inline comment, with the fix being to gate the focus effect on open or pass an explicit initialFocus prop to the dialog. I'm weighing whether this modest interaction nit is significant enough to fail the check versus just noting it, given the instructions lean toward failing by default unless everything is clearly clean. But the actual output format is JSON per schema, so "defaults to failure" really just means findings should translate to a failure verdict. Since explicit guidance exists here overriding the usual bias-to-success default, I should be confident the SSH focus issue is substantial enough to justify failing, so let me check whether Base UI's ScrollArea viewport actually receives a tabIndex by searching the source for that. So when the scroll area is scrollable, its viewport becomes tabbable and sits before the password input in tab order, meaning with the rAF focus gone, an overflowing SSH dialog would focus the scroll container instead of the password field. When it's not scrollable, the viewport isn't tabbable so the input gets focus naturally—confirming the regression is real but conditional on overflow. The cleanest fix is setting initialFocus={inputRef} on the DialogPopup, which Base UI supports directly, rather than gating the rAF on the open state. I'm now working out exactly which lines in the diff to attach my inline comment to, mapping the hunk header to the actual file lines containing the useEffect block with the rAF call, so I can place a precise multi-line suggestion comment on the SSH file. I'll draft the explanation: since setOpen and the focus call happen in the same frame, the popup isn't mounted yet when the ref is read, so the explicit focus/select never fires, and Base UI's default initial focus lands on the scrollable panel instead of the password field. I'll suggest passing initialFocus={inputRef} directly to DialogPopup and removing the now-dead focus logic. Now I'm deciding how to submit this — posting it as a review comment with state COMMENT, since the tool just needs body and comments without an explicit event. For the overall verdict, I'm weighing whether this modest, conditional focus regression warrants a failure or neutral outcome, leaning toward treating it as a real but moderate regression risk per the check's guidance.

1. modify_pr — One finding on the dialog-transition migration: the SSH password prompt's mount-time focus no longer runs now that open starts false. Everything else (Base UI onOpenChangeComplete for deferred reset/unmount, the ExpandedImageDialog port to Dialog.Root, and the RightPanelSheet open-prop change) preserves the existing geometry, class ownership, and close semantics.