fix(notifications): drop the legacy context union and the rollback lever - #404
Merged
Merged
Conversation
A live NATS push without a `context` key made the store updater call `node.setLinkedRecord(null, 'context')`. Relay's RecordProxy rejects a null link, the throwing updater stayed queued, and every later store commit failed until a reload: "Failed to mark as read", the presence error every 10 s. It never fired before because the backend dual-wrote the context on every push. The typed `context` union is retired on the backend in favour of the flat `type` + `attributes` pair, so the frontend stops reading it everywhere: - the row fragment no longer selects `context`; the live path writes only `type` and `attributes` and links nothing; - `mapNotificationNode` reads one contract, `meta.notificationType` replaces `meta.contextType`, and the `notifications-legacy-path` lever is gone; - routing drops `CONTEXT_TYPENAME_BY_TYPE` and the `context` fallback on the NATS envelope; the approval types come from the core lib (`isApprovalNotificationType`, bumped to 0.0.632); - `schema.graphql` drops `Notification.context`, `NotificationContext`, its twelve implementations and `ApprovalToolCall`. Rows without `type`/`attributes` still map to a plain tile, as before.
Contributor
🦩 Flamingo Code ReviewNo findings on the current head. Mode: advisory Need another pass? Commits pushed after this review are not reviewed automatically.
Prefer typing? Comment React 👍/👎 on inline comments to teach the reviewer. Started 2026-09-14 16:33 UTC · updated 2026-09-14 16:33 UTC · workflow run |
Replaces the hand-pruned SDL with the backend's own: `Notification.context`, `NotificationContext` and its implementations, `ApprovalToolCall` and the `ApprovalType`/`ApprovalResolution` enums are gone there too. The software schedule types and the paginated package search come along with the refresh; nothing in `src/` references what was removed, and every Relay document compiles against it.
pavlo-flamingo
force-pushed
the
fix/notifications-drop-legacy-context
branch
from
September 14, 2026 16:35
ac60aa5 to
6077b15
Compare
…, not the raw one
aliaska-varieva
approved these changes
Sep 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A live NATS push without a
contextkey made the store updater callnode.setLinkedRecord(null, 'context'). Relay'sRecordProxyrejects a null link, the throwing updater stayed queued, and every later store commit failed until a reload: "Failed to mark as read", the presence error every 10 s. It never fired before because the backend dual-wrote the context on every push.Fix
The backend has retired the typed
contextunion in favour of the flattype+attributespair, so the frontend stops reading it everywhere.notificationFields_notificationno longer selectscontext; the live NATS path writes onlytypeandattributesand links nothing.mapNotificationNodereads one contract.meta.notificationTypereplacesmeta.contextType; thenotifications-legacy-pathlever is removed fromfeature-flags.ts.CONTEXT_TYPENAME_BY_TYPEand thecontextfallback on the NATS envelope. Approval types come from the core lib (isApprovalNotificationType, fix(notifications): gate the approval tile on meta.notificationType openframe-oss-lib#2182), bumped to 0.0.632.schema.graphqlis re-fetched from tst-notifications vianpm run fetch-schema.Notification.context,NotificationContextand its implementations,ApprovalToolCalland theApprovalType/ApprovalResolutionenums are gone on the backend; the software schedule types and the paginated package search come along with the refresh. Nothing insrc/references what was removed.Rows without
type/attributes(not yet swept by the backfill) still map to a plain tile, as they did with the lever off.Companion PRs (every other consumer of the retired
context)meta.notificationType(merged, 0.0.632).{ type, attributes }. Lockstep:resolveNatsNotificationRoutehere reads that shape only, so a desktop release must bundle this frontend together with that shell change; either half alone breaks toast clicks on desktop.contextfallback.Caveats
notifications.legacy-pathmust not be flipped on after this ships: the frontend has no legacy reader any more, so context-only pushes would render as plain tiles.contextfrom its schema, so any bundle still selecting it fails the notifications query there. Native shells carry the bundle in the binary; this needs to reach them.Verification
npm run type-check,npm run lint:ci,npm run formatclean;npm run relaycompiles every document against the fetched schema.contexton the NATS envelope is ignored").origin/main: attribute key names, spec approval types, NATS payload fields and FCM data keys match what the frontend reads.contextagainst a running backend still needs a manual check (no "Failed to mark as read" / presence errors in the console).