Add all-tags item filter option#1493
Conversation
Adds an optional match-all mode for item tag filters so users can require every selected tag group while preserving the existing match-any behavior by default. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: sofiane <131229158+sohocine@users.noreply.github.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a MatchAllTags boolean to queries, implements per-root descendant resolution and AND/OR predicate composition in QueryByGroup, exposes the flag through API types, UI toggle, routing, and localization, and adds repository tests plus a tag-descendants helper. ChangesMatch All Tags Filter Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Security Recommendations
Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/internal/data/repo/repo_entities.go (1)
639-658:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBoth
NegateTagsandMatchAllTagsflags can be set independently; the backend silently ignoresMatchAllTagswhenNegateTagsis true.The frontend UI presents both as independent toggles (lines 436-441 in
frontend/pages/items.vue) with no mutual-exclusivity guard, and the handler extracts both flags without validation. However, in the backend (line 611), the per-tag descendant collection only runs whenq.MatchAllTags && !q.NegateTags. WhenNegateTagsis true, the condition is false,descendantGroupsfalls back to a single combined slice, and the negate predicate branch at line 653 constructsAND(NOT HasTag(...))— effectively "entity has none of the tags" — regardless ofMatchAllTags.A user toggling both flags would reasonably expect independent behavior, but the flag combination becomes a no-op. Choose one:
- Enforce mutual exclusivity at the handler layer to reject the combination as invalid, ensuring the API contract is unambiguous.
- Support both flags: when
NegateTags && MatchAllTags, implement negated-all semantics (e.g.,Not(And(HasAny(group1), HasAny(group2), ...))) so the combination is not silently broken.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/internal/data/repo/repo_entities.go` around lines 639 - 658, The current logic silently ignores q.MatchAllTags when q.NegateTags is true by building tagPredicates from only descendantGroups[0] and appending entity.And(entity.Not(...)) which yields "has none of the tags" instead of honoring both flags; fix by either validating and rejecting the combination early in the request handler (return a 400 if q.NegateTags && q.MatchAllTags) or by changing the predicate construction in this block to compute per-group predicates over descendantGroups and then wrap them in a negated AND (e.g., build per-group entity.Or(...) for each descendants slice, combine with entity.And(...), then wrap with entity.Not(...)) so q.NegateTags && q.MatchAllTags produces Not(And(HasAny(group1), HasAny(group2), ...)).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/internal/data/repo/repo_entities.go`:
- Around line 609-635: The loop in repo_entities.go causes N calls to
tagRepo.GetDescendantTagIDs when MatchAllTags && !NegateTags, enabling an N+1 DB
round-trip and potential DoS via large q.TagIDs; fix by (1) adding API-level
input validation in v1_ctrl_entities.go to cap len(q.TagIDs) (e.g., max 50) and
return a 400 when exceeded, and (2) add a batched repository method on
TagRepository (e.g., GetAllDescendantsByTagID(ctx, tagIDs)
map[uuid.UUID][]uuid.UUID) and change the MatchAllTags branch in the function
using q.TagIDs to call that single batched method and build descendantGroups
from the returned map instead of looping per tag.
---
Outside diff comments:
In `@backend/internal/data/repo/repo_entities.go`:
- Around line 639-658: The current logic silently ignores q.MatchAllTags when
q.NegateTags is true by building tagPredicates from only descendantGroups[0] and
appending entity.And(entity.Not(...)) which yields "has none of the tags"
instead of honoring both flags; fix by either validating and rejecting the
combination early in the request handler (return a 400 if q.NegateTags &&
q.MatchAllTags) or by changing the predicate construction in this block to
compute per-group predicates over descendantGroups and then wrap them in a
negated AND (e.g., build per-group entity.Or(...) for each descendants slice,
combine with entity.And(...), then wrap with entity.Not(...)) so q.NegateTags &&
q.MatchAllTags produces Not(And(HasAny(group1), HasAny(group2), ...)).
🪄 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: 085b99b5-f564-4499-bc5e-b80981aec755
📒 Files selected for processing (5)
backend/internal/data/repo/repo_entities.gobackend/internal/data/repo/repo_entities_test.gofrontend/lib/api/classes/items.tsfrontend/locales/en.jsonfrontend/pages/items.vue
Allow negateTags to combine with matchAllTags and batch descendant tag lookup for match-all queries. Signed-off-by: sofiane <131229158+sohocine@users.noreply.github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/internal/data/repo/repo_tags.go`:
- Around line 214-236: The GetDescendantTagIDsByRoot helper currently queries
all tags and must be scoped by group; change its signature to accept a gid
(uuid.UUID) and update its query r.db.Tag.Query().WithParent().All(ctx) to
include a group predicate (e.g., .Where(tag.GroupIDEQ(gid)) or use the
repository's QueryByGroup helper) so childrenByParent is built only from tags in
that group; also update the caller in QueryByGroup (the MatchAllTags path) to
pass the existing gid through into GetDescendantTagIDsByRoot and ensure
descendantsFromChildren continues to operate unchanged on the filtered
childrenByParent map.
🪄 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: 7df9022a-6da6-4df7-b0c4-180f00d1f7f2
📒 Files selected for processing (3)
backend/internal/data/repo/repo_entities.gobackend/internal/data/repo/repo_entities_test.gobackend/internal/data/repo/repo_tags.go
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/internal/data/repo/repo_entities_test.go
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: sofiane <131229158+sohocine@users.noreply.github.com>
|
Scoped the descendant tag lookup to the current group and pushed the fix. The match-all/negate tag logic remains supported. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: sofiane <131229158+sohocine@users.noreply.github.com>
|
Pushed fbe8c9b to address the latest review findings. The tag descendant lookup is now batched and group-scoped through Local checks passed:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: sofiane <131229158+sohocine@users.noreply.github.com>
coderabbitai
Bot
added
⬆️ enhancement
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/pages/location/[id]/index/index.vue (1)
51-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix reactive key for useAsyncData to prevent stale location data on route param changes.
At Line 51,
locationId.valuecaptures the route param as a snapshot string key. On in-app navigation between/location/:idroutes, this preventsuseAsyncDatafrom refetching, leaving the location data stale in the same component instance.Proposed fix
- const { data: location } = useAsyncData(locationId.value, async () => { - const { data, error } = await api.items.getLocation(locationId.value); - if (error) { - toast.error(t("locations.toast.failed_load_location")); - navigateTo("/home"); - return; - } - - return data; - }); + const { data: location } = useAsyncData( + () => `location-${locationId.value}`, + async () => { + const { data, error } = await api.items.getLocation(locationId.value); + if (error) { + toast.error(t("locations.toast.failed_load_location")); + navigateTo("/home"); + return; + } + return data; + }, + { watch: [locationId] } + );Use a reactive key (getter function) or the
watchoption to trigger refetches when the route param changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@frontend/pages/location/`[id]/index/index.vue around lines 51 - 60, useAsyncData is using the snapshot key locationId.value which prevents refetch when the route param changes; change the key to a reactive getter (e.g. pass () => locationId.value as the first argument) or use the watch option so the fetch block that calls api.items.getLocation(locationId.value) re-runs on param changes, preserving the existing error handling (toast.error, navigateTo) and return value semantics.
🧹 Nitpick comments (1)
.github/workflows/partial-frontend.yaml (1)
22-25: ⚡ Quick winPin the Node.js runtime version instead of using
lts/*for supply-chain consistency.While
actions/setup-nodeis securely pinned to a commit hash, thenode-version: lts/*parameter remains a floating target that can drift between CI runs, weakening reproducibility and creating non-deterministic behavior. Other workflows in this repository (e.g.,copilot-setup-steps.yml) correctly use explicit versions like"24". Consider pinning to a specific LTS version (e.g.,"20"or"22") to maintain consistency with your hardened action pinning strategy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/partial-frontend.yaml around lines 22 - 25, Replace the floating node-version specifier used with the actions/setup-node step by pinning to an explicit LTS major version string (e.g., "20" or "22") instead of "lts/*"; locate the setup step that references actions/setup-node@395ad326... and update the node-version: value to the chosen explicit version to ensure reproducible CI runs and match the repository’s hardened pinning strategy.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@frontend/pages/location/`[id]/index/index.vue:
- Around line 51-60: useAsyncData is using the snapshot key locationId.value
which prevents refetch when the route param changes; change the key to a
reactive getter (e.g. pass () => locationId.value as the first argument) or use
the watch option so the fetch block that calls
api.items.getLocation(locationId.value) re-runs on param changes, preserving the
existing error handling (toast.error, navigateTo) and return value semantics.
---
Nitpick comments:
In @.github/workflows/partial-frontend.yaml:
- Around line 22-25: Replace the floating node-version specifier used with the
actions/setup-node step by pinning to an explicit LTS major version string
(e.g., "20" or "22") instead of "lts/*"; locate the setup step that references
actions/setup-node@395ad326... and update the node-version: value to the chosen
explicit version to ensure reproducible CI runs and match the repository’s
hardened pinning strategy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 8a46578f-8734-4029-bf22-28d553102265
📒 Files selected for processing (7)
.github/workflows/partial-frontend.yamlbackend/app/api/main.gofrontend/components/Location/CreateModal.vuefrontend/pages/collection/index/entity-types.vuefrontend/pages/item/[id]/index/edit.vuefrontend/pages/location/[id]/index/edit.vuefrontend/pages/location/[id]/index/index.vue
💤 Files with no reviewable changes (3)
- frontend/pages/collection/index/entity-types.vue
- frontend/pages/item/[id]/index/edit.vue
- frontend/pages/location/[id]/index/edit.vue
✅ Files skipped from review due to trivial changes (1)
- frontend/components/Location/CreateModal.vue
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: sofiane <131229158+sohocine@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: sofiane <131229158+sohocine@users.noreply.github.com>
|
Pushed the remaining backend CI fixes; The remaining red I did not disable or relax typechecking in this PR because that would hide existing frontend type debt rather than fix this feature. Happy to split that baseline cleanup separately if desired. |
1 participant
Summary
Relates to #436.
Validation
pnpm exec eslint pages/items.vue lib/api/classes/items.tsgo test ./internal/data/repo -run TestEntityRepository_QueryByGroup_MatchAllTags -count=1go test ./internal/data/repo -count=1git diff --checkSummary by CodeRabbit
New Features
Refactor
Tests
Chores