Fix docgen source links for modules in nested directories - #6703
Open
leonitousconforti wants to merge 2 commits into
Open
Fix docgen source links for modules in nested directories#6703leonitousconforti wants to merge 2 commits into
leonitousconforti wants to merge 2 commits into
Conversation
🦋 Changeset detectedLatest commit: 7cc035a The changes in this PR will be included in the next version bump. This PR includes changesets to release 29 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDocgen source links now retain nested source-directory paths instead of using only file names. Parser tests construct full source paths and verify generated markdown for a nested module. ChangesDocgen source-link path handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/tools/docgen/test/Parser.test.ts (1)
154-195: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRetain coverage for modules directly under
srcDir.This updated case exercises only
src/nested/test.ts, so the unchangedsrc/test.tslink behavior is no longer asserted. Add a second case or parameterize the test for both direct and nested modules.🤖 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/tools/docgen/test/Parser.test.ts` around lines 154 - 195, Extend the source-link coverage in the test around Parser.parseModule and expectMarkdown to also parse a module directly under project.srcDir, such as src/test.ts, and assert its generated link uses the correct root-relative path. Preserve the existing nested src/nested/test.ts assertion and expected link.
🤖 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.
Nitpick comments:
In `@packages/tools/docgen/test/Parser.test.ts`:
- Around line 154-195: Extend the source-link coverage in the test around
Parser.parseModule and expectMarkdown to also parse a module directly under
project.srcDir, such as src/test.ts, and assert its generated link uses the
correct root-relative path. Preserve the existing nested src/nested/test.ts
assertion and expected link.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 212de654-d5c3-4bd0-b308-d604672bb196
📒 Files selected for processing (3)
.changeset/fix-docgen-nested-source-links.mdpackages/tools/docgen/src/Printer.tspackages/tools/docgen/test/Parser.test.ts
Contributor
Bundle Size Analysis
|
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
3 participants
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
[Source](...)links in docgen's generated markdown are built from only the file's base name:For modules in subdirectories of
srcDir(e.g.src/frida/FridaRpcClient.ts), the link drops the subdirectory and points one level too high (<srcLink>/FridaRpcClient.tsinstead of<srcLink>/frida/FridaRpcClient.ts), producing 404s.This changes the printer to use the module's path relative to
srcDir(source.path.slice(1).join("/")), which is the same conventiongetModuleMarkdownOutputPathalready uses for the generated markdown file layout. Links for modules directly undersrcDirare unchanged.Notes
makeSourcetest helper previously modeledSource.pathas[basename], which doesn't match production (file.path.split(path.sep)including thesrcDirprefix). It now derives the path from the source file's actual file path with asrcprefix, matching whatparseFileproduces.Noticed while adopting
@effect/docgen@4.0.0-beta.102in a project with nested modules: leonitousconforti/efffrida#158Summary by CodeRabbit
Bug Fixes
Tests