Render custom channel emoji in comments and descriptions - #5929
Render custom channel emoji in comments and descriptions#5929DeepanshuSagore wants to merge 1 commit into
Conversation
Custom channel emoji are only carried in the attachment runs. The text content just holds the ":shortcode:" placeholder, which was rendered as-is because parse_description only handled commandRuns. Parse attachmentRuns alongside commandRuns, walking both in positional order since they index into the same string, and substitute the image. Standard unicode emoji are sent as attachment runs too, but their text content is already the emoji character and renders fine, so those are left as text rather than proxying an image for every one of them.
📝 WalkthroughWalkthrough
ChangesDescription custom emoji
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/invidious/videos/description.cr (1)
92-137: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftRender overlapping
commandRunsandattachmentRunstogether.Line 115 discards the later run when two runs overlap. A command run can consume a custom emoji attachment and leave its
:shortcode:text in the output. An attachment run can also consume a command run and remove its link.Split overlapping ranges, or render the attachment inside the command output. Add a regression spec with an overlapping command run and attachment run.
🤖 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 `@src/invidious/videos/description.cr` around lines 92 - 137, Update the run-processing logic around the sorted `runs` collection and `runs.each` so overlapping command and attachment ranges are both rendered instead of discarding the later run via `next if run_start < index`; preserve correct positional text consumption while allowing attachments inside command output or splitting overlapping ranges. Add a regression spec covering both overlap directions, including an attachment consumed by a command and a command consumed by an attachment.
🤖 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.
Outside diff comments:
In `@src/invidious/videos/description.cr`:
- Around line 92-137: Update the run-processing logic around the sorted `runs`
collection and `runs.each` so overlapping command and attachment ranges are both
rendered instead of discarding the later run via `next if run_start < index`;
preserve correct positional text consumption while allowing attachments inside
command output or splitting overlapping ranges. Add a regression spec covering
both overlap directions, including an attachment consumed by a command and a
command consumed by an attachment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: dc713bd6-8db3-4336-80fd-fc104d0d7281
📒 Files selected for processing (2)
spec/invidious/videos/description_spec.crsrc/invidious/videos/description.cr
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
iv-org/invidious(manual)iv-org/invidious-companion(manual)iv-org/mocks(manual)iv-org/documentation(manual)
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| runs.each do |(run_start, run_length, is_attachment, run)| | ||
| # A command and an attachment can cover the same characters. The | ||
| # iterator can only move forward, so skip anything already consumed. | ||
| next if run_start < index |
There was a problem hiding this comment.
When a commandRun and an attachmentRun cover the same custom-emoji shortcode, the command is processed first and advances index across the shared range. This guard then skips the attachment, so :party: is rendered as a search link containing the raw shortcode instead of the channel-emoji image. Handle equal or overlapping attachment ranges together, or give custom-emoji attachments precedence while retaining the intended command behavior.
Artifacts
Focused Crystal reproduction source for overlapping emoji runs
- This executable constructs attachment-only and equal-range command-plus-attachment descriptions and calls the production parser, providing the focused validation source.
Attachment-only custom emoji control output
- The Crystal control run rendered the expected custom emoji image and exited successfully, establishing the expected output.
Overlapping command and attachment custom emoji output
- The Crystal overlap run emitted a link containing the raw shortcode instead of emoji HTML and exited with failure, confirming the defect.
There was a problem hiding this comment.
Thanks. The overlap case is real in principle, but I checked it against the actual
YouTube responses before i changed anything.
I instrumented parse_description locally and logged every payload that had
both commandRuns and attachmentRuns, across a batch of videos. 15 payloads
had both, with 47 overlapping pairs. Every single overlapping attachment run
had exactly length: 0.
They all turn out to be YouTube's social media link icons - instagram_1x.png,
twitter_1x_v2.png, tiktok_1x.png, yt_favicon_ringo2.png etc from gstatic.com,
pinned next to a social link. They actually consume no characters and have no
accessibility label, so there is no shortcode text to leak. I did not see a
single case of a real custom emoji overlapping the command run.
Also worth noting: for urlEndpoint links parse_link_endpoint replaces the
anchor text with the URL itself, so text inside those ranges is discarded by
existing code regardless of this change.
And in the overlap case this PR is not a regression - on master attachmentRuns
is not read at all, so those emoji render as shortcodes today too. The guard
keeps current behaviour rather than making anything worse.
Handling nested runs properly means rendering the <img> inside the <a>,
which needs parse_link_endpoint to accept pre-rendered inner HTML. Happy to
do that if you want it, but it felt out of scope here and I would rather not
add it speculatively for a case I could not reproduce.
There was a problem hiding this comment.
That's a thorough investigation and the data is convincing. Zero-length attachment runs being social media icons is a reasonable YouTube behavior — those icons are pinned to link text without consuming characters, so there's genuinely no shortcode to leak. The empirical "47 overlapping pairs, all length 0" result substantially changes the picture.
You're also right that this isn't a regression: master ignores attachmentRuns entirely, so the guard at line 115 preserving forward-only traversal is at worst neutral for the overlap case.
One thing worth adding: a code comment documenting that overlapping attachment runs are expected to be length: 0 social-media-icon runs. If a future YouTube format change produces a non-zero-length overlap, whoever hits it will want to know why the skip exists:
| next if run_start < index | |
| # A command and an attachment can cover the same characters. The | |
| # iterator can only move forward, so skip anything already consumed. | |
| # In practice, overlapping attachment runs from YouTube have length 0 | |
| # (social media link icons) and consume no characters, so skipping | |
| # them is safe and not a regression from the pre-attachment-run behavior. | |
| next if run_start < index |
That aside, the core concern is resolved by your data. I'm withdrawing the blocker.
1 participant
Checklist
AI Disclosure
Model(s) used (and thinking/reasoning level if relevant):
Claude Opus 5 (extended thinking)
Tool(s) used:
Claude Code (CLI)
How was AI used?
The code and the specs were written by the AI. I built and ran it locally and
checked the result myself before submitting (see Testing below).
Pull request description
Fixes: #5888
I want to be awarded the bounty associated to the issue this PR is fixing.
Custom channel emoji just shows as raw text like
:face-red-heart-shape:instead of images, both in the comments and in the video descriptions.
parse_descriptiononly handled thecommandRuns. The emoji image is inattachmentRuns, which nothing read and there is evan a matching# todo parse styleRuns, commandRuns and attachmentRuns for commentsleft in the
comments/youtube.cr.Both kinds of run index into the same string and the codepoint iterator only
moves forward, so they are now merged and walked in positional order.
Standard unicode emoji are sent as attachment runs too, but their text content
is already the emoji character and renders fine, so I left those as text.
Converting them would mean proxying an image for every emoji in every comment.
Testing
Built and ran locally, then compared the comment from the issue side by side
with YouTube.
https://www.youtube.com/watch?v=Gy3bmuzmWMM&lc=UgxVdYO3R6wrr9sLhT54AaABAg
Invidious (This PR):
Youtube:
crystal tool format --checkpassescrystal spec- 175 examples, 0 failuresspec/invidious/videos/description_spec.crSummary by CodeRabbit
Greptile Summary
This change adds support for attachment runs so custom channel emoji can render through the local image proxy while standard Unicode emoji remains text. A focused parser reproduction found that custom emoji still render incorrectly when YouTube supplies a command run over the same shortcode: the description shows a linked raw shortcode instead of the emoji image.
src/invidious/videos/description.crneeds to handle overlapping command and attachment ranges before this can be merged safely.Confidence Score: 4/5
Not safe to merge until overlapping command and attachment runs render the custom emoji correctly.
A focused executable reproduction invoked the production parser, verified the attachment-only control, and then showed that adding an equal-range command run exposes the raw shortcode in a link.
Files Needing Attention:
src/invidious/videos/description.cr, particularly the ordering and forward-only consumption of command and attachment runs around line 115.What T-Rex did
Comments Outside Diff (1)
General comment
:party:, an attachment-only run renders the expected custom emoji image. Adding a same-range command run causes the result to be<a href="/results?search_query=party">:party:</a>, exposing the shortcode in a link instead of rendering custom emoji HTML.indexover the shortcode, andnext if run_start < indexatsrc/invidious/videos/description.cr:115skips the attachment run.Reviews (1): Last reviewed commit: "Render custom channel emoji in comments ..." | Re-trigger Greptile