Skip to content

fix: add missing global keyboard shortcuts - #42

Merged
mpiton merged 5 commits into
mainfrom
fix/issue-31-keyboard-shortcuts
Apr 13, 2026
Merged

fix: add missing global keyboard shortcuts#42
mpiton merged 5 commits into
mainfrom
fix/issue-31-keyboard-shortcuts

Conversation

@mpiton

@mpiton mpiton commented Apr 13, 2026

Copy link
Copy Markdown
Owner

Summary

• add the missing global shortcuts for downloads actions, clipboard toggle, and escape handling
• route shortcut intent into Downloads and Link Grabber views for focus, selection, pause/resume, remove, and add-URLs flow
• extend frontend tests for keyboard coverage and fix the French Link Grabber i18n test to render inside a router

Type

fix


Summary by cubic

Adds the missing global keyboard shortcuts for managing downloads, toggling clipboard monitoring, and opening Link Grabber with focus. Addresses Linear issue #31 with scoped handlers and expanded tests.

  • New Features

    • Esc closes the details panel.
    • Downloads: Ctrl/Cmd+F focus search; Ctrl/Cmd+A select visible; Space toggles pause/resume for selected; Delete/Backspace remove selected.
    • Navigation: Ctrl/Cmd+N opens Link Grabber and auto-focuses the paste area (via route state).
    • Clipboard: Ctrl/Cmd+Shift+P toggles monitoring and persists via clipboard_toggle.
    • Added a lightweight shortcut bus (vortex:shortcut-action) via keyboardShortcuts, with handlers in AppLayout and DownloadsView; moved filtering into DownloadsView and added downloadsAreFiltered to DownloadsTable to keep selection/actions scoped to visible items.
  • Bug Fixes

    • Shortcuts are ignored when typing in inputs, textareas, or contenteditable.
    • Bulk remove handles partial failures and preserves selection intent; paste area focus in Link Grabber occurs once and clears route state.
    • Fixed the French Link Grabber i18n test by rendering inside a router.

Written for commit b866640. Summary will update on new commits.

Summary by CodeRabbit

  • New Features

    • Global keyboard shortcuts: Ctrl/Cmd+F (search), Ctrl/Cmd+A (select all), Space (toggle), Delete/Backspace (remove), Escape (close details).
    • Ctrl/Cmd+N navigates to Link Grabber and auto-focuses the paste area; Ctrl/Cmd+Shift+P toggles clipboard monitoring.
    • Bulk download actions (pause/resume/remove) operate only on visible/filtered items and handle partial failures gracefully.
  • Tests

    • Expanded tests for shortcuts, navigation-focus, selection behavior, and native-side effects.
  • Other

    • Search and paste fields annotated as shortcut targets; downloads table accepts pre-filtered data.

@coderabbitai

coderabbitai Bot commented Apr 13, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 86856fff-6113-499b-bca5-6990e0e8898d

📥 Commits

Reviewing files that changed from the base of the PR and between 254a3af and b866640.

📒 Files selected for processing (2)
  • src/views/DownloadsView/DownloadsView.tsx
  • src/views/DownloadsView/__tests__/DownloadsView.test.tsx
✅ Files skipped from review due to trivial changes (1)
  • src/views/DownloadsView/DownloadsView.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/views/DownloadsView/tests/DownloadsView.test.tsx

📝 Walkthrough

Walkthrough

Adds a validated, event-driven keyboard shortcut system and integrates it into AppLayout, DownloadsView, and LinkGrabberView (route-aware handling, editable-target checks, download bulk actions and Tauri invokes); updates tests to provide router context and mock Tauri behavior.

Changes

