Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughParses per-message "remote-echo" directives, optionally marks messages Pending, schedules cancellable delayed remote-echo tasks, returns Pending for delayed echoes, and adds unit tests and README usage examples. Changes
Sequence DiagramsequenceDiagram
participant Client as Matrix Client
participant Handler as HandleMatrixMessage
participant Parser as getRemoteEchoBehavior
participant DB as Database
participant Scheduler as queueRemoteEcho
participant Worker as Delayed Goroutine
participant Poster as simplevent Poster
Client->>Handler: Send message with "remote-echo ..." directive
Handler->>Parser: Parse message content for echo behavior
Parser-->>Handler: Return behavior {pending, delay}
alt pending
Handler->>DB: Save message marked Pending (with txn ID)
Handler->>Scheduler: Queue delayed echo (delay)
Handler-->>Client: Return Pending response
Scheduler->>Worker: Start cancellable goroutine
Worker->>Worker: Wait for delay or context cancel
alt timeout
Worker->>Poster: Post converted/echo message
Poster-->>DB: Update/deliver message
else canceled
Worker-->>DB: Abort pending echo
end
else immediate
Handler->>DB: Save and deliver immediate-success message
Handler-->>Client: Return Success response
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/connector/client.go (1)
279-288: Shallow copy may share pointer fields.
cloned := *contentcreates a shallow copy. IfMessageEventContentcontains pointer fields (e.g.,Info,RelatesTo), they remain shared with the original. For a test bridge this is likely fine, but consider a deep copy if content could be modified after queueing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/connector/client.go` around lines 279 - 288, The cloneMessageContent function currently does a shallow copy via "cloned := *content", which leaves pointer fields (e.g., Info, RelatesTo, any pointer/slice/map/struct fields on event.MessageEventContent) shared with the original; change it to perform a deep copy by allocating a new event.MessageEventContent and explicitly copying primitive fields and cloning any nested pointer/slice/map/struct fields (for example, clone content.Info, content.RelatesTo, formatted bodies, and any maps/slices) so the returned copy does not share mutable state with the original; update cloneMessageContent to check nils and deep-clone each nested field accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/connector/client.go`:
- Around line 279-288: The cloneMessageContent function currently does a shallow
copy via "cloned := *content", which leaves pointer fields (e.g., Info,
RelatesTo, any pointer/slice/map/struct fields on event.MessageEventContent)
shared with the original; change it to perform a deep copy by allocating a new
event.MessageEventContent and explicitly copying primitive fields and cloning
any nested pointer/slice/map/struct fields (for example, clone content.Info,
content.RelatesTo, formatted bodies, and any maps/slices) so the returned copy
does not share mutable state with the original; update cloneMessageContent to
check nils and deep-clone each nested field accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 96edf4fe-600a-4ac0-8252-c165137e986c
📒 Files selected for processing (3)
README.mdpkg/connector/client.gopkg/connector/client_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-docker-dummybridge
🔇 Additional comments (8)
README.md (1)
14-14: LGTM!Documentation clearly explains the new remote-echo testing feature with appropriate trigger phrase examples. The syntax aligns with the implementation.
Also applies to: 52-57
pkg/connector/client.go (6)
25-27: LGTM!Good use of
sync.WaitGroupfor goroutine coordination and context-based cancellation for proper lifecycle management.
39-39: LGTM!Regex pattern correctly handles case-insensitivity and anchors to the full message body. The integer-only duration constraint is a reasonable simplification for testing purposes.
77-78: LGTM!Proper context lifecycle management with cancellation propagation and graceful shutdown via
WaitGroup.Also applies to: 107-109
177-196: LGTM!The branching logic correctly handles pending vs immediate responses. The
TransactionIDproperly correlates the pending message with the eventual remote echo event.
208-237: LGTM!Clean implementation with proper nil checks, whitespace trimming, and case-insensitive matching.
239-277: LGTM!Well-structured goroutine with proper timer cleanup and context cancellation. The early return for
delay <= 0correctly implements the "pending forever" behavior forremote-echo none.pkg/connector/client_test.go (1)
10-33: LGTM!Well-structured table-driven tests covering the main scenarios. Consider adding edge cases like nil content, empty body, or invalid duration formats (e.g.,
remote-echo delay abc) for more comprehensive coverage.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/connector/client.go`:
- Around line 236-281: The goroutine closes over msg and clones msg.Content only
after the delay, so mutations to msg between scheduling and execution can change
the echoed payload; fix by snapshotting needed values before starting the
goroutine in queueRemoteEcho: read and store portalKey := msg.Portal.PortalKey
and clonedContent := cloneMessageContent(msg.Content) (and any other fields used
from msg) before calling dc.wg.Add(1)/go func(), then use portalKey and
clonedContent inside the goroutine instead of referencing msg.Portal or
msg.Content.
- Around line 177-190: The pending-path is incorrectly taken for "remote-echo
none" because getRemoteEchoBehavior can return pending=true with delay=0; update
the conditional around the pending handling in the connector to skip creating a
pending DB message and calling msg.AddPendingToSave when behavior.delay <= 0
(and when there's no portal flag if applicable): change the if that checks
behavior.pending (the block that calls getTransactionID, constructs
database.Message, calls msg.AddPendingToSave and dc.queueRemoteEcho) to require
behavior.pending && behavior.delay > 0 (and also ensure any portal-specific flag
is considered), so queueRemoteEcho is only invoked for real delayed remote-echo
cases and the zero-delay/no-echo path does not create unresolved pending
transactions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e446cd7-bef5-4bd2-9c36-92eaa398e42b
📒 Files selected for processing (1)
pkg/connector/client.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-docker-dummybridge
- GitHub Check: build-docker-dummybridge
| behavior := getRemoteEchoBehavior(msg.Content) | ||
| if behavior.pending { | ||
| transactionID := getTransactionID(msg) | ||
| dbMessage := &database.Message{ | ||
| ID: networkid.MessageID(transactionID), | ||
| SenderID: networkid.UserID(dc.UserLogin.ID), | ||
| Timestamp: timestamp, | ||
| } | ||
| msg.AddPendingToSave(dbMessage, transactionID, nil) | ||
| dc.queueRemoteEcho(msg, transactionID, timestamp, behavior.delay) | ||
| return &bridgev2.MatrixMessageResponse{ | ||
| DB: dbMessage, | ||
| Pending: true, | ||
| }, nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n pkg/connector/client.go | head -200Repository: beeper/dummybridge
Length of output: 7293
🏁 Script executed:
# Find the getRemoteEchoBehavior and queueRemoteEcho function definitions
rg -n "func.*getRemoteEchoBehavior|func.*queueRemoteEcho" pkg/connector/client.go -A 20Repository: beeper/dummybridge
Length of output: 1288
🏁 Script executed:
# Check lines 177-190 and 236-239 specifically
sed -n '170,200p' pkg/connector/client.goRepository: beeper/dummybridge
Length of output: 915
🏁 Script executed:
# Also check around line 236
sed -n '230,250p' pkg/connector/client.goRepository: beeper/dummybridge
Length of output: 520
🏁 Script executed:
# Search for any code that might resolve this pending state
rg -n "AddPendingToSave|Pending" pkg/connector/client.goRepository: beeper/dummybridge
Length of output: 141
🏁 Script executed:
# Check if there's any context about what happens with unresolved pending transactions
rg -n "pending" pkg/connector/client.go -B 2 -A 2Repository: beeper/dummybridge
Length of output: 661
🏁 Script executed:
# Search the broader codebase for any cleanup of pending states
rg -n "CancelPending|ResolvePending|pending.*resolve|pending.*cancel" --type goRepository: beeper/dummybridge
Length of output: 44
Skip pending path for no-echo messages.
When remote-echo none is sent, getRemoteEchoBehavior() returns {pending: true, delay: 0}. This enters the pending block at line 178, calls msg.AddPendingToSave(), then passes delay=0 to queueRemoteEcho(). Since queueRemoteEcho() immediately returns when delay <= 0, the pending transaction is never resolved. The Pending/AddPendingToSave flow is meant for cases where a real remote event arrives later; no-echo messages have no follow-up.
Skip the pending block for zero-delay and no-portal cases by adding those conditions to the if statement:
Suggested change
- if behavior.pending {
+ if behavior.pending && behavior.delay > 0 && msg.Portal != nil {
transactionID := getTransactionID(msg)
dbMessage := &database.Message{
ID: networkid.MessageID(transactionID),
SenderID: networkid.UserID(dc.UserLogin.ID),
Timestamp: timestamp,
}
msg.AddPendingToSave(dbMessage, transactionID, nil)
dc.queueRemoteEcho(msg, transactionID, timestamp, behavior.delay)
return &bridgev2.MatrixMessageResponse{
DB: dbMessage,
Pending: true,
}, nil
}Also applies to: 236-239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/connector/client.go` around lines 177 - 190, The pending-path is
incorrectly taken for "remote-echo none" because getRemoteEchoBehavior can
return pending=true with delay=0; update the conditional around the pending
handling in the connector to skip creating a pending DB message and calling
msg.AddPendingToSave when behavior.delay <= 0 (and when there's no portal flag
if applicable): change the if that checks behavior.pending (the block that calls
getTransactionID, constructs database.Message, calls msg.AddPendingToSave and
dc.queueRemoteEcho) to require behavior.pending && behavior.delay > 0 (and also
ensure any portal-specific flag is considered), so queueRemoteEcho is only
invoked for real delayed remote-echo cases and the zero-delay/no-echo path does
not create unresolved pending transactions.
| func (dc *DummyClient) queueRemoteEcho(msg *bridgev2.MatrixMessage, transactionID networkid.TransactionID, timestamp time.Time, delay time.Duration) { | ||
| if delay <= 0 || msg.Portal == nil { | ||
| return | ||
| } | ||
|
|
||
| dc.wg.Add(1) | ||
| go func() { | ||
| defer dc.wg.Done() | ||
|
|
||
| timer := time.NewTimer(delay) | ||
| defer timer.Stop() | ||
|
|
||
| select { | ||
| case <-dc.ctx.Done(): | ||
| return | ||
| case <-timer.C: | ||
| } | ||
|
|
||
| dc.UserLogin.QueueRemoteEvent(&simplevent.PreConvertedMessage{ | ||
| EventMeta: simplevent.EventMeta{ | ||
| Type: bridgev2.RemoteEventMessage, | ||
| PortalKey: msg.Portal.PortalKey, | ||
| Sender: bridgev2.EventSender{ | ||
| IsFromMe: true, | ||
| SenderLogin: dc.UserLogin.ID, | ||
| Sender: networkid.UserID(dc.UserLogin.ID), | ||
| }, | ||
| Timestamp: timestamp, | ||
| StreamOrder: time.Now().UnixNano(), | ||
| }, | ||
| Data: &bridgev2.ConvertedMessage{Parts: []*bridgev2.ConvertedMessagePart{{ | ||
| Type: event.EventMessage, | ||
| Content: cloneMessageContent(msg.Content), | ||
| }}}, | ||
| ID: randomMessageID(), | ||
| TransactionID: transactionID, | ||
| }) | ||
| }() | ||
| } | ||
|
|
||
| func cloneMessageContent(content *event.MessageEventContent) *event.MessageEventContent { | ||
| if content == nil { | ||
| return nil | ||
| } | ||
| cloned := *content | ||
| return &cloned |
There was a problem hiding this comment.
Snapshot the echoed payload before starting the timer goroutine.
This goroutine closes over msg and only clones msg.Content after the delay expires. Any mutation to the request object after HandleMatrixMessage returns will change what gets echoed. Capture the portal key and cloned content up front, then use those immutable values inside the goroutine.
Suggested change
func (dc *DummyClient) queueRemoteEcho(msg *bridgev2.MatrixMessage, transactionID networkid.TransactionID, timestamp time.Time, delay time.Duration) {
if delay <= 0 || msg.Portal == nil {
return
}
+ portalKey := msg.Portal.PortalKey
+ content := cloneMessageContent(msg.Content)
dc.wg.Add(1)
go func() {
defer dc.wg.Done()
@@
dc.UserLogin.QueueRemoteEvent(&simplevent.PreConvertedMessage{
EventMeta: simplevent.EventMeta{
Type: bridgev2.RemoteEventMessage,
- PortalKey: msg.Portal.PortalKey,
+ PortalKey: portalKey,
Sender: bridgev2.EventSender{
IsFromMe: true,
SenderLogin: dc.UserLogin.ID,
Sender: networkid.UserID(dc.UserLogin.ID),
},
@@
},
Data: &bridgev2.ConvertedMessage{Parts: []*bridgev2.ConvertedMessagePart{{
Type: event.EventMessage,
- Content: cloneMessageContent(msg.Content),
+ Content: content,
}}},
ID: randomMessageID(),
TransactionID: transactionID,
})
}()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (dc *DummyClient) queueRemoteEcho(msg *bridgev2.MatrixMessage, transactionID networkid.TransactionID, timestamp time.Time, delay time.Duration) { | |
| if delay <= 0 || msg.Portal == nil { | |
| return | |
| } | |
| dc.wg.Add(1) | |
| go func() { | |
| defer dc.wg.Done() | |
| timer := time.NewTimer(delay) | |
| defer timer.Stop() | |
| select { | |
| case <-dc.ctx.Done(): | |
| return | |
| case <-timer.C: | |
| } | |
| dc.UserLogin.QueueRemoteEvent(&simplevent.PreConvertedMessage{ | |
| EventMeta: simplevent.EventMeta{ | |
| Type: bridgev2.RemoteEventMessage, | |
| PortalKey: msg.Portal.PortalKey, | |
| Sender: bridgev2.EventSender{ | |
| IsFromMe: true, | |
| SenderLogin: dc.UserLogin.ID, | |
| Sender: networkid.UserID(dc.UserLogin.ID), | |
| }, | |
| Timestamp: timestamp, | |
| StreamOrder: time.Now().UnixNano(), | |
| }, | |
| Data: &bridgev2.ConvertedMessage{Parts: []*bridgev2.ConvertedMessagePart{{ | |
| Type: event.EventMessage, | |
| Content: cloneMessageContent(msg.Content), | |
| }}}, | |
| ID: randomMessageID(), | |
| TransactionID: transactionID, | |
| }) | |
| }() | |
| } | |
| func cloneMessageContent(content *event.MessageEventContent) *event.MessageEventContent { | |
| if content == nil { | |
| return nil | |
| } | |
| cloned := *content | |
| return &cloned | |
| func (dc *DummyClient) queueRemoteEcho(msg *bridgev2.MatrixMessage, transactionID networkid.TransactionID, timestamp time.Time, delay time.Duration) { | |
| if delay <= 0 || msg.Portal == nil { | |
| return | |
| } | |
| portalKey := msg.Portal.PortalKey | |
| content := cloneMessageContent(msg.Content) | |
| dc.wg.Add(1) | |
| go func() { | |
| defer dc.wg.Done() | |
| timer := time.NewTimer(delay) | |
| defer timer.Stop() | |
| select { | |
| case <-dc.ctx.Done(): | |
| return | |
| case <-timer.C: | |
| } | |
| dc.UserLogin.QueueRemoteEvent(&simplevent.PreConvertedMessage{ | |
| EventMeta: simplevent.EventMeta{ | |
| Type: bridgev2.RemoteEventMessage, | |
| PortalKey: portalKey, | |
| Sender: bridgev2.EventSender{ | |
| IsFromMe: true, | |
| SenderLogin: dc.UserLogin.ID, | |
| Sender: networkid.UserID(dc.UserLogin.ID), | |
| }, | |
| Timestamp: timestamp, | |
| StreamOrder: time.Now().UnixNano(), | |
| }, | |
| Data: &bridgev2.ConvertedMessage{Parts: []*bridgev2.ConvertedMessagePart{{ | |
| Type: event.EventMessage, | |
| Content: content, | |
| }}}, | |
| ID: randomMessageID(), | |
| TransactionID: transactionID, | |
| }) | |
| }() | |
| } | |
| func cloneMessageContent(content *event.MessageEventContent) *event.MessageEventContent { | |
| if content == nil { | |
| return nil | |
| } | |
| cloned := *content | |
| return &cloned |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/connector/client.go` around lines 236 - 281, The goroutine closes over
msg and clones msg.Content only after the delay, so mutations to msg between
scheduling and execution can change the echoed payload; fix by snapshotting
needed values before starting the goroutine in queueRemoteEcho: read and store
portalKey := msg.Portal.PortalKey and clonedContent :=
cloneMessageContent(msg.Content) (and any other fields used from msg) before
calling dc.wg.Add(1)/go func(), then use portalKey and clonedContent inside the
goroutine instead of referencing msg.Portal or msg.Content.
There was a problem hiding this comment.
Pull request overview
Adds per-message “remote echo” controls to the dummy connector so outbound Matrix sends can be intentionally left pending or remote-echoed after a delay, which is useful for testing slow/missing remote echo behavior.
Changes:
- Introduces trigger-phrase parsing (
remote-echo none/remote-echo delay <duration>) to switch a message into “pending” mode. - Implements delayed remote-echo scheduling via a background goroutine and connector shutdown cancellation.
- Adds unit tests for the trigger parser and documents the feature in the README.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/connector/client.go | Adds parsing + pending-send path and delayed remote echo scheduling/cancellation. |
| pkg/connector/client_test.go | Adds unit tests for remote-echo trigger parsing. |
| README.md | Documents how to use the new trigger phrases. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (dc *DummyClient) queueRemoteEcho(msg *bridgev2.MatrixMessage, transactionID networkid.TransactionID, timestamp time.Time, delay time.Duration) { | ||
| if delay <= 0 || msg.Portal == nil { | ||
| return | ||
| } |
There was a problem hiding this comment.
remote-echo delay 0s currently results in behavior.pending=true (parsed duration is 0), but queueRemoteEcho returns early for delay <= 0, so the message stays pending forever. Consider either treating a zero delay as an immediate remote echo (still queue with no timer) or rejecting/ignoring zero delays in getRemoteEchoBehavior so the send isn't marked pending.
| select { | ||
| case <-dc.ctx.Done(): | ||
| return |
There was a problem hiding this comment.
queueRemoteEcho uses dc.ctx.Done() in the goroutine, but dc.ctx is only initialized in Connect. If HandleMatrixMessage (or this helper) can run before Connect, this will panic due to a nil context.Context. Consider defaulting dc.ctx to the passed-in ctx (or context.Background()) when dc.ctx is nil.
| To test slow or missing remote echoes, send trigger phrases in the message body: | ||
|
|
||
| - `remote-echo none` or `remote-echo delay 0s` keeps that send pending forever. | ||
| - `remote-echo delay 5s` delays the remote echo for the parsed Go duration. |
There was a problem hiding this comment.
The README says remote-echo delay 5s delays the echo for the "parsed Go duration", but the implementation only accepts a restricted subset via regex ([0-9]+ + ms|s|m|h) and will reject valid time.ParseDuration inputs like 1m30s, 250ms, 1.5s, etc. Either broaden the parser/regex to match time.ParseDuration semantics or adjust the documentation to reflect the supported format.
| - `remote-echo delay 5s` delays the remote echo for the parsed Go duration. | |
| - `remote-echo delay 5s` delays the remote echo for the given duration, specified as an integer followed by a unit (`ms`, `s`, `m`, or `h`), for example `5s`, `250ms`, `2m`, or `1h`. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: befe033d84
ℹ️ 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".
| } | ||
|
|
||
| func (dc *DummyClient) queueRemoteEcho(msg *bridgev2.MatrixMessage, transactionID networkid.TransactionID, timestamp time.Time, delay time.Duration) { | ||
| if delay <= 0 || msg.Portal == nil { |
There was a problem hiding this comment.
When a message body is remote-echo delay 0s, HandleMatrixMessage marks it as pending and then queueRemoteEcho immediately returns because of delay <= 0, so no remote event is ever queued and the send stays pending forever. This differs from the documented “delay by duration” behavior, where a zero duration should complete immediately rather than behave like remote-echo none.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pkg/connector/client.go (2)
239-275:⚠️ Potential issue | 🟠 MajorSnapshot the echoed payload before starting the timer goroutine.
This worker still reads
msg.Portal.PortalKeyand clonesmsg.Contentonly after the delay expires. If the request object is mutated afterHandleMatrixMessage()returns, the remote echo can be sent to the wrong portal or with different content than the original message.Suggested change
func (dc *DummyClient) queueRemoteEcho(msg *bridgev2.MatrixMessage, transactionID networkid.TransactionID, timestamp time.Time, delay time.Duration) { if delay <= 0 || msg.Portal == nil { return } + portalKey := msg.Portal.PortalKey + content := cloneMessageContent(msg.Content) dc.wg.Add(1) go func() { defer dc.wg.Done() @@ dc.UserLogin.QueueRemoteEvent(&simplevent.PreConvertedMessage{ EventMeta: simplevent.EventMeta{ Type: bridgev2.RemoteEventMessage, - PortalKey: msg.Portal.PortalKey, + PortalKey: portalKey, Sender: bridgev2.EventSender{ IsFromMe: true, SenderLogin: dc.UserLogin.ID, Sender: networkid.UserID(dc.UserLogin.ID), }, @@ }, Data: &bridgev2.ConvertedMessage{Parts: []*bridgev2.ConvertedMessagePart{{ Type: event.EventMessage, - Content: cloneMessageContent(msg.Content), + Content: content, }}}, ID: randomMessageID(), TransactionID: transactionID, }) }() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/connector/client.go` around lines 239 - 275, The queueRemoteEcho worker reads fields from msg after the delay, which can be mutated; to fix, snapshot all needed values from msg before starting the goroutine in DummyClient.queueRemoteEcho: capture portalKey := msg.Portal.PortalKey (after the nil check), clonedContent := cloneMessageContent(msg.Content), and any other values used later (e.g., transactionID, timestamp, ID if generated now) into local variables, then reference those locals inside the goroutine instead of msg.* or msg.Portal.*, ensuring the queued PreConvertedMessage uses the copied portalKey and clonedContent.
177-190:⚠️ Potential issue | 🟠 MajorDon't return a pending send when no delayed echo will be queued.
remote-echo nonestill takes this branch becausegetRemoteEchoBehavior()returns{pending: true, delay: 0}. That creates a pending DB message and returnsPending: true, butqueueRemoteEcho()immediately exits on zero delay, so nothing ever resolves that pending state.Suggested change
- if behavior.pending { + if behavior.pending && behavior.delay > 0 && msg.Portal != nil { transactionID := getTransactionID(msg) dbMessage := &database.Message{ ID: randomMessageID(), SenderID: networkid.UserID(dc.UserLogin.ID), Timestamp: timestamp, } msg.AddPendingToSave(dbMessage, transactionID, nil) dc.queueRemoteEcho(msg, transactionID, timestamp, behavior.delay) return &bridgev2.MatrixMessageResponse{ DB: dbMessage, Pending: true, }, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/connector/client.go` around lines 177 - 190, getRemoteEchoBehavior may set pending:true with delay:0 causing a pending DB entry that's never resolved; only create the pending DB message, call msg.AddPendingToSave, invoke dc.queueRemoteEcho, and return Pending:true when both behavior.pending and behavior.delay>0. Modify the branch around getRemoteEchoBehavior(msg.Content) to check behavior.delay>0 (in addition to behavior.pending) before calling getTransactionID, creating dbMessage, msg.AddPendingToSave, dc.queueRemoteEcho, and returning a bridgev2.MatrixMessageResponse with Pending:true; otherwise fall through to the non-pending/send path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/connector/client.go`:
- Around line 239-275: The queueRemoteEcho worker reads fields from msg after
the delay, which can be mutated; to fix, snapshot all needed values from msg
before starting the goroutine in DummyClient.queueRemoteEcho: capture portalKey
:= msg.Portal.PortalKey (after the nil check), clonedContent :=
cloneMessageContent(msg.Content), and any other values used later (e.g.,
transactionID, timestamp, ID if generated now) into local variables, then
reference those locals inside the goroutine instead of msg.* or msg.Portal.*,
ensuring the queued PreConvertedMessage uses the copied portalKey and
clonedContent.
- Around line 177-190: getRemoteEchoBehavior may set pending:true with delay:0
causing a pending DB entry that's never resolved; only create the pending DB
message, call msg.AddPendingToSave, invoke dc.queueRemoteEcho, and return
Pending:true when both behavior.pending and behavior.delay>0. Modify the branch
around getRemoteEchoBehavior(msg.Content) to check behavior.delay>0 (in addition
to behavior.pending) before calling getTransactionID, creating dbMessage,
msg.AddPendingToSave, dc.queueRemoteEcho, and returning a
bridgev2.MatrixMessageResponse with Pending:true; otherwise fall through to the
non-pending/send path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e630d4b-5360-4c6e-b6b5-2c35a52bb4ae
📒 Files selected for processing (1)
pkg/connector/client.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-docker-dummybridge
- GitHub Check: build-docker-dummybridge
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
pkg/connector/client.go (1)
245-281:⚠️ Potential issue | 🟠 MajorSnapshot the echoed payload before starting the timer goroutine.
This still closes over
msg, so any mutation afterHandleMatrixMessagereturns can change the delayed echo’s portal or content. Capture immutable values up front and use those inside the goroutine.Suggested change
func (dc *DummyClient) queueRemoteEcho(msg *bridgev2.MatrixMessage, transactionID networkid.TransactionID, timestamp time.Time, delay time.Duration) { if delay <= 0 || msg.Portal == nil { return } + portalKey := msg.Portal.PortalKey + content := cloneMessageContent(msg.Content) dc.wg.Add(1) go func() { defer dc.wg.Done() @@ dc.UserLogin.QueueRemoteEvent(&simplevent.PreConvertedMessage{ EventMeta: simplevent.EventMeta{ Type: bridgev2.RemoteEventMessage, - PortalKey: msg.Portal.PortalKey, + PortalKey: portalKey, Sender: bridgev2.EventSender{ IsFromMe: true, SenderLogin: dc.UserLogin.ID, Sender: networkid.UserID(dc.UserLogin.ID), @@ }, Data: &bridgev2.ConvertedMessage{Parts: []*bridgev2.ConvertedMessagePart{{ Type: event.EventMessage, - Content: cloneMessageContent(msg.Content), + Content: content, }}}, ID: randomMessageID(), TransactionID: transactionID, }) }() }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/connector/client.go` around lines 245 - 281, In queueRemoteEcho, avoid closing over mutable msg by snapshotting the needed immutable pieces (e.g., portal := msg.Portal, clonedContent := cloneMessageContent(msg.Content), and any other fields like msg.Portal.PortalKey) before launching the goroutine, and then have the goroutine use those local copies (referencing DummyClient.queueRemoteEcho, msg.Portal, cloneMessageContent, and the PreConvertedMessage construction) so mutations to msg after HandleMatrixMessage returns cannot affect the delayed echo.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/connector/client.go`:
- Line 39: The regex delayedRemoteEchoPattern currently only matches single-unit
durations and should be broadened so time.ParseDuration handles full Go duration
syntax: update the pattern declared in delayedRemoteEchoPattern to accept the
entire duration token (e.g., use a capture that matches any non-empty sequence
instead of the current single-unit pattern) and keep using time.ParseDuration
where the capture is parsed (the code path around the time.ParseDuration call
that handles remote-echo delay). Ensure the variable name
delayedRemoteEchoPattern and the existing capture group used by the downstream
parsing logic are preserved so only validation shifts to time.ParseDuration.
- Around line 285-290: cloneMessageContent currently does a shallow copy of
*event.MessageEventContent which leaves pointer and slice fields shared and can
lead to races when the original msg.Content is mutated before a delayed
goroutine runs; update cloneMessageContent to perform a deep copy: allocate a
new event.MessageEventContent, copy primitive fields, and for each pointer field
(Info, File, Mentions, NewContent, RelatesTo, MessageSendRetry,
BeeperPerMessageProfile, BeeperActionMessage, BeeperLinkPreviews) allocate and
copy the pointed-to structs, and for slice fields (Methods, BeeperGalleryImages,
BeeperLinkPreviews, Mentions if a slice) create new slices and copy each element
(deep-cloning elements as needed) so the returned copy is fully independent of
the original.
---
Duplicate comments:
In `@pkg/connector/client.go`:
- Around line 245-281: In queueRemoteEcho, avoid closing over mutable msg by
snapshotting the needed immutable pieces (e.g., portal := msg.Portal,
clonedContent := cloneMessageContent(msg.Content), and any other fields like
msg.Portal.PortalKey) before launching the goroutine, and then have the
goroutine use those local copies (referencing DummyClient.queueRemoteEcho,
msg.Portal, cloneMessageContent, and the PreConvertedMessage construction) so
mutations to msg after HandleMatrixMessage returns cannot affect the delayed
echo.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc466abe-a93a-4564-b757-f534a3b72680
📒 Files selected for processing (3)
README.mdpkg/connector/client.gopkg/connector/client_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/connector/client_test.go
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build-docker-dummybridge
- GitHub Check: build-docker-dummybridge
| var _ bridgev2.DeleteChatHandlingNetworkAPI = (*DummyClient)(nil) | ||
| var _ bridgev2.MessageRequestAcceptingNetworkAPI = (*DummyClient)(nil) | ||
|
|
||
| var delayedRemoteEchoPattern = regexp.MustCompile(`(?i)^remote-echo\s+delay\s+([0-9]+(?:ms|s|m|h))$`) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the file around line 39 to see the pattern definition
cat -n pkg/connector/client.go | sed -n '30,50p'Repository: beeper/dummybridge
Length of output: 1088
🏁 Script executed:
# Read lines 224-242 mentioned in "Also applies to"
cat -n pkg/connector/client.go | sed -n '220,250p'Repository: beeper/dummybridge
Length of output: 1180
🏁 Script executed:
# Search for getRemoteEchoBehavior function to understand the parsing logic
rg -n "getRemoteEchoBehavior" pkg/connector/client.go -A 25Repository: beeper/dummybridge
Length of output: 1907
🏁 Script executed:
# Check for README to verify what documentation says about the format
fd -name "README*" -type fRepository: beeper/dummybridge
Length of output: 292
🏁 Script executed:
# Search for README files
find . -type f -iname "README*" | head -20Repository: beeper/dummybridge
Length of output: 73
🏁 Script executed:
# Check if there's documentation about remote-echo format
rg -i "remote-echo" -A 3 -B 3 README* 2>/dev/null | head -50Repository: beeper/dummybridge
Length of output: 406
🏁 Script executed:
# Verify what time.ParseDuration accepts by checking Go standard library documentation reference
# Let's also test the regex claim directly
python3 << 'EOF'
import re
pattern = re.compile(r'(?i)^remote-echo\s+delay\s+([0-9]+(?:ms|s|m|h))$')
test_cases = [
"remote-echo delay 100ms",
"remote-echo delay 5s",
"remote-echo delay 1m",
"remote-echo delay 2h",
"remote-echo delay 1m30s", # Should fail - two units
"remote-echo delay 250us", # Should fail - us not in list
"remote-echo delay 100", # Should fail - no unit
]
print("Testing regex pattern:")
for test in test_cases:
match = pattern.search(test)
print(f" '{test}': {'MATCH' if match else 'NO MATCH'}")
if match:
print(f" -> captured: '{match.group(1)}'")
EOFRepository: beeper/dummybridge
Length of output: 418
remote-echo delay regex only accepts single-unit Go durations, not compound or nanosecond-precision formats.
The regex pattern rejects valid Go durations like 1m30s or 250us, silently falling back to immediate-success behavior. This contradicts the README claim of "parsed Go duration" support. Either tighten the README wording to document the supported subset, or change the regex to (?i)^remote-echo\s+delay\s+(.+)$ and let time.ParseDuration (line 238) handle validation.
Suggested change
-var delayedRemoteEchoPattern = regexp.MustCompile(`(?i)^remote-echo\s+delay\s+([0-9]+(?:ms|s|m|h))$`)
+var delayedRemoteEchoPattern = regexp.MustCompile(`(?i)^remote-echo\s+delay\s+(.+)$`)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/connector/client.go` at line 39, The regex delayedRemoteEchoPattern
currently only matches single-unit durations and should be broadened so
time.ParseDuration handles full Go duration syntax: update the pattern declared
in delayedRemoteEchoPattern to accept the entire duration token (e.g., use a
capture that matches any non-empty sequence instead of the current single-unit
pattern) and keep using time.ParseDuration where the capture is parsed (the code
path around the time.ParseDuration call that handles remote-echo delay). Ensure
the variable name delayedRemoteEchoPattern and the existing capture group used
by the downstream parsing logic are preserved so only validation shifts to
time.ParseDuration.
| func cloneMessageContent(content *event.MessageEventContent) *event.MessageEventContent { | ||
| if content == nil { | ||
| return nil | ||
| } | ||
| cloned := *content | ||
| return &cloned |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and examine the context around cloneMessageContent
fd pkg/connector/client.goRepository: beeper/dummybridge
Length of output: 44
🏁 Script executed:
# Search for the MessageEventContent definition in the event package
fd -e go | xargs grep -l "type MessageEventContent struct"Repository: beeper/dummybridge
Length of output: 46
🏁 Script executed:
# Look at the cloneMessageContent function and its context
rg -A 20 "func cloneMessageContent" pkg/connector/client.goRepository: beeper/dummybridge
Length of output: 922
🏁 Script executed:
# Find all usages of cloneMessageContent to understand how it's used
rg "cloneMessageContent" --type goRepository: beeper/dummybridge
Length of output: 246
🏁 Script executed:
# Find where MessageEventContent is defined - likely in mautrix or similar dependency
fd -e go | xargs grep -l "type MessageEventContent struct"Repository: beeper/dummybridge
Length of output: 46
🏁 Script executed:
# Get the context around the usage of cloneMessageContent
rg -B 10 -A 3 "Content: cloneMessageContent" pkg/connector/client.goRepository: beeper/dummybridge
Length of output: 524
🏁 Script executed:
# Search for delayed echo references to understand the context
rg -i "delayed.*echo|echo.*delayed" --type go pkg/connector/Repository: beeper/dummybridge
Length of output: 273
🏁 Script executed:
# Look for the full context of delayed remote echo handling
rg -B 5 -A 20 "delayedRemoteEchoPattern" pkg/connector/client.goRepository: beeper/dummybridge
Length of output: 2182
🏁 Script executed:
# Check the imports in client.go to understand where event comes from
head -50 pkg/connector/client.go | grep -E "import|event"Repository: beeper/dummybridge
Length of output: 540
🏁 Script executed:
# Search for MessageEventContent definition anywhere in go files (might be imported from mautrix)
rg "type MessageEventContent" --type goRepository: beeper/dummybridge
Length of output: 44
🌐 Web query:
MessageEventContent mautrix go struct fields
💡 Result:
In maunium.net/go/mautrix/event, MessageEventContent is defined with these exported fields (and JSON keys): [1]
MsgType MessageTypejson:"msgtype,omitempty"Body stringjson:"body"Format Formatjson:"format,omitempty"FormattedBody stringjson:"formatted_body,omitempty"GeoURI stringjson:"geo_uri,omitempty"URL id.ContentURIStringjson:"url,omitempty"Info *FileInfojson:"info,omitempty"File *EncryptedFileInfojson:"file,omitempty"FileName stringjson:"filename,omitempty"Mentions *Mentionsjson:"m.mentions,omitempty"NewContent *MessageEventContentjson:"m.new_content,omitempty"RelatesTo *RelatesTojson:"m.relates_to,omitempty"To id.UserIDjson:"to,omitempty"FromDevice id.DeviceIDjson:"from_device,omitempty"Methods []VerificationMethodjson:"methods,omitempty"MessageSendRetry *BeeperRetryMetadatajson:"com.beeper.message_send_retry,omitempty"BeeperGalleryImages []*MessageEventContentjson:"com.beeper.gallery.images,omitempty"BeeperGalleryCaption stringjson:"com.beeper.gallery.caption,omitempty"BeeperGalleryCaptionHTML stringjson:"com.beeper.gallery.caption_html,omitempty"BeeperPerMessageProfile *BeeperPerMessageProfilejson:"com.beeper.per_message_profile,omitempty"BeeperActionMessage *BeeperActionMessagejson:"com.beeper.action_message,omitempty"BeeperLinkPreviews []*BeeperLinkPreviewjson:"com.beeper.linkpreviews,omitempty"BeeperDisappearingTimer *BeeperDisappearingTimerjson:"com.beeper.disappearing_timer,omitempty"MSC1767Audio *MSC1767Audiojson:"org.matrix.msc1767.audio,omitempty"MSC3245Voice *MSC3245Voicejson:"org.matrix.msc3245.voice,omitempty"MSC4391BotCommand *MSC4391BotCommandInputjson:"org.matrix.msc4391.command,omitempty"
It also “contains filtered or unexported fields” (so there are additional private fields not shown in GoDocs). [1]
Source(s):
[1] (pkg.go.dev)
Citations:
🏁 Script executed:
# Get more context around queueRemoteEcho to see how the cloned content is used
rg -A 30 "func (dc \*DummyClient) queueRemoteEcho" pkg/connector/client.goRepository: beeper/dummybridge
Length of output: 44
🏁 Script executed:
# Get the full queueRemoteEcho function
rg -A 40 "func \(dc \*DummyClient\) queueRemoteEcho" pkg/connector/client.goRepository: beeper/dummybridge
Length of output: 1182
🏁 Script executed:
# Look at the full file around line 285 to understand the complete context
sed -n '270,320p' pkg/connector/client.goRepository: beeper/dummybridge
Length of output: 1736
cloneMessageContent performs only a shallow copy, leaving nested reference fields shared with the original.
event.MessageEventContent contains pointer fields (Info, File, Mentions, NewContent, RelatesTo, MessageSendRetry, BeeperPerMessageProfile, BeeperActionMessage, BeeperLinkPreviews) and slice fields (Methods, BeeperGalleryImages, BeeperLinkPreviews) that are not deep-copied. When the delayed remote echo is queued, if the original msg.Content is modified after this function returns but before the delayed goroutine processes it, those mutations are visible through the cloned copy, breaking the state isolation that the delayed path requires. A deep copy is needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/connector/client.go` around lines 285 - 290, cloneMessageContent
currently does a shallow copy of *event.MessageEventContent which leaves pointer
and slice fields shared and can lead to races when the original msg.Content is
mutated before a delayed goroutine runs; update cloneMessageContent to perform a
deep copy: allocate a new event.MessageEventContent, copy primitive fields, and
for each pointer field (Info, File, Mentions, NewContent, RelatesTo,
MessageSendRetry, BeeperPerMessageProfile, BeeperActionMessage,
BeeperLinkPreviews) allocate and copy the pointed-to structs, and for slice
fields (Methods, BeeperGalleryImages, BeeperLinkPreviews, Mentions if a slice)
create new slices and copy each element (deep-cloning elements as needed) so the
returned copy is fully independent of the original.
To test slow or missing remote echoes, send trigger phrases in the message body:
remote-echo nonekeeps that send pending forever.remote-echo failmakes that send fail immediately with an error.remote-echo delay 5sdelays the remote echo for the parsed Go duration.