Information overlays #194
Information overlays #194
Conversation
WalkthroughThe changes in this pull request introduce a new CSS file, Changes
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: 17
🧹 Outside diff range and nitpick comments (18)
frontend/src/css/clock-weather-container.css (1)
1-5: Consider increasing the opacity value for better readabilityThe opacity value of 0.2 makes the content quite faint. Consider increasing it to around 0.4-0.5 to maintain visual hierarchy whilst ensuring content remains readable.
.polling-paused.more-info { #clock-weather-container { - opacity: 0.2; + opacity: 0.4; } }frontend/src/ts/menu.ts (2)
11-11: Fix spelling in variable nameThe variable name contains a spelling error: "Visable" should be "Visible".
-let imageOverlayVisable = false; +let imageOverlayVisible = false;
31-46: Consider implementing a proper state management solutionThe current implementation mixes DOM manipulation with state management. Consider:
- Using a dedicated state management solution (e.g., Redux, MobX)
- Separating overlay management into its own module
- Adding event emitters for state changes instead of direct DOM manipulation
This would improve maintainability and make the code more testable.
frontend/src/css/image-more-info.css (4)
1-3: Consider using a class selector instead of an IDUsing ID selectors can make the styles less reusable and harder to maintain. Consider converting
#more-infoto a class selector (e.g.,.more-info-overlay) for better modularity and reusability.-#more-info { +.more-info-overlay { display: none; }
6-8: Consider performance impact of filtersThe combination of grayscale, blur, and brightness filters might impact performance, especially on lower-end devices. Consider using opacity or a semi-transparent overlay instead.
.frame--image img { - filter: grayscale(1) blur(4px) brightness(0.4); + opacity: 0.4; }
21-21: Avoid arbitrary z-index valuesUsing very high z-index values (99998) can lead to maintainability issues. Consider implementing a z-index scale system with lower, more manageable values.
- z-index: 99998; + z-index: 100;
71-79: Consider using media queries for layout changesInstead of relying on a class to handle landscape layouts, consider using media queries for a more maintainable and automated approach.
-.layout-splitview-landscape { +@media (orientation: landscape) { #more-info { flex-direction: column; .more-info--image { flex-direction: row; } } }frontend/src/css/menu.css (1)
12-12: Consider using lower z-index valuesWhilst the relative difference between z-indices is correct (navigation menu above interaction area), using extremely high values like 99997 isn't necessary and could cause conflicts with third-party components. Consider scaling down all z-index values whilst maintaining their relative ordering.
- z-index: 99997; + z-index: 97;Also update the related
.navigationclass:.navigation { /* ... other properties ... */ z-index: 99; }frontend/src/ts/polling.ts (1)
Line range hint
107-116: LGTM: Clean parameter additionThe optional parameter addition is backwards-compatible and well-implemented. The conditional overlay hiding is placed appropriately after state updates.
Consider extracting the common state update logic between
startPollingandresumePollingto reduce code duplication:+function updatePollingState(hideOverlay: boolean = false) { + progressBarElement?.classList.remove("progress--bar-paused"); + menuElement?.classList.add("navigation-hidden"); + document.body.classList.remove("polling-paused"); + if (hideOverlay) hideImageOverlay(); +}custom.example.css (1)
Line range hint
1-161: Consider modernising the CSS architectureThe current file structure imports multiple CSS files and combines them into a single file. Consider adopting a more maintainable approach:
- Use CSS Modules or a CSS-in-JS solution to better encapsulate styles and prevent naming conflicts
- Implement a CSS preprocessor (like SASS/SCSS) to:
- Utilise variables for consistent theming
- Create mixins for repeated style patterns
- Nest selectors for better readability
- Consider implementing a utility-first approach (like Tailwind CSS) for rapid development
Would you like guidance on implementing any of these architectural improvements?
🧰 Tools
🪛 Biome
[error] 62-62: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 63-63: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 64-64: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 66-66: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 67-67: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 68-68: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 69-69: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 70-70: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 71-71: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 72-72: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 73-73: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 74-74: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 75-75: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 76-76: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 78-78: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 79-79: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
frontend/src/ts/kiosk.ts (1)
186-189: Maintain consistency with existing code patternsThe implementation should follow the established patterns in the codebase:
- Use htmx.find instead of querySelector
- Add error handling for missing elements
Apply this improvement:
- document - .querySelector(".navigation--more-info") - ?.addEventListener("click", () => toggleImageOverlay()); + const moreInfoButton = htmx.find(".navigation--more-info") as HTMLElement | null; + if (!moreInfoButton) { + console.error("More info button not found"); + return; + } + moreInfoButton.addEventListener("click", () => toggleImageOverlay());views/views_menu.templ (1)
23-23: Add ARIA attributes for better accessibilityThe more-info overlay should include proper ARIA attributes to ensure screen readers can properly announce its presence and state.
Apply this diff to improve accessibility:
-<div id="more-info" class="hidden" hx-swap-oob="true"></div> +<div id="more-info" class="hidden" hx-swap-oob="true" role="dialog" aria-modal="true" aria-hidden="true" aria-labelledby="more-info-title"></div>views/views_image.templ (2)
28-43: Consider adding input validation.Whilst the function handles URL parsing errors well, consider validating the input parameters to ensure they're not empty and the imageID follows the expected format.
func imageUrl(baseUrl, imageID string) string { + if baseUrl == "" || imageID == "" { + return "#" + } + u, err := url.Parse(baseUrl) if err != nil { return "#" }
50-59: Simplify nested div structure.The current structure has unnecessary nesting. Consider simplifying it for better maintainability.
-<div> - <div> - <h3> - Image ID - </h3> - </div> - <div> - { img.ImmichImage.ID } - </div> -</div> +<div class="more-info--field"> + <h3>Image ID</h3> + <div>{ img.ImmichImage.ID }</div> +</div>views/views_menu_templ.go (2)
32-32: Consider enhancing keyboard navigation and menu areaThe navigation interaction area could benefit from the following improvements:
- Add keyboard navigation for the right arrow key to match the left arrow implementation
- Consider adding content to the empty menu area or remove it if not needed
- keyup[key=='ArrowLeft'] from:body throttle:1s, + keyup[key=='ArrowLeft,ArrowRight'] from:body throttle:1s,
32-32: Enhance accessibility for the more info overlayConsider adding ARIA attributes to improve accessibility:
- <div id="more-info" class="hidden" hx-swap-oob="true"> + <div id="more-info" class="hidden" hx-swap-oob="true" + role="dialog" + aria-labelledby="more-info-title" + aria-hidden="true"> + <h2 id="more-info-title" class="sr-only">Additional Information</h2>frontend/public/assets/css/kiosk.css (1)
591-591: Consider standardising border-radius values.The border-radius value of
0.74remis inconsistent with other components that use0.75rem.Apply this diff for consistency:
- border-radius: 0.74rem; + border-radius: 0.75rem;views/views_image_templ.go (1)
70-85: Handle URL parsing errors more gracefullyCurrently, if
url.Parse(baseUrl)fails, the function returns"#". Returning a fragment identifier may not provide meaningful feedback to the user or assist in debugging. Consider logging the error and returning an empty string or propagating the error to the caller.Modify the error handling as follows:
func imageUrl(baseUrl, imageID string) string { u, err := url.Parse(baseUrl) if err != nil { - return "#" + // Log the error and return an empty string + fmt.Errorf("invalid base URL: %v", err) + return "" } // Rest of the code... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
custom.example.css(4 hunks)frontend/public/assets/css/kiosk.css(7 hunks)frontend/src/css/clock-weather-container.css(1 hunks)frontend/src/css/image-more-info.css(1 hunks)frontend/src/css/image.css(1 hunks)frontend/src/css/kiosk.css(1 hunks)frontend/src/css/menu.css(4 hunks)frontend/src/ts/kiosk.ts(2 hunks)frontend/src/ts/menu.ts(3 hunks)frontend/src/ts/polling.ts(4 hunks)go.mod(1 hunks)utils/utils.go(2 hunks)views/views_image.templ(2 hunks)views/views_image_templ.go(18 hunks)views/views_menu.templ(2 hunks)views/views_menu_templ.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/src/css/kiosk.css
🧰 Additional context used
🪛 Biome
custom.example.css
[error] 43-43: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 66-66: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 67-67: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 68-68: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 69-69: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 70-70: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 71-71: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 72-72: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 73-73: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 74-74: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 75-75: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 76-76: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 93-93: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 161-161: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
🔇 Additional comments (18)
frontend/src/css/clock-weather-container.css (1)
1-5: Verify integration with related CSS files
Let's ensure this styling integrates properly with the mentioned files in the summary (custom.example.css, image-more-info.css).
✅ Verification successful
CSS integration verified and consistent
The opacity styling for #clock-weather-container within the .polling-paused.more-info selector is properly integrated across the codebase. The implementation is consistent with other related elements that use the same class combination, where similar opacity values (0.2) are applied to create a unified dimming effect during the paused state.
- The styling in
clock-weather-container.cssaligns with similar opacity treatments in:kiosk.cssfor image metadata (0.2)image.cssfor image metadata (0.2)- Other related elements using the
.polling-paused.more-infoselector
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related CSS files and their integration
# Expected: Find references to polling-paused.more-info class and verify consistent styling
# Search for related CSS files
echo "Searching for related CSS files..."
fd -e css -x grep -l "polling-paused.more-info" {} \;
# Check for potential conflicting opacity rules
echo "\nChecking for potential conflicting opacity rules..."
rg "opacity:" $(fd -e css)
Length of output: 2729
Script:
#!/bin/bash
# Let's check the actual implementation of polling-paused.more-info class in the related files
echo "Checking polling-paused.more-info implementation in related files..."
rg -A 3 "polling-paused\.more-info" ./frontend/src/css/image.css ./frontend/src/css/image-more-info.css ./custom.example.css ./frontend/public/assets/css/kiosk.css
Length of output: 5019
frontend/src/ts/menu.ts (1)
65-67: LGTM: Export modifications are correct
The new overlay management functions are properly exported, maintaining consistency with the module's existing pattern.
go.mod (1)
43-43: Verify the QR code dependency's maintenance status and security
The go-qrcode library appears to be unmaintained (last commit from June 2020). Consider evaluating more actively maintained alternatives for QR code generation.
Let's check the repository status and potential alternatives:
Consider these maintained alternatives:
github.com/yeqown/go-qrcodegithub.com/boombuler/barcode
frontend/src/css/menu.css (2)
31-31: LGTM: Refined spacing and sizing
The reduced gap and SVG dimensions create a more compact navigation whilst maintaining proportional relationships. These changes complement the new more-info feature by establishing a clear visual hierarchy.
Also applies to: 74-75
105-110: LGTM: Well-structured more-info styling
The new class follows BEM naming conventions and creates appropriate visual distinction through icon sizing. However, let's verify its integration with the polling states mentioned in the PR summary.
✅ Verification successful
Let me gather more information about the templates since the previous HTML search didn't work correctly.
The more-info styling appears to be correctly integrated
The CSS integration shows that the .navigation--more-info class is part of a broader styling system for the polling states and information overlays. The styling is consistently applied across multiple CSS files including image-more-info.css, clock-weather-container.css, and others, with proper BEM naming conventions maintained throughout. The smaller SVG size (1.5rem) in the menu navigation complements the existing more-info styling structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration between more-info styling and polling states
# Expected: Find related more-info and polling state classes in templates and CSS
# Check for related more-info classes in templates
echo "Checking templates for more-info usage:"
rg -t html "navigation--more-info|polling-paused.more-info"
# Check for related styling in other CSS files
echo "Checking related CSS files:"
rg -t css "polling-paused.more-info|\.more-info" --glob '!menu.css'
Length of output: 2868
Script:
#!/bin/bash
# Search for HTML/template files first
echo "Finding template files:"
fd -e html -e vue -e jsx -e tsx
# Then search for more-info usage in those files
echo -e "\nChecking templates for more-info usage:"
fd -e html -e vue -e jsx -e tsx -x rg -l "navigation--more-info|more-info"
# Check the specific menu component files
echo -e "\nChecking menu-related components:"
fd -g '*menu*' -e js -e jsx -e tsx -e vue -x rg -l "navigation--more-info"
Length of output: 420
frontend/src/ts/polling.ts (3)
2-2: LGTM: Clean import addition
The import statement is well-placed and follows proper module import conventions.
124-125: LGTM: Clean parameter forwarding
The togglePolling function cleanly forwards the hideOverlay parameter while maintaining its simple toggle logic.
69-69: Consider timing of overlay hide operation
The hideImageOverlay call occurs after state updates but before animation frame completion. Consider whether this could cause visual flickering if the animation frame updates occur rapidly.
✅ Verification successful
Let me gather more context about the animation frame handling and hideImageOverlay implementation.
Let me check the animation frame handling in the updateKiosk function to understand the timing context better.
No timing issues with overlay hide operation
The hideImageOverlay call is safely placed after animation frame cancellation in pause operations and after state updates in resume operations. The implementation ensures proper synchronisation with the animation frame lifecycle, preventing any potential visual flickering.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other timing-dependent calls to hideImageOverlay
ast-grep --pattern 'hideImageOverlay()'
Length of output: 255
Script:
#!/bin/bash
# Check hideImageOverlay implementation and animation frame usage
ast-grep --pattern 'function hideImageOverlay() {
$$$
}'
# Check for requestAnimationFrame usage
ast-grep --pattern 'requestAnimationFrame'
# Check the full polling function context
rg -B 5 -A 5 'hideImageOverlay\(\)' frontend/src/ts/polling.ts
Length of output: 89425
Script:
#!/bin/bash
# Check updateKiosk implementation
ast-grep --pattern 'function updateKiosk() {
$$$
}'
# Check for any animation frame cancellation
rg -B 3 -A 3 'cancelAnimationFrame' frontend/src/ts/polling.ts
Length of output: 568
frontend/src/css/image.css (1)
2-6: Verify the cascade order with other opacity-related styles
The new class looks good, but we should ensure it doesn't conflict with other opacity-related styles when multiple states are active.
✅ Verification successful
Let me check the specific context around the opacity declarations in image.css to understand potential cascading conflicts.
No opacity cascade conflicts found in image.css
The two opacity declarations in image.css (.polling-paused.more-info .image--metadata at 0.2 and .image--metadata--exif--seperator at 0.3) target different elements and won't conflict with each other. The new style uses proper class nesting and specificity to ensure it only affects metadata when the polling-paused and more-info states are active.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential opacity conflicts in CSS files
# Look for other opacity declarations that might interact with .polling-paused.more-info
rg -g '*.css' 'opacity:' frontend/src/css/
Length of output: 760
Script:
#!/bin/bash
# Get full context of opacity-related styles in image.css
rg -g 'image.css' -B 3 -A 3 'opacity:' frontend/src/css/image.css
Length of output: 260
frontend/src/ts/kiosk.ts (1)
18-18: LGTM!
The import of toggleImageOverlay is properly added and aligns with the new image overlay functionality.
views/views_menu.templ (1)
Line range hint 23-54: Verify integration with supporting components
Please ensure that the following dependencies are properly implemented:
- The
/image/infoendpoint for fetching more information - The
image-more-info.cssstyles referenced in the AI summary - Any JavaScript handlers for managing the overlay state
✅ Verification successful
Integration is properly implemented and functional
The verification confirms that all required components are present and correctly integrated:
- CSS styles for the more-info overlay are implemented in
frontend/src/css/image-more-info.css - TypeScript handlers in
menu.tsprovidetoggleImageOverlay,showImageOverlay, andhideImageOverlayfunctions - The overlay state is managed via CSS classes and TypeScript event handlers
- Keyboard shortcuts (pressing 'I') and click handlers are properly wired up in
kiosk.ts - The overlay integrates with the polling system to pause image rotation when active
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the existence of required supporting files and endpoints
# Check for the CSS file
echo "Checking for CSS file..."
fd -t f "image-more-info.css"
# Check for route handler
echo "Checking for route handler..."
rg -l "/image/info"
# Check for JS handlers
echo "Checking for JS handlers..."
rg -l "toggleImageOverlay|showMoreInfo|hideMoreInfo"
Length of output: 383
Script:
#!/bin/bash
# Let's examine the contents of the found files to verify the integration
# Check the CSS implementation
echo "=== CSS Implementation ==="
cat frontend/src/css/image-more-info.css
# Check the TypeScript handlers
echo -e "\n=== Menu TypeScript Implementation ==="
cat frontend/src/ts/menu.ts
echo -e "\n=== Kiosk TypeScript Implementation ==="
cat frontend/src/ts/kiosk.ts
Length of output: 10973
views/views_image.templ (2)
7-8: LGTM: Required imports added for URL handling.
The new imports are appropriate for URL manipulation functionality.
25-25: LGTM: Proper placement of the new more info section.
The renderMoreInfo template is appropriately placed after the main content and history section.
utils/utils.go (1)
38-39: LGTM: Import statement is properly structured
The addition of the go-qrcode package is appropriate for the QR code generation functionality.
frontend/public/assets/css/kiosk.css (3)
371-373: LGTM!
The opacity adjustment for image metadata when polling is paused is appropriate and consistent with the overlay design pattern.
734-736: LGTM!
The opacity adjustment for the clock-weather container when polling is paused is appropriate and consistent with the overlay design pattern.
1233-1233: LGTM!
The z-index and SVG size adjustments are appropriate for the navigation elements:
- z-index ensures proper layering with the more-info overlay
- SVG sizes are consistent with the design system
Also applies to: 1297-1298, 1319-1322
views/views_image_templ.go (1)
194-196: Avoid modifying generated code
The code in this section appears to be generated by templ and is marked as "DO NOT EDIT". Modifying generated code can lead to inconsistencies and may be overwritten. Ensure that any required changes are made in the source templates instead.
frontend/src/ts/menu.ts
Outdated
| function hideImageOverlay() { | ||
| console.log("off"); | ||
| document.body.classList.remove("more-info"); | ||
| imageOverlayVisable = false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove debug logging and fix variable spelling
Production code shouldn't include console.log statements.
function hideImageOverlay() {
- console.log("off");
document.body.classList.remove("more-info");
- imageOverlayVisable = false;
+ imageOverlayVisible = false;
}📝 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.
| function hideImageOverlay() { | |
| console.log("off"); | |
| document.body.classList.remove("more-info"); | |
| imageOverlayVisable = false; | |
| } | |
| function hideImageOverlay() { | |
| document.body.classList.remove("more-info"); | |
| imageOverlayVisible = false; | |
| } |
frontend/src/ts/menu.ts
Outdated
| function toggleImageOverlay() { | ||
| imageOverlayVisable ? hideImageOverlay() : showImageOverlay(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix variable spelling in toggle function
Update the variable name to use correct spelling.
function toggleImageOverlay() {
- imageOverlayVisable ? hideImageOverlay() : showImageOverlay();
+ imageOverlayVisible ? hideImageOverlay() : showImageOverlay();
}📝 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.
| function toggleImageOverlay() { | |
| imageOverlayVisable ? hideImageOverlay() : showImageOverlay(); | |
| } | |
| function toggleImageOverlay() { | |
| imageOverlayVisible ? hideImageOverlay() : showImageOverlay(); | |
| } |
frontend/src/ts/menu.ts
Outdated
| function showImageOverlay() { | ||
| if (!document.body.classList.contains("polling-paused")) return; | ||
| console.log("on"); | ||
| document.body.classList.add("more-info"); | ||
| imageOverlayVisable = true; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add TypeScript type safety and remove console.log
The function could benefit from TypeScript type assertions and shouldn't include debug logging in production code.
function showImageOverlay() {
if (!document.body.classList.contains("polling-paused")) return;
- console.log("on");
document.body.classList.add("more-info");
- imageOverlayVisable = true;
+ imageOverlayVisible = true;
}📝 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.
| function showImageOverlay() { | |
| if (!document.body.classList.contains("polling-paused")) return; | |
| console.log("on"); | |
| document.body.classList.add("more-info"); | |
| imageOverlayVisable = true; | |
| } | |
| function showImageOverlay() { | |
| if (!document.body.classList.contains("polling-paused")) return; | |
| document.body.classList.add("more-info"); | |
| imageOverlayVisible = true; | |
| } |
frontend/src/css/image-more-info.css
Outdated
| display: flex; | ||
| flex-direction: column; | ||
| justify-content: space-between; | ||
|
|
||
| padding: 6rem 3rem 3rem 3rem; | ||
| color: #fff; | ||
| border-radius: 0.74rem; | ||
|
|
||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 2rem; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate flex properties
The flex properties are declared twice. Remove the duplicate declarations to improve maintainability.
display: flex;
flex-direction: column;
justify-content: space-between;
padding: 6rem 3rem 3rem 3rem;
color: #fff;
border-radius: 0.74rem;
- display: flex;
- flex-direction: column;
gap: 2rem;📝 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.
| display: flex; | |
| flex-direction: column; | |
| justify-content: space-between; | |
| padding: 6rem 3rem 3rem 3rem; | |
| color: #fff; | |
| border-radius: 0.74rem; | |
| display: flex; | |
| flex-direction: column; | |
| gap: 2rem; | |
| display: flex; | |
| flex-direction: column; | |
| justify-content: space-between; | |
| padding: 6rem 3rem 3rem 3rem; | |
| color: #fff; | |
| border-radius: 0.74rem; | |
| gap: 2rem; |
frontend/src/css/image-more-info.css
Outdated
| a { | ||
| font-weight: 600; | ||
| text-decoration: underline; | ||
| color: #fff; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and user experience
The link styles should include focus and hover states, and state changes should be smooth.
a {
font-weight: 600;
text-decoration: underline;
color: #fff;
+ transition: opacity 0.2s ease;
+
+ &:hover,
+ &:focus {
+ opacity: 0.8;
+ outline: 2px solid #fff;
+ outline-offset: 2px;
+ }
}📝 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.
| a { | |
| font-weight: 600; | |
| text-decoration: underline; | |
| color: #fff; | |
| } | |
| a { | |
| font-weight: 600; | |
| text-decoration: underline; | |
| color: #fff; | |
| transition: opacity 0.2s ease; | |
| &:hover, | |
| &:focus { | |
| opacity: 0.8; | |
| outline: 2px solid #fff; | |
| outline-offset: 2px; | |
| } | |
| } |
views/views_menu_templ.go
Outdated
| @@ -29,7 +29,7 @@ func menu() templ.Component { | |||
| templ_7745c5c3_Var1 = templ.NopComponent | |||
| } | |||
| ctx = templ.ClearChildren(ctx) | |||
| _, templ_7745c5c3_Err = templ_7745c5c3_Buffer.WriteString("<nav id=\"navigation-interaction-area\"><div id=\"navigation-interaction-area--previous-image\" hx-post=\"/image/previous\" hx-target=\"#kiosk\" hx-include=\".kiosk-history--entry\" hx-trigger=\"\n\t\t\t click throttle:1s,\n\t\t\t click from:.navigation--prev-image throttle:1s,\n\t\t\t keyup[key=='ArrowLeft'] from:body throttle:1s,\n\t\t\t kiosk-prev-image throttle:1s\n\t\t\t\" hx-on::before-request=\"kiosk.checkHistoryExists(event)\" hx-on::after-request=\"kiosk.startPolling()\" hx-on::after-swap=\"kiosk.cleanupFrames(), kiosk.releaseRequestLock()\"></div><div id=\"navigation-interaction-area--menu\"></div><div id=\"navigation-interaction-area--next-image\"></div></nav><nav class=\"navigation navigation-hidden\"><div class=\"navigation--media-buttons rounded\"><div class=\"navigation--item navigation--prev-image\"><svg xmlns=\"http://www.w3.org/2000/svg\" viewBox=\"0 0 448 512\"><!--!Font Awesome Free 6.6.0 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license/free Copyright 2024 Fonticons, Inc.--><path d=\"M9.4 233.4c-12.5 12.5-12.5 32.8 0 45.3l160 160c12.5 12.5 32.8 12.5 45.3 0s12.5-32.8 0-45.3L109.2 288 416 288c17.7 0 32-14.3 32-32s-14.3-32-32-32l-306.7 0L214.6 118.6c12.5-12.5 12.5-32.8 0-45.3s-32.8-12.5-45.3 0l-160 160z\"></path></svg></div><div class=\"navigation--item navigation--play-pause\"><svg class=\"navigation--play-pause--pause\" xmlns=\"http://www.w3.org/2000/svg\" viewBox=\"0 0 320 512\" height=\"32px\" width=\"32px\"><!--!Font Awesome Free 6.6.0 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license/free Copyright 2024 Fonticons, Inc.--><path d=\"M48 64C21.5 64 0 85.5 0 112L0 400c0 26.5 21.5 48 48 48l32 0c26.5 0 48-21.5 48-48l0-288c0-26.5-21.5-48-48-48L48 64zm192 0c-26.5 0-48 21.5-48 48l0 288c0 26.5 21.5 48 48 48l32 0c26.5 0 48-21.5 48-48l0-288c0-26.5-21.5-48-48-48l-32 0z\"></path></svg> <svg class=\"navigation--play-pause--play\" xmlns=\"http://www.w3.org/2000/svg\" viewBox=\"0 0 384 512\" height=\"32px\" width=\"32px\"><!--!Font Awesome Free 6.6.0 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license/free Copyright 2024 Fonticons, Inc.--><path d=\"M73 39c-14.8-9.1-33.4-9.4-48.5-.9S0 62.6 0 80L0 432c0 17.4 9.4 33.4 24.5 41.9s33.7 8.1 48.5-.9L361 297c14.3-8.7 23-24.2 23-41s-8.7-32.2-23-41L73 39z\"></path></svg></div><div class=\"navigation--item navigation--next-image\"><svg xmlns=\"http://www.w3.org/2000/svg\" viewBox=\"0 0 448 512\"><!--!Font Awesome Free 6.6.0 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license/free Copyright 2024 Fonticons, Inc.--><path d=\"M438.6 278.6c12.5-12.5 12.5-32.8 0-45.3l-160-160c-12.5-12.5-32.8-12.5-45.3 0s-12.5 32.8 0 45.3L338.8 224 32 224c-17.7 0-32 14.3-32 32s14.3 32 32 32l306.7 0L233.4 393.4c-12.5 12.5-12.5 32.8 0 45.3s32.8 12.5 45.3 0l160-160z\"></path></svg></div></div><div class=\"navigation--item navigation--flush-cache rounded\" hx-get=\"/cache/flush\" hx-swap=\"none\"><svg width=\"24\" height=\"24\" viewBox=\"0 0 24 24\" fill=\"none\" xmlns=\"http://www.w3.org/2000/svg\"><path fill-rule=\"evenodd\" clip-rule=\"evenodd\" d=\"M2 5C2 4.19711 2.43749 3.55194 2.96527 3.08401C3.49422 2.61504 4.20256 2.2384 4.99202 1.94235C6.57833 1.34749 8.70269 1 11 1C13.2973 1 15.4217 1.34749 17.008 1.94235C17.7974 2.2384 18.5058 2.61504 19.0347 3.08401C19.5625 3.55194 20 4.19711 20 5V9.98763C20 10.0333 19.9996 10.0781 19.9989 10.1219C19.9959 10.2876 19.9945 10.3704 19.9431 10.4263C19.8917 10.4822 19.7934 10.4923 19.5967 10.5124C18.9826 10.5753 18.401 10.8669 17.985 11.3266C17.9032 11.4169 17.8623 11.4621 17.8187 11.4792C17.7751 11.4964 17.719 11.4916 17.6067 11.4819C17.4199 11.4659 17.231 11.4577 17.0404 11.4577C15.0435 11.4577 13.2497 12.3522 12.0481 13.7655C11.9557 13.8742 11.9095 13.9285 11.8524 13.956C11.7953 13.9836 11.7298 13.9858 11.599 13.9901C11.3982 13.9967 11.1984 14 11 14C8.6113 14 6.01354 13.5188 4.0508 12.5952C3.64779 12.4055 3.28325 12.2037 2.95806 11.9907C2.15337 11.4637 2 10.9324 2 9.98763V5ZM5.57313 6.13845C4.97883 5.9045 4.55524 5.65279 4.29209 5.41948C3.9354 5.10324 3.9354 4.89676 4.29209 4.58052C4.57279 4.33166 5.03602 4.06185 5.69427 3.81501C7.0034 3.32409 8.87903 3 11 3C13.121 3 14.9966 3.32409 16.3057 3.81501C16.964 4.06185 17.4272 4.33166 17.7079 4.58052C18.0646 4.89676 18.0646 5.10324 17.7079 5.41948C17.4272 5.66834 16.964 5.93815 16.3057 6.18499C14.9966 6.67591 13.121 7 11 7C10.1029 7 9.24969 6.94202 8.46467 6.83796C7.48782 6.70847 6.52272 6.51225 5.57313 6.13845ZM6.21587 10.1237C5.81919 10.0045 5.40095 10.2294 5.2817 10.6261C5.16246 11.0228 5.38736 11.441 5.78404 11.5602C6.42365 11.7525 7.13136 11.9087 7.8874 12.0234C8.29692 12.0856 8.6793 11.804 8.74146 11.3945C8.80362 10.9849 8.52203 10.6026 8.11251 10.5404C7.41722 10.4349 6.77942 10.2932 6.21587 10.1237Z\" fill=\"white\"></path> <path fill-rule=\"evenodd\" clip-rule=\"evenodd\" d=\"M10.6635 16.5223C10.7204 16.2736 10.7488 16.1493 10.6912 16.0746C10.6336 15.9999 10.5104 15.9959 10.2641 15.9879C7.84409 15.9091 5.38708 15.4342 3.1992 14.4046C3.00604 14.3137 2.81512 14.2177 2.62747 14.1164C2.3392 13.9608 2.19506 13.883 2.09753 13.9412C2 13.9993 2 14.1584 2 14.4765V18.9998C2 19.8027 2.43749 20.4479 2.96527 20.9158C3.49422 21.3848 4.20256 21.7614 4.99202 22.0575C6.57833 22.6523 8.70269 22.9998 11 22.9998C11.277 22.9998 11.5514 22.9948 11.8227 22.9848C12.219 22.9703 12.4171 22.9631 12.4672 22.8473C12.5174 22.7315 12.3782 22.5719 12.1 22.2526C11.1041 21.1097 10.5 19.6168 10.5 17.9789C10.5 17.4778 10.5565 16.9903 10.6635 16.5223ZM6.21587 17.1237C5.81919 17.0045 5.40095 17.2294 5.2817 17.6261C5.16246 18.0228 5.38736 18.441 5.78404 18.5602C6.42365 18.7525 7.13136 18.9087 7.8874 19.0234C8.29692 19.0856 8.6793 18.804 8.74146 18.3945C8.80362 17.9849 8.52203 17.6026 8.11251 17.5404C7.41722 17.4349 6.77942 17.2932 6.21587 17.1237Z\" fill=\"white\"></path> <path fill-rule=\"evenodd\" clip-rule=\"evenodd\" d=\"M19.6056 12.0284C19.0689 12.159 18.7398 12.6999 18.8704 13.2365L18.8873 13.3058C18.3149 13.0811 17.6917 12.9576 17.0404 12.9576C14.2612 12.9576 12 15.2012 12 17.9788C12 20.7564 14.2612 23 17.0404 23C19.4765 23 21.5117 21.278 21.9798 18.9829C22.0902 18.4417 21.741 17.9136 21.1998 17.8032C20.6587 17.6928 20.1305 18.0421 20.0202 18.5832C19.7396 19.959 18.5137 21 17.0404 21C15.3567 21 14 19.6429 14 17.9788C14 16.3147 15.3567 14.9576 17.0404 14.9576C17.7271 14.9576 18.3577 15.1828 18.8659 15.5627C18.8901 15.5808 18.9151 15.5978 18.9408 15.6136L19.8888 16.1967C20.2341 16.4091 20.6734 16.392 21.0011 16.1535C21.3288 15.915 21.4802 15.5023 21.3844 15.1084L20.8137 12.7635C20.6831 12.2269 20.1422 11.8978 19.6056 12.0284Z\" fill=\"white\"></path></svg></div><div class=\"navigation--item navigation--fullscreen rounded\"><svg class=\"navigation--fullscreen--enter\" xmlns=\"http://www.w3.org/2000/svg\" viewBox=\"0 0 512 512\" height=\"32px\" width=\"32px\"><!--!Font Awesome Free 6.6.0 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license/free Copyright 2024 Fonticons, Inc.--><path d=\"M344 0L488 0c13.3 0 24 10.7 24 24l0 144c0 9.7-5.8 18.5-14.8 22.2s-19.3 1.7-26.2-5.2l-39-39-87 87c-9.4 9.4-24.6 9.4-33.9 0l-32-32c-9.4-9.4-9.4-24.6 0-33.9l87-87L327 41c-6.9-6.9-8.9-17.2-5.2-26.2S334.3 0 344 0zM168 512L24 512c-13.3 0-24-10.7-24-24L0 344c0-9.7 5.8-18.5 14.8-22.2s19.3-1.7 26.2 5.2l39 39 87-87c9.4-9.4 24.6-9.4 33.9 0l32 32c9.4 9.4 9.4 24.6 0 33.9l-87 87 39 39c6.9 6.9 8.9 17.2 5.2 26.2s-12.5 14.8-22.2 14.8z\"></path></svg> <svg class=\"navigation--fullscreen--exit\" xmlns=\"http://www.w3.org/2000/svg\" viewBox=\"0 0 512 512\" height=\"32px\" width=\"32px\"><!--!Font Awesome Free 6.6.0 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license/free Copyright 2024 Fonticons, Inc.--><path d=\"M439 7c9.4-9.4 24.6-9.4 33.9 0l32 32c9.4 9.4 9.4 24.6 0 33.9l-87 87 39 39c6.9 6.9 8.9 17.2 5.2 26.2s-12.5 14.8-22.2 14.8l-144 0c-13.3 0-24-10.7-24-24l0-144c0-9.7 5.8-18.5 14.8-22.2s19.3-1.7 26.2 5.2l39 39L439 7zM72 272l144 0c13.3 0 24 10.7 24 24l0 144c0 9.7-5.8 18.5-14.8 22.2s-19.3 1.7-26.2-5.2l-39-39L73 505c-9.4 9.4-24.6 9.4-33.9 0L7 473c-9.4-9.4-9.4-24.6 0-33.9l87-87L55 313c-6.9-6.9-8.9-17.2-5.2-26.2s12.5-14.8 22.2-14.8z\"></path></svg></div></nav>") | |||
| _, templ_7745c5c3_Err = templ_7745c5c3_Buffer.WriteString("<nav id=\"navigation-interaction-area\"><div id=\"navigation-interaction-area--previous-image\" hx-post=\"/image/previous\" hx-target=\"#kiosk\" hx-include=\".kiosk-history--entry\" hx-trigger=\"\n\t\t\t click throttle:1s,\n\t\t\t click from:.navigation--prev-image throttle:1s,\n\t\t\t keyup[key=='ArrowLeft'] from:body throttle:1s,\n\t\t\t kiosk-prev-image throttle:1s\n\t\t\t\" hx-on::before-request=\"kiosk.checkHistoryExists(event)\" hx-on::after-request=\"kiosk.startPolling()\" hx-on::after-swap=\"kiosk.cleanupFrames(), kiosk.releaseRequestLock()\"></div><div id=\"navigation-interaction-area--menu\"></div><div id=\"navigation-interaction-area--next-image\"></div></nav><div id=\"more-info\" class=\"hidden\" hx-swap-oob=\"true\"></div><nav class=\"navigation navigation-hidden\"><div class=\"navigation--media-buttons rounded\"><div class=\"navigation--item navigation--prev-image\"><svg xmlns=\"http://www.w3.org/2000/svg\" viewBox=\"0 0 448 512\"><!--!Font Awesome Free 6.6.0 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license/free Copyright 2024 Fonticons, Inc.--><path d=\"M9.4 233.4c-12.5 12.5-12.5 32.8 0 45.3l160 160c12.5 12.5 32.8 12.5 45.3 0s12.5-32.8 0-45.3L109.2 288 416 288c17.7 0 32-14.3 32-32s-14.3-32-32-32l-306.7 0L214.6 118.6c12.5-12.5 12.5-32.8 0-45.3s-32.8-12.5-45.3 0l-160 160z\"></path></svg></div><div class=\"navigation--item navigation--play-pause\"><svg class=\"navigation--play-pause--pause\" xmlns=\"http://www.w3.org/2000/svg\" viewBox=\"0 0 320 512\" height=\"32px\" width=\"32px\"><!--!Font Awesome Free 6.6.0 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license/free Copyright 2024 Fonticons, Inc.--><path d=\"M48 64C21.5 64 0 85.5 0 112L0 400c0 26.5 21.5 48 48 48l32 0c26.5 0 48-21.5 48-48l0-288c0-26.5-21.5-48-48-48L48 64zm192 0c-26.5 0-48 21.5-48 48l0 288c0 26.5 21.5 48 48 48l32 0c26.5 0 48-21.5 48-48l0-288c0-26.5-21.5-48-48-48l-32 0z\"></path></svg> <svg class=\"navigation--play-pause--play\" xmlns=\"http://www.w3.org/2000/svg\" viewBox=\"0 0 384 512\" height=\"32px\" width=\"32px\"><!--!Font Awesome Free 6.6.0 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license/free Copyright 2024 Fonticons, Inc.--><path d=\"M73 39c-14.8-9.1-33.4-9.4-48.5-.9S0 62.6 0 80L0 432c0 17.4 9.4 33.4 24.5 41.9s33.7 8.1 48.5-.9L361 297c14.3-8.7 23-24.2 23-41s-8.7-32.2-23-41L73 39z\"></path></svg></div><div class=\"navigation--item navigation--next-image\"><svg xmlns=\"http://www.w3.org/2000/svg\" viewBox=\"0 0 448 512\"><!--!Font Awesome Free 6.6.0 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license/free Copyright 2024 Fonticons, Inc.--><path d=\"M438.6 278.6c12.5-12.5 12.5-32.8 0-45.3l-160-160c-12.5-12.5-32.8-12.5-45.3 0s-12.5 32.8 0 45.3L338.8 224 32 224c-17.7 0-32 14.3-32 32s14.3 32 32 32l306.7 0L233.4 393.4c-12.5 12.5-12.5 32.8 0 45.3s32.8 12.5 45.3 0l160-160z\"></path></svg></div></div><div class=\"navigation--item navigation--more-info rounded\"><svg xmlns=\"http://www.w3.org/2000/svg\" viewBox=\"0 0 192 512\"><!--!Font Awesome Free 6.7.0 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license/free Copyright 2024 Fonticons, Inc.--><path d=\"M48 80a48 48 0 1 1 96 0A48 48 0 1 1 48 80zM0 224c0-17.7 14.3-32 32-32l64 0c17.7 0 32 14.3 32 32l0 224 32 0c17.7 0 32 14.3 32 32s-14.3 32-32 32L32 512c-17.7 0-32-14.3-32-32s14.3-32 32-32l32 0 0-192-32 0c-17.7 0-32-14.3-32-32z\"></path></svg></div><div class=\"navigation--item navigation--flush-cache rounded\" hx-get=\"/cache/flush\" hx-swap=\"none\"><svg width=\"24\" height=\"24\" viewBox=\"0 0 24 24\" fill=\"none\" xmlns=\"http://www.w3.org/2000/svg\"><path fill-rule=\"evenodd\" clip-rule=\"evenodd\" d=\"M2 5C2 4.19711 2.43749 3.55194 2.96527 3.08401C3.49422 2.61504 4.20256 2.2384 4.99202 1.94235C6.57833 1.34749 8.70269 1 11 1C13.2973 1 15.4217 1.34749 17.008 1.94235C17.7974 2.2384 18.5058 2.61504 19.0347 3.08401C19.5625 3.55194 20 4.19711 20 5V9.98763C20 10.0333 19.9996 10.0781 19.9989 10.1219C19.9959 10.2876 19.9945 10.3704 19.9431 10.4263C19.8917 10.4822 19.7934 10.4923 19.5967 10.5124C18.9826 10.5753 18.401 10.8669 17.985 11.3266C17.9032 11.4169 17.8623 11.4621 17.8187 11.4792C17.7751 11.4964 17.719 11.4916 17.6067 11.4819C17.4199 11.4659 17.231 11.4577 17.0404 11.4577C15.0435 11.4577 13.2497 12.3522 12.0481 13.7655C11.9557 13.8742 11.9095 13.9285 11.8524 13.956C11.7953 13.9836 11.7298 13.9858 11.599 13.9901C11.3982 13.9967 11.1984 14 11 14C8.6113 14 6.01354 13.5188 4.0508 12.5952C3.64779 12.4055 3.28325 12.2037 2.95806 11.9907C2.15337 11.4637 2 10.9324 2 9.98763V5ZM5.57313 6.13845C4.97883 5.9045 4.55524 5.65279 4.29209 5.41948C3.9354 5.10324 3.9354 4.89676 4.29209 4.58052C4.57279 4.33166 5.03602 4.06185 5.69427 3.81501C7.0034 3.32409 8.87903 3 11 3C13.121 3 14.9966 3.32409 16.3057 3.81501C16.964 4.06185 17.4272 4.33166 17.7079 4.58052C18.0646 4.89676 18.0646 5.10324 17.7079 5.41948C17.4272 5.66834 16.964 5.93815 16.3057 6.18499C14.9966 6.67591 13.121 7 11 7C10.1029 7 9.24969 6.94202 8.46467 6.83796C7.48782 6.70847 6.52272 6.51225 5.57313 6.13845ZM6.21587 10.1237C5.81919 10.0045 5.40095 10.2294 5.2817 10.6261C5.16246 11.0228 5.38736 11.441 5.78404 11.5602C6.42365 11.7525 7.13136 11.9087 7.8874 12.0234C8.29692 12.0856 8.6793 11.804 8.74146 11.3945C8.80362 10.9849 8.52203 10.6026 8.11251 10.5404C7.41722 10.4349 6.77942 10.2932 6.21587 10.1237Z\" fill=\"white\"></path> <path fill-rule=\"evenodd\" clip-rule=\"evenodd\" d=\"M10.6635 16.5223C10.7204 16.2736 10.7488 16.1493 10.6912 16.0746C10.6336 15.9999 10.5104 15.9959 10.2641 15.9879C7.84409 15.9091 5.38708 15.4342 3.1992 14.4046C3.00604 14.3137 2.81512 14.2177 2.62747 14.1164C2.3392 13.9608 2.19506 13.883 2.09753 13.9412C2 13.9993 2 14.1584 2 14.4765V18.9998C2 19.8027 2.43749 20.4479 2.96527 20.9158C3.49422 21.3848 4.20256 21.7614 4.99202 22.0575C6.57833 22.6523 8.70269 22.9998 11 22.9998C11.277 22.9998 11.5514 22.9948 11.8227 22.9848C12.219 22.9703 12.4171 22.9631 12.4672 22.8473C12.5174 22.7315 12.3782 22.5719 12.1 22.2526C11.1041 21.1097 10.5 19.6168 10.5 17.9789C10.5 17.4778 10.5565 16.9903 10.6635 16.5223ZM6.21587 17.1237C5.81919 17.0045 5.40095 17.2294 5.2817 17.6261C5.16246 18.0228 5.38736 18.441 5.78404 18.5602C6.42365 18.7525 7.13136 18.9087 7.8874 19.0234C8.29692 19.0856 8.6793 18.804 8.74146 18.3945C8.80362 17.9849 8.52203 17.6026 8.11251 17.5404C7.41722 17.4349 6.77942 17.2932 6.21587 17.1237Z\" fill=\"white\"></path> <path fill-rule=\"evenodd\" clip-rule=\"evenodd\" d=\"M19.6056 12.0284C19.0689 12.159 18.7398 12.6999 18.8704 13.2365L18.8873 13.3058C18.3149 13.0811 17.6917 12.9576 17.0404 12.9576C14.2612 12.9576 12 15.2012 12 17.9788C12 20.7564 14.2612 23 17.0404 23C19.4765 23 21.5117 21.278 21.9798 18.9829C22.0902 18.4417 21.741 17.9136 21.1998 17.8032C20.6587 17.6928 20.1305 18.0421 20.0202 18.5832C19.7396 19.959 18.5137 21 17.0404 21C15.3567 21 14 19.6429 14 17.9788C14 16.3147 15.3567 14.9576 17.0404 14.9576C17.7271 14.9576 18.3577 15.1828 18.8659 15.5627C18.8901 15.5808 18.9151 15.5978 18.9408 15.6136L19.8888 16.1967C20.2341 16.4091 20.6734 16.392 21.0011 16.1535C21.3288 15.915 21.4802 15.5023 21.3844 15.1084L20.8137 12.7635C20.6831 12.2269 20.1422 11.8978 19.6056 12.0284Z\" fill=\"white\"></path></svg></div><div class=\"navigation--item navigation--fullscreen rounded\"><svg class=\"navigation--fullscreen--enter\" xmlns=\"http://www.w3.org/2000/svg\" viewBox=\"0 0 512 512\" height=\"32px\" width=\"32px\"><!--!Font Awesome Free 6.6.0 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license/free Copyright 2024 Fonticons, Inc.--><path d=\"M344 0L488 0c13.3 0 24 10.7 24 24l0 144c0 9.7-5.8 18.5-14.8 22.2s-19.3 1.7-26.2-5.2l-39-39-87 87c-9.4 9.4-24.6 9.4-33.9 0l-32-32c-9.4-9.4-9.4-24.6 0-33.9l87-87L327 41c-6.9-6.9-8.9-17.2-5.2-26.2S334.3 0 344 0zM168 512L24 512c-13.3 0-24-10.7-24-24L0 344c0-9.7 5.8-18.5 14.8-22.2s19.3-1.7 26.2 5.2l39 39 87-87c9.4-9.4 24.6-9.4 33.9 0l32 32c9.4 9.4 9.4 24.6 0 33.9l-87 87 39 39c6.9 6.9 8.9 17.2 5.2 26.2s-12.5 14.8-22.2 14.8z\"></path></svg> <svg class=\"navigation--fullscreen--exit\" xmlns=\"http://www.w3.org/2000/svg\" viewBox=\"0 0 512 512\" height=\"32px\" width=\"32px\"><!--!Font Awesome Free 6.6.0 by @fontawesome - https://fontawesome.com License - https://fontawesome.com/license/free Copyright 2024 Fonticons, Inc.--><path d=\"M439 7c9.4-9.4 24.6-9.4 33.9 0l32 32c9.4 9.4 9.4 24.6 0 33.9l-87 87 39 39c6.9 6.9 8.9 17.2 5.2 26.2s-12.5 14.8-22.2 14.8l-144 0c-13.3 0-24-10.7-24-24l0-144c0-9.7 5.8-18.5 14.8-22.2s19.3-1.7 26.2 5.2l39 39L439 7zM72 272l144 0c13.3 0 24 10.7 24 24l0 144c0 9.7-5.8 18.5-14.8 22.2s-19.3 1.7-26.2-5.2l-39-39L73 505c-9.4 9.4-24.6 9.4-33.9 0L7 473c-9.4-9.4-9.4-24.6 0-33.9l87-87L55 313c-6.9-6.9-8.9-17.2-5.2-26.2s12.5-14.8 22.2-14.8z\"></path></svg></div></nav>") | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve navigation controls accessibility and safety
The navigation controls require the following enhancements:
- Add proper ARIA labels and roles for accessibility
- Add confirmation dialogue for cache flush operation
- <div class="navigation--item navigation--more-info rounded">
+ <button type="button" class="navigation--item navigation--more-info rounded"
+ aria-label="Show more information">
- <div class="navigation--item navigation--flush-cache rounded"
- hx-get="/cache/flush" hx-swap="none">
+ <button type="button" class="navigation--item navigation--flush-cache rounded"
+ hx-get="/cache/flush"
+ hx-confirm="Are you sure you want to flush the cache?"
+ hx-swap="none"
+ aria-label="Flush cache">Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Ignore all *_templ.go files.
There was a problem hiding this comment.
@damongolding, understood. To ignore all *_templ.go files in future code reviews, you can configure CodeRabbit to exclude these files using the path_filters setting in your configuration. This will ensure that these generated files are not included in automated reviews.
✏️ Learnings added
Learnt from: damongolding
PR: damongolding/immich-kiosk#194
File: views/views_menu_templ.go:32-32
Timestamp: 2024-11-20T22:25:29.813Z
Learning: Ignore all `*_templ.go` files in code reviews.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
utils/utils.go
Outdated
| func CreateQrCode(link string) string { | ||
| png, err := qrcode.Encode(link, qrcode.Medium, 256) | ||
| if err != nil { | ||
| log.Error("QR code", "err", err) | ||
| return "" | ||
| } | ||
|
|
||
| i, err := ImageToBase64(png) | ||
| if err != nil { | ||
| log.Error("QR code", "err", err) | ||
| return "" | ||
| } | ||
|
|
||
| return i | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
QR code size can be reduced to improve performance
The current 256x256 size for QR codes is indeed excessive, as the CSS shows the QR code is constrained to max-width: 14rem (approximately 224 pixels) and is displayed in a flex container with a border radius. A QR code size of 128x128 would be sufficient while maintaining readability.
The suggested improvements in the review are valid:
- Reducing QR size to 128x128 will improve performance without affecting usability
- Input validation is missing and should be added
- Error logging can be more descriptive
- Function documentation would improve maintainability
However, making the size configurable via parameter isn't necessary as the QR code has a single, well-defined use case in the template with consistent styling.
🔗 Analysis chain
Add input validation and improve error handling
The function has several areas for improvement:
- Missing input validation for the link parameter
- Silent failures might cause confusion downstream
- Fixed QR code size of 256x256 might be excessive for some use cases
- Missing function documentation
Consider this improved implementation:
+// CreateQrCode generates a QR code for the given link and returns it as a base64-encoded string.
+// It returns an empty string if the QR code generation or base64 encoding fails.
+// The QR code is generated with medium error correction level.
func CreateQrCode(link string) string {
+ if link == "" {
+ log.Error("QR code generation failed: empty link provided")
+ return ""
+ }
+
+ // Use a more reasonable default size
+ const qrSize = 128
+
- png, err := qrcode.Encode(link, qrcode.Medium, 256)
+ png, err := qrcode.Encode(link, qrcode.Medium, qrSize)
if err != nil {
- log.Error("QR code", "err", err)
+ log.Error("QR code generation failed", "link", link, "err", err)
return ""
}
i, err := ImageToBase64(png)
if err != nil {
- log.Error("QR code", "err", err)
+ log.Error("QR code base64 encoding failed", "link", link, "err", err)
return ""
}
return i
}Additionally, consider:
- Making the QR code size configurable via a parameter
- Adding URL validation for the link parameter
- Returning an error instead of an empty string to allow proper error handling upstream
Let's verify the QR code size impact:
Let me gather more information about the QR code usage and image dimensions.
Let me check the CSS styling for the QR code container to understand any size constraints.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check QR code usage in templates to determine if 256x256 size is necessary
# Search for QR code rendering in templates
rg -A 5 "CreateQrCode"
Length of output: 1123
Script:
#!/bin/bash
# Check if there are any CSS styles or attributes that might affect the QR code size
rg -g '*.{css,templ}' 'img.*src=.*CreateQrCode' -B 2 -A 2
# Check if there are any other QR code related configurations or constants
rg -g '*.go' 'qr.*size|qrcode.*size|QR.*size' -i
Length of output: 395
Script:
#!/bin/bash
# Check CSS styles for the QR code container
rg -g '*.{css,scss}' 'more-info--image--qr-code' -B 2 -A 5
# Check the template file structure to understand the QR code's context
cat views/views_image.templ
Length of output: 11512
frontend/public/assets/css/kiosk.css
Outdated
| /* src/css/image-more-info.css */ | ||
| #more-info { | ||
| display: none; | ||
| } | ||
| .polling-paused.more-info .frame--image img { | ||
| -webkit-filter: grayscale(1) blur(4px) brightness(0.4); | ||
| filter: grayscale(1) blur(4px) brightness(0.4); | ||
| } | ||
| .polling-paused.more-info #more-info { | ||
| position: absolute; | ||
| top: 0; | ||
| left: 0; | ||
| width: 100%; | ||
| height: 100%; | ||
| display: -webkit-flex; | ||
| display: -moz-box; | ||
| display: flex; | ||
| gap: 0.4rem; | ||
| padding: 0.4rem; | ||
| z-index: 99998; | ||
| } | ||
| .polling-paused.more-info #more-info .more-info--image { | ||
| width: 100%; | ||
| height: 100%; | ||
| background-color: rgb(51 52 96 / 60%); | ||
| display: -webkit-flex; | ||
| display: -moz-box; | ||
| display: flex; | ||
| -webkit-flex-direction: column; | ||
| -moz-box-orient: vertical; | ||
| -moz-box-direction: normal; | ||
| flex-direction: column; | ||
| -webkit-justify-content: space-between; | ||
| -moz-box-pack: justify; | ||
| justify-content: space-between; | ||
| padding: 6rem 3rem 3rem 3rem; | ||
| color: #fff; | ||
| border-radius: 0.74rem; | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: 2rem; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate display:flex declarations and consider accessibility improvements.
There are a few issues to address in the more-info overlay styles:
- Duplicate display:flex declarations (lines 568-570 and 592-593)
- The background colour
rgb(51 52 96 / 60%)might not provide sufficient contrast for text
Apply this diff to fix the duplicates and improve accessibility:
display: -webkit-flex;
display: -moz-box;
display: flex;
-webkit-flex-direction: column;
-moz-box-orient: vertical;
-moz-box-direction: normal;
flex-direction: column;
-webkit-justify-content: space-between;
-moz-box-pack: justify;
justify-content: space-between;
padding: 6rem 3rem 3rem 3rem;
color: #fff;
border-radius: 0.74rem;
- display: flex;
- flex-direction: column;
gap: 2rem;
+ background-color: rgba(51, 52, 96, 0.85);📝 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.
| /* src/css/image-more-info.css */ | |
| #more-info { | |
| display: none; | |
| } | |
| .polling-paused.more-info .frame--image img { | |
| -webkit-filter: grayscale(1) blur(4px) brightness(0.4); | |
| filter: grayscale(1) blur(4px) brightness(0.4); | |
| } | |
| .polling-paused.more-info #more-info { | |
| position: absolute; | |
| top: 0; | |
| left: 0; | |
| width: 100%; | |
| height: 100%; | |
| display: -webkit-flex; | |
| display: -moz-box; | |
| display: flex; | |
| gap: 0.4rem; | |
| padding: 0.4rem; | |
| z-index: 99998; | |
| } | |
| .polling-paused.more-info #more-info .more-info--image { | |
| width: 100%; | |
| height: 100%; | |
| background-color: rgb(51 52 96 / 60%); | |
| display: -webkit-flex; | |
| display: -moz-box; | |
| display: flex; | |
| -webkit-flex-direction: column; | |
| -moz-box-orient: vertical; | |
| -moz-box-direction: normal; | |
| flex-direction: column; | |
| -webkit-justify-content: space-between; | |
| -moz-box-pack: justify; | |
| justify-content: space-between; | |
| padding: 6rem 3rem 3rem 3rem; | |
| color: #fff; | |
| border-radius: 0.74rem; | |
| display: flex; | |
| flex-direction: column; | |
| gap: 2rem; | |
| } | |
| /* src/css/image-more-info.css */ | |
| #more-info { | |
| display: none; | |
| } | |
| .polling-paused.more-info .frame--image img { | |
| -webkit-filter: grayscale(1) blur(4px) brightness(0.4); | |
| filter: grayscale(1) blur(4px) brightness(0.4); | |
| } | |
| .polling-paused.more-info #more-info { | |
| position: absolute; | |
| top: 0; | |
| left: 0; | |
| width: 100%; | |
| height: 100%; | |
| display: -webkit-flex; | |
| display: -moz-box; | |
| display: flex; | |
| gap: 0.4rem; | |
| padding: 0.4rem; | |
| z-index: 99998; | |
| } | |
| .polling-paused.more-info #more-info .more-info--image { | |
| width: 100%; | |
| height: 100%; | |
| background-color: rgba(51, 52, 96, 0.85); | |
| display: -webkit-flex; | |
| display: -moz-box; | |
| display: flex; | |
| -webkit-flex-direction: column; | |
| -moz-box-orient: vertical; | |
| -moz-box-direction: normal; | |
| flex-direction: column; | |
| -webkit-justify-content: space-between; | |
| -moz-box-pack: justify; | |
| justify-content: space-between; | |
| padding: 6rem 3rem 3rem 3rem; | |
| color: #fff; | |
| border-radius: 0.74rem; | |
| gap: 2rem; | |
| } |
There was a problem hiding this comment.
Ignore everything in frontend/public/*
There was a problem hiding this comment.
@damongolding, to have CodeRabbit ignore everything in frontend/public/*, you can update your CodeRabbit configuration to exclude this path using the path_filters option. Here's how you can set it up:
path_filters:
exclude:
- 'frontend/public/*'This will prevent files under frontend/public/ from being included in future code reviews.
views/views_image_templ.go
Outdated
| templ_7745c5c3_Var6, templ_7745c5c3_Err = templ.JoinStringErrs(utils.CreateQrCode(imageUrl(viewData.ImmichUrl, img.ImmichImage.ID))) | ||
| if templ_7745c5c3_Err != nil { | ||
| return templ.Error{Err: templ_7745c5c3_Err, FileName: `views/views_image.templ`, Line: 80, Col: 84} | ||
| } | ||
| _, templ_7745c5c3_Err = templ_7745c5c3_Buffer.WriteString(templ.EscapeString(templ_7745c5c3_Var6)) |
There was a problem hiding this comment.
Add error handling for QR code generation
The utils.CreateQrCode() function may return an error that's currently not being checked. Ignoring this error could lead to runtime panics or silent failures. Ensure that errors are properly handled to maintain robustness.
Update the code to handle the error:
var templ_7745c5c3_Var6 string
-templ_7745c5c3_Var6, templ_7745c5c3_Err = templ.JoinStringErrs(utils.CreateQrCode(imageUrl(viewData.ImmichUrl, img.ImmichImage.ID)))
+qrCode, err := utils.CreateQrCode(imageUrl(viewData.ImmichUrl, img.ImmichImage.ID))
+if err != nil {
+ return templ.Error{Err: err, FileName: `views/views_image.templ`, Line: 80, Col: 84}
+}
+templ_7745c5c3_Var6, templ_7745c5c3_Err = templ.JoinStringErrs(qrCode)
if templ_7745c5c3_Err != nil {
return templ.Error{Err: templ_7745c5c3_Err, FileName: `views/views_image.templ`, Line: 80, Col: 84}
}📝 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.
| templ_7745c5c3_Var6, templ_7745c5c3_Err = templ.JoinStringErrs(utils.CreateQrCode(imageUrl(viewData.ImmichUrl, img.ImmichImage.ID))) | |
| if templ_7745c5c3_Err != nil { | |
| return templ.Error{Err: templ_7745c5c3_Err, FileName: `views/views_image.templ`, Line: 80, Col: 84} | |
| } | |
| _, templ_7745c5c3_Err = templ_7745c5c3_Buffer.WriteString(templ.EscapeString(templ_7745c5c3_Var6)) | |
| qrCode, err := utils.CreateQrCode(imageUrl(viewData.ImmichUrl, img.ImmichImage.ID)) | |
| if err != nil { | |
| return templ.Error{Err: err, FileName: `views/views_image.templ`, Line: 80, Col: 84} | |
| } | |
| templ_7745c5c3_Var6, templ_7745c5c3_Err = templ.JoinStringErrs(qrCode) | |
| if templ_7745c5c3_Err != nil { | |
| return templ.Error{Err: templ_7745c5c3_Err, FileName: `views/views_image.templ`, Line: 80, Col: 84} | |
| } | |
| _, templ_7745c5c3_Err = templ_7745c5c3_Buffer.WriteString(templ.EscapeString(templ_7745c5c3_Var6)) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.coderabbit.yaml (1)
9-9: Add a newline at the end of fileYAML files should end with a newline character to comply with POSIX standards and ensure proper file concatenation.
Apply this diff:
- path: "*_templ.go" - action: ignore + action: ignore +🧰 Tools
🪛 yamllint
[error] 9-9: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.coderabbit.yaml(1 hunks)frontend/src/css/image-more-info.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/css/image-more-info.css
🧰 Additional context used
🪛 yamllint
.coderabbit.yaml
[error] 9-9: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (1)
.coderabbit.yaml (1)
1-9: Path filters configuration looks sensible
The configuration appropriately excludes:
- Example CSS file that shouldn't be processed
- Public assets directory that likely contains generated files
- Generated Go template files
This helps focus the reviews on meaningful code changes.
🧰 Tools
🪛 yamllint
[error] 9-9: no new line character at the end of file
(new-line-at-end-of-file)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.coderabbit.yaml(1 hunks)
🔇 Additional comments (2)
.coderabbit.yaml (2)
6-7: Verify the frontend/public directory structure
The configuration to ignore the public directory is properly formatted. Let's confirm the directory structure.
✅ Verification successful
Frontend public directory exists and is properly configured for ignoring
The verification confirms that the frontend/public/ directory exists in the expected location, validating the ignore configuration in .coderabbit.yaml.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the frontend/public directory exists
# Expected: Directory should exist as it's typically used for static assets
fd -t d "^public$" frontend/
Length of output: 47
9-10: Verify the template file pattern
The wildcard pattern for ignoring template files is correct. Let's verify if there are matching files.
✅ Verification successful
Template file pattern correctly matches existing files
The pattern *_templ.go successfully matches 8 template files in the views directory, confirming that the ignore pattern is correctly configured to exclude Go template files from analysis.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for template files matching the pattern
# Expected: Should find Go template files if they exist
fd "_templ\.go$"
Length of output: 249
.coderabbit.yaml
Outdated
| reviews: | ||
| path_filters: | ||
| - path: "custom.example.css" | ||
| action: ignore |
There was a problem hiding this comment.
💡 Codebase verification
The path filter configuration appears to be incorrect
This CSS file appears to be an actual stylesheet containing real application styles, not just an example file. It contains numerous CSS rules for various components like clock, weather, menu, and kiosk functionality. Ignoring this file would mean excluding important styling rules from code reviews.
- Consider removing this path filter from
.coderabbit.yamlas the file appears to be a crucial part of the application's styling system. - If you need to exclude specific CSS files, ensure you're targeting the correct files that should genuinely be ignored.
🔗 Analysis chain
Verify the existence of the example CSS file
The path filter configuration looks correct. However, let's verify if this example file exists in the repository.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the example CSS file exists
# Expected: No matches as it should be an example file that doesn't exist
fd "custom.example.css"
Length of output: 42
Script:
#!/bin/bash
# Let's check the content of the CSS file to understand if it's truly an example file
cat custom.example.css
Length of output: 4854
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (25)
frontend/src/css/vars.css (1)
7-7: Consider grouping related gradientsWhilst the gradient section is well-commented, consider expanding this section if more gradients are planned. This would help maintain a scalable structure as the design system grows.
frontend/src/ts/menu.ts (2)
11-11: Add type annotation for better type safetyAdd explicit type annotation to the variable declaration for better TypeScript type safety and code readability.
-let imageOverlayVisible = false; +let imageOverlayVisible: boolean = false;
31-46: Add JSDoc documentation for the new functionsThe new overlay functions lack documentation. Add JSDoc comments to maintain consistency with the existing codebase style and improve maintainability.
+/** + * Shows the image overlay if polling is paused + */ function showImageOverlay() { +/** + * Hides the image overlay + */ function hideImageOverlay() { +/** + * Toggles the visibility of the image overlay + */ function toggleImageOverlay() {frontend/src/css/image-more-info.css (4)
1-6: Consider enhancing accessibility for screen readersWhilst the text selection functionality is good, consider adding ARIA attributes to improve screen reader support.
#more-info { display: none; * { user-select: text; + cursor: text; } + &[aria-hidden="true"] { + display: none; + } + &[aria-hidden="false"] { + display: flex; + } }
9-11: Consider using CSS custom properties for filter valuesExtract the filter values into CSS custom properties for better maintainability and consistency.
+:root { + --image-blur-amount: 4px; + --image-brightness: 0.4; +} .polling-paused.more-info { .frame--image img { - filter: grayscale(1) blur(4px) brightness(0.4); + filter: grayscale(1) blur(var(--image-blur-amount)) brightness(var(--image-brightness)); }
24-24: Consider reducing the z-index valueThe z-index value of 99998 is unnecessarily high and could cause stacking context issues.
- z-index: 99998; + z-index: 10;
29-29: Use CSS custom properties for coloursConvert the RGB colour value to a CSS custom property for better theme management.
+:root { + --overlay-background: rgb(51 52 96 / 60%); +} - background-color: rgb(51 52 96 / 60%); + background-color: var(--overlay-background);webhooks/webhooks.go (1)
Line range hint
67-116: The existing Trigger function seamlessly supports the new event.The generic webhook handling implementation with proper error handling, configurable timeouts, and flexible payload structure makes this a clean extension of functionality. No modifications to the core webhook logic were necessary.
Consider documenting the new webhook event in the project's webhook documentation to help users integrate with this feature.
custom.example.css (3)
65-79: Consider organising CSS with BEM methodologyThe new overlay-related styles could benefit from following the Block Element Modifier (BEM) methodology for better maintainability. For example:
.more-info {} .more-info__image {} .more-info__image--polling-paused {}This would make the relationship between components clearer and reduce CSS specificity issues.
Would you like me to help restructure these selectors using BEM?
🧰 Tools
🪛 Biome
[error] 66-66: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 67-67: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 68-68: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 69-69: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 70-70: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 71-71: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 72-72: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 73-73: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 74-74: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 75-75: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 76-76: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 77-77: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 78-78: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 79-79: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
76-77: Consider using CSS custom properties for layout valuesThe landscape layout styles could benefit from CSS custom properties to maintain consistent values:
:root { --more-info-width: 300px; --more-info-padding: 1rem; }This would make it easier to maintain consistent spacing and dimensions across different layouts.
🧰 Tools
🪛 Biome
[error] 76-76: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 77-77: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
Line range hint
1-1: Consider adding documentation for customisationAs this is an example file, it would be helpful to add comments explaining:
- How to customise the styles
- Which values should be modified
- The relationship between different CSS modules
This would make it easier for users to understand how to implement their own styles.
🧰 Tools
🪛 Biome
[error] 40-40: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 41-41: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 43-43: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 44-44: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 45-45: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 46-46: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
views/views_menu.templ (1)
23-23: Consider enhancing the more-info container's HTMX integrationThe more-info container is properly structured for out-of-band swapping, but it lacks explicit HTMX attributes for content loading and visibility toggling.
Consider adding these HTMX attributes:
-<div id="more-info" class="hidden" hx-swap-oob="true"></div> +<div + id="more-info" + class="hidden" + hx-swap-oob="true" + hx-swap="innerHTML" + hx-target="this" +></div>immich/immich.go (2)
50-55: Consider adjusting connection pool settings based on load patternsThe transport configuration uses fixed values that might need adjustment based on your application's traffic patterns:
MaxIdleConns: 100might be excessive for low-traffic scenariosIdleConnTimeout: 90scould lead to connection churn under bursty trafficConsider implementing monitoring to track connection pool utilisation and adjust these values accordingly. Key metrics to monitor:
- Connection pool saturation
- Connection establishment latency
- Connection lifetime distribution
47-59: Consider implementing circuit breaker patternThe HTTP client configuration would benefit from a circuit breaker implementation to handle API failures gracefully. This is particularly important given the connection pooling configuration.
Consider using a library like
sony/gobreakerorafex/hystrix-goto implement circuit breaking. Example implementation:import "github.com/sony/gobreaker" var ( breaker = gobreaker.NewCircuitBreaker(gobreaker.Settings{ Name: "immich-api", MaxRequests: 3, Interval: 10 * time.Second, Timeout: 60 * time.Second, ReadyToTrip: func(counts gobreaker.Counts) bool { failureRatio := float64(counts.TotalFailures) / float64(counts.Requests) return counts.Requests >= 3 && failureRatio >= 0.6 }, }) httpTransport = &http.Transport{ MaxIdleConns: 100, IdleConnTimeout: 90 * time.Second, DisableKeepAlives: false, MaxIdleConnsPerHost: 100, } )views/views_image.templ (3)
25-25: Consider lazy loading the additional informationThe
renderMoreInfotemplate is always rendered for all images, which might impact initial page load performance. Consider lazy loading this information when the user requests it.-@renderMoreInfo(viewData) +<div + hx-get="/more-info" + hx-trigger="click" + hx-target="#more-info" + hx-swap="innerHTML" +>Show more info</div> +<div id="more-info"></div>
79-88: Add feedback for webhook trigger buttonThe webhook trigger button lacks visual feedback and loading states. Consider adding HTMX indicators and success/error feedback.
<button class="more-info--webhook" hx-post="/webhooks" hx-trigger="click" hx-include=".kiosk-history--entry" hx-headers='{"kiosk-webhook-event": "user.webhook.trigger.info_overlay"}' + hx-indicator="#webhook-indicator" + hx-swap="outerHTML" > Trigger webhook + <span id="webhook-indicator" class="htmx-indicator"> + Processing... + </span> </button>
46-95: Add loading states for image informationThe more info section could benefit from progressive enhancement and loading states.
- Add loading indicators for QR code generation
- Progressive loading of image details
- Error states for failed operations
Example implementation:
<div id="more-info" class="hidden" hx-swap-oob="true"> for _, img := range viewData.Images { <div class="more-info--image" + hx-trigger="revealed" + hx-get="/api/image-details/{img.ImmichImage.ID}" + hx-indicator=".loading-indicator" > + <div class="loading-indicator">Loading image details...</div> ... </div> } </div>utils/utils.go (3)
Line range hint
42-52: Enhance type documentation with usage examplesThe type documentation could be more descriptive by including usage examples and explaining the relationship between
WeightedAssetandAssetWithWeighting.Add more detailed documentation:
-// WeightedAsset represents an asset with a type and ID +// WeightedAsset represents an asset with a type identifier and unique ID. +// It is typically used in conjunction with AssetWithWeighting for weighted random selection. type WeightedAsset struct { Type string ID string } -// AssetWithWeighting represents a WeightedAsset with an associated weight value +// AssetWithWeighting combines a WeightedAsset with its weight value. +// The weight value influences the probability of the asset being selected +// in weighted random selection operations. type AssetWithWeighting struct { Asset WeightedAsset Weight int }
Line range hint
394-428: Consider explicit time zone handlingThe function uses the time zone from
currentTime, but this might lead to unexpected behaviour if the caller provides times in different zones.Consider adding a time zone parameter or documenting the expected time zone behaviour:
-func IsSleepTime(sleepStartTime, sleepEndTime string, currentTime time.Time) (bool, error) { +// IsSleepTime checks if the current time falls within a sleep period. +// All times are interpreted in the time zone of currentTime. +func IsSleepTime(sleepStartTime, sleepEndTime string, currentTime time.Time) (bool, error) {
429-433: Enhance error handling and documentationThe function could be more robust by handling additional edge cases and providing more detailed documentation.
Consider this enhanced implementation:
-// FileExists checks if a file exists at the specified path +// FileExists checks if a file exists and is accessible at the specified path. +// It returns false if the file doesn't exist or if there are permission issues. func FileExists(filename string) bool { - _, err := os.Stat(filename) - return !os.IsNotExist(err) + info, err := os.Stat(filename) + if err != nil { + return false + } + return !info.IsDir() }README.md (1)
765-771: LGTM! Clear and well-structured webhook events documentation.The table provides a comprehensive overview of available webhook events with clear descriptions. The documentation will help users understand and implement webhook functionality effectively.
Consider adding example payloads for each event type to help users understand the data structure they'll receive. For instance:
| Event | Description | Example Payload | |--------------|----------------------------------------------------------|----------------| |`asset.new` | Triggered when a new image is requested from Kiosk | `{"event": "asset.new", ...}` |immich/immich_helpers.go (1)
Line range hint
147-154: Include unexpected status codes in retry logicCurrently, the function does not retry when an unexpected status code is received, and
lastErrmay remainnil, leading to incomplete error messages.Modify the logic to include unexpected status codes in the retry mechanism and ensure
lastErris assigned accordingly. Apply this diff to assignlastErr:if res.StatusCode < 200 || res.StatusCode >= 300 { err = fmt.Errorf("unexpected status code: %d", res.StatusCode) log.Error(err) + lastErr = err _, _ = io.Copy(io.Discard, res.Body) - return responseBody, err + continue }frontend/public/assets/css/kiosk.css (3)
588-590: Refactor to remove outdated-moz-boxdisplay propertyThe
display: -moz-box;property is outdated and no longer necessary. Modern browsers, including Firefox, supportdisplay: flex;without prefixes.Apply this change to simplify the code:
display: -webkit-flex; - display: -moz-box; display: flex;This enhances code maintainability and ensures better cross-browser compatibility.
591-594: Simplify flex-direction properties by removing obsolete-moz-boxprefixesThe
-moz-box-orientand-moz-box-directionproperties are obsolete. Use standardflex-directionfor consistency.Apply this diff to clean up the code:
-webkit-flex-direction: column; - -moz-box-orient: vertical; - -moz-box-direction: normal; flex-direction: column;This modernises the CSS and improves readability.
595-597: Remove outdated-moz-box-packpropertyThe
-moz-box-packproperty is deprecated. The standardjustify-contentproperty is sufficient.Apply this change:
-webkit-justify-content: space-between; - -moz-box-pack: justify; justify-content: space-between;This streamlines the code and ensures it adheres to current CSS standards.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (20)
README.md(1 hunks)custom.example.css(5 hunks)frontend/public/assets/css/kiosk.css(9 hunks)frontend/src/css/image-more-info.css(1 hunks)frontend/src/css/menu.css(5 hunks)frontend/src/css/vars.css(1 hunks)frontend/src/ts/kiosk.ts(2 hunks)frontend/src/ts/menu.ts(3 hunks)go.mod(1 hunks)immich/immich.go(1 hunks)immich/immich_album.go(1 hunks)immich/immich_helpers.go(3 hunks)main.go(1 hunks)routes/routes_webhooks.go(1 hunks)utils/utils.go(6 hunks)views/views_image.templ(2 hunks)views/views_image_templ.go(18 hunks)views/views_menu.templ(2 hunks)views/views_menu_templ.go(1 hunks)webhooks/webhooks.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/src/css/menu.css
- go.mod
- views/views_menu_templ.go
🧰 Additional context used
🪛 Biome
custom.example.css
[error] 43-43: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 66-66: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 67-67: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 68-68: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 69-69: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 70-70: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 71-71: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 72-72: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 73-73: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 74-74: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 75-75: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 76-76: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 77-77: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 78-78: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 79-79: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 96-96: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 156-156: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 165-165: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
frontend/src/ts/kiosk.ts
[error] 167-168: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 169-169: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
🔇 Additional comments (24)
frontend/src/css/vars.css (1)
2-5: Well-structured colour system implementation
The new colour variables establish a clear and maintainable colour system. The semantic naming convention (light-grey, cool-grey, mint-green) makes the purpose of each colour clear whilst maintaining consistency.
Let's verify the usage of these new colour variables across the codebase:
✅ Verification successful
New colour variables are actively used across multiple UI components
The verification confirms these colour variables are properly utilised across several key UI components:
image-more-info.css: Used for information overlaysmenu.css: Applied to menu componentskiosk.css: Implemented in the kiosk interface
The consistent usage of --mint-green for backgrounds and --cool-grey for text/icons demonstrates a cohesive design system implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for usage of the new colour variables
# Expected: Find references in CSS/SCSS files using these variables
rg --type css "var\(--(light-grey|cool-grey|mint-green)\)"
Length of output: 622
frontend/src/ts/menu.ts (2)
65-67: LGTM! Export modifications look good
The new functions are properly exported, maintaining the module's interface consistency.
31-46: Verify overlay functionality integration
Let's verify how these overlay functions are integrated with other components in the codebase.
✅ Verification successful
Overlay functionality is properly integrated across the codebase
The verification shows that the overlay functions are well-integrated and used appropriately:
- Functions are exported from
menu.tsand imported where needed (e.g.,hideImageOverlayinpolling.ts) toggleImageOverlayis used inkiosk.tsfor user interactions- CSS classes (
polling-pausedandmore-info) are consistently used across stylesheets with proper styling for different states and components - The implementation follows a clear pattern where
hideImageOverlayis called during polling operations to ensure proper state management
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of overlay functions and related CSS classes
# Check for overlay function usage
echo "Checking function usage:"
rg -t ts "(showImageOverlay|hideImageOverlay|toggleImageOverlay)"
# Check for related CSS classes
echo -e "\nChecking CSS class usage:"
rg -t css "\.more-info|\.polling-paused"
Length of output: 4032
frontend/src/css/image-more-info.css (1)
71-79: Responsive layout implementation looks good
The landscape view adaptation is well-implemented using flex direction changes.
webhooks/webhooks.go (1)
19-23: LGTM! The new webhook event constant follows established patterns.
The new UserWebhookTriggerInfoOverlay constant is well-integrated with the existing webhook events, following consistent naming and value patterns.
Let's verify the webhook event handling implementation:
✅ Verification successful
The new webhook event is properly integrated and handled
The verification confirms that:
- The new webhook event is properly defined and documented in README.md
- The event is triggered via an HTMX button in the image details overlay
- The
Triggerfunction handles all webhook events uniformly, including the new one, by:- Validating the event type against configured webhooks
- Creating a standardised payload with image details
- Sending POST requests to configured webhook URLs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the webhook event handling implementation
# Expected: Find the handler for the new info_overlay event
# Search for handlers of the new webhook event
rg -A 5 "user\.webhook\.trigger\.info_overlay"
# Look for webhook route definitions
ast-grep --pattern 'func $_($$_) {
$$$
$event := $$$
if $event == "user.webhook.trigger.info_overlay" {
$$$
}
$$$
}'
Length of output: 1947
Script:
#!/bin/bash
# Let's verify the webhook handling implementation by checking the Trigger function
# and related webhook handling code
# Find the Trigger function implementation
ast-grep --pattern 'func Trigger($$$) {
$$$
}'
# Look for webhook handler routes
rg -A 5 "func.*webhook.*handler"
# Check for any webhook event validation
rg -A 5 "WebhookEvent.*validation"
Length of output: 2538
main.go (1)
128-129: Implementation follows established patterns
The webhook route is properly integrated into the existing routing structure and follows the established pattern of route definitions. The use of POST method is appropriate for webhook endpoints.
views/views_menu.templ (2)
27-27: Excellent accessibility improvements
The addition of ARIA attributes to the SVG icons enhances screen reader compatibility whilst properly hiding decorative elements.
Also applies to: 43-43
49-54:
Address previous review feedback for the more-info button
The implementation still requires the improvements mentioned in the previous review regarding interactivity and accessibility.
Additionally, consider adding visual feedback for hover and active states:
-<div class="navigation--item navigation--more-info rounded">
+<div
+ class="navigation--item navigation--more-info rounded"
+ data-tooltip="More Information"
+ hx-get="/image/info"
+ hx-target="#more-info"
+ hx-trigger="click"
+ role="button"
+ aria-label="Show more information"
+>immich/immich.go (1)
58-59: Verify timeout configuration alignment
The HTTP client timeout is derived from requestConfig.Kiosk.HTTPTimeout, but there's no validation to ensure it's greater than the IdleConnTimeout (90s) set in the transport configuration.
✅ Verification successful
Let me gather more information about the timeout configurations.
Let me search for the transport configuration to verify the timeout settings.
Let me try a different search pattern to find the transport and timeout configurations.
HTTP client timeout configuration is properly aligned
The HTTP client timeout is set to the value from requestConfig.Kiosk.HTTPTimeout, which defaults to 20 seconds (from config.example.yaml). This is well below the IdleConnTimeout of 90 seconds set in the transport configuration, ensuring proper timeout hierarchy. The shorter HTTP timeout will trigger before the idle connection timeout, preventing any timing conflicts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for timeout configuration in the codebase
# Expected: Find the HTTPTimeout configuration to verify its value
# Search for HTTPTimeout configuration
rg -A 2 "HTTPTimeout.*=.*"
# Search for any timeout-related configuration
rg -A 2 "Timeout.*=.*[0-9]"
Length of output: 88338
Script:
#!/bin/bash
# Search for timeout-related configurations in the codebase
rg -A 2 "HTTPTimeout.*=.*[0-9]"
rg -A 2 "IdleConnTimeout.*=.*[0-9]"
rg -A 2 "timeout.*=.*[0-9]"
Length of output: 88381
Script:
#!/bin/bash
# Search for transport configuration and timeout settings
rg -A 5 "transport.*:.*{"
rg -A 5 "IdleConnTimeout.*=.*"
rg -A 5 "httpTransport.*:.*"
Length of output: 185
Script:
#!/bin/bash
# Search for transport and timeout configurations with fixed patterns
rg -A 5 "Transport:"
rg -A 5 "transport:"
rg -A 5 "Timeout:"
rg -A 5 "timeout:"
Length of output: 90421
views/views_image.templ (3)
7-8: LGTM: Appropriate standard library imports
The added imports are necessary for URL manipulation in the new imageUrl function.
74-76: Previous accessibility comment is still valid
91-91: Previous QR code accessibility comment is still valid
views/views_image_templ.go (3)
70-85: Past review comment is still valid.
The existing review comment about URL encoding the imageID parameter is still applicable.
194-196: LGTM: Improved variable naming
The variable renaming enhances code readability and maintains consistency across the codebase.
Also applies to: 227-229, 262-264
594-599: 🛠️ Refactor suggestion
Enhance image URL security
Consider implementing additional security measures for image URLs:
- Validate image URLs against an allowlist of trusted domains
- Implement Content Security Policy headers to restrict image sources
- Add URL sanitisation to prevent XSS via malicious URLs
Also applies to: 641-646, 688-693
routes/routes_webhooks.go (1)
85-85: Asynchronous Call to webhooks.Trigger May Suppress Errors
The webhooks.Trigger function is called asynchronously using go, which means any errors returned by this function will not be captured or logged. This could make debugging difficult if the webhook triggering fails.
Consider whether the asynchronous call is necessary. If it is, ensure that errors are properly handled within the webhooks.Trigger function itself. Alternatively, remove the goroutine to handle errors directly:
- go webhooks.Trigger(requestData, KioskVersion, webhooks.UserWebhookTriggerInfoOverlay, ViewData)
+ if err := webhooks.Trigger(requestData, KioskVersion, webhooks.UserWebhookTriggerInfoOverlay, ViewData); err != nil {
+ log.Error("Failed to trigger webhook", "error", err)
+ return err
+ }This modification allows you to capture and log any errors from the webhooks.Trigger function.
frontend/src/ts/kiosk.ts (4)
18-18: Import of toggleImageOverlay added correctly
The toggleImageOverlay function is properly imported from the menu module, preparing it for use later in the code.
155-156: Event listeners now correctly invoke togglePolling()
The event listeners for menuInteraction and menuPausePlayButton now correctly call togglePolling() when clicked, ensuring the polling functionality toggles as expected.
159-180: Refactor the keydown event handler for better maintainability
A previous suggestion to refactor the keydown event handler is still applicable. Refactoring can enhance readability and maintainability by simplifying the logic and clarifying variable usage.
🧰 Tools
🪛 Biome
[error] 167-168: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 169-169: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
188-190: Event listener for .navigation--more-info added appropriately
The event listener for the .navigation--more-info element correctly invokes toggleImageOverlay() on click, integrating the overlay functionality into the user interface.
frontend/public/assets/css/kiosk.css (4)
374-376: No issues with opacity adjustment for .image--metadata
The opacity reduction for .image--metadata when polling is paused and more info is displayed is appropriate.
587-587: Consider improving background colour for accessibility
The background colour rgb(51 52 96 / 60%) might not provide sufficient contrast with the text, potentially affecting readability.
756-758: No issues with opacity adjustment for #clock-weather-container
Dimming the clock and weather container when more info is displayed enhances focus on the overlay content.
1255-1255: Ensure correct stacking context with z-index
Setting z-index: 99997; for #navigation-interaction-area seems appropriate to manage layering of interactive elements.
frontend/src/css/image-more-info.css
Outdated
| .more-info--webhook { | ||
| background-color: var(--mint-green); | ||
| outline: none; | ||
| border: none; | ||
| color: var(--cool-grey); | ||
| font-weight: 600; | ||
| border-radius: 60rem; | ||
| padding: 0.5rem 2rem; | ||
| font-size: 1rem; | ||
| cursor: pointer; | ||
| &:hover { | ||
| filter: brightness(85%); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance button interaction states
Add transition effects and keyboard focus states for better accessibility and user experience.
.more-info--webhook {
background-color: var(--mint-green);
outline: none;
border: none;
color: var(--cool-grey);
font-weight: 600;
border-radius: 60rem;
padding: 0.5rem 2rem;
font-size: 1rem;
cursor: pointer;
+ transition: filter 0.2s ease;
+
&:hover {
filter: brightness(85%);
}
+ &:focus-visible {
+ outline: 2px solid var(--cool-grey);
+ outline-offset: 2px;
+ }
+ &:active {
+ filter: brightness(75%);
+ }
}📝 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.
| .more-info--webhook { | |
| background-color: var(--mint-green); | |
| outline: none; | |
| border: none; | |
| color: var(--cool-grey); | |
| font-weight: 600; | |
| border-radius: 60rem; | |
| padding: 0.5rem 2rem; | |
| font-size: 1rem; | |
| cursor: pointer; | |
| &:hover { | |
| filter: brightness(85%); | |
| } | |
| } | |
| .more-info--webhook { | |
| background-color: var(--mint-green); | |
| outline: none; | |
| border: none; | |
| color: var(--cool-grey); | |
| font-weight: 600; | |
| border-radius: 60rem; | |
| padding: 0.5rem 2rem; | |
| font-size: 1rem; | |
| cursor: pointer; | |
| transition: filter 0.2s ease; | |
| &:hover { | |
| filter: brightness(85%); | |
| } | |
| &:focus-visible { | |
| outline: 2px solid var(--cool-grey); | |
| outline-offset: 2px; | |
| } | |
| &:active { | |
| filter: brightness(75%); | |
| } | |
| } |
main.go
Outdated
| e.POST("/webhooks", routes.Webhooks(baseConfig)) | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Additional security measures needed for webhook endpoint
The codebase review confirms that while webhook functionality is implemented, it lacks essential security measures. The current implementation:
- Has no rate limiting middleware
- No CORS protection
- No request signature validation
- No IP allowlisting
Specific areas requiring attention:
main.go: Add rate limiting middleware before the webhook routeroutes/routes_webhooks.go: Implement webhook signature validationconfig/config.go: Add configuration options for webhook security settings (allowed IPs, secrets)
🔗 Analysis chain
Consider additional security measures for webhook endpoint
As webhook endpoints are exposed to external systems, consider implementing:
- Rate limiting to prevent DoS attacks
- CORS policy if needed for cross-origin requests
- Request signature validation or webhook secrets
- IP allowlisting for trusted sources
Let's check if any rate limiting or CORS middleware is already configured:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for rate limiting or CORS middleware configuration
rg -A 5 'middleware\.(CORS|RateLimit)'
# Check if there are any existing webhook security patterns
ast-grep --pattern 'func Webhooks($_) {
$$$
}'
Length of output: 91
Script:
#!/bin/bash
# Let's try different patterns to find security-related middleware and webhook handler
rg -i "(cors|rate.?limit|security|webhook)" -A 5
# Search for the webhook handler implementation
fd -e go -x cat {} | grep -A 10 "func.*Webhook"
# Look for middleware setup in main.go
cat main.go
Length of output: 18263
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (15)
frontend/src/css/vars.css (1)
2-6: LGTM! Consider documenting colour usage.The new colour variables are well-organised and follow consistent British English naming conventions. However, it would be beneficial to document the intended usage of each colour to maintain design consistency across the application.
Add comments to describe the intended usage of each colour variable:
/* colours */ - --light-grey: #f7f9f8; - --cool-grey: #333460; - --mint-green: #1ed2bb; - --yellow: #e7e65f; + /* Background colour for light surfaces */ + --light-grey: #f7f9f8; + /* Primary text and interactive elements */ + --cool-grey: #333460; + /* Accent colour for highlights and CTAs */ + --mint-green: #1ed2bb; + /* Warning and notification states */ + --yellow: #e7e65f;common/common.go (2)
19-21: Consider a more robust secret initialisation approach.The current implementation has several potential issues:
- Secret regenerates on every process restart, which might break existing sessions
- No visible error handling from
GenerateSharedSecret- Init functions can make testing more challenging
Consider implementing a more robust secret management system:
- Load from secure environment variables or a secret manager
- Implement proper error handling
- Add logging for secret rotation events
- Consider using a dedicated initialisation function called from main
Would you like me to provide a detailed implementation example?
SharedSecret implementation follows secure practices but needs minor improvements
The verification reveals a generally secure implementation with a few areas for enhancement:
- The secret generation uses cryptographically secure random numbers (
crypto/rand) and appropriate key length (256-bit)- Signature validation employs constant-time comparison via
hmac.Equal- However, there are two areas that need attention:
- Error handling in
IsValidSignaturesilently ignores hex decoding errors with_- The secret generation failure is handled with
log.Fatalf, which could expose the secret value in logsRecommendations:
- Add proper error handling in
IsValidSignatureto return false on invalid hex input- Replace
log.Fatalfwith a more secure error handling that doesn't risk exposing the secret🔗 Analysis chain
Line range hint
9-21: Verify the usage and security of SharedSecret across the codebase.Let's analyse how this secret is used and ensure proper security practices.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyse SharedSecret usage and security implications # Check for direct SharedSecret references echo "Checking for SharedSecret usage:" rg "SharedSecret" --type go -C 3 # Check for potential security risks echo -e "\nChecking for potential logging of the secret:" rg "log|fmt.Print|println" --type go -C 3 | rg -i "secret" # Check for proper error handling around secret usage echo -e "\nChecking error handling patterns:" ast-grep --pattern 'if err := $$$; err != nil { $$$ }' --type goLength of output: 2430
Script:
#!/bin/bash # Let's verify the security aspects more thoroughly # Check the implementation of GenerateSharedSecret and related functions echo "Checking secret generation and signature calculation:" rg -t go "func.*Shared.*|func.*Secret.*|func.*Signature.*" -C 5 # Check for any potential exposure of the secret in responses or logs echo -e "\nChecking for potential secret exposure:" rg -t go "json:|fmt\.|log\." -C 3 | rg -i "secret|signature" # Check webhook signature validation implementation echo -e "\nChecking webhook signature validation:" ast-grep --pattern 'func IsValidSignature($_, $_) bool { $$$ }'Length of output: 3877
immich/immich_faces.go (1)
Line range hint
28-33: Consider enhancing error logging for better debuggingThe current error logging could be more specific to help identify the source of failures. Consider adding more context to the error messages and utilising structured logging fields effectively.
Here's a suggested improvement:
body, err := immichApiCall("GET", apiUrl.String(), nil) if err != nil { _, err = immichApiFail(faces, err, body, apiUrl.String()) - log.Error("adding faces", "err", err) + log.Error("failed to fetch faces from API", + "err", err, + "asset_id", i.ID, + "endpoint", apiUrl.String()) return } err = json.Unmarshal(body, &faces) if err != nil { _, err = immichApiFail(faces, err, body, apiUrl.String()) - log.Error("adding faces", "err", err) + log.Error("failed to unmarshal faces response", + "err", err, + "asset_id", i.ID, + "response_size", len(body)) return }Also applies to: 35-40
frontend/src/ts/kiosk.ts (2)
160-181: Consider extracting the KeyI handler logicThe KeyI case handling is quite complex. Consider extracting it into a separate function for better readability and maintainability.
+ function handleInfoKeyPress(): void { + const isPollingPaused = document.body.classList.contains("polling-paused"); + const hasMoreInfo = document.body.classList.contains("more-info"); + + if (isPollingPaused && hasMoreInfo) { + togglePolling(); + toggleImageOverlay(); + } else { + if (!isPollingPaused) { + togglePolling(); + } + toggleImageOverlay(); + } + } switch (e.code) { case "Space": e.preventDefault(); togglePolling(true); break; case "KeyI": { e.preventDefault(); - const isPollingPaused = document.body.classList.contains("polling-paused"); - const hasMoreInfo = document.body.classList.contains("more-info"); - - if (isPollingPaused && hasMoreInfo) { - togglePolling(); - toggleImageOverlay(); - } else { - if (!isPollingPaused) { - togglePolling(); - } - toggleImageOverlay(); - } + handleInfoKeyPress(); break; } }
189-191: Maintain consistency in DOM element selectionFor consistency with other DOM element selections in this file, consider using
htmx.find()instead ofquerySelector.- document - .querySelector(".navigation--more-info") - ?.addEventListener("click", () => toggleImageOverlay()); + htmx + .find(".navigation--more-info") + ?.addEventListener("click", () => toggleImageOverlay());views/views_image.templ (1)
48-50: Add documentation for the webhookSignature functionConsider adding documentation to explain:
- The purpose of the function
- The format of the signature
- The security implications
+// webhookSignature generates a signature for webhook authentication. +// It combines the shared secret with the current Unix timestamp +// to create a time-based signature for securing webhook requests. +// +// Returns: +// - A string containing the calculated signature func webhookSignature() string { return utils.CalculateSignature(common.SharedSecret, strconv.FormatInt(time.Now().Unix(), 10)) }utils/utils.go (3)
46-50: Enhance type documentation with usage examplesThe
WeightedAssetandAssetWithWeightingtypes would benefit from more detailed documentation explaining their purpose and providing usage examples.Add documentation like this:
-// WeightedAsset represents an asset with a type and ID +// WeightedAsset represents an asset with a type and ID. +// It is used in weighted random selection algorithms. +// +// Example: +// asset := WeightedAsset{ +// Type: "image", +// ID: "123", +// } type WeightedAsset struct { Type string ID string } -// AssetWithWeighting represents a WeightedAsset with an associated weight value +// AssetWithWeighting represents a WeightedAsset with an associated weight value. +// The weight value influences the probability of the asset being selected. +// +// Example: +// weighted := AssetWithWeighting{ +// Asset: WeightedAsset{Type: "image", ID: "123"}, +// Weight: 10, +// } type AssetWithWeighting struct { Asset WeightedAsset Weight int }Also applies to: 51-55
Line range hint
339-396: Add validation for empty or whitespace-only inputThe
parseTimeStringfunction should validate for empty or whitespace-only input before processing.Consider this improvement:
func parseTimeString(timeStr string) (time.Time, error) { + // Trim whitespace and validate + timeStr = strings.TrimSpace(timeStr) + if timeStr == "" { + return time.Time{}, fmt.Errorf("invalid time format: empty or whitespace-only input") + } + // Extract only the digits digits := regexp.MustCompile(`\d`).FindAllString(timeStr, -1)
433-436: Enhance FileExists with additional checksThe
FileExistsfunction could be more robust by checking if the path is a directory and handling symlinks.Consider this enhancement:
func FileExists(filename string) bool { - _, err := os.Stat(filename) - return !os.IsNotExist(err) + info, err := os.Stat(filename) + if err != nil { + return false + } + return !info.IsDir() }routes/routes_webhooks.go (3)
80-80: Use consistent variable naming conventionsThe variable
ViewDatashould start with a lowercase letter to follow Go's naming conventions for local variables.Apply this change:
- ViewData := views.ViewData{ + viewData := views.ViewData{And update all references to
ViewDataaccordingly.
109-109: Consider handling errors fromwebhooks.TriggerLaunching
webhooks.Triggerin a background goroutine without handling potential errors or applying context cancellation may lead to unexpected behaviour.Consider modifying the code to handle errors and context:
- go webhooks.Trigger(requestData, KioskVersion, webhooks.UserWebhookTriggerInfoOverlay, ViewData) + go func() { + if err := webhooks.Trigger(requestData, KioskVersion, webhooks.UserWebhookTriggerInfoOverlay, viewData); err != nil { + log.Error("Failed to trigger webhook", "error", err) + } + }()
73-73: IncluderequestIDin error logs for better traceabilityIncluding
requestIDin error logs can improve debugging and traceability of issues.Apply this change:
- log.Error("webhook request missing history") + log.Error(requestID, "message", "webhook request missing history")frontend/public/assets/css/kiosk.css (2)
1307-1311: Verify colour contrast for hover statesWhen hovering over
.navigation--item, the background changes tovar(--mint-green)and the SVG fill changes tovar(--cool-grey). Please confirm that the contrast between the icon and background meets accessibility standards to maintain readability.
1345-1348: Ensure icon size is adequate for usabilityThe SVG icons for
.navigation--more-infoare set to a size of1.5rem. Please check that this size is sufficient for users to easily interact with, especially on devices with higher resolutions or for users with visual impairments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
.coderabbitignore(0 hunks)common/common.go(2 hunks)frontend/public/assets/css/kiosk.css(9 hunks)frontend/src/css/vars.css(1 hunks)frontend/src/ts/kiosk.ts(2 hunks)immich/immich.go(1 hunks)immich/immich_faces.go(1 hunks)immich/immich_helpers.go(4 hunks)main.go(2 hunks)routes/routes_webhooks.go(1 hunks)utils/utils.go(7 hunks)views/views_image.templ(2 hunks)views/views_image_templ.go(18 hunks)
💤 Files with no reviewable changes (1)
- .coderabbitignore
🚧 Files skipped from review as they are similar to previous changes (2)
- immich/immich.go
- immich/immich_helpers.go
🔇 Additional comments (15)
common/common.go (1)
4-7: LGTM! Import changes are appropriate.
The addition of the utils package import is necessary for accessing the GenerateSharedSecret function.
immich/immich_faces.go (1)
22-22: LGTM: API endpoint path update is consistent
The removal of the trailing slash aligns with modern API practices and maintains consistency with other endpoint changes in the codebase.
Let's verify the consistency of API endpoint paths across the codebase:
main.go (1)
28-28:
Critical: Enhance webhook endpoint security
Whilst the addition of rate limiting is a positive step, the webhook endpoint requires additional security measures:
-
The current rate limit of 20 requests/second appears excessive for a kiosk application. Consider reducing this to a more conservative value, such as 1-2 requests/second.
-
Several critical security measures are missing:
- CORS protection
- IP allowlisting for trusted sources
- Request signature validation
- Request body size limits
Consider implementing these security enhancements:
// Middleware
e.Use(middleware.Recover())
e.Use(middleware.RequestID())
+e.Use(middleware.CORSWithConfig(middleware.CORSConfig{
+ AllowOrigins: []string{"trusted-origin.com"},
+ AllowMethods: []string{http.MethodPost},
+}))
+
+// IP allowlist middleware
+e.Use(middleware.IPExtractor(func(c echo.Context) string {
+ return c.RealIP()
+}))
+e.Use(middleware.IPFilter(middleware.IPFilterConfig{
+ Skipper: middleware.DefaultSkipper,
+ Allow: []string{"127.0.0.1/32", "trusted-ip-range"},
+}))
+
+// Request body size limit
+e.Use(middleware.BodyLimit("10K"))
// ... existing middleware ...
-e.POST("/webhooks", routes.Webhooks(baseConfig), middleware.RateLimiter(middleware.NewRateLimiterMemoryStore(rate.Limit(20))))
+e.POST("/webhooks", routes.Webhooks(baseConfig),
+ middleware.RateLimiter(middleware.NewRateLimiterMemoryStore(rate.Limit(2))),
+ validateWebhookSignature(baseConfig),
+)Let's verify the current security measures:
Also applies to: 129-130
frontend/src/ts/kiosk.ts (2)
18-18: LGTM: Clean import addition
The new import is properly organised with related menu functionality imports.
155-156: LGTM: Event listener implementation fixed
The event listeners now properly invoke the togglePolling function, addressing the previous issue where the function reference wasn't being executed.
views/views_image.templ (4)
28-28: LGTM: Proper placement of the more info component
The addition of renderMoreInfo follows the existing pattern and is logically placed after the history component.
81-83: Improve link accessibility
The current link text "The link" is not descriptive enough for screen readers.
98-100: Add alt text and error handling for QR code
The QR code image lacks alt text and error handling for the QR code generation.
86-96: Review webhook trigger security implementation
The webhook trigger implementation requires security review:
- Consider rate limiting beyond throttling
- Verify the timestamp validation on the server side
- Add loading state to prevent double submissions
✅ Verification successful
Let me check the webhook implementation further to verify the security concerns.
Webhook security implementation is adequate
The implementation has proper security measures in place:
- Rate limiting is implemented using Echo's middleware with a limit of 20 requests
- Webhook signatures are generated using a shared secret
- Request throttling is implemented on the client side (2s)
The current implementation provides sufficient protection against abuse through:
- Server-side rate limiting
- Signature verification
- Client-side throttling
- Request ID tracking for audit purposes
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check webhook implementation security
# Check for timestamp validation in webhook handlers
rg -A 5 "X-Timestamp" --type go
# Check for rate limiting implementation
rg -A 5 "throttle|rate.?limit" --type go
Length of output: 12021
Script:
#!/bin/bash
# Check webhook handler implementation for security measures
rg -A 10 "func.*webhook" --type go
# Check for rate limiting middleware or similar security measures
rg -A 10 "middleware|limiter" --type go
Length of output: 2805
utils/utils.go (1)
439-465: LGTM! Well-implemented QR code generation
The function has proper input validation, error handling, and uses an appropriate QR code size.
routes/routes_webhooks.go (1)
93-95: Handle errors returned by image.AssetInfo
The call to image.AssetInfo(requestID) may return an error, which is not currently being checked or handled.
frontend/public/assets/css/kiosk.css (4)
375-377: Verify readability with reduced opacity
Reducing the opacity of .image--metadata to 0.2 may make the text difficult to read. Please ensure that the metadata remains legible to maintain good user experience and accessibility.
568-593: Duplicate display: flex declarations and background colour contrast issues remain
The previously mentioned issues regarding duplicate display: flex declarations and the potential low contrast of the background colour rgb(51 52 96 / 60%) are still present in this code segment. Please refer to the earlier review comment for details.
757-759: Ensure clock and weather information remain legible at reduced opacity
Setting the opacity of #clock-weather-container to 0.2 might result in the clock and weather elements being hard to read. Verify that these elements remain clear and accessible to users under this styling.
1256-1256: Check the effect of z-index: 99997 on stacking order
Adding a high z-index to #navigation-interaction-area could affect the stacking order of overlapping elements. Ensure that it does not unintentionally overlay other important UI components.
| "github.com/damongolding/immich-kiosk/utils" | ||
| ) | ||
|
|
||
| var SharedSecret string |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add documentation for the exported SharedSecret variable.
As per Go conventions, exported variables should be documented. Additionally, consider whether this secret truly needs to be globally accessible.
Add documentation and consider encapsulation:
+// SharedSecret is a cryptographic secret used for secure communications.
+// Warning: This is a global mutable state. Access should be controlled carefully.
var SharedSecret stringConsider wrapping this in a thread-safe accessor pattern:
type secretManager struct {
secret string
mu sync.RWMutex
}
func (sm *secretManager) GetSecret() string {
sm.mu.RLock()
defer sm.mu.RUnlock()
return sm.secret
}| func immichImageUrl(baseUrl, imageID string) string { | ||
|
|
||
| u, err := url.Parse(baseUrl) | ||
| if err != nil { | ||
| return "#" | ||
| } | ||
|
|
||
| apiUrl := url.URL{ | ||
| Scheme: u.Scheme, | ||
| Host: u.Host, | ||
| Path: path.Join("photos", imageID), | ||
| } | ||
|
|
||
| return apiUrl.String() | ||
|
|
||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in immichImageUrl function
The current implementation silently fails by returning "#" when URL parsing fails. Consider:
- Validating the input parameters
- Logging errors for debugging
- Using a more meaningful fallback URL
func immichImageUrl(baseUrl, imageID string) string {
+ if baseUrl == "" || imageID == "" {
+ log.Printf("Invalid parameters: baseUrl=%q, imageID=%q", baseUrl, imageID)
+ return "/static/error.jpg"
+ }
u, err := url.Parse(baseUrl)
if err != nil {
- return "#"
+ log.Printf("Failed to parse base URL %q: %v", baseUrl, err)
+ return "/static/error.jpg"
}
apiUrl := url.URL{
Scheme: u.Scheme,
Host: u.Host,
Path: path.Join("photos", imageID),
}
return apiUrl.String()
}Committable suggestion skipped: line range outside the PR's diff.
utils/utils.go
Outdated
| // generateSharedSecret generates a random 256-bit (32-byte) secret. | ||
| func GenerateSharedSecret() string { | ||
| secret := make([]byte, 32) | ||
| _, err := crand.Read(secret) | ||
| if err != nil { | ||
| log.Fatalf("Failed to generate secret: %v", err) | ||
| } | ||
| return hex.EncodeToString(secret) | ||
| } |
There was a problem hiding this comment.
Critical: Improve error handling in GenerateSharedSecret
Using log.Fatalf in a library function is problematic as it forces the entire application to terminate. Instead, return the error to allow the caller to handle it appropriately.
Apply this fix:
-func GenerateSharedSecret() string {
+func GenerateSharedSecret() (string, error) {
secret := make([]byte, 32)
_, err := crand.Read(secret)
if err != nil {
- log.Fatalf("Failed to generate secret: %v", err)
+ return "", fmt.Errorf("failed to generate secret: %w", err)
}
- return hex.EncodeToString(secret)
+ return hex.EncodeToString(secret), nil
}📝 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.
| // generateSharedSecret generates a random 256-bit (32-byte) secret. | |
| func GenerateSharedSecret() string { | |
| secret := make([]byte, 32) | |
| _, err := crand.Read(secret) | |
| if err != nil { | |
| log.Fatalf("Failed to generate secret: %v", err) | |
| } | |
| return hex.EncodeToString(secret) | |
| } | |
| // generateSharedSecret generates a random 256-bit (32-byte) secret. | |
| func GenerateSharedSecret() (string, error) { | |
| secret := make([]byte, 32) | |
| _, err := crand.Read(secret) | |
| if err != nil { | |
| return "", fmt.Errorf("failed to generate secret: %w", err) | |
| } | |
| return hex.EncodeToString(secret), nil | |
| } |
views/views_image_templ.go
Outdated
| func webhookSignature() string { | ||
| return utils.CalculateSignature(common.SharedSecret, strconv.FormatInt(time.Now().Unix(), 10)) | ||
| } |
There was a problem hiding this comment.
Enhance webhook security measures
The webhook implementation requires additional security controls:
- The signature generation should include more entropy than just the timestamp
- The webhook endpoint needs rate limiting
- The included data from
.kiosk-history--entryshould be validated - CSRF protection should be implemented
Apply this diff to improve signature generation:
func webhookSignature() string {
- return utils.CalculateSignature(common.SharedSecret, strconv.FormatInt(time.Now().Unix(), 10))
+ nonce := utils.GenerateRandomString(16)
+ timestamp := strconv.FormatInt(time.Now().Unix(), 10)
+ return utils.CalculateSignature(common.SharedSecret, timestamp + "." + nonce)
}Additionally:
- Implement rate limiting middleware for the webhook endpoint
- Add request validation for the included data
- Add CSRF protection using a middleware like
gorilla/csrf
Also applies to: 155-160
views/views_image_templ.go
Outdated
| func immichImageUrl(baseUrl, imageID string) string { | ||
|
|
||
| u, err := url.Parse(baseUrl) | ||
| if err != nil { | ||
| return "#" | ||
| } | ||
|
|
||
| apiUrl := url.URL{ | ||
| Scheme: u.Scheme, | ||
| Host: u.Host, | ||
| Path: path.Join("photos", imageID), | ||
| } | ||
|
|
||
| return apiUrl.String() | ||
|
|
||
| } |
There was a problem hiding this comment.
Add URL encoding for the imageID parameter
The imageID parameter should be URL-encoded to safely handle special characters in the ID.
Apply this diff to add URL encoding:
func immichImageUrl(baseUrl, imageID string) string {
u, err := url.Parse(baseUrl)
if err != nil {
return "#"
}
+ encodedID := url.PathEscape(imageID)
apiUrl := url.URL{
Scheme: u.Scheme,
Host: u.Host,
- Path: path.Join("photos", imageID),
+ Path: path.Join("photos", encodedID),
}
return apiUrl.String()
}📝 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.
| func immichImageUrl(baseUrl, imageID string) string { | |
| u, err := url.Parse(baseUrl) | |
| if err != nil { | |
| return "#" | |
| } | |
| apiUrl := url.URL{ | |
| Scheme: u.Scheme, | |
| Host: u.Host, | |
| Path: path.Join("photos", imageID), | |
| } | |
| return apiUrl.String() | |
| } | |
| func immichImageUrl(baseUrl, imageID string) string { | |
| u, err := url.Parse(baseUrl) | |
| if err != nil { | |
| return "#" | |
| } | |
| encodedID := url.PathEscape(imageID) | |
| apiUrl := url.URL{ | |
| Scheme: u.Scheme, | |
| Host: u.Host, | |
| Path: path.Join("photos", encodedID), | |
| } | |
| return apiUrl.String() | |
| } |
views/views_image_templ.go
Outdated
| var templ_7745c5c3_Var7 string | ||
| templ_7745c5c3_Var7, templ_7745c5c3_Err = templ.JoinStringErrs(utils.CreateQrCode(immichImageUrl(viewData.ImmichUrl, img.ImmichImage.ID))) | ||
| if templ_7745c5c3_Err != nil { |
There was a problem hiding this comment.
Add error handling for QR code generation
The utils.CreateQrCode function may return an error that's currently not being handled.
Apply this diff to add error handling:
-var templ_7745c5c3_Var7 string
-templ_7745c5c3_Var7, templ_7745c5c3_Err = templ.JoinStringErrs(utils.CreateQrCode(immichImageUrl(viewData.ImmichUrl, img.ImmichImage.ID)))
+qrCode, err := utils.CreateQrCode(immichImageUrl(viewData.ImmichUrl, img.ImmichImage.ID))
+if err != nil {
+ return templ.Error{Err: err, FileName: `views/views_image.templ`, Line: 173, Col: 84}
+}
+templ_7745c5c3_Var7, templ_7745c5c3_Err = templ.JoinStringErrs(qrCode)📝 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.
| var templ_7745c5c3_Var7 string | |
| templ_7745c5c3_Var7, templ_7745c5c3_Err = templ.JoinStringErrs(utils.CreateQrCode(immichImageUrl(viewData.ImmichUrl, img.ImmichImage.ID))) | |
| if templ_7745c5c3_Err != nil { | |
| qrCode, err := utils.CreateQrCode(immichImageUrl(viewData.ImmichUrl, img.ImmichImage.ID)) | |
| if err != nil { | |
| return templ.Error{Err: err, FileName: `views/views_image.templ`, Line: 173, Col: 84} | |
| } | |
| templ_7745c5c3_Var7, templ_7745c5c3_Err = templ.JoinStringErrs(qrCode) | |
| if templ_7745c5c3_Err != nil { |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (5)
frontend/src/ts/kiosk.ts (1)
148-167: Consider refactoring for better readability and efficiencyThe function logic can be simplified using early returns and consolidated state changes.
Consider this refactoring:
function handleInfoKeyPress(): void { const isPollingPaused = document.body.classList.contains("polling-paused"); const hasMoreInfo = document.body.classList.contains("more-info"); - if (isPollingPaused && hasMoreInfo) { - togglePolling(); - toggleImageOverlay(); - } else { - if (!isPollingPaused) { - togglePolling(); - } - toggleImageOverlay(); - } + // If showing more info, hide it and resume polling + if (isPollingPaused && hasMoreInfo) { + togglePolling(); + toggleImageOverlay(); + return; + } + + // Ensure polling is paused before showing more info + if (!isPollingPaused) { + togglePolling(); + } + toggleImageOverlay(); }views/views_image.templ (1)
54-56: Consider using millisecond precision for webhook timestampThe current implementation uses Unix seconds. For better precision in webhook signature verification, consider using milliseconds.
func webhookSignature() string { - return utils.CalculateSignature(common.SharedSecret, strconv.FormatInt(time.Now().Unix(), 10)) + return utils.CalculateSignature(common.SharedSecret, strconv.FormatInt(time.Now().UnixMilli(), 10)) }utils/utils.go (3)
46-55: Enhance type documentation with usage examplesThe
WeightedAssetandAssetWithWeightingtypes would benefit from more detailed documentation, including usage examples and field descriptions.Add comprehensive documentation like this:
-// WeightedAsset represents an asset with a type and ID +// WeightedAsset represents an asset with a type identifier and unique ID. +// It is used in weighted random selection algorithms where different types +// of assets need to be selected with varying probabilities. +// +// Example usage: +// asset := WeightedAsset{ +// Type: "image", +// ID: "abc123", +// } type WeightedAsset struct { Type string ID string }
Line range hint
80-92: Add input validation and error handlingThe function should validate input and handle edge cases gracefully.
Consider this improved implementation:
-func DateToJavascriptLayout(input string) string { +func DateToJavascriptLayout(input string) (string, error) { + if input == "" { + return "", fmt.Errorf("empty input string") + } + replacer := strings.NewReplacer( "YYYY", "yyyy", "YY", "yy", "DDDD", "EEEE", "DDD", "EEE", "DD", "dd", "D", "d", ) - return replacer.Replace(input) + result := replacer.Replace(input) + if result == input { + return "", fmt.Errorf("no valid date format tokens found in input") + } + return result, nil }
440-445: Consider additional error casesThe function could be more robust by distinguishing between different types of errors.
Consider this enhanced implementation:
-func FileExists(filename string) bool { +func FileExists(filename string) (bool, error) { + if filename == "" { + return false, fmt.Errorf("empty filename") + } info, err := os.Stat(filename) - return !os.IsNotExist(err) + if err != nil { + if os.IsNotExist(err) { + return false, nil + } + return false, fmt.Errorf("error checking file: %w", err) + } + return !info.IsDir(), nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
common/common.go(2 hunks)frontend/src/ts/kiosk.ts(4 hunks)routes/routes_webhooks.go(1 hunks)utils/utils.go(7 hunks)views/views_image.templ(2 hunks)views/views_image_templ.go(18 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- routes/routes_webhooks.go
🧰 Additional context used
📓 Path-based instructions (1)
views/views_image_templ.go (1)
Pattern views/*_templ.go: ignore these generated templ files
🔇 Additional comments (11)
common/common.go (2)
10-10: Previous review comment about documenting SharedSecret still applies
4-8: Verify the security of secret generation
Let's verify the implementation of GenerateSharedSecret to ensure it uses cryptographically secure random number generation.
Also applies to: 20-26
✅ Verification successful
Secret generation implementation is cryptographically secure
The implementation of GenerateSharedSecret in utils/utils.go uses crypto/rand package and Read function to generate random bytes, which is the recommended approach for cryptographically secure random number generation in Go. The function:
- Allocates 32 bytes of memory
- Uses
crypto/rand.Readto fill it with cryptographically secure random data - Properly handles errors
- Returns the hex-encoded string representation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the implementation of GenerateSharedSecret
# Look for the implementation
ast-grep --pattern 'func GenerateSharedSecret() (string, error) {
$$$
}'
# Check for secure random number generation usage
rg -A 5 'crypto/rand|rand\.Read'
Length of output: 917
frontend/src/ts/kiosk.ts (2)
18-18: LGTM: Import and DOM element setup look good!
The toggleImageOverlay import and moreInfoButton constant follow the established patterns in the codebase.
Also applies to: 80-82
200-201: LGTM: More info button handler is properly implemented
The click event listener follows the established patterns in the codebase.
views/views_image.templ (1)
31-46:
Enhance error handling and input validation
The current implementation has several potential issues:
- No validation of input parameters
- Silent failure with an invalid fallback URL
- No error logging for debugging
Apply this improvement:
-func immichImageUrl(baseUrl, imageID string) string {
+func immichImageUrl(baseUrl, imageID string) string {
+ if baseUrl == "" || imageID == "" {
+ log.Printf("immichImageUrl: Invalid parameters - baseUrl: %q, imageID: %q", baseUrl, imageID)
+ return "/static/placeholder-image.jpg"
+ }
+
u, err := url.Parse(baseUrl)
if err != nil {
- return "#"
+ log.Printf("immichImageUrl: Failed to parse base URL %q: %v", baseUrl, err)
+ return "/static/placeholder-image.jpg"
}
apiUrl := url.URL{
Scheme: u.Scheme,
Host: u.Host,
Path: path.Join("photos", imageID),
}
return apiUrl.String()
}Likely invalid or redundant comment.
utils/utils.go (3)
339-347: LGTM! Well-structured error handling
The input validation and error handling are thorough and well-implemented.
446-472: LGTM! Well-implemented QR code generation
The implementation follows best practices with proper input validation, error handling, and optimal QR code size.
498-506: Consider timezone handling in timestamp validation
The timestamp validation should consider timezone differences.
Let's verify the timestamp handling:
Consider this improvement:
func IsValidTimestamp(receivedTimestamp string, toleranceSeconds int) bool {
ts, err := strconv.ParseInt(receivedTimestamp, 10, 64)
if err != nil {
return false
}
- currentTime := time.Now().Unix()
+ currentTime := time.Now().UTC().Unix()
return abs(currentTime-ts) <= int64(toleranceSeconds)
}views/views_image_templ.go (3)
16-20: Imports look good!
The new imports are appropriate for the added URL construction and webhook functionality.
65-68: LGTM: Clean integration of the new renderMoreInfo component
The addition follows the established pattern of component rendering in the file.
220-222: Good improvement: More descriptive variable names
The variable renaming enhances code readability while maintaining functionality.
Also applies to: 253-255, 288-290
views/views_image.templ
Outdated
| <a href={ templ.SafeURL(immichImageUrl(viewData.ImmichUrl, img.ImmichImage.ID)) } target="_blank"> | ||
| The link | ||
| </a> |
There was a problem hiding this comment.
Use descriptive link text for better accessibility
The current generic link text "The link" doesn't provide context about its purpose.
<a href={ templ.SafeURL(immichImageUrl(viewData.ImmichUrl, img.ImmichImage.ID)) } target="_blank">
- The link
+ View original image
</a>📝 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.
| <a href={ templ.SafeURL(immichImageUrl(viewData.ImmichUrl, img.ImmichImage.ID)) } target="_blank"> | |
| The link | |
| </a> | |
| <a href={ templ.SafeURL(immichImageUrl(viewData.ImmichUrl, img.ImmichImage.ID)) } target="_blank"> | |
| View original image | |
| </a> |
views/views_image.templ
Outdated
| <button | ||
| class="more-info--webhook" | ||
| hx-post="/webhooks" | ||
| hx-trigger="click throttle:2s" | ||
| hx-include=".kiosk-history--entry" | ||
| hx-headers={ fmt.Sprintf(`{"X-Timestamp": "%d", "X-Signature": "%s", "kiosk-webhook-event": "user.webhook.trigger.info_overlay"}`, time.Now().Unix(), webhookSignature()) } | ||
| hx-swap="textContent" | ||
| > | ||
| Trigger webhook | ||
| </button> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance webhook button UX and reliability
The current implementation could benefit from several improvements:
- The throttle time of 2s might be too short for webhook processing
- Missing loading state during webhook execution
- No feedback on success/failure
<button
class="more-info--webhook"
hx-post="/webhooks"
- hx-trigger="click throttle:2s"
+ hx-trigger="click throttle:5s"
hx-include=".kiosk-history--entry"
hx-headers={ fmt.Sprintf(`{"X-Timestamp": "%d", "X-Signature": "%s", "kiosk-webhook-event": "user.webhook.trigger.info_overlay"}`, time.Now().Unix(), webhookSignature()) }
hx-swap="textContent"
+ hx-indicator="#webhook-spinner"
>
Trigger webhook
+ <span id="webhook-spinner" class="htmx-indicator">
+ Processing...
+ </span>
</button>📝 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.
| <button | |
| class="more-info--webhook" | |
| hx-post="/webhooks" | |
| hx-trigger="click throttle:2s" | |
| hx-include=".kiosk-history--entry" | |
| hx-headers={ fmt.Sprintf(`{"X-Timestamp": "%d", "X-Signature": "%s", "kiosk-webhook-event": "user.webhook.trigger.info_overlay"}`, time.Now().Unix(), webhookSignature()) } | |
| hx-swap="textContent" | |
| > | |
| Trigger webhook | |
| </button> | |
| </div> | |
| <button | |
| class="more-info--webhook" | |
| hx-post="/webhooks" | |
| hx-trigger="click throttle:5s" | |
| hx-include=".kiosk-history--entry" | |
| hx-headers={ fmt.Sprintf(`{"X-Timestamp": "%d", "X-Signature": "%s", "kiosk-webhook-event": "user.webhook.trigger.info_overlay"}`, time.Now().Unix(), webhookSignature()) } | |
| hx-swap="textContent" | |
| hx-indicator="#webhook-spinner" | |
| > | |
| Trigger webhook | |
| <span id="webhook-spinner" class="htmx-indicator"> | |
| Processing... | |
| </span> | |
| </button> | |
| </div> |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
frontend/src/css/image-more-info.css (2)
1-6: Consider using a class selector instead of IDUsing ID selectors (#more-info) creates tight coupling and makes the styles harder to override. Consider using a class-based approach for better maintainability.
-#more-info { +.more-info { display: none; * { user-select: text; } }
8-65: Consider improvements to overlay implementationA few suggestions to enhance the implementation:
- The z-index value (99998) seems arbitrarily high. Consider using a lower value or CSS custom properties for z-index management.
- The RGB colour with opacity could use modern CSS colour syntax.
- Consider extracting magic numbers (padding values) into CSS custom properties.
.polling-paused.more-info { .frame--image img { - filter: grayscale(1) blur(4px) brightness(0.4); + filter: grayscale(100%) blur(4px) brightness(0.4); } #more-info { /* ... */ - z-index: 99998; + z-index: var(--z-overlay, 100); .more-info--image { - background-color: rgb(51 52 96 / 60%); + background-color: oklch(30% 0.1 270 / 0.6); - padding: 6rem 3rem 3rem 3rem; + padding: var(--spacing-xl) var(--spacing-lg) var(--spacing-lg) var(--spacing-lg);views/views_image.templ (1)
51-59: Consider adding error handling for edge casesThe webhook signature generation should handle cases where SharedSecret is empty or invalid.
func webhookSignature() string { + if common.SharedSecret == "" { + log.Printf("Warning: SharedSecret is empty") + return "" + } return utils.CalculateSignature(common.SharedSecret, strconv.FormatInt(time.Now().Unix(), 10)) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
custom.example.css(5 hunks)frontend/public/assets/css/kiosk.css(9 hunks)frontend/src/css/image-more-info.css(1 hunks)go.mod(1 hunks)immich/immich.go(3 hunks)immich/immich_helpers.go(4 hunks)views/views_image.templ(2 hunks)views/views_image_templ.go(18 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- go.mod
- immich/immich.go
- immich/immich_helpers.go
🧰 Additional context used
📓 Path-based instructions (2)
custom.example.css (1)
Pattern custom.example.css: ignore this file
views/views_image_templ.go (1)
Pattern views/*_templ.go: ignore these generated templ files
🪛 Biome (1.9.4)
custom.example.css
[error] 43-43: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 66-66: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 67-67: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 68-68: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 69-69: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 70-70: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 71-71: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 72-72: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 73-73: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 74-74: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 75-75: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 76-76: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 77-77: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 78-78: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 79-79: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 96-96: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 156-156: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 165-165: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
🔇 Additional comments (11)
frontend/src/css/image-more-info.css (1)
78-83: LGTM! Clean implementation of button group styles
The flex container implementation for button grouping is well-structured and follows best practices.
custom.example.css (1)
Line range hint 1-999: Skipping review of this file
This file matches the ignore pattern custom.example.css specified in the coding guidelines. Additionally, the file owner has explicitly requested to add this file to the ignore list in past review comments.
🧰 Tools
🪛 Biome (1.9.4)
[error] 62-62: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 63-63: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 64-64: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 66-66: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 67-67: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 68-68: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 69-69: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 70-70: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 71-71: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 72-72: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 73-73: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 74-74: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 75-75: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 76-76: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 77-77: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 78-78: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 79-79: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 81-81: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 82-82: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
views/views_image.templ (3)
4-15: LGTM: Clean integration of new imports and template modification
The new imports are properly organised and the renderMoreInfo integration follows the existing template pattern.
Also applies to: 31-32
34-49: Previous review comments about error handling remain valid
89-136: Improve heading hierarchy in more info section
The camera make and model information uses h3 headings at the same level as the filename, which might not represent the correct content hierarchy.
- <h3>
+ <h4>
{ img.ImmichImage.ExifInfo.Make }
- </h3>
+ </h4>frontend/public/assets/css/kiosk.css (3)
3-5: LGTM: Well-structured colour variables
The colour variables are semantically named and follow consistent conventions.
375-377: LGTM: Appropriate visual hierarchy
Reducing metadata opacity when showing more info helps direct user attention to the overlay content.
1270-1270: LGTM: Consistent navigation styling
The navigation updates maintain visual consistency while improving interactive feedback through hover states and appropriate icon sizing.
Also applies to: 1287-1287, 1321-1325, 1337-1338, 1359-1362
views/views_image_templ.go (3)
76-91: Existing issue: URL encoding needed
This issue was previously identified. Please refer to the existing comment about adding URL encoding for the imageID parameter.
93-101: Existing issue: Enhance webhook security
This issue was previously identified. Please refer to the existing comment about improving the webhook signature generation with additional entropy.
131-286: Existing issue: Add error handling for QR code generation
This issue was previously identified. Please refer to the existing comment about adding error handling for the QR code generation.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
frontend/src/css/image-more-info.css (2)
1-6: Good accessibility consideration with text selection enabled!The base setup looks good. Consider adding
aria-hidden="true"to the HTML element when it's not visible to improve screen reader compatibility.
8-65: Consider standardising units and reviewing z-index valueThe styles are well-structured, but there are some consistency improvements to consider:
- Mixed usage of rem and px units (e.g., gap: 0.4rem vs padding: 6rem)
- Very high z-index (99998) might be unnecessary
Consider standardising to rem units and using a lower z-index value:
#more-info { - gap: 0.4rem; - padding: 0.4rem; + gap: 0.25rem; + padding: 0.25rem; - z-index: 99998; + z-index: 100; }views/views_image.templ (2)
51-59: Consider enhancing webhook security measuresThe current implementation could benefit from additional security measures:
- Add signature expiry validation to prevent replay attacks
- Implement rate limiting for signature generation
Consider these enhancements:
func webhookSignature() string { + // Add expiry time (e.g., 5 minutes from now) + expiryTime := time.Now().Add(5 * time.Minute).Unix() + payload := fmt.Sprintf("%d:%d", time.Now().Unix(), expiryTime) - return utils.CalculateSignature(common.SharedSecret, strconv.FormatInt(time.Now().Unix(), 10)) + return utils.CalculateSignature(common.SharedSecret, payload) }
89-136: Consider breaking down the large template into smaller componentsThe current implementation could benefit from being split into smaller, reusable components:
- Camera information component
- File details component
- Actions component (link and webhook button)
Example refactor for the camera information section:
+templ renderCameraInfo(image immich.ImmichAsset) { + <div> + <div> + <h3>{ image.ExifInfo.Make }</h3> + </div> + <div> + { image.ExifInfo.Model } + </div> + </div> +} templ renderMoreInfo(viewData ViewData) { <div id="more-info" hx-swap-oob="true"> for _, img := range viewData.Images { <div class="more-info--image"> <div class="more-info--image--data"> - <div> - <div> - <h3> - { img.ImmichImage.ExifInfo.Make } - </h3> - </div> - <div> - { img.ImmichImage.ExifInfo.Model } - </div> - </div> + @renderCameraInfo(img.ImmichImage)README.md (1)
684-689: Capitalise the 'I' key in keyboard shortcuts table for consistencyFor consistency with other key names in the table (Spacebar, Right Arrow, Left Arrow), the 'i' key should be capitalised.
| Key | Action | |---------------|----------------------------------------------------------| | _ Spacebar | Play/Pause and Toggle Menu | | → Right Arrow | Next Image(s) | | ← Left Arrow | Previous Image(s) | -| i Key | Play/Pause and Toggle Menu and display more info overlay | +| I Key | Play/Pause and Toggle Menu and display more info overlay |🧰 Tools
🪛 LanguageTool
[uncategorized] ~689-~689: Did you mean “I”?
Context: ... | | i Key | Play/Pause and Toggle Men...(I_LOWERCASE_PREMIUM)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
README.md(2 hunks)custom.example.css(5 hunks)frontend/public/assets/css/kiosk.css(9 hunks)frontend/src/css/image-more-info.css(1 hunks)views/views_image.templ(2 hunks)views/views_image_templ.go(18 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
custom.example.css (1)
Pattern custom.example.css: ignore this file
frontend/public/assets/css/kiosk.css (1)
Pattern frontend/public/**: exclude
views/views_image_templ.go (1)
Pattern views/*_templ.go: exclude
🪛 LanguageTool
README.md
[uncategorized] ~689-~689: Did you mean “I”?
Context: ... | | i Key | Play/Pause and Toggle Men...
(I_LOWERCASE_PREMIUM)
🪛 Biome (1.9.4)
custom.example.css
[error] 43-43: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 66-66: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 67-67: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 68-68: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 69-69: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 70-70: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 71-71: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 72-72: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 73-73: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 74-74: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 75-75: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 76-76: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 77-77: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 78-78: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 79-79: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 96-96: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 156-156: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 165-165: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
🔇 Additional comments (11)
frontend/src/css/image-more-info.css (1)
67-75: Clean landscape layout adaptation!
The landscape mode styles are well-implemented with appropriate flex direction changes.
custom.example.css (1)
Line range hint 1-165: Skipping review of this file
As per the coding guidelines, files matching the pattern custom.example.css should be ignored. Additionally, past review comments confirm this file should be added to the ignore list.
🧰 Tools
🪛 Biome (1.9.4)
[error] 62-62: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 63-63: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 64-64: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 66-66: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 67-67: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 68-68: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 69-69: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 70-70: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 71-71: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 72-72: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 73-73: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 74-74: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 75-75: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 76-76: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 77-77: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 78-78: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 79-79: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 81-81: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 82-82: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
views/views_image.templ (1)
61-87: Well-implemented with proper security measures
The function demonstrates good practices:
- Proper XSS protection using
templ.EscapeString - Appropriate handling of empty cases
- Clean separation of concerns
frontend/public/assets/css/kiosk.css (5)
3-6: LGTM! Good use of CSS variables for theming.
The addition of color variables improves maintainability and consistency across the application.
375-377: LGTM! Consistent opacity handling.
The opacity reduction for image metadata when showing more info creates a clear visual hierarchy.
774-776: LGTM! Consistent opacity treatment.
The opacity reduction for the clock-weather container aligns with the overall overlay design pattern.
1273-1273: LGTM! Well-structured navigation styling.
The z-index changes ensure proper layering of UI elements, and the hover states provide good visual feedback. The icon sizing is consistent with the UI scale.
Also applies to: 1290-1290, 1324-1328, 1340-1341, 1362-1365
558-604: 🛠️ Refactor suggestion
Consider improving overlay contrast and removing duplicate flex declarations.
The semi-transparent background (rgb(51 52 96 / 60%)) might not provide sufficient contrast for text content. Additionally, there are duplicate flex declarations that can be simplified.
Apply this diff to improve accessibility and remove duplicates:
.polling-paused.more-info #more-info .more-info--image {
width: 100%;
height: 100%;
- background-color: rgb(51 52 96 / 60%);
+ background-color: rgb(51 52 96 / 85%);
display: -webkit-flex;
display: -moz-box;
display: flex;
-webkit-flex-direction: column;
-moz-box-orient: vertical;
-moz-box-direction: normal;
flex-direction: column;
-webkit-justify-content: space-between;
-moz-box-pack: justify;
justify-content: space-between;
gap: 2rem;
padding: 6rem 3rem 3rem 3rem;
color: #fff;
+ text-shadow: 0 1px 2px rgba(0, 0, 0, 0.5);
border-radius: 0.74rem;
overflow: hidden;
}Likely invalid or redundant comment.
views/views_image_templ.go (2)
12-23: LGTM! Clean integration of new functionality.
The new imports are appropriate for the added features, and the integration of renderMoreInfo in the Image function follows the existing patterns.
Also applies to: 68-71
311-313: LGTM! Consistent variable naming improvements.
The variable naming changes for child components improve code readability while maintaining the existing patterns.
Also applies to: 344-346, 379-381, 475-477, 519-521, 572-574, 655-657, 702-704, 749-751, 796-798
README.md (1)
Line range hint 1-983: Documentation is comprehensive and well-structured
The README provides clear, detailed instructions and is well-organised with appropriate sections, examples, and visual aids. The installation steps, configuration options, and usage guidelines are thoroughly documented.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
.github/workflows/go-test.yml (1)
25-26: Enhance error handling and cachingThe template generation steps could benefit from improved error handling and caching.
+ - name: Cache templ binary + uses: actions/cache@v3 + with: + path: ~/go/bin/templ + key: ${{ runner.os }}-templ-${{ hashFiles('go.sum') }} + - name: Run tests env: CI: true run: | - go install github.com/a-h/templ/cmd/templ@latest - templ generate + if ! command -v templ &> /dev/null; then + echo "Installing templ..." + go install github.com/a-h/templ/cmd/[email protected] + fi + echo "Generating templates..." + templ generate || { echo "Template generation failed"; exit 1; } mkdir -p test-results go test -v ./... > test-results/test.log
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
.github/workflows/go-test.yml(1 hunks).gitignore(1 hunks)views/views_clock_templ.go(0 hunks)views/views_error_templ.go(0 hunks)views/views_home_templ.go(0 hunks)views/views_image-frame_templ.go(0 hunks)views/views_image-metadata_templ.go(0 hunks)views/views_image_templ.go(0 hunks)views/views_menu_templ.go(0 hunks)views/views_weather_templ.go(0 hunks)
💤 Files with no reviewable changes (8)
- views/views_clock_templ.go
- views/views_error_templ.go
- views/views_home_templ.go
- views/views_image-frame_templ.go
- views/views_image-metadata_templ.go
- views/views_image_templ.go
- views/views_menu_templ.go
- views/views_weather_templ.go
✅ Files skipped from review due to trivial changes (1)
- .gitignore
.github/workflows/go-test.yml
Outdated
| @@ -22,6 +22,8 @@ jobs: | |||
| env: | |||
| CI: true | |||
| run: | | |||
| go install github.com/a-h/templ/cmd/templ@latest | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider pinning the templ version for CI stability
Using @latest in CI environments can lead to non-deterministic builds if breaking changes are introduced. Consider pinning to a specific version.
-go install github.com/a-h/templ/cmd/templ@latest
+go install github.com/a-h/templ/cmd/[email protected]📝 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 install github.com/a-h/templ/cmd/templ@latest | |
| go install github.com/a-h/templ/cmd/templ@v0.2.513 |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.github/workflows/go-test.yml(1 hunks)Dockerfile(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/go-test.yml
🔇 Additional comments (2)
Dockerfile (2)
16-16: LGTM: Proper build configuration
The build command correctly utilises build arguments and includes appropriate flags for a production build.
1-1:
Incorrect Go version specified
The base image golang:1.23.3-alpine references a non-existent version of Go. The latest stable version is 1.22.0 as of February 2024.
Apply this correction:
-FROM --platform=$BUILDPLATFORM golang:1.23.3-alpine AS build
+FROM --platform=$BUILDPLATFORM golang:1.22.0-alpine AS buildLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
frontend/src/css/more-info-overlay.css (2)
1-6: Consider accessibility and text selection improvementsA few suggestions to enhance the base styles:
- Instead of
display: none, consider using thehiddenattribute orvisibility: hiddenfor better accessibility.- Rather than enabling text selection for all children, consider being more specific about which elements should be selectable.
#more-info { - display: none; + visibility: hidden; * { - user-select: text; + user-select: none; } + p, h3, .selectable { + user-select: text; + } }
77-82: Consider responsive paddingThe button group layout is good, but consider using relative units or clamp() for the padding to ensure it scales well across different viewport sizes.
.more-info--button-group { display: flex; gap: 1rem; flex-wrap: wrap; - padding-top: 1rem; + padding-top: clamp(0.5rem, 2vw, 1rem); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
.dockerignore(1 hunks)custom.example.css(5 hunks)frontend/public/assets/css/kiosk.css(9 hunks)frontend/src/css/kiosk.css(1 hunks)frontend/src/css/more-info-overlay.css(1 hunks)taskfile.yml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- .dockerignore
- taskfile.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/css/kiosk.css
🧰 Additional context used
📓 Path-based instructions (2)
custom.example.css (1)
Pattern custom.example.css: ignore this file
frontend/public/assets/css/kiosk.css (1)
Pattern frontend/public/**: exclude
🪛 Biome (1.9.4)
custom.example.css
[error] 43-43: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 66-66: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 67-67: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 68-68: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 69-69: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 70-70: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 71-71: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 72-72: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 73-73: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 74-74: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 75-75: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 76-76: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 77-77: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 78-78: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 79-79: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 96-96: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 156-156: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 165-165: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
🔇 Additional comments (4)
frontend/src/css/more-info-overlay.css (1)
67-75: LGTM! Clean and effective layout adaptation
The landscape layout implementation is well-structured and follows flexbox best practices.
custom.example.css (1)
Line range hint 1-200: Skipping review of this file
As per the coding guidelines and your explicit request in the past review comments, this file will be ignored.
🧰 Tools
🪛 Biome (1.9.4)
[error] 62-62: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 63-63: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 64-64: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 66-66: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 67-67: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 68-68: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 69-69: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 70-70: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 71-71: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 72-72: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 73-73: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 74-74: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 75-75: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 76-76: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 77-77: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 78-78: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 79-79: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 81-81: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
[error] 82-82: An empty block isn't allowed.
Consider removing the empty block or adding styles inside it.
(lint/suspicious/noEmptyBlock)
frontend/public/assets/css/kiosk.css (2)
3-5: LGTM! Well-structured colour variables.
The addition of semantic colour variables improves maintainability and ensures consistency across the UI.
1272-1272: LGTM! Improved navigation styling and interactions.
The changes enhance the navigation component with:
- Consistent z-index layering
- Better hover states with colour feedback
- Standardised icon sizing
- Improved spacing
Also applies to: 1289-1289, 1323-1327, 1339-1340, 1361-1364
add contributing docs
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
taskfile.frontend.yml (1)
1-28: Consider adding watch tasks for developmentThe taskfile could benefit from watch tasks for development purposes, allowing for automatic rebuilding of CSS and JS during development.
Consider adding these tasks:
+ frontend-dev: + dir: ./frontend + cmds: + - pnpm dev + + frontend-watch-css: + dir: ./frontend + cmds: + - pnpm watch:css + + frontend-watch-js: + dir: ./frontend + cmds: + - pnpm watch:jstaskfile.docker.yml (2)
5-8: Consider optimising the Docker build configurationThe
--no-cacheflag forces rebuilding all layers every time, which could significantly slow down your build process. Consider:
- Removing
--no-cacheto utilise layer caching- Making the repository name configurable via variables
- Adding
.dockerignoreverificationdocker-image: deps: [build] cmds: - - docker build --no-cache --build-arg VERSION={{.VERSION}} --load -t damongolding/immich-kiosk:{{.VERSION}} -t damongolding/immich-kiosk:latest . + - docker build --build-arg VERSION={{.VERSION}} --load -t {{.DOCKER_REPO}}/immich-kiosk:{{.VERSION}} -t {{.DOCKER_REPO}}/immich-kiosk:latest .
20-23: Review development image strategyThe current development image strategy could be improved:
- Consider using build stages for development
- Implement proper versioning for development tags
- Add development-specific health checks
docker-dev-push: deps: [build] cmds: - - docker build --build-arg VERSION={{.VERSION}}-DEVELOPMENT --platform linux/amd64,linux/arm64 --push -t damongolding/immich-kiosk-development:{{.VERSION}} -t damongolding/immich-kiosk-development:latest . + - docker build --target development --build-arg VERSION={{.VERSION}}-dev --platform linux/amd64,linux/arm64 --push -t {{.DOCKER_REPO}}/immich-kiosk:{{.VERSION}}-dev -t {{.DOCKER_REPO}}/immich-kiosk:dev .taskfile.yml (1)
5-12: Good modularisation of taskfilesThe separation of frontend and Docker tasks into their own taskfiles with
flatten: trueimproves maintainability and organisation. This is a positive architectural change.Consider documenting the purpose of each taskfile in the repository's README to help other contributors understand the new structure.
README.md (1)
684-689: Consider capitalising the 'i' key reference for clarity.The keyboard shortcuts table is well-structured, but the lowercase 'i' in "i Key" might be less clear than "I Key" for users.
| _ Spacebar | Play/Pause and Toggle Menu | | → Right Arrow | Next Image(s) | | ← Left Arrow | Previous Image(s) | -| i Key | Play/Pause and Toggle Menu and display more info overlay | +| I Key | Play/Pause and Toggle Menu and display more info overlay |🧰 Tools
🪛 LanguageTool
[uncategorized] ~689-~689: Did you mean “I”?
Context: ... | | i Key | Play/Pause and Toggle Men...(I_LOWERCASE_PREMIUM)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
README.md(3 hunks)go.mod(1 hunks)taskfile.docker.yml(1 hunks)taskfile.frontend.yml(1 hunks)taskfile.yml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~689-~689: Did you mean “I”?
Context: ... | | i Key | Play/Pause and Toggle Men...
(I_LOWERCASE_PREMIUM)
🔇 Additional comments (7)
taskfile.frontend.yml (2)
10-14: 🛠️ Refactor suggestion
Consider adding frontend-build as a dependency
The frontend-build task exists but isn't included in the main frontend task dependencies. This might lead to running tests against unbundled code.
Apply this diff to include the build step:
frontend:
- deps: [frontend-test, frontend-css, frontend-js]
+ deps: [frontend-build, frontend-test, frontend-css, frontend-js]
dir: ./frontend
cmds:
- pnpm lintLikely invalid or redundant comment.
15-28: Verify the existence of required npm scripts
The tasks reference several pnpm scripts (test, css, js) that must exist in package.json. Let's ensure these are properly defined.
✅ Verification successful
Let me gather more specific information about the package.json scripts.
All required npm scripts are properly defined
The taskfile's referenced scripts (test, css, js) are all properly defined in the frontend's package.json. Each script has a valid implementation:
test: Runs TypeScript type checkingcss: Bundles CSS with esbuild and processes with PostCSSjs: Bundles TypeScript with esbuild
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all referenced npm scripts exist in package.json
# Check if all required scripts exist
REQUIRED_SCRIPTS=("test" "css" "js" "lint" "build")
echo "Checking for required scripts in package.json:"
for script in "${REQUIRED_SCRIPTS[@]}"; do
echo "Checking for '$script' script..."
if ! cat ./frontend/package.json | jq -e ".scripts.$script" >/dev/null; then
echo "Warning: '$script' script not found"
fi
done
Length of output: 990
Script:
#!/bin/bash
# Let's see the actual content of the scripts section in package.json
cat ./frontend/package.json | jq '.scripts'
Length of output: 620
taskfile.docker.yml (2)
1-3: LGTM: Task file version and structure
The version "3" specification and overall structure follow Taskfile conventions correctly.
15-18: Enhance multi-platform build security and efficiency
Consider these improvements for the production image push:
- Add authentication verification
- Implement build caching for CI/CD efficiency
- Add platform-specific tests before pushing
✅ Verification successful
Let me check if there are any existing authentication mechanisms or CI/CD configurations in the repository.
Let me check the contents of the Docker-related CI/CD workflow to understand the existing build and push process.
Docker build configuration is already well-secured and optimised
The review comment's concerns are already addressed in the CI/CD pipeline:
- Authentication is properly implemented using Docker Hub secrets
- Multi-platform builds are handled via Docker Buildx
- The build and push process is automated and secured within GitHub Actions
The local taskfile.docker.yml is meant for development purposes, and it's appropriate to keep it simple as production builds are handled by the CI/CD pipeline.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify Docker Hub authentication status
docker info 2>&1 | grep -q "Username:"
if [ $? -ne 0 ]; then
echo "Warning: Not authenticated to Docker Hub"
fi
Length of output: 142
Script:
#!/bin/bash
# Check for CI/CD configurations
fd -e yml -e yaml . .github/workflows/
echo "=== Docker-related files ==="
fd -e dockerfile -e yml -e yaml . | grep -i docker
echo "=== Authentication patterns in CI/CD ==="
rg -i "docker.*login|docker.*auth" .
Length of output: 473
Script:
#!/bin/bash
echo "=== Docker Release Workflow ==="
cat .github/workflows/docker-release.yml
echo -e "\n=== Docker Compose Configuration ==="
cat docker-compose.yaml
Length of output: 2073
taskfile.yml (1)
3-3: Version bump looks appropriate
The increment from 0.14.0-beta.1 to 0.14.0-beta.2 follows semantic versioning conventions for pre-release versions.
README.md (2)
766-772: LGTM! Clear and well-structured webhooks events documentation.
The webhooks events table provides clear descriptions for each event type and maintains consistent formatting.
973-1030: Excellent addition of comprehensive contributing guidelines!
The new contributing section is well-structured and provides:
- Clear prerequisites and setup instructions
- Step-by-step contribution process
- Coding standards and best practices
- Helpful resources and support channels
taskfile.frontend.yml
Outdated
| frontend: | ||
| deps: [frontend-test, frontend-css, frontend-js] | ||
| dir: ./frontend | ||
| cmds: | ||
| - pnpm lint | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Linting should indeed run before tests in the build pipeline
The package.json scripts reveal that the linting is only checking a single file (./public/assets/js/kiosk.js), whilst the build process runs tests, CSS, and JS tasks. The current task order in taskfile.frontend.yml should be adjusted for two reasons:
- Linting should run before tests to catch syntax and style issues early
- The linting scope appears insufficient as it only checks the output JS file, not the source TypeScript files
Suggested changes:
- Move the lint task to run before the test dependency
- Expand the lint script in package.json to include TypeScript source files in
src/ts
🔗 Analysis chain
Verify the frontend task dependencies
The main frontend task runs the linter after running tests, CSS, and JS tasks. Consider whether the linting should run before the tests to catch basic issues earlier in the pipeline.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there's a separate lint configuration and verify the build order in package.json
# Check for lint configuration
fd -t f "^\.?lint" ./frontend
fd -t f "^\.?eslint" ./frontend
# Check package.json scripts
cat ./frontend/package.json | jq '.scripts'
Length of output: 715
taskfile.docker.yml
Outdated
| docker-buildx: | ||
| cmds: | ||
| - docker buildx create --name mybuilder --use | ||
| - docker buildx install |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for buildx setup
The buildx setup should handle existing builder instances and verify installation success.
docker-buildx:
cmds:
+ - docker buildx ls | grep -q mybuilder || \
- docker buildx create --name mybuilder --use
- docker buildx install
+ - docker buildx inspect mybuilder --bootstrap📝 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.
| docker-buildx: | |
| cmds: | |
| - docker buildx create --name mybuilder --use | |
| - docker buildx install | |
| docker-buildx: | |
| cmds: | |
| - docker buildx ls | grep -q mybuilder || \ | |
| - docker buildx create --name mybuilder --use | |
| - docker buildx install | |
| - docker buildx inspect mybuilder --bootstrap |
taskfile.yml
Outdated
| # Maintenance tasks | ||
| install: | ||
| deps: [update-templ] | ||
| cmds: | ||
| - docker build --build-arg VERSION={{.VERSION}}-DEVELOPMENT --platform linux/amd64,linux/arm64 --push -t damongolding/immich-kiosk-development:{{.VERSION}} -t damongolding/immich-kiosk-development:latest . | ||
| - go install github.com/golangci/golangci-lint/cmd/[email protected] | ||
| - go install github.com/psampaz/go-mod-outdated@latest | ||
| - go mod tidy |
There was a problem hiding this comment.
💡 Codebase verification
Pin go-mod-outdated to the latest stable version v0.9.0
The verification confirms that:
- golangci-lint is correctly pinned to the latest stable version v1.62.0
- go-mod-outdated has a stable release v0.9.0 available, which should be used instead of @latest
Suggested change:
- - go install github.com/psampaz/go-mod-outdated@latest
+ - go install github.com/psampaz/[email protected]🔗 Analysis chain
Verify Go package versions
The new install task includes specific versions of Go packages. Whilst the inclusion of golangci-lint at v1.62.0 is good, go-mod-outdated lacks version pinning.
Let's check if there are any known issues with these versions:
Consider pinning the version of go-mod-outdated to ensure consistent behaviour across different environments:
- - go install github.com/psampaz/go-mod-outdated@latest
+ - go install github.com/psampaz/[email protected]🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the specified Go package versions for security and compatibility
# Check latest golangci-lint version
curl -s https://api.github.com/repos/golangci/golangci-lint/releases/latest | jq -r '.tag_name'
# Check if go-mod-outdated has any releases that should be pinned
curl -s https://api.github.com/repos/psampaz/go-mod-outdated/releases | jq -r '.[0].tag_name'
Length of output: 206
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
frontend/src/ts/menu.ts (3)
11-11: Consider adding explicit type annotationWhilst TypeScript can infer the boolean type, adding an explicit type annotation would improve code clarity and maintainability.
-let imageOverlayVisible = false; +let imageOverlayVisible: boolean = false;
31-35: Add error handling and return type annotationConsider adding error handling and improving type safety:
-function showImageOverlay() { +function showImageOverlay(): void { + if (!document.body) return; if (!document.body.classList.contains("polling-paused")) return; document.body.classList.add("more-info"); imageOverlayVisible = true; }
37-44: Add return type annotations and error handling to remaining functionsThe hideImageOverlay and toggleImageOverlay functions would benefit from similar improvements:
-function hideImageOverlay() { +function hideImageOverlay(): void { + if (!document.body) return; document.body.classList.remove("more-info"); imageOverlayVisible = false; } -function toggleImageOverlay() { +function toggleImageOverlay(): void { imageOverlayVisible ? hideImageOverlay() : showImageOverlay(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
frontend/src/ts/menu.ts(3 hunks)views/views_image.templ(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- views/views_image.templ
🔇 Additional comments (2)
frontend/src/ts/menu.ts (2)
63-65: Export modifications look good
The new overlay control functions are appropriately exported for use in other modules.
31-44: Consider documenting the overlay-polling relationship
The overlay functionality is tightly coupled with the polling state through the "polling-paused" class check. Consider:
- Adding JSDoc comments to explain this relationship
- Verifying that all CSS classes referenced exist in the related CSS files
✅ Verification successful
The overlay-polling relationship is well-implemented and documented
The codebase shows clear implementation of the overlay-polling relationship:
- The
polling-pausedclass is used consistently across CSS files for styling overlays - The
more-infoclass is properly defined in multiple stylesheets with clear visual effects - The relationship between polling state and overlays is handled in both the menu and polling modules
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify CSS classes exist in stylesheets
rg -l "polling-paused|more-info" "frontend/src/css/"
Length of output: 216
Script:
#!/bin/bash
# Check the actual usage of these classes in CSS files
rg "\.polling-paused|\.more-info" "frontend/src/css/" -A 2
Length of output: 2355
Script:
#!/bin/bash
# Check for any existing documentation or related polling functionality
rg "polling.*pause|pause.*polling" -A 3
Length of output: 96977
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
taskfile.frontend.yml (1)
20-33: Consider adding status checks for asset tasksThe CSS and JS tasks might benefit from status checks to ensure they complete successfully. Additionally, consider adding descriptions to document each task's purpose.
Apply this diff to enhance task documentation and error handling:
frontend-css: dir: ./frontend + desc: Compile and minify CSS assets cmds: - pnpm css + - echo "CSS compilation completed" frontend-js: dir: ./frontend + desc: Bundle and transpile JavaScript assets cmds: - pnpm js + - echo "JS compilation completed"taskfile.yml (1)
5-12: Well-structured task modularisationGood architectural decision to split tasks into separate files. The
flatten: trueoption ensures a clean task namespace whilst maintaining modularity.Consider documenting the task structure in the repository's README to help other contributors understand the task organisation.
frontend/src/ts/menu.ts (1)
Line range hint
13-30: Consider enhancing error handlingWhile the type annotations are good additions, the error handling could be improved:
- Consider throwing an Error instead of logging to console
- Add error boundaries or fallback behaviour
function disableImageNavigationButtons(): void { if (!nextImageMenuButton || !prevImageMenuButton) { - console.error("Navigation buttons not initialized"); - return; + throw new Error("Navigation buttons not initialized"); } htmx.addClass(nextImageMenuButton, "disabled"); htmx.addClass(prevImageMenuButton, "disabled"); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
frontend/src/ts/menu.ts(4 hunks)taskfile.frontend.yml(1 hunks)taskfile.yml(2 hunks)
🔇 Additional comments (7)
taskfile.frontend.yml (2)
1-3: LGTM! Well-structured task file.
The file follows the standard Taskfile format with proper version specification.
4-9:
Linting should run before tests
The current task order runs linting after all other tasks, which could lead to running tests on code that doesn't meet the project's style guidelines. Consider moving the lint command to a separate task and making it a dependency of the test task.
Apply this diff to improve the task order:
frontend:
- deps: [frontend-test, frontend-css, frontend-js]
+ deps: [frontend-lint, frontend-test, frontend-css, frontend-js]
dir: ./frontend
+ cmds: []
+
+ frontend-lint:
+ dir: ./frontend
cmds:
- pnpm linttaskfile.yml (2)
3-3: LGTM: Version bump follows semantic versioning
The version increment from beta.1 to beta.2 is appropriate for a beta release cycle.
52-58: Well-structured install task with appropriate dependencies
The install task is well-organised with proper dependencies. However, the previously identified issue regarding version pinning for go-mod-outdated remains valid.
frontend/src/ts/menu.ts (3)
11-11: LGTM: Well-typed state variable
The boolean state variable is properly typed and initialised with a sensible default value.
67-69: LGTM: Clean export additions
The new overlay control functions are properly exported, maintaining the module's public API structure.
31-45: Verify integration with polling and keyboard events
Let's confirm that the overlay functions are properly integrated with the polling system and keyboard events.
✅ Verification successful
Let me check the actual integration points in these files to verify the proper usage.
Image overlay functions are properly integrated
The integration points show correct usage:
polling.tscorrectly imports and callshideImageOverlaywhen polling state changeskiosk.tsproperly integratestoggleImageOverlaywith click events and polling state transitions- The functions maintain synchronisation between polling state and overlay visibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check integration points in polling and keyboard event handlers
echo "Checking polling integration..."
rg -l "hideImageOverlay" --type ts
echo "\nChecking keyboard event handlers..."
rg -l "toggleImageOverlay.*keypress|keydown|keyup" --type ts
Length of output: 331
Script:
#!/bin/bash
# Check how hideImageOverlay is used in polling.ts
echo "Integration in polling.ts:"
rg "hideImageOverlay" frontend/src/ts/polling.ts -B 2 -A 2
echo "\nKeyboard event handling in kiosk.ts:"
rg "toggleImageOverlay" frontend/src/ts/kiosk.ts -B 2 -A 2
Length of output: 1089
frontend/src/ts/menu.ts
Outdated
| function showImageOverlay(): void { | ||
| if (!document.body) return; | ||
| if (!document.body.classList.contains("polling-paused")) return; | ||
| document.body.classList.add("more-info"); | ||
| imageOverlayVisible = true; | ||
| } | ||
|
|
||
| function hideImageOverlay(): void { | ||
| document.body.classList.remove("more-info"); | ||
| imageOverlayVisible = false; | ||
| } | ||
|
|
||
| function toggleImageOverlay(): void { | ||
| imageOverlayVisible ? hideImageOverlay() : showImageOverlay(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add consistent null checks across overlay functions
While showImageOverlay includes a null check for document.body, this check is missing in hideImageOverlay. Let's maintain consistency.
function hideImageOverlay(): void {
+ if (!document.body) return;
document.body.classList.remove("more-info");
imageOverlayVisible = false;
}📝 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.
| function showImageOverlay(): void { | |
| if (!document.body) return; | |
| if (!document.body.classList.contains("polling-paused")) return; | |
| document.body.classList.add("more-info"); | |
| imageOverlayVisible = true; | |
| } | |
| function hideImageOverlay(): void { | |
| document.body.classList.remove("more-info"); | |
| imageOverlayVisible = false; | |
| } | |
| function toggleImageOverlay(): void { | |
| imageOverlayVisible ? hideImageOverlay() : showImageOverlay(); | |
| } | |
| function showImageOverlay(): void { | |
| if (!document.body) return; | |
| if (!document.body.classList.contains("polling-paused")) return; | |
| document.body.classList.add("more-info"); | |
| imageOverlayVisible = true; | |
| } | |
| function hideImageOverlay(): void { | |
| if (!document.body) return; | |
| document.body.classList.remove("more-info"); | |
| imageOverlayVisible = false; | |
| } | |
| function toggleImageOverlay(): void { | |
| imageOverlayVisible ? hideImageOverlay() : showImageOverlay(); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
frontend/src/ts/menu.ts (2)
Line range hint
13-29: Consider adding type guards for better type safety.The error handling is good, but we could improve type safety by using type guards instead of type assertions.
Consider this refactoring:
function enableImageNavigationButtons(): void { if (!nextImageMenuButton || !prevImageMenuButton) { console.error("Navigation buttons not initialized"); return; } - htmx.removeClass(nextImageMenuButton as Element, "disabled"); - htmx.removeClass(prevImageMenuButton as Element, "disabled"); + if (nextImageMenuButton instanceof Element && prevImageMenuButton instanceof Element) { + htmx.removeClass(nextImageMenuButton, "disabled"); + htmx.removeClass(prevImageMenuButton, "disabled"); + } }
31-46: Consider adding debouncing for overlay toggles.The overlay functions are well-implemented, but rapid toggling could lead to visual glitches or performance issues.
Consider implementing debouncing:
import { debounce } from 'lodash'; const debouncedToggle = debounce(() => { imageOverlayVisible ? hideImageOverlay() : showImageOverlay(); }, 250); function toggleImageOverlay(): void { debouncedToggle(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
frontend/src/ts/menu.ts(4 hunks)taskfile.docker.yml(1 hunks)taskfile.frontend.yml(1 hunks)taskfile.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- taskfile.docker.yml
- taskfile.frontend.yml
🔇 Additional comments (8)
taskfile.yml (6)
3-12: LGTM! Well-structured task organisation
The version bump and new includes section provide a clean separation of concerns between main, frontend and Docker tasks.
17-26: LGTM! Clear task descriptions and appropriate debug configurations
The development tasks are well-documented with clear descriptions and proper debug flag settings.
30-41: LGTM! Well-defined backend tasks
Each task has a clear purpose and follows the single responsibility principle.
46-57: LGTM! Robust build configuration
The build tasks are well-configured with:
- Static builds (CGO_ENABLED=0)
- Proper version injection
- Appropriate dependencies
65-65: Pin go-mod-outdated version
The previous review comment about pinning go-mod-outdated to v0.9.0 is still valid.
59-77: LGTM! Well-structured maintenance tasks
The maintenance tasks provide good coverage for development setup and dependency management.
frontend/src/ts/menu.ts (2)
11-11: LGTM! Well-typed variable declaration.
The variable declaration follows TypeScript best practices with proper type annotation and initialisation.
68-70: Verify the usage patterns of newly exported functions.
Let's ensure these newly exported overlay functions are being used consistently across the application.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores