-
-
Notifications
You must be signed in to change notification settings - Fork 254
Allow clipboard.writeText without HTTPS #1723
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
📝 WalkthroughWalkthroughRemoved HTTPS gating from copy-to-clipboard across UI, refactored SmallStatBlock to use a CanBeCopied trait and client-side copying, replaced Livewire copy handlers with Alpine/Clipboard API, added a clipboard polyfill, updated console copy hotkey, adjusted a Field macro, and removed a translation key. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as SmallStatBlock (Blade/Alpine)
participant Clipboard as Clipboard API / Polyfill
participant Tooltip as Tooltip Renderer
User->>UI: Click copyable value
UI->>Clipboard: writeText(value)
alt Native Clipboard API available
Clipboard-->>UI: Promise resolved
else Fallback (execCommand)
Clipboard->>Clipboard: create hidden textarea, select, execCommand('copy')
Clipboard-->>UI: Promise resolved/rejected
end
UI->>Tooltip: show getCopyMessage(value) (2s)
sequenceDiagram
autonumber
actor User
participant Console as Server Console (Blade/JS)
participant Clipboard as Clipboard API / Polyfill
User->>Console: Ctrl/Cmd + C
Console->>Clipboard: writeText(terminal.getSelection())
Clipboard-->>Console: Promise resolved/rejected
Possibly related PRs
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (13)
app/Filament/Admin/Resources/ApiKeys/ApiKeyResource.php (1)
78-83: Un‑gated copy for API keys: gate on non‑empty stateLooks good. To avoid copying an empty value in edge cases, gate the action on filled state.
- ->copyable(), + ->copyable(fn ($state) => filled($state)),app/Filament/Admin/Resources/DatabaseHosts/Pages/CreateDatabaseHost.php (2)
100-101: Copy action should be no‑op when state is emptyMinor UX hardening: only enable copy when there’s something to copy.
- ->copyable() + ->copyable(fn ($state) => filled($state))
107-108: Same here for assign_permissionsMirror the guard for consistency.
- ->copyable() + ->copyable(fn ($state) => filled($state))app/Filament/Server/Resources/Databases/DatabaseResource.php (4)
78-82: Gate copy on non‑empty computed hostPrevents copying null/empty outputs.
- ->copyable(), + ->copyable(fn ($state) => filled($state)),
82-88: Database/username: same gatingKeeps copy UX clean when values are blank.
- ->copyable(), + ->copyable(fn ($state) => filled($state)),
88-98: Password: ensure state exists before enabling copyAuthorization already hides the field; this just avoids empty copies.
- ->copyable() + ->copyable(fn ($state) => filled($state))
103-110: JDBC: same gating- ->copyable() + ->copyable(fn ($state) => filled($state))app/Filament/App/Resources/Servers/Pages/ListServers.php (1)
76-82: Avoid copying placeholder “None”Copying “None” is misleading; gate on a real address.
- ->copyable() + ->copyable(fn ($state) => $state !== 'None' && filled($state))app/Filament/Server/Pages/Settings.php (2)
164-171: Gate SFTP connection copy on non‑empty stateMinor polish.
- ->copyable() + ->copyable(fn ($state) => filled($state))
185-191: Gate SFTP username copy as well- ->copyable() + ->copyable(fn ($state) => filled($state))resources/views/filament/components/server-console.blade.php (1)
103-107: Clipboard hotkey: guard empty selection, normalize key, and handle async errors.Prevent swallowing Ctrl/Cmd+C with no selection, normalize key case, and catch clipboard promise rejections.
- terminal.attachCustomKeyEventHandler((event) => { - if ((event.ctrlKey || event.metaKey) && event.key === 'c') { - navigator.clipboard.writeText(terminal.getSelection()) - return false; - } else if ((event.ctrlKey || event.metaKey) && event.key === 'f') { + terminal.attachCustomKeyEventHandler((event) => { + if ((event.ctrlKey || event.metaKey) && event.key.toLowerCase() === 'c') { + if (!terminal.hasSelection()) return true; + event.preventDefault(); + void navigator.clipboard.writeText(terminal.getSelection()).catch(() => {}); + return false; + } else if ((event.ctrlKey || event.metaKey) && event.key.toLowerCase() === 'f') {Also confirm the clipboard polyfill is loaded before this inline script on pages where this widget is rendered.
resources/views/filament/components/server-small-data-block.blade.php (1)
3-8: Optional a11y: make the copyable span keyboard-accessible.Consider
role="button",tabindex="0", andx-on:keyup.enter="$el.click()".- <span class="cursor-pointer" x-on:click=" + <span class="cursor-pointer" role="button" tabindex="0" x-on:keyup.enter="$el.click()" x-on:click="app/Filament/Server/Widgets/ServerOverview.php (1)
24-25: Avoid copying placeholder “None”. Make copyable conditional on a real address.Prevents copying meaningless text.
- SmallStatBlock::make(trans('server/console.labels.address'), $this->server?->allocation->address ?? 'None') - ->copyable(), + SmallStatBlock::make(trans('server/console.labels.address'), $this->server?->allocation->address ?? 'None') + ->copyable(fn (string $state): bool => $state !== 'None' && filled($state)),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
app/Filament/Admin/Resources/ApiKeys/ApiKeyResource.php(1 hunks)app/Filament/Admin/Resources/DatabaseHosts/Pages/CreateDatabaseHost.php(1 hunks)app/Filament/Admin/Resources/Servers/Pages/EditServer.php(2 hunks)app/Filament/App/Resources/Servers/Pages/ListServers.php(1 hunks)app/Filament/Server/Components/SmallStatBlock.php(1 hunks)app/Filament/Server/Pages/Settings.php(2 hunks)app/Filament/Server/Resources/Databases/DatabaseResource.php(3 hunks)app/Filament/Server/Widgets/ServerOverview.php(1 hunks)app/Providers/AppServiceProvider.php(1 hunks)lang/en/server/dashboard.php(0 hunks)resources/js/app.js(1 hunks)resources/views/filament/components/server-console.blade.php(1 hunks)resources/views/filament/components/server-small-data-block.blade.php(1 hunks)
💤 Files with no reviewable changes (1)
- lang/en/server/dashboard.php
🧰 Additional context used
🧬 Code graph analysis (3)
app/Filament/Admin/Resources/DatabaseHosts/Pages/CreateDatabaseHost.php (1)
app/Livewire/Installer/Steps/SessionStep.php (1)
make(19-36)
app/Filament/Server/Widgets/ServerOverview.php (1)
app/Filament/Server/Components/SmallStatBlock.php (1)
SmallStatBlock(8-13)
app/Providers/AppServiceProvider.php (1)
app/Livewire/Installer/Steps/QueueStep.php (1)
make(25-63)
🔇 Additional comments (5)
app/Filament/Admin/Resources/Servers/Pages/EditServer.php (2)
180-196: LGTM: copyable UUIDsUn‑gated copy for UUID/short UUID is fine here and respects readOnly/dehydrated flags.
191-201: LGTM: copyable short UUIDMatches the field above; consistent.
app/Providers/AppServiceProvider.php (1)
153-156: LGTM: unconditional hintCopy macroApproving; automated sweep returned no output — cannot confirm absence of HTTPS-gated copyable usages or the removed 'server/dashboard.copied' translation key. Re-run locally and paste results:
rg -nP "copyable\s*\(\s*fn\b.*isSecure" -C2 rg -nP "['\"]server/dashboard\.copied['\"]|trans\(['\"][^'\"]*copied['\"]\)|__\(['\"][^'\"]*copied['\"]\)" -n -C2app/Filament/Server/Widgets/ServerOverview.php (1)
21-22: LGTM: making name copyable aligns with the new SmallStatBlock behavior.No issues.
app/Filament/Server/Components/SmallStatBlock.php (1)
5-13: LGTM: adopt CanBeCopied and a dedicated view.Matches the PR’s direction and simplifies copy behavior. Please confirm the Filament version in this repo exposes the same
$isCopyable($state)and$getCopyMessage($state)helpers to the view viaCanBeCopied.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
♻️ Duplicate comments (1)
resources/js/app.js (1)
4-16: Harden the fallback for iOS/mobile, proper cleanup, and error objects.Current fallback may fail on iOS (no setSelectionRange), can leave selection active, and rejects with a string. Refactor as below.
- navigator.clipboard = navigator.clipboard || {}; - navigator.clipboard.writeText = async (text) => - new Promise((resolve, reject) => { - const textarea = document.createElement('textarea'); - textarea.value = text; - textarea.style.position = 'fixed'; - textarea.style.left = '-9999px'; - textarea.style.opacity = '0'; - document.body.appendChild(textarea); - textarea.select(); - const success = document.execCommand('copy'); - document.body.removeChild(textarea); - success ? resolve() : reject('Fallback copy failed'); - }); + // Ensure the clipboard object exists; bail out quietly if it's non-writable. + try { navigator.clipboard = navigator.clipboard || {}; } catch (_) { return; } + navigator.clipboard.writeText = async (text) => + new Promise((resolve, reject) => { + let textarea; + try { + const value = String(text ?? ''); + textarea = document.createElement('textarea'); + textarea.value = value; + textarea.setAttribute('readonly', ''); + textarea.style.position = 'fixed'; + textarea.style.top = '-9999px'; + textarea.style.left = '-9999px'; + textarea.style.opacity = '0'; + document.body.appendChild(textarea); + textarea.select(); + textarea.setSelectionRange(0, value.length); // iOS support + const ok = typeof document.execCommand === 'function' && document.execCommand('copy'); + document.body.removeChild(textarea); + const sel = document.getSelection?.(); + sel?.removeAllRanges?.(); // clear any selection side-effects + ok ? resolve() : reject(new Error('Fallback copy failed')); + } catch (e) { + try { textarea?.remove?.(); } catch {} + reject(e instanceof Error ? e : new Error(String(e))); + } + });
🧹 Nitpick comments (1)
resources/js/app.js (1)
1-1: Optional: SSR/worker safety guard.Avoid ReferenceError if this file ever loads in SSR/worker contexts.
(() => { + if (typeof navigator === 'undefined' || typeof document === 'undefined') return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
resources/js/app.js(1 hunks)resources/views/filament/components/server-small-data-block.blade.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- resources/views/filament/components/server-small-data-block.blade.php
🔇 Additional comments (1)
resources/js/app.js (1)
1-3: Good: polyfill only when native writeText is available (no clobbering).Early-return guard preserves native behavior and avoids replacing navigator.clipboard when supported.
No description provided.