lure: ensure metadata gaps are filled when generating links#5841
lure: ensure metadata gaps are filled when generating links#5841arthyn wants to merge 3 commits into
Conversation
Two related fixes to lure open-graph metadata. %reel-describe now backfills missing fields from server state when the caller's describe is sparse: title/description/image scried from %groups, and nickname/avatar from our cached profile. Caller-provided values always win, and scry failures are tolerated via mole. This lets pioneer create lures with a bare describe and still get a fully populated preview, instead of relying on later subscription updates. The "a Groupchat" fallback in the og-title formula was unconditional in the no-nickname branch, ignoring a real group title when one was present. It now uses group-title (which itself still falls back to "a Groupchat" when the title is empty), so a titled group renders "You're Invited to <Title>" instead of "You're Invited to a Groupchat". Tests gain a scry mock returning an empty group:v9:gv for the gap-fill path, and the test-groups-update expectation is updated to reflect the corrected fallback behavior. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
Two call sites in the /contacts and /groups subscription handlers were computing the same group-invite open-graph title. Extracted the formula (including the "a Groupchat" fallback) into a single helper in the upper |% core. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Fang-
left a comment
There was a problem hiding this comment.
Some safety concerns for the stuff you tried making safe. Looks good otherwise!
| %- mole |. | ||
| .^ group:v9:groups-ver %gx | ||
| /(scot %p our.bowl)/groups/(scot %da now.bowl)/v2/groups/(scot %p p.u.parsed)/[q.u.parsed]/group-2 | ||
| == |
There was a problem hiding this comment.
Did you test the failure case for these? Pretty sure you still can't catch/virtualize .^ crashes in userspace, so this will blow up. If you want to harden, you must use (maybe even implement) the relevant %u scry endpoints in groups agent instead.
(Good one for the gall-2026 nice-to-haves list, gall auto-filling the %u response based on whether the %x would succeed.)
| =? fld | ||
| ?& ?=(^ nickname) | ||
| !=('' u.nickname) | ||
| =(~ (~(get by fld) %'inviterNickname')) |
There was a problem hiding this comment.
| =(~ (~(get by fld) %'inviterNickname')) | |
| !(~(has by fld) %'inviterNickname') |
Here and elsewhere. Alternatively, maybe you want to check these fields for the '' empty string value too, in case that snuck in?
mikolajpp
left a comment
There was a problem hiding this comment.
The changes look good, the logic is sound.
A few coding nits, but that's about it!
| =/ gt=@t | ||
| ?: =('' group-title) 'a Groupchat' | ||
| group-title | ||
| %- crip | ||
| ?: |(?=(~ nickname) =('' u.nickname)) | ||
| "Tlon Messenger: You're Invited to {(trip gt)}" | ||
| "Tlon Messenger: {(trip u.nickname)} invited you to {(trip gt)}" |
There was a problem hiding this comment.
| =/ gt=@t | |
| ?: =('' group-title) 'a Groupchat' | |
| group-title | |
| %- crip | |
| ?: |(?=(~ nickname) =('' u.nickname)) | |
| "Tlon Messenger: You're Invited to {(trip gt)}" | |
| "Tlon Messenger: {(trip u.nickname)} invited you to {(trip gt)}" | |
| =/ title=@t | |
| ?: =('' group-title) 'a Groupchat' | |
| group-title | |
| %- crip | |
| ?: |(?=(~ nickname) =('' u.nickname)) | |
| "Tlon Messenger: You're Invited to {(trip title)}" | |
| "Tlon Messenger: {(trip u.nickname)} invited you to {(trip title)}" |
| :: gap-fill open-graph metadata from server state when the caller | ||
| :: didn't provide it: title / description / image from %groups, | ||
| :: and nickname / avatar from our cached profile. Caller-provided | ||
| :: values always win. | ||
| :: |
There was a problem hiding this comment.
| :: gap-fill open-graph metadata from server state when the caller | |
| :: didn't provide it: title / description / image from %groups, | |
| :: and nickname / avatar from our cached profile. Caller-provided | |
| :: values always win. | |
| :: | |
| :: gap-fill open-graph metadata from server state when the caller | |
| :: didn't provide it: title / description / image from %groups, | |
| :: and nickname / avatar from our cached profile. caller-provided | |
| :: values take priority. | |
| :: |
| :: | ||
| =/ type=(unit @t) (~(get by fields.metadata) %'inviteType') | ||
| =? fields.metadata |(?=(~ type) =('group' u.type)) | ||
| =/ fld fields.metadata |
There was a problem hiding this comment.
This does not feel idiomatic, one would just an alias here
| =/ fld fields.metadata | |
| =* fields fields.metadata |
| =/ type=(unit @t) (~(get by fields.metadata) %'inviteType') | ||
| =? fields.metadata |(?=(~ type) =('group' u.type)) | ||
| =/ fld fields.metadata | ||
| :: treat absent or empty-string fields as gap-fillable. |
There was a problem hiding this comment.
| :: treat absent or empty-string fields as gap-fillable. | |
| :: treat absent or empty-string fields as gap-fillable |
| |= [m=(map cord cord) k=cord] | ||
| ^- ? | ||
| =/ v (~(get by m) k) | ||
| ?|(?=(~ v) =('' u.v)) |
There was a problem hiding this comment.
This should be called is-blank for clarity. We should also tighten the arguments. The key of this map is not just any cord, but the specific set of keys from reel metadata.
Not a big fan of single letter variable names. This used to be reserved for standard library functions. CC @Fang- for second opinion.
There was a problem hiding this comment.
Eh, it's a super narrow function with straightforward logic, ultra-lapidary is perfectly fine here imo. (I agree is-blank is slightly clearer.)
We always pass fld as the map though. Especially in this context, might be nicer to use that directly instead of passing it as an argument.
(Maybe that gets you into trouble if you make edits and then later check the value again? But we don't do that. =* blank wouldn't suffer from it either way.)
| =/ parsed=(unit flag:v0:groups-ver) (rush id flag) | ||
| ?~ parsed fld |
There was a problem hiding this comment.
| =/ parsed=(unit flag:v0:groups-ver) (rush id flag) | |
| ?~ parsed fld | |
| ?~ parsed=(rush id flag) fld |
| .^ group:v9:groups-ver %gx | ||
| (weld base /v2/groups/(scot %p p.u.parsed)/[q.u.parsed]/group-2) | ||
| == | ||
| =* gmeta meta.grp |
There was a problem hiding this comment.
| =* gmeta meta.grp | |
| =* meta meta.grp |
| :: scry mock for the gap-fill path in %reel-describe. the %gu check | ||
| :: reports the group as present so the %gx fetch proceeds; the %gx | ||
| :: returns an empty group so the agent's gap-fill checks | ||
| :: (!=('' title.gmeta) etc.) all fall through and no fields are added. | ||
| :: |
There was a problem hiding this comment.
I think these scry paths are generally usable, it is not just for mocking, though we only use it for that now. I would remove this comment and think about specializing the group scry with a special case for a specific group that would indeed have an empty title.
Fang-
left a comment
There was a problem hiding this comment.
Changes look good, thank you. Concur with @mikolajpp's comments.
Summary
%reel had a bug whereby empty nicknames wouldn't fill with any existing group title, minor but still good to fix. The other piece here is making sure we always fill in any gaps in the metadata. %reel has drifted into being more heavily integrated into groups/contacts and this just furthers that by making sure any kind of reel describe calls actually fill in gaps in the metadata from what we have on the backend. This was present in https://github.com/tloncorp/ylem/blob/8af770c82083f1031c84b72a877a7b8ad226c56f/pkg/runtime/pioneer/src/Pioneer/Command/Click.hs#L1537 where we're only giving
userId,type, andgroupId.Fixes TLON-5790
Changes
%reel-describegap-filling (desk/app/reel.hoon): when a caller's describe is sparse, backfill missing open-graph fields from server state —invitedGroupTitle/invitedGroupDescription/invitedGroupIconImageUrlscried from%groups, andinviterNickname/inviterAvatarImageread from reel's cachedour-profile. Caller-provided values always win; the group scry is wrapped inmoleso a missing group doesn't crash the poke.group-title(which itself still falls back to "a Groupchat" when the title is empty).group-og-titlehelper: extracted the formula shared by the/contactsand/groupssubscription handlers into a single helper in the upper|%core. No behavior change.desk/tests/app/reel.hoon): added a scry mock returning an emptygroup:v9:gvfor the gap-fill path, wired it into the three tests that exercise%reel-describe(test-reel-describe,test-groups-update,test-contacts-update). Updated thetest-groups-update$og_titleexpectation to reflect the corrected fallback ("You're Invited to Early Sunrise"instead of"…a Groupchat").How did I test?
Ran
./backend/run-tests.shlocally against a moon— all three reel tests pass (test-reel-describe,test-groups-update,test-contacts-update). No e2e or client changesRisks and impact
%reelagent)Rollback plan
Revert the merge commit. The agent state-version is unchanged, so no migration concerns. Any lures registered while this is live keep their backfilled metadata fields, which is forward-compatible with the prior code path (extra fields are just ignored downstream if reverted).
Screenshots / videos
N/A — backend / Hoon only. The user-visible effect is open-graph previews on shared invite links: titles like "Tlon Messenger: You're Invited to <Group Title>" instead of "…to a Groupchat" when the inviter has no nickname yet.