Conversation
📝 WalkthroughWalkthroughAdds a preview-copy button and clipboard flow that serializes the rendered Typst preview SVG (optionally inverting colors), normalizes alpha-bearing colors, writes rich/plain clipboard payloads, integrates with preview lifecycle, and adds Playwright tests and test mock state to control the preview SVG. ChangesPreview SVG Clipboard Copy Feature
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/preview.ts`:
- Around line 33-40: The preview copy button is only hidden when
!window.isSecureContext, but should also be hidden when the Clipboard API is
unavailable; update setupPreviewListeners to check both window.isSecureContext
and navigator.clipboard?.writeText (or navigator.clipboard presence) before
enabling the button: if either secure context is false or
navigator.clipboard.writeText is missing, set
previewCopyButton.dataset.clipboardUnavailable = "true" and
previewCopyButton.hidden = true so setPreviewCopyButtonEnabled will never
re-enable it; reference previewCopyButton, setupPreviewListeners,
setPreviewCopyButtonEnabled, and navigator.clipboard.writeText when making the
change.
In `@web/styles/main.css`:
- Around line 324-327: The CSS rule .preview-copy-icon-clipboard uses the value
keyword "currentColor" for the fill property which violates stylelint's
lowercase requirement; update the fill value in the .preview-copy-icon-clipboard
rule (the fill declaration) to the lowercase "currentcolor" so the property
reads fill: currentcolor; and rerun linting to confirm the stylelint error is
resolved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e262d2d6-a29c-48cc-a9fe-eda47dd49f3d
📒 Files selected for processing (12)
tests/_support/browser-mocks/typst-state-module.d.tstests/_support/browser-mocks/typst-state.tstests/_support/browser-mocks/typst.tstests/_support/fixtures.tstests/_support/typst-mock.tstests/pages/powerpoint-page.tstests/preview.spec.tsweb/powerpoint.htmlweb/src/constants.tsweb/src/preview.tsweb/src/svg.tsweb/styles/main.css
Comment on lines
+324
to
+327
| .preview-copy-icon-clipboard { | ||
| width: 14px; | ||
| fill: currentColor; | ||
| opacity: 1; |
There was a problem hiding this comment.
Fix value-keyword case to satisfy stylelint.
Line 326 uses currentColor; stylelint requires lowercase currentcolor here.
Suggested patch
.preview-copy-icon-clipboard {
width: 14px;
- fill: currentColor;
+ fill: currentcolor;
opacity: 1;
transform: translate(-50%, -50%) scale(1);
}📝 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.
Suggested change
| .preview-copy-icon-clipboard { | |
| width: 14px; | |
| fill: currentColor; | |
| opacity: 1; | |
| .preview-copy-icon-clipboard { | |
| width: 14px; | |
| fill: currentcolor; | |
| opacity: 1; | |
| transform: translate(-50%, -50%) scale(1); | |
| } |
🧰 Tools
🪛 Stylelint (17.12.0)
[error] 326-326: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/styles/main.css` around lines 324 - 327, The CSS rule
.preview-copy-icon-clipboard uses the value keyword "currentColor" for the fill
property which violates stylelint's lowercase requirement; update the fill value
in the .preview-copy-icon-clipboard rule (the fill declaration) to the lowercase
"currentcolor" so the property reads fill: currentcolor; and rerun linting to
confirm the stylelint error is resolved.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
web/src/copy.ts (1)
19-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate availability on
writeText, not onlynavigator.clipboard.The current gate can enable the button in hosts with partial Clipboard API support, then every copy fails on fallback write. Tighten the check to require
writeText.Suggested patch
function isClipboardAvailable(): boolean { - return window.isSecureContext && isDefined(navigator.clipboard); + return window.isSecureContext && typeof navigator.clipboard?.writeText === "function"; }Also applies to: 94-96
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/src/copy.ts` around lines 19 - 23, The current availability gate only checks isClipboardAvailable() which can be true when navigator.clipboard exists but writeText is missing; update the check to require the writeText method (e.g., ensure navigator.clipboard?.writeText is a function) before enabling copy UI: change the block that sets clipboardUnavailable and previewCopyButton.hidden to test for navigator.clipboard?.writeText (or update isClipboardAvailable() to include that check), and apply the same tightened check to the other occurrence that handles the fallback (the block referenced around lines 94-96) so all copy enabling/attempts only run when writeText is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/src/svg.ts`:
- Around line 343-346: The combineOpacity function misparses percentage strings
like "50%" because parseFloat yields 50 which then gets clamped to 1; update
combineOpacity to detect and handle percent-formatted opacity values (trim
whitespace, check for trailing '%'), convert percentages to a 0-1 value by
dividing by 100 before applying Number.isFinite checks and multiplying by alpha,
and keep the existing clamping logic to ensure the final result stays between 0
and 1; reference the combineOpacity function to locate and modify the parsing
branch to support both numeric and percentage opacity inputs.
---
Duplicate comments:
In `@web/src/copy.ts`:
- Around line 19-23: The current availability gate only checks
isClipboardAvailable() which can be true when navigator.clipboard exists but
writeText is missing; update the check to require the writeText method (e.g.,
ensure navigator.clipboard?.writeText is a function) before enabling copy UI:
change the block that sets clipboardUnavailable and previewCopyButton.hidden to
test for navigator.clipboard?.writeText (or update isClipboardAvailable() to
include that check), and apply the same tightened check to the other occurrence
that handles the fallback (the block referenced around lines 94-96) so all copy
enabling/attempts only run when writeText is present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c2790371-bbc8-4068-9967-2fc215a5b804
📒 Files selected for processing (5)
tests/pages/powerpoint-page.tstests/preview.spec.tsweb/src/copy.tsweb/src/preview.tsweb/src/svg.ts
Comment on lines
+343
to
+346
| function combineOpacity(opacity: string | null, alpha: number): number { | ||
| const existingOpacity = parseFloat(opacity || "1"); | ||
| const safeOpacity = Number.isFinite(existingOpacity) ? existingOpacity : 1; | ||
| return Math.max(0, Math.min(1, safeOpacity * alpha)); |
There was a problem hiding this comment.
Handle percentage opacity correctly when combining alpha.
combineOpacity uses parseFloat, so values like "50%" are treated as 50 and then clamped to 1, which loses intended transparency.
Suggested patch
function combineOpacity(opacity: string | null, alpha: number): number {
- const existingOpacity = parseFloat(opacity || "1");
- const safeOpacity = Number.isFinite(existingOpacity) ? existingOpacity : 1;
+ const parsedOpacity = parseAlpha(opacity ?? undefined);
+ const safeOpacity = parsedOpacity ?? 1;
return Math.max(0, Math.min(1, safeOpacity * alpha));
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/src/svg.ts` around lines 343 - 346, The combineOpacity function misparses
percentage strings like "50%" because parseFloat yields 50 which then gets
clamped to 1; update combineOpacity to detect and handle percent-formatted
opacity values (trim whitespace, check for trailing '%'), convert percentages to
a 0-1 value by dividing by 100 before applying Number.isFinite checks and
multiplying by alpha, and keep the existing clamping logic to ensure the final
result stays between 0 and 1; reference the combineOpacity function to locate
and modify the parsing branch to support both numeric and percentage opacity
inputs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In this PR, we add a new preview button that allows users to copy the preview SVG to their clipboard. This is especially useful to continue working in external programs like Cavalry. Hold shift while clicking on the button to copy the SVG with inverted colors.