DEV Community

Cover image for Contributing to one of my personal Bookmarks manager - and making it better for everyone
Abdul S
Abdul S Subscriber

Posted on Edited on

Contributing to one of my personal Bookmarks manager - and making it better for everyone

Summer Bug Smash: Clear the Lineup 🐛🛹

This is a submission for DEV's Summer Bug Smash: Clear the Lineup powered by Sentry.

The Background

When I first got to know about this bugsmash challenge, I was like I only have a few decent projects, and it was amusing to revisit the long forgotten repos and searching for any fix or optimization I could do.
The focus shifted on the opensource software that I personally use (confidence). Fortunately, Canonical and Gnome have an amazing community and patch the interesting issues promptly.

So, this was not happening, until I revisited my current manager Chrome Bookmarks and the several old stashes on multiple other platforms I once used in search of a particular link saved long back.

I found the link, and then decided to refactor all my managers and distribute data among them for future ease.

Among the archived managers was Raindrop, which had a decent volume. So, I installed its open-source desktop app to refactor, but found this.

The native app was refreshing at every action. I quickly turned off the net, and yes, it was unusable. A disappointing limitation for my personal use case.

Or maybe I can fix the issue myself and make it better for everyone...

Here I am, having opened the PR and trying to write an intrigue.
.

Getting Started ✦

How does someone who is now more into ideas and less into code from last few months, quickly understand a codebase written by someone else.
The one thing I follow when working on open source is to work within the scope. Hence, I didn't put my time into understanding the code first, since I had no extra stake in it.
Got the code into antigravity app for linux from the start.
.

What's up

Initial thought was that I would store loaded bookmarks into some storage in the desktop (from my perspective there was no storage at all on client side), so opened the source code of desktop with webapp submodule.

I asked for opinions and ways to store the bookmarks while the refreshed data gets fetched (not a true cache but not my decision to make either).

and surprise


.

Visual Deception

The cache is there according to this, but then is it not working? Reading more in the response, the subheading was Collections. I again checked the behavior, and saw I had no collections after I had deleted them for refactor and was navigating in sidebar filters only.

Collection pages were indeed cached. Moreover, report told me that the whole logic was in the webapp repo, and not the desktop wrapper.

This time I did carefully read the architecture report. Skimmed through some code as well and switched active directory to webapp repo.

Deeply tested out the behavior, and gave the revised query.

thankfully

Your inference is correct. Here’s the precise mechanism.....


.

Pointing the Pin

Webapp's bookmark cache is a KV store keyed by the first route segment: state.bookmarks.spaces[spaceId].

Collections get their own id collectionId key, but every non-collection route -

  • all-bookmarks
  • every filter
  • every tag

collapses onto the single key '0'. So bookmarks list of /my/0/type:article and /my/0/#nature overwrite each other on every switch.

Raised the issue at #253
.

I can't run this app?

What do we do before making edits. We run the dev server.
But we can't. Why? Because the repo is not configured for local usage. It relies heavily on original environment, and hardcodes.
I went back and forth in another session, and could get the signup screen after editing a lot of files by myself too, but the api is apparently only configured to take auth requests from the deployed url only. It also never signed in to the local session.

After learning so much about the code and getting so close to the solution, I was not ready to withdraw.

Decided to have a second agent replace the visual review.
Main session does the edits and commits them, a second (review agent) as adversary on the git diff (plus point if you can tune the jealousy-driven development).

The first commit in PR is actually the final work of over 20 commits locally.
.

Back and Forth

(These are not exact prompts and results, but semantic partitions. The sequence is however intact. I am showing the most valuable segments.)

💬 Proposal
Since filters are predefined as links, articles etc. then why not assign negative keys, like -11 to -20, just like trash is assigned -99

✦︎ Gemini Main Agent

Pointed out that filters types come from api and are subject to change.

[spaceId refers to the key used to query cache]

👤 Me

Focusing on these 2 points was the most important decision that saved a lot of compute and time. It also reminded me that this was not my codebase and I can't just make big changes.

The whole logic of keys in cache is mirroring the routing decisions and is hence are coupled in the code too. This includes rehydration, updates, invalidation etc. As pointed earlier, the '0' comes from the first route segment of all these pages.

Forces at work
spaceId is threaded through the entire bookmarks tree

✦︎ Main Agent
suggests

👤 Me

After a lot of back and forth, we agreed that '0' has to be kept the key or there would be too many changes to keep track of, in absence of a live preview.

🌟 Instead, there would be multiple composite keys with the respective identifiers as suffix to '0' as shown in above response

The clever idea is that when the consumer of this key is cache-related, then use the full composite key thus giving it a separate space in that flow. (remember these are keys, not indexes)

When the consumer is not related to cache, mostly when directly fed in route as shown below, then use parseInt to trim it to '0'

This enables us to be compatible to opinions of existing logic, and also surgically apply our modification.

👤 Me again

Decided to go only with filters and not tags. There were just too many bugs that could be introduced since tags are user defined and have their own flow of create and delete.

Filters on the other hand are server defined and not editable. So, told Gemini my decision to do it only for filters.

✦︎ Main Agent

Component where the key is passed down to all other consumers.

That single prop becomes the cache key everywhere

💬 Proposal

Pass a second prop cacheId alongside an untouched spaceId, then swap it in only where cache reads happen. Nothing else sees a changed value. Surely safe.

✦︎ Main Agent

Audited every reachable consumer and argued against me.

👤 Me

I sorted consumers into three ways.
Bucket 1 already calls parseInt
Bucket 2 treats the value as an opaque string.
Bucket 3, consumers interpolating spaceId raw into a URL: popover, info chips, "open in new tab" etc. Can Solve with with parseInt(spaceId)||0
(so all three can be handled)

But right now all these use the spaceId props, the consumers who are supposed to use the composite cacheId prop may overlap in these, and may also pass them down in future where two different props for same route is being maintained.
A lot of silent bugs can be introduced esp with improper documentation.

Brainstormed through multiple suggestions and gave it my own, to modify the main key being passed down. Modifying it only when current route is a filter page.
Told Gemini and it updated its plan with a hook and some refactors

✦︎ Main Agent

Implements the plan

Modified the passed prop with composite key

Catching filter routes in a new file

👤 Me

Added parseInts to all consumers.
Changed regex to /^(?:[A-Za-z]+:[A-Za-z]+|❤️)$/ as i didn't want to cache if filter was combined with a search query, for ex. type:link & searchstring. Only pure routes should be cached.

✦︎ REVIEW AGENT

It is not fully correct. Composite cache keys are not integrated into mutation and refresh paths, producing stale or blank filter views.

On deletion, status correction only processes numeric collection keys and bare 0, not 0:type:*

Therefore 0:type:article can have zero bookmarks after deletion while retaining status.main as loaded.
Instead of UI for empty bookmark list, the UI is of non-empty bookmark list but without bookmarks.

👤 Me

Confirmed that this is an introduced bug, and not already present. asked for list of status and their effects for reference.

Tried to trace from where the above code gets data.

One line in bookmark helper

and agent's comments on it

It parseInts. 0:type:article collapses back to '0' before it is ever looked up. So even passing the composite key wouldn't have helped.

iterateSpaceId was never taught to parse the key we created. Hence, the whole actualize/insert/remove helper family is blind to it.

[Note the parseInt here was not added by our previous work]

🌟 Bonus Bug

The issue highlighted is related to the above discussed problem and will be fixed together.
But, that last line was a genuine bug in the deployed app where bookmark was being added to current filter page irrespective of its filter type.

Raised another issue for it at #255

💬 Proposal

In bookmark helpers iterateSpaceId function, the end goal is to pass the spaceId to func callback. I asked Gemini if it could make this callback available to our composite key too.

✦︎ Main Agent

Now iterates over all filter keys, and calls func, hence registers them for status correction listeners.

It may seem redundant to use the loop, but we are to avoid all edge cases and also call with one key only once.

An Important Distinction

Additionally the fix pointed out that the blank case was not exactly our doing.
The broken view code is not our edit. The switch in empty/view.js traces to upstream Raindrop, untouched by edits. The missing case 'loaded': has always been there.

But whether that gap was reachable is the part we changed. Earlier, every space a bookmark could live in was either in action.spaceId or explicitly actualized. loaded status with zero ids wasn't realistically reachable via removal.
The unhandled case was a harmless impossible gap.

👤 Me

I accepted the edits, and also asked to apply defensive guards that mitigate against the existing gaps we discussed.

✦︎ Main Agent

loaded status comes into effect only if the bookmark array is empty (ideally should never reach here).

In the bookmark reducer's REMOVE case, the same case which calls the iterateSpaceId with callback, added this line that sets status to empty if the cache has been emptied.

Thus we fixed a subtle bug that was present in upstream repo too.

✦︎ Review Agent

It flagged a msjor issue in the AI flow.

AI refresh remains pointed at raw cId (0) rather than the visible cache ID (0:type:article), so it does not refresh a filter view after a tool changes bookmarks.

The AI code calls refresh(cId), while the visible page reads spaces['0:type:article']. Refreshing cId neither fetches nor invalidates 0:type:article, therefore it does not fulfill the “next refresh” contract.

👤 Me

This was a real risk, we were finding downstream consumers of cId/spaceId, but then we were in the root of normal data flow of bookmarks.
The Ai flows in header and page are detached and separate from above. (recent features).

✦︎ Main Agent

It suggested strongly that the same useMemo we used initially hook can be reused by moving to a new file. No need to undo any work.

Changed original hook to import

Then in both AI flows which consume cId, replace it with composite spaceId.

.

some more fixes here and there manually

✦︎ Review Agent

Off you go

👤 Me

Off we go
Raised the Pull Request #254

Code

Issues
https://github.com/raindropio/app/issues/253
https://github.com/raindropio/app/issues/255

Pull Requests
https://github.com/raindropio/app/pull/254

Best Use of Google AI

Included Above
Model Used: Gemini 3.5 Flash mostly
Antigravity

Top comments (2)

Collapse
 
iabdsam profile image
Abdul S

@jess @ben
my post is no longer visible in any feed at dev.to/t/bugsmash/latest

Collapse
 
iabdsam profile image
Abdul S • Edited

@jess @sergical
looks like some bug in visibility. i reworded the intro and republished today but again this is not visible on feed or searches. if its because the pr is not merged yet, will try.