frontend: LogViewer: Add copy to clipboard action - #6813
Conversation
kubernetes-prow
Bot
added
size/L
8d879e2 to
2b7d4ca
Compare
|
I think the implementation could be shared with the other copy to clipboard buttons in the code |
|
@skoeva Good catch, thanks. I'll look through the existing copy-to-clipboard implementations in the codebase and pull this into a shared hook or utility instead of duplicating the logic here. I'll push an update once that's done. |
illume
added
kind/feature
There was a problem hiding this comment.
🟡 Changes recommended
The new useSnackbar dependency can break the LogViewer Storybook stories unless they are wrapped in a SnackbarProvider, and the current snapshot normalization approach is overly destructive.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR enhances the frontend LogViewer by adding a “Copy to clipboard” action (with snackbar feedback), and updates Storybook snapshot testing to reduce churn from xterm’s dynamically generated output while regenerating i18n locale keys for the new user-facing strings.
Changes:
- Added a clipboard-copy action and snackbar notifications to
LogViewer. - Added unit tests for copy success/failure behavior.
- Reduced Storybook snapshot flakiness by normalizing xterm-generated output and updated affected snapshots + regenerated i18n locale files.
File summaries
| File | Description |
|---|---|
| frontend/src/components/common/LogViewer.tsx | Adds copyLogs handler, snackbar notifications, and a new ActionButton for copying logs. |
| frontend/src/components/common/LogViewer.test.tsx | New unit tests covering clipboard copy success and failure paths. |
| frontend/src/components/common/LogViewer.stories.tsx | Adds a new story variant that renders LogViewer with the copy action present. |
| frontend/src/storybook.test.tsx | Normalizes xterm-generated output during Storybook snapshot runs to reduce flakiness. |
| frontend/src/components/common/snapshots/Terminal.TerminalNoDialog.stories.storyshot | Updates snapshot output to reflect normalized xterm styles. |
| frontend/src/components/common/snapshots/Terminal.TerminalDisconnected.stories.storyshot | Updates snapshot output to reflect normalized xterm styles. |
| frontend/src/components/common/snapshots/Terminal.TerminalConnectionFailed.stories.storyshot | Updates snapshot output to reflect normalized xterm styles. |
| frontend/src/i18n/locales/ar/translation.json | Adds new LogViewer clipboard-related strings to the locale output. |
| frontend/src/i18n/locales/bn/translation.json | Adds new LogViewer clipboard-related strings to the locale output. |
| frontend/src/i18n/locales/de/translation.json | Adds new LogViewer clipboard-related strings to the locale output. |
| frontend/src/i18n/locales/en/translation.json | Adds new LogViewer clipboard-related strings to the locale output. |
| frontend/src/i18n/locales/es/translation.json | Adds new LogViewer clipboard-related strings to the locale output. |
| frontend/src/i18n/locales/fr/translation.json | Adds new LogViewer clipboard-related strings to the locale output. |
| frontend/src/i18n/locales/he/translation.json | Adds new LogViewer clipboard-related strings to the locale output. |
| frontend/src/i18n/locales/hi/translation.json | Adds new LogViewer clipboard-related strings to the locale output. |
| frontend/src/i18n/locales/it/translation.json | Adds new LogViewer clipboard-related strings to the locale output. |
| frontend/src/i18n/locales/ja/translation.json | Adds new LogViewer clipboard-related strings to the locale output. |
| frontend/src/i18n/locales/ko/translation.json | Adds new LogViewer clipboard-related strings to the locale output. |
| frontend/src/i18n/locales/pt/translation.json | Adds new LogViewer clipboard-related strings to the locale output. |
| frontend/src/i18n/locales/ru/translation.json | Adds new LogViewer clipboard-related strings to the locale output. |
| frontend/src/i18n/locales/ta/translation.json | Adds new LogViewer clipboard-related strings to the locale output. |
| frontend/src/i18n/locales/ur/translation.json | Adds new LogViewer clipboard-related strings to the locale output. |
| frontend/src/i18n/locales/zh/translation.json | Adds new LogViewer clipboard-related strings to the locale output. |
| frontend/src/i18n/locales/zh-tw/translation.json | Adds new LogViewer clipboard-related strings to the locale output. |
Review details
- Files reviewed: 37/37 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
illume
left a comment
illume
left a comment
There was a problem hiding this comment.
Thanks for the contribution.
There are some open Copilot review comments — could you take a look at them? Please mark each one as resolved once you've addressed it.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Priyanshu-rgbb The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
2b7d4ca to
7fbf478
Compare
📝 WalkthroughWalkthrough
ChangesLogViewer clipboard workflow
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (1)
frontend/src/components/common/LogViewer.tsx (1)
96-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared clipboard behavior.
copyLogsinlines clipboard writes with its own success/error snackbar, whilefrontend/src/components/common/Resource/CopyButton.tsxand other copy handlers use reusable copy wrappers. Move this handler into a shared clipboard utility/hook and call it fromLogViewerand any other new copy action.🤖 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 `@frontend/src/components/common/LogViewer.tsx` around lines 96 - 106, Replace the inline clipboard logic in copyLogs with the existing shared clipboard utility or hook used by Resource/CopyButton and other copy handlers, including its standard success and error behavior. Update LogViewer to invoke that shared mechanism with the joined logs text, and use the same mechanism for any newly added copy actions instead of duplicating navigator.clipboard and snackbar handling.
🤖 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.
Nitpick comments:
In `@frontend/src/components/common/LogViewer.tsx`:
- Around line 96-106: Replace the inline clipboard logic in copyLogs with the
existing shared clipboard utility or hook used by Resource/CopyButton and other
copy handlers, including its standard success and error behavior. Update
LogViewer to invoke that shared mechanism with the joined logs text, and use the
same mechanism for any newly added copy actions instead of duplicating
navigator.clipboard and snackbar handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aa899884-ecb5-4dc5-826f-02db70ffd8b2
📒 Files selected for processing (37)
frontend/src/components/common/LogViewer.stories.tsxfrontend/src/components/common/LogViewer.test.tsxfrontend/src/components/common/LogViewer.tsxfrontend/src/components/common/__snapshots__/Terminal.TerminalAttachEmptyFirstOutput.stories.storyshotfrontend/src/components/common/__snapshots__/Terminal.TerminalConnectedAndReady.stories.storyshotfrontend/src/components/common/__snapshots__/Terminal.TerminalConnectionFailed.stories.storyshotfrontend/src/components/common/__snapshots__/Terminal.TerminalDefaultNodeSelector.stories.storyshotfrontend/src/components/common/__snapshots__/Terminal.TerminalDisconnected.stories.storyshotfrontend/src/components/common/__snapshots__/Terminal.TerminalInitAndEphemeralContainers.stories.storyshotfrontend/src/components/common/__snapshots__/Terminal.TerminalNoDialog.stories.storyshotfrontend/src/components/common/__snapshots__/Terminal.TerminalShellNotFoundTryNext.stories.storyshotfrontend/src/components/common/__snapshots__/Terminal.TerminalWindowsNodeSelector.stories.storyshotfrontend/src/components/common/__snapshots__/Terminal.TerminalWindowsShellNotFound.stories.storyshotfrontend/src/components/common/__snapshots__/Terminal.TerminalWithCommandOutput.stories.storyshotfrontend/src/components/pod/__snapshots__/PodLogs.BigJsonLogs.stories.storyshotfrontend/src/components/pod/__snapshots__/PodLogs.FormattedJsonLogs.stories.storyshotfrontend/src/components/pod/__snapshots__/PodLogs.FormattingLogs.stories.storyshotfrontend/src/components/pod/__snapshots__/PodLogs.JsonLogs.stories.storyshotfrontend/src/components/pod/__snapshots__/PodLogs.PlainLogs.stories.storyshotfrontend/src/i18n/locales/ar/translation.jsonfrontend/src/i18n/locales/bn/translation.jsonfrontend/src/i18n/locales/de/translation.jsonfrontend/src/i18n/locales/en/translation.jsonfrontend/src/i18n/locales/es/translation.jsonfrontend/src/i18n/locales/fr/translation.jsonfrontend/src/i18n/locales/he/translation.jsonfrontend/src/i18n/locales/hi/translation.jsonfrontend/src/i18n/locales/it/translation.jsonfrontend/src/i18n/locales/ja/translation.jsonfrontend/src/i18n/locales/ko/translation.jsonfrontend/src/i18n/locales/pt/translation.jsonfrontend/src/i18n/locales/ru/translation.jsonfrontend/src/i18n/locales/ta/translation.jsonfrontend/src/i18n/locales/ur/translation.jsonfrontend/src/i18n/locales/zh-tw/translation.jsonfrontend/src/i18n/locales/zh/translation.jsonfrontend/src/storybook.test.tsx
There was a problem hiding this comment.
🟢 Ready to approve
The clipboard action, notifications, tests, snapshot normalization, and regenerated i18n outputs are consistent and self-contained with no issues found in the reviewed diffs.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 21/37 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Hey @illume, this PR is ready for another look. Whenever you have a chance, I'd really appreciate a final review. Thanks! |
kubernetes-prow
Bot
added
the
needs-rebase
7fbf478 to
0228f6d
Compare
kubernetes-prow
Bot
removed
the
needs-rebase
unlikelyzero
left a comment
unlikelyzero
left a comment
There was a problem hiding this comment.
The feature itself is small and fine, but there's a CI blocker and a few things worth fixing before this merges.
The one that'll actually fail the build: frontend/src/i18n/locales/pt-br/translation.json wasn't regenerated. Every other locale got the two new keys and had "Copy to clipboard" relocated to sit after Download - pt-br has neither, it's still got "Copy to clipboard" at its old position. CI runs npm run i18n -- --fail-on-update, which fails the whole PR if regenerating produces any diff, and it will here.
Also worth flagging: this repo's contributing guide asks for atomic commits, and this one bundles two unrelated things - the copy-to-clipboard feature, and a fix to shared Storybook test infrastructure (the xterm snapshot normalization in storybook.test.tsx, which rewrites 10+ Terminal storyshots that have nothing to do with LogViewer). Splitting those into two commits would make each one reviewable and revertable on its own - the infra fix is genuinely useful and would probably land quickly by itself.
And there's a still-open request from an earlier review round: skoeva asked for this to share an implementation with the other copy-to-clipboard buttons in the codebase, and the author agreed to pull it into a shared hook - that never happened. There are now five separate navigator.clipboard.writeText call sites (CopyButton.tsx, two in Resource.tsx, ErrorPage.tsx, and this one), each with different error handling - CopyButton.tsx doesn't even catch a rejection. A shared hook would fix that gap too, not just dedupe.
A few more inline.
cf0e3cd to
09a7809
Compare
kubernetes-prow
Bot
added
do-not-merge/work-in-progress
09a7809 to
de39cc4
Compare
kubernetes-prow
Bot
removed
the
needs-rebase
kubernetes-prow
Bot
removed
the
do-not-merge/work-in-progress
|
Hey @illume all the comments have been resolved , CI is green and the PR is ready for a final review. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 38 changed files in this pull request and generated no new comments.
Suppressed comments (2)
frontend/src/components/common/LogViewer.test.tsx:30
- The terminal mock has no
buffer, so all three tests take the fallbacklogs.join('')branch; the production path that reads xterm rows is never exercised. Please add a buffer-backed case covering ordinary output, a soft-wrapped line, and unused viewport rows so the copied text is verified against actual xterm semantics.
Terminal: vi.fn().mockImplementation(() => ({
frontend/src/components/common/LogViewer.tsx:107
buffer.lengthincludes unused viewport rows, so short logs are copied with many extra blank lines. It also represents soft-wrapped rows separately, and joining every row with\ninserts line breaks that were not in the log. Limit iteration to the cursor's last written row and merge rows markedisWrappedso pasted text matches the rendered log.
const lines: string[] = [];
for (let i = 0; i < buffer.length; i++) {
lines.push(buffer.getLine(i)?.translateToString(true) ?? '');
}
return lines.join('\n');
@illume This PR is ready. |
kubernetes-prow
Bot
added
the
needs-rebase
de39cc4 to
70067b9
Compare
kubernetes-prow
Bot
removed
the
needs-rebase
Summary
This PR adds a Copy to Clipboard action to the
LogViewer, allowing users to copy log output directly instead of downloading it first. It also updates the affected Storybook snapshots, reduces snapshot flakiness caused by xterm-generated output, and regenerates the frontend i18n files for the new user-facing strings.Related Issue
No related issue. This is a small, self-contained improvement.
Changes
copyLogshandler that copies the current log output usingnavigator.clipboard.writeText.ActionButtonnext to the existing Download button.LogViewer.test.tsxcovering both the success and failure paths.PodLogs.*storyshots since they renderLogViewerand now include the new action.src/storybook.test.tsxto reduce Storybook snapshot flakiness.common/Terminal.*storyshots to match the normalized snapshot output.Steps to Test
Screenshots
Not applicable — this adds a single action button next to the existing Download button.
Notes for the Reviewer
useSnackbarpattern already used elsewhere in the codebase.npx vitest run src/components/common/LogViewer.test.tsx --reporter=verbosenpm run lint -- src/components/common/LogViewer.test.tsx src/components/common/LogViewer.tsxnpx vitest run src/components/common/LogViewer.test.tsxnpx vitest run src/storybook.test.tsx -t "Pod/PodLogViewer" -u --reporter=dotnpx vitest run src/storybook.test.tsx -t "common/Terminal" -u --reporter=dotnpx vitest run src/storybook.test.tsx --reporter=dotmake frontend-i18nPodLogs.*andcommon/Terminal.*storyshot updates are expected. They reflect the new LogViewer action and the normalized xterm snapshot output.Summary by CodeRabbit