feat(ui): build the packages view (task 29)#133
Conversation
Replaces the Packages placeholder with a full tree-based management UI: - PackagesView root + PackageToolbar (filter chips + debounced search + New package trigger), PackageTree (empty state + list), PackageRow (chevron toggle, inline rename, source-type badge, file count, total bytes, aggregated progress bar, folder browse, password trigger, auto-extract switch, 1-10 priority select, Pause-all / Start-all / Delete buttons), PackageDownloadRow (HTML5 native draggable child row) and PackageDialogs (Add / Rename / Password / Folder / Delete). - Wires every command from task 27 (`package_create`, `package_update`, `package_set_password`, `package_set_priority`, `package_move_to_folder`, `package_toggle_auto_extract`, `package_delete`, `package_add_download`, `package_remove_download`) and the queries from task 28 (`package_list`, `package_list_downloads`). - Drag-and-drop between packages uses the native HTML5 dataTransfer with `application/x-vortex-download` + `application/x-vortex-source-package` payloads, then chains `package_remove_download` + `package_add_download` with a single user-facing toast. - Bulk Pause-all / Start-all fans out the existing `download_pause` / `download_resume` IPC over `Promise.allSettled` for every member of the expanded package. - Filter + 300 ms debounced search re-key the TanStack Query so the filtering happens server-side in one round-trip; empty filter object is collapsed so the no-filter SQL path is taken. - New `src/types/package.ts`, `src/hooks/usePackagesQuery.ts`, `packageQueries` cache key factory, EN + FR translation namespace with `_one` / `_other` plural variants for file counts. - 16 new Vitest tests cover the six acceptance criteria (expand/collapse, auto-extract toggle, masked password dialog, drag-and-drop FK update, ≥80 % coverage, ≤2-level prop drilling) plus filter chips, debounced search, dialog flows, fan-out bulk actions and the error state. Coverage on the PackagesView folder: 87.28 % statements / 90.07 % lines / 79.59 % functions.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 32361158 | Triggered | Generic Password | 75800ff | src/views/PackagesView/PackagesView.tsx | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
github-actions
Bot
added
documentation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a complete frontend "Packages" feature: TypeScript types, React Query hooks, a modular PackagesView with toolbar/tree/rows/dialogs, drag‑and‑drop transfer logic with rollback, Tauri IPC usage via tauriInvoke, English/French i18n, tests, and a changelog entry documenting the rollout. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Packages View
participant Query as React Query
participant Tauri as Tauri IPC
participant Backend as Backend Service
participant Toast as Toast Notification
User->>UI: Load / Filter / Search packages
UI->>Query: usePackagesQuery(filter)
Query->>Tauri: tauriInvoke('package_list', { sourceType?, nameQ? })
Tauri->>Backend: request package_list
Backend-->>Tauri: PackageView[]
Tauri-->>Query: PackageView[]
Query-->>UI: render package list
User->>UI: Expand package
UI->>Query: usePackageDownloadsQuery(id)
Query->>Tauri: tauriInvoke('package_list_downloads', { id })
Tauri->>Backend: request downloads
Backend-->>Tauri: DownloadView[]
Tauri-->>Query: DownloadView[]
Query-->>UI: render child downloads
User->>UI: Drag download to other package
UI->>Tauri: tauriInvoke('package_remove_download', { id })
Tauri->>Backend: remove download
Backend-->>Tauri: success
UI->>Tauri: tauriInvoke('package_add_download', { toPackageId, downloadId })
Tauri->>Backend: add download
Backend-->>Tauri: success/failure
UI->>Query: invalidate relevant queries
UI->>Toast: show move success/error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/i18n/__tests__/issue30-ui-fr.test.tsx (1)
144-147: ⚡ Quick winFail fast on unexpected IPC commands in this test mock.
At Line 144, returning
undefinedfor unknown commands can mask accidental new IPC usage and make this test pass for the wrong reason. Prefer throwing on unhandled commands.Suggested test hardening
mockInvoke.mockImplementation(async (command: string) => { - if (command === "package_list") return []; - return undefined; + if (command === "package_list") return []; + throw new Error(`Unhandled IPC command in test: ${command}`); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/i18n/__tests__/issue30-ui-fr.test.tsx` around lines 144 - 147, The test mock for mockInvoke (mockInvoke.mockImplementation) currently returns undefined for any command other than "package_list", which can hide accidental IPC calls; change the implementation so it returns [] for "package_list" but throws a descriptive error (e.g., "Unexpected IPC command: <command>") for any other command to fail fast and surface unintended usage in this test.src/views/PackagesView/PackageToolbar.tsx (1)
50-56: ⚡ Quick winGive the search field an accessible name.
Right now the input is identified only by its placeholder, which is not a reliable programmatic label for screen readers. Please add
aria-label={t("packages.search")}or associate it with a visible<label>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/PackagesView/PackageToolbar.tsx` around lines 50 - 56, The search Input lacks a programmatic label for accessibility; update the Input in PackageToolbar (the Input with props value={search}, onChange={(e) => onSearchChange(e.target.value)}, placeholder={t("packages.search")}, data-testid="packages-search") to include an accessible name by adding aria-label={t("packages.search")} or by associating it with a visible <label> (e.g., add an id to the Input and a <label htmlFor={id}> around t("packages.search")). Ensure the label text uses the same t("packages.search") translation so screen readers get the correct accessible name.src/types/package.ts (1)
7-10: ⚡ Quick winTighten
sourceTypetoPackageSourceType.
PackageRowbuilds translation keys frompkg.sourceType, andusePackagesQueryforwardsfilter.sourceTypeto the validatedpackage_listIPC call. Keeping these fields as plainstringremoves the compile-time guard and lets invalid source types slip to runtime as missing labels or rejected queries.Also applies to: 29-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types/package.ts` around lines 7 - 10, The PackageView interface (and any other interfaces in this file that declare sourceType as a string) should use the discriminated type PackageSourceType instead of plain string: replace sourceType: string with sourceType: PackageSourceType in PackageView and the other occurrences around the same block (the ones used by PackageRow / usePackagesQuery), and import or reference the PackageSourceType type where needed so the compiler enforces valid source types when PackageRow builds translation keys and when usePackagesQuery forwards filter.sourceType to the package_list IPC call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/views/PackagesView/PackageDialogs.tsx`:
- Around line 274-285: The form allows submitting the same folder value because
canSubmit only checks non-empty; update the submit gating to also require the
trimmed folder to differ from the original folder captured in initialFolderRef.
Concretely, compare folder.trim() against initialFolderRef.current.trim() (use
the same trimming for both) and include that change-check in the canSubmit
condition alongside !submitting and non-empty; keep references to
initialFolderRef, folder, setFolder and the submit handler that calls
package_move_to_folder so unchanged values are blocked from being submitted.
In `@src/views/PackagesView/PackageDownloadRow.tsx`:
- Around line 16-24: The current PackageDownloadRow uses pointer-only
drag-and-drop (onDragStart) which isn't keyboard-accessible; update the
PackageDownloadRow component so the draggable row is keyboard-focusable (add
tabIndex={0} to the container using data-testid
`package-download-row-${download.id}`) and implement an onKeyDown handler that
triggers the same move behavior as onDragStart (calling onDragStart(download,
packageId) or a new wrapper like handleMoveAction) when Space or Enter is
pressed; also ensure appropriate ARIA attributes (e.g., role="button" or
aria-grabbed/aria-dropeffect depending on your app semantics) are set so screen
readers announce the action.
In `@src/views/PackagesView/PackagesView.tsx`:
- Around line 186-193: handleChangeFolder currently always shows a success toast
after moveFolderMut.mutateAsync even when PackageMoveOutcome.failed contains
items; update the callback to inspect the outcome returned by
moveFolderMut.mutateAsync (the PackageMoveOutcome with moved and failed arrays)
and if failed.length > 0 show an error or warning toast (including counts and
optionally failed ids/names) instead of the unconditional toast.success,
otherwise show the success toast; reference the handleChangeFolder function,
moveFolderMut.mutateAsync call, and the PackageMoveOutcome.failed/moved arrays
when making the change.
- Around line 285-291: The current two-step move (calling
removeFromPackageMut.mutateAsync then addToPackageMut.mutateAsync in
PackagesView) can leave the download lost if the second call fails; modify the
logic to make the move atomic by either (a) using a single backend endpoint
(e.g., implement and call a package_move_download API that performs remove+add
server-side) or (b) adding a compensating rollback on failure (catch errors from
addToPackageMut and call removeFromPackageMut.mutateAsync({ packageId:
toPackageId, downloadId }) or re-add to fromId) so that removeFromPackageMut and
addToPackageMut are not left in an inconsistent state; update error handling to
await and verify the rollback before showing toast and ensure
invalidatePackages() runs only after a successful move or completed rollback.
---
Nitpick comments:
In `@src/i18n/__tests__/issue30-ui-fr.test.tsx`:
- Around line 144-147: The test mock for mockInvoke
(mockInvoke.mockImplementation) currently returns undefined for any command
other than "package_list", which can hide accidental IPC calls; change the
implementation so it returns [] for "package_list" but throws a descriptive
error (e.g., "Unexpected IPC command: <command>") for any other command to fail
fast and surface unintended usage in this test.
In `@src/types/package.ts`:
- Around line 7-10: The PackageView interface (and any other interfaces in this
file that declare sourceType as a string) should use the discriminated type
PackageSourceType instead of plain string: replace sourceType: string with
sourceType: PackageSourceType in PackageView and the other occurrences around
the same block (the ones used by PackageRow / usePackagesQuery), and import or
reference the PackageSourceType type where needed so the compiler enforces valid
source types when PackageRow builds translation keys and when usePackagesQuery
forwards filter.sourceType to the package_list IPC call.
In `@src/views/PackagesView/PackageToolbar.tsx`:
- Around line 50-56: The search Input lacks a programmatic label for
accessibility; update the Input in PackageToolbar (the Input with props
value={search}, onChange={(e) => onSearchChange(e.target.value)},
placeholder={t("packages.search")}, data-testid="packages-search") to include an
accessible name by adding aria-label={t("packages.search")} or by associating it
with a visible <label> (e.g., add an id to the Input and a <label htmlFor={id}>
around t("packages.search")). Ensure the label text uses the same
t("packages.search") translation so screen readers get the correct accessible
name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: df279f55-c80c-46f2-bf7b-9caebe8bdef9
📒 Files selected for processing (16)
CHANGELOG.mdsrc/api/queries.tssrc/hooks/usePackagesQuery.tssrc/i18n/__tests__/issue30-ui-fr.test.tsxsrc/i18n/locales/en.jsonsrc/i18n/locales/fr.jsonsrc/types/package.tssrc/views/PackagesView.tsxsrc/views/PackagesView/PackageDialogs.tsxsrc/views/PackagesView/PackageDownloadRow.tsxsrc/views/PackagesView/PackageRow.tsxsrc/views/PackagesView/PackageToolbar.tsxsrc/views/PackagesView/PackageTree.tsxsrc/views/PackagesView/PackagesView.tsxsrc/views/PackagesView/__tests__/PackagesView.test.tsxsrc/views/PackagesView/index.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75800ff69b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- Block FolderDialog submit when the trimmed path matches the current package folder, so a no-op move never reaches `package_move_to_folder`. - Surface partial failures from `package_move_to_folder` via a new `packages.toast.movePartialError` key (EN + FR) instead of falsely reporting success when `outcome.failed` is non-empty. - Compensate the drag-and-drop two-step move: if `package_add_download` fails after `package_remove_download` succeeded, re-add the download to the source package; surface a dedicated rollback-failed toast (`moveDownloadRollbackError`) when the compensation also fails so the user knows to refresh.
|
On the GitGuardian finding for The flagged line is a TypeScript type annotation: const passwordMut = useTauriMutation<void, { id: string; password: string | null }>(
"package_set_password",
Marking as false positive on the GitGuardian dashboard (incident 32361158). |
|
@coderabbitai on the pre-merge "Docstring Coverage" check
False positive. The check is applying a Python-style docstring rule to a TypeScript/React tree. |
|
Tip For best results, initiate chat on the files or code changes.
I'll record the project's comment policy to avoid flagging missing JSDoc in future reviews. ✏️ Learnings added
🧠 Learnings used |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b870d0ab64
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/i18n/locales/fr.json`:
- Line 597: The translation for the key "moveDownloadRollbackError" still
contains the English word "rollback"; update its value to a fully French
sentence, e.g. replace the current string with something like "Le déplacement a
échoué et l'annulation a également échoué — actualisez pour vérifier le contenu
du paquet" so the toast reads fully in French and uses a proper French word for
rollback (annulation/rétablissement) in src/i18n/locales/fr.json under the
"moveDownloadRollbackError" key.
In `@src/views/PackagesView/PackagesView.tsx`:
- Around line 296-309: The rollback branch in PackagesView (inside the outer
try/catch around the move flow) currently re-throws addError after a failed
rollback which causes the outer catch to also show the generic moveDownloadError
toast and mask the rollback-specific toast; modify the inner catch that handles
addToPackageMut.mutateAsync failure so it does not re-throw addError (e.g.,
return/exit the function after
toast.error(t("packages.toast.moveDownloadRollbackError")) and
invalidatePackages())—leave re-throws for other paths intact so only the
rollback-failed message is shown when the rollback itself fails and the outer
catch only handles genuine move errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91e4e885-08b6-44ef-8a80-a6b288750cad
📒 Files selected for processing (4)
src/i18n/locales/en.jsonsrc/i18n/locales/fr.jsonsrc/views/PackagesView/PackageDialogs.tsxsrc/views/PackagesView/PackagesView.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/i18n/locales/en.json
- src/views/PackagesView/PackageDialogs.tsx
- Switch the `PackageSourceType` union, toolbar filter chips, dialog source select and tests from `split-archive` to `split_archive` to match the backend wire form parsed by `PackageSourceType::from_str` (`src-tauri/src/domain/model/package.rs:62`). Filter and create paths now succeed for split archives, and `PackageView.sourceType` values returned by the backend resolve their `packages.filter.*` i18n label correctly. - Cascade `package_set_priority` invalidation to `downloadQueries.all()` so the Downloads view/details refresh immediately after a package priority change instead of waiting for a background refetch. - Fix the drag-and-drop fallback: `dataTransfer.getData()` returns `""` for missing keys, not `null`, so `??` never selected the `dragRef` fallback. Switched to an explicit empty-string check so cross-package drags still work in environments that strip custom MIME payloads. - Remove the duplicate generic toast when the rollback also fails: `return` instead of re-throwing so only `moveDownloadRollbackError` is surfaced. - Translate the rollback toast fully into French.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/views/PackagesView/__tests__/PackagesView.test.tsx (1)
378-417: ⚡ Quick winAdd regression tests for drag-drop rollback branches.
Current coverage validates only the happy path. Please add cases where
package_add_downloadfails and rollback tofromId(a) succeeds and (b) fails, asserting the right toast path and follow-up calls. This protects the recently fixed failure-handling logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/PackagesView/__tests__/PackagesView.test.tsx` around lines 378 - 417, Add two new tests alongside the existing "moves a download between packages via drag and drop" test that simulate the failure branches: (1) simulate package_add_download throwing an error while package_add_download rollback (package_add_download back to fromId or package_remove_download?) succeeds and assert that mockInvoke was called to rollback to the original package and that the success/toast path used is the "rollback succeeded" toast; (2) simulate package_add_download throwing an error and the rollback call also failing and assert that the code attempted the rollback call and that the failure toast/error toast path was shown. Use the same drag/drop setup and userEvent flow, control responses by changing mockInvoke behavior for "package_add_download" and the rollback call, and assert calls to mockInvoke with "package_remove_download"/"package_add_download" and appropriate toast assertions (checking the same toast API used in the test suite).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/views/PackagesView/PackageDialogs.tsx`:
- Line 149: The rename no-op check uses trimmed !== initialNameRef.current which
still treats edge whitespace on the original as a change; update the comparison
to trim both sides by comparing trimmed against initialNameRef.current.trim()
(in the same scope where canSubmit is computed) so canSubmit becomes const
canSubmit = !submitting && trimmed.length > 0 && trimmed !==
initialNameRef.current.trim(), ensuring no-op renames are correctly detected;
reference variables: canSubmit, trimmed, initialNameRef.
---
Nitpick comments:
In `@src/views/PackagesView/__tests__/PackagesView.test.tsx`:
- Around line 378-417: Add two new tests alongside the existing "moves a
download between packages via drag and drop" test that simulate the failure
branches: (1) simulate package_add_download throwing an error while
package_add_download rollback (package_add_download back to fromId or
package_remove_download?) succeeds and assert that mockInvoke was called to
rollback to the original package and that the success/toast path used is the
"rollback succeeded" toast; (2) simulate package_add_download throwing an error
and the rollback call also failing and assert that the code attempted the
rollback call and that the failure toast/error toast path was shown. Use the
same drag/drop setup and userEvent flow, control responses by changing
mockInvoke behavior for "package_add_download" and the rollback call, and assert
calls to mockInvoke with "package_remove_download"/"package_add_download" and
appropriate toast assertions (checking the same toast API used in the test
suite).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f714f1aa-6982-49cf-a915-548bfe17fdd1
📒 Files selected for processing (7)
src/i18n/locales/en.jsonsrc/i18n/locales/fr.jsonsrc/types/package.tssrc/views/PackagesView/PackageDialogs.tsxsrc/views/PackagesView/PackageToolbar.tsxsrc/views/PackagesView/PackagesView.tsxsrc/views/PackagesView/__tests__/PackagesView.test.tsx
✅ Files skipped from review due to trivial changes (1)
- src/types/package.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/views/PackagesView/PackageToolbar.tsx
Original package name with edge whitespace would falsely enable submit even when the user made no real edit. Compare `trimmed` against `initialNameRef.current.trim()` so no-op renames are correctly blocked, matching the FolderDialog pattern.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/views/PackagesView/PackageDialogs.tsx`:
- Around line 289-292: The handleBrowse handler awaits onPickFolder() without
catching rejections which can cause unhandled promise rejections and leave the
dialog hanging; wrap the await in a try/catch inside handleBrowse (catch any
error from onPickFolder), and in the catch block swallow the exception and
provide graceful feedback (e.g., call a UI error state updater or
notification/toast) while not calling setFolder or rethrowing; update the
handler that currently references handleBrowse, onPickFolder, and setFolder to
implement this local error handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ffa7ddb1-56d9-4a2c-8d40-883067ab0d6d
📒 Files selected for processing (1)
src/views/PackagesView/PackageDialogs.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a830d7f7e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
- `PackageDownloadRow` now exposes `onDragEnd` and `PackagesView` clears `dragRef` on every drag termination, including drags cancelled or dropped outside the package dropzones. Stops a stale ref from being picked up by an unrelated subsequent drop with empty custom MIME payloads (which would otherwise move the previously dragged download to whichever package the user happens to drop on). - Wrap `FolderDialog.handleBrowse` in a try/catch so a future `onPickFolder` implementation that does not pre-catch its rejection cannot leak an unhandled promise rejection. Current `pickFolder` already returns null on failure, but the dialog stays defensive.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a9487ab75d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/views/PackagesView/PackageDialogs.tsx`:
- Around line 46-59: The submit handler currently trims folderPath and sends
trimmedFolder in the payload, which strips intentional leading/trailing spaces;
instead keep using trimmedName/trimmedFolder only for validation (canSubmit) but
pass the original raw folderPath value to onSubmit (or undefined if the raw
value is empty), i.e., in handleSubmit use trimmedName for name validation but
set folderPath in the mutation payload from the original folderPath variable (or
undefined when folderPath.trim().length === 0) so you don't mutate legitimate
paths with whitespace; apply the same change to the other dialog handler that
mirrors this logic (the block referenced around the second occurrence).
In `@src/views/PackagesView/PackagesView.tsx`:
- Around line 203-210: The handleDelete callback currently removes the package
but leaves expandedId pointing to a now-deleted package; update handleDelete
(where you reference deleting and call deleteMut.mutateAsync) to, after the
await completes successfully, check if expandedId === deleting.id and call
setExpandedId(null) (or the component's expanded id setter) to clear the
expanded state before/after showing the success toast so no stale expansion or
subsequent downloads query runs for the removed package.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 75ecd473-e486-440d-afff-705c68200f43
📒 Files selected for processing (4)
src/views/PackagesView/PackageDialogs.tsxsrc/views/PackagesView/PackageDownloadRow.tsxsrc/views/PackagesView/PackageRow.tsxsrc/views/PackagesView/PackagesView.tsx
- `pauseMut` and `resumeMut` now invalidate `INVALIDATE_KEYS_WITH_DOWNLOADS` (covers `packageQueries.all()`), so the rows under an expanded package refresh immediately after bulk pause-all/start-all instead of waiting for an unrelated refetch. - `handleDelete` clears `expandedId` when the deleted package was the expanded one. Avoids a stale expansion pointing at a removed row and the unnecessary `package_list_downloads` query that follows. - `AddPackageDialog` and `FolderDialog` now send the raw `folderPath` / `folder` value to the mutation while still using the trimmed copy only to decide whether the field is empty. Paths with intentional edge whitespace are no longer silently rewritten.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16fb47b786
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await onSubmit({ | ||
| name: trimmedName, | ||
| sourceType, | ||
| folderPath: trimmedFolder.length > 0 ? folderPath : undefined, |
There was a problem hiding this comment.
This dialog computes trimmedFolder but still sends the raw folderPath string, so leading/trailing whitespace is preserved in the persisted package folder. In practice, a user input like "/downloads " creates a package pointing at a different (often invalid) path, which can later break save/move behavior until manually corrected. Use the trimmed value in the payload.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
a user input like
"/downloads "creates a package pointing at a different (often invalid) path
Fixed in 789c885. Reverted the raw-path forwarding from 16fb47b for the folder field — AddPackageDialog now sends trimmedFolder to package_create again. Filesystem paths basically never have intentional edge whitespace, so the practical UX win beats the theoretical "preserve raw input" stance from the previous CodeRabbit thread. name continues to be trimmed as before.
There was a problem hiding this comment.
To use Codex here, create an environment for this repo.
| if (!canSubmit) return; | ||
| setSubmitting(true); | ||
| try { | ||
| await onSubmit(folder); |
There was a problem hiding this comment.
The submit guard is based on folder.trim(), but the mutation sends the untrimmed folder value. If a user enters/pastes a folder with trailing whitespace, the UI accepts it as a real change and sends a path that may not exist, causing avoidable move failures and partial-error toasts. Submit the trimmed path instead.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
the mutation sends the untrimmed
foldervalue
Fixed in 789c885. FolderDialog.handleSubmit now passes trimmed to onSubmit so package_move_to_folder receives the trimmed path. Same reasoning as the AddPackageDialog thread: filesystem paths don't carry intentional edge whitespace, so trimming avoids spurious move failures and partial-error toasts.
There was a problem hiding this comment.
To use Codex here, create an environment for this repo.
Reverts the raw-folder-path forwarding from `16fb47b` for folder fields only. Filesystem paths almost never have intentional edge whitespace, and `"/downloads "` would create a package or folder pointing at an invalid location, then surface as broken save/move behaviour or partial-error toasts. Both `AddPackageDialog` (`folderPath`) and `FolderDialog` (`folder`) now forward the trimmed value while the trim continues to gate the empty-check. `name` was already trimmed and remains so.
…137) * feat(ui): keyboard-accessible move-download-between-packages (#134) Add a keyboard-driven and screen-reader-friendly alternative to the pointer-only HTML5 drag-and-drop introduced for moving downloads between packages in PR #133. - PackageDownloadRow: new "Move…" / "Cancel move" toggle button + aria-grabbed on the row. - PackageRow: "Move selected here" target action + aria-dropeffect on the dropzone (source package excluded). - PackagesView: shared moveDownloadBetweenPackages helper, pendingMove state, and a visually-hidden role="status" aria-live="polite" live region for selected/started/success/cancelled/error announcements. - Pointer drag-and-drop path unchanged. - New i18n keys (EN + FR) under packages.move.*. - Five new Vitest tests cover all four AC from #134. * fix(ui): toast feedback on keyboard move path (#137 review) Match the pointer drag-and-drop path: surface toast.success on successful keyboard move and toast.error on rollback / catch so sighted keyboard users see a visible notification instead of only the sr-only live region. Live region announcements are kept as-is for screen readers. * test(ui): tighten keyboard-move error-path test (#137 review) Per CodeRabbit nitpick: the previous mock made `package_add_download` throw on every call, which exercised the rollback-failure branch of `moveDownloadBetweenPackages` instead of the intended "target add fails, rollback succeeds" path. Throw only when `packageId === "pkg-2"` so the rollback `package_add_download` to `pkg-1` returns null, the inner `throw addError` propagates, and `executeMove`'s outer catch surfaces `packages.move.announce.error` + `toast.error(moveDownloadError)`. Two new ordering assertions on `package_add_download` calls pin the contract. * fix(ui): drop deprecated ARIA + guard executeMove race (#137 review) Address two cubic-dev-ai findings: 1. `aria-grabbed` and `aria-dropeffect` are deprecated in WAI-ARIA 1.1. Replace `aria-grabbed` on the download row with `aria-pressed` on a single `PackageDownloadRow` toggle button (Move ↔ Cancel-move), which is the natural toggle-button semantic for a two-state action. Drop `aria-dropeffect` from the package dropzone wrapper: the `Move selected here` button already announces the affordance through its `aria-label`, and the `aria-live="polite"` region announces start/success/cancel/error transitions. 2. `executeMove` cleared `pendingMove` unconditionally in its `finally` block, which could erase a fresher selection made by the user while the move's async work was still in flight. Wrap the clear in a functional `setPendingMove` that only nulls when the current value still matches the snapshot taken at the start of `executeMove`. A new vitest case pins the race guard via a deferred `package_remove_download` mock; the existing aria-attribute assertions are migrated to the new `aria-pressed` shape. CHANGELOG updated to reflect the modern ARIA choice. * fix(core): bump wasmtime 43.0.1 → 43.0.2 (RUSTSEC-2026-0114) cargo-audit started flagging wasmtime 43.0.1 on 2026-04-30 with RUSTSEC-2026-0114 ("Panic when allocating a table exceeding the size of the host's address space", medium severity, fixed in 43.0.2). The patch bump is constrained by the wasi-common 43.0.1 → 43.0.2 pin, so both crates and their internal helpers (wasmtime-environ, wiggle, winch-codegen, every wasmtime-internal-* shim, etc.) move to 43.0.2 in lockstep. No source change required — the extism git dep at mpiton/extism@f14e56d still resolves cleanly against the new patch. cargo audit --file src-tauri/Cargo.lock: 0 vulnerabilities. cargo clippy --workspace --all-targets -- -D warnings: clean. cargo test --workspace: 1330 passing, 7 ignored. --------- Co-authored-by: Mathieu Piton <27002047+mpiton@users.noreply.github.com>
1 participant
Summary
Replaces the placeholder Packages view with a full tree-based package management UI. Implements all requirements from task 29: tree expand/collapse, inline rename, source-type filtering, auto-extract toggle, password masking, priority selection, folder browsing, and drag-and-drop between packages. Unblocks task 30 (auto-grouping) and completes P1.10 acceptance criteria.
Why
Task 27 and 28 delivered the command and query infrastructure for packages. This task builds the React UI to expose all functionality to users, replacing the placeholder with a complete feature set matching the PRD §6.3 requirements.
Changes
PackagesViewroot component managing filter state, search, dialog targets, and expanded packagepackage_create,package_update,package_set_password,package_set_priority,package_move_to_folder,package_toggle_auto_extract,package_delete,package_add_download,package_remove_downloadPackageToolbarwith source-type filter chips (all/container/playlist/manual/split-archive) and 300ms debounced searchPackageRow(expandable) displaying: name, source-type badge, file count with plural i18n, total bytes, progress bar, folder button, password trigger, auto-extract switch, priority 1-10 select, pause-all/start-all/delete buttonsPackageDownloadRowwith native HTML5 draggable attribute and dataTransfer payloadspackage_remove_download+package_add_downloadon target packagePromise.allSettledoverdownload_pause/download_resumesrc/types/package.tswith PackageView, PackageSourceType enum, PackagePatch, CreatePackageInputsrc/hooks/usePackagesQuery.tswith usePackagesQuery and usePackageDownloadsQuery (TanStack Query v5, 30s/10s staleTime)packageQueriescache key factory tosrc/api/queries.tsTesting
Verified locally:
npx vitest run→ 622/622 total suite greentsc --noEmit→ exit 0npx oxlint .→ 0 warnings, 0 errors across 245 filesRelated Issues
Checklist
Summary by cubic
Replaces the placeholder Packages view with a full tree-based package management UI that completes Task 29. Users can filter, edit, drag-and-drop, and run bulk actions with EN/FR i18n and tests.
New Features
PackagesViewwith expand/collapse rows, aggregated progress, and a toolbar.package_list.package_remove_download+package_add_download.usePackagesQuery/usePackageDownloadsQueryon@tanstack/react-queryv5 withpackageQueries; EN/FRpackages.*translations; 16 Vitest tests.Bug Fixes
package_move_to_folderviapackages.toast.movePartialError(EN/FR).package_add_downloadfails after removal; show a dedicated rollback error; handle emptydataTransfer.getData()and clear stale drag refs ononDragEnd.split_archiveacross filters, dialogs, and tests.downloadQueries.all()afterpackage_set_priorityso Downloads refresh immediately.packageQueries.all()so expanded package downloads refresh immediately.Written for commit 789c885. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
i18n
Tests
Documentation