fix: allow reusing LINE custom reactions - #227
Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (4)
📝 WalkthroughSummary by CodeRabbit
WalkthroughChangesReaction handling refactor
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LINE as LINE SSE
participant Connector as Reaction handlers
participant Bridge as RemoteEventReactionSync
LINE->>Connector: reaction add or remove
Connector->>Bridge: enqueue sender-bound sync
Bridge-->>Connector: reaction state update
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 |
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
| if err != nil { | ||
| return lineReactionRef{}, fmt.Errorf("get target message reactions: %w", err) | ||
| } | ||
| ref, ok := storedLineReactionForMatrixKey(key, reactions) |
There was a problem hiding this comment.
Latent: legacy EmojiID="" rows are unreachable via this lookup.
Rows persisted before this PR were written with EmojiID="" and Metadata=nil. The Reaction metadata factory now returns &ReactionMetadata{} on load, so those rows come back with an empty MatrixKey, fail the meta.MatrixKey != key filter in storedLineReactionForMatrixKey, and this call returns unsupportedMatrixReactionError.
Trigger: any user tries to re-react (from Matrix) to a paid reaction that was originally bridged before this PR shipped. It resolves itself once someone else reacts and a live 139/140 event creates a new stable-ID row (the HasAllReactions=true sweep then also redacts the legacy row), but until then the paid MXC in the picker looks broken.
Worth calling out in the changelog / considering a one-shot migration that populates metadata for existing rows from Emoji if it's an MXC URL that maps to a known LINE sticker.
![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
🤖 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_test.go`:
- Around line 797-810: Strengthen the assertions around liveReactionSyncEvent in
the reaction test to verify that only Uother is present in add.Reactions.Users
and remove.Reactions.Users. Keep the existing Uother validation, and assert the
collection size or explicitly reject any additional sender entries so
regressions affecting other users fail.
- Around line 581-582: Update the response assertion in the reaction test to
require the stable resolved EmojiID value for 👍️, replacing the current
non-empty check while preserving the existing SenderID, Emoji, and MaxReactions
assertions.
🪄 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: 8bab1afb-13f0-40cf-87c3-5eea0a2c265d
📒 Files selected for processing (5)
pkg/connector/client.gopkg/connector/connector.gopkg/connector/reaction.gopkg/connector/reaction_test.gopkg/connector/sync.go
💤 Files with no reviewable changes (1)
- pkg/connector/client.go
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: build-docker
- GitHub Check: Lint with 1.25
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/connector.gopkg/connector/sync.gopkg/connector/reaction_test.gopkg/connector/reaction.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/connector.gopkg/connector/sync.gopkg/connector/reaction_test.gopkg/connector/reaction.go
pkg/connector/connector.go
📄 CodeRabbit inference engine (AGENTS.md)
Implement
bridgev2.NetworkConnectorandbridgev2.NetworkAPIinterfaces in the connector package for bridge logic
Files:
pkg/connector/connector.go
🔇 Additional comments (15)
pkg/connector/reaction_test.go (2)
437-503: LGTM!Also applies to: 505-563, 720-742
34-36: 📐 Maintainability & Code QualityNo change needed here.
> Likely an incorrect or invalid review comment.pkg/connector/reaction.go (9)
39-46: LGTM!
59-84: LGTM!
86-95: LGTM!
107-113: LGTM!
365-394: LGTM!
412-421: LGTM!Also applies to: 431-431
562-593: LGTM!
707-719: LGTM!Also applies to: 721-751
97-105: 🎯 Functional Correctness
equal()may be too strict for reaction reuse. IfResourceTypeorVersioncan differ for the same visible paid reaction,storedLineReactionForMatrixKeywill treat matching rows as conflicting and skip reuse.pkg/connector/connector.go (1)
102-107: LGTM!pkg/connector/sync.go (3)
2063-2079: LGTM!
2081-2127: LGTM!
2129-2139: LGTM!
| if resp.SenderID != "Uself" || resp.EmojiID == "" || resp.Emoji != "\U0001F44D\uFE0F" || resp.MaxReactions != 1 { | ||
| t.Fatalf("pre-handle response = %#v", resp) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C 3 --type go '1F44D|predefined:2|linePaidReactionForMatrixEmoji' pkg/connector pkg/lineRepository: beeper/line
Length of output: 21601
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '500,590p' pkg/connector/reaction_test.go
printf '\n----\n'
sed -n '520,590p' pkg/connector/reaction.go
printf '\n----\n'
sed -n '1,140p' pkg/line/reaction.goRepository: beeper/line
Length of output: 8056
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' pkg/connector/reaction.go
printf '\n----\n'
sed -n '220,520p' pkg/connector/reaction.go
printf '\n----\n'
sed -n '1,220p' pkg/connector/reaction_test.goRepository: beeper/line
Length of output: 26827
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'func .*PreHandleMatrixReaction|EmojiID:' pkg/connectorRepository: beeper/line
Length of output: 8873
Assert the resolved LINE emoji ID here. EmojiID should be pinned to the expected stable value for 👍️ (for example, paid:670e0cce840a8236ddd4ee4c:143) instead of only checking it is non-empty.
🤖 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 581 - 582, Update the response
assertion in the reaction test to require the stable resolved EmojiID value for
👍️, replacing the current non-empty check while preserving the existing
SenderID, Emoji, and MaxReactions assertions.
| t.Fatal("single-sender live sync was marked authoritative for all users") | ||
| } | ||
| userSync := add.Reactions.Users["Uother"] | ||
| if userSync == nil || !userSync.HasAllReactions || len(userSync.Reactions) != 1 || userSync.Reactions[0] != reaction { | ||
| t.Fatalf("add user sync = %#v", userSync) | ||
| } | ||
| if want := time.UnixMilli(1784930400123); !add.Timestamp.Equal(want) { | ||
| t.Fatalf("add timestamp = %s, want %s", add.Timestamp, want) | ||
| } | ||
|
|
||
| remove := lc.liveReactionSyncEvent(op, "Cgroup", "Uother", nil) | ||
| userSync = remove.Reactions.Users["Uother"] | ||
| if userSync == nil || !userSync.HasAllReactions || len(userSync.Reactions) != 0 { | ||
| t.Fatalf("remove user sync = %#v", userSync) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Verify no other sender is marked authoritative.
The test checks Uother's entry but not that it is the only entry. A regression that also clears another sender would still pass.
Proposed test strengthening
if add.Reactions.HasAllUsers {
t.Fatal("single-sender live sync was marked authoritative for all users")
}
+ if len(add.Reactions.Users) != 1 {
+ t.Fatalf("add sync has %d authoritative users, want 1", len(add.Reactions.Users))
+ }
userSync := add.Reactions.Users["Uother"]
if userSync == nil || !userSync.HasAllReactions || len(userSync.Reactions) != 1 || userSync.Reactions[0] != reaction {
t.Fatalf("add user sync = %#v", userSync)
}
@@
remove := lc.liveReactionSyncEvent(op, "Cgroup", "Uother", nil)
+ if len(remove.Reactions.Users) != 1 {
+ t.Fatalf("remove sync has %d authoritative users, want 1", len(remove.Reactions.Users))
+ }
userSync = remove.Reactions.Users["Uother"]📝 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.
| t.Fatal("single-sender live sync was marked authoritative for all users") | |
| } | |
| userSync := add.Reactions.Users["Uother"] | |
| if userSync == nil || !userSync.HasAllReactions || len(userSync.Reactions) != 1 || userSync.Reactions[0] != reaction { | |
| t.Fatalf("add user sync = %#v", userSync) | |
| } | |
| if want := time.UnixMilli(1784930400123); !add.Timestamp.Equal(want) { | |
| t.Fatalf("add timestamp = %s, want %s", add.Timestamp, want) | |
| } | |
| remove := lc.liveReactionSyncEvent(op, "Cgroup", "Uother", nil) | |
| userSync = remove.Reactions.Users["Uother"] | |
| if userSync == nil || !userSync.HasAllReactions || len(userSync.Reactions) != 0 { | |
| t.Fatalf("remove user sync = %#v", userSync) | |
| t.Fatal("single-sender live sync was marked authoritative for all users") | |
| } | |
| if len(add.Reactions.Users) != 1 { | |
| t.Fatalf("add sync has %d authoritative users, want 1", len(add.Reactions.Users)) | |
| } | |
| userSync := add.Reactions.Users["Uother"] | |
| if userSync == nil || !userSync.HasAllReactions || len(userSync.Reactions) != 1 || userSync.Reactions[0] != reaction { | |
| t.Fatalf("add user sync = %#v", userSync) | |
| } | |
| if want := time.UnixMilli(1784930400123); !add.Timestamp.Equal(want) { | |
| t.Fatalf("add timestamp = %s, want %s", add.Timestamp, want) | |
| } | |
| remove := lc.liveReactionSyncEvent(op, "Cgroup", "Uother", nil) | |
| if len(remove.Reactions.Users) != 1 { | |
| t.Fatalf("remove sync has %d authoritative users, want 1", len(remove.Reactions.Users)) | |
| } | |
| userSync = remove.Reactions.Users["Uother"] | |
| if userSync == nil || !userSync.HasAllReactions || len(userSync.Reactions) != 0 { | |
| t.Fatalf("remove user sync = %#v", userSync) |
🤖 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 797 - 810, Strengthen the
assertions around liveReactionSyncEvent in the reaction test to verify that only
Uother is present in add.Reactions.Users and remove.Reactions.Users. Keep the
existing Uother validation, and assert the collection size or explicitly reject
any additional sender entries so regressions affecting other users fail.
1 participant
No description provided.