Allow any Font Awesome style for the OAuth2 button icon (#7641)#10036
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe OAuth2 login button in ChangesOAuth2 Font Awesome Icon Style Support
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes 🚥 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
web/pgadmin/static/js/SecurityPages/LoginPage.jsx (1)
52-53: 💤 Low valueConsider adding Font Awesome 6 Sharp family styles for completeness.
The
iconStylesarray covers standard Font Awesome 6 styles but omits the Sharp family variants:fa-sharp-solid,fa-sharp-regular,fa-sharp-light, andfa-sharp-thin. While these are premium/less common styles, adding them would make the detection more comprehensive for users with Font Awesome Pro licenses.✨ Optional enhancement for Sharp styles
const iconStyles = ['fab', 'fas', 'far', 'fal', 'fat', 'fad', - 'fa-brands', 'fa-solid', 'fa-regular', 'fa-light', 'fa-thin', 'fa-duotone']; + 'fa-brands', 'fa-solid', 'fa-regular', 'fa-light', 'fa-thin', 'fa-duotone', + 'fa-sharp-solid', 'fa-sharp-regular', 'fa-sharp-light', 'fa-sharp-thin'];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/pgadmin/static/js/SecurityPages/LoginPage.jsx` around lines 52 - 53, The iconStyles array in LoginPage.jsx (variable name iconStyles) misses Font Awesome 6 Sharp family variants; update the array to include the sharp prefixes (e.g., 'fa-sharp-solid', 'fa-sharp-regular', 'fa-sharp-light', 'fa-sharp-thin') so detection covers Sharp styles as well — locate the iconStyles definition and append these four strings to the list.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@web/pgadmin/static/js/SecurityPages/LoginPage.jsx`:
- Around line 52-53: The iconStyles array in LoginPage.jsx (variable name
iconStyles) misses Font Awesome 6 Sharp family variants; update the array to
include the sharp prefixes (e.g., 'fa-sharp-solid', 'fa-sharp-regular',
'fa-sharp-light', 'fa-sharp-thin') so detection covers Sharp styles as well —
locate the iconStyles definition and append these four strings to the list.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a2b504fc-fc64-42e2-89d2-82107965ee96
📒 Files selected for processing (4)
docs/en_US/oauth2.rstdocs/en_US/release_notes.rstdocs/en_US/release_notes_9_16.rstweb/pgadmin/static/js/SecurityPages/LoginPage.jsx
7d41944 to
f4513a2
Compare
…7641 The login page hardcoded the 'fab' (brands) Font Awesome style for the OAuth2 button icon, so non-brand icons could not be used. Use the configured OAUTH2_ICON as-is when it already specifies a style class (e.g. 'fas fa-key'), and fall back to 'fab' when only an icon name is given, preserving backward compatibility. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
f4513a2 to
0616849
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes pgAdmin’s OAuth2 login button icon rendering to support any Font Awesome style class (not just fab), while preserving backward compatibility for existing configurations that only provide an icon name.
Changes:
- Update
LoginPage.jsxto useOAUTH2_ICONas-is when it already includes a Font Awesome style class; otherwise default tofab. - Clarify OAuth2 documentation to note that
OAUTH2_ICONmay include an explicit style class (e.g.fas fa-key). - Add a 9.16 release note entry referencing Issue #7641.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| web/pgadmin/static/js/SecurityPages/LoginPage.jsx | Implements style-aware OAUTH2_ICON handling with a backward-compatible fab fallback. |
| docs/en_US/oauth2.rst | Documents that OAUTH2_ICON may include a style class to select non-brand icons. |
| docs/en_US/release_notes_9_16.rst | Adds a release note entry for Issue #7641 describing the enhancement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
asheshv
left a comment
There was a problem hiding this comment.
No XSS — the value is rendered as className (not HTML), so this is fine. Two functional issues:
- Coverage gap. The
iconStyleslist includes the old aliasesfad/fas/fab/fal/farbut omitsfa-duotone,fa-sharp, andfa-sharp-duotonefrom the FA6 prefixing convention. A user who configuresfa-sharp fa-keyfalls throughhasStyle, gets'fab 'prepended, and silently sees the wrong icon. Either extend the list or replace it with a forward-compatible regex like/^fa[a-z-]*$/. iconStylesdeclared inside the.map()callback. Reallocated per-render per-provider. With one OAuth2 provider it's trivial, but hoist to module scope — it's a stable constant.
Summary
Fixes #7641.
The login page hardcoded the
fab(brands) Font Awesome style for the OAuth2 button icon (<Icon className={'fab '+oauth.OAUTH2_ICON} ...>), so administrators couldn't use non-brand icons (e.g. a genericfas fa-keyfor a custom provider).Now the configured
OAUTH2_ICONis used as-is when it already includes a style class, and falls back tofabwhen only an icon name is given — preserving existing configurations.Changes
LoginPage.jsx: derive the icon class fromOAUTH2_ICON, defaulting tofabonly when no style is present.oauth2.rst: document that a style class can be included.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
fas fa-key), not just brand icons.Documentation