Conversation
|
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:
WalkthroughRemoves React 17-specific client renderers and version-gating logic, standardizing on the modern client renderer ( Sequence Diagram(s)mermaid Script->>NVM: load nvm & record initial Node Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ 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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/pages/src/common/src/template/internal/getTemplateFilepaths.test.ts (1)
51-67:⚠️ Potential issue | 🟡 MinorMake the “default client template” test deterministic by mocking
existsSynctofalse.Right now this case depends on actual filesystem state; if
src/templates/_client.tsxexists, the assertion can flip.✅ Proposed fix
it("uses default client template when no custom client", async () => { const rootPath = path.join("src", "templates"); const util = await import("./util.js"); vi.spyOn(util, "globSync").mockImplementation(() => [ path.join(rootPath, "_client.tsx"), path.join(rootPath, "_server.tsx"), path.join(rootPath, "custom.tsx"), ]); + vi.spyOn(util, "existsSync").mockImplementation(() => false); const clientServerRenderTemplates = getGlobalClientServerRenderTemplates([ new Path(path.join(process.cwd(), rootPath)), ]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/pages/src/common/src/template/internal/getTemplateFilepaths.test.ts` around lines 51 - 67, The test for default client template is nondeterministic because it relies on the real filesystem; update the test to mock util.existsSync to always return false so the code path chooses the default instead of a real file. In the test where util.globSync is mocked and getGlobalClientServerRenderTemplates is called, add vi.spyOn(util, "existsSync").mockImplementation(() => false) before invoking getGlobalClientServerRenderTemplates (and restore the spy after the test if necessary) to ensure clientRenderTemplatePath resolves to the default in a deterministic way.
🤖 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/pages/src/vite-plugin/modules/plugin.ts`:
- Around line 148-149: The code calls
createRoot(moduleContainerForBuildUseOnly).render(...) but the existing guard
only checks for a module-level import of 'react-dom/client' (the conditional
around line 171) and not whether the createRoot symbol is actually imported;
update the import-injection logic so that when a file imports from
'react-dom/client' you also inspect its specifiers and if createRoot is not
among them, inject an explicit import { createRoot } from 'react-dom/client' (or
otherwise ensure a locally-declared createRoot binding exists) before emitting
the createRoot(...) call—make this change around the same logic that currently
detects 'react-dom/client' imports so createRoot is guaranteed to be defined at
the createRoot(moduleContainerForBuildUseOnly).render(...) site.
In `@scripts/test-matrix-local.sh`:
- Around line 99-101: The matrix loop in scripts/test-matrix-local.sh only
updates type packages (the pnpm --filter `@yext/pages` up
"@types/react@${REACT_TYPES_VERSION}"
"@types/react-dom@${REACT_DOM_TYPES_VERSION}") but not the actual React runtime,
causing builds to always use a single installed React version; update the loop
to also bump the runtime packages by including "react@${REACT_VERSION}" and
"react-dom@${REACT_DOM_VERSION}" (or align the existing REACT_TYPES_VERSION vars
to runtime vars) so that the pnpm up command updates both types and runtime for
each matrix iteration (modify the pnpm --filter `@yext/pages` up invocation in
scripts/test-matrix-local.sh accordingly).
---
Outside diff comments:
In
`@packages/pages/src/common/src/template/internal/getTemplateFilepaths.test.ts`:
- Around line 51-67: The test for default client template is nondeterministic
because it relies on the real filesystem; update the test to mock
util.existsSync to always return false so the code path chooses the default
instead of a real file. In the test where util.globSync is mocked and
getGlobalClientServerRenderTemplates is called, add vi.spyOn(util,
"existsSync").mockImplementation(() => false) before invoking
getGlobalClientServerRenderTemplates (and restore the spy after the test if
necessary) to ensure clientRenderTemplatePath resolves to the default in a
deterministic way.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
.github/workflows/build.yml.github/workflows/unit_test.ymlpackage.jsonpackages/pages/package.jsonpackages/pages/src/common/src/module/internal/_client17.tsxpackages/pages/src/common/src/module/internal/getModuleFilepaths.tspackages/pages/src/common/src/template/internal/_client17.tsxpackages/pages/src/common/src/template/internal/getTemplateFilepaths.test.tspackages/pages/src/common/src/template/internal/getTemplateFilepaths.tspackages/pages/src/common/src/template/internal/util.tspackages/pages/src/vite-plugin/modules/plugin.tsplayground/multibrand-site/package.jsonscripts/test-matrix-local.sh
💤 Files with no reviewable changes (4)
- packages/pages/src/common/src/template/internal/_client17.tsx
- packages/pages/src/common/src/module/internal/_client17.tsx
- .github/workflows/unit_test.yml
- packages/pages/src/common/src/template/internal/util.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/build.yml (1)
36-50: Consider quoting the version variables to prevent potential shell word splitting.The version variables are unquoted in the
pnpm upcommand. While these specific values are safe, quoting them is a defensive practice.♻️ Suggested improvement
pnpm --filter `@yext/pages` up \ - `@types/react`@${REACT_TYPES_VERSION} \ - `@types/react-dom`@${REACT_DOM_TYPES_VERSION} + "@types/react@${REACT_TYPES_VERSION}" \ + "@types/react-dom@${REACT_DOM_TYPES_VERSION}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 36 - 50, The shell command uses unquoted variables REACT_TYPES_VERSION and REACT_DOM_TYPES_VERSION in the pnpm --filter `@yext/pages` up invocation which can risk word-splitting; update the command that calls pnpm (the pnpm --filter `@yext/pages` up line) to use the quoted forms of those variables so the arguments are passed as single tokens (quote the variable expansions for REACT_TYPES_VERSION and REACT_DOM_TYPES_VERSION).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/build.yml:
- Around line 36-50: The shell command uses unquoted variables
REACT_TYPES_VERSION and REACT_DOM_TYPES_VERSION in the pnpm --filter `@yext/pages`
up invocation which can risk word-splitting; update the command that calls pnpm
(the pnpm --filter `@yext/pages` up line) to use the quoted forms of those
variables so the arguments are passed as single tokens (quote the variable
expansions for REACT_TYPES_VERSION and REACT_DOM_TYPES_VERSION).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/build.yml.github/workflows/build_and_test_pages.ymlTHIRD-PARTY-NOTICESpackage.json
💤 Files with no reviewable changes (1)
- .github/workflows/build_and_test_pages.yml
|
Check if packages/pages/src/bin/loader.js is still necessary. I think so. |
|
Seems ok without |
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 36-50: The workflow step only updates `@types` packages but not
runtime React, so modify the script that sets
REACT_TYPES_VERSION/REACT_DOM_TYPES_VERSION to also set REACT_VERSION and
REACT_DOM_VERSION based on matrix.react-major (e.g., when matrix.react-major ==
"18" set REACT_VERSION and REACT_DOM_VERSION to the appropriate React 18 semver,
otherwise set them for React 19), then include "@yext/pages" runtime updates in
the pnpm command by adding "react@${REACT_VERSION}" and
"react-dom@${REACT_DOM_VERSION}" to the pnpm --filter `@yext/pages` up invocation
so both runtime and type packages are bumped during the matrix runs.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
.github/workflows/build.yml.github/workflows/playwright.ymlpackages/pages/package.jsonpackages/pages/src/bin/loader.jspackages/pages/src/bin/spawn.tspackages/pages/src/vite-plugin/modules/plugin.tsscripts/test-matrix-local.sh
💤 Files with no reviewable changes (2)
- packages/pages/src/bin/spawn.ts
- packages/pages/src/bin/loader.js
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/pages/package.json
- scripts/test-matrix-local.sh
- packages/pages/src/vite-plugin/modules/plugin.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.github/workflows/build.yml (1)
70-70: Consider consistent artifact naming for both React versions.The current naming creates asymmetry: React 19 builds include
-react-19in the artifact name, but React 18 builds don't include a version indicator. This could cause confusion when viewing artifacts.For improved clarity, consider always including the React version for pages builds:
♻️ Proposed fix for consistent naming
- name: ${{ inputs.package == 'pages' && matrix.react-major == '19' && format('build-output-{0}-react-19', matrix.node-version) || format('build-output-{0}', matrix.node-version) }} + name: ${{ inputs.package == 'pages' && format('build-output-{0}-react-1}', matrix.node-version, matrix.react-major) || format('build-output-{0}', matrix.node-version) }}This would produce
build-output-20.x-react-18andbuild-output-20.x-react-19for pages, making both versions explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml at line 70, The artifact name expression currently only appends "-react-19" when matrix.react-major == '19', causing inconsistent names; update the name expression used for the job (the line with name: ${{ ... format('build-output-{0}-react-19', matrix.node-version) || format('build-output-{0}', matrix.node-version) }}) so that pages builds always include the React major version (e.g., format('build-output-{0}-react-{1}', matrix.node-version, matrix.react-major) or equivalent), ensuring both React 18 and 19 artifacts are named like build-output-20.x-react-18 and build-output-20.x-react-19.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/build.yml:
- Line 70: The artifact name expression currently only appends "-react-19" when
matrix.react-major == '19', causing inconsistent names; update the name
expression used for the job (the line with name: ${{ ...
format('build-output-{0}-react-19', matrix.node-version) ||
format('build-output-{0}', matrix.node-version) }}) so that pages builds always
include the React major version (e.g., format('build-output-{0}-react-{1}',
matrix.node-version, matrix.react-major) or equivalent), ensuring both React 18
and 19 artifacts are named like build-output-20.x-react-18 and
build-output-20.x-react-19.
| fi | ||
|
|
||
| echo "Running Playwright in ${playground_dir#$ROOT_DIR/}" | ||
| if ! PLAYWRIGHT_HTML_OPEN=never pnpm --dir "$playground_dir" exec playwright test --reporter=line; then |
There was a problem hiding this comment.
And this also happens in the github action?
There was a problem hiding this comment.
Yep, .github/workflows/playwright.yml:40
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 `@scripts/test-matrix-local.sh`:
- Line 2: Change the shell options to fail-fast by updating the existing "set
-uo pipefail" invocation to include -e (i.e. "set -euo pipefail"), and harden
the unchecked backup/restore cp operations by adding explicit pre-checks before
any cp that restores package.json or lockfiles: verify the source backup file
exists and is non-empty (e.g., test -s /path/to/backup) and only then run cp,
otherwise exit with an error; locate the "set -uo pipefail" line and the cp
commands that copy package.json / lockfiles (the unchecked cp lines referenced
in the comment) and wrap those restore cp calls with the existence/size tests so
you never overwrite real files with missing/empty backups.
benlife5
requested review from
asanehisa,
briantstephan,
jwartofsky-yext and
mkilpatrick
3 participants
Testing