Feature/relative splitview images #300
Conversation
WalkthroughThis pull request introduces several enhancements, including new data structures and helper functions to support kiosk-related image processing. New fields for asset metadata (Bucket and BucketID) are added across multiple modules, along with a new options struct to refine image view handling. Additionally, a method to reset bucket arrays in configuration is implemented, and new layout constants are added. The modifications also refactor how image requests are processed via improved helper functions, while documentation comments are updated for clarity. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RequestHandler
participant ConfigHelper
participant AssetCreator
participant ProcessingHelpers
Client->>RequestHandler: ProcessViewImageDataWithOptions(requestConfig, context, isPrefetch, options)
RequestHandler->>ConfigHelper: setupRequestConfig(config)
RequestHandler->>AssetCreator: setupImmichAsset(config, orientation)
RequestHandler->>ProcessingHelpers: determineAllowedAssetTypes(config, isPrefetch)
RequestHandler->>ProcessingHelpers: handleRelativeAssetConfig(config, options)
RequestHandler->>ProcessingHelpers: handleFaceProcessing(image, asset, config, metadata)
RequestHandler->>ProcessingHelpers: convertImages(image, assetType, config, metadata, isPrefetch)
AssetCreator-->>RequestHandler: Asset populated (with Bucket/BucketID)
RequestHandler-->>Client: Return ViewImageData
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lint (1.62.2)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package : could not load export data: no export data for "github.com/damongolding/immich-kiosk/internal/templates/components/image"" ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
internal/routes/routes_asset_helpers.go (5)
379-383: Propagate errors or log them for more context.
Currently, the code returns an error directly to the caller. Adding a debug log before returning may ease diagnostics.+ log.Debug(metadata.requestID, "processAsset error:", err) return nil, err
396-400: Exception handling for conversion.
The approach to constructing two variants (normal and blurred) is sound, though consider logging or handling if either conversion fails.
410-419: Random user selection may cause unpredictability.
If random selection is undesired in certain scenarios, consider offering a deterministic ordering or explicit user pick to avoid confusion for kiosk operators.
474-488: Dual conversion approach for normal and blurred image.
This design is well-thought-out, enhancing code clarity. As an optional improvement, handle potential errors separately to log more specific failure cases.
639-645: Repeated second image logic.
This block largely duplicates the approach from the portrait splitview. Extracting common functionality into a helper function could improve maintainability.- for i := ... - if ... - break + fetchSecondSplitViewAsset(&viewData, ...)internal/immich/immich_favourites.go (1)
208-209: Consider using a distinct source category for favourites.The current implementation sets
Buckettokiosk.SourceAlbums, but favourites might be better represented as a distinct source category since they can span across multiple albums. Consider introducing a dedicated source category likekiosk.SourceFavouritesto better reflect the nature of favourite images.internal/immich/immich.go (1)
180-181: LGTM! Consider adding field documentation.The new fields are correctly added with appropriate JSON tags. Consider adding documentation comments to explain their purpose and expected values.
+// Bucket represents the source category of the asset (e.g., albums, date ranges) Bucket kiosk.Source `json:"-"` +// BucketID uniquely identifies the specific source within the bucket category BucketID string `json:"-"`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
internal/common/common.go(3 hunks)internal/config/config.go(1 hunks)internal/immich/immich.go(2 hunks)internal/immich/immich_album.go(1 hunks)internal/immich/immich_date.go(1 hunks)internal/immich/immich_favourites.go(1 hunks)internal/immich/immich_memories.go(1 hunks)internal/immich/immich_person.go(1 hunks)internal/immich/immich_random.go(1 hunks)internal/kiosk/kiosk.go(1 hunks)internal/routes/routes.go(1 hunks)internal/routes/routes_asset_helpers.go(6 hunks)internal/templates/partials/metadata.templ(2 hunks)
🔇 Additional comments (24)
internal/routes/routes_asset_helpers.go (14)
348-361: Well-structured documentation and function signature.
This docstring clearly outlines the purpose and usage of the function. It improves maintainability by clarifying each parameter and return value.
362-365: Validate fallback for empty headers.
Consider verifying scenarios wheredeviceIDorrequestIDmight be absent or blank, ensuring the rest of the flow handles missing metadata gracefully.
369-377: Collation of initial setup is neat.
By grouping request configuration, asset setup, and allowed asset types in one place, the code remains readable and maintainable. Keep an eye on concurrency if future changes rely on shared state.
385-387: Face processing logic is straightforward.
The code is neatly separated into a helper function, adhering to single-responsibility principles.
388-394: Robust image optimisation.
Processing remains optional and is neatly toggled by a config flag, keeping performance overhead in check.
421-429: Orientation-based asset setup.
Clear separation of orientation logic from asset creation is a positive approach, aiding future expansions or different orientation rules.
431-439: Conditional asset type determination.
The logic is straightforward, toggling video inclusion only when both the feature flag and isPrefetch conditions are met. This helps keep prefetch overhead manageable.
441-457: Relative asset config is properly isolated.
Resetting buckets before appending the specific type ensures a clean slate, preventing unexpected overlap with previous choices.
459-472: Efficient face check.
CallingCheckForFacesonly when necessary avoids redundant requests. Including a user-configurable approach ensures custom control over processing cost.
492-493: Convenient wrapper function.
Providing a default invocation ofprocessViewImageDatawithout orientation constraints is a good pattern for simpler usage.
495-497: Extended functionality with options.
TheProcessViewImageDataWithOptionsmethod neatly encapsulates the newly introduced parameters, ensuring backwards compatibility.
575-582: Conditional orientation assignment.
Defining options for landscape or portrait explicitly aligns with the kiosk layout while preserving flexibility. Ensure that any future layouts receive a similar block.
599-609: Second image retrieval in splitview.
This incremental approach to fetch a second portrait-oriented image is sound. Consider randomised fallback if the second image picks the same asset.
619-635: Landscape splitview logic.
Mirroring the portrait splitview approach ensures consistent behaviour. The code is nicely self-documenting.internal/kiosk/kiosk.go (1)
17-20: New layout constants are neatly introduced.
These provide more flexible options for kiosk modes without impacting existing logic. Consider grouping or naming them in a manner that highlights their usage scope (e.g., “LayoutMode”).internal/routes/routes.go (1)
41-47: requestMetadata struct solidifies request context.
This newly added structure is a good practice, bundling key request data consistently and facilitating future expansions (e.g., adding more context fields).internal/immich/immich_random.go (1)
128-130: LGTM! Source tracking enhancement.The addition of
asset.Bucket = kiosk.SourceRandombefore the asset assignment properly tracks the source of randomly selected images.internal/common/common.go (2)
103-103: LGTM! Improved documentation.The added comment clarifies the purpose of the User field.
116-121: LGTM! Well-structured options type.The new ViewImageDataOptions struct is well-designed with clear field names and appropriate types for handling relative asset relationships.
internal/immich/immich_memories.go (1)
159-162: LGTM! Enhanced memory asset metadata.The changes properly set the source bucket and memory title before assigning the asset, maintaining clear asset origin tracking.
internal/immich/immich_person.go (1)
168-170: LGTM! Comprehensive person asset metadata.The changes properly set both the source bucket and person ID, providing complete context for person-related assets.
internal/immich/immich_date.go (1)
139-140: LGTM! Appropriate source categorisation for date range assets.The implementation correctly categorises date range assets with a distinct source and uses the date range as the bucket identifier.
internal/immich/immich_album.go (1)
272-273: LGTM! Appropriate source categorisation for album assets.The implementation correctly categorises album assets with the appropriate source and uses the album ID as the bucket identifier.
internal/config/config.go (1)
433-441: LGTM! Well-documented and properly implemented.The
ResetBucketsmethod effectively centralises the logic for clearing asset bucket fields, making the code more maintainable and reducing the likelihood of inconsistencies. The documentation clearly explains the purpose and importance of resetting these fields.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/routes/routes_asset_helpers.go (3)
412-419: Add defensive check for empty user list.The function should handle the case where
config.Useris nil to prevent potential panics.Apply this diff to add the defensive check:
func setupRequestConfig(config *config.Config) { - if len(config.User) > 0 { + if config.User != nil && len(config.User) > 0 { randomIndex := rand.IntN(len(config.User)) config.SelectedUser = config.User[randomIndex] } else { config.SelectedUser = "" } }
578-578: Fix typo in constant name.The constant name has a typo: "Attepmts" should be "Attempts".
Apply this diff to fix the typo:
-const maxImageRetrievalAttepmts = 3 +const maxImageRetrievalAttempts = 3
580-592: Consider adding exponential backoff to retry logic.The retry logic could benefit from exponential backoff to handle temporary failures more gracefully.
Apply this diff to add exponential backoff:
- for i := 0; i < maxImageRetrievalAttempts; i++ { + for i := 0; i < maxImageRetrievalAttempts; i++ { + if i > 0 { + time.Sleep(time.Duration(1<<uint(i-1)) * time.Second) + } viewDataSplitViewSecond, err := ProcessViewImageDataWithOptions(requestConfig, c, isPrefetch, options) if err != nil { return err }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/routes/routes_asset_helpers.go(5 hunks)
🔇 Additional comments (1)
internal/routes/routes_asset_helpers.go (1)
348-361: LGTM! Well-documented function signature change.The updated function signature with
ViewImageDataOptionsparameter improves flexibility and maintainability. The documentation is clear and comprehensive.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/immich/immich_helpers.go (1)
33-34: Fix typo in documentation comment.The word "impliment" should be "implement".
-// withImmichApiCache Decorator to impliment cache for the immichApiCall func +// withImmichApiCache Decorator to implement cache for the immichApiCall func
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
internal/immich/immich_album.go(3 hunks)internal/immich/immich_date.go(2 hunks)internal/immich/immich_faces.go(1 hunks)internal/immich/immich_favourites.go(3 hunks)internal/immich/immich_helpers.go(2 hunks)internal/immich/immich_memories.go(2 hunks)internal/immich/immich_person.go(3 hunks)internal/immich/immich_random.go(2 hunks)taskfile.yml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- taskfile.yml
🚧 Files skipped from review as they are similar to previous changes (6)
- internal/immich/immich_date.go
- internal/immich/immich_album.go
- internal/immich/immich_person.go
- internal/immich/immich_random.go
- internal/immich/immich_favourites.go
- internal/immich/immich_memories.go
🔇 Additional comments (2)
internal/immich/immich_faces.go (1)
80-80: LGTM! The function name change improves clarity.The renaming from
immichApiCallDecoratortowithImmichApiCachebetter reflects the function's purpose of managing API call caching.internal/immich/immich_helpers.go (1)
274-274: LGTM! The function call is consistently updated.The change to use
withImmichApiCachealigns with the function renaming.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/routes/routes_asset_helpers.go (1)
462-472:⚠️ Potential issueAdd error handling for face detection.
The
CheckForFacescall should include error handling to prevent potential failures from being silently ignored.Apply this diff to add error handling:
func handleFaceProcessing(img image.Image, asset *immich.ImmichAsset, config config.Config, metadata requestMetadata) image.Image { if strings.EqualFold(config.ImageEffect, "smart-zoom") && len(asset.People)+len(asset.UnassignedFaces) == 0 { - asset.CheckForFaces(metadata.requestID, metadata.deviceID) + if err := asset.CheckForFaces(metadata.requestID, metadata.deviceID); err != nil { + log.Error("Failed to check for faces", "error", err) + } } if ShouldDrawFacesOnImages() { log.Debug("Drawing faces") return DrawFaceOnImage(img, asset) } return img }
🧹 Nitpick comments (1)
internal/routes/routes_asset_helpers.go (1)
578-592: Enhance error handling in fetchSecondSplitViewAsset.The function should return a more descriptive error after maximum attempts, and the magic number should be documented.
Apply this diff to improve the implementation:
const maxImageRetrievalAttempts = 3 +// maxImageRetrievalAttempts defines the maximum number of attempts to find a unique +// second image for split view layouts before giving up to prevent infinite loops. for i := 0; i < maxImageRetrievalAttempts; i++ { viewDataSplitViewSecond, err := ProcessViewImageDataWithOptions(requestConfig, c, isPrefetch, options) if err != nil { return err } if viewDataSplitView.ImmichAsset.ID != viewDataSplitViewSecond.ImmichAsset.ID { viewData.Assets = append(viewData.Assets, viewDataSplitViewSecond) return nil } } -return nil +return fmt.Errorf("failed to find unique second image after %d attempts", maxImageRetrievalAttempts)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/immich/immich_helpers.go(2 hunks)internal/routes/routes_asset_helpers.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/immich/immich_helpers.go
🔇 Additional comments (1)
internal/routes/routes_asset_helpers.go (1)
361-408: Excellent refactoring of processViewImageData!The function has been well-structured with clear separation of concerns, making it more maintainable and easier to test.
Summary by CodeRabbit
New Features
Refactor