Conversation
Co-authored-by: jbreton-boreas <42969636+jbreton-boreas@users.noreply.github.com> Agent-Logs-Url: https://github.com/jbreton-boreas/Koffan/sessions/6e367aa8-cd3c-4886-9640-92ca9f377839
…rscroll-behavior-y: none Co-authored-by: jbreton-boreas <42969636+jbreton-boreas@users.noreply.github.com> Agent-Logs-Url: https://github.com/jbreton-boreas/Koffan/sessions/528cce9d-4572-4397-8a65-f50d7d365fd0
…s met Co-authored-by: jbreton-boreas <42969636+jbreton-boreas@users.noreply.github.com> Agent-Logs-Url: https://github.com/jbreton-boreas/Koffan/sessions/a6c8fadb-4975-4e83-b196-9fadcf6fa560
…issue' into origin/fix-refresh-icon-issue
…s on refresh Co-authored-by: jbreton-boreas <42969636+jbreton-boreas@users.noreply.github.com> Agent-Logs-Url: https://github.com/jbreton-boreas/Koffan/sessions/b392018e-cd12-4672-8c3b-7c529c512650
…race condition Co-authored-by: jbreton-boreas <42969636+jbreton-boreas@users.noreply.github.com> Agent-Logs-Url: https://github.com/jbreton-boreas/Koffan/sessions/a5847bfd-6331-416d-a1e1-be7da9db4ac2
StephanMeijer
left a comment
There was a problem hiding this comment.
Code Review — PR #104: Fixed some issues
Good PR. The three fixes address real user-facing bugs and the implementation is clean overall. The list_id scoping, touchcancel handler, and home WS support are all solid additions. A few issues to address, mostly around edge cases in the new home page WebSocket logic.
Bugs
1. Progress bar won't appear for initially empty lists
The server-side template (home.html:54) wraps the progress bar and percentage in {{if gt $list.Stats.TotalItems 0}}, so lists that start with zero items have no .bg-pink-400 or .w-10.text-right elements in the DOM. Similarly, buildListCardHtml() doesn't include progress bar elements.
When items are later added (and refreshListStats() runs), it tries to query these selectors — but they don't exist. The item count text updates correctly via data-stats-count, but the progress bar never appears until a full page reload.
Suggestion: Either always render the progress bar (with width: 0%), or have refreshListStats() inject the progress bar HTML when it doesn't exist yet and total > 0.
2. WebSocket reconnect is permanently dead after 5 failures
scheduleWSReconnect() caps at 5 attempts, after which WebSocket is never reestablished. The visibilitychange listener only calls refreshListStats() — not connectWS(). So if a user backgrounds the app long enough for the WS to die (mobile is aggressive about this), returning to the app gives them an HTTP refresh but no further real-time updates.
Suggestion: Reset _reconnectAttempts and call connectWS() in the visibilitychange handler (when becoming visible), alongside refreshListStats(). This matches the pattern already used in app.js's list page WS implementation.
Missing event coverage
3. lists_reordered and template_applied not handled
The server broadcasts lists_reordered (when another client reorders via move up/down) and template_applied (which adds items to a list, affecting counts), but the home page WS handler's switch statement doesn't handle them.
lists_reorderedmatters — the DOM order of list cards will be stale if another client reorders.template_appliedaffects item counts and should triggerrefreshListStats().
Suggestion: Add case 'lists_reordered': that reloads the lists container (or reorders DOM nodes), and case 'template_applied': that calls this.refreshListStats().
Minor / Nitpicks
4. getSectionsForList() silently falls back on invalid list_id
if listIDStr := c.Query("list_id"); listIDStr != "" {
listID, err := strconv.ParseInt(listIDStr, 10, 64)
if err == nil {
return db.GetSectionsByList(listID)
}
}
return db.GetAllSections()If list_id=abc, ParseInt fails and silently falls back to GetAllSections(). This isn't a security issue (just returns active list sections), but a non-numeric list_id probably indicates a client bug. Consider logging a warning or returning a 400.
5. currentListId() URL helper — nice pattern
The currentListId() method and the URL-building pattern (listId ? \...&list_id=${listId}` : '...') is clean and appears 5 times. If this grows further, consider extracting a small helper like sectionsUrl(format)` to DRY it up. Not necessary now though.
6. Haptic feedback
navigator.vibrate(30) — the guard is correct, and it's a nice touch (pun intended). 30ms is on the short end; 40-50ms might be more perceptible on budget Android devices. Very minor.
7. PR description says "two issues" but lists three
Fixed two issues encountered while using the app:
- Items not refreshing after opening the app again on my phone.
- The refresh icon stuck in the screen after a pull down to refresh
- List item count not updating on home screen
Three bullets, not two :)
What's done well
touchcancelhandler +overscroll-behavior-y: none— Root-cause fix for the stuck spinner. The touchcancel handler resets all state correctly, and the CSS prevents Chrome Android's native PTR from competing. Good combination.- Explicit
list_idscoping — Much more robust than relying on server-side "active list" state, which can be stale if another device changed it. ThegetSectionsForList()helper with fallback is a clean approach. - Race condition guard in
insertRemoteItem()—if (document.getElementById(\item-${itemId}`)) return;` is a simple and effective dedup. Good defensive programming. visibilitychangelistener — Correctly addresses the "items not refreshing" issue when returning to the app. The stats refresh on tab focus is the right UX pattern for a mobile-first PWA.- Home page WS architecture — Follows the exact same patterns as the list page WS (connect/reconnect/ping/message dispatch). Clean and consistent.
Fixed two issues encountered while using the app: