feat: implement bookmarks page with list management and deep-link scroll targeting #196

Open
bhavishy2801 wants to merge 5 commits from bhavishy2801/flotilla:feat-bookmarks into dev
Contributor

Summary

Implements the Bookmarks feature end-to-end. Users can now save messages to
bookmark lists, manage those lists, and browse saved content from a dedicated /bookmarks
page in the primary nav.
Addresses #19.


What's Changed

Bookmarks Page (/bookmarks)

  • New full-page view with a sidebar listing all bookmark lists (kind 10003 and 30003)
  • Grid layout for saved items — each card shows author avatar, display name, timestamp,
    content preview, and media (image/video) when present
  • Search bar to filter within the selected list
  • Content-type filter popover (All / Image / Video / Text)
  • Right-click context menu on list items for rename and delete
  • Per-card overflow menu with open, copy link, add to Saved Items, and remove actions
  • "Saved Items" (10003) is always present as the default list and cannot be deleted or renamed

Bookmark List Picker (BookmarkListPicker.svelte)

  • Reusable modal for selecting which list to save an event to, or managing lists standalone
  • Inline list creation and deletion
  • Correctly filters out deleted lists using NIP-09 delete events

Commands (commands.ts)

Added four new exported functions backed by NIP-51:

  • createBookmarkList — creates a new kind 30003 list
  • addEventBookmark — adds an event to a list (defaults to 10003)
  • removeEventBookmark — removes an event from a list
  • deleteBookmarkList — publishes a NIP-09 delete for a 30003 list
  • renameBookmarkList — updates the d tag and cleans up the old list

Bookmark Button in Message Menus

  • Added a bookmark action to ChatMessageMenu, ChatMessageMenuMobile, RoomItemMenu,
    and RoomItemMenuMobile — opens the BookmarkListPicker modal

Deep-Link Scroll & Highlight

  • Chat and space pages now accept an e query param (event ID) in addition to at
    (timestamp) for precise message targeting
  • Scroll-to-target now uses tick() before firing to ensure the DOM is ready, replacing
    the previous requestAnimationFrame approach
  • Target message gets a brief brightness highlight animation on arrival
  • userHasScrolled resets when at or e params change, so navigating to a bookmarked
    message always scrolls correctly
  • Interaction detection (markInteraction) prevents programmatic scrolls from locking
    userHasScrolled prematurely — only real user gestures (wheel, touch, pointer drag,
    keyboard navigation) count

Navigation & Routing

  • Bookmarks nav item added to PrimaryNav (both desktop and mobile layouts)
  • /bookmarks title registered in title.ts
  • makeChatPath now accepts an optional event to append ?at=...&e=... for direct
    linking from bookmarks
  • makeMessagePath and getEventPath updated to include the e param alongside at
  • goToSpace navigation calls no longer use replaceState, fixing back-button behaviour
  • MESSAGE_KINDS constant extracted to state.ts and used consistently across space
    chat filters

Testing

  • Save a message from chat and space room menus
  • Create, rename, and delete a custom bookmark list
  • Navigate to a bookmarked message and verify scroll + highlight
  • Filter bookmarks by content type (image / video / text)
  • Search within a list
  • Verify "Saved Items" cannot be deleted or renamed
  • Confirm back-button works correctly after navigating to a space
## Summary Implements the Bookmarks feature end-to-end. Users can now save messages to bookmark lists, manage those lists, and browse saved content from a dedicated `/bookmarks` page in the primary nav. Addresses #19. --- ## What's Changed ### Bookmarks Page (`/bookmarks`) - New full-page view with a sidebar listing all bookmark lists (kind `10003` and `30003`) - Grid layout for saved items — each card shows author avatar, display name, timestamp, content preview, and media (image/video) when present - Search bar to filter within the selected list - Content-type filter popover (All / Image / Video / Text) - Right-click context menu on list items for rename and delete - Per-card overflow menu with open, copy link, add to Saved Items, and remove actions - "Saved Items" (`10003`) is always present as the default list and cannot be deleted or renamed ### Bookmark List Picker (`BookmarkListPicker.svelte`) - Reusable modal for selecting which list to save an event to, or managing lists standalone - Inline list creation and deletion - Correctly filters out deleted lists using NIP-09 delete events ### Commands (`commands.ts`) Added four new exported functions backed by NIP-51: - `createBookmarkList` — creates a new kind `30003` list - `addEventBookmark` — adds an event to a list (defaults to `10003`) - `removeEventBookmark` — removes an event from a list - `deleteBookmarkList` — publishes a NIP-09 delete for a `30003` list - `renameBookmarkList` — updates the `d` tag and cleans up the old list ### Bookmark Button in Message Menus - Added a bookmark action to `ChatMessageMenu`, `ChatMessageMenuMobile`, `RoomItemMenu`, and `RoomItemMenuMobile` — opens the `BookmarkListPicker` modal ### Deep-Link Scroll & Highlight - Chat and space pages now accept an `e` query param (event ID) in addition to `at` (timestamp) for precise message targeting - Scroll-to-target now uses `tick()` before firing to ensure the DOM is ready, replacing the previous `requestAnimationFrame` approach - Target message gets a brief brightness highlight animation on arrival - `userHasScrolled` resets when `at` or `e` params change, so navigating to a bookmarked message always scrolls correctly - Interaction detection (`markInteraction`) prevents programmatic scrolls from locking `userHasScrolled` prematurely — only real user gestures (wheel, touch, pointer drag, keyboard navigation) count ### Navigation & Routing - Bookmarks nav item added to `PrimaryNav` (both desktop and mobile layouts) - `/bookmarks` title registered in `title.ts` - `makeChatPath` now accepts an optional event to append `?at=...&e=...` for direct linking from bookmarks - `makeMessagePath` and `getEventPath` updated to include the `e` param alongside `at` - `goToSpace` navigation calls no longer use `replaceState`, fixing back-button behaviour - `MESSAGE_KINDS` constant extracted to `state.ts` and used consistently across space chat filters --- ## Testing - [x] Save a message from chat and space room menus - [x] Create, rename, and delete a custom bookmark list - [x] Navigate to a bookmarked message and verify scroll + highlight - [x] Filter bookmarks by content type (image / video / text) - [x] Search within a list - [x] Verify "Saved Items" cannot be deleted or renamed - [x] Confirm back-button works correctly after navigating to a space
bhavishy2801 added 1 commit 2026-04-13 20:47:24 +00:00
Author
Contributor

Hi @hodlbod. I've implemented the bookmarks section and related features, as per the approved design made by @Conceptionify. Kindly review the PR :)

Hi @hodlbod. I've implemented the bookmarks section and related features, as per the approved design made by @Conceptionify. Kindly review the PR :)
bhavishy2801 added 1 commit 2026-04-13 21:19:49 +00:00
hodlbod requested changes 2026-04-13 21:53:20 +00:00
hodlbod left a comment
Owner

A few first things before I get into detail:

  • Please go ahead and split the bookmarks page into several different components, it's far too large to follow right now.
  • Try to re-use components where possible, in particular NoteContent and NoteContentMinimal might help with a lot of the parsing/rendering logic (although I haven't tested the rendering behavior thoroughly, maybe it's worth being its own component).
  • This PR unfortunately runs afoul of a major refactor I merged late last week, in which non-chat messages are no longer rendered directly in chat, but are quoted in messages. There's no direct conflict, but there is some awkwardness in (for example) RoomItem, in which it's a little ambiguous whether the message itself or the quoted content is the thing the user wants to interact with. Anyway, I don't think this necessitates any changes in particular, just something to be aware of.
  • There should be a "bookmark" menu item on threads/polls/classifieds/etc in the item menu.
  • The new event id pinning logic that complements at makes sense, but it changes a lot of the scrolling logic in chat which makes me nervous. Can you see if you can leave the scrolling logic alone there and just add the event highlighting bit?
  • Instead of passing around a list key as its own thing, are you able to pass around an Address (there's a helper in welshman/util)?
  • I've left some comments on the commands stuff, refer to other commands and the code in welshman for more details on how this should all work.
A few first things before I get into detail: - Please go ahead and split the bookmarks page into several different components, it's far too large to follow right now. - Try to re-use components where possible, in particular NoteContent and NoteContentMinimal might help with a lot of the parsing/rendering logic (although I haven't tested the rendering behavior thoroughly, maybe it's worth being its own component). - This PR unfortunately runs afoul of a [major refactor](https://gitea.coracle.social/coracle/flotilla/pulls/187) I merged late last week, in which non-chat messages are no longer rendered directly in chat, but are quoted in messages. There's no direct conflict, but there is some awkwardness in (for example) RoomItem, in which it's a little ambiguous whether the message itself or the quoted content is the thing the user wants to interact with. Anyway, I don't think this necessitates any changes in particular, just something to be aware of. - There should be a "bookmark" menu item on threads/polls/classifieds/etc in the item menu. - The new event id pinning logic that complements `at` makes sense, but it changes a lot of the scrolling logic in chat which makes me nervous. Can you see if you can leave the scrolling logic alone there and just add the event highlighting bit? - Instead of passing around a list key as its own thing, are you able to pass around an `Address` (there's a helper in welshman/util)? - I've left some comments on the commands stuff, refer to other commands and the code in welshman for more details on how this should all work.
@@ -0,0 +33,4 @@
const {event}: Props = $props()
const listEvents = deriveEvents([{kinds: [10003, 30003, DELETE]}])
Owner

Import these from @welshman/util

Import these from @welshman/util
Author
Contributor

Done. Now these are imported from @welshman/util

Done. Now these are imported from @welshman/util
hodlbod marked this conversation as resolved
@@ -168,2 +172,4 @@
// List updates
const BOOKMARKS = 10003
const BOOKMARK_LISTS = 30003
Owner

These can be imported from @welshman/util

These can be imported from @welshman/util
Author
Contributor

Done. Now these are imported from @welshman/util

Done. Now these are imported from @welshman/util
hodlbod marked this conversation as resolved
@@ -170,0 +197,4 @@
)
return latest ? readList(asDecryptedEvent(latest)) : makeList({kind})
}
Owner

This should go in state and should be as simple as:

export const bookmarkListsByPubkey = deriveItemsByKey({
  repository,
  eventToItem: event => readList(asDecryptedEvent(event)),
  filters: [{kinds: [BOOKMARKS]}],
  getKey: list => list.event.pubkey,
})

export const getBookmarkList = (pubkey: string) => getBookmarkList().get(pubkey)

export const loadBookmarkList = makeLoadItem(makeOutboxLoader(BOOKMARKS), getBookmarkList)

export const userBookmarkList = makeUserData(bookmarkListsByPubkey, loadBookmarkList)

See @welshman/app's user.ts file for examples.

This should go in `state` and should be as simple as: ``` export const bookmarkListsByPubkey = deriveItemsByKey({ repository, eventToItem: event => readList(asDecryptedEvent(event)), filters: [{kinds: [BOOKMARKS]}], getKey: list => list.event.pubkey, }) export const getBookmarkList = (pubkey: string) => getBookmarkList().get(pubkey) export const loadBookmarkList = makeLoadItem(makeOutboxLoader(BOOKMARKS), getBookmarkList) export const userBookmarkList = makeUserData(bookmarkListsByPubkey, loadBookmarkList) ``` See `@welshman/app`'s `user.ts` file for examples.
Owner

I think part of the problem is that you're using the same function to handle two different kinds of list. You should create separate functions for 10003 vs 30003 since they work quite differently.

I think part of the problem is that you're using the same function to handle two different kinds of list. You should create separate functions for 10003 vs 30003 since they work quite differently.
Author
Contributor

Done.

