-
-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat:_ add settings to enable thumbnails on dashboard #227
Conversation
WalkthroughThis update enhances the BirdNET-Go dashboard by adding thumbnail and tooltip functionalities, as well as other structural and configuration adjustments. Key modifications include styling for thumbnails and tooltips, conditional cache initialization for bird images, new dashboard settings for thumbnails, and updates to HTML templates to reflect these settings. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Browser
participant Server
participant DataStore
User->>+Browser: Load Dashboard
Browser->>+Server: Request Dashboard Data
Server->>+DataStore: Fetch Dashboard Data
DataStore-->>-Server: Send Dashboard Data
opt Dashboard Thumbnails Enabled
Server->>+DataStore: Initialize Bird Image Cache
DataStore-->>-Server: Bird Image Cache
end
Server-->>-Browser: Render Data with Thumbnails
Browser-->>-User: Display Thumbnails on Dashboard
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
Outside diff range and nitpick comments (1)
internal/conf/config.yaml (1)
Line range hint
26-26
: Fix formatting issues in YAML configuration.There are several formatting issues detected by
yamllint
, including wrong indentation and trailing spaces. These should be corrected to maintain the cleanliness and readability of the YAML file.- rotationday: 0 # day of the week for weekly rotation, 0 = Sunday + rotationday: 0 # day of the week for weekly rotation, 0 = Sunday - realtime: - interval: 15 # duplicate prediction interval in seconds +realtime: + interval: 15 # duplicate prediction interval in seconds - thumbnails: - summary: false + thumbnails: + summary: false - enabled: false + enabled: falseAlso applies to: 33-33, 41-41, 44-44, 51-51, 58-58, 64-64, 89-89, 130-130
Tools
yamllint
[error] 44-44: trailing spaces (trailing-spaces)
[error] 51-51: trailing spaces (trailing-spaces)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- assets/custom.css (1 hunks)
- internal/analysis/realtime.go (1 hunks)
- internal/conf/config.go (3 hunks)
- internal/conf/config.yaml (1 hunks)
- internal/conf/defaults.go (1 hunks)
- internal/httpcontroller/handlers.go (3 hunks)
- internal/httpcontroller/init.go (2 hunks)
- internal/httpcontroller/utils.go (3 hunks)
- internal/imageprovider/imageprovider.go (2 hunks)
- internal/imageprovider/wikipedia.go (4 hunks)
- views/fragments/birdsTableHTML.html (2 hunks)
- views/fragments/recentDetections.html (4 hunks)
Files skipped from review due to trivial changes (2)
- assets/custom.css
- views/fragments/birdsTableHTML.html
Additional context used
yamllint
internal/conf/config.yaml
[warning] 26-26: wrong indentation: expected 4 but found 6 (indentation)
[error] 33-33: trailing spaces (trailing-spaces)
[error] 41-41: trailing spaces (trailing-spaces)
[error] 44-44: trailing spaces (trailing-spaces)
[error] 51-51: trailing spaces (trailing-spaces)
[error] 58-58: trailing spaces (trailing-spaces)
[error] 64-64: trailing spaces (trailing-spaces)
[error] 89-89: trailing spaces (trailing-spaces)
[error] 130-130: no new line character at the end of file (new-line-at-end-of-file)
Additional comments not posted (10)
views/fragments/recentDetections.html (2)
32-41
: Thumbnail conditional rendering logic appears correctly implemented.This segment properly integrates the thumbnail display within the conditional block which checks the
Thumbnails.Recent
setting. This is a good practice as it ensures that thumbnails are only rendered when enabled in the settings, potentially saving bandwidth and processing time.
Line range hint
68-91
: Responsive layout adaptation is well-handled.The responsive layout for smaller screens using
.sm:hidden
and conditional rendering for thumbnails is well implemented. This ensures a good user experience across different device sizes. However, ensure that the attributes and methods used in the template (like{{title .CommonName}}
) are supported and correctly implemented.internal/imageprovider/imageprovider.go (2)
16-20
: Field naming and struct organization is clear and consistent.The renaming of struct fields in
BirdImage
to capitalize the first letter is a necessary change for fields that need to be exported in Go. This change adheres to Go's convention for visibility, making these fields accessible outside the package.
115-117
: Efficient memory size estimation forBirdImage
.The method
EstimateSize
efficiently calculates the memory size of aBirdImage
instance, which is crucial for managing resources in image caching mechanisms. This implementation is a good practice for performance optimization.internal/httpcontroller/init.go (1)
Line range hint
31-49
: Proper initialization of server components with clear struct organization.The
Server
struct initialization in theNew
function is well-structured, ensuring that all necessary components likeDashboardSettings
andBirdImageCache
are properly initialized. This setup is crucial for the robustness and scalability of the server configuration.internal/conf/config.yaml (1)
47-51
: Configuration settings for thumbnails are correctly added.The addition of thumbnail settings under the
dashboard
key is correctly implemented. This allows easy on/off toggling of thumbnail features in different sections of the application, aligning with the PR's objectives.Tools
yamllint
[error] 51-51: trailing spaces (trailing-spaces)
internal/imageprovider/wikipedia.go (1)
21-23
: Good refactor to improve code readability and compliance with Go conventions.Changing the field names from lowercase to uppercase enhances visibility (exported vs. unexported) and follows Go naming conventions. Ensure that all references to these fields in the application are updated to reflect these changes.
Also applies to: 72-76
internal/conf/config.go (2)
20-25
: NewDashboard
struct for configuration settingsThe addition of the
Dashboard
struct withThumbnails
nested struct is well-defined and aligns with the PR's objective to manage thumbnail settings on the dashboard. The comments on each field provide clarity on their purpose.
77-77
: Integration ofDashboard
settings into theRealtime
structThe integration of the
Dashboard
struct into theRealtime
struct of theSettings
configuration is correctly implemented. This centralizes the management of dashboard-related configurations, facilitating easier maintenance and scalability.internal/httpcontroller/handlers.go (1)
155-163
: Proper integration ofDashboardSettings
in thetopBirdsHandler
The addition of
DashboardSettings
to the data struct intopBirdsHandler
is correctly implemented. This allows the handler to access dashboard configuration settings, which is necessary for conditional rendering based on the thumbnail settings. This change supports the PR's objective to enhance dashboard functionality.
viper.SetDefault("realtime.dashboard.thumbnails.summary", false) | ||
viper.SetDefault("realtime.dashboard.thumbnails.recent", true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
Ensure the new settings are integrated and used within the application.
The search results indicate that the new settings realtime.dashboard.thumbnails.summary
and realtime.dashboard.thumbnails.recent
are only defined in internal/conf/defaults.go
and are not referenced or used elsewhere in the codebase. This suggests that these settings might not be integrated into the application logic.
- Integration Required: Ensure that the new settings are referenced and utilized appropriately in the relevant parts of the application where dashboard thumbnails are handled.
Analysis chain
Ensure consistency in default settings across the application.
The new dashboard thumbnail settings (summary
and recent
) are added correctly. Verify that these settings are consistently handled across different parts of the application, especially in areas where these settings are checked or modified.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for usage of the new thumbnail settings across the application.
# Test: Search for the settings usage. Expect: Consistent handling across the application.
rg --type go "realtime.dashboard.thumbnails.(summary|recent)"
Length of output: 245
Script:
#!/bin/bash
# Description: Search for usage of the new thumbnail settings across the entire codebase.
# Test: Search for the settings usage in all files. Expect: Consistent handling across the application.
rg 'realtime.dashboard.thumbnails.(summary|recent)'
Length of output: 235
// ThumbnailAttribution returns the HTML-formatted attribution for a bird's thumbnail image. | ||
// It takes the bird's scientific name as input and returns a template.HTML string. | ||
// If the attribution information is incomplete or an error occurs, it returns an empty template.HTML. | ||
func (s *Server) thumbnailAttribution(scientificName string) template.HTML { | ||
if s.BirdImageCache == nil { | ||
// Return empty string if the cache is not initialized | ||
return template.HTML("") | ||
} | ||
|
||
birdImage, err := s.BirdImageCache.Get(scientificName) | ||
if err != nil { | ||
log.Printf("Error getting thumbnail info for %s: %v", scientificName, err) | ||
return template.HTML("") | ||
} | ||
|
||
// Skip if no author or license information | ||
if birdImage.AuthorName == "" || birdImage.LicenseName == "" { | ||
return template.HTML("") | ||
} | ||
|
||
var toReturn string | ||
if birdImage.AuthorUrl == "" { | ||
toReturn = fmt.Sprintf("© %s / <a href=%s>%s</a>", birdImage.AuthorName, birdImage.LicenseUrl, birdImage.LicenseName) | ||
var attribution string | ||
if birdImage.AuthorURL == "" { | ||
attribution = fmt.Sprintf("© %s / <a href=\"%s\">%s</a>", | ||
html.EscapeString(birdImage.AuthorName), | ||
html.EscapeString(birdImage.LicenseURL), | ||
html.EscapeString(birdImage.LicenseName)) | ||
} else { | ||
toReturn = fmt.Sprintf("© <a href=%s>%s</a> / <a href=%s>%s</a>", birdImage.AuthorUrl, birdImage.AuthorName, birdImage.LicenseUrl, birdImage.LicenseName) | ||
attribution = fmt.Sprintf("© <a href=\"%s\">%s</a> / <a href=\"%s\">%s</a>", | ||
html.EscapeString(birdImage.AuthorURL), | ||
html.EscapeString(birdImage.AuthorName), | ||
html.EscapeString(birdImage.LicenseURL), | ||
html.EscapeString(birdImage.LicenseName)) | ||
} | ||
|
||
return template.HTML(toReturn) | ||
return template.HTML(attribution) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refine HTML generation in the ThumbnailAttribution function.
The function handles various error conditions well. However, the HTML generation could be vulnerable to XSS if not properly sanitized. Ensure that html.EscapeString
is used consistently to prevent XSS vulnerabilities, especially in dynamically generated links.
- attribution = fmt.Sprintf("© %s / <a href=\"%s\">%s</a>", birdImage.AuthorName, birdImage.LicenseURL, birdImage.LicenseName)
+ attribution = fmt.Sprintf("© %s / <a href=\"%s\">%s</a>", html.EscapeString(birdImage.AuthorName), html.EscapeString(birdImage.LicenseURL), html.EscapeString(birdImage.LicenseName))
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.
// ThumbnailAttribution returns the HTML-formatted attribution for a bird's thumbnail image. | |
// It takes the bird's scientific name as input and returns a template.HTML string. | |
// If the attribution information is incomplete or an error occurs, it returns an empty template.HTML. | |
func (s *Server) thumbnailAttribution(scientificName string) template.HTML { | |
if s.BirdImageCache == nil { | |
// Return empty string if the cache is not initialized | |
return template.HTML("") | |
} | |
birdImage, err := s.BirdImageCache.Get(scientificName) | |
if err != nil { | |
log.Printf("Error getting thumbnail info for %s: %v", scientificName, err) | |
return template.HTML("") | |
} | |
// Skip if no author or license information | |
if birdImage.AuthorName == "" || birdImage.LicenseName == "" { | |
return template.HTML("") | |
} | |
var toReturn string | |
if birdImage.AuthorUrl == "" { | |
toReturn = fmt.Sprintf("© %s / <a href=%s>%s</a>", birdImage.AuthorName, birdImage.LicenseUrl, birdImage.LicenseName) | |
var attribution string | |
if birdImage.AuthorURL == "" { | |
attribution = fmt.Sprintf("© %s / <a href=\"%s\">%s</a>", | |
html.EscapeString(birdImage.AuthorName), | |
html.EscapeString(birdImage.LicenseURL), | |
html.EscapeString(birdImage.LicenseName)) | |
} else { | |
toReturn = fmt.Sprintf("© <a href=%s>%s</a> / <a href=%s>%s</a>", birdImage.AuthorUrl, birdImage.AuthorName, birdImage.LicenseUrl, birdImage.LicenseName) | |
attribution = fmt.Sprintf("© <a href=\"%s\">%s</a> / <a href=\"%s\">%s</a>", | |
html.EscapeString(birdImage.AuthorURL), | |
html.EscapeString(birdImage.AuthorName), | |
html.EscapeString(birdImage.LicenseURL), | |
html.EscapeString(birdImage.LicenseName)) | |
} | |
return template.HTML(toReturn) | |
return template.HTML(attribution) | |
var attribution string | |
if birdImage.AuthorURL == "" { | |
attribution = fmt.Sprintf("© %s / <a href=\"%s\">%s</a>", | |
html.EscapeString(birdImage.AuthorName), | |
html.EscapeString(birdImage.LicenseURL), | |
html.EscapeString(birdImage.LicenseName)) | |
} else { | |
attribution = fmt.Sprintf("© <a href=\"%s\">%s</a> / <a href=\"%s\">%s</a>", | |
html.EscapeString(birdImage.AuthorURL), | |
html.EscapeString(birdImage.AuthorName), | |
html.EscapeString(birdImage.LicenseURL), | |
html.EscapeString(birdImage.LicenseName)) | |
} |
// Thumbnail returns the URL of a given bird's thumbnail image. | ||
// It takes the bird's scientific name as input and returns the image URL as a string. | ||
// If the image is not found or an error occurs, it returns an empty string. | ||
func (s *Server) thumbnail(scientificName string) string { | ||
if s.BirdImageCache == nil { | ||
// Return empty string if the cache is not initialized | ||
return "" | ||
} | ||
|
||
birdImage, err := s.BirdImageCache.Get(scientificName) | ||
if err != nil { | ||
// Return empty string if an error occurs | ||
return "" | ||
} | ||
|
||
return birdImage.Url | ||
return birdImage.URL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve error handling and logging in thumbnail retrieval function.
The function correctly checks for an uninitialized cache and handles errors by returning an empty string, which is a safe failover. Consider adding a log statement for the error scenario to aid in debugging issues related to thumbnail retrieval.
+ log.Printf("Error retrieving thumbnail for %s: %v", scientificName, err)
Committable suggestion was skipped due to low confidence.
var birdImageCache *imageprovider.BirdImageCache | ||
if settings.Realtime.Dashboard.Thumbnails.Summary || settings.Realtime.Dashboard.Thumbnails.Recent { | ||
// Initialize the bird image cache | ||
birdImageCache = initBirdImageCache(dataStore, metrics) | ||
} else { | ||
birdImageCache = nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure robust initialization logic for birdImageCache.
The conditional initialization of birdImageCache
based on dashboard settings is implemented correctly. This is a good use of configuration to control resource utilization. However, consider adding error handling within initBirdImageCache
to ensure that issues during initialization are caught and logged properly.
+ if err != nil {
+ log.Printf("Failed to initialize bird image cache: %v", err)
+ return nil
+ }
Committable suggestion was skipped due to low confidence.
{{if .DashboardSettings.Thumbnails.Recent}} | ||
<th scope="col" class="py-2 px-6" style="width:20%;">Thumbnail</th> | ||
{{end}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tip
Codebase Verification
Ensure robust error handling for dynamic settings in template logic.
The review comment correctly highlights the need for error handling when rendering templates based on dynamic settings. The current implementation does not include nil checks for DashboardSettings
in the Go code or default handling in the template.
- Go Code:
internal/httpcontroller/handlers.go
does not have nil checks forDashboardSettings
before using it in the template. - Template:
views/fragments/recentDetections.html
directly accesses.DashboardSettings.Thumbnails.Recent
without any default handling.
To prevent potential runtime errors, consider adding nil checks in the Go code and default values or conditional checks in the template.
- Add nil checks for
DashboardSettings
in the Go code. - Implement default handling or conditional checks in the template.
Analysis chain
Ensure robust error handling for dynamic settings in template logic.
The conditional rendering based on .DashboardSettings.Thumbnails.Recent
is a good approach for dynamic UI changes. However, ensure that there's error handling or a default case in scenarios where .DashboardSettings
might be nil
or not properly initialized, to prevent runtime errors in the template rendering.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that there's a fallback or error handling for `.DashboardSettings` being nil.
ast-grep --lang go --pattern $'func $_($_, $_, $_) $_ {
if $_ == nil {
$$$
}
}'
Length of output: 88
Script:
#!/bin/bash
# Search for `.DashboardSettings` usage in the Go codebase to find any nil checks or error handling.
rg '\.DashboardSettings' --glob '*.go'
# Inspect the template file for any conditional checks or defaults related to `.DashboardSettings`.
rg '\.DashboardSettings' --glob 'views/fragments/recentDetections.html'
Length of output: 537
Script:
#!/bin/bash
# Inspect the `handlers.go` file for any nil checks or error handling for `DashboardSettings`.
rg 'DashboardSettings' internal/httpcontroller/handlers.go -A 10 -B 10
# Verify if the template file has any conditional checks or defaults for `DashboardSettings`.
rg 'DashboardSettings' views/fragments/recentDetections.html -A 5 -B 5
Length of output: 3206
No description provided.