From 39e75f4eea4c0f1aea96c6c549898c73ff3b2f37 Mon Sep 17 00:00:00 2001 From: Jared Vititoe Date: Thu, 9 Jul 2026 23:09:42 -0400 Subject: [PATCH] fix(bookmarks): harden sort/group after review Address findings from 2 review agents on the bookmark sort/group feature: - Flash on open: the persisted-sort atom now uses getOnInit so the saved sort applies on the first render instead of briefly showing Newest and reordering after mount. - Stale collapse state: prune collapsed roomIds that no longer have any bookmark, so a room re-saved later doesn't reappear pre-collapsed and the Set can't grow unbounded across a session. - Corrupt persisted value: validate the stored sort with a new isBookmarkSort type guard, normalizing anything unexpected to Newest so exactly one sort button is always active. - a11y: room group headers now expose an explicit aria-label (", N saved messages") instead of announcing the avatar alt and the visible name twice with a bare count, plus aria-controls linking the header to its collapsible content region. - Layout: move the sort control to its own toolbar row so the three buttons don't crowd the count text in the narrow (266px) panel. - Memoize filtered/sortedItems/groups for consistency with renderItem. Adds unit tests for isBookmarkSort, group-order tie-break, and groupBookmarksByRoom immutability. Co-Authored-By: Claude Opus 4.8 --- src/app/features/bookmarks/BookmarksPanel.tsx | 94 +++++++++++++++---- src/app/utils/bookmarks.test.ts | 33 ++++++- src/app/utils/bookmarks.ts | 7 ++ 3 files changed, 114 insertions(+), 20 deletions(-) diff --git a/src/app/features/bookmarks/BookmarksPanel.tsx b/src/app/features/bookmarks/BookmarksPanel.tsx index 06206ae54..67fc77009 100644 --- a/src/app/features/bookmarks/BookmarksPanel.tsx +++ b/src/app/features/bookmarks/BookmarksPanel.tsx @@ -1,4 +1,11 @@ -import React, { ChangeEvent, ReactNode, useCallback, useEffect, useState } from 'react'; +import React, { + ChangeEvent, + ReactNode, + useCallback, + useEffect, + useMemo, + useState, +} from 'react'; import { Room } from 'matrix-js-sdk'; import { useAtom } from 'jotai'; import { atomWithStorage, createJSONStorage } from 'jotai/utils'; @@ -20,6 +27,7 @@ import classNames from 'classnames'; import { useBookmarks, Bookmark } from '../../hooks/useBookmarks'; import { BookmarkSort, + isBookmarkSort, sortBookmarks, groupBookmarksByRoom, } from '../../utils/bookmarks'; @@ -49,10 +57,13 @@ function formatTimeAgo(ts: number): string { } // Remember the last-chosen sort across panel opens (the panel unmounts on close). +// getOnInit reads localStorage synchronously at init so the persisted sort is +// applied on the very first render (no flash from the 'newest' default). const bookmarkSortAtom = atomWithStorage( 'cinny_bookmarks_sort_v1', 'newest', createJSONStorage(() => localStorage), + { getOnInit: true }, ); const SORT_OPTIONS: { value: BookmarkSort; label: string }[] = [ @@ -196,12 +207,20 @@ type RoomGroupHeaderProps = { roomName: string; count: number; collapsed: boolean; + contentId: string; onToggle: () => void; }; // Collapsible section header for the "By room" grouping. Uses the live room name // / avatar when the room is joined, falling back to the stored snapshot name. -function RoomGroupHeader({ roomId, roomName, count, collapsed, onToggle }: RoomGroupHeaderProps) { +function RoomGroupHeader({ + roomId, + roomName, + count, + collapsed, + contentId, + onToggle, +}: RoomGroupHeaderProps) { const mx = useMatrixClient(); const useAuthentication = useMediaAuthentication(); const room = mx.getRoom(roomId) ?? undefined; @@ -218,6 +237,10 @@ function RoomGroupHeader({ roomId, roomName, count, collapsed, onToggle }: RoomG radii="300" onClick={onToggle} aria-expanded={!collapsed} + aria-controls={contentId} + // Explicit name avoids the avatar alt + visible name being announced twice, + // and gives the bare count meaning for screen readers. + aria-label={`${displayRoomName}, ${count} saved message${count !== 1 ? 's' : ''}`} style={{ justifyContent: 'flex-start', padding: config.space.S200 }} > @@ -226,7 +249,7 @@ function RoomGroupHeader({ roomId, roomName, count, collapsed, onToggle }: RoomG {nameInitials(displayRoomName)}} /> @@ -250,7 +273,9 @@ export function BookmarksPanel({ onClose }: BookmarksPanelProps) { const { bookmarks, removeBookmark } = useBookmarks(); const { navigateRoom } = useRoomNavigate(); const [filter, setFilter] = useState(''); - const [sort, setSort] = useAtom(bookmarkSortAtom); + const [storedSort, setSort] = useAtom(bookmarkSortAtom); + // Normalize a stale/corrupt persisted value so exactly one sort is always active. + const sort: BookmarkSort = isBookmarkSort(storedSort) ? storedSort : 'newest'; // roomIds whose group section is collapsed (only relevant in "By room" mode). const [collapsed, setCollapsed] = useState>(new Set()); @@ -288,14 +313,33 @@ export function BookmarksPanel({ onClose }: BookmarksPanelProps) { }; const query = filter.trim().toLowerCase(); - const filtered: Bookmark[] = - query.length === 0 - ? bookmarks - : bookmarks.filter( - (bk) => - bk.previewText.toLowerCase().includes(query) || - bk.roomName.toLowerCase().includes(query), - ); + const filtered: Bookmark[] = useMemo( + () => + query.length === 0 + ? bookmarks + : bookmarks.filter( + (bk) => + bk.previewText.toLowerCase().includes(query) || + bk.roomName.toLowerCase().includes(query), + ), + [bookmarks, query], + ); + + // Prune collapsed roomIds that no longer have any bookmark, so a room re-saved + // later doesn't reappear pre-collapsed and the Set can't grow unbounded. + useEffect(() => { + setCollapsed((prev) => { + if (prev.size === 0) return prev; + const live = new Set(bookmarks.map((bk) => bk.roomId)); + let changed = false; + const next = new Set(); + prev.forEach((roomId) => { + if (live.has(roomId)) next.add(roomId); + else changed = true; + }); + return changed ? next : prev; + }); + }, [bookmarks]); // Live render when the room is joined (useRoomEvent needs a non-null Room); // otherwise fall back to the stored snapshot for rooms we've left. Shared by @@ -323,8 +367,14 @@ export function BookmarksPanel({ onClose }: BookmarksPanelProps) { [mx, handleJump, removeBookmark], ); - const sortedItems = sort === 'room' ? filtered : sortBookmarks(filtered, sort); - const groups = sort === 'room' ? groupBookmarksByRoom(filtered) : []; + const sortedItems = useMemo( + () => (sort === 'room' ? filtered : sortBookmarks(filtered, sort)), + [filtered, sort], + ); + const groups = useMemo( + () => (sort === 'room' ? groupBookmarksByRoom(filtered) : []), + [filtered, sort], + ); return ( {bookmarks.length > 0 && ( - - + <> + {filtered.length === bookmarks.length ? `${bookmarks.length} saved message${bookmarks.length !== 1 ? 's' : ''}` : `${filtered.length} of ${bookmarks.length} messages`} - + {SORT_OPTIONS.map((opt) => ( ))} - + )} @@ -415,6 +465,7 @@ export function BookmarksPanel({ onClose }: BookmarksPanelProps) { {sort === 'room' ? groups.map((group) => { const isCollapsed = collapsed.has(group.roomId); + const contentId = `bookmark-group-${group.roomId}`; return ( toggleGroup(group.roomId)} /> - {!isCollapsed && group.items.map((bk) => renderItem(bk))} + {!isCollapsed && ( + + {group.items.map((bk) => renderItem(bk))} + + )} ); }) diff --git a/src/app/utils/bookmarks.test.ts b/src/app/utils/bookmarks.test.ts index 7bc7972c4..87a6402d7 100644 --- a/src/app/utils/bookmarks.test.ts +++ b/src/app/utils/bookmarks.test.ts @@ -1,6 +1,6 @@ import { test } from 'node:test'; import assert from 'node:assert/strict'; -import { sortBookmarks, groupBookmarksByRoom } from './bookmarks'; +import { sortBookmarks, groupBookmarksByRoom, isBookmarkSort } from './bookmarks'; import { Bookmark } from '../hooks/useBookmarks'; const bk = (eventId: string, roomId: string, savedAt: number, roomName = roomId): Bookmark => ({ @@ -99,6 +99,37 @@ test('groupBookmarksByRoom carries the stored room name', () => { assert.equal(groups[0].roomName, 'General'); }); +test('groupBookmarksByRoom orders equal-newest groups deterministically by roomId', () => { + // Both rooms' newest save is 200 → tie broken by roomId ascending. + const input = [bk('a', 'rB', 200), bk('b', 'rA', 200), bk('c', 'rA', 100)]; + const groups = groupBookmarksByRoom(input); + assert.deepEqual( + groups.map((g) => g.roomId), + ['rA', 'rB'], + ); +}); + +test('groupBookmarksByRoom does not mutate its input', () => { + const input = [bk('a', 'r1', 100), bk('b', 'r2', 300), bk('c', 'r1', 200)]; + const before = input.map((b) => b.eventId); + groupBookmarksByRoom(input); + assert.deepEqual( + input.map((b) => b.eventId), + before, + ); +}); + test('groupBookmarksByRoom returns empty for empty input', () => { assert.deepEqual(groupBookmarksByRoom([]), []); }); + +test('isBookmarkSort accepts valid values and rejects everything else', () => { + assert.equal(isBookmarkSort('newest'), true); + assert.equal(isBookmarkSort('oldest'), true); + assert.equal(isBookmarkSort('room'), true); + assert.equal(isBookmarkSort('bogus'), false); + assert.equal(isBookmarkSort(''), false); + assert.equal(isBookmarkSort(undefined), false); + assert.equal(isBookmarkSort(null), false); + assert.equal(isBookmarkSort(3), false); +}); diff --git a/src/app/utils/bookmarks.ts b/src/app/utils/bookmarks.ts index 0eb309761..6fc53deaf 100644 --- a/src/app/utils/bookmarks.ts +++ b/src/app/utils/bookmarks.ts @@ -2,6 +2,13 @@ import { Bookmark } from '../hooks/useBookmarks'; export type BookmarkSort = 'newest' | 'oldest' | 'room'; +const BOOKMARK_SORTS: readonly BookmarkSort[] = ['newest', 'oldest', 'room']; + +/** Type guard for persisted/untrusted sort values (localStorage can hold stale/corrupt data). */ +export function isBookmarkSort(value: unknown): value is BookmarkSort { + return typeof value === 'string' && (BOOKMARK_SORTS as readonly string[]).includes(value); +} + export type BookmarkRoomGroup = { roomId: string; roomName: string;