fix(bot): update embeds after flag edits#94
Conversation
|
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
embedly-docs | fea3c67 | Commit Preview URL Branch Preview URL |
Jul 16 2026, 02:45 AM |
Greptile SummaryThis PR adds in-place Embedly message updates when a user edits link flag modifiers (
Confidence Score: 3/5The update path is safe for the common case but misbehaves after a URL reorder, corrupting bot-message assignments on subsequent flag edits. The cache stores requestIndex values anchored to message-creation time. After the user reorders URLs (correctly skipped because sameUrls is false), those indices are permanently misaligned with the new URL order. A later flag-only edit on the reordered content passes the sameUrls check against the most-recent content but queries the cache with stale indices, causing the wrong bot message to receive the wrong URL's embed. apps/bot/src/listeners/messageUpdate.ts — the requestIndex alignment assumption after URL reorders needs a closer look before merging. Important Files Changed
Prompt To Fix All With AIFix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
apps/bot/src/listeners/messageUpdate.ts:38-53
**Stale `requestIndex` after URL reorder corrupts subsequent flag updates**
`requestIndex` values in the cache are assigned at message-creation time and never updated. If a user reorders URLs in a prior edit, `sameUrls` correctly returns `false` and skips that edit — but the cache entries still hold the original creation-time positions. On the *next* edit (where only a modifier changes and `sameUrls` is `true` relative to the reordered content), those stale indices are used to select bot messages, mapping the wrong bot message to the wrong URL.
Concretely: create message "url1 url2" → two bot messages (botMsg1↔index 0, botMsg2↔index 1) → user reorders to "url2 url1" (skipped, cache unchanged) → user adds `@` to url2 giving "@url2 url1" → `sameUrls` is `true` vs. the reordered content → `requestIndex=0` points to botMsg1 (originally url1) but `newUrls[0]` is url2, so botMsg1 gets overwritten with url2's embed while botMsg2 still shows url2 from creation. url1's embed disappears.
### Issue 2 of 3
apps/bot/src/listeners/messageUpdate.ts:6-13
`hasSameOptions` uses `===` on optional `force?: boolean`, so `undefined` and `false` are treated as different values. Since `parseMessageURLs` always writes `force = false` (never `undefined`), this won't trigger today, but it's a latent trap if another caller ever omits `force`. Normalizing with `??` makes the comparison explicit and safe.
```suggestion
function hasSameOptions(left: EmbedURLRequest, right: EmbedURLRequest) {
return (
(left.force ?? false) === (right.force ?? false) &&
left.flags?.MediaOnly === right.flags?.MediaOnly &&
left.flags?.SourceOnly === right.flags?.SourceOnly &&
left.flags?.Spoiler === right.flags?.Spoiler
);
}
```
### Issue 3 of 3
apps/bot/src/lib/handleUrls.ts:154-164
**Unnecessary `matchURL` calls for non-target URLs**
When called with `updateTargets`, `handleUrls` still invokes `matchURL` for every URL in the full `urls` array and then filters down to only the update targets. For a message with N URLs where only one flag changed, N−1 unnecessary `matchURL` evaluations happen in parallel. Consider pre-filtering `urls` to only entries whose index is in `updateTargets` before the `Promise.all`.
Reviews (1): Last reviewed commit: "fix(bot): update embeds after flag edits" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (sameUrls) { | ||
| const updateTargets = new Map<number, string>(); | ||
|
|
||
| for (const { id, requestIndex } of botMessages) { | ||
| if (requestIndex === undefined) continue; | ||
| const oldRequest = oldUrls[requestIndex]; | ||
| const newRequest = newUrls[requestIndex]; | ||
| if (!oldRequest || !newRequest) continue; | ||
| if (hasSameOptions(oldRequest, newRequest)) continue; | ||
| updateTargets.set(requestIndex, id); | ||
| } | ||
|
|
||
| if (updateTargets.size > 0) { | ||
| await handleUrls(newUrls, message, { updateTargets }); | ||
| } | ||
| } |
There was a problem hiding this comment.
requestIndex after URL reorder corrupts subsequent flag updates
requestIndex values in the cache are assigned at message-creation time and never updated. If a user reorders URLs in a prior edit, sameUrls correctly returns false and skips that edit — but the cache entries still hold the original creation-time positions. On the next edit (where only a modifier changes and sameUrls is true relative to the reordered content), those stale indices are used to select bot messages, mapping the wrong bot message to the wrong URL.
Concretely: create message "url1 url2" → two bot messages (botMsg1↔index 0, botMsg2↔index 1) → user reorders to "url2 url1" (skipped, cache unchanged) → user adds @ to url2 giving "@url2 url1" → sameUrls is true vs. the reordered content → requestIndex=0 points to botMsg1 (originally url1) but newUrls[0] is url2, so botMsg1 gets overwritten with url2's embed while botMsg2 still shows url2 from creation. url1's embed disappears.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/bot/src/listeners/messageUpdate.ts
Line: 38-53
Comment:
**Stale `requestIndex` after URL reorder corrupts subsequent flag updates**
`requestIndex` values in the cache are assigned at message-creation time and never updated. If a user reorders URLs in a prior edit, `sameUrls` correctly returns `false` and skips that edit — but the cache entries still hold the original creation-time positions. On the *next* edit (where only a modifier changes and `sameUrls` is `true` relative to the reordered content), those stale indices are used to select bot messages, mapping the wrong bot message to the wrong URL.
Concretely: create message "url1 url2" → two bot messages (botMsg1↔index 0, botMsg2↔index 1) → user reorders to "url2 url1" (skipped, cache unchanged) → user adds `@` to url2 giving "@url2 url1" → `sameUrls` is `true` vs. the reordered content → `requestIndex=0` points to botMsg1 (originally url1) but `newUrls[0]` is url2, so botMsg1 gets overwritten with url2's embed while botMsg2 still shows url2 from creation. url1's embed disappears.
How can I resolve this? If you propose a fix, please make it concise.
| function hasSameOptions(left: EmbedURLRequest, right: EmbedURLRequest) { | ||
| return ( | ||
| left.force === right.force && | ||
| left.flags?.MediaOnly === right.flags?.MediaOnly && | ||
| left.flags?.SourceOnly === right.flags?.SourceOnly && | ||
| left.flags?.Spoiler === right.flags?.Spoiler | ||
| ); | ||
| } |
There was a problem hiding this comment.
hasSameOptions uses === on optional force?: boolean, so undefined and false are treated as different values. Since parseMessageURLs always writes force = false (never undefined), this won't trigger today, but it's a latent trap if another caller ever omits force. Normalizing with ?? makes the comparison explicit and safe.
| function hasSameOptions(left: EmbedURLRequest, right: EmbedURLRequest) { | |
| return ( | |
| left.force === right.force && | |
| left.flags?.MediaOnly === right.flags?.MediaOnly && | |
| left.flags?.SourceOnly === right.flags?.SourceOnly && | |
| left.flags?.Spoiler === right.flags?.Spoiler | |
| ); | |
| } | |
| function hasSameOptions(left: EmbedURLRequest, right: EmbedURLRequest) { | |
| return ( | |
| (left.force ?? false) === (right.force ?? false) && | |
| left.flags?.MediaOnly === right.flags?.MediaOnly && | |
| left.flags?.SourceOnly === right.flags?.SourceOnly && | |
| left.flags?.Spoiler === right.flags?.Spoiler | |
| ); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/bot/src/listeners/messageUpdate.ts
Line: 6-13
Comment:
`hasSameOptions` uses `===` on optional `force?: boolean`, so `undefined` and `false` are treated as different values. Since `parseMessageURLs` always writes `force = false` (never `undefined`), this won't trigger today, but it's a latent trap if another caller ever omits `force`. Normalizing with `??` makes the comparison explicit and safe.
```suggestion
function hasSameOptions(left: EmbedURLRequest, right: EmbedURLRequest) {
return (
(left.force ?? false) === (right.force ?? false) &&
left.flags?.MediaOnly === right.flags?.MediaOnly &&
left.flags?.SourceOnly === right.flags?.SourceOnly &&
left.flags?.Spoiler === right.flags?.Spoiler
);
}
```
How can I resolve this? If you propose a fix, please make it concise.| const matches = ( | ||
| await Promise.all( | ||
| urls.map(async (request) => { | ||
| urls.map(async (request, requestIndex) => { | ||
| const match = await matchURL(request.url); | ||
| return match ? { ...request, ...match } : null; | ||
| return match ? { ...request, ...match, requestIndex } : null; | ||
| }), | ||
| ) | ||
| ).filter((m) => m !== null); | ||
| ).filter( | ||
| (match) => | ||
| match !== null && (!options.updateTargets || options.updateTargets.has(match.requestIndex)), | ||
| ); |
There was a problem hiding this comment.
matchURL calls for non-target URLs
When called with updateTargets, handleUrls still invokes matchURL for every URL in the full urls array and then filters down to only the update targets. For a message with N URLs where only one flag changed, N−1 unnecessary matchURL evaluations happen in parallel. Consider pre-filtering urls to only entries whose index is in updateTargets before the Promise.all.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/bot/src/lib/handleUrls.ts
Line: 154-164
Comment:
**Unnecessary `matchURL` calls for non-target URLs**
When called with `updateTargets`, `handleUrls` still invokes `matchURL` for every URL in the full `urls` array and then filters down to only the update targets. For a message with N URLs where only one flag changed, N−1 unnecessary `matchURL` evaluations happen in parallel. Consider pre-filtering `urls` to only entries whose index is in `updateTargets` before the `Promise.all`.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Summary
Behavior
Edits update in place only when URL values and order stay unchanged. URL additions, removals, replacements, and reordering are ignored. Failed refreshes keep the existing Embedly message and react to the source message with ❌.
Validation
apps/bot/node_modules/.bin/oxlint apps/bot/srcapps/bot/node_modules/.bin/oxfmt --checkon changed bot filestsdownbot build@,!,?@,?!, and spoiler modifiersgit diff --checkRaw bot
tscremains noisy from pre-existing stale workspace declaration outputs.