fix(onboarding): make folder picker dialog reliably open on top#2484
Open
posthog[bot] wants to merge 2 commits into
Open
fix(onboarding): make folder picker dialog reliably open on top#2484posthog[bot] wants to merge 2 commits into
posthog[bot] wants to merge 2 commits into
Conversation
Users on the onboarding "pick a repo" step could click "Select folder..." and have nothing visible happen, leaving them unable to progress. The picker chain (FolderPicker → os.selectDirectory → ElectronDialog.pickFile) parented the native dialog to `BrowserWindow.getFocusedWindow()`, which returns null whenever the app has no focused window — common during onboarding flows that hand focus to external windows (OS auth, the GitHub device-login browser tab). With no parent, Electron opens the dialog unparented, where it can render behind the app window or off-screen, indistinguishable from an unresponsive button. Resolve the dialog parent to a visible window (falling back to any window) and focus it before showing, so the picker is always parented to and rendered on top of a real window. Apply the same hardening to confirm() dialogs. Add unit coverage for the focused / focus-lost / no-windows cases. Reported via PostHog inbox report 019e0cc2-3335-792a-a4a7-3b9003f217b8. Generated-By: PostHog Code Task-Id: d0697d03-6f9f-479d-a02b-a348b29f7579
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/code/src/main/platform-adapters/electron-dialog.test.ts:32-82
**Tests only cover `pickFile`, not `confirm()`**
The PR description explicitly calls out applying the same `resolveDialogParent()` fix to `confirm()`, but the test file has no coverage for that path. If the wiring in `confirm()` is later accidentally removed or regressed, nothing would catch it. A minimal test mirroring the focused-window case for `confirm()` (checking that `showMessageBox` receives the parent window) would close this gap.
### Issue 2 of 2
apps/code/src/main/platform-adapters/electron-dialog.test.ts:41-81
**Prefer parameterised tests**
The three cases here test the same function entry point (`pickFile`) with varying window configurations and differing `showOpenDialog`/`focus` assertions — a good fit for `it.each`. The team's convention prefers parameterised tests for exactly this shape of variation.
Reviews (1): Last reviewed commit: "fix(onboarding): make folder picker dial..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+32
to
+82
| describe("ElectronDialog.pickFile", () => { | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| mockShowOpenDialog.mockResolvedValue({ | ||
| canceled: false, | ||
| filePaths: ["/repo"], | ||
| }); | ||
| }); | ||
|
|
||
| it("parents the picker to the focused window when one exists", async () => { | ||
| const focused = makeWindow(); | ||
| mockGetFocusedWindow.mockReturnValue(focused); | ||
|
|
||
| const result = await new ElectronDialog().pickFile({ directories: true }); | ||
|
|
||
| expect(result).toEqual(["/repo"]); | ||
| expect(mockShowOpenDialog).toHaveBeenCalledWith( | ||
| focused, | ||
| expect.any(Object), | ||
| ); | ||
| // A focused window is already on top, so we never reach the fallback windows. | ||
| expect(mockGetAllWindows).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("falls back to a visible window and focuses it when nothing is focused", async () => { | ||
| mockGetFocusedWindow.mockReturnValue(null); | ||
| const hidden = makeWindow({ visible: false }); | ||
| const visible = makeWindow({ visible: true }); | ||
| mockGetAllWindows.mockReturnValue([hidden, visible]); | ||
|
|
||
| await new ElectronDialog().pickFile({ directories: true }); | ||
|
|
||
| expect(visible.focus).toHaveBeenCalledTimes(1); | ||
| // The dialog must be parented to the visible window, not opened unparented | ||
| // (which is what previously left it stuck behind/off-screen). | ||
| expect(mockShowOpenDialog).toHaveBeenCalledWith( | ||
| visible, | ||
| expect.any(Object), | ||
| ); | ||
| }); | ||
|
|
||
| it("opens unparented only when there are no windows at all", async () => { | ||
| mockGetFocusedWindow.mockReturnValue(null); | ||
| mockGetAllWindows.mockReturnValue([]); | ||
|
|
||
| await new ElectronDialog().pickFile({ directories: true }); | ||
|
|
||
| expect(mockShowOpenDialog).toHaveBeenCalledWith(expect.any(Object)); | ||
| expect(mockShowOpenDialog.mock.calls[0]).toHaveLength(1); | ||
| }); | ||
| }); |
Contributor
There was a problem hiding this comment.
pickFile, not confirm()
The PR description explicitly calls out applying the same resolveDialogParent() fix to confirm(), but the test file has no coverage for that path. If the wiring in confirm() is later accidentally removed or regressed, nothing would catch it. A minimal test mirroring the focused-window case for confirm() (checking that showMessageBox receives the parent window) would close this gap.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/main/platform-adapters/electron-dialog.test.ts
Line: 32-82
Comment:
**Tests only cover `pickFile`, not `confirm()`**
The PR description explicitly calls out applying the same `resolveDialogParent()` fix to `confirm()`, but the test file has no coverage for that path. If the wiring in `confirm()` is later accidentally removed or regressed, nothing would catch it. A minimal test mirroring the focused-window case for `confirm()` (checking that `showMessageBox` receives the parent window) would close this gap.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
+41
to
+81
| it("parents the picker to the focused window when one exists", async () => { | ||
| const focused = makeWindow(); | ||
| mockGetFocusedWindow.mockReturnValue(focused); | ||
|
|
||
| const result = await new ElectronDialog().pickFile({ directories: true }); | ||
|
|
||
| expect(result).toEqual(["/repo"]); | ||
| expect(mockShowOpenDialog).toHaveBeenCalledWith( | ||
| focused, | ||
| expect.any(Object), | ||
| ); | ||
| // A focused window is already on top, so we never reach the fallback windows. | ||
| expect(mockGetAllWindows).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it("falls back to a visible window and focuses it when nothing is focused", async () => { | ||
| mockGetFocusedWindow.mockReturnValue(null); | ||
| const hidden = makeWindow({ visible: false }); | ||
| const visible = makeWindow({ visible: true }); | ||
| mockGetAllWindows.mockReturnValue([hidden, visible]); | ||
|
|
||
| await new ElectronDialog().pickFile({ directories: true }); | ||
|
|
||
| expect(visible.focus).toHaveBeenCalledTimes(1); | ||
| // The dialog must be parented to the visible window, not opened unparented | ||
| // (which is what previously left it stuck behind/off-screen). | ||
| expect(mockShowOpenDialog).toHaveBeenCalledWith( | ||
| visible, | ||
| expect.any(Object), | ||
| ); | ||
| }); | ||
|
|
||
| it("opens unparented only when there are no windows at all", async () => { | ||
| mockGetFocusedWindow.mockReturnValue(null); | ||
| mockGetAllWindows.mockReturnValue([]); | ||
|
|
||
| await new ElectronDialog().pickFile({ directories: true }); | ||
|
|
||
| expect(mockShowOpenDialog).toHaveBeenCalledWith(expect.any(Object)); | ||
| expect(mockShowOpenDialog.mock.calls[0]).toHaveLength(1); | ||
| }); |
Contributor
There was a problem hiding this comment.
The three cases here test the same function entry point (pickFile) with varying window configurations and differing showOpenDialog/focus assertions — a good fit for it.each. The team's convention prefers parameterised tests for exactly this shape of variation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/main/platform-adapters/electron-dialog.test.ts
Line: 41-81
Comment:
**Prefer parameterised tests**
The three cases here test the same function entry point (`pickFile`) with varying window configurations and differing `showOpenDialog`/`focus` assertions — a good fit for `it.each`. The team's convention prefers parameterised tests for exactly this shape of variation.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Add coverage for the resolveDialogParent() wiring in confirm(), mirroring the focused-window and focus-lost cases already covered for pickFile, so a regression in the confirm() path is caught. Addresses Greptile review feedback. Generated-By: PostHog Code Task-Id: d0697d03-6f9f-479d-a02b-a348b29f7579
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
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
Acts on PostHog inbox report
019e0cc2-3335-792a-a4a7-3b9003f217b8(P1, immediately actionable).A session replay showed a user on the onboarding "pick a repo" step repeatedly clicking Select folder... with no response, then giving up. For first-time users (no recent folders)
FolderPickerrenders a plain button whoseonClickdoes fire — so the click is not swallowed. The dead end is downstream, in how the native dialog is parented.Root cause
The picker chain is:
FolderPicker.handleOpenFilePicker→trpcClient.os.selectDirectory→ElectronDialog.pickFile(apps/code/src/main/platform-adapters/electron-dialog.ts)pickFileparented the dialog toBrowserWindow.getFocusedWindow(). That returnsnullwhenever the app has no focused window — which is common during onboarding flows that hand focus to external windows (OS auth prompts, the GitHub device-login browser tab). With anullparent, Electron opens the dialog unparented, where it can appear behind the app window or off-screen — indistinguishable from an unresponsive button.Fix
resolveDialogParent(): prefer the focused window, otherwise fall back to a visible window (then any window) andfocus()it before showing the dialog, so the picker is always parented to and rendered on top of a real window.confirm()dialogs.Testing
vitest run src/main/platform-adapters/electron-dialog.test.ts— 3 passing@posthog/platformbuilt locally)🤖 Generated with Claude Code