Bind every server push in the desktop app

[alexeyzimarev]

Closes #1046 — AI-2994

What & why

The app's hub connection sits in the server's UI-clients group, and every push there that carries an argument but has no handler threw an InvalidDataException that the SignalR client caught and dropped. The app now registers a no-op handler for each push it does not use, with the same number of arguments as the server sends (each taken as a JsonElement), and HubBroadcasts names every push a UI client can receive, including RawStreamAccessRevoked and WelcomeStateChanged from the server's audience transport. Leaving the app out of the group would cut off the permission, launch-failure and agent/daemon pushes it does consume.

Where to look

• A push's argument count is now as frozen as a hub method's: an added argument fails to bind in installed apps, and a consumed push is lost, not just noisy.
• The client's log reports ArgumentBindingFailure at Error, not Debug.
• A kcap-server guard test that fails when the server sends a push HubBroadcasts does not list remains a follow-up.

Verification

• EveryServerPushBindsToAHandler sends every HubBroadcasts push to the test hub and records the client's missing-handler and binding-failure logs. Without the no-op registrations it records a failure for every unused push (ArgumentBindingFailure:ActiveSessionAdded, MissingHandler:WelcomeStateChanged, …) on top of the 3 from its two deliberate unknown pushes; with them, exactly those 3.
• ServerConnectionServiceTests: 30/30. App suite: 2563/2564; Size_label_and_image_flag fails locally on a decimal comma (nb locale) and passes under en_US.UTF-8.
• dotnet build Capacitor.slnx --no-incremental: 0 warnings.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-19T13:24:33.844919Z	57c0fd3	PR opened

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[qodo-code-review]

PR Summary by Qodo

Bind every SignalR server push in the desktop app

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

AI Description

• Registers arity-correct no-op handlers for server pushes the desktop app ignores.
• Completes the shared broadcast vocabulary for every UI-client push.
• Verifies every declared push binds without missing-handler or argument-binding failures.

Diagram

sequenceDiagram
    actor Server
    participant Hub as Sessions Hub
    participant Client as Desktop Connection
    participant Used as App Observables
    participant NoOp as No-op Handlers
    Server->>Hub: Broadcast push
    Hub->>Client: Name and arguments
    alt Consumed push
        Client->>Used: Publish event
    else Unused push
        Client->>NoOp: Bind exact arity
    end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Dedicated desktop broadcast group

• ➕ Avoids sending irrelevant UI broadcasts to desktop clients
• ➕ Reduces client-side no-op registrations
• ➖ Requires server-side audience and subscription redesign
• ➖ Risks excluding permission, launch-failure, agent, or daemon pushes the app needs
• ➖ Still requires strict arity compatibility for consumed pushes

2. Generated shared broadcast contract

• ➕ Can validate server names and argument schemas from one source
• ➕ Reduces manual drift between server broadcasts and client handlers
• ➖ Introduces generation tooling and cross-project coupling
• ➖ Requires broader server changes beyond this targeted fix
• ➖ Migration complexity is disproportionate to the immediate defect

Recommendation: Keep the PR's explicit arity-correct handlers as the safest targeted fix because the desktop app must remain in the UI-client group. A future server-side guard generated from a shared broadcast manifest would complement this approach by detecting newly emitted or arity-changed pushes before release.

Files changed (5) +125 / -2

Bug fix (2) +27 / -2

ServerConnectionService.csBind unused SignalR pushes at their wire arity +17/-1

Bind unused SignalR pushes at their wire arity

• Adds optional SignalR client logging configuration for tests. Registers zero-argument and one-argument no-op handlers for broadcasts the desktop app does not consume, preventing missing-handler and binding failures.

src/Capacitor.App/Services/ServerConnectionService.cs

RemoteWire.csComplete the UI-client broadcast vocabulary +10/-1

Complete the UI-client broadcast vocabulary

• Adds constants for previously unnamed session, subagent, project, workflow, and raw-stream broadcasts. Documents that the desktop app handles every declared push.

src/Capacitor.Remote.Models/RemoteWire.cs

Tests (2) +85 / -0

ServerConnectionServiceTests.csVerify every server push binds successfully +57/-0

Verify every server push binds successfully

• Defines representative wire arguments for every HubBroadcasts constant and broadcasts each through the test hub. The test confirms only deliberate unknown pushes produce missing-handler or argument-binding logs.

test/Capacitor.App.Tests.Unit/ServerConnectionServiceTests.cs

