feat(tags): tag system with explorer integration, and media context menu fixes#3054
Conversation
Tags were silently ignored by FilterBuilder. Adds find_entry_ids_for_tag() (batch lookup via user_metadata_tag → user_metadata → entry, handles both entry_uuid and content_identity_uuid paths) and resolve_tag_filter() (AND logic for include, OR for exclude). Applied in both execute_fast_search_no_fts() and execute_fast_search() FTS path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…s.by_tag Fixes "Tag not found" when clicking a tag in the sidebar, and shows the actual tagged files in the tag view. Backend (register_library_query via inventory): - tags.by_id: find tag by UUID via TagManager::get_tags_by_ids - tags.ancestors: get ancestor tags via TagManager::get_ancestors - tags.children: get descendant tags via TagManager::get_descendants - files.by_tag: find entries via user_metadata_tag → user_metadata → entry (handles both entry_uuid and content_identity_uuid paths) Frontend: - TagView: replace ExplorerView (used global Explorer context, ignored files.by_tag results) with a direct file list rendered from TaggedFileSummary TODOs for tag feature follow-up: - files.by_tag: return full File objects with sd_path so files are clickable/actionable (currently: id, name, extension, size only) - tags.related handler (sidebar shows related tags) - "Filters" button in TagView: secondary filters (type/date/size) within tagged files - tags.children in TagView currently uses get_descendants (all levels); should use get_direct_children for the quick-filter chips - DEL key binding for removing a tag from a file (spacedriveapp#21 dependency resolved) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The apply_tags_to_metadata() relied on catching a unique constraint error to detect duplicates, but no such constraint existed — so every call to tags.apply would silently create a new row. - Migration m20260125: deduplicates existing rows (keeps MIN(id) per pair), then adds UNIQUE INDEX(user_metadata_id, tag_id) - apply_tags_to_metadata(): explicit check-before-insert (upsert pattern), independent of DB constraint Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…button - files.by_tag query now joins directory_paths/volumes/devices to build SdPath::Physical, enabling navigation from tag view to explorer - Tag view: double-click navigates to parent folder (files) or into directory; use react-router navigate() instead of window.location.href - Overview: search button now navigates to /explorer instead of no-op
…jects Backend: - files.by_tag now returns Vec<File> (full domain objects with File::from_entity_model) instead of lightweight TaggedFileSummary, matching the same data format as directory_listing and search.files Frontend: - Add tag mode to explorer context (ENTER_TAG_MODE/EXIT_TAG_MODE) - useExplorerFiles supports tag source via files.by_tag query - Tag route activates tag mode and renders ExplorerView directly, giving tagged files the same UI as normal file browsing (list/grid views, thumbnails, selection, context menus, keyboard shortcuts) - Fix ExplorerView empty state guard to allow tag/recents/search modes without requiring a currentPath Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Backend: - Add tags.unapply action: remove tags from files by entry UUID, resolves via both entry_uuid and content_identity_uuid paths - Add tags.delete action: delete a tag and all its relationships via TagManager::delete_tag() - Add EntryUuid variant to TagTargets and ApplyToTargets to accept frontend UUIDs (fixes parseInt(uuid) bug that tagged wrong files) - files.by_tag: batch load tags for returned files (same pattern as directory_listing) so Inspector shows tags in tag view - navigateToPath exits tag mode to prevent empty directory on nav Frontend: - Tag primitive: add onRemove prop with X button for inline removal - FileInspector: optimistic tag updates via updateSelectedFileTags, refetchQueries with correct query keys (query:files.by_tag prefix) - TagsGroup: right-click delete with confirmation, active state - useFileContextMenu: "Remove tag" option when in tag mode - TagSelector: fix create+apply with EntryUuid fallback - Generated types: add DeleteTagInput/Output, UnapplyTagsInput/Output, EntryUuid variant to TagTargets and ApplyToTargets
Extract duplicated refetchQueries calls from FileInspector, useFileContextMenu, TagsGroup, and TagSelector into a single useRefetchTagQueries hook. Removes direct useQueryClient usage from those files.
…back When the SQL join to devices table returns no result (volume_id or device_id NULL), fall back to get_current_device_slug() instead of the hardcoded \"unknown-device\" string. The previous fallback caused SdPath::is_local() to return false, breaking ephemeral indexing when navigating to directories from the tag view. Fixed in both files.by_tag and search.files queries. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Context menu used useJobDispatch/jobs.dispatch which has no backend handler, causing all media processing (thumbnail, OCR, transcribe, thumbstrip, proxy) to fail from the context menu. - Replace all 15 runJob() calls with direct useLibraryMutation calls (media.thumbnail.regenerate, media.ocr.extract, etc.) - Add forEachTarget helper for batch operations - Add mime_from_extension() fallback in RegenerateThumbnailAction for indexed files where content_identity MIME lookup fails - useJobDispatch.ts is now dead code (no remaining imports) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- TOCTOU race: replace check-then-insert with atomic ON CONFLICT upsert in metadata manager (prevents duplicate tag applications under concurrency) - children query: use get_direct_children (depth=1) instead of get_descendants (entire subtree) for tags.children endpoint - delete atomicity: wrap tag cascade deletion in a transaction (relationships, closure, applications, usage patterns, tag) - files_by_tag: implement include_children and min_confidence filters (were declared in input but ignored) - files_by_tag: map content_id from SQL result instead of fabricating None - files_by_tag: merge entry-scoped and content-scoped tags with dedup (previously content-scoped tags were silently dropped) - unapply: emit resource events for all entries sharing content, not just the directly specified entries - frontend: derive tagModeActive from mode.type instead of storing separately (prevents state desynchronization) - Document sync deletion gaps with TODO(sync) comments Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…cations The dedup query before creating the unique index on (user_metadata_id, tag_id) was keeping MIN(id) — the oldest row. Since user_metadata_tag rows carry mutable state (version, updated_at, device_uuid), keeping MAX(id) preserves the most recent state. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous commit incorrectly derived tagModeActive from mode.type,
conflating two separate concepts:
- mode: {type: "tag"} = viewing files by tag (sidebar navigation)
- tagModeActive = bulk tag assignment UI bar
These are independent: clicking a tag in the sidebar should show tagged
files without opening the assignment bar. Reverts the context.tsx portion
of 04a181535.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Increment version on ON CONFLICT update path so sync detects changes - Only report/notify entries that actually lost a tag (skip when 0 rows deleted) - Exit tag mode on all navigation paths (navigateToView, goBack, goForward) to prevent tag mode leaking through non-path navigation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…icating entry_id=0 and modified_at=now() hide real decode failures. Required fields (entry_id, entry_name, created_at, modified_at) now skip the row with a warning log. Optional/numeric fields (size, child_count) keep sensible defaults since 0 is a valid value. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- FileInspector: remove updateSelectedFileTags() which mutated selectedFiles while the pane renders from file.tags — the refetch on mutation success is what actually updates the UI - TagsGroup: remove alert() on delete error (console.error suffices, alert() breaks platform-agnostic design) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…spector - delete/action.rs: collect affected entry UUIDs before deleting the tag, then emit "file" resource events so the explorer grid updates (removes tag dots). Follows the same pattern as apply and unapply actions. - useRefetchTagQueries: add files.by_id to the refetch list so the Inspector panel updates immediately after tag mutations. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- files_by_tag.rs: root-level files (no parent_path) were missing their extension in the constructed path — now uses the same name+ext logic as the parent_path branch - apply/action.rs: validate that entry UUIDs exist in the entry table before creating user_metadata rows, since there is no FK constraint at the SQLite level on user_metadata.entry_uuid — prevents orphaned metadata from invalid UUIDs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ntry_kind - load_tags_for_entries: pre-build HashMap<content_uuid, Vec<entry_uuid>> from rows in a single pass, then lookup in the content-scoped branch instead of rescanning all rows per metadata record - entry_kind treated as required field (skip row with warning instead of silently defaulting to 0, which would misclassify directories as files) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- FTS5 search: wrap each query token in double quotes to prevent operator injection (AND, OR, NOT, -, *, etc.) - apply/action.rs: replace per-entry UUID lookups with batch query (Entry branch: single WHERE IN instead of N round trips) - apply/action.rs: replace per-entry existence validation with batch query (EntryUuid branch: single WHERE IN instead of N round trips) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ColumnTrait, EntityTrait, QueryFilter are already imported at top-level. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
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:
WalkthroughAdds a migration to enforce unique (user_metadata_id, tag_id); makes tag upserts transactional and idempotent; adds tag library queries/actions (ancestors, by_id, children, files_by_tag, delete, unapply); integrates tag-aware search; introduces explorer tag-browsing mode and UI tag removal with query refetching. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Frontend/User
participant Explorer as Explorer Context
participant Query as files.by_tag Query
participant DB as Database
participant Action as Unapply/Delete Action
participant Event as Event Emitter
User->>Explorer: enterTagMode(tagId)
Explorer->>Query: request files.by_tag(tagId)
Query->>DB: expand descendants (if requested) + find matching entries/tags
DB-->>Query: return file rows
Query-->>Explorer: files result
Explorer-->>User: render tag-mode UI
User->>Explorer: click "remove tag" on file
Explorer->>Action: call tags.unapply(entry_ids, tag_ids)
Action->>DB: resolve tag ids -> user_metadata -> delete user_metadata_tag rows (transaction)
DB-->>Action: rows_deleted & affected entries
Action->>Event: emit file change events for affected entries
Action-->>Explorer: unapply result
Explorer->>Query: refetch files.by_tag
Query->>DB: return updated rows
Query-->>Explorer: updated files
Explorer-->>User: update UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/interface/src/components/SpacesSidebar/TagsGroup.tsx (1)
117-155:⚠️ Potential issue | 🟡 MinorRefresh the sidebar query after tag creation.
This mutation navigates to the new tag, but it never invalidates/refetches the
tags.searchquery used on Lines 119-125. The sidebar count/list can stay stale until some later tag mutation happens.Suggested fix
- const createTag = useLibraryMutation('tags.create'); + const createTag = useLibraryMutation('tags.create', { + onSuccess: refetchTagQueries + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/interface/src/components/SpacesSidebar/TagsGroup.tsx` around lines 117 - 155, After creating a tag in handleCreateTag, the component never refreshes the tags.search query used by useNormalizedQuery, so the sidebar can stay stale; fix this by obtaining the React Query client (useQueryClient) in this component and, after a successful createTag.mutateAsync (e.g., inside the block where result?.tag_id is truthy or immediately after the mutation resolves), call queryClient.invalidateQueries(['tags.search']) or otherwise trigger a refetch of the 'tags.search' query so the sidebar list/count updates. Ensure you import useQueryClient and call invalidateQueries in handleCreateTag after the mutation succeeds.packages/interface/src/routes/explorer/hooks/useFileContextMenu.ts (1)
98-103:⚠️ Potential issue | 🟠 MajorFilter the batch targets by media type before running these submenu actions.
Each submenu is gated by the clicked file, but the handler uses
getTargetFiles()and then runs against the whole selection. With a mixed selection, image/video/audio/document actions will be sent to incompatible files and fail partially.Also applies to: 369-557
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/interface/src/routes/explorer/hooks/useFileContextMenu.ts` around lines 98 - 103, getTargetFiles() currently returns the full selection (minus virtual files) but submenu handlers operate based on the clicked file type, causing mixed selections to send incompatible files to media-specific actions. Update each submenu handler in useFileContextMenu (and other handlers in this file) to first compute targets = getTargetFiles(), then derive filteredTargets by filtering targets to the media type required for that submenu (e.g., use isImage(file) / isVideo(file) / isAudio(file) / isDocument(file) or a similar type guard matching the clicked file), and run the action only against filteredTargets; if filteredTargets.length differs from targets.length, show/emit a user-friendly partial-selection message or disable the submenu option. Ensure filtered arrays keep proper type narrowing (e.g., (f): f is File => ...) so downstream code compiles.
🧹 Nitpick comments (5)
packages/interface/src/routes/overview/OverviewTopBar.tsx (2)
121-201: Memoization here is likely unnecessary and partially unstable.Several
useMemoblocks wrap simple JSX, andoverviewTitleContentdepends on values (handleLibrarySwitch,librarySwitcher) that are likely recreated, reducing memoization benefit.As per coding guidelines: "Use useMemo only when actually needed for expensive computations, not for simple values like string concatenation".
Also applies to: 204-261
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/interface/src/routes/overview/OverviewTopBar.tsx` around lines 121 - 201, overviewTitleContent is wrapped in useMemo but renders simple JSX and depends on likely non-stable references (handleLibrarySwitch, librarySwitcher), making the memo unnecessary and potentially unstable; remove the useMemo wrapper and return the JSX directly (replace the const overviewTitleContent = useMemo(...) with a plain const overviewTitleContent = (...) or inline the JSX where used), and repeat the same change for the other similar block (lines ~204-261) to avoid memoizing simple presentation components that rely on changing callbacks/state.
160-164: Align changed class strings with Tailwind ordering and semantic text tokens.The updated class lists are not in the required order, and
text-whiteshould be replaced with the project’s semantic text color token.As per coding guidelines: "Follow Tailwind class order for readability: Layout, Spacing, Typography, Colors, Borders, Effects, States, Transitions" and "Use semantic Tailwind color classes only...".
Also applies to: 177-177, 184-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/interface/src/routes/overview/OverviewTopBar.tsx` around lines 160 - 164, The class lists used for the library item rendering in OverviewTopBar (the conditional classes using lib.id === currentLibraryId) are out of Tailwind order and use a raw color token; reorder classes to follow Layout, Spacing, Typography, Colors, Borders, Effects, States, Transitions (e.g., 'w-full cursor-pointer rounded-md px-3 py-2 text-left text-sm' -> keep layout/spacing/typography first, then color/state classes) and replace the literal 'text-white' with the project semantic text color token (for example text-on-accent or the repo's equivalent) in the active branch; apply the same ordering and token replacement for the other similar class lists in OverviewTopBar where lib.id/currentLibraryId are used.core/src/infra/db/migration/mod.rs (1)
85-85: Minor formatting inconsistency.Line 85 appears to use spaces for indentation instead of tabs, which is inconsistent with the other entries in the
vec![]. Per coding guidelines, use tabs for indentation.🔧 Fix indentation
- Box::new(m20260125_000001_unique_user_metadata_tag::Migration), + Box::new(m20260125_000001_unique_user_metadata_tag::Migration),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/infra/db/migration/mod.rs` at line 85, The listed migration entry uses spaces for indentation; replace the leading spaces with a tab so the line starting with Box::new(m20260125_000001_unique_user_metadata_tag::Migration) matches the other vec! entries' tab indentation style, ensuring consistent formatting across the migration list.packages/interface/src/routes/tag/index.tsx (1)
14-23: GatingExplorerViewuntil tag mode is initialized is optional, not required.The effect runs after the first render, so
modestarts as{ type: "browse" }whileExplorerViewrenders. However, no extra query fires on first render because the directory query is guarded by!!currentPath, which isnulluntil explicitly navigated. Only the tag query fires once the effect setsmode: { type: "tag", tagId }.If you want to avoid the post-render mode state change, you could gate the component until
mode.type === "tag", but it's an optional optimization since no unnecessary query occurs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/interface/src/routes/tag/index.tsx` around lines 14 - 23, The component currently renders <ExplorerView /> immediately while useEffect sets tag mode afterward via enterTagMode/exitTagMode, causing a post-render mode change; to avoid that optional flash, gate rendering by checking the mode/tag initialization before returning ExplorerView (for example, only return ExplorerView when mode.type === "tag" or when tagId-driven initialization is complete); locate useEffect, enterTagMode, exitTagMode and the return that renders ExplorerView and change it to conditionally render a loader/null until the tag mode is initialized.packages/interface/src/routes/explorer/hooks/useExplorerFiles.ts (1)
145-194: Typefiles.by_taginstead of casting the result toany.The new tag-mode path opts out of TS checks right where explorer assumes a
{ files: File[] }payload. If that response shape drifts, this will quietly fall back to[].♻️ Suggested typing
+interface FilesByTagInput { + tag_id: string; + include_children: boolean; + min_confidence: number; +} + +interface FilesByTagOutput { + files: File[]; +} + -const tagQuery = useNormalizedQuery({ +const tagQuery = useNormalizedQuery<FilesByTagInput, FilesByTagOutput>({ query: "files.by_tag", input: tagQueryInput!, resourceType: "file", enabled: isTagMode && !!tagQueryInput, }); ... - return (tagQuery.data as any)?.files || []; + return tagQuery.data?.files ?? [];As per coding guidelines: "Never use any type in TypeScript - use unknown with type guards if needed".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/interface/src/routes/explorer/hooks/useExplorerFiles.ts` around lines 145 - 194, The tag-mode branch currently casts tagQuery.data to any; change it to use a properly typed response (or unknown with a type guard) so TypeScript enforces the expected { files: File[] } shape: update the useNormalizedQuery call that creates tagQuery to specify the response generic (or return type) for "files.by_tag" instead of using any, add a minimal type/interface (e.g., FilesByTagResponse) or a runtime/type-guard function to assert tagQuery.data has files: File[], and then replace (tagQuery.data as any)?.files with a safe access like isFilesByTagResponse(tagQuery.data) ? tagQuery.data.files : [] so files in useMemo uses the typed payload; reference symbols: tagQuery, useNormalizedQuery, FilesByTagResponse (or the chosen guard), and the files variable in useExplorerFiles.ts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/ops/media/thumbnail/action.rs`:
- Around line 126-149: The MIME lookup currently swallows SeaORM errors via
.ok().flatten()—update the block that queries
entities::content_identity::Entity::find_by_id and
entities::mime_type::Entity::find_by_id (the branch deciding mime_type for
entry.content_id) to distinguish Ok(None) (fall back to
mime_from_extension(&path)) from Err(e) (propagate the DB error); do this by
using ? or .map_err(...) when awaiting the DB calls instead of .ok().flatten(),
and return or propagate the error from the surrounding function so real DB
failures surface rather than being converted into a fallback.
In `@core/src/ops/metadata/manager.rs`:
- Around line 252-310: The loop over tag_applications performs each
insert/upsert directly on the shared db so partial progress can be committed on
error; wrap the entire loop in a single DB transaction by beginning a
transaction from db before the loop, use that transaction handle (instead of
&*db) for user_metadata_tag::Entity::insert(...).on_conflict(...).exec(...) and
for the subsequent user_metadata_tag::Entity::find()...one(...).await re-reads,
and commit the transaction after the loop completes (or rollback on any error)
so all upserts and re-queries occur atomically on the same transaction handle.
In `@core/src/ops/search/query.rs`:
- Around line 394-396: The current creation of SdPath::Physical uses
device_slug.unwrap_or_else(|| crate::device::get_current_device_slug()) where
get_current_device_slug() can return an empty String, leading to empty keys in
by_device() grouping; change the fallback to a clear sentinel (e.g.,
"unknown-device") so that the device_slug used in SdPath::Physical is never
empty — either update the unwrap_or_else here to use "unknown-device" or modify
get_current_device_slug() to return that sentinel on error, ensuring by_device()
and downstream consumers see a distinct, non-empty identifier.
In `@core/src/ops/tags/apply/input.rs`:
- Around line 102-107: The EntryUuid branch in TagTargets::EntryUuid only checks
for empty vector but allows nil (all-zero) UUIDs; update the validation in that
branch to iterate the ids and reject any Uuid::nil() by returning an Err with a
clear message (e.g., "entry UUIDs cannot contain nil/zero UUIDs"). Locate the
TagTargets::EntryUuid handling in the function that currently does
ids.is_empty() and add a check like ids.iter().any(|id| id.is_nil() or *id ==
Uuid::nil()) to fail fast, ensuring malformed client input yields a proper error
instead of being skipped later.
In `@core/src/ops/tags/delete/action.rs`:
- Around line 101-105: The tag deletion path in action.rs currently does not
propagate a sync deletion, so deleted tags can reappear after syncing; update
the tags.delete flow in core::ops::tags::delete::action (the delete handler) to
call library.sync_model(..., ChangeType::Delete) after successful removal (and
before returning), ensuring you pass the deleted tag's model id/context and
handle any sync errors appropriately; remove or replace the TODO with a brief
why-comment noting sync deletion was intentionally invoked and ensure
unit/integration tests cover that a deleted tag is not reintroduced after a sync
cycle.
In `@core/src/ops/tags/unapply/action.rs`:
- Around line 110-114: The tags.unapply flow currently deletes user_metadata_tag
rows locally but does not record or send a sync delete, so removed tags reappear
on other devices; update the unapply implementation in action.rs (the
tags.unapply handler) to create the same delete-side sync change used by apply:
follow the delete/action.rs pattern to emit a ChangeType::Delete (or otherwise
record the model deletion) for the Tag/user_metadata_tag entity and invoke the
library sync path (e.g., library.sync_model() or the project’s sync enqueue) so
the deletion is propagated; ensure the sync metadata (change record, tombstone)
is created consistently with apply and tests updated to cover cross-device
delete propagation.
In `@packages/interface/src/components/SpacesSidebar/TagsGroup.tsx`:
- Around line 42-62: The right-click delete flow in TagsGroup.tsx currently uses
a DOM confirm() inside handleContextMenu which bypasses the app-wide context
menu system; replace this by wiring the delete action into the shared
useContextMenu flow: remove the onContextMenu handler that calls confirm(), call
useContextMenu inside the TagsGroup component to register a "Delete tag" menu
item (with the same semantics: call deleteTag.mutateAsync({ tag_id: tag.id }),
handle errors, and navigate('/') when isActive), and trigger that context menu
on right-click (or via the hook's provided trigger) so confirmation, icons,
keybinds and platform-agnostic behavior are handled by useContextMenu rather
than inline confirm().
In `@packages/interface/src/components/Tags/TagSelector.tsx`:
- Around line 43-61: Resolve the unresolved git merge markers in TagSelector.tsx
by keeping the refactor that uses the shared refetchTagQueries handler: replace
the inline onSuccess arrow that calls queryClient.refetchQueries with onSuccess:
refetchTagQueries (ensuring the useRefetchTagQueries hook is imported and used
where needed), remove the leftover conflict markers (<<<<<<<, =======, >>>>>>>)
and delete any now-unused imports introduced by the old inline implementation so
the file compiles cleanly.
In `@packages/interface/src/hooks/useRefetchTagQueries.ts`:
- Around line 11-16: The useRefetchTagQueries callback currently calls
queryClient.refetchQueries (inside function useRefetchTagQueries) without
specifying type, so inactive cached queries remain untouched; update each
queryClient.refetchQueries call that references the keys
["query:files.directory_listing"], ["query:files.by_tag"],
["query:files.by_id"], and ["query:tags.search"] to include type: 'all' (or
alternatively replace with queryClient.invalidateQueries(..., { refetchType:
'all' })) so both active and inactive queries are refreshed.
In `@packages/interface/src/routes/explorer/hooks/useFileContextMenu.ts`:
- Around line 169-214: Replace direct browser alert() calls in the
useFileContextMenu onClick error handlers (the handlers that call
platform.revealFile and platform.shareFiles and any similar branches mentioned)
with the project’s platform-agnostic toast/error reporting utility: import and
call the shared error/toast function (e.g., notifyError or showToastError from
the common UI/toast module) and pass the error message/context instead of using
alert; update all occurrences in useFileContextMenu (including the revealFile
and shareFiles catch blocks and the other ranges noted) so logging remains
console.error(...) but user-facing messages use the shared toast API.
In `@packages/interface/src/routes/overview/OverviewTopBar.tsx`:
- Around line 107-109: The mutateAsync call on volumeRefreshMutation currently
uses unnecessary "as any" casts; remove both casts so the call becomes
volumeRefreshMutation.mutateAsync({ force: false }) and let TypeScript use the
inferred VolumeRefreshInput/VolumeRefreshOutput from
useLibraryMutation('volumes.refresh') to type the argument and result (no manual
casting or any types in the mutateAsync invocation or assignment).
---
Outside diff comments:
In `@packages/interface/src/components/SpacesSidebar/TagsGroup.tsx`:
- Around line 117-155: After creating a tag in handleCreateTag, the component
never refreshes the tags.search query used by useNormalizedQuery, so the sidebar
can stay stale; fix this by obtaining the React Query client (useQueryClient) in
this component and, after a successful createTag.mutateAsync (e.g., inside the
block where result?.tag_id is truthy or immediately after the mutation
resolves), call queryClient.invalidateQueries(['tags.search']) or otherwise
trigger a refetch of the 'tags.search' query so the sidebar list/count updates.
Ensure you import useQueryClient and call invalidateQueries in handleCreateTag
after the mutation succeeds.
In `@packages/interface/src/routes/explorer/hooks/useFileContextMenu.ts`:
- Around line 98-103: getTargetFiles() currently returns the full selection
(minus virtual files) but submenu handlers operate based on the clicked file
type, causing mixed selections to send incompatible files to media-specific
actions. Update each submenu handler in useFileContextMenu (and other handlers
in this file) to first compute targets = getTargetFiles(), then derive
filteredTargets by filtering targets to the media type required for that submenu
(e.g., use isImage(file) / isVideo(file) / isAudio(file) / isDocument(file) or a
similar type guard matching the clicked file), and run the action only against
filteredTargets; if filteredTargets.length differs from targets.length,
show/emit a user-friendly partial-selection message or disable the submenu
option. Ensure filtered arrays keep proper type narrowing (e.g., (f): f is File
=> ...) so downstream code compiles.
---
Nitpick comments:
In `@core/src/infra/db/migration/mod.rs`:
- Line 85: The listed migration entry uses spaces for indentation; replace the
leading spaces with a tab so the line starting with
Box::new(m20260125_000001_unique_user_metadata_tag::Migration) matches the other
vec! entries' tab indentation style, ensuring consistent formatting across the
migration list.
In `@packages/interface/src/routes/explorer/hooks/useExplorerFiles.ts`:
- Around line 145-194: The tag-mode branch currently casts tagQuery.data to any;
change it to use a properly typed response (or unknown with a type guard) so
TypeScript enforces the expected { files: File[] } shape: update the
useNormalizedQuery call that creates tagQuery to specify the response generic
(or return type) for "files.by_tag" instead of using any, add a minimal
type/interface (e.g., FilesByTagResponse) or a runtime/type-guard function to
assert tagQuery.data has files: File[], and then replace (tagQuery.data as
any)?.files with a safe access like isFilesByTagResponse(tagQuery.data) ?
tagQuery.data.files : [] so files in useMemo uses the typed payload; reference
symbols: tagQuery, useNormalizedQuery, FilesByTagResponse (or the chosen guard),
and the files variable in useExplorerFiles.ts.
In `@packages/interface/src/routes/overview/OverviewTopBar.tsx`:
- Around line 121-201: overviewTitleContent is wrapped in useMemo but renders
simple JSX and depends on likely non-stable references (handleLibrarySwitch,
librarySwitcher), making the memo unnecessary and potentially unstable; remove
the useMemo wrapper and return the JSX directly (replace the const
overviewTitleContent = useMemo(...) with a plain const overviewTitleContent =
(...) or inline the JSX where used), and repeat the same change for the other
similar block (lines ~204-261) to avoid memoizing simple presentation components
that rely on changing callbacks/state.
- Around line 160-164: The class lists used for the library item rendering in
OverviewTopBar (the conditional classes using lib.id === currentLibraryId) are
out of Tailwind order and use a raw color token; reorder classes to follow
Layout, Spacing, Typography, Colors, Borders, Effects, States, Transitions
(e.g., 'w-full cursor-pointer rounded-md px-3 py-2 text-left text-sm' -> keep
layout/spacing/typography first, then color/state classes) and replace the
literal 'text-white' with the project semantic text color token (for example
text-on-accent or the repo's equivalent) in the active branch; apply the same
ordering and token replacement for the other similar class lists in
OverviewTopBar where lib.id/currentLibraryId are used.
In `@packages/interface/src/routes/tag/index.tsx`:
- Around line 14-23: The component currently renders <ExplorerView />
immediately while useEffect sets tag mode afterward via
enterTagMode/exitTagMode, causing a post-render mode change; to avoid that
optional flash, gate rendering by checking the mode/tag initialization before
returning ExplorerView (for example, only return ExplorerView when mode.type ===
"tag" or when tagId-driven initialization is complete); locate useEffect,
enterTagMode, exitTagMode and the return that renders ExplorerView and change it
to conditionally render a loader/null until the tag mode is initialized.
🪄 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: b7db4fb4-3c4b-43a5-b79b-e58cb3e24eb2
⛔ Files ignored due to path filters (1)
packages/ts-client/src/generated/types.tsis excluded by!**/generated/**,!**/generated/**
📒 Files selected for processing (34)
core/src/infra/db/migration/m20260125_000001_unique_user_metadata_tag.rscore/src/infra/db/migration/mod.rscore/src/ops/media/thumbnail/action.rscore/src/ops/metadata/manager.rscore/src/ops/search/query.rscore/src/ops/tags/ancestors.rscore/src/ops/tags/apply/action.rscore/src/ops/tags/apply/input.rscore/src/ops/tags/by_id.rscore/src/ops/tags/children.rscore/src/ops/tags/create/action.rscore/src/ops/tags/create/input.rscore/src/ops/tags/delete/action.rscore/src/ops/tags/delete/input.rscore/src/ops/tags/delete/mod.rscore/src/ops/tags/delete/output.rscore/src/ops/tags/files_by_tag.rscore/src/ops/tags/manager.rscore/src/ops/tags/mod.rscore/src/ops/tags/unapply/action.rscore/src/ops/tags/unapply/input.rscore/src/ops/tags/unapply/mod.rscore/src/ops/tags/unapply/output.rspackages/interface/src/components/Inspector/primitives/Tag.tsxpackages/interface/src/components/Inspector/variants/FileInspector.tsxpackages/interface/src/components/SpacesSidebar/TagsGroup.tsxpackages/interface/src/components/Tags/TagSelector.tsxpackages/interface/src/hooks/useRefetchTagQueries.tspackages/interface/src/routes/explorer/ExplorerView.tsxpackages/interface/src/routes/explorer/context.tsxpackages/interface/src/routes/explorer/hooks/useExplorerFiles.tspackages/interface/src/routes/explorer/hooks/useFileContextMenu.tspackages/interface/src/routes/overview/OverviewTopBar.tsxpackages/interface/src/routes/tag/index.tsx
- Resolve merge conflict markers in TagSelector.tsx - Add type: 'all' to refetchQueries for inactive cache refresh - Replace browser confirm() with useContextMenu in TagsGroup - Add onSuccess refetch to createTag mutation in TagsGroup - Remove unnecessary 'as any' casts in OverviewTopBar - Replace alert() with toast.error() in useFileContextMenu - Remove 'any' casts in useExplorerFiles tag/directory queries - Add nil UUID rejection in tag apply input validation - Propagate SeaORM errors in thumbnail MIME lookup - Wrap tag upsert loop in DB transaction for atomicity - Fix tab indentation in migration mod.rs
…wildcards, remove dead code
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
core/src/ops/metadata/manager.rs (1)
351-351:⚠️ Potential issue | 🟠 Major
remove_semantic_tagsuses a placeholder UUID—method is effectively non-functional.Line 351 calls
get_metadata_by_entry_uuid(Uuid::new_v4()), which generates a random UUID instead of using theentry_idparameter. This means the method will never find existing metadata and will silently returnOk(())without removing any tags.🐛 This pattern repeats in multiple methods
The same issue appears in:
- Line 397:
get_semantic_tags_for_entry- Line 502:
set_favorite- Line 546:
apply_semantic_tag- Line 563:
apply_user_semantic_tags- Line 584:
apply_ai_semantic_tagsAll these methods need the actual entry UUID lookup rather than generating a random one.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/metadata/manager.rs` at line 351, Multiple methods (remove_semantic_tags, get_semantic_tags_for_entry, set_favorite, apply_semantic_tag, apply_user_semantic_tags, apply_ai_semantic_tags) call get_metadata_by_entry_uuid(Uuid::new_v4()) which generates a random UUID and prevents any real lookup; update each call to pass the actual entry UUID parameter (use the method's entry_id / entry_uuid argument) instead of Uuid::new_v4() so get_metadata_by_entry_uuid receives the real entry identifier and the operations perform as intended (search for occurrences in those functions and replace the placeholder with the corresponding entry_id variable).core/src/ops/tags/apply/action.rs (1)
272-285:⚠️ Potential issue | 🟡 MinorRemove the dead
lookup_entry_uuidhelper function.This function has no usages in the codebase. It was superseded by the batch lookup refactor in the
TagTargets::Entryarm and should be removed to keep the code clean.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/tags/apply/action.rs` around lines 272 - 285, Remove the dead helper function lookup_entry_uuid entirely (including its internal use crate::infra::db::entities::entry; and the async fn block) since TagTargets::Entry now uses the batch lookup refactor; delete the whole function definition from action.rs, run a build/check to confirm there are no remaining references, and clean up any now-unused imports in that module caused by removing this function.core/src/ops/tags/manager.rs (2)
742-751:⚠️ Potential issue | 🟠 MajorUse
LikeExprwith ESCAPE clause for proper pattern escaping.In SQLite, the LIKE operator requires an explicit
ESCAPEclause to interpret backslashes as escape characters. Without it, backslashes are literal characters and%and_remain wildcards. The current code escapes with backslashes but passes a plain string to.like(), which generates SQL without an ESCAPE clause—so searches for literal%or_will match incorrectly.Fix this by using SeaORM's
LikeExpr(available since 1.1.9):Suggested fix
use sea_query::LikeExpr; let escaped_query = query.replace('%', r"\%").replace('_', r"\_"); let search_pattern = format!("%{}%", escaped_query); let like_expr = LikeExpr::new(&search_pattern).escape('\\'); let like_models = tag::Entity::find() .filter( tag::Column::CanonicalName .like(&like_expr) .or(tag::Column::DisplayName.like(&like_expr)) .or(tag::Column::FormalName.like(&like_expr)) .or(tag::Column::Abbreviation.like(&like_expr)) .or(tag::Column::Description.like(&like_expr)), ) .all(&*db) .await .map_err(|e| TagError::DatabaseError(e.to_string()))?;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/tags/manager.rs` around lines 742 - 751, The current search builds escaped_query and search_pattern then calls tag::Entity::find() with tag::Column::*.like(&search_pattern), but this generates SQL without an ESCAPE clause so backslash escapes are ignored; fix by constructing a sea_query::LikeExpr from the search_pattern and set its escape char (e.g., LikeExpr::new(&search_pattern).escape('\\')), then pass that LikeExpr to the .like(...) calls for CanonicalName, DisplayName, FormalName, Abbreviation, and Description so the generated SQL includes the ESCAPE '\\' clause and literal '%'/'_' are matched correctly.
724-730:⚠️ Potential issue | 🟠 MajorUse parameterized queries for the FTS5 MATCH clause instead of string formatting.
escape_fts5_query()only escapes FTS5 operators; it does not escape SQL string literals. Inputs containing'produce invalid SQL (breaking the string literal), and theif let Ok(...)fallback silently masks this by dropping to the LIKE path.Replace
Statement::from_string()withStatement::from_sql_and_values()and use a?placeholder for the MATCH term. This pattern is already used elsewhere in the codebase (e.g.,search/query.rs) and SQLite FTS5 supports bind parameters.Suggested fix
- if let Ok(fts_results) = db.query_all( - sea_orm::Statement::from_string( - sea_orm::DatabaseBackend::Sqlite, - format!( - "SELECT rowid FROM tag_search_fts WHERE tag_search_fts MATCH '{}' ORDER BY bm25(tag_search_fts)", - escape_fts5_query(&query) - ) - ) - ).await { + if let Ok(fts_results) = db.query_all( + sea_orm::Statement::from_sql_and_values( + sea_orm::DatabaseBackend::Sqlite, + "SELECT rowid FROM tag_search_fts WHERE tag_search_fts MATCH ? ORDER BY bm25(tag_search_fts)", + vec![escape_fts5_query(query).into()], + ) + ).await {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/tags/manager.rs` around lines 724 - 730, The FTS5 MATCH term is being injected via format! into Statement::from_string (in the db.query_all call that queries "tag_search_fts"), which breaks on quotes and can produce invalid SQL; replace Statement::from_string(...) with Statement::from_sql_and_values(...) using a single '?' placeholder in the MATCH clause and pass the escaped query as a bound value (keep using escape_fts5_query(&query) for FTS5 operator escaping but do not interpolate it into the SQL string). Update the db.query_all invocation to use the new statement + values so the MATCH parameter is bound safely (same pattern as used in search/query.rs).packages/interface/src/routes/overview/OverviewTopBar.tsx (1)
204-206:⚠️ Potential issue | 🟠 MajorSearch action has no click path to
/explorer.Line 205 renders
CircleButtonwithoutonClick, and Line 279 also omitsTopBarItem.onClick, so the search control is currently inert.Proposed fix
+ const handleSearch = () => { + navigate('/explorer'); + }; const searchButton = useMemo( - () => <CircleButton icon={MagnifyingGlass} title="Search" />, - [] + () => ( + <CircleButton + icon={MagnifyingGlass} + title="Search" + onClick={handleSearch} + /> + ), + [handleSearch] ); - <TopBarItem id="search" label="Search" priority="high"> + <TopBarItem + id="search" + label="Search" + priority="high" + onClick={handleSearch} + >Also applies to: 279-281
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/interface/src/routes/overview/OverviewTopBar.tsx` around lines 204 - 206, The Search control is rendered inert because CircleButton (in the searchButton useMemo) and the TopBarItem instance lack an onClick handler; update the OverviewTopBar component to attach an onClick that navigates to "/explorer" (use the existing navigation mechanism in the module, e.g., useNavigate or router helper) — add the onClick prop to the CircleButton created in the searchButton useMemo and to the TopBarItem that renders the search control so clicking either routes to "/explorer".
🧹 Nitpick comments (8)
core/src/ops/tags/create/input.rs (1)
120-151: Consider validating nil UUIDs forContentvariant too.
EntryUuidrejects nil UUIDs (lines 143-145), butContentdoesn't have the same check. For consistency and to prevent potential issues with nil content identity UUIDs, consider adding the same validation.♻️ Proposed fix
ApplyToTargets::Content(ids) => { if ids.is_empty() { return Err("apply_to content IDs cannot be empty".to_string()); } + if ids.iter().any(Uuid::is_nil) { + return Err("apply_to content IDs cannot contain nil values".to_string()); + } if ids.len() > 1000 { return Err("Cannot apply to more than 1000 targets at once".to_string()); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/tags/create/input.rs` around lines 120 - 151, Add the same nil-UUID validation to the ApplyToTargets::Content branch: inside the ApplyToTargets::Content(ids) arm (alongside the existing empty and length checks) iterate over ids and return an error if any Uuid::is_nil is found (e.g., "apply_to content UUIDs cannot contain nil values"), mirroring the logic used in ApplyToTargets::EntryUuid so Content rejects nil UUIDs as well.packages/interface/src/components/SpacesSidebar/TagsGroup.tsx (1)
58-60: Consider usingtoast.errorinstead ofconsole.errorfor user feedback.When tag deletion fails, the user only sees the error in the console. Adding a toast notification would improve UX consistency with other error handling in this PR (e.g.,
TagAssignmentMode.tsxline 85).♻️ Proposed fix
+import { toast } from '@spacedrive/primitives'; // ... in onClick handler } catch (err) { console.error('Failed to delete tag:', err); + toast.error(`Failed to delete tag: ${err}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/interface/src/components/SpacesSidebar/TagsGroup.tsx` around lines 58 - 60, Replace the console error in the TagsGroup deletion catch block with a user-facing toast: in the catch of the delete handler inside TagsGroup.tsx (the block catching errors from the tag deletion call), call toast.error with a clear message that includes the error text (e.g., "Failed to delete tag: " + (err?.message || String(err))) and ensure toast is imported (e.g., from 'react-hot-toast'); you may still log to console.error for debugging but the primary change is adding toast.error so users see the failure.packages/interface/src/routes/explorer/hooks/useExplorerFiles.ts (1)
136-143: Minor: Redundant type guard intagQueryInputmemo.Line 137 checks
!isTagMode || mode.type !== "tag", butisTagModeis already derived frommode.type === "tag"(line 35), making the second condition redundant.♻️ Simplified version
const tagQueryInput = useMemo(() => { - if (!isTagMode || mode.type !== "tag") return null; + if (!isTagMode) return null; + // TypeScript narrowing: at this point mode.type === "tag" + const tagMode = mode as { type: "tag"; tagId: string }; return { - tag_id: mode.tagId, + tag_id: tagMode.tagId, include_children: false, min_confidence: 0.0, }; }, [isTagMode, mode]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/interface/src/routes/explorer/hooks/useExplorerFiles.ts` around lines 136 - 143, The memoized tagQueryInput currently uses a redundant guard `!isTagMode || mode.type !== "tag"` even though `isTagMode` is derived as `mode.type === "tag"`; update the useMemo in useExplorerFiles.ts (the tagQueryInput useMemo) to only check `if (!isTagMode) return null;` and return the same object when true, keeping the rest of the logic and dependencies intact (ensure useMemo still depends on the values used to build the object such as isTagMode and mode.tagId).core/src/ops/tags/apply/action.rs (3)
179-192: Consider importingHashSetat the module level.The implementation correctly batch-validates entry UUIDs and uses
HashSetfor O(1) existence checks. However,std::collections::HashSetis used with its full path whileHashMapis imported at line 17.♻️ Suggested fix for import consistency
use std::collections::HashMap; +use std::collections::HashSet;Then update line 181:
- let existing_entries: std::collections::HashSet<Uuid> = + let existing_entries: HashSet<Uuid> =🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/tags/apply/action.rs` around lines 179 - 192, The code uses the fully-qualified std::collections::HashSet when building existing_entries inside the TagTargets::EntryUuid match arm; to keep imports consistent with the existing HashMap import, add use std::collections::HashSet at the module level and replace the fully-qualified path in the existing_entries declaration (the expression that constructs the HashSet from Entity::find()...collect()) to reference HashSet directly so the type is imported rather than referenced by full path.
133-149: Good optimization: batch lookup eliminates N+1 queries.The refactor from per-entry lookups to a single batch query is a solid performance improvement. The pattern is correct: batch fetch, build lookup map, iterate with O(1) lookups.
Minor inconsistency: the entity path
crate::infra::db::entities::entry::Entityis used inline here, but at line 274 it's properly imported with ausestatement inside the function. Consider adding a localusestatement at the top of this match arm for consistency:use crate::infra::db::entities::entry;This matches the pattern already used in the
TagTargets::Contentarm (line 109).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/tags/apply/action.rs` around lines 133 - 149, The code performs a batch lookup using fully-qualified paths (crate::infra::db::entities::entry::Entity and Column) which is inconsistent with the other arm; add a local use inside this match arm/function like `use crate::infra::db::entities::entry;` and then update the calls to use entry::Entity and entry::Column (and any other entry:: symbols) so the style matches the TagTargets::Content arm and keeps imports consistent.
254-254: Remove TODO comment; track in a GitHub issue instead.As per coding guidelines, TODO comments should not be left in production code. If returning target IDs is needed, please track it in a GitHub issue.
♻️ Suggested fix
- vec![], // TODO: Return target IDs if needed + vec![],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/tags/apply/action.rs` at line 254, Remove the inline TODO comment next to the vec![] literal (i.e., delete " // TODO: Return target IDs if needed") and keep the empty vec![] as-is; create a GitHub issue to track the work to "return target IDs" and reference that issue in the PR/commit message instead of leaving a TODO in the code.core/src/ops/tags/manager.rs (1)
359-359: Expand the changed public API docs to cover the contract, not just the name.These summaries miss the non-obvious behavior that matters here:
delete_tag()now guarantees a single transactional delete across related tables, andget_direct_children()intentionally excludes transitive descendants. As per coding guidelines, "Function documentation should explain design rationale and non-obvious behavior in the second paragraph, not just restate the code" and "core/**/*.rs: Use///for public items, and include examples".Also applies to: 648-648
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/tags/manager.rs` at line 359, Update the public doc comments for delete_tag() and get_direct_children(): use triple-slash `///` docs, add a second paragraph that states the contract and non-obvious behavior (delete_tag() performs a single atomic transactional delete across all related tables and will roll back on failure; get_direct_children() returns only immediate children and intentionally excludes transitive descendants), include brief rationale for these design choices and a small usage example for each to show expected behavior; ensure the comments follow the project guideline style for core public items and mention error/rollback semantics for delete_tag().packages/interface/src/routes/overview/OverviewTopBar.tsx (1)
169-169: Normalize Tailwind utility order in changed className strings.These changed class strings place state/colors before layout/spacing, which makes scanning inconsistent.
Proposed cleanup
-<div className="border-app-line my-1 border-t" /> +<div className="border-t my-1 border-app-line" /> -className="hover:bg-app-selected text-ink flex w-full cursor-pointer items-center gap-2 rounded-md px-3 py-2 text-sm font-medium" +className="flex w-full cursor-pointer items-center gap-2 rounded-md px-3 py-2 text-sm font-medium text-ink hover:bg-app-selected" -className="hover:bg-app-selected text-ink flex w-full cursor-pointer items-center gap-2 rounded-md px-3 py-2 text-sm font-medium" +className="flex w-full cursor-pointer items-center gap-2 rounded-md px-3 py-2 text-sm font-medium text-ink hover:bg-app-selected"As per coding guidelines, "Follow Tailwind class order for readability: Layout, Spacing, Typography, Colors, Borders, Effects, States, Transitions".
Also applies to: 177-177, 184-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/interface/src/routes/overview/OverviewTopBar.tsx` at line 169, The Tailwind classes in OverviewTopBar's divider div(s) are ordered inconsistently (e.g., "border-app-line my-1 border-t"); reorder them to follow the Tailwind convention (Layout, Spacing, Typography, Colors, Borders, Effects, States, Transitions) so the divider classNames become "my-1 border-app-line border-t" (or equivalent ordering where spacing precedes color then border). Update all occurrences inside the OverviewTopBar component (the divider <div className="..."/>) — including the instances noted around the changed lines — to use the normalized class order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/ops/metadata/manager.rs`:
- Around line 260-268: The serialization call using
serde_json::to_value(&app.instance_attributes).unwrap() can panic; change it to
propagate the error instead: call serde_json::to_value(&app.instance_attributes)
and propagate failures with ? (or map_err to the function's error type) when
building instance_attributes_value so that app.instance_attributes and
instance_attributes_value handle serialization errors without unwrap; update the
enclosing function's return type to Result<..., E> (or convert the
serde_json::Error into the existing error type) so the error can flow out
safely.
In `@packages/interface/src/components/SpacesSidebar/TagsGroup.tsx`:
- Around line 43-64: Replace the browser-only confirm() call in the context menu
onClick with a modal-based confirmation flow: add a component state like
showDeleteConfirm via useState (e.g., const [showDeleteConfirm,
setShowDeleteConfirm] = useState(false)), change the context menu item's onClick
to setShowDeleteConfirm(true) (inside the useContextMenu items array where the
Trash/Delete Tag action is defined), and move the delete logic that calls
deleteTag.mutateAsync({ tag_id: tag.id }) (and the isActive -> navigate('/')
behavior) into the modal's confirm button handler; render a modal when
showDeleteConfirm is true that displays the tag name (tag.canonical_name ||
tag.display_name) and has Cancel (setShowDeleteConfirm(false)) and Delete (call
mutateAsync, handle errors, then close modal and navigate if needed) buttons to
match the SourceDetail.tsx pattern.
---
Outside diff comments:
In `@core/src/ops/metadata/manager.rs`:
- Line 351: Multiple methods (remove_semantic_tags, get_semantic_tags_for_entry,
set_favorite, apply_semantic_tag, apply_user_semantic_tags,
apply_ai_semantic_tags) call get_metadata_by_entry_uuid(Uuid::new_v4()) which
generates a random UUID and prevents any real lookup; update each call to pass
the actual entry UUID parameter (use the method's entry_id / entry_uuid
argument) instead of Uuid::new_v4() so get_metadata_by_entry_uuid receives the
real entry identifier and the operations perform as intended (search for
occurrences in those functions and replace the placeholder with the
corresponding entry_id variable).
In `@core/src/ops/tags/apply/action.rs`:
- Around line 272-285: Remove the dead helper function lookup_entry_uuid
entirely (including its internal use crate::infra::db::entities::entry; and the
async fn block) since TagTargets::Entry now uses the batch lookup refactor;
delete the whole function definition from action.rs, run a build/check to
confirm there are no remaining references, and clean up any now-unused imports
in that module caused by removing this function.
In `@core/src/ops/tags/manager.rs`:
- Around line 742-751: The current search builds escaped_query and
search_pattern then calls tag::Entity::find() with
tag::Column::*.like(&search_pattern), but this generates SQL without an ESCAPE
clause so backslash escapes are ignored; fix by constructing a
sea_query::LikeExpr from the search_pattern and set its escape char (e.g.,
LikeExpr::new(&search_pattern).escape('\\')), then pass that LikeExpr to the
.like(...) calls for CanonicalName, DisplayName, FormalName, Abbreviation, and
Description so the generated SQL includes the ESCAPE '\\' clause and literal
'%'/'_' are matched correctly.
- Around line 724-730: The FTS5 MATCH term is being injected via format! into
Statement::from_string (in the db.query_all call that queries "tag_search_fts"),
which breaks on quotes and can produce invalid SQL; replace
Statement::from_string(...) with Statement::from_sql_and_values(...) using a
single '?' placeholder in the MATCH clause and pass the escaped query as a bound
value (keep using escape_fts5_query(&query) for FTS5 operator escaping but do
not interpolate it into the SQL string). Update the db.query_all invocation to
use the new statement + values so the MATCH parameter is bound safely (same
pattern as used in search/query.rs).
In `@packages/interface/src/routes/overview/OverviewTopBar.tsx`:
- Around line 204-206: The Search control is rendered inert because CircleButton
(in the searchButton useMemo) and the TopBarItem instance lack an onClick
handler; update the OverviewTopBar component to attach an onClick that navigates
to "/explorer" (use the existing navigation mechanism in the module, e.g.,
useNavigate or router helper) — add the onClick prop to the CircleButton created
in the searchButton useMemo and to the TopBarItem that renders the search
control so clicking either routes to "/explorer".
---
Nitpick comments:
In `@core/src/ops/tags/apply/action.rs`:
- Around line 179-192: The code uses the fully-qualified
std::collections::HashSet when building existing_entries inside the
TagTargets::EntryUuid match arm; to keep imports consistent with the existing
HashMap import, add use std::collections::HashSet at the module level and
replace the fully-qualified path in the existing_entries declaration (the
expression that constructs the HashSet from Entity::find()...collect()) to
reference HashSet directly so the type is imported rather than referenced by
full path.
- Around line 133-149: The code performs a batch lookup using fully-qualified
paths (crate::infra::db::entities::entry::Entity and Column) which is
inconsistent with the other arm; add a local use inside this match arm/function
like `use crate::infra::db::entities::entry;` and then update the calls to use
entry::Entity and entry::Column (and any other entry:: symbols) so the style
matches the TagTargets::Content arm and keeps imports consistent.
- Line 254: Remove the inline TODO comment next to the vec![] literal (i.e.,
delete " // TODO: Return target IDs if needed") and keep the empty vec![] as-is;
create a GitHub issue to track the work to "return target IDs" and reference
that issue in the PR/commit message instead of leaving a TODO in the code.
In `@core/src/ops/tags/create/input.rs`:
- Around line 120-151: Add the same nil-UUID validation to the
ApplyToTargets::Content branch: inside the ApplyToTargets::Content(ids) arm
(alongside the existing empty and length checks) iterate over ids and return an
error if any Uuid::is_nil is found (e.g., "apply_to content UUIDs cannot contain
nil values"), mirroring the logic used in ApplyToTargets::EntryUuid so Content
rejects nil UUIDs as well.
In `@core/src/ops/tags/manager.rs`:
- Line 359: Update the public doc comments for delete_tag() and
get_direct_children(): use triple-slash `///` docs, add a second paragraph that
states the contract and non-obvious behavior (delete_tag() performs a single
atomic transactional delete across all related tables and will roll back on
failure; get_direct_children() returns only immediate children and intentionally
excludes transitive descendants), include brief rationale for these design
choices and a small usage example for each to show expected behavior; ensure the
comments follow the project guideline style for core public items and mention
error/rollback semantics for delete_tag().
In `@packages/interface/src/components/SpacesSidebar/TagsGroup.tsx`:
- Around line 58-60: Replace the console error in the TagsGroup deletion catch
block with a user-facing toast: in the catch of the delete handler inside
TagsGroup.tsx (the block catching errors from the tag deletion call), call
toast.error with a clear message that includes the error text (e.g., "Failed to
delete tag: " + (err?.message || String(err))) and ensure toast is imported
(e.g., from 'react-hot-toast'); you may still log to console.error for debugging
but the primary change is adding toast.error so users see the failure.
In `@packages/interface/src/routes/explorer/hooks/useExplorerFiles.ts`:
- Around line 136-143: The memoized tagQueryInput currently uses a redundant
guard `!isTagMode || mode.type !== "tag"` even though `isTagMode` is derived as
`mode.type === "tag"`; update the useMemo in useExplorerFiles.ts (the
tagQueryInput useMemo) to only check `if (!isTagMode) return null;` and return
the same object when true, keeping the rest of the logic and dependencies intact
(ensure useMemo still depends on the values used to build the object such as
isTagMode and mode.tagId).
In `@packages/interface/src/routes/overview/OverviewTopBar.tsx`:
- Line 169: The Tailwind classes in OverviewTopBar's divider div(s) are ordered
inconsistently (e.g., "border-app-line my-1 border-t"); reorder them to follow
the Tailwind convention (Layout, Spacing, Typography, Colors, Borders, Effects,
States, Transitions) so the divider classNames become "my-1 border-app-line
border-t" (or equivalent ordering where spacing precedes color then border).
Update all occurrences inside the OverviewTopBar component (the divider <div
className="..."/>) — including the instances noted around the changed lines — to
use the normalized class order.
🪄 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: 8ae9843e-c047-4540-b1f3-b1a0b2ae1752
📒 Files selected for processing (16)
core/src/infra/db/migration/mod.rscore/src/ops/media/thumbnail/action.rscore/src/ops/metadata/manager.rscore/src/ops/tags/apply/action.rscore/src/ops/tags/apply/input.rscore/src/ops/tags/create/input.rscore/src/ops/tags/manager.rspackages/interface/src/components/Inspector/variants/FileInspector.tsxpackages/interface/src/components/SpacesSidebar/TagsGroup.tsxpackages/interface/src/components/Tags/TagSelector.tsxpackages/interface/src/hooks/useRefetchTagQueries.tspackages/interface/src/routes/explorer/TagAssignmentMode.tsxpackages/interface/src/routes/explorer/hooks/useExplorerFiles.tspackages/interface/src/routes/explorer/hooks/useFileContextMenu.tspackages/interface/src/routes/explorer/views/GridView/GridView.tsxpackages/interface/src/routes/overview/OverviewTopBar.tsx
✅ Files skipped from review due to trivial changes (1)
- core/src/infra/db/migration/mod.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- core/src/ops/tags/apply/input.rs
- packages/interface/src/components/Tags/TagSelector.tsx
- core/src/ops/media/thumbnail/action.rs
- packages/interface/src/routes/explorer/hooks/useFileContextMenu.ts
…rialization errors
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 `@core/src/ops/metadata/manager.rs`:
- Around line 292-329: The upsert currently always returns the final Model but
not whether it was an insert or update, causing callers (e.g., in
core/src/ops/tags/apply/action.rs) to treat every result as ChangeType::Insert;
change the upsert flow in manager.rs to determine and return the change kind:
before calling user_metadata_tag::Entity::insert(...).on_conflict(...), query
for existence with
user_metadata_tag::Entity::find().filter(UserMetadataId.eq(...)).filter(TagId.eq(...)).one(...)
and branch—if Some(_) perform an explicit update (use
user_metadata_tag::ActiveModel::from(...) or Entity::update() to set fields and
Exec) and return ChangeType::Update with the model, else perform the insert and
return ChangeType::Insert with the model; update the method signature to return
(Model, ChangeType) so callers in core/src/ops/tags/apply/action.rs can
distinguish insert vs update.
In `@core/src/ops/tags/apply/action.rs`:
- Around line 229-233: The current branch unconditionally returns
ActionError::InvalidInput when successfully_tagged_count == 0 and warnings is
non-empty, which hides real DB/tagging failures; update the logic in
apply/action.rs to separate "missing target" warnings from execution failures
(e.g., maintain missing_targets_count or a missing_targets Vec and a separate
errors Vec), only return InvalidInput("These files need to be indexed before
they can be tagged") when there are zero successes and at least one
missing-target warning and no other execution errors, and otherwise propagate
the actual failure (the original DB/tagging error) so callers see real errors;
ensure the code paths that perform DB/tagging (content/entry branches) continue
to use SeaORM entities with async queries and return/propagate their errors
instead of being swallowed by the generic warning check.
🪄 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: 0e2b0d7c-db1a-41e0-a27d-1155156c7bd0
📒 Files selected for processing (4)
core/src/ops/metadata/manager.rscore/src/ops/tags/apply/action.rscore/src/ops/tags/manager.rspackages/interface/src/components/SpacesSidebar/TagsGroup.tsx
✅ Files skipped from review due to trivial changes (1)
- packages/interface/src/components/SpacesSidebar/TagsGroup.tsx
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 `@apps/tauri/src/platform.ts`:
- Around line 55-57: The code calls ask(...).then(...).catch(...), which causes
callback to be invoked twice when callback itself throws (catch catches both
ask() rejections and callback rejections); replace the promise chain with the
two-argument .then form so the rejection handler only handles ask() failures:
change ask(message, { title: "Spacedrive", kind: "warning" }).then((result) =>
callback(result), () => callback(false)) so that only ask() rejections trigger
the false callback; keep the same callback symbol and message options (this
avoids catching errors thrown inside callback, e.g., from TagsGroup.tsx).
🪄 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: 0d66e6ac-082d-444b-a586-d5f85fed299d
⛔ Files ignored due to path filters (2)
apps/tauri/src-tauri/capabilities/default.jsonis excluded by!**/*.jsonapps/tauri/src-tauri/gen/schemas/capabilities.jsonis excluded by!**/gen/**,!**/*.json,!**/gen/**
📒 Files selected for processing (1)
apps/tauri/src/platform.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
core/src/ops/tags/apply/action.rs (1)
146-149:⚠️ Potential issue | 🟠 MajorDon't collapse stale/null-UUID entries into the “needs indexing” error.
A miss in
entry_id_to_uuidcurrently covers both “entry ID not found” and “entry row hasuuid = NULL”, but the fail-fast block maps both toInvalidInput("These files need to be indexed..."). That is misleading for stale references / integrity issues, and thestarts_with("Failed to tag")check still leaves error classification coupled to warning text. Please track missing targets, null-UUID rows, and execution failures separately, and reserve the indexing message for the real unindexed-target case only.As per coding guidelines, "Use SeaORM entities for database access with async queries and proper error propagation".
Also applies to: 229-241
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/tags/apply/action.rs` around lines 146 - 149, The current lookup of entry_id_to_uuid collapses three distinct cases (missing entry_id key, entry row present but uuid == NULL, and downstream execution/tagging failures) into the same "needs indexing" InvalidInput error; separate those: when entry_id_to_uuid.get(&entry_id) returns None treat as the genuine unindexed-target case and keep the InvalidInput("These files need to be indexed...") only there; when the map has an entry but the UUID is null push a distinct warning (e.g., "Entry X has NULL UUID - possible stale/integrity issue") into warnings; and propagate/tag execution failures separately (do not convert them into the indexing error or rely on starts_with("Failed to tag") text checks). Apply the same separation in the other block referenced around the entry_id UUID handling (the 229-241 area) so missing keys, null-UUID rows, and execution errors are tracked and reported distinctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@core/src/ops/tags/apply/action.rs`:
- Around line 146-149: The current lookup of entry_id_to_uuid collapses three
distinct cases (missing entry_id key, entry row present but uuid == NULL, and
downstream execution/tagging failures) into the same "needs indexing"
InvalidInput error; separate those: when entry_id_to_uuid.get(&entry_id) returns
None treat as the genuine unindexed-target case and keep the InvalidInput("These
files need to be indexed...") only there; when the map has an entry but the UUID
is null push a distinct warning (e.g., "Entry X has NULL UUID - possible
stale/integrity issue") into warnings; and propagate/tag execution failures
separately (do not convert them into the indexing error or rely on
starts_with("Failed to tag") text checks). Apply the same separation in the
other block referenced around the entry_id UUID handling (the 229-241 area) so
missing keys, null-UUID rows, and execution errors are tracked and reported
distinctly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 498098c0-35f9-40e5-a86e-2408f095ce99
📒 Files selected for processing (2)
apps/tauri/src/platform.tscore/src/ops/tags/apply/action.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/tauri/src/platform.ts
Resolve migration registration conflict by keeping both migrations in chronological order: m20260125 (unique user metadata tag) before m20260417 (entries sync cursor index).
Summary
Complete implementation of the tag system: backend handlers, search integration, explorer view, and context menu actions. Also fixes the broken media processing context menu (
useJobDispatch-> direct mutations).Tags backend
search.files:find_entry_ids_for_tag()+resolve_tag_filter()inquery.rstags.by_id,tags.ancestors,tags.children,files.by_tag(returns fullVec<File>)tags.unapply(batch remove tags from entries),tags.delete(delete tag)files.by_tagandsearch.filesuseget_current_device_slug()instead of"unknown-device"fallbackApplyTagsActionreturnsInvalidInputerror when ALL target entries are not found in DB (ephemeral files have in-memory-only UUIDs), instead of silently returning success with 0 taggedTags frontend
TagsGrouprefetches on tag changesuseRefetchTagQueriesfor consistent cache invalidationMedia context menu fix
The context menu's media processing actions (Regenerate Thumbnail, Extract Text, etc.) were broken - they called
jobs.dispatch, a generic RPC endpoint that doesn't exist.Fix: replaced with calls to the existing single-file action endpoints (
media.thumbnail.regenerate,media.ocr.extract, etc.). These actions call theThumbnailProcessor/OcrProcessordirectly, which is the intended pattern for UI-triggered single-file operations.Code review fixes (CodeRabbit R1)
TagSelector.tsxalert()withtoast.error()inuseFileContextMenu.tsconfirm()withuseContextMenuinTagsGroup.tsx(with confirmation guard before destructive delete)type: 'all'touseRefetchTagQueriesfor inactive query refreshas anycasts inOverviewTopBar.tsxthumbnail/action.rsinstead of silently swallowing via.ok().flatten()metadata/manager.rs)apply/input.rsandcreate/input.rs%,_) in tag search fallback (manager.rs)copyFilesmutation fromuseFileContextMenu.tstoast.error()for tag removal failure inFileInspector.tsxCode review fixes (CodeRabbit R2)
apps/tauri/src/platform.ts):platform.confirm()was wrappingwindow.confirm()which silently returnstrueon WebView2 without showing any dialog. Fixed to useask()from@tauri-apps/plugin-dialogon Windows; macOS/Linux keepwindow.confirm()(works fine in those webviews). Addeddialog:allow-asktocapabilities/default.json. TODO: consider usingask()on all platforms for consistent native UX.tags/manager.rs): replacedStatement::from_string+ string interpolation withStatement::from_sql_and_values+?placeholder. Even withescape_fts5_queryescaping, interpolation was vulnerable to single-quote injection.tags/manager.rs): SeaORM's.like()doesn't emit a SQLESCAPEclause, so backslash escapes for%/_were silently ignored. Replaced with raw parameterized SQL includingESCAPE '\\'.lookup_entry_uuidfunction (apply/action.rs): superseded by batch lookup refactor, removed.metadata/manager.rs):serde_json::to_value(...).unwrap()replaced with.map_err(|e| TagError::DatabaseError(...))?.Design choices
file.content_identity === null— the canonical check used throughout the project (Thumb, ThumbstripScrubber, InstancesTab, SidecarsTab)files.by_tag:useNormalizedQueryuses string-based runtime routing (toWireMethod), so the query works without entries intypes.ts. Type annotations useas { files: File[] }cast.tags.deleteandtags.unapplyhave explicitTODO(sync)comments for future cross-device sync. Not in scope here (tracked separately).Tag scope: content-level vs entry-level (needs clarification)
Current behavior: both frontends (FileInspector and TagAssignmentMode) apply tags using
{ type: 'Content', ids: [content_identity_uuid] }. This means a tag is stored against the content identity, not the specific file entry. As a result, all files sharing the same content (duplicates) automatically inherit the tag.The backend fully supports this:
files_by_tag,directory_listing, and resource events all resolve content-scoped tags to every entry sharing that content identity.This is a design decision that should be confirmed. Three approaches are possible:
Content-level (current): "this content is tagged 'vacation'" — all duplicates share the tag. Makes sense for semantic/descriptive tagging (the photo IS a vacation photo regardless of where it's stored). Consistent with Spacedrive's content-addressable philosophy.
Entry-level: "this specific copy is tagged" — each file path has independent tags. Useful for organizational tagging (e.g., tagging files differently in different project folders).
User choice: show a prompt when applying a tag to a file that has duplicates — "Apply to all copies of this file, or just this one?" Gives the most flexibility but adds friction.
The backend already supports both target types (
ContentandEntryUuid), so switching or adding a choice is straightforward. The question is purely about UX and intended semantics.Not done / remaining work
tags.deleteandtags.unapplydon't propagate deletes to peers yet. Tracked in existing sync work.File::from_ephemeral()) always returntags: Vec::new(). Tags applied to indexed files show correctly in indexed directory listings. This is by design — ephemeral files aren't in the DB.resolve_tag_filter: per-tag DB loop when filtering by multiple tags. Acceptable for typical use (1-3 tags) but won't scale to many tags.files_by_tag.rs:i32::to_string()values informat!()— safe (integers can't inject) and matches upstream patterns (search/query.rs), but could use parameterized queries for consistency.Uuid::new_v4()placeholder inmetadata/manager.rs: 6 pre-existing methods (remove_semantic_tags,get_semantic_tags_for_entry,set_favorite, etc.) pass random UUIDs instead of the actual entry UUID parameter. Pre-existing bug, not introduced by this PR, but noted here for awareness.Files changed (40 files)
Backend (17 files):
query.rs,files_by_tag.rs,by_id.rs,ancestors.rs,children.rs,apply/,unapply/,delete/,create/,metadata/manager.rs, migration,tags/mod.rsFrontend (21 files):
useFileContextMenu.ts,useExplorerFiles.ts,context.tsx,ExplorerView.tsx,tag/index.tsx,FileInspector.tsx,TagsGroup.tsx,TagSelector.tsx,Tag.tsx,useRefetchTagQueries.ts,OverviewTopBar.tsx,TagAssignmentMode.tsx,GridView.tsx,generated/types.tsTauri app (2 files):
apps/tauri/src/platform.ts,apps/tauri/src-tauri/capabilities/default.json