Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR removes client-side WebP/format flags from render-helper and callers, standardizes proxied images to format=match (no file extensions), simplifies renderPostBody calls (drops third arg), adds 3Speak iframe orientation detection with speak-{orientation} classes, and updates tests/snapshots. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as Post Renderer
participant Extension as ThreeSpeak Extension
participant IFrame as IFrame Element
participant Player as 3Speak Player (iframe window)
Renderer->>Extension: mount 3Speak embed (create iframe & set src)
Extension->>IFrame: insert iframe (attach ref)
IFrame->>Player: load embed URL
Player->>IFrame: postMessage { type: "3speak-player-ready", isVertical, aspectRatio }
IFrame->>Extension: message event (validated origin & source)
Extension->>Extension: map isVertical/aspectRatio -> orientation
Extension->>Renderer: apply speak-{orientation} class to container
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/render-helper/src/methods/img.method.ts (2)
53-63:createImageHTMLlacks a guard for an emptyproxifyImageSrcresult.If
srcis somehow invalid,proxifyImageSrc(src)returns""and the returned HTML will containsrc="", rendering a broken image. Consistent with the fix above, a guard here keeps the output safe.♻️ Proposed fix
export function createImageHTML(src: string, isLCP: boolean): string { const loading = isLCP ? 'eager' : 'lazy'; const fetch = isLCP ? 'fetchpriority="high"' : 'decoding="async"'; const proxified = proxifyImageSrc(src); + if (!proxified) return ''; return `<img class="markdown-img-link" src="${proxified}" loading="${loading}" ${fetch} itemprop="image" />`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/render-helper/src/methods/img.method.ts` around lines 53 - 63, createImageHTML should guard against an empty proxified URL: call proxifyImageSrc(src) into proxified and if proxified is falsy/empty, return an empty string (no <img> tag) instead of producing src=""; otherwise continue building the img HTML using loading, fetch and proxified as before so you avoid rendering broken images.
4-4:srcis never reassigned — preferconst.
srcis read-only after initialisation;letis no longer needed now that the proxification path usessetAttributerather than mutating the local variable.♻️ Proposed change
- let src = el.getAttribute("src") || ""; + const src = el.getAttribute("src") || "";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/render-helper/src/methods/img.method.ts` at line 4, The variable src in img.method.ts is never reassigned so replace its declaration with a const to reflect immutability: change the declaration of src (currently "let src = el.getAttribute('src') || ''") to use const so that the value retrieved from el.getAttribute remains read-only and aligns with the proxification path that now uses setAttribute rather than mutating the local variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/render-helper/src/methods/img.method.ts`:
- Around line 47-50: The code passes the raw undecoded src to proxifyImageSrc
and unconditionally sets the element src even if proxifyImageSrc returns an
empty string; change the call in the shouldReplace && !hasAlreadyProxied block
to pass decodedSrc (the value used for validation) to proxifyImageSrc, then
check the returned proxified value and only call el.setAttribute("src",
proxified) when proxified is a non-empty truthy string; leave the original src
untouched if proxifyImageSrc returns an empty string or falsy value so images
are not blanked out.
---
Nitpick comments:
In `@packages/render-helper/src/methods/img.method.ts`:
- Around line 53-63: createImageHTML should guard against an empty proxified
URL: call proxifyImageSrc(src) into proxified and if proxified is falsy/empty,
return an empty string (no <img> tag) instead of producing src=""; otherwise
continue building the img HTML using loading, fetch and proxified as before so
you avoid rendering broken images.
- Line 4: The variable src in img.method.ts is never reassigned so replace its
declaration with a const to reflect immutability: change the declaration of src
(currently "let src = el.getAttribute('src') || ''") to use const so that the
value retrieved from el.getAttribute remains read-only and aligns with the
proxification path that now uses setAttribute rather than mutating the local
variable.
feruzm
added
the
patch
Contributor
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/render-helper/src/methods/img.method.ts`:
- Around line 46-51: Ensure the img replacement block uses the decoded URL and
only writes back a truthy proxified value: call proxifyImageSrc with decodedSrc
and, if the result is truthy, set it on the element via el.setAttribute("src",
...); keep the existing guards shouldReplace and !hasAlreadyProxied to avoid
overwriting an already-proxied src and preserve the original src on proxify
failure (references: proxifyImageSrc, decodedSrc, shouldReplace,
hasAlreadyProxied, el.setAttribute).
This was referenced Mar 12, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes