feat(notifications): add label to action types for bulletin button

[esaugomez31]

Summary

• Add optional label field to OpenDeepLinkAction and OpenExternalLinkAction across all layers (proto, Rust, GraphQL, TypeScript)
• The label is used by the mobile app to display an action button on bulletin cards. The button is purely visual — it triggers the same action as tapping the card (open deep link or external URL)
• Update GraphQL schemas (admin input + notifications subgraph output)
• Update protobuf: label lives on the Action message (outside oneof) so it applies to both action types
• Backward-compatible serde for ExternalUrl (supports old string format and new struct format)

Tests

Rust unit tests

• Serde roundtrip with action label
• Serde backward compat for ExternalUrl as string (old format)
• Serde backward compat without label (old records)
• Action label returns Some when set on deep link
• Action label returns Some when set on external URL
• Action label returns None when not set
• Push message uses localized content when available
• Push message uses default content
• Icon returns value when set

e2e BATS

• Create bulletin with label and external URL action, verify action.label is returned
• Create bulletin with label and deep link action, verify action.label is returned
• Create bulletin without label, verify action.label is null
• List stateful notifications, paginate with cursor
• Acknowledge stateful notification
• List unacknowledged with bulletin enabled, paginate
• List without bulletin enabled, paginate
• Unacknowledged count

[grimen]

@blink-claw-bot review

[blink-claw-bot]

Review: feat(bulletin): bulletin dismissible and button action

Nice, clean feature addition. The bulletinButton field flows correctly through all layers (GraphQL → TypeScript → protobuf → gRPC → Rust → back to GraphQL). Good test coverage on both the Rust unit tests and BATS integration tests.

✅ What looks good

• Backward compatibility handled well — #[serde(default)] on the new field, and an explicit deserialization test for old payloads without bulletin_button
• Protobuf schema uses optional correctly (field 8, no conflicts)
• Rust tests are thorough: serde roundtrip, backward compat, coexistence with action, localization
• BATS tests cover all three scenarios: external URL action, no button, and deep link action
• GraphQL schema additions are consistent with existing patterns (BulletinButton type, optional field on StatefulNotification)

Issues

1. bulletinButton validation is missing on the input side — The BulletinButtonInput.label is String! (non-null) in GraphQL, which prevents empty-via-null. But an empty string "" would pass through. Consider whether an empty label is valid. If not, add validation similar to how openDeepLink/openExternalUrl inputs are validated in marketing-notification-trigger.ts. (minor)

2. No mutual exclusion between bulletinButton and the absence of an action — A button with a label but no action (no openDeepLink or openExternalUrl) would render a button that does nothing when tapped. Should the API enforce that bulletinButton requires an action? At minimum, document the expected behavior. (minor — depends on client-side handling)

3. PR title mentions "dismissible" but the diff doesn't show dismissible changes — The title says "bulletin dismissible and button action" but I only see button/action additions. Is the dismissible part handled elsewhere, or is it implicit in the existing acknowledge flow? Clarifying the PR description would help. (clarification)

Nits

• nit: notifications_pb.js has a lot of whitespace reformatting (indentation changes in toObject methods throughout the file). This is likely from regenerating the protobuf JS code, but it inflates the diff significantly. If there's a way to regenerate only the changed parts, it'd make reviews easier in the future.

• nit: In notifications_pb.js, the icon field changed from jspb.Message.getFieldWithDefault(msg, 7, 0) to (f = jspb.Message.getField(msg, 7)) == null ? undefined : f. Same for DeepLink.screen, DeepLink.action, Action.externalUrl, and NotificationSettings.locale. These semantic changes (default 0/"" → undefined when unset) are correct for optional proto fields but are unrelated to the bulletin button feature. Worth noting in the PR description that the codegen was re-run.

Verdict

Looks good overall. The core feature is well-implemented with proper test coverage. The issues raised are minor — the empty label and button-without-action cases are worth considering but not blocking.

