v0.16.0 #295
v0.16.0 #295
Conversation
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
bypass sleep mode via URL
deps and go bump
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
redirect frontend GUI
feature/date_filter
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
experimental album videos
|
Warning Rate limit exceeded@damongolding has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 35 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis PR introduces multiple updates to the Immich Kiosk project. The Dockerfile is updated to use a newer Golang base image, while configuration files and README now include new sections for filtering, user-specific settings, and experimental features. Frontend changes standardise terminology from “image” to “asset” with extensive CSS renaming, updated TypeScript functionality (including localisation, overlay, and polling improvements), and support for video. Meanwhile, backend Go modules are refactored for improved API caching, asset management, album and memory processing, and a new video management system. Additionally, templates, routes, and utilities have been revised accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant VideoManager
participant FileSystem
Client->>Router: GET /video/:videoID
Router->>VideoManager: GetVideo(videoID)
VideoManager->>FileSystem: Check video file and last accessed time
FileSystem-->>VideoManager: Return video file info
VideoManager-->>Router: Return video data (stream)
Router-->>Client: Stream video content
Possibly related PRs
Suggested labels
Poem
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: 21
🔭 Outside diff range comments (1)
frontend/src/ts/kiosk.ts (1)
425-431: Refine input sanitisation to preserve<and>In the
sanitiseInputfunction,<and>characters are entirely removed. It might be more appropriate to encode them as<and>to prevent XSS attacks while preserving the original input data that may legitimately contain these characters.Apply this diff to encode
<and>instead of removing them:- .replace(/[<>]/g, "") + .replace(/[<]/g, "<") + .replace(/[>]/g, ">")
🧹 Nitpick comments (38)
frontend/src/ts/locales.ts (1)
46-88: Consider using object property shorthand for improved readability.The locale mapping is comprehensive and well-structured. However, since the property names match the variable names, you can use object property shorthand to make the code more concise.
Apply this diff to use object property shorthand:
export const locales: Record<string, Locale> = { - enUS: enUS, - enGB: enGB, - da: da, + enUS, + enGB, + da, // ... rest of the properties };frontend/src/ts/clock.ts (1)
41-78: Consider adding JSDoc comments for the language fallback logic.The
initialiseLangmethod implements a robust fallback mechanism for language selection. However, adding JSDoc comments would help explain the fallback strategy.Add JSDoc comments to explain the fallback logic:
+/** + * Initialises the locale for date/time formatting. + * Fallback strategy: + * 1. Use exact match if available + * 2. Try joining split language code (e.g., en_GB -> enGB) + * 3. Use base language code without region + * 4. Fall back to enGB if no match found + * @returns {Locale} The selected locale object + */ private initialiseLang(): Locale { // ... rest of the method }internal/routes/routes_previous_asset.go (1)
93-99: Remove unused variableprocessingErrThe variable
processingErris assigned but never used, which introduces unnecessary code. Remove it to clean up the function.Apply this diff to simplify the error handling:
- var processingErr error if err := asset.AssetInfo(requestID, deviceID); err != nil { - processingErr = fmt.Errorf("failed to get asset info: %w", err) - log.Error(processingErr) + log.Errorf("failed to get asset info: %v", err) }internal/immich/immich_date.go (3)
147-168: Add unit tests for new date parsing functionsConsider adding unit tests for
determineDateRange,processDateRange, andprocessLastDaysto ensure they handle all expected input formats correctly, including edge cases. This will improve code reliability and ease future modifications.
164-165: Enhance error messages for invalid date formatsImprove the error message by providing examples of valid date formats to assist users in correcting input errors.
Apply this diff to enhance the error message:
return dateStart, dateEnd, fmt.Errorf("invalid date filter format: %s. Expected formats: 'YYYY-MM-DD_to_YYYY-MM-DD' (e.g., '2025-01-01_to_2025-12-31') or 'last-X' (e.g., 'last-7')", dateRange)
213-219: Optimise regular expression compilation inextractDaysCurrently, the regular expression is compiled every time
extractDaysis called. For better performance, consider compiling the regex once and reusing it.Apply this diff to move the regex compilation outside the function:
+var numberRegex = regexp.MustCompile(`\d+`) func extractDays(s string) (int, error) { - re := regexp.MustCompile(`\d+`) - match := re.FindString(s) + match := numberRegex.FindString(s) if match == "" { return 0, fmt.Errorf("no number found") } return strconv.Atoi(match) }frontend/src/ts/polling.ts (1)
14-40: Simplify singleton pattern usageThe singleton pattern is manually implemented using a private static instance and a private constructor. In TypeScript modules, top-level variables are inherently singleton. Consider simplifying the code by directly exporting an instance of
PollingController, which can enhance readability.Apply this diff to simplify the singleton implementation:
class PollingController { - private static instance: PollingController; - private constructor() { - // Private constructor to enforce singleton pattern - } - public static getInstance(): PollingController { - if (!PollingController.instance) { - PollingController.instance = new PollingController(); - } - return PollingController.instance; - } + constructor() { + // Initialization code + } } -const pollingController = PollingController.getInstance(); +const pollingController = new PollingController();Then, update the export statements accordingly.
internal/routes/routes_asset_helpers.go (3)
48-49: Consider renaming method for consistencyThe method
PersonImageCountis called onimmichAsset. Since the system now handles both images and videos, consider renaming the method toPersonAssetCountfor clarity and consistency.
72-73: Method name may need updating for assetsSimilarly,
AlbumImageCountmight be better renamed toAlbumAssetCountto reflect that albums may contain various asset types.
151-152: Method name may be misleading with asset typesIn
return immichAsset.RandomImageFromFavourites(...), the method name suggests it only retrieves images. If it can retrieve videos as well, consider renaming it toRandomAssetFromFavouritesfor accuracy.internal/immich/immich_video.go (1)
20-24: Consider usingpath.Joinfor the entire path.The URL path construction could be more maintainable by using a single
path.Joincall.- apiUrl := url.URL{ - Scheme: u.Scheme, - Host: u.Host, - Path: path.Join("api", "assets", i.ID, "video", "playback"), - } + apiUrl := url.URL{ + Scheme: u.Scheme, + Host: u.Host, + Path: path.Join("/", "api", "assets", i.ID, "video", "playback"), + }internal/templates/components/video/video.templ (1)
11-14: Consider adding error handling for video playback.The script should handle potential video playback failures.
script videoHandler(id string) { kiosk.stopPolling(); - kiosk.videoHandler(id); + try { + kiosk.videoHandler(id); + } catch (error) { + console.error('Video playback failed:', error); + } }internal/templates/partials/history.templ (1)
31-42: Consider using strings.Builder for better performance.While the function correctly handles empty input and properly sanitises the output, consider using
strings.Builderfor more efficient string concatenation, especially when dealing with large datasets.func newHistoryEntry(images []common.ViewImageData) string { if len(images) == 0 { return "" } - newImages := make([]string, len(images)) + var builder strings.Builder + builder.Grow(len(images) * 40) // Pre-allocate space for better performance for i, entry := range images { assetHistory := fmt.Sprintf("%s:%s", entry.ImmichAsset.ID, entry.User) sanitisedID := template.HTMLEscapeString(assetHistory) - newImages[i] = sanitisedID + if i > 0 { + builder.WriteString(",") + } + builder.WriteString(sanitisedID) } - return strings.Join(newImages, ",") + return builder.String() }internal/immich/immich_faces.go (2)
10-55: Add nil check for input parameter.While the function is well-structured and efficient, consider adding a nil check for the input parameter to prevent potential panics.
func convertFaceResponse(faceResponse []AssetFaceResponse) []Person { + if faceResponse == nil { + return []Person{} + } personMap := make(map[string]*Person)
64-98: Enhance error handling and add context with timeout.Consider the following improvements:
- Add more descriptive error messages
- Use context with timeout for API calls to prevent hanging
-func (i *ImmichAsset) CheckForFaces(requestID, deviceID string) { +func (i *ImmichAsset) CheckForFaces(ctx context.Context, requestID, deviceID string) { var faceResponse []AssetFaceResponse u, err := url.Parse(requestConfig.ImmichUrl) if err != nil { - _, _, err = immichApiFail(faceResponse, err, nil, "") + _, _, err = immichApiFail(faceResponse, fmt.Errorf("failed to parse Immich URL: %w", err), nil, "") log.Error("parsing faces url", "err", err) return }internal/common/common.go (1)
96-97: Consider documenting the User field.The new User field lacks documentation explaining its purpose and usage.
Add documentation for the User field:
type ViewImageData struct { ImmichAsset immich.ImmichAsset // ImmichAsset contains immich asset data ImageData string // ImageData contains the image as base64 data ImageBlurData string // ImageBlurData contains the blurred image as base64 data ImageDate string // ImageDate contains the date of the image - User string + User string // User contains the username associated with the asset }Also applies to: 100-100
internal/routes/routes.go (1)
33-33: Document the VideoManager variable.The new VideoManager variable lacks documentation explaining its purpose and usage.
Add documentation for the VideoManager variable:
var ( KioskVersion string drawFacesOnImages string - VideoManager *video.VideoManager + // VideoManager handles video processing and streaming operations + VideoManager *video.VideoManager )internal/cache/cache.go (1)
110-134: Consider adding error handling for edge cases.While the assetToCache function handles basic type validation, consider adding:
- Nil checks for requestConfig
- Validation for position values
func assetToCache[T any](viewDataToAdd T, requestConfig *config.Config, deviceID, url string, position CachePosition) { + if requestConfig == nil { + log.Error("Invalid request config") + return + } + + if position != PREPEND && position != APPEND { + log.Error("Invalid cache position") + return + } + utils.TrimHistory(&requestConfig.History, 10)main.go (1)
178-178: Consider adding timeout for video cleanup.The video cleanup during shutdown could benefit from a timeout to ensure it doesn't delay the shutdown process.
- video.Delete() + done := make(chan bool) + go func() { + video.Delete() + done <- true + }() + select { + case <-done: + log.Info("Video cleanup completed") + case <-time.After(5 * time.Second): + log.Warn("Video cleanup timed out") + }frontend/src/ts/menu.ts (2)
31-39: Consider adding error handling for button initialization.The navigation button functions properly check for disabled navigation, but they could benefit from more descriptive error handling when buttons aren't initialized.
- console.debug("Navigation buttons not initialized."); + console.warn("Navigation buttons not initialized. Please ensure buttons are properly set up in initMenu().");Also applies to: 45-53
86-112: Consider adding keyboard event debouncing.The redirect key handler could benefit from debouncing to prevent rapid-fire keyboard events, especially for the arrow key navigation.
+let keyHandlerTimeout: number | null = null; +const DEBOUNCE_DELAY = 150; // milliseconds + function redirectKeyHandler(e: KeyboardEvent) { if (!redirects) return; + + if (keyHandlerTimeout) { + clearTimeout(keyHandlerTimeout); + } + + keyHandlerTimeout = setTimeout(() => { switch (e.code) { // ... existing cases ... } + }, DEBOUNCE_DELAY); }internal/immich/immich_memories.go (1)
162-167: Consider improving error message clarity.The error message when no viable assets are left could be more descriptive about the state of the memory lane.
- // no viable assets left in memoroy lane + // No viable assets left in memory lane - clearing cache entry memories[pickedMemoryIndex].Assets = make([]ImmichAsset, 1)internal/immich/immich_favourites.go (1)
112-214: Consider adding a validation check for empty allowedAssetType.The function implementation is robust, but it might be worth adding a validation check for an empty
allowedAssetTypeslice to prevent unintended behaviour.Apply this diff to add the validation:
func (i *ImmichAsset) RandomImageFromFavourites(requestID, deviceID string, allowedAssetType []ImmichAssetType, isPrefetch bool) error { + if len(allowedAssetType) == 0 { + return fmt.Errorf("allowedAssetType cannot be empty") + }internal/immich/immich_test.go (1)
226-271: Add test cases for edge scenarios.Consider adding the following test cases to improve coverage:
- Negative numbers (e.g., "last_-7")
- Zero (e.g., "last_0")
- Non-numeric characters (e.g., "last_abc")
Apply this diff to add the test cases:
{ name: "empty string", input: "", want: 0, wantErr: true, }, + { + name: "negative number", + input: "last_-7", + want: 0, + wantErr: true, + }, + { + name: "zero", + input: "last_0", + want: 0, + wantErr: true, + }, + { + name: "non-numeric", + input: "last_abc", + want: 0, + wantErr: true, + },internal/config/config_test.go (1)
36-36: Consider improving JSON readability.The JSON string could be more readable by using a constant or helper function.
- q.Add("immich_users_api_keys", "{\"user1\": \"9999\"}") + const testUsersApiKeys = `{"user1": "9999"}` + q.Add("immich_users_api_keys", testUsersApiKeys)internal/immich/immich.go (1)
241-245: Update function documentation to match new name.The function rename from NewImage to NewAsset is appropriate, but the comment still refers to "image".
-// NewImage returns a new image instance +// NewAsset returns a new asset instanceinternal/utils/utils.go (1)
731-736: Add input validation to prevent potential issues.While the function logic is correct, consider adding validation for negative maxLength and nil slice pointer to prevent potential runtime issues.
func TrimHistory(history *[]string, maxLength int) { + if history == nil || maxLength < 0 { + return + } if len(*history) > maxLength { *history = (*history)[len(*history)-maxLength:] } }frontend/src/css/video.css (1)
1-13: Consider adding vendor prefixes for broader browser support.The implementation is solid, but could benefit from vendor prefixes for maximum compatibility.
.frame--video { position: relative; display: flex; + display: -webkit-flex; justify-content: center; + -webkit-justify-content: center; width: 100%; height: 100%; video { max-width: 100%; max-height: 100%; object-fit: contain; + -webkit-object-fit: contain; } }frontend/src/css/redirects.css (1)
1-6: Initial container styling and universal selector
The#redirects-containeris correctly initialised withdisplay: none, and the universal selector (*) inside it forcesuser-select: texton all child elements. Please double-check that this broad application is intentional and does not cause unwanted side effects.README.md (4)
385-385: Image Time Format Description – Minor Rewording Suggested
The description forKIOSK_IMAGE_TIME_FORMATreads:
"Display image time in either 12 hour or 24 hour format. Can either be 12 or 24."
For better clarity, consider revising it to:
"Display image time in either a 12‑hour or 24‑hour format. It can be set to either 12 or 24."🧰 Tools
🪛 LanguageTool
[grammar] ~385-~385: When ‘24-hour’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...Display image time in either 12 hour or 24 hour format. Can either be 12 or 24. ...(HOUR_HYPHEN)
[style] ~385-~385: To form a complete sentence, be sure to include a subject or ‘there’.
Context: ...me in either 12 hour or 24 hour format. Can either be 12 or 24. | | show...(MISSING_IT_THERE)
558-558: Fenced Code Blocks Lacking a Language Identifier
Some fenced code blocks (e.g. those in the Docker Compose examples) do not specify a language. For improved readability and to adhere to markdownlint guidelines, please consider adding the appropriate language identifier (for example,yamlfor configuration blocks orshfor shell commands).🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
558-558: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
345-397: General Text and Grammar Improvements Recommended
Across several configuration table rows, minor grammatical refinements are suggested based on static analysis feedback. For instance, using hyphenated forms (e.g. "12‑hour" and "24‑hour") and ensuring complete sentences (adding a subject where necessary) will further polish the documentation.🧰 Tools
🪛 LanguageTool
[grammar] ~347-~347: When ‘24-hour’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...Display clock time in either 12 hour or 24 hour format. Can either be 12 or 24. ...(HOUR_HYPHEN)
[style] ~347-~347: To form a complete sentence, be sure to include a subject or ‘there’.
Context: ...me in either 12 hour or 24 hour format. Can either be 12 or 24. | | show...(MISSING_IT_THERE)
[uncategorized] ~350-~350: The preposition ‘of’ seems more likely in this position.
Context: ... | 60 | The amount in seconds a image will be displayed for. ...(AI_HYDRA_LEO_REPLACE_IN_OF)
[misspelling] ~350-~350: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ... | 60 | The amount in seconds a image will be displayed for. ...(EN_A_VS_AN)
[grammar] ~378-~378: Consider using either the past participle “contained” or the present participle “containing” here.
Context: ...mage will fit on the screen. Default is contain. See Image fit for more i...(BEEN_PART_AGREEMENT)
[grammar] ~381-~381: The word “fallback” is a noun. The verb is spelled with a space.
Context: ...not a png, gif, jpeg or webp Kiosk will fallback to using the preview. | | show_album_na...(NOUN_VERB_CONFUSION)
[grammar] ~385-~385: When ‘24-hour’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...Display image time in either 12 hour or 24 hour format. Can either be 12 or 24. ...(HOUR_HYPHEN)
[style] ~385-~385: To form a complete sentence, be sure to include a subject or ‘there’.
Context: ...me in either 12 hour or 24 hour format. Can either be 12 or 24. | | show...(MISSING_IT_THERE)
🪛 markdownlint-cli2 (0.17.2)
345-345: Bare URL used
null(MD034, no-bare-urls)
1300-1316: Fenced Code Blocks in the Contributing Section
Within the "Contributing" guidelines, the fenced code blocks do not have language identifiers. Adding these (e.g. specifyingshfor shell commands) will improve clarity and meet markdown best practices.internal/immich/immich_album.go (2)
24-24: Fix typo in variable name.The variable name
albumsContaingAssetcontains a typo. It should bealbumsContainingAsset.- albumsContaingAsset := []string{} + albumsContainingAsset := []string{}
233-237: Add documentation for experimental video support.The experimental video support is well-implemented, but it would benefit from additional documentation explaining the feature's limitations and potential impacts.
Add a comment block explaining:
// ExperimentalAlbumVideo enables video asset support in albums. // Note: This is an experimental feature and may have performance implications // or unexpected behaviour. Currently limited to album contexts only.internal/immich/immich_helpers.go (1)
108-118: Consider logging API key usage without exposing sensitive data.While the API key handling is secure, consider adding debug logging to track which user's API key is being used, without exposing the actual key.
if key, ok := requestConfig.ImmichUsersApiKeys[requestConfig.SelectedUser]; ok { + if requestConfig.Kiosk.DebugVerbose { + log.Debug("Using API key for user", "user", requestConfig.SelectedUser) + } apiKey = key } else { return responseBody, fmt.Errorf("no API key found for user %s in the config", requestConfig.SelectedUser) }internal/utils/utils_test.go (1)
61-71: Enhance test coverage with additional test cases.While the test effectively verifies non-destructive behaviour, consider adding test cases for:
- Empty slices
- Slices with different types (strings, structs)
- Large slices
func TestRandomMutate(t *testing.T) { tests := []struct { name string input interface{} expected interface{} }{ { name: "integers", input: []int{1, 2, 3, 4}, expected: []int{1, 2, 3, 4}, }, { name: "empty slice", input: []string{}, expected: []string{}, }, { name: "strings", input: []string{"a", "b", "c"}, expected: []string{"a", "b", "c"}, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { RandomItem(tt.input) assert.Equal(t, tt.expected, tt.input, "RandomItem should not mutate the original slice") }) } }internal/config/config.go (1)
226-229: Consider additional configuration options for video support.The experimental video support could benefit from additional configuration options such as:
- Maximum video duration
- Video quality settings
- Video format restrictions
These options would help manage resource usage and user experience when video support is enabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlgo.sumis excluded by!**/*.sum
📒 Files selected for processing (66)
Dockerfile(1 hunks)README.md(11 hunks)config.example.yaml(2 hunks)docker-compose.yaml(1 hunks)frontend/package.json(2 hunks)frontend/public/assets/css/kiosk.css(14 hunks)frontend/src/css/clock-weather-container.css(1 hunks)frontend/src/css/globals.css(1 hunks)frontend/src/css/image.css(5 hunks)frontend/src/css/kiosk.css(2 hunks)frontend/src/css/menu.css(3 hunks)frontend/src/css/more-info.css(1 hunks)frontend/src/css/progress.css(1 hunks)frontend/src/css/redirects.css(1 hunks)frontend/src/css/utis.css(1 hunks)frontend/src/css/vars.css(1 hunks)frontend/src/css/video.css(1 hunks)frontend/src/ts/clock.ts(6 hunks)frontend/src/ts/frames.ts(0 hunks)frontend/src/ts/kiosk.ts(12 hunks)frontend/src/ts/locales.ts(1 hunks)frontend/src/ts/menu.ts(1 hunks)frontend/src/ts/polling.ts(1 hunks)go.mod(2 hunks)internal/cache/cache.go(3 hunks)internal/common/common.go(1 hunks)internal/config/config.go(3 hunks)internal/config/config_test.go(2 hunks)internal/immich/immich.go(3 hunks)internal/immich/immich_album.go(6 hunks)internal/immich/immich_date.go(4 hunks)internal/immich/immich_faces.go(2 hunks)internal/immich/immich_favourites.go(4 hunks)internal/immich/immich_helpers.go(3 hunks)internal/immich/immich_memories.go(3 hunks)internal/immich/immich_person.go(3 hunks)internal/immich/immich_random.go(1 hunks)internal/immich/immich_test.go(1 hunks)internal/immich/immich_video.go(1 hunks)internal/routes/routes.go(2 hunks)internal/routes/routes_asset.go(4 hunks)internal/routes/routes_asset_helpers.go(21 hunks)internal/routes/routes_previous_asset.go(5 hunks)internal/routes/routes_test.go(2 hunks)internal/routes/routes_video.go(1 hunks)internal/routes/routes_weather.go(1 hunks)internal/routes/routes_webhooks.go(2 hunks)internal/templates/components/image/history.templ(0 hunks)internal/templates/components/image/image.templ(2 hunks)internal/templates/components/image/layout.templ(4 hunks)internal/templates/components/image/metadata.templ(0 hunks)internal/templates/components/video/video.templ(1 hunks)internal/templates/partials/history.templ(1 hunks)internal/templates/partials/menu.templ(3 hunks)internal/templates/partials/metadata.templ(1 hunks)internal/templates/partials/more_info.templ(7 hunks)internal/templates/partials/redirects.templ(1 hunks)internal/templates/partials/sleep.templ(1 hunks)internal/templates/partials/weather.templ(1 hunks)internal/templates/views/views_home.templ(3 hunks)internal/utils/utils.go(2 hunks)internal/utils/utils_test.go(1 hunks)internal/video/video.go(1 hunks)internal/webhooks/webhooks.go(1 hunks)main.go(5 hunks)taskfile.yml(1 hunks)
💤 Files with no reviewable changes (3)
- frontend/src/ts/frames.ts
- internal/templates/components/image/history.templ
- internal/templates/components/image/metadata.templ
✅ Files skipped from review due to trivial changes (4)
- frontend/src/css/utis.css
- taskfile.yml
- frontend/package.json
- frontend/src/css/kiosk.css
🧰 Additional context used
📓 Path-based instructions (1)
`frontend/public/**`: exclude
frontend/public/**: exclude
frontend/public/assets/css/kiosk.css
📓 Learnings (1)
frontend/src/ts/clock.ts (1)
Learnt from: damongolding
PR: damongolding/immich-kiosk#192
File: frontend/src/ts/clock.ts:1-1
Timestamp: 2024-11-20T10:52:53.804Z
Learning: In the `frontend/src/ts` files of the `immich-kiosk` project, importing `{ format }` from `"date-fns"` causes a TypeScript error. The correct import statement is `import { format } from "date-fns/format";`.
🪛 Biome (1.9.4)
frontend/src/css/globals.css
[error] 7-7: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
frontend/public/assets/css/kiosk.css
[error] 144-144: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
🪛 LanguageTool
README.md
[grammar] ~347-~347: When ‘24-hour’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...Display clock time in either 12 hour or 24 hour format. Can either be 12 or 24. ...
(HOUR_HYPHEN)
[style] ~347-~347: To form a complete sentence, be sure to include a subject or ‘there’.
Context: ...me in either 12 hour or 24 hour format. Can either be 12 or 24. | | show...
(MISSING_IT_THERE)
[uncategorized] ~350-~350: The preposition ‘of’ seems more likely in this position.
Context: ... | 60 | The amount in seconds a image will be displayed for. ...
(AI_HYDRA_LEO_REPLACE_IN_OF)
[misspelling] ~350-~350: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ... | 60 | The amount in seconds a image will be displayed for. ...
(EN_A_VS_AN)
[grammar] ~378-~378: Consider using either the past participle “contained” or the present participle “containing” here.
Context: ...mage will fit on the screen. Default is contain. See Image fit for more i...
(BEEN_PART_AGREEMENT)
[grammar] ~381-~381: The word “fallback” is a noun. The verb is spelled with a space.
Context: ...not a png, gif, jpeg or webp Kiosk will fallback to using the preview. | | show_album_na...
(NOUN_VERB_CONFUSION)
[grammar] ~385-~385: When ‘24-hour’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...Display image time in either 12 hour or 24 hour format. Can either be 12 or 24. ...
(HOUR_HYPHEN)
[style] ~385-~385: To form a complete sentence, be sure to include a subject or ‘there’.
Context: ...me in either 12 hour or 24 hour format. Can either be 12 or 24. | | show...
(MISSING_IT_THERE)
[uncategorized] ~569-~569: Possible missing article found.
Context: ... Video Selection Process: - When Kiosk selects a video from an album, it first...
(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~576-~576: Possible missing article found.
Context: ...- Videos are automatically removed when Kiosk shuts down. 3. Playback Handling: ...
(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~687-~687: Possible missing article found.
Context: ... When you specify multiple date ranges, Kiosk maintains separate buckets for each ran...
(AI_HYDRA_LEO_MISSING_THE)
🪛 markdownlint-cli2 (0.17.2)
README.md
345-345: Bare URL used
null
(MD034, no-bare-urls)
463-463: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
558-558: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🔇 Additional comments (142)
frontend/src/ts/locales.ts (1)
1-44: Well-organised imports section with comprehensive locale support.The imports are properly organised and provide extensive locale support from
date-fns/locale. The type import is correctly separated from the locale imports.frontend/src/ts/clock.ts (5)
1-3: LGTM! Imports follow the correct pattern.The imports follow the correct pattern as per the retrieved learning, avoiding TypeScript errors with
date-fns.
13-19: LGTM! Interface updated with language support.The
ClockConfiginterface has been properly updated to include language configuration.
27-39: LGTM! Constructor properly initialises language support.The
Clockclass constructor correctly initialises the language-related properties.
99-109: LGTM! Proper locale integration in formatting methods.The
updateDateandupdateTimemethods correctly integrate the locale configuration with appropriate error handling.Also applies to: 111-131
158-182: LGTM! Function signature properly updated.The
initClockfunction signature has been correctly updated to include language configuration.internal/immich/immich_date.go (2)
35-37: Refactored date range parsing improves code clarityRefactoring the date range parsing into the
determineDateRangefunction enhances code readability and maintainability. This change makes theRandomImageInDateRangefunction cleaner and more focused on its primary responsibility.
99-99: Cache key enhancement improves specificityIncluding
requestConfig.SelectedUserin the cache key enhances cache specificity and prevents potential collisions when multiple users are involved.frontend/src/ts/polling.ts (1)
65-68: Confirm accuracy of polling time adjustments after pauseIn the
updateProgressmethod, when resuming from a pause,lastPollTimeis adjusted by the elapsed paused time. Verify that this logic correctly accounts for the paused duration to ensure smooth progress updates.internal/templates/partials/metadata.templ (1)
191-309:⚠️ Potential issueEnsure user-provided content is properly escaped
When rendering user-provided content such as asset descriptions, user names, album names, etc., ensure that the content is properly escaped to prevent Cross-Site Scripting (XSS) vulnerabilities. Verify that the templating engine or rendering functions handle HTML escaping appropriately.
frontend/src/ts/kiosk.ts (15)
10-13: Imports updated appropriatelyThe addition of
stopPollingandvideoHandlerto the imports from./pollingis correct and necessary for the new functionalities.
18-22: Menu functions correctly importedThe imported menu functions
disableAssetNavigationButtons,enableAssetNavigationButtons,toggleAssetOverlay, andtoggleRedirectsOverlayare appropriately added for enhanced menu interactions.
94-98: Ensure consistent class name updatesThe class names have been updated from
.navigation--next-imageand.navigation--prev-imageto.navigation--next-assetand.navigation--prev-asset. Please verify that all occurrences of these class names throughout the codebase, including CSS and HTML templates, have been updated to maintain consistency and prevent potential bugs.
102-102: Initialisation oflinksButtonis correctThe
linksButtonis properly initialised to handle the redirects overlay.
126-132: Locale support added to clock initialisationThe inclusion of
kioskData.langCodein theinitClockfunction enhances localisation support for date and time formatting.
156-156: Polling initialisation parameters verifiedThe call to
initPollingwithpollInterval,kiosk, andmenuis correct and aligns with the updated function signature.
191-204: Overlay toggle logic verifiedThe
handleOverlayTogglefunction effectively manages the overlay states and controls polling based on whether an overlay is opened or closed.
211-213: Info overlay keypress handler implementedThe
handleInfoKeyPressfunction correctly utiliseshandleOverlayToggleto manage the info overlay with keyboard interaction.
220-222: Redirects overlay keypress handler addedThe
handleRedirectsKeyPressfunction appropriately toggles the redirects overlay when the 'r' key is pressed.
270-291: Keyboard event listeners correctly set upThe event listeners for keyboard shortcuts, including 'Space', 'KeyI', and 'KeyR', are implemented correctly with necessary checks for modifier keys and focus contexts.
317-320: Review script tag removal incleanupFramesThe
cleanupFramesfunction removes script tags from the kiosk element after a delay to prevent memory leaks. Ensure that this does not inadvertently remove necessary scripts required for functionality, potentially causing issues in dynamic content loading.
347-356: Asset navigation buttons correctly disabled during requestsCalling
disableAssetNavigationButtonsinsetRequestLockappropriately prevents user interaction with navigation buttons during ongoing requests, enhancing UI responsiveness and preventing concurrent modifications.
367-371: Asset navigation buttons re-enabled after requestsThe
enableAssetNavigationButtonsfunction is correctly called inreleaseRequestLockto restore user interaction after requests complete.
406-411: Client data retrieval function accurateThe
clientDatafunction accurately captures the browser's viewport dimensions, which can be utilised for responsive behaviours.
468-477: Exports updated to include new functionsThe addition of
stopPollingandvideoHandlerto the export list ensures these functions are available for other modules, aligning with the new functionalities.internal/routes/routes_asset_helpers.go (7)
39-39: Function parameter updated appropriatelyThe parameter
immichImagehas been renamed toimmichAssetingatherAssetBuckets, reflecting the generalisation from images to assets.
133-133: Allowed asset types parameter correctly addedThe inclusion of
allowedAssetType []immich.ImmichAssetTypeinretrieveImageallows for more granular control over the types of assets retrieved.
179-201: Ensure EXIF orientation applied appropriatelyIn
fetchImagePreview, after retrieving the preview, EXIF orientation is applied usingutils.ApplyExifOrientation. Confirm that this function correctly handles both images and video previews, and that EXIF data is applicable to the asset type.
223-225: Video processing logic correctly implementedThe
processAssetfunction now appropriately checks if the asset is a video and callsprocessVideowhen necessary, enabling proper handling of video assets.
273-291: Conditional logic inprocessBlurredImageThe function now checks
assetTypeand adjusts whether to apply blur effects based on whether the asset is an image or video. The logic appears sound for optimising performance and visual effects.
382-384: Face detection considerations for video assetsIn
processViewImageData, when applying the 'smart-zoom' effect, there is a check for faces in the asset. Verify that face detection is applicable and effective for video assets, or adjust the logic to handle videos appropriately.
471-570: Asset handling ingenerateViewDataexpanded correctlyThe
generateViewDatafunction now includes logic to handle different layouts and asset types, including videos. This expansion seems to be implemented thoughtfully.internal/templates/partials/redirects.templ (1)
8-25: NewRedirectstemplate function implementedThe
Redirectstemplate function correctly generates the HTML for the redirects overlay, handling the inclusion of thepasswordquery parameter securely usingtempl.SafeURL. Iterating over the redirects and constructing the links is appropriately handled.internal/templates/components/image/image.templ (2)
3-7: LGTM! Well-organised imports.The imports are properly structured and include all necessary packages for the template's functionality.
16-16: LGTM! Consistent terminology change.The transition from
ImagestoAssetsand the use of thepartialspackage improves code organisation and maintains consistent terminology across the codebase.Also applies to: 21-21, 23-23
internal/immich/immich_video.go (1)
8-11: LGTM! Well-documented function.The documentation clearly explains the function's purpose, return values, and response format.
internal/templates/components/video/video.templ (1)
32-35: Verify video source URL construction.Ensure that the video source URL is properly escaped to prevent XSS attacks.
internal/templates/partials/sleep.templ (1)
23-24: LGTM! Well-structured sleep mode control.The addition of the
disableSleepparameter provides better control over sleep mode functionality whilst maintaining backwards compatibility.internal/templates/partials/history.templ (1)
18-29: LGTM! Well-structured template function.The function is well-documented and correctly handles the rendering of history entries.
internal/routes/routes_weather.go (1)
30-30: Verify HTTP method change in route handlers.The change from
QueryParamtoFormValuesuggests a shift from GET to POST method. Please ensure that all route handlers and client-side code have been updated accordingly.internal/routes/routes_test.go (1)
82-82: LGTM! Good refactoring to use utility function.The change to use
utils.TrimHistoryimproves code organisation by centralising utility functions.internal/routes/routes_webhooks.go (1)
87-87: LGTM! Consistent renaming from "Images" to "Assets".The changes align with the broader effort to standardise terminology across the codebase.
Also applies to: 96-96, 104-106
internal/immich/immich_random.go (1)
88-88: LGTM! Enhanced caching with user-specific keys.The addition of
requestConfig.SelectedUserto the cache key ensures proper isolation of cached assets between users.internal/webhooks/webhooks.go (1)
83-87: LGTM! Consistent with asset terminology refactoring.The changes correctly implement the transition from images to assets in the webhook payload construction.
internal/templates/views/views_home.templ (2)
23-25: LGTM! Endpoint and trigger changes align with asset terminology.The HTMX configuration updates correctly reflect the shift to asset-based endpoints.
79-101: Well-structured triggers function with clear documentation.The function effectively handles conditional navigation triggers based on the disable flag.
internal/cache/cache.go (2)
14-19: LGTM! Well-defined cache position type and constants.The CachePosition type and its constants provide clear semantics for cache operations.
59-62: LGTM! Enhanced cache key generation with user context.The ApiCacheKey function now correctly incorporates user-specific context in the cache key.
main.go (2)
70-78: Consider graceful degradation for video manager failures.While the error is logged, the application continues even if video manager initialization fails. Consider whether this could lead to runtime issues.
Would it be better to fail fast if video management is essential to the application's functionality?
143-159: LGTM! Route updates align with asset terminology.The endpoint changes consistently reflect the shift from image to asset terminology.
frontend/src/ts/menu.ts (2)
9-25: LGTM! Well-structured variable declarations.The variables are logically grouped and properly typed, with clear separation between navigation controls, overlay states, and redirect-related variables.
60-84: LGTM! Robust overlay management.The asset overlay functions are well-implemented with proper checks for document body and polling state. The toggle function uses a clean ternary operation.
internal/immich/immich_memories.go (1)
77-107: LGTM! Well-implemented cache update function.The
updateMemoryCachefunction properly handles deep copying of memory data and cache updates. The error handling is appropriate for the operations involved.internal/templates/components/image/layout.templ (2)
35-36: LGTM! Consistent asset terminology.The change from
ImagestoAssetsmaintains consistency with the broader codebase changes.
93-93: Verify metadata component compatibility.Ensure that the
partials.AssetMetadatacomponent is compatible with all asset types, not just images.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for AssetMetadata implementation and usage rg "AssetMetadata" -A 5Length of output: 2246
AssetMetadata component is compatible with both image and video assets.
- The AssetMetadata partial (defined in
internal/templates/partials/metadata.templ) accepts aviewDataobject and an asset index, making it flexible enough for use across asset types.- Both the image layout (
internal/templates/components/image/layout.templ) and video template (internal/templates/components/video/video.templ) invoke this component, demonstrating its broad compatibility.internal/templates/partials/more_info.templ (4)
23-60: LGTM!The function has been correctly renamed to follow Go's exported function naming convention, and the migration from
ImagestoAssetshas been implemented consistently.
65-86: LGTM!The function correctly handles all edge cases and has been updated to use the new
AssetExiffunction.
91-102: LGTM!The function has been correctly renamed to reflect the shift from image-specific to asset-generic handling, and the implementation remains robust.
109-138: LGTM!The function has been updated to use the new asset-related functions whilst maintaining robust handling of missing information and location links.
internal/immich/immich_favourites.go (1)
16-84: LGTM!The function implementation is robust, with proper error handling and pagination support.
internal/config/config_test.go (1)
23-28: LGTM! Test coverage for ImmichUsersApiKeys immutability.The test case properly verifies that the ImmichUsersApiKeys configuration field remains immutable, following the same pattern as other immutable fields.
Also applies to: 36-36, 49-49
internal/templates/partials/weather.templ (1)
92-92: LGTM! Appropriate HTTP method change.Changing from GET to POST is more suitable for operations that may modify server state.
internal/templates/partials/menu.templ (3)
3-8: LGTM! Well-structured imports and parameter additions.The new parameters and imports are properly organised and typed.
11-12: LGTM! Consistent terminology update.The change from "image" to "asset" is applied consistently throughout the template.
Also applies to: 17-19
65-77: LGTM! Accessible redirects implementation.The new redirects section follows accessibility best practices with proper ARIA attributes and SVG roles.
internal/immich/immich.go (3)
74-76: LGTM! Well-organised asset type constants.The new asset type slices provide clear categorisation and follow consistent naming conventions.
165-169: LGTM! Enhanced asset metadata fields.The new fields for memory title and appearance tracking improve the asset metadata capabilities with appropriate types.
250-261: LGTM! Comprehensive API response type support.The ImmichApiResponse interface has been properly updated to support all necessary response types.
internal/utils/utils_test.go (1)
61-71: LGTM!Good addition of a test case that verifies the non-destructive behaviour of
RandomItem. The test is well-documented and ensures the function maintains input integrity.internal/config/config.go (2)
162-169: LGTM! Well-structured user management configuration.The new user management fields are well-documented and follow the existing configuration patterns. The default values are sensible, and the field tags are consistent.
223-229: LGTM! Clear experimental feature flags.Good practice to clearly mark the video support as experimental and document its current limitations to albums only.
internal/utils/utils.go (1)
96-97: LGTM! Correct JavaScript date format mappings.The updated mappings (
DDDD→eeeeandDDD→eee) align with the modern JavaScript Intl.DateTimeFormat patterns for weekday formatting.docker-compose.yaml (1)
12-12: Verify config.yaml existence before deployment.The change from directory to single file mapping is good, but ensure the config.yaml file exists to prevent container startup issues.
frontend/src/css/progress.css (1)
18-21: LGTM! Performance optimisations for progress bar animations.The changes improve animation smoothness and rendering performance through hardware acceleration and optimised transition timing.
Dockerfile (1)
2-2: Base Image Update ConfirmationThe base image has been updated to
golang:1.23.6-alpinewith the--platformflag and build stage alias applied. This change is consistent with the adjustments in the Go version ingo.modand should deliver any relevant bug fixes or improvements from the updated image.frontend/src/css/clock-weather-container.css (1)
1-2: Nested Selector Syntax VerificationA new compound selector for
.polling-paused.more-infoand.polling-paused.redirects-openhas been added to apply a reduced opacity to the#clock-weather-container. Please ensure that the nested syntax (i.e. nesting#clock-weather-containerwithin the selectors) is processed correctly by your CSS toolchain (e.g. PostCSS, Sass, etc.) because standard CSS does not support nesting.If you need help configuring a preprocessor or refactoring this into valid plain CSS, please let me know.
frontend/src/css/vars.css (1)
9-9: New Z-index Variable AdditionThe new CSS variable
--z-below: -1;has been successfully added to the z-index hierarchy. This will be useful for layering elements behind the base layer. The inclusion is clear and follows the existing naming conventions.go.mod (1)
3-25: Go Version and Dependency UpdatesThe
go.modfile now reflects a bump in the Go version from1.23.4to1.23.5and updates multiple dependencies (includinggithub.com/a-h/templ,golang.org/x/image,golang.org/x/sync,golang.org/x/text, andgolang.org/x/time). These changes appear consistent with the overall project upgrade strategy. Please ensure that all tests are run to validate compatibility with these updated versions.frontend/src/css/redirects.css (7)
8-14: Filter effects for paused state on various media elements
The nested rule under.polling-paused.redirects-openapplies a uniform filter to images, background images, and videos. This improves consistency; however, note that native CSS does not support nesting. Ensure you are using a compatible preprocessor (or PostCSS with nesting enabled) so that these selectors compile as intended.
15-34: Responsive container styling when redirects are open
The nested#redirects-containerwithin the paused state is well defined with absolute positioning, calculated dimensions, flexbox centring, and a semi-transparent background. This should provide a centred overlay as intended.
36-45: Styling for internal.redirectsblock
The rules for.redirectsinside#redirects-containerset maximum widths, overflow behaviour, and a flex column layout. This is a clear and organised approach to structuring the content.
46-73: Shadow effects using pseudo-elements
The nested.redirects--shadowblock utilises&::beforeand&::afterto create a radial-gradient shadow effect. This is a neat effect; please ensure that the gradient parameters render correctly across the browsers you support.
76-83: Anchor link styling within redirects
The styling for the<a>elements (display, padding, border, and colour) is clear and provides a good interactive baseline.
84-90: Hover and focus state for links
The hover/focus rules that change the background colour tovar(--mint-green), the text colour tovar(--cool-grey), and increase the font weight are well defined for user interaction.
92-100: Frameless mode adjustments
The.frameless.redirects-openrule ensures that#redirects-containeroccupies the full available space with no margins or border radii. Confirm that this aligns with the overall responsive design expectations.frontend/src/css/more-info.css (7)
1-6: Hidden state and universal user-select for #more-info
The#more-infoelement is initially hidden withdisplay: noneand forcesuser-select: textfor its children. This approach is straightforward and appropriate for the intended use.
8-14: Filter effects for more-info in paused state
Within.polling-paused.more-info, the nested selectors for.frame--image img,.frame--background img, and.frame--video videoapply a grayscale, blur, and brightness filter. As with the previous file, please ensure that nesting is supported by your build toolchain.
15-74: Layout for #more-info in paused state
The nested#more-infoblock applies absolute positioning, flexbox layout with a gap and specific padding values. Nested rules for.more-info--imageand its child elements detail dimensions, alignment, and styling. The structure is clear; however, consider verifying that the fixed padding values (e.g.6rem 3rem 3rem 3rem) work well across different screen sizes.
76-84: Adaptations for split-view landscape layout
The override for.polling-paused.more-info.layout-splitview-landscapealters the flex-direction to column and adjusts the.more-info--imagedirection accordingly. This is a sensible adjustment for landscape views.
86-91: Button group styling for additional info section
The.more-info--button-grouprule utilises flexbox with a gap and wrap behaviour, providing a flexible layout for buttons.
93-110: Interactive button styling for more-info links
The combined selectors for.more-info--webhookand.more-info--image-linkset clear typographic and background properties, with a smooth transition on hover. This enhances both visual appeal and interactivity.
112-120: Frameless mode for more-info layout
The.frameless.more-inforule reconfigures#more-infoby reducing padding and removing border radii for child images. This is effective for a frameless presentation; please ensure it integrates seamlessly with existing UI patterns.config.example.yaml (3)
39-41: New Filters section addition
A new section labelled "## Filters" has been added with a commented-out option fordate_filter: last-30-days. This addition provides flexibility for filtering assets. Make sure the documentation is updated to explain when and how to enable this option.
42-44: Experimental feature toggle
The addition of an "## Experimental" section withexperimental_album_video: falseis a forward-thinking change. Ensure that corresponding documentation and warnings (if necessary) accompany the use of experimental features.
97-101: User-specific API key configuration
The new section "## Immich API keys for specific users" introduces commented-out configuration forimmich_users_api_keysandshow_user. This is useful for future customisation. Please update the README or relevant docs with usage examples when this feature is activated.frontend/src/css/menu.css (4)
16-20: Renaming navigation selectors for asset consistency
Selectors previously identified as--previous-imageand--next-imagehave been updated to--previous-assetand--next-asset. This change aligns with the asset-centric terminology now used across the project.
115-134: Polling state adjustments for menu controls
The nested rules for.polling-pausednow include overrides for both.more-infoand.redirects-openstates, updating background colours and SVG fills. These visual cues are valuable, but please verify that the chosen colours (usingvar(--mint-green)andvar(--cool-grey)) maintain sufficient contrast and consistency with the overall design palette.
161-167: Rounded corners for asset navigation controls
The new rules in.navigation--next-assetand.navigation--prev-assetprovide rounded corners via adjustedborder-radiussettings. This subtly improves the visual polish and consistency of navigation elements.
169-175: Disabled navigation asset styling
The styles for.navigation--next-asset.disabledand.navigation--prev-asset.disabledreduce opacity and disable pointer events, offering a clear visual indicator when a navigation control is inactive.frontend/src/css/image.css (20)
2-7: Opacity adjustment for asset metadata in paused state
The rule under.polling-paused.more-info, .polling-paused.redirects-openlowers the opacity of.asset--metadatato0.2when polling is paused. This provides a visual cue that the overlay is in a paused state; ensure that the reduced opacity still allows critical information to remain legible.
9-23: Global styling for asset metadata
The base.asset--metadatarules set a solid foundation with absolute positioning, flex layout, and text styling. The design appears effective for overlay metadata.
26-38: Fade theme pseudo-element for metadata
The pseudo-element defined in.asset--metadata--theme-fade::beforecreates a fade effect using a radial gradient. The use of percentages and positioning should be verified in multiple screen resolutions to ensure consistency.
40-53: Alternate fade styling for splitview layout
The adjusted styling for.frame--layout-splitview:nth-child(1) .asset--metadata--theme-fade::beforerepositions the gradient for left-aligned content. This level of detail helps maintain visual consistency across layouts.
56-60: Solid theme styling for asset metadata
The.asset--metadata--theme-solidblock applies an opaque background with padding and a border radius, which is ideal for ensuring legibility of metadata.
62-66: Splitview override for solid theme
The override for the solid theme when used in a splitview (nth-child(1)) appropriately adjusts padding and border-radius for a left-aligned layout.
68-70: Responsive break handling within metadata
Setting.asset--metadata .responsive-breaktodisplay: noneis acceptable if its primary usage is for controlling layout breaks in larger views; ensure this does not impact readability on smaller screens.
72-76: Hiding empty metadata elements
The rules that hide.asset--metadata:emptyand empty<div>elements help prevent unwanted gaps in the layout. This is a good practice for dynamic content.
78-80: Z-index adjustment for metadata containers
Applying a higherz-indexto nested divs within.asset--metadatahelps ensure that interactive content displays correctly above background elements.
82-86: Icon container styling within metadata
The.asset--metadata--has-iconrule utilises flex layout for aligning icons alongside text, which is a clean and modern approach.
88-90: Ordering for first metadata icon
The rule for.asset--metadata--is-first .asset--metadata--iconrearranges the order for display purposes. This appears to be a subtle yet useful detail.
92-102: Styling for metadata icons
The.asset--metadata--iconblock, including specific dimensions for embedded SVGs, is well crafted to ensure the icons stay consistent in size and appearance.
103-108: EXIF f-number display styling
The styling for.asset--metadata--exif--fnumberis clear, with adjustments for font size and translation to improve vertical alignment.
110-113: Separator styling for EXIF data
The.asset--metadata--exif--seperatorrule provides a subtle visual separator.
122-136: Alternate metadata styling for left-aligned splitview
The block redefines.asset--metadatafor.frame--layout-splitview:nth-child(1), switching alignment from right to left. This ensures that metadata aligns appropriately with the layout changes in splitview mode.
138-149: Image zoom effect configuration
The image zoom effect rules are straightforward, utilising inherited animation properties. Ensure that the specified transition timing functions and fill modes work consistently across different browsers.
151-157: Animation naming for zoom in/out effects
Animation names for zooming in and out are explicitly assigned and should tie in correctly with keyframe definitions elsewhere in the codebase.
158-165: Smart zoom animation settings
The definitions for smart zoom animations follow a similar pattern to the standard zoom effects, ensuring consistency throughout media transitions.
167-173: Pause animations when polling is paused
The rules under.polling-paused .framepause animations effectively. This consideration should improve the user experience when the kiosk is in a paused state.
175-193: Responsive adjustments within a media query
Within the@mediaquery for screens with a max-width of 31.25rem, the metadata padding and maximum widths are adjusted. The modifications to.asset--metadata--desciption(again, note the typo) and.asset--metadata--locationensure that content scales appropriately on small devices.README.md (20)
58-58: New Table of Contents Entry for Filters
The "Filters" entry has been added to the Table of Contents, which accurately reflects the new filtering capability introduced. Please double‐check that the anchor (#filters) correctly links to the Filters section.
65-65: Updated Table of Contents Entry for Custom CSS
The "Custom CSS" entry now appears with the appropriate marker. This update helps maintain consistency with the extended documentation on custom styling.
345-345: Enhanced External URL Configuration
The row forimmich_external_urlnow provides a more detailed explanation of its use—for example, when accessing the server via a reverse proxy. This additional context is very useful for the user.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
345-345: Bare URL used
null(MD034, no-bare-urls)
351-351: Clarified Disable Screensaver Option
The description forKIOSK_DISABLE_SCREENSAVERhas been updated to clarify its purpose and limitations (notably on iOS). This precise wording sets the right expectations.
358-358: Experimental Album Video Feature
The newKIOSK_EXPERIMENTAL_ALBUM_VIDEOoption is now documented with clear warnings about its experimental nature and limitations. This is a valuable addition for users testing video playback features.
362-362: Addition of Date Filter Option
Introducing theKIOSK_DATE_FILTERconfiguration provides enhanced filtering for person and random asset selections. The description is clear and aligns well with the new Filters section.
372-372: Disable Sleep Mode Override
The update to include the[disable_sleep]option offers a straightforward way to bypass sleep mode by URL parameter. The change is concise and clear.
379-379: Image Effect Option Update
TheKIOSK_IMAGE_EFFECTsetting now explicitly lists “zoom” and “smart-zoom” as options. This update enhances clarity for users when selecting image effects.
380-380: Image Effect Intensity Specification
The addition ofKIOSK_IMAGE_EFFECT_AMOUNTwith guidance on the acceptable range (using a number without the % symbol) is very informative and should help users fine-tune the effect.
388-388: Display Image Description Option
TheKIOSK_SHOW_IMAGE_DESCRIPTIONsetting's description is now clear and adequately explains that image descriptions from metadata can be displayed.
389-389: Image EXIF Information Display
The updated row forKIOSK_SHOW_IMAGE_EXIFnow clearly indicates that various EXIF data (F-number, shutter speed, focal length, ISO) will be shown when available.
390-390: Display Image Location Setting
The explanation forKIOSK_SHOW_IMAGE_LOCATIONis straightforward and leaves no ambiguity about its function.
391-391: Hide Countries Option
TheKIOSK_HIDE_COUNTRIESoption now explicitly states that it accepts a list of country names to be hidden in image location data. This level of detail is beneficial.
392-392: Enable Additional Information Display
The description forKIOSK_SHOW_MORE_INFOis succinct and clearly states that additional details about the current image(s) will be enabled when active.
393-393: Display Original Image Link in More Info Overlay
The updatedKIOSK_SHOW_MORE_INFO_IMAGE_LINKconfiguration now explicitly mentions that a link to the original image in Immich will be provided.
395-395: Multiple User API Keys Support
The row forimmich_users_api_keysnow clearly details the mapping of user names to their respective API keys. This added support increases flexibility for multi-user scenarios.
396-396: Display User Option
The newKIOSK_SHOW_USERoption, which allows the display of the user used to fetch the image, is a welcome addition that enhances transparency.
441-452: New Multiple Users Section
The "Multiple Users" section (lines 441–452) clearly explains how to configure multiple user API keys and how to access a specific user’s content via theuserURL parameter. The inclusion of a helpful tip is appreciated.
547-556: Experimental Album Video Support Section
This section now provides comprehensive details about experimental video playback support, including its limitations (e.g. autoplay with muted audio, requirement for prefetch, and varying browser codec support). The warning is appropriately highlighted.
724-742: Detailed Explanation of Filters and Date Filter Examples
The new Filters section, especially the "Date filter" subsection, is very comprehensive. The examples provided (e.g. usinglast-30-daysor a custom date range) are clear and will aid users in utilising this feature effectively.internal/immich/immich_album.go (1)
41-66: LGTM!The changes to support filtering albums by asset ID are well-implemented, with proper query parameter handling and error management.
internal/immich/immich_helpers.go (1)
33-37: LGTM!The addition of variadic headers and user-specific cache keys enhances the API call handling while maintaining backward compatibility.
frontend/public/assets/css/kiosk.css (1)
1-1467: File Excluded from ReviewThis file is located under
frontend/public/**and is therefore excluded from the review process as per the coding guidelines.🧰 Tools
🪛 Biome (1.9.4)
[error] 144-144: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.(lint/correctness/noUnknownProperty)
internal/routes/routes_asset.go
Outdated
| return c.NoContent(http.StatusNoContent) | ||
| } | ||
|
|
||
| // get and use prefetch data (if found) | ||
| if requestConfig.Kiosk.PreFetch { | ||
| if cachedViewData := fromCache(c.Request().URL.String(), deviceID); cachedViewData != nil { | ||
| requestEchoCtx := c | ||
| go imagePreFetch(requestData, requestEchoCtx) | ||
| go assetPreFetch(requestData, requestEchoCtx) |
There was a problem hiding this comment.
Avoid using Echo context c inside goroutines
Passing the Echo context c to a goroutine may lead to concurrency issues because c is not goroutine-safe. Consider extracting only the necessary data from c before starting the goroutine to prevent potential race conditions.
Apply this diff to address the issue:
- requestEchoCtx := c
- go assetPreFetch(requestData, requestEchoCtx)
+ url := c.Request().URL.String()
+ go assetPreFetch(requestData, url, deviceID)Modify the assetPreFetch function to accept the extracted parameters instead of the entire context.
Also applies to: 67-67
internal/routes/routes_asset.go
Outdated
| "github.com/damongolding/immich-kiosk/internal/utils" | ||
| "github.com/damongolding/immich-kiosk/internal/webhooks" | ||
| ) | ||
|
|
||
| // NewImage returns an echo.HandlerFunc that handles requests for new images. | ||
| // NewAsset returns an echo.HandlerFunc that handles requests for new images. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update function comment to reflect asset handling of assets
The comment for NewAsset still mentions handling requests for new images. Since the function now handles various asset types, please update the comment to accurately describe its functionality.
Apply this diff to correct the comment:
-// NewAsset returns an echo.HandlerFunc that handles requests for new images.
+// NewAsset returns an echo.HandlerFunc that handles requests for new assets.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // NewAsset returns an echo.HandlerFunc that handles requests for new images. | |
| // NewAsset returns an echo.HandlerFunc that handles requests for new assets. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
internal/routes/routes_previous_asset.go (1)
81-146:⚠️ Potential issuePotential race condition due to loop variable capture in goroutine
In the loop starting at line 81, the loop variable
iis used inside a goroutine without being properly captured, which can lead to race conditions. To ensure each goroutine uses the correct index, declare a new variable inside the loop to capture the value ofi.Apply this diff to fix the variable capture:
for i, assetID := range prevAssets { + idx := i // ... g.Go(func(id int, currentAssetID string) func() error { return func() error { // ... - ViewData.Assets[i] = common.ViewImageData{ + ViewData.Assets[idx] = common.ViewImageData{ // ... } return nil } }(prevAssetsID, currentAssetID)) }internal/video/video.go (1)
218-290: 🛠️ Refactor suggestionAdd safety checks for video downloads
To prevent issues such as downloading excessively large files or running out of disk space, consider adding safety measures like file size limits, disk space availability checks, and download timeouts.
Apply this diff to add safety checks:
func (v *VideoManager) DownloadVideo(immichAsset immich.ImmichAsset, requestConfig config.Config, deviceID string, requestUrl string) { + // Check file size + if immichAsset.ExifInfo.FileSizeInByte > maxVideoSize { + log.Error("video file too large", "size", immichAsset.ExifInfo.FileSizeInByte) + return + } + + // Check available disk space + if space, err := utils.GetAvailableDiskSpace(customTempVideoDir); err != nil { + log.Error("checking disk space", "err", err) + return + } else if space < uint64(immichAsset.ExifInfo.FileSizeInByte) { + log.Error("insufficient disk space", "required", immichAsset.ExifInfo.FileSizeInByte, "available", space) + return + } + + // Add timeout context + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel()internal/routes/routes_asset.go (1)
49-54:⚠️ Potential issueAvoid using HTTP context inside goroutines
Passing
requestCtxinto a goroutine may lead to concurrency issues because the context may not be goroutine-safe. Consider extracting only the necessary data fromrequestCtxbefore starting the goroutine to prevent potential race conditions.Apply this diff to address the issue:
- requestCtx := common.CopyContext(c) // ... - go assetPreFetch(requestData, requestCtx) + url := c.Request().URL.String() + go assetPreFetch(requestData, url, deviceID)Modify the
assetPreFetchfunction to accept the extracted parameters instead of the entire context.
🧹 Nitpick comments (7)
frontend/src/ts/polling.ts (2)
48-56: Enhance parameter validation with type-specific checks.The validation could be more specific about which parameter is missing.
- if (!interval || !kiosk) { - throw new Error("PollingController: Missing required parameters"); + if (!interval) { + throw new Error("PollingController: Missing interval parameter"); + } + if (!kiosk) { + throw new Error("PollingController: Missing kiosk element parameter");
217-222: Consider making the video timeout duration configurable.The 5-second timeout is hardcoded. Consider making it configurable through the init method for different network conditions.
+ private videoTimeout: number = 5000; // Default 5 seconds + init(interval: number, kiosk: HTMLElement | null, menu: HTMLElement | null) { // ... existing code ... + this.videoTimeout = options?.videoTimeout ?? 5000; }internal/common/common.go (1)
115-134: Consider adding mutex for concurrent header modifications.While
Clone()creates a safe copy of headers, the struct might be modified concurrently after creation. Consider adding a mutex if concurrent modifications are possible.type ContextCopy struct { + mu sync.RWMutex URL url.URL RequestHeader http.Header ResponseHeader http.Header }internal/routes/routes_video.go (1)
136-149: Consider using io.CopyN for additional safety.While using
io.CopywithSectionReaderis safe,io.CopyNprovides an extra layer of protection against reading beyond the intended chunk size.- return c.Stream(statusCode, vid.ImmichAsset.OriginalMimeType, - io.NewSectionReader(video, start, chunkSize)) + return c.Stream(statusCode, vid.ImmichAsset.OriginalMimeType, + io.LimitReader(io.NewSectionReader(video, start, chunkSize), chunkSize))internal/routes/routes_asset_helpers.go (1)
355-360: Add error handling for empty user list.When
requestConfig.Useris empty, consider logging a warning or returning an error instead of silently settingSelectedUserto empty string.if len(requestConfig.User) > 0 { randomIndex := rand.IntN(len(requestConfig.User)) requestConfig.SelectedUser = requestConfig.User[randomIndex] } else { + log.Warn("No users configured", "requestID", requestID) requestConfig.SelectedUser = "" }frontend/public/assets/css/kiosk.css (2)
401-411: Thematic Gradient Background
The styling for.asset--metadata--theme-fade::beforeutilises a radial gradient to create a fade effect. Please check if the-moz-radial-gradientprefix is still necessary alongside the standardradial-gradientfor your target browsers.
1362-1454: Comprehensive Redirects Styling
The rules in this section (including those for#redirects-containerand its child elements) offer an extensive and responsive styling solution for the redirects overlay. The use of both standard and vendor-prefixed properties (such as-moz-calc) is appropriate; please verify cross-browser consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
frontend/public/assets/css/kiosk.css(14 hunks)frontend/src/css/image.css(5 hunks)frontend/src/ts/polling.ts(1 hunks)internal/common/common.go(3 hunks)internal/routes/routes_asset.go(3 hunks)internal/routes/routes_asset_helpers.go(21 hunks)internal/routes/routes_previous_asset.go(1 hunks)internal/routes/routes_video.go(1 hunks)internal/templates/partials/metadata.templ(1 hunks)internal/video/video.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`frontend/public/**`: exclude
frontend/public/**: exclude
frontend/public/assets/css/kiosk.css
🪛 Biome (1.9.4)
frontend/public/assets/css/kiosk.css
[error] 144-144: Unknown property is not allowed.
See CSS Specifications and browser specific properties for more details.
To resolve this issue, replace the unknown property with a valid CSS property.
(lint/correctness/noUnknownProperty)
🔇 Additional comments (54)
frontend/src/ts/polling.ts (3)
4-40: Well-structured implementation of the singleton pattern and type definitions!The implementation follows TypeScript best practices with proper encapsulation and type safety.
279-279: Fix incorrect event listener removal.The current implementation creates a new function instead of using the stored reference.
- this.video?.removeEventListener("playing", () => this.handlePlayStart); + if (this.handlePlayStart) { + this.video?.removeEventListener("playing", this.handlePlayStart); + }
326-341: Clean and consistent export implementation!The exported functions provide a clean interface to the singleton instance while maintaining backwards compatibility.
internal/common/common.go (2)
98-102: LGTM! Field renaming and addition enhance clarity.The renaming from
ImmichImagetoImmichAssetbetter reflects the type's purpose, and the addition of theUserfield improves ownership tracking.
109-109: LGTM! Consistent terminology.The renaming from
ImagestoAssetsmaintains consistency with the asset-centric approach.internal/routes/routes_video.go (2)
18-37: LGTM! Robust error handling and resource cleanup.The function properly validates input, handles errors, and ensures file cleanup with
defer.
45-65: LGTM! Comprehensive caching implementation.The implementation correctly sets cache headers and handles conditional requests using ETags and Last-Modified headers.
internal/templates/partials/metadata.templ (2)
39-40: Use case-insensitive comparison for country names.The current implementation uses case-sensitive comparison which might miss matches. Use
strings.EqualFoldfor case-insensitive comparison.-if info.Country != "" && !slices.Contains(hideCountries, strings.ToLower(info.Country)) { +if info.Country != "" && !slices.ContainsFunc(hideCountries, func(c string) bool { + return strings.EqualFold(c, info.Country) +}) {
77-121: Avoid building HTML strings manually to prevent XSS vulnerabilities.Use the templating engine to render HTML content to ensure proper escaping of variables.
internal/routes/routes_asset_helpers.go (2)
6-6: Incorrect import path for math/rand.The import statement uses an incorrect path. The standard package is
math/rand, notmath/rand/v2.- "math/rand/v2" + "math/rand"
231-249: Potential for recursive calls in processVideo.The function might cause infinite recursion if video download fails repeatedly. Consider implementing a retry limit.
frontend/src/css/image.css (19)
2-4: Nested Selector Syntax for Paused States
The updates renaming from image-based selectors to asset-based selectors are clear and consistent. Please ensure that the nested syntax is correctly processed by your CSS preprocessor or build system if you are not using native nested CSS.
9-23: Asset Metadata Base Style Update
The base.asset--metadatarule is updated with absolute positioning, flex layout, and appropriate styling properties. This change standardises the styling for asset metadata and is fully aligned with the new naming conventions.
26-38: Fade Theme Pseudo-element Styling
The.asset--metadata--theme-fade::beforerule now uses a radial gradient and proper positioning. The styling here is accurate and consistent with the updated asset nomenclature.
40-53: Split View Fade Theme Adjustment
Adjustments for.frame--layout-splitview:nth-child(1) .asset--metadata--theme-fade::beforeensure the fade theme is correctly oriented for split views. The switch fromrighttoleft(withright: unset) is handled well.
56-60: Solid Theme Styling Update
The updated.asset--metadata--theme-solidrule, which includes padding and background-colour modifications, is consistent with the new design requirements.
62-66: Split View Solid Theme Update
The modifications in.frame--layout-splitview:nth-child(1) .asset--metadata--theme-solid(adjusting padding and border-radius) maintain consistency in the asset-based styling for split views.
68-70: Responsive Break Element in Asset Metadata
The rule for.asset--metadata .responsive-breaksetting the display to none is appropriate for managing the layout.
72-76: Empty State Handling for Asset Metadata
The use of the:emptypseudo-class on both.asset--metadataand its childdivefficiently hides empty elements while removing any padding. This approach is well implemented.
78-80: Z-Index Adjustment for Child Divs
Applyingz-index: 1;to alldivelements within.asset--metadataensures proper stacking order. This simple yet effective update is appreciated.
82-86: Asset Metadata Has-Icon Flex Configuration
The flex display settings on.asset--metadata--has-icon(with centre alignment and a gap) are excellent for ensuring that icons are uniformly aligned with accompanying text.
88-90: Icon Order Adjustment for First Asset Metadata
Settingorder: 2;on.asset--metadata--is-first .asset--metadata--iconcorrectly manages the order of elements in flex layouts. This change is clear and effective.
92-101: Asset Icon Styling and SVG Sizing
The styling for.asset--metadata--icon, including the nestedsvgrules for fixed dimensions and fill colour, ensures a consistent icon display. The structure is concise and clear.
103-108: EXIF F-Number Display Update
The.asset--metadata--exif--fnumberrule, with its inline display, font-size, weight, and translate transformation, is well-tuned to display EXIF f-number data legibly.
110-113: EXIF Separator Styling
The styling applied to.asset--metadata--exif--seperator(adjusting opacity and padding) provides a subtle visual separation for EXIF data. This is well executed.
115-120: Asset Metadata Description Sizing
Within the split view frames, updating.asset--metadata--descriptionto have a maximum width of 50% reinforces the design’s responsiveness. This update also addresses previous typographical issues with the class name.
122-136: Split View Asset Metadata Alignment Update
The update for.frame--layout-splitview:nth-child(1) .asset--metadatanow enforces left-aligned text (withalign-items: flex-start) and correct text styling. This refines the layout in split view contexts.
176-179: Responsive Adjustments for Asset Metadata
Within the media query for screens up to 31.25rem, reducing the padding and adjusting the max-width for.asset--metadatahelps improve the mobile display.
181-183: Responsive Adjustment for Asset Metadata Description
In smaller viewports, setting.asset--metadata--description’s max-width to 100% ensures better utilisation of available space. This is a well-considered improvement.
185-192: Responsive Styling for Asset Metadata Location
The changes applied to.asset--metadata--location—hiding span elements and displaying.responsive-breakinline—enhance responsiveness. Please confirm that these modifications integrate seamlessly with the overall layout.frontend/public/assets/css/kiosk.css (24)
7-7: New CSS Variable for Layering
The addition of the--z-below: -1;variable provides finer control over stacking order. Please ensure that its usage remains consistent across the codebase.
143-143: Optimised Text Rendering
The use oftext-rendering: optimizeLegibility;is appropriate to improve text clarity.
145-146: Vendor-Specific Font Smoothing
The inclusion of-webkit-font-smoothing: subpixel-antialiased;and-moz-osx-font-smoothing: grayscale;is correctly implemented for optimising font rendering across browsers.
379-381: Updated Asset Metadata Opacity
The rule for.polling-paused.more-info .asset--metadata, .polling-paused.redirects-open .asset--metadata { opacity: 0.2; }provides the intended visual feedback when polling is paused. This adjustment aligns with the new asset-centric naming.
412-423: Consistent Fade Effect in Splitview
The rule applied for.frame--layout-splitview:nth-child(1) .asset--metadata--theme-fade::beforemirrors the fade effect for splitview layouts. Ensure consistent rendering across different layout modes.
424-428: Solid Background for Asset Metadata
The.asset--metadata--theme-soliddeclaration, with its padding and background colour, is well defined to enhance readability.
429-433: Adjusted Solid Theme for Splitview
The modifications in.frame--layout-splitview:nth-child(1) .asset--metadata--theme-solidensure that the background styling adapts cleanly within splitview components.
434-434: Responsive Break Element
The rule for.asset--metadata .responsive-breakappears minimal and intentional for layout control.
437-441: Handling Empty Asset Metadata
The styling that hides empty.asset--metadataelements (and their child divs) is a neat way to maintain a clean layout.
442-442: Z-index on Asset Metadata Divs
The declarationz-index: 1;for.asset--metadata divis clear and helps ensure correct stacking.
445-451: Icon Alignment in Asset Metadata
The flexbox properties in.asset--metadata--has-iconeffectively align icons with text. This approach is robust and clear.
452-455: Ordering Icons for First Items
The use of order adjustments in.asset--metadata--is-first .asset--metadata--iconis well applied to set the desired layout.
456-462: Icon Container Styling
The styling for.asset--metadata--iconsets up a neat container for the icons and is implemented correctly.
463-467: SVG Icon Sizing
The dimensions and fill settings in.asset--metadata--icon svgensure that icons maintain a consistent look.
468-475: EXIF FNumber Styling
The styling applied to.asset--metadata--exif--fnumberhighlights EXIF data clearly and is appropriately emphasised.
476-479: EXIF Separator Styling
The rule for.asset--metadata--exif--seperatorwith reduced opacity and padding contributes to a refined data display.
480-482: Metadata Description Width Constraint
The constraints applied to.frame--layout-splitview .asset--metadata--descriptionhelp maintain readability on different layouts.
483-501: Layout Adjustments for Asset Metadata
The detailed styling within.frame--layout-splitview:nth-child(1) .asset--metadata—including positioning, alignment, and text shadow—appears comprehensive and well-considered.
543-556: Responsive Styling Enhancements
The media query adjustments that modify padding and maximum widths for.asset--metadataand related selectors should improve the appearance on small screens. Please verify that the changes render as expected on target devices.
568-570: Grayscale and Blur Effects in More-Info Overlay
The filter effects applied to images and videos under the.polling-paused.more-infostate create a distinct visual style. It is recommended to test performance on lower-end devices due to the application of CSS filters.
744-751: Enhanced Progress Bar Transitions
The updated transition and transform properties, along with the use of backface-visibility, are well chosen to ensure smooth animations for the progress bar. Do test across your supported browsers to confirm compatibility.
777-779: Clock Weather Container Opacity
Reducing the opacity of#clock-weather-containerwhen in a polling-paused state is consistent with similar UI feedback styling throughout the file.
1345-1360: Video Element Styling
The new rules within thevideo.cssblock for.frame--videoand its childvideoensure that video content is displayed responsively and centred within its container.
1456-1464: Utility Classes for Ordering
The newly added.order-1and.order-2classes deliver a simple and effective method for controlling the order of flex items.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
frontend/src/ts/polling.ts (1)
276-276:⚠️ Potential issueFix event listener memory leak.
The
handlePlayStartcreates a new function on each call, making it impossible to remove the correct listener invideoCleanup.- this.video?.removeEventListener("playing", this.handlePlayStart(0)); + if (this.handlePlayStartListener) { + this.video?.removeEventListener("playing", this.handlePlayStartListener); + this.handlePlayStartListener = null; + }Add this property to the class:
private handlePlayStartListener: (() => void) | null = null;And modify
handlePlayStart:handlePlayStart = (playTimeout: number) => { this.handlePlayStartListener = () => { clearTimeout(playTimeout); this.video?.removeEventListener("playing", this.handlePlayStartListener!); this.handlePlayStartListener = null; }; return this.handlePlayStartListener; };
🧹 Nitpick comments (6)
internal/routes/routes_previous_asset.go (5)
22-36: Consider enhancing documentation with error conditions.The documentation is comprehensive but could benefit from mentioning potential error conditions, such as invalid history entry format or asset processing failures.
39-42: Consider enhancing error logging.When
InitializeRequestDatafails, consider logging the error details before returning to aid in debugging.requestData, err := InitializeRequestData(c, baseConfig) if err != nil { + log.Error("Failed to initialize request data", "error", err) return err }
63-65: Consider separating validation conditions.The combined condition could be split for better readability and maintainability.
- if (!requestConfig.DisableSleep && isSleepMode(requestConfig)) || historyLen < 2 { + if !requestConfig.DisableSleep && isSleepMode(requestConfig) { + return c.NoContent(http.StatusNoContent) + } + + if historyLen < 2 { return c.NoContent(http.StatusNoContent) }
67-69: Consider safer history manipulation.The current implementation modifies the history slice in place. Consider validating the history length before accessing it and using a safer approach to history manipulation.
+ if historyLen < 2 { + return fmt.Errorf("insufficient history entries: %d", historyLen) + } + lastHistoryEntry := requestConfig.History[historyLen-2] prevAssets := strings.Split(lastHistoryEntry, ",") - requestConfig.History = requestConfig.History[:historyLen-2] + newHistory := make([]string, historyLen-2) + copy(newHistory, requestConfig.History[:historyLen-2]) + requestConfig.History = newHistory
153-153: Consider handling webhook errors.The webhook is triggered asynchronously without error handling. Consider adding error logging or a mechanism to track webhook failures.
- go webhooks.Trigger(requestData, KioskVersion, webhooks.PreviousAsset, ViewData) + go func() { + if err := webhooks.Trigger(requestData, KioskVersion, webhooks.PreviousAsset, ViewData); err != nil { + log.Error("Failed to trigger webhook", "error", err) + } + }()frontend/src/ts/polling.ts (1)
7-12: Consider making properties more type-safe.The interface could benefit from stricter type definitions:
startTimeanddurationshould be required for type "image"elementshould be required for type "video"interface ProgressSource { - type: "image" | "video"; - startTime?: number; - duration?: number; - element?: HTMLVideoElement; + type: "image" | "video"; + startTime: number; + duration: number; +} | { + type: "video"; + element: HTMLVideoElement; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/ts/polling.ts(1 hunks)internal/routes/routes_previous_asset.go(1 hunks)
🔇 Additional comments (2)
frontend/src/ts/polling.ts (2)
96-108: Well-structured asset management implementation.The asset transition and video completion handling are implemented correctly with proper cleanup.
321-338: Well-structured exports using singleton pattern.The exports are cleanly implemented, providing a consistent interface to the singleton instance.
| go func(asset *immich.ImmichAsset, requestID string, wg *sync.WaitGroup) { | ||
| defer wg.Done() | ||
| var processingErr error | ||
|
|
||
| if err := asset.AssetInfo(requestID, deviceID); err != nil { | ||
| processingErr = fmt.Errorf("failed to get asset info: %w", err) | ||
| log.Error(processingErr) | ||
| } | ||
|
|
||
| if requestConfig.ShowAlbumName { | ||
| asset.AlbumsThatContainAsset(requestID, deviceID) | ||
| } | ||
|
|
||
| }(&asset, requestID, &wg) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in the goroutine.
The error from AssetInfo is logged but not propagated to the main error group. Consider using a channel or modifying the asset's state to indicate the error.
go func(asset *immich.ImmichAsset, requestID string, wg *sync.WaitGroup) {
defer wg.Done()
- var processingErr error
+ asset.Error = nil // Add an Error field to ImmichAsset struct
if err := asset.AssetInfo(requestID, deviceID); err != nil {
- processingErr = fmt.Errorf("failed to get asset info: %w", err)
- log.Error(processingErr)
+ asset.Error = fmt.Errorf("failed to get asset info: %w", err)
+ log.Error(asset.Error)
+ return
}
if requestConfig.ShowAlbumName {
- asset.AlbumsThatContainAsset(requestID, deviceID)
+ if err := asset.AlbumsThatContainAsset(requestID, deviceID); err != nil {
+ asset.Error = fmt.Errorf("failed to get album info: %w", err)
+ log.Error(asset.Error)
+ }
}
}(&asset, requestID, &wg)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| go func(asset *immich.ImmichAsset, requestID string, wg *sync.WaitGroup) { | |
| defer wg.Done() | |
| var processingErr error | |
| if err := asset.AssetInfo(requestID, deviceID); err != nil { | |
| processingErr = fmt.Errorf("failed to get asset info: %w", err) | |
| log.Error(processingErr) | |
| } | |
| if requestConfig.ShowAlbumName { | |
| asset.AlbumsThatContainAsset(requestID, deviceID) | |
| } | |
| }(&asset, requestID, &wg) | |
| go func(asset *immich.ImmichAsset, requestID string, wg *sync.WaitGroup) { | |
| defer wg.Done() | |
| asset.Error = nil // Add an Error field to ImmichAsset struct | |
| if err := asset.AssetInfo(requestID, deviceID); err != nil { | |
| asset.Error = fmt.Errorf("failed to get asset info: %w", err) | |
| log.Error(asset.Error) | |
| return | |
| } | |
| if requestConfig.ShowAlbumName { | |
| if err := asset.AlbumsThatContainAsset(requestID, deviceID); err != nil { | |
| asset.Error = fmt.Errorf("failed to get album info: %w", err) | |
| log.Error(asset.Error) | |
| } | |
| } | |
| }(&asset, requestID, &wg) |
frontend/src/ts/polling.ts
Outdated
| resumePolling = (hideOverlay: boolean = false) => { | ||
| if (!this.isPaused) return; | ||
|
|
||
| /** | ||
| * Toggle the polling state (pause/restart) | ||
| */ | ||
| function togglePolling(hideOverlay: boolean = false) { | ||
| isPaused ? resumePolling(hideOverlay) : pausePolling(); | ||
| if (this.currentProgressSource?.type === "video" && this.video) { | ||
| this.video.play(); | ||
| } else { | ||
| this.currentProgressSource = { | ||
| type: "image", | ||
| startTime: performance.now(), | ||
| duration: this.pollInterval, | ||
| }; | ||
| } | ||
|
|
||
| this.animationFrameId = requestAnimationFrame(this.updateProgress); | ||
| this.progressBarElement?.classList.remove("progress--bar-paused"); | ||
| this.menuElement?.classList.add("navigation-hidden"); | ||
| document.body.classList.remove("polling-paused"); | ||
|
|
||
| if (hideOverlay) hideAssetOverlay(); | ||
| this.isPaused = false; | ||
| }; |
There was a problem hiding this comment.
Guard against potential race condition.
When resuming polling, there's a potential race condition if this.isPaused changes between the initial check and the actual resumption.
resumePolling = (hideOverlay: boolean = false) => {
+ if (!this.isPaused || this.animationFrameId !== null) return;
- if (!this.isPaused) return;
if (this.currentProgressSource?.type === "video" && this.video) {
this.video.play();
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resumePolling = (hideOverlay: boolean = false) => { | |
| if (!this.isPaused) return; | |
| /** | |
| * Toggle the polling state (pause/restart) | |
| */ | |
| function togglePolling(hideOverlay: boolean = false) { | |
| isPaused ? resumePolling(hideOverlay) : pausePolling(); | |
| if (this.currentProgressSource?.type === "video" && this.video) { | |
| this.video.play(); | |
| } else { | |
| this.currentProgressSource = { | |
| type: "image", | |
| startTime: performance.now(), | |
| duration: this.pollInterval, | |
| }; | |
| } | |
| this.animationFrameId = requestAnimationFrame(this.updateProgress); | |
| this.progressBarElement?.classList.remove("progress--bar-paused"); | |
| this.menuElement?.classList.add("navigation-hidden"); | |
| document.body.classList.remove("polling-paused"); | |
| if (hideOverlay) hideAssetOverlay(); | |
| this.isPaused = false; | |
| }; | |
| resumePolling = (hideOverlay: boolean = false) => { | |
| + if (!this.isPaused || this.animationFrameId !== null) return; | |
| - if (!this.isPaused) return; | |
| if (this.currentProgressSource?.type === "video" && this.video) { | |
| this.video.play(); | |
| } else { | |
| this.currentProgressSource = { | |
| type: "image", | |
| startTime: performance.now(), | |
| duration: this.pollInterval, | |
| }; | |
| } | |
| this.animationFrameId = requestAnimationFrame(this.updateProgress); | |
| this.progressBarElement?.classList.remove("progress--bar-paused"); | |
| this.menuElement?.classList.add("navigation-hidden"); | |
| document.body.classList.remove("polling-paused"); | |
| if (hideOverlay) hideAssetOverlay(); | |
| this.isPaused = false; | |
| }; |
| if (this.pausedTime !== null) { | ||
| this.lastPollTime! += timestamp - this.pausedTime; | ||
| this.pausedTime = null; | ||
| } |
There was a problem hiding this comment.
Fix potential null reference issue.
The code assumes this.lastPollTime is not null when this.pausedTime is not null. This assumption should be validated.
if (this.pausedTime !== null) {
+ if (this.lastPollTime === null) return;
this.lastPollTime += timestamp - this.pausedTime;
this.pausedTime = null;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (this.pausedTime !== null) { | |
| this.lastPollTime! += timestamp - this.pausedTime; | |
| this.pausedTime = null; | |
| } | |
| if (this.pausedTime !== null) { | |
| if (this.lastPollTime === null) return; | |
| this.lastPollTime += timestamp - this.pausedTime; | |
| this.pausedTime = null; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
frontend/src/ts/polling.ts (1)
65-68:⚠️ Potential issueFix potential null reference issue.
The code assumes
this.lastPollTimeis not null whenthis.pausedTimeis not null.if (this.pausedTime !== null) { + if (this.lastPollTime === null) return; this.lastPollTime! += timestamp - this.pausedTime; this.pausedTime = null; }
🧹 Nitpick comments (1)
frontend/src/ts/polling.ts (1)
7-12: Enhance type safety in ProgressSource interface.Consider making properties required based on the type to prevent runtime errors.
interface ProgressSource { type: "image" | "video"; - startTime?: number; - duration?: number; - element?: HTMLVideoElement; + startTime: number | null; + duration: number | null; + element: HTMLVideoElement | null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/ts/polling.ts(1 hunks)
🔇 Additional comments (3)
frontend/src/ts/polling.ts (3)
96-100: LGTM! Clean implementation of asset transition.The method properly handles state cleanup before triggering a new asset load.
163-183: LGTM! Race condition fixed.The implementation now properly guards against race conditions with the check
if (!this.isPaused || this.animationFrameId !== null) return;.
321-338: LGTM! Clean singleton exports.The exports provide a well-structured interface to the singleton instance, maintaining encapsulation.
frontend/src/ts/polling.ts
Outdated
| private videoCleanup = () => { | ||
| this.video?.removeEventListener("ended", this.videoEndedHandler); | ||
| this.video?.removeEventListener("error", this.handleVideoError); | ||
| this.video?.removeEventListener("playing", this.handlePlayStart(0)); | ||
|
|
||
| this.progressBarElement?.classList.add("progress--bar-paused"); | ||
|
|
||
| this.video?.pause(); | ||
| this.video = null; | ||
|
|
||
| this.currentProgressSource = null; | ||
|
|
||
| if (this.animationFrameId) { | ||
| cancelAnimationFrame(this.animationFrameId); | ||
| this.animationFrameId = null; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Fix event listener cleanup in videoCleanup.
The handlePlayStart call in removeEventListener creates a new listener function, which won't match the original listener.
private videoCleanup = () => {
this.video?.removeEventListener("ended", this.videoEndedHandler);
this.video?.removeEventListener("error", this.handleVideoError);
- this.video?.removeEventListener("playing", this.handlePlayStart(0));
this.progressBarElement?.classList.add("progress--bar-paused");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private videoCleanup = () => { | |
| this.video?.removeEventListener("ended", this.videoEndedHandler); | |
| this.video?.removeEventListener("error", this.handleVideoError); | |
| this.video?.removeEventListener("playing", this.handlePlayStart(0)); | |
| this.progressBarElement?.classList.add("progress--bar-paused"); | |
| this.video?.pause(); | |
| this.video = null; | |
| this.currentProgressSource = null; | |
| if (this.animationFrameId) { | |
| cancelAnimationFrame(this.animationFrameId); | |
| this.animationFrameId = null; | |
| } | |
| }; | |
| private videoCleanup = () => { | |
| this.video?.removeEventListener("ended", this.videoEndedHandler); | |
| this.video?.removeEventListener("error", this.handleVideoError); | |
| this.progressBarElement?.classList.add("progress--bar-paused"); | |
| this.video?.pause(); | |
| this.video = null; | |
| this.currentProgressSource = null; | |
| if (this.animationFrameId) { | |
| cancelAnimationFrame(this.animationFrameId); | |
| this.animationFrameId = null; | |
| } | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
internal/immich/immich_favourites.go (1)
129-135: Utilise the allowedAssetType parameter.The new allowedAssetType parameter is not being used. The function still hardcodes ImageType in the request body.
requestBody := ImmichSearchRandomBody{ - Type: string(ImageType), + Type: string(allowedAssetType[0]), IsFavorite: true, WithExif: true, WithPeople: true, Size: requestConfig.Kiosk.FetchedAssetsSize, }
♻️ Duplicate comments (2)
internal/immich/immich_person.go (1)
90-98: Add date format validation.The date filter implementation should validate the date format before attempting to parse it.
if requestConfig.DateFilter != "" { + if !isValidDateFormat(requestConfig.DateFilter) { + log.Error("invalid date filter format", "filter", requestConfig.DateFilter) + return fmt.Errorf("invalid date filter format: %s", requestConfig.DateFilter) + } dateStart, dateEnd, err := determineDateRange(requestConfig.DateFilter) if err != nil { log.Error("malformed filter", "err", err)internal/config/config.go (1)
162-170: Add validation for user-specific API keys configuration.The configuration needs validation to ensure API keys are properly set.
As suggested in the previous review, add validation for user API keys:
+func (c *Config) validateUserApiKeys() error { + for user, key := range c.ImmichUsersApiKeys { + if key == "" { + return fmt.Errorf("empty API key for user %s", user) + } + } + + for _, user := range c.User { + if _, exists := c.ImmichUsersApiKeys[user]; !exists { + return fmt.Errorf("user %s specified but no API key found", user) + } + } + return nil +}Call this method from the
Loadfunction:func (c *Config) Load() error { // ... existing code ... c.checkRequiredFields() + if err := c.validateUserApiKeys(); err != nil { + return fmt.Errorf("user API keys validation failed: %w", err) + } // ... remaining code ... }
🧹 Nitpick comments (12)
internal/immich/immich.go (3)
124-130: Add documentation for the Tag struct.Consider adding a documentation comment explaining the purpose and usage of the Tag struct to improve code maintainability.
Add this documentation above the struct:
+// Tag represents metadata associated with an asset in the Immich system. +// It allows for flexible categorisation and organisation of assets. type Tag struct {
174-177: Consider consolidating orientation-related fields.The
IsPortraitandIsLandscapefields appear redundant as this information can be derived fromRatioWanted. Consider using a helper method instead.- RatioWanted ImageOrientation `json:"-"` - IsPortrait bool `json:"-"` - IsLandscape bool `json:"-"` + RatioWanted ImageOrientation `json:"-"` + // Helper methods + func (a *ImmichAsset) IsPortrait() bool { + return a.RatioWanted == PortraitOrientation + } + + func (a *ImmichAsset) IsLandscape() bool { + return a.RatioWanted == LandscapeOrientation + }
250-254: Update function documentation to reflect new name and purpose.The function comment still refers to "image" despite the rename to "NewAsset".
-// NewImage returns a new image instance +// NewAsset returns a new asset instance. An asset can be an image, video, +// or other supported media type. func NewAsset(base config.Config) ImmichAsset {internal/immich/immich_random.go (1)
103-106: Enhance error handling for AssetInfo.Consider adding more context to the error log and potentially handling critical failures that might affect asset display.
- err := asset.AssetInfo(requestID, deviceID) - if err != nil { - log.Error("Failed to get additional asset data", "error", err) - } + err := asset.AssetInfo(requestID, deviceID) + if err != nil { + log.Error("Failed to get additional asset data", + "error", err, + "requestID", requestID, + "assetID", asset.ID) + if isCriticalError(err) { + continue + } + }internal/immich/immich_memories.go (2)
78-108: Enhance error messages in updateMemoryCache.While the function effectively manages memory cache updates, the error messages could be more descriptive.
- log.Error("Failed to marshal assetsToCache", "error", err) + log.Error("Failed to marshal memory assets cache", + "error", err, + "memoryCount", len(memories), + "pickedMemoryIndex", pickedMemoryIndex)
161-161: Consider validating memory titles.Add validation to ensure memory titles are properly formatted and sanitised before assignment.
- i.MemoryTitle = memories[pickedMemoryIndex].Title + i.MemoryTitle = sanitiseTitle(memories[pickedMemoryIndex].Title)internal/immich/immich_date.go (2)
152-173: Consider adding error wrapping for better error context.The error handling is correct but could be more informative.
Apply this diff to improve error context:
-return dateStart, dateEnd, err +return dateStart, dateEnd, fmt.Errorf("processing date range: %w", err)
217-224: Consider adding input validation for extractDays.The function should validate that the extracted number is positive.
Apply this diff to add validation:
func extractDays(s string) (int, error) { re := regexp.MustCompile(`\d+`) match := re.FindString(s) if match == "" { return 0, fmt.Errorf("no number found") } - return strconv.Atoi(match) + days, err := strconv.Atoi(match) + if err != nil { + return 0, err + } + if days <= 0 { + return 0, fmt.Errorf("days must be positive, got %d", days) + } + return days, nil }internal/immich/immich_helpers.go (1)
109-120: Enhance error handling for API key selection.The error message could be more descriptive for troubleshooting.
Apply this diff to improve error messaging:
- return responseBody, fmt.Errorf("no API key found for user %s in the config", requestConfig.SelectedUser) + return responseBody, fmt.Errorf("no API key found for user '%s'. Please check the 'immich_users_api_keys' configuration", requestConfig.SelectedUser)README.md (3)
348-348: Fix hyphenation in time format descriptions.Use proper hyphenation for time format descriptions.
Apply these changes:
- Change "12 hour or 24 hour format" to "12-hour or 24-hour format"
Also applies to: 387-387
🧰 Tools
🪛 LanguageTool
[grammar] ~348-~348: When ‘24-hour’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...Display clock time in either 12 hour or 24 hour format. Can either be 12 or 24. ...(HOUR_HYPHEN)
[style] ~348-~348: To form a complete sentence, be sure to include a subject or ‘there’.
Context: ...me in either 12 hour or 24 hour format. Can either be 12 or 24. | | show...(MISSING_IT_THERE)
351-351: Fix grammar in refresh description.The description has incorrect article usage.
Change "The amount in seconds a image" to "The amount in seconds an image"
🧰 Tools
🪛 LanguageTool
[uncategorized] ~351-~351: The preposition ‘of’ seems more likely in this position.
Context: ... | 60 | The amount in seconds a image will be displayed for. ...(AI_HYDRA_LEO_REPLACE_IN_OF)
[misspelling] ~351-~351: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ... | 60 | The amount in seconds a image will be displayed for. ...(EN_A_VS_AN)
383-383: Fix verb usage in fallback description.The word "fallback" is used incorrectly as a verb.
Change "will fallback to" to "will fall back to"
🧰 Tools
🪛 LanguageTool
[grammar] ~383-~383: The word “fallback” is a noun. The verb is spelled with a space.
Context: ...not a png, gif, jpeg or webp Kiosk will fallback to using the preview. | | show_album_na...(NOUN_VERB_CONFUSION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
README.md(11 hunks)internal/config/config.go(4 hunks)internal/immich/immich.go(5 hunks)internal/immich/immich_album.go(5 hunks)internal/immich/immich_date.go(5 hunks)internal/immich/immich_favourites.go(4 hunks)internal/immich/immich_helpers.go(7 hunks)internal/immich/immich_memories.go(4 hunks)internal/immich/immich_person.go(4 hunks)internal/immich/immich_random.go(3 hunks)internal/kiosk/kiosk.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- internal/kiosk/kiosk.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/immich/immich_album.go
🧰 Additional context used
🪛 LanguageTool
README.md
[grammar] ~348-~348: When ‘24-hour’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...Display clock time in either 12 hour or 24 hour format. Can either be 12 or 24. ...
(HOUR_HYPHEN)
[style] ~348-~348: To form a complete sentence, be sure to include a subject or ‘there’.
Context: ...me in either 12 hour or 24 hour format. Can either be 12 or 24. | | show...
(MISSING_IT_THERE)
[uncategorized] ~351-~351: The preposition ‘of’ seems more likely in this position.
Context: ... | 60 | The amount in seconds a image will be displayed for. ...
(AI_HYDRA_LEO_REPLACE_IN_OF)
[misspelling] ~351-~351: Use “an” instead of ‘a’ if the following word starts with a vowel sound, e.g. ‘an article’, ‘an hour’.
Context: ... | 60 | The amount in seconds a image will be displayed for. ...
(EN_A_VS_AN)
[uncategorized] ~353-~353: Possible missing article found.
Context: ...not necessary, but if you are accessing Kiosk on a low-powered device, this may help....
(AI_HYDRA_LEO_MISSING_THE)
[grammar] ~380-~380: Consider using either the past participle “contained” or the present participle “containing” here.
Context: ...mage will fit on the screen. Default is contain. See Image fit for more i...
(BEEN_PART_AGREEMENT)
[grammar] ~383-~383: The word “fallback” is a noun. The verb is spelled with a space.
Context: ...not a png, gif, jpeg or webp Kiosk will fallback to using the preview. | | show_album_na...
(NOUN_VERB_CONFUSION)
[grammar] ~387-~387: When ‘24-hour’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...Display image time in either 12 hour or 24 hour format. Can either be 12 or 24. ...
(HOUR_HYPHEN)
[style] ~387-~387: To form a complete sentence, be sure to include a subject or ‘there’.
Context: ...me in either 12 hour or 24 hour format. Can either be 12 or 24. | | show...
(MISSING_IT_THERE)
🪛 markdownlint-cli2 (0.17.2)
README.md
346-346: Bare URL used
null
(MD034, no-bare-urls)
465-465: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
560-560: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
🔇 Additional comments (10)
internal/immich/immich.go (3)
74-76: Well-structured asset type categorisation!The new asset type slices provide a clear and type-safe way to filter assets by their type.
256-256: Verify header usage in API calls.The updated
ImmichApiCallsignature now supports optional headers. Ensure all existing call sites are updated accordingly.✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for existing ImmichApiCall usages rg -A 5 "ImmichApiCall.*\(" || rg -A 5 "func.*\(.*ImmichApiCall\)"Length of output: 356
Header usage in ImmichApiCall verified
Based on the search results, the updated signature using a variadic parameter for headers is only defined in one location and does not break existing call sites. Existing invocations that don’t pass header values remain valid, while new code can include headers when needed.
239-248: Add validation for bounding box coordinates.Consider adding validation to ensure bounding box coordinates form a valid rectangle (x2 > x1, y2 > y1) and are within image dimensions.
❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Search for existing coordinate validation in the codebase rg -A 5 "func.*validate.*[Bb]ounding[Bb]ox" || rg -A 5 "func.*check.*[Bb]ounding[Bb]ox"Length of output: 88
MISSING BOUNDING BOX VALIDATION IN ASSETFACE RESPONSE
The
AssetFaceResponsestruct ininternal/immich/immich.gocurrently does not enforce any checks to ensure the bounding box coordinates form a valid rectangle (i.e. ensuring thatBoundingBoxX2 > BoundingBoxX1andBoundingBoxY2 > BoundingBoxY1) or that they reside within the provided image dimensions. A repository-wide search for functions that perform such validation (e.g. functions matchingvalidateBoundingBoxor similar) returned no results, confirming that no coordinate validation is implemented at this stage.Please consider introducing explicit validation either during struct initialisation or in a dedicated function to prevent potential issues with invalid data. This will help enhance the robustness of face detection handling.
- Ensure
BoundingBoxX2 > BoundingBoxX1andBoundingBoxY2 > BoundingBoxY1during validation.- Validate that the bounding coordinates do not exceed
ImageWidthandImageHeight.internal/immich/immich_random.go (1)
89-130: LGTM! Improved caching and asset validation.The changes enhance the code by:
- Adding user-specific cache keys for better isolation
- Centralising asset validation logic
- Adding granular control through tag skipping
internal/immich/immich_date.go (3)
34-40: LGTM! Good refactoring of date range handling.The code has been nicely refactored to use a dedicated helper function for date range determination, improving readability and maintainability.
180-213: LGTM! Comprehensive date range processing.The function handles all edge cases well:
- Special "today" value
- Date range validation
- Date swapping for incorrect order
- End date adjustment to last nanosecond
229-241: LGTM! Clean implementation of last days processing.The function correctly calculates the date range based on the number of days.
internal/immich/immich_helpers.go (2)
469-476: LGTM! Efficient tag checking implementation.The case-insensitive comparison is a good choice for tag matching.
486-498: LGTM! Comprehensive asset validation.The function effectively checks all required conditions:
- Type validation
- Trash status
- Archive status
- Ratio requirements
- Blacklist check
internal/config/config.go (1)
206-208: LGTM! Clear sleep mode configuration.The
DisableSleepfield is well-documented and properly defaulted.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
frontend/src/ts/polling.ts (2)
66-69: Fix potential null reference issue.The code assumes
this.lastPollTimeis not null whenthis.pausedTimeis not null. This assumption should be validated.if (this.pausedTime !== null) { + if (this.lastPollTime === null) return; this.lastPollTime! += timestamp - this.pausedTime; this.pausedTime = null; }
281-281: Fix event listener cleanup in videoCleanup.The
handlePlayStartcall inremoveEventListenercreates a new listener function, which won't match the original listener.- this.video?.removeEventListener("playing", this.handlePlayStart());
🧹 Nitpick comments (1)
frontend/src/ts/polling.ts (1)
15-15: Consider marking the singleton instance as readonly.To prevent accidental reassignment of the singleton instance, consider marking it as readonly.
- private static instance: PollingController; + private static readonly instance: PollingController;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/ts/polling.ts(1 hunks)
🔇 Additional comments (3)
frontend/src/ts/polling.ts (3)
97-130: Well-structured asset management implementation.The asset transition and polling initialization logic is clean and properly handles state management.
164-184: Robust polling control implementation.The polling control methods are well-implemented with proper guard clauses to prevent race conditions.
328-343: Clean and well-organized exports.The exports properly delegate to the singleton instance, maintaining encapsulation while providing a clean public API.
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests
Chores