fix(intent): walk up to meta/ instead of hardcoded depth#194
Conversation
getMetaDir hardcoded ../../meta, which is correct for the src/commands/ source layout but overshoots to @tanstack/meta from the flat dist/ layout the build ships — so setup, setup-github-actions, meta, and scaffold fail with "No templates directory found" / "Meta-skills directory not found" in the published package. Walk up to the first meta/ directory so resolution works in both layouts and symlinked installs.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesgetMetaDir Resolution Fix
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/intent/src/commands/support.ts (1)
48-48: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnexplained magic number
10.The iteration limit isn't documented — a brief comment on why 10 is sufficient (e.g., deepest expected nesting) would aid future maintenance.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/src/commands/support.ts` at line 48, The loop in support.ts uses an unexplained magic number for the iteration cap, so add a brief inline comment near the for loop in the support command explaining why the chosen limit is sufficient. Keep the reference tied to the support command’s retry/nesting logic around the limit variable so future readers understand why the bound in this iteration is 10.packages/intent/tests/support.test.ts (1)
33-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo test for symlinked installs.
The PR objective explicitly calls out symlinked installs (e.g., pnpm) as a case this fix should handle, but no test exercises a symlinked module path. Consider adding a case that creates a symlink to the module directory and asserts
findMetaDirstill resolves correctly, since that scenario is the actual motivating bug fix beyond the dist/src layout difference.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/intent/tests/support.test.ts` around lines 33 - 58, Add coverage for symlinked installs in the findMetaDir tests, since the current cases only cover flat dist and deep src/commands paths. Extend support.test.ts with a scenario that creates a symlinked module directory (pnpm-like install layout) and verify findMetaDir still resolves the expected meta directory from that symlinked start path. Use the existing findMetaDir helper and seedMeta setup to keep the new case aligned with the current test structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/intent/src/commands/support.ts`:
- Around line 46-55: findMetaDir currently treats any existing path named meta
as valid, even if it is a file, which can break downstream commands that expect
a directory. Update the check in findMetaDir to verify the candidate is both
present and a directory before returning it, and keep the fallback behavior
unchanged; this affects callers like runMetaCommand, runScaffoldCommand, and
runSetupGithubActionsCommand that rely on the returned path being a real meta
folder.
---
Nitpick comments:
In `@packages/intent/src/commands/support.ts`:
- Line 48: The loop in support.ts uses an unexplained magic number for the
iteration cap, so add a brief inline comment near the for loop in the support
command explaining why the chosen limit is sufficient. Keep the reference tied
to the support command’s retry/nesting logic around the limit variable so future
readers understand why the bound in this iteration is 10.
In `@packages/intent/tests/support.test.ts`:
- Around line 33-58: Add coverage for symlinked installs in the findMetaDir
tests, since the current cases only cover flat dist and deep src/commands paths.
Extend support.test.ts with a scenario that creates a symlinked module directory
(pnpm-like install layout) and verify findMetaDir still resolves the expected
meta directory from that symlinked start path. Use the existing findMetaDir
helper and seedMeta setup to keep the new case aligned with the current test
structure.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa0b3de2-b118-42bd-9b3d-00aabe4e1830
📒 Files selected for processing (3)
.changeset/getmetadir-walk-up.mdpackages/intent/src/commands/support.tspackages/intent/tests/support.test.ts
…ray-file cases Address review: statSync(candidate).isDirectory() so a stray file named `meta` can't short-circuit the walk-up; add a comment on the iteration cap; add tests for the stray-file skip and a pnpm-style symlinked install path.
|
View your CI Pipeline Execution ↗ for commit 2891205
☁️ Nx Cloud last updated this comment at |
commit: |
LadyBluenotes
left a comment
There was a problem hiding this comment.
Thank you for fixing this!
Closes #193.
What
getMetaDir()hardcoded../../meta. That's right in source (src/commands/→ two up → package root →meta/), but the build ships the module flat indist/, where two..overshoots to@tanstack/metaandmeta/is actually one..up at@tanstack/intent/meta. Result:setup,setup-github-actions,meta, andscaffoldall fail in the published package with "No templates directory found" / "Meta-skills directory not found".Fix: walk up from the module dir to the first
meta/instead of a fixed depth. Handles both the deep source layout and the flatdist/layout, plus symlinked installs (pnpm). Falls back to the old../../metaif nothing's found, so it's never worse than before.Extracted
findMetaDir(startDir)so the resolution is unit-testable without depending onimport.meta.url.Verification
vitest run tests/support.test.ts tests/setup.test.ts→ 29 passed (newfindMetaDirtests cover the flat-dist/layout, the deepsrc/commands/layout, and the no-meta/fallback;getMetaDirresolves to the realmeta/templates/workflows/check-skills.yml).tsc --noEmitclean;eslint+prettierclean on touched files.node dist/cli.mjs meta→ lists the meta-skills (was "Meta-skills directory not found.")node dist/cli.mjs setupin a temp project →✓ Copied workflow: …/check-skills.yml+ template vars applied (was "No templates directory found.")tests/integration/source-policy-surfaces.test.ts:79) — confirmed failing on pristinemainwith this branch's changes stashed, so unrelated to this fix (it's aboutintent listsource-policy notices, a different code path).Changeset
@tanstack/intent: patch.Summary by CodeRabbit
meta/assets.meta/folder.dist/,src/layouts, fallback behavior, and symlinked installs.