Deduped attribute list for html transformation #732
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughCentralized attribute-matching was introduced: Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
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.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @packages/url-utils/src/utils/html-transform.ts:
- Line 2: Replace the CommonJS require with an ES6 import: change the top-level
"const cheerio = require('cheerio');" to an ES module import (for example
"import * as cheerio from 'cheerio';" or "import { load } from 'cheerio';"
depending on how you use it in this file) so lint no longer flags the require;
keep the rest of the code (e.g., uses of cheerio or load) unchanged and ensure
the file is treated as ESM by your build setup.
- Around line 4-8: The early-exit pattern computed by transformAttributes ->
earlyExitMatchStr intentionally excludes 'style', which means
html-relative-to-absolute and html-relative-to-transform-ready may early-return
(see the early return around line 45) when only style attributes contain
relative URLs; either add a concise comment next to
transformAttributes/earlyExitMatchStr explaining this deliberate omission and
its rationale (style requires url() parsing) so future readers aren't surprised,
or adjust the early-exit logic to account for style (e.g., include a separate
check for 'style' url(...) occurrences) to ensure style-based URLs are not
skipped.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/url-utils/src/utils/html-relative-to-absolute.tspackages/url-utils/src/utils/html-relative-to-transform-ready.tspackages/url-utils/src/utils/html-transform.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/url-utils/src/utils/html-relative-to-transform-ready.ts (1)
packages/url-utils/src/utils/html-transform.ts (1)
earlyExitMatchStr(5-8)
🪛 GitHub Actions: Test
packages/url-utils/src/utils/html-transform.ts
[error] 2-2: ESLint: A require() style import is forbidden. (no-require-imports)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (5)
packages/url-utils/src/utils/html-transform.ts (1)
79-79: LGTM! Centralized attribute list.Good refactoring to use the centralized
transformAttributesarray instead of a hardcoded list.packages/url-utils/src/utils/html-relative-to-absolute.ts (2)
2-2: LGTM! Correct import of centralized constant.The import properly brings in the
earlyExitMatchStrconstant from the centralized location.
14-14: LGTM! Replaced hardcoded value with centralized constant.Good refactoring to use the imported
earlyExitMatchStrinstead of the hardcoded string literal.packages/url-utils/src/utils/html-relative-to-transform-ready.ts (2)
2-2: LGTM! Correct import of centralized constant.The import properly brings in the
earlyExitMatchStrconstant from the centralized location, consistent with the other transformation utility.
30-30: LGTM! Replaced hardcoded value with centralized constant.Good refactoring to use the imported
earlyExitMatchStrinstead of the hardcoded string literal, maintaining consistency across transformation utilities.
| @@ -1,6 +1,12 @@ | |||
| import type {HtmlTransformOptions, HtmlTransformOptionsInput, UrlTransformFunction} from './types'; | |||
| const cheerio = require('cheerio'); | |||
There was a problem hiding this comment.
Fix ESLint error: Use ES6 import syntax.
The pipeline is failing due to this line using CommonJS require() instead of ES6 import syntax.
🔎 Proposed fix
-const cheerio = require('cheerio');
+import * as cheerio from 'cheerio';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const cheerio = require('cheerio'); | |
| import * as cheerio from 'cheerio'; |
🧰 Tools
🪛 GitHub Actions: Test
[error] 2-2: ESLint: A require() style import is forbidden. (no-require-imports)
🤖 Prompt for AI Agents
In @packages/url-utils/src/utils/html-transform.ts at line 2, Replace the
CommonJS require with an ES6 import: change the top-level "const cheerio =
require('cheerio');" to an ES module import (for example "import * as cheerio
from 'cheerio';" or "import { load } from 'cheerio';" depending on how you use
it in this file) so lint no longer flags the require; keep the rest of the code
(e.g., uses of cheerio or load) unchanged and ensure the file is treated as ESM
by your build setup.
allouis
force-pushed
the
cleanup-early-exit-match
branch
from
00c2a3b to
8391d6b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/url-utils/test/unit/utils/html-relative-to-absolute.test.js (1)
276-281: Consider adding a test case for style-only HTML.While the current test verifies that
styletriggers parsing, consider adding a test case with HTML containing only a style attribute (no href/src/srcset) to more directly demonstrate the bug fix—ensuring HTML like<div style="background-image: url('/image.png')">content</div>(without other transformable attributes) actually gets parsed and transformed.🔎 Example test case
it('transforms relative URLs in style attributes when no other transformable attributes exist', function () { const html = '<div style="background-image: url(\'/content/images/bg.jpg\')">Content</div>'; const result = htmlRelativeToAbsolute(html, siteUrl, itemPath, options); result.should.match(/background-image: url\('http:\/\/my-ghost-blog\.com\/content\/images\/bg\.jpg'\)/); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/url-utils/src/utils/html-relative-to-absolute.tspackages/url-utils/src/utils/html-relative-to-transform-ready.tspackages/url-utils/src/utils/html-transform.tspackages/url-utils/test/unit/utils/html-relative-to-absolute.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/url-utils/src/utils/html-relative-to-transform-ready.ts
- packages/url-utils/src/utils/html-relative-to-absolute.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,ts}
📄 CodeRabbit inference engine (CLAUDE.md)
Use
@tryghost/errorspackage for consistent error formatting across packages
Files:
packages/url-utils/src/utils/html-transform.tspackages/url-utils/test/unit/utils/html-relative-to-absolute.test.js
**/test/**/*.test.js
📄 CodeRabbit inference engine (CLAUDE.md)
**/test/**/*.test.js: Use Mocha testing framework with Should.js for assertions
Use Sinon for stubs and mocks in tests
Files:
packages/url-utils/test/unit/utils/html-relative-to-absolute.test.js
🧠 Learnings (2)
📚 Learning: 2026-01-06T10:55:56.890Z
Learnt from: CR
Repo: TryGhost/SDK PR: 0
File: packages/limit-service/CLAUDE.md:0-0
Timestamp: 2026-01-06T10:55:56.890Z
Learning: Applies to packages/limit-service/**/*.js : Fix linting issues using: npm run lint -- --fix
Applied to files:
packages/url-utils/src/utils/html-transform.ts
📚 Learning: 2026-01-06T10:55:42.617Z
Learnt from: CR
Repo: TryGhost/SDK PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-06T10:55:42.617Z
Learning: Applies to **/*.{js,ts} : Use `tryghost/errors` package for consistent error formatting across packages
Applied to files:
packages/url-utils/src/utils/html-transform.ts
🧬 Code graph analysis (1)
packages/url-utils/test/unit/utils/html-relative-to-absolute.test.js (1)
packages/url-utils/src/UrlUtils.ts (1)
htmlRelativeToAbsolute(398-412)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (4)
packages/url-utils/test/unit/utils/html-relative-to-absolute.test.js (2)
276-277: Good test coverage for style attribute transformation.The test correctly verifies that inline style attributes with
url()values trigger DOM parsing, ensuring the fix for the early-exit bug is working as intended.
281-281: Correct call count update.The assertion correctly reflects that after the style test (count 4), the
assetsOnly=truecase with a non-asset href path should not trigger additional parsing, keeping the count at 4.packages/url-utils/src/utils/html-transform.ts (2)
5-8: Excellent centralization of transformable attributes.Exporting
transformAttributesand derivingearlyExitMatchStrfrom it creates a single source of truth, preventing future mismatches between the attribute list and early-exit pattern. Includingstylefixes the bug where HTML with style-only URLs would skip transformation.Note that
earlyExitMatchStrwill match anystyle=attribute (even without URLs), triggering parsing unnecessarily in some cases. This is an acceptable trade-off for a simple, maintainable pattern that ensures all style-based URLs are caught.
79-79: Good use of centralized constant.Replacing the hardcoded array with
transformAttributesensures consistency and makes future attribute additions easier to maintain.
no-issue The early exit condition doesn't actually match the attributes which we were transforming (`style` is missing), by having a single source of truth we can easily fix the bug and prevent issues like this in future.
no-issue The early exit optimization was filtering out the style attribute, which meant HTML containing only style attributes with URLs (like background-image: url(...)) would skip parsing entirely and not have their URLs transformed.
allouis
force-pushed
the
cleanup-early-exit-match
branch
from
8391d6b to
98302cf
Compare
no-issue
The early exit condition didn't actually match the attributes which we were transforming (
stylewas missing), by have a single source of truth we fix the existing bug and prevent issues like this in future.Note
transformAttributes(href,src,srcset,style) and derivedearlyExitMatchStrinhtml-transformstyleattributes (parsingurl(...)), reusing existing transform logichtml-relative-to-absoluteandhtml-relative-to-transform-readyto use sharedearlyExitMatchStr; preservesassetsOnlyoverride behaviorstyletriggers DOM parsing and keeps early-exit call counts consistentWritten by Cursor Bugbot for commit 98302cf. This will update automatically on new commits. Configure here.