Refactor connection module into submodules

[leynos]

Summary

• Refactors the connection module into internal submodules: event, state, multi_packet, plus additional internal helper modules to isolate logic (dispatch, drain, frame, polling, response, shutdown).
• Moves RunState and ActorState into a new state.rs; moves multi-packet related types into multi_packet.rs; introduces internal Event enum in event.rs.
• Adjusts module wiring in src/connection/mod.rs and updates test support imports accordingly.

Changes

Module structure

• Added new files:
  • src/connection/event.rs: defines internal Event enum for ConnectionActor::next_event with variants Shutdown, High, Low, MultiPacket, Response, Idle.
  • src/connection/multi_packet.rs: defines MultiPacketStamp, MultiPacketTerminationReason, MultiPacketContext with methods to manage multi-packet channel and stamping.
  • src/connection/state.rs: defines RunState and ActorState with lifecycle management methods (new, mark_closed, start_shutdown, is_active, is_shutting_down, is_done).
  • src/connection/dispatch.rs: event dispatching for the connection actor, wiring to state and frame handling.
  • src/connection/drain.rs: drain logic and fairness helpers for high/low/multi queues.
  • src/connection/frame.rs: frame emission helpers including multi-packet stamping support.
  • src/connection/polling.rs: async polling utilities for shutdown, queue polling, and response polling.
  • src/connection/response.rs: streaming response handling for the connection actor.
  • src/connection/shutdown.rs: shutdown handling, including multi-packet channel closure and logging.
  • src/connection/mod.rs: updated to declare the new modules and import types used by tests, and removed inlined Event enum previously defined in mod.rs.
• Updated src/connection/test_support.rs to reflect the new module paths:
  • use multi_packet::MultiPacketTerminationReason
  • use state::ActorState
• Removed direct import of ActorState from the previous location.

Test support changes

• Updated test_support imports to reference new internal modules under connection:: (state, multi_packet) and test wiring preserved.

Backwards compatibility

• Internal APIs have been reorganized into internal modules, but public behaviour remains unchanged. External wiring and usage of the connection actor interfaces should be unaffected.

Test plan

• Compile workspace with new modules
• Run unit tests for connection subsystem
• Ensure test_support imports resolve

🌿 Generated by Terry

📎 Task: https://www.terragonlabs.com/task/05cb208e-ef51-4495-b6a6-d1f5d051f8cd

[sourcery-ai]

Reviewer's Guide

Refactors the monolithic connection module into three focused internal submodules (event, state, multi_packet), moving the Event enum, multi-packet channel/correlation logic, and actor lifecycle tracking into their own files while updating module wiring and test imports to preserve existing behaviour.

Class diagram for refactored connection state and multi-packet submodules

classDiagram

class Event~F,E~ {
  <<enumeration>>
  Shutdown
  High
  Low
  MultiPacket
  Response
  Idle
}

class MultiPacketStamp {
  <<enumeration>>
  Disabled
  Enabled
}

class MultiPacketTerminationReason {
  <<enumeration>>
  Drained
  Disconnected
  Shutdown
  +as_str() &'static str
}

class MultiPacketContext~F~ {
  -channel Option_mpsc_Receiver_F_
  -stamp MultiPacketStamp
  +new() MultiPacketContext_F_
  +install(channel Option_mpsc_Receiver_F_, stamp MultiPacketStamp) void
  +clear() void
  +channel_mut() Option_mpsc_Receiver_F__ref
  +take_channel() Option_mpsc_Receiver_F_
  +stamp() MultiPacketStamp
  +correlation_id() Option_u64_
  +is_active() bool
}

class RunState {
  <<enumeration>>
  Active
  ShuttingDown
  Finished
}

class ActorState {
  -run_state RunState
  +closed_sources usize
  +total_sources usize
  +new(has_response bool, has_multi_packet bool) ActorState
  +mark_closed() void
  +start_shutdown() void
  +is_active() bool
  +is_shutting_down() bool
  +is_done() bool
}

class ConnectionActor {
  -state ActorState
  -multi_packet MultiPacketContext_Frame_
  +next_event() Event_Frame,Error_
}

Event~F,E~ <.. ConnectionActor : uses
MultiPacketStamp <.. MultiPacketContext~F~ : uses
MultiPacketTerminationReason <.. MultiPacketContext~F~ : used_for_logging
RunState <.. ActorState : uses
ActorState <.. ConnectionActor : lifecycle_state
MultiPacketContext~F~ <.. ConnectionActor : multi_packet_handling

