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
  ("<room>, 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 <noreply@anthropic.com>

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<BookmarkSort>(
   '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 }}
     >
       <Box grow="Yes" alignItems="Center" gap="200" style={{ minWidth: 0 }}>
@@ -226,7 +249,7 @@ function RoomGroupHeader({ roomId, roomName, count, collapsed, onToggle }: RoomG
           <RoomAvatar
             roomId={roomId}
             src={avatarUrl}
-            alt={displayRoomName}
+            alt=""
             renderFallback={() => <Text size="H6">{nameInitials(displayRoomName)}</Text>}
           />
         </Avatar>
@@ -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<Set<string>>(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<string>();
+      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 (
     <Box
@@ -372,13 +422,13 @@ export function BookmarksPanel({ onClose }: BookmarksPanelProps) {
           }
         />
         {bookmarks.length > 0 && (
-          <Box alignItems="Center" justifyContent="SpaceBetween" gap="200">
-            <Text size="T200" priority="300">
+          <>
+            <Text size="T200" priority="300" truncate>
               {filtered.length === bookmarks.length
                 ? `${bookmarks.length} saved message${bookmarks.length !== 1 ? 's' : ''}`
                 : `${filtered.length} of ${bookmarks.length} messages`}
             </Text>
-            <Box as="div" role="group" aria-label="Sort saved messages" gap="100" shrink="No">
+            <Box as="div" role="group" aria-label="Sort saved messages" gap="100">
               {SORT_OPTIONS.map((opt) => (
                 <SortButton
                   key={opt.value}
@@ -388,7 +438,7 @@ export function BookmarksPanel({ onClose }: BookmarksPanelProps) {
                 />
               ))}
             </Box>
-          </Box>
+          </>
         )}
       </Box>
 
@@ -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 (
                       <Box key={group.roomId} direction="Column" gap="200">
                         <RoomGroupHeader
@@ -422,9 +473,14 @@ export function BookmarksPanel({ onClose }: BookmarksPanelProps) {
                           roomName={group.roomName}
                           count={group.items.length}
                           collapsed={isCollapsed}
+                          contentId={contentId}
                           onToggle={() => toggleGroup(group.roomId)}
                         />
-                        {!isCollapsed && group.items.map((bk) => renderItem(bk))}
+                        {!isCollapsed && (
+                          <Box id={contentId} direction="Column" gap="200">
+                            {group.items.map((bk) => renderItem(bk))}
+                          </Box>
+                        )}
                       </Box>
                     );
                   })

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);
+});

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;