fix(unplugin): Avoid generating empty routes#2642
fix(unplugin): Avoid generating empty routes#2642FrontEndDog wants to merge 9 commits intovuejs:mainfrom
Conversation
✅ Deploy Preview for vue-router canceled.
|
|
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:
📝 WalkthroughWalkthroughThis PR adds a test case for handling nested Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/router/src/unplugin/codegen/generateRouteRecords.ts (1)
22-28:⚠️ Potential issue | 🟠 MajorMissing
.filter(Boolean)for root-level children.The same empty-route scenario that's fixed at line 101 can occur here. If a root-level child node is non-matchable with no children (lines 33-35),
generateRouteRecordsreturns'', and the join would produce invalid JavaScript like[,\n{...}]or[{...},\n].Proposed fix to apply the same filter
if (node.isRoot()) { return `[ ${node .getChildrenSorted() .map(child => generateRouteRecords(child, options, importsMap, indent + 1)) + .filter(Boolean) .join(',\n')} ]` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router/src/unplugin/codegen/generateRouteRecords.ts` around lines 22 - 28, The root branch in generateRouteRecords (when node.isRoot()) maps over node.getChildrenSorted() but doesn't filter out empty return values from generateRouteRecords, so non-matchable children can produce '' and create invalid array output; update the root branch to first filter out falsy results (e.g., .filter(Boolean)) before mapping and joining (same approach used elsewhere in generateRouteRecords), keeping the rest of the call using generateRouteRecords(child, options, importsMap, indent + 1) so empty strings are excluded from the joined array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/router/src/unplugin/codegen/generateRouteRecords.ts`:
- Around line 22-28: The root branch in generateRouteRecords (when
node.isRoot()) maps over node.getChildrenSorted() but doesn't filter out empty
return values from generateRouteRecords, so non-matchable children can produce
'' and create invalid array output; update the root branch to first filter out
falsy results (e.g., .filter(Boolean)) before mapping and joining (same approach
used elsewhere in generateRouteRecords), keeping the rest of the call using
generateRouteRecords(child, options, importsMap, indent + 1) so empty strings
are excluded from the joined array.
There was a problem hiding this comment.
Pull request overview
Fixes route code generation in the router unplugin to avoid emitting “empty routes” when route nodes are deleted (e.g., via beforeWriteFiles) but empty intermediate folders remain in the route tree.
Changes:
- Filters out falsy generated child route-record strings during
childrenarray generation to prevent empty array slots.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
posva
left a comment
posva
left a comment
There was a problem hiding this comment.
Thanks for the PR!
This is probably not the right fix. Maybe the delete is leaving some unused nodes in the tree or not cleaning itself from the parent.
A unit test for the tree is also needed
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2642 +/- ##
==========================================
+ Coverage 85.58% 85.59% +0.01%
==========================================
Files 86 86
Lines 9960 9967 +7
Branches 2285 2288 +3
==========================================
+ Hits 8524 8531 +7
Misses 1423 1423
Partials 13 13 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Additional supplements unplugin-vue-router@0.19.2 does not have this issue. These codes were newly added in vue-router@5 
I will attempt to repair it using the correct method. |
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 `@packages/router/src/unplugin/codegen/generateRouteRecords.ts`:
- Around line 22-28: The current loop in generateRouteRecords.ts mutates
node.children during forEach (child.delete()) and won't remove parents that
become empty after their children are pruned; change to a safe post-order
pruning: traverse children first (recursively prune each child), collect
children to delete (using child.isMatchable() and child.children.size === 0
checks after recursive pruning), then delete them in a separate pass using
child.delete(); alternatively implement a separate cleanup pass (pruneNode) that
returns whether a node should be deleted so parent callers can remove now-empty
non-matchable nodes. Ensure references to node.children, child.isMatchable(),
and child.delete() are used in the new post-order logic.
| // delete lone children nodes - they only provide layout wrapping for children | ||
| // so without children they don't make sense to be included in the route records | ||
| node.children.forEach(child => { | ||
| if (!child.isMatchable() && child.children.size === 0) { | ||
| child.delete() | ||
| } | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the full context of the file and the changes
head -100 packages/router/src/unplugin/codegen/generateRouteRecords.ts | cat -nRepository: vuejs/router
Length of output: 3625
🏁 Script executed:
# Find the TreeNode class to understand the delete() method
find . -type f -name "*.ts" -o -name "*.tsx" | head -20Repository: vuejs/router
Length of output: 1011
🏁 Script executed:
# Search for TreeNode class definition and delete() method
rg "class TreeNode" -A 20Repository: vuejs/router
Length of output: 5609
🏁 Script executed:
# Search for the delete() method specifically
rg "delete\(\)" -B 3 -A 8 packages/router/src/Repository: vuejs/router
Length of output: 5112
🏁 Script executed:
# Let's examine the full delete() method and surrounding code
sed -n '80,95p' packages/router/src/unplugin/core/tree.ts | cat -nRepository: vuejs/router
Length of output: 718
🏁 Script executed:
# Check if there are any tests or comments about this behavior
rg "forEach.*delete" packages/router/src/ -B 3 -A 3Repository: vuejs/router
Length of output: 38
🏁 Script executed:
# Look for any post-processing or cleanup passes that might handle cascade pruning
rg "cascad|cleanup|post" packages/router/src/unplugin/ -iRepository: vuejs/router
Length of output: 2085
🏁 Script executed:
# Let's verify the full delete() method context and see the Map behavior
sed -n '60,100p' packages/router/src/unplugin/core/tree.ts | cat -nRepository: vuejs/router
Length of output: 1648
🏁 Script executed:
# Check how generateRouteRecords is actually called and if there's a higher-level cleanup
rg "generateRouteRecords" packages/router/src/ -B 2 -A 2Repository: vuejs/router
Length of output: 4860
🏁 Script executed:
# Look at getChildrenSorted to see what happens after delete
rg "getChildrenSorted" packages/router/src/unplugin/core/tree.ts -A 10Repository: vuejs/router
Length of output: 367
🏁 Script executed:
# Let's check the test file to see if there are test cases for the pruning behavior
cat packages/router/src/unplugin/codegen/generateRouteRecords.spec.ts | head -150Repository: vuejs/router
Length of output: 5053
🏁 Script executed:
# Look for test snapshots to understand expected behavior
fd "generateRouteRecords.spec.ts.snap" packages/router/Repository: vuejs/router
Length of output: 140
🏁 Script executed:
# Check if there are any tests specifically for non-matchable nodes or layout wrappers
rg "isMatchable|layout|wrapper" packages/router/src/unplugin/codegen/generateRouteRecords.spec.ts -B 2 -A 5Repository: vuejs/router
Length of output: 38
🏁 Script executed:
# Let's find the isMatchable method to understand what makes a node matchable
rg "isMatchable" packages/router/src/unplugin/core/tree.ts -A 5Repository: vuejs/router
Length of output: 273
🏁 Script executed:
# Let's look at the snapshot file to see what behavior is expected
head -200 packages/router/src/unplugin/codegen/__snapshots__/generateRouteRecords.spec.ts.snapRepository: vuejs/router
Length of output: 3829
🏁 Script executed:
# Check what happens with groups (parentheses in filenames) since they might be non-matchable
rg "group|Group" packages/router/src/unplugin/core/tree.ts -B 2 -A 2Repository: vuejs/router
Length of output: 459
🏁 Script executed:
# Let's create a test to verify if delete() during forEach on a Map causes issues
python3 << 'EOF'
# Test Map.forEach behavior when deleting during iteration
test_map = {}
class MockNode:
def __init__(self, name, parent=None):
self.name = name
self.parent = parent
self.deleted = False
def delete(self):
if self.parent:
del self.parent[self.name]
self.deleted = True
# Simulate the scenario
parent_map = {}
for i in range(5):
parent_map[f'child{i}'] = MockNode(f'child{i}', parent_map)
print("Initial children:", list(parent_map.keys()))
# Simulate forEach with delete (Python dict iteration is equivalent to Map.forEach)
to_delete = []
for key, child in list(parent_map.items()):
if key in ['child0', 'child2', 'child4']: # Non-matchable with no children
to_delete.append(key)
print("Nodes to delete:", to_delete)
# Delete in iteration (problematic)
print("\n--- Problematic: Delete during iteration ---")
test_map1 = {f'child{i}': MockNode(f'child{i}', test_map1 if i == 0 else None) for i in range(5)}
# Can't easily simulate this in Python, but in JS this causes issues
# Safe approach (recommended)
print("\n--- Safe: Collect first, delete after ---")
test_map2 = {f'child{i}': MockNode(f'child{i}') for i in range(5)}
to_delete_safe = [k for k in test_map2.keys() if k in ['child0', 'child2', 'child4']]
for key in to_delete_safe:
del test_map2[key]
print("Final children:", list(test_map2.keys()))
EOFRepository: vuejs/router
Length of output: 420
🏁 Script executed:
# Look at the actual git history/blame for this code to see if it was recently added
git log --oneline -20 packages/router/src/unplugin/codegen/generateRouteRecords.ts 2>/dev/null | head -5Repository: vuejs/router
Length of output: 136
🏁 Script executed:
# Check if there are any bug reports or issues related to this pruning logic
rg "delete.*lone|prune|layout.*wrap" packages/router/ -i --type-listRepository: vuejs/router
Length of output: 445
🏁 Script executed:
# Get more details about the recent commit that added this pruning logic
git show a07b7fe --statRepository: vuejs/router
Length of output: 37058
🏁 Script executed:
# Look at the full commit message and diff
git show a07b7feRepository: vuejs/router
Length of output: 50372
Unsafe iteration and incomplete tree pruning during node deletion.
This code has two issues:
-
Modifying a Map during forEach iteration: Calling
child.delete()while iterating overnode.childrenwithforEachmodifies the parent's Map concurrently, which can cause entries to be skipped. Collect nodes to delete first, then delete in a separate pass. -
Cascade pruning doesn't clean parent nodes: This only removes direct leaf nodes. If a parent becomes childless after its children are pruned, it won't be deleted because the parent-level pruning check already occurred before recursing. Example: If
A → B → C(all non-matchable), then C gets deleted when processing B, but B itself won't be removed since it had children when checked at A's level.
Safe iteration pattern
// delete lone children nodes - they only provide layout wrapping for children
// so without children they don't make sense to be included in the route records
- node.children.forEach(child => {
- if (!child.isMatchable() && child.children.size === 0) {
- child.delete()
- }
- })
+ const toDelete: TreeNode[] = []
+ node.children.forEach(child => {
+ if (!child.isMatchable() && child.children.size === 0) {
+ toDelete.push(child)
+ }
+ })
+ toDelete.forEach(child => child.delete())The second issue likely requires a different architectural approach—consider post-order traversal pruning or a separate cleanup pass after code generation.
📝 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.
| // delete lone children nodes - they only provide layout wrapping for children | |
| // so without children they don't make sense to be included in the route records | |
| node.children.forEach(child => { | |
| if (!child.isMatchable() && child.children.size === 0) { | |
| child.delete() | |
| } | |
| }) | |
| // delete lone children nodes - they only provide layout wrapping for children | |
| // so without children they don't make sense to be included in the route records | |
| const toDelete: TreeNode[] = [] | |
| node.children.forEach(child => { | |
| if (!child.isMatchable() && child.children.size === 0) { | |
| toDelete.push(child) | |
| } | |
| }) | |
| toDelete.forEach(child => child.delete()) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/router/src/unplugin/codegen/generateRouteRecords.ts` around lines 22
- 28, The current loop in generateRouteRecords.ts mutates node.children during
forEach (child.delete()) and won't remove parents that become empty after their
children are pruned; change to a safe post-order pruning: traverse children
first (recursively prune each child), collect children to delete (using
child.isMatchable() and child.children.size === 0 checks after recursive
pruning), then delete them in a separate pass using child.delete();
alternatively implement a separate cleanup pass (pruneNode) that returns whether
a node should be deleted so parent callers can remove now-empty non-matchable
nodes. Ensure references to node.children, child.isMatchable(), and
child.delete() are used in the new post-order logic.
|
I have revised the logic for skipping lone parent nodes and added unit test. |
|
I discovered an issue during subsequent testing, if the file structure is |
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 `@packages/router/src/unplugin/core/tree.ts`:
- Around line 212-216: The cascade delete can climb to the root and trigger the
"Cannot delete the root node" error; modify the condition around the recursive
delete (the block using this.parent.isMatchable(), this.parent.children.size and
calling this.parent.delete()) to guard against deleting the root—e.g. check that
this.parent is not the root (use whatever root predicate exists or ensure
this.parent.parent !== undefined) before calling this.parent.delete(); update
the condition so the root node is never passed to delete().
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| it('skips nested lone _parent files', () => { | ||
| const tree = new PrefixTree(DEFAULT_OPTIONS) | ||
| tree.insert('users/index', 'users/index.vue') | ||
| tree.insert('users/settings/_parent', 'users/settings/_parent.vue') | ||
|
|
||
| const routes = generateRouteRecordSimple(tree) | ||
|
|
||
| expect(routes).toContain("path: '/users'") | ||
| expect(routes).not.toContain("path: '/users/settings'") | ||
| }) |
There was a problem hiding this comment.
While this test verifies that route generation correctly skips nested lone _parent files, it doesn't test the dynamic deletion scenario that the bug fix addresses. Consider adding a test in tree.spec.ts that inserts a parent node with children, then deletes the children (using node.delete()), and verifies that the parent node is also removed. This would directly test the recursive cleanup logic added to the delete() method.
There was a problem hiding this comment.
The comment is relevant
Thank for you review, done!
There was a problem hiding this comment.
I'm sorry, my comment wasn't clear: you don't necessarily need to remove your test, it's great to have an e2e test like that but it still need the deletion part to get closer to the scenario you were describing with a deletion. IIRC there are EditableTree tests, which should feel closer to your actual scenario
| it('skips nested lone _parent files', () => { | ||
| const tree = new PrefixTree(DEFAULT_OPTIONS) | ||
| tree.insert('users/index', 'users/index.vue') | ||
| tree.insert('users/settings/_parent', 'users/settings/_parent.vue') | ||
|
|
||
| const routes = generateRouteRecordSimple(tree) | ||
|
|
||
| expect(routes).toContain("path: '/users'") | ||
| expect(routes).not.toContain("path: '/users/settings'") | ||
| }) |
…ble node is deleted
|
Just a moment. I've found some other issues. I'll fix them. |
|
Hi @posva ,I find a new problem. router 
When I try to delete route start with a capital letter in beforeWriteFiles, an error is reported. |
Maybe this line of code should be deleted or modify the judgment logic of the root node When deleting |
…hable node is deleted
|
I have already dealt with all the issues. |
|
Hi @posva I have already dealt with all the issues. Could you spare some time to review this PR. |
fix #2641
Summary by CodeRabbit
Release Notes
Tests
Bug Fixes