feat(bookmarks): add message bookmarks (MSC4438)#601
Draft
Just-Insane wants to merge 18 commits intoSableClient:devfrom
Draft
feat(bookmarks): add message bookmarks (MSC4438)#601Just-Insane wants to merge 18 commits intoSableClient:devfrom
Just-Insane wants to merge 18 commits intoSableClient:devfrom
Conversation
dozro
reviewed
Mar 30, 2026
dozro
requested changes
Mar 30, 2026
Just-Insane
force-pushed
the
feat/message-bookmarks
branch
from
396ee68 to
2a9fa52
Compare
- Mount BookmarksFeature in ClientNonUIFeatures so useInitBookmarks() is called; bookmarkListAtom was never populated, causing the viewer to always show "No Bookmarks Yet" even with data on the server. - Fix removeBookmark to tombstone any existing item event regardless of whether it passes validation, so malformed/orphan bmk_ items cannot be resurrected by orphan recovery in listBookmarks. - Add regression tests: tombstoning malformed item, idempotent tombstoning.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds MSC4438 “message bookmarks” to the client by persisting a per-user bookmark index + items in Matrix account data, wiring startup sync + UI entry points, and gating rollout behind both an operator experiment flag and a per-user experimental toggle.
Changes:
- Introduces bookmark domain/repository layers (account-data types, CRUD, validation, orphan/tombstone handling) plus Jotai state + init hook to keep local cache in sync.
- Adds Bookmarks UI (sidebar entries, routes, list/filter/group viewer, remove/restore flows) and message context-menu integration.
- Adds experiment framework support in client config, settings toggle UI, and unit tests for domain/repository/state.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/matrix/accountData.ts | Adds MSC4438 account-data event types/prefixes. |
| src/app/state/settings.ts | Adds per-user experimental toggle (enableMessageBookmarks). |
| src/app/state/bookmarks.ts | Adds bookmark Jotai atoms + derived ID Set. |
| src/app/state/bookmarks.test.ts | Tests derived bookmark ID Set atom behavior. |
| src/app/pages/Router.tsx | Adds Bookmarks route under Home and Inbox. |
| src/app/pages/pathUtils.ts | Adds helpers for Home/Inbox bookmarks paths. |
| src/app/pages/paths.ts | Adds bookmarks/ path segment + concrete paths. |
| src/app/pages/client/inbox/Inbox.tsx | Adds Inbox sidebar nav item gated by experiment/setting. |
| src/app/pages/client/home/Home.tsx | Adds Home sidebar nav item gated by experiment/setting. |
| src/app/pages/client/ClientNonUIFeatures.tsx | Mounts bookmark init hook; adjusts in-app audio gating. |
| src/app/pages/client/bookmarks/index.ts | Re-exports Bookmarks page module. |
| src/app/pages/client/bookmarks/BookmarksList.tsx | Implements Bookmarks list UI (filter/group/jump/remove/restore). |
| src/app/hooks/useClientConfig.ts | Adds experiments config typing + deterministic assignment helpers/hook. |
| src/app/hooks/router/useInbox.ts | Adds selection matcher for Inbox Bookmarks route. |
| src/app/hooks/router/useHomeSelected.ts | Adds selection matcher for Home Bookmarks route. |
| src/app/features/settings/experimental/MSC4438MessageBookmarks.tsx | Adds experimental settings tile for bookmarks toggle. |
| src/app/features/settings/experimental/Experimental.tsx | Wires bookmarks toggle into Experimental settings section. |
| src/app/features/room/message/Message.tsx | Adds message context-menu item to add/remove bookmark. |
| src/app/features/bookmarks/useInitBookmarks.ts | Initializes and keeps bookmark atoms synced with account data. |
| src/app/features/bookmarks/useBookmarks.ts | Adds read + action hooks for bookmarks. |
| src/app/features/bookmarks/bookmarkRepository.ts | Implements MSC4438 account-data CRUD + listing/orphan recovery. |
| src/app/features/bookmarks/bookmarkRepository.test.ts | Adds repository unit tests. |
| src/app/features/bookmarks/bookmarkDomain.ts | Defines bookmark types, ID algorithm, validators, item builder. |
| src/app/features/bookmarks/bookmarkDomain.test.ts | Adds domain unit tests + reference vectors. |
| config.json | Adds operator-controlled experiments.messageBookmarks flag. |
| .changeset/message-bookmarks.md | Adds release note entry for message bookmarks feature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+563
to
+578
| {groupedByRoom.map((group, i) => ( | ||
| <> | ||
| {i > 0 && <Line variant="Background" size="300" />} | ||
| <BookmarkResultGroup | ||
| key={group.roomId} | ||
| roomId={group.roomId} | ||
| roomName={group.roomName} | ||
| items={group.items} | ||
| highlight={filterTerm} | ||
| onJump={handleJump} | ||
| onRemove={setRemovingItem} | ||
| hour24Clock={hour24Clock} | ||
| dateFormatString={dateFormatString} | ||
| /> | ||
| </> | ||
| ))} |
There was a problem hiding this comment.
groupedByRoom.map(...) returns an un-keyed fragment (<>...</>) containing multiple siblings. React will warn that each child in a list needs a unique key because the fragment itself is the list element; the key on BookmarkResultGroup doesn’t satisfy that. Wrap the block in <Fragment key={...}> (or move the key onto the fragment) so the list element is keyed.
Comment on lines
+224
to
+241
| const mx = useMatrixClient(); | ||
| const bookmarksExperiment = useExperimentVariant('messageBookmarks', mx.getUserId() ?? undefined); | ||
| const [enableMessageBookmarks] = useSetting(settingsAtom, 'enableMessageBookmarks'); | ||
| const eventId = mEvent.getId() ?? ''; | ||
| const isBookmarked = useIsBookmarked(room.roomId, eventId); | ||
| const { add, remove } = useBookmarkActions(); | ||
|
|
||
| if (!bookmarksExperiment.inExperiment && !enableMessageBookmarks) return null; | ||
|
|
||
| const handleClick = async () => { | ||
| if (isBookmarked) { | ||
| await remove(computeBookmarkId(room.roomId, eventId)); | ||
| } else { | ||
| const item = createBookmarkItem(room, mEvent); | ||
| if (item) await add(item); | ||
| } | ||
| onClose?.(); | ||
| }; |
There was a problem hiding this comment.
MessageBookmarkItem doesn’t guard against mEvent.getId() being missing. With eventId defaulting to '', useIsBookmarked()/computeBookmarkId() can compute a bogus ID (and in the worst case remove the wrong bookmark due to 32-bit hash collisions), and the “Bookmark Message” action can become a silent no-op because createBookmarkItem() returns undefined while the menu still closes. Consider returning null (or disabling the item) when !mEvent.getId().
Comment on lines
+77
to
+82
| * 1. Remove the ID from the index. | ||
| * 2. Soft-delete the item (set deleted: true). | ||
| * | ||
| * Account data events cannot be deleted from the server, so soft-deletion is | ||
| * used. Other clients that encounter the item event can see it is explicitly | ||
| * removed. |
There was a problem hiding this comment.
The doc comment for removeBookmark says the MSC4438 removal order is “remove from index” then “soft-delete the item”, but the implementation intentionally tombstones the item first and updates the index second (to mirror addBookmark’s item-first ordering and avoid transient orphan recovery). Please update the comment to match the actual ordering so future changes don’t “fix” this back accidentally.
Suggested change
| * 1. Remove the ID from the index. | |
| * 2. Soft-delete the item (set deleted: true). | |
| * | |
| * Account data events cannot be deleted from the server, so soft-deletion is | |
| * used. Other clients that encounter the item event can see it is explicitly | |
| * removed. | |
| * 1. Soft-delete the item first (set deleted: true). | |
| * 2. Remove the ID from the index. | |
| * 3. Increment revision and update timestamp. | |
| * 4. Write the updated index. | |
| * | |
| * Account data events cannot be deleted from the server, so soft-deletion is | |
| * used. This implementation intentionally tombstones the item before updating | |
| * the index to mirror addBookmark()'s item-first ordering and avoid transient | |
| * orphan recovery/resurrection if a removal only partially completes. |
…e() optimistic update - useInitBookmarks: also trigger loadBookmarks on bookmark item events (prefix org.matrix.msc4438.bookmark.*), not just the index event. Fixes cross-device sync not updating the UI when sliding sync delivers individual item events. - useBookmarks: remove() now moves the item to bookmarkDeletedListAtom optimistically so the Recently Removed section updates immediately. - Added regression tests for both fixes.
…okmark doc comment
- Wrap grouped bookmark list items in <Fragment key={}> instead of un-keyed <>
- Return null from MessageBookmarkItem when mEvent.getId() is undefined
- Update removeBookmark doc comment to match item-first deletion ordering
Mirrors the fix already applied in useNotificationJumper: when a room belongs to both the direct-message list and a space, prefer the /direct route over the space route. Previously useRoomNavigate checked orphan space parents first, which caused bookmark jumps and room-nav clicks on DMs-in-spaces to open the room via the space path instead of the direct path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…croll, fix eventId drag-to-bottom, increase list timeline limit - useTimelineSync: change auto-scroll recovery useEffect → useLayoutEffect to prevent one-frame flash after timeline reset - useTimelineSync: remove premature scrollToBottom from useLiveTimelineRefresh (operated on pre-commit DOM with stale scrollSize) - useTimelineSync: remove scrollToBottom + eventsLengthRef suppression from useLiveEventArrive; let useLayoutEffect handle scroll after React commits - RoomTimeline: init atBottomState to false when eventId is set, and reset it in the eventId useEffect, so auto-scroll doesn't drag to bottom on bookmark nav - RoomTimeline: change instant scrollToBottom to use scrollToIndex instead of scrollTo(scrollSize) — works correctly regardless of VList measurement state - slidingSync: increase DEFAULT_LIST_TIMELINE_LIMIT 1→3 to reduce empty previews when recent events are reactions/edits/state Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Restore scrollToBottom call in useLiveEventArrive with instant/smooth based on sender, add back eventsLengthRef and lastScrolledAt suppression, restore scrollToBottom in useLiveTimelineRefresh when wasAtBottom, and revert instant scrollToBottom to scrollTo(scrollSize) matching upstream. The previous changes removed all scroll calls from event arrival handlers and relied solely on the useLayoutEffect auto-scroll recovery, which has timing issues with VList measurement. Upstream's pattern of scrolling in the event handler and suppressing the effect works reliably. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Remove behavior parameter from scrollToBottom — always use scrollTo(scrollSize) matching upstream. The smooth scrollToIndex was scrolling to stale lastIndex (before new item measured), leaving new messages below the fold. - Revert auto-scroll recovery from useLayoutEffect back to useEffect (matches upstream). useLayoutEffect fires before VList measures new items and before setAtBottom(false) in eventId effect. - Remove stale scrollCacheForRoomRef that referenced missing imports. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
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.
Description
Implements MSC4438 message bookmarks — lets users save any room event to a personal bookmark list stored in Matrix account data.
Commits in this PR:
feat(bookmarks): add message bookmarks (MSC4438)— full feature implementation:BookmarkItemContent/BookmarkIndextypes,bookmarkRepository.tsCRUD operations (addBookmark,removeBookmark,listBookmarks,useInitBookmarks), auseBookmarksReact hook, context menu integration, and the bookmarks viewer panel.test(bookmarks): add unit tests for MSC4438 bookmark domain and repository— covers the core repository logic and hook behaviour.fix(bookmarks): add missing focusId to MSC4438 settings SettingTile— prevents a React key-prop warning that appeared in the Settings modal.fix(bookmarks): soft-delete item before updating index in removeBookmark— corrects the write order inremoveBookmarkto mirror the item-first ordering ofaddBookmark, preventing orphan-recovery inlistBookmarksfrom transiently resurrecring a bookmark between the two writes.fix(bookmarks): wire useInitBookmarks and fix orphan tombstoning— mountsBookmarksFeatureinClientNonUIFeaturessouseInitBookmarks()is actually called on startup (the bookmark list was never populated without this). Also replacesreadItem()(which validates and may return null for malformed items) with a direct account-data read in the tombstone path, so malformed or partially-written bookmark items can always be fully deleted. Includes regression tests for both cases.Fixes #
Type of change
Checklist:
AI disclosure:
The
bookmarkRepository.tsCRUD helpers and theuseInitBookmarkshook skeleton were drafted with AI assistance and reviewed against the MSC4438 spec and Sable's existing account-data patterns. The tombstone fix logic and theClientNonUIFeatureswiring were written manually after tracing the runtime call graph.