Skip to content

fix(mobile): accept the confirmation-time notification re-render, and let an unresolved start be cleared - #512

Merged
krishagel merged 1 commit into
mainfrom
fix/unresolved-start-gate
Sep 16, 2026
Merged

krishagel merged 1 commit into
mainfrom
fix/unresolved-start-gate

Conversation

@krishagel

@krishagel krishagel commented Sep 16, 2026

Copy link
Copy Markdown
Member

What broke

A drill started from the phone at 18:54 PDT on 2026-09-15 reached the server, created the event, and sent three push notifications. The app then refused the response, reported that it could not determine the outcome, and left the device unable to start or join anything afterwards.

Server evidence: start-event "outcome":"success" at 2026-09-16T01:54:05.930Z, request e11a614b, followed by push-worker-message-completed count 3. No server error. The failure was entirely on the client's side of a successful exchange.

Root cause

buildNotification re-renders notification copy at confirmation on purpose — the sent message carries the real start time, the confirming person, and current school and response names rather than the preview's guesses. The source says so at events.ts:512, then replaces renderedMessage on every channel of the intent.

activationMatchesPreview compared the intent's channels to the preview's with structurallyEqual — byte-for-byte deep equality. The preview renders with its own createdAt (start-preview.ts:214); the activation renders with the real activation time. {{startTime}} formats at minute granularity (START_TIME_FORMATTER, no seconds), so the two agree whenever preview and confirmation fall in the same clock minute and disagree whenever they straddle one. The longer an operator spends on the confirmation screen, the more likely the start is rejected — the opposite of what an emergency tool should do. The same applies to the other values the comment names: the confirming person's name, and school or response names changed inside the preview's fifteen-minute window.

Both the confirmation-time re-render and the {{startTime}} token entered in 34432c58 (2026-09-09), which reached production in the 2026-09-09 deploys. The mechanism is proven and reproduced in test; which of those values differed in this particular run is not individually confirmed, and the fix covers all of them.

Second defect: the unknown outcome was absorbing

Once latched, nothing could clear it. acknowledge accepts only succeeded or failed; every other clearDurableRecordOrBlock path carries the same guard; sign-out clears the session vault but not the start-mutation record, which lives in the keychain and survives restarts. The screen offered only a read-only check.

resolveActivationFromFreshEvents previously promoted an ambiguous activation from the active-event list and was removed — correctly, since that list carries no request-specific idempotency correlation. But removing the inference left no exit, so one ambiguous request disabled every later start and join on the device.

The inference stays removed; the decision returns to the operator via an explicit Clear and allow new decisions control. The copy still makes no claim about whether the earlier request succeeded or failed.

The test that should have caught this

The activation fixture built the intent as channels: preview.channels — comparing the preview to itself. The strictest condition in the client was never exercised. It now models the confirmation re-render, and restoring the old comparison reproduces the original rejection.

Coverage added

  • A re-rendered message is accepted.
  • A changed classification marker, endpoint count, integration, dropped channel, or reordered channel set is still refused.
  • An unresolved fence clears on acknowledgement and admits the next start.
  • The clear is refused for another owner and once already idle, and is disabled while offline.

Gate

Prettier, ESLint, and typecheck clean across the repository. 6/6 test shards green; 61 mobile native tests pass. Database-backed suite not run in this worktree.

Platform parity

Client-only logic and shared React Native UI — no platform-specific code. Needs verifying on iOS and Android before release.

… let an unresolved start be cleared

A drill started from the phone reached the server, created the event and sent
its notifications, and then the app refused the response and said it could not
determine the outcome. The device was left unable to start or join anything
afterwards. Two separate defects combined to produce that.

The first is the activation match check. `buildNotification` re-renders the
notification copy at confirmation on purpose: the message that actually goes
out carries the real start time, the person confirming, and the school and
response names as they stand at that moment, rather than the values the
preview guessed. The server documents this and then replaces `renderedMessage`
on every channel of the intent. `activationMatchesPreview` nonetheless
compared the intent's channels to the preview's with `structurallyEqual`, a
byte-for-byte deep equality. `{{startTime}}` is rendered at minute
granularity, so the two agreed whenever the preview and the confirmation fell
in the same clock minute and disagreed whenever they straddled one. A correct
server was then read as one that had returned the wrong event, and a start
that had already notified staff was reported as unresolved.

The check now compares what the operator actually authorized: the channel set
and its order, the endpoint count per channel, the sending integration, and
the classification marker that separates a drill from a real incident. Those
must still match exactly; the wording is allowed to move.

