Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (6)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughVersion bump for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/render-helper/src/methods/img.method.ts (1)
58-71: Consider adding the samehasAlreadyProxiedcheck here for consistency.Unlike the
img()function above,createImageHTML()doesn't check whether the URL is already proxied before callingproxifyImageSrc(). If a user pastes an already-proxied URL (e.g.,https://images.ecency.com/p/...) into their content, this could result in double-proxification.♻️ Suggested fix to add proxy-skip check
export function createImageHTML(src: string, isLCP: boolean): string { + // Skip re-proxification for URLs already going through proxy/avatar/cover routes + const hasAlreadyProxied = src.startsWith("https://images.ecency.com/p/") + || src.startsWith("https://images.ecency.com/u/") + || /^https:\/\/images\.ecency\.com\/\d+x\d+\//.test(src); + + const proxified = hasAlreadyProxied ? src : proxifyImageSrc(src); - const proxified = proxifyImageSrc(src); if (!proxified) return ''; const loading = isLCP ? 'eager' : 'lazy';Alternatively, extract the check into a shared helper to keep both functions in sync.
🤖 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 58 - 71, createImageHTML currently calls proxifyImageSrc(src) unconditionally which can double-proxy already-proxied URLs; before proxifying, check the same condition used in img() (the hasAlreadyProxied check) and skip proxifyImageSrc if true, returning an empty string or the original proxied src as appropriate, or factor the logic into a shared helper to keep createImageHTML and img() consistent (reference createImageHTML, proxifyImageSrc, and the hasAlreadyProxied check used in img()).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/render-helper/src/methods/img.method.ts`:
- Around line 58-71: createImageHTML currently calls proxifyImageSrc(src)
unconditionally which can double-proxy already-proxied URLs; before proxifying,
check the same condition used in img() (the hasAlreadyProxied check) and skip
proxifyImageSrc if true, returning an empty string or the original proxied src
as appropriate, or factor the logic into a shared helper to keep createImageHTML
and img() consistent (reference createImageHTML, proxifyImageSrc, and the
hasAlreadyProxied check used in img()).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 999d61a1-90f7-45fc-bcbb-761167b6b98d
📒 Files selected for processing (2)
apps/web/package.jsonpackages/render-helper/src/methods/img.method.ts
feruzm
added
patch
1 participant
Summary by CodeRabbit