UnboundPushRecorder.csRecord unbound SignalR push failures +28/-0

Record unbound SignalR push failures

• Adds a concurrent test logger that captures SignalR MissingHandler and ArgumentBindingFailure events by target name.

test/Capacitor.App.Tests.Unit/UnboundPushRecorder.cs

Documentation (1) +13 / -0

CHANGES.mdDocument the all-push binding invariant +13/-0

Document the all-push binding invariant

• Explains why the desktop app receives all UI-client broadcasts, why unused pushes still require handlers, and why broadcast arity is a compatibility contract.

docs/CHANGES.md

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. A logging comment describes a test ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The _hubLogging field comment describes which test consumes the callback and merely repeats that a
null callback is not configured. Because the note records a test implementation detail rather than a
behavior-critical constraint, later test refactoring can leave unrelated production documentation
stale.

Code

src/Capacitor.App/Services/ServerConnectionService.cs[R35-36]

+    /// The client's own logging, which a test records; null leaves the client unlogged.
+    readonly Action<ILoggingBuilder>? _hubLogging;

Relevance

●●● Strong

Recent history accepts removing redundant or test-specific comments under the comment-minimality
rule.

PR-#904
PR-#703

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Compliance rule 2762993 permits comments only for non-obvious, behavior-critical constraints. The
added comment explicitly refers to a test consumer and restates the nullable callback's
straightforward behavior.

Rule 2762993: Restrict comments to documenting non-obvious, behavior‑critical constraints
src/Capacitor.App/Services/ServerConnectionService.cs[35-36]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The `_hubLogging` comment documents its test consumer and repeats behavior already evident from the nullable type and guarded configuration call, rather than explaining a non-obvious production constraint.

## Fix Focus Areas
- src/Capacitor.App/Services/ServerConnectionService.cs[35-36]

## Recommended Fix
Remove the comment, or replace it with a concise explanation only if the logging callback has a non-obvious behavior-critical requirement that cannot be inferred from the field and its use.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

✅ Compliance rules (platform): 64 rules

✅ Cross-repo context — repo relationships

  Explored: repo: kurrent-io/kcap-server (sha: 2bebd0ee)

Review mode: ⚖️ Balanced: This changes SignalR runtime binding across many server push contracts and adds logging/test infrastructure, creating meaningful compatibility and behavioral risk, but not enough independent logic density to warrant extended review.

Tip of the day

💡 Did you know, you can describe a rule in plain language on the Rules page and Qodo drafts it for you

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 57c0fd3247

ℹ️ 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".

[chatgpt-codex-connector]

Bind ActiveSession payloads as JSON

When ActiveSessionAdded, ActiveSessionChanged, or ActiveSessionRemoved carries an object-shaped session payload, these string handlers still produce the ArgumentBindingFailure this change is intended to eliminate. The existing compatibility sinks for the same /hubs/sessions connection deliberately bind these broadcasts as JsonElement in src/Capacitor.Cli.Daemon/Services/ServerConnection.cs:386-388, while the new test only broadcasts fabricated string values, so it cannot catch this failure; use a payload type that accepts the actual JSON shape.

Useful? React with 👍 / 👎.

[alexeyzimarev]

Both server paths that send these, UiBroadcast and PublicHubTransport, pass the session id as a string, so they bind today, and the test's samples are those wire shapes. For a no-op handler, taking the argument as a JsonElement costs nothing and leaves the argument count as the only thing that has to match, so every unused push with an argument now binds that way (e630188).

[chatgpt-codex-connector]

Include the WelcomeStateChanged broadcast

Add WelcomeStateChanged to this registry and register its zero-argument no-op handler. The daemon's RegisterUiBroadcastSinks identifies it as a UI-only broadcast sent on the same /hubs/sessions connection (src/Capacitor.Cli.Daemon/Services/ServerConnection.cs:363-384), but it is absent here, so desktop clients still log MissingHandler whenever the server emits it. The reflection test cannot detect the omission because it derives its expected set from this incomplete class.

Useful? React with 👍 / 👎.

[alexeyzimarev]

Confirmed: PublicHubTransport sends it with no argument to audience groups the app joins. Added to HubBroadcasts and the unused no-argument pushes; the test failed with MissingHandler:WelcomeStateChanged before the registration (e630188). Catching a server push that is missing from HubBroadcasts needs a guard in kcap-server, which is tracked as a follow-up.