api recursion #267
api recursion #267
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"" WalkthroughThe changes in the Changes
Possibly related PRs
Suggested labels
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: 0
🧹 Nitpick comments (4)
internal/immich/immich_album.go (4)
114-121: Consider enhancing the albumID parameter documentation.The documentation could be more specific about the valid special keywords. Consider listing the supported keywords (AlbumKeywordAll, AlbumKeywordShared, AlbumKeywordFavourites, AlbumKeywordFavorites) in the documentation.
177-177: Consider using iteration instead of recursion for retries.The current recursive approach for retries could potentially lead to stack overflow in edge cases. Consider refactoring to use a loop-based retry mechanism instead.
-func (i *ImmichAsset) imageFromAlbum(albumID string, albumAssetsOrder ImmichAssetOrder, requestID, deviceID string, isPrefetch bool, retries int) error { +func (i *ImmichAsset) imageFromAlbum(albumID string, albumAssetsOrder ImmichAssetOrder, requestID, deviceID string, isPrefetch bool) error { + for retries := 0; retries < MaxRetries; retries++ { if retries >= MaxRetries { return fmt.Errorf("No images found for '%s'. Max retries reached.", albumID) } // ... existing logic ... - return i.imageFromAlbum(albumID, albumAssetsOrder, requestID, deviceID, isPrefetch, retries+1) + continue + } + return fmt.Errorf("No images found for '%s'. Max retries reached.", albumID) }Also applies to: 225-226
Line range hint
169-169: Use structured logging consistently.Replace string concatenation in log messages with structured logging for consistency and better log processing:
-log.Debug(requestID+" No images left in cache. Refreshing and trying again for album", albumID) +log.Debug("No images left in cache. Refreshing and trying again", "requestID", requestID, "albumID", albumID) -log.Debug(requestID + " No viable images left in cache. Refreshing and trying again") +log.Debug("No viable images left in cache. Refreshing and trying again", "requestID", requestID)Also applies to: 224-224
239-240: Consider wrapping the error with additional context.When delegating to the internal method, consider wrapping the error with additional context about the operation:
-return i.imageFromAlbum(albumID, albumAssetsOrder, requestID, deviceID, isPrefetch, 0) +if err := i.imageFromAlbum(albumID, albumAssetsOrder, requestID, deviceID, isPrefetch, 0); err != nil { + return fmt.Errorf("failed to fetch image from album %s: %w", albumID, err) +} +return nil
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/immich/immich_album.go(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (typescript)
🔇 Additional comments (1)
internal/immich/immich_album.go (1)
157-159: Verify MaxRetries constant declaration.The MaxRetries constant is used but not visible in the provided code. Ensure it's properly declared and documented.
✅ Verification successful
MaxRetries constant is properly declared but could benefit from documentation.
The constant is correctly declared in
internal/immich/immich.gowith a reasonable value of 3 retries. Consider adding a brief documentation comment to explain its purpose and usage context.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for MaxRetries constant declaration rg -l "MaxRetries.*=.*\d+"Length of output: 54
Script:
#!/bin/bash # View MaxRetries constant declaration with context rg "MaxRetries.*=.*\d+" -B 2 -A 2 internal/immich/immich.goLength of output: 139
Script:
#!/bin/bash # View the complete const block with any preceding documentation rg "const \(" -B 5 -A 10 internal/immich/immich.goLength of output: 469
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
internal/immich/immich_random.go (1)
26-28: Consider improving the error message format.The error message could be more consistent with Go's error message style guide.
- return fmt.Errorf("No images found for random. Max retries reached.") + return fmt.Errorf("no images found for random: max retries reached")internal/immich/immich_favourites.go (2)
97-99: Align error message format with other files.For consistency, the error message format should match the style used in other files.
- return fmt.Errorf("No images found for favourites. Max retries reached.") + return fmt.Errorf("no images found for favourites: max retries reached")
Line range hint
182-182: Fix typo in comment.There's a typo in the comment: "cwith" should be "with".
- // replace cwith cache minus used image + // replace with cache minus used image
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
internal/immich/immich_favourites.go(3 hunks)internal/immich/immich_person.go(3 hunks)internal/immich/immich_random.go(3 hunks)
🧰 Additional context used
📓 Learnings (1)
internal/immich/immich_person.go (1)
Learnt from: damongolding
PR: damongolding/immich-kiosk#252
File: internal/immich/immich_person.go:101-103
Timestamp: 2025-01-07T16:39:18.914Z
Learning: In immich-kiosk, the `RandomImageOfPerson` function is only called after validating that the person has images through `PersonImageCount` in `gatherAssetBuckets`, so it doesn't need retry limits for handling no-image scenarios. The recursive call is only used for cache refresh.
🔇 Additional comments (5)
internal/immich/immich_random.go (2)
14-19: LGTM! Well-documented public interface.The public method is well-documented and correctly delegates to the internal implementation with initial retry count.
88-88: Verify retry increment logic.The retry increment logic appears in two places. Whilst this is correct for the current implementation, we should ensure it's properly tested to avoid potential infinite loops.
Also applies to: 119-119
✅ Verification successful
Retry logic implementation is safe and consistent
The retry mechanism is properly implemented with a global
MaxRetries = 3limit, preventing infinite loops. The increment logic is consistently applied across all image retrieval functions with appropriate error handling.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases covering retry scenarios rg -l "test.*retry" --type goLength of output: 31
Script:
#!/bin/bash # Find the implementation of randomImage and surrounding context ast-grep --pattern 'func $_ randomImage($$$) { $$$ }' # Search for retry-related constants or variables rg -i "max.*retries|retry.*limit" --type go # Find all retry increment patterns rg "retries\s*\+\s*1" --type go -A 3Length of output: 4431
internal/immich/immich_person.go (2)
48-55: LGTM! Clear and consistent interface.The public method follows the same pattern as other files, with clear documentation.
66-68: Consider if retry limits are necessary here.Based on previous feedback,
RandomImageOfPersonis only called after validating that the person has images throughPersonImageCount. The retry logic might be unnecessary in this context.internal/immich/immich_favourites.go (1)
83-95: LGTM! Excellent documentation.The function documentation is comprehensive and well-structured, following Go's documentation conventions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/immich/immich_person.go (1)
48-155: 🛠️ Refactor suggestionRe-evaluate the necessity of the retry mechanism in
RandomImageOfPersonPrevious learnings indicate that
RandomImageOfPersonis called only after confirming the person has associated images viaPersonImageCount. Introducing a retry loop may be redundant and could lead to unnecessary API calls. Consider simplifying the function by removing the retry mechanism and handling cache refresh differently.As per retrieved learnings:
In immich-kiosk, the `RandomImageOfPerson` function is only called after validating that the person has images through `PersonImageCount` in `gatherAssetBuckets`, so it doesn't need retry limits for handling no-image scenarios. The recursive call is only used for cache refresh.
🧹 Nitpick comments (11)
internal/immich/immich_date.go (2)
75-166: Optimise error handling within the retry loopIn the retry loop, any error encountered (e.g., parsing URL, marshaling request body, API call failures) results in immediate termination of the function without further retries. Consider refining the error handling to retry on transient errors, such as network issues, while failing fast on non-recoverable errors. This will enhance the robustness of the function.
135-136: Simplify conditionals for image filteringThe conditional statement used for filtering images could be more readable by breaking it into multiple lines or adding explanatory comments. This will improve maintainability and clarity of the code.
Apply this diff to enhance readability:
- if img.Type != ImageType || img.IsTrashed || (img.IsArchived && !requestConfig.ShowArchived) || !i.ratioCheck(&img) { + isInvalidType := img.Type != ImageType + isTrashed := img.IsTrashed + isArchived := img.IsArchived && !requestConfig.ShowArchived + isInvalidRatio := !i.ratioCheck(&img) + if isInvalidType || isTrashed || isArchived || isInvalidRatio {internal/immich/immich_person.go (1)
99-100: Handle potential errors when marshalling request bodyUsing
log.Fatalon marshalling errors will terminate the program. It's preferable to return the error to allow the calling function to handle it appropriately.Apply this diff to return the error instead:
- log.Fatal("marshaling request body", err) + return fmt.Errorf("marshalling request body: %w", err)internal/immich/immich_favourites.go (4)
84-108: Consider documenting the MaxRetries constant valueThe documentation thoroughly explains the retry mechanism but doesn't specify the actual number of retries. Consider adding this information to help users understand the timeout behaviour.
117-170: Consider handling cache deletion errorsThe cache deletion operation on line 170 doesn't check for errors. While cache errors might not be critical, logging them could help with debugging cache-related issues.
- cache.Delete(apiCacheKey) + if err := cache.Delete(apiCacheKey); err != nil { + log.Debug(requestID + " Failed to delete cache", "error", err) + }
174-197: Improve error handling in cache update sectionThe error handling for cache operations could be more informative. Consider adding context to the error message when marshalling fails.
- log.Error("Failed to marshal immichAssetsToCache", "error", err) + log.Error("Failed to marshal immichAssetsToCache", + "error", err, + "deviceID", deviceID, + "retryAttempt", retries)
200-204: Enhance error message with retry countConsider including the number of retry attempts in the error message for better debugging context.
- return fmt.Errorf("No images found for favourites. Max retries reached.") + return fmt.Errorf("No images found for favourites after %d attempts", MaxRetries)internal/immich/immich_album.go (4)
Line range hint
114-153: Consider extracting error messages as constantsThe error messages are well-formed but could be extracted as constants to maintain consistency and ease maintenance.
+const ( + errGetAllAlbums = "failed to get all albums: %w" + errGetSharedAlbums = "failed to get shared albums: %w" + errGetFavorites = "failed to get favorite images: %w" + errGetAlbumAssets = "failed to get album assets for album %s: %w" +) func (i *ImmichAsset) AlbumImageCount(albumID string, requestID, deviceID string) (int, error) { switch albumID { case kiosk.AlbumKeywordAll: albums, _, err := i.allAlbums(requestID, deviceID) if err != nil { - return 0, fmt.Errorf("failed to get all albums: %w", err) + return 0, fmt.Errorf(errGetAllAlbums, err) }
154-167: Add example usage to documentationConsider adding an example usage section to the documentation to demonstrate typical use cases and parameter values.
170-188: Enhance log message with retry attempt countConsider adding the retry attempt count to the log message for better debugging context.
- log.Debug(requestID+" No images left in cache. Refreshing and trying again for album", albumID) + log.Debug(requestID+" No images left in cache. Refreshing and trying again for album", + "albumID", albumID, + "retryAttempt", retries + 1, + "maxRetries", MaxRetries)
209-238: Maintain consistency with favourites implementationConsider applying the same error handling improvements suggested for the favourites implementation:
- Add context to cache operation errors
- Include retry count in the final error message
- return fmt.Errorf("No images found for '%s'. Max retries reached.", albumID) + return fmt.Errorf("No images found for album '%s' after %d attempts", albumID, MaxRetries)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
internal/immich/immich_album.go(2 hunks)internal/immich/immich_date.go(2 hunks)internal/immich/immich_favourites.go(2 hunks)internal/immich/immich_memories.go(1 hunks)internal/immich/immich_person.go(1 hunks)internal/immich/immich_random.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/immich/immich_random.go
🧰 Additional context used
📓 Learnings (1)
internal/immich/immich_person.go (1)
Learnt from: damongolding
PR: damongolding/immich-kiosk#252
File: internal/immich/immich_person.go:101-103
Timestamp: 2025-01-07T16:39:18.914Z
Learning: In immich-kiosk, the `RandomImageOfPerson` function is only called after validating that the person has images through `PersonImageCount` in `gatherAssetBuckets`, so it doesn't need retry limits for handling no-image scenarios. The recursive call is only used for cache refresh.
🔇 Additional comments (1)
internal/immich/immich_memories.go (1)
77-152: Function improvements acceptedThe refactoring of
RandomMemoryLaneImageenhances readability and simplifies the control flow by replacing recursion with a loop for retries. The caching logic and error handling are well-maintained.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
internal/immich/immich_album.go (3)
170-175: Consider extracting MaxRetries to a configurationThe retry mechanism is well-implemented, but the
MaxRetriesconstant could be moved to the configuration to allow for runtime adjustments.
180-181: Consider using constants for debug messagesDebug messages are duplicated and could benefit from being defined as constants to maintain consistency and ease maintenance.
+const ( + msgNoImagesInCache = "No images left in cache. Refreshing and trying again for album" + msgNoViableImages = "No viable images left in cache. Refreshing and trying again" +)Also applies to: 240-241
215-230: Improve cache error handlingThe cache error handling could be more robust. Consider implementing a retry mechanism for cache updates or degrading gracefully when cache operations fail.
if requestConfig.Kiosk.Cache { // Remove the current image from the slice assetsToCache := album assetsToCache.Assets = append(album.Assets[:assetIndex], album.Assets[assetIndex+1:]...) jsonBytes, err := json.Marshal(assetsToCache) if err != nil { - log.Error("Failed to marshal assetsToCache", "error", err) - return err + log.Error("Failed to marshal assetsToCache, continuing without cache update", "error", err) + return nil } // replace with cache minus used asset err = cache.Replace(apiCacheKey, jsonBytes) if err != nil { - log.Debug("Failed to update device cache for album", "albumID", albumID, "deviceID", deviceID) + log.Error("Failed to update device cache", + "albumID", albumID, + "deviceID", deviceID, + "error", err, + ) } }internal/immich/immich_date.go (2)
75-82: Consider extracting URL parsing to a helper function.This URL parsing logic is repeated across multiple methods. Consider extracting it to a shared helper function to reduce code duplication.
+func (i *ImmichAsset) parseImmichURL() (*url.URL, error) { + return url.Parse(requestConfig.ImmichUrl) +}
127-129: Cache refresh could be more efficient.Consider implementing a bulk refresh mechanism instead of deleting and refetching immediately. This could reduce API load during high-traffic periods.
internal/immich/immich_favourites.go (1)
199-201: Enhance debug logging for cache operations.The current debug message "cache not found!" is not very informative. Consider including more context about the cache operation and the specific key that wasn't found.
-log.Debug("cache not found!") +log.Debug("Failed to replace cache entry", "key", apiCacheKey, "error", err)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
internal/immich/immich_album.go(4 hunks)internal/immich/immich_date.go(2 hunks)internal/immich/immich_faces.go(1 hunks)internal/immich/immich_favourites.go(4 hunks)internal/immich/immich_memories.go(2 hunks)internal/immich/immich_person.go(2 hunks)internal/immich/immich_random.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/immich/immich_memories.go
- internal/immich/immich_random.go
🧰 Additional context used
📓 Learnings (1)
internal/immich/immich_person.go (1)
Learnt from: damongolding
PR: damongolding/immich-kiosk#252
File: internal/immich/immich_person.go:101-103
Timestamp: 2025-01-07T16:39:18.914Z
Learning: In immich-kiosk, the `RandomImageOfPerson` function is only called after validating that the person has images through `PersonImageCount` in `gatherAssetBuckets`, so it doesn't need retry limits for handling no-image scenarios. The recursive call is only used for cache refresh.
🔇 Additional comments (8)
internal/immich/immich_album.go (4)
24-24: Consistent error handling implementation looks good!The use of
immichApiFailhelper function provides a uniform approach to handling API failures.Also applies to: 76-76
Line range hint
114-153: Well-structured implementation with comprehensive error handling!The function effectively handles different album types with proper error wrapping and clear error messages. The documentation is thorough and follows good practices.
191-202: Well-implemented ordering logic!The implementation efficiently handles both random and ascending order using the standard library's
slicespackage. TheAssetsOrderedflag prevents unnecessary reordering.
203-214: Clear and efficient validation logic!The asset validation criteria are well-structured and clearly documented. The use of descriptive boolean variables makes the code self-documenting.
internal/immich/immich_faces.go (1)
16-18: Improved error handling approach!The changes enhance robustness by replacing fatal error handling with proper error logging and propagation.
internal/immich/immich_date.go (1)
16-30: Well-documented function with clear explanations!The comprehensive documentation clearly explains the function's purpose, parameters, and behaviour.
internal/immich/immich_person.go (1)
67-75: Retry mechanism may be unnecessary here.Based on previous discussions,
RandomImageOfPersonis only called after validating that the person has images throughPersonImageCount. Consider simplifying this implementation since the retry logic might be redundant.internal/immich/immich_favourites.go (1)
86-111: Excellent documentation with clear examples!The documentation thoroughly explains the function's behaviour, retry mechanism, and filtering criteria.
Summary by CodeRabbit
The changes focus on making the image retrieval process more reliable and informative, with better handling of potential retrieval failures across various functionalities.