fix: restrict guard type and tighten typed state for router.go()#27
Merged
fix: restrict guard type and tighten typed state for router.go()#27
Conversation
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.
|
Caution Review failedPull request was closed or merged during review 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 selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a new PR automation skill for creating release and doc PRs via Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Repo as Local Repo (filesystem)
participant Git as Git
participant GH as GitHub (gh CLI)
participant Files as Docs/Package files
Dev->>Repo: run PR skill
Repo->>Git: inspect commits & diff vs main
alt no commits to PR
Git-->>Repo: exit early
else commits present
Git->>Files: detect uncommitted src/ changes
Files-->>Git: auto-commit docs or src per guidance
Git->>Files: read packages/*/package.json for version bumps
Files-->>Git: read/update CHANGELOG.md and READMEs
Git->>Git: reorder commits if release bump (version-bump as tip)
Git->>GH: push branch (maybe --force-with-lease)
GH->>GH: create PR with templated title/body
GH-->>Dev: return PR URL
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ 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: 1
🧹 Nitpick comments (1)
.claude/skills/pr/SKILL.md (1)
109-123: Git reorder logic lacks error handling and edge-case guards.The cherry-pick reorder sequence can fail silently or leave the branch in an inconsistent state:
- Line 116: If
VERSION_COMMITcontains regex metacharacters (unlikely but possible with commit hashes),grep -vcould misbehave. Usinggrep -Fxvwould be safer.- Line 120: If
OTHER_COMMITSis empty (version bump is the only commit),xargs -ris correct on GNU systems but-ris not POSIX and won't work on macOS. Consider adding a check.- Lines 119-122: No error handling—if any cherry-pick fails (e.g., conflicts), the branch is left in a broken state with no recovery path.
Consider adding explicit checks and error trapping:
Suggested improvements
# Rebuild branch on top of main with version bump last git checkout -B ${CURRENT_BRANCH}_reorder main -echo "$OTHER_COMMITS" | xargs -r git cherry-pick +if [ -n "$OTHER_COMMITS" ]; then + echo "$OTHER_COMMITS" | xargs git cherry-pick || { echo "Cherry-pick failed"; exit 1; } +fi git cherry-pick $VERSION_COMMIT git branch -M ${CURRENT_BRANCH}_reorder $CURRENT_BRANCH🤖 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 109 - 123, The reorder script around VERSION_COMMIT/OTHER_COMMITS and CURRENT_BRANCH lacks portability and error recovery; update it to (1) compute OTHER_COMMITS using grep -Fxv to avoid regex issues with VERSION_COMMIT, (2) detect an empty OTHER_COMMITS and skip the xargs/cherry-pick step instead of relying on GNU xargs -r, (3) wrap the rebuild sequence that uses git checkout -B, git cherry-pick and final git branch -M in a safe trap/try sequence that on any error runs git cherry-pick --abort (if in progress) and resets the branch (e.g., git reset --hard or git checkout back to CURRENT_BRANCH) to avoid leaving the repo in a conflicted state, and (4) surface non-zero exit codes so callers know when reorder failed. Reference the variables/functions VERSION_COMMIT, OTHER_COMMITS, CURRENT_BRANCH and the git commands git checkout -B, git cherry-pick, git branch -M when applying these changes.
🤖 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-19: The git diff command currently targets a non-existent root
src/ directory (`git diff main...HEAD -- src/`), so change that invocation to
target the actual package source directories by replacing `src/` with
`packages/esroute/src/ packages/esroute-lit/src/` (i.e., use `git diff
main...HEAD -- packages/esroute/src/ packages/esroute-lit/src/`) so source
changes in the monorepo are correctly shown.
---
Nitpick comments:
In @.claude/skills/pr/SKILL.md:
- Around line 109-123: The reorder script around VERSION_COMMIT/OTHER_COMMITS
and CURRENT_BRANCH lacks portability and error recovery; update it to (1)
compute OTHER_COMMITS using grep -Fxv to avoid regex issues with VERSION_COMMIT,
(2) detect an empty OTHER_COMMITS and skip the xargs/cherry-pick step instead of
relying on GNU xargs -r, (3) wrap the rebuild sequence that uses git checkout
-B, git cherry-pick and final git branch -M in a safe trap/try sequence that on
any error runs git cherry-pick --abort (if in progress) and resets the branch
(e.g., git reset --hard or git checkout back to CURRENT_BRANCH) to avoid leaving
the repo in a conflicted state, and (4) surface non-zero exit codes so callers
know when reorder failed. Reference the variables/functions VERSION_COMMIT,
OTHER_COMMITS, CURRENT_BRANCH and the git commands git checkout -B, git
cherry-pick, git branch -M when applying these changes.
🪄 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: 9be90e05-67e6-47ed-8cff-49d05a961b52
📒 Files selected for processing (4)
.claude/skills/pr/SKILL.mdCHANGELOG.mdpackages/esroute/src/router.spec.tspackages/esroute/src/router.ts
Comment on lines
+17
to
+19
| - `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 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the source directory structure
fd -t d -d 2 'src' | head -20Repository: sv2dev/esroute
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Explore the overall structure of the repo
tree -L 2 -d --dirsfirst 2>/dev/null || find . -maxdepth 2 -type d -not -path '*/\.*' | sort | head -30Repository: sv2dev/esroute
Length of output: 123
🏁 Script executed:
#!/bin/bash
# Read the SKILL.md file to see the actual commands
cat .claude/skills/pr/SKILL.mdRepository: sv2dev/esroute
Length of output: 5953
🏁 Script executed:
#!/bin/bash
# Check if packages/esroute/src exists
fd -t d 'src' packages/ 2>/dev/null || echo "No src found in packages"Repository: sv2dev/esroute
Length of output: 105
Fix the diff command to target the actual source directories in the monorepo.
Line 18 uses git diff main...HEAD -- src/ but source files are in packages/esroute/src/ and packages/esroute-lit/src/. The root-level src/ directory does not exist, so this diff will always be empty even when source code changes. Update the command to:
git diff main...HEAD -- packages/esroute/src/ packages/esroute-lit/src/
🤖 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 - 19, The git diff command
currently targets a non-existent root src/ directory (`git diff main...HEAD --
src/`), so change that invocation to target the actual package source
directories by replacing `src/` with `packages/esroute/src/
packages/esroute-lit/src/` (i.e., use `git diff main...HEAD --
packages/esroute/src/ packages/esroute-lit/src/`) so source changes in the
monorepo are correctly shown.
sv2dev
force-pushed
the
fix/guard-type-restriction
branch
from
5bec992 to
8793c4b
Compare
Merged
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
"?"guard key inRoutesto function type only, preventing accidental assignment of a sub-routes object as a guardrouter.go()to omit thestateoption entirely when the target route handler declares no state typeChanges
Routes["?"]is now typed asResolve<T, S> | undefinedinstead ofRoutes<T, S> | Resolve<T, S>, closing the type hole that allowed objects to be assigned as guardsrouter.go()overload for routes without a required state now usesOmit<NavMeta<...>, "state">so that passingstateis a compile-time errorroute-resolver.tsafter the virtual route lookup to handleundefinedroute values safelyvisibilitychangelistener moved fromwindowtodocument(included via the merged bundle-size improvements)Changelog
Fixed
"?"is now restricted to function type only, preventing accidental assignment of sub-routes objects (closes Route guard properties should not only allow a guard function #10)router.go()no longer exposes thestateoption for routes where no state type is declared, preventing accidental state passing on untyped routesSummary by CodeRabbit
Bug Fixes
Tests
Documentation