feat: implement bookmarks page with list management and deep-link scroll targeting #196
Reference in New Issue
Block a user
Delete Branch "bhavishy2801/flotilla:feat-bookmarks"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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
/bookmarkspage in the primary nav.
Addresses #19.
What's Changed
Bookmarks Page (
/bookmarks)10003and30003)content preview, and media (image/video) when present
10003) is always present as the default list and cannot be deleted or renamedBookmark List Picker (
BookmarkListPicker.svelte)Commands (
commands.ts)Added four new exported functions backed by NIP-51:
createBookmarkList— creates a new kind30003listaddEventBookmark— adds an event to a list (defaults to10003)removeEventBookmark— removes an event from a listdeleteBookmarkList— publishes a NIP-09 delete for a30003listrenameBookmarkList— updates thedtag and cleans up the old listBookmark Button in Message Menus
ChatMessageMenu,ChatMessageMenuMobile,RoomItemMenu,and
RoomItemMenuMobile— opens theBookmarkListPickermodalDeep-Link Scroll & Highlight
equery param (event ID) in addition toat(timestamp) for precise message targeting
tick()before firing to ensure the DOM is ready, replacingthe previous
requestAnimationFrameapproachuserHasScrolledresets whenatoreparams change, so navigating to a bookmarkedmessage always scrolls correctly
markInteraction) prevents programmatic scrolls from lockinguserHasScrolledprematurely — only real user gestures (wheel, touch, pointer drag,keyboard navigation) count
Navigation & Routing
PrimaryNav(both desktop and mobile layouts)/bookmarkstitle registered intitle.tsmakeChatPathnow accepts an optional event to append?at=...&e=...for directlinking from bookmarks
makeMessagePathandgetEventPathupdated to include theeparam alongsideatgoToSpacenavigation calls no longer usereplaceState, fixing back-button behaviourMESSAGE_KINDSconstant extracted tostate.tsand used consistently across spacechat filters
Testing
Hi @hodlbod. I've implemented the bookmarks section and related features, as per the approved design made by @Conceptionify. Kindly review the PR :)
A few first things before I get into detail:
atmakes 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?Address(there's a helper in welshman/util)?@@ -0,0 +33,4 @@const {event}: Props = $props()const listEvents = deriveEvents([{kinds: [10003, 30003, DELETE]}])Import these from @welshman/util
Done. Now these are imported from @welshman/util
@@ -168,2 +172,4 @@// List updatesconst BOOKMARKS = 10003const BOOKMARK_LISTS = 30003These can be imported from @welshman/util
Done. Now these are imported from @welshman/util
@@ -170,0 +197,4 @@)return latest ? readList(asDecryptedEvent(latest)) : makeList({kind})}This should go in
stateand should be as simple as:See
@welshman/app'suser.tsfile for examples.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.
Done.
@@ -170,0 +200,4 @@}export const createBookmarkList = async (title: string) => {const d = title.trim()dshould be randomly generated, not a slug of the titleFixed. d is now opaque/random, and title is stored separately.
@@ -170,0 +237,4 @@const relays = uniq([...Router.get().FromUser().getUrls(),...Router.get().Event(target).limit(3).getUrls(),...getRelayTagValues(event.tags),The bookmark list shouldn't be published to relays based on its content, just publish it to the user's outbox
Fixed. Bookmark list updates are now published only to user outbox relays.
@@ -170,0 +316,4 @@const relays = uniq([...INDEXER_RELAYS, ...getRelayTagValues(list.event.tags)])return publishThunk({event, relays})}Making
dtags opaque random ids will help a lot with this complexity.Yes, and I've fixed this. Opaque d is now the canonical identifier for custom bookmark lists, with display name in title.
Hi @hodlbod. I've made all the necessary changes. Kindly review the PR :)
Popover.ptagged items, which are filtered out and aren't displayed.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]}])These kinds shouldn't be hard-coded
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) ||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(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,}),)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}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.
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.