-
Notifications
You must be signed in to change notification settings - Fork 4k
Fix markdownPageURL for PageActions #3702
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
🦋 Changeset detectedLatest commit: 9c4b254 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
030ef38
to
9c4b254
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
This PR fixes an issue with the markdownPageURL
for PageActions by ensuring the URL has a .md
extension when passed to the PageActions component, but removes it when sending to LLM providers to avoid bad requests.
- Add
.md
extension tomarkdownPageURL
in PageHeader component - Strip
.md
extension from URLs before sending to LLM providers in PageActions - Add changeset documenting the fix
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
File | Description |
---|---|
packages/gitbook/src/components/PageBody/PageHeader.tsx | Adds .md extension to the markdown page URL |
packages/gitbook/src/components/PageActions/PageActions.tsx | Removes .md extension from URL before passing to LLM providers |
.changeset/eight-eels-sort.md | Documents the fix in changeset |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Summary of the deployments:
Test content
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
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.
A bit weird to pass it then remove it because the logic to compute the markdown URL is now duplicated but let's fix it.
@gregberge yeah I know but it's either we add it for copy and view, or remove it once for LLMs |
The good way to fix it is to pass urls property with html / markdown property and use the correct one. Or better to compute it in the component where it's actually needed (because of RSC payload). |
we can't compute in the component because we can't pass context from server to client, let's merge this first and I'll update to use |
No description provided.