perf(mobile): move profile sig checks off main thread - #7648
Conversation
Signed-off-by: Tom Brow <tomb@block.xyz>
🔐 Codex Security Review
|
|
@codex review |
|
@builderbot review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@buzz-security-review 2572c55 |
|
Codex Review: Didn't find any major issues. 🚀 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2572c55ad1
ℹ️ 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".
jedwards27
left a comment
There was a problem hiding this comment.
Review verdict: request changes
Reviewed base 4cd82f513214aad11c2b742ce7cc7c681e8e32a0 → exact head 2572c55ad1968376f190eeb415a41de5c5c8eb1d.
Blocking correctness defect: a confirmed profile save can be rolled back by an older in-flight batch
UserCacheNotifier.put() updates only state, without recording the profile event's NIP-01 replacement order (mobile/lib/shared/profile/user_cache_provider.dart:49-52). The production save path verifies that submittedEvent is the relay head, but then calls this orderless put(profile) (mobile/lib/features/profile/profile_provider.dart:219-239). If an older relay batch is paused across the new isolate await, _mergeParsedProfiles() later sees no corresponding newer entry in _profileEventOrders and overwrites the confirmed name/avatar/about/NIP-05/owner attribution (mobile/lib/shared/profile/user_cache_provider.dart:180-205).
This was reproduced independently through the production parser seam at this head: start refresh() with an old createdAt=1 event, pause the batch parser, apply put() with Saved locally, then release parsing. Expected Saved locally; actual Old relay profile.
Required change: make the confirmed local-save path update profile state and replacement order atomically—for example, feed submittedEvent through cacheProfileEvent(submittedEvent), or extend the cache API to accept and record the confirmed event order. Add a deterministic regression covering an in-flight parser followed by a confirmed local save, then assert the saved profile and owner attribution survive the stale batch.
Additional review evidence
- Traced isolate serialization, shared one-slot verification queue/recovery, malformed/stale event admission, generation fencing across community changes, live-event preservation, and shared NIP-OA verification; no second concrete defect found.
flutter analyze/ mobile checks passed.user_cache_provider_test.dart: 16/16 passed.nip_oa_test.dart: 7/7 passed.- Required GitHub
Clients / Mobile,Mobile, and DCO checks are green. - Independent full local Flutter runs each encountered one different unrelated test failure; each exact focused rerun passed. These are not attributed to this PR.
Native same-device responsiveness/AOT benchmarking was not independently reproduced. That is a reviewer/tooling confidence gap, not additional author action; the source-level timer mutation supports that the isolate boundary is causal.
Signed-off-by: Tom Brow <tomb@block.xyz>
|
@codex review |
|
@buzz-security-review f50e9fd |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
🤖 Addressed the confirmed-save race in f50e9fd81. The production save path now caches the confirmed event and its replacement order together. The new regression exercises the real save path while an older batch is paused and verifies that saved metadata and verified ownership survive. It fails before the fix; all 38 focused tests, 2,132 mobile tests, and static analysis pass. Re-review requested. |
jedwards27
left a comment
There was a problem hiding this comment.
Re-review verdict: approve
Re-reviewed base 4cd82f513214aad11c2b742ce7cc7c681e8e32a0 → exact head f50e9fd8128a2bc0c7655bda2faad43ee2a448be. The prior request for changes was not treated as inherited clearance.
The blocking stale-profile rollback is repaired. After relay confirmation, the production save path now feeds the exact submittedEvent through cacheProfileEvent (mobile/lib/features/profile/profile_provider.dart:219-241). That synchronous mutation parses the visible profile and verified owner attribution while recording the same event's NIP-01 replacement order before publishing state (mobile/lib/shared/profile/user_cache_provider.dart:98-102,207-216). An older isolate completion is consequently rejected by _isNewer during merge (mobile/lib/shared/profile/user_cache_provider.dart:180-205).
The new regression exercises the actual confirmed save path while an older production parser result is paused, and verifies display name, avatar, about, NIP-05, and owner attribution both before and after releasing the stale batch (mobile/test/features/profile/profile_provider_test.dart:80-150). Two independent re-review lanes mutation-proved it: replacing cacheProfileEvent(submittedEvent) with the old put(profile) makes the regression fail with expected Saved locally, actual Old relay profile; restoring the repair passes with clean trees.
No unresolved author-actionable defect was found after re-tracing parser serialization, queue recovery/coalescing, malformed and stale admission, community-generation retirement, live freshness, failure truthfulness, and NIP-OA equivalence. This PR changes no widget, layout, label, focus, or semantics code.
Exact-head evidence
- Full mobile suite: 2,132/2,132 passed independently in both lanes.
- Affected profile tests: 38/38 passed (systems lane); paired provider suites 31/31 passed (product lane).
just mobile-check: formatting unchanged; analyzer clean.just file-size-checkandgit diff --check: passed.- GitHub
Clients / Mobileand aggregateMobile: passed.
A reviewer-run same-device native AOT/frame-latency cohort remains outstanding. The timer regression establishes that real OA batch verification yields the main isolate, but it is not native performance characterization. Author action: none; verification owner: reviewer/tooling or release performance gate.
Current Rust unit and PostgreSQL leaf failures are on unchanged non-mobile code and have no changed-path causal link in this four-file Dart diff; mobile gates pass. Author action: none for this PR; verification owner: CI/release gate.
…ush-cache-offload * commit '41c5ace93740261ee5a5d962c2a17e8d846c2c1b': perf(mobile): move profile sig checks off main thread (#7648) perf(mobile): avoid redundant message list sorting (#7647) perf(mobile): avoid reparsing unchanged Markdown (#7649) Fix desktop onboarding regressions (#7659) Deduplicate thread context by ACP session delivery (#7620) feat(mobile): hard-block under-18 users on the store age signal (#4665) Document incoming Buzz turn structure (#7624) Refresh desktop onboarding flow (#7528) Fix Apple Silicon iOS simulator builds (#7646) Signed-off-by: Tom Brow <tomb@block.xyz>
The mobile app loads user profiles to show names, avatars, and agent ownership. Some profiles include a signature that proves who owns the agent. Checking these signatures for a batch of profiles can block interaction because the checks run on the same thread that draws the screen and handles input.
This change moves profile batch parsing and signature checks to a background worker. It processes one batch at a time and combines queued profile requests. It keeps the same signature checks, preserves newer profiles that arrive while a batch runs, and discards results after a community change. Individual live profile updates still use synchronous verification.
Tests cover newer live updates, community changes, overlapping requests, stale malformed events, and UI timer responsiveness through the production parser. The responsiveness test fails if parsing is moved back to the UI isolate. A regression also exercises a confirmed profile save while an older batch is paused, including preservation of verified ownership. All 38 focused tests, all 2,132 mobile tests, and the analyzer pass.
With 500 valid owner signatures, an AOT test reduced the main-isolate timer delay from about 3.4 seconds to less than a millisecond. Total verification time stayed similar. This measures verification work, not overall screen speed.
GitHub CI passes. The full local test run failed on two timing-sensitive ACP tests in unchanged Rust code; both passed isolated retries. All other local checks passed, including the downstream tests and builds run after that failure.