Enhance login styling and add iframe defenses with dark mode#4
Open
nolderoos wants to merge 11 commits into
Open
Enhance login styling and add iframe defenses with dark mode#4nolderoos wants to merge 11 commits into
nolderoos wants to merge 11 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds admin-controlled branding and theming with WCAG contrast utilities, appearance sanitizers, admin collapsible sections, login-shell theme rendering, dark-mode tokens, JS helpers, tests, installer defaults, and login security headers. ChangesBranding, Theming, and Contrast Validation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
assets/css/magicauth.css (1)
454-454: ⚡ Quick winConsider extracting the dark surface constant.
The comment notes that
--magicauth-color-surface:#181c22`` must matchAdmin\Settings::DARK_SURFACE_HEX. Hardcoding the same hex in two places creates a maintenance risk—if the PHP constant is updated but the CSS is overlooked, the contrast validation logic will diverge from the rendered surface. Consider dynamically injecting this value via inline styles (similar to how `templates/login-shell.php` emits other theme variables) or documenting this sync requirement more prominently in both locations.🤖 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 `@assets/css/magicauth.css` at line 454, The CSS hardcodes --magicauth-color-surface: `#181c22` which must match the PHP constant Admin\Settings::DARK_SURFACE_HEX; extract this value so it’s injected dynamically instead of duplicated. Update the code that renders theme variables (e.g., templates/login-shell.php) to emit the surface hex into the stylesheet or as an inline style variable used by magicauth.css (replace the hardcoded --magicauth-color-surface), or if injection isn’t possible add a prominent comment linking to Admin\Settings::DARK_SURFACE_HEX to ensure both places stay in sync.includes/Admin/Settings.php (2)
35-38: ⚡ Quick winHardcoded surface color must stay synchronized with CSS.
The comment correctly notes that
DARK_SURFACE_HEXmust match the value inmagicauth.css. However, there's no automated check to prevent drift. If the CSS value changes without updating this constant, the dark-mode contrast validation will use stale data.Consider adding a test assertion that parses the CSS file and verifies the surface color matches this constant, or document the sync requirement in both locations more prominently.
🤖 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 `@includes/Admin/Settings.php` around lines 35 - 38, Add an automated assertion that the DARK_SURFACE_HEX constant in Settings (Settings::DARK_SURFACE_HEX) stays in sync with the CSS by parsing the magicauth.css file at test time and comparing the extracted body/.magicauth-mode-dark surface color token to Settings::DARK_SURFACE_HEX; implement this as a unit/integration test that fails when the colors differ so developers must update the constant when changing the CSS (or vice versa). Ensure the test locates the magicauth.css resource used in production, extracts the hex value used for body.magicauth-mode-dark or .magicauth-mode-auto, and asserts equality to the Settings constant, with a clear failure message indicating which source needs updating.
648-660: ⚡ Quick winExtract the 1 MB threshold to a constant.
Lines 649 and 656 both reference
1024 * 1024(1 MB). Extracting this to a named constant (e.g.,BACKGROUND_SIZE_SOFT_LIMIT_BYTES) would improve maintainability and make the threshold more discoverable if it needs adjustment.♻️ Proposed refactor
Near the top of the class:
private const BACKGROUND_MIMES = [ 'png' => 'image/png', 'jpg|jpeg' => 'image/jpeg', 'webp' => 'image/webp', ]; + + // Soft warning threshold for background images (bytes). Never blocks. + private const BACKGROUND_SIZE_SOFT_LIMIT_BYTES = 1024 * 1024; // 1 MBThen update the function:
- $size = (int) `@filesize`( $path ); - if ( $size > 1024 * 1024 ) { + $size = (int) `@filesize`( $path ); + if ( $size > self::BACKGROUND_SIZE_SOFT_LIMIT_BYTES ) { add_settings_error( self::OPTION_NAME, 'magicauth_bg_large', sprintf( /* translators: %s: file size in MB */ __( 'Background image saved. File is %s MB — consider compressing for faster page load.', 'magicauth' ), - number_format( $size / ( 1024 * 1024 ), 1 ) + number_format( $size / self::BACKGROUND_SIZE_SOFT_LIMIT_BYTES, 1 ) ), 'warning' );🤖 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 `@includes/Admin/Settings.php` around lines 648 - 660, Extract the numeric 1 MB literal into a class constant and use it in both places; add a class-level constant (e.g., BACKGROUND_SIZE_SOFT_LIMIT_BYTES = 1024 * 1024) inside the Settings class in includes/Admin/Settings.php, then replace the two occurrences of 1024 * 1024 in the method that checks $size (the comparison $size > 1024 * 1024 and the division number_format( $size / ( 1024 * 1024 ), 1 )) with that constant (use it for the comparison and for converting bytes to MB by dividing $size by BACKGROUND_SIZE_SOFT_LIMIT_BYTES) so behavior remains identical but the threshold is discoverable and configurable.
🤖 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 `@assets/css/magicauth.css`:
- Line 454: The CSS hardcodes --magicauth-color-surface: `#181c22` which must
match the PHP constant Admin\Settings::DARK_SURFACE_HEX; extract this value so
it’s injected dynamically instead of duplicated. Update the code that renders
theme variables (e.g., templates/login-shell.php) to emit the surface hex into
the stylesheet or as an inline style variable used by magicauth.css (replace the
hardcoded --magicauth-color-surface), or if injection isn’t possible add a
prominent comment linking to Admin\Settings::DARK_SURFACE_HEX to ensure both
places stay in sync.
In `@includes/Admin/Settings.php`:
- Around line 35-38: Add an automated assertion that the DARK_SURFACE_HEX
constant in Settings (Settings::DARK_SURFACE_HEX) stays in sync with the CSS by
parsing the magicauth.css file at test time and comparing the extracted
body/.magicauth-mode-dark surface color token to Settings::DARK_SURFACE_HEX;
implement this as a unit/integration test that fails when the colors differ so
developers must update the constant when changing the CSS (or vice versa).
Ensure the test locates the magicauth.css resource used in production, extracts
the hex value used for body.magicauth-mode-dark or .magicauth-mode-auto, and
asserts equality to the Settings constant, with a clear failure message
indicating which source needs updating.
- Around line 648-660: Extract the numeric 1 MB literal into a class constant
and use it in both places; add a class-level constant (e.g.,
BACKGROUND_SIZE_SOFT_LIMIT_BYTES = 1024 * 1024) inside the Settings class in
includes/Admin/Settings.php, then replace the two occurrences of 1024 * 1024 in
the method that checks $size (the comparison $size > 1024 * 1024 and the
division number_format( $size / ( 1024 * 1024 ), 1 )) with that constant (use it
for the comparison and for converting bytes to MB by dividing $size by
BACKGROUND_SIZE_SOFT_LIMIT_BYTES) so behavior remains identical but the
threshold is discoverable and configurable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 79d6bced-0008-472a-b110-ef7f0a46163e
📒 Files selected for processing (11)
assets/css/magicauth-admin.cssassets/css/magicauth.cssincludes/Admin/Settings.phpincludes/Frontend/LoginScreen.phpincludes/Installer.phpincludes/helpers.phpreadme.txttemplates/login-shell.phptests/phpunit/Model/ContrastRatioTest.phptests/phpunit/Model/SettingsSanitizeTest.phptests/phpunit/stubs/wp-functions.php
fix: split branding and appearance
fix: link color follows brand when blank
fix: match diagnostics row padding
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@assets/js/magicauth-admin.js`:
- Around line 130-151: The swatch follower isn't initialized on first render so
it can show a stale color until input occurs; call mirrorFromSource() once
during setup when the follower is "connected" to the source: after defining
isConnected() and mirrorFromSource(), check if isConnected() is true and if so
invoke mirrorFromSource() (using the same HEX_RE/normalize logic) so that the
swatch and text are synchronized immediately on load; reference the
functions/variables isConnected, mirrorFromSource, text, sourceText, swatch,
HEX_RE, and normalize when adding this initial call.
🪄 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 Plus
Run ID: 969d80af-9493-449b-b958-1ceb75b12c71
📒 Files selected for processing (3)
assets/css/magicauth-admin.cssassets/js/magicauth-admin.jsincludes/Admin/Settings.php
🚧 Files skipped from review as they are similar to previous changes (1)
- includes/Admin/Settings.php
Updates the Plugin URI header so the "Visit plugin site" link on the WordPress Plugins screen resolves to the canonical product page instead of the GitHub source repo.
- Item 4 (Minor): initialize follower swatch on first render so the preview matches the source picker before any user input. #4 (comment) - Item 5 (Nitpick): add prominent bidirectional SYNC-LOCK comments cross-referencing the duplicated dark-surface hex in both assets/css/magicauth.css and includes/Admin/Settings.php. Chose option (c) — option (a) would require injecting only one of nine dark-mode tokens via PHP, creating asymmetry vs the eight others that legitimately live in CSS. - Item 6 (Nitpick): same SYNC-LOCK comment on the PHP constant side names both CSS line numbers and the cascade direction. - Item 7 (Nitpick): extract magic number into private const BACKGROUND_SIZE_SOFT_LIMIT_BYTES = 1024 * 1024 and replace both call sites. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Point Plugin URI to plugins.ettic.nl/magicauth
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.
Adds dark mode + more branding controls to the login screen.
For the admin: three new settings on the Branding page, a link color picker, a light / dark / auto color mode toggle, and a page background image upload. The Branding section splits into "Identity" (logo, brand, colors) and "Appearance" (mode, page, layout) sub-cards, and every settings section is now collapsible.
For visitors: the sign-in screen renders in a curated dark palette when the admin chooses dark, or follows the browser's preference on auto. Any background image sits behind the card, with safe MIME checks, a soft 1 MB warning, and
prefers-reduced-datasuppression for low-bandwidth users.A small accessibility guardrail runs at save time, pick a link color that would be unreadable against the card and the change is rejected with an explanation; pick one that's just borderline and it saves with a warning. Same idea catches a brand color that would have a hard-to-see focus ring in dark mode.
Under the hood: a reusable WCAG contrast helper, a memoized settings reader (was being called 7× per login render), and 24 new tests bringing the suite to 165.
Summary by CodeRabbit
New Features
UI / Styling
Accessibility
Security
Tests / Docs