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:
📝 WalkthroughWalkthroughAdds responsive image srcset generation and sanitization, exposes the srcset builder, updates image whitelist, updates tests/fixtures, adds upstream validation for empty post bodies in post-edit, and wraps analytics submission in a try/catch to swallow network errors. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
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.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 53-71: The code marks /u/ and /<w>x<h>/ as "already proxied" but
still passes the full proxy URL into buildSrcSet(src), causing buildSrcSet to
re-proxify the proxy URL; change the branch that handles shouldReplace &&
hasAlreadyProxied to pass the original/unwrapped image identifier to buildSrcSet
(use decodedSrc) except when the src truly is a /p/ entry; i.e., in the else-if
for hasAlreadyProxied, check if src startsWith `${base}/p/` then call
buildSrcSet(src) otherwise call buildSrcSet(decodedSrc) so proxifyImageSrc isn't
applied to an already-proxied wrapper and you avoid nested /p/ entries.
In `@packages/render-helper/src/methods/sanitize-html.method.ts`:
- Around line 21-25: The comment for img[srcset] is inaccurate; update the
comment above the srcset handling in sanitize-html.method.ts (the block that
checks tag === 'img' && name === 'srcset', uses decoded and computes candidates)
to state the actual behavior: user-supplied srcset is stripped/normalized and
will be rejected (return '') if any candidate does not start with http:// or
https:// — i.e., the code currently enforces an http(s) URL requirement for all
srcset candidates rather than only allowing renderer-generated values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85379d0d-2683-4dd3-8008-3a1bf7a7d9e0
📒 Files selected for processing (7)
packages/render-helper/src/consts/allowed-attributes.const.tspackages/render-helper/src/markdown-2-html.spec.tspackages/render-helper/src/methods/img.method.tspackages/render-helper/src/methods/sanitize-html.method.tspackages/render-helper/src/proxify-image-src.spec.tspackages/render-helper/src/proxify-image-src.tspackages/render-helper/src/test/data/legacy/2112524.json
✅ Files skipped from review due to trivial changes (2)
- packages/render-helper/src/test/data/legacy/2112524.json
- packages/render-helper/src/consts/allowed-attributes.const.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/render-helper/src/proxify-image-src.spec.ts
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/render-helper/src/methods/img.method.ts (1)
84-91:⚠️ Potential issue | 🟠 Major
createImageHTML()still builds srcset for non-/p/proxied routes, causing nested proxy variants.Line 84 unconditionally calls
buildSrcSet(src). For${base}/u/and${base}/<w>x<h>/inputs, this re-proxifies proxy URLs and produces non-canonical/p/<b58(proxy-url)>entries.img()already avoids this;createImageHTML()should align.Suggested patch
export function createImageHTML(src: string, isLCP: boolean): string { const proxified = proxifyImageSrc(src); if (!proxified) return ''; - const srcset = buildSrcSet(src); + const base = getProxyBase().replace(/\/+$/, ""); + const isPRoute = src.startsWith(`${base}/p/`); + const isAvatarOrCoverRoute = + src.startsWith(`${base}/u/`) || + new RegExp(`^${base.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}/\\d+x\\d+/`).test(src); + const srcset = isAvatarOrCoverRoute ? '' : buildSrcSet(isPRoute ? src : src); const loading = isLCP ? 'eager' : 'lazy'; const fetch = isLCP ? 'fetchpriority="high"' : 'decoding="async"'; const srcsetAttr = srcset ? `srcset="${srcset}" sizes="${IMAGE_SIZES}"` : '';🤖 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 84 - 91, createImageHTML() currently always calls buildSrcSet(src), which causes double-proxied entries when src is already a proxied URL (e.g., routes under /p/, /u/, or <w>x<h>/). Modify createImageHTML to mirror img() behavior: detect when the incoming src is already a proxied route and skip calling buildSrcSet (or pass the original proxified flag) so you do not generate srcset entries that re-proxify the proxy URL; call buildSrcSet only for canonical non-proxy sources. Update logic around createImageHTML, buildSrcSet, and the src/proxified variables to conditionally build srcset based on whether src is a proxy URL (check the /p/ or other proxy patterns) and ensure srcsetAttr remains empty when skipping srcset generation.
🧹 Nitpick comments (1)
packages/render-helper/src/methods/img.method.ts (1)
52-55: Normalize proxy base before route checks to avoid false negatives.If
getProxyBase()is configured with a trailing slash, these checks can miss already-proxied URLs (//p/,//u/, etc.). Normalize once before building route prefixes/regex.Suggested patch
- const base = getProxyBase(); + const base = getProxyBase().replace(/\/+$/, ""); const hasAlreadyProxied = src.startsWith(`${base}/p/`) || src.startsWith(`${base}/u/`) || new RegExp(`^${base.replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}/\\d+x\\d+/`).test(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` around lines 52 - 55, Normalize the proxy base returned by getProxyBase() once (e.g., strip any trailing slash) before using it to build the route prefixes and regex so checks for already-proxied URLs aren't skipped when the base has a trailing slash; update the logic that sets base and hasAlreadyProxied (referencing getProxyBase, hasAlreadyProxied, src) to use the normalized base and then construct the `${base}/p/`, `${base}/u/` prefixes and the escaped-regex from that normalized value.
🤖 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 84-91: createImageHTML() currently always calls buildSrcSet(src),
which causes double-proxied entries when src is already a proxied URL (e.g.,
routes under /p/, /u/, or <w>x<h>/). Modify createImageHTML to mirror img()
behavior: detect when the incoming src is already a proxied route and skip
calling buildSrcSet (or pass the original proxified flag) so you do not generate
srcset entries that re-proxify the proxy URL; call buildSrcSet only for
canonical non-proxy sources. Update logic around createImageHTML, buildSrcSet,
and the src/proxified variables to conditionally build srcset based on whether
src is a proxy URL (check the /p/ or other proxy patterns) and ensure srcsetAttr
remains empty when skipping srcset generation.
---
Nitpick comments:
In `@packages/render-helper/src/methods/img.method.ts`:
- Around line 52-55: Normalize the proxy base returned by getProxyBase() once
(e.g., strip any trailing slash) before using it to build the route prefixes and
regex so checks for already-proxied URLs aren't skipped when the base has a
trailing slash; update the logic that sets base and hasAlreadyProxied
(referencing getProxyBase, hasAlreadyProxied, src) to use the normalized base
and then construct the `${base}/p/`, `${base}/u/` prefixes and the escaped-regex
from that normalized value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bd0d9b77-904a-4923-b449-ca6754b71e6d
📒 Files selected for processing (3)
packages/render-helper/src/markdown-2-html.spec.tspackages/render-helper/src/methods/img.method.tspackages/render-helper/src/methods/sanitize-html.method.ts
✅ Files skipped from review due to trivial changes (1)
- packages/render-helper/src/methods/sanitize-html.method.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/render-helper/src/markdown-2-html.spec.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 84-87: The isAlreadyProxied check in img.method.ts uses
getProxyBase() without normalizing trailing slashes, causing false negatives
when setProxyBase() was configured with a trailing '/'; update the logic around
getProxyBase(), isAlreadyProxied and buildSrcSet so you first normalize the
proxy base (trim trailing slashes, e.g., normalizeBase = base.replace(/\/+$/,
'') or call the same normalization used in img()), then use that normalizedBase
in the startsWith(`${normalizedBase}/u/`) check and in the RegExp construction
for the dimension-prefixed path so already-proxied URLs are reliably detected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 03877926-7965-4d3f-86c1-09bdb8eeea75
📒 Files selected for processing (2)
packages/render-helper/src/markdown-2-html.spec.tspackages/render-helper/src/methods/img.method.ts
✅ Files skipped from review due to trivial changes (1)
- packages/render-helper/src/markdown-2-html.spec.ts
feruzm
added
the
patch
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
Bug Fixes
Tests