Skip to content

fix(convert): resolve directory pruning failure on Windows due to pat…#148

Merged
KayleeWilliams merged 3 commits into
inthhq:mainfrom
Adityakk9031:#146
Jul 15, 2026
Merged

fix(convert): resolve directory pruning failure on Windows due to pat…#148
KayleeWilliams merged 3 commits into
inthhq:mainfrom
Adityakk9031:#146

Conversation

@Adityakk9031

@Adityakk9031 Adityakk9031 commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

close:#146

Description

During output pruning (convertAllMdx --prune), empty parent directories left behind by deleted markdown files were not being deleted on Windows. This was caused by a path separator mismatch between glob output paths (which use /) and resolved output directories (which use \).

This PR normalizes all paths to platform-specific format using path.resolve() before performing prefix/directory matching.

Cause

In pruneOrphanedOutputs, the directory sweep checks if a parent directory is inside outDir using:

const resolvedOutDir = resolve(outDir); // uses backslashes on Windows
// ...
while (dir !== resolvedOutDir && dir.startsWith(resolvedOutDir + sep)) {

@coderabbitai

coderabbitai Bot commented Jul 12, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: c8ea3b76-0c0f-43b2-9241-31b37d16dd94

📥 Commits

Reviewing files that changed from the base of the PR and between a89492a and 3311f25.

📒 Files selected for processing (3)
  • .changeset/fuzzy-dodos-prune.md
  • packages/leadtype/src/convert/convert.test.ts
  • packages/leadtype/src/convert/convert.ts
📜 Recent review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{ts,tsx}: Use explicit types for function parameters and return values when they enhance clarity
Prefer unknown over any when the type is genuinely unknown
Use const assertions (as const) for immutable values and literal types
Leverage TypeScript's type narrowing instead of type assertions

Files:

  • packages/leadtype/src/convert/convert.test.ts
  • packages/leadtype/src/convert/convert.ts
**/*.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{js,ts,jsx,tsx}: Use meaningful variable names instead of magic numbers - extract constants with descriptive names
Use arrow functions for callbacks and short functions
Prefer for...of loops over .forEach() and indexed for loops
Use optional chaining (?.) and nullish coalescing (??) for safer property access
Prefer template literals over string concatenation
Use destructuring for object and array assignments
Use const by default, let only when reassignment is needed, never var
Always await promises in async functions - don't forget to use the return value
Use async/await syntax instead of promise chains for better readability
Handle errors appropriately in async code with try-catch blocks
Don't use async functions as Promise executors
Remove console.log, debugger, and alert statements from production code
Throw Error objects with descriptive messages, not strings or other values
Use try-catch blocks meaningfully - don't catch errors just to rethrow them
Prefer early returns over nested conditionals for error cases
Extract complex conditions into well-named boolean variables
Use early returns to reduce nesting
Prefer simple conditionals over nested ternary operators
Don't use eval() or assign directly to document.cookie
Avoid spread syntax in accumulators within loops
Use top-level regex literals instead of creating them in loops
Prefer specific imports over namespace imports
Use descriptive names for functions, variables, and types for meaningful naming
Add comments for complex logic, but prefer self-documenting code

Files:

  • packages/leadtype/src/convert/convert.test.ts
  • packages/leadtype/src/convert/convert.ts
**/*.{test,spec}.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

**/*.{test,spec}.{js,ts,jsx,tsx}: Write assertions inside it() or test() blocks
Avoid done callbacks in async tests - use async/await instead
Don't use .only or .skip in committed code
Keep test suites reasonably flat - avoid excessive describe nesting

Files:

  • packages/leadtype/src/convert/convert.test.ts
🧠 Learnings (1)
📚 Learning: 2026-06-09T18:30:08.038Z
Learnt from: KayleeWilliams
Repo: inthhq/leadtype PR: 97
File: .changeset/search-prototype-safety-and-scaling.md:5-5
Timestamp: 2026-06-09T18:30:08.038Z
Learning: In this repo, `.changeset/*.md` files must not start the body with an H1/first-line heading (`#`) immediately after the YAML frontmatter. The changesets tool inlines the body as bullet entries into `CHANGELOG.md` during release, and a leading `#` heading would break the generated changelog format. As a result, MD041 (`first-line-heading`) warnings for files under `.changeset/` are expected false positives and should be ignored.

Applied to files:

  • .changeset/fuzzy-dodos-prune.md
🪛 ast-grep (0.44.1)
packages/leadtype/src/convert/convert.test.ts

[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').

(detect-child-process-typescript)


[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').

(detect-child-process-typescript)

packages/leadtype/src/convert/convert.ts

[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').

(detect-child-process-typescript)


[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFile } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').

(detect-child-process-typescript)

🪛 markdownlint-cli2 (0.23.0)
.changeset/fuzzy-dodos-prune.md

[warning] 5-5: First line in a file should be a top-level heading

(MD041, first-line-heading, first-line-h1)

🔇 Additional comments (4)
packages/leadtype/src/convert/convert.ts (2)

1272-1278: LGTM!


1290-1304: LGTM!

packages/leadtype/src/convert/convert.test.ts (1)

86-97: LGTM!

.changeset/fuzzy-dodos-prune.md (1)

1-6: LGTM!


📝 Walkthrough

Summary by CodeRabbit

  • Bug Fixes

    • Fixed Windows path handling during MDX conversion, ensuring orphaned directories are pruned correctly.
    • Improved consistency when processing paths with mixed slash formats.
  • Tests

    • Added coverage for Windows-style path normalization and directory pruning behavior.

Walkthrough

convertAllMdx() now normalizes orphaned file paths before pruning parent directories, with an injectable path API, Windows-specific regression coverage, and a patch changeset.

Changes

Windows prune path normalization

Layer / File(s) Summary
Prune parent path resolver
packages/leadtype/src/convert/convert.ts
Adds exported resolvePruneParentDirectories, which normalizes orphan paths and returns unique parent directories.
Pruning integration and regression coverage
packages/leadtype/src/convert/convert.ts, packages/leadtype/src/convert/convert.test.ts, .changeset/fuzzy-dodos-prune.md
Uses the resolver during orphan cleanup, tests mixed Windows path separators, and records a patch release.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

Possibly related PRs

  • inthhq/leadtype#119 — Modifies the same convertAllMdx pruning area with cache-driven stale-output cleanup.
  • inthhq/leadtype#125 — Introduces the pruning feature whose path resolution is corrected here.

Suggested reviewers: kayleewilliams

Poem

I hopped through paths with slash and dash,
Then cleared the stale files in a flash.
Windows trails now line up right,
Parent folders vanish out of sight.
A patch blooms softly—what a bash!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly matches the main change: fixing Windows directory pruning in convert.
Description check ✅ Passed The description directly explains the Windows pruning bug and the path-normalization fix.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — a Windows-only fix in pruneOrphanedOutputs so that empty parent directories left by pruned .md files are actually removed.

  • Normalize parent paths before the prefix checkorphans are produced by fast-glob, which returns POSIX forward-slash paths on every platform, while resolve(outDir) returns backslash paths on Windows. The old dir.startsWith(resolvedOutDir + sep) guard therefore never matched on Windows, so empty directories were never swept. Wrapping the parent derivation (resolve(dirname(filePath)) and the loop step resolve(dirname(dir))) normalizes to platform-native separators, restoring correct behavior.

The change is minimal and correct. One harmless redundancy worth noting for a future pass: after the first loop iteration dir is already resolved and dirname of a native path stays native, so the loop-step resolve() is a no-op — not worth changing on its own.

Pullfrog  | View workflow run | Using Claude Opus𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — a one-line cleanup in pruneOrphanedOutputs that removes the redundant loop-step resolve() flagged by the prior review.

  • Drop redundant resolve() in the directory-sweep loop — the loop step is now dir = dirname(dir) instead of dir = resolve(dirname(dir)). Since the parents set is seeded with resolve(dirname(filePath)), dir starts as a platform-native path and dirname of a native path stays native, so the removed resolve() was a no-op. Behavior is unchanged and the Windows prefix check still holds.

Pullfrog  | View workflow run | Using Claude Opus𝕏

@coderabbitai
coderabbitai Bot requested a review from KayleeWilliams July 15, 2026 10:07

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes — the delta since the prior pullfrog review (ea63aa0) adds test coverage and a changeset for the Windows directory-pruning fix.

  • Extracted resolvePruneParentDirectories seam — parent-dir derivation moved out of pruneOrphanedOutputs into an exported helper that accepts an injectable PrunePathApi ({ dirname, resolve }), defaulting to the platform node:path bindings. Production behavior is unchanged.
  • Added a Windows-path test — injects path.win32 and asserts that a POSIX (C:/…) and a backslash (C:\…) orphan dedupe to the single native parent C:\repo\public\guides. This exercises the separator normalization on Linux CI, addressing the "no test" gap noted in the prior review.
  • Added changesetleadtype patch describing the Windows pruning fix.

The refactor preserves the original fix's behavior, the test is deterministic on Linux (drive-letter inputs are absolute under path.win32, so resolve never consults cwd), and the injected PrunePathApi is a reasonable seam for cross-platform path testing.

Pullfrog  | View workflow run | Using Claude Opus𝕏

@KayleeWilliams
KayleeWilliams merged commit 646ae9f into inthhq:main Jul 15, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants