Conversation
|
View your CI Pipeline Execution ↗ for commit 6b75094
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Caution
Revert all changes to this file before merging
| // TODO/FIXME/TEMP/DEBUG: never merge this to next. | ||
| 'preact-vite/prerelease-ts', |
There was a problem hiding this comment.
Caution
Revert this before merging
📝 WalkthroughWalkthroughAdds Preact 11 prerelease compatibility and a new preact-vite prerelease template, introduces a template-level preferNoLink flag and a forceLink option, centralizes link/no-link decision in sandbox task via sanitizeOptions, updates E2E version detection, and adjusts the sandboxes CI workflow to target branch preact-11 and disable generate-main. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
📝 Coding Plan
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)
code/e2e-tests/addon-docs.spec.ts (1)
206-217:⚠️ Potential issue | 🟠 MajorLogic error:
preact-vite/defaultcheck will incorrectly matchpreact-vite/default-jsandpreact-vite/default-ts.The condition on line 206 (
templateName.includes('preact-vite/default')) will match before lines 212-213 can be evaluated, because'preact-vite/default-js'.includes('preact-vite/default')istrue. This meanspreact-vite/default-jsandpreact-vite/default-tswill incorrectly expect React 16 instead of React 18.The more specific checks (
preact-vite/default-js,preact-vite/default-ts) must come before the genericpreact-vite/defaultcheck, or use exact matching.Proposed fix: reorder conditions
} else if (templateName.includes('react16')) { expectedReactVersionRange = /^16/; - } else if (templateName.includes('preact-vite/default')) { - expectedReactVersionRange = /^16/; - } else if (templateName.includes('preact-vite/prerelease')) { - expectedReactVersionRange = /^18/; } else if ( templateName.includes('internal/react18-webpack-babel') || templateName.includes('preact-vite/default-js') || templateName.includes('preact-vite/default-ts') || templateName.includes('react-webpack/18-ts') ) { expectedReactVersionRange = /^18/; + } else if (templateName.includes('preact-vite/prerelease')) { + expectedReactVersionRange = /^18/; }Alternatively, if there's truly a
preact-vite/defaulttemplate (without-jsor-tssuffix) that expects React 16, you could remove the redundantpreact-vite/defaultcheck entirely since it doesn't appear in the template definitions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/e2e-tests/addon-docs.spec.ts` around lines 206 - 217, The templateName.includes('preact-vite/default') check is too broad and will match 'preact-vite/default-js' and 'preact-vite/default-ts' before their specific checks, causing expectedReactVersionRange to be set to /^16/ incorrectly; fix by reordering the conditions so the more specific checks (templateName.includes('preact-vite/default-js') and templateName.includes('preact-vite/default-ts')) appear before the generic templateName.includes('preact-vite/default') check, or replace the generic includes check with an exact match (=== 'preact-vite/default') and ensure the branches that set expectedReactVersionRange to /^18/ remain for the specific templates.
🧹 Nitpick comments (2)
code/frameworks/preact-vite/package.json (1)
61-61: Peer dependency range is redundant but shows intentional Preact 11 prerelease support.The range
>=10already includes all versions>= 11.0.0-0, so the additional|| >= 11.0.0-0clause is technically redundant. However, it does make the prerelease support explicit. Consider simplifying to just>=10if the explicit signal isn't needed, or aligning with the pattern incode/renderers/preact/package.jsonwhich uses caret ranges (^10.0.0 || >= 11.0.0-0).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/frameworks/preact-vite/package.json` at line 61, The peerDependency declaration for "preact" is redundant: the current range ">=10 || >= 11.0.0-0" can be simplified; update the "preact" peer dependency in package.json (the "preact" entry) to either a single range ">=10" to remove redundancy or to match the project's pattern used in code/renderers/preact/package.json (e.g., use a caret-style range like "^10.0.0 || >= 11.0.0-0") depending on whether you want to keep explicit prerelease support.scripts/tasks/sandbox.ts (1)
22-37: Side-effect mutation pattern and inconsistent template reference.The function mutates
options.linkdirectly without returning a value, which is a side-effect pattern that can be confusing. Additionally, the log messages referenceoptions.template(the key string) rather thandetails.template.namefor better readability.Consider either:
- Returning a new options object instead of mutating, or
- At minimum, documenting that this function mutates its input
Minor improvement: use template name in logs
const sanitizeOptions = (details: TemplateDetails, options: PassedOptionValues) => { if (options.link && !options.forceLink && details.template.inDevelopment) { logger.log( - `The ${options.template} has inDevelopment property enabled, therefore the sandbox for that template cannot be linked. Enabling --no-link mode... Pass --force-link to use link mode anyway, but be aware the sandbox may partially or completely not work.` + `The ${details.key} template has inDevelopment property enabled, therefore the sandbox for that template cannot be linked. Enabling --no-link mode... Pass --force-link to use link mode anyway, but be aware the sandbox may partially or completely not work.` ); options.link = false; } if (options.link && !options.forceLink && details.template.preferNoLink) { logger.log( - `The ${options.template} has preferNoLink property enabled. Defaulting to --no-link mode. Pass --force-link to use link mode anyway, but be aware the sandbox may partially or completely not work.` + `The ${details.key} template has preferNoLink property enabled. Defaulting to --no-link mode. Pass --force-link to use link mode anyway, but be aware the sandbox may partially or completely not work.` ); options.link = false; } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/tasks/sandbox.ts` around lines 22 - 37, sanitizeOptions currently mutates options.link and logs the template key; change it to be pure: have sanitizeOptions(details: TemplateDetails, options: PassedOptionValues) return a new PassedOptionValues (create newOptions = { ...options } and set newOptions.link = false when needed) instead of mutating the input, update all call sites to use the returned value, and update the logger messages to reference details.template.name (not options.template) for clearer output; keep type annotations consistent with PassedOptionValues/TemplateDetails and adjust any callers expecting void.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/lib/cli-storybook/src/sandbox-templates.ts`:
- Around line 769-774: The jq expression inside the template script (the
multi-line string assigned to the script property in sandbox-templates.ts)
contains an invalid trailing comma in the object literal; update the jq fragment
to remove the trailing comma so the JSON is valid (e.g., change the object
{"preact": "npm:preact@beta",} to {"preact": "npm:preact@beta"}) and keep the
rest of the shell pipeline (jq ... > tmp.json && mv tmp.json package.json)
unchanged.
In `@scripts/tasks/sandbox.ts`:
- Around line 45-48: The dependsOn implementation is incorrectly doing an async
file-exists check (pathExists) and thus evaluates a Promise instead of boolean;
because Task.dependsOn must be synchronous, move any filesystem checks into the
ready() function (which accepts MaybePromise<boolean>) or return a static
dependency list from dependsOn. Concretely: in the function that currently
contains "if ('inDevelopment' in details.template &&
details.template.inDevelopment) { if (pathExists(join(SANDBOX_DIRECTORY,
details.key))) { return ['run-registry']; } }" remove the async pathExists call
from dependsOn and instead either (A) have dependsOn unconditionally return the
static prerequisites (e.g., [] or ['run-registry'] if appropriate), and perform
the pathExists(join(SANDBOX_DIRECTORY, details.key)) check inside the ready()
method (awaiting it there) to gate readiness, or (B) keep dependsOn purely
synchronous and shift the conditional that depends on pathExists into ready();
also fix the similar missing-await at the ready() implementation around line 77
by awaiting pathExists there.
---
Outside diff comments:
In `@code/e2e-tests/addon-docs.spec.ts`:
- Around line 206-217: The templateName.includes('preact-vite/default') check is
too broad and will match 'preact-vite/default-js' and 'preact-vite/default-ts'
before their specific checks, causing expectedReactVersionRange to be set to
/^16/ incorrectly; fix by reordering the conditions so the more specific checks
(templateName.includes('preact-vite/default-js') and
templateName.includes('preact-vite/default-ts')) appear before the generic
templateName.includes('preact-vite/default') check, or replace the generic
includes check with an exact match (=== 'preact-vite/default') and ensure the
branches that set expectedReactVersionRange to /^18/ remain for the specific
templates.
---
Nitpick comments:
In `@code/frameworks/preact-vite/package.json`:
- Line 61: The peerDependency declaration for "preact" is redundant: the current
range ">=10 || >= 11.0.0-0" can be simplified; update the "preact" peer
dependency in package.json (the "preact" entry) to either a single range ">=10"
to remove redundancy or to match the project's pattern used in
code/renderers/preact/package.json (e.g., use a caret-style range like "^10.0.0
|| >= 11.0.0-0") depending on whether you want to keep explicit prerelease
support.
In `@scripts/tasks/sandbox.ts`:
- Around line 22-37: sanitizeOptions currently mutates options.link and logs the
template key; change it to be pure: have sanitizeOptions(details:
TemplateDetails, options: PassedOptionValues) return a new PassedOptionValues
(create newOptions = { ...options } and set newOptions.link = false when needed)
instead of mutating the input, update all call sites to use the returned value,
and update the logger messages to reference details.template.name (not
options.template) for clearer output; keep type annotations consistent with
PassedOptionValues/TemplateDetails and adjust any callers expecting void.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
.github/workflows/generate-sandboxes.ymlcode/e2e-tests/addon-docs.spec.tscode/frameworks/preact-vite/package.jsoncode/lib/cli-storybook/src/sandbox-templates.tscode/renderers/preact/package.jsonscripts/task.tsscripts/tasks/sandbox.ts
| script: ` | ||
| npm create vite --yes {{beforeDir}} -- --template preact-ts && \ | ||
| cd {{beforeDir}} && \ | ||
| jq '.resolutions += {"preact": "npm:preact@beta",}' package.json > tmp.json && mv tmp.json package.json && \ | ||
| yarn add preact@beta | ||
| `, |
There was a problem hiding this comment.
JSON syntax error: trailing comma in jq command will cause script failure.
The jq command on line 772 has a trailing comma after the value which is invalid JSON syntax:
"preact": "npm:preact@beta",}
This will cause the template generation script to fail.
Proposed fix
script: `
npm create vite --yes {{beforeDir}} -- --template preact-ts && \
cd {{beforeDir}} && \
- jq '.resolutions += {"preact": "npm:preact@beta",}' package.json > tmp.json && mv tmp.json package.json && \
+ jq '.resolutions += {"preact": "npm:preact@beta"}' package.json > tmp.json && mv tmp.json package.json && \
yarn add preact@beta
`,📝 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.
| script: ` | |
| npm create vite --yes {{beforeDir}} -- --template preact-ts && \ | |
| cd {{beforeDir}} && \ | |
| jq '.resolutions += {"preact": "npm:preact@beta",}' package.json > tmp.json && mv tmp.json package.json && \ | |
| yarn add preact@beta | |
| `, | |
| script: ` | |
| npm create vite --yes {{beforeDir}} -- --template preact-ts && \ | |
| cd {{beforeDir}} && \ | |
| jq '.resolutions += {"preact": "npm:preact@beta"}' package.json > tmp.json && mv tmp.json package.json && \ | |
| yarn add preact@beta | |
| `, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/lib/cli-storybook/src/sandbox-templates.ts` around lines 769 - 774, The
jq expression inside the template script (the multi-line string assigned to the
script property in sandbox-templates.ts) contains an invalid trailing comma in
the object literal; update the jq fragment to remove the trailing comma so the
JSON is valid (e.g., change the object {"preact": "npm:preact@beta",} to
{"preact": "npm:preact@beta"}) and keep the rest of the shell pipeline (jq ... >
tmp.json && mv tmp.json package.json) unchanged.
| if ('inDevelopment' in details.template && details.template.inDevelopment) { | ||
| if (pathExists(join(SANDBOX_DIRECTORY, details.key))) { | ||
| return ['run-registry']; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find and examine the pathExists function definition
rg -n "pathExists" -t ts -t tsx --no-heading | head -20Repository: storybookjs/storybook
Length of output: 92
🏁 Script executed:
#!/bin/bash
# Check the Task type definition to understand dependsOn signature
fd "task" -t ts | head -10Repository: storybookjs/storybook
Length of output: 234
🏁 Script executed:
#!/bin/bash
# Look at the sandbox.ts file around the mentioned lines
cat -n scripts/tasks/sandbox.ts | sed -n '40,55p'Repository: storybookjs/storybook
Length of output: 682
🏁 Script executed:
#!/bin/bash
# Find pathExists function definition
rg -n "export.*pathExists|function pathExists|const pathExists" --type-add 'ts:*.ts' --type-add 'tsx:*.tsx' -t ts -t tsxRepository: storybookjs/storybook
Length of output: 1030
🏁 Script executed:
#!/bin/bash
# Search for Task type definition
rg -n "type Task|interface Task" --type-add 'ts:*.ts' --type-add 'tsx:*.tsx' -t ts -t tsxRepository: storybookjs/storybook
Length of output: 959
🏁 Script executed:
#!/bin/bash
# Look for how dependsOn is invoked/used
rg -n "dependsOn\(" --type-add 'ts:*.ts' --type-add 'tsx:*.tsx' -t ts -t tsx -B 2 -A 2Repository: storybookjs/storybook
Length of output: 383
🏁 Script executed:
#!/bin/bash
# Read the Task type definition
cat -n scripts/task.ts | sed -n '57,90p'Repository: storybookjs/storybook
Length of output: 1535
🏁 Script executed:
#!/bin/bash
# Check line 13 of sandbox.ts to confirm pathExists import/definition
cat -n scripts/tasks/sandbox.ts | sed -n '1,20p'Repository: storybookjs/storybook
Length of output: 737
🏁 Script executed:
#!/bin/bash
# Read the invocation context more thoroughly
cat -n scripts/task.ts | sed -n '270,285p'Repository: storybookjs/storybook
Length of output: 635
🏁 Script executed:
#!/bin/bash
# Check if there are other dependsOn functions and how they handle async
rg -n "dependsOn:" scripts/tasks/ -A 5 | head -60Repository: storybookjs/storybook
Length of output: 3259
🏁 Script executed:
#!/bin/bash
# Check if any dependsOn functions are async
rg -n "dependsOn:.*async\|dependsOn.*=.*async" scripts/tasks/Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Look for synchronous alternatives or how pathExists is used elsewhere
rg -n "pathExists" scripts/tasks/sandbox.ts -B 2 -A 2Repository: storybookjs/storybook
Length of output: 608
Cannot make dependsOn async—restructure the logic instead.
The missing await on pathExists at line 46 is confirmed as a bug. However, the proposed fix of making dependsOn async is incorrect. The Task type definition (scripts/task.ts:69) explicitly requires dependsOn to be a synchronous function returning TaskKey[], not a Promise. The invocation at scripts/task.ts:277 calls it without awaiting.
The condition at line 46 currently evaluates the Promise object itself, which is always truthy.
Move the path existence check into the ready() function (which supports MaybePromise<boolean>), or restructure dependsOn to return a static set of prerequisites without conditional async checks.
Note: The same missing await bug exists at line 77 in the ready() function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/tasks/sandbox.ts` around lines 45 - 48, The dependsOn implementation
is incorrectly doing an async file-exists check (pathExists) and thus evaluates
a Promise instead of boolean; because Task.dependsOn must be synchronous, move
any filesystem checks into the ready() function (which accepts
MaybePromise<boolean>) or return a static dependency list from dependsOn.
Concretely: in the function that currently contains "if ('inDevelopment' in
details.template && details.template.inDevelopment) { if
(pathExists(join(SANDBOX_DIRECTORY, details.key))) { return ['run-registry']; }
}" remove the async pathExists call from dependsOn and instead either (A) have
dependsOn unconditionally return the static prerequisites (e.g., [] or
['run-registry'] if appropriate), and perform the
pathExists(join(SANDBOX_DIRECTORY, details.key)) check inside the ready() method
(awaiting it there) to gate readiness, or (B) keep dependsOn purely synchronous
and shift the conditional that depends on pathExists into ready(); also fix the
similar missing-await at the ready() implementation around line 77 by awaiting
pathExists there.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
code/lib/cli-storybook/src/sandbox-templates.ts (1)
779-783:⚠️ Potential issue | 🔴 CriticalFix invalid
jqobject syntax in the prerelease Preact script.Line 782 uses
{"preact": "npm:preact@beta",}(trailing comma), which causesjqparse failure and breaks template generation.Proposed fix
- jq '.resolutions += {"preact": "npm:preact@beta",}' package.json > tmp.json && mv tmp.json package.json && \ + jq '.resolutions += {"preact": "npm:preact@beta"}' package.json > tmp.json && mv tmp.json package.json && \Verification (expect 1 hit before fix, 0 hits after fix):
#!/bin/bash set -euo pipefail FILE="code/lib/cli-storybook/src/sandbox-templates.ts" rg -nF '{"preact": "npm:preact@beta",}' "$FILE" sed -n '779,784p' "$FILE"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/lib/cli-storybook/src/sandbox-templates.ts` around lines 779 - 783, In sandbox-templates.ts update the script string where the jq expression '.resolutions += {"preact": "npm:preact@beta",}' is used (the script property around the Vite/preact template) to remove the trailing comma from the object literal so the jq JSON is valid (i.e., change the object to '{"preact": "npm:preact@beta"}'); keep the rest of the pipeline (redirect to tmp.json and mv) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@code/lib/cli-storybook/src/sandbox-templates.ts`:
- Around line 779-783: In sandbox-templates.ts update the script string where
the jq expression '.resolutions += {"preact": "npm:preact@beta",}' is used (the
script property around the Vite/preact template) to remove the trailing comma
from the object literal so the jq JSON is valid (i.e., change the object to
'{"preact": "npm:preact@beta"}'); keep the rest of the pipeline (redirect to
tmp.json and mv) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bc95997f-0fcb-450a-bafe-877698e9d2e9
📒 Files selected for processing (3)
.github/workflows/generate-sandboxes.ymlcode/frameworks/preact-vite/package.jsoncode/lib/cli-storybook/src/sandbox-templates.ts
✅ Files skipped from review due to trivial changes (1)
- code/frameworks/preact-vite/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/generate-sandboxes.yml
Sidnioulz
moved this from Empathy Backlog
to Empathy Queue (prioritized)
in Core Team Projects
3 participants
N/A. Running preact prerelease sandbox in CI.
Summary by CodeRabbit
New Features
Tests
Refactor
Chores