Cohort / File(s) Summary
Keyboard Shortcuts Infra
src/lib/keyboardShortcuts.ts
New module exporting SHORTCUT_ACTION_EVENT, SHORTCUT_ACTIONS, ShortcutAction type, dispatchShortcutAction() and subscribeShortcutAction() for validated CustomEvent-based shortcut actions.
AppLayout + tests
src/layouts/AppLayout.tsx, src/layouts/__tests__/AppLayout.test.tsx
Route-aware global keydown handling (useLocation), editable-target bypass, Escape handling, non-modifier downloads actions (Space/Delete/Backspace), modifier shortcuts (Ctrl/Cmd+F, Ctrl/Cmd+N, Ctrl/Cmd+A, Shift+P invoking clipboard_toggle), dependency update for effects, plus tests mocking Tauri invoke and using MemoryRouter.
DownloadsView + table/search/tests
src/views/DownloadsView/DownloadsView.tsx, src/views/DownloadsView/DownloadsTable.tsx, src/views/DownloadsView/SearchBar.tsx, src/views/DownloadsView/__tests__/DownloadsView.test.tsx
Introduces filteredDownloads, selection synchronization constrained to visible items, bulk Tauri mutations (pause/resume/remove) with query invalidation, wiring to shortcut actions (focus/search/select-all/toggle/remove), adds data-shortcut-target on search input, and expands tests to verify shortcut-driven behaviors and partial-failure handling.
LinkGrabberView + paste/tests
src/views/LinkGrabberView/LinkGrabberView.tsx, src/views/LinkGrabberView/PasteZone.tsx, src/views/LinkGrabberView/__tests__/LinkGrabberView.test.tsx
Adds router-state-driven autofocus (reads location.state.focusPaste, then clears state via replace) and data-shortcut-target on paste textarea; tests use MemoryRouter and assert focus behavior from route state.
Misc tests
src/i18n/__tests__/issue30-ui-fr.test.tsx
Test render wrapped with MemoryRouter to supply router context for components under test.

Sequence Diagram

sequenceDiagram
    actor User
    participant AppLayout
    participant Router
    participant KeyboardShortcuts
    participant DownloadsView
    participant Tauri

    User->>AppLayout: press key (e.g. Ctrl+F, Space, Del, Esc, Ctrl+Shift+P, Ctrl+N, Ctrl+A)
    AppLayout->>Router: read location.pathname
    AppLayout->>AppLayout: ignore if editable target
    AppLayout->>KeyboardShortcuts: dispatchShortcutAction(action)
    KeyboardShortcuts->>window: emit CustomEvent (SHORTCUT_ACTION_EVENT)
    DownloadsView->>window: subscribed listener receives event
    DownloadsView->>DownloadsView: handle action (focus/search/select/toggle/remove)
    DownloadsView->>Tauri: invoke pause/resume/remove or clipboard_toggle
    Tauri-->>DownloadsView: return result
    DownloadsView->>DownloadsView: update selection/UI
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

Possibly related PRs

Suggested labels

ui

Poem

🐇
I tapped the keys and set them free,
Routes guide hops from A to B.
I nudge the paste, I herd the files,
Shortcuts bound in carrot smiles.
Hooray — a tiny rabbit spree!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: add missing global keyboard shortcuts' is concise and directly reflects the main change—implementing new keyboard shortcuts across the application. It aligns with the primary objective of the PR.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/issue-31-keyboard-shortcuts

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps

greptile-apps Bot commented Apr 13, 2026

Copy link
Copy Markdown

Greptile Summary

This PR wires up a suite of global keyboard shortcuts — Escape, Space, Delete/Backspace, Ctrl+Shift+P, Ctrl+F, Ctrl+N, and Ctrl+A — in AppLayout and routes the resulting custom vortex:shortcut-action events into DownloadsView. It also adds location-state-driven paste focus for LinkGrabberView and wraps several tests in a router to fix a pre-existing rendering issue.

Confidence Score: 5/5

Safe to merge — all findings are P2 style/robustness suggestions with no production-blocking defects.

