Add Matrix metadata for LINE images#189
Conversation
|
@coderabbitai review |
📝 WalkthroughWalkthroughLINE image uploads now derive filename, MIME type, and image dimensions from the image bytes, and OBS media downloads now use configurable processing retry timing and counts. ChangesLINE image handling and OBS retry control
Sequence Diagram(s)sequenceDiagram
participant line.Client
participant OBS server
participant caller
caller->>line.Client: DownloadOBSWithSIDOptions(opts)
line.Client->>OBS server: GET media request
OBS server-->>line.Client: 202 or 404 while processing
line.Client->>line.Client: wait retryDelay / increment attempt
line.Client->>OBS server: retry until maxRetries
OBS server-->>line.Client: media body or final error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/connector/handlers/image.go (2)
180-198: ⚡ Quick winConsider logging the fallback to image/jpeg.
When the MIME type cannot be detected, the function silently defaults to
image/jpeg. Logging this fallback would improve observability and help identify unexpected formats from LINE.📊 Proposed enhancement for observability
If you add a logger parameter (or refactor to a method), you could log the fallback:
} } + // Fallback to JPEG; log for observability + // (Consider adding logger param to enable this) return "image/jpeg" }Alternatively, consider returning an explicit indicator (like a second bool return value) when using the fallback, so the caller can log it.
🤖 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/image.go` around lines 180 - 198, The detectLineImageMimeType function silently falls back to returning "image/jpeg" when the MIME type cannot be detected, which reduces observability. Either add a logger parameter to the function and log when the fallback to "image/jpeg" occurs (just before the final return statement), or refactor the function to return both the MIME type string and a boolean flag indicating whether a fallback was used, so the caller can log it. Choose the approach that best fits your codebase's error handling patterns.
172-175: ⚡ Quick winConsider logging DecodeConfig failures for observability.
The error from
image.DecodeConfigis silently ignored. While the fallback behavior (omitting dimensions) is safe, logging the error would help diagnose format support issues, especially for formats like HEIC/HEIF that may not decode with the standard library.📊 Proposed enhancement for observability
if config, _, err := image.DecodeConfig(bytes.NewReader(data)); err == nil { info.Width = config.Width info.Height = config.Height + } else { + // Log decode failure for observability; dimensions will be omitted + // Note: HEIC/HEIF and some formats may not have stdlib decoder support + h.Log.Debug(). + Err(err). + Str("mime_type", mimeType). + Msg("Could not decode image dimensions") }Note: This would require passing the logger (h.Log) to the function or refactoring to a method.
🤖 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/image.go` around lines 172 - 175, The error returned from image.DecodeConfig is being silently ignored, which prevents observability into why image dimensions cannot be decoded, especially for formats like HEIC/HEIF. When the err returned from image.DecodeConfig is not nil, log this error using the logger (h.Log) instead of silently ignoring it. This may require refactoring the function to accept the logger as a parameter or converting the logic to a method that has access to h.Log.
🤖 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/image.go`:
- Around line 180-198: The detectLineImageMimeType function silently falls back
to returning "image/jpeg" when the MIME type cannot be detected, which reduces
observability. Either add a logger parameter to the function and log when the
fallback to "image/jpeg" occurs (just before the final return statement), or
refactor the function to return both the MIME type string and a boolean flag
indicating whether a fallback was used, so the caller can log it. Choose the
approach that best fits your codebase's error handling patterns.
- Around line 172-175: The error returned from image.DecodeConfig is being
silently ignored, which prevents observability into why image dimensions cannot
be decoded, especially for formats like HEIC/HEIF. When the err returned from
image.DecodeConfig is not nil, log this error using the logger (h.Log) instead
of silently ignoring it. This may require refactoring the function to accept the
logger as a parameter or converting the logic to a method that has access to
h.Log.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ce4ce9b-63a9-4f62-8088-d30b9a5537f1
📒 Files selected for processing (3)
.gitignorepkg/connector/handlers/image.gopkg/connector/handlers/image_test.go
💤 Files with no reviewable changes (1)
- .gitignore
📜 Review details
🧰 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/image.gopkg/connector/handlers/image_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/image.gopkg/connector/handlers/image_test.go
🔇 Additional comments (10)
pkg/connector/handlers/image.go (6)
4-22: LGTM!
116-118: LGTM!
147-148: LGTM!
154-163: LGTM!
200-217: LGTM!
1-243: Verify compliance with coding guidelines — Unable to complete verification.The review comment requests verification that this file passes
staticcheck,go vet, andgoimportschecks as specified in the coding guidelines. The initial linting verification failed due to a network error and could not be completed.However, from code inspection, the file does comply with zerolog usage requirements: all logging calls use
.Msg()with structured fields (.Str(),.Bool(),.Dur(),.Err(),.Int()) and do not use.Msgf(). Stringer interface is not applicable to this file.Complete verification of
go vet,staticcheck, andgoimportscompliance requires running these tools directly.pkg/connector/handlers/image_test.go (4)
15-78: LGTM!
80-98: LGTM!
100-128: LGTM!
1-129: Manual verification required: staticcheck, go vet, and goimports compliance.Repository access is currently unavailable for automated verification. Please ensure this test file passes the following checks as required by the coding guidelines:
go vet ./pkg/connector/handlers/image_test.gostaticcheck ./pkg/connector/handlers/image_test.gogoimports -local "github.com/highesttt/matrix-line-messenger" -d ./pkg/connector/handlers/image_test.go
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR improves Matrix image message conversion by detecting the actual image MIME type, generating an appropriate filename/extension, and attaching FileInfo (including dimensions when decodable), with new tests to validate the behavior.
Changes:
- Add
lineImageMediaInfoto detect MIME type, compute filename/extension, and populateevent.FileInfo(size + optional dimensions). - Factor message content creation into
lineImageEventContentto consistently attachInfoand preserverelates_to. - Add Go tests for JPEG/PNG dimension extraction and HEIC/fallback MIME detection; update
.gitignoreto allow tracking_test.gofiles.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/connector/handlers/image.go | Detect image MIME type/extension, attach FileInfo, refactor content creation. |
| pkg/connector/handlers/image_test.go | Add table-driven tests for MIME/filename/dimension behavior and content wiring. |
| .gitignore | Stop ignoring *_test.go so tests are committed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/image_test.go`:
- Line 156: The test case for image/svg+xml in the test data lacks an explicit
switch case in the imageExtensionForMIME function, making it depend on the
system's MIME type database instead of having deterministic behavior like other
image types. Either add an explicit case statement in the imageExtensionForMIME
function to handle "image/svg+xml" and return "svg" for consistency with other
image types, or remove the test case from the test data if SVG support is not a
required feature. Choose the approach based on whether SVG image handling is
needed in your application.
🪄 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: 14c0bbbb-25c7-4927-9d73-8d4c4e0a61de
📒 Files selected for processing (2)
pkg/connector/handlers/image.gopkg/connector/handlers/image_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/connector/handlers/image.go
📜 Review details
🧰 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/image_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/image_test.go
🔇 Additional comments (1)
pkg/connector/handlers/image_test.go (1)
46-155: LGTM!Also applies to: 157-217
|
@clins1994 this might fix your download issue on ios too? |
|
@highesttt yeah. I'm not self-hosting at the moment so can't verify @Adri11334 for testing it'd be great to try getting a friend to send you a HEIC photo or one of those iOS Live Photos and see how it shows on Beeper mobile and what shows when you try to share I'd like to understand how LINE sends those special image formats to another LINE user using Android. e.g. does it flatten the image? to what? I will have both Android/iOS LINE soon so can help out with testing these funky edge cases. For now I'm just chilling on Beeper Cloud until I buy a SIM card for my new Android device 🏖️ |
Quick testing update (only iOS, no Android used)I ran this branch locally against my Beeper account and tested in the LINE official bot chat. Normal LINE -> Matrix images hit the new path fine: OBS download + Matrix upload succeeded, with no MIME fallback or dimension warning in the bridge logs. For HEIF, LINE appears to convert the media to JPEG in this flow. I saw this both directions in my tests (LINE -> Matrix and Matrix -> LINE), so a raw HEIF does not seem to reach the bridge here. I attached two screenshots for that. I also checked Live Photos. LINE stores/displays them as Live Photos in the native app, but Matrix/Beeper does not have a native Live Photo representation here, and sending the paired video as a second Matrix message gives the wrong UX. So this keeps the bridged media as the JPEG image side only. I attached two screenshots for that case as well. 
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/line/client_test.go (1)
69-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a 404 processing-path test.
Line 747 in
pkg/line/client.gonow retries both202and404, but this suite only pins202and non-retryable errors. A small404case would keep that second retry branch from regressing silently.🤖 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/line/client_test.go` around lines 69 - 143, Add a test in TestDownloadOBSWithSIDOptions coverage for the new 404 retry path in DownloadOBSWithSIDOptions: verify that when the OBS handler returns 404 while the media is still processing, the client retries up to the configured MaxProcessingRetries and eventually succeeds or fails with the expected processing error. Use the existing testOBSClient helper, setCachedOBSToken, and patterns from TestDownloadOBSWithSIDOptionsRetriesWhileMediaIsProcessing and TestDownloadOBSWithSIDOptionsUsesDefaultProcessingRetryLimit to keep the retry behavior pinned.
🤖 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/line/client_test.go`:
- Around line 69-143: Add a test in TestDownloadOBSWithSIDOptions coverage for
the new 404 retry path in DownloadOBSWithSIDOptions: verify that when the OBS
handler returns 404 while the media is still processing, the client retries up
to the configured MaxProcessingRetries and eventually succeeds or fails with the
expected processing error. Use the existing testOBSClient helper,
setCachedOBSToken, and patterns from
TestDownloadOBSWithSIDOptionsRetriesWhileMediaIsProcessing and
TestDownloadOBSWithSIDOptionsUsesDefaultProcessingRetryLimit to keep the retry
behavior pinned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e16a99ce-a9f1-4a3f-a9ac-31ae9b631ebe
📒 Files selected for processing (6)
pkg/connector/handlers/image.gopkg/connector/handlers/image_media.gopkg/connector/handlers/image_media_test.gopkg/connector/handlers/image_test.gopkg/line/client.gopkg/line/client_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/handlers/image_media_test.gopkg/line/client_test.gopkg/connector/handlers/image_media.gopkg/connector/handlers/image.gopkg/line/client.gopkg/connector/handlers/image_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/image_media_test.gopkg/line/client_test.gopkg/connector/handlers/image_media.gopkg/connector/handlers/image.gopkg/line/client.gopkg/connector/handlers/image_test.go
🪛 ast-grep (0.44.0)
pkg/connector/handlers/image_test.go
[warning] 253-253: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint32(len(data))
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🔇 Additional comments (6)
pkg/connector/handlers/image_test.go (1)
5-5: LGTM!Also applies to: 20-25, 26-74, 90-109, 125-126, 139-145, 154-154, 165-170, 252-267
pkg/line/client.go (1)
33-46: LGTM!Also applies to: 67-80, 723-775
pkg/line/client_test.go (1)
15-67: LGTM!pkg/connector/handlers/image_media_test.go (1)
9-30: LGTM!pkg/connector/handlers/image_media.go (1)
17-159: LGTM!pkg/connector/handlers/image.go (1)
112-127: LGTM!Also applies to: 147-151, 160-161
Sync main branch
|
I forgot to add this, @clins1994, but here is a screenshot of the option Beeper offers for saving a shared Live Photo (as mentioned earlier, Beeper doesn't support Live Photos, so it can only be saved as a JPEG). Let me know (@highesttt & @clins1994) if you need any further testing before a potential merge. 
|
Summary
content.infoto bridged image events so clients can identify them as imagesoriginalmedia for longer while LINE is still processing it, before falling back to the placeholderFixes #155
Notes
I tested this locally against my Beeper account using the LINE official bot chat on iOS.
For HEIF, LINE appears to convert the media to JPEG in this flow. I saw this both directions in my tests (LINE -> Matrix and Matrix -> LINE), so a raw HEIF did not seem to reach the bridge in this setup.
I also checked Live Photos. LINE stores/displays them as Live Photos in the native app, but Matrix/Beeper does not have a native Live Photo representation here. Sending the paired video as a second Matrix message gives the wrong UX, so this PR keeps the bridged media as the JPEG image side only.
I did not test Android locally. I don't think Android E2E is required for this bridge-side metadata/retry fix, but it would still be useful follow-up coverage for how LINE itself flattens special iOS media formats between native LINE clients.
Testing
GOWORK=off go test ./pkg/connector/handlers ./pkg/lineGOWORK=off go vet ./pkg/connector/handlers ./pkg/lineGOWORK=off go run golang.org/x/tools/cmd/goimports@latest -local github.com/highesttt/matrix-line-messenger -l ...GOWORK=off go run honnef.co/go/tools/cmd/staticcheck@latest ./pkg/connector/handlers ./pkg/linegit diff --checkdocker build -t matrix-line-image-fileinfo-obs-original-retry .docker compose -p line-image-test up --build -d