-
Notifications
You must be signed in to change notification settings - Fork 46
[FTF-461] Preserve import/export in code blocks during markdown transpilation #3126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThe transpiler's import/export removal logic is being refactored from a regex-based approach to a line-by-line stateful parser that only removes top-of-file import/export statements while preserving similar content inside code blocks. Test coverage is expanded to validate the refined behavior across various scenarios including multi-line imports, blank lines, and code block preservation. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes The changes introduce a significantly more complex stateful parser replacing simpler regex logic, requiring careful review of brace-tracking and multi-line statement handling. The expanded test suite aids verification but the implementation logic density and state management increase review complexity. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@data/onPostBuild/transpileMdxToMarkdown.ts`:
- Around line 61-81: The current multi-line handling resets inMultiLineStatement
for non-function exports on seeing any semicolon, which breaks arrow-function
exports; update the logic so that any export/export-like statement that contains
an opening brace starts tracking braceDepth (similar to the 'export-function'
branch) and only exits when braceDepth returns to 0, i.e., change the else
branch handling of inMultiLineStatement (and the code that sets
inMultiLineStatement to 'export') to initialize and update braceDepth when a '{'
is present and avoid treating semicolons inside braces as the end; preserve the
existing braceDepth increment/decrement logic and ensure inMultiLineStatement
transitions to 'none' only when braceDepth === 0.
🧹 Nitpick comments (2)
data/onPostBuild/transpileMdxToMarkdown.ts (1)
56-59: Blank lines are stripped from the import/export section.All blank lines encountered while in the top import/export section are skipped and not added to the result. This means the output loses any blank line that appeared between the last import/export and the first content line.
If this is intentional for cleaner output, consider adding a single blank line after the removal phase to maintain separation. Otherwise, the current behavior is acceptable if downstream processing handles spacing.
data/onPostBuild/transpileMdxToMarkdown.test.ts (1)
156-164: Consider adding a test for multi-line arrow function exports.The current test covers single-line
export function, but multi-line arrow function exports have a bug in the implementation. Adding a test case would help catch this:it('should handle multi-line arrow function exports', () => { const input = `export const foo = () => { return 'bar'; }; ## Content`; const output = removeImportExportStatements(input); expect(output).toContain('## Content'); expect(output).not.toContain('export'); expect(output).not.toContain('return'); });This test would currently fail due to the implementation issue flagged in the other file.
097c0f0 to
4b5a9e8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Updates the MDX→Markdown post-build transpilation to avoid stripping import/export statements that appear inside code examples, by limiting removal to the initial import/export section at the top of the MDX body.
Changes:
- Replaced the regex-based
removeImportExportStatements()with a line-by-line parser that stops after the first non-import/export line. - Added new unit tests covering code-block preservation and several multi-line import/export patterns.
- Updated the fixture MDX and snapshot output to reflect the new behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| data/onPostBuild/transpileMdxToMarkdown.ts | Rewrites removeImportExportStatements() to only remove top-of-body import/export statements and preserve later content. |
| data/onPostBuild/transpileMdxToMarkdown.test.ts | Adds test coverage for preserving code-sample import/exports and multi-line statement handling. |
| data/onPostBuild/snapshots/transpileMdxToMarkdown.test.ts.snap | Updates snapshot due to changed whitespace/output from the new removal logic. |
| data/onPostBuild/fixtures/input.mdx | Extends fixture with an additional exported arrow function to validate multi-line export stripping. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace regex-based import/export removal with line-by-line parser that only removes import/export statements from the top of MDX files. This preserves code samples containing legitimate import/export statements. The previous regex approach used /gm flags that matched import/export at the start of any line in the file, breaking JavaScript/TypeScript code examples. Changes: - Rewrote removeImportExportStatements() to use state machine approach - Only processes import/export at top of file (after frontmatter) - Tracks multi-line statements with brace depth counting - Added test cases for code sample preservation Fixes #3034 Co-Authored-By: Claude Sonnet 4.5 (1M context) <noreply@anthropic.com>
4b5a9e8 to
d0efbe9
Compare
m-hulbert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested various pages where this occurred and LGTM thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from copilot comment, changes look good to me ( tested changes on the local )
PS. Asking copilot to fix anything will result in creation of new PR : \
Yeah, its a funny thing this Copilot, I thought I could just prompt it to review again like we do with Abbot, didn't expect a PR ¯\_(ツ)_/¯ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
PS. You can double check new copilot review comments though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
Fixes #3034
The previous implementation of
removeImportExportStatements()used aggressive regex patterns with/gmflags that matched import/export statements at the start of any line in the file, including inside code blocks. This broke JavaScript/TypeScript code examples that legitimately contain import/export statements.This PR replaces the regex-based approach with a line-by-line parser that:
Implementation
The new implementation uses a state machine that:
Manual testing
Compare the output of the following URLs and you'll see a marked improvement in the MD output, as well as the issue of the missing prose being addressed (both from #3034)
Testing
Files Changed
data/onPostBuild/transpileMdxToMarkdown.ts- RewroteremoveImportExportStatements()functiondata/onPostBuild/transpileMdxToMarkdown.test.ts- Added new test casesSummary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.