-
Notifications
You must be signed in to change notification settings - Fork 46
Fix generated markdown pages to have explicit markdown links #3155
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
base: main
Are you sure you want to change the base?
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
WalkthroughA new exported utility Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 review |
✅ Actions performedReview 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: 2
🤖 Fix all issues with AI agents
In `@data/onPostBuild/transpileMdxToMarkdown.test.ts`:
- Around line 417-433: The long string literals in the tests for
convertDocsLinksToMarkdown (the consts named input in the three failing it
blocks) exceed Prettier's line length; refactor each long input assignment into
multiple concatenated string segments (e.g. const input =
'https://raw.github...' + '/more/path' ) or use short concatenation so the final
value is identical but no single source line is too long, leaving the output
assertions and convertDocsLinksToMarkdown calls unchanged.
In `@data/onPostBuild/transpileMdxToMarkdown.ts`:
- Around line 333-375: The link-rewrite in convertDocsLinksToMarkdown is using
naive string checks and can misidentify non‑Ably domains and produce trailing
'/.md'; fix by parsing the url with the URL constructor (or a safe parse
fallback) to validate the hostname is an Ably docs host (e.g.,
endsWith('ably.com') or endsWith('ably-dev.com') but exclude 'sdk.ably.com'),
then reconstruct the target by taking url.pathname (trim any trailing '/') and
appending '.md', preserving the original hash (anchor) but dropping query params
as before; update all checks in convertDocsLinksToMarkdown to operate on the
parsed URL object and return the original match if the host/path validation
fails.
🧹 Nitpick comments (1)
data/onPostBuild/transpileMdxToMarkdown.ts (1)
333-376: Consider skipping fenced code blocks during link conversion.Other transforms preserve code blocks, but this function rewrites all Markdown links, including those inside ``` fences. That can alter code examples unintentionally. Consider splitting by fenced blocks and only converting non‑code sections.
| it('should not modify sdk.ably.com links (API documentation)', () => { | ||
| const input = '[Room](https://sdk.ably.com/builds/ably/ably-chat-js/main/typedoc/interfaces/chat-js.Room.html)'; | ||
| const output = convertDocsLinksToMarkdown(input); | ||
| expect(output).toBe('[Room](https://sdk.ably.com/builds/ably/ably-chat-js/main/typedoc/interfaces/chat-js.Room.html)'); | ||
| }); | ||
|
|
||
| it('should not modify sdk.ably.com links with /docs/ in path', () => { | ||
| const input = '[Docs](https://sdk.ably.com/docs/some-path)'; | ||
| const output = convertDocsLinksToMarkdown(input); | ||
| expect(output).toBe('[Docs](https://sdk.ably.com/docs/some-path)'); | ||
| }); | ||
|
|
||
| it('should not add .md to URLs that already have a file extension (.png)', () => { | ||
| const input = '[Image](https://raw.githubusercontent.com/ably/docs/main/src/images/content/diagrams/test.png)'; | ||
| const output = convertDocsLinksToMarkdown(input); | ||
| expect(output).toBe('[Image](https://raw.githubusercontent.com/ably/docs/main/src/images/content/diagrams/test.png)'); | ||
| }); |
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.
Fix Prettier violations for long string literals.
The linter reports formatting errors on the long test strings. Reformatting to multiline assignments should clear the Prettier errors.
🧹 Proposed fix
- const input = '[Room](https://sdk.ably.com/builds/ably/ably-chat-js/main/typedoc/interfaces/chat-js.Room.html)';
+ const input =
+ '[Room](https://sdk.ably.com/builds/ably/ably-chat-js/main/typedoc/interfaces/chat-js.Room.html)';
@@
- const input = '[Image](https://raw.githubusercontent.com/ably/docs/main/src/images/content/diagrams/test.png)';
+ const input =
+ '[Image](https://raw.githubusercontent.com/ably/docs/main/src/images/content/diagrams/test.png)';📝 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.
| it('should not modify sdk.ably.com links (API documentation)', () => { | |
| const input = '[Room](https://sdk.ably.com/builds/ably/ably-chat-js/main/typedoc/interfaces/chat-js.Room.html)'; | |
| const output = convertDocsLinksToMarkdown(input); | |
| expect(output).toBe('[Room](https://sdk.ably.com/builds/ably/ably-chat-js/main/typedoc/interfaces/chat-js.Room.html)'); | |
| }); | |
| it('should not modify sdk.ably.com links with /docs/ in path', () => { | |
| const input = '[Docs](https://sdk.ably.com/docs/some-path)'; | |
| const output = convertDocsLinksToMarkdown(input); | |
| expect(output).toBe('[Docs](https://sdk.ably.com/docs/some-path)'); | |
| }); | |
| it('should not add .md to URLs that already have a file extension (.png)', () => { | |
| const input = '[Image](https://raw.githubusercontent.com/ably/docs/main/src/images/content/diagrams/test.png)'; | |
| const output = convertDocsLinksToMarkdown(input); | |
| expect(output).toBe('[Image](https://raw.githubusercontent.com/ably/docs/main/src/images/content/diagrams/test.png)'); | |
| }); | |
| it('should not modify sdk.ably.com links (API documentation)', () => { | |
| const input = | |
| '[Room](https://sdk.ably.com/builds/ably/ably-chat-js/main/typedoc/interfaces/chat-js.Room.html)'; | |
| const output = convertDocsLinksToMarkdown(input); | |
| expect(output).toBe('[Room](https://sdk.ably.com/builds/ably/ably-chat-js/main/typedoc/interfaces/chat-js.Room.html)'); | |
| }); | |
| it('should not modify sdk.ably.com links with /docs/ in path', () => { | |
| const input = '[Docs](https://sdk.ably.com/docs/some-path)'; | |
| const output = convertDocsLinksToMarkdown(input); | |
| expect(output).toBe('[Docs](https://sdk.ably.com/docs/some-path)'); | |
| }); | |
| it('should not add .md to URLs that already have a file extension (.png)', () => { | |
| const input = | |
| '[Image](https://raw.githubusercontent.com/ably/docs/main/src/images/content/diagrams/test.png)'; | |
| const output = convertDocsLinksToMarkdown(input); | |
| expect(output).toBe('[Image](https://raw.githubusercontent.com/ably/docs/main/src/images/content/diagrams/test.png)'); | |
| }); |
🧰 Tools
🪛 ESLint
[error] 420-420: Replace '[Room](https://sdk.ably.com/builds/ably/ably-chat-js/main/typedoc/interfaces/chat-js.Room.html)' with ⏎········'[Room](https://sdk.ably.com/builds/ably/ably-chat-js/main/typedoc/interfaces/chat-js.Room.html)',⏎······
(prettier/prettier)
[error] 432-432: Replace '[Image](https://raw.githubusercontent.com/ably/docs/main/src/images/content/diagrams/test.png)' with ⏎········'[Image](https://raw.githubusercontent.com/ably/docs/main/src/images/content/diagrams/test.png)',⏎······
(prettier/prettier)
🤖 Prompt for AI Agents
In `@data/onPostBuild/transpileMdxToMarkdown.test.ts` around lines 417 - 433, The
long string literals in the tests for convertDocsLinksToMarkdown (the consts
named input in the three failing it blocks) exceed Prettier's line length;
refactor each long input assignment into multiple concatenated string segments
(e.g. const input = 'https://raw.github...' + '/more/path' ) or use short
concatenation so the final value is identical but no single source line is too
long, leaving the output assertions and convertDocsLinksToMarkdown calls
unchanged.
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
This PR enhances the MDX→Markdown post-build pipeline so that Ably documentation links in generated .md files use explicit markdown links pointing to .md resources, making them friendlier for LLM consumption.
Changes:
- Introduced
convertDocsLinksToMarkdownto normalize Ably/docs/links: stripping query parameters (e.g.?lang=), preserving hash anchors, avoiding.mdon assets, and skippingsdk.ably.com. - Integrated the new link conversion step into the
transformMdxToMarkdownpipeline after relative URL expansion. - Added comprehensive unit tests for
convertDocsLinksToMarkdown, covering different domains, query parameters, anchors, nested paths, multiple links, and asset/file extensions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
data/onPostBuild/transpileMdxToMarkdown.ts |
Adds convertDocsLinksToMarkdown and wires it into the MDX→Markdown transform flow, updating stage comments and exports. |
data/onPostBuild/transpileMdxToMarkdown.test.ts |
Extends the test suite with detailed cases for the new link conversion behavior, alongside existing pipeline utility tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
1. Updated script to replace generated markdown page containing ably html doc links with corresponding markdown link 2. Added relevant unit tests for the same
bb8fd35 to
1bd5fc1
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
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
- Removed unnecessary unit tests from transpileMdxToMarkdown
2e1c3d5 to
33ca54d
Compare
|
Fix for all |
Summary by CodeRabbit
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.