Copy response text as both plain text and rich HTML#1878
Copy response text as both plain text and rich HTML#1878kyoto wants to merge 1 commit intoopenshift:mainfrom
Conversation
kyoto
added
the
approved
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds runtime dependencies ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
1 similar comment
|
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/clipboard.ts (1)
3-14:⚠️ Potential issue | 🟠 MajorPropagate clipboard failures back to the caller.
This helper now returns a
Promise, but the current callers insrc/components/CopyAction.tsxandsrc/components/GeneralPage.tsxstill fire-and-forget it and mark the UI as copied immediately. That can show success before the write completes or fails. Please either rethrow after logging or update the callers toawaitthis helper before setting the copied state.♻️ Minimal change inside this helper
} catch (err) { // eslint-disable-next-line no-console console.error('Failed to copy to clipboard: ', err); + throw err; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/clipboard.ts` around lines 3 - 14, The helper copyToClipboard currently swallows errors, causing callers (CopyAction.tsx, GeneralPage.tsx) to show success prematurely; modify copyToClipboard to propagate failures by rethrowing the caught error (or remove the try/catch) after logging so callers can await the Promise and only mark UI as copied on success; ensure the function signature remains Promise<void> and keep the existing logging via console.error or processLogger before rethrowing the error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/clipboard.ts`:
- Around line 3-14: The helper copyToClipboard currently swallows errors,
causing callers (CopyAction.tsx, GeneralPage.tsx) to show success prematurely;
modify copyToClipboard to propagate failures by rethrowing the caught error (or
remove the try/catch) after logging so callers can await the Promise and only
mark UI as copied on success; ensure the function signature remains
Promise<void> and keep the existing logging via console.error or processLogger
before rethrowing the error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4ea7e5b3-148e-44fb-8cf2-b0af5b234a09
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.jsonsrc/clipboard.ts
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/clipboard.ts (1)
4-19:⚠️ Potential issue | 🟠 MajorPropagate copy failures from the new async API.
Line 4 now exposes a
Promise, but Lines 16-18 swallow every failure. That leaves callers unable to distinguish success from failure, and the current consumers already show the downstream problem:src/components/CopyAction.tsx:25-26flips to “Copied” immediately, whilesrc/components/GeneralPage.tsx:225ignores the promise entirely. Please either rethrow/return a success flag here or keep this helper fire-and-forget so the contract matches reality.Proposed direction
-export const copyToClipboard = async (value: string): Promise<void> => { +export const copyToClipboard = async (value: string): Promise<boolean> => { try { if (typeof ClipboardItem !== 'undefined' && navigator.clipboard?.write) { const html = DOMPurify.sanitize(marked.parse(value) as string); const clipboardItem = new ClipboardItem({ 'text/plain': new Blob([value], { type: 'text/plain' }), 'text/html': new Blob([html], { type: 'text/html' }), }); await navigator.clipboard.write([clipboardItem]); } else { await navigator.clipboard.writeText(value); } + return true; } catch (err) { // eslint-disable-next-line no-console console.error('Failed to copy to clipboard: ', err); + return false; } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/clipboard.ts` around lines 4 - 19, copyToClipboard currently swallows all errors in its try/catch, preventing callers from detecting failures; change it to propagate failures by either removing the catch or rethrowing after logging so the returned Promise rejects on error, e.g. keep the DOMPurify/ClipboardItem logic inside try but after logging do "throw err" (or return a boolean success flag from copyToClipboard and resolve true/false consistently), and ensure callers of copyToClipboard (e.g., CopyAction component) await the Promise instead of treating it as fire-and-forget so they can react to success/failure.
🧹 Nitpick comments (1)
src/clipboard.ts (1)
7-7: Remove theas stringcast and awaitmarked.parse()instead.By default,
marked.parse()returnsstring | Promise<string>in TypeScript's type definitions. Theas stringcast masks this union type, potentially hiding the mismatch if async mode is ever enabled. Since this function is already async, awaiting the result properly handles both cases and removes the unsafe cast.Suggested fix
- const html = DOMPurify.sanitize(marked.parse(value) as string); + const rendered = await marked.parse(value); + const html = DOMPurify.sanitize(rendered);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/clipboard.ts` at line 7, In src/clipboard.ts, remove the unsafe "as string" cast and await the possibly asynchronous marked.parse() call: call await marked.parse(value) and pass its result into DOMPurify.sanitize so DOMPurify.sanitize receives a definite string; update the declaration of html (used where DOMPurify.sanitize is called) to use the awaited value rather than casting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/clipboard.ts`:
- Around line 6-14: The rich clipboard write should be wrapped in a try/catch
and fall back to navigator.clipboard.writeText on any failure; before adding the
'text/html' blob, detect support with ClipboardItem.supports('text/html') and
only include the html entry when supported (otherwise construct the
ClipboardItem with only 'text/plain'), then attempt await
navigator.clipboard.write([clipboardItem]) inside the try block and on catch
call await navigator.clipboard.writeText(value) to ensure plain-text fallback;
update the code around ClipboardItem, navigator.clipboard.write and
navigator.clipboard.writeText and keep DOMPurify.sanitize(marked.parse(value))
for the html content.
---
Outside diff comments:
In `@src/clipboard.ts`:
- Around line 4-19: copyToClipboard currently swallows all errors in its
try/catch, preventing callers from detecting failures; change it to propagate
failures by either removing the catch or rethrowing after logging so the
returned Promise rejects on error, e.g. keep the DOMPurify/ClipboardItem logic
inside try but after logging do "throw err" (or return a boolean success flag
from copyToClipboard and resolve true/false consistently), and ensure callers of
copyToClipboard (e.g., CopyAction component) await the Promise instead of
treating it as fire-and-forget so they can react to success/failure.
---
Nitpick comments:
In `@src/clipboard.ts`:
- Line 7: In src/clipboard.ts, remove the unsafe "as string" cast and await the
possibly asynchronous marked.parse() call: call await marked.parse(value) and
pass its result into DOMPurify.sanitize so DOMPurify.sanitize receives a
definite string; update the declaration of html (used where DOMPurify.sanitize
is called) to use the awaited value rather than casting.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 24e8d11f-02d3-4668-be7d-77a7a0099dc8
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
package.jsonsrc/clipboard.tstsconfig.json
✅ Files skipped from review due to trivial changes (1)
- tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
When an OLS response is copied to the clipboard, write both text/plain (raw Markdown) and text/html (rendered HTML via marked) so that pasting into rich text editors preserves formatting, while plain text editors still receive the raw Markdown. Also overrides the dompurify version to address a vulnerability in the version used by monaco-editor. Made-with: Cursor
|
/cherry-pick release-4.19 |
|
@kyoto: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
2 participants
When an OLS response is copied to the clipboard, write both text/plain (raw Markdown) and text/html (rendered HTML via marked) so that pasting into rich text editors preserves formatting, while plain text editors still receive the raw Markdown.
Also overrides the dompurify version to address a vulnerability in the version used by monaco-editor.
Made-with: Cursor
Summary by CodeRabbit
New Features
Chores