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

💭 I'm checking whether the earlier flagged issues at those prior commits were resolved by the time of the current head commit. It looks like both fixes landed—the password dialog now properly sets initial focus, and the image dialog's overlay button is hidden from accessibility tools—so I'm now scanning the rest of the diff for any new problems.

1. browse_code — apps/web/src/components/chat/ExpandedImageDialog.tsx

2. browse_code — apps/web/src/components/RightPanelSheet.tsx

💭 I'm checking whether ExpandedImageDialog should use the shared ui/dialog wrappers instead of raw Base UI Dialog primitives, but since it's a full-viewport lightbox with different geometry, the shared components might not actually fit its needs well.

3. browse_code — apps/web/src/components/ui/{dialog,sheet}*.tsx

4. browse_code — apps/web/src/components/ui/dialog.tsx

5. browse_code — apps/web/src/components/ui/sheet.tsx

💭 I'm removing keepMounted from RightPanelSheet since open is now directly controlled by rightPanelOpen, so the portal can safely unmount on close while Base UI still handles the exit animation. Now I want to check the other changed files more closely — ChatView.tsx's dialog usages, GitActionsControl, ConnectionsSettings, CommandPalette, and SshPasswordPromptDialog — to see if ExpandedImageDialog's switch to raw Base UI primitives instead of the shared ui/dialog components introduces any inconsistency. Looking at ExpandedImageDialog's structure, the different backdrop styling and full-viewport popup seem intentional for the lightbox use case rather than a bug, so I'm inclined not to flag it. The overlay z-indexing and Escape key handling both look correctly delegated to Base UI's native behavior, so those are fine. I want to verify whether the preview prop can ever be null when the dialog renders. I'm also noticing a potential edge case: if item is undefined due to an out-of-range index, the portal returns null while the dialog is still open, leaving an empty dialog. I should check the ChatView usage around line 7074 to see how this is handled, and confirm the sr-only Dialog.Title still preserves the accessible name that was previously set via aria-label. More importantly, I'm concerned about the removal of keepMounted in RightPanelSheet — if that panel hosts content like a webview or terminal, unmounting on close could lose scroll position or reload iframes, which would be a real regression worth tracing through git history.

6. git_tools — apps/web/src/components/RightPanelSheet.tsx

7. git_tools — RightPanelSheet

💭 I'm considering whether mounting the dialog already in the open state properly triggers Base UI's starting-style transition, since the popup might need to render as closed first before flipping to open for the animation to actually play. Now I want to check how the pull request dialog handles reopening after a close — specifically whether opening it again correctly resets open to true, so I need to look at the relevant code sections in ChatView.

8. browse_code — apps/web/src/components/ChatView.tsx

9. browse_code — apps/web/src/components/ChatView.tsx

💭 I'm checking the CommandPalette's dialogContentMounted gating logic to make sure content only unmounts after the transition completes and that reopening doesn't leave stale state. Seems fine so far, but I want to look at the CommandPalette code around lines 390-530 to verify.

10. browse_code — apps/web/src/components/CommandPalette.tsx

💭 I'm noticing that toggleMode can actually close the palette if the same shortcut is pressed while it's already open in that mode, and in that case setDialogContentMounted(true) still gets called even though we're closing — this is harmless though, since onOpenChangeComplete will reset it afterward anyway. Rapid toggling — closing then reopening before the transition completes — also seems fine, since Base UI cancels the pending close and fires onOpenChangeComplete with the final open state. Now I want to check whether CommandDialog actually forwards the onOpenChangeComplete prop.

11. git_tools — onOpenChangeComplete

12. git_tools — CommandDialog|initialFocus

💭 Since CommandDialog uses Dialog.Root, onOpenChangeComplete should be a native prop there too. I need to check whether AlertDialog's Root passes through onOpenChangeComplete similarly, and confirm DialogPopup accepts initialFocus since it spreads props to the Base UI Popup. Then I want to revisit the earlier comment about SshPasswordPrompt. Dialog's initialFocus change — previously they used focus plus select, but now with initialFocus the input just gets focused without selecting text, which probably doesn't matter since the field starts empty. Let me check that file to confirm.

13. browse_code — apps/web/src/components/desktop/SshPasswordPromptDialog.tsx

💭 The initialFocus ref approach seems fine since the input usually isn't disabled at mount. Now I'm moving on to check GitActionsControl and ConnectionsSettings, looking at whether the handleOpenChange dependency change to just onOpenChange might trigger an eslint exhaustive-deps warning. I also want to verify that the removed dialog reset calls after successful commits still properly close the dialog through onOpenChangeComplete, so the state actually resets as intended.

14. browse_code — apps/web/src/components/GitActionsControl.tsx

15. browse_code — apps/web/src/components/GitActionsControl.tsx

💭 I'm noting that openDialogForMenuItem resets excludedFiles and isEditingFiles on open rather than dialogCommitMessage, which is fine since that field now clears on close-complete instead. The key behavior is that after a successful commit, the dialog stays visible with its text intact during the close animation, which is exactly the intended fix — now I'm checking handleOpenChange in PublishR to confirm it follows the same pattern. Now I want to look closer at the WSL dialog changes in ConnectionsSettings around where onOpenChange and onOpenChangeComplete were added, since the diff hunk seemed to cut off mid-block with a pendingWslChange update — I need to view more context to see how it fits with the surrounding code.

16. browse_code — apps/web/src/components/settings/ConnectionsSettings.tsx

💭 I'm checking whether spreading a possibly-null pendingWslChange object in that state update would cause a TypeScript error, since TS generally permits spreading null/undefined in object literals without complaint. But the resulting shape would be missing the kind field, which could conflict with the PendingWslChange type. I want to look at those lines directly to confirm what guards exist.

17. browse_code — apps/web/src/components/settings/ConnectionsSettings.tsx

💭 Now I'm checking the ProviderSettingsPanel change where AddProviderInstanceDialog is always rendered instead of conditionally mounted, meaning its hooks and queries now run continuously rather than only when opened. I want to verify whether this dialog does expensive work while closed or initializes state from props on mount, since previously it remounted each time but now its state persists.

18. browse_code — apps/web/src/components/settings/AddProviderInstanceDialog.tsx

💭 Looking at the ProviderSettingsPanel change, I notice the dialog and hooks are now always rendered rather than conditionally mounted. Since this component seems to be instantiated per environment in a map, always mounting it could mean a subscription cost multiplied across every environment instead of just the one being edited. Turning to ExpandedImageDialog, it reimplements the dialog using raw primitives rather than the shared DialogPopup styling, but that's reasonable since it needs a lightbox-style backdrop rather than the card chrome the shared component provides — so I wouldn't flag this as inconsistent. The import path for Dialog matches the pattern used elsewhere, so that's consistent too. I also check the accessibility setup: the sr-only Dialog.Title supplies the accessible name, and portaling to body versus rendering inline shouldn't cause stacking issues. The switch from a src-index key to a generation counter is meant to fix remounting behavior when the user clicks through images while the dialog is already open, so I want to trace whether that change actually resets the image offset correctly. Now I'm considering a real regression risk: removing keepMounted from RightPanelSheet means terminal sessions and preview iframes will actually unmount when the sheet closes rather than staying hidden in the DOM, which could kill running terminals or reload previews instead of just hiding them. Actually keepMounted might've kept content mounted regardless of open state, which would've been wasteful now that the sheet mounts per-thread even when closed. So dropping keepMounted is the correct call — closed sheets simply unmount their children, matching prior behavior without wasting resources on every thread. Now I want to double check the ExpandedImageDialog wiring and see if any other code still calls setExpandedImage directly that could conflict with the new expandedImageDialog state.

19. git_tools — expandedImage

💭 The expandedImage variable is only used once, so that's fine for lint purposes. I should check whether there are existing tests referencing ExpandedImageDialog, since changing primitive composition behavior might require focused test coverage there.

20. git_tools — ExpandedImageDialog|onOpenChangeComplete

