Repository navigation
feat(settings): pinned default download folder and save prompt preferences (#203) - #233
DhruvilPatel603 wants to merge 1 commit into
Conversation
Arpit-Khandelwal
left a comment
Arpit-Khandelwal
left a comment
There was a problem hiding this comment.
Thanks for picking up #203, and for the e2e spec. I merged current main and ran typecheck (node/web/e2e), lint, format:check, the build, e2e/settings.spec.ts and the settings @smoke tests. Your 6 new tests pass, and they fail without the change. An existing user's remembered folder is still used after the upgrade, and the new settings are checked in settings.ts like destinationDir. A few things to fix:
- The settings smoke test breaks.
NetworkEditPopover.tsx:127changes the label back toEdit network, but main's test (#234) now looks forEdit Office fibre, soui.spec.ts:288fails. Please keep main's label. - A missing pinned folder is still used. Main already checks the folder (
handlers.ts:138) and sends the result asdestinationDir. But the store and New download use the uncheckeddefaultDestinationDirinstead. I tested it: with a missing pinned folder, New download shows that folder. On an unplugged drive, Download fails with "…choose another folder". Starting frominitial.destinationDirfixes it. Please add a test like the existing "broken settings file" one. - With the prompt off, you can't change the folder for one download. The whole Save to row is hidden, so the user only sees the error from point 2, with nothing to click. Please keep a way to change the folder in that mode, and always show the row when the folder is missing.
- Existing users see the wrong folder in Settings. If the remembered folder is
~/Movies, Settings shows~/Downloads (system default), but New download uses~/Movies. Please show the folder new downloads will really use.
Scope questions for @anmolkapil: is the "ask each time" switch needed, since New download already shows the folder and waits for a click? And should the status bar gear, ⌘, / Ctrl+, and the LimitsDialog move stay here, or go in a separate PR? If they stay, the dialog should probably be called "Settings" instead of "Speed & data limits".
Heads-up: #227 adds free space and a low-space warning to the same Save to row. Hiding the row hides that warning, and the two PRs will conflict there.
| variant="ghost" | ||
| size="icon-xs" | ||
| aria-label={`Edit ${visual.name}`} | ||
| aria-label="Edit network" |
There was a problem hiding this comment.
This breaks ui.spec.ts:288 on current main (#234 changed the test to Edit Office fibre). Please keep `Edit ${visual.name}`. Screen readers need to know which network the button edits.
| destinationDir: initial.destinationDir ?? initial.downloadsDir, | ||
| defaultDestinationDir: initial.defaultDestinationDir, | ||
| askDestinationEachTime: initial.askDestinationEachTime ?? true, | ||
| destinationDir: initial.defaultDestinationDir ?? initial.destinationDir ?? initial.downloadsDir, |
There was a problem hiding this comment.
Main already checks the pinned folder and sends the result as initial.destinationDir (handlers.ts:138). Reading initial.defaultDestinationDir here skips that check, so a deleted or unplugged folder still gets used. initial.destinationDir ?? initial.downloadsDir would be enough.
| const askDestinationEachTime = useAppStore((store) => store.askDestinationEachTime) | ||
| const storeDestinationDir = useAppStore((store) => store.destinationDir) | ||
| const [destinationDir, setDestinationDir] = useState( | ||
| () => defaultDestinationDir ?? storeDestinationDir |
There was a problem hiding this comment.
Same as in the store: please start from the folder main checked (storeDestinationDir), not the raw pinned path.
| <div className={labelClass}>Save to</div> | ||
| <div className="min-w-0 flex-1 truncate font-mono text-[12.5px]"> | ||
| {toDisplayPath(destinationDir, homeDir)} | ||
| {askDestinationEachTime && ( |
There was a problem hiding this comment.
With the row hidden, the user can't change the folder for this one download. If the pinned folder is missing, they get an error telling them to choose another folder, with no control to do it. Please keep some way to change the folder, and always show the row when the folder is missing.
| {toDisplayPath(draft.defaultDestinationDir ?? downloadsDir, homeDir)} | ||
| {draft.defaultDestinationDir === undefined && ( | ||
| <span className="ml-2 font-sans text-[11px] text-muted-foreground"> | ||
| (system default) |
There was a problem hiding this comment.
For an existing user whose remembered folder is not Downloads, this shows ~/Downloads (system default), but New download uses the remembered folder. Please show the folder new downloads will really use.
|
Thanks for the thorough review and feedback @Arpit-Khandelwal! I’ve addressed all the review points and resolved the CI smoke test issues. Here is a breakdown of the updates: 1. Restored Main's Network Label (
|
Arpit-Khandelwal
left a comment
Arpit-Khandelwal
left a comment
There was a problem hiding this comment.
Thanks for the quick update. I re-checked 099ebe3 and all 4 points from my last review are fixed. I also tried them by hand: with a deleted pinned folder, New download shows ~/Downloads and Change works, and a remembered ~/Movies shows up correctly in Settings. Typecheck, lint, format:check, the build and the related e2e specs pass, except one test:
settings.spec.ts:240fails every time. After the relaunch the app is already on the downloads list, sogetByRole('button', { name: 'Downloads' })matches the "All downloads" filter button and opens its menu. Then New download can't be clicked. Removing that line makes all 8 tests pass. CI only runs the smoke tests, so it didn't catch this.- A pinned folder can only be unpinned with "Reset to defaults". That also resets the speed limits, slow mode and schedule. Your first version had a "Reset to default" button for the folder. Please bring back a small one, shown only when a folder is pinned.
Please also update the PR description. It still talks about the "ask each time" switch and the hidden Save to row.
@anmolkapil: the "ask each time" switch from #203 is gone now. New download already shows the folder and waits for a click, so I think that's fine, but it's your call. The status bar gear and ⌘, / Ctrl+, are still here, and the folder setting now lives in "Speed & data limits".
#227 and #226 are merged now, so please rebase on main. There is one small conflict in the StatusBar.tsx imports (useFreeSpace vs the Settings icon). I tried it with #227, and with that fixed everything builds and the tests pass, apart from the one above.
| expect(newDefaultFiles.length).toBe(0) | ||
|
|
||
| // And verify subsequent new download defaults to newDefaultDir | ||
| await plexo.page.getByRole('button', { name: 'Downloads' }).click() |
There was a problem hiding this comment.
After relaunch() the app is already on the list, so this matches the "All downloads" filter button (not exact) and opens its menu. The next newDownload() then times out because the menu covers the page. Removing this line makes the test pass.
| if (chosen) change({ defaultDestinationDir: chosen }) | ||
| }} | ||
| > | ||
| Change |
There was a problem hiding this comment.
Once a folder is pinned, the only way to unpin it is "Reset to defaults". Could you add a small button next to Change, shown only when draft.defaultDestinationDir is set, that does change({ defaultDestinationDir: undefined })? Your first version had one. Also, when the pinned folder is missing, please say so here, because New download falls back to ~/Downloads.
- Add pinned default download folder in Settings under General - Show missing folder warning when pinned drive/folder is unavailable - Provide Reset to default button to unpin default folder - Default new downloads to pinned folder while allowing one-off overrides - Fall back safely to system Downloads when pinned directory is missing - Preserve remembered directory across restarts when unpinned - Add full E2E test coverage in e2e/settings.spec.ts
099ebe3 to
74bd727
Compare
|
Thanks for the review @Arpit-Khandelwal! I've addressed all the feedback and rebased the branch on latest 1. Fixed
|
Closes #203
Problem
Previously, Plexo remembered whichever folder was last picked for a download, which meant one-off folder changes permanently overwrote the user's preferred download destination. Additionally, users had no way to configure a permanent pinned default download location.
Solution & Changes
Pinned Default Download Folder:
defaultDestinationDirtoAppSettingsandInitialState.Global Settings Shortcut & Status Bar Button:
⌘,on macOS /Ctrl+,on Windows/Linux).E2E Test Suite:
e2e/settings.spec.tscovering pinned persistence, one-off overrides, missing folder fallbacks, unpinning with Reset to default, legacy folder upgrades, unpinned persistence, and resume destination preservation.