refactor(events): collapse three PairingRequired emission sites into one constructor - #2607
Conversation
Three call sites (bridge::router, channels::web::server, extensions::manager) hand-constructed AppEvent::OnboardingState with state=PairingRequired and identical 8-field payloads. Any new field on OnboardingState required touching all three sites and they could silently disagree. Collapse emission through a single associated function in ironclaw_common so auth_url/setup_url/state are invariant and the three sites only supply the fields that genuinely vary (request_id, thread_id, message, instructions, onboarding).
github-actions
Bot
added
scope: channel/web
There was a problem hiding this comment.
Pull request overview
Refactors pairing-required onboarding event emission to use a single constructor in ironclaw_common, reducing duplication and ensuring invariant fields (state, auth_url, setup_url) can’t drift across emit sites.
Changes:
- Added
OnboardingStateDto::pairing_required(...) -> AppEventconstructor incrates/ironclaw_common/src/event.rs. - Replaced three hand-built
AppEvent::OnboardingState { state: PairingRequired, ... }call sites with the new constructor. - Added unit tests in
ironclaw_commonto assert invariant fields + serialization shape.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
crates/ironclaw_common/src/event.rs |
Adds pairing_required constructor + tests to centralize invariant pairing event construction. |
src/bridge/router.rs |
Uses constructor when transitioning an auth-gated flow into pairing-required. |
src/channels/web/server.rs |
Uses constructor in /api/extensions/{name}/setup submit handler when pairing becomes required. |
src/extensions/manager.rs |
Uses constructor when activation detects a channel still needs pairing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a canonical pairing_required constructor for OnboardingStateDto to standardize the creation of AppEvent::OnboardingState events during pairing transitions. By centralizing this logic, the PR ensures that invariant fields such as auth_url and setup_url are consistently set to None across the bridge router, web server, and extension manager. Additionally, unit tests have been implemented to verify the constructor's field assignments and serialization behavior. I have no feedback to provide.
nearai#2607) Three call sites (bridge::router, channels::web::server, extensions::manager) hand-constructed AppEvent::OnboardingState with state=PairingRequired and identical 8-field payloads. Any new field on OnboardingState required touching all three sites and they could silently disagree. Collapse emission through a single associated function in ironclaw_common so auth_url/setup_url/state are invariant and the three sites only supply the fields that genuinely vary (request_id, thread_id, message, instructions, onboarding).
Summary
src/bridge/router.rs:2080,src/channels/web/server.rs:4010,src/extensions/manager.rs:7065) hand-constructAppEvent::OnboardingState { state: PairingRequired, ... }with identical 8-field payloads. Any new field added toOnboardingStaterequires touching all three, and they can silently disagree on invariants.OnboardingStateDto::pairing_required(extension_name, request_id, thread_id, message, instructions, onboarding)tocrates/ironclaw_common/src/event.rsas the single construction path.auth_url,setup_url, andstatebecome invariant inside the constructor; callers only supply what genuinely varies between sites.server.rs,router.rs, and extension/pairing code". This is the smallest, most isolated slice of that consolidation.Why now
After #2515 merged, #2574 (a6443dd) had to undo a duplication that landed 2 days later in
pending_gate_extension_name— evidence the leak is live. The pair-emit path is the next-highest-pain boundary:requeue_pairing_pending_gate(router) and the/api/extensions/{name}/setupsubmit handler (server) computerequest_idandthread_idvia different helpers, so a field rename or new optional field lands in one place and not the other.Notes
OnboardingStateDto::pairing_required(...) -> AppEventper the original consolidation proposal. Slightly unusual (an associated function on the state DTO returning anAppEvent), but it keeps the discoverability at theOnboardingStateDto::path where the variant itself lives.AuthRequired/SetupRequired/Ready/Failedemissions. Those have fewer duplicate sites and less invariant surface; a follow-up can consolidate them once this pattern is established.src/channels/web/types.rs(test_app_event_onboarding_state_pairing_required_serialize) continue to pass unchanged.Test plan
cargo fmtcargo clippy --all --benches --tests --examples --all-features— zero warningscargo test --lib -p ironclaw_common— 2 new constructor tests pass (field invariants + serialization)cargo test --lib --features libsql— all 4,998 unit tests pass, including existingtest_app_event_onboarding_state_pairing_required_serializeand the 10 onboarding/handler testsRegression test
The commit adds
pairing_required_constructor_sets_invariant_fieldsandpairing_required_constructor_serializes_to_onboarding_state_eventincrates/ironclaw_common/src/event.rs. Per.claude/rules/testing.mdthe rule is to test through the caller for helper-gated side effects — here the existingtest_app_event_onboarding_state_pairing_required_serializeinsrc/channels/web/types.rscontinues to exercise the wire format, and the three call-site refactors are covered by existing integration tests that round-tripAppEvent::OnboardingStateover SSE (e.g.test_chat_auth_token_handler_expired_auth_broadcasts_failed_onboarding_statepattern). No behavior change — pure compile-time refactor.