💭 I'm considering an edge case: onOpenChangeComplete only fires with an actual transition/mount, so if the popup never mounted before closing, callbacks like onRemove in SshPasswordPromptDialog might not trigger reliably—though this seems unlikely in practice. Checking whether abrupt unmounts (like clearing the expanded image dialog on thread change) still fire the completion callback properly, and they seem fine even without transitions or reduced motion. Now I'm second-guessing my earlier claim about Base UI's default initial focus behavior in the image dialog—I said it focuses the first tabbable element, but I need to verify whether Base UI actually defaults to focusing the popup element itself instead. I'm reasoning through whether the aria-hidden overlay button with tabIndex=-1 would trip any accessibility lint rule, and concluding it's fine since tabindex="-1" elements aren't treated as focusable for that rule's purposes. Now I want to systematically check the main visual/interaction risk areas: the dialog popup's focus outline styling, and backdrop click-to-close behavior. Since the popup covers the full viewport, "outside press" never fires, so the overlay button remains necessary for closing—consistent with prior behavior. But I'm now worried about Escape key handling: previously stopPropagation blocked other Escape listeners, while Base UI's own escape handling might not stop propagation, potentially triggering other global Escape handlers in ChatView like closing panels or stopping generation, so I want to check for other Escape listeners there.

21. git_tools — key === "Escape"

22. git_tools — Escape

💭 I don't see a global Escape handler in ChatView, so I'm wondering if the composer's stop-generation binding could be interfering while the image dialog is open. I should check the keybinding defaults to see if "chat.stop" is mapped to Escape.

23. browse_code — In apps/web, is there a global window keydown handler that reacts to the Escape key (e.g. keybindings like stop generation, close right panel, close terminal) which would fire while a Base UI Dialog is open? Show the handler code and whether it checks for open dialogs/modals.

💭 I'm tracing how the Escape key handlers interact between the chat thread selection and the image dialog — the dialog's window listener previously called preventDefault, which the chat handler checks via event.defaultPrevented to decide whether to skip clearing the thread selection, so both handlers could still fire in registration order since stopPropagation alone wouldn't block sibling listeners on the same target. I'm now checking that the base-ui import path matches the rest of the repo's convention, then moving on to verify PullRequestThreadDialog's forwarding and how ChatView handles its close-then-unmount lifecycle around the pullRequestDialogState and key prop.

24. browse_code — apps/web/src/components/ChatView.tsx

💭 I'm checking whether reopening the dialog quickly after closing would cause issues with the remount key, and whether the success path of the pull request dialog properly triggers onOpenChange to close it after completion. Now let me look at the commit dialog section in GitActionsControl around lines 1830-1850 and 1990-2030.

25. browse_code — apps/web/src/components/GitActionsControl.tsx

26. browse_code — apps/web/src/components/GitActionsControl.tsx

💭 I'm checking that pendingDefaultBranchActionCopy is memoized off pendingDefaultBranchAction, so it correctly persists the copy until it's cleared to null. Now I'm looking at whether GitActionsControl's runGitActionWithToast might overwrite pendingDefaultBranchAction when a new PR action starts, which could cause a conflict. I'm tracing whether onOpenChangeComplete(false) could incorrectly null out a freshly reopened action if it's still firing from a stale close transition while a new open:true state was just set — since the reopen happens quickly, the completion callback should fire with true instead, but I want to make sure there's no race where the stale close-complete callback lands after the new one and wrongly clears the state. Now I want to check the ConnectionsSettings AuthorizedClientsHeaderAction change around lines 990-1050, where the diff removed a reset call from a handler near line 998-999 — I need to confirm that reset still happens appropriately elsewhere.

27. browse_code — apps/web/src/components/settings/ConnectionsSettings.tsx

💭 I'm digging into how keepMounted interacts with SheetPortal—previously RightPanelSheet only rendered while open, so adding keepMounted changes whether the portal content stays mounted even when closed, which could be a real regression source.

28. git_tools — apps/web/src/components/RightPanelSheet.tsx

29. git_tools — keepMounted

