fix: use the translated roles from editorRoles for the search tree #3580
Conversation
WalkthroughReplaces hard-coded role labels with dynamic values from Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant UI as Settings UI
participant Roles as editorRoles
participant Search as Search logic
participant Renderer as Renderer
User->>UI: open Settings / type search
UI->>Roles: read editorRoles (Object.values or {})
Roles-->>UI: role list (possibly empty)
Search->>UI: currentSearch value
UI->>UI: build SEARCH_TREE (set group.children = null if currentSearch empty)
UI->>Renderer: send SEARCH_TREE + hasGroupMatch flags
alt any group has children
Renderer-->>User: render matching groups and items
else no groups usable
Renderer-->>User: render "No matching settings"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
🤖 Pull request artifacts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/welcome/admin.js (3)
171-172: Defensive guard to prevent runtime error ifeditorRolesis undefined.If, for any reason,
editorRolesisn’t exported/initialized (older bundles, feature flags, or race during module wiring),Object.values(editorRoles)will throw. A small guard avoids a hard crash and gracefully degrades to an empty list. Also, coercing toStringis a safe hedge for unexpected non-string values.Apply this diff:
- ...Object.values( editorRoles ), + ...Object.values( editorRoles || {} ).map( String ),
184-185: Mirror the same safeguard in Custom Fields.Keep the behavior consistent across both sections by guarding and normalizing the values here as well.
Apply this diff:
- ...Object.values( editorRoles ), + ...Object.values( editorRoles || {} ).map( String ),
571-587: Optional: make the search more robust to non-string entries.The filter assumes all children are strings. Given SEARCH_TREE now partially depends on imported data, a small normalization prevents failures or false negatives if any entry isn’t a string.
Apply this diff:
- const filteredChildren = group.children.filter( child => { - return child.toLowerCase().includes( loweredSearch.toLowerCase() ) - } ) + const filteredChildren = group.children.filter( child => { + const c = String( child ) + return c.toLowerCase().includes( loweredSearch ) + } )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/welcome/admin.js(3 hunks)
🔇 Additional comments (2)
src/welcome/admin.js (2)
32-32: Good call: centralizing role labels viaeditorRoles.Importing
editorRolesfrom the shared source eliminates hard-coded strings and keeps role labels in sync and localized.
26-33: Manual Verification Required: ConfirmeditorRolesExport instackablePlease ensure that the
editorRolessymbol you’re importing from thestackablepackage:
- Exists and is exported in the source (JS/TS) of
stackable- Is defined with the expected shape (string values, properly localized)
- Remains available across all supported versions/releases of
stackableSteps to verify:
- Inspect the
editorRolesdefinition instackable’s source (e.g., innode_modules/stackable/srcor its GitHub repo)- Review any accompanying TypeScript declaration files (
.d.ts) to confirm the export and its type- Check release notes or version history to ensure no breaking changes around
editorRoles
bfintal
left a comment
bfintal
left a comment
There was a problem hiding this comment.
this seems to be the wrong fix for this. the underlying issue is this line in AdminBaseSetting:
const isSearched = props.searchedSettings ? props.searchedSettings.includes( props.label ) : trueIt's searching right away without any search string and not matching because it's a different language.
For me the fix should be that if there's no searching being done, there should be no filtering.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/welcome/admin.js (4)
595-604: Null sentinel for “no search” is sensible; harden search and remove redundant lowercaseUsing
children: nullto indicate “no active search” is a clean way to distinguish from “no results.” Two micro-fixes:
- Guard non-string children defensively: use
String(child).toLowerCase()to avoid crashes if any child isn’t a string.loweredSearchis already lowercase; drop the second.toLowerCase().- const filteredChildren = group.children.filter( child => { - return child.toLowerCase().includes( loweredSearch.toLowerCase() ) - } ) + const filteredChildren = group.children.filter( child => { + return String( child ).toLowerCase().includes( loweredSearch ) + } )Also verify that downstream components (
Admin*Setting, etc.) safely handlesearchedSettings={null}as “no search highlight,” not as an array.
657-658: Consistent “hasGroupMatch” gating and null-aware children checksNice, consistent pattern across sections:
hasGroupMatch = groups.some(group => group.children === null || group.children.length > 0)- Render “No matching settings” when applicable.
- Per-group rendering guarded by
(group.children === null || group.children.length > 0).Optional DRY: extract helpers to reduce repetition and future drift.
Example:
const groupHasVisibleChildren = g => g.children === null || g.children.length > 0const tabHasAnyMatches = groups => groups.some(groupHasVisibleChildren)Also applies to: 661-661, 665-665, 691-691, 750-750, 801-801, 840-840, 845-845, 849-849, 912-913, 1093-1093, 1112-1112, 1180-1180, 1184-1184, 1188-1188, 1221-1221, 1225-1225, 1229-1229, 1252-1253, 1263-1263, 1266-1266, 1304-1305, 1316-1316, 1319-1319, 1366-1367, 1377-1377, 1381-1381, 1474-1474, 1484-1484, 1488-1488, 1545-1545, 1608-1608, 1620-1621
1476-1479: searchClassname: null-aware check looks good; consider optional chainingYou already handle
children === null. Minor polish: optional chaining avoids assumptions on the shape ofsearchedSettings.- return searchedSettings.children === null || searchedSettings.children.includes( label ) + return searchedSettings?.children === null || searchedSettings?.children?.includes( label ) ? '' : 'ugb-admin-setting--not-highlight'
443-445: FYI: Sidenav highlight relies on short-circuit to avoid null childrenNot a change here, but with the new null sentinel,
isSearched = currentSearch && groups.some( g => g.children.length > 0 )is safe because of short-circuiting. If you want to align readability with the rest of the file, you can mirror the(children === null || children.length > 0)guard here too.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/welcome/admin.js(25 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: PHP 7.3 and WP 6.5.5
- GitHub Check: PHP 8.2 and WP 6.5.5
- GitHub Check: PHP 8.2 and WP 6.7.2
- GitHub Check: PHP 8.2 and WP 6.6.2
- GitHub Check: PHP 7.3 and WP latest
- GitHub Check: PHP 8.2 and WP latest
- GitHub Check: build
🔇 Additional comments (2)
src/welcome/admin.js (2)
32-33: Importing editorRoles looks right; verify export shape and availabilityConfirm that
editorRolesis a defined object (or array of strings) at runtime and contains translated role labels when the site language changes. If it can be an empty object in some locales or load after this file, add a defensive fallback (see role children comments below).
610-614: Passing currentSearch down is appropriatePlumbing
currentSearchvia props aligns with the new filtering semantics.
src/welcome/admin.js
Outdated
| children: ( editorRoles | ||
| ? Object.values( editorRoles ) | ||
| : [ | ||
| __( 'Role Manager', i18n ), | ||
| __( 'Administrator', i18n ), | ||
| __( 'Editor', i18n ), | ||
| __( 'Author', i18n ), | ||
| __( 'Contributor', i18n ), | ||
| __( 'Subscriber', i18n ), | ||
| ] | ||
| ), | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Make fallback robust and retain group keyword in search results
Good move switching to dynamic, translated role labels. Two improvements:
- Robust fallback:
editorRolesmay be defined but empty (e.g., missing translations). The current truthy check will select[]instead of the fallback labels. Check for a non-empty values array. - Retain discoverability: keep the group keyword in the searchable children so typing “Role Manager” still surfaces this section when
editorRolesis present.
Apply these diffs:
Role Manager group
- children: ( editorRoles
- ? Object.values( editorRoles )
- : [
- __( 'Role Manager', i18n ),
- __( 'Administrator', i18n ),
- __( 'Editor', i18n ),
- __( 'Author', i18n ),
- __( 'Contributor', i18n ),
- __( 'Subscriber', i18n ),
- ]
- ),
+ children: [
+ __( 'Role Manager', i18n ),
+ ...(
+ editorRoles && Object.values( editorRoles ).length > 0
+ ? Object.values( editorRoles )
+ : [
+ __( 'Administrator', i18n ),
+ __( 'Editor', i18n ),
+ __( 'Author', i18n ),
+ __( 'Contributor', i18n ),
+ __( 'Subscriber', i18n ),
+ ]
+ ),
+ ],Custom Fields group
- ...( editorRoles
- ? Object.values( editorRoles )
- : [
- __( 'Role Manager', i18n ),
- __( 'Administrator', i18n ),
- __( 'Editor', i18n ),
- __( 'Author', i18n ),
- __( 'Contributor', i18n ),
- __( 'Subscriber', i18n ),
- ]
- ),
+ ...(
+ editorRoles && Object.values( editorRoles ).length > 0
+ ? Object.values( editorRoles )
+ : [
+ __( 'Role Manager', i18n ),
+ __( 'Administrator', i18n ),
+ __( 'Editor', i18n ),
+ __( 'Author', i18n ),
+ __( 'Contributor', i18n ),
+ __( 'Subscriber', i18n ),
+ ]
+ ),Also applies to: 192-202
Ensure robust fallback and preserve “Role Manager” in searchable children
We should guard against an empty editorRoles object (truthy but no entries) and always include the “Role Manager” keyword so this group remains discoverable when translations are available.
– In src/welcome/admin.js (lines 170–181 and 192–202):
• Change the fallback logic to check Object.values(editorRoles).length > 0 rather than just editorRoles.
• Always prepend __( 'Role Manager', i18n ) to the children array.
Apply these diffs:
Role Manager group (around line 170)
- children: ( editorRoles
- ? Object.values( editorRoles )
- : [
- __( 'Role Manager', i18n ),
- __( 'Administrator', i18n ),
- __( 'Editor', i18n ),
- __( 'Author', i18n ),
- __( 'Contributor', i18n ),
- __( 'Subscriber', i18n ),
- ]
- ),
+ children: [
+ __( 'Role Manager', i18n ),
+ ...(
+ editorRoles && Object.values( editorRoles ).length > 0
+ ? Object.values( editorRoles )
+ : [
+ __( 'Administrator', i18n ),
+ __( 'Editor', i18n ),
+ __( 'Author', i18n ),
+ __( 'Contributor', i18n ),
+ __( 'Subscriber', i18n ),
+ ]
+ ),
+ ],Custom Fields group (around line 192)
- ...( editorRoles
- ? Object.values( editorRoles )
- : [
- __( 'Role Manager', i18n ),
- __( 'Administrator', i18n ),
- __( 'Editor', i18n ),
- __( 'Author', i18n ),
- __( 'Contributor', i18n ),
- __( 'Subscriber', i18n ),
- ]
- ),
+ ...(
+ editorRoles && Object.values( editorRoles ).length > 0
+ ? Object.values( editorRoles )
+ : [
+ __( 'Role Manager', i18n ),
+ __( 'Administrator', i18n ),
+ __( 'Editor', i18n ),
+ __( 'Author', i18n ),
+ __( 'Contributor', i18n ),
+ __( 'Subscriber', i18n ),
+ ]
+ ),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| children: ( editorRoles | |
| ? Object.values( editorRoles ) | |
| : [ | |
| __( 'Role Manager', i18n ), | |
| __( 'Administrator', i18n ), | |
| __( 'Editor', i18n ), | |
| __( 'Author', i18n ), | |
| __( 'Contributor', i18n ), | |
| __( 'Subscriber', i18n ), | |
| ] | |
| ), | |
| }, | |
| // — Role Manager group (originally lines 170–181) | |
| children: [ | |
| __( 'Role Manager', i18n ), | |
| ...( | |
| editorRoles && Object.values( editorRoles ).length > 0 | |
| ? Object.values( editorRoles ) | |
| : [ | |
| __( 'Administrator', i18n ), | |
| __( 'Editor', i18n ), | |
| __( 'Author', i18n ), | |
| __( 'Contributor', i18n ), | |
| __( 'Subscriber', i18n ), | |
| ] | |
| ), | |
| ], | |
| // — Custom Fields group (originally lines 192–202) | |
| children: [ | |
| // … any existing items before the roles … | |
| ...( | |
| editorRoles && Object.values( editorRoles ).length > 0 | |
| ? Object.values( editorRoles ) | |
| : [ | |
| __( 'Role Manager', i18n ), | |
| __( 'Administrator', i18n ), | |
| __( 'Editor', i18n ), | |
| __( 'Author', i18n ), | |
| __( 'Contributor', i18n ), | |
| __( 'Subscriber', i18n ), | |
| ] | |
| ), | |
| // … any existing items after the roles … | |
| ], |
🤖 Prompt for AI Agents
In src/welcome/admin.js around lines 170–181 and 192–202, the current fallback
uses a truthy check on editorRoles and can produce an empty children list when
editorRoles is an object with no entries, and it also omits the “Role Manager”
keyword; change the condition to use Object.values(editorRoles).length > 0 for
the fallback and ensure __("'Role Manager'", i18n) is always prepended to the
children array (so build children as [__( 'Role Manager', i18n ),
...(Object.values(editorRoles).length > 0 ? Object.values(editorRoles) : [ other
translated defaults ])] for both the Role Manager group and the Custom Fields
group locations).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/welcome/admin.js (1)
170-171: Make fallback robust and keep “Role Manager” discoverable in searchIf editorRoles is defined but empty, Object.values(editorRoles || {}) yields [], hiding role entries from search. Also, removing the "Role Manager" keyword makes this section harder to find via search. Use a non-empty check and always include the “Role Manager” keyword.
Apply these diffs:
- children: Object.values( editorRoles || {} ), + children: [ + __( 'Role Manager', i18n ), + ...( + editorRoles && Object.values( editorRoles ).length > 0 + ? Object.values( editorRoles ) + : [ + __( 'Administrator', i18n ), + __( 'Editor', i18n ), + __( 'Author', i18n ), + __( 'Contributor', i18n ), + __( 'Subscriber', i18n ), + ] + ), + ],- ...Object.values( editorRoles || {} ), + ...( + editorRoles && Object.values( editorRoles ).length > 0 + ? Object.values( editorRoles ) + : [ + __( 'Role Manager', i18n ), + __( 'Administrator', i18n ), + __( 'Editor', i18n ), + __( 'Author', i18n ), + __( 'Contributor', i18n ), + __( 'Subscriber', i18n ), + ] + ),Verification: Please sanity-check in a locale where editorRoles may be temporarily empty/missing to ensure the section remains searchable and populated.
Also applies to: 179-183
🧹 Nitpick comments (2)
src/welcome/admin.js (2)
575-585: Tighten filteredSearchTree: remove redundant lowercase and fix typoTwo small cleanups:
- loweredSearch is already lowercased, no need to lowercase it again inside includes.
- Typo: filtedGroups → filteredGroups.
- return SEARCH_TREE.map( tabs => { - const filtedGroups = tabs.groups.map( group => { + return SEARCH_TREE.map( tabs => { + const filteredGroups = tabs.groups.map( group => { if ( ! currentSearch ) { return { ...group, children: null } } const filteredChildren = group.children.filter( child => { - return child.toLowerCase().includes( loweredSearch.toLowerCase() ) + return child.toLowerCase().includes( loweredSearch ) } ) return { ...group, children: filteredChildren } } ) - return { ...tabs, groups: filtedGroups } + return { ...tabs, groups: filteredGroups } } )
637-638: Reduce duplication for group match checks with a small helperThe pattern groups.some(group => group.children === null || group.children.length > 0) is repeated in many sections. A tiny utility improves readability and reduces the chance of inconsistent checks.
Example refactor (add once near the top-level of the file):
// Utility for search visibility const hasMatches = children => children == null || children.length > 0 const anyGroupHasMatches = groups => groups.some(group => hasMatches(group.children))Then replace repeated checks with anyGroupHasMatches(groups) and group-level guards with hasMatches(group.children).
Also applies to: 820-821, 892-893, 1160-1160, 1201-1201, 1232-1233, 1284-1285, 1347-1347, 1454-1454
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/welcome/admin.js(25 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: PHP 8.2 and WP 6.5.5
- GitHub Check: PHP 8.2 and WP 6.6.2
- GitHub Check: PHP 8.2 and WP 6.7.2
- GitHub Check: PHP 7.3 and WP latest
- GitHub Check: PHP 7.3 and WP 6.5.5
- GitHub Check: PHP 8.2 and WP latest
- GitHub Check: build
🔇 Additional comments (4)
src/welcome/admin.js (4)
32-33: Importing translated editorRoles looks goodUsing editorRoles from stackable directly aligns with the PR goal of surfacing translated role labels in the search tree.
575-577: Setting children to null changes contract; verify all consumers are guardedSwitching to children: null when no search is active requires all consumers to handle null. Most callsites were updated, but third-party injected components (via applyFilters) might still assume arrays.
Please verify:
- EditorModeSettings and CustomFields* injected components handle searchedSettings being null.
- No runtime errors in the console when toggling search on/off across all tabs.
If needed, consider centralizing a helper (e.g., const hasMatches = c => c == null || c.length > 0) and consistently using it for guards to avoid missed spots.
590-594: Threading currentSearch through props is appropriatePropagating currentSearch enables consistent “No matching settings” handling across sections. Looks correct.
641-671: Guard patterns for group sections are consistent—nice coverageAcross all sections you consistently used (group.children === null || group.children.length > 0) to conditionally render groups and the “No matching settings” banner otherwise. This aligns with the new null semantics from filteredSearchTree and prevents crashes.
Also applies to: 671-729, 730-781, 781-805, 824-870, 1073-1145, 1164-1192, 1205-1225, 1242-1277, 1296-1333, 1356-1439, 1464-1524, 1524-1571, 1588-1606
fixes #3565
Summary by CodeRabbit
New Features
Bug Fixes