Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughSplit Playwright into a short Changes
Sequence Diagram(s)sequenceDiagram
participant Build as Playwright Build Job
participant Test as Playwright Sharded Test Job
participant Artifacts as GitHub Artifact Storage
participant Merge as Merge Reports Job
Build->>Artifacts: upload build artifacts (dist, node_modules)
Test->>Artifacts: restore build artifacts
Test->>Test: run sharded tests (per browser / shard)
Test->>Artifacts: upload blob-report-${matrix.browser}-${matrix.shardIndex} and playwright-report-${matrix.browser}
Merge->>Artifacts: download blob-report-* artifacts
Merge->>Merge: run Playwright merge to combine blob reports
Merge->>Artifacts: upload playwright-report-merged (merged HTML)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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: 5
🧹 Nitpick comments (3)
packages/mantine/src/suggestionMenu/SuggestionMenuItem.tsx (1)
28-31: Formatting improvement looks good, but consider adding a safety check for the selector.The multi-line formatting improves readability. However, the non-null assertion operator (
!) onclosest()could lead to a runtime error if the element is not found within the expected container. While the current DOM structure should ensure this selector matches, consider adding a fallback or early return to make the code more defensive against future refactoring.🛡️ Optional: Add defensive null check
- const overflow = elementOverflow( - itemRef.current, - itemRef.current.closest(".bn-suggestion-menu, `#ai-suggestion-menu`")!, - ); + const container = itemRef.current.closest( + ".bn-suggestion-menu, `#ai-suggestion-menu`" + ); + if (!container) { + return; + } + const overflow = elementOverflow(itemRef.current, container);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/mantine/src/suggestionMenu/SuggestionMenuItem.tsx` around lines 28 - 31, The call to elementOverflow uses itemRef.current.closest(... ) with a non-null assertion which may throw if the container isn't found; update the code around elementOverflow/itemRef to first verify itemRef.current and itemRef.current.closest(".bn-suggestion-menu, `#ai-suggestion-menu`") exist (or assign a fallback container) and return early or skip calling elementOverflow when missing, so elementOverflow is only invoked with a valid container reference.packages/xl-ai/src/api/formats/html-blocks/tools/rebaseTool.ts (1)
51-51: Consider making the error message more descriptive.The error message "html diff" is quite terse. Consider providing more context about what went wrong, e.g., "HTML round-trip conversion produced unexpected differences" or "Block structure changed after HTML conversion".
💬 Suggested improvement for error clarity
- throw new Error("html diff", { + throw new Error("HTML round-trip conversion failed: unexpected differences detected", {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xl-ai/src/api/formats/html-blocks/tools/rebaseTool.ts` at line 51, The thrown Error in rebaseTool.ts currently uses the terse message "html diff"; update the throw in rebaseTool (the throw new Error("html diff", { ... }) statement) to include a descriptive message such as "HTML round-trip conversion produced unexpected differences" or "Block structure changed after HTML conversion", and augment the error with contextual details (e.g., original vs. converted HTML or affected block IDs) so callers and logs can diagnose the failure.tests/src/end-to-end/copypaste/copypaste.test.ts (1)
151-151: Promote the embed URL to a shared test fixture constant.This literal is now duplicated with
tests/src/end-to-end/images/images.test.ts, so the next placeholder swap will require coordinated edits again. Moving it intotests/src/utils/const.tsor a dedicated image-fixtures helper will keep the e2e flows aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/src/end-to-end/copypaste/copypaste.test.ts` at line 151, Extract the duplicated literal IMAGE_EMBED_URL into a single exported constant in a shared test-fixture module (e.g., a new tests utils/const or image-fixtures helper) and replace the local declaration in both copypaste.test.ts and images.test.ts with an import of that exported IMAGE_EMBED_URL; remove the local const declarations, update imports to reference the shared module, and ensure the exported name is IMAGE_EMBED_URL so existing references in the tests continue to work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Around line 116-122: The upload-artifact step using actions/upload-artifact@v7
is setting archive: false which forces single-file direct upload and breaks when
path is a directory (tests/playwright-report/) and causes name to be ignored;
fix by removing the archive: false input so the directory is uploaded as a zip,
or alternatively change path to point to a single file (e.g.,
tests/playwright-report/index.html) if you truly want direct upload—update the
step that references actions/upload-artifact@v7 and the inputs name, path, and
archive accordingly.
In `@packages/xl-odt-exporter/src/odt/template/content.xml`:
- Around line 328-345: The frame currently sets svg:width="5.3335in" and
svg:height="5.3335in" while the image URL and metadata reference a 332x322
(non‑square) placeholder; update the frame dimensions to preserve aspect ratio
or swap the image for a square placeholder. Specifically, when modifying the
xlink:href "Pictures/100000000000014C0000014CDD284996.jpg" / visible URL
"https://placehold.co/332x322.jpg", also adjust draw:frame attributes svg:width
and svg:height (and any related draw:frame draw:name, svg:title, svg:desc,
Caption text) so the width/height match the 332:322 aspect ratio (or use a
332x332 URL) to avoid stretching.
In `@shared/testDocument.ts`:
- Around line 171-182: The test fixture testDocument uses external placehold.co
image URLs which cause Exporter.resolveFile() (invoked by toODTDocument() and
toDocxJsDocument()) to fetch live resources during tests; replace those external
URLs with checked-in fixture assets (e.g., a local test image path or
base64/data-URI) or update the fixture to use a test-only mockable URL token and
adjust tests to provide a mocked resolveFile implementation so no runtime fetch
is performed. Locate the image entries in testDocument (the objects with type:
"image" and props.url) and swap the url values accordingly or switch tests to
pass a mocked resolveFile when calling toODTDocument()/toDocxJsDocument().
In
`@tests/src/end-to-end/images/images.test.ts-snapshots/embedImage-webkit-linux.json`:
- Around line 18-19: Replace the external image URL by switching the
IMAGE_EMBED_URL constant in images.test.ts to point at the local fixture
(placeholder.png) included in the test directory, update any test setup that
serves static assets to ensure the test loads that local file (or adjust the
test to import the fixture directly), and then regenerate the snapshots so the
embedded image URLs in the snapshot reflect the local fixture (e.g.,
"800x540.png") instead of "https://placehold.co/800x540.png".
In `@tests/src/unit/react/formatConversion/export/__snapshots__/html/notion.json`:
- Around line 366-368: The snapshot JSON contains JavaScript `undefined`
literals which are invalid JSON; open the snapshot file and replace the
undefined values in the "content" object (e.g., the "columnWidths": [undefined,
undefined, undefined] array and "headerCols": undefined) with JSON-safe values
such as null (e.g., "columnWidths": [null, null, null] and "headerCols": null)
or remove those keys entirely so the file parses as valid JSON.
---
Nitpick comments:
In `@packages/mantine/src/suggestionMenu/SuggestionMenuItem.tsx`:
- Around line 28-31: The call to elementOverflow uses
itemRef.current.closest(... ) with a non-null assertion which may throw if the
container isn't found; update the code around elementOverflow/itemRef to first
verify itemRef.current and itemRef.current.closest(".bn-suggestion-menu,
`#ai-suggestion-menu`") exist (or assign a fallback container) and return early or
skip calling elementOverflow when missing, so elementOverflow is only invoked
with a valid container reference.
In `@packages/xl-ai/src/api/formats/html-blocks/tools/rebaseTool.ts`:
- Line 51: The thrown Error in rebaseTool.ts currently uses the terse message
"html diff"; update the throw in rebaseTool (the throw new Error("html diff", {
... }) statement) to include a descriptive message such as "HTML round-trip
conversion produced unexpected differences" or "Block structure changed after
HTML conversion", and augment the error with contextual details (e.g., original
vs. converted HTML or affected block IDs) so callers and logs can diagnose the
failure.
In `@tests/src/end-to-end/copypaste/copypaste.test.ts`:
- Line 151: Extract the duplicated literal IMAGE_EMBED_URL into a single
exported constant in a shared test-fixture module (e.g., a new tests utils/const
or image-fixtures helper) and replace the local declaration in both
copypaste.test.ts and images.test.ts with an import of that exported
IMAGE_EMBED_URL; remove the local const declarations, update imports to
reference the shared module, and ensure the exported name is IMAGE_EMBED_URL so
existing references in the tests continue to work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5ae1199-c84d-4e06-9d49-47ee2bd3acf5
⛔ Files ignored due to path filters (12)
packages/xl-email-exporter/src/react-email/__snapshots__/reactEmailExporter.test.tsx.snapis excluded by!**/*.snaptests/src/end-to-end/basics/basicblocks.test.ts-snapshots/basicblocks-chromium-linux.pngis excluded by!**/*.pngtests/src/end-to-end/basics/basicblocks.test.ts-snapshots/basicblocks-firefox-linux.pngis excluded by!**/*.pngtests/src/end-to-end/images/images.test.ts-snapshots/embed-image-chromium-linux.pngis excluded by!**/*.pngtests/src/end-to-end/images/images.test.ts-snapshots/embed-image-firefox-linux.pngis excluded by!**/*.pngtests/src/end-to-end/images/images.test.ts-snapshots/embed-image-webkit-linux.pngis excluded by!**/*.pngtests/src/end-to-end/images/images.test.ts-snapshots/resize-image-chromium-linux.pngis excluded by!**/*.pngtests/src/end-to-end/images/images.test.ts-snapshots/resize-image-firefox-linux.pngis excluded by!**/*.pngtests/src/end-to-end/images/images.test.ts-snapshots/resize-image-webkit-linux.pngis excluded by!**/*.pngtests/src/end-to-end/static/static.test.ts-snapshots/static-rendering-equality-chromium-linux.pngis excluded by!**/*.pngtests/src/end-to-end/static/static.test.ts-snapshots/static-rendering-equality-firefox-linux.pngis excluded by!**/*.pngtests/src/end-to-end/static/static.test.ts-snapshots/static-rendering-equality-webkit-linux.pngis excluded by!**/*.png
📒 Files selected for processing (61)
.github/workflows/build.ymlexamples/01-basic/04-default-blocks/src/App.tsxexamples/03-ui-components/02-formatting-toolbar-buttons/src/App.tsxexamples/05-interoperability/05-converting-blocks-to-pdf/src/App.tsxexamples/05-interoperability/06-converting-blocks-to-docx/src/App.tsxexamples/05-interoperability/07-converting-blocks-to-odt/src/App.tsxexamples/05-interoperability/08-converting-blocks-to-react-email/src/App.tsxexamples/05-interoperability/09-blocks-to-html-static-render/src/App.tsxexamples/05-interoperability/10-static-html-render/src/App.tsxexamples/06-custom-schema/react-custom-blocks/src/App.tsxexamples/vanilla-js/react-vanilla-custom-blocks/src/App.tsxpackages/ariakit/src/suggestionMenu/SuggestionMenuItem.tsxpackages/ariakit/src/suggestionMenu/SuggestionMenuLoader.tsxpackages/ariakit/src/suggestionMenu/gridSuggestionMenu/GridSuggestionMenuItem.tsxpackages/code-block/vite.config.tspackages/core/src/api/blockManipulation/commands/mergeBlocks/mergeBlocks.tspackages/core/src/editor/managers/ExtensionManager/symbol.tspackages/core/src/editor/managers/SelectionManager.tspackages/core/src/extensions/tiptap-extensions/KeyboardShortcuts/KeyboardShortcutsExtension.tspackages/core/src/i18n/locales/fa.tspackages/core/src/i18n/locales/index.tspackages/core/src/i18n/locales/uz.tspackages/core/vite.config.tspackages/mantine/src/form/TextInput.tsxpackages/mantine/src/suggestionMenu/SuggestionMenuItem.tsxpackages/mantine/src/suggestionMenu/gridSuggestionMenu/GridSuggestionMenuItem.tsxpackages/react/src/editor/BlockNoteView.tsxpackages/server-util/vite.config.tspackages/xl-ai-server/src/routes/model-playground/index.tspackages/xl-ai-server/src/routes/regular.tspackages/xl-ai-server/src/routes/serverPersistence.tspackages/xl-ai/src/api/formats/html-blocks/tools/rebaseTool.tspackages/xl-ai/src/api/promptHelpers/convertBlocks.tspackages/xl-ai/src/util/trimArray.tspackages/xl-ai/vitestSetup.tspackages/xl-docx-exporter/src/docx/__snapshots__/basic/document.xmlpackages/xl-docx-exporter/src/docx/defaultSchema/blocks.tspackages/xl-docx-exporter/src/docx/defaultSchema/styles.tspackages/xl-docx-exporter/vite.config.tspackages/xl-email-exporter/vite.config.tspackages/xl-multi-column/vite.config.tspackages/xl-odt-exporter/src/odt/__snapshots__/basic/content.xmlpackages/xl-odt-exporter/src/odt/__snapshots__/withCustomOptions/content.xmlpackages/xl-odt-exporter/src/odt/template/content.xmlpackages/xl-odt-exporter/vite.config.tspackages/xl-pdf-exporter/src/pdf/__snapshots__/example.jsxpackages/xl-pdf-exporter/src/pdf/__snapshots__/exampleWithHeaderAndFooter.jsxpackages/xl-pdf-exporter/vite.config.tsshared/testDocument.tstests/src/end-to-end/copypaste/copypaste.test.tstests/src/end-to-end/copypaste/copypaste.test.ts-snapshots/images-json-chromium-linux.jsontests/src/end-to-end/images/images.test.tstests/src/end-to-end/images/images.test.ts-snapshots/embedImage-chromium-linux.jsontests/src/end-to-end/images/images.test.ts-snapshots/embedImage-firefox-linux.jsontests/src/end-to-end/images/images.test.ts-snapshots/embedImage-webkit-linux.jsontests/src/end-to-end/images/images.test.ts-snapshots/resizeImage-chromium-linux.jsontests/src/end-to-end/images/images.test.ts-snapshots/resizeImage-firefox-linux.jsontests/src/end-to-end/images/images.test.ts-snapshots/resizeImage-webkit-linux.jsontests/src/unit/core/formatConversion/parse/__snapshots__/html/notion.jsontests/src/unit/core/formatConversion/parse/parseTestInstances.tstests/src/unit/react/formatConversion/export/__snapshots__/html/notion.json
💤 Files with no reviewable changes (12)
- packages/server-util/vite.config.ts
- packages/xl-multi-column/vite.config.ts
- packages/core/vite.config.ts
- packages/code-block/vite.config.ts
- packages/xl-pdf-exporter/vite.config.ts
- packages/core/src/editor/managers/ExtensionManager/symbol.ts
- packages/mantine/src/suggestionMenu/gridSuggestionMenu/GridSuggestionMenuItem.tsx
- packages/ariakit/src/suggestionMenu/SuggestionMenuItem.tsx
- packages/ariakit/src/suggestionMenu/gridSuggestionMenu/GridSuggestionMenuItem.tsx
- packages/xl-odt-exporter/vite.config.ts
- packages/xl-email-exporter/vite.config.ts
- packages/xl-docx-exporter/vite.config.ts
tests/src/end-to-end/images/images.test.ts-snapshots/embedImage-webkit-linux.json
Show resolved
Hide resolved
tests/src/unit/react/formatConversion/export/__snapshots__/html/notion.json
Show resolved
Hide resolved
@blocknote/ariakit
@blocknote/code-block
@blocknote/core
@blocknote/mantine
@blocknote/react
@blocknote/server-util
@blocknote/shadcn
@blocknote/xl-ai
@blocknote/xl-docx-exporter
@blocknote/xl-email-exporter
@blocknote/xl-multi-column
@blocknote/xl-odt-exporter
@blocknote/xl-pdf-exporter
commit: |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/xl-odt-exporter/src/odt/__snapshots__/basic/content.xml (1)
344-344: Round serialized image dimensions before snapshotting.
193.97590361445782pxlooks like raw floating-point output. Persisting full precision in generated ODT XML makes these snapshots noisy and can cause churn for layout-equivalent output. Please clamp exported dimensions to a stable precision before serializing them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xl-odt-exporter/src/odt/__snapshots__/basic/content.xml` at line 344, The snapshot shows full-precision floats in svg:height/svg:width (e.g., "193.97590361445782px") which creates noisy diffs; update the ODT export code that serializes image frame dimensions (the logic that emits svg:width and svg:height in the exporter/serializer) to clamp/round dimensions to a stable precision before appending "px" (e.g., round to 2 decimal places or nearest integer), and ensure the same rounding is applied in the function that computes or formats frame attributes (search for the code paths that produce draw:frame attributes or methods like serializeImageFrame/formatDimension) so snapshots contain stable, deterministic values.tests/playwright.config.ts (1)
50-54: Considertrace: "on-first-retry"andvideo: "off"to reduce CI overhead in passing test runs.In Playwright Test,
trace: "retain-on-failure"andvideo: "retain-on-failure"record artifacts for every test run, then delete them only for passing runs. This incurs recording overhead (CPU, disk I/O, temp disk usage) on all tests, even those that pass—a cost that compounds across a 3-browser suite. Playwright's official guidance recommends usingtrace: "on-first-retry"(paired with retries) for CI, which records only on test retry attempts, reducing overhead to near-zero for fully passing runs. Keepvideo: "off"unless video debugging is routinely needed.Proposed adjustment
- trace: "retain-on-failure", + trace: process.env.CI ? "on-first-retry" : "retain-on-failure", /* Capture screenshot on failure for better debugging */ screenshot: "only-on-failure", /* Record video on failure for better debugging */ - video: "retain-on-failure", + video: process.env.CI ? "off" : "retain-on-failure",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/playwright.config.ts` around lines 50 - 54, The Playwright config currently uses trace: "retain-on-failure" and video: "retain-on-failure", which records artifacts on every run; change trace to "on-first-retry" and video to "off" in the exported test config to avoid recording overhead, and verify the retries setting (e.g., retries in the same config) is enabled so traces are captured on retry; keep screenshot: "only-on-failure" as-is unless you also want to change it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build.yml:
- Around line 152-161: The workflow downloads artifacts to all-blob-reports at
the repository root but the "Merge reports" step runs with working-directory:
tests so npx playwright merge-reports --reporter html ./all-blob-reports will
look in tests/all-blob-reports and fail; fix by either changing the download
step "path" to tests/all-blob-reports or by updating the Merge reports command
to point to the parent directory (e.g., ../all-blob-reports) so the npx
playwright merge-reports invocation finds the downloaded files; update the steps
named "Download blob reports" and/or "Merge reports" accordingly.
---
Nitpick comments:
In `@packages/xl-odt-exporter/src/odt/__snapshots__/basic/content.xml`:
- Line 344: The snapshot shows full-precision floats in svg:height/svg:width
(e.g., "193.97590361445782px") which creates noisy diffs; update the ODT export
code that serializes image frame dimensions (the logic that emits svg:width and
svg:height in the exporter/serializer) to clamp/round dimensions to a stable
precision before appending "px" (e.g., round to 2 decimal places or nearest
integer), and ensure the same rounding is applied in the function that computes
or formats frame attributes (search for the code paths that produce draw:frame
attributes or methods like serializeImageFrame/formatDimension) so snapshots
contain stable, deterministic values.
In `@tests/playwright.config.ts`:
- Around line 50-54: The Playwright config currently uses trace:
"retain-on-failure" and video: "retain-on-failure", which records artifacts on
every run; change trace to "on-first-retry" and video to "off" in the exported
test config to avoid recording overhead, and verify the retries setting (e.g.,
retries in the same config) is enabled so traces are captured on retry; keep
screenshot: "only-on-failure" as-is unless you also want to change it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3155169-7d90-46db-9cab-ae86e89ab8f6
📒 Files selected for processing (6)
.github/workflows/build.ymlpackages/xl-docx-exporter/src/docx/__snapshots__/basic/document.xmlpackages/xl-docx-exporter/src/docx/__snapshots__/withCustomOptions/document.xml.relspackages/xl-odt-exporter/src/odt/__snapshots__/basic/content.xmlpackages/xl-odt-exporter/src/odt/__snapshots__/withCustomOptions/content.xmltests/playwright.config.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/xl-odt-exporter/src/odt/snapshots/withCustomOptions/content.xml
- Split build and test phases to avoid rebuilding 3x (once per browser) - Add test sharding (2 shards per browser) for parallel test execution - Enable parallel workers (50% CPU) in CI instead of sequential - Fix blob report download path for merge-reports job - Add blob-report to gitignore
- Change trace from retain-on-failure to on-first-retry - Disable video recording entirely - Keep screenshot only-on-failure for lightweight debugging
Use github.event.pull_request.head.sha for PRs to ensure cache key matches between playwright-build and playwright jobs. The github.sha variable returns the merge commit SHA for PRs which differs between jobs.
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 @.github/workflows/build.yml:
- Around line 158-165: The artifact name "blob-report-${{ matrix.browser }}" in
the "Upload blob report" step isn't unique across shard jobs; change the
artifact name to include a shard-specific identifier (for example:
"blob-report-${{ matrix.browser }}-${{ matrix.shard }}" or "blob-report-${{
matrix.browser }}-${{ matrix.shard_index }}" or include "github.run_id" or
"github.job") so each shard uploads with a unique name; update the with.name
value accordingly in the upload-artifact step.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 057071a5-f345-4f97-a144-33cc3b123813
📒 Files selected for processing (3)
.github/workflows/build.yml.gitignoretests/playwright.config.ts
✅ Files skipped from review due to trivial changes (1)
- .gitignore
GitHub Actions cache doesn't work across host/container boundaries. The playwright-build job runs on ubuntu-latest host while playwright test jobs run inside mcr.microsoft.com/playwright container. Switch to artifacts which properly transfer between host and container.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/build.yml (1)
158-163:⚠️ Potential issue | 🔴 CriticalMake the blob artifact name shard-specific.
Line 162 still omits
matrix.shardIndex, so both shard jobs for a browser publish the same artifact name.actions/upload-artifact@v4requires artifact names to be unique within a workflow run, and Playwright’s sharding example keys blob artifacts by shard. (github.com)Suggested fix
- name: Upload blob report uses: actions/upload-artifact@v4 if: ${{ !cancelled() }} with: - name: blob-report-${{ matrix.browser }} + name: blob-report-${{ matrix.browser }}-${{ matrix.shardIndex }} path: tests/blob-report/ retention-days: 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 158 - 163, The Upload blob report step uses actions/upload-artifact@v4 and currently sets the artifact name to blob-report-${{ matrix.browser }} which is not shard-unique; update the step that defines name (the "Upload blob report" step) to include the shard identifier (matrix.shardIndex) in the artifact name so each shard publishes a distinct artifact (e.g., append -shard-${{ matrix.shardIndex }} or similar) ensuring unique artifact names per workflow run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/build.yml:
- Around line 158-163: The Upload blob report step uses
actions/upload-artifact@v4 and currently sets the artifact name to
blob-report-${{ matrix.browser }} which is not shard-unique; update the step
that defines name (the "Upload blob report" step) to include the shard
identifier (matrix.shardIndex) in the artifact name so each shard publishes a
distinct artifact (e.g., append -shard-${{ matrix.shardIndex }} or similar)
ensuring unique artifact names per workflow run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8c04b37b-e6a4-4485-8175-230be1771ed4
📒 Files selected for processing (1)
.github/workflows/build.yml
Both shards of the same browser were trying to upload to the same artifact name causing a conflict error.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/build.yml (1)
154-160:⚠️ Potential issue | 🔴 CriticalMake the blob artifact name shard-specific.
Line 158 is still shared by both shard jobs for a browser. With
actions/upload-artifact@v4, artifact names are immutable and must be unique within a workflow run, and the action docs call matrix uploads out as a conflict case. One of the two shard uploads will fail, which leavesmerge-reportswithout a complete blob set. Appendmatrix.shardIndex(or another shard-unique suffix) to the artifact name. (github.com)Proposed fix
- name: Upload blob report uses: actions/upload-artifact@v4 if: ${{ !cancelled() }} with: - name: blob-report-${{ matrix.browser }} + name: blob-report-${{ matrix.browser }}-${{ matrix.shardIndex }} path: tests/blob-report/ retention-days: 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 154 - 160, The artifact name for the upload-artifact step ("name: blob-report-${{ matrix.browser }}") is not unique across shard jobs; update the name to include a shard-unique suffix (e.g., append `${{ matrix.shardIndex }}` or an equivalent shard identifier) so each shard creates a distinct artifact name when using actions/upload-artifact@v4 and prevents upload conflicts.
🧹 Nitpick comments (1)
.github/workflows/build.yml (1)
106-113: Fail fast if the build artifact upload resolves to nothing.This artifact is a hard prerequisite for the Playwright matrix, but
actions/upload-artifactonly warns by default when no files are found. If these dist paths ever stop matching, the producer job can still look green and the failure moves downstream into the shard jobs. Setif-no-files-found: errorhere so the breakage stays local. (github.com)Proposed fix
- name: Upload build artifacts uses: actions/upload-artifact@v4 with: name: playwright-build path: | packages/*/dist playground/dist + if-no-files-found: error retention-days: 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 106 - 113, The upload step named "Upload build artifacts" uses actions/upload-artifact@v4 and currently doesn't fail if no files are matched; update that step to include the input if-no-files-found: error so the job fails fast when packages/*/dist or playground/dist resolve to nothing, keeping the producer job from appearing green when artifacts are missing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/build.yml:
- Around line 154-160: The artifact name for the upload-artifact step ("name:
blob-report-${{ matrix.browser }}") is not unique across shard jobs; update the
name to include a shard-unique suffix (e.g., append `${{ matrix.shardIndex }}`
or an equivalent shard identifier) so each shard creates a distinct artifact
name when using actions/upload-artifact@v4 and prevents upload conflicts.
---
Nitpick comments:
In @.github/workflows/build.yml:
- Around line 106-113: The upload step named "Upload build artifacts" uses
actions/upload-artifact@v4 and currently doesn't fail if no files are matched;
update that step to include the input if-no-files-found: error so the job fails
fast when packages/*/dist or playground/dist resolve to nothing, keeping the
producer job from appearing green when artifacts are missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 28a6d1bd-3c1d-4315-94d9-b81d87c68333
⛔ Files ignored due to path filters (6)
tests/src/end-to-end/customblocks/customblocks.test.ts-snapshots/react-interactivity-chromium-linux.pngis excluded by!**/*.pngtests/src/end-to-end/customblocks/customblocks.test.ts-snapshots/react-interactivity-firefox-linux.pngis excluded by!**/*.pngtests/src/end-to-end/customblocks/customblocks.test.ts-snapshots/react-interactivity-webkit-linux.pngis excluded by!**/*.pngtests/src/end-to-end/customblocks/customblocks.test.ts-snapshots/vanilla-interactivity-chromium-linux.pngis excluded by!**/*.pngtests/src/end-to-end/customblocks/customblocks.test.ts-snapshots/vanilla-interactivity-firefox-linux.pngis excluded by!**/*.pngtests/src/end-to-end/customblocks/customblocks.test.ts-snapshots/vanilla-interactivity-webkit-linux.pngis excluded by!**/*.png
📒 Files selected for processing (7)
.github/workflows/build.ymltests/src/end-to-end/customblocks/customblocks.test.ts-snapshots/reactInteractivity-chromium-linux.jsontests/src/end-to-end/customblocks/customblocks.test.ts-snapshots/reactInteractivity-firefox-linux.jsontests/src/end-to-end/customblocks/customblocks.test.ts-snapshots/reactInteractivity-webkit-linux.jsontests/src/end-to-end/customblocks/customblocks.test.ts-snapshots/vanillaInteractivity-chromium-linux.jsontests/src/end-to-end/customblocks/customblocks.test.ts-snapshots/vanillaInteractivity-firefox-linux.jsontests/src/end-to-end/customblocks/customblocks.test.ts-snapshots/vanillaInteractivity-webkit-linux.json
✅ Files skipped from review due to trivial changes (3)
- tests/src/end-to-end/customblocks/customblocks.test.ts-snapshots/reactInteractivity-webkit-linux.json
- tests/src/end-to-end/customblocks/customblocks.test.ts-snapshots/vanillaInteractivity-webkit-linux.json
- tests/src/end-to-end/customblocks/customblocks.test.ts-snapshots/vanillaInteractivity-chromium-linux.json
The placehold.co URL renders differently between live and static screenshots (Unicode × vs ASCII x character), causing ~152 pixel differences. Increased from 10 to 200 pixels to accommodate this.
- Update resize-image-webkit-linux.png snapshot (resize handle rendering changed) - Skip basicblocks test on webkit due to flaky locator.click timeout
The first toMatchSnapshot call (line 37) was missing maxDiffPixels, causing failures even though the second call had it.
nperez0111
changed the title
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
Rationale
Changes
Impact
Testing
Screenshots/Video
Checklist
Additional Notes
Summary by CodeRabbit
Chores
Updates
Style