fix: backfilling system events and reactions#224
Conversation
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughLINE reaction payloads are parsed and converted into cached MXC-backed reactions for backfill and synchronization. Historical system messages are classified, converted into chat-info events, and applied during backfill with updated queue and startup synchronization handling. ChangesReaction and historical synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LINE
participant LineClient
participant BridgeWorker
participant Portal
LINE->>LineClient: return recent messages with reactions
LineClient->>LineClient: convert reactions and resolve MXC icons
LineClient->>BridgeWorker: queue backfill and ReactionSync events
BridgeWorker->>Portal: apply messages and reactions
LINE->>LineClient: return historical system messages
LineClient->>Portal: apply ChatInfoChange events
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/connector/reaction_test.go (1)
522-574: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the
complete == falseand empty/nilReactionscases.The current test only exercises the cache-hit happy path. The behavioral hinges that protect against data loss are elsewhere:
complete == falsemust suppress the sync (otherwiseHasAllUsers: trueredacts reactions LINE still reports), andReactions == []must produce an intentional clear-all. A couple of small table cases would lock those in.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/connector/reaction_test.go` around lines 522 - 574, Extend TestConvertMessageReactionsUsesEmbeddedHistory with table-driven cases for incomplete conversion and empty or nil msg.Reactions. Verify complete == false suppresses synchronization, while both empty and nil reactions intentionally produce a clear-all result. Preserve the existing cache-hit assertions for populated reactions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/connector/reaction.go`:
- Around line 340-343: Update the per-reaction timestamp initialization near
reaction.AtMillis.Int64 so missing, malformed, or non-positive atMillis values
fall back to the enclosing message timestamp instead of remaining zero. Preserve
the parsed time.UnixMilli value for valid positive atMillis values, ensuring
queueMessageReactionSync and FetchMessages receive a usable reaction timestamp.
- Around line 260-271: Update the paid reaction icon download flow around
lc.HTTPClient.Get to create and use a context with the operation’s ctx and
ensure cancellation propagates during shutdown. Replace unbounded io.ReadAll of
resp.Body with a bounded read using the established maximum icon size, while
preserving the existing HTTP-status and error-wrapping behavior.
---
Nitpick comments:
In `@pkg/connector/reaction_test.go`:
- Around line 522-574: Extend TestConvertMessageReactionsUsesEmbeddedHistory
with table-driven cases for incomplete conversion and empty or nil
msg.Reactions. Verify complete == false suppresses synchronization, while both
empty and nil reactions intentionally produce a clear-all result. Preserve the
existing cache-hit assertions for populated reactions.
🪄 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: bc351f2f-0cb0-424b-90e3-c3550f79b556
📒 Files selected for processing (10)
pkg/connector/client.gopkg/connector/consts.gopkg/connector/handle_message.gopkg/connector/reaction.gopkg/connector/reaction_test.gopkg/connector/sync.gopkg/connector/sync_test.gopkg/line/reaction.gopkg/line/structs.gopkg/line/structs_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: build-docker
- GitHub Check: Lint with 1.25
- GitHub Check: build-docker
- GitHub Check: Lint with 1.25
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Usego fmtfor code formatting across all Go files
Usegoimportswith-local "github.com/highesttt/matrix-line-messenger"flag to group project-local imports correctly
Usezerologfor logging throughout the codebase
Do not useMsgfin logging; useMsgwith structured fields instead
UseStringerinterface where applicable in Go code
Files:
pkg/connector/consts.gopkg/line/structs.gopkg/line/reaction.gopkg/connector/client.gopkg/line/structs_test.gopkg/connector/handle_message.gopkg/connector/reaction_test.gopkg/connector/sync_test.gopkg/connector/reaction.gopkg/connector/sync.go
**/!(ltsm)/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/!(ltsm)/**/*.go: Runstaticcheckon all Go files excludingpkg/ltsmpackage (transpiled WASM code)
Rungo veton all Go files excludingpkg/ltsmpackage (transpiled WASM code)
Files:
pkg/connector/consts.gopkg/line/structs.gopkg/line/reaction.gopkg/connector/client.gopkg/line/structs_test.gopkg/connector/handle_message.gopkg/connector/reaction_test.gopkg/connector/sync_test.gopkg/connector/reaction.gopkg/connector/sync.go
🔇 Additional comments (16)
pkg/line/reaction.go (1)
69-75: LGTM!pkg/line/structs.go (1)
112-113: LGTM!pkg/line/structs_test.go (1)
48-91: LGTM!Also applies to: 93-109
pkg/connector/client.go (1)
82-82: LGTM!pkg/connector/reaction.go (2)
206-245: LGTM!
353-395: LGTM!pkg/connector/sync.go (7)
650-657: LGTM!
773-840: LGTM!
1197-1197: LGTM!Also applies to: 2039-2039
2063-2083: LGTM!Also applies to: 2093-2119
2468-2607: LGTM!
739-742: 🩺 Stability & AvailabilityNo deadlock here —
syncChatsalready owns the matchingwg.Done(). Thewg.Add(1)at this call site is balanced bydefer lc.wg.Done()at the top ofsyncChats, solc.wg.Wait()won’t hang on this path.> Likely an incorrect or invalid review comment.
2635-2658: 🗄️ Data Integrity & Integration
DontBridgerows still get inserted with a fake MXID, and chat-info updates already run through the normal portal event path.> Likely an incorrect or invalid review comment.pkg/connector/consts.go (1)
41-41: LGTM!pkg/connector/sync_test.go (1)
69-121: LGTM!Also applies to: 123-161
pkg/connector/handle_message.go (1)
116-116: LGTM!Also applies to: 126-126, 167-168
| resp, err := lc.HTTPClient.Get(iconURL) | ||
| if err != nil { | ||
| return "", fmt.Errorf("download paid reaction icon: %w", err) | ||
| } | ||
| defer resp.Body.Close() | ||
| if resp.StatusCode != 200 { | ||
| return "", fmt.Errorf("download paid reaction icon: HTTP %d", resp.StatusCode) | ||
| } | ||
| data, err := io.ReadAll(resp.Body) | ||
| if err != nil { | ||
| return "", fmt.Errorf("read paid reaction icon: %w", err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate HTTPClient construction for LineClient and check for Timeout settings.
rg -nP -C4 'HTTPClient\s*[:=]' --type=go
rg -nP -C3 '&http\.Client\{' --type=goRepository: beeper/line
Length of output: 149
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all HTTP client construction/use sites related to the connector/client.
rg -n -C3 --type=go '\bHTTPClient\b|&http\.Client\{|http\.Client\{' .
# Inspect the file under review and nearby context.
sed -n '220,310p' pkg/connector/reaction.go
# Find the LineClient type and constructor.
rg -n -C4 --type=go 'type LineClient|func NewLineClient|NewLineClient\(' pkg
# Search for any request-timeout or context-aware download helpers around this path.
rg -n -C3 --type=go 'NewRequestWithContext|LimitReader|io\.ReadAll|StatusOK|Timeout:' pkgRepository: beeper/line
Length of output: 34768
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the startup backfill / wait path mentioned in the comment.
rg -n -C4 --type=go 'queueMessageReactionSync|backfillRecentMessages|wg\.Wait\(\)|Wait\(\)' pkg/connector
# Inspect the surrounding implementation in the connector.
sed -n '520,690p' pkg/connector/connector.go
# Inspect the paid reaction download call site in full context.
sed -n '240,290p' pkg/connector/reaction.goRepository: beeper/line
Length of output: 12191
Propagate ctx into the icon download and bound the body size. This runs on the startup backfill path, so a slow fetch can still hold up wg.Wait() on logout/shutdown even though lc.HTTPClient has a 10s timeout, and io.ReadAll is unbounded on remote content.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/connector/reaction.go` around lines 260 - 271, Update the paid reaction
icon download flow around lc.HTTPClient.Get to create and use a context with the
operation’s ctx and ensure cancellation propagates during shutdown. Replace
unbounded io.ReadAll of resp.Body with a bounded read using the established
maximum icon size, while preserving the existing HTTP-status and error-wrapping
behavior.
| var timestamp time.Time | ||
| if timestampMillis, err := reaction.AtMillis.Int64(); err == nil && timestampMillis > 0 { | ||
| timestamp = time.UnixMilli(timestampMillis) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fall back to the message timestamp when atMillis is unusable.
A missing/malformed atMillis leaves Timestamp as the zero time.Time. queueMessageReactionSync only falls back for the aggregate event timestamp; the per-reaction value stays zero, and FetchMessages (pkg/connector/sync.go Line 650) puts these straight into BackfillMessage.Reactions, producing epoch-1970 reactions in the batch.
🔧 Proposed fix
- var timestamp time.Time
- if timestampMillis, err := reaction.AtMillis.Int64(); err == nil && timestampMillis > 0 {
- timestamp = time.UnixMilli(timestampMillis)
- }
+ timestamp := lc.parseMessageTimestamp(msg)
+ if timestampMillis, err := reaction.AtMillis.Int64(); err == nil && timestampMillis > 0 {
+ timestamp = time.UnixMilli(timestampMillis)
+ }📝 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.
| var timestamp time.Time | |
| if timestampMillis, err := reaction.AtMillis.Int64(); err == nil && timestampMillis > 0 { | |
| timestamp = time.UnixMilli(timestampMillis) | |
| } | |
| timestamp := lc.parseMessageTimestamp(msg) | |
| if timestampMillis, err := reaction.AtMillis.Int64(); err == nil && timestampMillis > 0 { | |
| timestamp = time.UnixMilli(timestampMillis) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/connector/reaction.go` around lines 340 - 343, Update the per-reaction
timestamp initialization near reaction.AtMillis.Int64 so missing, malformed, or
non-positive atMillis values fall back to the enclosing message timestamp
instead of remaining zero. Preserve the parsed time.UnixMilli value for valid
positive atMillis values, ensuring queueMessageReactionSync and FetchMessages
receive a usable reaction timestamp.
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/connector/client.go (1)
82-82: 📐 Maintainability & Code Quality | 🔵 TrivialRun the repo Go checks before merge.
gofmt,goimports -local "github.com/highesttt/matrix-line-messenger",staticcheck, andgo vetshould pass for all Go packages exceptpkg/ltsm.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/connector/client.go` at line 82, Run gofmt, goimports with the repository’s local import prefix, staticcheck, and go vet across all Go packages except pkg/ltsm; resolve any reported issues and formatting changes before merging, without modifying pkg/ltsm checks.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@pkg/connector/client.go`:
- Line 82: Run gofmt, goimports with the repository’s local import prefix,
staticcheck, and go vet across all Go packages except pkg/ltsm; resolve any
reported issues and formatting changes before merging, without modifying
pkg/ltsm checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 65d77776-cc29-4684-be83-b264ca5f50ce
📒 Files selected for processing (5)
pkg/connector/client.gopkg/connector/reaction.gopkg/connector/reaction_test.gopkg/connector/sync.gopkg/connector/sync_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/connector/reaction.go
- pkg/connector/sync.go
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: build-docker
- GitHub Check: Lint with 1.25
- GitHub Check: build-docker
- GitHub Check: Lint with 1.25
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Usego fmtfor code formatting across all Go files
Usegoimportswith-local "github.com/highesttt/matrix-line-messenger"flag to group project-local imports correctly
Usezerologfor logging throughout the codebase
Do not useMsgfin logging; useMsgwith structured fields instead
UseStringerinterface where applicable in Go code
Files:
pkg/connector/client.gopkg/connector/sync_test.gopkg/connector/reaction_test.go
**/!(ltsm)/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/!(ltsm)/**/*.go: Runstaticcheckon all Go files excludingpkg/ltsmpackage (transpiled WASM code)
Rungo veton all Go files excludingpkg/ltsmpackage (transpiled WASM code)
Files:
pkg/connector/client.gopkg/connector/sync_test.gopkg/connector/reaction_test.go
🔇 Additional comments (9)
pkg/connector/client.go (2)
655-655: LGTM!
82-82: 🩺 Stability & AvailabilityNo change needed for
paidReactionIconMXCIt is allocated lazily ingetPaidReactionMXCbefore the first write, so leaving it nil on construction is safe.> Likely an incorrect or invalid review comment.pkg/connector/reaction_test.go (4)
5-12: LGTM!
576-586: LGTM!
588-604: LGTM!
522-574: 🎯 Functional CorrectnessIncorrect concern: the embedded-reaction test body is present here and already covers predefined/paid reactions, sender checks, and millisecond timestamps.
> Likely an incorrect or invalid review comment.pkg/connector/sync_test.go (3)
4-9: LGTM!
165-190: LGTM!
71-123: 🎯 Functional CorrectnessNo action needed The historical membership, malformed-record, and unsupported-
LOC_KEYcases are already present inpkg/connector/sync_test.go, so this concern doesn’t apply.> Likely an incorrect or invalid review comment.
1 participant
No description provided.