fix(settings): show 'Copied!' feedback on API key copy button (#1563)#1890
fix(settings): show 'Copied!' feedback on API key copy button (#1563)#1890RenzoMXD wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAPI key views now render two clipboard-targeted button states (default and hidden success) so the clipboard controller can toggle to a "Copied!" state; three locale entries for ChangesAPI Key Copy Button Success Feedback
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
093e436 to
42dc0df
Compare
|
Heads-up: duplicate fix in review PR #1885 ( Both PRs are open and non-draft as of today. Maintainers should pick one to merge and close the other to avoid a double-apply. A quick diff comparison would confirm which implementation is preferred. Posted by automated PR review scan. Generated by Claude Code |
The api_keys show/created views wired DS::Button to clipboard#copy without supplying the iconDefault/iconSuccess targets that clipboard_controller#showSuccess expects. The clipboard write succeeded but showSuccess threw on the missing target, so users saw no visual confirmation. Wrap each Copy button in iconDefault/iconSuccess spans containing a "Copy API Key" / "Copied!" pair of DS::Buttons, matching the existing pattern in mfa/new, invite_codes, and settings/profiles. The icon and label swap for ~3s after copy, then revert. Fixes we-promise#1563
42dc0df to
bca330c
Compare
|
@coderabbitai please review again. |
|
✅ Actions performedFull review triggered. |
ReviewSimple, correct fix. The One minor UX note: Both the Missing locale key at end of file: The Otherwise looks good — the three copy button locations are consistently updated. Generated by Claude Code |
|
@jjmata Thanks for your review. |
Summary
clipboard_controller.jsalready handles the swap; the views just weren't supplying the targets it needs.Root cause
clipboard_controller#showSuccess()togglesdata-clipboard-target="iconDefault"andiconSuccesselements. The api_keys views rendered a singleDS::Buttonwithdata-action="clipboard#copy"but did not provide those targets, so the controller threw a missing-target error on click and no feedback rendered.Fix
Wrap each
DS::ButtonCopy action iniconDefault/iconSuccessclipboard-target spans, with the success span containing a parallelDS::Buttonusing the check icon and the new "Copied!" label. This matches the existing working pattern inapp/views/mfa/new.html.erb,app/views/invite_codes/_invite_code.html.erb, andapp/views/settings/profiles/show.html.erb.Files:
app/views/settings/api_keys/show.html.erb— both the newly created and current_api_key sections.app/views/settings/api_keys/created.html.erb.config/locales/views/settings/api_keys/en.yml— addedcopied: "Copied!"under each affected section.Test plan
settings/api_keys/show(currently active key) andsettings/api_keys/created(just-after-creation flow).bundle exec erb_lint app/views/settings/api_keys/show.html.erb app/views/settings/api_keys/created.html.erb→ No errors.bundle exec brakeman --no-pager --no-progress→ 0 security warnings.Notes
en.ymlis updated; the other 13 locale files forsettings/api_keysfall back to English for the newcopiedkey. Translation PRs can follow separately, consistent with how other recent UI strings have been added.fix/1668-slug because this branch was repurposed in-place — the previous content (transaction status filter forenable_banking) is now covered by the merged work tracked in fix(transactions): include enable_banking in pending/confirmed status filter #1885.Fixes #1563
Summary by CodeRabbit