Repository navigation
feat(sender): honour link_preview_options on the send path - #41
Conversation
A link preview was a transport-wide constant, not a per-message decision. build_send_message/1 set disable_web_page_preview: true unconditionally (delivery/core.ex) and the sender's send/reply path built its attrs from a fixed key set with no preview key (objects/sender.ex), so NO message this transport sent could carry a preview and nothing a caller put in the message map changed it. The asymmetry that makes this look unintended: EDITS already accept the option — build_edit_message_text/1 does maybe_put(:link_preview_options, …) and the sender passes msg["link_preview_options"] straight through on the edit path. A message could therefore be edited into having a preview it could never be sent with. The default is unchanged. With no option the payload still carries disable_web_page_preview: true — the right default for an agent that quotes a URL mid-thread, and the wrong one for an operator broadcast; the point is that those two can now differ per message. An explicit link_preview_options wins and the deprecated flag is then omitted (Bot API 7.0 replaced disable_web_page_preview with link_preview_options, so sending both would be contradictory). A non-object option raises ArgumentError via the existing optional_map/2 rather than being silently dropped, and the option is read by atom or string key through option/2 — the same idiom the edit builder uses. Applied to every chunk of a chunked message: chunking is the transport's own implementation detail, not something the caller asked for, and Telegram previews at most the first link of each message it actually sends, so a chunk with no link is unaffected. Motivation: an operator broadcast carrying a link to an X post reached its groups with no preview card — the link was live, but the post it pointed at was invisible until someone clicked. The only host-side workaround is to route those targets to a different sender object, i.e. to fork the egress path and re-implement the throttle, chunking, delivery-effects audit, dead-recipient classification and retry/backoff this package already gets right, for one boolean. Validated: 7 new tests in test/sender_link_preview_test.exs cover the unchanged default, an explicit option (on and off), the extra option fields, string-key reading, the raise on a non-object, and both sender send-path halves. Full suite 242/242 green; no new compiler warnings (3 before, 3 after).
|
Warning Review limit reachedNext included review available in 38 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (4)
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 |
The new test's Effects module implemented only the configured /2 form, so compiling the test suite warned 'function before_send/1 required by behaviour Genswarms.Telegram.DeliveryEffects is not implemented'. Same shape the other sender tests use. No behaviour change; 242/242 still green.
|
Self-review follow-up, pushed as c4c9d67: Correction to the PR body. I claimed "no new compiler warnings (3 before, 3 after)" — that count was from The test's No change to |
2 participants
The problem
A link preview is a transport-wide constant, not a per-message decision.
build_send_message/1setsdisable_web_page_preview: trueunconditionally (delivery/core.ex:15), and the sender's send/reply path builds its attrs from a fixed key set with no preview key (objects/sender.ex:1350-1365). So no message this transport sends can carry a preview, and nothing a caller puts in the message map changes it.The asymmetry that makes this look unintended: edits already accept the option.
build_edit_message_text/1doesmaybe_put(:link_preview_options, option(attrs, :link_preview_options))(delivery/edits.ex:25) and the sender passesmsg["link_preview_options"]straight through on the edit path (sender.ex:1516). A message can be edited into having a preview it could never be sent with.The change
Two small edits, mirroring what the edit builder already does:
build_send_message/1readslink_preview_optionsfrom attrs via the existingoption/2+optional_map/2helpers.do_send_text/5passesmsg["link_preview_options"]into attrs.The default is unchanged. With no option the payload still carries
disable_web_page_preview: true— the right default for an agent quoting a URL mid-thread, and the wrong one for an operator broadcast. The point is that those two can now differ per message.Details worth a reviewer's eye:
disable_web_page_previewwithlink_preview_options, so sending both would be contradictory.ArgumentErrorthrough the existingoptional_map/2rather than being silently dropped.option/2— same idiom as the edit builder.reply_markupalready hasidx == lastprecedent for chunk-position logic.)Sender.build_send_body/5(sender.ex:3886) still hardcodes the flag. It is not on the send path — nothing in the package calls it — so I left it alone rather than widen the diff. Say the word and I'll include it.Motivation
An operator broadcast carrying a link to an X post reached its groups with no preview card. The link was live, but the post it pointed at was invisible until someone clicked.
The only host-side workaround is to route those targets to a different sender object — i.e. fork the egress path and re-implement the throttle, chunking, delivery-effects audit, dead-recipient classification and retry/backoff this package already gets right, all for one boolean.
Validation
test/sender_link_preview_test.exs: the unchanged default, an explicit option (on and off), the extra option fields (url,prefer_large_media), string-key reading, the raise on a non-object, and both halves of the sender send path.