💭 I'm checking whether keepMounted was actually meaningful before this change—since the sheet was always open, keepMounted had no visible effect on rendering, so its removal now that RightPanelSheet is gone seems safe, though I want to double-check there isn't a hidden edge case. Actually there is a real difference: previously the component unmounted instantly with no exit animation on close, whereas now it animates out properly, which is an improvement. I don't think this is a regression worth flagging, so I'm moving on to scan for other potential issues, starting with the ExpandedImageDialog and whether its fixed positioning and button styling introduced any changes. I'm checking the z-index stacking between the Backdrop and Popup layers — both use z-50, but since the Popup renders after the Backdrop in the DOM, it still paints on top correctly. I confirm Dialog.Viewport is a legitimate Base UI export, and that the memoized ExpandedImageDialog receiving inline arrow functions as props doesn't actually benefit from memoization, though that's not a meaningful finding. I'm also tracing the updated useEffect dependencies — swapping in open and preview instead of the old array seems safe since preview is stable state, and gating the arrow-key handler on open correctly disables navigation during the closing transition. The setImageOffset(0) reset on close seems redundant since ChatView already nulls out the component on close-complete anyway. Now I'm looking at CommandPalette's mounting logic — verifying that dialogContentMounted gets set true in the same batch as setOpen(true), tracing through the reducer paths for opening the palette, and checking that onOpenChange and toggleMode handle open/close transitions correctly, including the edge case where toggling closes the palette without Base UI firing onOpenChange. Now I'm checking ProviderSettingsPanel — noting that AddProviderInstanceDialog stays mounted with an always-active settings subscription, and that if there are multiple environments each renders its own dialog and subscription, which seems acceptable but worth flagging. I'm also looking at SshPasswordPromptDialog's initialFocus ref usage to confirm it correctly targets an HTML input element. The rAF-based mount effect that triggers the open transition looks fine, as does the cancel/respond logic in the SSH dialog — both paths close correctly without duplication issues. In GitActionsControl, handleOpenChange now just forwards to onOpenChange, which is redundant but not a real bug, and I'm moving on to check the dialog-opening logic there next. That intersection type distributes fine over the union, so narrowing on kind still works correctly. I'm checking the setPendingWslChange calls with the spread pattern where pendingWslChange is already guarded as non-null, and TypeScript handles the narrowing properly there too since the earlier guard excludes the "enable" kind before reaching those lines. No obvious functional bug turns up so far. I'm now looking at whether ExpandedImageDialog's sr-only title without a description would trigger any Base UI warning, and considering whether the Popup's fixed positioning inside an already-fixed Viewport is redundant—using absolute positioning there would likely be more correct. I do notice something worth flagging on consistency grounds: ExpandedImageDialog implements its own backdrop/viewport/popup transition classes rather than reusing the shared dialog styles contract from the repo's dialog-styles module. Let me check that file to compare.

30. browse_code — apps/web/src/components/ui/dialog-styles.ts

💭 I notice the shared dialog-backdrop CSS class handles background blur and nested-dialog stacking via a variable, while the ExpandedImageDialog intentionally uses its own darker backdrop for the lightbox look—that's fine since it doesn't need nesting support. I'm now wondering if that shared class affects anything else I should account for.

31. git_tools — dialog-backdrop

💭 I'm noticing a discrepancy: the file content I got from viewing the PR head tree differs from what git_grep returned for what should be the same ref. It seems git_grep may default to a different commit than the one I explicitly fetched, so I need to figure out which revision each tool is actually using. The real question is whether ExpandedImageDialog's transition lacks the motion-reduce: handling present in the shared DIALOG_POPUP_CLASS - that would be a legitimate inconsistency worth flagging. I should verify by checking the file at both refs explicitly before drawing conclusions.

32. browse_code — apps/web/src/components/ui/dialog-styles.ts

33. git_tools — 18085c3:apps/web/src/components/ui/dialog-styles.ts

💭 I'm noticing a discrepancy: the actual PR head shows the plain ease-in-out transition without motion-reduce, while the git_grep result oddly showed a different version with motion-reduce and custom easing variables. Since I can't explain why grep would return content from a different revision, I'll treat the git_show result at the PR head as the authoritative source. The lightbox intentionally mirrors the shared dialog popup's transition timing values, just applied inline rather than importing the shared class constants, and it deliberately uses different backdrop styling without the glass/rounded/border treatment - that all seems like a reasonable, intentional design choice. I've now covered the main changes, so I'm checking for lingering edge cases like scroll locking with nested dialogs, but the image lightbox is top-level so that's not a concern. I want to double check one more edge case around forceRender and conditional rendering when preview exists but the item index might be out of range. If preview.images is ever empty, the index modulo would resolve to 0 but item would be undefined, meaning the dialog opens with nothing rendered inside — no popup mounted means Escape might not work since Base UI's escape listener attaches to the popup itself, leaving the dialog stuck open with no visible content or close mechanism. It would still be recoverable since clicking another image bumps the generation counter and forces a remount, but I want to verify whether images can actually be empty by checking how openExpandedImage builds the preview object from message images.

