feat(tags): tag system with explorer integration, and media context menu fixes#3044
Open
slvnlrt wants to merge 21 commits intospacedriveapp:mainfrom
Open
feat(tags): tag system with explorer integration, and media context menu fixes#3044slvnlrt wants to merge 21 commits intospacedriveapp:mainfrom
slvnlrt wants to merge 21 commits intospacedriveapp:mainfrom
Conversation
WalkthroughAdds multiple tag-management queries and actions (ancestors, by_id, children, files_by_tag, delete, unapply), UUID-based entry targeting for tag application, a migration to dedupe/enforce unique user_metadata_tag pairs, metadata upsert refactor, search tag-filtering, thumbnail MIME fallback, transactional tag deletion, and frontend tag-mode UI/hooks. Changes
Sequence Diagram(s)sequenceDiagram
participant FE as Frontend
participant API as Backend (Action Dispatcher)
participant Action as LibraryAction
participant Manager as Tag/Metadata Manager
participant DB as Database
FE->>API: POST action (tags.apply / tags.unapply) with EntryUuid payload
API->>Action: instantiate & run action
Action->>Manager: resolve metadata, upsert or delete user_metadata_tag
Manager->>DB: SELECT / INSERT ON CONFLICT / DELETE (transaction)
DB-->>Manager: result
Manager-->>Action: operation result
Action-->>API: return action output
API-->>FE: respond
FE->>FE: call useRefetchTagQueries() -> refetch tag/file queries
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 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 |
7 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
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/routes/explorer/hooks/useFileContextMenu.ts (1)
355-388:⚠️ Potential issue | 🟠 MajorFilter the selected set by media type before fan-out.
These handlers now dispatch one mutation per selected file, but the target list is only stripped of virtual entries. Right-clicking an image/video in a mixed selection can still send blurhash/OCR/proxy jobs for directories or incompatible files. Build each action’s target list from
getTargetFiles()plus the matchinggetContentKind()/file.kindpredicate before callingforEachTarget.Also applies to: 403-467, 483-528, 544-574
🤖 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 355 - 388, The handlers currently call getTargetFiles() and fan out mutations with forEachTarget without filtering by media type; update each action (those invoking regenerateThumbnail.mutateAsync, extractText.mutateAsync, proxy/generate jobs, etc.) to build the target list by taking getTargetFiles() and filtering by the appropriate content kind using getContentKind(file) or file.kind (e.g., only image/video for thumbnail/blurhash, only image/pdf for OCR, only files—not directories—for proxy) before passing to forEachTarget so incompatible entries are excluded.core/src/ops/search/query.rs (1)
203-229:⚠️ Potential issue | 🟠 MajorTag filters need to run before final pagination.
In fast/FTS mode,
execute_fts5_search()has already appliedLIMIT/OFFSETbefore the newretain()runs here, so a tag-filtered search can return short pages and skip valid tagged hits just beyond the first slice.get_total_count()also still ignoresfilters.tags, so pagination metadata won’t match the returned items. Push the tag constraint into the query/count path, or over-fetch and paginate after filtering.Also applies to: 948-957
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/search/query.rs` around lines 203 - 229, The FTS tag-filtering is applied after execute_fts5_search’s LIMIT/OFFSET causing truncated pages and mismatched get_total_count; modify the search path so tag constraints are applied before pagination by either (A) extending execute_fts5_search and get_total_count to accept the tags filter and include JOIN/WHERE clauses (or additional FTS query terms) so results and counts respect filters, or (B) over-fetch by calling execute_fts5_search without LIMIT/OFFSET (or with an increased fetch size), apply resolve_tag_filter and retain on fts_results, then apply pagination (slice OFFSET/LIMIT) and compute total from the filtered set; update usages of execute_fts5_search, resolve_tag_filter, fts_results, and get_total_count accordingly so both returned items and total count honor filters.tags.
🧹 Nitpick comments (7)
core/src/ops/media/thumbnail/action.rs (2)
127-149: Database errors are silently swallowed in MIME resolution.The use of
.ok()on lines 128-131 and.ok().flatten()on lines 137-138 converts database errors intoNone, silently falling back to extension-based MIME detection. While the graceful degradation is reasonable for user-facing thumbnail regeneration, this could mask underlying database connectivity issues.Consider at minimum logging a warning when the DB lookup fails unexpectedly:
♻️ Suggested improvement with tracing
let mime_type = if let Some(content_id) = entry.content_id { - if let Ok(Some(ci)) = - entities::content_identity::Entity::find_by_id(content_id) - .one(db) - .await - { + match entities::content_identity::Entity::find_by_id(content_id) + .one(db) + .await + { + Ok(Some(ci)) => { if let Some(mime_id) = ci.mime_type_id { - entities::mime_type::Entity::find_by_id(mime_id) + match entities::mime_type::Entity::find_by_id(mime_id) .one(db) .await - .ok() - .flatten() - .map(|m| m.mime_type) - .or_else(|| mime_from_extension(&path)) + { + Ok(Some(m)) => Some(m.mime_type), + Ok(None) => mime_from_extension(&path), + Err(e) => { + tracing::warn!(?e, "Failed to fetch mime_type, falling back to extension"); + mime_from_extension(&path) + } + } } else { mime_from_extension(&path) } - } else { - mime_from_extension(&path) + } + Ok(None) => mime_from_extension(&path), + Err(e) => { + tracing::warn!(?e, "Failed to fetch content_identity, falling back to extension"); + mime_from_extension(&path) + } } } else { mime_from_extension(&path) };As per coding guidelines: "Use
tracingmacros (info!,debug!,warn!,error!) instead ofprintln!for logging".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/media/thumbnail/action.rs` around lines 127 - 149, The DB lookups in the mime_type resolution (the block that sets mime_type using entities::content_identity::Entity::find_by_id and entities::mime_type::Entity::find_by_id) are swallowing errors via .ok()/.flatten(), so change the logic to detect and log unexpected DB errors with tracing (e.g., warn! or error!) before falling back to mime_from_extension(&path); specifically, handle the Result from .one(db).await and .one(db).await for mime type explicitly, log the Err case with context (content_id or mime_id and path) using tracing macros, and only then return mime_from_extension(&path) on error or missing values while preserving the existing success path that extracts m.mime_type into mime_type.
3-15: Optional: Reorganize imports per coding guidelines.The imports are not grouped according to the guidelines (standard library → external crates → local modules with blank lines between groups). The
std::path::PathBufandstd::sync::Arcimports should be grouped together at the top, followed by external crates (specta,uuid), then local modules.♻️ Suggested import reorganization
-use super::{ - job::{ThumbnailJob, ThumbnailJobConfig}, - processor::ThumbnailProcessor, -}; -use crate::{ - context::CoreContext, - infra::action::{error::ActionError, LibraryAction}, - ops::indexing::{path_resolver::PathResolver, processor::ProcessorEntry}, -}; -use specta::Type; -use std::path::PathBuf; -use std::sync::Arc; -use uuid::Uuid; +use std::path::PathBuf; +use std::sync::Arc; + +use specta::Type; +use uuid::Uuid; + +use super::{ + job::{ThumbnailJob, ThumbnailJobConfig}, + processor::ThumbnailProcessor, +}; +use crate::{ + context::CoreContext, + infra::action::{error::ActionError, LibraryAction}, + ops::indexing::{path_resolver::PathResolver, processor::ProcessorEntry}, +};As per coding guidelines: "Group imports with blank lines: standard library, external crates, then local modules".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/media/thumbnail/action.rs` around lines 3 - 15, Reorder the import blocks in this file to follow the project guideline: group standard library imports together (move std::path::PathBuf and std::sync::Arc to a single std block at the top), then list external crates (specta::Type and uuid::Uuid) in a separate block, and finally the local/module imports (the super::{job::..., processor::ThumbnailProcessor} and crate::{context::CoreContext, infra::action::{error::ActionError, LibraryAction}, ops::indexing::{path_resolver::PathResolver, processor::ProcessorEntry}}) with a blank line between each group; adjust only the use statements near ThumbnailJob/ThumbnailProcessor and CoreContext so the symbols ThumbnailJob, ThumbnailJobConfig, ThumbnailProcessor, CoreContext, ActionError, LibraryAction, PathResolver, ProcessorEntry, Type, and Uuid are preserved but reordered into the three groups.core/src/ops/tags/ancestors.rs (1)
1-64: Split new tag queries into the standard ops module layout.This file already mixes request types, response types, and execution logic together, and the sibling query files added in this PR repeat the same pattern. Keeping those pieces in separate per-feature files will be much easier to extend and test as the tag RPC surface grows.
As per coding guidelines "Feature modules in
src/ops/should containaction.rs,input.rs,output.rs, andjob.rsfiles where applicable".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/tags/ancestors.rs` around lines 1 - 64, The file mixes request/response types and execution logic; split into the ops module layout by moving GetTagAncestorsInput into input.rs, GetTagAncestorsOutput into output.rs, and the query implementation (GetTagAncestorsQuery, its impl including execute, and the crate::register_library_query! call) into action.rs (create job.rs only if background work is needed), then update the module (mod.rs) to pub use those symbols so callers still import GetTagAncestorsQuery, GetTagAncestorsInput, and GetTagAncestorsOutput; ensure the execute method and references to CoreContext, TagManager, and session remain identical when moved.core/src/ops/tags/unapply/output.rs (1)
7-10: Add rustdoc on the public output type and fields.Please document the public contract here with
///comments (including a brief example) to match core module documentation standards.Proposed doc update
#[derive(Debug, Clone, Serialize, Deserialize, Type)] +/// Result payload for `tags.unapply`. +/// +/// Example: removing two tags from three selected entries can return +/// `entries_affected = 3` and `tags_removed = 6`. pub struct UnapplyTagsOutput { + /// Number of unique entries impacted by the unapply operation. pub entries_affected: usize, + /// Total number of tag-entry associations removed. pub tags_removed: usize, + /// Non-fatal issues encountered while processing specific targets. pub warnings: Vec<String>, }As per coding guidelines,
core/**/*.rs: "Use//!for module documentation,///for public items, and include examples".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/tags/unapply/output.rs` around lines 7 - 10, Add Rustdoc comments to the public struct UnapplyTagsOutput and each of its public fields (entries_affected, tags_removed, warnings) describing their meaning and expected units/types; include a short example usage showing how to inspect the output (e.g., call an unapply function and check entries_affected/tags_removed) and any guarantees (e.g., warnings contain human-readable messages). Use /// comments above the struct and above each field to match core module standards and include a brief example snippet in the struct-level doc.packages/interface/src/components/Tags/TagSelector.tsx (1)
44-50: Remove theanyescape hatch from the tag-search projection.This is on the cache-normalization path, so a
tags.searchresponse-shape drift will compile and fail at runtime. Please binduseNormalizedQueryto the generated output type and narrowresultinstead of casting both values toany.As per coding guidelines, "Never use
anytype in TypeScript - useunknownwith 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/components/Tags/TagSelector.tsx` around lines 44 - 50, The select projection for useNormalizedQuery bound to 'tags.search' uses any for both the response and result which bypasses TS checks; update the useNormalizedQuery call to be generically typed with the generated response type for 'tags.search' and replace the any casts in the select callback by narrowing the union (e.g., type-guard or discriminant check) on the result so you safely access result.tag when present and otherwise return result, ensuring the select returns the filtered array (refer to useNormalizedQuery, the select callback, the 'tags.search' query and the result.tag shape).packages/interface/src/routes/explorer/hooks/useExplorerFiles.ts (1)
146-150: Type thefiles.by_tagquery instead of casting its data toany.Tag mode now feeds the main explorer file list, so this unchecked cast removes compile-time protection for one of the core data sources.
useNormalizedQueryalready uses explicit generics earlier in this hook; doing the same here would let you readtagQuery.data.filesdirectly withoutany.As per coding guidelines, "Never use
anytype in TypeScript - useunknownwith type guards if needed".Also applies to: 181-195
🤖 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 146 - 150, The tagQuery call uses an unchecked any cast for the "files.by_tag" response; replace that by supplying the correct generic type to useNormalizedQuery so tagQuery is strongly typed (e.g., the response shape that includes files), remove the any casts where tagQuery.data.files is accessed, and update the other occurrences around the hook (the similar usage at the block referenced as 181-195) to use the same typed useNormalizedQuery so you can read tagQuery.data.files directly with compile-time safety.packages/interface/src/components/SpacesSidebar/TagsGroup.tsx (1)
120-126: Remove theanyescape hatch from the tag query select.
tags.searchalready has a generated result shape, so this TODO throws away the type-safety the hook is supposed to provide and will hide schema drift in the sidebar. Type the callback against the generated search types (or useunknown+ a guard) and drop theanys.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/components/SpacesSidebar/TagsGroup.tsx` around lines 120 - 126, The select callback for useNormalizedQuery (query: 'tags.search') uses `any`, removing type-safety; replace the `any` types in the select parameter with the generated response type for the tags.search endpoint (or use `unknown` and add a runtime type guard) and update the select body to use that typed shape to extract `data.tags[].tag` (or the fallback) without `any`; update or remove the // TODO comment and ensure the signature of the select callback matches the generated types used by useNormalizedQuery.
🤖 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/infra/db/migration/m20260125_000001_unique_user_metadata_tag.rs`:
- Around line 11-18: The current dedupe SQL deletes all but MIN(id) in
user_metadata_tag, which preserves the oldest row and loses newer mutable state;
change the logic to keep the newest tag application instead—use MAX(id) (or, if
available, pick the row with greatest version or latest updated_at) in the
subquery that groups by user_metadata_id, tag_id so the DELETE removes rows NOT
IN (SELECT MAX(id) ...) or otherwise selects the row with highest
version/updated_at; update the DELETE statement that references
user_metadata_tag and the MIN(id) subselect accordingly to retain the latest
payload.
In `@core/src/ops/metadata/manager.rs`:
- Around line 266-299: The code that upserts user metadata tags (the block
building and saving user_metadata_tag::ActiveModel when `existing` is Some vs
None) currently returns only the saved model and hides whether it was an Insert
or Update; change the method return to include the change kind (e.g., an enum or
ChangeType) so callers can distinguish paths. Modify the upsert logic around
`existing`, `update_model.update(&*db).await...` and the
`.insert(&*db).await...` branches to return a tuple or struct containing the
persisted user_metadata_tag record and a change marker (e.g., ChangeType::Update
for the update branch and ChangeType::Insert for the insert branch), update the
function signature and any call sites (notably
core/src/ops/tags/apply/action.rs) to handle the new return shape so sync events
can emit Update vs Insert correctly.
- Around line 258-299: The current find-then-insert TOCTOU around
user_metadata_tag::Entity::find() and user_metadata_tag::ActiveModel::insert()
must be made atomic: wrap the logic in a DB transaction (begin_transaction /
transaction closure) and perform an upsert using SQL ON CONFLICT
(user_metadata_id, tag_id) DO UPDATE that sets applied_context, applied_variant,
confidence, source, instance_attributes, updated_at, device_uuid and increments
version, or alternatively catch the unique-violation error from
ActiveModel::insert(), then in the same transaction re-query the row and perform
the update on the resulting user_metadata_tag::ActiveModel; ensure you reference
and update the same fields
(applied_context/applied_variant/confidence/source/instance_attributes/updated_at/device_uuid/version)
and return the final model.
In `@core/src/ops/tags/apply/action.rs`:
- Around line 169-199: In the TagTargets::EntryUuid branch, validate each
entry_uuid against the entry table before calling
metadata_manager.apply_semantic_tags_to_entry: for each entry_uuid, attempt to
resolve it via the entry entity lookup (e.g., Entry::find /
entry::Entity::find_by_id or the existing repository method you have) and if not
found push a warning like "Skipped tagging missing entry {entry_uuid}" and
continue; only call metadata_manager.apply_semantic_tags_to_entry,
library.sync_model, increment successfully_tagged_count, and push to
affected_entry_uuids when the entry lookup succeeds to avoid creating orphaned
user_metadata and emitting sync events for non-existent files.
In `@core/src/ops/tags/children.rs`:
- Around line 55-58: The current implementation uses
manager.get_descendants(self.input.tag_id) which returns all deeper descendants
(grandchildren, etc.) instead of only direct children; change the call to the
manager method that returns only immediate children (e.g., manager.get_children
or manager.get_direct_children) or perform a query/filter that selects nodes
whose parent_id == self.input.tag_id, keeping the same error mapping to
QueryError::Internal(...) so the returned `children` variable contains only
direct children for the tags.children RPC.
In `@core/src/ops/tags/delete/action.rs`:
- Around line 33-39: The cascade deletes in TagManager::delete_tag are executed
as separate delete_many() calls and must be made atomic: modify
TagManager::delete_tag (the method on struct TagManager that currently
orchestrates the delete_many() calls) to begin a database transaction, perform
all dependent deletes and the final tag deletion inside that transaction, and
commit at the end (rolling back on any error) so partial deletions cannot
persist; use the DB connection's transaction API (e.g., conn.transaction(...) or
begin/commit/rollback) around the sequence referenced in the delete_many() calls
(lines ~360-419) so errors propagate and trigger rollback.
- Around line 41-42: The delete action currently skips cross-device sync (see
TODO) causing divergence; in the tags::delete handler (action.rs) call the
existing sync_model function with ChangeType::Delete when a tag is removed,
ensuring you import or reference ChangeType::Delete, await the sync_model call
(or handle its Result) and propagate/log any errors; remove the placeholder TODO
comment and, if necessary, create a GitHub issue reference instead of leaving
the inline TODO so deletions are propagated to peers consistently.
In `@core/src/ops/tags/files_by_tag.rs`:
- Around line 285-304: The code currently uses tags_by_entry.insert(...) and
.or_insert_with(...) which makes first-wins behavior and prevents merging
content-scoped tags into an existing entry-scoped tag list; update the logic in
the loop over metadata_records (symbols: metadata_records, tags_by_metadata,
metadata.entry_uuid, metadata.content_identity_uuid, rows, tags_by_entry) so
that when adding tags for an entry you merge/extend into an existing Vec/set
instead of replacing it: for entry-scoped (metadata.entry_uuid) and
content-scoped (inside the rows loop for content_identity_uuid) use
tags_by_entry.entry(eu).and_modify(|existing| merge existing with
tags).or_insert(tags.clone()) (or equivalent merge that deduplicates if needed)
so both metadata scopes contribute to the final tags for a file.
- Around line 131-150: The current decoding in files_by_tag.rs fabricates
critical File fields on failure (e.g. entry_id, entry_kind, entry_created_at,
entry_modified_at) by using unwrap_or/unwrap_or_else; instead, make these
mandatory and propagate the error from row.try_get so callers fail fast: replace
the unwrap_or/unwrap_or_else calls for required fields (entry_id, entry_uuid if
required, entry_kind, entry_name if required, entry_size, entry_aggregate_size,
entry_child_count, entry_file_count, entry_created_at, entry_modified_at) with
fallible returns (use row.try_get(...)? or map_err to return the error) and only
keep .ok() for truly optional fields (entry_extension, entry_accessed_at,
content_kind_name, parent_path, device_slug). Ensure the decoder/signature (the
function surrounding these rows) returns a Result so the error can be propagated
rather than constructing fake values.
- Around line 21-26: GetFilesByTagInput's include_children and min_confidence
are never used: update the files-by-tag logic so the tag resolution that
currently matches only direct tag_id (the code around the direct match
resolution) expands to include descendant/child tag IDs when
GetFilesByTagInput.include_children is true (use the tag hierarchy
lookup/recursive query or Tag children relation to produce a list of tag IDs),
and apply GetFilesByTagInput.min_confidence when joining/filtering taggings (or
the equivalent confidence column) so only taggings with confidence >=
min_confidence are returned. Modify the query/filters in the function that
produces the result set (the files-by-tag handler/query logic that currently
ignores include_children and min_confidence) to accept the expanded tag ID list
and add a confidence predicate, and update any tests to cover both
include_children true/false and min_confidence thresholds.
In `@core/src/ops/tags/unapply/action.rs`:
- Around line 66-89: The current logic discovers additional user_metadata IDs
via the content_identity path (variables entries → content_ids → cis → ci_uuids
→ um_by_content → um_ids) but still computes entries_affected and emits events
only from self.input.entry_ids, under-reporting mutations; modify the flow to
first resolve the full set of affected entry UUIDs (start from
self.input.entry_ids, then expand to sibling entries that share content_identity
by querying entry::Entity for entries whose content_id matches the discovered
content_ids or whose content_identity.uuid matches ci_uuids), deduplicate into a
single affected_entries set, and then use that set for both entries_affected and
all emitted resource events (apply the same change where the symmetric
content-identity fan-out logic repeats around the later block referencing
um_ids/entries_affected).
In `@packages/interface/src/components/Inspector/primitives/Tag.tsx`:
- Around line 32-42: The remove control button in the Tag component is missing
an explicit type and therefore defaults to "submit" inside forms; update the
<button> element used when onRemove is present in
packages/interface/src/components/Inspector/primitives/Tag.tsx to include
type="button" so clicking the remove control won't submit parent forms (locate
the JSX block that calls onRemove and renders the X icon).
In `@packages/interface/src/components/Inspector/variants/FileInspector.tsx`:
- Around line 634-645: The optimistic tag updater is only replacing the
closed-over selectedFiles snapshot via updateSelectedFileTags and so leaves the
inspector (which renders from file.tags via fileQuery.data || file) stale and
risks clobbering newer selections; change updateSelectedFileTags to (1) update
the source-of-truth query/state you render from (e.g. update the file/query data
used by the inspector such as fileQuery.setData or the library query that
contains file.tags) and (2) if you still need to sync selection, use the
functional form setSelectedFiles(prev => ...) to merge tags into the
selectedFiles list without overwriting concurrent changes; apply the same fixes
to the other occurrence noted (lines ~920-955) and keep onSuccess
refetchTagQueries for backend reconciliation.
In `@packages/interface/src/components/SpacesSidebar/TagsGroup.tsx`:
- Around line 42-57: Replace the browser confirm/alert flow inside
handleContextMenu with the app-wide useContextMenu pattern: remove confirm() and
alert() and instead invoke the existing useContextMenu (or create a tag-specific
wrapper modeled on useVolumeContextMenu / useSpaceItemContextMenu) to open a
menu/action with variant: "danger" that, when confirmed, calls
deleteTag.mutateAsync({ tag_id: tag.id }); preserve the post-delete behavior (if
isActive then navigate('/')), and ensure errors are routed to the app logger/UI
rather than alert(). Update any other direct confirm/alert uses in this
component (the other deletion branch lines) to the same useContextMenu pattern.
In `@packages/interface/src/routes/explorer/context.tsx`:
- Around line 266-276: The reducer cases for ENTER_TAG_MODE and EXIT_TAG_MODE
only update state.mode but not the boolean state.tagModeActive, leaving
consumers reading tagModeActive out of sync; update the reducer returns for the
ENTER_TAG_MODE and EXIT_TAG_MODE branches (and any other similar branches like
the other ENTER_/EXIT_ tag-mode cases) to set tagModeActive: true when entering
tag mode and tagModeActive: false when exiting, ensuring both mode (mode.type
=== "tag") and tagModeActive reflect the same state; locate the cases by the
action names ENTER_TAG_MODE and EXIT_TAG_MODE in the reducer and add the
corresponding tagModeActive property to the returned state objects.
---
Outside diff comments:
In `@core/src/ops/search/query.rs`:
- Around line 203-229: The FTS tag-filtering is applied after
execute_fts5_search’s LIMIT/OFFSET causing truncated pages and mismatched
get_total_count; modify the search path so tag constraints are applied before
pagination by either (A) extending execute_fts5_search and get_total_count to
accept the tags filter and include JOIN/WHERE clauses (or additional FTS query
terms) so results and counts respect filters, or (B) over-fetch by calling
execute_fts5_search without LIMIT/OFFSET (or with an increased fetch size),
apply resolve_tag_filter and retain on fts_results, then apply pagination (slice
OFFSET/LIMIT) and compute total from the filtered set; update usages of
execute_fts5_search, resolve_tag_filter, fts_results, and get_total_count
accordingly so both returned items and total count honor filters.tags.
In `@packages/interface/src/routes/explorer/hooks/useFileContextMenu.ts`:
- Around line 355-388: The handlers currently call getTargetFiles() and fan out
mutations with forEachTarget without filtering by media type; update each action
(those invoking regenerateThumbnail.mutateAsync, extractText.mutateAsync,
proxy/generate jobs, etc.) to build the target list by taking getTargetFiles()
and filtering by the appropriate content kind using getContentKind(file) or
file.kind (e.g., only image/video for thumbnail/blurhash, only image/pdf for
OCR, only files—not directories—for proxy) before passing to forEachTarget so
incompatible entries are excluded.
---
Nitpick comments:
In `@core/src/ops/media/thumbnail/action.rs`:
- Around line 127-149: The DB lookups in the mime_type resolution (the block
that sets mime_type using entities::content_identity::Entity::find_by_id and
entities::mime_type::Entity::find_by_id) are swallowing errors via
.ok()/.flatten(), so change the logic to detect and log unexpected DB errors
with tracing (e.g., warn! or error!) before falling back to
mime_from_extension(&path); specifically, handle the Result from .one(db).await
and .one(db).await for mime type explicitly, log the Err case with context
(content_id or mime_id and path) using tracing macros, and only then return
mime_from_extension(&path) on error or missing values while preserving the
existing success path that extracts m.mime_type into mime_type.
- Around line 3-15: Reorder the import blocks in this file to follow the project
guideline: group standard library imports together (move std::path::PathBuf and
std::sync::Arc to a single std block at the top), then list external crates
(specta::Type and uuid::Uuid) in a separate block, and finally the local/module
imports (the super::{job::..., processor::ThumbnailProcessor} and
crate::{context::CoreContext, infra::action::{error::ActionError,
LibraryAction}, ops::indexing::{path_resolver::PathResolver,
processor::ProcessorEntry}}) with a blank line between each group; adjust only
the use statements near ThumbnailJob/ThumbnailProcessor and CoreContext so the
symbols ThumbnailJob, ThumbnailJobConfig, ThumbnailProcessor, CoreContext,
ActionError, LibraryAction, PathResolver, ProcessorEntry, Type, and Uuid are
preserved but reordered into the three groups.
In `@core/src/ops/tags/ancestors.rs`:
- Around line 1-64: The file mixes request/response types and execution logic;
split into the ops module layout by moving GetTagAncestorsInput into input.rs,
GetTagAncestorsOutput into output.rs, and the query implementation
(GetTagAncestorsQuery, its impl including execute, and the
crate::register_library_query! call) into action.rs (create job.rs only if
background work is needed), then update the module (mod.rs) to pub use those
symbols so callers still import GetTagAncestorsQuery, GetTagAncestorsInput, and
GetTagAncestorsOutput; ensure the execute method and references to CoreContext,
TagManager, and session remain identical when moved.
In `@core/src/ops/tags/unapply/output.rs`:
- Around line 7-10: Add Rustdoc comments to the public struct UnapplyTagsOutput
and each of its public fields (entries_affected, tags_removed, warnings)
describing their meaning and expected units/types; include a short example usage
showing how to inspect the output (e.g., call an unapply function and check
entries_affected/tags_removed) and any guarantees (e.g., warnings contain
human-readable messages). Use /// comments above the struct and above each field
to match core module standards and include a brief example snippet in the
struct-level doc.
In `@packages/interface/src/components/SpacesSidebar/TagsGroup.tsx`:
- Around line 120-126: The select callback for useNormalizedQuery (query:
'tags.search') uses `any`, removing type-safety; replace the `any` types in the
select parameter with the generated response type for the tags.search endpoint
(or use `unknown` and add a runtime type guard) and update the select body to
use that typed shape to extract `data.tags[].tag` (or the fallback) without
`any`; update or remove the // TODO comment and ensure the signature of the
select callback matches the generated types used by useNormalizedQuery.
In `@packages/interface/src/components/Tags/TagSelector.tsx`:
- Around line 44-50: The select projection for useNormalizedQuery bound to
'tags.search' uses any for both the response and result which bypasses TS
checks; update the useNormalizedQuery call to be generically typed with the
generated response type for 'tags.search' and replace the any casts in the
select callback by narrowing the union (e.g., type-guard or discriminant check)
on the result so you safely access result.tag when present and otherwise return
result, ensuring the select returns the filtered array (refer to
useNormalizedQuery, the select callback, the 'tags.search' query and the
result.tag shape).
In `@packages/interface/src/routes/explorer/hooks/useExplorerFiles.ts`:
- Around line 146-150: The tagQuery call uses an unchecked any cast for the
"files.by_tag" response; replace that by supplying the correct generic type to
useNormalizedQuery so tagQuery is strongly typed (e.g., the response shape that
includes files), remove the any casts where tagQuery.data.files is accessed, and
update the other occurrences around the hook (the similar usage at the block
referenced as 181-195) to use the same typed useNormalizedQuery so you can read
tagQuery.data.files directly with compile-time safety.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8f3f27fe-8bd4-4267-ae40-0006c6447f88
⛔ Files ignored due to path filters (1)
packages/ts-client/src/generated/types.tsis excluded by!**/generated/**,!**/generated/**
📒 Files selected for processing (33)
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/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
core/src/infra/db/migration/m20260125_000001_unique_user_metadata_tag.rs
Outdated
Show resolved
Hide resolved
packages/interface/src/components/Inspector/variants/FileInspector.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
core/src/ops/metadata/manager.rs (1)
299-308:⚠️ Potential issue | 🟠 MajorReturn whether each upserted row was inserted or updated.
This method now returns models from both upsert branches, but
core/src/ops/tags/apply/action.rs:77-116still syncs every returned model asChangeType::Insert. Reapplying an existing tag will therefore publish the wrong sync mutation unless the return value carries the change kind.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/metadata/manager.rs` around lines 299 - 308, The upsert re-query in manager.rs currently always pushes models into created_models without indicating whether the row was inserted or updated, causing apply/action.rs to treat every result as Insert; modify the upsert path in the function that builds created_models (the block around user_metadata_tag::Entity::find() and created_models.push(model)) to record the change kind alongside the model (e.g., push a tuple or a small struct of (model, ChangeType)) by detecting whether the operation did an insert vs update in the upsert branches, update the function return type to return Vec<(UserMetadataTagModel, ChangeType)> (or equivalent), and adjust callers such as core/src/ops/tags/apply/action.rs to consume the new pair and use ChangeType::Insert or ChangeType::Update when syncing.
🧹 Nitpick comments (1)
core/src/ops/tags/delete/action.rs (1)
41-45: Replace TODO comment with a GitHub issue reference.As per coding guidelines: "Never use placeholder comments like 'TODO: extract this later' or 'for now'; track future work in GitHub issues instead."
While this comment is detailed and explains the limitation well, consider creating a GitHub issue to track the sync-deletion work and reference it here with a brief note like:
- // TODO(sync): Tag deletion is not synced to other devices. - // The sync infrastructure supports ChangeType::Delete but the tag deletion - // path does not yet call library.sync_model() with it. This means deleted - // tags will reappear on other devices after sync. Tracked for a dedicated - // sync-deletion PR. + // Note: Tag deletion sync not yet implemented. See issue `#XXXX`.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/tags/delete/action.rs` around lines 41 - 45, Replace the inline TODO comment about unsynced tag deletion with a short reference to a created GitHub issue (e.g., "See issue #<N>") instead of leaving the placeholder; create an issue tracking the work to call library.sync_model() with ChangeType::Delete for the tag deletion path, then update the comment in action.rs to a single-line note referencing that issue and remove the "TODO" block.
🤖 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 275-290: The conflict-update path currently updates several
columns but omits the version field, so include
user_metadata_tag::Column::Version in the OnConflict update and make it
increment (not set to a constant) so re-applies bump the version; modify the
OnConflict::update_columns/exprs in the upsert code in manager.rs (the block
constructing OnConflict for user_metadata_tag) to update Version (e.g., use an
expression that sets Version = Version + 1 or the equivalent sea-query/sea-orm
increment expression) alongside the other updated columns.
In `@core/src/ops/tags/delete/action.rs`:
- Around line 47-57: The resource emission happens after the tag is deleted so
from_ids() returns nothing; move the call to
ResourceManager::emit_resource_events (or call a specialized deletion-only
event) to run before the deletion in the delete action (action.rs) so the event
is emitted with the tag ID; specifically, invoke
resource_manager.emit_resource_events("tag", vec![self.input.tag_id]) (or use a
"tag:deleted" deletion-only event variant) prior to calling the deletion code so
frontend cache invalidation is guaranteed even though the DB row will be
removed.
In `@core/src/ops/tags/unapply/action.rs`:
- Around line 110-114: The unapply action currently deletes rows from
user_metadata_tag locally but does not publish a sync deletion/tombstone, so
other devices can resurrect the tag; update the tag unapply logic (the function
in action.rs that removes user_metadata_tag rows) to emit a sync deletion for
the tag (same pattern used in delete/action.rs), e.g. create a
ChangeType::Delete/tombstone for the tag and invoke the library sync publish
path (library.sync_model() or the project's sync-publish helper) so the deletion
is propagated to other devices.
- Around line 116-149: The current logic builds all_affected_uuids from
self.input.entry_ids and content-siblings even when no tags were actually
deleted, causing overreporting; change the flow in the UnapplyTags action
(action.rs) so you either short-circuit and return early when total_removed ==
0, or (preferred) build all_affected_uuids from the actual rows selected for
deletion (use the UUIDs from the query that produced the user_metadata_tag rows
to delete and their content-id siblings) and then emit_resource_events("file",
affected_vec) and set entries_affected based on that set; update any uses of
all_affected_uuids, resource_manager, emit_resource_events and UnapplyTagsOutput
accordingly so only entries that actually lost a tag are reported and notified.
---
Duplicate comments:
In `@core/src/ops/metadata/manager.rs`:
- Around line 299-308: The upsert re-query in manager.rs currently always pushes
models into created_models without indicating whether the row was inserted or
updated, causing apply/action.rs to treat every result as Insert; modify the
upsert path in the function that builds created_models (the block around
user_metadata_tag::Entity::find() and created_models.push(model)) to record the
change kind alongside the model (e.g., push a tuple or a small struct of (model,
ChangeType)) by detecting whether the operation did an insert vs update in the
upsert branches, update the function return type to return
Vec<(UserMetadataTagModel, ChangeType)> (or equivalent), and adjust callers such
as core/src/ops/tags/apply/action.rs to consume the new pair and use
ChangeType::Insert or ChangeType::Update when syncing.
---
Nitpick comments:
In `@core/src/ops/tags/delete/action.rs`:
- Around line 41-45: Replace the inline TODO comment about unsynced tag deletion
with a short reference to a created GitHub issue (e.g., "See issue #<N>")
instead of leaving the placeholder; create an issue tracking the work to call
library.sync_model() with ChangeType::Delete for the tag deletion path, then
update the comment in action.rs to a single-line note referencing that issue and
remove the "TODO" block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f66ca8f0-3de2-4226-9d53-2a465da1ce1b
📒 Files selected for processing (8)
core/src/infra/db/migration/m20260125_000001_unique_user_metadata_tag.rscore/src/ops/metadata/manager.rscore/src/ops/tags/children.rscore/src/ops/tags/delete/action.rscore/src/ops/tags/files_by_tag.rscore/src/ops/tags/manager.rscore/src/ops/tags/unapply/action.rspackages/interface/src/routes/explorer/context.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/interface/src/routes/explorer/context.tsx
- core/src/ops/tags/files_by_tag.rs
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)
packages/interface/src/routes/explorer/context.tsx (1)
609-618:⚠️ Potential issue | 🟠 MajorUnify mode reset across all navigation paths.
navigateToPathclears tag mode (Line 604), butnavigateToView/goBack/goForwardonly clear search mode (Lines 615, 627, 638). This leavesmode.type === "tag"reachable after non-path navigation and can drive the wrong data mode in consumers keyed offmode.type.Suggested fix
+const exitSpecialModes = useCallback(() => { + uiDispatch({ type: "EXIT_SEARCH_MODE" }); + uiDispatch({ type: "EXIT_TAG_MODE" }); +}, []); + const navigateToPath = useCallback( (path: SdPath) => { const target: NavigationTarget = { type: "path", path }; navDispatch({ type: "NAVIGATE", target }); routerNavigate(targetToUrl(target)); - // Exit special modes when navigating to a path - uiDispatch({ type: "EXIT_SEARCH_MODE" }); - uiDispatch({ type: "EXIT_TAG_MODE" }); + exitSpecialModes(); }, - [routerNavigate], + [routerNavigate, exitSpecialModes], ); const navigateToView = useCallback( (view: string, id?: string, params?: Record<string, string>) => { const target: NavigationTarget = { type: "view", view, id, params }; navDispatch({ type: "NAVIGATE", target }); routerNavigate(targetToUrl(target)); - // Exit search mode when navigating - uiDispatch({ type: "EXIT_SEARCH_MODE" }); + exitSpecialModes(); }, - [routerNavigate], + [routerNavigate, exitSpecialModes], ); const goBack = useCallback(() => { navDispatch({ type: "GO_BACK" }); const targetIndex = navState.index - 1; if (targetIndex >= 0) { const target = navState.history[targetIndex]; routerNavigate(targetToUrl(target), { replace: true }); - // Exit search mode when navigating - uiDispatch({ type: "EXIT_SEARCH_MODE" }); + exitSpecialModes(); } -}, [navState.index, navState.history, routerNavigate]); +}, [navState.index, navState.history, routerNavigate, exitSpecialModes]); const goForward = useCallback(() => { navDispatch({ type: "GO_FORWARD" }); const targetIndex = navState.index + 1; if (targetIndex < navState.history.length) { const target = navState.history[targetIndex]; routerNavigate(targetToUrl(target), { replace: true }); - // Exit search mode when navigating - uiDispatch({ type: "EXIT_SEARCH_MODE" }); + exitSpecialModes(); } -}, [navState.index, navState.history, routerNavigate]); +}, [navState.index, navState.history, routerNavigate, exitSpecialModes]);Also applies to: 620-640
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/interface/src/routes/explorer/context.tsx` around lines 609 - 618, navigateToView, goBack, and goForward only exit search mode while navigateToPath also clears tag mode, leaving mode.type === "tag" active after non-path navigation; update navigateToView, goBack, and goForward to perform the same mode reset as navigateToPath (either dispatch the same tag-mode-clearing action used in navigateToPath or replace both search/tag clears with the unified RESET_MODE action) by adding the matching uiDispatch call(s) alongside the existing EXIT_SEARCH_MODE dispatch so all navigation paths consistently reset mode.type.
🤖 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 `@packages/interface/src/routes/explorer/context.tsx`:
- Around line 609-618: navigateToView, goBack, and goForward only exit search
mode while navigateToPath also clears tag mode, leaving mode.type === "tag"
active after non-path navigation; update navigateToView, goBack, and goForward
to perform the same mode reset as navigateToPath (either dispatch the same
tag-mode-clearing action used in navigateToPath or replace both search/tag
clears with the unified RESET_MODE action) by adding the matching uiDispatch
call(s) alongside the existing EXIT_SEARCH_MODE dispatch so all navigation paths
consistently reset mode.type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 976bc743-e12f-4f48-ae4e-2f478c040b6c
📒 Files selected for processing (1)
packages/interface/src/routes/explorer/context.tsx
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/interface/src/routes/explorer/context.tsx (1)
266-276: Consider consolidatingtagModeActivewithmode.type === "tag".There are now two mechanisms for tracking tag mode:
tagModeActiveboolean (viaSET_TAG_MODEaction)mode.type === "tag"(viaENTER_TAG_MODE/EXIT_TAG_MODEactions)This duplication could confuse consumers—calling
enterTagMode()leavestagModeActive === false, and callingsetTagModeActive(true)leavesmode.type === "browse".If
tagModeActiveis legacy state, consider deprecating it in favor of deriving the boolean frommode.type === "tag". If both are needed for different purposes, document the distinction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/interface/src/routes/explorer/context.tsx` around lines 266 - 276, The reducer currently tracks tag mode in two places (the boolean tagModeActive via SET_TAG_MODE / setTagModeActive() and the mode object via ENTER_TAG_MODE / EXIT_TAG_MODE / enterTagMode()), causing inconsistent state; pick one canonical source and unify: either remove tagModeActive and any SET_TAG_MODE/setTagModeActive() usages and derive the boolean as mode.type === "tag" everywhere, or update ENTER_TAG_MODE/EXIT_TAG_MODE to also set tagModeActive so both stay in sync and deprecate the other approach with comments—search for tagModeActive, SET_TAG_MODE, setTagModeActive(), ENTER_TAG_MODE, EXIT_TAG_MODE, enterTagMode(), and mode.type to update all callers and tests accordingly.core/src/ops/metadata/manager.rs (1)
255-259: Consider replacingunwrap()withunwrap_or_default()for defensive coding.While serializing a
HashMap<String, serde_json::Value>to JSON is infallible in practice, usingunwrap()could panic if an unexpected type is introduced in the future. A safer alternative:🛡️ Defensive alternative
let instance_attributes_value = if app.instance_attributes.is_empty() { None } else { - Some(serde_json::to_value(&app.instance_attributes).unwrap().into()) + serde_json::to_value(&app.instance_attributes) + .ok() + .map(|v| v.into()) };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/metadata/manager.rs` around lines 255 - 259, The code uses serde_json::to_value(&app.instance_attributes).unwrap() when building instance_attributes_value, which can panic; change this to a non-panicking fallback by replacing unwrap() with unwrap_or_default() (i.e., serde_json::to_value(&app.instance_attributes).unwrap_or_default().into()) or otherwise map the Result to a default JSON value so instance_attributes_value is Some(default) instead of triggering a panic; update the expression around instance_attributes_value and its use with app.instance_attributes and serde_json::to_value accordingly.core/src/ops/tags/unapply/action.rs (1)
117-131: Affected entries set may include entries that didn't actually lose a tag.When
total_removed > 0, the code notifies all input entries plus all content-sharing siblings, even if only a subset had the tag applied. This is a conservative approach ensuring UI consistency, but could cause unnecessary refreshes.If precise reporting is desired, you could query the
user_metadata_ids from the deleted rows before deletion (using a SELECT with the same filters), then resolve only those to entry UUIDs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/tags/unapply/action.rs` around lines 117 - 131, The current logic builds all_affected_uuids from self.input.entry_ids and all entries with matching content_ids (using entry::Entity::find() and entry::Column::ContentId) whenever total_removed > 0, which can include entries that never had the specific tag removed; instead, before performing the DELETE, run a SELECT for the same filters to collect the exact user_metadata_id values (the rows that will be deleted), then resolve those user_metadata_id -> entry UUIDs and use that resulting set to populate all_affected_uuids so only entries that actually lost metadata are notified; reference the same filter criteria used for deletion and replace the current content-scoped extension logic that blindly includes all siblings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core/src/ops/metadata/manager.rs`:
- Around line 255-259: The code uses
serde_json::to_value(&app.instance_attributes).unwrap() when building
instance_attributes_value, which can panic; change this to a non-panicking
fallback by replacing unwrap() with unwrap_or_default() (i.e.,
serde_json::to_value(&app.instance_attributes).unwrap_or_default().into()) or
otherwise map the Result to a default JSON value so instance_attributes_value is
Some(default) instead of triggering a panic; update the expression around
instance_attributes_value and its use with app.instance_attributes and
serde_json::to_value accordingly.
In `@core/src/ops/tags/unapply/action.rs`:
- Around line 117-131: The current logic builds all_affected_uuids from
self.input.entry_ids and all entries with matching content_ids (using
entry::Entity::find() and entry::Column::ContentId) whenever total_removed > 0,
which can include entries that never had the specific tag removed; instead,
before performing the DELETE, run a SELECT for the same filters to collect the
exact user_metadata_id values (the rows that will be deleted), then resolve
those user_metadata_id -> entry UUIDs and use that resulting set to populate
all_affected_uuids so only entries that actually lost metadata are notified;
reference the same filter criteria used for deletion and replace the current
content-scoped extension logic that blindly includes all siblings.
In `@packages/interface/src/routes/explorer/context.tsx`:
- Around line 266-276: The reducer currently tracks tag mode in two places (the
boolean tagModeActive via SET_TAG_MODE / setTagModeActive() and the mode object
via ENTER_TAG_MODE / EXIT_TAG_MODE / enterTagMode()), causing inconsistent
state; pick one canonical source and unify: either remove tagModeActive and any
SET_TAG_MODE/setTagModeActive() usages and derive the boolean as mode.type ===
"tag" everywhere, or update ENTER_TAG_MODE/EXIT_TAG_MODE to also set
tagModeActive so both stay in sync and deprecate the other approach with
comments—search for tagModeActive, SET_TAG_MODE, setTagModeActive(),
ENTER_TAG_MODE, EXIT_TAG_MODE, enterTagMode(), and mode.type to update all
callers and tests accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e4300ec1-3cb1-4b89-a00c-1d6c3b5b88b3
📒 Files selected for processing (3)
core/src/ops/metadata/manager.rscore/src/ops/tags/unapply/action.rspackages/interface/src/routes/explorer/context.tsx
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/src/ops/tags/files_by_tag.rs (1)
321-342: Index rows bycontent_identity_uuidbefore this merge.This branch rescans every SQL row for each content-scoped metadata record. On popular tags, hydration becomes
O(metadata_records × rows)on top of the existing queries, which is likely to show up in Explorer tag views. Precomputing acontent_identity_uuid -> Vec<entry_uuid>map once would keep this step linear.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/tags/files_by_tag.rs` around lines 321 - 342, The current loop under the branch checking metadata.content_identity_uuid rescans all SQL rows for each metadata record, causing O(metadata_records × rows) work; instead, precompute an index mapping content_identity_uuid to Vec<entry_uuid> once and use it to populate tags_by_entry. Create a temporary HashMap (e.g., content_index) by iterating rows once and extracting each row's content_identity_uuid and entry_uuid (using the same try_get calls used now), then in the metadata.content_identity_uuid branch look up content_index.get(&content_uuid) and extend tags_by_entry.entry(eu) for each entry_uuid found; update places referencing rows and the existing try_get logic to read from the index to avoid repeated scans.
🤖 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/tags/files_by_tag.rs`:
- Around line 178-190: The bug is that root-level files lose their extension
because the else branch builds format!("/{}", entry_name) instead of using the
assembled file_name; move the file_name computation out so it's always available
(compute file_name from entry_name and entry_extension before the parent_path
if-let), then change the else branch to format!("/{}", file_name) so file_path
preserves extensions for root files (affecting file_path, parent_path,
entry_extension, entry_name, file_name and resulting SdPath).
---
Nitpick comments:
In `@core/src/ops/tags/files_by_tag.rs`:
- Around line 321-342: The current loop under the branch checking
metadata.content_identity_uuid rescans all SQL rows for each metadata record,
causing O(metadata_records × rows) work; instead, precompute an index mapping
content_identity_uuid to Vec<entry_uuid> once and use it to populate
tags_by_entry. Create a temporary HashMap (e.g., content_index) by iterating
rows once and extracting each row's content_identity_uuid and entry_uuid (using
the same try_get calls used now), then in the metadata.content_identity_uuid
branch look up content_index.get(&content_uuid) and extend
tags_by_entry.entry(eu) for each entry_uuid found; update places referencing
rows and the existing try_get logic to read from the index to avoid repeated
scans.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 089929a2-0bac-4e9f-93d1-f1bfb58bf7d1
📒 Files selected for processing (1)
core/src/ops/tags/files_by_tag.rs
slvnlrt
force-pushed
the
tags-and-media-fixes
branch
from
4705475 to
352a5d5
Compare
Open
5 tasks
Contributor
Author
|
@CodeRabbit resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
core/src/ops/tags/files_by_tag.rs (1)
165-166:⚠️ Potential issue | 🟠 MajorDon't coerce an unreadable
entry_kindto0.
File::from_entity_modelneeds a real kind to preserve file-vs-directory behavior. Falling back to0turns a corrupt row into a valid-looking entry and can reintroduce bad Explorer/context-menu behavior; handleentry_kindlike the other required fields instead.Proposed fix
- let entry_kind: i32 = row.try_get("", "entry_kind").unwrap_or(0); + let Ok(entry_kind) = row.try_get::<i32>("", "entry_kind") else { + tracing::warn!(entry_id = entry_id, "Skipping row: failed to decode entry_kind"); + continue; + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/tags/files_by_tag.rs` around lines 165 - 166, The code currently coerces a missing or unreadable entry_kind to 0 (let entry_kind: i32 = row.try_get("", "entry_kind").unwrap_or(0)), which masks corruption and breaks File::from_entity_model’s file-vs-directory logic; change the handling of entry_kind to treat it as a required field like the others by using row.try_get("", "entry_kind").unwrap() (or otherwise propagate/return an error) so an unreadable/missing entry_kind fails fast instead of silently defaulting to 0, and update any surrounding error/context messages as needed.packages/interface/src/components/SpacesSidebar/TagsGroup.tsx (1)
42-56:⚠️ Potential issue | 🟠 MajorDon’t use browser
confirm()in@sd/interface.This right-click delete path is tied to a web-only API and bypasses the app’s shared context-menu flow. Route it through
useContextMenu(or the existing destructive dialog pattern) instead.As per coding guidelines "The
@sd/interfacepackage is platform agnostic and works on Tauri, Web, and React Native - never use platform-specific APIs directly" and "Use the useContextMenu hook for platform-agnostic context menus with conditional items, icons, keybinds, and variants".🤖 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 42 - 56, The right-click handler handleContextMenu uses the browser confirm() which is web-only and must be replaced with the shared context-menu/destructive flow; refactor handleContextMenu to open the app-wide context menu (useContextMenu hook) or the existing destructive dialog pattern instead of confirm(), present a "Delete tag" destructive action for tag.canonical_name, and when that action is confirmed call deleteTag.mutateAsync({ tag_id: tag.id }) and then navigate('/') if isActive; ensure you remove direct use of window.confirm and wire the menu item to the same error handling currently around deleteTag.mutateAsync.core/src/ops/tags/delete/action.rs (1)
101-105:⚠️ Potential issue | 🟡 MinorReplace the inline sync TODO with the tracking reference.
This still ships a placeholder comment in production code. Point at the follow-up issue/PR here instead of leaving a raw
TODO(sync)block.As per coding guidelines "Never use placeholder comments like 'TODO: extract this later' or 'for now'; track future work in GitHub issues instead".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/tags/delete/action.rs` around lines 101 - 105, Replace the inline TODO(sync) block in the tag deletion path with a tracked reference: create (or locate) the follow-up GitHub issue/PR that will implement calling library.sync_model() with ChangeType::Delete for tag deletions, then replace the TODO in core/src/ops/tags/delete/action.rs with a short comment referencing that issue/PR (include the issue/PR number or URL) and mention the relevant symbols — e.g., the tag deletion code path, ChangeType::Delete, and library.sync_model() — so future readers can find the tracked work.
🧹 Nitpick comments (1)
core/src/ops/tags/files_by_tag.rs (1)
1-1: Document the new query module and public API.This new
core/src/opsquery still only has a one-line module doc, andGetFilesByTagInput/GetFilesByTagOutput/GetFilesByTagQueryare undocumented. The repo guidelines require a titled//!module doc that explains why the module exists and includes an example, plus///docs on public items.As per coding guidelines, "Module documentation should explain WHY the module exists, use prose with one code example, and include a title with
#" and "Use//!for module documentation,///for public items, and include examples".Also applies to: 22-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/tags/files_by_tag.rs` at line 1, Add proper module-level documentation to core/src/ops/tags/files_by_tag.rs using a titled //! doc (e.g., "# Get Files By Tag") that explains why this query module exists, outlines its behavior, and includes a short usage example; then add /// docs to the public types GetFilesByTagInput, GetFilesByTagOutput, and GetFilesByTagQuery describing their purpose, fields/semantics, and an example snippet where appropriate, following the repo guideline to use prose, a code example, and the specified doc comment styles.
🤖 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/tags/files_by_tag.rs`:
- Around line 317-347: The content-scoped branch currently rescans and decodes
every SQL row for each metadata record (metadata_records /
metadata.content_identity_uuid), making the merge O(metadata × rows); instead,
pre-index rows once into a HashMap from content_identity_uuid -> Vec<entry_uuid>
by iterating rows a single time and extracting the two UUIDs (same try_get used
now), then in the existing loop over metadata_records (where you consult
tags_by_metadata and update tags_by_entry) simply lookup the content_uuid in
that index and extend tags for each entry_uuid found; update references to
rows/rescans to use the new index to avoid repeated decoding and quadratic
behavior.
In `@packages/interface/src/components/SpacesSidebar/TagsGroup.tsx`:
- Around line 123-124: Update the tag search selector to use proper generated
types: import type { Tag, SearchTagsInput, SearchTagsOutput } from
'@sd/ts-client', bind those generics on the useNormalizedQuery call
(useNormalizedQuery<SearchTagsInput, SearchTagsOutput>), and change the select
callback signature to (data: SearchTagsOutput) => Tag[]; inside select map the
results using (result) => result.tag || result and filter(Boolean) to return a
Tag[] instead of using any. Ensure the local variables (e.g., const { data: tags
= [], isLoading }) reflect the typed result.
---
Duplicate comments:
In `@core/src/ops/tags/delete/action.rs`:
- Around line 101-105: Replace the inline TODO(sync) block in the tag deletion
path with a tracked reference: create (or locate) the follow-up GitHub issue/PR
that will implement calling library.sync_model() with ChangeType::Delete for tag
deletions, then replace the TODO in core/src/ops/tags/delete/action.rs with a
short comment referencing that issue/PR (include the issue/PR number or URL) and
mention the relevant symbols — e.g., the tag deletion code path,
ChangeType::Delete, and library.sync_model() — so future readers can find the
tracked work.
In `@core/src/ops/tags/files_by_tag.rs`:
- Around line 165-166: The code currently coerces a missing or unreadable
entry_kind to 0 (let entry_kind: i32 = row.try_get("",
"entry_kind").unwrap_or(0)), which masks corruption and breaks
File::from_entity_model’s file-vs-directory logic; change the handling of
entry_kind to treat it as a required field like the others by using
row.try_get("", "entry_kind").unwrap() (or otherwise propagate/return an error)
so an unreadable/missing entry_kind fails fast instead of silently defaulting to
0, and update any surrounding error/context messages as needed.
In `@packages/interface/src/components/SpacesSidebar/TagsGroup.tsx`:
- Around line 42-56: The right-click handler handleContextMenu uses the browser
confirm() which is web-only and must be replaced with the shared
context-menu/destructive flow; refactor handleContextMenu to open the app-wide
context menu (useContextMenu hook) or the existing destructive dialog pattern
instead of confirm(), present a "Delete tag" destructive action for
tag.canonical_name, and when that action is confirmed call
deleteTag.mutateAsync({ tag_id: tag.id }) and then navigate('/') if isActive;
ensure you remove direct use of window.confirm and wire the menu item to the
same error handling currently around deleteTag.mutateAsync.
---
Nitpick comments:
In `@core/src/ops/tags/files_by_tag.rs`:
- Line 1: Add proper module-level documentation to
core/src/ops/tags/files_by_tag.rs using a titled //! doc (e.g., "# Get Files By
Tag") that explains why this query module exists, outlines its behavior, and
includes a short usage example; then add /// docs to the public types
GetFilesByTagInput, GetFilesByTagOutput, and GetFilesByTagQuery describing their
purpose, fields/semantics, and an example snippet where appropriate, following
the repo guideline to use prose, a code example, and the specified doc comment
styles.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8919e5f2-0ba9-4578-bdd6-c791780ede73
📒 Files selected for processing (6)
core/src/ops/tags/apply/action.rscore/src/ops/tags/delete/action.rscore/src/ops/tags/files_by_tag.rspackages/interface/src/components/Inspector/variants/FileInspector.tsxpackages/interface/src/components/SpacesSidebar/TagsGroup.tsxpackages/interface/src/hooks/useRefetchTagQueries.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/interface/src/hooks/useRefetchTagQueries.ts
Comment on lines
+123
to
124
| // TODO: replace `any` with proper generated types when available | ||
| select: (data: any) => data?.tags?.map((result: any) => result.tag || result).filter(Boolean) ?? [] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the generated tags.search output type and existing typed useNormalizedQuery patterns.
rg -n "tags\\.search|type .*Tag.*Search|interface .*Tag.*Search|Search.*Tag" packages/ts-client/src/generated/types.ts packages/interface/src --type ts --type tsx
rg -n "useNormalizedQuery<" packages/interface/src --type ts --type tsx -A2 -B2Repository: spacedriveapp/spacedrive
Length of output: 123
🏁 Script executed:
#!/bin/bash
# Find the file and read the context around lines 123-124
cat -n packages/interface/src/components/SpacesSidebar/TagsGroup.tsx | sed -n '115,135p'Repository: spacedriveapp/spacedrive
Length of output: 869
🏁 Script executed:
#!/bin/bash
# Search for tags.search and related type definitions (corrected rg syntax)
rg "tags\.search|type.*TagSearch|interface.*TagSearch" packages/ts-client/src/generated/ -A2 -B1Repository: spacedriveapp/spacedrive
Length of output: 1337
🏁 Script executed:
#!/bin/bash
# Find useNormalizedQuery usage patterns with proper typing
rg "useNormalizedQuery<" packages/interface/src -A3 -B1 -g "*.ts" -g "*.tsx"Repository: spacedriveapp/spacedrive
Length of output: 10095
🏁 Script executed:
#!/bin/bash
# Check what ts-client exports related to tags
rg "tags.*search|SearchTags" packages/ts-client/src/generated/ -A2 -B1 | head -50Repository: spacedriveapp/spacedrive
Length of output: 1387
🏁 Script executed:
#!/bin/bash
# Get complete SearchTagsOutput type definition
rg -A20 "export type SearchTagsOutput" packages/ts-client/src/generated/types.tsRepository: spacedriveapp/spacedrive
Length of output: 464
🏁 Script executed:
#!/bin/bash
# Find Tag type definition and TagSearchResult structure
rg -B2 -A10 "export type Tag " packages/ts-client/src/generated/types.ts | head -40Repository: spacedriveapp/spacedrive
Length of output: 322
🏁 Script executed:
#!/bin/bash
# Check what the select output should be - look at the data structure
rg -B5 -A15 "export type TagSearchResult" packages/ts-client/src/generated/types.tsRepository: spacedriveapp/spacedrive
Length of output: 492
🏁 Script executed:
#!/bin/bash
# Check imports in TagsGroup.tsx
head -30 packages/interface/src/components/SpacesSidebar/TagsGroup.tsxRepository: spacedriveapp/spacedrive
Length of output: 1126
🏁 Script executed:
#!/bin/bash
# Verify if SearchTagsInput/SearchTagsOutput are exported from ts-client
rg "export.*SearchTags(Input|Output)" packages/ts-client/src/index.ts packages/ts-client/src/generated/index.tsRepository: spacedriveapp/spacedrive
Length of output: 149
🏁 Script executed:
#!/bin/bash
# Find the correct ts-client export file
find packages/ts-client/src -name "index.ts" -o -name "*.ts" | grep -E "(index|export)" | head -10Repository: spacedriveapp/spacedrive
Length of output: 137
🏁 Script executed:
#!/bin/bash
# Check what's exported from ts-client
rg "export.*SearchTags" packages/ts-client/src/Repository: spacedriveapp/spacedrive
Length of output: 218
🏁 Script executed:
#!/bin/bash
# Check if SearchTagsInput/SearchTagsOutput are exported from main index
rg "SearchTagsInput|SearchTagsOutput" packages/ts-client/src/index.tsRepository: spacedriveapp/spacedrive
Length of output: 50
🏁 Script executed:
#!/bin/bash
# See the full structure of what's exported from ts-client
cat packages/ts-client/src/index.ts | head -50Repository: spacedriveapp/spacedrive
Length of output: 1457
🏁 Script executed:
#!/bin/bash
# Check more of the index.ts file to see all exports
cat packages/ts-client/src/index.ts | tail -100Repository: spacedriveapp/spacedrive
Length of output: 2087
🏁 Script executed:
#!/bin/bash
# Look for re-exports of generated types
rg "export.*from.*generated" packages/ts-client/src/index.tsRepository: spacedriveapp/spacedrive
Length of output: 102
Replace any types in the tag search selector with proper generated types.
The select function should be typed as (data: SearchTagsOutput) => Tag[]. Import SearchTagsInput and SearchTagsOutput from @sd/ts-client and bind the generic type parameters to useNormalizedQuery<SearchTagsInput, SearchTagsOutput>.
import type { Tag, SearchTagsInput, SearchTagsOutput } from '@sd/ts-client';
const { data: tags = [], isLoading } = useNormalizedQuery<SearchTagsInput, SearchTagsOutput>({
query: 'tags.search',
input: { query: '' },
resourceType: 'tag',
select: (data: SearchTagsOutput) => data?.tags?.map((result) => result.tag || result).filter(Boolean) ?? []
});🤖 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
123 - 124, Update the tag search selector to use proper generated types: import
type { Tag, SearchTagsInput, SearchTagsOutput } from '@sd/ts-client', bind those
generics on the useNormalizedQuery call (useNormalizedQuery<SearchTagsInput,
SearchTagsOutput>), and change the select callback signature to (data:
SearchTagsOutput) => Tag[]; inside select map the results using (result) =>
result.tag || result and filter(Boolean) to return a Tag[] instead of using any.
Ensure the local variables (e.g., const { data: tags = [], isLoading }) reflect
the typed result.
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 `@core/src/ops/tags/files_by_tag.rs`:
- Around line 201-205: The code constructs an SdPath::Physical using
device_slug.unwrap_or_else(|| crate::device::get_current_device_slug()) which
can be an empty String (get_current_device_slug returns "" if uninitialized),
producing invalid SdPath; update the block in files_by_tag.rs so after resolving
device_slug (the value passed to SdPath::Physical) you validate it is non-empty
and either return an error (propagate a suitable error type) or log a warning
and skip/handle the row, and ensure any callers of this code handle that error;
reference the symbols device_slug, SdPath::Physical, and get_current_device_slug
when adding the guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6973d875-49ff-4cb4-9853-dd506102d473
📒 Files selected for processing (1)
core/src/ops/tags/files_by_tag.rs
Comment on lines
+201
to
+205
| let sd_path = SdPath::Physical { | ||
| device_slug: device_slug | ||
| .unwrap_or_else(|| crate::device::get_current_device_slug()), | ||
| path: PathBuf::from(file_path), | ||
| }; |
There was a problem hiding this comment.
Empty device_slug fallback may produce invalid SdPath.
get_current_device_slug() returns an empty String when CURRENT_DEVICE_SLUG is uninitialized (see core/src/device/id.rs:64-67). If the SQL row lacks a device slug and the global isn't set—e.g., in tests or library-only contexts—this silently constructs an SdPath::Physical with an empty device_slug, which could cause downstream file operations to fail or target the wrong device.
Consider logging a warning or returning an error when both the row and fallback yield empty/missing slugs.
💡 Suggested defensive check
let sd_path = SdPath::Physical {
- device_slug: device_slug
- .unwrap_or_else(|| crate::device::get_current_device_slug()),
+ device_slug: device_slug.or_else(|| {
+ let current = crate::device::get_current_device_slug();
+ if current.is_empty() {
+ tracing::warn!("No device_slug for entry_id={}, using empty slug", entry_id);
+ }
+ Some(current)
+ }).unwrap_or_default(),
path: PathBuf::from(file_path),
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/ops/tags/files_by_tag.rs` around lines 201 - 205, The code
constructs an SdPath::Physical using device_slug.unwrap_or_else(||
crate::device::get_current_device_slug()) which can be an empty String
(get_current_device_slug returns "" if uninitialized), producing invalid SdPath;
update the block in files_by_tag.rs so after resolving device_slug (the value
passed to SdPath::Physical) you validate it is non-empty and either return an
error (propagate a suitable error type) or log a warning and skip/handle the
row, and ensure any callers of this code handle that error; reference the
symbols device_slug, SdPath::Physical, and get_current_device_slug when adding
the guard.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/src/ops/tags/apply/action.rs (2)
15-15: Consider consolidating imports.
ColumnTraitandQueryFilterare imported at the top level (line 15) but also imported inline within theContentbranch (lines 109-110). Consider removing the inline imports for consistency since the top-level imports are now available.🤖 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 15, Remove the redundant inline imports of ColumnTrait and QueryFilter inside the Content branch and rely on the existing top-level imports; locate the inline use statements (the imports inside the Content match arms) and delete the `use sea_orm::ColumnTrait;` and `use sea_orm::QueryFilter;` lines so only the top-level `use sea_orm::{ColumnTrait, DatabaseConnection, EntityTrait, QueryFilter};` remains, ensuring no other symbols from those inline use lines are relied upon in that branch.
262-274: Remove unusedlookup_entry_uuidfunction.This function is dead code after the batch-lookup refactor. The
TagTargets::Entrybranch uses a batchedHashMapapproach, and theTagTargets::EntryUuidbranch receives UUIDs directly. No calls to this function exist in this file.Proposed removal
-/// Look up entry UUID from entry database ID -async fn lookup_entry_uuid(db: &DatabaseConnection, entry_id: i32) -> Result<Uuid, String> { - use crate::infra::db::entities::entry; - - let entry_model = entry::Entity::find_by_id(entry_id) - .one(db) - .await - .map_err(|e| format!("Database error: {}", e))? - .ok_or_else(|| format!("Entry with ID {} not found", entry_id))?; - - entry_model - .uuid - .ok_or_else(|| format!("Entry {} has no UUID assigned", entry_id)) -}🤖 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 262 - 274, Remove the dead helper function lookup_entry_uuid from this file: it is no longer used after the batch lookup refactor (TagTargets::Entry now uses a batched HashMap and TagTargets::EntryUuid gets UUIDs directly). Delete the async fn lookup_entry_uuid(...) definition and any associated imports that become unused (e.g., DatabaseConnection, crate::infra::db::entities::entry, Uuid) so the file compiles without unused symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core/src/ops/tags/apply/action.rs`:
- Line 15: Remove the redundant inline imports of ColumnTrait and QueryFilter
inside the Content branch and rely on the existing top-level imports; locate the
inline use statements (the imports inside the Content match arms) and delete the
`use sea_orm::ColumnTrait;` and `use sea_orm::QueryFilter;` lines so only the
top-level `use sea_orm::{ColumnTrait, DatabaseConnection, EntityTrait,
QueryFilter};` remains, ensuring no other symbols from those inline use lines
are relied upon in that branch.
- Around line 262-274: Remove the dead helper function lookup_entry_uuid from
this file: it is no longer used after the batch lookup refactor
(TagTargets::Entry now uses a batched HashMap and TagTargets::EntryUuid gets
UUIDs directly). Delete the async fn lookup_entry_uuid(...) definition and any
associated imports that become unused (e.g., DatabaseConnection,
crate::infra::db::entities::entry, Uuid) so the file compiles without unused
symbols.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9f30250c-41de-45d8-87e6-0c55f04525ab
📒 Files selected for processing (3)
core/src/ops/tags/apply/action.rscore/src/ops/tags/manager.rspackages/interface/src/hooks/useJobDispatch.ts
💤 Files with no reviewable changes (1)
- packages/interface/src/hooks/useJobDispatch.ts
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 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…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 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Extract duplicated refetchQueries calls from FileInspector, useFileContextMenu, TagsGroup, and TagSelector into a single useRefetchTagQueries hook. Removes direct useQueryClient usage from those files. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…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>
useJobDispatch called a non-existent `jobs.dispatch` backend endpoint. All media operations now use direct mutations (media.thumbnail.regenerate, etc.) which are the correct pattern. No remaining imports. 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>
slvnlrt
force-pushed
the
tags-and-media-fixes
branch
from
59094ca to
0da17e4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
core/src/ops/search/query.rs (1)
1462-1570: Consider extracting shared tag→entry resolution logic.The
find_entry_ids_for_taghelper here andfind_entry_ids_for_tagsinfiles_by_tag.rsshare similar logic for resolving tags to entry IDs via both entry-scoped and content-scoped metadata paths. This works correctly but creates maintenance overhead.Consider extracting this to a shared helper in the tags module (e.g.,
manager.rsor a dedicatedresolution.rs) to consolidate the logic. Not blocking, but would reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/search/query.rs` around lines 1462 - 1570, Extract the duplicated tag→entry resolution into a shared helper (e.g., tags::resolution::resolve_entry_ids_for_tag or tags::manager::find_entry_ids_for_tag) and update both find_entry_ids_for_tag (in query.rs) and find_entry_ids_for_tags (in files_by_tag.rs) to call it; the helper should accept &DatabaseConnection and a tag UUID (or a slice of tag UUIDs if you want a batch API), perform the same steps currently in find_entry_ids_for_tag (lookup Tag, fetch UserMetadataTag rows, batch load UserMetadata, collect entry_uuid and content_identity_uuid, load ContentIdentity and Entry entities and return unique entry IDs), and reuse the same entity names (Tag, UserMetadataTag, UserMetadata, ContentIdentity, Entry) so callers simply delegate to the new function to remove duplicated logic.core/src/ops/tags/files_by_tag.rs (1)
201-205: Consider adding a warning when device_slug fallback is empty.Per the previous review,
get_current_device_slug()can return an empty string when uninitialized. While functional, logging a warning when both the row and fallback yield empty slugs would improve observability for debugging in tests or edge cases.💡 Optional defensive check
let sd_path = SdPath::Physical { - device_slug: device_slug - .unwrap_or_else(|| crate::device::get_current_device_slug()), + device_slug: device_slug.unwrap_or_else(|| { + let slug = crate::device::get_current_device_slug(); + if slug.is_empty() { + tracing::warn!("No device_slug for entry_id={}, using empty slug", entry_id); + } + slug + }), path: PathBuf::from(file_path), };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/tags/files_by_tag.rs` around lines 201 - 205, The code constructs SdPath::Physical using device_slug.unwrap_or_else(|| crate::device::get_current_device_slug()), but get_current_device_slug() may return an empty string; add a defensive check that captures the resolved slug into a local variable, and if that variable is empty emit a warning (e.g., tracing::warn! or the project's logger) including the file_path and context before constructing SdPath::Physical so callers can observe the fallback produced an empty device slug. Reference: SdPath::Physical, device_slug, get_current_device_slug(), file_path.packages/interface/src/routes/explorer/hooks/useExplorerFiles.ts (1)
146-151: Add type parameters touseNormalizedQueryfor type safety.The tag query lacks explicit type parameters, making the return type inferred as
unknown. This leads to theas anycast on line 183.💡 Add type parameters
-const tagQuery = useNormalizedQuery({ +const tagQuery = useNormalizedQuery< + { tag_id: string; include_children: boolean; min_confidence: number }, + { files: File[] } +>({ query: "files.by_tag", input: tagQueryInput!, resourceType: "file", enabled: isTagMode && !!tagQueryInput, });Then line 183 becomes:
-return (tagQuery.data as any)?.files || []; +return tagQuery.data?.files || [];🤖 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 146 - 151, The tagQuery call to useNormalizedQuery is missing explicit generic type parameters, causing the return to be inferred as unknown and forcing an unsafe cast; update the useNormalizedQuery invocation that creates tagQuery to include the proper input and output type parameters (based on tagQueryInput's type and the expected file result shape) so the hook returns a correctly typed result and you can remove the "as any" on the later usage; specifically modify the useNormalizedQuery call that references tagQueryInput and assigns to tagQuery to supply the appropriate generics.packages/interface/src/routes/explorer/hooks/useFileContextMenu.ts (2)
88-90: Type assertion on physical path.The
as anycast onfile.sd_pathbypasses type safety. Consider a type guard or narrowing pattern.🤖 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 88 - 90, Replace the unsafe "as any" cast by adding a proper type guard and using it in the filter/map in useFileContextMenu: implement a type predicate function (e.g. isPhysicalSdPath(sd_path): sd_path is { Physical: { path: string } }) and use it instead of the `'Physical' in f.sd_path` check so the subsequent map may safely read f.sd_path.Physical.path without casting; update the filter to `f && f.sd_path && isPhysicalSdPath(f.sd_path)` and remove the `as any` in the map.
66-78: Consider surfacing per-file errors to users.
forEachTargetlogs failures to console but doesn't notify users when some files in a batch fail. For multi-select operations, users won't know which files succeeded vs. failed unless they check the console.💡 Optional: Add toast notification for partial failures
const forEachTarget = async ( targets: File[], fn: (f: File) => Promise<unknown> ) => { + const failures: string[] = []; for (const f of targets) { try { await fn(f); } catch (err) { console.error(`Failed for ${f.name}:`, err); + failures.push(f.name); } } + if (failures.length > 0) { + toast.error({ + title: 'Partial failure', + body: `Failed for: ${failures.join(', ')}` + }); + } };🤖 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 66 - 78, The helper forEachTarget currently swallows per-file failures by only console.error-ing; update forEachTarget (targets: File[], fn: (f: File) => Promise<unknown>) to collect failures (e.g., accumulate failed File.name and error messages) as it iterates, and after the loop surface a user-facing notification (use the app's toast/notification API used elsewhere) summarizing partial failures (e.g., "X files failed" with a list or first N names), so callers of forEachTarget see which files failed; keep console.error for debugging but ensure a concise toast is shown when any file fails.core/src/ops/tags/apply/action.rs (1)
260-273: Remove unusedlookup_entry_uuidfunction.The
Entrybranch now uses batch lookup (lines 134-142), making this per-entry helper redundant. The function is not called within this file.🤖 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 260 - 273, Remove the unused helper lookup_entry_uuid: delete the entire async fn lookup_entry_uuid(...) implementation from this file (referencing the function name lookup_entry_uuid and its use of entry::Entity), and also remove any now-unused imports or type references tied only to that function (e.g., crate::infra::db::entities::entry, DatabaseConnection, Uuid) to keep imports clean.
🤖 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/tags/create/action.rs`:
- Around line 200-228: The EntryUuid branch is missing validation that the
supplied UUIDs exist in the entries table; before calling
metadata_manager.apply_semantic_tags_to_entry, perform a batch existence check
(like the validation used in apply/action.rs) against the entry store for all
entry_uuids, return or error on any missing UUIDs (or filter out non-existent
ones and report them), then only call
metadata_manager.apply_semantic_tags_to_entry for validated UUIDs and push those
into affected_entry_uuids; update error paths to surface which UUIDs were
invalid and reuse the same helper/query used elsewhere to locate entries rather
than assuming referential integrity.
---
Nitpick comments:
In `@core/src/ops/search/query.rs`:
- Around line 1462-1570: Extract the duplicated tag→entry resolution into a
shared helper (e.g., tags::resolution::resolve_entry_ids_for_tag or
tags::manager::find_entry_ids_for_tag) and update both find_entry_ids_for_tag
(in query.rs) and find_entry_ids_for_tags (in files_by_tag.rs) to call it; the
helper should accept &DatabaseConnection and a tag UUID (or a slice of tag UUIDs
if you want a batch API), perform the same steps currently in
find_entry_ids_for_tag (lookup Tag, fetch UserMetadataTag rows, batch load
UserMetadata, collect entry_uuid and content_identity_uuid, load ContentIdentity
and Entry entities and return unique entry IDs), and reuse the same entity names
(Tag, UserMetadataTag, UserMetadata, ContentIdentity, Entry) so callers simply
delegate to the new function to remove duplicated logic.
In `@core/src/ops/tags/apply/action.rs`:
- Around line 260-273: Remove the unused helper lookup_entry_uuid: delete the
entire async fn lookup_entry_uuid(...) implementation from this file
(referencing the function name lookup_entry_uuid and its use of entry::Entity),
and also remove any now-unused imports or type references tied only to that
function (e.g., crate::infra::db::entities::entry, DatabaseConnection, Uuid) to
keep imports clean.
In `@core/src/ops/tags/files_by_tag.rs`:
- Around line 201-205: The code constructs SdPath::Physical using
device_slug.unwrap_or_else(|| crate::device::get_current_device_slug()), but
get_current_device_slug() may return an empty string; add a defensive check that
captures the resolved slug into a local variable, and if that variable is empty
emit a warning (e.g., tracing::warn! or the project's logger) including the
file_path and context before constructing SdPath::Physical so callers can
observe the fallback produced an empty device slug. Reference: SdPath::Physical,
device_slug, get_current_device_slug(), file_path.
In `@packages/interface/src/routes/explorer/hooks/useExplorerFiles.ts`:
- Around line 146-151: The tagQuery call to useNormalizedQuery is missing
explicit generic type parameters, causing the return to be inferred as unknown
and forcing an unsafe cast; update the useNormalizedQuery invocation that
creates tagQuery to include the proper input and output type parameters (based
on tagQueryInput's type and the expected file result shape) so the hook returns
a correctly typed result and you can remove the "as any" on the later usage;
specifically modify the useNormalizedQuery call that references tagQueryInput
and assigns to tagQuery to supply the appropriate generics.
In `@packages/interface/src/routes/explorer/hooks/useFileContextMenu.ts`:
- Around line 88-90: Replace the unsafe "as any" cast by adding a proper type
guard and using it in the filter/map in useFileContextMenu: implement a type
predicate function (e.g. isPhysicalSdPath(sd_path): sd_path is { Physical: {
path: string } }) and use it instead of the `'Physical' in f.sd_path` check so
the subsequent map may safely read f.sd_path.Physical.path without casting;
update the filter to `f && f.sd_path && isPhysicalSdPath(f.sd_path)` and remove
the `as any` in the map.
- Around line 66-78: The helper forEachTarget currently swallows per-file
failures by only console.error-ing; update forEachTarget (targets: File[], fn:
(f: File) => Promise<unknown>) to collect failures (e.g., accumulate failed
File.name and error messages) as it iterates, and after the loop surface a
user-facing notification (use the app's toast/notification API used elsewhere)
summarizing partial failures (e.g., "X files failed" with a list or first N
names), so callers of forEachTarget see which files failed; keep console.error
for debugging but ensure a concise toast is shown when any file fails.
🪄 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: 0e610e6d-4d32-4d16-af1c-b3b178b21f3d
⛔ Files ignored due to path filters (1)
packages/ts-client/src/generated/types.tsis excluded by!**/generated/**,!**/generated/**
📒 Files selected for processing (35)
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/useJobDispatch.tspackages/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
💤 Files with no reviewable changes (1)
- packages/interface/src/hooks/useJobDispatch.ts
✅ Files skipped from review due to trivial changes (9)
- core/src/ops/tags/delete/output.rs
- packages/interface/src/hooks/useRefetchTagQueries.ts
- core/src/ops/tags/delete/mod.rs
- core/src/ops/tags/unapply/output.rs
- core/src/ops/tags/unapply/mod.rs
- core/src/ops/tags/delete/input.rs
- core/src/infra/db/migration/mod.rs
- packages/interface/src/components/Tags/TagSelector.tsx
- core/src/ops/tags/apply/input.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/interface/src/routes/overview/OverviewTopBar.tsx
- core/src/infra/db/migration/m20260125_000001_unique_user_metadata_tag.rs
- core/src/ops/tags/unapply/input.rs
- packages/interface/src/components/Inspector/primitives/Tag.tsx
- core/src/ops/media/thumbnail/action.rs
- packages/interface/src/routes/explorer/context.tsx
- core/src/ops/tags/mod.rs
- core/src/ops/tags/ancestors.rs
- packages/interface/src/routes/tag/index.tsx
- core/src/ops/metadata/manager.rs
Comment on lines
+200
to
228
| ApplyToTargets::EntryUuid(entry_uuids) => { | ||
| for &entry_uuid in entry_uuids { | ||
| let models = metadata_manager | ||
| .apply_semantic_tags_to_entry( | ||
| entry_uuid, | ||
| vec![tag_application.clone()], | ||
| device_id, | ||
| ) | ||
| .await | ||
| .map_err(|e| { | ||
| ActionError::Internal(format!( | ||
| "Failed to apply tag to entry: {}", | ||
| e | ||
| )) | ||
| })?; | ||
| for model in models { | ||
| library | ||
| .sync_model(&model, ChangeType::Insert) | ||
| .await | ||
| .map_err(|e| { | ||
| ActionError::Internal(format!( | ||
| "Failed to sync tag association: {}", | ||
| e | ||
| )) | ||
| })?; | ||
| } | ||
| affected_entry_uuids.push(entry_uuid); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing entry UUID validation before tagging.
Unlike apply/action.rs (which now batch-validates UUIDs exist in the entry table), this EntryUuid branch calls apply_semantic_tags_to_entry without checking that each UUID references an existing entry. Per the learned context, SeaORM relations are declarative only and SQLite won't enforce referential integrity, so invalid UUIDs can create orphaned user_metadata rows.
🔧 Suggested fix: Add batch validation
ApplyToTargets::EntryUuid(entry_uuids) => {
+ // Batch-validate all entry UUIDs exist
+ use crate::infra::db::entities::entry;
+ let existing_entries: std::collections::HashSet<Uuid> =
+ entry::Entity::find()
+ .filter(entry::Column::Uuid.is_in(entry_uuids.clone()))
+ .all(library.db().conn())
+ .await
+ .map_err(|e| ActionError::Internal(format!("DB error: {}", e)))?
+ .into_iter()
+ .filter_map(|e| e.uuid)
+ .collect();
+
for &entry_uuid in entry_uuids {
+ if !existing_entries.contains(&entry_uuid) {
+ tracing::warn!("Entry {} not found, skipping tag application", entry_uuid);
+ continue;
+ }
let models = metadata_manager
.apply_semantic_tags_to_entry(Based on learnings: "In the spacedriveapp/spacedrive codebase (SeaORM + SQLite), SeaORM relationship definitions are declarative and are not enforced by SQLite unless you add actual FK constraints."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/ops/tags/create/action.rs` around lines 200 - 228, The EntryUuid
branch is missing validation that the supplied UUIDs exist in the entries table;
before calling metadata_manager.apply_semantic_tags_to_entry, perform a batch
existence check (like the validation used in apply/action.rs) against the entry
store for all entry_uuids, return or error on any missing UUIDs (or filter out
non-existent ones and report them), then only call
metadata_manager.apply_semantic_tags_to_entry for validated UUIDs and push those
into affected_entry_uuids; update error paths to surface which UUIDs were
invalid and reuse the same helper/query used elsewhere to locate entries rather
than assuming referential integrity.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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"fallbackTags frontend
TagsGrouprefetches on tag changesuseRefetchTagQueriesfor consistent cache invalidation/explorerfrom OverviewMedia 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. All media processors follow this same dual architecture:ThumbnailJobetc. for library-wide operations with progress/persistenceuseJobDispatch.tsis now dead code (zero imports).Merge note
This PR and #3041 both modify
useFileContextMenu.ts. If both are merged, the second will need a minor rebase (2-3 lines in the imports/hooks section). No functional conflict.Files changed (34 files, +1603/-468)
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 (17 files):
useFileContextMenu.ts,useExplorerFiles.ts,context.tsx,ExplorerView.tsx,tag/index.tsx,FileInspector.tsx,TagsGroup.tsx,TagSelector.tsx,Tag.tsx,useRefetchTagQueries.ts,OverviewTopBar.tsx,generated/types.tsTest plan