Fix:Bug:API Key copy action lacks success feedback (Copied! text/icon/tooltip) after clicking <Copy API Key>#1564
Open
francmart514 wants to merge 4 commits intowe-promise:mainfrom
Open
Conversation
…/tooltip) after clicking <Copy API Key>
📝 WalkthroughWalkthroughThe Stimulus clipboard controller was extended to support success text targets, a configurable success duration, robust reset timeout handling (cleared on new timers and on disconnect), and helper methods; the copy button markup was extracted to a shared partial; a system test verifies the temporary success feedback and locales for copy labels were added. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant Stimulus as ClipboardController
participant ClipboardAPI as navigator.clipboard
participant DOM as View
User->>Browser: click "Copy API Key" button
Browser->>Stimulus: trigger clipboard#copy action
Stimulus->>DOM: read `data-clipboard-target="source"` content
Stimulus->>ClipboardAPI: writeText(key)
ClipboardAPI-->>Stimulus: promise resolved
Stimulus->>DOM: toggle iconDefault -> hidden
Stimulus->>DOM: toggle iconSuccess -> shown
Stimulus->>DOM: toggle textDefault -> hidden
Stimulus->>DOM: toggle textSuccess -> shown
Stimulus->>Stimulus: set resetTimeout(successDurationValue)
Note over Stimulus,DOM: after timeout -> restore default icon/text (if still connected)
Browser->>Stimulus: disconnect (navigation/unmount)
Stimulus-->>Stimulus: clear resetTimeout
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
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)
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 |
brin-security-scanner
Bot
added
the
contributor:flagged
|
| Dimension | Score | What it measures |
|---|---|---|
| Identity | 45 | Account age, contribution history, GPG keys, org memberships |
| Behavior | 90 | PR patterns, unsolicited contribution ratio, activity cadence |
| Content | 100 | PR body substance, issue linkage, contribution quality |
| Graph | 30 | Cross-repo trust, co-contributor relationships |
Analyzed by Brin · Full profile
![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
🧹 Nitpick comments (1)
app/javascript/controllers/clipboard_controller.js (1)
4-44: LGTM — clean, backward-compatible refactor.The
toggleTargethelper plus thehas{Name}Targetguard means existing call sites that only defineiconDefault/iconSuccess(e.g.app/views/mfa/new.html.erb,app/views/invite_codes/_invite_code.html.erb,app/views/settings/profiles/show.html.erb) keep working without changes. ClearingresetTimeoutindisconnect()mirrors the pattern inapp/javascript/controllers/turbo_frame_timeout_controller.js. Target count (5) stays under the 7-target guideline.One tiny optional nit: the
2000ms timeout is a magic number — consider promoting it to a static or adata-*-value-driven duration so views/tests can override it.♻️ Optional nit
export default class extends Controller { static targets = ["source", "iconDefault", "iconSuccess", "textDefault", "textSuccess"]; + static values = { resetMs: { type: Number, default: 2000 } }; @@ - this.resetTimeout = setTimeout(() => { + this.resetTimeout = setTimeout(() => { this.toggleTarget("iconDefault", false); this.toggleTarget("iconSuccess", true); this.toggleTarget("textDefault", false); this.toggleTarget("textSuccess", true); - }, 2000); + }, this.resetMsValue);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/javascript/controllers/clipboard_controller.js` around lines 4 - 44, The 2000ms magic number in showSuccess should be configurable; add a Stimulus value (e.g. static values = { duration: Number }) or a static property (e.g. static DURATION = 2000) to clipboard_controller and replace the hard-coded 2000 with this value (use this.durationValue || clipboard_controller.DURATION or similar), and ensure disconnect still clears this.resetTimeout; update showSuccess to use the configurable duration when calling setTimeout and keep the rest of the logic unchanged so views/tests can override the duration via data-duration-value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/views/settings/api_keys/created.turbo_stream.erb`:
- Around line 32-37: The duplicated copy-button markup (the button with
data-action="clipboard#copy" and data-clipboard-target spans) should be
extracted into a single partial named settings/api_keys/_copy_button.html.erb;
replace raw hover classes hover:bg-gray-100 theme-dark:hover:bg-gray-700 with
the design token hover:bg-surface-inset-hover and replace hardcoded strings
"Copy API Key" and "Copied!" with i18n lookups
t("settings.api_keys.copy_button.default") and
t("settings.api_keys.copy_button.success"); then remove the inline button blocks
from created.turbo_stream.erb, created.html.erb, and show.html.erb and render
the partial via <%= render "settings/api_keys/copy_button" %> so any future
changes (targets, icons, classes) are centralized.
In `@test/system/settings/api_keys_test.rb`:
- Around line 86-101: The success state can disappear before Capybara asserts
because the controller resets after 2000ms; update the test "should show
temporary copied feedback when copying api key" to avoid flakiness by wrapping
the two assertions for 'span[data-clipboard-target="textSuccess"]' and
'span[data-clipboard-target="iconSuccess"]' inside a transient longer wait
(e.g., using using_wait_time) or by modifying the component to accept a
data-attribute for the reset duration and set that longer only for tests; locate
the test in settings/api_keys_test.rb and either temporarily increase Capybara's
wait around the click/assert block or add a test-only data-reset-value to the
copy button/component and set it longer for this test, then restore default
behavior for production.
---
Nitpick comments:
In `@app/javascript/controllers/clipboard_controller.js`:
- Around line 4-44: The 2000ms magic number in showSuccess should be
configurable; add a Stimulus value (e.g. static values = { duration: Number })
or a static property (e.g. static DURATION = 2000) to clipboard_controller and
replace the hard-coded 2000 with this value (use this.durationValue ||
clipboard_controller.DURATION or similar), and ensure disconnect still clears
this.resetTimeout; update showSuccess to use the configurable duration when
calling setTimeout and keep the rest of the logic unchanged so views/tests can
override the duration via data-duration-value.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee216cdd-01c7-47b9-81df-a0425840c687
📒 Files selected for processing (5)
app/javascript/controllers/clipboard_controller.jsapp/views/settings/api_keys/created.html.erbapp/views/settings/api_keys/created.turbo_stream.erbapp/views/settings/api_keys/show.html.erbtest/system/settings/api_keys_test.rb
…d button color, label
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/javascript/controllers/clipboard_controller.js (1)
36-45: Optional: tighten timeout cleanup and marktoggleTargetas private.Two small alignment nits with conventions/codebase patterns:
toggleTargetis a controller-internal helper but is exposed as a public method. Per coding guidelines, Stimulus controllers should "use private methods and expose a clear public API." Renaming with#(or a leading_) signals it isn't part of the controller's public surface.- Other controllers in this repo (e.g.
turbo_frame_timeout_controller.js) null out the stored timeout id after clearing, which avoids any chance of acting on a stale handle.♻️ Proposed refactor
showSuccess() { - this.toggleTarget("iconDefault", true); - this.toggleTarget("iconSuccess", false); - this.toggleTarget("textDefault", true); - this.toggleTarget("textSuccess", false); + this.#toggleTarget("iconDefault", true); + this.#toggleTarget("iconSuccess", false); + this.#toggleTarget("textDefault", true); + this.#toggleTarget("textSuccess", false); - clearTimeout(this.resetTimeout); + if (this.resetTimeout) clearTimeout(this.resetTimeout); this.resetTimeout = setTimeout(() => { - this.toggleTarget("iconDefault", false); - this.toggleTarget("iconSuccess", true); - this.toggleTarget("textDefault", false); - this.toggleTarget("textSuccess", true); + this.#toggleTarget("iconDefault", false); + this.#toggleTarget("iconSuccess", true); + this.#toggleTarget("textDefault", false); + this.#toggleTarget("textSuccess", true); + this.resetTimeout = null; }, this.successDurationValue); } disconnect() { - clearTimeout(this.resetTimeout); + if (this.resetTimeout) { + clearTimeout(this.resetTimeout); + this.resetTimeout = null; + } } - toggleTarget(targetName, hide) { + `#toggleTarget`(targetName, hide) { const hasTarget = this[`has${targetName[0].toUpperCase()}${targetName.slice(1)}Target`]; if (!hasTarget) return; this[`${targetName}Target`].classList.toggle("hidden", hide); }As per coding guidelines: "Use private methods in Stimulus controllers and expose a clear public API."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/javascript/controllers/clipboard_controller.js` around lines 36 - 45, The controller exposes an internal helper and leaves a stale timeout handle; make toggleTarget private (rename toggleTarget to a private method like `#toggleTarget` or _toggleTarget) and update disconnect() to both clearTimeout(this.resetTimeout) and set this.resetTimeout = null to match other controllers (refer to the existing toggleTarget method and disconnect function names to locate the changes); ensure any internal calls referencing toggleTarget are updated to use the new private name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/javascript/controllers/clipboard_controller.js`:
- Around line 36-45: The controller exposes an internal helper and leaves a
stale timeout handle; make toggleTarget private (rename toggleTarget to a
private method like `#toggleTarget` or _toggleTarget) and update disconnect() to
both clearTimeout(this.resetTimeout) and set this.resetTimeout = null to match
other controllers (refer to the existing toggleTarget method and disconnect
function names to locate the changes); ensure any internal calls referencing
toggleTarget are updated to use the new private name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ab2bcf3-d4e3-481a-a6fa-d763937a3870
📒 Files selected for processing (7)
app/javascript/controllers/clipboard_controller.jsapp/views/settings/api_keys/_copy_button.html.erbapp/views/settings/api_keys/created.html.erbapp/views/settings/api_keys/created.turbo_stream.erbapp/views/settings/api_keys/show.html.erbconfig/locales/views/settings/api_keys/en.ymltest/system/settings/api_keys_test.rb
✅ Files skipped from review due to trivial changes (2)
- app/views/settings/api_keys/_copy_button.html.erb
- config/locales/views/settings/api_keys/en.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- app/views/settings/api_keys/created.turbo_stream.erb
- app/views/settings/api_keys/created.html.erb
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/javascript/controllers/clipboard_controller.js (2)
21-35: Toggle logic is correct, but the success state isn't reasserted on the DOM the second time around.If
showSuccess()is invoked again while a reset is already pending, lines 22–25 will be a no-op onclassList.toggle("hidden", boolean)(idempotent) — that's fine. However, the timer is correctly cleared and rescheduled via_clearResetTimeout()on line 27, so the visible "Copied!" window is extended as a user expects. Behavior verified.Minor optional cleanup: the four
_toggleTargetcalls in each branch could be condensed to reduce repetition, e.g.:♻️ Optional refactor
showSuccess() { - this._toggleTarget("iconDefault", true); - this._toggleTarget("iconSuccess", false); - this._toggleTarget("textDefault", true); - this._toggleTarget("textSuccess", false); - - this._clearResetTimeout(); - this.resetTimeout = setTimeout(() => { - this._toggleTarget("iconDefault", false); - this._toggleTarget("iconSuccess", true); - this._toggleTarget("textDefault", false); - this._toggleTarget("textSuccess", true); - this.resetTimeout = null; - }, this.successDurationValue); + this._setSuccessState(true); + this._clearResetTimeout(); + this.resetTimeout = setTimeout(() => { + this._setSuccessState(false); + this.resetTimeout = null; + }, this.successDurationValue); } + + _setSuccessState(success) { + this._toggleTarget("iconDefault", success); + this._toggleTarget("iconSuccess", !success); + this._toggleTarget("textDefault", success); + this._toggleTarget("textSuccess", !success); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/javascript/controllers/clipboard_controller.js` around lines 21 - 35, showSuccess() currently relies on idempotent toggles so calling it while a reset is pending can leave the DOM unchanged; change the four _toggleTarget(...) calls in both the immediate branch and the timeout callback to explicitly set the hidden state instead of toggling (e.g. implement/use a helper like _setTargetHidden(targetName, hidden) or modify _toggleTarget to always call element.classList.toggle('hidden', Boolean(force)) so it forces the desired visible/hidden state); update calls for "iconDefault", "iconSuccess", "textDefault", and "textSuccess" and keep _clearResetTimeout(), resetTimeout handling and successDurationValue unchanged.
46-51: Use Stimulus's typedhasIconDefaultTargetand similar accessors directly.The dynamic string construction works, but since targets are predefined in
static targets, Stimulus already creates typed accessors likehasIconDefaultTarget,hasIconSuccessTarget, etc. Using these directly would be more readable and greppable than indexing intothisvia a computed string.Refactor the
_toggleTarget()method to accept the target element directly, or call the specific accessors:Example refactor
_toggleTarget(target, hide) { target.classList.toggle("hidden", hide); } // Then in showSuccess(): this._toggleTarget(this.iconDefaultTarget, true); this._toggleTarget(this.iconSuccessTarget, false); // ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/javascript/controllers/clipboard_controller.js` around lines 46 - 51, Refactor _toggleTarget to stop computing property names and instead accept a target element (or call Stimulus's typed accessors) directly: change _toggleTarget(target, hide) to operate on the passed DOM element (target.classList.toggle("hidden", hide)), then update callers (e.g. in showSuccess) to pass this.iconDefaultTarget, this.iconSuccessTarget, etc., or use the typed boolean accessors like this.hasIconDefaultTarget before calling; ensure any places that previously referenced has{Name}Target via computed strings now use the Stimulus-generated accessors such as hasIconDefaultTarget and iconDefaultTarget.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/javascript/controllers/clipboard_controller.js`:
- Around line 21-35: showSuccess() currently relies on idempotent toggles so
calling it while a reset is pending can leave the DOM unchanged; change the four
_toggleTarget(...) calls in both the immediate branch and the timeout callback
to explicitly set the hidden state instead of toggling (e.g. implement/use a
helper like _setTargetHidden(targetName, hidden) or modify _toggleTarget to
always call element.classList.toggle('hidden', Boolean(force)) so it forces the
desired visible/hidden state); update calls for "iconDefault", "iconSuccess",
"textDefault", and "textSuccess" and keep _clearResetTimeout(), resetTimeout
handling and successDurationValue unchanged.
- Around line 46-51: Refactor _toggleTarget to stop computing property names and
instead accept a target element (or call Stimulus's typed accessors) directly:
change _toggleTarget(target, hide) to operate on the passed DOM element
(target.classList.toggle("hidden", hide)), then update callers (e.g. in
showSuccess) to pass this.iconDefaultTarget, this.iconSuccessTarget, etc., or
use the typed boolean accessors like this.hasIconDefaultTarget before calling;
ensure any places that previously referenced has{Name}Target via computed
strings now use the Stimulus-generated accessors such as hasIconDefaultTarget
and iconDefaultTarget.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 73bc060a-9631-4456-a5e6-ef1207a447fc
📒 Files selected for processing (1)
app/javascript/controllers/clipboard_controller.js
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/javascript/controllers/clipboard_controller.js (1)
21-29: Consider makingshowSuccessprivate.It's only invoked internally from
copy()and isn't part of the controller's declarative public API (nodata-actionreferences it). Renaming to_showSuccesswould keep the public surface limited tocopy, consistent with the underscore-prefixed private helpers introduced in this PR.As per coding guidelines: "Use private methods in Stimulus controllers and expose a clear public API".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/javascript/controllers/clipboard_controller.js` around lines 21 - 29, Rename the internal helper showSuccess to _showSuccess and update all internal references (notably the call from copy()) so the method follows the controller's private naming convention; keep the implementation identical (it should still call _setSuccessState(true), clear/reset this.resetTimeout, and use this.successDurationValue) and ensure any existing references to showSuccess (e.g., in copy() or other helper methods) are updated to _showSuccess so the public Stimulus API remains limited to copy().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/javascript/controllers/clipboard_controller.js`:
- Around line 21-29: Rename the internal helper showSuccess to _showSuccess and
update all internal references (notably the call from copy()) so the method
follows the controller's private naming convention; keep the implementation
identical (it should still call _setSuccessState(true), clear/reset
this.resetTimeout, and use this.successDurationValue) and ensure any existing
references to showSuccess (e.g., in copy() or other helper methods) are updated
to _showSuccess so the public Stimulus API remains limited to copy().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94847e3c-ab29-4a28-b2ee-2779e0ac9e0c
📒 Files selected for processing (1)
app/javascript/controllers/clipboard_controller.js
jjmata
removed
the
contributor:flagged
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
2 participants
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.
Description
This PR improves the API key copy-to-clipboard UX by adding immediate, temporary success feedback after clicking Copy API Key.
Previously, the key was copied correctly but the UI did not show a clear confirmation state.
Closes #1563
Changes made:
Updated clipboard_controller.js to support optional text targets (textDefault, textSuccess) and to toggle both icon and text success states.
Added reset handling (disconnect) and target-safe toggling so existing clipboard usages remain compatible.
Replaced API key copy buttons in:
app/views/settings/api_keys/show.html.erb
app/views/settings/api_keys/created.html.erb
app/views/settings/api_keys/created.turbo_stream.erb with markup that shows Copied! and a check icon briefly after successful copy.
Added system test coverage in test/system/settings/api_keys_test.rb for the temporary copied-feedback behavior.
Summary by CodeRabbit
New Features
Bug Fixes
Tests