add deployment-wide user directory for user search - #474
maxwellpeterson wants to merge 2 commits into
Conversation
github-actions
Bot
added
workshop/frontend
Preview:
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Posted 2 actionable inline findings. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
8495984 to
f896d13
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Posted 1 actionable inline finding. |
f896d13 to
c739b82
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Posted 1 actionable inline finding. |
097a586 to
ab0dbb9
Compare
| ON CONFLICT (id) DO UPDATE SET name = excluded.name, search_text = excluded.search_text, | ||
| rev = excluded.rev | ||
| WHERE excluded.rev > users.rev`, | ||
| record.id, record.name, `${record.id}\n${record.name}`.toLowerCase(), rev); |
There was a problem hiding this comment.
[P1] Bound user-controlled profile fields before indexing them. setOwnDisplayName() and password signup accept arbitrary-length strings, and this line copies the id/name into the one deployment-wide singleton. A user can persist megabytes into name/search_text; every substring scan then processes that row and a matching search returns it over RPC, so repeated accounts can exhaust singleton storage/CPU and disrupt search for everyone. Enforce shared server-side limits on ids/display names, and reject oversized records defensively here.
| const handleAddCollaborator = async () => { | ||
| const username = addUsername.trim() | ||
| if (!username || addingRef.current) return | ||
| const userId = selectedUser?.id ?? (canInviteUser ? directoryQuery : '') |
There was a problem hiding this comment.
[P2] Make the primary Invite action honor the highlighted directory result. After typing a display name such as Ada, the first match is rendered highlighted and aria-selected, but selectedUser remains null until the option itself is clicked, so Invite submits raw Ada; an email-backed account such as ada@example.com then reports no account found. Submit the active match here, or expose raw canonical-id submission as a separate explicit option so the lazy-index fallback remains available.
|
Posted 3 actionable inline findings: one P1 and two P2 issues. |
|
No additional findings beyond the existing inline review comments. |
| const directoryQuery = addUsername.trim() | ||
| // Everyone already on the workspace: the caller, the owner (absent from listCollaborators() | ||
| // when the caller is a collaborator), and every collaborator. | ||
| const directoryExcludeIds = useMemo(() => [ |
There was a problem hiding this comment.
[P2] Keep exclusions within the directory RPC contract. This array grows with every collaborator, but UserDirectoryDurableObject.searchUsers() rejects more than 1,000 distinct IDs and there is no corresponding collaborator cap. Once a workspace has 1,000 collaborators (plus its owner), every lookup fails with the temporary-unavailable state, so display-name search stops working for that workspace. Either avoid sending the entire membership list while still filtering results correctly, or make the RPC support the workspace size.
| user’s next connection. | ||
| </p> | ||
| </div> | ||
| <Switch |
There was a problem hiding this comment.
[P2] Give this switch an accessible name. With no label, aria-label, or aria-labelledby, Kumo falls back to aria-label="Switch"; the Access tab therefore exposes this and the sign-up control under the same generic name, so a screen-reader user cannot tell which deployment policy they are changing. Associate it with the heading or provide an explicit accessible label.
|
Posted 2 actionable inline findings. |
df488e1 to
6b1a09d
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
6b1a09d to
713a581
Compare
| const directoryQuery = addUsername.trim() | ||
| // Everyone already on the workspace: the caller, the owner (absent from listCollaborators() | ||
| // when the caller is a collaborator), and every collaborator. | ||
| const directoryExcludeIds = useMemo(() => [ |
There was a problem hiding this comment.
[P2] Wait for the membership list before searching. collaborators starts empty while loadData() is in flight, so on a slow workspace lookup this search can return an existing collaborator and let the user select it. When listCollaborators() later fills the exclusions, selectedUser keeps directorySearching false, so that stale selection is never invalidated; Invite can then add or upgrade a direct permission edge for someone who was already present via a share link. Gate directory search on the initial membership load, and clear any selected result when the exclusions change.
|
Posted 1 actionable inline finding. |
713a581 to
b273b22
Compare
| ...collaborators.map(({ profile }) => profile.id), | ||
| ], [collaborators, currentUser, metadata.owner]) | ||
| const directorySearching = userSearchEnabled && selectedUser === null && directoryQuery !== '' | ||
| const directoryOpen = directorySearching && !directoryDismissed |
There was a problem hiding this comment.
[P2] Keep previous-query results closed. After results for one query settle, changing the input commits while directory still contains those old results; the effect clears them only in a later update. Because directoryOpen remains true, a rapid Enter can select the old highlighted account while the field shows the new query, and aria-activedescendant also references that stale option. Require directory.query === directoryQuery here so mismatched results cannot render or enter the keyboard path.
|
Posted 1 actionable inline finding. |
| return this.#user.setOwnDisplayName(name); | ||
| } | ||
| async searchUsers(query: string, excludeIds: string[]): Promise<UserDirectoryRecord[]> { | ||
| if (!(await readAdminConfig(this.env)).userSearchEnabled) return []; |
There was a problem hiding this comment.
Will searchUsers() be invoked several times in rapid succession as a user types? If so, it may be worthwhile to cache the admin setting lookup in-memory in the AuthenticatedApiImpl object.
2 participants
Adds a central user directory to support platform-wide user search. This is implemented as a singleton DO in the workshop backend that stores a copy of user metadata from each user DO. When user metadata is updated in the user DO, the user DO propagates those changes through to the directory DO by calling the
syncUserRPC method. The directory is incrementally backfilled with existing user records on next login.The backend implementation is as simple as possible. DO SQLite supports FTS5 for full-text search, but this didn't seem worth the complexity to use here. Local benchmarks showed that it was sometimes slower than a full table scan with substring search, and simple substring search is fine for our use case.
I would recommended reviewing
packages/workshop-backend/src/user.tsandpackages/workshop-backend/src/user-directory.tsfirst, since they contain the bulk of the backend changes. This PR is also split into two independently reviewable commits. The first adds the user directory, and the second adds an admin toggle for controlling access to the user directory: