feat(ui): add custom feed icons and rework feed tabs - #2353
Conversation
Show a muted pencil on the right of editable feed tabs so users can open Edit without navigating. Clicking the name still opens the feed. Refs: #1898 Co-authored-by: Cursor <cursoragent@cursor.com>
Greptile SummaryThe PR updates custom-feed navigation so each tab exposes a separate pencil control for editing while preserving feed navigation across the full tab area.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/components/organisms/FeedNavigation/FeedNavigation.tsx | The full-tab navigation fix now uses an inset Link beneath a higher-stacking edit trigger, resolving the previously reported hit-area issue. |
| src/components/organisms/CustomFeedDialog/CustomFeedDialog.tsx | The dialog can receive an explicit feed for editing while retaining route-derived fallback behavior. |
| src/components/organisms/FeedNavigation/FeedNavigation.test.tsx | Tests cover the revised tab structure, styling, edit controls, and create-tab label. |
| src/components/organisms/CustomFeedDialog/CustomFeedDialog.test.tsx | The added test verifies that an explicitly supplied edit feed takes precedence over the route feed. |
Reviews (3): Last reviewed commit: "fix(ui): stretch custom feed link across..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Keep name clicks and empty tab space navigating to the feed while the pencil stays a separate edit control. Co-authored-by: Cursor <cursoragent@cursor.com>
h-full does not fill a min-height flex wrapper, so clicks on the tab padding still missed the link. Pin the link to inset-0 instead. Co-authored-by: Cursor <cursoragent@cursor.com>
🚀 Preview DeploymentURL: https://pubky-app-pr-2353-fzxmjul7ya-oa.a.run.app
|
There was a problem hiding this comment.
The mobile feed menu looks bad now, I think this was an unintentional change because it is not mentioned in the PR description. I don't see where this menu is in the Figma, but this doesn't look right.
Before:
After:
Another UI one--having only pr-6 makes the feed label off-center, we can use px-6 to keep it centered while still padding the pencil icon:
Also we can now edit feeds while not viewing them, but then on success we are navigated either to the feed on edit or back to home in the delete case. Is this expected behavior?
And I think the VRTs are QA/CI responsibility right? Or did @catch-21 say he would like us to include these? Only partial coverage was generated here anyway (Linux missing).
Last thing, small nit - the pencil icon is pretty tiny so it could be easy to miss especially on mobile. Consider giving this button some padding.
You are right, this was an unintentional change. @aldertnl Looking at the v25 prototype - https://www.figma.com/design/01ZvjSPZnKTNmaEWz0yJsq/Pubky-SHADCN?node-id=35688-283878&t=AGgGqjTMDG5oJVff-4 - I see that the right header button in mobile is not triggering feeds menu anymore - instead it shows Who to follow, Hot tags and Feedback sections - is that intentional? As for VRT, checked with @catch-21 and he is ok with us including VRT. Waiting for @aldertnl feedback before i add Linux as well. |
|
In prototype v26 feed selection on mobile now behaves the same as on desktop (tabbar), but with only the label showing for the selected tab, icon only on other tabs. This has been done to make mobile more consistent with the desktop experience. As you are working on this feed tab feature now, it makes sense to include this v26 change in this PR. Figma component Another thing: 'Home' tab should reflected selected reach (so not 'Home', but 'My network', or 'All', etc, with correct icon). 
|
- Feed navigation is now a tab bar on mobile too (v26): the selected tab shows icon + label, other tabs collapse to icon-only, Create is a plus icon; the strip sticks under the compact mobile header (Hot pattern) and the mobile right drawer falls back to Who to follow / Hot tags. - The first tab mirrors the selected reach (My network, All, Tagged as, ...) with the matching icon, via a shared REACH_FILTER_META map. - Custom feeds get a selectable Lucide icon (icon picker dialog, dynamic icon atom, spec 0.7.0 with the icon field synced to the homeserver), ported from PR #2284 and de-i18n-ized; feeds without an icon fall back to Activity. - CustomFeedDialog moves to a useCustomFeedForm hook (react-hook-form + zod); edit/delete only navigate away when you are viewing the edited feed, and config edits use replace since the old hash id is dead. - Pencil affordance: hover-revealed on desktop, always visible on the selected custom feed tab on mobile, with a larger hit area.
- Pencil: on hover-capable lg+ pointers it hides (and ignores events) until hover/focus-within; hover-less devices (iPad, touch laptops) keep it visible since :hover can never fire there. - Feed tab bar: sticky chrome and the gradient fade move to a non-scrolling wrapper so the fade paints below the bar instead of being clipped into the scrollport with phantom vertical scroll. - Icon picker: search also matches deprecated Lucide aliases mapped to their canonical glyph (querying 'home' finds 'house'); the aliases file now stores the alias->canonical map. - Lucide catalog (~116KB dynamic-import map) is lazily chunked instead of shipping in the /home bundle; name checks are shape-based and the loader validates against the real catalog. - One controlled edit CustomFeedDialog per strip instead of a mounted react-hook-form instance per feed tab; dialog supports controlled open/onOpenChange without a trigger. - useCustomFeedForm guards submit/delete re-entrancy with a ref so a queued second click cannot start a second commit. - Shared useSelectedReachFilter hook replaces the copied reach derivation in HomeFeedSidebar and FeedNavigation; the tab falls back to All for an out-of-enum persisted reach instead of crashing. - CustomFeedDialog derives reach options from REACH_FILTER_META; tab width/padding constants unified (active pill pads px-8 everywhere). - Cleanups: sanitizeIcon at the FeedNormalizer chokepoint, stale JSDoc and duplicate import removed in feed.validators, dead HomeFeedRightDrawerMobile removed.
- Icon state moves into a subscribable module store read through
useSyncExternalStore: no impure map read during render (React
Compiler-safe), hydration correct by construction, one shared state
per icon, and a failed chunk heals every mounted instance when any
retry succeeds. Unknown names are answered from the resident catalog
instead of stored, so peer-synced garbage cannot grow memory, and
'unknown' is distinguished from 'error'.
- Loaded dynamic icons carry the same lucide-<name> class as static
imports so CSS rules and test selectors match both.
- Icon picker drops the hand-rolled scroll virtualizer (per-scroll-event
renders, Tailwind-coupled constants, Tab-unreachable cells) for
progressive append via the existing useInfiniteScroll hook, drops the
open-animation gate that blanked the grid on every open, and defers
the search query with useDeferredValue so superseded keystrokes abandon
their work instead of mounting throwaway cells.
- Search now also matches lucide's synonym tags ('delete' finds the
trash icons) via a generated lucideIcons.tags map loaded lazily with
the catalog; name matches rank first. Result count is announced via
role=status.
- Verified on the production build: the catalog (57KB) and tags (148KB
raw) chunks stay out of the /home initial scripts.
- Run VRT serialized (--no-file-parallelism): parallel page startup intermittently kills the whole run before any test executes with an unhandled 'route.fulfill: target closed' rejection from @vitest/browser-playwright (reproduced locally on two consecutive commits; serialized runs are reliably stable). Documented in docs/visual-regression-testing.md. - Pre-bundle lucide-react/dynamic.js in the vrt project so the lazy catalog import cannot trigger a mid-run dependency re-optimization and page reload under in-flight requests. - renderForVRT now waits for dynamic Lucide icons to finish loading (no empty svg.lucide in the capture root), so a baseline can never nondeterministically bake in a blank icon box; a hung chunk fails the test, matching the existing image policy.
… exports - Replace plain div/p/span in IconPickerDialog and the FeedNavigation custom-feed tab wrapper with Container/Typography (overrideDefaults), matching house style; sentinel follows the WhoToFollow precedent. - Un-export IconPickerDialogProps, DynamicLucideIconProps and LucideIconState — no external consumers.
…ds-hover-interaction # Conflicts: # src/components/molecules/Filters/FilterReach/FilterReach.tsx
There was a problem hiding this comment.
- Can we swap Save and Delete button positions and use a trash icon for Delete and check icon for Save?
- Please increase space between name and edit icon on mobile
- The feed tab bar does not span full width on mobile currently, please fix.
Otherwise, LGTM
…ds-hover-interaction # Conflicts: # package.json
infin1t3
changed the title
…sm note Pre-bundling lucide-react/dynamic.js does not prevent the browser-mode teardown crash — measured with and without it, parallel runs fail either way. Keep only the serialization flag, which does prevent it, and record what was actually measured.
Drops the serialization flag: the browser-mode teardown race it worked around is intermittent, and a rerun is preferable to slowing every VRT run. Leaves the VRT scripts, config and docs identical to dev.
…dings - Move the lucide store, alias map, tag map and their tests from libs/utils into a dedicated libs/lucide folder. - Normalize icon-name case in one place: toLucideIconName (UI) and FeedValidators.sanitizeIcon (persistence) now agree, so an icon another client stored as 'Activity' renders its real glyph instead of silently falling back. - Cancel the VRT dynamic-icon poll on timeout so it stops burning frames for the rest of the page. - Drop the unused reset from useCustomFeedForm. - Use the shared useControlledState in IconPickerDialog instead of its own controlled/uncontrolled handling. - Close the feed edit dialog when the session ends. - Drop reach options with no home-store equivalent instead of defaulting them onto another option's label and icon. - Update the docs that referenced the old path and helper.
…omponent TAGGED_AS_FILTER_KEY moves to config/feed and the ReachFilterValue type next to ReachType in the home store types, so hooks and core-adjacent code no longer import a constant from a filter component. FilterReach keeps only the presentational REACH_FILTER_META.
…failures
- Look up the Lucide catalog with Object.hasOwn: 'constructor' is an
inherited key that resolves to Object, whose module has no icon node,
so a synced feed carrying that name crashed the feed pages. Guard the
resolved node too.
- Handle a rejected catalog load in the icon picker: the loader's own
catch does not cover the caller's derived promise, so a failed chunk
became an unhandled rejection and left the grid loading forever.
- Store lucide tags hyphenated so a multi-word query ('air conditioner')
matches the 227 multi-word synonyms, not just single words.
- Validate stored icons against the same name pattern the UI resolves
with, now shared from config, so a name cannot be persisted and then
permanently render as the fallback.
- Retry icons that failed to load when the browser comes back online,
which is what the component's self-healing promise claimed.
- Cap the active feed tab width below lg so one long feed name truncates
instead of pushing every other tab off-screen.
- Drop the edit dialog's feed snapshot when a background sync removes
that feed, and show a stored reach or content this dialog cannot offer
as a disabled option instead of an apparently empty select.
|
I tested with 1 desktop and 1 mobile with the same pubky. Scenario
Since it's not involved with Nexus, waiting/refresh doesn't help. WDYT @infin1t3 ? |
|
There some dead-code. Found by AI.
The drawer itself (HomeFeedRightDrawerMobile) was properly deleted. Removing 1–2 makes 3–5 fall out with it. |
|
@Taewa good catch. I agree this is not caused by this PR. The feed list is read from the local database, and the homeserver is only checked when the app boots — there is no live subscription to the feeds folder, so a second device only learns about a new feed on the next login. The same applies to edits and deletions. Opened a follow-up for it: #2416. It also proposes reusing the pattern we already have for the mute list, which keeps itself in sync through a homeserver event stream. |
…wer slot - The icon picker renders ~1700 cells and lucide ships one chunk per icon, so scrolling the catalog cost ~1700 requests and ~920KB. Add a generated bundle of every canonical icon node and load it once when the picker opens: one request, ~109KB, and every cell paints straight away. Feed tabs keep the per-icon path — they render a handful of icons and must not pull the whole set. - Stop notifying subscribers when an icon enters the loading state: it renders exactly like a name that was never requested, so no output can change. Halves the notifications a picker session produces. - Skip writing an icon node that the bundle already resolved while a per-icon request was in flight. - Remove the mobile right-drawer slot from ContentLayout, the FeedSection molecule it used to hold, and the stale mock left behind: nothing has passed that slot since feed selection moved into the tab bar. - Document the three generated lucide files and when to regenerate them.
|
@Taewa both points were right, thanks — fixed in 23bbeb4. Dead code: removed all of it. The mobile right-drawer slot on ContentLayout, the FeedSection component it used to hold, its test and snapshot, and the leftover mock in FeedRightSidebar's test. Nothing passed that slot once feed selection moved into the tab bar. Icon picker cost: we knew about this one and had measured it while building the picker, so let me share the numbers. The part you describe as the listener storm is real in count but cheap in practice: we measured the ~3M callbacks at about 2.2ms in total, spread across a whole scroll session, where a single frame is 16.7ms. So on its own that was not a reason to change anything. What did convince us is the other half — the requests. Every cell fetched its own file, so scrolling the full catalog meant ~1700 requests and ~920KB, which is a real cost on a phone on a first visit. Loading everything in one file is ~109KB, so the break-even is around 200 icons, and the picker already shows 150 in its first batch. Below that the old approach was cheaper, which is why we started there; anything past the first screen tips the other way. It also removes the waiting: cells now paint the moment they appear, instead of showing an empty square until their file arrives. The feed tabs keep loading icons one by one, since they only ever show a few and should not pull the whole set. I also took your point about the loading state: it looks identical on screen to an icon that was never requested, so telling every subscriber about it was pure waste. That notification is gone, which halves the callbacks a picker session produces. One note on |
|
Fixed locally, was a dependency issue |
4 participants
Summary
Review feedback
Notes
pubky-app-specsto 0.7.0 for theiconfield. The icon is stored next to the feed name on the homeserver and is not part of the feed id hash, so changing an icon does not change the feed URL. Feeds saved by an older client keep their icon.useCustomFeedForm), which is where the redirect rule lives./homeand/feed.Testing
npm test— 12429 passednpm run typecheck,npm run lint,npm run test:vrt:check-baselinesCloses #1898
Closes #1897
Screen.Recording.2026-08-25.at.11.24.24.mov