The second is that an unknown outcome was an absorbing state.
`acknowledge` clears only a `succeeded` or `failed` record, every other
`clearDurableRecordOrBlock` path carries the same guard, sign-out removes the
session vault but not the start-mutation record, and the unresolved screen
offered nothing but a read-only check. `resolveActivationFromFreshEvents` used
to promote an ambiguous activation from the active-event list and was removed,
correctly, because that list carries no request-specific idempotency
correlation. Removing the machine's inference left no exit at all: one
ambiguous request disabled every later start and join on the device, and the
record survives restarts in the keychain. Being unable to raise a new
emergency is a worse failure than an unproven old one, so the decision returns
to the operator: `acknowledgeUnresolved` clears the record on an explicit
press of "Clear and allow new decisions", while the copy continues to make no
claim about whether the earlier request succeeded or failed. The inference
stays removed.

The activation fixture built the intent as `channels: preview.channels`, so
the strictest condition in the client compared the preview to itself and could
never fail. It now models the confirmation-time re-render, which reproduces
the original rejection when the old comparison is restored. New coverage: a
re-rendered message is accepted; a changed classification marker, endpoint
count, integration, dropped channel or reordered channel set is still refused;
an unresolved fence clears on acknowledgement and admits the next start;
the clear is refused for another owner and once already idle, and is disabled
while offline.

Gate: Prettier, ESLint and typecheck clean across the repository; 6/6 test
shards green and 61 mobile native tests pass.
Copilot AI lite review requested due to automatic review settings September 16, 2026 02:55

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@claude

claude Bot commented Sep 16, 2026

Copy link
Copy Markdown

Review: #512 — accept confirmation-time re-render, allow clearing an unresolved start

Clean diff. No IMPORTANT findings. This is a well-scoped fix with strong test coverage for both defects it addresses.

What I checked

  • channelAuthorizationMatches (start-api-client.ts:456-491) replaces byte-for-byte structurallyEqual on intent.channels with a comparison of channel, endpointCount, integrationId, and renderedMessage.classificationMarker per index. Cross-checked against the server's own DispatchBatchSchema (packages/contracts/src/notification.ts:379-462), which treats channel/eventKind/templateMode/purpose/classificationMarker as batch identity and renderedMessage wording as free to vary — the new client check is a faithful subset of that server-side invariant. eventKind, templateMode, purpose, rosterSnapshotId, and rosterPopulation are still asserted at the intent level in activationMatchesPreview (start-api-client.ts:500-523), so per-channel classificationMarker is defense-in-depth, not the only guard. No weakening of what the operator authorized (channel set, order, endpoint count, sending integration, drill-vs-incident marker all still exact-match).
  • acknowledgeUnresolved (start-mutation-coordinator.ts:869-896, start-mutation-provider.tsx:315-320) mirrors the existing acknowledge method's owner/online/phase guards exactly, and reuses the already-tested clearDurableRecordOrBlock path rather than introducing new persistence-failure handling. The UI wiring in app/index.tsx:395-400 and app/start/index.tsx:686-692 differs only by an appropriate returnHome() call on the start screen vs. none on the home screen — consistent with each screen's role.
  • Type modeling: onAcknowledgeUnresolved is never on Pending/Failed props and required on Unresolved props, so the compiler enforces the control only appears where it's meaningful.
  • Tests actually assert behavior: the new start-api-client.test.ts fixture (reRenderedAtConfirmation) now diverges preview vs. confirmation copy, which is the root-cause fix for the original bug — previously channels: preview.channels made the strictest check compare the preview to itself. The added "still refuses" test varies classification marker, endpoint count, integration, dropped channel, and reordered channels independently and asserts outcomeUnknown: true for each. Coordinator tests cover clear-then-resubmit, wrong-owner refusal, and already-idle refusal. The offline-disabled test checks accessibilityState/disabled props only (doesn't simulate a blocked press), but that matches the existing convention for the sibling "Check active events" offline test (start-mutation-attention.test.tsx:318-320) — not a new gap.
  • No logging/telemetry changes in this diff and no student PII in scope — this is client-side start/join mutation-state handling only.
  • No CI weakening: no tests removed or skipped; one assertion in an existing test was tightened (toHaveLength(1) → explicit label list) to reflect the new control, not loosened.

Nits (0)

None worth raising.

@krishagel
krishagel merged commit f44b0e7 into main Sep 16, 2026
12 checks passed
@krishagel
krishagel deleted the fix/unresolved-start-gate branch September 16, 2026 04:13
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.

2 participants