Fix notification routes and goal dedupe for #334

[thomasluizon]

Summary

• Centralize every notification destination in NotificationUrls and replace nonexistent social destinations with null.
• Move goal deadline idempotency into a nullable DedupeKey column with a filtered unique index, while routing both persisted and push notifications to /progress.
• Preserve automatic streak freeze activation, route new notifications to /progress, and migrate historic /streak rows in bounded batches.
• Preserve destinationless notifications through FCM and Web Push by omitting the url payload key when the notification URL is null.
• Add route guards, producer coverage, dedupe coverage, migration operation coverage, push payload coverage, and regenerated architecture artifacts.

Closes thomasluizon/orbit-tickets#334

Testing

• dotnet build Orbit.slnx: 0 errors.
• dotnet test Orbit.slnx --no-build --no-restore: 5,877 passed.
• Focused push service tests: 29 passed.
• Null and real destinations are asserted field by field for FCM and Web Push payloads.
• node tools/arch-map.mjs: 0 orphan endpoints and 0 untested feature folders.
• Migration idempotency guard: passed.
• Notification route inventory checks: no /streak or /social literals remain in src.

Client compatibility

The mobile notification response handler at apps/mobile/hooks/use-push-notifications.ts reads content data url through optional access and navigates only when the value is a string accepted by isViewableNotificationUrl. A missing url key therefore produces no navigation and does not throw. No client change is required for the missing key.

Rollout decision

The persisted streak rewrite runs in this release, as Thomas decided on 2026-08-22. Builds that lack /progress are covered by the minimum supported version rollout in #203. Historic streak freeze rows are accepted as a short, bounded exposure.

Both migrations update at most 1,000 rows per batch and emit one row count. Their Down paths restore values before removing schema state.

Assumptions

• Goal deadline notifications use the shared /progress destination. I rejected inventing a goal specific query route because neither client defines one and the ticket establishes progress as the surviving shared surface.
• DedupeKey uses a filtered unique index. I rejected a nonunique lookup index because it would leave the existing concurrent duplicate catch unreachable and would not preserve deduplication under overlapping scheduler runs.
• notification_opened remains in the blocked client ticket. I rejected server emission because only the client knows whether its router resolved the destination, and this API ticket has no notification open request carrying that result.

[thomasluizon]

Implementation approach:

1. Add a single NotificationUrls class in Orbit.Application and replace every notification write and route predicate with its members or null. Social events will use null because no destination exists; preserving the invalid social strings would keep producing inert notifications.
2. Add nullable DedupeKey state to the Notification domain entity and persistence model, then switch goal deadline reads and writes to that field while setting Url to a real goal route. This separates navigation from idempotency instead of encoding two meanings in Url.
3. Add two EF migrations and update the model snapshot. The schema migration adds and indexes DedupeKey and backfills recognized goal deadline keys while assigning their goal destinations. The route migration rewrites persisted /streak values to /progress. Separate migrations keep the schema and route rewrite independently reversible, and both Down paths restore values rather than delete data.
4. Update focused domain, application, infrastructure, and migration tests, including the constant invariant, goal deadline dedupe, social null handling, unchanged route values, and repeatable data transforms.
5. Regenerate openapi.json and architecture.json with the repository generators, then run the focused tests, build, full test suite, analyzer checks, and repository gates.

Expected files are the notification entity and tests, OrbitDbContext and snapshot, NotificationUrls plus all inventoried producers and their tests, GoalDeadlineNotificationService and tests, two migration file sets, and the generated OpenAPI and architecture artifacts. I will not recreate the streak activation service removed by #331.

[pullfrog]

✅ No new issues found.

Reviewed changes Reviewed the current implementation at 9e6a5a7; this scaffold contains no file changes relative to main.

• Implementation scaffold The commit establishes the PR branch only; the notification route, goal deduplication, migration, persisted destination, and test changes remain described in the PR body but are not implemented yet.

  ^{｜ View workflow run ｜ Using GPT Sol ｜ 𝕏}

[thomasluizon]

Updated implementation approach after the 2026-08-23 ticket correction:

1. Preserve StreakFreezeAutoActivationService, remove its private route constant, and route its notification through NotificationUrls.Progress.
2. Centralize every notification destination and route predicate in NotificationUrls. Social notifications will use null because neither client has a valid destination.
3. Add nullable DedupeKey state to Notification and persistence, switch goal deadline reads and writes to it, and set Url to the goal route.
4. Add separate reversible migrations for the DedupeKey schema and recognized goal key backfill, and for the persisted /streak to /progress rewrite. The route rewrite will run now as Thomas decided.
5. Update focused tests for route constants, producers, dedupe behavior, and migration SQL, then regenerate openapi.json and architecture.json and run the repository verification sequence.

Expected changes remain limited to the named notification entity, persistence configuration and migrations, notification producers and predicates, their focused tests, and generated contract artifacts. The streak service will not be deleted or disabled.
�

