Skip to content

#2846 - Replace Components User Profile#2911

Open
kodinkat wants to merge 2 commits intoDiscipleTools:developfrom
kodinkat:2846-replace-components-user-profile
Open

#2846 - Replace Components User Profile#2911
kodinkat wants to merge 2 commits intoDiscipleTools:developfrom
kodinkat:2846-replace-components-user-profile

Conversation

@kodinkat
Copy link
Copy Markdown
Collaborator

@kodinkat kodinkat commented Apr 13, 2026

kodinkat added 2 commits April 13, 2026 17:24
@kodinkat
Refactor settings page components and enhance user experience
695c4e4 
- Replaced traditional input elements with custom web components for app state toggles and language selection, improving UI consistency and accessibility.
- Introduced new JavaScript functions to handle multi-select language and people group settings, enhancing user interaction and data management.
- Updated enqueue scripts to include necessary dependencies for web components.
- Improved error handling in app switch functionality to provide better feedback on failures.
- Enhanced SCSS styles for new components to ensure visual consistency across the settings page.
@kodinkat
Remove unnecessary whitespace from settings.js to improve code cleanl…
951d60f 
…iness.
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 13, 2026

PR Review: #2846 — Replace Components User Profile

Medium

Shadow DOM introspection for people-group search may silently fail
dt-assets/js/settings.js — initSettingsPeopleGroupsMultiselect / attachInputHandlers

The function attaches focus and keyup search handlers by directly querying the component's internal shadow DOM:

const input = el.shadowRoot && el.shadowRoot.querySelector('input[part=input]');
if (\!input) {
  return; // silent no-op
}

If el.shadowRoot is null or the shadow DOM doesn't contain an input[part=input] at the moment this runs, the function exits silently. No error is thrown, no retry is scheduled, and the user gets no feedback. The visible result is a people-group picker that loads but can never search — existing selections are shown, but typing or focusing the input does nothing.

In the normal load path (web-components enqueued blocking in <head>, dt-settings deferred to footer) the shadow root will be attached by DOMContentLoaded, so the first branch (if (el.shadowRoot)) fires and works. The concern is the else branch (element not yet upgraded): after customElements.whenDefined() resolves and a single microtask tick passes, there is still no guarantee the shadow root exists, and the same silent return applies.

The use of CSS part attributes is the right mechanism for cross-boundary querying; the risk is the lack of any observable error or graceful degradation if it fails.

No High Issues

No logic errors, data-loss paths, authorization bypasses, or broken callers found.

Specific items checked and confirmed correct:

  • makeRequestOnPosts('GET', 'peoplegroups/compact', { s }) correctly hits dt-posts/v2/peoplegroups/compact; get_viewable_compact returns { total, posts: [{ID, name, label}] }, matching what mergeCompactPosts(data.posts) expects.
  • user_people_groups removed from wp_localize_script in enqueue-scripts.php is safe — the only JS consumer was the typeahead block that is also removed.
  • esc_attr(wp_json_encode(...)) is used for all JSON-in-HTML-attribute output; no XSS introduced.
  • switch_preference and app_switch changes are backward-compatible — onFail defaults to null and existing call-sites that ignore the return value are unaffected.
  • checked attribute on dt-toggle uses checked($checked) (PHP) / bare 'checked' string (hardcoded) — both are correct.
  • e.detail.oldValue / e.detail.newValue pattern is consistent with usage in modular-list-bulk.js and details.js elsewhere in the codebase.
  • Adding web-components to the dt-settings script dependency list is correct; it was already registered globally in <head> but wasn't explicitly declared as a dependency.

The PR looks ready to merge pending acknowledgement of the silent-fail path in the people-group search shadow DOM access.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 13, 2026

Code Review

Medium

1. Partial server update not rolled back on failure

dt-assets/js/settings.js

In initSettingsLanguageMultiselect and initSettingsPeopleGroupsMultiselect, changes are serialized as individual API calls chained with .then(). If the chain fails mid-way (e.g., the second of three items errors), the UI reverts to prev via el.value = [...prev], but the already-completed server updates from earlier in the chain are not undone. The server ends up in a partially-updated state that disagrees with what the UI shows, until the user reloads.

For languages this is low risk (small, bounded list), but for people groups a user who adds several at once could silently end up with only some of them saved. A simple improvement would be to reload the component value from the server after any failure, rather than reverting to the cached prev.

2. attachInputHandlers silently no-ops if shadow DOM input not found

dt-assets/js/settings.js (~line 141)

The fallback path (when el.shadowRoot is absent) correctly defers with a microtask after whenDefined to let Lit finish its first render. But the fast path (when el.shadowRoot already exists) calls attachInputHandlers() synchronously. If Lit's initial render microtask has not yet drained, input[part=input] returns null and the handlers are dropped permanently. In practice this resolves before jQuery(document).ready fires on most browsers, but the fast path should apply the same deferral for consistency and safety.

Without the handlers, users can still see the initial options (pre-loaded from the PHP options attribute) but cannot search for people groups beyond that initial set.

Summary

The PR is a solid conversion from legacy Foundation switches / jQuery typeahead to the DT web-component system, with proper error-reversion on toggle failures and correct escaping throughout. The two issues above are medium-risk edge cases rather than blockers; the PR is close to ready to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Replace components: user profile

1 participant

@kodinkat