add memories asset bucket #266
Conversation
|
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"" WalkthroughThis pull request introduces enhancements for memory lane functionality within the application. It includes the addition of a new boolean field, Changes
Sequence DiagramsequenceDiagram
participant User
participant Config
participant ImmichAsset
participant API
User->>Config: Request images with Memories=true
Config->>ImmichAsset: Call MemoryLaneAssetsCount
ImmichAsset->>API: Fetch memory lane data
API-->>ImmichAsset: Return memories
ImmichAsset->>ImmichAsset: Select random memory image
ImmichAsset-->>User: Return memory lane image
Possibly related PRs
Poem
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: 2
🧹 Nitpick comments (2)
internal/immich/immich_memories.go (1)
32-33: Correct typographical errors in commentsThere are typographical errors in the comments: "seperate" should be "separate", and "becasue" should be "because".
Apply this diff to correct the typos:
- // If we want the memories assets count we will use a seperate cache entry - // becasue Kiosk removes used assets from the normal cache entry + // If we want the memories assets count, we will use a separate cache entry + // because Kiosk removes used assets from the normal cache entryinternal/config/config.go (1)
207-208: LGTM! Consider adding documentation.The
Memoriesfield is well-integrated with appropriate tags. Consider adding a documentation comment to describe its purpose and behaviour, following the pattern used for other fields in the struct.Add this documentation above the field:
+ // Memories enables the display of memory lane assets Memories bool `json:"memories" mapstructure:"memories" query:"memories" form:"memories" default:"false"`
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
internal/config/config.go(2 hunks)internal/immich/immich.go(2 hunks)internal/immich/immich_album.go(3 hunks)internal/immich/immich_date.go(1 hunks)internal/immich/immich_memories.go(1 hunks)internal/kiosk/kiosk.go(1 hunks)internal/routes/routes_image_helpers.go(2 hunks)internal/templates/components/image/metadata.templ(2 hunks)
🔇 Additional comments (11)
internal/kiosk/kiosk.go (1)
15-15: Looks good!The addition of
SourceMemoriescorrectly extends theSourcetype to include memories as a new source.internal/immich/immich_date.go (1)
42-47: Looks good!The date parsing logic has been corrected to properly extract the start and end dates from the
dateRangestring.internal/templates/components/image/metadata.templ (2)
Line range hint
123-131: LGTM! The function name change better reflects its purpose.The renaming from
shouldShowAlbumPersonNametoshouldShowSourceNameand the inclusion ofSourceMemoriesin the album source check aligns well with the broader scope of source types.
137-141: LGTM! Variable name updated for consistency.The variable renaming from
showAlbumPersonNametoshowSourceNamemaintains consistency with the function name change.internal/immich/immich_album.go (2)
30-30: LGTM! More robust URL path construction.Using
path.Joinis a more robust way to construct URL paths as it handles path separators correctly.
Line range hint
105-110: LGTM! Function scope change improves code organisation.Moving
countAssetsInAlbumsfrom a method to a standalone function is appropriate as it doesn't require access to the receiver's state.internal/immich/immich.go (2)
221-225: LGTM! Well-structured type definition for memory lane data.The
MemoryLaneResponsetype effectively models the memory lane data structure with appropriate JSON tags.
236-236: LGTM! Interface updated to support new response type.The
ImmichApiResponseinterface has been correctly updated to includeMemoryLaneResponse.internal/routes/routes_image_helpers.go (2)
85-94: LGTM! Memory lane asset handling is well-implemented.The implementation correctly:
- Checks the configuration flag
- Handles the error case of zero assets
- Adds appropriate weighting for memory lane assets
150-151: LGTM! Memory lane image retrieval case added.The new case for
SourceMemoriesaligns well with the existing pattern of source-specific image retrieval.internal/config/config.go (1)
414-414: LGTM! Consistent handling of query parameters.The addition of the
memoriesparameter to the condition maintains consistency with how other image source parameters are handled.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/immich/immich_memories.go (1)
25-26:⚠️ Potential issueAvoid using
log.Fatalin library code; return error insteadUsing
log.Fatal(err)will terminate the entire application upon encountering an error, which may not be appropriate in library code. Instead, return the error to allow the caller to handle it gracefully.Apply this diff to handle the error properly:
if err != nil { - log.Fatal(err) + return memoryLane, "", err }
🧹 Nitpick comments (3)
internal/immich/immich_memories.go (3)
38-38: Fix typo in comment: 'becasue' → 'because'There is a typo in the comment; correcting it improves readability.
Apply this diff to correct the typo:
- // becasue Kiosk removes used assets from the normal cache entry + // because Kiosk removes used assets from the normal cache entry
58-58: Fix typo in comment: 'sepetate' → 'separate'There is a typo in the comment; correcting it improves readability.
Apply this diff to correct the typo:
-// that counts the total assets in memory lane using a sepetate cache entry +// that counts the total assets in memory lane using a separate cache entry
118-118: Clarify comment for asset filtering conditionThe comment can be improved for clarity. Suggest rephrasing to enhance readability.
Apply this diff to rephrase the comment:
- // We only want images and that are not trashed or archived (unless wanted by user) + // We only want images that are not trashed or archived (unless desired by user)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
README.md(2 hunks)config.example.yaml(1 hunks)internal/immich/immich_memories.go(1 hunks)
🔇 Additional comments (5)
internal/immich/immich_memories.go (2)
6-6:⚠️ Potential issueCorrect import path for 'math/rand' package
The import statement
import ("math/rand/v2")may be incorrect, as the standard library'smath/randpackage does not use a versioned path. Unless you are using a specific versioned package, you should importmath/randdirectly to prevent compile errors.Apply this diff to fix the import statement:
-import ( - "math/rand/v2" +import ( + "math/rand"Likely invalid or redundant comment.
88-88: Verify thatMaxRetriesis defined and properly scopedEnsure that
MaxRetriesis defined and accessible within this context to avoid undefined variable errors.Run the following script to check for the definition of
MaxRetries:✅ Verification successful
MaxRetriesconstant is properly defined and accessibleThe constant is defined in
internal/immich/immich.gowith a value of 3 and is correctly accessible within the package scope.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for the definition of MaxRetries in the codebase # Expected: MaxRetries should be defined and accessible in the package # Search for MaxRetries definition rg -A 2 -B 2 'const\s+MaxRetries\s*='Length of output: 39
Script:
#!/bin/bash # Search for any MaxRetries declarations or assignments rg -i "maxretries\s*[=:]" -A 2 -B 2 # Search for any MaxRetries usage to understand context rg -i "\bmaxretries\b" -A 2 -B 2 # Use ast-grep to find variable declarations ast-grep --pattern 'var MaxRetries = $_' ast-grep --pattern 'const MaxRetries = $_'Length of output: 1406
config.example.yaml (1)
37-37: Addition of 'memories' configuration option looks goodThe new
memories: false # show memoriesconfiguration option is correctly added and properly commented.README.md (2)
250-250: New configuration option 'KIOSK_MEMORIES' added correctlyThe addition of
KIOSK_MEMORIES: falseto the environment variables in the Docker Compose example is accurate and follows the existing format.
355-355: Configuration table updated with 'memories' option appropriatelyThe
memoriesoption has been correctly added to the configuration table, including its description and default value.
internal/immich/immich_memories.go
Outdated
| assetsToCache := memories | ||
| assetsToCache[pickedMemoryIndex].Assets = append(assetsToCache[pickedMemoryIndex].Assets[:assetIndex], assetsToCache[pickedMemoryIndex].Assets[assetIndex+1:]...) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid modifying shared data when updating cache
Assigning assetsToCache := memories copies the slice headers but shares the underlying data. Modifying assetsToCache[pickedMemoryIndex].Assets will also modify memories[pickedMemoryIndex].Assets, which might lead to unintended side effects. To prevent this, create deep copies of the memories before modifying them.
Consider using the following approach to deep copy the memories slice:
// Create a deep copy of memories
assetsToCache := make(MemoryLaneResponse, len(memories))
for i, memory := range memories {
assetsToCache[i].YearsAgo = memory.YearsAgo
assetsToCache[i].Title = memory.Title
assetsToCache[i].Assets = make([]ImmichAsset, len(memory.Assets))
copy(assetsToCache[i].Assets, memory.Assets)
}There was a problem hiding this comment.
can I use the copy func?
There was a problem hiding this comment.
@damongolding The built-in copy function performs a shallow copy, which means it would still share the underlying data for nested slices. However, you can use it in combination with new slice allocation to achieve a deep copy:
// Create a deep copy of memories
assetsToCache := make(MemoryLaneResponse, len(memories))
for i, memory := range memories {
assetsToCache[i].YearsAgo = memory.YearsAgo
assetsToCache[i].Title = memory.Title
// Create a new slice and use copy
assetsToCache[i].Assets = make([]ImmichAsset, len(memory.Assets))
copy(assetsToCache[i].Assets, memory.Assets)
}This ensures that modifying assetsToCache won't affect the original memories slice.
(_/)
(•.•)
(>🔍)>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
internal/immich/immich_memories.go (4)
30-35: Consider extracting API path components as constantsThe API path components could be extracted as package-level constants to improve maintainability and reduce the risk of typos.
+const ( + apiBasePath = "api" + assetsPath = "assets" + memoryLanePath = "memory-lane" +) apiUrl := url.URL{ Scheme: u.Scheme, Host: u.Host, - Path: path.Join("api", "assets", "memory-lane"), + Path: path.Join(apiBasePath, assetsPath, memoryLanePath), RawQuery: fmt.Sprintf("month=%d&day=%d", now.Month(), now.Day()), }
68-72: Consider logging errors before returning 0Silently returning 0 on error might mask issues. Consider logging the error for debugging purposes.
m, _, err := i.memories(requestID, deviceID, true) if err != nil { + log.Debug("Failed to fetch memory lane assets count", "error", err, "requestID", requestID) return 0 }
123-152: Consider extracting cache update logic to a separate methodThe caching logic is complex enough to warrant its own method. This would improve readability and maintainability.
Consider extracting the cache update logic to a new method like
updateMemoryLaneCache(memories MemoryLaneResponse, pickedMemoryIndex, assetIndex int, apiCacheKey string) error.
148-150: Improve error logging for cache update failureThe debug log for cache update failure should include the error details.
err = cache.Replace(apiCacheKey, jsonBytes) if err != nil { - log.Debug("Failed to update device cache for memories", "deviceID", deviceID) + log.Debug("Failed to update device cache for memories", "error", err, "deviceID", deviceID) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/immich/immich_memories.go(1 hunks)
🔇 Additional comments (2)
internal/immich/immich_memories.go (2)
23-26: Replace log.Fatal with proper error handlingUsing
log.Fatalwill terminate the entire application upon encountering an error, which isn't appropriate for library code. The caller should handle the error gracefully.Apply this diff:
u, err := url.Parse(requestConfig.ImmichUrl) if err != nil { - log.Fatal(err) + return memoryLane, "", fmt.Errorf("failed to parse Immich URL: %w", err) }
59-65: Implementation looks good!The function correctly calculates the total number of assets across all memories.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
internal/immich/immich_memories.go (1)
24-26:⚠️ Potential issueReplace
log.Fatalwith proper error handlingUsing
log.Fatalwill terminate the entire application upon encountering an error, which isn't appropriate for library code. Return the error to allow the caller to handle it gracefully.Apply this diff:
if err != nil { - log.Fatal(err) + return memoryLane, "", fmt.Errorf("failed to parse Immich URL: %w", err) }
🧹 Nitpick comments (3)
internal/immich/immich_memories.go (3)
68-75: Consider logging errors before returning 0Silent error handling might mask issues from users. Consider logging the error with appropriate context before returning 0.
m, _, err := i.memories(requestID, deviceID, true) if err != nil { + log.Debug("Failed to fetch memory lane assets count", + "error", err, + "requestID", requestID, + "deviceID", deviceID) return 0 }
148-150: Improve error logging for cache updatesThe debug message could be more descriptive and include the actual error.
err = cache.Replace(apiCacheKey, jsonBytes) if err != nil { - log.Debug("Failed to update device cache for memories", "deviceID", deviceID) + log.Debug("Failed to update device cache for memories", + "error", err, + "deviceID", deviceID, + "apiCacheKey", apiCacheKey) }
154-156: Consider using a constructor or setter method instead of direct assignmentDirect modification of the receiver state can be error-prone and harder to maintain. Consider using a dedicated method for updating the asset state.
- *i = asset - i.KioskSourceName = memories[pickedMemoryIndex].Title + i.updateFromMemoryAsset(asset, memories[pickedMemoryIndex].Title) // Add this method: func (i *ImmichAsset) updateFromMemoryAsset(asset ImmichAsset, title string) { *i = asset i.KioskSourceName = title }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/immich/immich_memories.go(1 hunks)
🔇 Additional comments (1)
internal/immich/immich_memories.go (1)
59-65: LGTM! Clean and efficient implementationThe function is well-focused and correctly implements the asset counting logic.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Technical Updates