Skip to content

Fix:Bug: Responsive issue in mid width range when name field is empty and copy to clipboard not working#1573

Open
francmart514 wants to merge 2 commits intowe-promise:mainfrom
francmart514:fix/issue7-admin-sso-new-responsive-copy
Open

Fix:Bug: Responsive issue in mid width range when name field is empty and copy to clipboard not working#1573
francmart514 wants to merge 2 commits intowe-promise:mainfrom
francmart514:fix/issue7-admin-sso-new-responsive-copy

Conversation

@francmart514
Copy link
Copy Markdown

@francmart514 francmart514 commented Apr 28, 2026

Description

This PR fixes two regressions on Admin -> SSO Providers -> Add SSO Provider (/admin/sso_providers/new) : medium-width responsive clipping and callback URL copy-to-clipboard failure.

Closes #1572

Changes made:
Updated settings layout containers to allow shrinking/wrapping at medium widths (min-w-0, header/action wrap), preventing content clipping when the sidebar is visible.
Hardened callback URL rows in the SSO form with responsive overflow-safe classes (min-w-0, overflow-x-auto, break-all, whitespace-pre-wrap).
Switched SSO callback copy buttons to the shared clipboard Stimulus controller (same pattern as API key copy), instead of custom copy handlers.
Removed custom SSO form copy methods (copyCallback, copySamlCallback) to avoid browser-specific clipboard failures and keep behavior consistent.

Summary by CodeRabbit

  • New Features

    • Callback URL copy-buttons now use a unified clipboard handler with updated success feedback (temporary success icon/text) and improved error handling.
  • Style

    • Callback URL text wraps better for long values.
    • Settings and form action rows now wrap on smaller screens for improved responsive layout.

@brin-security-scanner brin-security-scanner Bot added the contributor:flagged Contributor flagged for review by trust analysis. label Apr 28, 2026
@brin-security-scanner
Copy link
Copy Markdown

brin-security-scanner Bot commented Apr 28, 2026

⚠️ Contributor Trust Check — Review Recommended

This contributor's profile shows patterns that may warrant additional review. This is based on their GitHub activity, not the contents of this PR.

francmart514 · Score: 77/100

Dimension breakdown
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
Copy link
Copy Markdown

coderabbitai Bot commented Apr 28, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2195ff5e-40d6-4ad9-bde1-0d39da678fcb

📥 Commits

Reviewing files that changed from the base of the PR and between 5ae86d3 and 31812e0.

📒 Files selected for processing (1)
  • app/views/admin/sso_providers/_form.html.erb
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/views/admin/sso_providers/_form.html.erb

📝 Walkthrough

Walkthrough

Removed custom clipboard copy methods from the SSO form Stimulus controller and switched callback URL copy buttons to a shared clipboard controller. Adjusted callback URL markup/styling and updated layout flex constraints and wrapping to fix responsive behavior.

Changes

Cohort / File(s) Summary
Clipboard Controller Removal
app/javascript/controllers/admin_sso_form_controller.js
Deleted copySamlCallback and copyCallback methods that handled clipboard writes, button feedback, and error handling.
Clipboard Integration & URL Display
app/views/admin/sso_providers/_form.html.erb
Replaced admin-sso-form#copy* actions with clipboard#copy; made callback <code> a data-clipboard-target="source", added clipboard icon targets (iconDefault, iconSuccess), and adjusted URL styling to allow wrapping and better min-width behavior. Footer/action row now uses flex-wrap.
Flex Layout Constraints
app/views/layouts/settings.html.erb
Added min-w-0 to nested flex containers, changed header row to flex-wrap gap-3, and updated page_actions sizing from shrink-0 to max-w-full to improve responsive layout behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hopped through code to mend a broken clip,
Swapped actions for a shared, steadfast grip,
Flex boxes now bow and gently embrace,
Callback URLs wrapped in a cozy space,
A tiny hop — and the form found its grace. ✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes both main issues fixed: responsive layout problem at mid-widths and copy-to-clipboard functionality, matching the code changes.
Linked Issues check ✅ Passed The PR successfully addresses both objectives from issue #1572: responsive layout fixes via min-w-0 and flex-wrap classes, and clipboard reliability by switching to the shared clipboard controller.
Out of Scope Changes check ✅ Passed All changes align with the linked issue requirements; no out-of-scope modifications were introduced beyond the responsive and clipboard fixes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@brin-security-scanner brin-security-scanner Bot added the pr:verified PR passed security analysis. label Apr 28, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5ae86d31c0

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

