feat(notify): typed title/body/type fields for non-keysign notifications

[gomesalexandre]

Closes #26

Paired with vultisig/agent-backend#838 (that PR uses the new fields once this lands).

what

Reminders and scheduled task notifications were rendering as "Vultisig Keysign request" because the notification server hardcoded that title everywhere. This PR adds a type field + explicit title/body fields so callers can send the right text.

how

• New NotificationType enum: keysign (default) | reminder | task_success | task_failure | generic
• ResolveTitle() returns the explicit title if set, otherwise a per-type default
• ResolveBody() for keysign keeps the legacy "Vault: <vault_name>" fallback; other types return the explicit body (or empty string if absent)
• IsValid() relaxes required-field validation for non-keysign types: vault_id is always required; vault_name / local_party_id / qr_code_data are only required for keysign (they carry the deeplink)
• All three push paths (APNs, FCM, WebPush) now call ResolveTitle() / ResolveBody() instead of the hardcoded strings
• /notify handler delegates to IsValid() instead of its own inline all-fields check

Backward-compat: empty type = keysign, so every existing caller is unaffected.

receipts

--- go test ./... output ---
?   	github.com/vultisig/notification/api	[no test files]
ok  	github.com/vultisig/notification/models	0.353s
ok  	github.com/vultisig/notification/service	0.743s
ok  	github.com/vultisig/notification/stream	7.343s
ok  	github.com/vultisig/notification/ws	1.233s

--- new model tests (13 cases) ---
TestResolveTitle/empty_type_defaults_to_keysign_title   PASS
TestResolveTitle/explicit_keysign_type                  PASS
TestResolveTitle/reminder_type                          PASS
TestResolveTitle/task_success_type                      PASS
TestResolveTitle/task_failure_type                      PASS
TestResolveTitle/generic_type                           PASS
TestResolveTitle/explicit_Title_overrides_type_default  PASS
TestResolveBody/keysign_with_vault_name_falls_back      PASS
TestResolveBody/explicit_Body_overrides_vault_name      PASS
TestResolveBody/reminder_with_body                      PASS
TestResolveBody/reminder_with_no_body_returns_empty     PASS
TestResolveBody/task_failure_with_body                  PASS
TestIsValid/valid_keysign_(all_fields)                  PASS
TestIsValid/keysign_missing_vault_name                  PASS
TestIsValid/keysign_missing_qr_code_data                PASS
TestIsValid/keysign_missing_local_party_id              PASS
TestIsValid/reminder_needs_only_vault_id                PASS
TestIsValid/task_success_needs_only_vault_id            PASS
TestIsValid/task_failure_needs_only_vault_id            PASS
TestIsValid/missing_vault_id_always_invalid             PASS

🤖 Agent-generated

[coderabbitai]

Warning

Review limit reached

@gomesalexandre, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 10 minutes and 46 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7de936eb-9cff-4499-b6dd-a007e70f69d5

📥 Commits

Reviewing files that changed from the base of the PR and between a3f89b7 and 92756d6.

📒 Files selected for processing (5)

• api/server.go
• models/notification.go
• models/notification_test.go
• service/notification.go
• service/notification_test.go

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/typed-notification-fields

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[NeOMakinG]

testing approach

• Cloned vultisig/notification at 92756d6b, checked out feat/typed-notification-fields
• go test ./... locally: 4 packages pass (models 0.36s, service 0.48s, stream 7.8s, ws 1.3s)
• Full diff walk across all 5 changed files: api/server.go, models/notification.go, models/notification_test.go, service/notification.go, service/notification_test.go
• ab#838 companion PR: 404 (not yet opened); field names are canonical Go JSON snake_case so no drift risk
• GLM glm-4.6 third-eye lane: ran independently; findings deduped below
• Codex lane: skipped (TTY/auth unavailable)

blocking

None.

preferably-blocking

None.

should-fix

s1 — dedup key doesn't include type, cross-type notifications silently dropped

api/server.go:224 — the dedup key is vaultId + ":" + appID. A keysign push followed within 30 seconds by a scheduler-issued task_success, task_failure, or reminder for the same vault collapses into the same bucket and the second notification is silently swallowed (HTTP 200 returned to the caller, zero push delivered).

