fix(ux): cap the in-app toast stack; stable "Unread First" room sort
- Toast queue: a burst of notifications appended unboundedly and could cover the viewport. Cap at 5 in the atom writer, dropping the OLDEST non-sticky toast (sticky = action toasts requiring a click, never dropped). The drop scan excludes the just-appended newest (`length - 1` bound) so a fresh toast is never the one eaten when the cap is full of stickies — it stretches instead. Container gains a maxHeight + overflowY safety net and scrolls the newest (bottom) toast into view if the stack ever overflows. +4 unit tests incl. the cap-full-of-stickies boundary. - "Unread First" room sort left the entire read tail (all counts tie at 0) in arbitrary Map order. factoryRoomIdByUnread now breaks ties by recent activity. Relocated from Home.tsx (module-private) to utils/sort.ts (exported, pure) and unit-tested (equal-count and read-tail cases fall back to activity). Bug-hunt findings from LOTUS_TODO. Three review passes: the second caught that the cap could silently drop the newest notification when full of stickies (real bug, untested boundary) — fixed and covered; a third traced the corrected loop. Gate-green (tsc, eslint, prettier, 920 tests, build). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -222,6 +222,15 @@ export function LotusToastContainer() {
|
|||||||
|
|
||||||
const toasts = useAtomValue(toastQueueAtom);
|
const toasts = useAtomValue(toastQueueAtom);
|
||||||
const isMobile = useScreenSize() === ScreenSize.Mobile;
|
const isMobile = useScreenSize() === ScreenSize.Mobile;
|
||||||
|
const listRef = useRef<HTMLDivElement>(null);
|
||||||
|
|
||||||
|
// The newest toast is the last (bottom) child; if the stack ever overflows its
|
||||||
|
// max-height (many sticky action toasts), keep that newest one in view instead
|
||||||
|
// of leaving it scrolled below the fold.
|
||||||
|
useEffect(() => {
|
||||||
|
const el = listRef.current;
|
||||||
|
if (el) el.scrollTop = el.scrollHeight;
|
||||||
|
}, [toasts.length]);
|
||||||
|
|
||||||
if (toasts.length === 0) return null;
|
if (toasts.length === 0) return null;
|
||||||
|
|
||||||
@@ -237,10 +246,14 @@ export function LotusToastContainer() {
|
|||||||
flexDirection: 'column',
|
flexDirection: 'column',
|
||||||
gap: config.space.S200,
|
gap: config.space.S200,
|
||||||
pointerEvents: 'auto',
|
pointerEvents: 'auto',
|
||||||
|
// Safety net beyond the queue cap: if many sticky action toasts pile up they
|
||||||
|
// scroll within a bounded height instead of covering the whole screen.
|
||||||
|
maxHeight: isMobile ? '70vh' : '80vh',
|
||||||
|
overflowY: 'auto',
|
||||||
};
|
};
|
||||||
|
|
||||||
return (
|
return (
|
||||||
<div style={containerStyle} aria-live="polite" aria-label="Notifications">
|
<div ref={listRef} style={containerStyle} aria-live="polite" aria-label="Notifications">
|
||||||
{toasts.map((toast) => (
|
{toasts.map((toast) => (
|
||||||
<ToastCard key={toast.id} toast={toast} />
|
<ToastCard key={toast.id} toast={toast} />
|
||||||
))}
|
))}
|
||||||
|
|||||||
@@ -27,8 +27,11 @@ import { useVirtualizer } from '@tanstack/react-virtual';
|
|||||||
import { useAtom, useAtomValue } from 'jotai';
|
import { useAtom, useAtomValue } from 'jotai';
|
||||||
import { selectAtom } from 'jotai/utils';
|
import { selectAtom } from 'jotai/utils';
|
||||||
import FocusTrap from 'focus-trap-react';
|
import FocusTrap from 'focus-trap-react';
|
||||||
import { Unread } from '../../../../types/matrix/room';
|
import {
|
||||||
import { factoryRoomIdByActivity, factoryRoomIdByAtoZ } from '../../../utils/sort';
|
factoryRoomIdByActivity,
|
||||||
|
factoryRoomIdByAtoZ,
|
||||||
|
factoryRoomIdByUnread,
|
||||||
|
} from '../../../utils/sort';
|
||||||
import {
|
import {
|
||||||
NavButton,
|
NavButton,
|
||||||
NavCategory,
|
NavCategory,
|
||||||
@@ -210,17 +213,6 @@ function HomeEmpty() {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
const factoryRoomIdByUnread =
|
|
||||||
(roomToUnread: Map<string, Unread>) =>
|
|
||||||
(aId: string, bId: string): number => {
|
|
||||||
const aUnread = roomToUnread.get(aId);
|
|
||||||
const bUnread = roomToUnread.get(bId);
|
|
||||||
const aHas = (aUnread?.total ?? 0) > 0;
|
|
||||||
const bHas = (bUnread?.total ?? 0) > 0;
|
|
||||||
if (aHas !== bHas) return aHas ? -1 : 1;
|
|
||||||
return (bUnread?.total ?? 0) - (aUnread?.total ?? 0);
|
|
||||||
};
|
|
||||||
|
|
||||||
const DEFAULT_CATEGORY_ID = makeNavCategoryId('home', 'room');
|
const DEFAULT_CATEGORY_ID = makeNavCategoryId('home', 'room');
|
||||||
const FAVORITES_CATEGORY_ID = makeNavCategoryId('home', 'favorite');
|
const FAVORITES_CATEGORY_ID = makeNavCategoryId('home', 'favorite');
|
||||||
const LOW_PRIORITY_CATEGORY_ID = makeNavCategoryId('home', 'lowpriority');
|
const LOW_PRIORITY_CATEGORY_ID = makeNavCategoryId('home', 'lowpriority');
|
||||||
@@ -331,7 +323,7 @@ export function Home() {
|
|||||||
} else if (homeRoomSort === 'alpha') {
|
} else if (homeRoomSort === 'alpha') {
|
||||||
comparator = factoryRoomIdByAtoZ(mx);
|
comparator = factoryRoomIdByAtoZ(mx);
|
||||||
} else if (homeRoomSort === 'unread') {
|
} else if (homeRoomSort === 'unread') {
|
||||||
comparator = factoryRoomIdByUnread(roomToUnread);
|
comparator = factoryRoomIdByUnread(roomToUnread, mx);
|
||||||
} else {
|
} else {
|
||||||
comparator = factoryRoomIdByActivity(mx);
|
comparator = factoryRoomIdByActivity(mx);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -7,12 +7,13 @@ import { toastQueueAtom, dismissToastAtom, ToastNotif, createDownloadToast } fro
|
|||||||
// (toastQueueAtom append + null no-op guard, dismissToastAtom remove-by-id)
|
// (toastQueueAtom append + null no-op guard, dismissToastAtom remove-by-id)
|
||||||
// through a jotai store and read back via toastQueueAtom's getter.
|
// through a jotai store and read back via toastQueueAtom's getter.
|
||||||
|
|
||||||
const makeToast = (id: string): ToastNotif => ({
|
const makeToast = (id: string, sticky?: boolean): ToastNotif => ({
|
||||||
id,
|
id,
|
||||||
displayName: `name-${id}`,
|
displayName: `name-${id}`,
|
||||||
body: `body-${id}`,
|
body: `body-${id}`,
|
||||||
roomName: `room-${id}`,
|
roomName: `room-${id}`,
|
||||||
roomId: `!${id}:server`,
|
roomId: `!${id}:server`,
|
||||||
|
...(sticky ? { sticky: true } : {}),
|
||||||
});
|
});
|
||||||
|
|
||||||
test('starts empty', () => {
|
test('starts empty', () => {
|
||||||
@@ -86,6 +87,46 @@ test('dismissToastAtom for an unknown id is a no-op', () => {
|
|||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('toastQueueAtom caps at 5, dropping the oldest non-sticky', () => {
|
||||||
|
const store = createStore();
|
||||||
|
// Append 7 transient toasts; the queue should keep only the newest 5.
|
||||||
|
for (let i = 0; i < 7; i += 1) store.set(toastQueueAtom, makeToast(`t${i}`));
|
||||||
|
assert.deepEqual(
|
||||||
|
store.get(toastQueueAtom).map((t) => t.id),
|
||||||
|
['t2', 't3', 't4', 't5', 't6'],
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('toastQueueAtom never drops a sticky toast, even over cap', () => {
|
||||||
|
const store = createStore();
|
||||||
|
store.set(toastQueueAtom, makeToast('sticky-old', true));
|
||||||
|
for (let i = 0; i < 7; i += 1) store.set(toastQueueAtom, makeToast(`t${i}`));
|
||||||
|
const ids = store.get(toastQueueAtom).map((t) => t.id);
|
||||||
|
// The sticky action toast survives; the oldest non-sticky ones are dropped.
|
||||||
|
assert.ok(ids.includes('sticky-old'));
|
||||||
|
assert.ok(ids.includes('t6')); // newest kept
|
||||||
|
assert.ok(!ids.includes('t0')); // oldest non-sticky dropped
|
||||||
|
assert.equal(ids.length, 5);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('toastQueueAtom queue of all-sticky toasts is allowed to exceed the cap', () => {
|
||||||
|
const store = createStore();
|
||||||
|
for (let i = 0; i < 7; i += 1) store.set(toastQueueAtom, makeToast(`s${i}`, true));
|
||||||
|
// Nothing droppable → all 7 retained rather than silently losing action toasts.
|
||||||
|
assert.equal(store.get(toastQueueAtom).length, 7);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('toastQueueAtom keeps a new transient toast even when the cap is full of stickies', () => {
|
||||||
|
const store = createStore();
|
||||||
|
// Fill the cap with sticky action toasts, then a normal message toast arrives.
|
||||||
|
for (let i = 0; i < 5; i += 1) store.set(toastQueueAtom, makeToast(`s${i}`, true));
|
||||||
|
store.set(toastQueueAtom, makeToast('fresh'));
|
||||||
|
const ids = store.get(toastQueueAtom).map((t) => t.id);
|
||||||
|
// The newest is never the one dropped — the queue stretches instead of eating it.
|
||||||
|
assert.ok(ids.includes('fresh'));
|
||||||
|
assert.equal(ids.length, 6);
|
||||||
|
});
|
||||||
|
|
||||||
test('createDownloadToast: filename in body, no room navigation, unique ids', () => {
|
test('createDownloadToast: filename in body, no room navigation, unique ids', () => {
|
||||||
const a = createDownloadToast('photo.jpg');
|
const a = createDownloadToast('photo.jpg');
|
||||||
assert.equal(a.displayName, 'Downloaded');
|
assert.equal(a.displayName, 'Downloaded');
|
||||||
|
|||||||
+17
-1
@@ -46,12 +46,28 @@ export const createErrorToast = (
|
|||||||
|
|
||||||
const baseAtom = atom<ToastNotif[]>([]);
|
const baseAtom = atom<ToastNotif[]>([]);
|
||||||
|
|
||||||
|
// Cap concurrent toasts so a burst (e.g. many rooms lighting up while focused)
|
||||||
|
// can't stack unbounded and cover the viewport.
|
||||||
|
const MAX_TOASTS = 5;
|
||||||
|
|
||||||
// Write-only setter used in ClientNonUIFeatures
|
// Write-only setter used in ClientNonUIFeatures
|
||||||
export const toastQueueAtom = atom<ToastNotif[], [ToastNotif | null], void>(
|
export const toastQueueAtom = atom<ToastNotif[], [ToastNotif | null], void>(
|
||||||
(get) => get(baseAtom),
|
(get) => get(baseAtom),
|
||||||
(get, set, notif) => {
|
(get, set, notif) => {
|
||||||
if (notif === null) return; // no-op guard
|
if (notif === null) return; // no-op guard
|
||||||
set(baseAtom, [...get(baseAtom), notif]);
|
const next = [...get(baseAtom), notif];
|
||||||
|
// Over cap: drop the oldest NON-sticky toasts (transient message/error
|
||||||
|
// toasts auto-dismiss anyway); never drop a sticky action toast, which
|
||||||
|
// requires a click. The `length - 1` bound excludes the just-appended
|
||||||
|
// newest, so a fresh toast is never the one dropped — if everything older
|
||||||
|
// is sticky the cap simply stretches rather than eating the new notice.
|
||||||
|
for (let i = 0; i < next.length - 1 && next.length > MAX_TOASTS; i += 1) {
|
||||||
|
if (!next[i].sticky) {
|
||||||
|
next.splice(i, 1);
|
||||||
|
i -= 1;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
set(baseAtom, next);
|
||||||
},
|
},
|
||||||
);
|
);
|
||||||
|
|
||||||
|
|||||||
@@ -7,7 +7,9 @@ import {
|
|||||||
factoryRoomIdByUnreadCount,
|
factoryRoomIdByUnreadCount,
|
||||||
factoryRoomIdByActivity,
|
factoryRoomIdByActivity,
|
||||||
factoryRoomIdByAtoZ,
|
factoryRoomIdByAtoZ,
|
||||||
|
factoryRoomIdByUnread,
|
||||||
} from './sort';
|
} from './sort';
|
||||||
|
import type { Unread } from '../../types/matrix/room';
|
||||||
|
|
||||||
test('byTsOldToNew sorts ascending by timestamp', () => {
|
test('byTsOldToNew sorts ascending by timestamp', () => {
|
||||||
assert.ok(byTsOldToNew(1, 2) < 0);
|
assert.ok(byTsOldToNew(1, 2) < 0);
|
||||||
@@ -45,6 +47,40 @@ test('factoryRoomIdByActivity sorts most-recently-active first', () => {
|
|||||||
assert.deepEqual(['missing', 'new'].sort(cmp), ['new', 'missing']);
|
assert.deepEqual(['missing', 'new'].sort(cmp), ['new', 'missing']);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
test('factoryRoomIdByUnread: unread first, by count, then activity for ties', () => {
|
||||||
|
const ts: Record<string, number> = { a: 100, b: 300, c: 200, d: 400 };
|
||||||
|
const mx = {
|
||||||
|
getRoom: (id: string) => (id in ts ? { getLastActiveTimestamp: () => ts[id] } : null),
|
||||||
|
} as unknown as MatrixClient;
|
||||||
|
const mkUnread = (total: number): Unread => ({ total, highlight: 0, from: null });
|
||||||
|
// a: 2 unread, b: 5 unread, c: read, d: read
|
||||||
|
const roomToUnread = new Map<string, Unread>([
|
||||||
|
['a', mkUnread(2)],
|
||||||
|
['b', mkUnread(5)],
|
||||||
|
['c', mkUnread(0)],
|
||||||
|
['d', mkUnread(0)],
|
||||||
|
]);
|
||||||
|
const cmp = factoryRoomIdByUnread(roomToUnread, mx);
|
||||||
|
// b (5) and a (2) lead by unread; then the read tail c/d breaks by activity
|
||||||
|
// (d @400 more recent than c @200) rather than arbitrary order.
|
||||||
|
assert.deepEqual(['a', 'b', 'c', 'd'].sort(cmp), ['b', 'a', 'd', 'c']);
|
||||||
|
});
|
||||||
|
|
||||||
|
test('factoryRoomIdByUnread: equal unread counts break by activity', () => {
|
||||||
|
const ts: Record<string, number> = { x: 100, y: 500 };
|
||||||
|
const mx = {
|
||||||
|
getRoom: (id: string) => ({ getLastActiveTimestamp: () => ts[id] ?? 0 }),
|
||||||
|
} as unknown as MatrixClient;
|
||||||
|
const mkUnread = (total: number): Unread => ({ total, highlight: 0, from: null });
|
||||||
|
const roomToUnread = new Map<string, Unread>([
|
||||||
|
['x', mkUnread(3)],
|
||||||
|
['y', mkUnread(3)],
|
||||||
|
]);
|
||||||
|
const cmp = factoryRoomIdByUnread(roomToUnread, mx);
|
||||||
|
// Same unread count → y (more recent) before x.
|
||||||
|
assert.deepEqual(['x', 'y'].sort(cmp), ['y', 'x']);
|
||||||
|
});
|
||||||
|
|
||||||
test('factoryRoomIdByAtoZ sorts case-insensitively and ignores leading #', () => {
|
test('factoryRoomIdByAtoZ sorts case-insensitively and ignores leading #', () => {
|
||||||
const names: Record<string, string> = { a: 'Banana', b: 'apple', c: '#Cherry' };
|
const names: Record<string, string> = { a: 'Banana', b: 'apple', c: '#Cherry' };
|
||||||
const mx = {
|
const mx = {
|
||||||
|
|||||||
@@ -1,4 +1,5 @@
|
|||||||
import { MatrixClient } from 'matrix-js-sdk';
|
import { MatrixClient } from 'matrix-js-sdk';
|
||||||
|
import { Unread } from '../../types/matrix/room';
|
||||||
|
|
||||||
export type SortFunc<T> = (a: T, b: T) => number;
|
export type SortFunc<T> = (a: T, b: T) => number;
|
||||||
|
|
||||||
@@ -42,6 +43,26 @@ export const factoryRoomIdByUnreadCount =
|
|||||||
return bT - aT;
|
return bT - aT;
|
||||||
};
|
};
|
||||||
|
|
||||||
|
// "Unread First": rooms with unread sort before those without, then by unread
|
||||||
|
// count desc, then — crucially for the large all-read tail where counts tie —
|
||||||
|
// by recent activity, so it isn't left in arbitrary order.
|
||||||
|
export const factoryRoomIdByUnread = (
|
||||||
|
roomToUnread: Map<string, Unread>,
|
||||||
|
mx: MatrixClient,
|
||||||
|
): SortFunc<string> => {
|
||||||
|
const byActivity = factoryRoomIdByActivity(mx);
|
||||||
|
return (a, b) => {
|
||||||
|
const aUnread = roomToUnread.get(a);
|
||||||
|
const bUnread = roomToUnread.get(b);
|
||||||
|
const aHas = (aUnread?.total ?? 0) > 0;
|
||||||
|
const bHas = (bUnread?.total ?? 0) > 0;
|
||||||
|
if (aHas !== bHas) return aHas ? -1 : 1;
|
||||||
|
const byCount = (bUnread?.total ?? 0) - (aUnread?.total ?? 0);
|
||||||
|
if (byCount !== 0) return byCount;
|
||||||
|
return byActivity(a, b);
|
||||||
|
};
|
||||||
|
};
|
||||||
|
|
||||||
export const byTsOldToNew: SortFunc<number> = (a, b) => a - b;
|
export const byTsOldToNew: SortFunc<number> = (a, b) => a - b;
|
||||||
|
|
||||||
export const byOrderKey: SortFunc<string | undefined> = (a, b) => {
|
export const byOrderKey: SortFunc<string | undefined> = (a, b) => {
|
||||||
|
|||||||
Reference in New Issue
Block a user