🤖 Review by Claude Opus 4.6 via Blink-Claw-Bot

[grimen]

Review: feat(bulletin): bulletin dismissible and button action

Nice, clean feature addition. The bulletinButton field flows correctly through all layers (GraphQL → TypeScript → protobuf → gRPC → Rust → back to GraphQL). Good test coverage on both the Rust unit tests and BATS integration tests.

✅ What looks good

• Backward compatibility handled well — #[serde(default)] on the new field, and an explicit deserialization test for old payloads without bulletin_button
• Protobuf schema uses optional correctly (field 8, no conflicts)
• Rust tests are thorough: serde roundtrip, backward compat, coexistence with action, localization
• BATS tests cover all three scenarios: external URL action, no button, and deep link action
• GraphQL schema additions are consistent with existing patterns (BulletinButton type, optional field on StatefulNotification)

Issues

1. bulletinButton validation is missing on the input side — The BulletinButtonInput.label is String! (non-null) in GraphQL, which prevents empty-via-null. But an empty string "" would pass through. Consider whether an empty label is valid. If not, add validation similar to how openDeepLink/openExternalUrl inputs are validated in marketing-notification-trigger.ts. (minor)
2. No mutual exclusion between bulletinButton and the absence of an action — A button with a label but no action (no openDeepLink or openExternalUrl) would render a button that does nothing when tapped. Should the API enforce that bulletinButton requires an action? At minimum, document the expected behavior. (minor — depends on client-side handling)
3. PR title mentions "dismissible" but the diff doesn't show dismissible changes — The title says "bulletin dismissible and button action" but I only see button/action additions. Is the dismissible part handled elsewhere, or is it implicit in the existing acknowledge flow? Clarifying the PR description would help. (clarification)

Nits

• nit: notifications_pb.js has a lot of whitespace reformatting (indentation changes in toObject methods throughout the file). This is likely from regenerating the protobuf JS code, but it inflates the diff significantly. If there's a way to regenerate only the changed parts, it'd make reviews easier in the future.
• nit: In notifications_pb.js, the icon field changed from jspb.Message.getFieldWithDefault(msg, 7, 0) to (f = jspb.Message.getField(msg, 7)) == null ? undefined : f. Same for DeepLink.screen, DeepLink.action, Action.externalUrl, and NotificationSettings.locale. These semantic changes (default 0/"" → undefined when unset) are correct for optional proto fields but are unrelated to the bulletin button feature. Worth noting in the PR description that the codegen was re-run.

Verdict

Looks good overall. The core feature is well-implemented with proper test coverage. The issues raised are minor — the empty label and button-without-action cases are worth considering but not blocking.

🤖 Review by Claude Opus 4.6 via Blink-Claw-Bot

@esaugomez31 Have a look.

[dolcalmi]

according to answers this should be something more generic. action_label or label if it is part of NotificatioAction structure? @grimen

[grimen]

@designsats Let's discuss this new action/button design more in detail tomorrow UX/UI call - seems like the introduction of it raises more questions than answers for me currently.

[littledino2112]

Pre-existing bugs in bats tests (not introduced by this PR, but worth fixing)

These exist on main already — since this PR extends these test files, it's a good opportunity to fix them and harden the test suite. Other than this, I think the added changes look good.

Bug 1: Undefined variable $n_bulletins (lines 103, 179)

Two tests assign to n_notifications but assert against $n_bulletins which is never defined:

Line 102-103 (test: "list unacknowledged stateful notifications with bulletin enabled"):

n_notifications=$(graphql_output '...unacknowledgedStatefulNotificationsWithBulletinEnabled.nodes | length')
[[ $n_bulletins -eq 0 ]] || exit 1    # ← $n_bulletins is never set, always evaluates as 0

Line 178-179 (test: "list stateful notifications without bulletin enabled"):

n_notifications=$(graphql_output '...listStatefulNotificationsWithoutBulletinEnabled.nodes | length')
[[ $n_bulletins -eq 0 ]] || exit 1    # ← same bug

