[3 / 7] - Mail Return#200
Conversation
📝 WalkthroughWalkthroughIntroduces the ChangesCharacter Order & Mail Return UI Redesigns
Admin Dark Mode Feature
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin User
participant Toggle as Admin Bar Toggle
participant JS as Inline Toggle Script
participant Ajax as wp_ajax_acore_toggle_dark_mode
participant Meta as acore_dark_mode user meta
Admin->>Toggle: Click dark mode toggle
Toggle->>JS: Fire click handler
JS->>Ajax: POST action + nonce
Ajax->>Meta: Flip stored mode value
Meta-->>Ajax: Return enabled state
Ajax-->>JS: JSON success payload
JS->>Toggle: Update icon and title
JS->>Admin: Toggle body class acore-dark-mode
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 unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/acore-wp-plugin/web/assets/mail-return/mail-return.js (1)
36-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider providing constants via localized data.
The class colors (lines 36-40), race/class names (lines 50-57), and faction mappings (lines 60-62) duplicate the PHP constants in
AcoreCharColors. If these values change in the PHP class, the JS must be manually updated.Consider adding these to the
mailReturnDataobject inMailReturnView.phpviawp_localize_script()to maintain a single source of truth.🤖 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 `@src/acore-wp-plugin/web/assets/mail-return/mail-return.js` around lines 36 - 63, The classColors object, raceNames object, classNames object, and allianceRaces array in mail-return.js are hardcoded duplicates of PHP constants from AcoreCharColors. Move these data structures to the mailReturnData object in MailReturnView.php and pass them to the JavaScript using wp_localize_script() so they can be accessed from a single source of truth. Then update the references in mail-return.js to use the localized mailReturnData object properties instead of the hardcoded variable declarations.
🤖 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 `@src/acore-wp-plugin/src/Components/CharactersMenu/CharactersController.php`:
- Around line 17-28: Fix the spelling error in the success message displayed
after saving character settings. In the else block following the
saveCharacterOrder() method call, locate the text "succesfully" in the message
"Character settings succesfully saved." and correct it to "successfully" to
match the standard English spelling.
In `@src/acore-wp-plugin/src/Components/MailReturnMenu/MailReturnView.php`:
- Around line 58-59: The src attributes for the race-icon and class-icon img
elements in MailReturnView.php need to be wrapped with the esc_url() function to
comply with WordPress coding standards. Locate both img tags (those with
class="race-icon" and class="class-icon") and wrap their entire src attribute
values with esc_url(), ensuring the concatenated URL string constructed from
ACORE_URL_PLG and the intval() conversions is properly escaped for use in URL
context.
In `@src/acore-wp-plugin/src/Hooks/Various/DarkMode.php`:
- Line 41: The wp_add_inline_script call attaching the dark mode toggle JS to
the 'jquery-core' handle can fail silently on admin screens where jquery-core is
not enqueued or printed, breaking the toggle functionality. Instead of targeting
jquery-core, register a new dedicated script handle (for example, a custom
handle like 'acore-dark-mode') using wp_register_script with jquery as a
dependency, then enqueue it and attach the inline script to this new guaranteed
handle using wp_add_inline_script. This ensures the toggle JS is always
available regardless of the screen context.
In `@src/acore-wp-plugin/src/Utils/AcoreCharColors.php`:
- Around line 84-94: The rowStyle method defines a variable `$fDark` that is
assigned but never used in the sprintf output. Remove the line that assigns
`$fDark` since only the `$fLight` variable is utilized in the returned style
string. This will eliminate the unused variable and clean up the code.
---
Nitpick comments:
In `@src/acore-wp-plugin/web/assets/mail-return/mail-return.js`:
- Around line 36-63: The classColors object, raceNames object, classNames
object, and allianceRaces array in mail-return.js are hardcoded duplicates of
PHP constants from AcoreCharColors. Move these data structures to the
mailReturnData object in MailReturnView.php and pass them to the JavaScript
using wp_localize_script() so they can be accessed from a single source of
truth. Then update the references in mail-return.js to use the localized
mailReturnData object properties instead of the hardcoded variable declarations.
🪄 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: 79a1b2b5-82aa-43bd-aabe-eebeaeb6eff2
📒 Files selected for processing (11)
src/acore-wp-plugin/src/Components/CharactersMenu/CharactersController.phpsrc/acore-wp-plugin/src/Components/CharactersMenu/CharactersView.phpsrc/acore-wp-plugin/src/Components/MailReturnMenu/MailReturnController.phpsrc/acore-wp-plugin/src/Components/MailReturnMenu/MailReturnView.phpsrc/acore-wp-plugin/src/Hooks/Various/DarkMode.phpsrc/acore-wp-plugin/src/Utils/AcoreCharColors.phpsrc/acore-wp-plugin/src/boot.phpsrc/acore-wp-plugin/web/assets/css/dark-mode.csssrc/acore-wp-plugin/web/assets/css/main.csssrc/acore-wp-plugin/web/assets/css/theme.csssrc/acore-wp-plugin/web/assets/mail-return/mail-return.js
(Resume generated by Claude) **PR azerothcore#199 — Characters Menu** - Fixed typo: "succesfully" → "successfully" in save confirmation - Fixed broken sentence in the Order Character Screen description - Extended `user-select: none` to cover all `.acore-char-row` elements, not just `button` **PR azerothcore#200 — Mail Return** - *(issues covered by later PRs — no unique fixes)* **PR azerothcore#201 — Item Restoration** - Added `alt=` attributes to race/class icons in `CharactersView.php` - `ToolsApi`: `ItemRestoreList` now catches exceptions and returns a 403 instead of a 500 - `ToolsApi` + `ACoreServices`: **Security fix** — `ItemRestore` now verifies the recovery item belongs to the character, not just that the user owns the character name (IDOR vulnerability) **PR azerothcore#202 — Unstuck / Scroll / RAF** - `ItemRestorationPage`: removed brittle `data.includes('mail')` success check — any HTTP success now triggers the success path **PR azerothcore#203 — Security Tab** - `CharactersController`: `resetCharacterOrder()` now guards against invalid/null account ID before running UPDATE - `Tools.php`: added `esc_attr()` on banned-names table input (XSS fix) - `Tools.php`: added `is_array()` guard on thresholds `foreach` - `Tools.php` + `SettingsController`: added sentinel input so deleting all thresholds actually clears them in the DB - `Tools.php`: GeoIP hidden fallback `value="0"` is now always rendered, not conditionally **PR azerothcore#204 — Admin Settings / CSRF** - `AcoreCharColors`: removed unused `$fDark`, unknown race IDs now get a neutral color instead of Horde red - `MailReturnView`: added `esc_url()` on race/class icon `src` attributes - `DarkMode`: added `dmToggleInFlight` flag to prevent overlapping AJAX requests on rapid dark mode clicks
|
The reviews are valid for this, but the are fixed in the on the way to latest PR and including he last PR #205 |
Split from the main PR: #197
Split by Claude
Full overhaul: confirm prompt before returning, a 6×2 grid matching the 12-item mail cap, shows the recipient's faction/class, Wowhead tooltips + higher-res icons, and clean empty/loaded states.
Summary by CodeRabbit
Release Notes
New Features
Improvements