34. browse_code — apps/web/src/components/ChatView.tsx

💭 Actually, I realize there's a real issue worth checking: since the dialog key changes with each image, clicking a different thumbnail while the lightbox is already open causes the whole dialog to remount rather than just swap the image. Before, there was no transition so this remount was invisible, but now with a fade/scale transition added, remounting would replay the entrance animation every time someone switches images mid-preview, which would look jarring. But wait — the lightbox is a full-screen modal, so there's no way to click another thumbnail while it's open, meaning the only path to a remount is closing and reopening, which is a much smaller edge case. I'm satisfied there's nothing more substantial here, so I want to shift focus and check the PullRequestDialogState.open addition in ChatView.logic.ts against any existing tests that construct that state.

35. git_tools — PullRequestDialogState

💭 The dialog resets state on thread change since it's keyed and remounts on each open, which seems fine. Let me verify the props wiring by checking how the Dialog root receives both handlers in the PullRequestThreadDialog component.

36. browse_code — apps/web/src/components/PullRequestThreadDialog.tsx

💭 I should check PullRequestThreadDialog for a similar focus effect on referenceInputRef that might run before the popup exists, like the SSH issue. Let me grep for referenceInputRef usage.

37. git_tools — referenceInputRef

💭 The rAF timing is fine since the popup is already mounted by then. Let me think about whether there's a real finding tied to UI consistency and shared primitives—specifically that ExpandedImageDialog now hard-codes its backdrop, viewport, and popup composition and transition classes inline, duplicating the shared dialog contract instead of reusing it. Trying the shared Dialog/DialogPortal/DialogBackdrop/DialogViewport wrappers with the lightbox just overriding background and geometry via className runs into a problem: the custom dialog-backdrop utility applies a color-mix and blur background, and since it's a custom utility not recognized by tailwind-merge, overriding it with something like bg-black/75 leaves both classes applied with unclear precedence. So the author's choice to use raw primitives with their own classes is actually more robust here, not a shortcut. The repo's usual pattern always composes Portal, Backdrop, Viewport, and Popup together inside DialogPopup, but using raw parts for this lightbox looks like a deliberate, defensible exception rather than an inconsistency worth flagging. Since findings default to failure and "All clear" only applies when nothing is wrong, I want to do one more careful pass on the likely trouble spots, especially visual ones. Checking whether SheetPopup dropping keepMounted matters, I find that prop is already dead/unused elsewhere in the repo and untouched by this PR, so it's not a real finding. I also verify that rendering Sheet with open=false and no keepMounted produces no output and no scroll lock, and that mount/unmount behavior for the right panel content matches the previous implementation, so nothing looks broken there either. Now I'm tracing where panelToggleControls actually renders when the sheet is closed in sheet mode, since panelLayoutControls is gated to only render when the sheet isn't in use — I need to check if the header renders it separately.

38. git_tools — panelToggleControls

💭 I'm trying to locate where the toggle controls render when the panel is closed in sheet mode, since the current reference only covers the inline+open case. I should check whether WorkspacePageHeader is passed these controls through an "actions" prop.

39. git_tools — panelLayoutControls

💭 I'm tracing a timing issue: when the sheet closes, the header immediately swaps back to showing its own layout controls while the sheet is still animating out and briefly still rendering its own toggle controls, causing a potential flash of duplicate controls during the transition. Actually on open the same duplication already existed before this PR, so the close-direction behavior is just symmetric with existing open behavior rather than a new regression. I also check the data-workspace-titlebar-controls attribute — only one instance carries it during the transition, so drag-region measurement logic should be unaffected. I'm satisfied this area is clear, but let me still check ConfirmDialogHost and RelayClientInstallDialog for their onOpen handling before concluding. Turning to CommandPalette gating, dialogContentMounted starts false and the popup only mounts once open is set true, so a normal state update triggers the mount and Base UI's enter transition correctly. If there's a CommandDialogTrigger elsewhere in the tree, clicking it should route through Root's onOpenChange into our handler, which properly sets state and mounts the popup — so that path looks handled too. Now I want to check whether any imperative handle can open the command palette without going through onOpenChange, so I'm searching for other usages of a command palette handle.

40. git_tools — CommandCreateHandle|CommandDialogTrigger|data-command-palette

💭 I realize the timing changed: since the dialog content now stays mounted during the close animation, isCommandPaletteOpen() incorrectly reports true for about 200ms after the palette actually closes, because the DOM attribute it checks persists through that transition. Actually, checking further, the exit-transition behavior was already the same before — Base UI unmounts the popup identically in both versions, so that's not a real change. But there's a more meaningful question: previously CommandPaletteDialog was always mounted, so its data-fetching hooks for projects/files/threads ran continuously in the background; now they only run once the palette is opened, which could mean the first open is slower since nothing's prefetched. I want to check what CommandPaletteDialog actually does at the top of its body to confirm this.

41. browse_code — apps/web/src/components/CommandPalette.tsx

💭 I'm working out that Base UI's Dialog.Portal likely doesn't render its children at all when the dialog is closed, so the OpenCommandPaletteDialog hooks were never actually mounted while the palette was closed, even though the component tree structurally included them. So dialogContentMounted probably isn't about preventing the hooks from running — it's more likely there to keep the content stable during the close transition, so the mode doesn't reset to "command" and flash the wrong view while the popup is animating out. I should check whether SetOpen(false) actually resets mode in the reducer to confirm this theory.

42. git_tools — function reduceCommandPaletteUiState

💭 Setting open to false while preserving the mode field means the mode stays stable through the exit transition, so content doesn't switch mid-animation. I'm trying to figure out why mounting needs to be gated at all — maybe it's because the dialog child component needs to exist before the enter transition can trigger properly. I'm now walking through edge cases: what happens if the dialog is closed before ever mounting, or if onOpenChangeComplete never fires because the popup wasn't actually rendered when closed. This seems like it could be a real bug scenario if dialogContentMounted stays false but open transitions to true in a way that doesn't trigger completion callbacks correctly. I'm also considering prefers-reduced-motion — transitions might be instant but the completion callback should still fire regardless. Concluding this line of investigation doesn't reveal a bug, I want to check one remaining file — the ProviderSettingsPanel dialog that stays always mounted — to see whether EnvironmentProviderSettings gets rendered redundantly in a list.

43. browse_code — apps/web/src/components/settings/ProviderSettingsPanel.tsx

💭 Having only one dialog per environment panel seems fine, though its validation logic now runs on every settings render even while closed, which is trivial overhead. The bigger question is whether keeping the dialog permanently mounted changes behavior — previously it remounted fresh each time guaranteeing state reset, so I need to check whether reset still happens reliably now. Checking PublishRepositoryDialog's resetState, it covers six setters but I'm noting it doesn't reset every piece of state (like visibility, protocol, provider) — but that matches pre-existing behavior, so it's unchanged. The only difference is timing: reset now fires on close-complete rather than immediately during handleOpenChange, but the net effect is the same. I'm fairly confident both prior findings were already addressed by the author's fixes to the SSH dialog focus and the lightbox overlay button. Let me double check that overlay button fix once more to see if it introduces any new accessibility issue of its own. Since the overlay button is no longer tabbable, I'm verifying Base UI's default initial-focus behavior — it focuses the first focusable element in the popup, which would now be the "Previous image" or close button, both fine visible controls. Focus moving into the dialog on open is standard modal behavior, and I'm considering how close behavior interacts with the trigger not being a proper Dialog.Trigger since it's an image button. Base UI restores focus to whatever element was focused before the dialog opened by default, so that's handled correctly. I checked whether the thumbnail in ChatView is a real button, but that's unrelated to this diff, and there's no dead-code concern worth flagging either. I'm concluding there are no actual findings here. Given the check defaults to failure only when findings exist, I'll produce a success result with a summary of "All clear" rather than a literal plain-text response, resolving the earlier tension about format.