Skip to content

fix: group account notifications by feed - #254

Merged
Alessandro100 merged 4 commits into
mainfrom
fix/notifications-group-by-feed
Oct 27, 2026
Merged

Alessandro100 merged 4 commits into
mainfrom
fix/notifications-group-by-feed

Conversation

@Alessandro100

@Alessandro100 Alessandro100 commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

Closes #252

Problem

The Account > Notifications table showed one row per subscription. The backend keeps a single subscription per (user, notification type) and merges every subscribed feed into it, so the page listed one row per type with all of its feeds joined into the title.

Testing on the preview also showed a related bug: deleting a subscription to unsubscribe from one feed unsubscribed the user from every feed of that type. This affected the feed page's Unsubscribe button and the notification settings dialog.

Changes

  • One row per feed. Subscriptions are grouped by feed, and the Type column lists that feed's notification types. Global subscriptions (e.g. announcements) keep their own row. The grouping is a pure helper, account/notifications/lib/groupSubscriptions.ts, with unit tests.
  • Per-feed unsubscribe. Unsubscribe on a feed row, the feed page's Unsubscribe button and unticking a type in the settings dialog now remove only that feed, via PATCH /v1/user/subscriptions/{id} with remove_feed_ids (new removeFeedsFromUserSubscription). The backend deletes the subscription when its last feed is removed.
  • No per-feed pause. active is per notification type, so a single feed can't be paused. Feed rows show Notification Settings and Unsubscribe; Pause/Resume stays on global rows such as Announcements.
  • Notification Settings per feed. Feed rows open the existing NotificationSettingsDialog, which was rendered on this page but couldn't be opened.
  • API types synced. external_types/UserServiceAPI.yaml and DatabaseCatalogAPI.yaml are copied from mobility-feed-api main, and the user service types are regenerated with yarn generate:user-api-types. The catalog types (feeds/types.ts) regenerate identically, so only the YAML copy changed there.
  • Cache fix on subscribe. Subscribing can return an existing subscription that the feed was merged into, so cached entries are now replaced by ID rather than appended a second time. Helpers in utils/subscriptionCache.ts, with unit tests.

Row status is Active if any of the feed's subscriptions is active.

Testing

  • yarn lint: 0 errors
  • yarn test:ci: 541/541 pass
  • npx tsc --noEmit: clean
  • Checked on the Vercel preview with a signed-in account. Screenshots are in the comments.

🤖 Generated with Claude Code

notifications-grouped-by-feed notifications-row-actions-menu

The Account > Notifications table showed one row per subscription, and
subscribing to a feed creates one subscription per notification type, so a
single feed was split across several rows. Rows are now grouped by feed,
each listing all of that feed's notification types. Global subscriptions
(announcements) keep their own row.

Row actions apply to every subscription in the row, and feed rows gain a
"Notification Settings" entry that opens the existing settings dialog,
which was rendered on this page but never reachable.

Closes #252

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@vercel

vercel Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
mobilitydatabase-web Ready Ready Preview Oct 20, 2026 2:46pm UTC

Request Review

@github-actions

github-actions Bot commented Oct 19, 2026 •

Copy link
Copy Markdown

*Lighthouse ran on https://mobilitydatabase-dlnftyg73-mobility-data.vercel.app/ * (Desktop)
⚡️ HTML Report Lighthouse report for the changes in this PR:

Performance Accessibility Best Practices SEO
🟠 62 🟢 94 🟢 96 🟢 100

*Lighthouse ran on https://mobilitydatabase-dlnftyg73-mobility-data.vercel.app/feeds * (Desktop)
⚡️ HTML Report Lighthouse report for the changes in this PR:

Performance Accessibility Best Practices SEO
🟠 76 🟠 87 🟢 96 🟢 100

*Lighthouse ran on https://mobilitydatabase-dlnftyg73-mobility-data.vercel.app/feeds/gtfs/mdb-2126 * (Desktop)
⚡️ HTML Report Lighthouse report for the changes in this PR:

Performance Accessibility Best Practices SEO
🔴 33 🟢 94 🟢 96 🟢 100

*Lighthouse ran on https://mobilitydatabase-dlnftyg73-mobility-data.vercel.app/feeds/gtfs_rt/mdb-2585 * (Desktop)
⚡️ HTML Report Lighthouse report for the changes in this PR:

Performance Accessibility Best Practices SEO
🟠 83 🟠 84 🟢 96 🟢 100

*Lighthouse ran on https://mobilitydatabase-dlnftyg73-mobility-data.vercel.app/feeds/gbfs/gbfs-flamingo_porirua * (Desktop)
⚡️ HTML Report Lighthouse report for the changes in this PR:

Performance Accessibility Best Practices SEO
🟢 93 🟢 94 🟢 96 🟢 100

…iptions

The backend keeps one subscription per (user, notification type) and merges
new feeds into it, so deleting a subscription to unsubscribe from one feed
also unsubscribed the user from every other feed of that type. This affected
the account page row actions, the feed page Unsubscribe button and the
notification settings dialog.

- Sync UserServiceAPI.yaml and DatabaseCatalogAPI.yaml from
  mobility-feed-api main and regenerate the user service types, which adds
  remove_feed_ids to the subscription update request.
- Add removeFeedsFromUserSubscription and use it wherever a single feed is
  unsubscribed.
- Drop Pause/Resume from feed rows on the account page: active is per
  notification type, so one feed cannot be paused on its own. Global rows
  (announcements) keep it.
- Replace rather than append cached subscriptions on subscribe, since the
  API can return an existing subscription the feed was merged into.

Co-Authored-By: Claude Opus 5.5 <[email protected]>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Paused-subscription settings and concurrent unsubscribe handling can leave notification state inconsistent.

3 open issues
What changed in this PR

Addresses #252 by grouping account notifications by feed and limiting unsubscribe actions to the selected feed.

Changes:

  • Adds feed-grouped rows and per-feed notification settings.
  • Uses targeted feed removal and updates subscription caching.
  • Syncs API definitions and adds helper tests.

Applied vercel-react-best-practices (custom).

File Description
src/​app/​utils/​subscriptionCache.ts Adds cache update helpers.
src/​app/​utils/​subscriptionCache.spec.ts Tests cache helpers.
src/​app/​services/​user-service-api-types.ts Updates generated subscription types.
src/​app/​services/​notification-service.ts Adds targeted feed removal.
src/​app/​screens/​Feed/​components/​NotificationSettingsDialog.tsx Removes only the selected feed.
src/​app/​screens/​Feed/​components/​ClientSubscribeControls.tsx Updates unsubscribe and cache handling.
src/​app/​[locale]/​account/​notifications/​lib/​groupSubscriptions.ts Groups subscriptions by feed.
src/​app/​[locale]/​account/​notifications/​lib/​groupSubscriptions.spec.ts Tests grouping and titles.
src/​app/​[locale]/​account/​notifications/​AccountNotifications.tsx Renders feed rows and settings actions.
external_types/​UserServiceAPI.yaml Syncs subscription API contracts.
external_types/​DatabaseCatalogAPI.yaml Syncs validation-history definitions.

Review effort: Balanced

Comment thread src/app/[locale]/account/notifications/AccountNotifications.tsx Outdated
Comment thread src/app/[locale]/account/notifications/AccountNotifications.tsx Outdated
Comment thread src/app/[locale]/account/notifications/AccountNotifications.tsx Outdated
Address review feedback on the per-feed subscription actions:

- Initialize the notification settings checkboxes from feed membership
  rather than `active`, so a paused type can still be unchecked and the feed
  removed from it (account page and feed page).
- Add settleAll, which waits for every request before rejecting, and use it
  for multi-request updates so the resync after a partial failure sees every
  completed request. The settings dialog now resyncs after a partial failure
  too.
- Disable account page row actions while an update is in flight: feeds share
  subscriptions, and overlapping mutations on the same SWR key could restore
  a row that was already removed.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@Alessandro100

Copy link
Copy Markdown
Contributor Author

Important to highlight: The way the backend is setup, we cannot offer the 'pause' action on a per feed basis, only on the notification type as a whole

@Alessandro100

Copy link
Copy Markdown
Contributor Author

Important to highlight: The date we display on the table is for the subscription as a whole, but we display the notifications grouped by feed. Would it be possible to get a created_at field per feed element? Otherwise the dates of the subscription will always be the date when the original subscription was created cc @davidgamez

Another alternative is to just remove the dates all together

@Alessandro100 Alessandro100 self-assigned this Oct 20, 2026
Drop the Subscribed column and its sort option from the account
notifications table, along with the now-unused createdAt field on
grouped rows and its unit test.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@emmambd

emmambd commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

LGTM! Thanks for making this improvement

@abigailmbd abigailmbd left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@davidgamez davidgamez left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Alessandro100
Alessandro100 merged commit 177f733 into main Oct 27, 2026
4 checks passed

This branch was successfully deployed

1 active deployment
Preview — 6497b425 Deployed Oct 20, 2026 by vercel[bot]
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.

Account Notifications list of notifications

5 participants