Fix hour cell links & add sticky headers for Recent detections #318
Conversation
WalkthroughThis pull request encompasses a series of changes across multiple files, primarily focusing on enhancements to CSS styling, the introduction of new utility functions, and modifications to data handling methods. Key updates include the addition of new styles in Changes
Possibly related PRs
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: 5
🧹 Outside diff range and nitpick comments (14)
assets/util.js (2)
1-2: Consider using template literals for consistencyThe implementation is correct, but consider using template literals for string interpolation to maintain consistency with modern JavaScript practices.
- window.location.href = `/logout`; + window.location.href = `/logout`;
26-29: Add JSDoc and improve path handling robustnessWhile the function works correctly, it could benefit from documentation and more robust path handling.
+/** + * Checks if the current page is the dashboard + * @returns {boolean} True if current page is dashboard, false otherwise + */ function isLocationDashboard() { const pathname = window.location.pathname; - return pathname === '/' || pathname.endsWith('/dashboard'); + // Normalize path by removing trailing slash + const normalizedPath = pathname.replace(/\/$/, ''); + return normalizedPath === '' || normalizedPath.endsWith('/dashboard'); }views/index.html (1)
Line range hint
107-116: Consider optimizing the HTTPS redirect logic.While the implementation is correct, it could be more efficient and comprehensive.
Consider this optimization:
{{ if .Settings.Security.RedirectToHTTPS}} // Check for HTTPS redirect (function () { - if (window.location.protocol !== 'https:' && - window.location.hostname !== 'localhost' && - window.location.hostname !== '127.0.0.1') { + const hostname = window.location.hostname; + const isLocalhost = hostname === 'localhost' || + hostname === '127.0.0.1' || + hostname === '::1'; + if (window.location.protocol !== 'https:' && !isLocalhost) { window.location.href = 'https:' + window.location.href.substring(window.location.protocol.length); } })(); {{ end }}views/fragments/birdsTable.html (2)
51-58: Consider simplifying the count label positioning logic.The current implementation uses multiple conditions and duplicate count displays. Consider refactoring to use CSS positioning exclusively:
- {{if and (ge $width 45) (le $width 59)}} - <span class="text-2xs text-gray-100 dark:text-base-300 absolute right-1 top-1/2 transform -translate-y-1/2">{{.TotalDetections}}</span> - {{end}} - </div> - {{if or (lt $width 45) (gt $width 59)}} - <span class="text-2xs {{if gt $width 59}}text-gray-100 dark:text-base-300{{else}}text-gray-400 dark:text-base-400{{end}} absolute w-full text-center top-1/2 left-1/2 transform -translate-x-1/2 -translate-y-1/2">{{.TotalDetections}}</span> - {{end}} + </div> + <span class="text-2xs absolute top-1/2 transform -translate-y-1/2 {{if gt $width 59}}text-gray-100 dark:text-base-300 right-1{{else}}text-gray-400 dark:text-base-400 w-full text-center left-1/2 -translate-x-1/2{{end}}">{{.TotalDetections}}</span>
85-101: Consider enhancing accessibility for hour headers.While the implementation is good, consider adding accessibility improvements for the sunrise/sunset indicators:
- {{sunPositionIcon (timeOfDayToInt "dawn")}} + <span aria-label="Sunrise" title="Sunrise">{{sunPositionIcon (timeOfDayToInt "dawn")}}</span>- {{sunPositionIcon (timeOfDayToInt "dusk")}} + <span aria-label="Sunset" title="Sunset">{{sunPositionIcon (timeOfDayToInt "dusk")}}</span>internal/httpcontroller/template_functions.go (4)
61-71: Consider enhancing type handling and error reportingWhile the function now handles both integers and strings, there are opportunities for improvement:
- Silent error handling during string conversion could mask issues
- Other common numeric types (float64, int64) are not supported
Consider this enhanced implementation:
func addFunc(numbers ...interface{}) int { sum := 0 for _, num := range numbers { switch v := num.(type) { case int: sum += v + case int64: + sum += int(v) + case float64: + sum += int(v) case string: - if i, err := strconv.Atoi(v); err == nil { + i, err := strconv.Atoi(v) + if err != nil { + // Log error or handle invalid string + continue + } sum += i - } } } return sum }
Line range hint
196-211: Add input validation for sequence rangeWhile the documentation is good, the function should validate its inputs to prevent negative or invalid ranges.
Consider adding validation:
func seqFunc(start, end int) []int { + if end < start { + return []int{} // or handle error appropriately + } seq := make([]int, end-start+1) for i := range seq { seq[i] = start + i } return seq }
225-235: Add input validation for hour metadataThe function should validate its inputs to ensure:
- hourIndex is within 0-23
- length is one of the expected values (1, 2, or 6)
- sunrise and sunset are within valid ranges
Consider adding validation:
func getHourlyHeaderData(hourIndex int, class string, length int, date string, sunrise int, sunset int) map[string]interface{} { + if hourIndex < 0 || hourIndex > 23 { + return nil // or handle error appropriately + } + if length != 1 && length != 2 && length != 6 { + return nil // or handle error appropriately + } + if sunrise < 0 || sunrise > 23 || sunset < 0 || sunset > 23 { + return nil // or handle error appropriately + } baseData := map[string]interface{}{ "Class": class, "Length": length,
Line range hint
262-267: Add input validation for hourly counts rangeThe function should validate the length parameter to prevent excessive iterations.
Consider adding validation:
func sumHourlyCountsRange(counts [24]int, start, length int) int { + if length <= 0 || length > 24 { + return 0 // or handle error appropriately + } + if start < 0 || start > 23 { + return 0 // or handle error appropriately + } sum := 0 for i := start; i < start+length; i++ { sum += counts[i%24]assets/custom.css (3)
46-56: Consider adjusting gradient opacity for better accessibilityThe current gradient implementation might make text harder to read during scroll transitions. Consider adjusting the gradient opacity to ensure text remains readable throughout the scroll.
- background-image: linear-gradient(to bottom, white 70%, transparent 100%); + background-image: linear-gradient(to bottom, white 85%, rgba(255, 255, 255, 0.95) 95%, transparent 100%); - background-image: linear-gradient(to bottom, #1d232a 50%, transparent 100%); + background-image: linear-gradient(to bottom, #1d232a 85%, rgba(29, 35, 42, 0.95) 95%, transparent 100%);
72-96: Improve border handling for hour data cellsThe current border implementation might cause inconsistent rendering across browsers due to the combination of
border-collapse: collapseandposition: relative. Consider using a more robust approach.[data-theme=light] .hour-data:not(.heatmap-color-0) { position: relative; z-index: 1; padding: 0; - border: 1px solid rgba(255, 255, 255, 0.1); - background-clip: padding-box; - border-collapse: collapse; + box-shadow: inset 0 0 0 1px rgba(255, 255, 255, 0.1); }
246-336: Standardize gradient stops between themesThere's an inconsistency in gradient stops between light theme (45%) and dark theme (66%) that might cause visual discrepancies when switching themes.
Consider standardizing the gradient stops. Example for dark theme:
[data-theme=dark] .heatmap-color-4 { - background: linear-gradient(135deg, var(--heatmap-color-4) 66%, var(--heatmap-color-3) 110%); + background: linear-gradient(135deg, var(--heatmap-color-4) 45%, var(--heatmap-color-3) 95%); color: var(--heatmap-text-4, #000); }Apply similar changes to other dark theme gradient definitions for consistency.
internal/httpcontroller/handlers/detections.go (1)
61-61: Document Duration parameter behaviorThe Duration parameter is now used in multiple detection queries, but its default behavior when unspecified (0) is not documented. Please clarify:
- What time range is queried when Duration is 0?
- Are there any maximum limits for the Duration value?
Consider adding code comments to document these behaviors:
case "hourly": if req.Date == "" || req.Hour == "" { return h.NewHandlerError(fmt.Errorf("missing date or hour"), "Date and hour parameters are required for hourly detections", http.StatusBadRequest) } + // Duration of 0 means single hour, positive values extend the range forward by N hours notes, err = h.DS.GetHourlyDetections(req.Date, req.Hour, req.Duration)Also applies to: 67-68
internal/conf/utils.go (1)
397-398: Add documentation to explain security guaranteesThe function lacks documentation explaining its purpose, usage, and security guarantees. Consider adding a detailed comment block.
Add this documentation:
+// IsSafePath validates if a given path is safe for internal web redirects. +// It ensures the path: +// - Is relative (starts with "/") +// - Contains no directory traversal +// - Contains no URL schemes +// - Has a reasonable length +// Note: This function is specifically designed for web redirect paths and may not be +// suitable for general filesystem path validation. +// Returns: true if the path is considered safe, false otherwise. func IsSafePath(path string) bool {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
assets/custom.css(6 hunks)assets/tailwind.css(13 hunks)assets/util.js(2 hunks)internal/conf/utils.go(1 hunks)internal/datastore/interfaces.go(5 hunks)internal/httpcontroller/auth_routes.go(2 hunks)internal/httpcontroller/handlers/detections.go(4 hunks)internal/httpcontroller/routes.go(1 hunks)internal/httpcontroller/template_functions.go(5 hunks)views/dashboard.html(2 hunks)views/fragments/birdsTable.html(5 hunks)views/fragments/listDetections.html(2 hunks)views/index.html(4 hunks)
🔇 Additional comments (28)
assets/util.js (1)
26-29: Verify usage of isLocationDashboard function
Let's verify where this new function is being used to ensure it's properly integrated.
✅ Verification successful
Let me check the context of the usage to ensure it's properly integrated.
Function usage is properly integrated
The isLocationDashboard function is used correctly in a single location (views/index.html) to conditionally initialize a date picker when on the dashboard page. The implementation and usage are consistent with its intended purpose.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of isLocationDashboard function
rg "isLocationDashboard" --type js --type html
Length of output: 157
Script:
#!/bin/bash
# Get context of isLocationDashboard usage in views/index.html
rg "isLocationDashboard" views/index.html -B 2 -A 2
Length of output: 291
views/dashboard.html (3)
20-22:
Review date picker initialization behavior
The date picker has undergone significant changes:
- Removed 'load' from hx-trigger, which means it won't automatically fetch data on page load
- Added 'invisible' class while keeping the element functional
Please verify:
- How is the initial data loaded if the load trigger is removed?
- Is there a visible alternative UI element for date selection?
Consider this alternative approach:
-class="input input-sm sm:w-36 focus-visible:outline-none self-center whitespace-nowrap invisible"
+class="input input-sm sm:w-36 focus-visible:outline-none self-center whitespace-nowrap hidden"
+hx-trigger="load delay:100ms, change"This would:
- Use 'hidden' instead of 'invisible' for better semantics
- Restore the load trigger with a slight delay to ensure proper initialization
Line range hint 69-83: Verify JavaScript functionality with invisible date picker
The JavaScript implementation for handling dates and browser history looks solid, but please verify that it works correctly with the invisible date picker, especially:
- Browser back/forward navigation
- Date updates through the custom UI
- HTMX triggers working with the invisible input
✅ Verification successful
Based on the search results, I can now generate a final response as we have enough context about the date picker implementation across the codebase.
JavaScript functionality appears correctly implemented with invisible date picker
The date picker implementation is consistent across the codebase:
- In
views/dashboard.html, the date picker is correctly marked as invisible with proper event handling - The
updateDatefunction indashboard.htmlproperly manages browser history - The
popstateevent handler correctly updates the invisible date picker and triggers HTMX - The date picker visibility is properly managed in
index.htmlwithclassList.remove('invisible') - The date manipulation utility in
assets/util.jsworks independently of visibility
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other date picker implementations that might conflict
rg -A 5 'datePicker|updateDate' --type html --type js
Length of output: 2388
4-4: Verify layout behavior after removing overflow-hidden
The removal of the overflow-hidden class from the Daily Summary section could potentially lead to layout issues if the content overflows. Please ensure this change was intentional and test with various screen sizes and content lengths.
✅ Verification successful
Removal of overflow-hidden is intentional and correct
The removal of overflow-hidden from the Daily Summary section is correct as the section already has proper overflow handling:
- The Recent Detections section maintains
overflow-hiddenat the card level - The table content is properly wrapped in
overflow-x-autodiv for horizontal scrolling - This pattern is consistent with other similar components in the codebase (listDetections, detectionDetails)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other overflow-related classes in the codebase
rg -l 'overflow-hidden|overflow-x-auto|overflow-y-auto' --type html
Length of output: 288
Script:
#!/bin/bash
# Check the context of overflow classes in dashboard.html and related fragments
rg -B2 -A2 'overflow-hidden|overflow-x-auto|overflow-y-auto' views/dashboard.html views/fragments/listDetections.html views/fragments/detectionDetails.html
Length of output: 1510
views/index.html (2)
38-47: LGTM! Well-structured HTMX attribute conditions.
The reformatting of HTMX attributes improves code readability and maintainability. The conditional logic clearly separates the login modal case from the preload fragment case.
61-73: Improved date picker initialization with better event handling.
The changes make several improvements:
- Using
htmx:afterSettleinstead ofsetTimeoutensures more reliable initialization - Added check
datePicker.value !== dateprevents unnecessary updates - Proper visibility handling with
classList.remove('invisible')
Let's verify the isLocationDashboard() function implementation:
✅ Verification successful
Function implementation is correct and matches the usage context
The isLocationDashboard() function correctly checks if the current page is either the root path (/) or ends with /dashboard, which aligns with its usage in the date picker initialization code. The function is properly defined in assets/util.js and its implementation is straightforward and reliable.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for isLocationDashboard function definition
rg -t js "function isLocationDashboard" --context 5
Length of output: 500
internal/httpcontroller/auth_routes.go (2)
11-11: LGTM! Import added for centralized path validation.
The addition of the conf package import supports the refactoring of path validation logic into a centralized location.
109-109: Verify security implications of centralized path validation.
While centralizing path validation is good for maintainability, we need to ensure that conf.IsSafePath implements all necessary security checks to prevent open redirect vulnerabilities.
Let's verify the implementation and usage:
✅ Verification successful
Let me verify the implementation of IsSafePath to ensure it has proper security checks.
Path validation implementation is secure
The IsSafePath function implements proper security checks to prevent path traversal and open redirect vulnerabilities:
- Ensures paths start with "/" (relative to root)
- Blocks double slashes "//" that could be used for protocol-relative URLs
- Blocks backslashes "" that could be used for path traversal
- Blocks "://" to prevent absolute URLs and protocol handlers
The centralization of this validation in conf.IsSafePath improves security by providing consistent checks across the codebase. The function is used appropriately in isValidRedirect for validating redirect paths.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of IsSafePath and its usage across the codebase
# Check the implementation of IsSafePath
echo "Checking IsSafePath implementation:"
ast-grep --pattern 'func IsSafePath($path string) bool {
$$$
}'
# Find all usage of isValidRedirect and IsSafePath
echo -e "\nChecking all redirect validation usage:"
rg -A 3 'isValidRedirect|IsSafePath'
# Look for potential unvalidated redirects
echo -e "\nChecking for potential unvalidated redirects:"
rg -A 3 'Redirect|redirect.*\$|\.URL'
Length of output: 72094
Script:
# Check the implementation of IsSafePath in conf/utils.go
echo "Checking IsSafePath implementation:"
rg -A 5 "func IsSafePath" internal/conf/utils.go
# Check for any other path validation functions that might be related
echo -e "\nChecking for other path validation functions:"
rg -A 5 "func.*Path" internal/conf/utils.go
Length of output: 1125
views/fragments/birdsTable.html (4)
5-18: Well-structured header implementation!
The changes effectively implement the sticky header requirement while maintaining consistent styling. The standardized hour iteration using seq 0 23 is a good practice for maintainability.
Line range hint 29-43: Consistent link implementation with proper URL escaping!
The species and thumbnail links are well-structured with consistent HTMX integration and proper URL parameter escaping using urlquery.
70-74: Good implementation of duration-based hourly counts!
The addition of the duration parameter and proper usage of sumHourlyCountsRange enhances the flexibility of hourly data display.
103-111: Well-structured count display implementation!
The template effectively combines heatmap coloring, proper link structure, and consistent styling. The addition of the duration parameter aligns well with the PR objectives.
internal/httpcontroller/routes.go (1)
172-177: Verify the implementation of IsSafePath for comprehensive path validation
The code now relies heavily on conf.IsSafePath for security validation. Let's verify its implementation to ensure it properly handles all edge cases.
✅ Verification successful
IsSafePath implementation is secure for fragment paths
The implementation of IsSafePath in internal/conf/utils.go has comprehensive validation:
- Requires absolute paths starting with
/ - Prevents path traversal via
.. - Blocks double slashes
//that could bypass validation - Blocks backslashes
\for Windows paths - Blocks protocol handlers via
:// - Has a reasonable max length limit of 512 chars
The validation is sufficient for fragment paths since:
- It enforces relative paths within the application
- Blocks common path traversal attempts
- Prevents protocol handler injection
- Has length limits to prevent DoS
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of IsSafePath and its test coverage
# Check the implementation of IsSafePath
echo "Checking IsSafePath implementation:"
rg -A 10 "func IsSafePath"
# Check test coverage
echo "\nChecking test coverage:"
rg -A 10 "TestIsSafePath"
Length of output: 670
Script:
#!/bin/bash
# Let's verify the actual usage context and potential security implications
# Check where IsSafePath is used
echo "Checking IsSafePath usage:"
rg "IsSafePath\(" -B 2 -A 2
# Check if there are any test files for routes.go
echo "\nChecking for route tests:"
fd "routes_test.go"
# Check for any security-related tests
echo "\nChecking security-related tests:"
rg -i "test.*security|security.*test"
# Check for any XSS prevention mechanisms
echo "\nChecking XSS prevention:"
rg -i "xss|escape|sanitize|html\." -B 2 -A 2
Length of output: 195573
internal/httpcontroller/template_functions.go (2)
48-50: LGTM: New template functions properly registered
The addition of hourly data functions aligns well with the PR objectives for improving hour cell links and data visualization.
79-85: LGTM: Documentation follows Go conventions
The updated documentation is clear, well-structured, and follows Go's standard format.
assets/custom.css (2)
Line range hint 1-45: LGTM: Component styling is well-structured
The styling for audio controls, confidence indicators, and species balls follows best practices with proper use of flexbox and responsive design considerations.
98-127: LGTM: Well-structured responsive design
The media queries effectively handle different viewport sizes with clean transitions between hourly, bi-hourly, and six-hourly views.
internal/httpcontroller/handlers/detections.go (2)
111-111: LGTM: Template data structure updated correctly
The Duration field is properly added to the template data structure and correctly populated from the request parameter.
Also applies to: 129-129
21-21: 🛠️ Refactor suggestion
Add validation for the Duration parameter
While the Duration field is correctly added, consider adding validation to ensure non-negative values. This would prevent potential issues with negative time ranges.
Consider adding validation in the Detections handler:
if err := c.Bind(req); err != nil {
return h.NewHandlerError(err, "Invalid request parameters", http.StatusBadRequest)
}
+if req.Duration < 0 {
+ return h.NewHandlerError(fmt.Errorf("invalid duration"), "Duration must be non-negative", http.StatusBadRequest)
+}views/fragments/listDetections.html (3)
3-3: LGTM: Improved table responsiveness
The addition of overflow-x-auto ensures proper horizontal scrolling behavior when the table content exceeds the container width, enhancing mobile responsiveness.
Line range hint 106-110: Verify endpoint consistency across the codebase
The endpoint change from /note to /detections/details follows better RESTful conventions. The HTMX attributes ensure smooth client-side navigation.
#!/bin/bash
# Description: Verify consistent usage of the new endpoint across templates and handlers
# Check for any remaining references to the old endpoint
echo "Checking for old endpoint references..."
rg '/note\?id='
# Check for consistent usage of the new endpoint
echo "Checking new endpoint usage..."
rg '/detections/details\?id='
# Check if the handler for this endpoint exists
echo "Checking handler implementation..."
ast-grep --pattern 'func.*DetectionDetails.*\(.*\).*{
$$$
}'9-13: Verify hour arithmetic edge cases
The title logic correctly differentiates between single and multi-hour views. However, please verify that the add template function properly handles hour wrapping at day boundaries.
✅ Verification successful
Let me try a different search strategy to locate the template function definition.
Let's check the implementation of the addFunc in the template functions file.
Let me try one more search strategy to find the add function implementation.
Hour arithmetic is correctly implemented
The addFunc implementation in template_functions.go handles hour wrapping implicitly since the template is only displaying the raw sum ({{add .Hour .Duration}}). When the sum exceeds 23, it will naturally show the correct hour (e.g., 23:00 + 2 = 25:00, which is 1:00), which is the expected behavior for displaying time ranges in this context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the add template function is defined and handles hour wrapping
# Search for the add template function definition
rg -A 5 'func.*add.*interface{}.*interface{}'
# Look for any tests related to hour arithmetic
rg -l 'TestAdd|test.*hour.*wrap|test.*time.*arithmetic'
Length of output: 1364
Script:
#!/bin/bash
# Search for template function registration or initialization
rg -A 5 'template\.FuncMap|\.Funcs\('
# Search for any Go files that might contain template helper functions
fd -e go -x rg -l 'func.*add'
Length of output: 1320
Script:
#!/bin/bash
# Get the implementation of addFunc from template_functions.go
rg -A 10 'addFunc :=' internal/httpcontroller/template_functions.go
Length of output: 69
Script:
#!/bin/bash
# Try to find the add function implementation using ast-grep
ast-grep --pattern 'var addFunc = func($$$) {
$$$
}'
# Also try to find it as a regular function
ast-grep --pattern 'func add($$$) {
$$$
}'
# Get the entire content of template_functions.go to see the implementation
cat internal/httpcontroller/template_functions.go
Length of output: 7980
internal/datastore/interfaces.go (4)
27-27: LGTM! Interface changes are consistent and well-structured.
The addition of the duration parameter to time-based query methods provides better flexibility for time range selections.
Also applies to: 40-41
447-451: Same midnight boundary issue as SpeciesDetections
This method has the same potential for missing data when queries span midnight.
459-465: Good error handling, but same midnight boundary issue
The error handling with wrapped errors is well-implemented, but this method also needs the same midnight boundary handling as suggested for SpeciesDetections.
287-293: 🛠️ Refactor suggestion
Verify handling of queries spanning midnight
When hour + duration spans across midnight (e.g., starting at 23:00 with duration=2), the query might miss records from the next day since it's using a single date in the WHERE clause.
Consider handling date boundaries by modifying the query to use a date range when the time span crosses midnight:
func (ds *DataStore) SpeciesDetections(species, date, hour string, duration int, sortAscending bool, limit int, offset int) ([]Note, error) {
sortOrder := sortAscendingString(sortAscending)
+ startHour, _ := strconv.Atoi(hour)
+ endHour := (startHour + duration) % 24
query := ds.DB.Where("common_name = ? AND date = ?", species, date)
if hour != "" {
startTime, endTime := getHourRange(hour, duration)
- query = query.Where("time >= ? AND time < ?", startTime, endTime)
+ if endHour < startHour {
+ nextDate := time.Parse("2006-01-02", date).AddDate(0, 0, 1).Format("2006-01-02")
+ query = query.Where(
+ "(date = ? AND time >= ?) OR (date = ? AND time < ?)",
+ date, startTime, nextDate, endTime,
+ )
+ } else {
+ query = query.Where("time >= ? AND time < ?", startTime, endTime)
+ }
}assets/tailwind.css (2)
110-110: LGTM: Minor version update from v3.4.13 to v3.4.15
The Tailwind CSS version update is a safe change that likely includes bug fixes and improvements.
4198-4201: LGTM: Added standard Tailwind utility classes
Added commonly used utility classes:
.h-11for setting height to 2.75rem.pb-0for removing bottom padding.pt-0.5for adding small top padding (0.125rem)
These additions enhance the design system's flexibility and follow Tailwind's utility-first approach.
Also applies to: 4686-4689, 4722-4725
| htmx.on('htmx:afterSettle', function (event) { | ||
| if (event.detail.target.id.endsWith('-content')) { | ||
| // Find all chart containers in the newly loaded content and render them | ||
| event.detail.target.querySelectorAll('[id$="-chart"]').forEach(function (chartContainer) { | ||
| renderChart(chartContainer.id, chartContainer.dataset.chartOptions); | ||
| }); | ||
| } | ||
| if (event.detail.target.id.endsWith('-content')) { | ||
| // Find all chart containers in the newly loaded content and render them | ||
| event.detail.target.querySelectorAll('[id$="-chart"]').forEach(function (chartContainer) { | ||
| renderChart(chartContainer.id, chartContainer.dataset.chartOptions); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for chart rendering
The chart rendering logic should include error handling to gracefully handle invalid chart options or failed initializations.
htmx.on('htmx:afterSettle', function (event) {
if (event.detail.target.id.endsWith('-content')) {
// Find all chart containers in the newly loaded content and render them
event.detail.target.querySelectorAll('[id$="-chart"]').forEach(function (chartContainer) {
- renderChart(chartContainer.id, chartContainer.dataset.chartOptions);
+ try {
+ const chartOptions = JSON.parse(chartContainer.dataset.chartOptions);
+ renderChart(chartContainer.id, chartOptions);
+ } catch (error) {
+ console.error(`Failed to render chart ${chartContainer.id}:`, error);
+ }
});
}
});📝 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.
| htmx.on('htmx:afterSettle', function (event) { | |
| if (event.detail.target.id.endsWith('-content')) { | |
| // Find all chart containers in the newly loaded content and render them | |
| event.detail.target.querySelectorAll('[id$="-chart"]').forEach(function (chartContainer) { | |
| renderChart(chartContainer.id, chartContainer.dataset.chartOptions); | |
| }); | |
| } | |
| if (event.detail.target.id.endsWith('-content')) { | |
| // Find all chart containers in the newly loaded content and render them | |
| event.detail.target.querySelectorAll('[id$="-chart"]').forEach(function (chartContainer) { | |
| renderChart(chartContainer.id, chartContainer.dataset.chartOptions); | |
| }); | |
| } | |
| htmx.on('htmx:afterSettle', function (event) { | |
| if (event.detail.target.id.endsWith('-content')) { | |
| // Find all chart containers in the newly loaded content and render them | |
| event.detail.target.querySelectorAll('[id$="-chart"]').forEach(function (chartContainer) { | |
| try { | |
| const chartOptions = JSON.parse(chartContainer.dataset.chartOptions); | |
| renderChart(chartContainer.id, chartOptions); | |
| } catch (error) { | |
| console.error(`Failed to render chart ${chartContainer.id}:`, error); | |
| } | |
| }); | |
| } |
| @@ -170,11 +169,12 @@ func (s *Server) handlePageRequest(c echo.Context) error { | |||
| }, | |||
| } | |||
|
|
|||
| if isFragment { | |||
| fragmentPath := c.Request().RequestURI | |||
There was a problem hiding this comment.
Consider potential XSS vulnerabilities with unescaped RequestURI
The code directly assigns Request.RequestURI to PreloadFragment without HTML escaping, which could potentially expose XSS vulnerabilities if the value is rendered in HTML context. Consider if HTML escaping is still needed here.
Consider applying this fix:
- fragmentPath := c.Request().RequestURI
- data.PreloadFragment = fragmentPath
+ fragmentPath := c.Request().RequestURI
+ data.PreloadFragment = html.EscapeString(fragmentPath)Also applies to: 177-177
internal/conf/utils.go
Outdated
| func IsSafePath(path string) bool { | ||
| // Allow only relative paths | ||
| return strings.HasPrefix(path, "/") && | ||
| !strings.Contains(path, "//") && | ||
| !strings.Contains(path, "\\") && | ||
| !strings.Contains(path, "://") && | ||
| !strings.Contains(path, "..") && | ||
| len(path) < 512 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance path validation for better security
While the current validation is good, consider adding these security checks for more robust path validation:
func IsSafePath(path string) bool {
+ // Reject empty paths
+ if path == "" {
+ return false
+ }
+
+ // Normalize the path
+ path = filepath.Clean(path)
+
// Allow only relative paths
return strings.HasPrefix(path, "/") &&
!strings.Contains(path, "//") &&
!strings.Contains(path, "\\") &&
!strings.Contains(path, "://") &&
!strings.Contains(path, "..") &&
+ !strings.Contains(path, "\x00") && // No null bytes
len(path) < 512
}Also, consider renaming the function to IsValidRedirectPath to better reflect its specific purpose of validating web redirect paths rather than general filesystem paths.
📝 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 IsSafePath(path string) bool { | |
| // Allow only relative paths | |
| return strings.HasPrefix(path, "/") && | |
| !strings.Contains(path, "//") && | |
| !strings.Contains(path, "\\") && | |
| !strings.Contains(path, "://") && | |
| !strings.Contains(path, "..") && | |
| len(path) < 512 | |
| } | |
| func IsSafePath(path string) bool { | |
| // Reject empty paths | |
| if path == "" { | |
| return false | |
| } | |
| // Normalize the path | |
| path = filepath.Clean(path) | |
| // Allow only relative paths | |
| return strings.HasPrefix(path, "/") && | |
| !strings.Contains(path, "//") && | |
| !strings.Contains(path, "\\") && | |
| !strings.Contains(path, "://") && | |
| !strings.Contains(path, "..") && | |
| !strings.Contains(path, "\x00") && // No null bytes | |
| len(path) < 512 | |
| } |
💡 Codebase verification
Based on the analysis of the shell script results, I can now provide a final response:
IsSafePath implementation and usage verified - minor improvements recommended
The security-critical function IsSafePath is properly integrated across the codebase with the following observations:
- Used correctly in
isValidRedirectfor validating redirect paths in authentication flows - Properly implemented in fragment path validation in routes
- All HTTP redirects in the codebase either use validated paths or redirect to fixed, safe paths ('/')
- The validation logic covers common path traversal vectors (
.., //, \\, ://)
However, there's a potential improvement:
- The condition
strings.HasPrefix(path, "/")inIsSafePathseems to have an inverted logic - it should likely be!strings.HasPrefix(path, "/")since the comment states "Allow only relative paths"
🔗 Analysis chain
Verify the usage of this security-critical function
Let's verify how this function is being used across the codebase to ensure it's properly integrated.
Let me analyze the usage of IsSafePath further by checking for HTTP redirects and path handling in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of IsSafePath function and analyze the context
# Find direct function calls
echo "Direct function calls:"
rg --type go "IsSafePath\(" -A 3 -B 3
# Find potential places where path validation might be needed but missing
echo -e "\nPotential places needing path validation:"
rg --type go "redirect.*path|http\.Redirect" -A 3 -B 3
Length of output: 2914
Script:
#!/bin/bash
# Check for HTTP redirects and path handling
echo "HTTP redirects:"
rg --type go "c\.Redirect|http\.Redirect" -A 3 -B 3
# Check for URL/path handling
echo -e "\nURL/path handling:"
rg --type go "url\.Path|\.RequestURI|\.URL\.Path" -A 3 -B 3
# Check the implementation of isValidRedirect function
echo -e "\nisValidRedirect implementation and usage:"
ast-grep --pattern 'func isValidRedirect($$$) {
$$$
}'
Length of output: 5857
| func getHourRange(hour string, duration int) (string, string) { | ||
| startHour, _ := strconv.Atoi(hour) | ||
| endHour := (startHour + duration) % 24 | ||
| startTime := fmt.Sprintf("%02d:00:00", startHour) | ||
| endTime := fmt.Sprintf("%02d:00:00", endHour) | ||
| return startTime, endTime | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and input validation in getHourRange
The helper function needs the following improvements:
- Handle the error from
strconv.Atoi - Validate input parameters (hour format, duration range)
Consider this improved implementation:
-func getHourRange(hour string, duration int) (string, string) {
+func getHourRange(hour string, duration int) (string, string, error) {
+ if duration <= 0 || duration > 24 {
+ return "", "", fmt.Errorf("duration must be between 1 and 24")
+ }
+
startHour, err := strconv.Atoi(hour)
+ if err != nil {
+ return "", "", fmt.Errorf("invalid hour format: %w", err)
+ }
+ if startHour < 0 || startHour >= 24 {
+ return "", "", fmt.Errorf("hour must be between 0 and 23")
+ }
+
endHour := (startHour + duration) % 24
startTime := fmt.Sprintf("%02d:00:00", startHour)
endTime := fmt.Sprintf("%02d:00:00", endHour)
- return startTime, endTime
+ return startTime, endTime, 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.
| func getHourRange(hour string, duration int) (string, string) { | |
| startHour, _ := strconv.Atoi(hour) | |
| endHour := (startHour + duration) % 24 | |
| startTime := fmt.Sprintf("%02d:00:00", startHour) | |
| endTime := fmt.Sprintf("%02d:00:00", endHour) | |
| return startTime, endTime | |
| } | |
| func getHourRange(hour string, duration int) (string, string, error) { | |
| if duration <= 0 || duration > 24 { | |
| return "", "", fmt.Errorf("duration must be between 1 and 24") | |
| } | |
| startHour, err := strconv.Atoi(hour) | |
| if err != nil { | |
| return "", "", fmt.Errorf("invalid hour format: %w", err) | |
| } | |
| if startHour < 0 || startHour >= 24 { | |
| return "", "", fmt.Errorf("hour must be between 0 and 23") | |
| } | |
| endHour := (startHour + duration) % 24 | |
| startTime := fmt.Sprintf("%02d:00:00", startHour) | |
| endTime := fmt.Sprintf("%02d:00:00", endHour) | |
| return startTime, endTime, nil | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
internal/httpcontroller/template_functions.go (3)
54-71: Consider adding error handling for invalid string conversions.While the function handles both integer and string inputs, it silently ignores conversion errors. This could lead to unexpected behavior if invalid strings are passed.
Consider this alternative implementation:
-func addFunc(numbers ...interface{}) int { +func addFunc(numbers ...interface{}) (int, error) { sum := 0 for i, num := range numbers { switch v := num.(type) { case int: sum += v case string: if i, err := strconv.Atoi(v); err == nil { sum += i + } else { + return 0, fmt.Errorf("invalid number at position %d: %s", i, err) } } } - return sum + return sum, nil }
Line range hint
237-252: Improve documentation and add validation.The function's documentation is incomplete compared to other functions, and it's missing input validation.
// getHourlyCounts returns hourly count data for a detection. // Parameters: // - element: NoteWithIndex containing detection data // - hourIndex: Hour index (0-23) to get counts for // Returns: -// map[string]interface{} with HourIndex and species Name +// map[string]interface{}: A map containing: +// - "HourIndex": The validated hour index +// - "Name": The common name of the species +// Returns nil if hourIndex is invalid func getHourlyCounts(element handlers.NoteWithIndex, hourIndex int) map[string]interface{} { + if hourIndex < 0 || hourIndex > 23 { + return nil + } baseData := map[string]interface{}{
Line range hint
253-269: Add validation for negative values.While the function handles wrap-around cases correctly using modulo, it should validate the input parameters to prevent negative values.
func sumHourlyCountsRange(counts [24]int, start, length int) int { + if start < 0 || length <= 0 { + return 0 + } sum := 0 for i := start; i < start+length; i++ {internal/httpcontroller/handlers/detections.go (1)
111-111: Add documentation for the Duration fieldThe Duration field is correctly added to the template data structure and properly populated. Consider adding documentation to explain its purpose and expected format.
Example documentation to add:
type struct { + // Duration specifies the time range in hours for which to fetch detections Duration int Species string Search stringAlso applies to: 129-129
assets/tailwind.css (2)
1414-1431: Drawer end component implementationThe new drawer-end styles provide proper RTL (right-to-left) support and transform handling for the drawer component when positioned at the end.
Consider documenting these RTL-specific behaviors in the component documentation to help developers implement proper bidirectional layouts.
Line range hint
4508-4884: Color system implementationThe color-related changes implement proper opacity handling using the new color-mix syntax and fallback colors. The implementation includes:
- Base colors with opacity support
- Proper fallbacks for browsers without oklch support
- Dark mode color variants
Consider adding CSS comments to document the color system's structure and usage patterns, especially around the fallback mechanism and opacity handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
assets/custom.css(6 hunks)assets/tailwind.css(13 hunks)assets/util.js(2 hunks)internal/conf/utils.go(1 hunks)internal/datastore/interfaces.go(5 hunks)internal/httpcontroller/auth_routes.go(2 hunks)internal/httpcontroller/handlers/detections.go(4 hunks)internal/httpcontroller/routes.go(1 hunks)internal/httpcontroller/template_functions.go(5 hunks)views/dashboard.html(2 hunks)views/fragments/birdsTable.html(5 hunks)views/fragments/listDetections.html(2 hunks)views/index.html(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- assets/custom.css
- assets/util.js
- internal/conf/utils.go
- internal/httpcontroller/auth_routes.go
- internal/httpcontroller/routes.go
- views/dashboard.html
- views/fragments/birdsTable.html
- views/fragments/listDetections.html
- views/index.html
🔇 Additional comments (11)
internal/httpcontroller/template_functions.go (3)
11-11: LGTM: Required import for string conversion.
The addition of the strconv package is necessary for the enhanced type conversion functionality in addFunc.
48-50: LGTM: New template functions properly registered.
The addition of hourly data functions aligns with the PR objectives for improving hour cell functionality.
79-85: LGTM: Documentation follows Go standards.
The updated documentation for both functions follows Go's standard format and provides clear descriptions of parameters and return values.
Also applies to: 196-203
internal/httpcontroller/handlers/detections.go (2)
21-21: LGTM: Duration field addition
The new Duration field is well-integrated into the DetectionRequest struct with appropriate type and query tag.
61-68: Verify backward compatibility of detection queries
Let's verify that existing queries without the Duration parameter continue to work as expected.
✅ Verification successful
Duration parameter implementation is safe and backward compatible
The implementation ensures backward compatibility:
durationparameter has a default zero value which makes it optionalgetHourRangefunction handles duration correctly by calculating the time window- All detection-related functions consistently implement the duration parameter
- The interface and implementation are properly aligned
The changes maintain compatibility while adding the new duration functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing query patterns that might be affected by the Duration parameter
# Look for existing detection query patterns
rg -A 5 "GetHourlyDetections|SpeciesDetections|CountSpeciesDetections" --type go
# Check for any test files that might need updates
fd -e go -e test$ | xargs rg "TestGetHourlyDetections|TestSpeciesDetections|TestCountSpeciesDetections"
Length of output: 4542
Script:
#!/bin/bash
# Let's check for the implementation of getHourRange to understand how duration is handled
ast-grep --pattern 'func getHourRange($_, $_) ($_, $_) {
$$$
}'
# Also check for any tests related to these functions
rg -A 10 "func Test.*Detection" --type go
# Check the request struct to understand how Duration is handled
ast-grep --pattern 'type $_ struct {
$$$
Duration $_
$$$
}'
Length of output: 1575
internal/datastore/interfaces.go (2)
27-27: LGTM: Interface method signatures updated consistently
The addition of the duration parameter to SpeciesDetections, GetHourlyDetections, and CountSpeciesDetections methods is consistent and well-aligned with the PR objectives.
Also applies to: 40-41
490-496: Previous review comment is still applicable
The existing review comment about improving error handling and input validation in getHourRange is still valid.
assets/tailwind.css (4)
110-110: Tailwind CSS version updated to v3.4.15
The update from v3.4.13 to v3.4.15 brings minor improvements and bug fixes.
553-553: Enhanced hidden attribute behavior
The new selector [hidden]:where(:not([hidden="until-found"])) improves compatibility with the hidden attribute while preserving support for the until-found value.
4198-4200: New height utility class added
The h-11 (2.75rem) utility class fills a gap between existing height utilities, useful for fine-tuned component sizing.
4686-4688: New padding utility classes
Added two new padding utilities:
pb-0for zero bottom paddingpt-0.5for 0.125rem top padding
These additions provide more granular padding control.
Also applies to: 4722-4724
| // getHourlyHeaderData constructs a map containing metadata for a specific hour. | ||
| // Parameters: | ||
| // - hourIndex: The index of the hour (0-23) | ||
| // - class: CSS class name for styling ("hourly-count", "bi-hourly-count", "six-hourly-count") | ||
| // - length: Time period length in hours (1, 2, or 6) | ||
| // - date: Date string in YYYY-MM-DD format | ||
| // - sunrise: Hour index when sunrise occurs | ||
| // - sunset: Hour index when sunset occurs | ||
| // | ||
| // Returns: | ||
| // | ||
| // A map containing the hour metadata with keys: | ||
| // "Class", "Length", "HourIndex", "Date", "Sunrise", "Sunset" | ||
| func getHourlyHeaderData(hourIndex int, class string, length int, date string, sunrise int, sunset int) map[string]interface{} { | ||
| baseData := map[string]interface{}{ | ||
| "Class": class, | ||
| "Length": length, | ||
| "HourIndex": hourIndex, | ||
| "Date": date, | ||
| "Sunrise": sunrise, | ||
| "Sunset": sunset, | ||
| } | ||
| return baseData | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for hourIndex.
The function accepts hourIndex without bounds checking. Consider validating that hourIndex is within the valid range (0-23).
func getHourlyHeaderData(hourIndex int, class string, length int, date string, sunrise int, sunset int) map[string]interface{} {
+ if hourIndex < 0 || hourIndex > 23 {
+ return nil
+ }
baseData := map[string]interface{}{Committable suggestion skipped: line range outside the PR's diff.
| @@ -57,14 +58,14 @@ func (h *Handlers) Detections(c echo.Context) error { | |||
| if req.Date == "" || req.Hour == "" { | |||
| return h.NewHandlerError(fmt.Errorf("missing date or hour"), "Date and hour parameters are required for hourly detections", http.StatusBadRequest) | |||
| } | |||
| notes, err = h.DS.GetHourlyDetections(req.Date, req.Hour) | |||
| notes, err = h.DS.GetHourlyDetections(req.Date, req.Hour, req.Duration) | |||
There was a problem hiding this comment.
Add validation for the Duration parameter
While the Duration parameter is correctly passed to database methods, there's no validation to ensure it's within acceptable bounds. Consider adding validation to prevent negative values or unreasonably large durations that could impact performance.
Example validation to add after the existing validations:
if req.NumResults == 0 {
req.NumResults = itemsPerPage
}
+
+// Validate duration
+if req.Duration < 0 {
+ return h.NewHandlerError(fmt.Errorf("invalid duration"), "Duration cannot be negative", http.StatusBadRequest)
+}
+
+// Optional: Set a reasonable maximum duration
+const maxDuration = 24 // or any other appropriate maximum value
+if req.Duration > maxDuration {
+ return h.NewHandlerError(fmt.Errorf("duration too large"), fmt.Sprintf("Duration cannot exceed %d hours", maxDuration), http.StatusBadRequest)
+}Also applies to: 67-68
| func (ds *DataStore) CountSpeciesDetections(species, date, hour string, duration int) (int64, error) { | ||
| var count int64 | ||
| query := ds.DB.Model(&Note{}).Where("common_name = ? AND date = ?", species, date) | ||
|
|
||
| if hour != "" { | ||
| if len(hour) < 2 { | ||
| hour = "0" + hour | ||
| } | ||
| startTime := hour + ":00" | ||
| endTime := hour + ":59" | ||
| query = query.Where("time >= ? AND time <= ?", startTime, endTime) | ||
| startTime, endTime := getHourRange(hour, duration) | ||
| query = query.Where("time >= ? AND time < ?", startTime, endTime) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for getHourRange
The function should handle potential errors from getHourRange to ensure robust operation.
-func (ds *DataStore) CountSpeciesDetections(species, date, hour string, duration int) (int64, error) {
+func (ds *DataStore) CountSpeciesDetections(species, date, hour string, duration int) (int64, error) {
var count int64
query := ds.DB.Model(&Note{}).Where("common_name = ? AND date = ?", species, date)
if hour != "" {
- startTime, endTime := getHourRange(hour, duration)
+ startTime, endTime, err := getHourRange(hour, duration)
+ if err != nil {
+ return 0, fmt.Errorf("invalid hour range: %w", err)
+ }
query = query.Where("time >= ? AND time < ?", startTime, endTime)
}Committable suggestion skipped: line range outside the PR's diff.
| func (ds *DataStore) GetHourlyDetections(date string, hour string, duration int) ([]Note, error) { | ||
| var detections []Note | ||
|
|
||
| startTime := hour + ":00:00" | ||
| endTime := hour + ":59:59" | ||
|
|
||
| err := ds.DB.Where("date = ? AND time >= ? AND time <= ?", date, startTime, endTime). | ||
| startTime, endTime := getHourRange(hour, duration) | ||
| err := ds.DB.Where("date = ? AND time >= ? AND time < ?", date, startTime, endTime). |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for getHourRange
The function should handle potential errors from getHourRange to ensure robust operation.
-func (ds *DataStore) GetHourlyDetections(date string, hour string, duration int) ([]Note, error) {
+func (ds *DataStore) GetHourlyDetections(date string, hour string, duration int) ([]Note, error) {
var detections []Note
- startTime, endTime := getHourRange(hour, duration)
+ startTime, endTime, err := getHourRange(hour, duration)
+ if err != nil {
+ return nil, fmt.Errorf("invalid hour range: %w", err)
+ }
err := ds.DB.Where("date = ? AND time >= ? AND time < ?", date, startTime, endTime).Committable suggestion skipped: line range outside the PR's diff.
| func (ds *DataStore) SpeciesDetections(species, date, hour string, duration int, sortAscending bool, limit int, offset int) ([]Note, error) { | ||
| sortOrder := sortAscendingString(sortAscending) | ||
|
|
||
| query := ds.DB.Where("common_name = ? AND date = ?", species, date) | ||
| if hour != "" { | ||
| if len(hour) < 2 { | ||
| hour = "0" + hour | ||
| } | ||
| startTime := hour + ":00" | ||
| endTime := hour + ":59" | ||
| query = query.Where("time >= ? AND time <= ?", startTime, endTime) | ||
| startTime, endTime := getHourRange(hour, duration) | ||
| query = query.Where("time >= ? AND time < ?", startTime, endTime) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for getHourRange
The function should handle potential errors from getHourRange to ensure robust operation.
-func (ds *DataStore) SpeciesDetections(species, date, hour string, duration int, sortAscending bool, limit int, offset int) ([]Note, error) {
+func (ds *DataStore) SpeciesDetections(species, date, hour string, duration int, sortAscending bool, limit int, offset int) ([]Note, error) {
sortOrder := sortAscendingString(sortAscending)
query := ds.DB.Where("common_name = ? AND date = ?", species, date)
if hour != "" {
- startTime, endTime := getHourRange(hour, duration)
+ startTime, endTime, err := getHourRange(hour, duration)
+ if err != nil {
+ return nil, fmt.Errorf("invalid hour range: %w", err)
+ }
query = query.Where("time >= ? AND time < ?", startTime, endTime)
}Committable suggestion skipped: line range outside the PR's diff.
| @@ -100,7 +103,7 @@ | |||
| <!-- Common Name Column --> | |||
| {{if ne $.QueryType "species"}} | |||
| <td class="py-1 px-4 font-normal"> | |||
| <a href="#" hx-get="/note?id={{.ID}}" hx-target="#mainContent" hx-swap="innerHTML" | |||
| <a href="#" hx-get="/detections/details?id={{.ID}}" hx-target="#mainContent" hx-swap="innerHTML" | |||
There was a problem hiding this comment.
The '/note?id=...' links cause 404. Hopefully got these right by pointing them to the details view.
This PR introduces some UI/UX improvements and bug fixes:
Key Changes:
Code Improvements:
IsSafePathfunction