Loading

File-Level Changes

Change	Details	Files
Extract connection actor event handling into a dedicated internal event submodule.	Remove the inline generic Event<F, E> enum from the main connection module Introduce src/connection/event.rs defining the internal Event enum with the existing variants for Shutdown, High, Low, MultiPacket, Response, and Idle Wire the new event module into src/connection/mod.rs so ConnectionActor::next_event and related code use the moved Event type	src/connection.rs src/connection/mod.rs src/connection/event.rs
Move multi-packet channel state and correlation stamping into a focused multi_packet submodule.	Extract MultiPacketStamp, MultiPacketTerminationReason, and MultiPacketContext from the main connection module into src/connection/multi_packet.rs Keep the original behaviour of stamp handling, correlation_id computation, is_active checks, and debug assertions when installing channels Expose these types as pub(super) so they remain internal to the connection module but usable by the actor implementation and tests Update src/connection/mod.rs and test code to import MultiPacketTerminationReason from the new module	src/connection.rs src/connection/mod.rs src/connection/multi_packet.rs src/connection/test_support.rs
Move connection actor lifecycle management into a dedicated state submodule.	Extract RunState and ActorState from the main connection module into src/connection/state.rs Preserve the ActorState constructor logic, including closed_sources and total_sources accounting based on has_response and has_multi_packet Expose ActorState internals and methods as pub(super) so they can be used within the connection module while remaining non-public externally Update src/connection/mod.rs and test_support to import ActorState from the new state module	src/connection.rs src/connection/mod.rs src/connection/state.rs src/connection/test_support.rs
Adjust connection module wiring and test support imports to match the new internal layout.	Convert src/connection.rs into src/connection/mod.rs and declare the new submodules (event, state, multi_packet) Re-export or use the moved internal types where needed so the public API surface of the connection module remains unchanged Update src/connection/test_support.rs to reference state::ActorState and multi_packet::MultiPacketTerminationReason instead of their previous inline locations	src/connection.rs src/connection/mod.rs src/connection/test_support.rs

Possibly linked issues

• #0: The PR decomposes connection.rs into event, state, and multi_packet submodules, directly implementing the requested refactor.

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[coderabbitai]

Summary by CodeRabbit

Release Notes

• New Features
  • Introduced connection actor system for managing outbound frame delivery with support for priority-based queuing and multi-packet streams.
  • Added fairness configuration to balance high- and low-priority frame scheduling.
  • Implemented correlation stamping for multi-packet frame tracking.
  • Added response frame streaming support.
  • Introduced fragmentation capabilities for frame handling.
  • Added active connection counter for monitoring concurrent connections.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

Walkthrough

Summarise the Connection actor refactor: extract internal Event, MultiPacket and State modules; add fairness-aware draining, polling, response streaming, shutdown handling, multi-packet stamping and active-connection tracking; introduce ConnectionActor that selects from multiple sources and dispatches events until shutdown or stream exhaustion.

Changes

Cohort / File(s)	Summary
Core actor & API src/connection/mod.rs	Add ConnectionActor<F,E>, FairnessConfig, ConnectionChannels<F>, active-connection counter and ActiveConnection RAII guard; lifecycle constructors (new, with_hooks), configuration methods (fairness, fragmentation, response, multi-packet, low queue), run(...) main loop and next_event(...) selector.
Event enum src/connection/event.rs	New pub(super) enum Event<F,E> with variants Shutdown, High(Option<F>), Low(Option<F>), MultiPacket(Option<F>), Response(Option<Result<F, WireframeError<E>>>), Idle; derives Debug and documents select-loop semantics.
Multi-packet management src/connection/multi_packet.rs	New MultiPacketStamp, MultiPacketTerminationReason, and pub(super) MultiPacketContext<F> to manage optional multi-packet channel, stamping state and correlation id; install/clear/accessor methods.
Actor lifecycle state src/connection/state.rs	New internal RunState and pub(super) ActorState tracking run state, closed_sources and total_sources, with new, mark_closed, start_shutdown, and query helpers (is_active, is_shutting_down, is_done, closed_sources(), total_sources()).
Dispatch & selection src/connection/dispatch.rs, src/connection/event.rs	Add dispatch_event(...) that matches Event variants and delegates to per-source handlers; integrates with actor state and returns Result<(), WireframeError<E>>.
Drain & fairness src/connection/drain.rs	Implement process_high, process_low, process_multi_packet, process_queue, fairness tracking, opportunistic draining, and receiver-closure handling with a shared DrainContext.
Frame emission & fragmentation src/connection/frame.rs	Add multi-packet correlation stamping (apply_multi_packet_correlation, emit_multi_packet_frame), fragmenter-aware process_frame_with_hooks_and_metrics, and push_frame for pre-send hooks and metrics.
Async polling helpers src/connection/polling.rs	Add wait_shutdown, recv_push, poll_optional, poll_queue, and poll_response to uniformly poll optional receivers, response streams and shutdown token.
Response stream handling src/connection/response.rs	Add process_response and handle_response to consume FrameStream, handle protocol and other errors, emit end-of-stream frames, update state and invoke hooks/metrics.
Shutdown handling src/connection/shutdown.rs	Add process_shutdown, start_shutdown, handle_multi_packet_closed, and log_multi_packet_closure to close receivers, emit terminators, call lifecycle hooks and mark sources closed.
Test updates src/connection/test_support.rs	Update imports to state::ActorState, multi_packet::MultiPacketTerminationReason and hooks path; adjust tests to use total_sources()/closed_sources() accessors.

