Skip to content

Conversation

@Eshwar1212-maker
Copy link
Contributor

This is the feature to show the red dot inidcator for new messages, in case that issue gets approved if you guys want to merge it.

And feel free to give me feedback.

Repository owner deleted a comment May 13, 2025
Repository owner deleted a comment May 13, 2025
@Athou
Copy link
Owner

Athou commented May 13, 2025

Thank you for your PR! I’ve noted a few general remarks below.

  • Please lint the code using Biome (npm run eslint:fix). It’s currently difficult to see what actually changed due to formatting inconsistencies.

  • It looks like the indicator you added only appears under the "All" category. However, the requirement was to show an indicator for each feed individually, and also on the corresponding category if a feed is in a collapsed group, in addition to the "All" indicator.

  • I haven’t tested the PR yet, but I’m skeptical that the current implementation works as intended. Specifically, using a useEffect with root?.feeds.length as a dependency doesn't make sense for detecting new entries in a feed. The number of root feeds (those without a category) doesn’t reflect whether entries were discovered. A more robust solution would be to create a new model based on Subscription that includes a hasNewEntries field (TreeSubscription?). This field should be updated in the two Redux reducers that modify the tree: the one that handles the WebSocket message and the one that reloads the tree entirely when the WebSocket is unavailable.

  • The hasNewEntries flag is never cleared, so the indicator remains visible indefinitely. It should be reset as soon as the entries for that feed are shown, either directly, via the containing category, or through the "All" view.

  • I’m not a fan of accessing localStorage directly from the view. A cleaner approach would be to handle this logic within the Redux layer instead.

@Eshwar1212-maker
Copy link
Contributor Author

Got it thanks. I'll get working on that as soon as I can. The local storage solution should work because it holds the previous key that shows the amount and then every single time the component renders it checks if the new root feed lengths are bigger then whatever key we had. But I understand your idea so I'll work on this other solution.

@Eshwar1212-maker
Copy link
Contributor Author

Eshwar1212-maker commented May 16, 2025

Getting super super close you can see what I added if your curious, just need to touch it up for the linting, add it to all category, but the core logic is there. Busy with work might get it done in the next week or two. But yea your approach is way better, it looks way better now that it is all in Redux.

Ill let you know when im ready for another review to push(ignore the bad formatting for now and logs lol will fix that)!

@Eshwar1212-maker
Copy link
Contributor Author

Hey! I think the feature is about 95% complete and ready for review — I’ve attached a screenshot of how it looks in the UI.

The current behavior:

-The red dot appears when new articles are pulled (based on unread count increasing), which helps notify the user.
-When a user clicks into a feed and views the articles, the unread count decreases, and the red dot disappears after a page refresh.

I had two quick questions:

-Would you prefer that the indicator disappears immediately on click? I can easily add a small reducer to update the state instantly if you'd like.
-Do you want the red dot shown on the "All" category as well? The original issue author mentioned per-feed indicators were the main goal, but I'm happy to add that if needed.

Let me know what you think.

For the picture, I clicked on HackerNews and the indicator went away after i refreshed because the unread count went away, and all the other subscriptions are new ones I added so the indicator is there, and the logic applies so that when new feeds are pulled for specific articles the indicator will show.
Screenshot 2025-05-18 at 12 11 56 AM

@Athou
Copy link
Owner

Athou commented May 19, 2025

Would you prefer that the indicator disappears immediately on click? I can easily add a small reducer to update the state instantly if you'd like.

Yes, I think it would make sense for the indicator to disappear as soon as the entries are shown/reloaded. If that's the case, when the use revisits CommaFeed later, the entries for the 'All' category will be loaded and the indicator will instantly disappear, so I'm not sure it's actually useful to store anything in the localStorage. I would just set the flag to true when the tree is reloaded or a websocket event is received, and clear the flag as soon as the loadEntries.fulfilled redux event resolves.

Do you want the red dot shown on the "All" category as well? The original issue author mentioned per-feed indicators were the main goal, but I'm happy to add that if needed.

Yes, and also intermediate categories if they are collapsed.

@Eshwar1212-maker
Copy link
Contributor Author

Thank you, gonna be busy with work next few days give me up to a week to finish it!

@Eshwar1212-maker
Copy link
Contributor Author

Eshwar1212-maker commented May 23, 2025

Hey is this better? About 90% done, I did it for all the sub categories, I think of what the last one you said I just have to finish the 'All' category but I want to make sure my logic is good so far and i understand what you mean.

Addressing confusion about the 'All' category

I was confused in terms of what you meant by "when the use revisits CommaFeed later, the entries for the 'All' category will be loaded and the indicator will instantly disappear,"?

So when a user clicks a category, comes back later and it reloads, the 'All' indicator will go away? Then what is the point of the indicator for 'All'? I was thinking if any subcategoy is clicked, then the 'All' inidcator will go away as well as that specific subcategories.

Since “All” is the default view that loads automatically when CommaFeed starts, wouldn’t that cause the red dot on “All” to disappear immediately — before the user has a chance to notice it? If that's the case, what’s the intended value of showing an indicator for "All"?

If you think my confusion comes from not understanding some terminology of the application, please let me know.

What I did for the sub categories

-I adjusted the code to not use local storage, and it only adjust the hasNewEntries through the redux reducers/thunks/states.

-I turn that indicator on with a reducer called 'setHasNewEntries'(a new one i created) in the reducer that handles the websocket to detect if new feeds are created(newFeedEntriesDiscovered thunk).

-And then when each sub category is clicked(loadEntries thunk with "entries/load") I clear that flag and set hasNewEntries to false.

-And for the all category I think it was pretty simple I just

Other Questions

Is there a way for me to test it? is there any feeds you guys use for testing I would want to see how it looks if a new feed gets a new article but I can't test it.

@Athou
Copy link
Owner

Athou commented May 23, 2025

To clarify, there are two use cases:

Use Case 1:
A user opens CommaFeed. They have two categories, each containing a feed. The user leaves CommaFeed open in a tab and returns to it later.
In the meantime, one new entry has been discovered for each feed. As a result, indicators appear next to both feeds and also next to the "All" category.
If the user collapses the category containing feed 1, the indicator is shown next to the category instead.
When the user clicks on feed 1, its entries are loaded, and the indicator disappears. Then, when the user clicks on the "All" category, the indicator is cleared from both the "All" category and the individual feeds.

Use Case 2:
The user closes the tab and later reopens CommaFeed. New entries have been discovered in the meantime.
An indicator appears next to each updated feed and next to the "All" category. However, since the "All" category is the default view, its entries are loaded immediately, and the indicator is instantly cleared for both "All" and all the feeds.

This is why I don’t think persisting anything in localStorage is necessary.

Does this make things clearer?

If it helps, I can take it from here based on what you did so far.

@Eshwar1212-maker
Copy link
Contributor Author

Eshwar1212-maker commented May 23, 2025

Yea okay I understood why the local storage is not needed now I get the All stuff.

And yea feel free to take it from there, what I did so far is good? Feel free to merge it and work on the rest, id love to have my first official contribution lol.

@Athou
Copy link
Owner

Athou commented May 23, 2025

id love to have my first official contribution lol.

sure!

@Athou Athou merged commit dc23126 into Athou:master May 23, 2025
@Athou Athou linked an issue May 23, 2025 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

red dot indicates new messages

2 participants