Skip to content
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

Fix rendering issue for text with unhandled generic Markdown directives #1489

Merged
merged 1 commit into from
Feb 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/many-knives-type.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@astrojs/starlight': patch
---

Fixes a potential text rendering issue with text containing colons.
19 changes: 19 additions & 0 deletions packages/starlight/__tests__/remark-rehype/asides.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -143,3 +143,22 @@ test('runs without locales config', async () => {
const res = await processor.render(':::note\nTest\n::');
expect(res.code.includes('aria-label=Note"'));
});

test('tranforms back unhandled text directives', async () => {
const res = await processor.render(
`This is a:test of a sentence with a text:name[content]{key=val} directive.`
);
expect(res.code).toMatchInlineSnapshot(`
"<p>This is a:test
of a sentence with a text:name[content]{key="val"}
directive.</p>"
`);
Comment on lines +149 to +155
Copy link
Member

Choose a reason for hiding this comment

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

Noting a subtle difference here in the transformation back to text where the attribute value becomes wrapped in ". It should be extremely rare that this is an issue though, as it only applies in a relatively complex string that is a correctly formed directive, so I guess that’s fine?

Suggested change
`This is a:test of a sentence with a text:name[content]{key=val} directive.`
);
expect(res.code).toMatchInlineSnapshot(`
"<p>This is a:test
of a sentence with a text:name[content]{key="val"}
directive.</p>"
`);
`This is a:test of a sentence with a text:name[content]{key=val} directive.`
);
expect(res.code).toMatchInlineSnapshot(`
"<p>This is a:test
of a sentence with a text:name[content]{key="val"}
directive.</p>"
`);

Copy link
Member Author

Choose a reason for hiding this comment

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

Excellent catch, I totally missed that.

After a bit of digging, I found that mdast-util-directive respects options.quote altho in our case, this would not be enough to fix the issue.

I think the current approach is a good enough solution for now but if we ever hit the rare case you mentioned where we cannot restore the node to its exact original content including the quotes, we would need to revisit this. I think the 2 options would be to either:

  • Only restore the node if it's a directive with no attributes and document/emit a warning when encountering a case where we cannot restore (this seems to be the Docusaurus approach)
  • Find another way to properly restore the node (which I think would mean to have a copy of the AST before the directive was transformed)

Are you still okay with this approach?

  • If no, I can work a refactor based on the solution we would choose
  • If yes, I can merge this PR as it contains no doc changes in its current state

Copy link
Member

Choose a reason for hiding this comment

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

Yes, 100% approve — we can merge and release this as is.

If we run into this rare issue, we could also look into an alternative to remark-directive that only provides the container directive and not also text/lead directives.

});

test('tranforms back unhandled leaf directives', async () => {
const res = await processor.render(`::video[Title]{v=xxxxxxxxxxx}`);
expect(res.code).toMatchInlineSnapshot(`
"<p>::video[Title]{v="xxxxxxxxxxx"}
</p>"
`);
});
49 changes: 47 additions & 2 deletions packages/starlight/integrations/asides.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,14 @@

import type { AstroConfig, AstroUserConfig } from 'astro';
import { h as _h, s as _s, type Properties } from 'hastscript';
import type { Paragraph as P, Root } from 'mdast';
import type { Node, Paragraph as P, Parent, Root } from 'mdast';
import {
type Directives,
directiveToMarkdown,
type TextDirective,
type LeafDirective,
} from 'mdast-util-directive';
import { toMarkdown } from 'mdast-util-to-markdown';
import remarkDirective from 'remark-directive';
import type { Plugin, Transformer } from 'unified';
import { remove } from 'unist-util-remove';
Expand Down Expand Up @@ -37,6 +44,40 @@ function s(el: string, attrs: Properties = {}, children: any[] = []): P {
};
}

/** Checks if a node is a directive. */
function isNodeDirective(node: Node): node is Directives {
return (
node.type === 'textDirective' ||
node.type === 'leafDirective' ||
node.type === 'containerDirective'
);
}

/**
* Transforms back directives not handled by Starlight to avoid breaking user content.
* For example, a user might write `x:y` in the middle of a sentence, where `:y` would be
* identified as a text directive, which are not used by Starlight, and we definitely want that
* text to be rendered verbatim in the output.
*/
function transformUnhandledDirective(
node: TextDirective | LeafDirective,
index: number,
parent: Parent
) {
const textNode = {
type: 'text',
value: toMarkdown(node, { extensions: [directiveToMarkdown()] }),
} as const;
if (node.type === 'textDirective') {
parent.children[index] = textNode;
} else {
parent.children[index] = {
type: 'paragraph',
children: [textNode],
};
}
}

/**
* remark plugin that converts blocks delimited with `:::` into styled
* asides (a.k.a. “callouts”, “admonitions”, etc.). Depends on the
Expand Down Expand Up @@ -102,7 +143,11 @@ function remarkAsides(options: AsidesOptions): Plugin<[], Root> {
const locale = pathToLocale(file.history[0], options);
const t = options.useTranslations(locale);
visit(tree, (node, index, parent) => {
if (!parent || index === undefined || node.type !== 'containerDirective') {
if (!parent || index === undefined || !isNodeDirective(node)) {
return;
}
if (node.type === 'textDirective' || node.type === 'leafDirective') {
transformUnhandledDirective(node, index, parent);
return;
}
const variant = node.name;
Expand Down
1 change: 1 addition & 0 deletions packages/starlight/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@
"hast-util-select": "^6.0.2",
"hastscript": "^8.0.0",
"mdast-util-directive": "^3.0.0",
"mdast-util-to-markdown": "^2.1.0",
"pagefind": "^1.0.3",
"rehype": "^13.0.1",
"remark-directive": "^3.0.0",
Expand Down
3 changes: 3 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading