Skip to content

fix: Restructure the username typeahead so the search page gets a non-admin suggestion source - #11639

Open
arvid-e wants to merge 6 commits into
feat/186841-ui-for-search-filtersfrom
fix/186841-username-suggestion-source
Open

fix: Restructure the username typeahead so the search page gets a non-admin suggestion source#11639
arvid-e wants to merge 6 commits into
feat/186841-ui-for-search-filtersfrom
fix/186841-username-suggestion-source

Conversation

@arvid-e

@arvid-e arvid-e commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Why

Merging master into feat/186841-ui-for-search-filters (5d53d7d) switched the shared
SearchUsernameTypeahead to /activity/suggestions, which is adminRequired.
SearchFilterPanel renders that same component on the general search page, so the
author/editor username suggestions 403 for every non-admin user.

The merge commit resolved the textual conflicts only; this PR is the behavioural fix,
kept separate so it gets reviewed rather than buried in a 187-file merge diff.

Review then showed that injecting a second source is not enough on its own: the shared
directory ends up knowing about both domains, and the newly-reachable endpoint is
guest-allowed and hands out more than the search page needs. Those follow-ups are part of
this PR — see Scope added from review.

What changed

The suggestion source is now supplied by the caller instead of the component hard-coding
one endpoint:

adapter endpoint auth used by
useAuditlogUsernameSuggestions /activity/suggestions adminRequired AuditLogManagement, AuditLogExportModal
useRegisteredUsernameSuggestions /users/usernames loginRequired (guest-allowed) SearchFilterPanel

/users/usernames lost its client hook when master removed useSWRxUsernames, so a
narrower one is restored in ~/stores/user: active + inactive users, no activity-snapshot
users (audit-log specific, and admin-gated server-side anyway). Item 2a below narrows
this further to active users only
— unlike activity-snapshot, the inactive option turns
out not to be admin-gated server-side.

The group headers are now localised (#11640, fixed here). Making the menu render for
non-admins would otherwise have exposed the component's hardcoded English Active User /
Inactive User headers. The keys had to go in commons: admin pages load ['admin'] and
the search page loads ['translation'], so commons — always prepended by
pages/common-props/i18n.ts — is the only namespace both callers get. All five locales
are populated, following the existing admin.json user_management.user_statistics.*
terms.

Scope added from review

Four items came out of review (#11639 review comments, 2026-08-04). Items 1, 2 and 4 are
fixed here; item 3's scope is being confirmed (see it).

Progress: 2a and 2b are implemented (45da028), with mutation evidence under
Testing. Item 1, item 2's open decision and item 4 are still scope, not
completed work.

1. UI — empty group renders a dangling header (discussion) — in scope

renderMenu maps over every Categories value, and the only empty-check is the
whole-menu one. When a keyword matches active users but no inactive ones, the menu ends
with an Inactive User header and a divider above it, with nothing under it.

Pre-existing behaviour, but this PR changes how often it shows: on the admin audit-log
page a keyword usually matches both groups, whereas on the search page "zero inactive
matches" is the normal case — so the empty header becomes the default appearance. It is
also the same block this PR already rewrites for i18n.

Item 2a makes this unconditional, not merely common: once the search page stops
requesting inactive users, that group is always empty there, so every suggestion menu on
the search page ends with a dangling header until this is fixed.

  • Skip empty groups, and derive the divider from "a group has already been emitted"
    rather than from index
  • Spec case with one group empty (the current group-header spec covers
    both-groups-populated only)

2. Open-wiki permission — what a guest actually gets (comment) — in scope

/users/usernames is mounted with the guest-allowed loginRequired = loginRequiredFactory(crowi, true)
(users.js:120, :1556), and /_search is mounted the same way (routes/index.js:53,
:365). Both gate on the same aclService.isGuestAllowedToRead() (login-required.ts:57),
so on an open wiki an anonymous visitor reaches the page and the endpoint succeeds.

Two corrections to what was written here before, both of which change what needs fixing:

  • The original "What to review" note said the endpoint "already exposes the same data to any
    logged-in user". Wider than that — guests too, per the above.
  • The first revision of this section then concluded that anonymous username enumeration
    is the new exposure, and prescribed a useIsGuestUser() gate. That is mostly wrong.
    Active usernames are already anonymously readable on an open wiki: GET /_api/v3/users
    takes a searchText substring and is guest-allowed, deliberately so — it gates only
    email search behind if (req.user != null) (users.js:327, :377) — and every page
    displays its creator and last updater (AuthorInfo.tsx, no guest gate). So suggesting
    active usernames to a guest exposes no new class of data, and the useIsGuestUser() gate
    is dropped (see 2d for why it is also actively harmful).

What is genuinely wrong is narrower and sharper:

2a. The inactive-user option is not admin-gated (the actual leak)

The hook asks for active and inactive users (stores/user.tsx:61-62). The route
admin-gates the activity-snapshot option (users.js:1572) and the mixed option
(users.js:1634), but there is no check at all on the inactive option
(users.js:1570). "Inactive" is awaiting-approval / suspended / invited
(models/user/conts.ts:13-18; deleted accounts have their username rewritten, so the
regex cannot match them).

That contradicts the neighbouring guest-allowed endpoint, which explicitly refuses
non-admin status selection — "the param 'selectedStatusList' is not allowed to use by the
users except administrators"
(users.js:137-149). So as it stands the search page hands
every visitor, anonymous included, accounts that GROWI elsewhere treats as admin-only and
that appear nowhere in the normal UI: someone suspended, or invited last week and never
signed up. Unreachable before this PR because only admin screens called the route.

The gate follows this handler's own idiom — a silent downgrade (200, with the
inactiveUser key simply absent), the same shape as wantsActivitySnapshotUser and the
existing spec that locks it (users.integ.ts, "does not include activity snapshot
usernames for non-admins even when requested"
). validator.statusList is cited above for
the policy it already establishes, not as a style to copy: throwing a 400 would put a
second convention inside one handler and would force error handling into the typeahead
hook, where an absent group renders as nothing.

  • Stop requesting inactive users from the search page (active only)
  • Admin-gate isIncludeInactiveUser server-side — the client change alone leaves the
    option open to curl
  • Update the two existing specs that asserted a non-admin receives inactive users
    (users.integ.ts"returns inactive users when isIncludeInactiveUser is
    requested"
    , "classifies a deleted user as inactive rather than dropping them"):
    the requester is now an admin, since they encoded the pre-fix contract
  • Spec case: a non-admin request asking for inactive users does not receive them
  • Spec case: the withheld group cannot come back through mixedUsernames, whose
    non-admin clause (users.js:1635-1637) still permits the merged list

2b. A guest can error the endpoint, and the internal message is returned to them

req.user.admin is dereferenced with no optional chaining at users.js:1572 and :1634,
and 2a's fix adds a third site at :1570. req.user is null for a guest, so any
request that reaches one of those reads throws; the handler's try/catch funnels it into
res.apiv3Err(err), which returns the raw TypeError message. Verified against the integ
harness with no session:

GET /usernames?q=bob&options={"isIncludeInactiveUser":true}
→ 400 {"errors":[{"code":null,"message":"Cannot read properties of null (reading 'admin')"}]}

So it is a 400 carrying an internal error message, not a 500 — the leak is the worse
half, and it contravenes "Error messages don't leak sensitive data" in
.claude/rules/security.md. Before 2a only the obscure isIncludeMixedUsernames reached
it; after 2a the ordinary isIncludeInactiveUser: true does, so 2a and 2b must land
together
.

  • req.user?.admin at all three sites (:1571, :1573, :1634)
  • Spec case: an anonymous request asking for every privileged option returns 200
    with none of those groups, rather than an error carrying a TypeError message

2c. The load is now anonymously triggerable

The lookup is an unanchored case-insensitive $regex (models/user/index.js:853-859), so
MongoDB cannot use an index and reads every user document — twice per request while the
inactive group is still requested. Previously only an admin on the audit-log screen could
trigger that; now anyone typing in the search filter can, on an open wiki without logging
in and with no rate limit. This is the guest-specific edge of item 3 and is why the
prefix-vs-substring question there is load-bearing rather than cosmetic — an anchored
prefix match can use the existing username index. Fixing it is tracked in item 3, not
duplicated here.

2d. Why not gate the client hook

AsyncTypeahead is rendered without allowNew (SearchUsernameTypeahead.tsx:189-204), so
a chip can only be committed by picking from options. Emptying options for guests
therefore leaves an input that looks usable and silently accepts nothing — dead UI, not a
disabled control.

It also would not remove the capability. Guests can search (the apiv1 /_api/search mount
is guest-allowed, routes/index.js:212-215) and the query parser honours author:/editor:
typed straight into the keyword box (server/service/search.ts:635-645). The gate removes
the discovery affordance, not the filter.

Open decision for the reviewer

2a/2b are unambiguous bugs and are fixed either way. The remaining question is a product
one: on an open wiki, should the author/editor filter fields exist for anonymous
visitors?

  • A (assumed unless told otherwise) — keep them, suggesting active users only. The
    filter is then a convenience over names guests can already read on every page, at a cost
    item 3 makes indexable.
  • B — no suggestions for guests. Then the honest implementation is to hide the
    author/editor fields for guests rather than empty them, plus loginRequiredStrictly on
    the route (safe for the other callers — the audit-log screens are admin-only). Probably
    the group filter too, which already fails for guests (get-related-groups.ts:22 is
    loginRequiredStrictly). Note this still leaves GET /_api/v3/users anonymously
    searchable, which is pre-existing open-wiki behaviour and out of scope here.

This PR proceeds on A; say so and it flips to B.

3. Performance — /users/usernames call volume (comment) — scope needs confirming

The endpoint used to be called only from the admin audit-log screen. Wiring it to the
search page means every logged-in user can trigger it while typing — and, per 2c, every
anonymous visitor on an open wiki — so call volume rises sharply. A concern that already
existed with #11597 and is merely made visible here.

Each call is an unanchored case-insensitive $regex (models/user/index.js:853-859),
which no index can serve, so it scans the whole users collection. Whether the API keeps
substring matching or moves to prefix matching (changed in both directions before)
is therefore the crux, not a detail: an anchored prefix match can use the existing
username index.

Status discrepancy to resolve. This section previously read "agreed to be handled in
a separate PR". The review comment says the opposite —
「この PR のスコープでサーバ側も含めて直してほしいです」 (fix it within this PR's scope,
server side included). Unless that agreement happened somewhere not visible on the PR,
this belongs here. Confirm which it is.

4. Cohesion — split the typeahead per domain (discussion) — in scope

Two problems with the current shape:

  • The shared component's directory now imports both ~/stores/activity and
    ~/stores/user, so a generic UI part carries knowledge of both domains. And because
    its barrel re-exports the audit-log hook, the search page pulls in
    ~/stores/activity through a hook it never uses.
  • Passing a hook as a prop is unusual in React (values, callbacks, render props and
    context are the established forms). The type says
    (keyword: string) => UsernameSuggestions, but the real contract is "a function called
    during render that obeys the Rules of Hooks" — so
    useUsernameSuggestions={isAdmin ? useAuditlogUsernameSuggestions : useRegisteredUsernameSuggestions}
    type-checks and breaks at runtime. That is a plausible thing to write, and the failure
    is obscure. (Relatedly, the username-suggestions.ts JSDoc warns against inline
    closures, which is not the real hazard — React tracks hooks by call order, not
    function identity, so (kw) => useMySource(kw) is fine. The note points at the wrong
    risk and omits the conditional-swap one.)

Target structure: one shared typeahead, plus two dedicated pairs (component + its own
hook) co-located per domain.

client/components/UsernameTypeahead/                  ← shared (renamed)
├── UsernameTypeahead.tsx
├── username-suggestions.ts          (contract type + toUsernameSuggestions)
├── should-show-username-suggestion.ts
├── UsernameTypeahead.spec.tsx
└── index.ts                         → component + types only

client/components/Admin/AuditLog/AuditlogUsernameTypeahead/     ← dedicated
├── AuditlogUsernameTypeahead.tsx
├── use-auditlog-username-suggestions.ts
├── AuditlogUsernameTypeahead.spec.tsx   (the /activity/suggestions contract lives here)
└── index.ts                             → component only

features/search/client/components/SearchPage/SearchUsernameTypeahead/   ← dedicated
├── SearchUsernameTypeahead.tsx
├── use-registered-username-suggestions.ts
├── SearchUsernameTypeahead.spec.tsx     (the /users/usernames contract lives here)
└── index.ts                             → component only

toUsernameSuggestions is a generic mapping and stays on the shared side; both dedicated
hooks import it. The dependency direction stays one-way: dedicated → shared.

What this buys:

  • The shared directory imports no ~/stores/* at all.
  • The search page can no longer reach ~/stores/activity: each dedicated hook is used by
    exactly one component in its own directory, so it never leaves the barrel.
  • useUsernameSuggestions is passed in exactly two places, each with a fixed value, and is
    invisible outside the shared/dedicated pair — the conditional swap above becomes
    unwritable.
  • placeholder no longer has to be passed by callers. It is passed today only because the
    default key lives in the admin namespace, which the search page does not load; each
    dedicated component can carry its own namespace's key as the default.
  • The "which endpoint does it call" assertion moves out of SearchFilterPanel.spec.tsx
    into the dedicated component's spec, next to the file that decides it.

Rename: the shared component becomes UsernameTypeahead (an input for picking several
usernames). The original "Search" meant "an input used for searching", but now that it is
used on the actual search page it reads as "the search page's one" — and it would collide
with the dedicated search-page component. Freeing the name lets
SearchUsernameTypeahead sit where it is actually accurate.

Implementation notes carried over from the review:

  • ref must reach the shared component. AuditLogManagement clears the input via
    typeaheadRef.current.clear(). With a dedicated component in between, a plain
    function component makes React drop the ref and typeaheadRef.current stays null
    no error, no warning, clear() simply never runs. Write the dedicated components
    with forwardRef and forward to the shared component, and add a test that passing
    a ref to the dedicated component and calling clear() empties the input
    .
  • Name the dedicated props type. It is
    Omit<Props, 'useUsernameSuggestions'>; export it once from the shared component
    (e.g. UsernameTypeaheadOwnProps) instead of repeating the expression in both
    dedicated components.
  • Keep id overridable. SearchFilterPanel renders two inputs (author, editor)
    and gives each a distinct useId() value. That id also forms each option's id
    ({id}-item-0), which keyboard navigation and screen readers use to point at the
    active option — duplicated ids can point at the other input's options.

Unchanged by the refactor: the group-header keys stay in commons, because the shared
component still renders the headers and both the admin and search pages use it.

👀 What to review

The one product decision in item 2: on an open wiki, should the author/editor filter
fields exist for anonymous visitors (2 → Open decision)? The search page calling
/users/usernames as any logged-in user is the intended change, and 2a/2b get fixed
regardless — that question only decides whether guests keep the fields at all.

Whether item 3 stays deferred, since the review comment asked for it in this PR's
scope and 2c makes it anonymously triggerable.

The two sources match keywords differently, and that is accepted here.
/users/usernames matches by case-insensitive substring (see its OpenAPI description;
a previous commit deliberately reverted it from prefix to substring), while
/activity/suggestions is Elasticsearch fuzzy/wildcard. So the same keystrokes can suggest
different sets on the search page vs. the admin audit-log page. Unifying them would mean
either exposing the ES-backed endpoint to non-admins or changing audit-log behaviour — out
of scope here. The substring/prefix question itself belongs to item 3.

Testing

Landed with 2a / 2b (45da028)

users.integ.ts — 9 tests pass. Two pre-existing cases were updated because they encoded
the pre-fix contract (their requester is now an admin); three added:

Case Guards
does not include inactive users for non-admins even when requested 2a — and asserts it degrades (200, activeUser still served) rather than failing, matching this handler's idiom
does not leak inactive usernames to non-admins through mixedUsernames the merge path, whose non-admin clause still permits the flat list
serves a guest the active users only, without erroring, when privileged options are requested 2b — exercises all three req.user?.admin sites in one request, and asserts body.errors is absent so the TypeError message cannot be handed back

SearchFilterPanel.spec.tsxdoes not ask the suggestion endpoint for inactive users.
Asserted on the request the page actually issues, and on !== true rather than === false,
so omitting the option entirely — the equivalent request — still passes.

Mutation-checked, each reverted afterwards:

Mutation Result
Drop && req.user?.admin from wantsInactiveUser 3 red (non-admin, mixedUsernames, guest)
?.. at :1571 / :1573 / :1634 (separately) 1 red each (guest)
Client back to isIncludeInactiveUser: true 1 red (panel)

pnpm run lint:typecheck clean; Biome clean on the touched files (users.js reports three
findings at lines 144/465/1086, all pre-existing and untouched).

Already present (before the scope additions above land)

  • Guard for the privilege boundary this PR fixes (SearchFilterPanel.spec.tsx, new):
    asserts the panel requests /users/usernames and never /activity/suggestions.
    It asserts on the endpoint actually requested rather than on which hook was passed, so
    it survives refactoring of how the source is injected, and it waits for a suggestion
    request to land before the negative assertion so that assertion cannot pass vacuously.
    Mutation-checked: wiring the panel to the audit-log source turns it red.
    (Item 4 moves this assertion into the dedicated component's spec.)
  • Component spec now injects a fake source instead of mocking the store; new case
    asserts the injected source receives the typed keyword (mutation-checked: hardcoding
    the keyword to '' turns it red).
  • New spec per adapter covering the response-shape mapping that broke during the merge,
    the missing-field case, and isLoading suppression on error.
  • The group-header spec asserts the full commons:-prefixed key, so it locks the
    namespace choice, not merely that translation happens. Mutation-checked twice:
    reverting to the raw category value turns it red, and so does moving the keys to the
    admin namespace.
  • pnpm run lint:typecheck clean; vitest --project app-components 90 files / 668 tests
    and --project app-unit 275 files / 3488 tests pass.

Still to be added: the one-group-empty menu case (item 1) and the ref-clear() case
(item 4).

When merged

…eTypeahead

master switched the shared typeahead to `/activity/suggestions`, which is
adminRequired. SearchFilterPanel renders that same component on the general
search page, so the author/editor username suggestions 403 for every non-admin
user.

The source is now supplied by the caller as a required `useUsernameSuggestions`
prop:

- `useAuditlogUsernameSuggestions` -> `/activity/suggestions` (adminRequired),
  used by AuditLogManagement and AuditLogExportModal. It suggests operators
  recorded in activities, including users who no longer exist.
- `useRegisteredUsernameSuggestions` -> `/users/usernames` (loginRequired),
  used by SearchFilterPanel. Registered users only.

The prop is required rather than defaulted: the two sources demand different
privileges, so a silent default is precisely what produced the 403.

`/users/usernames` lost its client hook when master removed `useSWRxUsernames`,
so a narrower one is restored in `~/stores/user`: active + inactive users, no
activity-snapshot users (audit-log specific, and admin-gated server-side).

The component spec now injects a fake source instead of mocking the store, and
each adapter has its own spec covering the response-shape mapping that broke
during the merge.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@arvid-e arvid-e changed the title fix(search): inject the username-suggestion source into SearchUsernameTypeahead fix: Inject the username-suggestion source into SearchUsernameTypeahead Aug 4, 2026
@mergify

mergify Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

… endpoint

The privilege boundary this branch fixes had no coverage: nothing failed if a
caller wired `useAuditlogUsernameSuggestions` into SearchFilterPanel, silently
restoring the 403 for every non-admin. SearchFilterPanel had no spec of its own
and was stubbed out in the only spec that referenced it.

The assertion is on the endpoint actually requested, not on which hook was
passed as a prop, so it survives refactoring of how the source is injected. It
waits for a suggestion request to land before asserting the admin endpoint was
not called — otherwise the negative assertion passes vacuously, before the
debounced `onSearch` has fired.

Renders share the SWRConfig fresh-cache wrapper used elsewhere in the repo: the
suggestion hooks are `useSWRImmutable`, so a key cached by an earlier test is
served without calling the fetcher again, which made the assertions pass only
in first-run order.

Mutation-checked: swapping the panel to the audit-log source turns both tests
red; reverting turns them green.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
arvid-e and others added 3 commits August 4, 2026 05:51
The typeahead printed its group headers straight from a hardcoded English const
("Active User" / "Inactive User"). Those strings were tolerable while the
component was admin-only, but giving the search page a loginRequired suggestion
source makes the menu render for non-admins, so untranslated headers become
user-visible. Closes #11640.

The `Categories` values stay load-bearing — `renderMenu` groups options by them
and `toUserDataItem` defaults to one — so they become opaque internal keys and
the label is translated at render time via CATEGORY_LABEL_KEYS.

The keys live in `commons` because it is the only namespace every caller loads:
admin pages request ['admin'] and the search page requests ['translation'], and
`pages/common-props/i18n.ts` always prepends 'commons'. A key in either of the
other two would render raw on half the call sites — the same constraint that
already forced `placeholder` to be an overridable prop.

Translations follow the existing house terms in admin.json
(user_management.user_statistics.active / .inactive) for all five locales.

The spec asserts the full `commons:`-prefixed key, so it locks the namespace
choice rather than just the fact that some translation happens.
Mutation-checked: reverting to `{category}` turns it red, and so does moving the
keys to the `admin` namespace.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The three preceding commits left 75 lines of comment for ~380 lines of change,
with the adminRequired-vs-loginRequired rationale restated in six places and one
comment duplicated verbatim across both adapters.

Each non-obvious fact is now stated once, where it is most durable, and referred
to elsewhere: the injection rationale and the stable-reference requirement live
on `UseUsernameSuggestions`, the namespace constraint on CATEGORY_LABEL_KEYS, the
SWR-retry caveat in the audit-log adapter. Restatements shrink to a pointer, and
sentences that narrated the adjacent code are gone.

Kept in full: the constraints that are expensive to re-derive — why `options`
must be a JSON string, why the SWR cache needs a per-render provider, and why the
negative endpoint assertion needs to wait for a request first.

Comments only; no code or behaviour changed. 20 tests still pass.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…sources

Both adapters derived `isLoading` identically and defaulted their two lists the
same way, which the previous commit had papered over with a cross-file comment
pointer. `toUsernameSuggestions` now owns that shared derivation, so each adapter
is reduced to what actually differs: which endpoint it calls and where in that
payload the two username lists live.

The helper takes a single flat object rather than positional arguments — two
adjacent `string[] | undefined` parameters would be trivially swappable — and
stays out of the barrel, since only these two siblings need it.

No behaviour change. The existing adapter specs cover the extracted logic through
both callers; mutation-checked that they still do — dropping the error
suppression reddens both "reports not-loading once the request has failed" tests,
and dropping the empty-list default reddens the missing-field test.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@arvid-e
arvid-e requested a review from yuki-takei August 4, 2026 06:15
Comment on lines +151 to +156
const items = Object.values(Categories).map((category) => {
const userData = allUser.filter((user) => user.category === category);
return (
<Fragment key={category}>
{index !== 0 && <Menu.Divider />}
<Menu.Header>{t(CATEGORY_LABEL_KEYS[category])}</Menu.Header>

@yuki-takei yuki-takei Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1. UI

Object.values(Categories).map(...) renders a header (and a divider) for every category, and the only empty-check is the whole-menu one on L146. So when a keyword matches active users but no inactive ones, the menu ends with a dangling Inactive User header and a divider above it.

Verified on this branch (38311f33) by rendering the component with activeUsernames: ['alice'], inactiveUsernames: []:

<div class="dropdown-header">…username_suggestion.active_user</div>
<a class="dropdown-item">alice</a>
<div class="dropdown-divider"></div>
<div class="dropdown-header">…username_suggestion.inactive_user</div>   <!-- nothing under it -->

This is pre-existing behaviour, not something this PR introduced — but it is worth fixing here, because this PR is what changes how often it shows up. On the admin audit-log page a keyword usually matches both groups; on the search page "zero inactive matches" is the normal case, so the empty header becomes the default appearance rather than an edge case. It is also the same block this PR is already rewriting for i18n.

Suggestion: skip empty groups, and derive the divider from "a group has already been emitted" rather than from index:

const items = Object.values(Categories).flatMap((category) => {
  const userData = allUser.filter((user) => user.category === category);
  if (userData.length === 0) {
    return [];
  }
  const isFirstGroup = index === 0;
  return [
    <Fragment key={category}>
      {!isFirstGroup && <Menu.Divider />}
      <Menu.Header>{t(CATEGORY_LABEL_KEYS[category])}</Menu.Header>
      {/* … */}
    </Fragment>,
  ];
});

(index already works as that flag today, since it only advances when items are emitted — the point is just to make the empty case not reach the header.)

The new group-header spec covers the both-groups-populated case only, so a case with one group empty would be worth adding alongside the fix.

@yuki-takei

yuki-takei commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

2. オープン Wiki に対する権限の問題

ゲストユーザーが useSWRxUsernames を使った場合に問題が出るので対応してください

3. パフォーマンス懸念

この hook が呼ぶ /users/usernames は、これまで管理者しか開かない監査ログ画面からしか呼ばれていませんでした。この PR で、ログインしている人が誰でも開く検索画面から呼ばれるようになるので、呼び出し回数が大幅に増えることになりますが、それによってパフォーマンスに懸念が出ます。この PR のスコープでサーバ側も含めて直してほしいです。

その際、API 仕様として部分一致のままいくのか前方一致に仕様を変えるのか(過去に何度か変えた経緯もあるらしいが)、そこも再考の材料としてよいと思います。

Comment on lines +2 to +3
export { useAuditlogUsernameSuggestions } from './use-auditlog-username-suggestions';
export { useRegisteredUsernameSuggestions } from './use-registered-username-suggestions';

@yuki-takei yuki-takei Aug 4, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

4. 凝集度について

提案する構成

共通の typeahead を1つ置き、専用の typeahead とその専用 hook を同じディレクトリにまとめた組を、監査ログ用と検索ページ用で2つ作る形にしてください。

client/components/UsernameTypeahead/                    ← 共通(改名。後述)
├── UsernameTypeahead.tsx
├── username-suggestions.ts             (契約の型 + toUsernameSuggestions)
├── should-show-username-suggestion.ts
├── UsernameTypeahead.spec.tsx
└── index.ts                            → コンポーネントと型だけ

client/components/Admin/AuditLog/AuditlogUsernameTypeahead/     ← 専用
├── AuditlogUsernameTypeahead.tsx
├── use-auditlog-username-suggestions.ts
├── AuditlogUsernameTypeahead.spec.tsx  (/activity/suggestions を叩く契約はここ)
└── index.ts                            → コンポーネントだけ

features/search/client/components/SearchPage/SearchUsernameTypeahead/   ← 専用
├── SearchUsernameTypeahead.tsx
├── use-registered-username-suggestions.ts
├── SearchUsernameTypeahead.spec.tsx    (/users/usernames を叩く契約はここ)
└── index.ts                            → コンポーネントだけ

toUsernameSuggestions は汎用のマッピングなので共通側に残し、両方の専用 hook がそれを import します。向きは「専用 → 共通」の一方向で正しいままです。

現状の問題点の説明

1. 汎用部品のディレクトリが、両方のドメインに依存している

この PR の意図は「共通部品は取得元を知らなくて済む」という形にコードを変えることでした。ところが結果として、汎用の UI 部品のディレクトリが監査ログ(~/stores/activity)とユーザー一覧(~/stores/user)の両方の知識を use-*-username-suggestions.tsx の形で持つ形になってしまっています。

2. hook を props で渡す設計について

React で一般的に確立しているのは「値を渡す」「コールバックを渡す」「render prop を渡す」「context で注入する」で、hook そのものを渡すのは稀です。現在用意されている型は (keyword: string) => UsernameSuggestions としか言っていませんが、実際の契約は「レンダー中に必ず呼ばれ、hook のルールに従う関数」です。この差が、型検査を通る誤用を許します。

例えば以下のような1つの呼び出し箇所で取得元を条件によって入れ替えるような使い方をしてしまうと問題が出ます。

// 型は通る。しかし hook の呼び出し順が変わって実行時に壊れる
useUsernameSuggestions={isAdmin ? useAuditlogUsernameSuggestions : useRegisteredUsernameSuggestions}

「管理者なら検索ページでも ES 由来の候補を出したい」と考えた人が自然に書きそうな形で、しかもエラーの出方が分かりにくいです。

なお、username-suggestions.ts の JSDoc が警戒している「インラインのクロージャではなく module 直下の hook でなければならない」は、実際には成り立ちません。React は関数の同一性で hook を管理しておらず、呼び出し順で管理しているので、(kw) => useMySource(kw) のようなインライン関数でも中で呼ぶ hook が毎回同じなら安全です。注意書きが危険の在り処を外していて、本当に危ない条件分岐のほうは書かれていない状態なので、ここも直したいところです。

リファクタで改善されるもの

1. 共通部品のディレクトリが store を知らなくなる

いまは共通部品のディレクトリの中に監査ログ用とユーザー一覧用の hook が同居しているので、そのディレクトリが ~/stores/activity~/stores/user の両方を import しています。

提案する構成では、その2つの hook はそれぞれの専用ディレクトリへ移ります。共通部品のディレクトリは ~/stores/* を1つも import しなくなり、特定の画面の事情を知らない部品になります。

2. 検索ページが監査ログの store を読み込まなくなる

いまの index.ts(ディレクトリの入口になるファイル。以下 barrel と呼びます)は、監査ログ用の hook も外に出しています。そのため検索ページが ~/client/components/SearchUsernameTypeahead を import すると、使っていない監査ログ用の hook を経由して ~/stores/activity まで読み込まれます。

提案する構成では、専用の hook を使うのは同じディレクトリにある専用コンポーネント1つだけになるので、barrel から外に出す必要がありません。外に出ていないものは他のディレクトリから import できないので、この読み込みはそもそも起こせなくなります。

3. hook を props で渡す場所が2つだけになる

useUsernameSuggestions を渡すのは専用コンポーネント2つだけになり、しかもそれぞれ渡す値が固定になります。上の「現状の問題点の説明」の 2 で書いた、条件によって取得元を入れ替えて実行時に壊すような書き方が、そもそもできなくなります。

この prop は共通部品とその専用コンポーネントの間だけで使うものになるので、他のディレクトリからは見えなくなります。

4. placeholder を呼び出し側から渡さなくてよくなる

いま placeholder を呼び出し側から渡しているのは、既定値のキーが admin の名前空間にあり、検索ページがその名前空間を読んでいないからです。つまり「共通部品の既定値が片方の画面でしか使えない」ことへの回り道です。

専用コンポーネントを作れば、それぞれが自分の画面の名前空間のキーを既定値として持てます。呼び出し側は placeholder を書かなくて済むようになります。

5. 「どのエンドポイントを叩くか」を確かめるテストの置き場所が揃う

いまその確認は SearchFilterPanel.spec.tsx に書かれています。パネルのテストの中に、その中で使っている typeahead が何を叩くかの確認が入っている形です。

専用コンポーネントができれば、そのテストは専用コンポーネントの spec に置けます。叩く先を決めているファイルと、それを確かめるテストが同じディレクトリに並びます。

変わらないこと

グループ見出しのキーを commons に置く判断は、この構成でも変わりません。見出しを描画するのは共通部品のままで、それを管理画面と検索ページの両方が使う限り、両方が読む名前空間に置く必要があるからです。

あわせてお願いしたいこと: 共通部品の改名

共通部品の名前が SearchUsernameTypeahead のままだと、検索ページ用の専用コンポーネントと名前がぶつかります。

元の "Search" は「検索するための入力欄」という意味でしたが、本物の検索ページで使われるようになったので、いまは「検索ページ用のもの」と読めてしまいます。

共通部品を UsernameTypeahead(ユーザー名を複数選ぶ入力欄)に改名すれば、SearchUsernameTypeahead という名前を、それが実際に合っている場所、つまり検索ページ側の専用コンポーネントに使えます。

実装するときに気をつけたい点

ref が共通部品まで届くようにしてください

AuditLogManagement は、入力欄の中身をクリアするために ref を使っています。SearchUsernameTypeahead に ref を渡しておき、あとで typeaheadRef.current.clear() を呼ぶ形です。

この構成では、AuditLogManagement と共通部品の間に専用コンポーネントが1枚入ります。そのため AuditLogManagement が渡した ref は、まず専用コンポーネントに届きます。専用コンポーネントを普通の関数コンポーネントとして書くと React は ref を無視するので、ref は共通部品まで届かず、typeaheadRef.currentnull のままになります。

専用コンポーネントを forwardRef で書き、受け取った ref を中の共通部品の ref に渡してください。

これを忘れたときの壊れ方は気づきにくいです。例外も警告も出ず、clear() が呼ばれないだけなので、「クリアの操作をしても入力欄の中身が消えない」という形でしか表に出ません。レビューでも見落としやすいので、「専用コンポーネントに ref を渡して clear() を呼ぶと入力欄の中身が空になる」ことを確かめるテストを1つ書いてください。

専用コンポーネントが受け取る props の型に名前を付けてください

専用コンポーネントが受け取る props は、共通部品の props から useUsernameSuggestions を除いたものです。型で書くと Omit<Props, 'useUsernameSuggestions'> になります。

これを専用コンポーネント2つのそれぞれに直接書くと、同じ式が2箇所に散ります。共通部品の props を増やしたり減らしたりしたときに、2箇所とも直すことになります。共通部品の側でこの型に名前を付けて(例: UsernameTypeaheadOwnProps)export し、専用コンポーネントはその名前を使ってください。

id は引き続き外から渡せるようにしてください

検索ページは著者と編集者で入力欄を2つ並べるので、SearchFilterPaneluseId() で入力欄ごとに違う id を作って渡しています。専用コンポーネントでも id を受け取って、共通部品に渡してください。

id を渡せないと、2つの入力欄が同じ id を持つことになります。この id は候補一覧の各項目の id{id}-item-0 など)にも使われていて、キーボード操作や読み上げはその id を使って「いまどの候補を選んでいるか」を指しています。id が重複すると、もう一方の入力欄の候補を指してしまう可能性があります。

@arvid-e arvid-e changed the title fix: Inject the username-suggestion source into SearchUsernameTypeahead fix: Restructure the username typeahead so the search page gets a non-admin suggestion source Aug 5, 2026
`/users/usernames` admin-gated the activity-snapshot and mixed-username
options but left `isIncludeInactiveUser` open to anyone, and the search
page's author/editor typeahead requested it. The route is guest-allowed
(`loginRequiredFactory(crowi, true)`), as is `/_search`, so on an open
wiki that handed anonymous visitors suspended and invited account names —
data the sibling `GET /users` withholds from non-admins via
`validator.statusList`.

Gate the option on `req.user?.admin`, following this handler's existing
silent-downgrade idiom (200 with the group absent) rather than the 400
that `validator.statusList` raises, and stop the search page from asking
for the group at all.

The optional chaining is not cosmetic: `req.user` is null for a guest, so
the gate would otherwise throw a TypeError that the handler's catch turns
into a 400 carrying the raw message to the client. The two neighbouring
checks had the same latent bug, reachable via a hand-made query, and are
fixed too.

The two existing specs asserting a non-admin receives inactive users
encoded the pre-fix contract; their requester is now an admin.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

2 participants