Skip to content

v9.5.3 - add searchbar for settings - #5253

Draft
tpurschke wants to merge 8 commits into
CactuseSecurity:developfrom
tpurschke:feat/settings-searchbar
Draft

tpurschke wants to merge 8 commits into
CactuseSecurity:developfrom
tpurschke:feat/settings-searchbar

Conversation

@tpurschke

@tpurschke tpurschke commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

refs #5146

Adds a search field above the settings sidebar navigation that filters the settings pages by their localized labels. Matching is case and diacritic insensitive; a matching chapter heading keeps all of its pages. Role visibility is unchanged and still owned by ExecutionModeAuthorizeView.

Note: issue #5146 asks for full-text search across settings page content with navigable results. This PR only filters the navigation labels, so the issue stays open.

@tpurschke tpurschke self-assigned this Sep 8, 2026
@tpurschke

Copy link
Copy Markdown
Contributor Author

Review — PR #5253 "add searchbar for settings" (round 1)

Reviewer: Claude Opus 5 (1M context), claude-opus-5[1m], high reasoning effort, review depth standard.

Scope: gh pr diff 5253 against develop (5 files, +86/-3), plus the directly implicated production code (ExecutionModeAuthorizeView.razor, Sidebar.razor, site.css, SettingsOwner.razor), the linked issue #5146, and fworch-texts.sql.

Verified locally on a detached worktree of pr/5253 (no checkout in the primary tree):

  • dotnet build of roles/FWO.sln — clean
  • dotnet test .../FWO.Test.csproj --filter FullyQualifiedName~UiSettingsLayoutTest5/5 passed
  • dotnet format --verify-no-changes on the changed test file — clean

No schema, migration, Hasura, auth or installer surface is touched; the search text key already exists in the idempotent fworch-texts.sql (lines 398/399, German + English), so localization of the new placeholder/aria-label is covered and no upgrade concern arises.

Findings

# Criticality Status Subject
F1 medium new fixes #5146 closes an issue this PR only partly addresses (no full-text search)
F2 medium new Filter state goes stale: JS-set hidden is not reapplied when the nav re-renders
F3 medium new New JS/DOM filtering mechanism duplicates the existing C#-side search idiom
F4 medium new The whole filtering algorithm has zero test coverage; the added test's name overstates it
F5 medium new Unenforced markup contract; <hr> does not terminate a section
F6 medium new .settings-search copies the .bg-blue gradient instead of reusing it
F7 medium new Every keystroke is a SignalR round-trip; no debounce
F8 low new External users now get a rendered-but-empty <li>
F9 low new "settingsNavigation" is a magic string repeated in markup, C# and test
F10 low new All separators are dropped wholesale while a search term is active
F11 low new No help content update under Pages/Help/ for a UI feature change
F12 low new whats_new_facts not updated for the new feature
F13 low new Copy-pasted, unused setup plus magic number 50 in the new test
F14 low new No "no matches" feedback and no aria-live for the filtered result
F15 low new No diacritic folding — German umlaut labels are unsearchable without the umlaut

F1 — medium — fixes #5146 closes an issue this PR only partly addresses.
Issue #5146 is titled "Settings: Full-text search" and asks for "a searchbar in the settings nav/sidebar that searches for text in all settings pages and offers a quick way to navigate from the search results to the specific page where the text was found." This PR filters the sidebar's own navigation labels only — no settings-page content is searched, and there are no search results to navigate to. Merging with fixes #5146 in the body auto-closes the issue and the actual requirement is lost. Either change the body to refs #5146 (and leave the issue open for the content search), or implement content search.

F2 — medium — filter state goes stale when the navigation re-renders.
settingsSidebar.js mutates the hidden attribute on DOM nodes that Blazor owns, and the search term lives only in the DOM: the <input> at SettingsLayout.razor:11-12 has no @bind and there is no C# field holding it. Meanwhile ExecutionModeAuthorizeView subscribes to userConfig.OnChange and calls StateHasChanged on every config / execution-mode change (ExecutionModeAuthorizeView.razor, OnInitialized + OnChange), and SettingsLayout does the same (SettingsLayout.razor:308-316).

hidden is not part of the render tree, so the Blazor diff neither preserves it deliberately nor restores it: <li> elements newly created by an ExecutionModeAuthorizeView that just became authorized arrive with no hidden attribute. Switch execution mode (the selector is reachable from the navbar on any settings page) while a search term is active and unfiltered entries reappear alongside the filtered ones, with the search box still showing the term. Nothing re-invokes filterSettingsSidebar — only another keystroke does. Filtering in C# (see F3) removes this class of bug by construction.

F3 — medium — the JS/DOM approach duplicates an idiom the repo already has in C#.
roles/ui/files/FWO.UI/Pages/Settings/SettingsOwner.razor:46 already implements the same UI pattern server-side, with the same text key:

<input id="searchOwner" type="text" class="form-control me-1" @bind="OwnerSearchTerm" @bind:event="oninput"
       placeholder="@userConfig.GetText("search")" aria-label="@userConfig.GetText("search")" />

A searchTerm field plus @if guards around the nav entries needs no settingsSidebar.js, no _Host.cshtml script entry, no element-id contract (F5, F9), is directly unit-testable with bUnit (F4), and fixes F2 and F7. Right now the codebase carries two mechanisms for one job.

F4 — medium — the filtering algorithm has zero test coverage.
SonarQube reports 0.0% coverage on new code. The single added test, UiSettingsLayoutTest.cs:113-136, asserts only that the interop call happened with ("settingsNavigation", "manage"). Its name is SettingsLayout_SearchInput_FiltersRenderedNavigation, but nothing in the rendered navigation is filtered or asserted — bUnit does not execute the JS. Section grouping, heading-match reveal, the empty-term reset, and separator handling are all unverified, and there is no JS test harness in the repo to verify them. CLAUDE.md requires >80% coverage on new code. Either move the logic to C# (F3) and test it, or rename the test to what it actually checks (e.g. ..._InvokesSidebarFilter) and state the coverage gap.

F5 — medium — the JS depends on a markup contract nothing enforces, and <hr> does not end a section.
settingsSidebar.js:12-19 groups #settingsNavigation's children as "an <li> containing an <h5> opens a section, every subsequent <li> belongs to it", and <hr> elements do not reset currentSection. That holds today only because every <hr> in SettingsLayout.razor (lines 37, 55, 91, 115, 134, 153, 178, 213, 253, 269) happens to be immediately followed by a heading <li>. Add a group without an <h5> heading, or an entry before the first heading, and:

  • entries after an <hr> are silently attached to the previous section and get revealed whenever that unrelated heading matches;
  • an entry before the first heading hits the currentSection undefined guard and is never filtered at all — it stays visible for every search term.

No test or assertion protects the contract, and the contract is not documented in the razor file.

F6 — medium — .settings-search duplicates the .bg-blue gradient rather than reusing it.
SettingsLayout.razor.css:2 is a verbatim copy of roles/ui/files/FWO.UI/wwwroot/css/site.css:122, hardcoded fallbacks included:

background-image: linear-gradient(var(--bg-color, #054B8C), var(--bg-color-2, #03335E));

The sidebar container itself (#lsb-content) gets its background from that very class (Sidebar.razor, SidebarCssClass defaults to "bg-blue"). So the sticky bar's background is correct today purely because the copy is identical — and it will silently diverge the moment the sidebar palette changes. Note also that --bg-color / --bg-color-2 are declared only as an inline style on nav#navbar (NavigationMenu.razor:17-18); the sidebar is not in that subtree, so both rules currently resolve to the hardcoded fallbacks. Prefer adding bg-blue to the sticky div's class list (adjusting the z-index) or extracting a shared class, so there is one definition.

F7 — medium — every keystroke costs a server round-trip, undebounced.
@oninput on a Blazor Server circuit means each character goes browser → SignalR → server handler → interop → back to the browser, for work that is entirely client-side. There is no debounce. On a high-latency link typing into the box will feel laggy for no functional benefit. Either debounce, or do the filtering client-side only / in C# without the interop hop (F3).

F8 — low — external users now get a rendered-but-empty <li>.
The restructure at SettingsLayout.razor:273-280 moved the personal_settings NavLink out of the shared <li>, so the password <li> now wraps only the @if:

<li class="nav-item px-2">
    @if (userConfig.User.Dn.EndsWith(GlobalConst.kLdapInternalPostfix))
    {
        <NavLink class="nav-link" href="settings/password"> ... </NavLink>
    }
</li>

For an external (non-internal-LDAP) user this renders an empty list item — announced by screen readers as a blank list entry. Moving the @if outside the <li> keeps the section-grouping benefit without it. SettingsLayout_HidesPasswordLink_ForExternalUser passes because it only asserts the anchor is absent.

F9 — low — "settingsNavigation" is a magic string in three places.
The id appears in the markup (SettingsLayout.razor:14), in the interop call (:305), and in the test (UiSettingsLayoutTest.cs:133). A private const string referenced from both markup and code removes the drift risk; CODING_GUIDELINES.md asks for no magic values.

F10 — low — separators are dropped wholesale while searching.
settingsSidebar.js:29-30 hides every <hr> for any non-empty term, even one that matches everything, so a search that leaves several sections visible runs them together. Keeping separators between visible sections would be more consistent with the per-section heading logic.

F11 — low — no help content for a UI feature change.
CLAUDE.md requires help content under roles/ui/files/FWO.UI/Pages/Help/ to be updated for every UI feature change. HelpSettings.cshtml and HelpSettingsSidebar.cshtml exist and are untouched; the new search bar is undocumented.

F12 — low — whats_new_facts not updated.
CLAUDE.md requires new features to be written into the whats_new_facts entry in fworch-texts.sql (lines 532 German / 544 English), keeping all languages in sync. This user-visible feature is absent from both.

F13 — low — copy-pasted, unused setup and a magic number in the new test.
UiSettingsLayoutTest.cs:125-126 carries the navbar-height boilerplate from the neighbouring tests:

layout.WaitForAssertion(() => Assert.That(GetNavbarHeightSubscriberCount(eventService), Is.EqualTo(1)));
await layout.InvokeAsync(() => eventService.InvokeNavbarHeightChanged(50));

Nothing the test asserts depends on either line, and 50 is an unexplained magic number (it recurs in the pre-existing tests, so a shared named constant would fix all of them).

F14 — low — no feedback when nothing matches.
A term matching no entry leaves the sidebar empty with no explanation, and there is no aria-live region announcing how many entries remain, so screen-reader users get no signal that the list changed as they type.

F15 — low — no diacritic folding.
matchesSearch uses String.prototype.includes on toLocaleLowerCase() output, which does not fold accents. With the German UI, typing Uberwachung/Anderung matches nothing against Überwachung/Änderung. Normalizing both sides (normalize("NFD").replace(/\p{Diacritic}/gu, "")) would make the search usable without a German keyboard layout.

Security pass

No security findings. Concretely, for the code reachable from this diff:

  • XSS: the search term is never written back into the DOM — no innerHTML, no insertAdjacentHTML, no eval. It is used only as the argument to String.prototype.includes, and only textContent is read. Clean.
  • Authorization: no authz surface changes. The filter is presentational — it toggles hidden on entries the server already chose to render, so a user cannot reveal an entry their execution mode / role does not render, and page-level authorization on each settings/* route is untouched. ExecutionModeAuthorizeView semantics are unchanged.
  • Injection / secrets / deserialization / SSRF / TLS / tenant isolation: no SQL, GraphQL, LDAP-filter, shell or path handling; no credentials, tokens or log output; no new network call or deserialization path; no tenant-scoped query.
  • navigationId comes from a server-side literal, not user input, so document.getElementById cannot be steered.

Not raised as a finding: jsRuntime.InvokeVoidAsync in FilterSettings is unguarded against JSDisconnectedException, so a keystroke in flight when the circuit drops throws out of the event handler. grep -rn "JSDisconnectedException" roles/ui roles/lib returns nothing — every interop call in the UI is unguarded, so this is existing house style rather than something this PR introduces. Worth a separate cross-cutting ticket if it matters.

Recommendations

Should fix before merge

  • F1 — correct the issue linkage. Closing "Settings: Full-text search" with a label filter loses the requirement; refs #5146 costs nothing.
  • F2 + F3 + F7 — these are one decision. Doing the filter in C# (searchTerm field + @if, mirroring SettingsOwner.razor:46) drops settingsSidebar.js, the _Host.cshtml entry and the id contract, makes the filter survive re-renders, ends the per-keystroke interop hop, and makes the logic testable. If the JS route is kept deliberately, F2 still needs an answer: re-apply the filter after re-render (e.g. OnAfterRenderAsync invoking the filter with the current term held in C#).
  • F4 — either move the logic to C# and test it, or rename the test so it does not claim to verify filtering, and note the JS gap explicitly.

Nice to have

  • F5 — document the markup contract in SettingsLayout.razor and let <hr> close a section; harden the pre-heading case.
  • F6 — one definition for the sidebar gradient.
  • F8, F9, F13 — small hygiene fixes, cheap now.
  • F11, F12 — required by CLAUDE.md before the feature ships in a release.
  • F10, F14, F15 — UX/accessibility polish; F15 in particular affects the German UI.

Process disclosures

  • Review depth: standard, performed directly against this skill's correctness/quality and security checklists on the primary model (no separate built-in review command or review subagent was used for the analysis passes). Escalation to deep was not warranted: 86 added lines, UI-only, no schema/auth/installer surface.
  • Delegated to a reduced model tier (1 sub-agent, Haiku): mechanical evidence gathering only — presence and location of the search text key in fworch-texts.sql, the inventory of existing *.razor.css files, the _Host.cshtml <head> contents and CSS-isolation bundle link, the declaration sites of --bg-color/--bg-color-2, Sidebar.razor markup, and the first lines of the existing wwwroot/js/*.js files. It returned file/line evidence only. Every verdict, criticality rating, the security pass, the numbering and this comment were produced on the primary model. All builds, the test run and the dotnet format check were run inline, not delegated.
  • Usage budget: this environment exposes no usage/quota indicator to the agent, so the 25% ceiling could not be measured. The observable proxy limits were enforced instead and respected: 1 sub-agent dispatch of the 6 allowed, standard depth (no escalation), reads confined to the diff plus its direct callers/contracts/tests (no whole-tree scan), and no pass or fetch repeated. No checklist item was dropped for budget reasons; the security pass and the per-candidate verification step were both completed in full.
  • Prior history: gh pr view --json reviews,comments and a reviewThreads GraphQL query both returned no human or agent review (only the SonarQube bot comment), so this is round 1 and numbering starts at F1. F1–F15 are now reserved; a following round must keep these numbers and continue at F16.

@tpurschke tpurschke changed the title add searchbar for settings v9.5.3 - add searchbar for settings Sep 10, 2026
@tpurschke tpurschke linked an issue Sep 16, 2026 that may be closed by this pull request
@sonarqubecloud

Copy link
Copy Markdown

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Settings: Full-text search

1 participant