fix: 🐛 inline icon bug#205
Conversation
Co-authored-by: Codex <codex@openai.com>
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughInline LINE sticon detection is broadened, sticon replacement offsets now use UTF-16 code units, and message bodies are rebuilt with placeholder cleanup and HTML rendering. New helpers expose sticon-body and placeholder detection, with tests covering the updated conversion path. ChangesInline sticon detection and conversion rework
Estimated code review effort: 3 (Moderate) | ~30 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/connector/handlers/emoji_test.go (1)
8-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding failure-path coverage.
All tests exercise only the happy path.
sticonResourceByteRangereturnsfalsewhenr.End <= r.Startor when offsets don't resolve (per the upstream contract inemoji.go), andbuildSticonMessageBodiessilently skips replacements with invalid/overlapping ranges (r.start < pos || r.end <= r.start || ...). These fallback branches protect against malformed LINE payloads (the exact class of bug this PR fixes), so a regression there could reintroduce garbled output without any test failing.🤖 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/handlers/emoji_test.go` around lines 8 - 88, Add failure-path coverage for the emoji helpers: extend the tests around sticonResourceByteRange and buildSticonMessageBodies to verify they return false/skip replacements when ranges are invalid, reversed, overlapping, or otherwise unresolvable. Use the existing symbols sticonResourceByteRange, buildSticonMessageBodies, and SticonResource to target the fallback branches in emoji_test.go so malformed LINE payloads don’t regress silently.
🤖 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/handlers/emoji.go`:
- Around line 365-367: The placeholder check in isLineSticonPlaceholderRune is
too broad because unicode.Is(unicode.Co, r) matches every private-use rune,
causing unrelated PUA text to be treated as LINE placeholders. Narrow the logic
in isLineSticonPlaceholderRune, and any callers like ConvertInlineEmoji that
depend on it, so it only recognizes the actual LINE placeholder code points or
range instead of the entire Co category.
---
Nitpick comments:
In `@pkg/connector/handlers/emoji_test.go`:
- Around line 8-88: Add failure-path coverage for the emoji helpers: extend the
tests around sticonResourceByteRange and buildSticonMessageBodies to verify they
return false/skip replacements when ranges are invalid, reversed, overlapping,
or otherwise unresolvable. Use the existing symbols sticonResourceByteRange,
buildSticonMessageBodies, and SticonResource to target the fallback branches in
emoji_test.go so malformed LINE payloads don’t regress silently.
🪄 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: 82f433fa-42c2-4028-95ab-dbb5602e2841
📒 Files selected for processing (3)
pkg/connector/handle_message.gopkg/connector/handlers/emoji.gopkg/connector/handlers/emoji_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- 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/handle_message.gopkg/connector/handlers/emoji_test.gopkg/connector/handlers/emoji.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/handle_message.gopkg/connector/handlers/emoji_test.gopkg/connector/handlers/emoji.go
🔇 Additional comments (11)
pkg/connector/handlers/emoji_test.go (5)
8-22: LGTM!
24-55: LGTM! Directly exercisingbuildSticonMessageBodieswith byte offsets (bypassing the UTF-16 converter, already covered separately) is a clean way to isolate the body-construction logic, and the out-of-order replacement input correctly exercises the internal sort.
64-69: LGTM! Using a raw (backtick) string here keeps the\uescapes as literal JSON text rather than compiling to actual surrogate-half runes, which correctly simulates the raw payloadHasSticonBodywould receive.
71-76: LGTM!
1-6: LGTM!Also applies to: 78-88
pkg/connector/handle_message.go (1)
378-390: LGTM!pkg/connector/handlers/emoji.go (5)
73-80: LGTM!
103-176: LGTM!
187-275: LGTM!
277-313: LGTM!
387-395: LGTM!
Co-authored-by: Codex <codex@openai.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/connector/handlers/emoji_test.go (1)
43-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the LINE placeholder literals to a shared test constant.
\U00100084/\U00100085are hardcoded repeatedly across multiple test functions. A single shared var/const (e.g.,linePlaceholder1,linePlaceholder2) would reduce duplication and keep intent clearer if the placeholder set ever grows.♻️ Example refactor
+const ( + testLineSticonPlaceholder1 = "\U00100084" + testLineSticonPlaceholder2 = "\U00100085" +)Also applies to: 76-76, 110-110, 117-124
🤖 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/handlers/emoji_test.go` around lines 43 - 44, The test code repeats the LINE placeholder literals in multiple places, so extract them into shared test constants in emoji_test.go and update the affected test functions to use those named placeholders instead of hardcoded values. Use clear symbols such as linePlaceholder1 and linePlaceholder2 near the existing first/second declarations so the intent stays obvious and any future placeholder expansion only needs one update.
🤖 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/handlers/emoji_test.go`:
- Around line 43-44: The test code repeats the LINE placeholder literals in
multiple places, so extract them into shared test constants in emoji_test.go and
update the affected test functions to use those named placeholders instead of
hardcoded values. Use clear symbols such as linePlaceholder1 and
linePlaceholder2 near the existing first/second declarations so the intent stays
obvious and any future placeholder expansion only needs one update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f89a0160-a0fb-4625-990c-5a25c0854486
📒 Files selected for processing (2)
pkg/connector/handlers/emoji.gopkg/connector/handlers/emoji_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/connector/handlers/emoji.go
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- 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/handlers/emoji_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/handlers/emoji_test.go
🔇 Additional comments (3)
pkg/connector/handlers/emoji_test.go (3)
8-22: LGTM!
24-107: LGTM!Verified against
sticonResourceByteRangeandbuildSticonMessageBodiesimplementations — the empty/reversed/negative/unresolvable and overlap/empty/reversed/out-of-range skip cases are all correctly exercised.
109-126: LGTM!
Co-authored-by: Codex <codex@openai.com>
1 participant
Fixes #195.
Summary
Tests