[pullfrog]

Important

This implementation has a client rollout break and two migration data-safety failures that must be addressed before merge.

Reviewed changes Reviewed the implementation added since the prior Pullfrog review at 9e6a5a7.

• Centralized destinations: Replaced notification URL literals with NotificationUrls and removed nonexistent social destinations.
• Separated deduplication from navigation: Added nullable DedupeKey persistence and a filtered unique index for goal deadline notifications.
• Rewrote progress destinations: Routed new goal and streak notifications to /progress and migrated legacy goal and streak rows.
• Expanded safeguards: Added the domain URL guard plus producer, migration-shape, deduplication, and route-constant tests.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using GPT Sol ｜ 𝕏}

[thomasluizon]

Review follow-up for 81b5963:

• The rollout finding is intentionally unchanged under Thomas's explicit 2026-08-22 ticket decision: run the persisted /streak to /progress migration now. Ticket fix(gamification): on-read streak recalculation + AI summary cap 300 chars #203 supplies the minimum supported version gate, and the short historic-row exposure was explicitly accepted.
• Commit 3f1fcca8 deterministically preserves one canonical goal deadline dedupe key and assigns reversible per-row suffixes to any historical duplicates before the unique index is created.
• Commit 3f1fcca8 marks only legacy streak rows during the forward rewrite, so Down restores only rows proven to have come from /streak and leaves goal deadline /progress rows untouched.

Post-fix verification remains 0 build errors and 5,801 passing tests.

[pullfrog]

ℹ️ No new issues in the incremental delta. The existing client rollout thread remains open, so this review does not approve the PR.

Reviewed changes Reviewed 3f1fcca, which hardened the migration behavior added since the prior Pullfrog review.

• Normalized legacy duplicates: Ranked goal deadline rows deterministically, retained one canonical dedupe key per URL, and assigned stable ID-suffixed keys to additional rows across batches.
• Preserved rollback provenance: Tagged only migrated streak rows so the reverse migration no longer rewrites unrelated /progress notifications.
• Strengthened migration checks: Expanded migration-shape assertions for duplicate normalization, suffix restoration, and streak provenance markers.

  ^{｜ Fix it ➔ ｜ View workflow run ｜ Using GPT Sol ｜ 𝕏}

[thomasluizon]

@pullfrog review

[pullfrog]

Important

Native social and accountability pushes still resolve null to the home route, so the no-destination behavior is not implemented on Android.

Reviewed changes Reviewed notification destination handling, goal deadline deduplication, both data migrations, client contract compatibility, and focused coverage at b88c3f0.

• Destination inventory: Centralizes valid notification routes and removes nonexistent social destinations from persisted notifications.
• Deadline idempotency: Separates navigation from goal deadline deduplication with DedupeKey, a filtered unique index, and conflict-aware push ordering.
• Data migration: Backfills legacy goal keys, normalizes historical duplicates, and rewrites only proven streak routes with reversible provenance.
• Cross-repo behavior: Keeps API DTOs unchanged and verifies how web and Android clients validate and navigate notification URLs.
• Coverage: Updates producer, domain, scheduler, dedupe, route-constant, and migration-operation tests.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using GPT Sol ｜ 𝕏}

[thomasluizon]

Round 2 implementation approach:\n\n1. Change push payload construction so a null destination omits the url key. Apply this to the batched FCM path, its per subscription fallback, and Web Push so every transport preserves the same destinationless meaning.\n2. Keep nonnull payloads unchanged and cover null and real destinations field by field at payload construction boundaries.\n3. Confirm the mobile consumer tolerates a missing key by reading its notification response handler, then record that evidence in this PR.\n4. Run the focused infrastructure tests, commit immediately when they pass, then run the full build and test sequence, push this branch, and monitor PR #488 checks.\n\nThe mobile handler at apps/mobile/hooks/use-push-notifications.ts reads content data url through optional access and only calls router.push when the value is a valid string, so a missing key is safe and needs no client change.

[pullfrog]

✅ No new issues found.

Reviewed changes Reviewed 88ee925 and 45789e0, which corrected push payload construction and refreshed generated architecture artifacts since the prior Pullfrog review at b88c3f0.

• Preserved FCM destinationlessness: Routed both batch and fallback sends through one message builder that omits data.url when the producer passes null while preserving explicit routes.
• Preserved Web Push destinationlessness: Replaced anonymous-object serialization with conditional payload construction so null destinations omit the url key.
• Strengthened payload coverage: Added exact field-level tests for null and /progress destinations across both transports, and confirmed the mobile listener ignores an absent key.
• Regenerated architecture artifacts: Updated the architecture map with the push-service test references and reproduced the checked-in files cleanly with node tools/arch-map.mjs.

  ^{｜ View workflow run ｜ Using GPT Sol ｜ 𝕏}

[sonarqubecloud]

Quality Gate passed

Issues
7 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud