Feature/filter no faces#739
Merged
Merged
Conversation
Contributor
|
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 Changes
Sequence DiagramsequenceDiagram
participant Client
participant AssetValidator as Asset Validator
participant FaceService as Face Detection Service
participant DateValidator as Date Validator
Client->>AssetValidator: Request asset validation
AssetValidator->>AssetValidator: Check FilterExcludeFaces flag
alt FilterExcludeFaces enabled
AssetValidator->>FaceService: Request faces for asset (if People/Unassigned empty)
FaceService-->>AssetValidator: Return face data
AssetValidator->>AssetValidator: Fail if any faces/unassigned faces present
else FilterExcludeFaces disabled
AssetValidator->>AssetValidator: Skip face check (pass)
end
AssetValidator->>DateValidator: Validate date range / buckets
DateValidator-->>AssetValidator: Date valid / invalid
AssetValidator-->>Client: Return overall validation result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@flake.nix`:
- Around line 50-54: The Nix derivation is skipping the package's TypeScript
type-check because it runs individual scripts (bun run css/js/url-builder);
change buildPhase to invoke the package's top-level build script (e.g., run "bun
run build" or otherwise run the same pre-build/typecheck command the package
uses) before emitting assets so the TypeScript check in the package's build
pipeline (referenced by the build script and the buildPhase block) is executed;
ensure you either replace the three bun run lines with the single bun run build
or prepend the type-check command the package build uses so type errors fail the
build.
- Around line 75-80: The preBuild step currently copies the entire assets
directory into frontend/public/assets which can create
frontend/public/assets/assets and break go:embed paths; update the cp invocation
in the preBuild block to copy the contents instead of the directory (e.g., use
cp -r ${frontend}/assets/. frontend/public/assets or copy into
frontend/public/), or use a content-preserving tool like rsync -a
${frontend}/assets/ frontend/public/assets, ensuring mkdir -p frontend/public
remains and the final paths match the original assets layout expected by the Go
embed.
- Around line 14-36: The fixed-output derivation "node_modules" uses a single
outputHash which only matches the system it was generated on; change it to
produce per-system hashes by keying the fixed-output by ${system} — update the
"node_modules" derivation to replace the single shared outputHash usage with a
system-keyed hash (e.g. add an outputHashKey that includes ${system} or
otherwise make the hash lookup vary by ${system}) and keep
outputHash/outputHashAlgo/outputHashMode tied to that key so each platform
(darwin-arm64, linux-x64, etc.) can have its own correct vendored bun/esbuild
hash.
In `@internal/immich/immich_helpers.go`:
- Around line 739-741: The no-faces filter currently calls CheckForFaces when
either a.People or a.UnassignedFaces is empty, causing unnecessary API lookups;
change the condition in the block that inspects a.People and a.UnassignedFaces
to require both to be empty (use logical AND) so CheckForFaces(requestID,
deviceID) is only invoked when neither collection has been populated,
referencing the a.People, a.UnassignedFaces variables and the CheckForFaces
method to locate the change.
🪄 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: d247fe1c-77e2-47f2-a8b1-0f5256717e53
⛔ Files ignored due to path filters (1)
flake.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
.gitignoreflake.niximmich-kioskinternal/config/config.gointernal/immich/immich_helpers.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
flake.nix (3)
57-61:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun the package build script here too.
This derivation still invokes the individual asset scripts, so the package-level TypeScript/typecheck step remains skipped.
Suggested change
- bun run css - bun run js - bun run url-builder + bun run build🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flake.nix` around lines 57 - 61, The buildPhase currently runs only the individual asset scripts (bun run css, bun run js, bun run url-builder) which skips the package-level TypeScript/typecheck step; update the buildPhase (the buildPhase block with those bun run commands) to also invoke the package build script (e.g., add the package-level command such as "bun run build" or the repository's package build script name) so the TypeScript/typecheck runs as part of the derivation and preserves the existing asset steps.
13-43:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPopulate the real per-system hashes before merging.
pkgs.lib.fakeHashkeeps every fixed-output derivation unresolved, sonix buildwill fail on all supported systems until the actual hashes are filled in.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flake.nix` around lines 13 - 43, Replace the placeholder pkgs.lib.fakeHash entries in nodeModulesHashes with the real fixed-output hashes for each platform so the node_modules derivation can validate output; to do this, build or evaluate the node_modules derivation (the derivation named node_modules using outputHash = nodeModulesHashes.${system}) on each target system (or use nix-prefetch / nix build to obtain the expected sha256) and copy the computed sha256 values into the corresponding keys ("x86_64-linux", "aarch64-linux", "x86_64-darwin", "aarch64-darwin") so outputHash no longer points to fakeHash and the derivation can succeed.
84-86:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCopy the asset contents, not the directory.
cp -r ${frontend}/assets frontend/public/assetswill createfrontend/public/assets/assets, which breaks the expectedgo:embed frontend/publiclayout.Suggested change
- cp -r ${frontend}/assets frontend/public/assets + cp -r ${frontend}/assets/. frontend/public/assets/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@flake.nix` around lines 84 - 86, The current copy step uses "cp -r ${frontend}/assets frontend/public/assets" which creates a nested assets/assets directory and breaks go:embed; change the copy to copy the contents into the target directory instead (e.g. replace the cp invocation so it copies ${frontend}/assets/. or ${frontend}/assets/* into frontend/public/assets/) while keeping the existing mkdir -p frontend/public/assets line, so the frontend/public/assets layout contains the asset files at its root.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@flake.nix`:
- Around line 57-61: The buildPhase currently runs only the individual asset
scripts (bun run css, bun run js, bun run url-builder) which skips the
package-level TypeScript/typecheck step; update the buildPhase (the buildPhase
block with those bun run commands) to also invoke the package build script
(e.g., add the package-level command such as "bun run build" or the repository's
package build script name) so the TypeScript/typecheck runs as part of the
derivation and preserves the existing asset steps.
- Around line 13-43: Replace the placeholder pkgs.lib.fakeHash entries in
nodeModulesHashes with the real fixed-output hashes for each platform so the
node_modules derivation can validate output; to do this, build or evaluate the
node_modules derivation (the derivation named node_modules using outputHash =
nodeModulesHashes.${system}) on each target system (or use nix-prefetch / nix
build to obtain the expected sha256) and copy the computed sha256 values into
the corresponding keys ("x86_64-linux", "aarch64-linux", "x86_64-darwin",
"aarch64-darwin") so outputHash no longer points to fakeHash and the derivation
can succeed.
- Around line 84-86: The current copy step uses "cp -r ${frontend}/assets
frontend/public/assets" which creates a nested assets/assets directory and
breaks go:embed; change the copy to copy the contents into the target directory
instead (e.g. replace the cp invocation so it copies ${frontend}/assets/. or
${frontend}/assets/* into frontend/public/assets/) while keeping the existing
mkdir -p frontend/public/assets line, so the frontend/public/assets layout
contains the asset files at its root.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/immich/immich_helpers.go (1)
734-744:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid the redundant
CheckForFacescall.
CheckForFacesonly populatesPeople, so the||still forces a face lookup whenever either slice is empty. That means assets that already have one populated face collection still pay for an unnecessary Immich round trip.🔧 Suggested fix
- if len(a.People) == 0 || len(a.UnassignedFaces) == 0 { + if len(a.People) == 0 && len(a.UnassignedFaces) == 0 { a.CheckForFaces(requestID, deviceID) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/immich/immich_helpers.go` around lines 734 - 744, The hasValidFilterExcludeFaces function currently calls CheckForFaces when either a.People or a.UnassignedFaces is empty, causing unnecessary Immich lookups because CheckForFaces only populates People; change the logic so CheckForFaces(requestID, deviceID) is invoked only when len(a.People) == 0 (i.e., People is empty), keep the final return as len(a.People) == 0 && len(a.UnassignedFaces) == 0, and update the conditional in hasValidFilterExcludeFaces to reference a.People and a.UnassignedFaces accordingly to avoid redundant calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/immich/immich_helpers.go`:
- Around line 734-744: The hasValidFilterExcludeFaces function currently calls
CheckForFaces when either a.People or a.UnassignedFaces is empty, causing
unnecessary Immich lookups because CheckForFaces only populates People; change
the logic so CheckForFaces(requestID, deviceID) is invoked only when
len(a.People) == 0 (i.e., People is empty), keep the final return as
len(a.People) == 0 && len(a.UnassignedFaces) == 0, and update the conditional in
hasValidFilterExcludeFaces to reference a.People and a.UnassignedFaces
accordingly to avoid redundant calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4f037cbc-928f-4874-93ac-66fd180014c6
📒 Files selected for processing (2)
internal/config/config.gointernal/immich/immich_helpers.go
Contributor
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 `@flake.nix`:
- Around line 13-18: The nodeModulesHashes map currently uses pkgs.lib.fakeHash
for "x86_64-linux", "aarch64-linux", and "x86_64-darwin", causing the
fixed-output derivation for node_modules to fail on those platforms; replace
each pkgs.lib.fakeHash entry in the nodeModulesHashes attribute with the actual
fixed-output hash computed on that platform (e.g., build the derivation or run
nix hash --base32 --type=sha256 on the produced tarball) or remove the
unsupported platform keys entirely so they are not advertised as supported for
the immich-kiosk target; update the "x86_64-linux", "aarch64-linux", and
"x86_64-darwin" entries accordingly in the nodeModulesHashes 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
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 by CodeRabbit
New Features
Chores