Sequence Diagram(s)

sequenceDiagram
    participant Selector as Select loop
    participant HighQ as High queue
    participant LowQ as Low queue
    participant Multi as MultiPacket channel
    participant Resp as Response stream
    participant Shutdown as Cancellation token
    participant Dispatcher as dispatch_event
    participant Out as Output collector

    Selector->>Shutdown: poll cancellation
    Selector->>HighQ: poll high queue
    Selector->>LowQ: poll low queue
    Selector->>Multi: poll multi-packet channel
    Selector->>Resp: poll response stream
    alt Shutdown signalled
        Selector->>Dispatcher: Shutdown
    else Frame from High
        Selector->>Dispatcher: High(frame)
    else Frame from Low
        Selector->>Dispatcher: Low(frame)
    else Frame from Multi
        Selector->>Dispatcher: MultiPacket(frame)
    else Response item
        Selector->>Dispatcher: Response(result)
    else Idle
        Selector->>Dispatcher: Idle
    end
    Dispatcher->>Selector: return outcome (ok / error)
    Dispatcher->>Out: emit frames (if any)

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

• Reduce file size hotspots by splitting into submodules #428 — Implements the refactor splitting src/connection.rs into connection/* submodules as requested.
• Refactor src/connection.rs: exceeds 400-line coding guideline #404 — Extracts ActorState, MultiPacketContext and Event handling into new modules, matching the issue's objectives.
• Untangle layering: move fragment_packet from app to dedicated module #431 — Adds fragmentation and fragmenter integration (enable_fragmentation, fragment processing) which touches the same functionality raised by the issue.

Poem

🧩 Modular actor hums, queues line up in time,
High, low and multi stamped with a rhyme;
Events selected, dispatched and queued,
Shutdown sighs softly, responses conclude. ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarises the main change: refactoring a connection module into submodules, which matches the changeset's primary objective.
Description check	✅ Passed	The description comprehensively documents the refactoring work, detailing new submodules created, structural changes, and test plan execution, all of which directly relate to the changeset.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch terragon/refactor-connection-modularization-h5bucu

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[sourcery-ai]

Hey - I've left some high level feedback:

• In state.rs, consider keeping ActorState’s closed_sources and total_sources fields private and exposing any needed access via methods to better preserve the internal lifecycle invariants.
• If RunState is only consumed within ActorState, you may be able to make it private to the module (or even the struct) rather than pub(super) to tighten encapsulation around the actor’s lifecycle.
• For the multi-packet helpers, double-check whether MultiPacketStamp and MultiPacketTerminationReason really need pub(super) visibility or whether they can remain private to multi_packet.rs to avoid leaking internal implementation details across the connection submodules.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- In `state.rs`, consider keeping `ActorState`’s `closed_sources` and `total_sources` fields private and exposing any needed access via methods to better preserve the internal lifecycle invariants.
- If `RunState` is only consumed within `ActorState`, you may be able to make it private to the module (or even the struct) rather than `pub(super)` to tighten encapsulation around the actor’s lifecycle.
- For the multi-packet helpers, double-check whether `MultiPacketStamp` and `MultiPacketTerminationReason` really need `pub(super)` visibility or whether they can remain private to `multi_packet.rs` to avoid leaking internal implementation details across the connection submodules.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/connection/mod.rs`:
- Around line 8-10: The file still exceeds the 400-line cap; find the large
helper functions/impl blocks remaining in this module and extract them into new
submodules (e.g., create files and add mod declarations similar to mod event /
mod multi_packet / mod state) preserving existing visibility (pub / pub(crate))
and re-exporting symbols used externally; update all use paths and references to
the moved functions/structs (for example move heavy helpers into new submodules
like helpers.rs or utils.rs and add pub use self::helpers::{...}) and run cargo
build/tests to fix any visibility or import errors.

[leynos]

@coderabbitai please create a new GitHub issue for the following:

• Reduce file size hotspots (all >400 LOC): src/connection.rs:1, src/extractor.rs:1, src/app/builder.rs:1, src/client/builder.rs:1, src/app/frame_handling.rs:1, src/codec/recovery.rs:1, src/server/config/tests.rs:1 — split into feature-focused submodules (and move #[cfg(test)] blocks into sibling *_tests.rs files where possible).

[leynos]

@coderabbitai please create a new GitHub issue for the following:

• ConnectionActor has mutually-exclusive “response vs multi-packet” state + runtime invariants (src/connection.rs:137, src/connection.rs:464); replace response+multi_packet with a single enum (e.g., ActiveOutput::{None, Response(_), MultiPacket(_)}) to eliminate the invariant and simplify the tokio::select! logic.

[leynos]

@coderabbitai please create a new GitHub issue for the following:

• Untangle layering: FairnessConfig lives in connection but is consumed by fairness (src/connection.rs:82, src/fairness.rs:10); move it into src/fairness.rs:1 and pub use it from connection to keep the public API stable.

[leynos]

@coderabbitai please create a new GitHub issue for the following:

• Untangle layering: fragment_packet is under app but used by the connection actor (src/app/fragment_utils.rs:1, src/connection.rs:52, src/connection.rs:662); move it to a non-app module (e.g., src/fragment/packet.rs) and re-export from app if you want to preserve the old path.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@leynos: I'll create a GitHub issue for refactoring the file size hotspots.

---

🏁 Script executed:

#!/bin/bash
# Create GitHub issue for file size hotspot refactoring

gh issue create \
  --repo leynos/wireframe \
  --title "Reduce file size hotspots by splitting into submodules" \
  --assignee leynos \
  --body "## Context

Related to PR `#424` which demonstrates the pattern for modularisation by refactoring the connection module.

Request from: `@leynos`
Source: https://github.com/leynos/wireframe/pull/424

## Objective

Reduce file size hotspots by splitting large files (all >400 LOC) into feature-focused submodules and, where possible, moving \`#[cfg(test)]\` blocks into sibling \`*_tests.rs\` files.

## Files to refactor

The following files should be split into feature-focused submodules:

- \`src/connection.rs:1\`
- \`src/extractor.rs:1\`
- \`src/app/builder.rs:1\`
- \`src/client/builder.rs:1\`
- \`src/app/frame_handling.rs:1\`
- \`src/codec/recovery.rs:1\`
- \`src/server/config/tests.rs:1\`

## Approach

Follow the pattern established in PR `#424`:
- Extract logically cohesive components into dedicated submodules
- Maintain public API compatibility
- Move test code into separate \`*_tests.rs\` files where appropriate
- Ensure all tests continue to pass after refactoring"

Length of output: 46

---

@leynos: I've created GitHub issue #428 for refactoring the file size hotspots: #428

The issue includes all seven files you specified and references this PR as a pattern for the modularisation approach.

[leynos]

@coderabbitai please create a new GitHub issue for the following:

Duplication / similarity

• Consolidate the TypeId→Any app-data store duplicated between WireframeApp and MessageRequest (src/app/builder.rs:46, src/extractor.rs:21) into an AppDataStore newtype with insert<T>()/get<T>().
• Builder type-transition patterns are inconsistent: WireframeApp::rebuild_with_params suppresses too_many_arguments (src/app/builder.rs:138) while the client uses a dedicated macro (src/client/builder.rs:38); consider standardising on one approach to reduce boilerplate and make future splits easier.
• Endianness helpers are partly centralised (src/byte_order.rs:1 only covers u16), but u32 parsing still uses a local lint expectation (src/codec.rs:185); extend byte_order to u32/u64 and reuse across codec + tests.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@leynos: I'll create a GitHub issue for untangling the FairnessConfig layering.

---

🏁 Script executed:

#!/bin/bash
# Create GitHub issue for FairnessConfig layering refactor

gh issue create \
  --title "Untangle layering: move FairnessConfig from connection to fairness module" \
  --body "## Problem

\`FairnessConfig\` is currently defined in the \`connection\` module (src/connection.rs:82) but is consumed by the \`fairness\` module (src/fairness.rs:10). This creates a layering violation where a lower-level module depends on configuration defined in a higher-level module.

## Desired Outcome

Move \`FairnessConfig\` into \`src/fairness.rs:1\` where it logically belongs, and \`pub use\` it from the \`connection\` module to maintain the existing public API surface and avoid breaking changes.

## Context

Raised by `@leynos` in PR `#424`: https://github.com/leynos/wireframe/pull/424
Comment: https://github.com/leynos/wireframe/pull/424#discussion_r${RANDOM}" \
  --assignee leynos

Length of output: 46

---

@leynos: I've created GitHub issue #429 for untangling the FairnessConfig layering: #429

The issue documents the layering violation where FairnessConfig is defined in connection but consumed by fairness, and proposes moving it to src/fairness.rs with a pub use re-export to maintain API compatibility.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Read the ConnectionActor implementation to understand the current state structure
cat src/connection/mod.rs | head -200

Length of output: 6158

---

🏁 Script executed:

#!/bin/bash
# Search for the response and multi_packet fields in ConnectionActor
rg -n "response:" src/connection/mod.rs -A2 -B2

Length of output: 870

---

🏁 Script executed:

#!/bin/bash
# Search for multi_packet field usage
rg -n "multi_packet:" src/connection/mod.rs -A2 -B2

Length of output: 664

---

🏁 Script executed:

#!/bin/bash
# Look for the runtime invariants mentioned (debug_assert or similar)
rg -n "debug_assert" src/connection/mod.rs -C3

Length of output: 2445

---

🏁 Script executed:

#!/bin/bash
# Create a GitHub issue for the ConnectionActor refactoring
gh issue create \
  --title "Refactor ConnectionActor to use single enum for mutually-exclusive response/multi-packet state" \
  --body "## Context

The \`ConnectionActor\` currently maintains two separate optional fields for handling streaming output:
- \`response: Option<FrameStream<F, E>>\`
- \`multi_packet: MultiPacketContext<F>\`

These fields are mutually exclusive by design, but this invariant is enforced only through runtime assertions at multiple points in the code:

- \`src/connection/mod.rs:245\`: \`set_response()\` asserts multi_packet is not active
- \`src/connection/mod.rs:257\`: \`set_multi_packet()\` asserts response is None
- \`src/connection/mod.rs:295\`: \`set_multi_packet_with_stamp()\` asserts response is None
- \`src/connection/mod.rs:366\`: \`run()\` asserts at most one is active

## Proposed Solution

Replace the two separate fields with a single enum that encodes the mutual exclusion at the type level:

\`\`\`rust
enum ActiveOutput<F, E> {
    None,
    Response(FrameStream<F, E>),
    MultiPacket(MultiPacketContext<F>),
}
\`\`\`

This approach would:
1. Eliminate the runtime invariant checks entirely (making violation a compile-time error)
2. Simplify the \`tokio::select!\` logic by removing conditional branches
3. Make the mutual exclusion explicit in the type system
4. Reduce cognitive overhead when reading and maintaining the actor code

## Benefits

- **Type safety**: Impossible states become unrepresentable
- **Simplified logic**: No need for runtime checks or conditional branches
- **Clearer intent**: The mutual exclusion is self-documenting in the type
- **Easier maintenance**: Future modifications won't accidentally violate the invariant

## Related

Follow-up to PR `#424` which refactored the connection module into submodules.

---

**Requested by:** `@leynos`  
**Source:** https://github.com/leynos/wireframe/pull/424#discussion_r${DISCUSSION_ID:-XXXXX}" \
  --assignee leynos \
  --repo leynos/wireframe

Length of output: 46

---

@leynos, I've created GitHub issue #430 for refactoring ConnectionActor to use a single enum for the mutually-exclusive response/multi-packet state.

Issue: #430

The issue documents:

• The four runtime invariant locations (src/connection/mod.rs:245, 257, 295, 366)
• The proposed ActiveOutput::{None, Response(_), MultiPacket(_)} enum solution
• Benefits including type safety, simplified logic, and elimination of runtime checks
• Reference to PR #424 as the related refactoring work

The issue has been assigned to you.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@leynos: I'll create a GitHub issue for untangling the fragment_packet layering.

---

