feat: show images in album notifications - #226
Conversation
|
📝 WalkthroughWalkthroughChangesPost notification media conversion
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LINE
participant Handler
participant OBS
participant Matrix
LINE->>Handler: deliver post notification
Handler->>OBS: download album preview resource
OBS-->>Handler: return image bytes
Handler->>Matrix: upload image
Matrix-->>Handler: return MXC URI
Handler-->>LINE: produce notice and image parts
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ 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.
🧹 Nitpick comments (1)
pkg/connector/handlers/post_notification.go (1)
93-113: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCap the number of previews downloaded per notification.
previewMediascomes straight from remote metadata with no length limit, so a single notification can trigger an unbounded serial sequence of OBS downloads plus Matrix uploads on the message-handling path. A small ceiling (LINE shows at most a handful of previews) bounds latency and memory.♻️ Suggested cap
+ const maxAlbumPreviews = 9 + if len(previewMedias) > maxAlbumPreviews { + previewMedias = previewMedias[:maxAlbumPreviews] + } client := h.NewClient()🤖 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/post_notification.go` around lines 93 - 113, Cap the preview processing in the loop over previewMedias to a small fixed maximum before invoking downloadOBSResource or subsequent uploads. Preserve the existing handling for expired and other download errors, and ensure notifications with more previews stop processing once the cap is reached.
🤖 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/post_notification.go`:
- Around line 93-113: Cap the preview processing in the loop over previewMedias
to a small fixed maximum before invoking downloadOBSResource or subsequent
uploads. Preserve the existing handling for expired and other download errors,
and ensure notifications with more previews stop processing once the cap is
reached.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e9e5e909-f3de-44ba-a16e-0f96e26d24da
📒 Files selected for processing (7)
pkg/connector/handle_message.gopkg/connector/handle_message_test.gopkg/connector/handlers/handler.gopkg/connector/handlers/post_notification.gopkg/connector/handlers/post_notification_test.gopkg/line/client.gopkg/line/obs_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Lint with 1.25
- GitHub Check: build-docker
- GitHub Check: Lint with 1.25
- GitHub Check: build-docker
🧰 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_test.gopkg/line/obs_test.gopkg/connector/handlers/handler.gopkg/line/client.gopkg/connector/handle_message.gopkg/connector/handlers/post_notification_test.gopkg/connector/handlers/post_notification.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_test.gopkg/line/obs_test.gopkg/connector/handlers/handler.gopkg/line/client.gopkg/connector/handle_message.gopkg/connector/handlers/post_notification_test.gopkg/connector/handlers/post_notification.go
🔇 Additional comments (11)
pkg/line/client.go (2)
719-727: LGTM!
704-718: 🎯 Functional CorrectnessConfirm empty-SID callers. The new non-empty
sidcheck also applies toDownloadOBSWithSIDOptions; any existing caller that still passes an empty SID will now fail.pkg/line/obs_test.go (1)
91-133: LGTM!pkg/connector/handlers/post_notification.go (4)
4-33: LGTM!
115-146: LGTM!
148-194: LGTM!
196-208: 🎯 Functional CorrectnessWebP previews are already supported
http.DetectContentTypereturnsimage/webpin the Go toolchain targeted here, so this branch won’t silently drop WebP images.> Likely an incorrect or invalid review comment.pkg/connector/handlers/handler.go (1)
32-44: LGTM!pkg/connector/handle_message.go (1)
380-380: LGTM!pkg/connector/handlers/post_notification_test.go (1)
19-27: LGTM!Also applies to: 86-154, 156-189, 191-232, 234-265, 282-283
pkg/connector/handle_message_test.go (1)
125-125: LGTM!
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
| } else { | ||
| body.WriteString("\n\nOpen LINE for full details.") | ||
| } | ||
|
|
There was a problem hiding this comment.
The previous implementation appended Open in LINE: <postEndUrl> (plain + HTML anchor) for every notification, and Open LINE for full details. when the URL was missing. Both branches are gone now, so GB notes, unknown service types, and albums that produce zero image parts (all previews expired/unsupported, or intent/portal missing) drop back to a bare notice with no way to reach the original post. The commit message only mentions adding album images, so this collateral UX change looks worth calling out explicitly. If it is intentional, consider updating the commit message; otherwise, keeping the link for non-AB types (and as a fallback when no image parts get appended) would preserve the previous behavior.
| } | ||
|
|
||
| converted.Parts = append(converted.Parts, &bridgev2.ConvertedMessagePart{ | ||
| ID: networkid.PartID(fmt.Sprintf("album-image-%d", index+1)), |
There was a problem hiding this comment.
index here is the position in the post-filter/dedupe previewMedias slice, not the count of successfully-uploaded images (the test intentionally expects album-image-2 when the first entry was expired). Two consequences:
- If a re-process of the same message runs after an expired preview becomes downloadable, the surviving image that was
album-image-2last time will bealbum-image-1this time, and bridgev2 will treat them as different parts (potential duplicate upload). - If LINE ever returns
previewMediasin a different order between backfill and live paths, the same OID gets a differentPartID.
Using the OID (already unique per media after the dedupe map) would make part identity stable across retries and orderings — something like networkid.PartID("album-image-" + media.OID).
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/line/client.go`:
- Around line 726-777: Add a finite timeout to the OBS client used by
DownloadAlbumPreview so stalled requests cannot block workers indefinitely when
the caller context has no deadline. Update the OBSClient initialization in
NewClient, or derive a bounded context before the OBS request, while preserving
caller cancellation and existing download behavior.
In `@pkg/line/methods.go`:
- Around line 781-783: Update the RFC3339 parsing branch in the expiration-time
method to apply the same near-expiry validation as the numeric paths: compare
parsedTime against now.Add(obsTokenBuffer), and return the existing fallback
when it is too early; otherwise preserve the parsed expiration adjusted by
obsTokenBuffer.
🪄 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: 9ac34a2d-bda7-47a6-b6c7-886de592e1c1
📒 Files selected for processing (6)
pkg/connector/handlers/handler.gopkg/connector/handlers/post_notification.gopkg/connector/handlers/post_notification_test.gopkg/line/client.gopkg/line/methods.gopkg/line/obs_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/connector/handlers/post_notification.go
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Lint with 1.25
- GitHub Check: build-docker
- 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/line/methods.gopkg/line/obs_test.gopkg/connector/handlers/handler.gopkg/connector/handlers/post_notification_test.gopkg/line/client.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/line/methods.gopkg/line/obs_test.gopkg/connector/handlers/handler.gopkg/connector/handlers/post_notification_test.gopkg/line/client.go
🔇 Additional comments (7)
pkg/line/client.go (2)
33-34: LGTM!Also applies to: 49-63
716-724: 🗄️ Data Integrity & IntegrationNo changes needed for
DownloadOBSResource— it delegates todownloadOBSWithServiceAndSIDOptions, which validatesservice/sid/oidand escapes the OBS path components before building the request.pkg/line/methods.go (1)
719-767: LGTM!pkg/connector/handlers/handler.go (1)
32-47: LGTM!pkg/connector/handlers/post_notification_test.go (2)
23-177: LGTM!Also applies to: 307-537
179-250: 🎯 Functional CorrectnessNo issue:
for range <int>is valid here. The module targets Go 1.25.0, so bothfor range albumPreviewWorkerLimitandfor range 2compile as written.> Likely an incorrect or invalid review comment.pkg/line/obs_test.go (1)
91-133: LGTM!Also applies to: 424-473
| // DownloadAlbumPreview retrieves the thumbnail referenced by an album post | ||
| // notification. Album thumbnails use a dedicated TID and are authorized by the | ||
| // notification's chatId through LINE's Chrome home channel token. | ||
| func (c *Client) DownloadAlbumPreview(ctx context.Context, oid, chatID, albumID string) ([]byte, error) { | ||
| if oid == "" || chatID == "" { | ||
| return nil, errors.New("album preview OID and chat ID are required") | ||
| } | ||
| channelToken, err := c.AcquireChannelAccessToken(albumPreviewChannelID) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to acquire album preview channel token: %w", err) | ||
| } | ||
|
|
||
| requestURL := fmt.Sprintf( | ||
| "%s/r/album/a/%s/%s", | ||
| OBSBaseURL, | ||
| url.PathEscape(oid), | ||
| albumPreviewTID, | ||
| ) | ||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, requestURL, nil) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to create album preview request: %w", err) | ||
| } | ||
| req.Header.Set("User-Agent", UserAgent) | ||
| req.Header.Set("X-Line-ChannelToken", channelToken) | ||
| req.Header.Set("X-Line-Mid", chatID) | ||
| if albumID != "" { | ||
| req.Header.Set("X-Line-Album", albumID) | ||
| } | ||
| if c.AccessToken != "" { | ||
| req.Header.Set("X-Line-Access", c.AccessToken) | ||
| } | ||
|
|
||
| resp, err := c.obsHTTPClient().Do(req) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("OBS download request failed: %w", err) | ||
| } | ||
| body, readErr := io.ReadAll(resp.Body) | ||
| resp.Body.Close() | ||
| if readErr != nil { | ||
| return nil, fmt.Errorf("failed to read OBS response body: %w", readErr) | ||
| } | ||
| switch resp.StatusCode { | ||
| case http.StatusOK: | ||
| return body, nil | ||
| case http.StatusAccepted: | ||
| return nil, ErrOBSEncodingIncomplete | ||
| case http.StatusNotFound: | ||
| return nil, ErrOBSObjectNotFound | ||
| default: | ||
| return nil, fmt.Errorf("OBS download failed (%d): %s", resp.StatusCode, string(body)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Confirm whether the context passed into DownloadAlbumPreview/ConvertPostNotification carries a deadline upstream.
rg -n -A5 -B5 'ConvertPostNotification\(' pkg/connector/handle_message.go
rg -n 'context.WithTimeout|context.WithDeadline' pkg/connectorRepository: beeper/line
Length of output: 1148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant files and inspect the exact call path / client construction.
git ls-files 'pkg/line/client.go' 'pkg/connector/handle_message.go' 'pkg/connector/*.go' | sort
echo '--- client.go outline ---'
ast-grep outline pkg/line/client.go --view expanded
echo '--- handle_message.go outline ---'
ast-grep outline pkg/connector/handle_message.go --view expanded
echo '--- relevant snippets ---'
rg -n -A30 -B10 'func \(c \*Client\) NewClient|func \(c \*Client\) obsHTTPClient|type Client struct|func \(c \*Client\) DownloadAlbumPreview|func \(.*ConvertPostNotification|album-preview|worker pool|concurrently|WithTimeout|WithDeadline' pkg/line/client.go pkg/connector/handle_message.go pkg/connector/*.goRepository: beeper/line
Length of output: 23837
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A20 -B10 'OBSClient|HTTPClient|rpcClientTimeout|Timeout:|obsHTTPClient\(\)|DownloadAlbumPreview\(' pkg/line/client.go pkg/connector/handle_message.go pkg/connector/*.go
# Narrow search for album preview execution details and context handling.
rg -n -A20 -B10 'DownloadAlbumPreview|album preview|album-preview|preview' pkg/connector pkg/lineRepository: beeper/line
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the client constructor and OBS HTTP client selection.
ast-grep outline pkg/line/client.go --view expanded | sed -n '1,260p'
echo '--- constructor/client fields ---'
rg -n -A30 -B10 'func NewClient|type Client struct|OBSClient|HTTPClient|rpcClientTimeout|obsHTTPClient\(\)' pkg/line/client.go
echo '--- album-preview flow ---'
rg -n -A40 -B10 'album preview|album-preview|DownloadAlbumPreview|ConvertPostNotification|worker|pool|concurr|errOBSEncodingIncomplete|ErrOBSObjectNotFound' pkg/connector pkg/lineRepository: beeper/line
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' pkg/line/client.go | nl -ba | sed -n '1,220p'
echo '--- later client.go region ---'
sed -n '220,420p' pkg/line/client.go | nl -ba | sed -n '220,420p'Repository: beeper/line
Length of output: 189
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the post-notification conversion path and any album-preview concurrency.
rg -n -A40 -B20 'ConvertPostNotification|album preview|album-preview|DownloadAlbumPreview|errgroup|WaitGroup|worker pool|concurrently|TestConvertPostNotificationProcessesAlbumPreviewsConcurrentlyInOrder' pkg/connector pkg/lineRepository: beeper/line
Length of output: 50367
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A25 -B10 'func \(lc \*LineClient\) newClient|func newLineAPIClient|return &line.Client|OBSClient|HTTPClient: .*Timeout|NewClient\(' pkg/connector/client.go pkg/connector/connector.go pkg/connector/handlers/post_notification.go pkg/connector/handle_message.goRepository: beeper/line
Length of output: 9280
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -A20 -B10 'func newLineAPIClient|newLineAPIClient\(' pkg/connector pkg/lineRepository: beeper/line
Length of output: 13529
Add a timeout to the OBS client used for album previews. The album-preview path runs 4 concurrent workers, and newLineAPIClient() builds OBSClient with no timeout, so a stalled OBS download can block a worker indefinitely when the caller context has no deadline. Set a timeout on OBSClient in NewClient or bound the context before calling DownloadAlbumPreview.
🤖 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.go` around lines 726 - 777, Add a finite timeout to the OBS
client used by DownloadAlbumPreview so stalled requests cannot block workers
indefinitely when the caller context has no deadline. Update the OBSClient
initialization in NewClient, or derive a bounded context before the OBS request,
while preserving caller cancellation and existing download behavior.
| if parsedTime, parseErr := time.Parse(time.RFC3339, stringValue); parseErr == nil { | ||
| return parsedTime.Add(-obsTokenBuffer) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
RFC3339 branch skips the near-expiry fallback check applied elsewhere.
The numeric paths (lines 792-804) reject/replace an expiresAt that is too close to now (expiresAt.Before(now.Add(obsTokenBuffer)) → fallback), but the RFC3339 string branch returns parsedTime.Add(-obsTokenBuffer) unconditionally. If the server ever returns an RFC3339 expiration at or near "now", the cached token would be treated as immediately (or nearly) expired, silently defeating the cache for that response shape instead of falling back to defaultChannelTokenLifetime like the numeric path does.
🛠️ Proposed fix to align RFC3339 handling with the numeric path
if parsedTime, parseErr := time.Parse(time.RFC3339, stringValue); parseErr == nil {
- return parsedTime.Add(-obsTokenBuffer)
+ expiresAt := parsedTime.Add(-obsTokenBuffer)
+ if expiresAt.Before(now.Add(obsTokenBuffer)) {
+ return fallback
+ }
+ return expiresAt
}📝 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.
| if parsedTime, parseErr := time.Parse(time.RFC3339, stringValue); parseErr == nil { | |
| return parsedTime.Add(-obsTokenBuffer) | |
| } | |
| if parsedTime, parseErr := time.Parse(time.RFC3339, stringValue); parseErr == nil { | |
| expiresAt := parsedTime.Add(-obsTokenBuffer) | |
| if expiresAt.Before(now.Add(obsTokenBuffer)) { | |
| return fallback | |
| } | |
| return expiresAt | |
| } |
🤖 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/methods.go` around lines 781 - 783, Update the RFC3339 parsing
branch in the expiration-time method to apply the same near-expiry validation as
the numeric paths: compare parsedTime against now.Add(obsTokenBuffer), and
return the existing fallback when it is too early; otherwise preserve the parsed
expiration adjusted by obsTokenBuffer.
1 participant
Summary
Testing
go test -short -vet=off ./... -count=1go vet ./pkg/line ./pkg/connector/...staticcheckacross the repository, excluding generatedpkg/ltsm