fix: resolve 9 UI and backend bugs - #11
Conversation
…mework - Fix Sortable.js not loaded for sidebar nav reordering (added global script) - Remove excessive INFO-level debug logging from auth/session middleware - Fix Logs/Services 500 error on log-sources API for non-HAProxy servers - Fix Traffic plugin 'g is undefined' with null guards on SVG group - Fix root URL (/) blank page by adding haproxy case to integration path map - Fix backend lockup on quick refresh by using RLock for session validation - Fix OS Updates CDN noise by cleaning up assets middleware logging - Fix disabled-entities JS errors with null checks and move to HAProxy settings Closes #10 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses 9 distinct UI and backend bugs that were causing issues ranging from JavaScript errors to backend lockups. The fixes include improvements to session validation performance, error handling for servers without HAProxy configuration, null safety guards in JavaScript, and UI navigation fixes.
Changes:
- Fixed session validation performance bottleneck by using read locks and async updates for session activity tracking
- Added null guards to prevent JavaScript errors in traffic visualization and disabled entities pages
- Improved error handling for log sources API to gracefully handle servers without HAProxy
- Added missing navigation case for HAProxy plugin and loaded Sortable.js globally
- Cleaned up excessive debug logging that was cluttering server console output
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| gearbox/static/js/traffic/traffic-visualization.js | Added null guards for SVG group variable to prevent "g is undefined" errors |
| gearbox/internal/plugins/dashboard/handlers.go | Added missing "haproxy" case to fix blank page on root URL navigation |
| gearbox/internal/framework/templates/pages/user_pages.templ | Removed Disabled Entities button from main settings page |
| gearbox/internal/framework/templates/pages/haproxy_settings.templ | Moved Disabled Entities button to HAProxy settings page |
| gearbox/internal/framework/templates/pages/disabled_entities.templ | Added null checks for DOM elements to prevent JS errors when no entities exist |
| gearbox/internal/framework/templates/layouts/base.templ | Added global Sortable.js script for sidebar nav reordering |
| gearbox/internal/framework/middleware/assets.go | Removed excessive per-request logging with emoji prefixes |
| gearbox/internal/framework/database/users.go | Changed session validation to use RLock and async updates to prevent backend lockup |
| gearbox/internal/framework/database/log_sources.go | Changed error handling to return empty list instead of 500 error for non-HAProxy servers |
| gearbox/internal/framework/auth/auth.go | Removed excessive INFO-level debug logging from authentication flow |
Comments suppressed due to low confidence (1)
gearbox/internal/framework/database/log_sources.go:57
- The comment on line 52 mentions "Use LEFT JOIN so servers without HAProxy entries return empty results", but the query on line 57 uses a regular INNER JOIN. An INNER JOIN will return no rows when there's no matching haproxy_servers entry, but it won't cause the query to fail - it will simply return zero rows. The comment is misleading as written.
Either change the JOIN to LEFT JOIN to match the comment, or update the comment to accurately describe the INNER JOIN behavior (which is that it returns no rows for servers without HAProxy configuration).
// Use LEFT JOIN so servers without HAProxy entries return empty results instead of errors.
// The haproxy_servers table may not have an entry for every server.
query := `
SELECT ls.id, ls.haproxy_server_id, ls.log_name, ls.display_name
FROM log_source_settings ls
JOIN haproxy_servers hs ON ls.haproxy_server_id = hs.id
…vation The previous async goroutine approach still caused lockups because Go's sync.RWMutex is write-preferring: a pending Lock() blocks new RLock() callers. Every auth check spawned a goroutine calling Lock(), which blocked all subsequent RLock() calls (including new auth checks), causing cascading lockups during rapid page refreshes. Fix: debounce session_last_activity updates to at most once per 5 minutes per user using a separate lightweight mutex for the timing check. This eliminates the constant stream of Lock() calls that starved readers. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
D3.js was hardcoded to load from CDN (d3js.org) but CSP in local asset mode only allows 'self', blocking the script and causing "d3 is not defined" errors and a stuck loading spinner. - Update traffic.templ and widgets.templ to use UseLocalAssets pattern - Add d3js.org to production CSP whitelist - Add D3.js to Makefile dev-assets download target - D3.js v7 downloaded locally via make dev-assets or manual curl Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
SortableJS was loaded as an ES module from CDN via sortable-loader.js, which was blocked by CSP in local asset mode. This caused "Sortable not loaded!" errors preventing sidebar nav reordering and dashboard editing. - Add SortableJS UMD build to base.templ UseLocalAssets conditional - Remove sortable-loader.js module script from base.templ and dashboard_editor.templ - Simplify editor.js Sortable check (no longer needs async event listener) - Add SortableJS to Makefile dev-assets download target Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
| d.mu.RLock() | ||
| defer d.mu.RUnlock() | ||
|
|
||
| // Use LEFT JOIN so servers without HAProxy entries return empty results instead of errors. |
There was a problem hiding this comment.
The comment states "Use LEFT JOIN so servers without HAProxy entries return empty results" but the query actually uses an INNER JOIN (line 57). An INNER JOIN will only return rows where both tables have matching records, so servers without HAProxy entries won't return any results - which achieves the goal. However, the comment should be corrected to match the actual query type being used, or the query should be changed to LEFT JOIN if that was the original intent. Since the goal is to return empty results for servers without HAProxy, the INNER JOIN is functionally correct, but the comment is misleading.
| // Use LEFT JOIN so servers without HAProxy entries return empty results instead of errors. | |
| // Use a JOIN so only servers with HAProxy entries return log sources; others return empty results. |
| d.sessionActivityMu.Lock() | ||
| lastUpdate, exists := d.sessionActivityTimes[userID] | ||
| if exists && now.Sub(lastUpdate) < sessionActivityDebounce { | ||
| d.sessionActivityMu.Unlock() | ||
| return // Skip — updated recently | ||
| } | ||
| d.sessionActivityTimes[userID] = now | ||
| d.sessionActivityMu.Unlock() |
There was a problem hiding this comment.
The sessionActivityTimes map is never cleaned up and will grow unbounded over time as users log in. Even if a user logs out or their account is deleted, their entry remains in this map indefinitely. This is a memory leak that will accumulate entries for every unique user ID that ever authenticates. Consider adding a cleanup mechanism, such as: removing entries when users log out (in ClearUserSessionToken), periodically pruning entries older than sessionActivityDebounce, or using a size-limited cache structure like an LRU cache.
In edit mode, pointer-events:none was set on the <a> nav-link element to prevent navigation, but this also blocked mouse events on the drag handle (a child div), preventing SortableJS from detecting drags. Fix: use addEventListener/removeEventListener for click prevention instead of pointer-events:none, keeping pointer events active so drag handles work. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
SortableJS classList.add() only accepts single class names, but ghostClass was set to 'bg-blue-50 dark:bg-slate-700' (two classes). This threw DOMException on drag start, preventing items from moving. Fix: use single custom class names (sortable-ghost, sortable-chosen, sortable-drag) and define them in CSS using Tailwind @apply directives. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Browser natively makes <a> tags draggable, which competes with SortableJS drag handling and causes sporadic behavior — sometimes the browser's native link drag activates instead of SortableJS. Fix: set draggable="false" and prevent dragstart events on nav links when entering edit mode, restoring defaults when exiting. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
gearbox/internal/framework/database/log_sources.go:60
- The comment claims to use LEFT JOIN but the query uses INNER JOIN. This means servers without HAProxy entries will still return empty results (which is correct behavior), but not for the reason stated in the comment. Either update the comment to reflect the actual JOIN type, or change the JOIN to LEFT JOIN if that was the intended behavior. With INNER JOIN, the query will return no results when there's no matching haproxy_servers entry, which achieves the goal but the comment is misleading.
// Use LEFT JOIN so servers without HAProxy entries return empty results instead of errors.
// The haproxy_servers table may not have an entry for every server.
query := `
SELECT ls.id, ls.haproxy_server_id, ls.log_name, ls.display_name
FROM log_source_settings ls
JOIN haproxy_servers hs ON ls.haproxy_server_id = hs.id
WHERE hs.server_id = ?
ORDER BY ls.display_name ASC
`
The small drag handle was difficult to grab. Now the entire <li> is the SortableJS drag target (removed handle option), and the drag dots icon is just a visual indicator. Also permanently set draggable="false" on all nav-link <a> tags to prevent browser native drag from interfering. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
saveSidebarOrder() required window.ServerSelector which doesn't exist on pages without a server dropdown (e.g. Alerts). The API endpoint already defaults to the first enabled server when no server param is provided, so the client-side check was unnecessary. Also fixed the API URL from /api/integrations/sort-order (404) to the correct /api/plugins/sort-order route. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The API expects a flat map {"pluginName": sortIndex} but the sidebar JS
was sending {"order": [{name, sortOrder}]} causing a 400 Bad Request.
Also fixed the plugins settings page URL from the non-existent
/api/integrations/sort-order to the correct /api/plugins/sort-order.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
gearbox/internal/framework/templates/layouts/base.templ:556
- The drag handle elements are rendered and shown/hidden in edit mode (lines 498, 514), but the SortableJS configuration at line 547-556 doesn't include a
handleoption. This means the entire navigation item is draggable, making the visual drag handle indicators misleading - users might think they need to grab the handle, but they can actually drag from anywhere on the item.
Either add handle: '.nav-drag-handle' to the Sortable configuration to make the handles functional, or remove the visual handle indicators entirely if the design intent is to make the entire item draggable.
sidebarSortable = window.Sortable.create(navList, {
animation: 200,
draggable: '.nav-item-draggable',
ghostClass: 'sortable-ghost',
chosenClass: 'sortable-chosen',
dragClass: 'sortable-drag',
onEnd: function(evt) {
console.log('Item moved from index', evt.oldIndex, 'to', evt.newIndex);
}
});
| // sessionActivityMu protects the sessionActivityTimes map. | ||
| sessionActivityMu sync.Mutex | ||
| // sessionActivityTimes tracks the last time we updated session_last_activity per user. | ||
| // Updates are debounced to avoid write lock contention on the main mutex during auth checks. | ||
| sessionActivityTimes map[string]time.Time |
There was a problem hiding this comment.
The sessionActivityTimes map grows indefinitely and is never cleaned up. Each user ID that ever logs in will have an entry in this map that persists for the lifetime of the application. Over time, this will cause a memory leak, especially in systems with many users or where user IDs change frequently.
Consider implementing periodic cleanup to remove entries that haven't been accessed in a long time (e.g., entries older than sessionActivityDebounce * 2), or remove entries when users log out via ClearUserSessionToken.
| // Use LEFT JOIN so servers without HAProxy entries return empty results instead of errors. | ||
| // The haproxy_servers table may not have an entry for every server. |
There was a problem hiding this comment.
The comment on line 52 states "Use LEFT JOIN so servers without HAProxy entries return empty results" but the actual SQL query on line 57 uses an INNER JOIN. An INNER JOIN will only return rows where both tables have matching records, which means servers without HAProxy entries will indeed return empty results, but not for the reason stated. If the goal is to gracefully handle servers without HAProxy configuration, the comment should either:
- Be updated to match the implementation: "Use INNER JOIN which naturally returns empty results for servers without HAProxy entries", OR
- The query should be changed to use LEFT JOIN if there's a need to return additional information about servers without HAProxy configuration.
The current implementation appears correct for the stated goal (returning empty results), but the comment is misleading.
HAProxy plugin card on the plugins settings page was missing a Configure button. Links to /settings/boxes where HAProxy server management and disabled entities editor live. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add dedicated HAProxy settings page at /settings/plugins/haproxy with Disabled Entities button, Config Editor link, and HAProxy git config - Move HAProxy git configuration from box git settings to plugin settings - Remove Disabled Entities button from boxes page (now on HAProxy settings) - Remove HAProxy config icon from box action buttons - Rename "HAProxy Servers" to "Boxes" in settings sidebar - Update description to "Manage monitored boxes" - Update all "Back to Servers/HAProxy Servers" links to "Back to Boxes" Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
gearbox/internal/framework/database/users.go:1294
- When a session is cleared (on logout or password change), the corresponding entry in sessionActivityTimes map should also be removed to prevent the map from growing unbounded. Add cleanup logic to remove the map entry for this userID.
// ClearUserSessionToken invalidates a user's session token.
// Called on logout or password change to force re-authentication.
func (d *DB) ClearUserSessionToken(userID string) error {
d.mu.Lock()
defer d.mu.Unlock()
_, err := d.db.Exec(`
UPDATE users
SET session_token = NULL,
session_created_at = NULL,
session_last_activity = NULL,
session_ip = NULL,
session_user_agent = NULL
WHERE id = ?`, userID)
return err
}
- Clean up sessionActivityTimes map entry on logout to prevent unbounded growth over time - Log errors from async session activity DB writes instead of silently ignoring them - Fix misleading "LEFT JOIN" comment in log_sources.go (uses INNER JOIN) - Log database query errors in GetEnabledLogSourcesByServerID instead of silently returning nil - Prevent duplicate preventNavClick event listeners from rapid edit mode toggling by removing before adding Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Summary
GetEnabledLogSourcesByServerIDto gracefully return empty list for non-HAProxy servers instead of 500haproxycase tointegrationPathFromNamein dashboard handlersValidateSessionTokenfrom exclusive Lock to RLock for reads, async goroutine for session_last_activity updatesTest plan
Closes #10
🤖 Generated with Claude Code