Repository navigation
fix: use the standard folder picker on windows - #814
Conversation
 [Medium impact] The change is limited to Windows folder picking in the dashboard. The PR appears safe to merge, with a non-blocking request for tests around the new owner window. Findings
Fix with agent prompt### Issue 1
src/folder_picker.rs:59-64
The rewrite removes all four Windows picker tests without replacing coverage for the new owner window. Future changes could break its window flags or cleanup without a test catching it.
Consider extracting owner creation and adding a Windows-only test that checks visibility and topmost flags, then confirms that dropping `Owner` destroys the window. This would not need to open an interactive dialog.
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!
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryReplaces the Windows PowerShell folder picker with
Reviews (1) · Last reviewed commit: "fix: use the standard folder picker on w..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| impl Drop for Owner { | ||
| fn drop(&mut self) { | ||
| // SAFETY: this window was created on this thread and is no longer borrowed. | ||
| unsafe { | ||
| DestroyWindow(self.0.get() as _); | ||
| } |
There was a problem hiding this comment.
The rewrite removes all four Windows picker tests without replacing coverage for the new owner window. Future changes could break its window flags or cleanup without a test catching it.
Consider extracting owner creation and adding a Windows-only test that checks visibility and topmost flags, then confirms that dropping Owner destroys the window. This would not need to open an interactive dialog.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/folder_picker.rs
Line: 59-64
Comment:
**Owner window tests are missing**
The rewrite removes all four Windows picker tests without replacing coverage for the new owner window. Future changes could break its window flags or cleanup without a test catching it.
Consider extracting owner creation and adding a Windows-only test that checks visibility and topmost flags, then confirms that dropping `Owner` destroys the window. This would not need to open an interactive dialog.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.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!
dude do not use powershell for this shit
supercedes #336
i think this also supercedes #356
Assisted-by: gpt-6.1-terra