Enable screenshot both to file and clipboard simultaneously#1094
Conversation
|
Caution Review failedFailed to post review comments WalkthroughAdds a new screenshot save option "both" (file + clipboard) across types, locales, settings UI, and screenshot implementation. Splits playlist management flag into separate remove/reset flags, introduces hide-sidebar-recommended-videos feature (CSS + runtime), updates message handlers and default configuration, and normalizes many locale title capitalizations. Changes
Sequence Diagram(s)mermaid User->>SettingsUI: select "Both" for screenshot save-as Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/features/screenshotButton/index.ts (3)
29-54: Consider updating tooltip message when "both" is selected.When
screenshot_save_asis "both", the tooltip at line 37 displays "Screenshot copied to clipboard" but doesn't indicate that the file is also being saved. This could confuse users who expect confirmation that both operations are occurring.Consider checking the
screenshot_save_asvalue and displaying a more comprehensive message like "Screenshot copied to clipboard and saved to file" when "both" is selected.Example approach:
const copyToClipboard = async () => { const screenshotButton = getFeatureButton("screenshotButton"); if (!screenshotButton) return; const tooltipText = screenshot_save_as === "both" ? window.i18nextInstance.t("pages.content.features.screenshotButton.copiedToBoth") : window.i18nextInstance.t("pages.content.features.screenshotButton.copiedToClipboard"); const { listener, remove } = createTooltip({ direction: "up", element: screenshotButton, featureName: "screenshotButton", id: "yte-feature-screenshotButton-tooltip", text: tooltipText }); // ... rest of function };Note: This would require adding the new translation key to the locale files.
56-65: Add error feedback for file save failures.The
saveToFilefunction silently returns early at line 59 if the blob creation fails, and the outer catch block at line 73 silently swallows any errors. When "both" is selected, users might see the "copied to clipboard" tooltip and assume the file was also saved successfully, even if it failed.Consider adding user feedback (e.g., console error or tooltip) when file save operations fail, especially for the "both" mode where partial success could be confusing.
Example:
const saveToFile = async () => { try { const mimeType = `image/${screenshot_format}`; const blob = await new Promise<Nullable<Blob>>((resolve) => canvas.toBlob(resolve, mimeType)); if (!blob) { console.error("Failed to create screenshot blob"); return; } const timestamp = new Date().toISOString().replace(/[:.]/g, "-"); const a = document.createElement("a"); a.href = URL.createObjectURL(blob); a.download = `Screenshot-${location.href.match(/[\?|\&]v=([^&]+)/)?.[1]}-${timestamp}.${screenshot_format}`; a.click(); } catch (error) { console.error("Failed to save screenshot file:", error); } };
67-72: Consider parallel execution for better performance.The current implementation executes clipboard copy and file save sequentially with
await(lines 67-72). For the "both" option, these operations could be executed in parallel usingPromise.all()since they're independent.This is a minor optimization and the current sequential approach is safer, so this is optional.
if (screenshot_save_as === "both") { await Promise.all([copyToClipboard(), saveToFile()]); } else if (screenshot_save_as === "clipboard") { await copyToClipboard(); } else if (screenshot_save_as === "file") { await saveToFile(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
public/locales/en-GB.json(3 hunks)public/locales/en-US.json(2 hunks)public/locales/en-US.json.d.ts(2 hunks)src/components/Settings/Settings.tsx(1 hunks)src/features/screenshotButton/index.ts(1 hunks)src/types/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/features/screenshotButton/index.ts (3)
src/features/buttonPlacement/utils.ts (1)
getFeatureButton(35-37)src/utils/utilities.ts (1)
createTooltip(231-327)src/types/index.ts (1)
Nullable(29-29)
🔇 Additional comments (6)
src/types/index.ts (1)
114-115: LGTM! Type definition correctly expanded.The addition of "both" to the
screenshotTypesconstant properly extends the allowed values forScreenshotType. This change maintains type safety throughout the codebase.src/components/Settings/Settings.tsx (1)
267-271: LGTM! UI option correctly integrated.The new "both" option is properly added to the
ScreenshotSaveAsOptionsarray with appropriate translation key. The existing logic at line 174 correctly keeps the format selector enabled when "both" is selected, allowing users to configure the file format for the file save portion of the operation.src/features/screenshotButton/index.ts (1)
41-41: Note: MIME type inconsistency between clipboard and file operations.Clipboard copy always uses
"image/png"(line 41) regardless of the user's configuredscreenshot_format, while file save uses the configured format (line 57). When "both" is selected, this means the clipboard and file will have different formats.This is likely intentional due to browser clipboard API limitations with certain formats, but it might be worth documenting this behavior or ensuring the format selector description makes this clear to users.
Can you confirm this MIME type difference is intentional? If so, consider adding a comment explaining the constraint:
// Clipboard API has limited format support, always use PNG for reliability const mimeType = "image/png";Also applies to: 57-57
public/locales/en-US.json (1)
139-139: LGTM! Translations correctly added and updated.The new "both" option label is clear and descriptive, and the updated description at line 139 correctly reflects that the format setting applies to both 'File' and 'Both' save types. These changes align well with the feature implementation.
Also applies to: 531-532
public/locales/en-US.json.d.ts (1)
81-81: LGTM! Type definitions correctly updated.The TypeScript declarations properly reflect the changes made to the JSON locale file, maintaining type safety for the new "both" option and updated description string.
Also applies to: 410-410
public/locales/en-GB.json (1)
139-139: LGTM! British English translations consistent.The en-GB locale changes match the en-US additions appropriately. The new "both" option and updated description are correctly translated, and existing British English conventions (like "colour") are properly maintained throughout the file.
Also applies to: 531-532
|
@azatoth I appreciate your contribution but your changes should be made on the dev branch. |
To allow the use case where you want to screenshot to clipboard but still want to have it stored to file as well for future reference.
azatoth
force-pushed
the
add-screenshot-both-option
branch
from
7e18c45 to
8ba68d6
Compare
rebased onto dev |
|
@azatoth Never mind I didn't read the code right my bad |
github-project-automation
Bot
moved this from Todo
to Done - Released
in YouTube Enhancer
|
🎉 This PR is included in version 1.32.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
2 participants
To allow the use case where you want to screenshot to clipboard but
still want to have it stored to file as well for future reference.
Summary by CodeRabbit
New Features
UI/Text updates
Removed/Changed
✏️ Tip: You can customize this high-level summary in your review settings.