feat(ui): keyboard-accessible move-download-between-packages (#134)#137
Conversation
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.
github-actions
Bot
added
documentation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a keyboard-driven "move downloads between packages" flow: selectable source rows, per-package "Move selected here" targets, Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as PackageDownloadRow
participant State as PackagesView<br/>(state)
participant Tree as PackageRow<br/>(targets)
participant IPC as IPC<br/>(remove/add)
User->>UI: Activate Move button (keyboard)
UI->>State: onSelectForMove(download, fromPackageId)
State->>State: set pendingMove, set moveAnnouncement (selected)
State->>Tree: provide pendingMove + actions
Tree->>Tree: show Move-here on non-source packages
User->>Tree: Activate Move-here
Tree->>State: executeMove(toPackage)
State->>State: update moveAnnouncement (started)
State->>IPC: package_remove_download (source)
IPC-->>State: remove OK
State->>IPC: package_add_download (target)
alt add succeeds
IPC-->>State: add OK
State->>State: update moveAnnouncement (success), clear pendingMove
else add fails
IPC-->>State: error
State->>IPC: package_add_download (rollback to source)
IPC-->>State: rollback OK
State->>State: update moveAnnouncement (error), clear pendingMove
end
State->>Tree: refresh/invalidate packages
State->>UI: live region announces result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 unit tests (beta)
Comment |
![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: fea69fb654
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/views/PackagesView/PackagesView.tsx (1)
353-377: ⚡ Quick winAlign keyboard move feedback with pointer move toasts.
executeMovecurrently updates only the live region on success/error, while pointer drag-drop also shows visual toasts. Adding matching success/error toasts here would keep feedback consistent for non-screen-reader users.♻️ Suggested patch
try { const result = await moveDownloadBetweenPackages( move.downloadId, move.fromPackageId, toPackage.id, ); if (!result.ok) { setMoveAnnouncement( t("packages.move.announce.error", { name: move.fileName }), ); toast.error(t("packages.toast.moveDownloadRollbackError")); invalidatePackages(); return; } setMoveAnnouncement( t("packages.move.announce.success", { name: move.fileName, package: toPackage.name, }), ); + toast.success(t("packages.toast.moveDownloadSuccess")); invalidatePackages(); } catch { setMoveAnnouncement( t("packages.move.announce.error", { name: move.fileName }), ); + toast.error(t("packages.toast.moveDownloadError")); } finally { setPendingMove(null); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/PackagesView/PackagesView.tsx` around lines 353 - 377, executeMove updates the ARIA live region via setMoveAnnouncement but doesn't show the same visual toasts that the pointer drag-drop path does; update executeMove (where moveDownloadBetweenPackages is called) to call toast.success on successful move (using the same i18n message used in setMoveAnnouncement for success) and toast.error on failure (including in the catch block) so keyboard-initiated moves show the same visual feedback; keep existing invalidatePackages(), setMoveAnnouncement(...) and use move.fileName and toPackage.name as in the current success path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/views/PackagesView/PackagesView.tsx`:
- Around line 353-377: executeMove updates the ARIA live region via
setMoveAnnouncement but doesn't show the same visual toasts that the pointer
drag-drop path does; update executeMove (where moveDownloadBetweenPackages is
called) to call toast.success on successful move (using the same i18n message
used in setMoveAnnouncement for success) and toast.error on failure (including
in the catch block) so keyboard-initiated moves show the same visual feedback;
keep existing invalidatePackages(), setMoveAnnouncement(...) and use
move.fileName and toPackage.name as in the current success path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85143c7b-990a-419f-a38e-40c65baa94ea
📒 Files selected for processing (8)
CHANGELOG.mdsrc/i18n/locales/en.jsonsrc/i18n/locales/fr.jsonsrc/views/PackagesView/PackageDownloadRow.tsxsrc/views/PackagesView/PackageRow.tsxsrc/views/PackagesView/PackageTree.tsxsrc/views/PackagesView/PackagesView.tsxsrc/views/PackagesView/__tests__/PackagesView.test.tsx
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.
|
@coderabbitai resolve
Fixed in 1b83303. |
|
Tip For best results, initiate chat on the files or code changes.
[resolve] 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/views/PackagesView/PackagesView.tsx (1)
78-80:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInvalidate the download cache after moving a download.
invalidatePackages()only refreshespackageQueries.all(). The expandedusePackageDownloadsQuery(expandedId)result is left stale, so a moved row can still appear under the source package until the user refreshes or re-expands.♻️ Proposed fix
const invalidatePackages = useCallback(() => { queryClient.invalidateQueries({ queryKey: packageQueries.all() }); + queryClient.invalidateQueries({ queryKey: downloadQueries.all() }); }, [queryClient]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/views/PackagesView/PackagesView.tsx` around lines 78 - 80, invalidatePackages currently only invalidates packageQueries.all(), leaving the expanded usePackageDownloadsQuery(expandedId) stale after a move; update invalidatePackages to also invalidate the downloads query so moved rows disappear immediately — call queryClient.invalidateQueries for the packageDownloads key (e.g., packageQueries.downloads(expandedId) when expandedId is available, or the broader packageQueries.downloads() / non-exact prefix if you want to clear all package download caches) in the same useCallback that defines invalidatePackages.
🤖 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/__tests__/PackagesView.test.tsx`:
- Around line 523-545: The test currently makes package_add_download always
throw, which exercises rollback failure instead of the intended "target add
fails but rollback succeeds" path; update the mockInvoke implementation used in
this test so that package_add_download throws only for the target-add call and
returns successfully for the rollback add (e.g., use a counter or inspect the
call args to only throw on the first package_add_download call), ensuring the
moveDownloadBetweenPackages branch where rollback succeeds is exercised and the
assertion expecting an error announcement still holds.
---
Outside diff comments:
In `@src/views/PackagesView/PackagesView.tsx`:
- Around line 78-80: invalidatePackages currently only invalidates
packageQueries.all(), leaving the expanded usePackageDownloadsQuery(expandedId)
stale after a move; update invalidatePackages to also invalidate the downloads
query so moved rows disappear immediately — call queryClient.invalidateQueries
for the packageDownloads key (e.g., packageQueries.downloads(expandedId) when
expandedId is available, or the broader packageQueries.downloads() / non-exact
prefix if you want to clear all package download caches) in the same useCallback
that defines invalidatePackages.
🪄 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: b85dc45f-e93b-45d6-b7cd-3b4a17c36c3a
📒 Files selected for processing (2)
src/views/PackagesView/PackagesView.tsxsrc/views/PackagesView/__tests__/PackagesView.test.tsx
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.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 8 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/views/PackagesView/PackagesView.tsx">
<violation number="1" location="src/views/PackagesView/PackagesView.tsx:341">
P2: `executeMove` clears `pendingMove` unconditionally after async work, which can erase a newer selection made while the move request is still in flight.</violation>
</file>
<file name="src/views/PackagesView/PackageRow.tsx">
<violation number="1" location="src/views/PackagesView/PackageRow.tsx:91">
P2: `aria-dropeffect` is deprecated ARIA and should not be used for new accessibility behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a 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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a4e49a0f9c
ℹ️ 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".
| toast.success(t("packages.toast.moveDownloadSuccess")); | ||
| invalidatePackages(); |
There was a problem hiding this comment.
If a user marks a download with Move… and then moves that same item via pointer drag-and-drop, the success path here never clears pendingMove. The UI keeps showing keyboard move targets based on the old source package, and the next executeMove can attempt package_remove_download from the stale source, producing a false error even though the drag move already succeeded. This cross-flow state leak is introduced by the new keyboard-move state and should be reset when a drag move completes.
Useful? React with 👍 / 👎.
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a1efe653d
ℹ️ 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".
| const isMoveTarget = | ||
| pendingMove !== null && pendingMove.fromPackageId !== pkg.id; |
There was a problem hiding this comment.
isMoveTarget only checks that pendingMove exists and the current row is not the source, so if the source package disappears (for example, the user selects a move and then deletes or filters out that package), every remaining package still shows Move selected here. Clicking it then runs executeMove with a stale fromPackageId, causing package_remove_download to fail and surfacing a misleading move error for an action the UI still advertised as valid. Clear or revalidate pendingMove when its source package is no longer present before rendering targets.
Useful? React with 👍 / 👎.
1 participant
Summary
Closes #134.
Adds a keyboard-driven and screen-reader-friendly alternative to the pointer-only HTML5 drag-and-drop introduced in PR #133 for moving downloads between packages. The drag-and-drop path is unchanged.
PackageDownloadRow— newMove…button (toggles toCancel moveonce a selection exists) +aria-grabbedon the row.PackageRow—Move selected heretarget action surfaces on every non-source package while a move is pending; the dropzone wrapper exposesaria-dropeffect="move".PackagesView— sharedmoveDownloadBetweenPackageshelper (factor of the existingdropDownloadIPC sequence so rollback lives in one place),pendingMovestate, and a visually-hiddenrole="status" aria-live="polite"live region (packages-move-live-region) forselected/started/success/cancelled/errorannouncements.packages.move.*.Acceptance criteria
aria-grabbedon source,aria-dropeffecton targets).Test plan
npx vitest run src/views/PackagesView— 21/21 passingnpm run typecheck— cleannpm run lint— 0 warnings, 0 errorsSummary by CodeRabbit
New Features
Localization
Tests
Documentation