feat: iOS Notification Service Extension for rich chat notification previews - #2571
Draft
kaladivo wants to merge 6 commits into
Draft
feat: iOS Notification Service Extension for rich chat notification previews#2571kaladivo wants to merge 6 commits into
kaladivo wants to merge 6 commits into
Conversation
When markAsPulled is false the handler skips both the pulled-flag update and the inbox metadata write, making the request strictly read-only. Defaults to true (absent = true), so existing clients are unaffected. This is the server-side prerequisite for the iOS Notification Service Extension, which fetches messages to render notification previews but must never affect seen/pulled semantics. Must be deployed before any app build containing the NSE ships.
…tation Deterministic vector suite (pnpm generate:nse-vectors) covering eciesLegacy decrypt (the chat message scheme), eciesGTM decrypt, and ECDSA verify over secp256k1 keys in the exact PrivateKeyHolder encodings, including negative cases and a stripped-leading-zero DER scalar key. A jest suite validates every vector against the TS implementation so the checked-in file cannot drift. Consumed by the Swift VexlNotificationCore tests to prove parity of the iOS NSE crypto port.
…reviews Replaces the generic iOS chat notification with a real preview (sender name + message text) decrypted on-device, Signal-style. - targets/vexl-nse: NSE target via @bacons/apple-targets; always falls back to the generic content on any failure or timeout - native/VexlNotificationCore: testable SPM package (swift-secp256k1 + CryptoKit) implementing eciesLegacy/GTM decrypt, ECDSA challenge signing, bridge store readers, a read-only chat API client (markAsPulled: false), payload parsing and localized rendering with thread-grouping parity; 55 tests incl. shared TS vectors - modules/vexl-nse-bridge: local Expo module syncing vexl-token -> inbox private key entries into a shared keychain access group (AfterFirstUnlock) and non-secret metadata into the App Group container; declarative replace-all sync, cleared on logout and on logged-out startup - JS wiring: bridge sync on token-map/chat/session/locale changes; NSE-enriched notifications are dismissed before the richer JS local notification replaces them, and skipped by the generic cancel logic Only vexl_nt_ tokens and secp256k1 inbox keys are supported; legacy cyphers and other curves bail to the generic notification. MMKV remains the source of truth for keys; the bridge is additive. Requires the chat-service markAsPulled change to be deployed first.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds an optional ChangesmarkAsPulled retrieval flag
iOS Notification Service Extension pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant APNs
participant NotificationService
participant NotificationEnricher
participant ChatApiClient
participant KeychainInboxKeyStore
participant AppGroupMetadataStore
APNs->>NotificationService: didReceive(userInfo)
NotificationService->>NotificationEnricher: enrich(userInfo)
NotificationEnricher->>KeychainInboxKeyStore: inboxKeyPair(forVexlToken)
NotificationEnricher->>AppGroupMetadataStore: loadMetadata()
NotificationEnricher->>ChatApiClient: createChallenge(publicKey)
NotificationEnricher->>ChatApiClient: retrieveMessages(markAsPulled:false)
ChatApiClient-->>NotificationEnricher: ServerMessage[]
NotificationEnricher-->>NotificationService: RenderedNotification?
NotificationService-->>APNs: deliver enriched or original content
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🚀 Expo preview is ready!
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 085b0a9cdc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Greptile SummaryThis PR adds a Notification Service Extension (NSE) for iOS that intercepts incoming chat push notifications and replaces the generic "new message" alert with the decrypted sender name and message text, entirely on-device. A companion server-side change adds a
Confidence Score: 5/5Safe to merge after the deployment ordering documented in the PR description is followed (server change first, then app build). The server change is strictly additive and fully backward-compatible. The Swift crypto primitives (eciesLegacy, ECDH, PBKDF2, AES-CTR) are all pinned against shared TypeScript test vectors, and the 55-test Swift suite covers the decrypt/sign/verify paths along with adversarial negatives. The exactly-once delivery guarantee in NotificationService is sound: the enrichmentTask is stored under the stateLock before the Task body can execute, closing the window identified in a prior review. All failure modes fall back to the existing generic notification content, so a bug in the NSE path degrades gracefully.
Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant APNs as APNs
participant NSE as NotificationService (NSE)
participant KS as KeychainInboxKeyStore
participant MS as AppGroupMetadataStore
participant CS as ChatService
participant OS as iOS
APNs->>NSE: didReceive(request) — vexl_nt_ targetToken
NSE->>NSE: makeEnricher(), start Task under stateLock
NSE->>KS: inboxKeyPair(forVexlToken:)
KS-->>NSE: InboxKeyPair (privateKey, publicKey)
NSE->>MS: loadMetadata()
MS-->>NSE: NseMetadata (chatServiceUrl, senderNames, locale)
NSE->>NSE: VexlPrivateKey(pemBase64:) — curve check
NSE->>CS: "POST /api/v1/challenges {publicKey}"
CS-->>NSE: challenge string
NSE->>NSE: signChallenge(challenge, privateKey) — ECDSA SHA-256
NSE->>CS: "PUT /api/v1/inboxes/messages {markAsPulled:false}"
CS-->>NSE: "[{message: ciphertext, senderPublicKey}]"
NSE->>NSE: eciesLegacyDecrypt per message, pickPreviewCandidate
NSE->>NSE: NotificationRenderer.render(message, displayName, locale)
NSE->>NSE: RenderedNotification.apply(to: UNMutableNotificationContent)
NSE->>OS: contentHandler(enrichedContent)
note over NSE,OS: On any failure OR 20s timeout → contentHandler(originalContent)
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant APNs as APNs
participant NSE as NotificationService (NSE)
participant KS as KeychainInboxKeyStore
participant MS as AppGroupMetadataStore
participant CS as ChatService
participant OS as iOS
APNs->>NSE: didReceive(request) — vexl_nt_ targetToken
NSE->>NSE: makeEnricher(), start Task under stateLock
NSE->>KS: inboxKeyPair(forVexlToken:)
KS-->>NSE: InboxKeyPair (privateKey, publicKey)
NSE->>MS: loadMetadata()
MS-->>NSE: NseMetadata (chatServiceUrl, senderNames, locale)
NSE->>NSE: VexlPrivateKey(pemBase64:) — curve check
NSE->>CS: "POST /api/v1/challenges {publicKey}"
CS-->>NSE: challenge string
NSE->>NSE: signChallenge(challenge, privateKey) — ECDSA SHA-256
NSE->>CS: "PUT /api/v1/inboxes/messages {markAsPulled:false}"
CS-->>NSE: "[{message: ciphertext, senderPublicKey}]"
NSE->>NSE: eciesLegacyDecrypt per message, pickPreviewCandidate
NSE->>NSE: NotificationRenderer.render(message, displayName, locale)
NSE->>NSE: RenderedNotification.apply(to: UNMutableNotificationContent)
NSE->>OS: contentHandler(enrichedContent)
note over NSE,OS: On any failure OR 20s timeout → contentHandler(originalContent)
Reviews (4): Last reviewed commit: "wip docs" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (11)
packages/rest-api/src/services/chat/contracts.ts (1)
196-202: 🗄️ Data Integrity & Integration | 🔵 TrivialDeployment-ordering safety relies on manual sequencing.
The comment correctly documents that false = strictly read-only retrieve (used by the iOS notification service extension): messages are NOT marked pulled and inbox metadata is not updated. Absent/true = current app behavior. The doc also notes a chat-service with this field MUST be deployed before any app build containing the NSE ships. An old server silently ignores the field and marks the messages pulled, which races the app's deletePulledMessages call and can permanently delete messages the app never received.
This is purely a rollout/ops sequencing risk with no automated guard (e.g., server capability check or client-side version gating) — worth ensuring the deployment pipeline/runbook enforces this ordering before the NSE-enabled app build ships.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/rest-api/src/services/chat/contracts.ts` around lines 196 - 202, The deployment-ordering requirement for the chat-service/NSE read-only retrieve behavior is only documented and not enforced. Update the relevant contract/docs around the field in contracts.ts and the chat-service rollout path to add an explicit guard such as server capability checking or client/version gating, so NSE-enabled app builds cannot ship before the backend supports the field and the deletePulledMessages race is prevented.apps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Rendering/NotificationLocalization.swift (1)
44-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a parity test between
RenderableMessageTypeand the localization table.The mapping between
RenderableMessageTypecases and localization keys is only enforced by convention (a comment in the generator script). If a new case is added toRenderableMessageTypewithout updatingRENDERABLE_TYPES/the JSON,entry(for:locale:)silently returnsnilfor that type in every locale, andNotificationRenderer.rendersilently drops the preview (falls back to the generic notification) with no compile-time signal.Add a Swift test asserting
NotificationLocalization.loadBundled()!.entry(for: type, locale: "en")is non-nil for everyRenderableMessageType.allCases(assuming it'sCaseIterable) to catch drift early.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Rendering/NotificationLocalization.swift` around lines 44 - 54, Add a parity test to lock `RenderableMessageType` to the bundled localization keys. In the `NotificationLocalization` test suite, verify that `NotificationLocalization.loadBundled()!.entry(for:locale:)` returns a non-nil result for every case in `RenderableMessageType.allCases` when using the English locale. This should exercise the existing `entry(for:locale:)` and fail fast if `RENDERABLE_TYPES` or the JSON drifts from the enum.packages/cryptography/src/testVectors/generateNseTestVectors.ts (1)
77-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnbounded search loop for a leading-zero scalar.
The
for (let counter = 0; ; counter++)loop has no upper bound. It's statistically safe (finds a match in ~256 iterations) and only runs in a one-off dev script, but a hard cap with a clear error would make the failure mode explicit rather than relying purely on probability.♻️ Optional safety cap
function createLeadingZeroScalarKey(idPrefix: string): { key: TestVectorKey holder: PrivateKeyHolder } { - for (let counter = 0; ; counter++) { + const maxAttempts = 100_000 + for (let counter = 0; counter < maxAttempts; counter++) { const id = `${idPrefix}-${counter}` if (deterministicScalar(`vexl-nse-test-vectors-${id}`)[0] !== 0x00) continue return createDeterministicKey(id) } + throw new Error( + `Could not find a leading-zero scalar within ${maxAttempts} attempts` + ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cryptography/src/testVectors/generateNseTestVectors.ts` around lines 77 - 86, The createLeadingZeroScalarKey helper uses an unbounded for loop to શોધ a scalar with a leading zero, so add a fixed retry limit and fail explicitly if no match is found. Update createLeadingZeroScalarKey to keep the existing deterministicScalar and createDeterministicKey flow, but stop after a reasonable maximum number of attempts and throw a clear error mentioning the idPrefix when the cap is exceeded.packages/cryptography/src/testVectors/nseTestVectors.test.ts (2)
74-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse Effect
Array.filterinstead of native.filter().As per coding guidelines, "Prefer Effect
Arrayhelpers withpipeover native array methods likefilterandmap". Lines 82 and 89 use native.filter().♻️ Proposed fix
+import {Array as Arr, pipe} from 'effect' ... - it.each(vectors.filter((vector) => vector.valid))( + it.each(pipe(vectors, Arr.filter((vector) => vector.valid)))( 'vector $id decrypts to the expected plaintext', async (vector) => { expect(await runDecrypt(vector)).toEqual(vector.expectedPlaintext) } ) - it.each(vectors.filter((vector) => !vector.valid))( + it.each(pipe(vectors, Arr.filter((vector) => !vector.valid)))( 'vector $id fails to decrypt', async (vector) => { await expect(runDecrypt(vector)).rejects.toThrow() } )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cryptography/src/testVectors/nseTestVectors.test.ts` around lines 74 - 96, The test suite in eciesSuites is using native Array.filter in the two it.each calls, which should be replaced with Effect Array helpers to match the codebase style. Update the filtering in the describe block for the runDecrypt/it.each vectors setup to use the Effect Array API with pipe instead of calling .filter directly, while keeping the valid and invalid vector selection logic unchanged.Source: Coding guidelines
48-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse Effect
Arrayhelpers instead of native.map().As per coding guidelines, "Prefer Effect
Arrayhelpers withpipeover native array methods likefilterandmap". Line 49 uses native.map().♻️ Proposed fix
+import {Array as Arr, pipe} from 'effect' ... - const keyIds = new Set(vectorsFile.keys.map((key) => key.id)) + const keyIds = new Set(pipe(vectorsFile.keys, Arr.map((key) => key.id)))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cryptography/src/testVectors/nseTestVectors.test.ts` around lines 48 - 58, The test in nseTestVectors.test.ts uses a native Array .map() on vectorsFile.keys; replace that usage with the Effect Array helper style via pipe to match the project’s coding guidelines. Update the keyIds initialization in the it block so it derives ids from vectorsFile.keys using the appropriate Effect Array helper instead of key => key.id with native mapping, while keeping the rest of the vector validation logic in place.Source: Coding guidelines
apps/mobile/native/VexlNotificationCore/Tests/VexlNotificationCoreTests/CryptoVectorTests.swift (1)
102-120: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueInconsistent error handling:
try?silently swallows vector-load failures.Every other test in this file uses
try TestVectors.load()and fails loudly if the fixture can't be loaded. This test usestry?, silently falling back to an empty key string. Malformed-payload rejection doesn't strictly need real key data, but the inconsistency could mask a genuine fixture-loading regression in this test specifically.♻️ Proposed fix
- func testEciesLegacyRejectsMalformedPayloads() { - let vectors = try? TestVectors.load() - let key = vectors?.keys.first?.privateKeyPemBase64 ?? "" + func testEciesLegacyRejectsMalformedPayloads() throws { + let vectors = try TestVectors.load() + let key = try XCTUnwrap(vectors.keys.first?.privateKeyPemBase64)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/native/VexlNotificationCore/Tests/VexlNotificationCoreTests/CryptoVectorTests.swift` around lines 102 - 120, `testEciesLegacyRejectsMalformedPayloads` is using `try? TestVectors.load()`, which can hide fixture-loading failures by falling back to an empty private key. Change this test to use the same loud-loading pattern as the other tests in `CryptoVectorTests` so a bad `TestVectors.load()` call fails the test instead of silently continuing, while keeping the malformed payload loop and `eciesLegacyDecrypt` assertions unchanged.apps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Crypto/CryptoPrimitives.swift (1)
64-66: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePrefer throwing over
preconditionto preserve the graceful-bail contract.
preconditiontraps and crashes the NSE process on a size mismatch. Callers today pass invariant sizes (32-byte key, 16-byte counter), so this is defensive only, but converting to a thrownVexlCryptoError.aesCtrFailedkeeps the extension aligned with its "fall back to generic notification, never crash" design if a future caller violates the invariant.♻️ Optional: throw instead of trap
- precondition(key.count == kCCKeySizeAES256) - precondition(counterBlock.count == kCCBlockSizeAES128) + guard key.count == kCCKeySizeAES256, counterBlock.count == kCCBlockSizeAES128 else { + throw VexlCryptoError.aesCtrFailed + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Crypto/CryptoPrimitives.swift` around lines 64 - 66, Replace the defensive precondition checks in aes256Ctr(key:counterBlock:data:) with thrown validation so the NSE does not trap on bad input. Keep the existing key and counterBlock size checks, but if either size is invalid, throw VexlCryptoError.aesCtrFailed instead of calling precondition; preserve the current AES-CTR flow for valid inputs and update the function’s failure path so callers can gracefully bail out.apps/mobile/app.config.ts (1)
106-108: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winExtract the shared app-group id into a single constant.
`group.${extra.packageName}.shared`is duplicated betweeninfoPlist.VexlAppGroupand thecom.apple.security.application-groupsentitlement. These two must always match for the NSE's shared keychain access group to work — extracting a singleappGroupIdconstant and reusing it in both places would eliminate the risk of future drift.♻️ Suggested fix
+const sharedAppGroupId = `group.${extra.packageName}.shared` ... - 'VexlAppGroup': `group.${extra.packageName}.shared`, + 'VexlAppGroup': sharedAppGroupId, ... - 'com.apple.security.application-groups': [ - `group.${extra.packageName}.shared`, - ], + 'com.apple.security.application-groups': [sharedAppGroupId],Also applies to: 130-135
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/app.config.ts` around lines 106 - 108, The shared app-group identifier is duplicated between the VexlAppGroup infoPlist entry and the com.apple.security.application-groups entitlement, so extract it into a single appGroupId constant in app.config.ts and reuse it in both places. Update the app config logic around the infoPlist and ios.entitlements sections so the same constant is referenced for both the NSE shared keychain access group and the app group entitlement, preventing drift.apps/mobile/expo-plugins/with-nse-local-spm.js (1)
43-46: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueIdempotency check only covers package registration, not target linkage.
alreadyLinkedonly inspectsproject.rootObject.props.packageReferences; it doesn't verify the target'spackageProductDependenciesor frameworks build phase already include the product. A prior partial/interrupted run could leave the package referenced but not linked toVexlNSE, and this check would then skip re-linking on subsequent prebuilds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/expo-plugins/with-nse-local-spm.js` around lines 43 - 46, The idempotency guard in withNSELocalSPM only checks project.rootObject.props.packageReferences, so it can miss cases where VexlNSE is not yet linked. Update the alreadyLinked logic to also verify the VexlNSE target’s packageProductDependencies and frameworks/build phase entries for the package product, and only return early when both the package registration and target linkage are already present. Use the existing PACKAGE_RELATIVE_PATH and VexlNSE target lookup in withNSELocalSPM to keep the check comprehensive.apps/mobile/modules/vexl-nse-bridge/ios/VexlNseBridgeModule.swift (1)
71-130: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPartial-state risk in
syncAllon mid-loop failure.Deleting all keychain items for the service first, then re-adding entries one by one, means a failure partway through the loop leaves some previously-synced inbox keys permanently missing until the next successful sync trigger. Consider collecting per-entry failures and continuing, or building the full attribute set before performing the delete, to avoid transient gaps in the NSE's inbox key coverage.
♻️ Suggested approach
- for entry in payload.keys { - guard !entry.vexlToken.isEmpty else { - continue - } - let value: [String: String] = [ - "privateKeyPemBase64": entry.inboxPrivateKeyPemBase64, - "publicKeyPemBase64": entry.inboxPublicKeyPemBase64, - ] - try addKeychainItem( - service: NseBridgeStorage.inboxKeysService, - account: entry.vexlToken, - value: try jsonData(from: value), - accessGroup: appGroup - ) - } + var firstError: Error? + for entry in payload.keys { + guard !entry.vexlToken.isEmpty else { continue } + let value: [String: String] = [ + "privateKeyPemBase64": entry.inboxPrivateKeyPemBase64, + "publicKeyPemBase64": entry.inboxPublicKeyPemBase64, + ] + do { + try addKeychainItem( + service: NseBridgeStorage.inboxKeysService, + account: entry.vexlToken, + value: try jsonData(from: value), + accessGroup: appGroup + ) + } catch { + firstError = firstError ?? error + } + } + if let firstError { throw firstError }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/modules/vexl-nse-bridge/ios/VexlNseBridgeModule.swift` around lines 71 - 130, The current syncAll flow in VexlNseBridgeModule.syncAll deletes the inbox keychain service before re-adding keys, so a failure inside the payload.keys loop can leave the App Group keychain in a partially empty state. Update the logic to avoid destructive overwrite until the replacement data is fully prepared, or handle per-entry add failures while preserving existing entries; use the syncAll, deleteAllKeychainItems, and addKeychainItem flow as the main place to refactor the atomicity.apps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Protocol/ChatApiClient.swift (1)
82-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider typed Codable request bodies instead of untyped
[String: Any].
createChallenge/retrieveMessagesbuild request bodies as[String: Any]dictionaries encoded viaJSONSerialization. A smallEncodablerequest struct per endpoint would remove the risk of key-name typos (e.g."signedChallenge","publicKey") going undetected until a runtime request failure, and would align with the parsing side already done via typed structs likeServerMessage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Protocol/ChatApiClient.swift` around lines 82 - 164, The request payloads in ChatApiClient’s createChallenge, retrieveMessages, and send currently use untyped [String: Any] dictionaries, which makes key mistakes easy to miss; replace them with small endpoint-specific Encodable request models and update send to accept an Encodable body so JSON encoding is type-safe. Keep the existing behavior and endpoint names the same, but use typed request structs for fields like publicKey, signedChallenge, markAsPulled, and challenge/signature to align with the typed ServerMessage parsing already in place.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/mobile/native/VexlNotificationCore/scripts/generateNotificationLocalizations.mjs`:
- Around line 66-70: The fallback-locale check in
generateNotificationLocalizations.mjs only verifies that result.en exists, but
it does not confirm that en-base.json covers every
notifications.<TYPE>.title/body entry for all RENDERABLE_TYPES. Update the
validation in the generation flow to inspect the English fallback object and
ensure each renderable type has both title and body before writing outputs, and
throw a clear build-time error if any key pair is missing. Use the existing
generateNotificationLocalizations script and the RENDERABLE_TYPES-driven loop to
locate and enforce this completeness check.
In
`@apps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Protocol/ChatMessagePayload.swift`:
- Around line 59-84: Harden DecryptedChatMessage.parse(plaintextJson:) so the
time field only accepts real numeric JSON values and rejects booleans or other
non-timestamp values. Update the time extraction logic to mirror
NotificationPayloadParser.parseSentAt by checking the underlying NSNumber type
and explicitly excluding CFBooleanGetTypeID() before converting to Int64, while
keeping the existing nil-return failure path for invalid payloads.
In
`@apps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Rendering/NotificationRenderer.swift`:
- Around line 42-48: The message rendering in NotificationRenderer currently
uses message.text directly for .message notifications, so an empty string can
bypass the localized generic body fallback. Update the .message branch in
NotificationRenderer.render so it treats empty or whitespace-only message.text
the same as nil and falls back to interpolateThem(localized.body, them: them),
while keeping the existing displayName/title behavior unchanged.
In `@apps/mobile/src/state/notifications/nseBridge/syncNseBridgeActionAtom.ts`:
- Around line 45-71: The NSE bridge sync path can race with logout and
repopulate shared keychain data after it was cleared. In
syncNseBridgeActionAtom, re-check sessionAtom immediately before calling
syncAll(payload) and only proceed if the session is still loggedIn. Use the
existing session state guard in the same action to skip the write when logout
has already switched the session to loggedOut or unset.
---
Nitpick comments:
In `@apps/mobile/app.config.ts`:
- Around line 106-108: The shared app-group identifier is duplicated between the
VexlAppGroup infoPlist entry and the com.apple.security.application-groups
entitlement, so extract it into a single appGroupId constant in app.config.ts
and reuse it in both places. Update the app config logic around the infoPlist
and ios.entitlements sections so the same constant is referenced for both the
NSE shared keychain access group and the app group entitlement, preventing
drift.
In `@apps/mobile/expo-plugins/with-nse-local-spm.js`:
- Around line 43-46: The idempotency guard in withNSELocalSPM only checks
project.rootObject.props.packageReferences, so it can miss cases where VexlNSE
is not yet linked. Update the alreadyLinked logic to also verify the VexlNSE
target’s packageProductDependencies and frameworks/build phase entries for the
package product, and only return early when both the package registration and
target linkage are already present. Use the existing PACKAGE_RELATIVE_PATH and
VexlNSE target lookup in withNSELocalSPM to keep the check comprehensive.
In `@apps/mobile/modules/vexl-nse-bridge/ios/VexlNseBridgeModule.swift`:
- Around line 71-130: The current syncAll flow in VexlNseBridgeModule.syncAll
deletes the inbox keychain service before re-adding keys, so a failure inside
the payload.keys loop can leave the App Group keychain in a partially empty
state. Update the logic to avoid destructive overwrite until the replacement
data is fully prepared, or handle per-entry add failures while preserving
existing entries; use the syncAll, deleteAllKeychainItems, and addKeychainItem
flow as the main place to refactor the atomicity.
In
`@apps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Crypto/CryptoPrimitives.swift`:
- Around line 64-66: Replace the defensive precondition checks in
aes256Ctr(key:counterBlock:data:) with thrown validation so the NSE does not
trap on bad input. Keep the existing key and counterBlock size checks, but if
either size is invalid, throw VexlCryptoError.aesCtrFailed instead of calling
precondition; preserve the current AES-CTR flow for valid inputs and update the
function’s failure path so callers can gracefully bail out.
In
`@apps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Protocol/ChatApiClient.swift`:
- Around line 82-164: The request payloads in ChatApiClient’s createChallenge,
retrieveMessages, and send currently use untyped [String: Any] dictionaries,
which makes key mistakes easy to miss; replace them with small endpoint-specific
Encodable request models and update send to accept an Encodable body so JSON
encoding is type-safe. Keep the existing behavior and endpoint names the same,
but use typed request structs for fields like publicKey, signedChallenge,
markAsPulled, and challenge/signature to align with the typed ServerMessage
parsing already in place.
In
`@apps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Rendering/NotificationLocalization.swift`:
- Around line 44-54: Add a parity test to lock `RenderableMessageType` to the
bundled localization keys. In the `NotificationLocalization` test suite, verify
that `NotificationLocalization.loadBundled()!.entry(for:locale:)` returns a
non-nil result for every case in `RenderableMessageType.allCases` when using the
English locale. This should exercise the existing `entry(for:locale:)` and fail
fast if `RENDERABLE_TYPES` or the JSON drifts from the enum.
In
`@apps/mobile/native/VexlNotificationCore/Tests/VexlNotificationCoreTests/CryptoVectorTests.swift`:
- Around line 102-120: `testEciesLegacyRejectsMalformedPayloads` is using `try?
TestVectors.load()`, which can hide fixture-loading failures by falling back to
an empty private key. Change this test to use the same loud-loading pattern as
the other tests in `CryptoVectorTests` so a bad `TestVectors.load()` call fails
the test instead of silently continuing, while keeping the malformed payload
loop and `eciesLegacyDecrypt` assertions unchanged.
In `@packages/cryptography/src/testVectors/generateNseTestVectors.ts`:
- Around line 77-86: The createLeadingZeroScalarKey helper uses an unbounded for
loop to શોધ a scalar with a leading zero, so add a fixed retry limit and fail
explicitly if no match is found. Update createLeadingZeroScalarKey to keep the
existing deterministicScalar and createDeterministicKey flow, but stop after a
reasonable maximum number of attempts and throw a clear error mentioning the
idPrefix when the cap is exceeded.
In `@packages/cryptography/src/testVectors/nseTestVectors.test.ts`:
- Around line 74-96: The test suite in eciesSuites is using native Array.filter
in the two it.each calls, which should be replaced with Effect Array helpers to
match the codebase style. Update the filtering in the describe block for the
runDecrypt/it.each vectors setup to use the Effect Array API with pipe instead
of calling .filter directly, while keeping the valid and invalid vector
selection logic unchanged.
- Around line 48-58: The test in nseTestVectors.test.ts uses a native Array
.map() on vectorsFile.keys; replace that usage with the Effect Array helper
style via pipe to match the project’s coding guidelines. Update the keyIds
initialization in the it block so it derives ids from vectorsFile.keys using the
appropriate Effect Array helper instead of key => key.id with native mapping,
while keeping the rest of the vector validation logic in place.
In `@packages/rest-api/src/services/chat/contracts.ts`:
- Around line 196-202: The deployment-ordering requirement for the
chat-service/NSE read-only retrieve behavior is only documented and not
enforced. Update the relevant contract/docs around the field in contracts.ts and
the chat-service rollout path to add an explicit guard such as server capability
checking or client/version gating, so NSE-enabled app builds cannot ship before
the backend supports the field and the deletePulledMessages race is prevented.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1cf70735-7e68-48a9-8cd3-39aa5667186c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (55)
apps/chat-service/src/__tests__/routes/retrieveMessages.test.tsapps/chat-service/src/__tests__/utils/addChallengeForKey.tsapps/chat-service/src/routes/messages/retrieveMessages.tsapps/mobile/.gitignoreapps/mobile/app.config.tsapps/mobile/expo-plugins/with-nse-local-spm.jsapps/mobile/modules/vexl-nse-bridge/expo-module.config.jsonapps/mobile/modules/vexl-nse-bridge/index.tsapps/mobile/modules/vexl-nse-bridge/ios/VexlNseBridge.podspecapps/mobile/modules/vexl-nse-bridge/ios/VexlNseBridgeModule.swiftapps/mobile/native/VexlNotificationCore/.gitignoreapps/mobile/native/VexlNotificationCore/Package.resolvedapps/mobile/native/VexlNotificationCore/Package.swiftapps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Crypto/ChallengeSigner.swiftapps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Crypto/CryptoPrimitives.swiftapps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Crypto/EciesGtm.swiftapps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Crypto/EciesLegacy.swiftapps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Crypto/VexlKeys.swiftapps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/NotificationEnricher.swiftapps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Protocol/ChatApiClient.swiftapps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Protocol/ChatMessagePayload.swiftapps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Protocol/NotificationPayload.swiftapps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Rendering/NotificationContentApplier.swiftapps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Rendering/NotificationLocalization.swiftapps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Rendering/NotificationRenderer.swiftapps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Resources/notificationLocalizations.jsonapps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Storage/InboxKeyStore.swiftapps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Storage/MetadataStore.swiftapps/mobile/native/VexlNotificationCore/Sources/VexlNotificationCore/Storage/NseBridgeConstants.swiftapps/mobile/native/VexlNotificationCore/Tests/VexlNotificationCoreTests/BridgeContractTests.swiftapps/mobile/native/VexlNotificationCore/Tests/VexlNotificationCoreTests/CryptoVectorTests.swiftapps/mobile/native/VexlNotificationCore/Tests/VexlNotificationCoreTests/EnricherTests.swiftapps/mobile/native/VexlNotificationCore/Tests/VexlNotificationCoreTests/PayloadParsingTests.swiftapps/mobile/native/VexlNotificationCore/Tests/VexlNotificationCoreTests/RenderingTests.swiftapps/mobile/native/VexlNotificationCore/Tests/VexlNotificationCoreTests/TestVectors.swiftapps/mobile/native/VexlNotificationCore/scripts/generateNotificationLocalizations.mjsapps/mobile/package.jsonapps/mobile/src/App.tsxapps/mobile/src/state/notifications/nseBridge/syncNseBridgeActionAtom.tsapps/mobile/src/state/notifications/nseBridge/useSyncNseBridge.tsapps/mobile/src/state/useLogout.tsapps/mobile/src/utils/notifications/cancelNewChatNotifications.tsapps/mobile/src/utils/notifications/chatNotifications.tsapps/mobile/src/utils/notifications/nseEnrichedNotifications.tsapps/mobile/targets/vexl-nse/Info.plistapps/mobile/targets/vexl-nse/NotificationService.swiftapps/mobile/targets/vexl-nse/expo-target.config.jsapps/mobile/targets/vexl-nse/generated.entitlementspackages/cryptography/package.jsonpackages/cryptography/src/testVectors/generateNseTestVectors.tspackages/cryptography/src/testVectors/nseTestVectors.test.tspackages/cryptography/src/testVectors/nseTestVectorsFile.tspackages/cryptography/test-vectors/nse-test-vectors.jsonpackages/rest-api/src/services/chat/contracts.tspackages/rest-api/src/services/chat/index.ts
- declare VexlNSE extension to EAS (extra.eas.build.experimental.ios.appExtensions) so remote credentials cover the extension target - create/assign the NSE enrichment Task under stateLock so the timeout handler can always cancel in-flight work - parse the inbox private key once in the enricher and thread it through signChallenge/eciesLegacyDecrypt (was re-parsed per message) - harden the read-only retrieve test: snapshot client_version before and assert it is an unchanged concrete number after - document the bridge storage-contract ownership (NseBridgeConstants) and the BridgeContractTests drift guard at the CocoaPods write side - fail localization generation loudly when the en fallback misses any renderable type - reject boolean/non-numeric chat message time values (CFBoolean guard) - treat empty/whitespace message text like nil so the body falls back to the localized generic string
- make NSE-enriched notification dismissal error-tolerant: a failed dismissNotificationAsync is reported as a warning and can no longer block displayLocalNotification for the conversation - close the SEC1 curve-OID gap in VexlPrivateKey: params-less SEC1 keys are now rejected as unsupportedCurve (real Vexl keys are PKCS#8 or SEC1 with the OID present); regression tests for both forms
kaladivo
marked this pull request as draft
July 13, 2026 09:16
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
1 participant
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.
Summary
Replaces the generic iOS chat notification ("you have a new message") with a real preview — sender name + decrypted message text — rendered on-device by a Notification Service Extension, the way Signal does it. Android and the foreground WebSocket flow are untouched.
How it works: the push payload only carries an opaque
targetToken(vexl_nt_…). The NSE looks up the matching inbox private key in a shared keychain access group, signs a chat-service challenge, fetches the new messages read-only, decrypts them (eciesLegacy), and rewrites the notification content. On any failure — unknown token, non-secp256k1 key, network error, timeout, locked keychain — it delivers the original generic content, so the worst case is exactly today's behavior.The chat-service change (first commit) MUST be deployed to production before any app build containing the NSE ships. The new
markAsPulled: falserequest path is strictly read-only server-side. Against an old server the NSE request deterministically 500s before any mutation (missing client-version header hits the NOT NULL inbox-metadata write inside the transaction) — harmless generic fallback, but log noise. Do not add a client-version header to the NSE'sChatApiClientbefore the server ships; that would un-mask a pulled-marking message-loss race (documented inChatApiClient.swiftandcontracts.ts). The server change alone is fully backward compatible and can deploy immediately.Changes
markAsPulled(defaulttrue) onretrieveMessages; whenfalse, no pulled flags and no inbox-metadata writes. "Seen" semantics stay exclusively with the JS app.pnpm generate:nse-vectors) — eciesLegacy/GTM decrypt + ECDSA verify vectors pinned to the TS reference by a jest suite; consumed by the Swift tests.modules/vexl-nse-bridge): local Expo module; JS declaratively syncs vexl-token → inbox-key entries into a shared keychain group (kSecAttrAccessibleAfterFirstUnlock) and non-secret metadata (sender names, service URL, locale) into the App Group container. Cleared on logout and on logged-out startup. MMKV stays the source of truth — the bridge is purely additive.targets/vexl-nse+native/VexlNotificationCore): extension target via@bacons/apple-targets; testable SPM package (swift-secp256k1 + CryptoKit) with the crypto port, read-only chat client, payload parsing, and localized rendering with thread-grouping parity. 20s deadline, always-deliver guarantee.Privacy
No new server-side metadata: the payload is unchanged and the NSE fetch looks identical to the existing background fetch. Message content exists only on-device; the iOS system "Show Previews" setting (default: only when unlocked) governs lock-screen exposure — no in-app setting added. Keys move into the keychain (an upgrade over plain MMKV); the App Group store holds no secrets. Session credential headers are deliberately not synced — the NSE doesn't need them.
Testing
turbo:typecheck/turbo:lint/turbo:format— 29/29 eachdeletePulledMessages)markAsPulled: false+ a server-verifiable signatureexpo prebuildsmoke test: NSE target, local-SPM link, and app-group entitlements verified in the generated projectNot yet verified / follow-ups
.nsebundle ID + App Group; build image needs Xcode 16.3+ (swift-secp256k1 0.23.2). New target changes the fingerprint → new runtimeVersion.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
markAsPulledsupport for message retrieval to enable true read-only preview behavior.Bug Fixes