feat(bookmarks): add message bookmarks (MSC4438)#601
Draft
Just-Insane wants to merge 223 commits intoSableClient:devfrom
Draft
feat(bookmarks): add message bookmarks (MSC4438)#601Just-Insane wants to merge 223 commits intoSableClient:devfrom
Just-Insane wants to merge 223 commits intoSableClient:devfrom
Conversation
Updated configuration for homeserver and push notifications.
Pass VITE_SENTRY_DSN, VITE_SENTRY_ENVIRONMENT, VITE_APP_VERSION, SENTRY_AUTH_TOKEN, SENTRY_ORG, and SENTRY_PROJECT to the build step so that the Docker image build (dev, integration, and release tags) includes Sentry instrumentation and source map uploads, matching the Cloudflare deploy workflow. Environment mapping: - dev branch / release tags → production - integration branch / manual dispatch without tag → preview
- Adds pre-push hook that runs typecheck, lint, and format checks - Blocks pushes that would fail CI - Includes install script for easy setup - Tracked on personal/config to persist across dev pulls
Updated instructions for pull requests and feature flags.
Added instructions for syncing branches before creating a new branch.
Updated wording for clarity and consistency in instructions.
…elay When navigator.serviceWorker.controller is null — which happens when a window is opened by a notification tap before the SW has claimed it, or transiently during a SW update cycle — postMessage calls were silently dropped. This caused the decryption-result reply to never reach the SW, so the 5-second timeout fired and the notification either showed "Encrypted message" or was skipped entirely. Subsequent notifications after the first tended to fail because the tab that received the push was often the freshly-opened uncontrolled window. The same null-controller bug affected enablePushNotifications / disablePushNotifications: if the togglePush message was dropped, the homeserver pusher state was never updated, so no further pushes arrived. Fix: mirror the pattern already used by SyncNotificationSettingsWithSW and setNotificationSettings in the same file — send to both controller (fast path, usually correct) and registration.active (belt-and-suspenders via ready.then). Double delivery is safe: the SW's decryptionPendingMap deduplicates pushDecryptResult, and duplicate pusher SET/DELETE requests to the homeserver are idempotent. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Duplicate const introduced during integration merge causing build failure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The integration rebuild partially included perf/timeline-rendering — it
got the 'align initial room-fill thresholds' and 'stabilize bottom pin'
commits but missed the 'feat(timeline): restore room cache by event anchor'
commit chain. That commit introduced several refs and a utility module that
the later commits depend on, causing a ReferenceError crash on room open:
readyBlockedByPaginationRef is not defined (already restored earlier)
saveRoomScrollStateRef / currentScrollFingerprintRef (type-only — no-op without wiring)
prevScrollSizeRef (content-grow detection in handleVListScroll)
isReadyRef (scroll-event guard during init)
RoomScrollFingerprint type (return type of buildRoomScrollFingerprint)
Restore:
- src/app/utils/roomScrollCache.ts (deleted during merge)
- import { roomScrollCache, RoomScrollCache, RoomScrollFingerprint,
RoomScrollPosition } from roomScrollCache
- const prevScrollSizeRef = useRef(0)
- const isReadyRef = useRef(false); isReadyRef.current = isReady
- const saveRoomScrollStateRef / currentScrollFingerprintRef
The full scroll-position restore wiring (roomScrollCache.save/load,
saveRoomScrollState callback, restoreRoomScrollPosition) is not yet wired
up — the saveRoomScrollStateRef.current stays undefined so those call sites
are no-ops — but all runtime references are now defined and the app no
longer crashes on room open.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- RoomTimeline: remove unused TIMELINE_ANCHOR_SELECTOR, buildRoomScrollFingerprint, currentScrollFingerprintRef (incomplete perf/timeline wiring stubs) - RoomTimeline: narrow import to RoomScrollCache only (RoomScrollFingerprint unused) - sw-session: remove void operator to satisfy no-void rule - Auto-fixed prettier formatting in PushNotifications, ClientNonUIFeatures, useProcessedTimeline, BackgroundNotifications Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add Discord-style message grouping where consecutive messages from the same sender within a configurable time window collapse the sender header. - Add messageGroupingThreshold to Settings type (default: 2 min) - Replace hardcoded 2-min threshold in useProcessedTimeline with the setting - Wire useSetting in RoomTimeline and pass to useProcessedTimeline - Add MessageGrouping component in Experimental settings with 5 options: 2 min (default), 5 min, 15 min (Discord-style), 30 min, 60 min Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> # Conflicts: # src/app/features/settings/experimental/Experimental.tsx
Previously skipWaiting() was called automatically in the install event,
causing every new deployment to immediately activate the new SW mid-session.
When users then navigated to a lazy-loaded route, the old chunk hash no
longer existed on the server → 404 → chunk-error handler → silent reload.
This presented as the app 'randomly restarting' after deployments.
Changes:
- Remove auto-skipWaiting() from SW install handler
- Add SKIP_WAITING_AND_CLAIM message handler — the update prompt already
sends this message; now the SW acts on it with self.skipWaiting()
- Deduplicate the double navigator.serviceWorker.register() call in
index.tsx (was registering twice with different options in dev mode)
- Move sendSessionToSW into the single registration .then() handler so
the session is sent as soon as the registration resolves, and keep the
.ready fallback for the case where registration already settled
The new SW will now sit in 'waiting' state until the user confirms the
update prompt ('A new version of the app is available. Refresh?').
Existing sessions are uninterrupted until then.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Previously all three room list virtualizers used `key={vItem.index}`
(position-based). When a room moves position (e.g. a DM jumps to the
top after a new message), React reuses the component at that index
rather than remounting it. This caused `useRoomHasUnread`'s lazy
`useState` initializer to retain stale state from the previous room at
that index — resulting in the unread dot appearing on the wrong room
until the next Timeline/Receipt event fired.
Fix: add `getItemKey` to each `useVirtualizer` config so TanStack
Virtual assigns stable room-ID-based keys, then use `vItem.key` on
`<VirtualTile>`. React now correctly unmounts/remounts when a different
room occupies a position, so all per-room state is fresh.
- Direct.tsx: getItemKey = sortedDirects[index]
- Home.tsx: getItemKey = sortedRooms[index]
- Space.tsx: getItemKey = hierarchy[index]?.roomId ?? index
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Matrix presence is per-user on the server, not per-device. When an idle
device sends `setPresence({ presence: 'unavailable' })`, the shared
server state changes for all clients. The active device's PresenceFeature
only re-sends its state when `autoIdled`, `presenceMode`, or
`sendPresence` changes — none of which fire on the active device, so the
idle device permanently 'wins' until the user switches tabs or interacts.
Fix: add a 2-minute heartbeat that re-asserts `{ presence: 'online' }`
Within one heartbeat cycle the active device wins back the server state.
The heartbeat is idle-free (stops when autoIdled or mode changes), so it
doesn't fight against intentional DND/offline/idle changes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…r errors Previously, the pusher was only re-registered with Sygnal on visibility changes (backgrounding/foregrounding). If the browser rotated the push subscription endpoint while the app was closed, there was a window from app open until the first background event where pushes would silently fail (Sygnal 410s on the stale endpoint). Also, the togglePusher Promise was fire-and-forget — unhandled rejections from enablePushNotifications (e.g. pushManager.subscribe() failure due to transient network error) were silently swallowed, leaving the pusher in an unknown state with no log trail. Fixes: - Call togglePusher once immediately on mount with the current visibility state so the endpoint is always current on app startup - Wrap all togglePusher calls with .catch(debugLog.warn) so failures are surfaced in the debug log and Sentry Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… zero FaviconUpdater's badge-clearing logic only runs inside the roomToUnread useEffect. If roomToUnread reached highlightTotal=0 while the app was hidden (background sync from another device), document.visibilityState was not 'visible' so clearAppBadge was skipped. When the user then foregrounds the app roomToUnread hasn't changed so the effect doesn't re-run and the stale badge stays set. Fix: add a visibilitychange listener that clears the badge on foreground whenever highlightTotalRef is 0. A ref tracks the latest value so the listener doesn't need to close over the roomToUnread effect. Also suppress the pre-existing no-console lint warning on the push relay error path with an inline disable comment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two fallback paths in getUnreadInfo were accidentally dropped during a checkpoint commit (2eaefb19d): 1. PushProcessor fallback: when SDK total/highlight are both 0 but roomHaveUnread() confirms there ARE unread events (stale SDK counters), walk the live timeline with PushProcessor to compute real counts. 2. No-receipt sliding sync fallback: rooms that have never been visited in sliding sync have no read receipt, making SDK counts unreliable. If the timeline has activity from other users, return total=1 (or the existing SDK counts when present) so the dot badge appears. These fallbacks are essential for non-DM rooms: DMs benefit from shouldForceDMHighlight which forces highlight=total, but regular rooms rely entirely on these paths when SDK counts are zero or stale. Restores parity with the dev branch. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Remove the 24h TTL check from loadPersistedSession(). Sessions cached before the persistedAt field was added (age=Infinity) were being incorrectly rejected, causing every push notification to fall back to the generic 'New Message' text until the user opened the app again. Matrix access tokens are long-lived; if a token is truly revoked the downstream 401 handling in handleMinimalPushPayload provides the graceful fallback already. - Wrap event.data.json() in a try/catch with a 'New Message' fallback notification. A malformed push payload previously caused an unhandled rejection in onPushNotification, which could silently drop the push event on iOS. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
On iOS PWA, visibilityState can get stuck at 'visible' after the user
opens the app via a notification tap and then backgrounds it again.
This caused all subsequent push notifications to be silently suppressed
("first notification works, second and beyond don't").
The fix: require BOTH visibilityState === 'visible' AND client.focused.
The focused property transitions to false immediately when the user
presses the home button (before visibilitychange fires), so it is the
reliable signal that the user is actually looking at the app.
Also log focused state in the existing debug output.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
When a user opens a room via notification tap and loadEventTimeline encounters an error (e.g. network not yet ready after iOS app resume), the onError handler resets to the live timeline but never called setIsReady(true). This left the timeline permanently invisible (opacity: 0), producing a blank white screen. Add onJumpError callback to UseTimelineSyncOptions. RoomTimeline passes handleJumpError which calls setIsReady(true). The error handler in useTimelineSync now calls onJumpError() after restoring the live timeline and scrolling to bottom, so the room becomes visible. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
After iOS kills a backgrounded PWA and the app cold-starts, SyncStatus was showing a Connecting... banner for up to 20 seconds after the loading screen disappeared. This is because the banner fired on any transition where previous !== SyncState.Syncing, including the initial null→PREPARED→SYNCING cold-start sequence. With sliding sync the second long-poll can take up to 20 s (the poll timeout) when there are no new messages, keeping the banner visible the entire time even though the app is fully loaded and usable. Fix: only show Connecting... when recovering from an actual disconnect (previous === Reconnecting or previous === Error). The loading screen already covers the initial connection; the banner is only meaningful when we are re-connecting after a network disruption. State transitions: - null → PREPARED: no banner (loading screen handles it) ✓ - RECONNECTING → PREPARED/SYNCING: shows banner ✓ - ERROR → PREPARED/SYNCING: shows banner ✓ - SYNCING → SYNCING: no banner ✓ Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The SW has two paths where it suppresses OS notifications when the app appears visible: 1. hasVisibleClient (already fixed): requires both visibilityState === 'visible' AND client.focused from clients.matchAll(). 2. Decrypt relay result (this commit): after requestDecryptionFromClient(), the code checked only result?.visibilityState === 'visible' to decide whether the in-app UI was already showing the message. On iOS PWA, document.visibilityState can get stuck at 'visible' after the user backgrounds the app because visibilitychange doesn't always fire. document.hasFocus() is updated by the browser process immediately when the OS removes window focus (home button press), so it is the reliable signal. Add appFocused: document.hasFocus() to both pushDecryptResult reply payloads in ClientNonUIFeatures.tsx. In sw.ts, require BOTH visibilityState === 'visible' AND appFocused === true before skipping the OS notification, mirroring the same dual-guard logic in hasVisibleClient. This closes the second suppression path that was silently dropping background push notifications on iOS after the user had previously opened the app from a notification tap. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The previous change narrowed the 'Connecting...' banner to only show on reconnect/error. Reverting to upstream's condition: stateData.previous !== SyncState.Syncing 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.