The implementation is clean and well-tested. The three issues found (deprecated navigator.platform, missing confirmDelete check in the shortcut path, and an order-sensitive unchanged guard) are all P2 and do not affect correctness in normal operation. Good test coverage was added alongside the feature.

src/views/DownloadsView/DownloadsView.tsx — handleRemoveSelected skips confirmDelete and uses an order-sensitive guard; src/layouts/AppLayout.tsx — navigator.platform deprecation.

Important Files Changed

Filename Overview
src/lib/keyboardShortcuts.ts New module — clean event-bus approach with typed action constants, dispatch helper, and auto-cleanup subscribe helper.
src/layouts/AppLayout.tsx Adds global keydown handler; minor: uses deprecated navigator.platform for modifier-key detection.
src/views/DownloadsView/DownloadsView.tsx Subscribes to shortcut-action events; handleRemoveSelected skips confirmDelete check and uses an order-sensitive "unchanged" guard.
src/views/LinkGrabberView/LinkGrabberView.tsx Adds location-state-driven paste-area focus on Ctrl+N navigation; clears state after focus to prevent re-triggering.
src/views/DownloadsView/SearchBar.tsx Adds data-shortcut-target attribute to allow programmatic focus via the focus-search action.
src/views/LinkGrabberView/PasteZone.tsx Adds data-shortcut-target attribute to paste textarea to support focus-on-navigate shortcut.
src/layouts/tests/AppLayout.test.tsx Comprehensive new keyboard shortcut tests covering modifier detection, navigation, clipboard toggle, focus dispatch, and Escape handling.
src/views/DownloadsView/tests/DownloadsView.test.tsx New tests for focus-search, select-all, toggle-selected, and remove-selected shortcut events — good coverage.
src/views/LinkGrabberView/tests/LinkGrabberView.test.tsx Wraps render in MemoryRouter (required fix) and adds focusPaste location-state test.
src/i18n/tests/issue30-ui-fr.test.tsx Updated renderWithProviders to include MemoryRouter wrapper, fixing the French Link Grabber i18n test that was failing without a router context.

Sequence Diagram

sequenceDiagram
    participant U as User
    participant W as Window (keydown)
    participant AL as AppLayout
    participant UI as uiStore
    participant EB as CustomEvent Bus
    participant DV as DownloadsView
    participant LG as LinkGrabberView
    participant BE as Tauri Backend

    U->>W: keydown
    W->>AL: handleKeydown()

    alt Escape + detailsPanelOpen
        AL->>UI: setDetailsPanelOpen(false)
    else Space / Delete (no modifier, /downloads)
        AL->>EB: dispatchShortcutAction(toggle-selected / remove-selected)
        EB->>DV: subscribeShortcutAction handler
        DV->>BE: download_pause / download_resume / download_remove
    else Ctrl+Shift+P
        AL->>BE: clipboard_toggle(enabled)
        BE-->>AL: confirmed (bool)
        AL->>UI: setConfig(clipboardMonitoring)
    else Ctrl+F on /downloads
        AL->>EB: dispatchShortcutAction(focus-search)
        EB->>DV: input.focus() + input.select()
    else Ctrl+N
        AL->>LG: navigate(/link-grabber, focusPaste=true)
        LG->>LG: useEffect detects focusPaste state
        LG->>LG: textarea.focus()
        LG->>LG: navigate(replace, state=null)
    else Ctrl+A on /downloads
        AL->>EB: dispatchShortcutAction(select-all)
        EB->>DV: setSelectedDownloadIds(filteredDownloads)
    else Ctrl+,
        AL->>AL: navigate(/settings)
    else Ctrl+1-9
        AL->>AL: navigate(ROUTES[index])
    end
Loading

Fix All in Claude Code

Reviews (1): Last reviewed commit: "test: wrap link grabber i18n test in rou..." | Re-trigger Greptile

Comment on lines +113 to +134
const handleRemoveSelected = useCallback(async () => {
if (selectedDownloadIds.length === 0) return;

const snapshot = [...selectedDownloadIds];
const results = await Promise.allSettled(
snapshot.map((id) => removeMut.mutateAsync({ id, deleteFiles: false })),
);
const failedIds = snapshot.filter((_, index) => results[index].status === 'rejected');
const currentIds = useUiStore.getState().selectedDownloadIds;
const unchanged =
currentIds.length === snapshot.length &&
currentIds.every((id, index) => id === snapshot[index]);

if (!unchanged) return;

if (failedIds.length === 0) {
clearSelection();
return;
}

setSelectedDownloadIds(failedIds);
}, [clearSelection, removeMut, selectedDownloadIds, setSelectedDownloadIds]);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 confirmDelete setting not checked before bulk remove

The keyboard shortcut path calls removeMut.mutateAsync directly without consulting the confirmDelete flag in AppConfig. If a user has confirmDelete: true, they would expect a confirmation step before downloads are removed — the same expectation they have for UI-driven removals. Is skipping the confirmation intentional for keyboard shortcuts? Is bypassing confirmDelete for the keyboard shortcut intentional, or should the shortcut also respect that setting?

Fix in Claude Code

Comment on lines +121 to +126
const currentIds = useUiStore.getState().selectedDownloadIds;
const unchanged =
currentIds.length === snapshot.length &&
currentIds.every((id, index) => id === snapshot[index]);

if (!unchanged) return;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Order-sensitive "unchanged" guard may skip selection cleanup

The comparison is positional (every((id, index) => id === snapshot[index])), so if the store's selectedDownloadIds array contains the same IDs but in a different order than snapshot (e.g. the user clicked a row while the removes were in-flight), unchanged evaluates to false and neither clearSelection() nor setSelectedDownloadIds(failedIds) is called — leaving stale (already-removed) IDs in the selection state until the next list refresh. A set-based check is more robust:

Suggested change
const currentIds = useUiStore.getState().selectedDownloadIds;
const unchanged =
currentIds.length === snapshot.length &&
currentIds.every((id, index) => id === snapshot[index]);
if (!unchanged) return;
const snapshotSet = new Set(snapshot);
const unchanged =
currentIds.length === snapshot.length &&
currentIds.every((id) => snapshotSet.has(id));

Fix in Claude Code

Comment thread src/layouts/AppLayout.tsx
return;
}

const modifier = navigator.platform.includes("Mac") ? event.metaKey : event.ctrlKey;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 navigator.platform is deprecated

navigator.platform is flagged as deprecated in the WHATWG spec and can be inaccurate on newer Chromium builds. The modern approach uses navigator.userAgentData?.platform with a fallback for environments that don't support it yet. Updating this guard will keep the shortcut modifier detection reliable across Tauri's embedded Chromium runtime as it evolves.

Fix in Claude Code

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (5)
src/lib/keyboardShortcuts.ts (1)

24-26: Harden listener payload handling.

Line 25 trusts event.detail unconditionally. A small runtime guard avoids invalid payloads reaching handlers.

Suggested refactor
 export type ShortcutAction =
   (typeof SHORTCUT_ACTIONS)[keyof typeof SHORTCUT_ACTIONS];
 