In bash, an unset variable in [[ $n_bulletins -eq 0 ]] evaluates as 0, so the assertion always passes regardless of the actual notification count. The initial empty-state precondition check is completely defeated.

Fix: Replace $n_bulletins with $n_notifications on both lines.

Bug 2: Wrong GraphQL response path (line 178)

Line 178 uses jq path .data.me.listStatefulNotificationsWithoutBulletinEnabled.nodes but the GQL query field (in bats/gql/list-stateful-notifications-without-bulletin-enabled.gql:4) is statefulNotificationsWithoutBulletinEnabled — no list prefix. This means n_notifications is always null.

Note that line 207 in the same test correctly uses .data.me.statefulNotificationsWithoutBulletinEnabled.nodes, so only the initial zero-count check is broken.

Fix: Remove the list prefix:

n_notifications=$(graphql_output '.data.me.statefulNotificationsWithoutBulletinEnabled.nodes | length')

[esaugomez31]

Hi @grimen @dolcalmi, this seems to be a CI issue. In this PR we already addressed a previous CI problem 9c622fd (pinning DOCKER_API_VERSION=1.44), which was resolved following @openoms recommendation based on blinkbitcoin/bria#27. However, this is a different CI error.

The integration tests are currently failing, but this doesn't seem to be related to this PR since recent PRs also have failing integration tests:

1. chore(deps): update nix flake inputs for Docker Engine 29 compat #473 — fix(ci)--nix-flake-update
2. chore(deps): bump axios from 1.11.0 to 1.13.5 #456 — dependabot: axios-1.13.5
3. chore(deps-dev): bump webpack from 5.99.9 to 5.104.1 #449 — dependabot: webpack-5.104.1
4. chore(deps): bump @apollo/server from 4.12.2 to 5.4.0 #445 — dependabot: apollo/server-5.4.0
5. chore(deps): bump next from 14.2.26 to 16.1.5 #436 — dependabot: next-16.1.5

The tests themselves pass, the issue is on the CI side. This PR doesn't include integration tests, only Rust unit tests and e2e bats tests, and both pass locally:

Rust unit tests (cargo test):

E2E bats tests (nix develop -c bats bats/core/notifications/notifications.bats):

[openoms]

this seems to be a CI issue. In this PR we already addressed a previous CI problem 9c622fd (pinning DOCKER_API_VERSION=1.44), which was resolved following @openoms recommendation based on blinkbitcoin/bria#27. However, this is a different CI error.

working on a fix here:

• chore(deps): update nix flake inputs for Docker Engine 29 compat #473
  will need to rebase here when merged.

[esaugomez31]

Hi @dolcalmi @grimen, I applied the recommendation and it looks like this:

Before

label lived in a separate BulletinButton type:

type BulletinButton {
  label: String!
}

type StatefulNotification {
  action: NotificationAction
  bulletinButton: BulletinButton
}

{
  "openDeepLink": { "screen": "PEOPLE" },
  "bulletinButton": { "label": "Learn more" }
}

{
  "openExternalUrl": { "url": "https://example.com" },
  "bulletinButton": { "label": "Learn more" }
}

pub struct BulletinButton {
    pub label: String,
}

After

label lives inside each action type:

type OpenDeepLinkAction {
  deepLink: String!
  label: String
}

type OpenExternalLinkAction {
  url: String!
  label: String
}

{
  "openDeepLink": { "screen": "PEOPLE", "label": "Learn more" }
}

{
  "openExternalUrl": { "url": "https://example.com", "label": "Learn more" }
}

pub struct DeepLink {
    pub screen: Option<DeepLinkScreen>,
    pub action: Option<DeepLinkAction>,
    pub label: Option<String>,
}

pub struct ExternalUrl {
    url: String,
    pub label: Option<String>,
}

[dolcalmi]

looks good, just minor changes

Resolved