🏁 Script executed:

#!/bin/bash
# Create GitHub issue for fragment_packet layering violation

gh issue create \
  --repo leynos/wireframe \
  --title "Untangle layering: move fragment_packet from app to dedicated module" \
  --assignee leynos \
  --body "## Context

Related to PR `#424` which demonstrates the pattern for modularisation by refactoring the connection module.

Request from: `@leynos`
Source: https://github.com/leynos/wireframe/pull/424

## Objective

Untangle layering violation where \`fragment_packet\` is under \`app\` but used by the connection actor. Move it to a non-\`app\` module and re-export from \`app\` if needed to preserve the old path.

## Current situation

- \`fragment_packet\` is defined in \`src/app/fragment_utils.rs:1\`
- Used by connection actor at \`src/connection.rs:52\` and \`src/connection.rs:662\`
- This creates a layering violation where the connection module depends on app-specific utilities

## Proposed solution

- Move \`fragment_packet\` to a non-\`app\` module (e.g., \`src/fragment/packet.rs\`)
- Re-export from \`app\` to preserve the old public API path if needed
- Update imports in \`src/connection.rs\` to reference the new location

## Approach

Follow the pattern established in PR `#424`:
- Extract the functionality into a dedicated module
- Maintain public API compatibility through re-exports where appropriate
- Ensure all tests continue to pass after refactoring"

Length of output: 46

---

@leynos: I've created GitHub issue #431 for untangling the fragment_packet layering violation: #431

The issue documents the current situation where fragment_packet is in src/app/fragment_utils.rs but used by the connection actor, and proposes moving it to a dedicated module (e.g., src/fragment/packet.rs) whilst re-exporting from app to preserve the old API path.

[leynos]

@coderabbitai please create a new GitHub issue for the following:

• Integration test helpers rely on file-level #![allow] due to per-test-crate compilation (tests/common/mod.rs:7); best fix is to move these helpers into the wireframe_testing crate (public API) and import them from tests, removing the dead_code/allow_attributes allowances.

[leynos]

@coderabbitai please create a new GitHub issue for the following:

• EOF classification in codec-error world steps is brittle string matching (tests/worlds/codec_error/decoder_ops.rs:101); since CodecError::Eof becomes an io::Error carrying EofError internally (src/codec/error.rs:298), downcast via io::Error::get_ref() to EofError and delete the heuristic.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@leynos: I'll create a GitHub issue for consolidating the duplication and similarity patterns.

---

🏁 Script executed:

#!/bin/bash
# Create GitHub issue for duplication/similarity consolidation

gh issue create \
  --repo leynos/wireframe \
  --title "Consolidate duplicate patterns and improve consistency" \
  --assignee leynos \
  --body "## Context

Related to PR `#424` which demonstrates the pattern for modularization by refactoring the connection module.

Request from: `@leynos`
Source: https://github.com/leynos/wireframe/pull/424

## Objective

Consolidate duplicate patterns and improve consistency across the codebase to reduce maintenance burden and improve future refactoring efforts.

## Areas to address

### 1. TypeId→Any app-data store consolidation

Consolidate the TypeId→Any app-data store duplicated between \`WireframeApp\` and \`MessageRequest\` (\`src/app/builder.rs:46\`, \`src/extractor.rs:21\`) into an \`AppDataStore\` newtype with \`insert<T>()/get<T>()\` methods.

### 2. Builder type-transition pattern standardization

Builder type-transition patterns are inconsistent: \`WireframeApp::rebuild_with_params\` suppresses \`too_many_arguments\` (\`src/app/builder.rs:138\`) while the client uses a dedicated macro (\`src/client/builder.rs:38\`). Consider standardizing on one approach to reduce boilerplate and make future splits easier.

### 3. Endianness helpers extension

Endianness helpers are partly centralized (\`src/byte_order.rs:1\` only covers \`u16\`), but \`u32\` parsing still uses a local lint expectation (\`src/codec.rs:185\`). Extend \`byte_order\` to \`u32\`/\`u64\` and reuse across codec + tests.

## Benefits

- Reduces code duplication
- Improves maintainability
- Provides consistent patterns for future development
- Simplifies future refactoring efforts"

Length of output: 46

---

@leynos: I've created GitHub issue #432 for consolidating duplicate patterns and improving consistency: #432

The issue includes all three areas you specified:

1. TypeId→Any app-data store consolidation into AppDataStore
2. Builder type-transition pattern standardisation
3. Endianness helpers extension to u32/u64

[leynos]

@coderabbitai please create a new GitHub issue for the following:

• Documentation inconsistency: WireframeProtocol docs show arbitrary ProtocolError (e.g., String) (src/hooks.rs:22), but WireframeApp::with_protocol only accepts ProtocolError = () today (src/app/builder.rs:294); either document this limitation in the builder docs, or generalise the app/connection pipeline to carry a ProtocolError type parameter.
• One module is missing the required //! module doc (src/fragment/tests.rs:1).

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@leynos: I'll create a GitHub issue for moving the integration test helpers into the wireframe_testing crate.

---

🏁 Script executed:

#!/bin/bash
# Create GitHub issue for moving integration test helpers to wireframe_testing crate

gh issue create \
  --repo leynos/wireframe \
  --title "Move integration test helpers from tests/common to wireframe_testing crate" \
  --assignee leynos \
  --body "## Context

Related to PR `#424` which demonstrates the pattern for modularisation by refactoring the connection module.

