fix(viewer): only show album grid for photos and videos#157
Merged
Conversation
📝 WalkthroughWalkthroughSingle-file Vue template update introducing lightbox media viewing, refining message/album handling, strengthening WebSocket and push notification infrastructure, improving date-based navigation, and reformatting template markup throughout with functional behavior refinements. ChangesChat UI, Lightbox, and Notification Updates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/web/templates/index.html (4)
3325-3345:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSeed the picker with
viewerTimezone, not the browser timezone.
openDatePicker()usesmoment(initialDate), while the date separator andjumpToDate()are based onviewerTimezone. If those timezones differ, clicking a separator near midnight can preselect the wrong day and jump to the wrong messages.Suggested fix
- selectedDate.value = initialDate ? moment(initialDate).format('YYYY-MM-DD') : null + selectedDate.value = initialDate + ? moment.utc(initialDate).tz(viewerTimezone.value).format('YYYY-MM-DD') + : null🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/web/templates/index.html` around lines 3325 - 3345, openDatePicker currently seeds the picker with moment(initialDate) which uses the browser timezone; change it to parse/format using viewerTimezone so the picker defaultDate and selectedDate.value use the same timezone as jumpToDate and the date separators. In the openDatePicker function (and where flatpickr defaultDate is set), replace moment(initialDate).format('YYYY-MM-DD') / new Date() usage with moment.tz(initialDate || /* fallback */, viewerTimezone).format('YYYY-MM-DD') (or construct a timezone-aware Date for flatpickr) so flatpickrInstance.setDate and the selectedDate.value are based on viewerTimezone rather than the client's local timezone.
2049-2087:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid reconnect storms when replacing the socket.
initWebSocket()closes the previous socket, but that socket'sonclosestill queuessetTimeout(initWebSocket, 5000). A manual reinit or logout can leave stale reconnect timers behind and eventually create parallel sockets, duplicate subscriptions, and duplicate notifications.As per coding guidelines, `src/**`: "Check for proper async/await usage and connection cleanup."Suggested fix
const initWebSocket = () => { - if (ws) { - ws.close() + if (wsReconnectTimer) { + clearTimeout(wsReconnectTimer) + wsReconnectTimer = null + } + if (ws) { + ws.onclose = null + ws.close() + ws = null } const protocol = window.location.protocol === 'https:' ? 'wss:' : 'ws:' const wsUrl = `${protocol}//${window.location.host}/ws/updates` try { - ws = new WebSocket(wsUrl) + const socket = new WebSocket(wsUrl) + ws = socket - ws.onopen = () => { + socket.onopen = () => { // ... } - ws.onmessage = (event) => { + socket.onmessage = (event) => { // ... } - ws.onclose = () => { + socket.onclose = () => { + if (ws !== socket || !isAuthenticated.value) return console.log('[WS] Connection closed, reconnecting in 5s...') wsReconnectTimer = setTimeout(initWebSocket, 5000) } - ws.onerror = (e) => { + socket.onerror = (e) => { console.warn('[WS] Error:', e) } } catch (e) { console.warn('[WS] Failed to connect:', e) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/web/templates/index.html` around lines 2049 - 2087, initWebSocket currently closes an existing ws but leaves its onclose handler active, causing reconnect storms; to fix, ensure you clear any pending wsReconnectTimer (clearTimeout on wsReconnectTimer) and suppress the old socket's onclose before calling close (e.g., remove or overwrite ws.onclose, ws.onmessage, ws.onopen, ws.onerror to no-ops) so the old socket cannot queue another initWebSocket; after closing also null out ws to indicate no active socket and only set the reconnect timer from the new socket's onclose handler (referencing initWebSocket, ws, wsReconnectTimer, and the ws.onclose handler).
3402-3475:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThe gap-fill loop is indexing newest/oldest backwards.
sortedMessagesis newest-first, but this block storessortedMessages[0]as the "oldest" loaded date and breaks whentargetIdx > 0. That makes the background fill stop immediately for most jumped-to messages, so the surrounding history never loads.Suggested fix
- const oldestLoadedDateBeforeJump = sortedMessages.value.length > 0 - ? sortedMessages.value[0].date + const oldestLoadedDateBeforeJump = sortedMessages.value.length > 0 + ? sortedMessages.value[sortedMessages.value.length - 1].date : null @@ - // Check if gap is filled by looking at target's position - // in sorted messages (oldest first) + // Check if gap is filled by looking at target's position + // in sorted messages (newest first) const sorted = sortedMessages.value const targetIdx = sorted.findIndex(m => m.id === message.id) - // Gap is filled when target is no longer the oldest - if (targetIdx > 0) { + // Gap is filled when target is no longer the oldest loaded message + if (targetIdx !== -1 && targetIdx < sorted.length - 1) { break }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/web/templates/index.html` around lines 3402 - 3475, The code treats sortedMessages as newest-first but uses index 0 as the oldest and stops filling when targetIdx > 0; fix by treating the last element as the oldest and reversing the targetIndex check: set oldestLoadedDateBeforeJump to sortedMessages.value[sortedMessages.value.length - 1].date (or null) instead of [0], and inside fillGap compute targetIdx and consider the gap filled when targetIdx < sorted.length - 1 (i.e., target is no longer the last/oldest), updating the break condition accordingly; leave other logic (loadMessages, hasMore, messages, scrollToMessage) unchanged.
2526-2545:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGate shared captions the same way as album hiding.
Now that grouped documents stay visible,
getAlbumCaption(msg)still treats anygrouped_idas a shared album. A grouped file batch with one caption will render that same caption under every document card.Suggested follow-up
const getAlbumCaption = (msg) => { const groupedId = getGroupedId(msg) if (!groupedId) return null + if (msg.media?.type !== 'photo' && msg.media?.type !== 'video') return null const album = getAlbumForMessage(msg) if (!album) return null const captionMsg = album.find(m => m.text && m.text.trim() !== '') return captionMsg ? captionMsg.text : null }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/web/templates/index.html` around lines 2526 - 2545, getAlbumCaption currently treats any message with a grouped_id as a shared album caption and shows it for every card; to fix, gate caption rendering the same way as album hiding by making getAlbumCaption only return the caption for the first item in the album. Update getAlbumCaption(msg) to either accept an index (e.g., getAlbumCaption(msg, index)) or call isFirstInAlbum(msg, index) (using sortedMessages.value.indexOf(msg) if not passing index) and return the caption only when isFirstInAlbum(...) is true (and keep existing checks for msg.media?.type and getGroupedId(msg)). Ensure you reference getAlbumCaption, isFirstInAlbum, isHiddenAlbumMessage, getGroupedId, and sortedMessages.value in the change so captions only appear on the first album item.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/web/templates/index.html`:
- Around line 3325-3345: openDatePicker currently seeds the picker with
moment(initialDate) which uses the browser timezone; change it to parse/format
using viewerTimezone so the picker defaultDate and selectedDate.value use the
same timezone as jumpToDate and the date separators. In the openDatePicker
function (and where flatpickr defaultDate is set), replace
moment(initialDate).format('YYYY-MM-DD') / new Date() usage with
moment.tz(initialDate || /* fallback */, viewerTimezone).format('YYYY-MM-DD')
(or construct a timezone-aware Date for flatpickr) so flatpickrInstance.setDate
and the selectedDate.value are based on viewerTimezone rather than the client's
local timezone.
- Around line 2049-2087: initWebSocket currently closes an existing ws but
leaves its onclose handler active, causing reconnect storms; to fix, ensure you
clear any pending wsReconnectTimer (clearTimeout on wsReconnectTimer) and
suppress the old socket's onclose before calling close (e.g., remove or
overwrite ws.onclose, ws.onmessage, ws.onopen, ws.onerror to no-ops) so the old
socket cannot queue another initWebSocket; after closing also null out ws to
indicate no active socket and only set the reconnect timer from the new socket's
onclose handler (referencing initWebSocket, ws, wsReconnectTimer, and the
ws.onclose handler).
- Around line 3402-3475: The code treats sortedMessages as newest-first but uses
index 0 as the oldest and stops filling when targetIdx > 0; fix by treating the
last element as the oldest and reversing the targetIndex check: set
oldestLoadedDateBeforeJump to sortedMessages.value[sortedMessages.value.length -
1].date (or null) instead of [0], and inside fillGap compute targetIdx and
consider the gap filled when targetIdx < sorted.length - 1 (i.e., target is no
longer the last/oldest), updating the break condition accordingly; leave other
logic (loadMessages, hasMore, messages, scrollToMessage) unchanged.
- Around line 2526-2545: getAlbumCaption currently treats any message with a
grouped_id as a shared album caption and shows it for every card; to fix, gate
caption rendering the same way as album hiding by making getAlbumCaption only
return the caption for the first item in the album. Update getAlbumCaption(msg)
to either accept an index (e.g., getAlbumCaption(msg, index)) or call
isFirstInAlbum(msg, index) (using sortedMessages.value.indexOf(msg) if not
passing index) and return the caption only when isFirstInAlbum(...) is true (and
keep existing checks for msg.media?.type and getGroupedId(msg)). Ensure you
reference getAlbumCaption, isFirstInAlbum, isHiddenAlbumMessage, getGroupedId,
and sortedMessages.value in the change so captions only appear on the first
album item.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a362493c-e210-49ac-8620-a7e9580230be
📒 Files selected for processing (1)
src/web/templates/index.html
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #157 +/- ##
==========================================
+ Coverage 93.11% 93.20% +0.09%
==========================================
Files 22 22
Lines 6300 6300
==========================================
+ Hits 5866 5872 +6
+ Misses 434 428 -6 🚀 New features to boost your workflow:
|
Owner
|
Thanks @dom1n1nk4s for the fix! Released in v7.8.4. The album grid now correctly renders only for photos and videos — documents with grouped_id no longer get squeezed into a media grid. Appreciate the clear screenshots showing the before/after! |
5 tasks
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Type of Change
Database Changes
scripts/Data Consistency Checklist
chat_idvalues use marked format (via_get_marked_id())_strip_tz()before DB operationsTesting
python -m pytest tests/ -v)ruff check .)ruff format --check .)Security Checklist
Here's an example of how it looks like on Telegram:
And how it looks like on latest version:
After this PR's fix, it looks like this:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements