feat(detections): quick review (Correct/Incorrect) in action menu#2809
feat(detections): quick review (Correct/Incorrect) in action menu#2809
Conversation
Replace snake_case fields and the `as Detection` cast with a fully typed object matching the camelCase Detection interface, adding the required beginTime, endTime, speciesCode, and verified fields.
Adds variant='default'|'overlay' prop to ActionMenu. Default keeps light in-row styling with marker class am-trigger-default; overlay renders a semi-transparent dark rounded button for spectrogram use with marker class am-trigger-overlay. TDD: tests written first.
Adds onMarkCorrect and onMarkFalsePositive props with corresponding menu items at the top of the action menu. Items are hidden when detection is locked. Active state badges (tick/cross) appear next to the relevant item when the detection is already verified. Updates the i18n mock in test setup with the two new translation keys.
Add handleMarkCorrect and handleMarkFalsePositive to the shared useDetectionActions composable and pass them through DetectionCardGrid and DetectionsCardView. The handlers fire the review API immediately without a confirmation modal, matching DetectionRow fast-triage UX.
- Restructure canEdit gate so trigger and Download are available to unauthenticated users (matches old CardActionMenu behavior) - Add hover/padding to menu items (previously relied on dead DaisyUI classes) - Restore focus to trigger button after an action is selected, matching the Escape handler - Use role="separator" on the quick-review divider - Drop leftover dropdown class from the root div - Switch overlay variant to theme-aware CSS vars with backdrop blur instead of hardcoded slate
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughConsolidates card menus by removing Changes
Sequence DiagramsequenceDiagram
actor User
participant ActionMenu
participant DetectionRow
participant Composable as useDetectionActions
participant API as Detection API
participant Toast
User->>ActionMenu: Click "Correct" / "False Positive"
ActionMenu->>DetectionRow: call onMarkCorrect/onMarkFalsePositive()
DetectionRow->>Composable: invoke handleMarkCorrect/handleMarkFalsePositive(detection)
Composable->>API: POST /api/v2/detections/{id}/review {verified: "correct"|"false_positive"}
API-->>Composable: 200 OK
Composable->>DetectionRow: return success
DetectionRow->>DetectionRow: set detection.verified
DetectionRow->>Toast: show success message
DetectionRow->>ActionMenu: request menu close
ActionMenu-->>User: menu closed, UI updated inline
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request merges the CardActionMenu into a unified ActionMenu component, introducing a new overlay variant for spectrogram cards and quick-review actions ('Correct' and 'Incorrect'). It also adds a download action and updates the relevant features and tests to support these changes. Feedback focuses on ensuring quick-review items are only displayed when their callbacks are provided, addressing a regression where verification status is hidden for locked detections, and centralizing duplicated API logic for posting reviews.
| </div> | ||
| </button> | ||
| </li> | ||
| {#if canEdit && !detection.locked} |
There was a problem hiding this comment.
The quick-review items (Correct/Incorrect) are currently displayed whenever canEdit is true and the detection is not locked. However, since onMarkCorrect and onMarkFalsePositive are optional props, the menu will show non-functional items if these handlers are not provided. These items should also be gated by the presence of their respective callbacks to ensure a consistent user experience.
{#if canEdit && !detection.locked && onMarkCorrect && onMarkFalsePositive}
There was a problem hiding this comment.
Fixed in e2cd999 — added onMarkCorrect && onMarkFalsePositive to the guard so the quick-review items only render when their handlers are provided, matching the onDownload pattern.
| <li> | ||
| <button | ||
| onclick={() => handleAction(onReview)} | ||
| class={cn( | ||
| 'text-sm w-full text-left px-3 py-2 rounded-md transition-colors', | ||
| itemHoverClass | ||
| )} | ||
| role="menuitem" | ||
| > | ||
| <div class="flex items-center gap-2"> | ||
| <SquarePen class="size-4" /> | ||
| <span>{t('dashboard.recentDetections.actions.review')}</span> | ||
| </div> | ||
| </button> | ||
| </li> |
There was a problem hiding this comment.
The verification status badges (✓/✗) have been removed from the 'Review' menu item and moved to the new quick-review items. However, since the quick-review items are hidden when a detection is locked (line 240), the verification status is no longer visible anywhere in the action menu for locked detections. This is a regression in information visibility compared to the previous implementation. Consider retaining the status badge in the 'Review' item or ensuring it remains visible when the detection is locked.
There was a problem hiding this comment.
Good catch. Fixed in e2cd999 — restored the ✓/✗ badge on the "Review detection" item so the verified status is visible in the menu even when the detection is locked and the quick-review items are hidden. When not locked the badge now appears twice (on Review and on Correct/Incorrect), which I decided is acceptable: the Review item acts as a persistent status indicator independent of lock state.
| async function postReview(verified: 'correct' | 'false_positive') { | ||
| try { | ||
| await fetchWithCSRF(`/api/v2/detections/${detection.id}/review`, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ verified }), | ||
| }); | ||
| detection.verified = verified; | ||
| toastActions.success( | ||
| verified === 'correct' | ||
| ? t('search.review.markedCorrect') | ||
| : t('search.review.markedFalsePositive') | ||
| ); | ||
| onRefresh?.(); | ||
| } catch (error) { | ||
| toastActions.error(t('search.review.failed')); | ||
| logger.error('Error setting verification status:', error); | ||
| } | ||
| } |
There was a problem hiding this comment.
The postReview logic is duplicated here and in useDetectionActions.svelte.ts. To improve maintainability and ensure consistency across the application, this logic should be centralized. Since this component is part of the detections feature, it should ideally leverage the useDetectionActions composable or a shared utility function for these API calls rather than re-implementing the fetch logic locally.
There was a problem hiding this comment.
Fixed in e2cd999 — extracted the review POST into a new shared utility $lib/utils/reviewDetection.ts (setDetectionVerification). Both DetectionRow and the useDetectionActions composable now call it, so the fetch + toast + logger logic lives in one place.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| <button | ||
| onclick={() => handleAction(onMarkFalsePositive)} | ||
| class={cn( | ||
| 'text-sm w-full text-left px-3 py-2 rounded-md transition-colors', | ||
| itemHoverClass | ||
| )} | ||
| role="menuitem" | ||
| > | ||
| <div class="flex items-center gap-2"> | ||
| <CircleX class="size-4 text-[var(--color-error)]" /> | ||
| <span>{t('dashboard.recentDetections.actions.markFalsePositive')}</span> |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
False positive. The keys dashboard.recentDetections.actions.markCorrect and dashboard.recentDetections.actions.markFalsePositive were added to all 8 locale files in commit d6938c3 (the first commit on this branch), placed right after the existing menuLabel key. They render correctly in tests and in the app; the static analysis must be looking at a stale index. Verify with: grep -c '"markCorrect"' frontend/static/messages/*.json — every file prints 1.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
frontend/static/messages/fr.json (1)
660-661: Consider aligning translations with existing keys for consistency.The translations for
markCorrectandmarkFalsePositivein this section differ from identical keys in thesearch.reviewnamespace (lines 521-522):
- Here (dashboard actions): "Correct" / "Incorrect"
- search.review: "Marquer comme correcte" / "Marquer comme faux positif"
While both are valid French, using consistent translations across namespaces improves UX and reduces maintenance burden. If the shorter forms are intentional for space-constrained UI elements, consider documenting this decision. Otherwise, aligning with the existing verb phrases would be more consistent.
♻️ Suggested alignment with existing translations
- "markCorrect": "Correct", - "markFalsePositive": "Incorrect", + "markCorrect": "Marquer comme correcte", + "markFalsePositive": "Marquer comme faux positif",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/static/messages/fr.json` around lines 660 - 661, The dashboard translations for the keys markCorrect and markFalsePositive are inconsistent with the existing phrases used in the search.review namespace; update the fr.json entries for markCorrect and markFalsePositive to match the existing translations ("Marquer comme correcte" and "Marquer comme faux positif") unless the shorter "Correct"/"Incorrect" forms are intentionally used for UI constraints—in that case, add a short comment in the JSON or documentation indicating the intentional short form; locate and change the values for the keys markCorrect and markFalsePositive (or add the documentation note) so the two namespaces remain consistent.frontend/src/lib/desktop/features/detections/composables/useDetectionActions.svelte.ts (1)
134-160: Centralize the review status literals.
'correct'and'false_positive'are backend API contract values and are repeated across the new handlers. Use named constants to avoid typos and make future reuse safer. As per coding guidelines, Do not use magic numbers or strings - use named constants with descriptive names.♻️ Proposed refactor
+const REVIEW_STATUS = { + CORRECT: 'correct', + FALSE_POSITIVE: 'false_positive', +} as const; + +type ReviewStatus = (typeof REVIEW_STATUS)[keyof typeof REVIEW_STATUS]; + - async function postReview(detection: Detection, verified: 'correct' | 'false_positive') { + async function postReview(detection: Detection, verified: ReviewStatus) { try { await fetchWithCSRF(`/api/v2/detections/${detection.id}/review`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ verified }), }); detection.verified = verified; toastActions.success( - verified === 'correct' + verified === REVIEW_STATUS.CORRECT ? t('search.review.markedCorrect') : t('search.review.markedFalsePositive') ); options.onRefresh?.(); } catch (err) { toastActions.error(t('search.review.failed')); logger.error('Error setting verification status:', err); } } function handleMarkCorrect(detection: Detection) { - postReview(detection, 'correct'); + postReview(detection, REVIEW_STATUS.CORRECT); } function handleMarkFalsePositive(detection: Detection) { - postReview(detection, 'false_positive'); + postReview(detection, REVIEW_STATUS.FALSE_POSITIVE); }Also applies to: 186-187
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/desktop/features/detections/composables/useDetectionActions.svelte.ts` around lines 134 - 160, The code repeats the backend status literals 'correct' and 'false_positive' in postReview and the handlers (postReview, handleMarkCorrect, handleMarkFalsePositive); introduce named constants (e.g., REVIEW_VERIFIED_CORRECT and REVIEW_VERIFIED_FALSE_POSITIVE) at the top of the module and replace all literal occurrences in postReview calls and any other handlers so the API contract values are centralized and less error-prone.frontend/src/lib/desktop/features/detections/components/DetectionRow.svelte (1)
189-215: Avoid duplicating the quick-review POST flow here.
useDetectionActions.svelte.tsnow contains the samepostReviewlogic, so this row can drift from the card/grid behavior. Consider routing the row through the shared composable or extracting a sharedreviewDetection(...)helper used by both paths. Based on learnings, Prefer existing components and extend them rather than creating duplicate functionality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/desktop/features/detections/components/DetectionRow.svelte` around lines 189 - 215, The DetectionRow.svelte duplicate postReview flow should be replaced by the shared action from useDetectionActions.svelte.ts: remove the local async postReview and instead import and call the shared reviewDetection (or the exported function that posts reviews) when handling verification; update handleMarkCorrect and handleMarkFalsePositive to invoke reviewDetection(detection.id, 'correct') and reviewDetection(detection.id, 'false_positive') (or adapt to that helper's signature), keep the toast and onRefresh logic where the shared helper doesn't already handle them, and ensure any error logging uses the centralized logger; this makes postReview logic consistent across DetectionRow.svelte and the card/grid components.frontend/src/lib/desktop/components/ui/ActionMenu.svelte (1)
250-282: Reflect verified state in the menuitem's accessible name/state.The ✓ / ✗ badges are bare text inside the
<button role="menuitem">, so the button's accessible name becomes e.g. "Correct ✓" (announced by screen readers as "Correct, check mark"). Two minor improvements worth considering:
- Convey the verified status semantically rather than via a glyph — e.g., add
aria-current="true"(oraria-pressed/disabled) on the active item, andaria-hidden="true"on the badge span (or replace with ansr-onlylabel like "currently marked correct").- Consider visually distinguishing the active item (e.g., disabled/pressed styling) since clicking an already-verified action still fires
onMarkCorrect/onMarkFalsePositiveand re-hits the API.♻️ Sketch
- <button - onclick={() => handleAction(onMarkCorrect)} - class={cn( - 'text-sm w-full text-left px-3 py-2 rounded-md transition-colors', - itemHoverClass - )} - role="menuitem" - > + {`@const` isCorrect = detection.verified === 'correct'} + <button + onclick={() => handleAction(onMarkCorrect)} + class={cn( + 'text-sm w-full text-left px-3 py-2 rounded-md transition-colors', + itemHoverClass + )} + role="menuitem" + aria-current={isCorrect ? 'true' : undefined} + > <div class="flex items-center gap-2"> <CircleCheck class="size-4 text-[var(--color-success)]" /> <span>{t('dashboard.recentDetections.actions.markCorrect')}</span> - {`#if` detection.verified === 'correct'} + {`#if` isCorrect} <span + aria-hidden="true" class="ml-auto inline-flex items-center px-1.5 py-0.5 rounded text-xs font-medium bg-[var(--color-success)]/15 text-[var(--color-success)]" >✓</span > {/if} </div> </button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/desktop/components/ui/ActionMenu.svelte` around lines 250 - 282, The menu items for markCorrect/markFalsePositive render glyph badges that become part of the button accessible name; update the menu item buttons (the elements using onclick={() => handleAction(onMarkCorrect)} and onclick={() => handleAction(onMarkFalsePositive)}) to expose semantic state and hide decorative glyphs: add aria-current="true" (or aria-pressed="true") on the active item when detection.verified matches the action, add aria-hidden="true" to the badge <span> (or replace the glyph with an sr-only label like "currently marked correct"), and prevent or short-circuit the click handler (in handleAction or before invoking onMarkCorrect/onMarkFalsePositive) when the item is already active so the API is not re-hit; also apply a distinct disabled/active class to visually indicate the current state.frontend/src/lib/desktop/features/dashboard/components/DetectionCard.svelte (1)
7-16: Optional: update top-of-file doc comment to list new callbacks.The header docblock enumerates the supported callback props but doesn't mention the newly added
onMarkCorrect/onMarkFalsePositive. Minor docs drift.📝 Suggested doc update
- onReview?: () => void - Callback for review action + - onMarkCorrect?: () => void - Callback for marking detection as correct + - onMarkFalsePositive?: () => void - Callback for marking detection as false positive - onToggleSpecies?: () => void - Callback for toggle species action - onToggleLock?: () => void - Callback for toggle lock action - onDelete?: () => void - Callback for delete action🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/desktop/features/dashboard/components/DetectionCard.svelte` around lines 7 - 16, Update the top-of-file docblock in DetectionCard.svelte to include the two new callback props: onMarkCorrect?: () => void - Callback invoked when a detection is marked as correct, and onMarkFalsePositive?: () => void - Callback invoked when a detection is marked as a false positive; edit the header that currently lists detection, isNew, onFreezeStart/onFreezeEnd, onReview, onToggleSpecies, onToggleLock, onDelete to also mention these two new handlers so the props list stays accurate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/static/messages/nl.json`:
- Around line 660-661: The label pair uses mixed languages: the "markCorrect"
key is English while "markFalsePositive" is Dutch; update the value of
markCorrect to "Juist" so both quick-review labels are consistent in Dutch
(adjust the string value for the markCorrect key in the NL localization to
"Juist").
---
Nitpick comments:
In `@frontend/src/lib/desktop/components/ui/ActionMenu.svelte`:
- Around line 250-282: The menu items for markCorrect/markFalsePositive render
glyph badges that become part of the button accessible name; update the menu
item buttons (the elements using onclick={() => handleAction(onMarkCorrect)} and
onclick={() => handleAction(onMarkFalsePositive)}) to expose semantic state and
hide decorative glyphs: add aria-current="true" (or aria-pressed="true") on the
active item when detection.verified matches the action, add aria-hidden="true"
to the badge <span> (or replace the glyph with an sr-only label like "currently
marked correct"), and prevent or short-circuit the click handler (in
handleAction or before invoking onMarkCorrect/onMarkFalsePositive) when the item
is already active so the API is not re-hit; also apply a distinct
disabled/active class to visually indicate the current state.
In `@frontend/src/lib/desktop/features/dashboard/components/DetectionCard.svelte`:
- Around line 7-16: Update the top-of-file docblock in DetectionCard.svelte to
include the two new callback props: onMarkCorrect?: () => void - Callback
invoked when a detection is marked as correct, and onMarkFalsePositive?: () =>
void - Callback invoked when a detection is marked as a false positive; edit the
header that currently lists detection, isNew, onFreezeStart/onFreezeEnd,
onReview, onToggleSpecies, onToggleLock, onDelete to also mention these two new
handlers so the props list stays accurate.
In `@frontend/src/lib/desktop/features/detections/components/DetectionRow.svelte`:
- Around line 189-215: The DetectionRow.svelte duplicate postReview flow should
be replaced by the shared action from useDetectionActions.svelte.ts: remove the
local async postReview and instead import and call the shared reviewDetection
(or the exported function that posts reviews) when handling verification; update
handleMarkCorrect and handleMarkFalsePositive to invoke
reviewDetection(detection.id, 'correct') and reviewDetection(detection.id,
'false_positive') (or adapt to that helper's signature), keep the toast and
onRefresh logic where the shared helper doesn't already handle them, and ensure
any error logging uses the centralized logger; this makes postReview logic
consistent across DetectionRow.svelte and the card/grid components.
In
`@frontend/src/lib/desktop/features/detections/composables/useDetectionActions.svelte.ts`:
- Around line 134-160: The code repeats the backend status literals 'correct'
and 'false_positive' in postReview and the handlers (postReview,
handleMarkCorrect, handleMarkFalsePositive); introduce named constants (e.g.,
REVIEW_VERIFIED_CORRECT and REVIEW_VERIFIED_FALSE_POSITIVE) at the top of the
module and replace all literal occurrences in postReview calls and any other
handlers so the API contract values are centralized and less error-prone.
In `@frontend/static/messages/fr.json`:
- Around line 660-661: The dashboard translations for the keys markCorrect and
markFalsePositive are inconsistent with the existing phrases used in the
search.review namespace; update the fr.json entries for markCorrect and
markFalsePositive to match the existing translations ("Marquer comme correcte"
and "Marquer comme faux positif") unless the shorter "Correct"/"Incorrect" forms
are intentionally used for UI constraints—in that case, add a short comment in
the JSON or documentation indicating the intentional short form; locate and
change the values for the keys markCorrect and markFalsePositive (or add the
documentation note) so the two namespaces remain consistent.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 01321b2c-c1e1-46c2-b7e8-bb822e0552ab
📒 Files selected for processing (19)
frontend/src/lib/desktop/components/README.mdfrontend/src/lib/desktop/components/ui/ActionMenu.sveltefrontend/src/lib/desktop/components/ui/ActionMenu.test.tsfrontend/src/lib/desktop/components/ui/README.mdfrontend/src/lib/desktop/features/dashboard/components/CardActionMenu.sveltefrontend/src/lib/desktop/features/dashboard/components/DetectionCard.sveltefrontend/src/lib/desktop/features/dashboard/components/DetectionCardGrid.sveltefrontend/src/lib/desktop/features/detections/components/DetectionRow.sveltefrontend/src/lib/desktop/features/detections/components/DetectionsCardView.sveltefrontend/src/lib/desktop/features/detections/composables/useDetectionActions.svelte.tsfrontend/src/test/setup.tsfrontend/static/messages/de.jsonfrontend/static/messages/en.jsonfrontend/static/messages/es.jsonfrontend/static/messages/fi.jsonfrontend/static/messages/fr.jsonfrontend/static/messages/nl.jsonfrontend/static/messages/pl.jsonfrontend/static/messages/pt.json
💤 Files with no reviewable changes (2)
- frontend/src/lib/desktop/components/README.md
- frontend/src/lib/desktop/features/dashboard/components/CardActionMenu.svelte
- Gate quick-review items by handler presence (not just canEdit/locked) (Gemini: ActionMenu.svelte:240) - Restore verified badge on Review item so status stays visible when the detection is locked and quick-review items are hidden (Gemini: ActionMenu.svelte:301) - Extract setDetectionVerification into $lib/utils/reviewDetection and reuse from DetectionRow and useDetectionActions composable (Gemini: DetectionRow.svelte:207) - Use consistent Dutch wording Juist/Onjuist in nl.json (CodeRabbit: nl.json:660) - Update ActionMenu tests to supply both callbacks when expecting quick-review items to be rendered
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request unifies the detection action menus by merging CardActionMenu into a single ActionMenu component that supports both standard and overlay variants. It introduces "Quick Review" functionality (Mark Correct/Incorrect) and a download option, supported by a new setDetectionVerification utility and updated action composables across the dashboard and detection views. The review feedback highlights that the conditional rendering for the quick-review section is currently too restrictive and suggests avoiding direct object mutation within the utility function to ensure more predictable state management.
| </div> | ||
| </button> | ||
| </li> | ||
| {#if canEdit && !detection.locked && onMarkCorrect && onMarkFalsePositive} |
There was a problem hiding this comment.
The condition onMarkCorrect && onMarkFalsePositive is overly restrictive. If a consumer of this component only provides one of these callbacks (e.g., in a specialized view), the entire quick-review section will be hidden. It is better to use an OR condition here and then conditionally render each item based on the presence of its respective callback to ensure the component is flexible for different use cases.
{#if canEdit && !detection.locked && (onMarkCorrect || onMarkFalsePositive)}
There was a problem hiding this comment.
Fixed in 0c80beb — used OR at the outer quick-review guard and added per-item {#if onMarkCorrect} / {#if onMarkFalsePositive} gates so consumers can wire just one callback if they want.
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify({ verified }), | ||
| }); | ||
| detection.verified = verified; |
There was a problem hiding this comment.
Directly mutating the detection object (detection.verified = verified) inside a utility function is a side effect that can make state management harder to reason about. While using object property access is the preferred way to preserve reactivity in Svelte 5 (as destructuring would break it), it is generally recommended to treat props and shared objects as read-only and use callbacks to update state in the owner component. Since onRefresh is typically called immediately after this utility, ensure this local mutation doesn't conflict with the subsequent data reload or lead to inconsistent UI states if the refresh fails.
References
- In Svelte 5, use object property access instead of destructuring to preserve reactivity when accessing properties of reactive objects.
There was a problem hiding this comment.
Fixed in 0c80beb — made setDetectionVerification pure: it takes detectionId: number, posts to the API, shows the toast, and returns a boolean. The callers in DetectionRow and useDetectionActions now own the detection.verified update and onRefresh?.() call, so the utility no longer has a hidden mutation side effect.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/src/lib/desktop/components/ui/ActionMenu.svelte (1)
286-399:⚠️ Potential issue | 🟡 MinorGate optional menu item handlers individually, not just by
canEdit.The
onReview,onToggleSpecies,onToggleLock, andonDeletecallbacks are optional in the component's type definition, but menu items render whenevercanEditis true. A future consumer that omits one or more of these callbacks would display clickable no-op actions that close the menu without executing any handler.While the current
handleActionfunction safely handles undefined callbacks viaif (action), gating each item by its corresponding handler prevents UI clutter and confusion when callbacks are intentionally omitted.Proposed callback gating
- {`#if` canEdit} + {`#if` canEdit && onReview} <li> <button onclick={() => handleAction(onReview)} @@ </button> </li> + {/if} + {`#if` canEdit && onToggleSpecies} <li> <button onclick={() => handleAction(onToggleSpecies)} @@ </button> </li> + {/if} + {`#if` canEdit && onToggleLock} <li> <button onclick={() => handleAction(onToggleLock)} @@ </button> </li> {/if} @@ - {`#if` canEdit && !detection.locked} + {`#if` canEdit && !detection.locked && onDelete}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/desktop/components/ui/ActionMenu.svelte` around lines 286 - 399, Menu items for onReview, onToggleSpecies, onToggleLock, and onDelete are rendered whenever canEdit is true even if their callback props are undefined; update the Svelte conditionals to gate each menu item by its specific handler as well (e.g., use {`#if` canEdit && onReview} for the review item, {`#if` canEdit && onToggleSpecies} for species toggle, {`#if` canEdit && onToggleLock} for lock toggle, and {`#if` canEdit && !detection.locked && onDelete} for delete) so that handleAction is only reachable when a callback exists.
🧹 Nitpick comments (1)
frontend/src/lib/desktop/components/ui/ActionMenu.svelte (1)
17-65: Forward rest HTML attributes on the root element.
ActionMenuis now a shared UI primitive, but it drops valid root attributes such asid,data-*, and test hooks. Capture...restfrom$props()and spread it onto the wrapper.♻️ Proposed rest-prop forwarding
<script lang="ts"> import { cn } from '$lib/utils/cn'; import type { Detection } from '$lib/types/detection.types'; + import type { HTMLAttributes } from 'svelte/elements'; @@ - interface Props { + interface Props extends HTMLAttributes<HTMLDivElement> { @@ onMenuClose?: () => void; } @@ onMenuOpen, onMenuClose, + class: classProp = '', + ...rest }: Props = $props(); @@ -{`#if` canEdit || onDownload} - <div class={cn('relative', className)}> +{`#if` canEdit || onDownload} + <div {...rest} class={cn('relative', classProp, className)}>As per coding guidelines, “All components must accept additional HTML attributes via rest parameters for extensibility.”
Also applies to: 207-208
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/lib/desktop/components/ui/ActionMenu.svelte` around lines 17 - 65, The ActionMenu component is not forwarding arbitrary HTML attributes (id, data-*, test hooks); capture the component's rest props by calling Svelte's $props() or using the ...$$restProps pattern and spread them onto the root wrapper element (the element that renders the menu UI) so external attributes are preserved; update the component around the root wrapper (referencing ActionMenu, $props(), or $$restProps) to include {...rest} (or {...$$restProps}) and ensure any existing className/variant handling merges with the forwarded props.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@frontend/src/lib/desktop/components/ui/ActionMenu.svelte`:
- Around line 286-399: Menu items for onReview, onToggleSpecies, onToggleLock,
and onDelete are rendered whenever canEdit is true even if their callback props
are undefined; update the Svelte conditionals to gate each menu item by its
specific handler as well (e.g., use {`#if` canEdit && onReview} for the review
item, {`#if` canEdit && onToggleSpecies} for species toggle, {`#if` canEdit &&
onToggleLock} for lock toggle, and {`#if` canEdit && !detection.locked &&
onDelete} for delete) so that handleAction is only reachable when a callback
exists.
---
Nitpick comments:
In `@frontend/src/lib/desktop/components/ui/ActionMenu.svelte`:
- Around line 17-65: The ActionMenu component is not forwarding arbitrary HTML
attributes (id, data-*, test hooks); capture the component's rest props by
calling Svelte's $props() or using the ...$$restProps pattern and spread them
onto the root wrapper element (the element that renders the menu UI) so external
attributes are preserved; update the component around the root wrapper
(referencing ActionMenu, $props(), or $$restProps) to include {...rest} (or
{...$$restProps}) and ensure any existing className/variant handling merges with
the forwarded props.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: caa86835-8c08-4082-8268-013607776eaf
📒 Files selected for processing (6)
frontend/src/lib/desktop/components/ui/ActionMenu.sveltefrontend/src/lib/desktop/components/ui/ActionMenu.test.tsfrontend/src/lib/desktop/features/detections/components/DetectionRow.sveltefrontend/src/lib/desktop/features/detections/composables/useDetectionActions.svelte.tsfrontend/src/lib/utils/reviewDetection.tsfrontend/static/messages/nl.json
✅ Files skipped from review due to trivial changes (2)
- frontend/static/messages/nl.json
- frontend/src/lib/desktop/components/ui/ActionMenu.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/lib/desktop/features/detections/composables/useDetectionActions.svelte.ts
- Gate every menu item by its own callback presence, so a consumer that omits a handler does not render a no-op item (Gemini + CodeRabbit) - Make setDetectionVerification pure: accept detectionId, return boolean, let callers update detection.verified and trigger refresh themselves (Gemini) - Forward rest HTML attributes on the root div so callers can pass id, data-*, aria-* and other standard attributes, per the ui components convention (CodeRabbit)
|
@coderabbitai Fixed both of your outside-diff findings in 0c80beb:
|
|
Tip For best results, initiate chat on the files or code changes.
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
1 participant
Summary
ActionMenu(detections row) andCardActionMenu(dashboard card) components into a single reusable component with avariant: 'default' | 'overlay'propCloses #2712
Behavior
/api/v2/detections/{id}/reviewendpoint immediately with no confirmation modal, then updates the local card state and shows a success toast✓/✗) appear next to the matching item when the detection is already verifiedReviewCardbehavior)canEditTest plan
npm run check:allclean (typecheck, ESLint, Stylelint, ast-grep)npm testgreen across 1895 tests / 98 filesFiles
frontend/src/lib/desktop/components/ui/ActionMenu.svelte- merged component, new props, de-DaisyUIfrontend/src/lib/desktop/components/ui/ActionMenu.test.ts- fixture fix + 8 new testsfrontend/src/lib/desktop/features/dashboard/components/CardActionMenu.svelte- deletedfrontend/src/lib/desktop/features/dashboard/components/DetectionCard.svelte- switched to merged ActionMenu withvariant="overlay"frontend/src/lib/desktop/features/dashboard/components/DetectionCardGrid.svelte- wires new handlersfrontend/src/lib/desktop/features/detections/components/DetectionRow.svelte- wires new handlersfrontend/src/lib/desktop/features/detections/components/DetectionsCardView.svelte- wires new handlersfrontend/src/lib/desktop/features/detections/composables/useDetectionActions.svelte.ts- sharedpostReviewhelper and handlersfrontend/static/messages/{en,de,es,fi,fr,nl,pl,pt}.json-markCorrect/markFalsePositivelabels (short, wrap-safe)frontend/src/lib/desktop/components/ui/README.md+frontend/src/lib/desktop/components/README.md- inventory updatesfrontend/src/test/setup.ts- adds the two new keys to the shared i18n mockSummary by CodeRabbit
New Features
Refactor
Utilities
Documentation
Tests & i18n