Request from: `@leynos`
Source: https://github.com/leynos/wireframe/pull/424

## Problem

Integration test helpers in \`tests/common/mod.rs:7\` rely on file-level \`#![allow]\` attributes to suppress \`dead_code\` and \`allow_attributes\` warnings. This is necessary because Rust's per-test-crate compilation model means that helpers used by only some integration tests appear unused within each test crate.

## Proposed Solution

Move these helpers into the \`wireframe_testing\` crate as part of the public API, then import them from integration tests. This approach:

1. Eliminates the need for \`#![allow(dead_code)]\` and \`#![allow(allow_attributes)]\` suppressions
2. Makes the helpers available as a proper public API for testing
3. Follows proper module boundaries and compilation unit structure
4. Allows the helpers to be reused across different test crates without duplication

## Benefits

- **Cleaner code**: Remove unnecessary lint allowances
- **Better structure**: Proper separation between production code and test utilities
- **Reusability**: Helpers become available for external users of the library
- **Maintainability**: Helpers are compiled once as part of the library, not per-test-crate

## Implementation Notes

- Extract helpers from \`tests/common/mod.rs\` into \`wireframe_testing\` crate
- Update integration tests to import from \`wireframe_testing\`
- Remove the \`#![allow]\` attributes from \`tests/common/mod.rs\`
- Ensure all integration tests continue to pass"

Length of output: 46

---

@leynos: I've created GitHub issue #433 for moving the integration test helpers into the wireframe_testing crate: #433

The issue documents:

• The current problem with file-level #![allow] attributes in tests/common/mod.rs:7
• The root cause (per-test-crate compilation model)
• The proposed solution to move helpers into wireframe_testing crate as public API
• Benefits including cleaner code, better structure, reusability, and maintainability
• Implementation notes for the migration

The issue has been assigned to you.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@leynos: I'll create a GitHub issue for replacing the brittle EOF classification with proper downcasting.

---

🏁 Script executed:

#!/bin/bash
# Create GitHub issue for EOF classification refactoring

gh issue create \
  --repo leynos/wireframe \
  --title "Replace brittle EOF string matching with type-safe downcasting" \
  --assignee leynos \
  --body "## Context

Related to PR `#424` which demonstrates the pattern for modularisation by refactoring the connection module.

Request from: `@leynos`
Source: https://github.com/leynos/wireframe/pull/424

## Objective

Replace brittle string matching for EOF classification in codec-error world steps with type-safe downcasting.

## Current situation

- \`tests/worlds/codec_error/decoder_ops.rs:101\` uses brittle string matching to classify EOF errors
- This is fragile and prone to breakage if error messages change

## Proposed solution

Since \`CodecError::Eof\` becomes an \`io::Error\` carrying \`EofError\` internally (\`src/codec/error.rs:298\`):

1. Downcast via \`io::Error::get_ref()\` to \`EofError\`
2. Delete the string-matching heuristic
3. Use type-safe error classification instead

## Benefits

- Type-safe error classification
- More robust to error message changes
- Eliminates brittle string matching
- Clearer intent and better maintainability"

Length of output: 46

---

@leynos: I've created GitHub issue #434 for replacing the brittle EOF string matching with type-safe downcasting: #434

The issue documents the current brittle string matching in tests/worlds/codec_error/decoder_ops.rs:101 and proposes using io::Error::get_ref() to downcast to EofError for type-safe error classification.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

@leynos: I'll create a GitHub issue for the documentation inconsistencies.

---

🏁 Script executed:

#!/bin/bash
# Create GitHub issue for documentation inconsistencies

gh issue create \
  --repo leynos/wireframe \
  --title "Fix documentation inconsistencies and missing module docs" \
  --assignee leynos \
  --body "## Context

Related to PR `#424` which refactored the connection module and highlighted documentation quality concerns.

Request from: `@leynos`
Source: https://github.com/leynos/wireframe/pull/424

## Objective

Address documentation inconsistencies and missing module documentation to improve developer experience and API clarity.

## Issues to address

### 1. WireframeProtocol documentation inconsistency

**Problem:**
\`WireframeProtocol\` documentation shows arbitrary \`ProtocolError\` types (e.g., \`String\`) at \`src/hooks.rs:22\`, but \`WireframeApp::with_protocol\` only accepts \`ProtocolError = ()\` today at \`src/app/builder.rs:294\`.

**Proposed solutions:**
- Option A: Document this limitation explicitly in the builder documentation to set correct expectations
- Option B: Generalise the app/connection pipeline to carry a \`ProtocolError\` type parameter to support arbitrary error types as documented

### 2. Missing module documentation

**Problem:**
The module \`src/fragment/tests.rs:1\` is missing the required \`//!\` module documentation comment.

**Solution:**
Add appropriate \`//!\` module documentation to \`src/fragment/tests.rs\` describing the module's purpose and contents.

## Benefits

- Improves API documentation accuracy
- Reduces developer confusion
- Ensures consistent documentation standards across the codebase"

Length of output: 46

---

@leynos: I've created GitHub issue #435 for the documentation inconsistencies: #435

The issue includes both concerns:

1. The WireframeProtocol documentation inconsistency between src/hooks.rs:22 and WireframeApp::with_protocol at src/app/builder.rs:294, with two proposed solutions (document the limitation or generalise the pipeline)
2. The missing //! module documentation in src/fragment/tests.rs:1

The issue has been assigned to you.

[leynos]

@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix:

src/connection/mod.rs

Comment on file

                "response stream is active"
            ),
        );
        self.multi_packet.install(channel, None);

❌ New issue: Code Duplication
The module contains 2 functions with similar structure: ConnectionActor.set_multi_packet,ConnectionActor.set_multi_packet_with_correlation

[coderabbitai]

Actionable comments posted: 3

🤖 Fix all issues with AI agents

In `@src/connection/drain.rs`:
- Around line 113-131: The match arm for QueueKind::High in
try_opportunistic_drain currently uses debug_assert!(false, ...) which
unconditionally panics in debug builds; replace it with a meaningful assertion
or an explicit unreachable to reflect intent: either add a guard so
QueueKind::High cannot reach this function (e.g., handle/exclude High earlier)
or change the arm to debug_assert!(some_condition_reflecting_invariant) and
return false, or if the branch truly cannot happen, use unreachable!() to signal
a hard invariant; update references to QueueKind::High and
try_opportunistic_drain accordingly so the code documents the intended invariant
instead of asserting false.

In `@src/connection/multi_packet.rs`:
- Around line 60-76: The doc comment for install currently omits that
correlation_id is ignored when channel is None; update the comment for the
install method to explicitly state that correlation_id is only used when channel
is Some and is ignored when channel is None (i.e., stamping is disabled via
MultiPacketStamp::Disabled), referencing the install method, the channel
parameter, correlation_id parameter, and the MultiPacketStamp enum to make the
behavior clear to callers.

In `@src/connection/polling.rs`:
- Around line 45-55: The small wrapper await_shutdown merely forwards to
wait_shutdown with no extra behavior; either remove the wrapper and replace all
call sites with Self::wait_shutdown(token).await to eliminate indirection, or
keep it but add a one-line doc comment to await_shutdown explaining its purpose
(e.g., a future extension or test seam) so callers understand why it exists;
update any references to await_shutdown accordingly and run tests to ensure no
behavioral changes.

[coderabbitai]

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In `@src/connection/drain.rs`:
- Around line 113-175: Remove the #[expect(clippy::unreachable, ...)] annotation
and change the QueueKind::High match arm in try_opportunistic_drain to be a
neutral, non-panicking branch: replace the unreachable! macro with a
debug_assert!(false, "try_opportunistic_drain(High) should not be called") (or
assert! in debug builds) and return false so the function remains exhaustive
without silencing the lint; keep QueueKind::Low and QueueKind::Multi logic
unchanged.

[leynos]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.