Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 20 minutes and 3 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
WalkthroughA version bump (0.12.1 → 0.12.3) with CHANGELOG documentation, ESM package metadata additions (exports, types fields), explicit Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 4
🧹 Nitpick comments (1)
packages/esroute/src/scroll-restoration.ts (1)
36-36: Gatevisibilitychangesaves to hidden state only.Right now
saveruns on both visible/hidden transitions. Restricting todocument.visibilityState === "hidden"avoids unnecessaryhistory.replaceStatechurn.♻️ Suggested change
- document.addEventListener("visibilitychange", save); + document.addEventListener("visibilitychange", () => { + if (document.visibilityState === "hidden") save(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/esroute/src/scroll-restoration.ts` at line 36, The current listener calls save on every visibilitychange; update the handler so it only runs when document.visibilityState === "hidden" to avoid unnecessary history.replaceState churn—replace the direct listener registration (document.addEventListener("visibilitychange", save)) with a small wrapper that checks document.visibilityState === "hidden" and then calls save (referencing the existing save function) so only hidden-state transitions trigger the save.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/pr/SKILL.md:
- Around line 17-24: Update the monorepo source checks in SKILL.md so they cover
package-level source paths, not just root src/: replace or extend uses of the
`git diff main...HEAD -- src/` and the uncommitted-change detection/auto-commit
step that only reference `src/` to also include package directories (e.g.,
`packages/*/src/` or `packages/**/src/`) and any other repository source
locations; ensure the logic that checks for uncommitted changes (the auto-commit
via the commit skill) and the “what changed” diffs (`git diff ...`) use the
expanded glob patterns so package changes aren’t skipped.
- Around line 27-33: The release detection currently only checks whether
package.json files were modified (the git log invocation in SKILL.md) and may
misclassify PRs; update the git log invocation to search commits that
specifically touched the "version" field by using git log with a grep-by-diff
(-G) regex that matches the "version" JSON key so only commits that changed the
version line in packages/esroute/package.json or
packages/esroute-lit/package.json mark the PR as a release.
In `@CHANGELOG.md`:
- Line 99: The breaking-change line for createRouter is malformed due to an
extra trailing parenthesis; update the changelog entry so it reads:
"**Breaking**: `createRouter` is now a factory function instead of a class — use
`createRouter({ routes, ... })` instead of `new Router(...)`" (remove the stray
trailing `)` at the end), ensuring the text references createRouter and Router
exactly as shown.
In `@packages/esroute-lit/package.json`:
- Around line 5-13: The patch added "type": "module" causing a breaking change
for CommonJS consumers; fix by either releasing as a major version bump or by
restoring CJS compatibility: build a CommonJS bundle (e.g., dist/index.cjs) and
adjust the package.json exports to provide both "import" and "require"
conditions and ensure "main" or "exports" points to the CJS entry for require
users; update the "types" mapping if needed so dist/index.d.ts still matches
both builds and remove or document "type": "module" if you choose the dual-build
approach.
---
Nitpick comments:
In `@packages/esroute/src/scroll-restoration.ts`:
- Line 36: The current listener calls save on every visibilitychange; update the
handler so it only runs when document.visibilityState === "hidden" to avoid
unnecessary history.replaceState churn—replace the direct listener registration
(document.addEventListener("visibilitychange", save)) with a small wrapper that
checks document.visibilityState === "hidden" and then calls save (referencing
the existing save function) so only hidden-state transitions trigger the save.
🪄 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: 08fa9f74-76f4-4fff-94bb-2cc27464a0e0
📒 Files selected for processing (15)
.claude/skills/pr/SKILL.mdCHANGELOG.mdpackages/esroute-lit/package.jsonpackages/esroute-lit/src/index.tspackages/esroute-lit/src/render-routes-directive.spec.tspackages/esroute/package.jsonpackages/esroute/src/index.tspackages/esroute/src/nav-opts.spec.tspackages/esroute/src/route-resolver.spec.tspackages/esroute/src/route-resolver.tspackages/esroute/src/router.spec.tspackages/esroute/src/router.tspackages/esroute/src/routes.tspackages/esroute/src/scroll-restoration.tstsconfig.json
.claude/skills/pr/SKILL.md
Outdated
| - `git log main..HEAD --oneline` — commits on this branch | ||
| - `git diff main...HEAD -- src/` — what changed in source | ||
| - `git diff main...HEAD -- CHANGELOG.md README.md packages/esroute/README.md packages/esroute-lit/README.md` — what docs already changed | ||
|
|
||
| If `git log main..HEAD` is empty, tell the user there's nothing to PR and stop. | ||
|
|
||
| If there are uncommitted changes in `src/` (shown by `git status`), commit them automatically using the commit skill before proceeding — they're part of the work being PR'd. | ||
|
|
There was a problem hiding this comment.
Monorepo source checks are too narrow (src/ only).
Line 18 and Line 23 only target root src/, but this repository’s source changes are primarily under package paths. That can skip intended PR/docs behavior.
🛠️ Suggested fix
-- `git diff main...HEAD -- src/` — what changed in source
+- `git diff main...HEAD -- packages/esroute/src/ packages/esroute-lit/src/` — what changed in source
...
-If there are uncommitted changes in `src/` (shown by `git status`), commit them automatically using the commit skill before proceeding — they're part of the work being PR'd.
+If there are uncommitted changes in `packages/esroute/src/` or `packages/esroute-lit/src/` (shown by `git status`), commit them automatically using the commit skill before proceeding — they're part of the work being PR'd.📝 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.
| - `git log main..HEAD --oneline` — commits on this branch | |
| - `git diff main...HEAD -- src/` — what changed in source | |
| - `git diff main...HEAD -- CHANGELOG.md README.md packages/esroute/README.md packages/esroute-lit/README.md` — what docs already changed | |
| If `git log main..HEAD` is empty, tell the user there's nothing to PR and stop. | |
| If there are uncommitted changes in `src/` (shown by `git status`), commit them automatically using the commit skill before proceeding — they're part of the work being PR'd. | |
| - `git log main..HEAD --oneline` — commits on this branch | |
| - `git diff main...HEAD -- packages/esroute/src/ packages/esroute-lit/src/` — what changed in source | |
| - `git diff main...HEAD -- CHANGELOG.md README.md packages/esroute/README.md packages/esroute-lit/README.md` — what docs already changed | |
| If `git log main..HEAD` is empty, tell the user there's nothing to PR and stop. | |
| If there are uncommitted changes in `packages/esroute/src/` or `packages/esroute-lit/src/` (shown by `git status`), commit them automatically using the commit skill before proceeding — they're part of the work being PR'd. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/pr/SKILL.md around lines 17 - 24, Update the monorepo source
checks in SKILL.md so they cover package-level source paths, not just root src/:
replace or extend uses of the `git diff main...HEAD -- src/` and the
uncommitted-change detection/auto-commit step that only reference `src/` to also
include package directories (e.g., `packages/*/src/` or `packages/**/src/`) and
any other repository source locations; ensure the logic that checks for
uncommitted changes (the auto-commit via the commit skill) and the “what
changed” diffs (`git diff ...`) use the expanded glob patterns so package
changes aren’t skipped.
.claude/skills/pr/SKILL.md
Outdated
| Check whether any commit on this branch touched a `package.json` version field: | ||
|
|
||
| ```bash | ||
| git log main..HEAD --oneline --diff-filter=M -- packages/esroute/package.json packages/esroute-lit/package.json | ||
| ``` | ||
|
|
||
| If this returns output, this is a **release PR**. Read the new version from `packages/esroute/package.json`. |
There was a problem hiding this comment.
Release detection does not actually validate version field changes.
The command on Line 30 only checks whether those files were modified, not whether "version" changed. This can incorrectly classify non-release PRs as releases.
🛠️ Suggested fix
-Check whether any commit on this branch touched a `package.json` version field:
+Check whether any commit on this branch changed a `package.json` `version` field:
```bash
-git log main..HEAD --oneline --diff-filter=M -- packages/esroute/package.json packages/esroute-lit/package.json
+git log main..HEAD --oneline --diff-filter=M -G '^\s*"version"\s*:' -- packages/esroute/package.json packages/esroute-lit/package.json</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/pr/SKILL.md around lines 27 - 33, The release detection
currently only checks whether package.json files were modified (the git log
invocation in SKILL.md) and may misclassify PRs; update the git log invocation
to search commits that specifically touched the "version" field by using git log
with a grep-by-diff (-G) regex that matches the "version" JSON key so only
commits that changed the version line in packages/esroute/package.json or
packages/esroute-lit/package.json mark the PR as a release.
</details>
<!-- fingerprinting:phantom:poseidon:hawk:429bf112-f3c6-4bab-be03-2e2f510ce663 -->
<!-- This is an auto-generated comment by CodeRabbit -->
| ## [0.5.0] - 2022-09-18 | ||
|
|
||
| ### Changed | ||
| - **Breaking**: `createRouter` is now a factory function instead of a class — use `createRouter({ routes, ... })` instead of `new Router(...)`) |
There was a problem hiding this comment.
Fix malformed markdown in breaking-change entry.
Line 99 has an extra trailing ) which renders awkwardly in the changelog text.
✏️ Suggested fix
-- **Breaking**: `createRouter` is now a factory function instead of a class — use `createRouter({ routes, ... })` instead of `new Router(...)`)
+- **Breaking**: `createRouter` is now a factory function instead of a class — use `createRouter({ routes, ... })` instead of `new Router(...)`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` at line 99, The breaking-change line for createRouter is
malformed due to an extra trailing parenthesis; update the changelog entry so it
reads: "**Breaking**: `createRouter` is now a factory function instead of a
class — use `createRouter({ routes, ... })` instead of `new Router(...)`"
(remove the stray trailing `)` at the end), ensuring the text references
createRouter and Router exactly as shown.
| "type": "module", | ||
| "types": "dist/index.d.ts", | ||
| "main": "dist/index.js", | ||
| "exports": { | ||
| ".": { | ||
| "types": "./dist/index.d.ts", | ||
| "default": "./dist/index.js" | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Compare published metadata to validate whether CJS compatibility changed.
for v in 0.12.2 0.12.3; do
echo "=== `@esroute/lit`@$v ==="
curl -s "https://registry.npmjs.org/@esroute/lit/$v" | jq '{
version,
type,
main,
exports
}'
doneRepository: sv2dev/esroute
Length of output: 286
🏁 Script executed:
cat packages/esroute-lit/package.json | head -20Repository: sv2dev/esroute
Length of output: 644
🏁 Script executed:
#!/bin/bash
# Find what version is in the current package.json
version=$(grep -oP '"version"\s*:\s*"\K[^"]+' packages/esroute-lit/package.json)
echo "Current version in repo: $version"
# Query NPM for versions around this one
npm_response=$(curl -s "https://registry.npmjs.org/@esroute/lit" | jq -r '.versions | keys | .[-5:]' 2>/dev/null)
echo "Recent versions on NPM:"
echo "$npm_response"Repository: sv2dev/esroute
Length of output: 171
Patch release introduces breaking module-format change that should be a major version bump.
Adding "type": "module" without providing a "require" export condition breaks CommonJS consumers. Version 0.12.2 had no "type" field (treating .js files as CommonJS), while 0.12.3 now treats them as ESM-only. This violates semver for a patch release.
Either (a) release this as 1.0.0+ (breaking change), or (b) provide dual exports with corresponding CommonJS build artifacts:
"exports": {
".": {
"import": "./dist/index.js",
"require": "./dist/index.cjs"
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/esroute-lit/package.json` around lines 5 - 13, The patch added
"type": "module" causing a breaking change for CommonJS consumers; fix by either
releasing as a major version bump or by restoring CJS compatibility: build a
CommonJS bundle (e.g., dist/index.cjs) and adjust the package.json exports to
provide both "import" and "require" conditions and ensure "main" or "exports"
points to the CJS entry for require users; update the "types" mapping if needed
so dist/index.d.ts still matches both builds and remove or document "type":
"module" if you choose the dual-build approach.
Prevent passing state to router.go() for routes where no state type is declared. Previously, the optional NavMeta still exposed the state field, allowing accidental state usage on untyped routes.
Configure packages as ES modules with proper exports field, update TypeScript compiler options for Node16 module resolution, and add .js file extensions to internal imports for ESM compatibility.
sv2dev
force-pushed
the
fix/guard-type-restriction
branch
from
2f486ab to
24b4a63
Compare
1 participant
Summary
Changes
router.go()now omits thestateoption from the type signature when the route handler declares no state type, preventing accidental state passing on untyped routes"?"is restricted to function type only (released in 0.12.2, included here as this branch tracks both fixes)Changelog
[0.12.3] - 2026-04-02
Fixed
router.go()no longer exposes thestateoption for routes where no state type is declared, preventing accidental state passing on untyped routesSummary by CodeRabbit
New Features
Bug Fixes
"?"behavior to correctly reject invalid configurations.router.go()state exposure rules to prevent invalid state parameters on stateless routes.Documentation
Chores