[4 / 7] - Item Restoration#201
Open
TheSCREWEDSoftware wants to merge 4 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds ChangesCharacter Color Utility, Security, Feature UI Overhaul
Admin Dark Mode Toggle
Sequence Diagram(s)sequenceDiagram
rect rgba(100, 149, 237, 0.5)
Note over Browser, ToolsApi: Item Restoration Flow
end
participant Browser
participant ItemRestorationPage as ItemRestorationPage.php (IIFE)
participant ToolsApi
participant ACoreServices
participant DB as characters / recovery_item
Browser->>ItemRestorationPage: click .acore-char-row (guid, cname)
ItemRestorationPage->>ToolsApi: GET /item-restore/list/{cguid}
ToolsApi->>ACoreServices: getRestorableItemsByCharacter(guid)
ACoreServices->>DB: verify account owns non-deleted char
DB-->>ACoreServices: row or empty
ACoreServices->>DB: SELECT Id, ItemEntry, Count, DeleteDate FROM recovery_item
DB-->>ACoreServices: item rows
ACoreServices-->>ToolsApi: items[]
ToolsApi-->>ItemRestorationPage: JSON items[]
ItemRestorationPage->>Browser: render item cards + applyWowheadIcons()
Browser->>ItemRestorationPage: click Restore button (item, cname)
ItemRestorationPage->>ToolsApi: POST /item-restore {item, cname}
ToolsApi->>ACoreServices: currentAccountOwnsCharacterName(cname)
ACoreServices-->>ToolsApi: bool
ToolsApi->>ToolsApi: SOAP item restore $item $cname
ToolsApi-->>ItemRestorationPage: success
ItemRestorationPage->>Browser: remove card, renumber, show empty-state if none remain
sequenceDiagram
rect rgba(144, 238, 144, 0.5)
Note over AdminBar, UserMeta: Dark Mode Toggle Flow
end
participant AdminBar as WP Admin Bar
participant JS as acore_dark_mode_js
participant AJAX as wp_ajax_acore_toggle_dark_mode
participant UserMeta as acore_dark_mode user meta
AdminBar->>JS: click `#acore-dark-mode` icon link
JS->>AJAX: POST admin-ajax.php {action, nonce}
AJAX->>AJAX: check_ajax_referer(acore_dark_mode)
AJAX->>UserMeta: get current value → toggle 0↔1
AJAX->>UserMeta: update_user_meta
AJAX-->>JS: {success: true, data: {dark: bool}}
JS->>JS: toggle body.acore-dark-mode class
JS->>AdminBar: swap icon HTML + update link title
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (1)
src/acore-wp-plugin/src/Components/UserPanel/Pages/ItemRestorationPage.php (1)
217-233: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSuccess detection is overly coupled to message text.
parseJsonResponsealready enforces HTTP success, but this branch treats success only when a string contains"mail". That is brittle against structured responses or wording changes.Suggested refactor
.then(function (data) { - if (typeof data === 'string' && data.toLowerCase().includes('mail')) { + var message = typeof data === 'string' + ? data + : (data && data.message ? data.message : 'Item restored successfully.'); + if (!data || !data.error) { cardEl.parentElement.removeChild(cardEl); - successBox.textContent = data; + successBox.textContent = message; successBox.classList.remove('invisible'); ... } else { - errorBox.textContent = typeof data === 'string' ? data : JSON.stringify(data); + errorBox.textContent = typeof data === 'string' ? data : JSON.stringify(data); btnEl.disabled = false; btnEl.textContent = 'Restore'; } })🤖 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/src/Components/UserPanel/Pages/ItemRestorationPage.php` around lines 217 - 233, The success detection is tightly coupled to the response message containing the word "mail", which is brittle and fragile to message wording changes. Since parseJsonResponse already enforces HTTP success status, remove the brittle string content check (the includes('mail') condition) and instead trust that if the response reaches this point without throwing an error, it is successful. Refactor the conditional logic so that the success path (removing the card, displaying the success box, renumbering remaining cards, and checking if no results remain) is executed whenever the HTTP response is successful, regardless of the specific message content, and only execute the error path if parseJsonResponse throws an exception or returns an error status.
🤖 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/CharactersView.php`:
- Around line 48-49: The race-icon and class-icon img tags are missing alt
attributes, which are critical for accessibility as screen readers rely on alt
text to describe images. Add an alt attribute to both img elements: for the
race-icon, use the race name from
AcoreCharColors::getRaceName(intval($char["race"])) and for the class-icon, use
the class name from AcoreCharColors::getClassName(intval($char["class"])).
Ensure the alt text is wrapped with esc_attr() for proper escaping, similar to
how the title attributes are currently handled.
In `@src/acore-wp-plugin/src/Components/MailReturnMenu/MailReturnView.php`:
- Around line 44-49: User-facing strings in the MailReturnView.php file are
hardcoded in English without translation function wrappers, preventing
localization. Wrap all hardcoded text strings like "Mail Return", the
instruction paragraph "You can return sent mails...", "Select Character:", and
any other similar strings mentioned at lines 75-77 in the appropriate WordPress
translation function (such as __() for returning translated strings or _e() for
echoing them directly) using the plugin's text domain. Ensure consistency with
how other user-facing strings are handled throughout the rest of the WP UI in
this plugin.
In `@src/acore-wp-plugin/src/Components/Tools/ToolsApi.php`:
- Around line 30-31: The callback function for the REST endpoint does not handle
exceptions that may be thrown by ToolsApi::ItemRestoreList() when
getRestorableItemsByCharacter() encounters invalid or unowned GUIDs. Wrap the
call to ToolsApi::ItemRestoreList($request) in a try-catch block to catch any
exceptions, and in the catch block, convert them into proper REST error
responses using appropriate WordPress REST API error handling functions so that
exceptions result in controlled error responses rather than uncaught 500 errors.
- Around line 14-22: The current authorization only verifies that the user owns
the character name via currentAccountOwnsCharacterName, but does not verify that
the recovery item ID ($item) actually belongs to that character. This creates a
security vulnerability where a user could submit another user's recovery item ID
with their own character name. Add a new authorization check using a service
helper method (similar to getOwnedRestorableItemCharacterName) that joins the
recovery_item table with the characters table to verify both the item ID and
character name are owned together by the current user, then use this method to
authorize the request before calling executeCommand with the item restore
command.
In `@src/acore-wp-plugin/src/Components/UserPanel/Pages/ItemRestorationPage.php`:
- Around line 34-35: The race-icon and class-icon img elements on this page are
missing alt attributes required for accessibility. Add alt attributes to both
img elements that display the same descriptive text as their title attributes;
for the race-icon use AcoreCharColors::getRaceName() with the race value, and
for the class-icon use AcoreCharColors::getClassName() with the class value.
Ensure both alt attributes are properly escaped using esc_attr() for consistency
with the existing title attributes.
- Around line 199-202: The catch block handling fetch failures sets the error
message in errorBox but does not make it visible to the user since the
containing element remains hidden. In the catch function that currently hides
the loading element and sets errorBox.textContent, also explicitly display the
errorBox or its parent container by setting the appropriate CSS display property
to make the error message visible when a fetch failure occurs.
- Around line 22-35: The ItemRestorationPage template is accessing character
array keys (guid, name, class, race, level, gender) that are not guaranteed to
be present in the upstream data source, causing undefined key warnings. Rather
than fixing this in the template, locate the character query or data provider
that populates the $characters array passed to this template and ensure it
always includes all required keys (guid, name, class, race, level, gender) in
the result set. This ensures the contract between the data source and the
template is fulfilled at the point of data retrieval, not masked in the view
layer.
- Around line 114-203: The selectCharacter function has a race condition where
multiple in-flight fetch requests can complete out of order, causing an older
response to overwrite newer data in the grid. To fix this, implement a request
token mechanism by creating a unique identifier that gets incremented or updated
each time selectCharacter is called, capturing this token in the fetch promise
chain, and then checking if the token is still current before rendering the
items in the response handler. Only proceed with rendering the grid if the
stored token matches the current request token; otherwise, discard the stale
results to prevent outdated character items from being displayed.
In `@src/acore-wp-plugin/src/Hooks/Various/DarkMode.php`:
- Around line 121-129: The dark mode toggle click handler does not prevent
concurrent AJAX requests, which causes multiple overlapping toggle operations
when users click rapidly. Fix this by introducing a flag variable (e.g.,
isToggling) that tracks whether a request is currently in progress. Before
making the $.post request in the click handler on '`#wp-admin-bar-acore-dark-mode`
> a', check if isToggling is true and return early if it is. Set isToggling to
true before the request and set it back to false in the callback after the
response is processed, ensuring only one toggle operation can occur at a time.
In `@src/acore-wp-plugin/src/Utils/AcoreCharColors.php`:
- Around line 86-92: The current ternary logic in the faction color assignment
treats any non-Alliance value (including the 'unknown' default) as Horde,
causing invalid or missing race IDs to render with Horde colors. Replace the
simple ternary operators for $fLight with explicit branching using
if/elseif/else statements to handle three distinct cases: when $faction equals
'alliance', when $faction equals 'horde', and a fallback neutral color for the
'unknown' case. Also remove the unused $fDark variable which duplicates the
$fLight logic, and ensure the sprintf call uses the correctly assigned faction
color variable.
In `@src/acore-wp-plugin/web/assets/css/main.css`:
- Around line 364-368: The `#mail-return-items` grid is using a fixed two-column
layout with grid-template-columns: 1fr 1fr, which forces items to stay in two
columns and causes overflow in narrow layouts. Replace the fixed two-column
setup with a responsive grid layout using auto-fit or auto-fill with minmax to
allow mail cards to wrap naturally based on available width. Apply the same
responsive approach to the .mail-items-grid rule mentioned in lines 473-484 so
that item slots also adapt to their container width instead of always reserving
space for six 56px slots.
- Around line 690-695: The `.item-restore-grid` class uses a fixed 4-column
layout that doesn't respect the actual available space, causing cards to shrink
below acceptable dimensions when the content area narrows. Replace the fixed
`grid-template-columns: repeat(4, 1fr)` with a responsive approach using
`auto-fit` or `auto-fill` combined with `minmax()` function to set a minimum
width for each card (approximately 80px for the icon plus padding), allowing
cards to wrap to the next row naturally when space is constrained rather than
squeezing horizontally.
In `@src/acore-wp-plugin/web/assets/mail-return/mail-return.js`:
- Around line 2-18: There is a race condition where overlapping AJAX requests
can cause stale mail data to be rendered if an older request completes after a
newer one. In the click handler for .acore-char-card, you need to abort any
previous AJAX request before initiating a new one (or alternatively store the
current charGuid as a request ID and validate responses match the current
selection). Store a reference to the active AJAX request (likely made in the
lines around 25-33 where the actual data fetch occurs) and call .abort() on any
existing request before making a new one, or add a guard in the response handler
to ensure the returned charGuid still matches the currently selected character
before rendering the mailItems.
---
Nitpick comments:
In `@src/acore-wp-plugin/src/Components/UserPanel/Pages/ItemRestorationPage.php`:
- Around line 217-233: The success detection is tightly coupled to the response
message containing the word "mail", which is brittle and fragile to message
wording changes. Since parseJsonResponse already enforces HTTP success status,
remove the brittle string content check (the includes('mail') condition) and
instead trust that if the response reaches this point without throwing an error,
it is successful. Refactor the conditional logic so that the success path
(removing the card, displaying the success box, renumbering remaining cards, and
checking if no results remain) is executed whenever the HTTP response is
successful, regardless of the specific message content, and only execute the
error path if parseJsonResponse throws an exception or returns an error status.
🪄 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: 119c3e4d-b692-43bb-b91a-45504a752e16
📒 Files selected for processing (14)
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/Components/Tools/ToolsApi.phpsrc/acore-wp-plugin/src/Components/UserPanel/Pages/ItemRestorationPage.phpsrc/acore-wp-plugin/src/Hooks/Various/DarkMode.phpsrc/acore-wp-plugin/src/Manager/ACoreServices.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
Comment on lines
+30
to
31
| 'callback' => function( $request ) { | ||
| return ToolsApi::ItemRestoreList($request); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Convert denied list requests into REST errors.
getRestorableItemsByCharacter() now throws for invalid or unowned GUIDs; this callback returns it directly, so probing another character can surface as an uncaught exception/500 instead of a controlled REST response.
Proposed REST error handling
'permission_callback' => function() { return is_user_logged_in(); },
'callback' => function( $request ) {
- return ToolsApi::ItemRestoreList($request);
+ try {
+ return ToolsApi::ItemRestoreList($request);
+ } catch (\InvalidArgumentException $e) {
+ return new \WP_Error('invalid_character', 'Character not found.', ['status' => 404]);
+ }
}🤖 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/src/Components/Tools/ToolsApi.php` around lines 30 - 31,
The callback function for the REST endpoint does not handle exceptions that may
be thrown by ToolsApi::ItemRestoreList() when getRestorableItemsByCharacter()
encounters invalid or unowned GUIDs. Wrap the call to
ToolsApi::ItemRestoreList($request) in a try-catch block to catch any
exceptions, and in the catch block, convert them into proper REST error
responses using appropriate WordPress REST API error handling functions so that
exceptions result in controlled error responses rather than uncaught 500 errors.
TheSCREWEDSoftware
added a commit
to TheSCREWEDSoftware/acore-cms
that referenced
this pull request
Jun 24, 2026
(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
Contributor
Author
|
The reviews are valid for this, but the are fixed in the on the way to latest PR and including he last PR #205 PR #201 — Item Restoration
|
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
1 participant
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.
Split from the main PR: #197
Split by Claude
Redesigned item-restoration page (cards, quality-coloured icons, tooltips) and hardening: ownership checks so a user can only restore their own characters' items, plus input validation. Carries the shared
UserController/UserViewthe later branches need.Realised after that this UI is what we show the users I think via the shop implementation
This is broken until the 6th PR
Current PR
6th PR #203
Summary by CodeRabbit
New Features
Bug Fixes
Style