This is pre-existing code, but the PR's value proposition — letting the scheduler send non-keysign notifications — activates the latent bug. Before this PR the only callers were keysign senders; after ab#838 lands, two independent producers can co-occur within the 30-second window.

Suggested fix (api/server.go around line 224):

resolvedType := req.Type
if resolvedType == "" {
    resolvedType = string(models.NotificationTypeKeysign)
}
dedupKey := req.VaultId + ":" + dedupAppID + ":" + string(resolvedType)

s2 — deeplink always delivered for non-keysign APNs/FCM pushes

service/notification.go:215 — Custom("deeplink", request.QRCodeData) is set unconditionally. For reminder/task_success/task_failure types, QRCodeData is empty (not required by IsValid()). The iOS app receives a push with deeplink: "". If the notification tap handler calls URL(string: deeplink) without nil-checking the result, navigation fails silently or crashes.

Same issue on FCM: Data["message"] = request.QRCodeData sends an empty string.

Suggested fix:

if request.QRCodeData != "" {
    p = p.Custom("deeplink", request.QRCodeData)
}

And in processAndroidNotification:

data := map[string]string{}
if request.QRCodeData != "" {
    data["message"] = request.QRCodeData
}

suggestion

NotificationTypeGeneric hits default in ResolveTitle switch rather than a named case — works correctly (test covers it, returns "Vultisig"), but a future reader sees a 5-value enum with only 4 explicit cases and can't tell if generic is intentional or missing. Consider adding case NotificationTypeGeneric: to make it explicit.

q

q1 — ab#838 not yet opened. PR body says paired with agent-backend#838 but that PR returns 404 today. The notification server changes are backward-safe to merge independently, but confirming ab#838 will reference and rebase onto this would be useful.

q2 — dedup intent for scheduler notifications. With the scheduler path, if a task fails and immediately retries, two task_failure notifications fire within 30 seconds. Is dedup suppressing that second one intentional? If not, s1 above should be addressed (and the 30s TTL may also be too broad for scheduler notifications vs keysign ones).

risk

Low. The core enum dispatch, backward-compat default (empty type == keysign), and all three push delivery paths (APNs/FCM/WebPush) are correct. The IsValid() centralization in server.go removes the inline 4-field all-required check that would have silently rejected every non-keysign call — that latent bug is properly fixed here.

The dedup finding (s1) is the only operationally live issue once ab#838 ships; it can be fixed in-line in ab#838 or as a follow-up before the scheduler goes to production.

verdict

APPROVED. Core change is correct, backward-compat is verified, test coverage is solid (20 cases across 3 table-driven suites). s1 (dedup key) should be resolved before or alongside ab#838 to avoid silent notification drops.

---

triple-lane: Claude primary + GLM glm-4.6 third-eye (ran independently); Codex skipped (TTY-unavailable). Runtime: go test ./... green on PR HEAD 92756d6b.

[NeOMakinG]

QA Evidence

Branch: feat/typed-notification-fields @ 92756d6b

Test run:

$ go test ./...
?   github.com/vultisig/notification/api            [no test files]
?   github.com/vultisig/notification/cache          [no test files]
?   github.com/vultisig/notification/cmd/server     [no test files]
?   github.com/vultisig/notification/cmd/worker     [no test files]
?   github.com/vultisig/notification/config         [no test files]
?   github.com/vultisig/notification/contexthelper  [no test files]
?   github.com/vultisig/notification/internal/health [no test files]
ok  github.com/vultisig/notification/models         0.363s
ok  github.com/vultisig/notification/service        0.483s
ok  github.com/vultisig/notification/stream         7.774s
ok  github.com/vultisig/notification/ws             1.285s

All 4 packages pass. 20 new model test cases (TestResolveTitle 7 + TestResolveBody 5 + TestIsValid 8) verified non-vacuous against the implementation.

Key invariants verified by reading source:

• Empty type → resolvedType() returns NotificationTypeKeysign → backward compat confirmed
• All 5 enum values produce expected titles in ResolveTitle()
• IsValid() relaxes to vault_id-only for non-keysign types
• All 3 push paths (APNs/FCM/WebPush) call ResolveTitle()/ResolveBody() instead of hardcoded strings

ab#838 status: PR not yet opened (404); no cross-repo drift risk identified as fields follow canonical Go JSON snake_case convention.