Copy header link #30411
Copy header link #30411
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
6 file(s) reviewed, 4 comment(s)
Edit PR Review Bot Settings | Greptile
| document.body.appendChild(tmp); | ||
| tmp.select(); | ||
| document.execCommand('copy'); | ||
| document.body.removeChild(tmp); |
There was a problem hiding this comment.
style: potential memory leak if an error occurs between appendChild and removeChild - wrap in try/finally
| document.body.appendChild(tmp); | |
| tmp.select(); | |
| document.execCommand('copy'); | |
| document.body.removeChild(tmp); | |
| document.body.appendChild(tmp); | |
| try { | |
| tmp.select(); | |
| document.execCommand('copy'); | |
| } finally { | |
| document.body.removeChild(tmp); | |
| } |
There was a problem hiding this comment.
Adding the focus in the finally block would also be needed to ensure the app is restored to its intended state should there ever be a failure (execCommand is being removed, after all).
| global.document = { | ||
| createElement: vi.fn(), | ||
| } as any as Document; |
There was a problem hiding this comment.
logic: Mocking document with just createElement is insufficient for testing the fallback copy mechanism. Need to mock body.appendChild, execCommand, body.removeChild, and activeElement.
| global.document = { | |
| createElement: vi.fn(), | |
| } as any as Document; | |
| global.document = { | |
| createElement: vi.fn(), | |
| body: { | |
| appendChild: vi.fn(), | |
| removeChild: vi.fn() | |
| }, | |
| execCommand: vi.fn(), | |
| activeElement: null | |
| } as any as Document; |
| // DOM mocking is not enabled which will cause the function to throw | ||
| try { | ||
| await copyToClipboard('text'); | ||
| } catch (e) {} |
There was a problem hiding this comment.
style: Silently catching all errors masks potential issues. Should verify the expected error is thrown when DOM APIs are not available.
| const copyUrl = new URL(window.parent.location.href); | ||
| copyUrl.hash = hash; |
There was a problem hiding this comment.
logic: Using window.parent.location.href could break if Storybook is embedded in a multi-level iframe structure. Consider using a more robust way to get the root URL.
There was a problem hiding this comment.
I don't think this is a valid concern here considering this is the Docs renderer's own code. What would more likely be a concern is if Storybook removes the iframe and uses Web Components to render preview in the future.
Considering DocsContainer.tsx is on the same level and also gets the URL, you could consider making a getRootUrl util in the utils.ts file in the same folder for both files to use.
There was a problem hiding this comment.
6 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
| Object.assign(navigator, { | ||
| clipboard: { | ||
| writeText, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
style: Consider using vi.stubGlobal('navigator', {...}) instead of Object.assign for more reliable test setup and teardown
| onClick={(event: SyntheticEvent) => { | ||
| copyToClipboard(copyUrl.href); |
There was a problem hiding this comment.
logic: copyToClipboard should handle promise rejection from clipboard API
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 52 | 52 | 0 |
| Self size | 19.26 MB | 19.45 MB | 🚨 +198 KB 🚨 |
| Dependency size | 14.19 MB | 14.19 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 53 | 53 | 0 |
| Self size | 23 KB | 23 KB | 0 B |
| Dependency size | 33.45 MB | 33.65 MB | 🚨 +198 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
sb
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 54 | 54 | 0 |
| Self size | 1 KB | 1 KB | 0 B |
| Dependency size | 33.47 MB | 33.67 MB | 🚨 +198 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 387 | 387 | 0 |
| Self size | 503 KB | 503 KB | 0 B |
| Dependency size | 75.43 MB | 75.63 MB | 🚨 +198 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 276 | 276 | 0 |
| Self size | 617 KB | 617 KB | 0 B |
| Dependency size | 65.51 MB | 65.71 MB | 🚨 +198 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 111 | 111 | 0 |
| Self size | 1.11 MB | 1.11 MB | 0 B |
| Dependency size | 42.63 MB | 42.83 MB | 🚨 +198 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
|
View your CI Pipeline Execution ↗ for commit b949c2d.
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Good job! Works as advertised.
Note the comments on how you can improve memory usage and robustness of the URL handling. I'd love to see some tests for what is being copied, if you can sort out an AnchorMdx stories file. This part technically isn't covered by tests yet.
Other reviewers, please note: it is normal that when reopening a tab with the copied URL, the specific subsection does not load. This is a bug caused by navigate internally eating up URL hashes which I've fixed in another PR.
| document.body.appendChild(tmp); | ||
| tmp.select(); | ||
| document.execCommand('copy'); | ||
| document.body.removeChild(tmp); |
There was a problem hiding this comment.
Adding the focus in the finally block would also be needed to ensure the app is restored to its intended state should there ever be a failure (execCommand is being removed, after all).
| const copyUrl = new URL(window.parent.location.href); | ||
| copyUrl.hash = hash; |
There was a problem hiding this comment.
I don't think this is a valid concern here considering this is the Docs renderer's own code. What would more likely be a concern is if Storybook removes the iframe and uses Web Components to render preview in the future.
Considering DocsContainer.tsx is on the same level and also gets the URL, you could consider making a getRootUrl util in the utils.ts file in the same folder for both files to use.
| @@ -185,6 +192,9 @@ const HeaderWithOcticonAnchor: FC<PropsWithChildren<HeaderWithOcticonAnchorProps | |||
| const OcticonHeader = OcticonHeaders[as]; | |||
| const hash = `#${id}`; | |||
|
|
|||
| const copyUrl = new URL(window.parent.location.href); | |||
There was a problem hiding this comment.
Is there a reason why you don't use the whole URL but just the href? E.g. you are not preserving search params, but they could be used to filter content in some docs pages and could be relevant for the person copying the URL to provide an exact replica to the person receiving it.
| return (text: string) => navigator.clipboard.writeText(text); | ||
| } | ||
|
|
||
| return async (text: string) => { |
There was a problem hiding this comment.
nit: You could make these two functions top-level functions instead of arrow functions, so that no matter how often this file is imported at the top-level of new ES modules like you new in mdx.tsx, the same function declarations are reused instead of new ones being held in memory. Function declarations are hoisted and arrow functions aren't.
There was a problem hiding this comment.
6 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings | Greptile
| @@ -0,0 +1,21 @@ | |||
| export function createCopyToClipboardFunction() { | |||
| if (navigator?.clipboard) { | |||
There was a problem hiding this comment.
style: consider checking for writeText method specifically since clipboard API may exist without writeText support
| if (navigator?.clipboard) { | |
| if (navigator?.clipboard?.writeText) { |
| onCopyLink={() => { | ||
| navigator.clipboard?.writeText(whatsNewData.blogUrl ?? whatsNewData.url); | ||
| copyToClipboard(whatsNewData.blogUrl ?? whatsNewData.url); | ||
| }} |
There was a problem hiding this comment.
logic: The copyToClipboard function returns a Promise but it's not being awaited here. This could cause issues with error handling.
| onCopyLink={() => { | |
| navigator.clipboard?.writeText(whatsNewData.blogUrl ?? whatsNewData.url); | |
| copyToClipboard(whatsNewData.blogUrl ?? whatsNewData.url); | |
| }} | |
| onCopyLink={async () => { | |
| try { | |
| await copyToClipboard(whatsNewData.blogUrl ?? whatsNewData.url); | |
| } catch (error) { | |
| console.error('Failed to copy to clipboard:', error); | |
| } | |
| }} |
|
I personally do not agree with the original problem statement.
From my general experience, the "click heading to get URL"-feature does not automatically copy the URL to the clipboard. Can anyone share a few links to sites that do this today? Can anyone share a few links to sites that copies to the clipboard automatically today? Here are a few examples of sites that have clickable headings without copying to clipboard:
In my search I found one website that added to the clipboard automatically, https://web.dev, but they do that with an explicit callout, speaking into my perception that it is generally unexpected: |
Closes #30407
What I did
createCopyToClipboardFunctionto a separate file for clearer availability for reuse.createCopyToClipboardFunction. Decided not to fully test the fallback method since DOM mocking doesn't seem to be working andnavigator.clipboardis pretty much generally available.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
yarn task --task sandbox --start-from auto --template react-vite/default-tsDocumentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>Greptile Summary
This PR refactors clipboard functionality in Storybook by introducing a reusable utility with modern API support and DOM fallback.
createCopyToClipboardFunction.tswith Clipboard API support and DOM fallback for older browserscreateCopyToClipboardFunction.test.tsfocusing on Clipboard API functionalitysyntaxhighlighter.tsxto use centralized clipboard functionalitywhats_new.tsxto use the new utility with copy feedback stateThe changes improve code organization by centralizing clipboard functionality while maintaining backward compatibility through a DOM fallback mechanism. The implementation appears solid though some additional testing for URL handling would be beneficial.
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!