Conversation
|
@maiieul is attempting to deploy a commit to the Open Circle Team on Vercel. A member of the Team first needs to authorize it. |
dosubot
bot
added
the
size:M
|
No actionable comments were generated in the recent review. 🎉 ℹ️ 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 (15)
✅ Files skipped from review due to trivial changes (12)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughDependencies and devDependencies for Qwik and valibot were updated across multiple package.json files (several 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Pull request overview
Updates the Qwik/Vite toolchain for the website/workspaces (aiming to improve developer experience via HMR) and adjusts a couple of components to accommodate updated signal/track() typing/behavior.
Changes:
- Update Qwik deps to PR tarball builds (
pkg.pr.new) and bump related tooling (including Vite). - Simplify the Vite preview entry to use
createQwikRouter({ render })without@qwik-router-config. - Adjust
InputErrors/FormErrorto assign tracked errors via.value.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| website/src/entry.preview.tsx | Removes @qwik-router-config usage from the Vite preview adapter. |
| website/src/components/InputErrors.tsx | Changes how tracked input errors are assigned to the frozen signal. |
| website/src/components/FormError.tsx | Changes how tracked form errors are assigned to the frozen signal. |
| website/package.json | Moves Qwik deps to PR tarballs and upgrades Vite / eslint-plugin-qwik. |
| playgrounds/qwik/package.json | Moves Qwik deps to PR tarballs and upgrades eslint-plugin-qwik. |
| packages/core/package.json | Moves @qwik.dev/core devDependency to a PR tarball. |
| package.json | Removes pnpm patchedDependencies configuration. |
| frameworks/qwik/package.json | Moves @qwik.dev/core devDependency to a PR tarball; adjusts peer dep range. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -26,7 +26,7 @@ export const InputErrors = component$(({ name, errors }: InputErrorProps) => { | |||
| const timeout = setTimeout(() => (frozenError.value = null), 200); | |||
| cleanup(() => clearTimeout(timeout)); | |||
| } else { | |||
| frozenError.value = nextErrors; | |||
| frozenError.value = nextErrors.value; | |||
There was a problem hiding this comment.
track(errors) is used throughout this codebase as returning the tracked value (see e.g. useStorageSignal / useResetSignal), but here the result is treated like a signal (nextErrors.value). This makes the logic inconsistent: if track returns a value, .value will be undefined; if it returns a signal, the !nextErrors check will never be true. Consider tracking the value explicitly (e.g. track(() => errors.value)) and then use that value consistently for both the !… check and frozenError.value assignment.
| "@qwik.dev/core": "https://pkg.pr.new/QwikDev/qwik/@qwik.dev/core@8541", | ||
| "@qwik.dev/router": "https://pkg.pr.new/QwikDev/qwik/@qwik.dev/router@8541", |
There was a problem hiding this comment.
Using https://pkg.pr.new/...@8541 tarball URLs directly in devDependencies is brittle operationally (installs can break if the PR build artifacts are cleaned up or the service is unavailable) and makes long-term upgrades harder to track. If this is meant to be temporary, consider centralizing it via a root-level pnpm override/resolution and add a clear follow-up to revert back to a published semver version once the upstream fix is released.
| @@ -33,10 +33,5 @@ | |||
| "prettier-plugin-svelte": "^3.4.0", | |||
| "prettier-plugin-tailwindcss": "^0.6.14", | |||
| "typescript": "^5.8.3" | |||
There was a problem hiding this comment.
The pnpm patchedDependencies entry was removed, but patches/@qwik.dev__router@2.0.0-beta.9.patch still exists in the repo. If the patch is no longer needed, consider removing the patch file as well to avoid confusion about whether pnpm patches are still expected to be applied.
| "typescript": "^5.8.3" | |
| "typescript": "^5.8.3" | |
| }, | |
| "pnpm": { | |
| "patchedDependencies": { | |
| "@qwik.dev/router@2.0.0-beta.9": "patches/@qwik.dev__router@2.0.0-beta.9.patch" | |
| } |
| ); | ||
| cleanup(() => clearTimeout(timeout)); | ||
| } else { | ||
| frozenFormError.value = nextFormError; | ||
| frozenFormError.value = nextFormError.value; | ||
| } |
There was a problem hiding this comment.
nextFormError is treated inconsistently in this task: the if (isBrowser && !nextFormError) branch assumes it’s a nullable value, but the else assigns nextFormError.value as if it were a signal. Depending on the actual track() return type, this either breaks the condition (if it’s a signal) or breaks the assignment (if it’s a value). Refactor to track the error value explicitly (e.g. track form.errors.value) and then use that tracked value consistently for both the condition and frozenFormError.value assignment (and in the timeout callback).
| "@formisch/eslint-config": "workspace:*", | ||
| "@formisch/methods": "workspace:*", | ||
| "@qwik.dev/core": "2.0.0-beta.5", | ||
| "@qwik.dev/core": "https://pkg.pr.new/QwikDev/qwik/@qwik.dev/core@8541", |
There was a problem hiding this comment.
This switches @qwik.dev/core to a pkg.pr.new PR tarball URL. Even when pinned, this introduces an external availability risk for installs and can be hard to audit/upgrade later. If this is temporary, consider documenting the intended rollback target (published version) and/or centralizing the override at the workspace root so it doesn’t have to be duplicated across multiple package manifests.
| "@qwik.dev/core": "https://pkg.pr.new/QwikDev/qwik/@qwik.dev/core@8541", | |
| "@qwik.dev/core": "2.0.0-beta.5", |
![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
🧹 Nitpick comments (3)
packages/core/package.json (1)
73-73: PR package URL should be temporary.Using
pkg.pr.newURLs is appropriate for testing PR-specific fixes, but this should be reverted to a proper version (e.g.,2.0.0-beta.xor stable release) before merging to main or publishing the package.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/package.json` at line 73, The dependency entry for "@qwik.dev/core" in package.json is pointing to a temporary PR URL (https://pkg.pr.new/QwikDev/qwik/@qwik.dev/core@8541); change this to the appropriate stable or beta version string (e.g., "2.0.0-beta.x" or the project's canonical release version) by replacing the pkg.pr.new URL with the real version specifier in packages/core/package.json so the dependency is no longer a PR artifact.frameworks/qwik/package.json (2)
50-50: PR package URL should be reverted before publishing.Same note as
packages/core/package.json: ensure thispkg.pr.newURL is replaced with a proper version before releasing the package.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frameworks/qwik/package.json` at line 50, The dependency entry for "@qwik.dev/core" currently points to a temporary PR registry URL; before publishing, replace the "https://pkg.pr.new/QwikDev/qwik/@qwik.dev/core@8541" value with the intended published version spec (e.g., a semver like "1.2.3" or the monorepo package version string) so the package.json no longer references pkg.pr.new; update the "@qwik.dev/core" value accordingly and verify npm/yarn installs the real published package.
63-63: Consider aligning peer dependency constraints across packages.This package now requires
>=2.0.0-beta.5, whilepackages/core/package.jsonspecifies^2.0.0for the same peer dependency. The^2.0.0constraint may exclude pre-release versions depending on the package manager's semver interpretation. Consider aligning these constraints for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frameworks/qwik/package.json` at line 63, Peer dependency ranges for "@qwik.dev/core" are inconsistent between packages (frameworks/qwik package.json uses ">=2.0.0-beta.5" while packages/core/package.json uses "^2.0.0"); pick a single semver constraint and apply it to both package.json files so all packages expect the same compatible versions of "@qwik.dev/core". Update the dependency string for "@qwik.dev/core" in frameworks/qwik/package.json (or in packages/core/package.json) to match the chosen format (e.g., change ">=2.0.0-beta.5" to "^2.0.0" or vice versa) and run a quick install to verify no peer-dependency warnings remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@website/src/components/FormError.tsx`:
- Line 31: The assignment uses nextFormError (the result of track()) as if it
were a signal by accessing .value; instead, treat nextFormError as the unwrapped
value and assign it directly to frozenFormError.value. Update the code in
FormError.tsx to remove the erroneous `.value` access on nextFormError (i.e.,
set frozenFormError.value = nextFormError) and ensure any other usages in this
component that call track() are not dereferencing `.value`.
In `@website/src/components/InputErrors.tsx`:
- Line 29: The runtime error comes from treating the result of track(errors) as
a signal and accessing .value; change the assignment in InputErrors.tsx so that
frozenError.value is assigned the unwrapped value returned by track(errors)
(i.e., replace frozenError.value = nextErrors.value with frozenError.value =
nextErrors), and ensure nextErrors (the result of track(errors)) is handled for
null/undefined to match frozenError's type.
---
Nitpick comments:
In `@frameworks/qwik/package.json`:
- Line 50: The dependency entry for "@qwik.dev/core" currently points to a
temporary PR registry URL; before publishing, replace the
"https://pkg.pr.new/QwikDev/qwik/@qwik.dev/core@8541" value with the intended
published version spec (e.g., a semver like "1.2.3" or the monorepo package
version string) so the package.json no longer references pkg.pr.new; update the
"@qwik.dev/core" value accordingly and verify npm/yarn installs the real
published package.
- Line 63: Peer dependency ranges for "@qwik.dev/core" are inconsistent between
packages (frameworks/qwik package.json uses ">=2.0.0-beta.5" while
packages/core/package.json uses "^2.0.0"); pick a single semver constraint and
apply it to both package.json files so all packages expect the same compatible
versions of "@qwik.dev/core". Update the dependency string for "@qwik.dev/core"
in frameworks/qwik/package.json (or in packages/core/package.json) to match the
chosen format (e.g., change ">=2.0.0-beta.5" to "^2.0.0" or vice versa) and run
a quick install to verify no peer-dependency warnings remain.
In `@packages/core/package.json`:
- Line 73: The dependency entry for "@qwik.dev/core" in package.json is pointing
to a temporary PR URL (https://pkg.pr.new/QwikDev/qwik/@qwik.dev/core@8541);
change this to the appropriate stable or beta version string (e.g.,
"2.0.0-beta.x" or the project's canonical release version) by replacing the
pkg.pr.new URL with the real version specifier in packages/core/package.json so
the dependency is no longer a PR artifact.
🪄 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: 35381cf4-49bf-4ada-93bd-5d856b185d98
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
frameworks/qwik/package.jsonpackage.jsonpackages/core/package.jsonplaygrounds/qwik/package.jsonwebsite/package.jsonwebsite/src/components/FormError.tsxwebsite/src/components/InputErrors.tsxwebsite/src/entry.preview.tsx
💤 Files with no reviewable changes (1)
- package.json
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="website/src/components/InputErrors.tsx">
<violation number="1" location="website/src/components/InputErrors.tsx:29">
P1: `track(signal)` already returns the unwrapped value, not the signal. Accessing `.value` on the result will yield `undefined` (when `nextErrors` is an array) or throw a TypeError (when `nextErrors` is `null` during SSR). This prevents error messages from ever being displayed.
Revert to the original assignment without `.value`.</violation>
</file>
<file name="website/src/components/FormError.tsx">
<violation number="1" location="website/src/components/FormError.tsx:31">
P2: Adding `.value` here implies `track()` now returns the signal itself rather than its unwrapped value. If so, the if-branch condition `!nextFormError` two lines above is always `false` (signal objects are truthy), so the 200ms collapse-delay animation never triggers. The condition should likely be `!nextFormError.value` to preserve the freeze-while-collapsing behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
|
Will the pkg.pr.new dep of |
|
Can you check the PR review comments for me? |
dosubot
bot
added
size:XS
maiieul
force-pushed
the
update-qwik-deps
branch
from
4efc18d to
8e29022
Compare
dosubot
bot
added
size:M
|
PR comments addressed. It seems there is a little bug with
|
I can also try upgrading to Vite 8 + Rolldown QwikDev/qwik#8379 when the PR is merged. Not sure how it'll play out for the other packages in the project but I can try.