feat: add L2PS messaging client and protocol types#82
Merged
Conversation
- L2PSMessagingPeer: WebSocket client with auto-reconnect, event handlers, request-response pattern - l2ps_types: full protocol types (client/server messages, encryption, storage, errors) - L2PSInstantMessagingPayload: transaction payload type for L2PS-backed messaging - Updated Transaction union type and exports Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Caution Review failedPull request was closed or merged during review WalkthroughAdds a new L2PS WebSocket instant-messaging client and protocol types, re-exports, and related blockchain payload/transaction types; implements registration, send/history/discover/requestPublicKey flows, event handlers, reconnection, request/response waiters, and outgoing frame queuing. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Code
participant Peer as L2PSMessagingPeer
participant WS as WebSocket Server
participant Queue as Outgoing Queue
Client->>Peer: connect()
activate Peer
Peer->>WS: open WebSocket(serverUrl)
WS-->>Peer: onopen
Peer->>Peer: sign register proof
Peer->>WS: RegisterMessage
WS-->>Peer: RegisteredResponse (peers)
Peer-->>Client: connect() resolves
deactivate Peer
Client->>Peer: send(to, encrypted, messageHash)
activate Peer
Peer->>Queue: enqueue if socket not ready
Peer->>WS: SendMessage
Peer->>Peer: await [message_sent | message_queued]
WS-->>Peer: MessageSentResponse / MessageQueuedResponse
Peer-->>Client: send() resolves
deactivate Peer
WS->>Peer: IncomingMessage
activate Peer
Peer->>Peer: dispatch to message handlers
deactivate Peer
sequenceDiagram
participant Client as Client Code
participant Peer as L2PSMessagingPeer
participant WS as WebSocket Server
Client->>Peer: discover()
activate Peer
Peer->>WS: DiscoverMessage
Peer->>Peer: await DiscoverResponse
WS-->>Peer: DiscoverResponse (peer list)
Peer-->>Client: discover() resolves
deactivate Peer
WS->>Peer: PeerJoinedNotification
activate Peer
Peer->>Peer: update peers set
Peer-->>Client: invoke onPeerJoined handlers
deactivate Peer
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Contributor
Review Summary by QodoAdd L2PS messaging client and protocol types
WalkthroughsDescription• Add L2PSMessagingPeer WebSocket client with auto-reconnect and event handlers • Define comprehensive L2PS messaging protocol types for client-server communication • Add L2PSInstantMessagingPayload transaction type for L2PS-backed messaging • Update Transaction union types to support L2PS instant messaging transactions Diagramflowchart LR
A["L2PSMessagingPeer<br/>WebSocket Client"] -->|"sends/receives"| B["l2ps_types<br/>Protocol Types"]
B -->|"defines"| C["Message Envelope<br/>Encryption Types"]
A -->|"handles"| D["Connection<br/>Registration<br/>Messaging"]
E["L2PSInstantMessagingPayload"] -->|"extends"| F["Transaction Types"]
A -.->|"uses"| E
File Changes1. src/instant_messaging/L2PSMessagingPeer.ts
|
Contributor
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/instant_messaging/index.ts (1)
4-12: Use@/aliases for the new barrel re-exports.These new exports are still wired through relative paths. Switching them to the repository alias keeps this public entrypoint aligned with the SDK import convention.
♻️ Proposed change
-export { L2PSMessagingPeer } from "./L2PSMessagingPeer" +export { L2PSMessagingPeer } from "@/instant_messaging/L2PSMessagingPeer" export type { L2PSMessagingConfig, L2PSMessageHandler, L2PSErrorHandler, L2PSPeerHandler, L2PSConnectionStateHandler, -} from "./L2PSMessagingPeer" -export * from "./l2ps_types" +} from "@/instant_messaging/L2PSMessagingPeer" +export * from "@/instant_messaging/l2ps_types"As per coding guidelines, "Use
@/path aliases instead of relative imports".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instant_messaging/index.ts` around lines 4 - 12, The current barrel in index.ts exports L2PSMessagingPeer and related types via relative paths; update these to use the repository path alias (e.g., replace "./L2PSMessagingPeer" and "./l2ps_types" with the corresponding "@/..." alias) so the public entrypoint follows the SDK import convention; ensure the exported symbols L2PSMessagingPeer, L2PSMessagingConfig, L2PSMessageHandler, L2PSErrorHandler, L2PSPeerHandler, L2PSConnectionStateHandler and the re-export of l2ps_types all reference the alias form.src/instant_messaging/L2PSMessagingPeer.ts (1)
57-80: ModelIncomingFrameas a discriminated union and type the pending metadata storage instead of usingas anycasts.
IncomingFrame.payload,pendingResponsesvalue types, and the_metaattachment patterns all useany, bypassing type checking where frames are parsed and matched. ReplaceIncomingFramewith a union of the imported server response types (e.g.,RegisteredResponse | IncomingMessage | MessageSentResponse | ...), and refactor the pending entry storage to include a properly typedmetaproperty instead of mutating via(entry as any)._meta.Per coding guidelines: "Ensure full TypeScript type coverage."
Also applies to: 416–469
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instant_messaging/L2PSMessagingPeer.ts` around lines 57 - 80, Replace the ad-hoc IncomingFrame and the any-typed pendingResponses values with proper typed constructs: change IncomingFrame to a discriminated union of the actual server response interfaces (e.g., RegisteredResponse | IncomingMessage | MessageSentResponse | ... using the existing ServerMessageType discriminant), and update the pendingResponses Map value type to an interface that includes resolve, reject, timer and a typed meta property (e.g., meta?: PendingMetaType) so you stop using (entry as any)._meta; then update parsing/dispatch logic in L2PSMessagingPeer where frames are parsed and where pendingResponses entries are created/mutated to use entry.meta (with the correct type) and pattern-match on the discriminant to narrow payload types instead of treating payload as any.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/instant_messaging/l2ps_types.ts`:
- Around line 65-70: Update the wire frame types to carry a request correlation
id and make the request/response/error frames include it: add a required
requestId: string property to the payload shape used for request-response flows
(or add requestId: string directly on ProtocolFrame where you define
request/response/error variants) so messages round-trip with a stable id; then
update L2PSMessagingPeer.waitForResponse() to match incoming frames by that
requestId (see L2PSMessagingPeer.waitForResponse and the request/response/error
shapes currently guessed as "discover" or "history:${peerKey}" and the
ProtocolFrame<T> interface in l2ps_types.ts) so concurrent/late responses and
error frames can be correlated reliably.
In `@src/instant_messaging/L2PSMessagingPeer.ts`:
- Around line 119-143: The connect() promise can time out but leave the
checkOpen interval running, allowing register() to run later; fix by having the
timeout handler also clearInterval(checkOpen) and stop reconnection attempts
(e.g. set this.shouldReconnect = false and/or close the socket) so no further
register() is triggered after rejection; update the timeout callback in
connect() to clear the checkOpen interval and disable reconnection before
rejecting, and keep the existing register() path unchanged.
- Around line 335-349: onclose currently unsets _isRegistered but onopen simply
sets _isConnected and calls flushQueue, which leaves the peer permanently
unregistered and causes ensureRegistered() to fail; change the onopen handler
(and/or the reconnect flow in attemptReconnect/flushQueue) to attempt
re-registration (call the same registration logic used initially) before calling
notifyConnectionState("connected") and flushing queued frames, and if
re-registration fails close the socket and trigger attemptReconnect() (or retry
with backoff) instead of marking connected; ensure you reference and update
_isRegistered only after successful registration and use
notifyConnectionState("disconnected") when closing for retries.
In `@src/types/blockchain/TransactionSubtypes/InstantMessagingTransaction.ts`:
- Around line 13-20: The two transaction variants share the same discriminators
so runtime narrowing fails; change the L2PS-specific discriminators to unique
values (e.g. update L2PSInstantMessagingTransactionContent.type and its data[0]
from 'instantMessaging' to a unique token like 'l2psInstantMessaging') and
update any corresponding entries in the TransactionContentData union (in
Transaction.ts) so the union contains the new L2PS discriminator; ensure helpers
in src/types/blockchain/TransactionSubtypes/utils.ts that compare content.type
and data[0] will now correctly distinguish the L2PS variant from the non-L2PS
InstantMessaging variant.
---
Nitpick comments:
In `@src/instant_messaging/index.ts`:
- Around line 4-12: The current barrel in index.ts exports L2PSMessagingPeer and
related types via relative paths; update these to use the repository path alias
(e.g., replace "./L2PSMessagingPeer" and "./l2ps_types" with the corresponding
"@/..." alias) so the public entrypoint follows the SDK import convention;
ensure the exported symbols L2PSMessagingPeer, L2PSMessagingConfig,
L2PSMessageHandler, L2PSErrorHandler, L2PSPeerHandler,
L2PSConnectionStateHandler and the re-export of l2ps_types all reference the
alias form.
In `@src/instant_messaging/L2PSMessagingPeer.ts`:
- Around line 57-80: Replace the ad-hoc IncomingFrame and the any-typed
pendingResponses values with proper typed constructs: change IncomingFrame to a
discriminated union of the actual server response interfaces (e.g.,
RegisteredResponse | IncomingMessage | MessageSentResponse | ... using the
existing ServerMessageType discriminant), and update the pendingResponses Map
value type to an interface that includes resolve, reject, timer and a typed meta
property (e.g., meta?: PendingMetaType) so you stop using (entry as any)._meta;
then update parsing/dispatch logic in L2PSMessagingPeer where frames are parsed
and where pendingResponses entries are created/mutated to use entry.meta (with
the correct type) and pattern-match on the discriminant to narrow payload types
instead of treating payload as any.
🪄 Autofix (Beta)
✅ Autofix completed
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5788318b-c75a-49d1-86f5-306a375d5da6
📒 Files selected for processing (7)
src/instant_messaging/L2PSMessagingPeer.tssrc/instant_messaging/index.tssrc/instant_messaging/l2ps_types.tssrc/types/blockchain/Transaction.tssrc/types/blockchain/TransactionSubtypes/InstantMessagingTransaction.tssrc/types/blockchain/TransactionSubtypes/index.tssrc/types/instantMessaging/index.ts
Contributor
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 4 file(s) based on 4 unresolved review comments. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 4 file(s) based on 4 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
src/instant_messaging/L2PSMessagingPeer.ts (5)
165-170: Remove unused loop variable.The
keyvariable is extracted but never used. Either prefix it with underscore to indicate intentional discard, or iterate over values only.🧹 Proposed fix
// Reject all pending responses - for (const [key, pending] of this.pendingResponses) { + for (const [_key, pending] of this.pendingResponses) { clearTimeout(pending.timer) pending.reject(new Error("Disconnected")) }Or simpler:
// Reject all pending responses - for (const [key, pending] of this.pendingResponses) { + for (const pending of this.pendingResponses.values()) { clearTimeout(pending.timer) pending.reject(new Error("Disconnected")) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instant_messaging/L2PSMessagingPeer.ts` around lines 165 - 170, The loop over this.pendingResponses extracts an unused key; update the iteration to avoid the unused variable by iterating values only (e.g., use this.pendingResponses.values() or destructure to skip the key with an underscore) so you still call clearTimeout(pending.timer) and pending.reject(new Error("Disconnected")) and then call this.pendingResponses.clear(); keep references to pending.timer and pending.reject intact.
13-29: Remove unused imports and consider using path alias.Static analysis indicates
StoredMessage,PeerJoinedNotification, andPeerLeftNotificationare imported but never used. Additionally, per coding guidelines, prefer@/path aliases over relative imports.🧹 Proposed fix
import type { SerializedEncryptedMessage, ClientMessageType, ServerMessageType, RegisteredResponse, IncomingMessage, MessageSentResponse, MessageQueuedResponse, HistoryResponse, DiscoverResponse, PublicKeyResponse, - PeerJoinedNotification, - PeerLeftNotification, ErrorResponse, ErrorCode, - StoredMessage, -} from "./l2ps_types" +} from "@/instant_messaging/l2ps_types"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instant_messaging/L2PSMessagingPeer.ts` around lines 13 - 29, Remove the unused imports StoredMessage, PeerJoinedNotification, and PeerLeftNotification from the import list in L2PSMessagingPeer (they are not referenced anywhere in this file) and update the module import path from the relative "./l2ps_types" to the project path alias "@/instant_messaging/l2ps_types" (or the appropriate "@/..." alias used in the repo) so the import uses the canonical alias per guidelines; ensure only the actually used types (e.g., SerializedEncryptedMessage, ClientMessageType, ServerMessageType, RegisteredResponse, IncomingMessage, MessageSentResponse, MessageQueuedResponse, HistoryResponse, DiscoverResponse, PublicKeyResponse, ErrorResponse, ErrorCode) remain in the import statement.
78-82: Consider integratingPendingMetainto the pending response type.Using
(entry as any)._metato attach metadata is fragile. Integrating the metadata into the Map's value type would improve type safety.🧹 Proposed fix
+interface PendingResponseEntry { + resolve: (value: any) => void + reject: (error: Error) => void + timer: NodeJS.Timeout + expectedTypes: ServerMessageType[] +} // Pending request-response waiters - private pendingResponses: Map< - string, - { resolve: (value: any) => void; reject: (error: Error) => void; timer: NodeJS.Timeout } - > = new Map() + private readonly pendingResponses: Map<string, PendingResponseEntry> = new Map()Then update
waitForResponse:- const entry = { resolve, reject, timer } - // Attach metadata for matching - ;(entry as any)._meta = { types: expectedTypes } as PendingMeta - this.pendingResponses.set(requestId, entry) + this.pendingResponses.set(requestId, { resolve, reject, timer, expectedTypes })And
handleFrame:- const meta = (pending as any)._meta as PendingMeta | undefined - if (meta?.types.includes(frame.type)) { + if (pending.expectedTypes.includes(frame.type)) {Also applies to: 516-519
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instant_messaging/L2PSMessagingPeer.ts` around lines 78 - 82, The pendingResponses Map currently stores value objects without typed metadata and code uses (entry as any)._meta; introduce a PendingMeta type and change the Map value type to include meta, e.g. { resolve, reject, timer, meta: PendingMeta }, then update any places that create or read entries (notably waitForResponse and handleFrame) to set and read entry.meta instead of using (entry as any)._meta so the metadata is strongly typed and you can drop the casts; also update other similar maps/usages around lines referenced (516-519) to the same typed shape.
459-479: Simplify frame handling with optional chaining and remove unnecessary assertions.The non-null assertions after
.has()checks are redundant. Use optional chaining for cleaner code.🧹 Proposed fix
private handleFrame(frame: IncomingFrame): void { // First, check if any pending response matches by requestId - if (frame.requestId && this.pendingResponses.has(frame.requestId)) { - const pending = this.pendingResponses.get(frame.requestId)! + if (frame.requestId) { + const pending = this.pendingResponses.get(frame.requestId) + if (!pending) { + // No waiter, fall through to event dispatch + } else { const meta = (pending as any)._meta as PendingMeta | undefined - if (meta && meta.types.includes(frame.type)) { + if (meta?.types.includes(frame.type)) { clearTimeout(pending.timer) this.pendingResponses.delete(frame.requestId) pending.resolve(frame.payload) return } + } } // Handle error frames with requestId - if (frame.type === "error" && frame.requestId && this.pendingResponses.has(frame.requestId)) { - const pending = this.pendingResponses.get(frame.requestId)! + if (frame.type === "error" && frame.requestId) { + const pending = this.pendingResponses.get(frame.requestId) + if (pending) { clearTimeout(pending.timer) this.pendingResponses.delete(frame.requestId) pending.reject(new Error(frame.payload?.message || "Server error")) return + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instant_messaging/L2PSMessagingPeer.ts` around lines 459 - 479, In handleFrame, remove redundant non-null assertions after Map.has() by using optional chaining and early-returns: when checking this.pendingResponses.has(frame.requestId) replace the get(...)! usage with const pending = this.pendingResponses.get(frame.requestId); guard with if (!pending) return; use pending?._meta and pending?.timer etc.; similarly in the error frame block get the pending via get and guard instead of using !, and use optional chaining for frame.payload?.message when constructing the Error; ensure you still clearTimeout, delete from this.pendingResponses, and call pending.resolve/pending.reject as before.
68-96: Mark non-reassigned members asreadonly.Static analysis correctly identifies that several class members are never reassigned. Marking them
readonlyprevents accidental mutation and clarifies intent.🧹 Proposed fix
export class L2PSMessagingPeer { private ws: WebSocket | null = null - private config: L2PSMessagingConfig + private readonly config: L2PSMessagingConfig // Event handlers - private messageHandlers: Set<L2PSMessageHandler> = new Set() - private errorHandlers: Set<L2PSErrorHandler> = new Set() - private peerJoinedHandlers: Set<L2PSPeerHandler> = new Set() - private peerLeftHandlers: Set<L2PSPeerHandler> = new Set() - private connectionStateHandlers: Set<L2PSConnectionStateHandler> = new Set() + private readonly messageHandlers: Set<L2PSMessageHandler> = new Set() + private readonly errorHandlers: Set<L2PSErrorHandler> = new Set() + private readonly peerJoinedHandlers: Set<L2PSPeerHandler> = new Set() + private readonly peerLeftHandlers: Set<L2PSPeerHandler> = new Set() + private readonly connectionStateHandlers: Set<L2PSConnectionStateHandler> = new Set() // Pending request-response waiters - private pendingResponses: Map< + private readonly pendingResponses: Map< string, { resolve: (value: any) => void; reject: (error: Error) => void; timer: NodeJS.Timeout } > = new Map() // State private _isConnected = false private _isRegistered = false private onlinePeers: Set<string> = new Set() - private messageQueue: OutgoingFrame[] = [] + private readonly messageQueue: OutgoingFrame[] = [] // Reconnection private reconnectAttempts = 0 - private maxReconnectAttempts = 10 - private baseReconnectDelay = 1000 + private readonly maxReconnectAttempts = 10 + private readonly baseReconnectDelay = 1000🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instant_messaging/L2PSMessagingPeer.ts` around lines 68 - 96, Several instance fields in L2PSMessagingPeer are never reassigned; mark them readonly to prevent accidental reassignment. Update the declarations in class L2PSMessagingPeer for config, messageHandlers, errorHandlers, peerJoinedHandlers, peerLeftHandlers, connectionStateHandlers, pendingResponses, onlinePeers, messageQueue, baseReconnectDelay, and maxReconnectAttempts to include the readonly modifier (leave fields that are mutated such as ws, _isConnected, _isRegistered, reconnectAttempts, reconnectTimeout, shouldReconnect, isReconnecting unchanged). This keeps existing mutation of collection contents intact while preventing reassignment of the references.src/instant_messaging/l2ps_types.ts (1)
229-237: Consider adding a"read"status for delivery receipts.The
MessageStatustype covers the message lifecycle well, but lacks a status for when the recipient has read the message. If read receipts are planned, consider adding this now to avoid a breaking change later.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/instant_messaging/l2ps_types.ts` around lines 229 - 237, The MessageStatus union type is missing a "read" state for delivery receipts; update the MessageStatus type definition (the exported type named MessageStatus in l2ps_types.ts) to include a "read" variant (e.g., | "read") and ensure any switch/case, serializers, database enums, and tests that consume MessageStatus are updated to handle the new "read" value so this addition won't break type narrowing elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/instant_messaging/L2PSMessagingPeer.ts`:
- Around line 559-561: The generateRequestId function uses the deprecated
String.prototype.substr; replace that call in generateRequestId with a
non-deprecated alternative (e.g., Math.random().toString(36).substring(2, 11) or
.slice(2, 11)) to preserve the same 9-character extraction, so update the return
expression in generateRequestId to use substring (or slice) instead of substr.
- Around line 392-396: The WebSocket error handler currently passes useless
details via String(event); update the this.ws.onerror callback (the handler that
invokes this.errorHandlers) to provide meaningful information by extracting
event.type when available or event.message if event is an Error, and fall back
to a safe JSON serialization (e.g., JSON.stringify with try/catch) or an empty
string; ensure the ErrorCode stays "INTERNAL_ERROR" and that the message remains
"WebSocket error" while replacing details: String(event) with the
extracted/serialized value.
- Around line 363-374: The catch block that handles re-registration failures
currently clears state, closes the socket and triggers reconnection but swallows
the error; update it to notify consumers by invoking an error-notification path
(e.g., call this.notifyError(err) or emit an 'error' event) before/after calling
notifyConnectionState("disconnected") so handlers can react and debug, and if
notifyError/emit isn't implemented add a small helper (e.g., notifyError) that
forwards the error to registered listeners; keep the existing state changes and
the existing calls to this.ws.close() and this.attemptReconnect().
---
Nitpick comments:
In `@src/instant_messaging/l2ps_types.ts`:
- Around line 229-237: The MessageStatus union type is missing a "read" state
for delivery receipts; update the MessageStatus type definition (the exported
type named MessageStatus in l2ps_types.ts) to include a "read" variant (e.g., |
"read") and ensure any switch/case, serializers, database enums, and tests that
consume MessageStatus are updated to handle the new "read" value so this
addition won't break type narrowing elsewhere.
In `@src/instant_messaging/L2PSMessagingPeer.ts`:
- Around line 165-170: The loop over this.pendingResponses extracts an unused
key; update the iteration to avoid the unused variable by iterating values only
(e.g., use this.pendingResponses.values() or destructure to skip the key with an
underscore) so you still call clearTimeout(pending.timer) and pending.reject(new
Error("Disconnected")) and then call this.pendingResponses.clear(); keep
references to pending.timer and pending.reject intact.
- Around line 13-29: Remove the unused imports StoredMessage,
PeerJoinedNotification, and PeerLeftNotification from the import list in
L2PSMessagingPeer (they are not referenced anywhere in this file) and update the
module import path from the relative "./l2ps_types" to the project path alias
"@/instant_messaging/l2ps_types" (or the appropriate "@/..." alias used in the
repo) so the import uses the canonical alias per guidelines; ensure only the
actually used types (e.g., SerializedEncryptedMessage, ClientMessageType,
ServerMessageType, RegisteredResponse, IncomingMessage, MessageSentResponse,
MessageQueuedResponse, HistoryResponse, DiscoverResponse, PublicKeyResponse,
ErrorResponse, ErrorCode) remain in the import statement.
- Around line 78-82: The pendingResponses Map currently stores value objects
without typed metadata and code uses (entry as any)._meta; introduce a
PendingMeta type and change the Map value type to include meta, e.g. { resolve,
reject, timer, meta: PendingMeta }, then update any places that create or read
entries (notably waitForResponse and handleFrame) to set and read entry.meta
instead of using (entry as any)._meta so the metadata is strongly typed and you
can drop the casts; also update other similar maps/usages around lines
referenced (516-519) to the same typed shape.
- Around line 459-479: In handleFrame, remove redundant non-null assertions
after Map.has() by using optional chaining and early-returns: when checking
this.pendingResponses.has(frame.requestId) replace the get(...)! usage with
const pending = this.pendingResponses.get(frame.requestId); guard with if
(!pending) return; use pending?._meta and pending?.timer etc.; similarly in the
error frame block get the pending via get and guard instead of using !, and use
optional chaining for frame.payload?.message when constructing the Error; ensure
you still clearTimeout, delete from this.pendingResponses, and call
pending.resolve/pending.reject as before.
- Around line 68-96: Several instance fields in L2PSMessagingPeer are never
reassigned; mark them readonly to prevent accidental reassignment. Update the
declarations in class L2PSMessagingPeer for config, messageHandlers,
errorHandlers, peerJoinedHandlers, peerLeftHandlers, connectionStateHandlers,
pendingResponses, onlinePeers, messageQueue, baseReconnectDelay, and
maxReconnectAttempts to include the readonly modifier (leave fields that are
mutated such as ws, _isConnected, _isRegistered, reconnectAttempts,
reconnectTimeout, shouldReconnect, isReconnecting unchanged). This keeps
existing mutation of collection contents intact while preventing reassignment of
the references.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68f5edc0-dabf-49ee-a68d-1c45f5eeb70a
📒 Files selected for processing (4)
src/instant_messaging/L2PSMessagingPeer.tssrc/instant_messaging/l2ps_types.tssrc/types/blockchain/Transaction.tssrc/types/blockchain/TransactionSubtypes/InstantMessagingTransaction.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/types/blockchain/Transaction.ts
- src/types/blockchain/TransactionSubtypes/InstantMessagingTransaction.ts
added 3 commits
March 26, 2026 16:50
- Replace IncomingFrame.payload: any with ServerPayload discriminated union - Eliminate (entry as any)._meta casts by typing pendingResponses map properly - Add type-narrowing casts in handleFrame switch branches - Notify error handlers when re-registration fails after reconnect - Fix WebSocket error stringification (String(event) → proper extraction) - Replace deprecated substr() with substring()
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
AGENTS.md (1)
244-246: Add a language specifier to the fenced code block.The code block lacks a language identifier. Since this is a pseudo-code formula, consider using
textorplaintextto satisfy markdown linters.📝 Proposed fix
-``` +```text score = urgency + (blocked_tasks_count × 1.5)</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@AGENTS.mdaround lines 244 - 246, The fenced code block showing the formula
"score = urgency + (blocked_tasks_count × 1.5)" is missing a language specifier;
update the markdown fence to include a language such as "text" or "plaintext" so
linters recognize it as plain text (referencing the formula symbols score,
urgency, and blocked_tasks_count), e.g., changetotext and keep the
formula unchanged.</details> </blockquote></details> <details> <summary>src/instant_messaging/L2PSMessagingPeer.ts (5)</summary><blockquote> `180-183`: **Use `_` for the unused loop variable.** The `key` variable is declared but never used. Use `_` to indicate intentional discard. <details> <summary>🧹 Proposed fix</summary> ```diff - for (const [key, pending] of this.pendingResponses) { + for (const [_, pending] of this.pendingResponses) { clearTimeout(pending.timer) pending.reject(new Error("Disconnected")) } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/instant_messaging/L2PSMessagingPeer.ts` around lines 180 - 183, The for-loop over this.pendingResponses introduces an unused variable named key; update the loop to discard the first tuple element (use _ or an empty slot) so the intent is clear and linting warnings are avoided — e.g., change the iteration in L2PSMessagingPeer that currently reads for (const [key, pending] of this.pendingResponses) to use a discard for the key (for (const [, pending] ...) or for (const [_ , pending] ...)) while leaving the clearTimeout(pending.timer) and pending.reject(new Error("Disconnected")) logic unchanged. ``` </details> --- `28-28`: **Remove unused import `StoredMessage`.** Static analysis indicates `StoredMessage` is imported but never used in this file. <details> <summary>🧹 Proposed fix</summary> ```diff ErrorCode, - StoredMessage, } from "./l2ps_types" ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/instant_messaging/L2PSMessagingPeer.ts` at line 28, The import list in L2PSMessagingPeer.ts includes an unused symbol StoredMessage; remove StoredMessage from the import statement (where StoredMessage is referenced) so the file only imports actually used symbols, e.g., update the import that currently lists StoredMessage to exclude it. ``` </details> --- `479-486`: **Use optional chaining for cleaner code.** The `meta` check can be simplified with optional chaining. <details> <summary>♻️ Proposed fix</summary> ```diff if (frame.requestId && this.pendingResponses.has(frame.requestId)) { const pending = this.pendingResponses.get(frame.requestId)! - const meta = pending._meta - if (meta && meta.types.includes(frame.type)) { + if (pending._meta?.types.includes(frame.type)) { clearTimeout(pending.timer) this.pendingResponses.delete(frame.requestId) pending.resolve(frame.payload) return } } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/instant_messaging/L2PSMessagingPeer.ts` around lines 479 - 486, Replace the explicit null check on pending._meta with optional chaining to simplify the condition in L2PSMessagingPeer: instead of checking meta and then meta.types.includes(frame.type), use pending._meta?.types.includes(frame.type) (or meta?.types.includes(frame.type) if you keep the local variable). Keep the rest of the logic (clearTimeout, delete from pendingResponses, resolve) intact. ``` </details> --- `82-107`: **Consider marking non-reassigned members as `readonly`.** Static analysis identifies several members that are never reassigned. Adding `readonly` improves type safety by preventing accidental reassignment. <details> <summary>♻️ Proposed fix</summary> ```diff export class L2PSMessagingPeer { private ws: WebSocket | null = null - private config: L2PSMessagingConfig + private readonly config: L2PSMessagingConfig // Event handlers - private messageHandlers: Set<L2PSMessageHandler> = new Set() - private errorHandlers: Set<L2PSErrorHandler> = new Set() - private peerJoinedHandlers: Set<L2PSPeerHandler> = new Set() - private peerLeftHandlers: Set<L2PSPeerHandler> = new Set() - private connectionStateHandlers: Set<L2PSConnectionStateHandler> = new Set() + private readonly messageHandlers: Set<L2PSMessageHandler> = new Set() + private readonly errorHandlers: Set<L2PSErrorHandler> = new Set() + private readonly peerJoinedHandlers: Set<L2PSPeerHandler> = new Set() + private readonly peerLeftHandlers: Set<L2PSPeerHandler> = new Set() + private readonly connectionStateHandlers: Set<L2PSConnectionStateHandler> = new Set() // Pending request-response waiters - private pendingResponses: Map<...> = new Map() + private readonly pendingResponses: Map<...> = new Map() // State - private messageQueue: OutgoingFrame[] = [] + private readonly messageQueue: OutgoingFrame[] = [] // Reconnection - private maxReconnectAttempts = 10 - private baseReconnectDelay = 1000 + private readonly maxReconnectAttempts = 10 + private readonly baseReconnectDelay = 1000 ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/instant_messaging/L2PSMessagingPeer.ts` around lines 82 - 107, Several instance fields are never reassigned and should be marked readonly to improve type safety: add the readonly modifier to messageHandlers, errorHandlers, peerJoinedHandlers, peerLeftHandlers, connectionStateHandlers, pendingResponses, onlinePeers, messageQueue, maxReconnectAttempts and baseReconnectDelay (leave mutable state fields like _isConnected/_isRegistered and reconnectAttempts unchanged). Locate these members in L2PSMessagingPeer (the declarations shown for messageHandlers, errorHandlers, peerJoinedHandlers, peerLeftHandlers, connectionStateHandlers, pendingResponses, onlinePeers, messageQueue, maxReconnectAttempts, baseReconnectDelay) and prefix them with readonly so they cannot be reassigned later. ``` </details> --- `316-334`: **Add JSDoc comments to public event handler methods.** Per coding guidelines, new methods should have JSDoc documentation. These public API methods would benefit from brief descriptions for IDE support and discoverability. <details> <summary>📝 Example JSDoc additions</summary> ```typescript /** Register a handler for incoming messages */ onMessage(handler: L2PSMessageHandler): void { ... } /** Register a handler for protocol errors */ onError(handler: L2PSErrorHandler): void { ... } /** Register a handler for peer join events */ onPeerJoined(handler: L2PSPeerHandler): void { ... } /** Register a handler for peer leave events */ onPeerLeft(handler: L2PSPeerHandler): void { ... } /** Register a handler for connection state changes */ onConnectionStateChange(handler: L2PSConnectionStateHandler): void { ... } ``` </details> As per coding guidelines: "Use JSDoc format for all new methods and functions". <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/instant_messaging/L2PSMessagingPeer.ts` around lines 316 - 334, Add JSDoc comments to the public event registration methods to follow coding guidelines: for each of onMessage, onError, onPeerJoined, onPeerLeft, and onConnectionStateChange add a brief JSDoc summary sentence, an `@param` tag documenting the handler argument and its type (e.g., L2PSMessageHandler, L2PSErrorHandler, L2PSPeerHandler, L2PSConnectionStateHandler), and an `@returns` tag indicating void; place the comment directly above each method declaration so IDEs surface the descriptions and parameter types. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@src/instant_messaging/L2PSMessagingPeer.ts:
- Around line 377-381: In the catch block that notifies this.errorHandlers about
a re-registration failure inside L2PSMessagingPeer, replace the current
String(err) stringification with the same robust pattern used at line 411 (e.g.,
use err instanceof Error ? err.message : JSON.stringify(err,
Object.getOwnPropertyNames(err)) or similar) so non-Error objects don't
stringify to "[object Object]"; update the call to h(...) in the re-registration
catch to include that improved error string.
Nitpick comments:
In@AGENTS.md:
- Around line 244-246: The fenced code block showing the formula "score =
urgency + (blocked_tasks_count × 1.5)" is missing a language specifier; update
the markdown fence to include a language such as "text" or "plaintext" so
linters recognize it as plain text (referencing the formula symbols score,
urgency, and blocked_tasks_count), e.g., changetotext and keep the
formula unchanged.In
@src/instant_messaging/L2PSMessagingPeer.ts:
- Around line 180-183: The for-loop over this.pendingResponses introduces an
unused variable named key; update the loop to discard the first tuple element
(use _ or an empty slot) so the intent is clear and linting warnings are avoided
— e.g., change the iteration in L2PSMessagingPeer that currently reads for
(const [key, pending] of this.pendingResponses) to use a discard for the key
(for (const [, pending] ...) or for (const [_ , pending] ...)) while leaving the
clearTimeout(pending.timer) and pending.reject(new Error("Disconnected")) logic
unchanged.- Line 28: The import list in L2PSMessagingPeer.ts includes an unused symbol
StoredMessage; remove StoredMessage from the import statement (where
StoredMessage is referenced) so the file only imports actually used symbols,
e.g., update the import that currently lists StoredMessage to exclude it.- Around line 479-486: Replace the explicit null check on pending._meta with
optional chaining to simplify the condition in L2PSMessagingPeer: instead of
checking meta and then meta.types.includes(frame.type), use
pending._meta?.types.includes(frame.type) (or meta?.types.includes(frame.type)
if you keep the local variable). Keep the rest of the logic (clearTimeout,
delete from pendingResponses, resolve) intact.- Around line 82-107: Several instance fields are never reassigned and should be
marked readonly to improve type safety: add the readonly modifier to
messageHandlers, errorHandlers, peerJoinedHandlers, peerLeftHandlers,
connectionStateHandlers, pendingResponses, onlinePeers, messageQueue,
maxReconnectAttempts and baseReconnectDelay (leave mutable state fields like
_isConnected/_isRegistered and reconnectAttempts unchanged). Locate these
members in L2PSMessagingPeer (the declarations shown for messageHandlers,
errorHandlers, peerJoinedHandlers, peerLeftHandlers, connectionStateHandlers,
pendingResponses, onlinePeers, messageQueue, maxReconnectAttempts,
baseReconnectDelay) and prefix them with readonly so they cannot be reassigned
later.- Around line 316-334: Add JSDoc comments to the public event registration
methods to follow coding guidelines: for each of onMessage, onError,
onPeerJoined, onPeerLeft, and onConnectionStateChange add a brief JSDoc summary
sentence, an@paramtag documenting the handler argument and its type (e.g.,
L2PSMessageHandler, L2PSErrorHandler, L2PSPeerHandler,
L2PSConnectionStateHandler), and an@returnstag indicating void; place the
comment directly above each method declaration so IDEs surface the descriptions
and parameter types.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `121db510-6f49-4afe-8086-15bd20265f0d` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between a9ac32124145c8ef8dafb66b0997307ff48d7e83 and ec99a6650c543b1f004084d2464d8c846794c5a3. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `.mycelium/mycelium.db` is excluded by `!**/*.db` </details> <details> <summary>📒 Files selected for processing (4)</summary> * `.mycelium/.gitignore` * `.serena/project.yml` * `AGENTS.md` * `src/instant_messaging/L2PSMessagingPeer.ts` </details> <details> <summary>✅ Files skipped from review due to trivial changes (1)</summary> * .mycelium/.gitignore </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Chores