Fix/set signer on global subs #353
Conversation
WalkthroughCentralizes signer usage with a with_signer helper in NostrManager, adds signer-scoped subscription wrappers, moves per-user global-subscription refresh from User to Whitenoise, tightens a subscription function's visibility, adds Account::first with tests, and ensures integration tests create an identity when metadata or relays are expected. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant W as Whitenoise
participant DB as Database
participant Sec as SecretsStore
participant NM as NostrManager
rect rgba(200,230,255,0.25)
Note over W: Global subscription setup (new signer-aware flow)
W->>DB: fetch signer account (first/oldest)
alt signer found
W->>Sec: get signer keys
W->>NM: setup_batched_relay_subscriptions_with_signer(users_with_relays, default_relays, signer)
NM->>NM: with_signer -> lock & set signer
NM->>NM: setup_batched_relay_subscriptions(...)
NM-->>W: Result
NM->>NM: unset signer & unlock
else no signer
W-->>W: log "no signer" and return
end
end
sequenceDiagram
autonumber
participant EH as RelayListHandler
participant W as Whitenoise
participant DB as Database
participant Sec as SecretsStore
participant NM as NostrManager
rect rgba(255,240,200,0.25)
Note over EH: On relay-list change -> per-user refresh
EH->>W: refresh_global_subscription_for_user(user)
W->>DB: fetch signer account (first/oldest)
alt signer found
W->>Sec: get signer keys
W->>NM: refresh_user_global_subscriptions_with_signer(user_pubkey, users_with_relays, default_relays, signer)
NM->>NM: with_signer -> lock & set signer
NM->>NM: refresh_user_global_subscriptions(...)
NM-->>W: Result
NM->>NM: unset signer & unlock
else no signer
W-->>EH: warn and return
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ Finishing touches
🧪 Generate unit tests
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (11)
src/nostr_manager/subscriptions.rs (1)
99-116: Known batching limitation persists (rehash on user-count change).This PR doesn’t address the batch-ID reshuffling caused by
hash(pubkey) % batch_count, which can lead to stale/duplicated author lists when only the triggering user’s batch is refreshed. Not blocking here; consider consistent hashing (fixed ring/virtual nodes) to stabilize batch assignment.src/integration_tests/test_cases/user_discovery/find_or_create_user.rs (1)
91-94: Precondition makes tests reliable with signer-backed flows; consider avoiding redundant identity creation.Create an identity only when no accounts exist to reduce test time/noise across cases.
- if self.should_have_metadata || self.should_have_relays { - // Create an account: We need to have at least one account to be able to subscribe to events - context.whitenoise.create_identity().await?; - } + if (self.should_have_metadata || self.should_have_relays) + && context.whitenoise.get_accounts_count().await? == 0 + { + // Ensure at least one signer is present for subscription flows + context.whitenoise.create_identity().await?; + }src/whitenoise/mod.rs (4)
237-254: Graceful handling when signer keys are missing; optional non-signer fallback.Current code will bubble up errors if signer keys are absent, potentially aborting initialization. Consider logging and skipping (or falling back to non-signer setup) to avoid startup failure on key-store drift.
- let Some(signer_account) = Account::first(&whitenoise_ref.database).await? else { + let Some(signer_account) = Account::first(&whitenoise_ref.database).await? else { tracing::info!( target: "whitenoise::setup_global_users_subscriptions", "No signer account found, skipping global user subscriptions" ); return Ok(()); }; - - let keys = whitenoise_ref - .secrets_store - .get_nostr_keys_for_pubkey(&signer_account.pubkey)?; - - whitenoise_ref - .nostr - .setup_batched_relay_subscriptions_with_signer(users_with_relays, &default_relays, keys) - .await?; + let keys = match whitenoise_ref + .secrets_store + .get_nostr_keys_for_pubkey(&signer_account.pubkey) + { + Ok(k) => k, + Err(e) => { + tracing::warn!( + target: "whitenoise::setup_global_users_subscriptions", + "Signer keys unavailable ({}), skipping signer-backed setup", e + ); + // Optional best-effort fallback for public relays: + return whitenoise_ref + .nostr + .setup_batched_relay_subscriptions(users_with_relays, &default_relays) + .await + .map(|_| ()); + } + }; + + whitenoise_ref + .nostr + .setup_batched_relay_subscriptions_with_signer(users_with_relays, &default_relays, keys) + .await?;
375-401: Optional fallback path when no signer (or keys) on per-user refresh.Today you early-return when no signer; consider a best-effort non-signer refresh to keep global subs fresh on public relays.
- let Some(signer_account) = Account::first(&self.database).await? else { + let Some(signer_account) = Account::first(&self.database).await? else { tracing::info!( target: "whitenoise::users::refresh_global_subscription", "No signer account found, skipping global user subscriptions" ); return Ok(()); }; - - let keys = self - .secrets_store - .get_nostr_keys_for_pubkey(&signer_account.pubkey)?; - - self.nostr - .refresh_user_global_subscriptions_with_signer( - user.pubkey, - users_with_relays, - &default_relays, - keys, - ) - .await?; + let keys = match self.secrets_store.get_nostr_keys_for_pubkey(&signer_account.pubkey) { + Ok(k) => k, + Err(e) => { + tracing::warn!( + target: "whitenoise::users::refresh_global_subscription", + "Signer keys unavailable ({}), attempting non-signer refresh", e + ); + return self + .nostr + .refresh_user_global_subscriptions( + user.pubkey, + users_with_relays, + &default_relays, + ) + .await; + } + }; + + self.nostr + .refresh_user_global_subscriptions_with_signer( + user.pubkey, + users_with_relays, + &default_relays, + keys, + ) + .await?; Ok(())
232-253: Signer selection heuristic could be improved.Using
Account::first()(oldest) is arbitrary. If you track an active/primary account in settings, prefer that as signer; otherwise consider round-robin or “most recently used” to avoid overloading one account and to match user intent.Also applies to: 375-401
232-253: Verify signer guard behavior in wrappers — prefer an RAII guard.Wrappers in src/nostr_manager (e.g. setup_batched_relay_subscriptions_with_signer, refresh_user_global_subscriptions_with_signer, setup_account_subscriptions_with_signer and publisher paths) call self.client.set_signer(...) before awaiting work and then call unset_signer() afterward; they do not use an RAII guard — a panic/unwind or unexpected early exit could leave the signer set. Replace the manual set/unset pattern with a SignerGuard (set on construction, unset in Drop) or otherwise ensure unset_signer() runs on every path.
src/whitenoise/database/accounts.rs (2)
104-129: Preferfetch_optionaland add a deterministic tiebreaker in ORDER BY
- Simplifies control flow and avoids matching on
RowNotFound.- Breaks ties deterministically when
created_atcollides (common in tests) by addingid ASC.- pub(crate) async fn first(database: &Database) -> Result<Option<Account>, WhitenoiseError> { - match sqlx::query_as::<_, AccountRow>( - "SELECT * FROM accounts ORDER BY created_at ASC LIMIT 1", - ) - .fetch_one(&database.pool) - .await - { - Ok(account_row) => Ok(Some(account_row.into_account()?)), - Err(sqlx::Error::RowNotFound) => Ok(None), - Err(other) => Err(WhitenoiseError::Database(DatabaseError::Sqlx(other))), - } - } + pub(crate) async fn first(database: &Database) -> Result<Option<Account>, WhitenoiseError> { + let row_opt = sqlx::query_as::<_, AccountRow>( + "SELECT * FROM accounts ORDER BY created_at ASC, id ASC LIMIT 1", + ) + .fetch_optional(&database.pool) + .await + .map_err(DatabaseError::Sqlx)?; + + row_opt.map(|row| row.into_account()).transpose() + }
1200-1240: Stabilize the test against timestamp tiesCurrent assertions depend on
created_atordering; if two accounts share the same millisecond, results can flip. Either (a) adopt the SQL tie-breaker above (preferred) or (b) insert a tiny sleep between saves.Example (if you keep current SQL):
- let (test_account2, _) = Account::new(&whitenoise, Some(test_keys2)).await.unwrap(); + // Ensure distinct created_at for deterministic ordering + tokio::time::sleep(std::time::Duration::from_millis(2)).await; + let (test_account2, _) = Account::new(&whitenoise, Some(test_keys2)).await.unwrap();src/whitenoise/users.rs (1)
467-470: Consider light dedup/throttle for refresh calls
background_fetch_user_datamay trigger a refresh close to creation-time refresh, causing back-to-back work. Consider per-user debounce (e.g., a short in-memory TTL) to avoid redundant global subscription refreshes.src/nostr_manager/mod.rs (2)
272-291: Wrap set/unset signer with a reusable helper (reduces repetition, guards against early returns)Pattern repeats across methods. Introduce a small
with_signerhelper to ensure signer is always unset and to cut boilerplate.+ // Reusable helper + async fn with_signer<F, Fut, T>( + &self, + signer: impl NostrSigner + 'static, + f: F, + ) -> Result<T> + where + F: FnOnce() -> Fut, + Fut: std::future::Future<Output = Result<T>> + Send, + { + self.client.set_signer(signer).await; + let res = f().await; + self.client.unset_signer().await; + res + } + - pub(crate) async fn setup_batched_relay_subscriptions_with_signer( + pub(crate) async fn setup_batched_relay_subscriptions_with_signer( &self, users_with_relays: Vec<(PublicKey, Vec<RelayUrl>)>, default_relays: &[RelayUrl], signer: impl NostrSigner + 'static, ) -> Result<()> { - tracing::debug!(target: "whitenoise::nostr_manager::setup_batched_relay_subscriptions_with_signer","Setting up batched relay subscriptions with signer"); - self.client.set_signer(signer).await; - let result = async { - self.setup_batched_relay_subscriptions(users_with_relays, default_relays).await - }.await; - self.client.unset_signer().await; - result + tracing::debug!( + target: "whitenoise::nostr_manager::setup_batched_relay_subscriptions_with_signer", + "Setting up batched relay subscriptions with signer (users={}, defaults={})", + users_with_relays.len(), + default_relays.len() + ); + self.with_signer(signer, || async { + self.setup_batched_relay_subscriptions(users_with_relays, default_relays).await + }).await }
292-311: Apply the same helper to refresh wrapper and enrich logsUse
with_signerhere too; include the size ofusers_with_relaysfor traceability.- pub(crate) async fn refresh_user_global_subscriptions_with_signer( + pub(crate) async fn refresh_user_global_subscriptions_with_signer( &self, user_pubkey: PublicKey, users_with_relays: Vec<(PublicKey, Vec<RelayUrl>)>, default_relays: &[RelayUrl], signer: impl NostrSigner + 'static, ) -> Result<()> { - tracing::debug!(target: "whitenoise::nostr_manager::refresh_user_global_subscriptions_with_signer","Refreshing user global subscriptions with signer"); - self.client.set_signer(signer).await; - let result = async { - self.refresh_user_global_subscriptions(user_pubkey, users_with_relays, default_relays).await - }.await; - self.client.unset_signer().await; - result + tracing::debug!( + target: "whitenoise::nostr_manager::refresh_user_global_subscriptions_with_signer", + "Refreshing global subs for user {} with signer (users={}, defaults={})", + user_pubkey, + users_with_relays.len(), + default_relays.len() + ); + self.with_signer(signer, || async { + self.refresh_user_global_subscriptions(user_pubkey, users_with_relays, default_relays).await + }).await }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/integration_tests/test_cases/user_discovery/find_or_create_user.rs(1 hunks)src/nostr_manager/mod.rs(1 hunks)src/nostr_manager/subscriptions.rs(1 hunks)src/whitenoise/accounts.rs(1 hunks)src/whitenoise/database/accounts.rs(3 hunks)src/whitenoise/event_processor/event_handlers/handle_relay_list.rs(1 hunks)src/whitenoise/mod.rs(2 hunks)src/whitenoise/users.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/integration_tests/**
📄 CodeRabbit inference engine (.cursor/rules/mls-protocol-docs.mdc)
Place MLS integration tests under src/integration_tests
Files:
src/integration_tests/test_cases/user_discovery/find_or_create_user.rs
🧠 Learnings (6)
📓 Common learnings
Learnt from: jgmontoya
PR: parres-hq/whitenoise#310
File: src/whitenoise/accounts/mod.rs:669-692
Timestamp: 2025-08-11T10:25:55.436Z
Learning: In the Whitenoise codebase, the `NostrManager::publish_event_builder_with_signer` method automatically ensures relay connectivity by calling `self.add_relays()` before publishing events. This means explicit relay connectivity checks are not needed before calling publishing methods that use this function.
📚 Learning: 2025-09-04T03:38:54.953Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#340
File: src/nostr_manager/subscriptions.rs:72-89
Timestamp: 2025-09-04T03:38:54.953Z
Learning: In the global subscription batching system for Whitenoise (src/nostr_manager/subscriptions.rs), the current hash(pubkey) % batch_count approach causes batch ID reshuffling when user count changes, potentially leading to stale/duplicate author lists since only the triggering user's batch is refreshed. This is a known limitation documented in PR #340 that the team is aware of and will address in future iterations.
Applied to files:
src/whitenoise/mod.rssrc/nostr_manager/mod.rssrc/whitenoise/accounts.rssrc/whitenoise/users.rs
📚 Learning: 2025-08-11T10:25:55.436Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#310
File: src/whitenoise/accounts/mod.rs:669-692
Timestamp: 2025-08-11T10:25:55.436Z
Learning: In the Whitenoise codebase, the `NostrManager::publish_event_builder_with_signer` method automatically ensures relay connectivity by calling `self.add_relays()` before publishing events. This means explicit relay connectivity checks are not needed before calling publishing methods that use this function.
Applied to files:
src/whitenoise/mod.rssrc/nostr_manager/mod.rs
📚 Learning: 2025-08-11T10:23:59.406Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#310
File: src/whitenoise/accounts/relays.rs:260-278
Timestamp: 2025-08-11T10:23:59.406Z
Learning: In `src/whitenoise/accounts/relays.rs`, the `publish_relay_list_for_account` method is only called after the account setup is complete, which guarantees that `account.nip65_relays` is already populated and will never be empty. Therefore, no fallback logic is needed when `target_relays` is None.
Applied to files:
src/whitenoise/mod.rs
📚 Learning: 2025-08-11T10:25:55.436Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#310
File: src/whitenoise/accounts/mod.rs:669-692
Timestamp: 2025-08-11T10:25:55.436Z
Learning: In the Whitenoise codebase, the `NostrManager::publish_event_builder_with_signer` method automatically ensures relay connectivity by calling `self.ensure_relays_connected()` before publishing events. This means explicit relay connectivity checks are not needed before calling methods like `publish_relay_list_for_pubkey` that internally use `publish_event_builder_with_signer`.
Applied to files:
src/whitenoise/mod.rssrc/nostr_manager/mod.rs
📚 Learning: 2025-08-17T19:34:30.333Z
Learnt from: jmcorgan
PR: parres-hq/whitenoise#318
File: src/whitenoise/accounts/messages.rs:193-195
Timestamp: 2025-08-17T19:34:30.333Z
Learning: In the whitenoise codebase, `Account::create_nostr_mls()` returns `Result<NostrMls<NostrMlsSqliteStorage>, AccountError>`, and `WhitenoiseError` has an `Account(#[from] AccountError)` variant that automatically converts. When handling MLS creation errors, use the `?` operator to let `AccountError` convert naturally to `WhitenoiseError::Account` rather than manually mapping to `WhitenoiseError::Configuration`.
Applied to files:
src/whitenoise/database/accounts.rs
🧬 Code graph analysis (3)
src/whitenoise/mod.rs (3)
src/whitenoise/database/accounts.rs (2)
first(117-128)user(178-185)src/whitenoise/users.rs (1)
all_users_with_relay_urls(323-336)src/whitenoise/relays.rs (1)
defaults(78-93)
src/whitenoise/database/accounts.rs (2)
src/whitenoise/database/users.rs (1)
find_or_create_by_pubkey(101-120)src/whitenoise/mod.rs (2)
create_mock_whitenoise(449-508)new(56-70)
src/whitenoise/accounts.rs (1)
src/whitenoise/database/accounts.rs (1)
user(178-185)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: check (ubuntu-latest, native)
- GitHub Check: check (macos-latest, native)
🔇 Additional comments (5)
src/nostr_manager/subscriptions.rs (1)
274-283: Approve — visibility reduction is safeOnly internal usage found: definition at src/nostr_manager/subscriptions.rs:274 and calls at src/nostr_manager/mod.rs:206,257 — no external consumers detected in this repository.
src/whitenoise/event_processor/event_handlers/handle_relay_list.rs (1)
26-26: Route refresh through Whitenoise: centralizes signer-aware logic.Switching to
Whitenoise::refresh_global_subscription_for_usermatches the new signer-backed flow and keeps the handler slim. LGTM.src/whitenoise/accounts.rs (1)
436-445: Activate uses centralized per-user global refresh; sequencing looks correct.Calling
self.refresh_global_subscription_for_user(&user)during activation (after relays are set up for new accounts) is the right place. Proceed.src/whitenoise/database/accounts.rs (1)
374-374: Module-level import is goodConsolidating
create_mock_whitenoiseat module scope reduces duplication in tests.src/whitenoise/users.rs (1)
424-431: Good move to centralize per-user global refresh on WhitenoiseSwitching to
whitenoise.refresh_global_subscription_for_user(&user)aligns with the signer-scoped flow and reduces User’s responsibilities.
move refresh_global_subscription from User to Whitenoise as refresh_global_subscription_for_user require signer account for global subscription operations
jgmontoya
force-pushed
the
fix/set-signer-on-global-subs
branch
from
a489029 to
e7ec5ba
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/whitenoise/users.rs (1)
467-470: Avoid double refresh for newly-created usersNew users trigger a refresh here and also in
find_or_create_user_by_pubkeywhencreated == true. Consider gating or de-duping (e.g., a param tobackground_fetch_user_dataor a one-shot flag) to prevent redundant work.src/whitenoise/database/accounts.rs (1)
104-127: Deterministic “oldest account” selection — LGTM
ORDER BY created_at ASC, id ASC LIMIT 1is correct and stable;transpose()is idiomatic.If the table grows, consider a composite index on
(created_at, id)to keep this query fast.src/nostr_manager/mod.rs (2)
286-304: Avoid moving large vectors; accept slicesTake
&[(PublicKey, Vec<RelayUrl>)]and&[RelayUrl]to reduce clones and copies; forward as-is to the underlying method.-pub(crate) async fn setup_batched_relay_subscriptions_with_signer( - &self, - users_with_relays: Vec<(PublicKey, Vec<RelayUrl>)>, - default_relays: &[RelayUrl], +pub(crate) async fn setup_batched_relay_subscriptions_with_signer( + &self, + users_with_relays: &[(PublicKey, Vec<RelayUrl>)], + default_relays: &[RelayUrl], signer: impl NostrSigner + 'static, ) -> Result<()> { - self.with_signer(signer, || async { - self.setup_batched_relay_subscriptions(users_with_relays, default_relays).await + self.with_signer(signer, || async { + self.setup_batched_relay_subscriptions(users_with_relays.to_vec(), default_relays).await }) .await }(Adjust the callee signature similarly if feasible.)
305-321: Same slice-based signature adviceMirror the slice acceptance in this wrapper to avoid unnecessary moves.
-pub(crate) async fn refresh_user_global_subscriptions_with_signer( +pub(crate) async fn refresh_user_global_subscriptions_with_signer( &self, user_pubkey: PublicKey, - users_with_relays: Vec<(PublicKey, Vec<RelayUrl>)>, - default_relays: &[RelayUrl], + users_with_relays: &[(PublicKey, Vec<RelayUrl>)], + default_relays: &[RelayUrl], signer: impl NostrSigner + 'static, ) -> Result<()> { - self.with_signer(signer, || async { - self.refresh_user_global_subscriptions(user_pubkey, users_with_relays, default_relays).await + self.with_signer(signer, || async { + self.refresh_user_global_subscriptions(user_pubkey, users_with_relays.to_vec(), default_relays).await }) .await }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/integration_tests/test_cases/user_discovery/find_or_create_user.rs(1 hunks)src/nostr_manager/mod.rs(7 hunks)src/nostr_manager/subscriptions.rs(1 hunks)src/whitenoise/accounts.rs(1 hunks)src/whitenoise/database/accounts.rs(3 hunks)src/whitenoise/event_processor/event_handlers/handle_relay_list.rs(1 hunks)src/whitenoise/mod.rs(2 hunks)src/whitenoise/users.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/integration_tests/test_cases/user_discovery/find_or_create_user.rs
- src/whitenoise/accounts.rs
- src/nostr_manager/subscriptions.rs
- src/whitenoise/event_processor/event_handlers/handle_relay_list.rs
- src/whitenoise/mod.rs
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: jgmontoya
PR: parres-hq/whitenoise#310
File: src/whitenoise/accounts/mod.rs:669-692
Timestamp: 2025-08-11T10:25:55.436Z
Learning: In the Whitenoise codebase, the `NostrManager::publish_event_builder_with_signer` method automatically ensures relay connectivity by calling `self.add_relays()` before publishing events. This means explicit relay connectivity checks are not needed before calling publishing methods that use this function.
📚 Learning: 2025-08-17T19:34:30.333Z
Learnt from: jmcorgan
PR: parres-hq/whitenoise#318
File: src/whitenoise/accounts/messages.rs:193-195
Timestamp: 2025-08-17T19:34:30.333Z
Learning: In the whitenoise codebase, `Account::create_nostr_mls()` returns `Result<NostrMls<NostrMlsSqliteStorage>, AccountError>`, and `WhitenoiseError` has an `Account(#[from] AccountError)` variant that automatically converts. When handling MLS creation errors, use the `?` operator to let `AccountError` convert naturally to `WhitenoiseError::Account` rather than manually mapping to `WhitenoiseError::Configuration`.
Applied to files:
src/whitenoise/database/accounts.rs
📚 Learning: 2025-09-04T03:38:54.953Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#340
File: src/nostr_manager/subscriptions.rs:72-89
Timestamp: 2025-09-04T03:38:54.953Z
Learning: In the global subscription batching system for Whitenoise (src/nostr_manager/subscriptions.rs), the current hash(pubkey) % batch_count approach causes batch ID reshuffling when user count changes, potentially leading to stale/duplicate author lists since only the triggering user's batch is refreshed. This is a known limitation documented in PR #340 that the team is aware of and will address in future iterations.
Applied to files:
src/whitenoise/users.rs
📚 Learning: 2025-08-11T10:25:55.436Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#310
File: src/whitenoise/accounts/mod.rs:669-692
Timestamp: 2025-08-11T10:25:55.436Z
Learning: In the Whitenoise codebase, the `NostrManager::publish_event_builder_with_signer` method automatically ensures relay connectivity by calling `self.add_relays()` before publishing events. This means explicit relay connectivity checks are not needed before calling publishing methods that use this function.
Applied to files:
src/nostr_manager/mod.rs
📚 Learning: 2025-08-11T10:25:55.436Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#310
File: src/whitenoise/accounts/mod.rs:669-692
Timestamp: 2025-08-11T10:25:55.436Z
Learning: In the Whitenoise codebase, the `NostrManager::publish_event_builder_with_signer` method automatically ensures relay connectivity by calling `self.ensure_relays_connected()` before publishing events. This means explicit relay connectivity checks are not needed before calling methods like `publish_relay_list_for_pubkey` that internally use `publish_event_builder_with_signer`.
Applied to files:
src/nostr_manager/mod.rs
🧬 Code graph analysis (1)
src/whitenoise/database/accounts.rs (4)
src/whitenoise/mod.rs (2)
create_mock_whitenoise(449-508)new(56-70)src/whitenoise/accounts.rs (3)
account(1070-1076)account(1168-1174)new(51-70)src/whitenoise/database/users.rs (1)
find_or_create_by_pubkey(101-120)src/whitenoise/database/mod.rs (1)
new(52-96)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: check (macos-latest, native)
- GitHub Check: check (ubuntu-latest, native)
🔇 Additional comments (7)
src/whitenoise/users.rs (1)
424-431: Switched to Whitenoise-scoped refresh — LGTMCorrectly routes per-user global-subscription refresh through
Whitenoise::refresh_global_subscription_for_user. Error is logged without failing the flow.src/whitenoise/database/accounts.rs (2)
372-373: Test helper import — LGTMConsolidating
create_mock_whitenoiseimport is fine.
1198-1239: Comprehensive tests forfirst()— LGTMCovers empty, single, multiple, and deletion paths; tie-breaker by
idensures determinism even with equal timestamps.src/nostr_manager/mod.rs (4)
219-231: Signer-scoped account subscription setup — LGTMWrapper cleanly delegates via
with_signer.
240-249: Signer-scoped group messages subscription — LGTMConsistent with the new pattern.
270-284: Signer-scoped subscription update with cleanup — LGTMBuffer time and unsubscribe-before-setup look good; relies on
with_signer.
535-631: Signer-scoped full sync — LGTM (pending with_signer fix)Good refactor; once the mutex guard is added to
with_signer, this becomes safe under concurrency.Ensure upstream callers don’t concurrently invoke multiple signer-scoped ops until the guard is in place.
erskingardner
left a comment
erskingardner
left a comment
There was a problem hiding this comment.
I think the rabbit is right. I had a look and most of this newer subscription set up code runs in series internally to the methods, the log in flow still has the background_sync_account_data that triggers things happening on different threads at the same time. It could definitely cause an issue.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/nostr_manager/mod.rs (1)
332-334: Serialize unset_signer in delete_all_data to avoid racing with in-flight with_signer opsWithout the lock, logout/cleanup can toggle signer mid-op. Take the same mutex when unsetting.
pub(crate) async fn delete_all_data(&self) -> Result<()> { tracing::debug!( target: "whitenoise::nostr_manager::delete_all_data", "Deleting Nostr data" ); - self.client.unset_signer().await; - self.client.unsubscribe_all().await; + // Serialize signer state changes with ongoing signer-scoped operations. + let _guard = self.signer_lock.lock().await; + self.client.unset_signer().await; + drop(_guard); // don't block unsubscribe work + self.client.unsubscribe_all().await; Ok(()) }
🧹 Nitpick comments (1)
src/nostr_manager/mod.rs (1)
222-234: Use move closures to avoid borrowing outer stack across awaitUsing
move || async move { ... }avoids capturing by reference and makes lifetimes clearer across.await.- self.with_signer(signer, || async { + self.with_signer(signer, move || async move { self.setup_account_subscriptions(- self.with_signer(signer, || async { + self.with_signer(signer, move || async move { self.setup_group_messages_subscription(pubkey, nostr_group_ids, user_relays, None)- self.with_signer(signer, || async { + self.with_signer(signer, move || async move { let buffer_time = Timestamp::now() - Duration::from_secs(10);- self.with_signer(signer, || async { + self.with_signer(signer, move || async move { self.setup_batched_relay_subscriptions(users_with_relays, default_relays)- self.with_signer(signer, || async { + self.with_signer(signer, move || async move { self.refresh_user_global_subscriptions(user_pubkey, users_with_relays, default_relays)- self.with_signer(signer, || async { + self.with_signer(signer, move || async move { let mut contacts_and_self =Also applies to: 243-252, 273-287, 289-306, 308-324, 538-634
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/nostr_manager/mod.rs(8 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: jgmontoya
PR: parres-hq/whitenoise#310
File: src/whitenoise/accounts/mod.rs:669-692
Timestamp: 2025-08-11T10:25:55.436Z
Learning: In the Whitenoise codebase, the `NostrManager::publish_event_builder_with_signer` method automatically ensures relay connectivity by calling `self.add_relays()` before publishing events. This means explicit relay connectivity checks are not needed before calling publishing methods that use this function.
Learnt from: jgmontoya
PR: parres-hq/whitenoise#310
File: src/whitenoise/accounts/mod.rs:669-692
Timestamp: 2025-08-11T10:25:55.436Z
Learning: In the Whitenoise codebase, the `NostrManager::publish_event_builder_with_signer` method automatically ensures relay connectivity by calling `self.ensure_relays_connected()` before publishing events. This means explicit relay connectivity checks are not needed before calling methods like `publish_relay_list_for_pubkey` that internally use `publish_event_builder_with_signer`.
📚 Learning: 2025-08-11T10:25:55.436Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#310
File: src/whitenoise/accounts/mod.rs:669-692
Timestamp: 2025-08-11T10:25:55.436Z
Learning: In the Whitenoise codebase, the `NostrManager::publish_event_builder_with_signer` method automatically ensures relay connectivity by calling `self.add_relays()` before publishing events. This means explicit relay connectivity checks are not needed before calling publishing methods that use this function.
Applied to files:
src/nostr_manager/mod.rs
📚 Learning: 2025-08-11T10:25:55.436Z
Learnt from: jgmontoya
PR: parres-hq/whitenoise#310
File: src/whitenoise/accounts/mod.rs:669-692
Timestamp: 2025-08-11T10:25:55.436Z
Learning: In the Whitenoise codebase, the `NostrManager::publish_event_builder_with_signer` method automatically ensures relay connectivity by calling `self.ensure_relays_connected()` before publishing events. This means explicit relay connectivity checks are not needed before calling methods like `publish_relay_list_for_pubkey` that internally use `publish_event_builder_with_signer`.
Applied to files:
src/nostr_manager/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check (ubuntu-latest, native)
🔇 Additional comments (1)
src/nostr_manager/mod.rs (1)
58-58: Good call: serialize signer usage across clonesAdding
signer_lock: Arc<tokio::sync::Mutex<()>>and initializing it ensures global signer state is not raced across tasks. Looks correct and clone-safe.Also applies to: 185-186
| /// Reusable helper to execute operations with a temporary signer. | ||
| /// | ||
| /// This helper ensures that the signer is always unset after the operation completes, | ||
| /// even if the operation returns early or encounters an error. | ||
| async fn with_signer<F, Fut, T>(&self, signer: impl NostrSigner + 'static, f: F) -> Result<T> | ||
| where | ||
| F: FnOnce() -> Fut, | ||
| Fut: std::future::Future<Output = Result<T>> + Send, | ||
| { | ||
| let _guard = self.signer_lock.lock().await; | ||
| self.client.set_signer(signer).await; | ||
| let result = f().await; | ||
| self.client.unset_signer().await; | ||
| result | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make with_signer panic-safe to always unset the signer
If the inner future panics, unset_signer() won’t run and the client may remain stuck with a signer. Catch panics, then unset before returning.
Apply this diff in with_signer:
- let _guard = self.signer_lock.lock().await;
- self.client.set_signer(signer).await;
- let result = f().await;
- self.client.unset_signer().await;
- result
+ let _guard = self.signer_lock.lock().await;
+ self.client.set_signer(signer).await;
+ // Ensure signer cleanup even if the operation panics.
+ let result = std::panic::AssertUnwindSafe(f())
+ .catch_unwind()
+ .await;
+ self.client.unset_signer().await;
+ match result {
+ Ok(r) => r,
+ Err(_) => {
+ tracing::error!(
+ target: "whitenoise::nostr_manager::with_signer",
+ "Signer-scoped operation panicked"
+ );
+ Err(NostrManagerError::EventProcessingError(
+ "Signer-scoped operation panicked".into(),
+ ))
+ }
+ }Add this import near the other use lines:
use futures_util::FutureExt as _;💡 Verification agent
🧩 Analysis chain
Quick sanity check: ensure no stray set/unset outside the lock
Run this to verify only with_signer (and the locked delete_all_data) touch signer state.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Calls to set_signer/unset_signer:"
rg -nP '\b(set_signer|unset_signer)\s*\(' -g '!**/target/**' -n -C2
echo
echo "with_signer wrappers (ensure no nested with_signer calls inside them):"
rg -nP '\bwith_signer\s*\(' -g '!**/target/**' -n -C2Length of output: 182
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo
echo "Top-level files:"
ls -la | sed -n '1,200p' || true
echo
echo "ripgrep version (if available):"
rg --version || true
echo
echo "Debug run of previous pattern (shows why ripgrep may have skipped files):"
rg --debug -nP '\b(set_signer|unset_signer)\s*\(' || true
echo
echo "Search (no ignore, include hidden) for set_signer/unset_signer:"
rg -n --hidden --no-ignore -S '\b(set_signer|unset_signer)\s*\(' || true
echo
echo "Search (no ignore, include hidden) for with_signer (calls and defs):"
rg -n --hidden --no-ignore -S '\bwith_signer\s*\(' || true
rg -n --hidden --no-ignore -S '\bfn\s+with_signer\b' || true
echo
echo "Fallback: git grep (if inside a git work tree):"
if git rev-parse --is-inside-work-tree >/dev/null 2>&1; then
git grep -nE '\b(set_signer|unset_signer|with_signer)\b' || true
else
echo "Not inside a git work tree."
fi
echo
echo "Fallback: search .rs files with find+grep:"
rs_count=$(find . -type f -name '*.rs' | wc -l || true)
echo "Found .rs files: $rs_count"
if [ "${rs_count:-0}" -gt 0 ]; then
find . -type f -name '*.rs' -print0 | xargs -0 grep -nE '\b(set_signer|unset_signer|with_signer)\b' || true
else
echo "No .rs files found to search."
fiLength of output: 9483
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== src/nostr_manager/mod.rs (188-206) =="
nl -ba src/nostr_manager/mod.rs | sed -n '188,206p' || true
echo
echo "== src/nostr_manager/mod.rs (216-226) =="
nl -ba src/nostr_manager/mod.rs | sed -n '216,226p' || true
echo
echo "== src/nostr_manager/mod.rs (242-252) =="
nl -ba src/nostr_manager/mod.rs | sed -n '242,252p' || true
echo
echo "== src/nostr_manager/mod.rs (268-278) =="
nl -ba src/nostr_manager/mod.rs | sed -n '268,278p' || true
echo
echo "== src/nostr_manager/mod.rs (296-306) =="
nl -ba src/nostr_manager/mod.rs | sed -n '296,306p' || true
echo
echo "== src/nostr_manager/mod.rs (312-336) =="
nl -ba src/nostr_manager/mod.rs | sed -n '312,336p' || true
echo
echo "== src/nostr_manager/mod.rs (532-542) =="
nl -ba src/nostr_manager/mod.rs | sed -n '532,542p' || true
echo
echo "== src/nostr_manager/publisher.rs (196-220) =="
nl -ba src/nostr_manager/publisher.rs | sed -n '196,220p' || true
echo
echo "== src/integration_tests/core/test_clients.rs (1-30) =="
nl -ba src/integration_tests/core/test_clients.rs | sed -n '1,30p' || trueLength of output: 773
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo root: $(pwd)"
echo
echo "1) All matches for set_signer/unset_signer (no ignore):"
rg -n --hidden --no-ignore -S '\b(set_signer|unset_signer)\s*\(' || true
echo
echo "2) All matches for with_signer (defs and calls):"
rg -n --hidden --no-ignore -S '\bwith_signer\b' || true
echo
echo "3) Any uses of signer_lock or delete_all_data:"
rg -n --hidden --no-ignore -S 'signer_lock|delete_all_data' || true
echo
echo "---- File context: src/nostr_manager/mod.rs (180-210) ----"
sed -n '180,210p' src/nostr_manager/mod.rs || true
echo
echo "---- File context: src/nostr_manager/mod.rs (320-336) ----"
sed -n '320,336p' src/nostr_manager/mod.rs || true
echo
echo "---- File context: src/nostr_manager/mod.rs (532-540) ----"
sed -n '532,540p' src/nostr_manager/mod.rs || true
echo
echo "---- File context: src/nostr_manager/publisher.rs (192-220) ----"
sed -n '192,220p' src/nostr_manager/publisher.rs || true
echo
echo "---- File context: src/integration_tests/core/test_clients.rs (1-30) ----"
sed -n '1,30p' src/integration_tests/core/test_clients.rs || trueLength of output: 7355
Protect signer set/unset with signer_lock
- Unguarded signer calls found — fix by acquiring signer_lock or using with_signer:
- src/nostr_manager/publisher.rs:208–213 — calls self.client.set_signer(...) / unset_signer(...) without locking.
- src/nostr_manager/mod.rs:327–332 — delete_all_data calls self.client.unset_signer().await without locking.
- Tests only: src/integration_tests/core/test_clients.rs:12 sets signer in tests (acceptable).
- with_signer exists and locks (src/nostr_manager/mod.rs:193–201); prefer reusing it for publisher and guard delete_all_data.
🤖 Prompt for AI Agents
In src/nostr_manager/publisher.rs around lines 208–213 and
src/nostr_manager/mod.rs around lines 327–332, unguarded calls to
self.client.set_signer(...) and self.client.unset_signer() exist; replace those
by using the existing with_signer helper for operations that need a temporary
signer (wrap the publisher logic that sets a signer in self.with_signer(signer,
|| async move { ... }).await) and for delete_all_data either acquire the
signer_lock before calling unset_signer (let _guard =
self.signer_lock.lock().await; self.client.unset_signer().await) or refactor
delete_all_data to use with_signer if it needs to run a closure while the signer
is set; ensure every set_signer/unset_signer pair is protected by signer_lock to
prevent races.
Summary by CodeRabbit