data-clipboard-target="source"><%= "#{request.base_url}/auth/#{sso_provider.name.presence || 'PROVIDER_NAME'}/callback" %></code>
<button type="button"
data-action="click->admin-sso-form#copyCallback"
data-action="clipboard#copy"
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Provide required clipboard icon targets

clipboard#copy now handles these SSO buttons, but this markup only defines data-clipboard-target="source". The shared controller’s showSuccess method unconditionally reads iconDefaultTarget/iconSuccessTarget (app/javascript/controllers/clipboard_controller.js), so clicking this button throws a missing-target error after writeText resolves and falls into the catch path. In practice this produces a false “copy failed” error and no success feedback on every click for the new SSO callback copy UI.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
app/views/admin/sso_providers/_form.html.erb (1)

93-103: ⚠️ Potential issue | 🟠 Major

Clipboard controller targets incomplete on both instances—runtime error on successful copy.

Both clipboard instances (lines 93–103 and 177–187) only provide the source target. The controller's showSuccess() method directly references iconDefaultTarget and iconSuccessTarget without checking existence, causing a runtime error when copy succeeds.

Wrap button icons in spans with the missing targets:

Fix for lines 97–102
        <button type="button"
                data-action="clipboard#copy"
                class="p-2 text-secondary hover:text-primary shrink-0"
                title="Copy to clipboard">
+         <span data-clipboard-target="iconDefault">
            <%= icon "copy", class: "w-4 h-4" %>
+         </span>
+         <span data-clipboard-target="iconSuccess" class="hidden">
+           <%= icon "check", class: "w-4 h-4" %>
+         </span>
        </button>

Apply the same fix to the second instance at lines 181–186.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/views/admin/sso_providers/_form.html.erb` around lines 93 - 103, The
clipboard controller is missing targets for the button icons used by
showSuccess() (it expects iconDefaultTarget and iconSuccessTarget), causing a
runtime error; update both instances that currently only set
data-clipboard-target="source" (the code block with
data-admin-sso-form-target="callbackUrl" and the second similar instance) to
include the missing targets by wrapping the copy button's icon markup in
elements that provide data-clipboard-target="iconDefault" and
data-clipboard-target="iconSuccess" (or the exact target names used by the
controller) so showSuccess() can safely find iconDefaultTarget and
iconSuccessTarget on success.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@app/views/admin/sso_providers/_form.html.erb`:
- Around line 93-103: The clipboard controller is missing targets for the button
icons used by showSuccess() (it expects iconDefaultTarget and
iconSuccessTarget), causing a runtime error; update both instances that
currently only set data-clipboard-target="source" (the code block with
data-admin-sso-form-target="callbackUrl" and the second similar instance) to
include the missing targets by wrapping the copy button's icon markup in
elements that provide data-clipboard-target="iconDefault" and
data-clipboard-target="iconSuccess" (or the exact target names used by the
controller) so showSuccess() can safely find iconDefaultTarget and
iconSuccessTarget on success.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0a6ce3fe-5ebd-46e8-95b3-8169fdab5075

📥 Commits

Reviewing files that changed from the base of the PR and between 3960582 and 5ae86d3.

📒 Files selected for processing (3)
  • app/javascript/controllers/admin_sso_form_controller.js
  • app/views/admin/sso_providers/_form.html.erb
  • app/views/layouts/settings.html.erb
💤 Files with no reviewable changes (1)
  • app/javascript/controllers/admin_sso_form_controller.js

@francmart514
Copy link
Copy Markdown
Author

@jjmata , waiting to merge

@francmart514
Copy link
Copy Markdown
Author

image

@jjmata , what is more needed to do on my side to make this pull request merge?

@jjmata
Copy link
Copy Markdown
Collaborator

jjmata commented Apr 29, 2026

If you start nagging like that every few minutes we might just stop looking ... did you see the Gittensor contribs notice?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr:verified PR passed security analysis.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: Responsive issue in mid width range when name field is empty and copy to clipboard not working in admin/sso_providers/new page

2 participants