Add GitHub code permalink embeds#8683
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
potatowagon
left a comment
There was a problem hiding this comment.
Reviewed by Navi (Tater Thoughts Bobblehead) on behalf of @potatowagon.
Assessment: LGTM ✅ — Playground-only feature (GitHub code permalink embeds). No core library changes, no www impact.
What I checked:
-
Node implementation (GitHubCodeNode.tsx, 502 lines): Well-structured DecoratorBlockNode subclass. Proper
importJSON/exportJSONround-trip withupdateFromJSON.exportDOMuses data attributes.getTextContent()returns the URL (good for copy-paste). UsesLexicalNestedComposerwithcreateEditorfor syntax highlighting via@lexical/code-prism— readonly nested editor, non-editable. Clean React component with AbortController cleanup, progressive line reveal (show more/collapse), refresh button. All state management is local. -
URL parsing (AutoEmbedPlugin/index.tsx): Supports blob, blame, gist, and raw.githubusercontent.com patterns. Language detection from file extension via
LANGUAGE_BY_EXTENSIONmap. Regex patterns are well-scoped and tested. TheisGitHubCodeMatchResulttype guard is clean. -
Extension registration (GitHubCodeExtension/index.ts): Uses
defineExtension+$insertNodeToNearestRoot+COMMAND_PRIORITY_EDITOR. Standard pattern consistent with FigmaExtension, TwitterExtension, etc. -
Test coverage: 272-line unit test (creation, type checking, serialization round-trip, DOM export, cloning) + 185-line URL parsing test (blob, blame, gist, raw, invalid URLs). Comprehensive edge case coverage.
-
CSS (155 lines): GitHub-style embed UI. No global selector leaks — all scoped under
.github-code-*. Responsive grid layout for line numbers + code. -
www compat: No impact — purely additive playground feature with no core library API changes, no removed/renamed exports.
Minor observations (non-blocking):
- The
fetchCodefunction makes unauthenticated GitHub API requests, which are rate-limited to 60/hour. This is fine for a playground demo but worth noting in docs if it ever becomes a core feature. - The gist regex uses
[a-f0-9]+which is correct for gist IDs. - Missing a
DOMImportConversion(noimportDOMstatic) — pasting an exported GitHub embed div won't reconstitute the node. Acceptable for a playground-only feature.
CI: CLA ✅, Vercel deploys ✅. Full test matrix not yet triggered (likely awaiting PR age/label). No blocking failures.
Ready to approve.
etrepum
left a comment
There was a problem hiding this comment.
I haven't done a close review of the react code yet but the line numbers aren't set up properly, they overflow and scroll separately from the content
This PR's title and description also doesn't follow the pull request template
| export type GitHubCodePayload = { | ||
| url: string; | ||
| owner: string; | ||
| repo: string; | ||
| path: string; | ||
| branch: string; | ||
| startLine?: number; | ||
| endLine?: number; | ||
| language?: string; | ||
| }; |
There was a problem hiding this comment.
| export type GitHubCodePayload = { | |
| url: string; | |
| owner: string; | |
| repo: string; | |
| path: string; | |
| branch: string; | |
| startLine?: number; | |
| endLine?: number; | |
| language?: string; | |
| }; | |
| export interface GitHubCodePayload { | |
| url: string; | |
| owner: string; | |
| repo: string; | |
| path: string; | |
| branch: string; | |
| startLine?: number; | |
| endLine?: number; | |
| language?: string; | |
| } |
| SerializedDecoratorBlockNode | ||
| >; | ||
|
|
||
| export class GitHubCodeNode extends DecoratorBlockNode { |
There was a problem hiding this comment.
Using $config and NodeState will make a lot of this boilerplate go away
| TweetNode, | ||
| YouTubeNode, | ||
| FigmaNode, | ||
| GitHubCodeNode, |
There was a problem hiding this comment.
This is a legacy file that you don't need to update, the extension takes care of configuring the node
|
@JiatangDong Why do you keep opening and closing PRs here? It's a waste of maintainer time if you don't intend for them to be reviewed and merged. This is the fourth time. |
Summary:
Verification: