feat: 🎸 Implement message prefetching and link previews#22
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds message prefetching and link preview functionality to the LINE messenger bridge. The implementation allows the bridge to fetch recent messages from all active chats on startup, and generates link previews for URLs found in incoming messages.
Changes:
- Added data structures for message boxes, page info, and link previews
- Implemented API methods to fetch message boxes and recent messages
- Added message prefetching on connection to populate chat history
- Integrated link preview generation for URLs in messages
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/line/structs.go | Added new struct definitions for MessageBoxes, PageInfo, and related response wrappers |
| pkg/line/methods.go | Implemented GetMessageBoxes and GetRecentMessagesV2 RPC methods |
| pkg/line/client.go | Added GetPageInfo method to fetch link preview metadata from LINE API |
| pkg/connector/client.go | Implemented prefetchMessages goroutine, link preview extraction, and improved timestamp handling |
| README.md | Updated feature list to mark prefetch chats and link previews as completed |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| LinkPreview: event.LinkPreview{ | ||
| Title: info.Title, | ||
| Description: info.Summary, | ||
| CanonicalURL: info.Domain, |
There was a problem hiding this comment.
The CanonicalURL field is being set to info.Domain instead of info.URL. According to the MSC2481 spec for link previews, CanonicalURL should contain the canonical URL of the page, not just the domain. This should likely be set to info.URL.
| CanonicalURL: info.Domain, | |
| CanonicalURL: info.URL, |
| tsInt, _ := msg.CreatedTime.Int64() | ||
| ts := time.UnixMilli(tsInt) | ||
| if ts.IsZero() { | ||
| ts = time.Now() |
There was a problem hiding this comment.
The error from msg.CreatedTime.Int64() is silently ignored. If the conversion fails, tsInt will be 0, leading to an incorrect timestamp (Unix epoch). Consider checking the error and logging a warning or handling it appropriately.
| tsInt, _ := msg.CreatedTime.Int64() | |
| ts := time.UnixMilli(tsInt) | |
| if ts.IsZero() { | |
| ts = time.Now() | |
| var ts time.Time | |
| if tsInt, err := msg.CreatedTime.Int64(); err != nil { | |
| lc.UserLogin.Bridge.Log.Warn(). | |
| Err(err). | |
| Str("msg_id", msg.ID). | |
| Msg("Failed to convert message CreatedTime to int64, using current time") | |
| ts = time.Now() | |
| } else { | |
| ts = time.UnixMilli(tsInt) | |
| if ts.IsZero() { | |
| ts = time.Now() | |
| } |
| existing, err := lc.UserLogin.Bridge.DB.Message.GetPartByID(ctx, lc.UserLogin.ID, networkid.MessageID(msg.ID), "") | ||
| if err == nil && existing != nil { | ||
| continue |
There was a problem hiding this comment.
During prefetch, a database query is executed for every message (up to 5000 messages) to check if it already exists. This could create a performance bottleneck, especially with a large number of messages. Consider batching the existence checks or using a bloom filter to reduce database queries.
| if !strings.HasPrefix(match, "http") { | ||
| requestURL = "https://" + match | ||
| } | ||
| client := line.NewClient(lc.AccessToken) |
There was a problem hiding this comment.
A new LINE client is created for every message that contains a URL for fetching link preview information. This creates unnecessary overhead. Consider reusing the client instance that was already created in the queueIncomingMessage function scope (available as lc.AccessToken via line.NewClient).
| client := line.NewClient(lc.AccessToken) |
| if strings.Contains(unwrappedText, ".") { | ||
| urlRegex := regexp.MustCompile(`(https?://)?([a-zA-Z0-9.-]+\.[a-zA-Z]{2,})(/[^\s]*)?`) | ||
| if match := urlRegex.FindString(unwrappedText); match != "" { | ||
| match = strings.TrimRight(match, ".,;:!?") | ||
| requestURL := match | ||
| if !strings.HasPrefix(match, "http") { | ||
| requestURL = "https://" + match | ||
| } | ||
| client := line.NewClient(lc.AccessToken) | ||
| if info, err := client.GetPageInfo(requestURL); err == nil { | ||
| preview := &event.BeeperLinkPreview{ | ||
| MatchedURL: match, | ||
| LinkPreview: event.LinkPreview{ | ||
| Title: info.Title, | ||
| Description: info.Summary, | ||
| CanonicalURL: info.Domain, | ||
| }, | ||
| } | ||
| if info.Image != "" && info.Obs.CDN != "" { | ||
| preview.ImageURL = id.ContentURIString(info.Obs.CDN + info.Image) | ||
| } | ||
| content.BeeperLinkPreviews = []*event.BeeperLinkPreview{preview} | ||
| } |
There was a problem hiding this comment.
The URL check using strings.Contains(unwrappedText, ".") is too broad and will trigger regex matching for any text containing a period, including sentences, decimal numbers, or file extensions. Consider a more specific check or move this logic after a successful regex match to avoid unnecessary processing.
| if strings.Contains(unwrappedText, ".") { | |
| urlRegex := regexp.MustCompile(`(https?://)?([a-zA-Z0-9.-]+\.[a-zA-Z]{2,})(/[^\s]*)?`) | |
| if match := urlRegex.FindString(unwrappedText); match != "" { | |
| match = strings.TrimRight(match, ".,;:!?") | |
| requestURL := match | |
| if !strings.HasPrefix(match, "http") { | |
| requestURL = "https://" + match | |
| } | |
| client := line.NewClient(lc.AccessToken) | |
| if info, err := client.GetPageInfo(requestURL); err == nil { | |
| preview := &event.BeeperLinkPreview{ | |
| MatchedURL: match, | |
| LinkPreview: event.LinkPreview{ | |
| Title: info.Title, | |
| Description: info.Summary, | |
| CanonicalURL: info.Domain, | |
| }, | |
| } | |
| if info.Image != "" && info.Obs.CDN != "" { | |
| preview.ImageURL = id.ContentURIString(info.Obs.CDN + info.Image) | |
| } | |
| content.BeeperLinkPreviews = []*event.BeeperLinkPreview{preview} | |
| } | |
| urlRegex := regexp.MustCompile(`(https?://)?([a-zA-Z0-9.-]+\.[a-zA-Z]{2,})(/[^\s]*)?`) | |
| if match := urlRegex.FindString(unwrappedText); match != "" { | |
| match = strings.TrimRight(match, ".,;:!?") | |
| requestURL := match | |
| if !strings.HasPrefix(match, "http") { | |
| requestURL = "https://" + match | |
| } | |
| client := line.NewClient(lc.AccessToken) | |
| if info, err := client.GetPageInfo(requestURL); err == nil { | |
| preview := &event.BeeperLinkPreview{ | |
| MatchedURL: match, | |
| LinkPreview: event.LinkPreview{ | |
| Title: info.Title, | |
| Description: info.Summary, | |
| CanonicalURL: info.Domain, | |
| }, | |
| } | |
| if info.Image != "" && info.Obs.CDN != "" { | |
| preview.ImageURL = id.ContentURIString(info.Obs.CDN + info.Image) | |
| } | |
| content.BeeperLinkPreviews = []*event.BeeperLinkPreview{preview} |
| ts := time.UnixMilli(tsInt) | ||
| if ts.IsZero() { | ||
| ts = time.Now() |
There was a problem hiding this comment.
The timestamp check ts.IsZero() will not work as expected for Unix timestamps. A Unix timestamp of 0 (January 1, 1970) is a valid timestamp and will not be zero according to time.Time.IsZero(). Consider checking if tsInt == 0 before creating the time object, or check if tsInt < 0 to catch invalid values.
| ts := time.UnixMilli(tsInt) | |
| if ts.IsZero() { | |
| ts = time.Now() | |
| var ts time.Time | |
| if tsInt <= 0 { | |
| ts = time.Now() | |
| } else { | |
| ts = time.UnixMilli(tsInt) |
| } | ||
|
|
||
| if strings.Contains(unwrappedText, ".") { | ||
| urlRegex := regexp.MustCompile(`(https?://)?([a-zA-Z0-9.-]+\.[a-zA-Z]{2,})(/[^\s]*)?`) |
There was a problem hiding this comment.
The URL regex pattern (https?://)?([a-zA-Z0-9.-]+\.[a-zA-Z]{2,})(/[^\s]*)? does not account for URLs with authentication credentials, ports, or certain special characters in paths (like parentheses, which are common in URLs). This could miss valid URLs or incorrectly match URL-like text. Consider using a more comprehensive URL matching pattern or a URL parsing library.
| } | ||
|
|
||
| q := req.URL.Query() | ||
| q.Add("url", url) |
There was a problem hiding this comment.
The URL parameter is passed directly to the LINE API without validation or sanitization. Consider validating that the URL is properly formatted and not empty before making the API call to avoid unnecessary requests with invalid URLs.
| } | ||
|
|
||
| if strings.Contains(unwrappedText, ".") { | ||
| urlRegex := regexp.MustCompile(`(https?://)?([a-zA-Z0-9.-]+\.[a-zA-Z]{2,})(/[^\s]*)?`) |
There was a problem hiding this comment.
The regex pattern is compiled on every message conversion. This regex should be compiled once and stored as a package-level variable to avoid repeated compilation overhead, especially during message prefetching where many messages could be processed.
| requestURL := match | ||
| if !strings.HasPrefix(match, "http") { | ||
| requestURL = "https://" + match | ||
| } |
There was a problem hiding this comment.
The GetPageInfo API call is made synchronously during message processing. This could significantly slow down message handling, especially during the prefetch phase where 50 messages per chat are processed. If a URL takes a long time to respond or times out, it will block message processing. Consider adding a timeout to the HTTP client or making link preview fetching asynchronous.
| } | |
| } | |
| // Ensure a bounded timeout for any HTTP requests made by GetPageInfo. | |
| if http.DefaultClient != nil { | |
| clientCopy := *http.DefaultClient | |
| clientCopy.Timeout = 5 * time.Second | |
| http.DefaultClient = &clientCopy | |
| } |
Only create 1 new client when queuing incoming messages Added replies as fully supported and read receipts as true
No description provided.