Update deps#852
Open
MartinKolarik wants to merge 8 commits into
Open
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
WalkthroughThis PR replaces legacy ESLint files with flat configs (eslint.config.js and eslint-html.config.js), updates package.json scripts and dependency versions, and bumps Actions steps. It removes unused catch parameters across many source files, changes synchronous font loading to parse file buffers, and adds async wait helpers in test/utils.js while refactoring Selenium package tests to use explicit waits. 🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 3
🧹 Nitpick comments (2)
test/utils.js (1)
22-35: ⚡ Quick winReturn the matched element directly from the wait condition.
Both helpers validate one element and then re-query a potentially different one. In dynamic DOM states this can make waits pass but return the wrong node.
Proposed diff
module.exports.waitForText = async (browser, locator, expectedText, timeout = DEFAULT_WAIT_TIMEOUT) => { - await browser.wait(async () => { + return browser.wait(async () => { let elements = await browser.findElements(locator); if (!elements.length) { return false; } - let text = await elements[0].getText(); - return text.includes(expectedText); + for (let element of elements) { + let text = await element.getText(); + if (text.includes(expectedText)) { + return element; + } + } + + return false; }, timeout); - - return browser.findElement(locator); }; module.exports.waitForAttribute = async (browser, locator, attribute, expectedValue, timeout = DEFAULT_WAIT_TIMEOUT) => { - await browser.wait(async () => { + return browser.wait(async () => { let elements = await browser.findElements(locator); if (!elements.length) { return false; } - let value = await elements[0].getAttribute(attribute); - return value === expectedValue; + for (let element of elements) { + let value = await element.getAttribute(attribute); + if (value === expectedValue) { + return element; + } + } + + return false; }, timeout); - - return browser.findElement(locator); };Also applies to: 37-50
🤖 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 `@test/utils.js` around lines 22 - 35, The waitForText helper currently re-queries the DOM after the wait and can return a different element; change waitForText (in test/utils.js) so the browser.wait callback captures and returns the matching element (the found element that contains expectedText) and then have waitForText return that captured element instead of calling browser.findElement again; apply the same change to the sibling helper at lines ~37-50 to return the element instance obtained during the wait rather than re-querying.package.json (1)
161-161: ⚡ Quick winAlign CI test matrix with declared Node engine support
Line 161 declares Node 22 and 24 support, but CI currently validates only 24.x. Add 22.x to the workflow matrix to enforce the contract.
🤖 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 `@package.json` at line 161, CI currently only tests Node 24.x while package.json's engines field ("node": "22 || 24") promises Node 22 support; update the workflow's test matrix to include Node 22.x. Locate the GitHub Actions job that defines the matrix (e.g., matrix.node-version or the test job matrix in the CI workflow) and add "22.x" alongside "24.x" so both versions are run, ensuring any matrix aliases (node-version, node) match the engines string; commit the updated workflow.
🤖 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.
Inline comments:
In `@package.json`:
- Around line 153-154: The lint-staged config currently runs HTML files twice
because the glob "*.{html,js}" overlaps with "*.html"; update the mixed glob to
only target JS so HTML is handled by the HTML-specific command: change the "*.
{html,js}" entry in package.json's lint-staged to only include JS (e.g.,
"*.{js}") so staged HTML files are no longer matched by both "*. {html,js}" and
"*.html".
In `@src/middleware/open-graph/image/fonts.js`:
- Line 22: The code calls opentype.parse with a Node Buffer (font =
opentype.parse(fs.readFileSync(fontPath))) which v2.x expects to be an
ArrayBuffer; replace that direct call by reading the file into a Buffer and
converting it to an ArrayBuffer before calling opentype.parse (e.g., get the
Buffer from fs.readFileSync(fontPath), then create an ArrayBuffer view of the
buffer data) so opentype.parse receives an ArrayBuffer; update the use site
where font is assigned (font variable and any code that calls opentype.parse
with fs.readFileSync(fontPath)) to perform this conversion.
In `@test/tests/package.js`:
- Around line 39-47: The test opens a second tab (via openFilesTab +
browser.getAllWindowHandles()) then makes assertions but doesn't guarantee
cleanup if the assertion throws; wrap the interaction after obtaining tabs (the
code that switches to tabs[1], waits for 'pre', and asserts its text) in a
try/finally so that in the finally block you always close the second window
(browser.close()) if it exists and switch back to the original window
(browser.switchTo().window(tabs[0])); ensure you check that tabs.length > 1
before closing/switching to avoid errors.
---
Nitpick comments:
In `@package.json`:
- Line 161: CI currently only tests Node 24.x while package.json's engines field
("node": "22 || 24") promises Node 22 support; update the workflow's test matrix
to include Node 22.x. Locate the GitHub Actions job that defines the matrix
(e.g., matrix.node-version or the test job matrix in the CI workflow) and add
"22.x" alongside "24.x" so both versions are run, ensuring any matrix aliases
(node-version, node) match the engines string; commit the updated workflow.
In `@test/utils.js`:
- Around line 22-35: The waitForText helper currently re-queries the DOM after
the wait and can return a different element; change waitForText (in
test/utils.js) so the browser.wait callback captures and returns the matching
element (the found element that contains expectedText) and then have waitForText
return that captured element instead of calling browser.findElement again; apply
the same change to the sibling helper at lines ~37-50 to return the element
instance obtained during the wait rather than re-querying.
🪄 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: 15c55092-bee7-48b0-a653-3843a516a9f9
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (17)
.eslintrc.eslintrc-html.github/workflows/nodejs.ymleslint-html.config.jseslint.config.jspackage.jsonsrc/assets/js/_.jssrc/assets/js/app-docs.jssrc/assets/js/app.jssrc/assets/js/utils/has.jssrc/assets/js/utils/is-url-valid.jssrc/lib/assets/index.jssrc/middleware/open-graph/image/fonts.jssrc/proxy.jssrc/routes/debug.jstest/tests/package.jstest/utils.js
💤 Files with no reviewable changes (2)
- .eslintrc-html
- .eslintrc
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
2 participants
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.
No description provided.