Done.
hodlbod marked this conversation as resolved
@@ -170,0 +200,4 @@
}
export const createBookmarkList = async (title: string) => {
const d = title.trim()
Owner

d should be randomly generated, not a slug of the title

`d` should be randomly generated, not a slug of the title
Author
Contributor

Fixed. d is now opaque/random, and title is stored separately.

Fixed. d is now opaque/random, and title is stored separately.
hodlbod marked this conversation as resolved
@@ -170,0 +237,4 @@
const relays = uniq([
...Router.get().FromUser().getUrls(),
...Router.get().Event(target).limit(3).getUrls(),
...getRelayTagValues(event.tags),
Owner

The bookmark list shouldn't be published to relays based on its content, just publish it to the user's outbox

The bookmark list shouldn't be published to relays based on its content, just publish it to the user's outbox
Author
Contributor

Fixed. Bookmark list updates are now published only to user outbox relays.

Fixed. Bookmark list updates are now published only to user outbox relays.
hodlbod marked this conversation as resolved
@@ -170,0 +316,4 @@
const relays = uniq([...INDEXER_RELAYS, ...getRelayTagValues(list.event.tags)])
return publishThunk({event, relays})
}
Owner

Making d tags opaque random ids will help a lot with this complexity.

Making `d` tags opaque random ids will help a lot with this complexity.
Author
Contributor

Yes, and I've fixed this. Opaque d is now the canonical identifier for custom bookmark lists, with display name in title.

Yes, and I've fixed this. Opaque d is now the canonical identifier for custom bookmark lists, with display name in title.
hodlbod marked this conversation as resolved
bhavishy2801 added 1 commit 2026-04-14 13:42:39 +00:00
bhavishy2801 added 1 commit 2026-04-14 14:15:58 +00:00
Author
Contributor

Hi @hodlbod. I've made all the necessary changes. Kindly review the PR :)

Hi @hodlbod. I've made all the necessary changes. Kindly review the PR :)
bhavishy2801 added 1 commit 2026-04-14 15:38:44 +00:00
hodlbod reviewed 2026-04-14 18:33:03 +00:00
hodlbod left a comment
Owner
  • Remove the numbered badge from the bookmarks nav item. Also, put it under the profile and above the messages icon. On mobile, it can go in the settings menu instead of in the primary nav bar.
  • Add icons to the menu items in BookmarkCard. The menu should animate, see how we do things in (for example) RoomMembers, which uses Popover.
  • When saving an item to a list, show loading/disabled state while saving.
  • Zap goals don't have a bookmark action menu item
  • There is inconsistent rendering behavior between the sidebar and the manage lists dialog. The latter correctly shows the number of items. My guess is that these lists contain p tagged items, which are filtered out and aren't displayed.
  • When deleting, the toast should show "Successfully deleted your list!" I also got an error toast once, which doesn't really make sense — if we have a list, we can delete it.

I'm going to stop here. I think you need to do more self-validation on this PR before another review. There is a lot of stuff here that doesn't really make sense, for example https://gitea.coracle.social/coracle/flotilla/pulls/196/files#issuecomment-2217. It's hard to divine boundaries for separation of responsibility in someone else's code, but please take a read through other components, forms, commands, derived stores, etc to see how these problems are already solved in the codebase before introducing new patterns.

- Remove the numbered badge from the bookmarks nav item. Also, put it under the profile and above the messages icon. On mobile, it can go in the settings menu instead of in the primary nav bar. - Add icons to the menu items in BookmarkCard. The menu should animate, see how we do things in (for example) RoomMembers, which uses `Popover`. - When saving an item to a list, show loading/disabled state while saving. - Zap goals don't have a bookmark action menu item - There is inconsistent rendering behavior between the sidebar and the manage lists dialog. The latter correctly shows the number of items. My guess is that these lists contain `p` tagged items, which are filtered out and aren't displayed. - When deleting, the toast should show "Successfully deleted your list!" I also got an error toast once, which doesn't really make sense — if we have a list, we can delete it. I'm going to stop here. I think you need to do more self-validation on this PR before another review. There is a lot of stuff here that doesn't really make sense, for example https://gitea.coracle.social/coracle/flotilla/pulls/196/files#issuecomment-2217. It's hard to divine boundaries for separation of responsibility in someone else's code, but please take a read through other components, forms, commands, derived stores, etc to see how these problems are already solved in the codebase before introducing new patterns.
@@ -0,0 +46,4 @@
const {event}: Props = $props()
const listEvents = deriveEvents([{kinds: [10003, 30003, DELETE]}])
Owner

These kinds shouldn't be hard-coded

These kinds shouldn't be hard-coded
Owner

Also, no need to derive deletes, deriveEvents automatically filters out deleted stuff.

Also, no need to derive deletes, deriveEvents automatically filters out deleted stuff.
@@ -0,0 +55,4 @@
const getListLabel = (listEvent: TrustedEvent) =>
getTagValue("title", listEvent.tags) ||
getTagValue("d", listEvent.tags) ||
Owner

Remove the d fallback, if it doesn't have a title we should just show untitled.

Remove the d fallback, if it doesn't have a title we should just show untitled.
@@ -0,0 +86,4 @@
}
const lists = $derived.by(() => {
const uniqueEvents = uniqBy(getListKey, userLists).filter(
Owner

Events are automatically deduplicated based on replacement rules/address, no need to do that here.

Events are automatically deduplicated based on replacement rules/address, no need to do that here.
@@ -0,0 +96,4 @@
label: getListLabel(listEvent),
count: getTagValues(["e", "a", "p", "r"], listEvent.tags).length,
}),
)
Owner

Just return the events themselves here and save the rendering stuff for elsewhere

Just return the events themselves here and save the rendering stuff for elsewhere
@@ -0,0 +131,4 @@
if (!thunk) {
pushToast({message: "List already exists"})
return
}
Owner

This pattern in wrong, commands should not be doing validation, they should just be sending the provided thunk. Move the validation into this function. Actually, the listName empty check is duplicated above, so this code will never run. The error message is also wrong.

This pattern in wrong, commands should not be doing validation, they should just be sending the provided thunk. Move the validation into this function. Actually, the listName empty check is duplicated above, so this code will never run. The error message is also wrong.
Checking for merge conflicts…
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u https://gitea.coracle.social/bhavishy2801/flotilla feat-bookmarks:bhavishy2801-feat-bookmarks
git checkout bhavishy2801-feat-bookmarks
Sign in to join this conversation.