+function isShortcutAction(value: unknown): value is ShortcutAction {
+  return (
+    typeof value === "string" &&
+    Object.values(SHORTCUT_ACTIONS).includes(value as ShortcutAction)
+  );
+}
+
 export function dispatchShortcutAction(action: ShortcutAction) {
   window.dispatchEvent(
     new CustomEvent<ShortcutAction>(SHORTCUT_ACTION_EVENT, {
       detail: action,
     }),
   );
 }
 
 export function subscribeShortcutAction(
   handler: (action: ShortcutAction) => void,
 ) {
   const listener = (event: Event) => {
-    handler((event as CustomEvent<ShortcutAction>).detail);
+    const detail = (event as CustomEvent<unknown>).detail;
+    if (isShortcutAction(detail)) {
+      handler(detail);
+    }
   };
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/lib/keyboardShortcuts.ts` around lines 24 - 26, The listener currently
assumes event.detail is a valid ShortcutAction; add a runtime guard in the
listener to verify event is a CustomEvent and that (event as CustomEvent).detail
is defined and matches the expected shape before calling handler. Implement a
simple type-check (e.g., event instanceof CustomEvent && detail != null &&
typeof detail.action === 'string' or other expected properties) or extract a
small type-guard function for ShortcutAction, return early (or warn) on invalid
payloads, and only call handler((event as CustomEvent<ShortcutAction>).detail)
when the checks pass.
src/views/DownloadsView/__tests__/DownloadsView.test.tsx (1)

146-148: Avoid hardcoded shortcut event/action strings in tests.

Using shared constants here will reduce drift risk if identifiers change (Line 146, Line 160, Line 185, Line 210).

Suggested refactor
 import { DownloadsView } from '../DownloadsView';
+import {
+  SHORTCUT_ACTION_EVENT,
+  SHORTCUT_ACTIONS,
+} from '@/lib/keyboardShortcuts';
 ...
     window.dispatchEvent(
-      new CustomEvent('vortex:shortcut-action', {
-        detail: 'downloads.focus-search',
+      new CustomEvent(SHORTCUT_ACTION_EVENT, {
+        detail: SHORTCUT_ACTIONS.downloadsFocusSearch,
       }),
     );
 ...
     window.dispatchEvent(
-      new CustomEvent('vortex:shortcut-action', {
-        detail: 'downloads.select-all',
+      new CustomEvent(SHORTCUT_ACTION_EVENT, {
+        detail: SHORTCUT_ACTIONS.downloadsSelectAll,
       }),
     );
 ...
     window.dispatchEvent(
-      new CustomEvent('vortex:shortcut-action', {
-        detail: 'downloads.toggle-selected',
+      new CustomEvent(SHORTCUT_ACTION_EVENT, {
+        detail: SHORTCUT_ACTIONS.downloadsToggleSelected,
       }),
     );
 ...
     window.dispatchEvent(
-      new CustomEvent('vortex:shortcut-action', {
-        detail: 'downloads.remove-selected',
+      new CustomEvent(SHORTCUT_ACTION_EVENT, {
+        detail: SHORTCUT_ACTIONS.downloadsRemoveSelected,
       }),
     );

Also applies to: 160-162, 185-186, 210-210

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/views/DownloadsView/__tests__/DownloadsView.test.tsx` around lines 146 -
148, Replace the hardcoded event/action strings used in the test (the literal
'vortex:shortcut-action' event name and 'downloads.focus-search' action string)
with the shared shortcut constants exported by your app; import the appropriate
constants (e.g., the event name constant and the downloads focus action
constant) and use them in the CustomEvent(...) calls and any other assertions so
all occurrences in DownloadsView.test.tsx are driven by the canonical constants
instead of string literals.
src/views/DownloadsView/DownloadsView.tsx (3)

160-166: Redundant dependency in effect array.

clearSelection is not directly used in the effect callback—it's captured through handleRemoveSelected which is already in the dependency array. Removing it simplifies the deps without changing behavior.

♻️ Suggested fix
   }, [
-    clearSelection,
     filteredDownloads,
     handleRemoveSelected,
     handleToggleSelected,
     setSelectedDownloadIds,
   ]);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/views/DownloadsView/DownloadsView.tsx` around lines 160 - 166, The effect
dependency array for the useEffect includes a redundant dependency:
clearSelection is not referenced directly in the effect body (it's only captured
via handleRemoveSelected), so remove clearSelection from the dependency list;
update the array that currently contains clearSelection, filteredDownloads,
handleRemoveSelected, handleToggleSelected, setSelectedDownloadIds to exclude
clearSelection while keeping filteredDownloads, handleRemoveSelected,
handleToggleSelected, and setSelectedDownloadIds to preserve correct behavior.

53-88: Consider memoizing filteredDownloads.

This array is recomputed on every render and is included in the useEffect dependency array, causing the shortcut subscription to be recreated whenever downloads, filter, or search changes. For large download lists, wrapping this in useMemo could reduce unnecessary re-subscriptions.

♻️ Suggested change
-  const filteredDownloads = (downloads ?? []).filter((download) => {
+  const filteredDownloads = useMemo(() => (downloads ?? []).filter((download) => {
     if (
       filter === 'active' &&
       ...
     return (
       download.fileName.toLowerCase().includes(query) ||
       download.url.toLowerCase().includes(query) ||
       hostname.includes(query)
     );
-  });
+  }), [downloads, filter, searchQuery]);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/views/DownloadsView/DownloadsView.tsx` around lines 53 - 88, The
filteredDownloads array is being recomputed on every render and triggers
useEffect re-subscriptions; wrap the computation of filteredDownloads in a
useMemo in the DownloadsView component (e.g., const filteredDownloads =
useMemo(() => { /* current filter logic */ }, [downloads, filter, searchQuery]))
so that filteredDownloads is stable unless downloads, filter, or searchQuery
change, and update the useEffect dependency to reference this memoized
filteredDownloads.

178-183: Filtering logic is duplicated between components.

DownloadsView computes filteredDownloads with state and search query filtering (lines 53–80) but passes the full downloads array to DownloadsTable alongside the filter and searchQuery props. DownloadsTable then re-applies the identical filtering logic independently using STATE_FILTER_MAP. Consider passing filteredDownloads directly to DownloadsTable to eliminate the duplicate computation and reduce the risk of divergence.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/views/DownloadsView/DownloadsView.tsx` around lines 178 - 183,
DownloadsView and DownloadsTable both apply the same filtering logic (see
filteredDownloads in DownloadsView and STATE_FILTER_MAP in DownloadsTable),
causing duplicated computation and risk of divergence; update DownloadsView to
pass the precomputed filteredDownloads to DownloadsTable (instead of the full
downloads plus filter/searchQuery) and then remove or short-circuit the
duplicate filter logic inside DownloadsTable (or make DownloadsTable accept an
already-filtered prop and skip STATE_FILTER_MAP when that prop is provided) so
filtering is done only once.
🤖 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/lib/keyboardShortcuts.ts`:
- Around line 24-26: The listener currently assumes event.detail is a valid
ShortcutAction; add a runtime guard in the listener to verify event is a
CustomEvent and that (event as CustomEvent).detail is defined and matches the
expected shape before calling handler. Implement a simple type-check (e.g.,
event instanceof CustomEvent && detail != null && typeof detail.action ===
'string' or other expected properties) or extract a small type-guard function
for ShortcutAction, return early (or warn) on invalid payloads, and only call
handler((event as CustomEvent<ShortcutAction>).detail) when the checks pass.

In `@src/views/DownloadsView/__tests__/DownloadsView.test.tsx`:
- Around line 146-148: Replace the hardcoded event/action strings used in the
test (the literal 'vortex:shortcut-action' event name and
'downloads.focus-search' action string) with the shared shortcut constants
exported by your app; import the appropriate constants (e.g., the event name
constant and the downloads focus action constant) and use them in the
CustomEvent(...) calls and any other assertions so all occurrences in
DownloadsView.test.tsx are driven by the canonical constants instead of string
literals.

In `@src/views/DownloadsView/DownloadsView.tsx`:
- Around line 160-166: The effect dependency array for the useEffect includes a
redundant dependency: clearSelection is not referenced directly in the effect
body (it's only captured via handleRemoveSelected), so remove clearSelection
from the dependency list; update the array that currently contains
clearSelection, filteredDownloads, handleRemoveSelected, handleToggleSelected,
setSelectedDownloadIds to exclude clearSelection while keeping
filteredDownloads, handleRemoveSelected, handleToggleSelected, and
setSelectedDownloadIds to preserve correct behavior.
- Around line 53-88: The filteredDownloads array is being recomputed on every
render and triggers useEffect re-subscriptions; wrap the computation of
filteredDownloads in a useMemo in the DownloadsView component (e.g., const
filteredDownloads = useMemo(() => { /* current filter logic */ }, [downloads,
filter, searchQuery])) so that filteredDownloads is stable unless downloads,
filter, or searchQuery change, and update the useEffect dependency to reference
this memoized filteredDownloads.
- Around line 178-183: DownloadsView and DownloadsTable both apply the same
filtering logic (see filteredDownloads in DownloadsView and STATE_FILTER_MAP in
DownloadsTable), causing duplicated computation and risk of divergence; update
DownloadsView to pass the precomputed filteredDownloads to DownloadsTable
(instead of the full downloads plus filter/searchQuery) and then remove or
short-circuit the duplicate filter logic inside DownloadsTable (or make
DownloadsTable accept an already-filtered prop and skip STATE_FILTER_MAP when
that prop is provided) so filtering is done only once.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4f846a6c-45f4-4d9d-955f-3836eef25874

📥 Commits

Reviewing files that changed from the base of the PR and between 5b02656 and 291c7d9.

📒 Files selected for processing (10)
  • src/i18n/__tests__/issue30-ui-fr.test.tsx
  • src/layouts/AppLayout.tsx
  • src/layouts/__tests__/AppLayout.test.tsx
  • src/lib/keyboardShortcuts.ts
  • src/views/DownloadsView/DownloadsView.tsx
  • src/views/DownloadsView/SearchBar.tsx
  • src/views/DownloadsView/__tests__/DownloadsView.test.tsx
  • src/views/LinkGrabberView/LinkGrabberView.tsx
  • src/views/LinkGrabberView/PasteZone.tsx
  • src/views/LinkGrabberView/__tests__/LinkGrabberView.test.tsx

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 10 files

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
src/views/DownloadsView/DownloadsTable.tsx (1)

315-340: Avoid maintaining two filter implementations.

These rules now exist here and again in src/views/DownloadsView/DownloadsView.tsx Lines 52-94. The next change to a state bucket or search field can easily drift one path from the other. Please extract a shared helper or make DownloadsTable fully presentational in the prefiltered flow.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/views/DownloadsView/DownloadsTable.tsx` around lines 315 - 340, The
filtering logic duplicated between DownloadsTable (filteredDownloads inside
useMemo) and DownloadsView should be centralized: extract the shared filter into
a single helper (e.g., export a function like filterDownloads(downloads, {
filter, searchQuery, downloadsAreFiltered }) that uses STATE_FILTER_MAP and
extractHostname) and have both DownloadsTable and DownloadsView call that
helper, or alternatively make DownloadsTable purely presentational by removing
its internal filteredDownloads logic and accepting a prefiltered downloads prop
from DownloadsView; update references to filteredDownloads, useMemo,
STATE_FILTER_MAP and extractHostname accordingly so only the shared helper
contains the filtering rules.
🤖 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/DownloadsView/DownloadsView.tsx`:
- Around line 96-140: The handlers handleToggleSelected and handleRemoveSelected
operate on selectedDownloadIds against the full downloads list causing actions
on hidden items; restrict actions to the visible selection by intersecting
selectedDownloadIds with filteredDownloads (or the same filtered array used by
DownloadsTable) before building tasks or mutation payloads, and if the
intersection is empty return early; additionally clear stale selection via
clearSelection() or call setSelectedDownloadIds(...) with the intersected IDs
whenever filteredDownloads changes to keep UI and shortcuts in sync.
- Around line 122-140: The partial-removal branch only updates
selectedDownloadIds via setSelectedDownloadIds but doesn't update the
single-item selection selectedDownloadId, leaving it pointing at a deleted item;
after computing failedIds, call setSelectedDownloadIds(failedIds) and then sync
the single selection using selectDownload (or useUiStore.setState) by checking
useUiStore.getState().selectedDownloadId — if that id is not in failedIds, call
selectDownload(undefined) (or select the first id from failedIds if you prefer
keeping an active selection) so both selectedDownloadIds and selectedDownloadId
stay consistent.

---

Nitpick comments:
In `@src/views/DownloadsView/DownloadsTable.tsx`:
- Around line 315-340: The filtering logic duplicated between DownloadsTable
(filteredDownloads inside useMemo) and DownloadsView should be centralized:
extract the shared filter into a single helper (e.g., export a function like
filterDownloads(downloads, { filter, searchQuery, downloadsAreFiltered }) that
uses STATE_FILTER_MAP and extractHostname) and have both DownloadsTable and
DownloadsView call that helper, or alternatively make DownloadsTable purely
presentational by removing its internal filteredDownloads logic and accepting a
prefiltered downloads prop from DownloadsView; update references to
filteredDownloads, useMemo, STATE_FILTER_MAP and extractHostname accordingly so
only the shared helper contains the filtering rules.
🪄 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: ea088b8d-a3c0-4ec3-8642-0b36a34c7921

📥 Commits

Reviewing files that changed from the base of the PR and between 291c7d9 and 1dbebfe.

📒 Files selected for processing (4)
  • src/lib/keyboardShortcuts.ts
  • src/views/DownloadsView/DownloadsTable.tsx
  • src/views/DownloadsView/DownloadsView.tsx
  • src/views/DownloadsView/__tests__/DownloadsView.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/views/DownloadsView/tests/DownloadsView.test.tsx
  • src/lib/keyboardShortcuts.ts

Comment thread src/views/DownloadsView/DownloadsView.tsx Outdated
Comment thread src/views/DownloadsView/DownloadsView.tsx Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/DownloadsView/DownloadsView.tsx`:
- Around line 75-98: The effect currently clears the details panel by checking
visibleSelectedDownloadIds, which wrongly clears the active details when the
active row is still visible but not part of the multi-selection; change the
condition to check visibility against filteredDownloads instead (e.g., if
selectedDownloadId && !filteredDownloads.some(d => d.id === selectedDownloadId)
then call selectDownload(null)), and add filteredDownloads to the useEffect
dependency array so the visibility check updates properly; keep the other sync
logic for selectedDownloadIds as-is.
- Around line 121-150: The stale-state guard is too loose: capture both the
current selectedDownloadIds and selectedDownloadId into local snapshots before
awaiting removeMut.mutateAsync calls (using visibleSelectedDownloadIds for ids
and useUiStore.getState().selectedDownloadId for the single selection), then
after Promise.allSettled compare that post-await state exactly to the pre-await
snapshots (ensure array length/order equality for selectedDownloadIds and exact
equality for selectedDownloadId) and bail if either differs; only if both match
exactly proceed to clearSelection or setSelectedDownloadIds/fallback
selectDownload as currently implemented.
🪄 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: ba9a356d-121e-408f-ab7c-36c351ec8040

📥 Commits

Reviewing files that changed from the base of the PR and between 1dbebfe and 254a3af.

📒 Files selected for processing (3)
  • src/views/DownloadsView/DownloadsTable.tsx
  • src/views/DownloadsView/DownloadsView.tsx
  • src/views/DownloadsView/__tests__/DownloadsView.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/views/DownloadsView/DownloadsTable.tsx

Comment thread src/views/DownloadsView/DownloadsView.tsx
Comment thread src/views/DownloadsView/DownloadsView.tsx Outdated
@mpiton
mpiton merged commit 2af143e into main Apr 13, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant