implement kernel changes required for context collection sharing - #479
maxwellpeterson wants to merge 1 commit into
Conversation
github-actions
Bot
added
workshop/frontend
Preview:
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| (value: string) => authenticatedApi.searchUsers(value, pickedIds), [authenticatedApi, pickedIds]) | ||
|
|
||
| const pick = (user: UserDirectoryRecord) => { | ||
| authenticatedApi.selectGatekeeperUser(gatekeeperId, user.id).then( |
There was a problem hiding this comment.
This request can settle after Cancel/unmount, or after Done has transferred the currently resolved picks. In either case a successful late response reaches setPicks() after the dialog is gone and neither returned stub is disposed, leaking both remote capabilities for the lifetime of the WebSocket session. The same missing in-flight tracking permits double-click duplicates and out-of-order/omitted selections. Track requests against the picker session, reserve each user/order when the request starts, prevent completion while requests are pending, and dispose any result that arrives after completion or unmount.
| return ( | ||
| <Dialog.Root open onOpenChange={(open) => { if (!open) cancel() }}> | ||
| <Dialog | ||
| className="responsive-dialog !z-[2147483100] !w-[min(520px,calc(100vw-32px))] bg-kumo-base p-0 !outline-none" |
There was a problem hiding this comment.
This raises only Kumo’s popup. Kumo renders its backdrop as a separate fixed portal sibling with no z-index, while this app iframe can already be at 2147483000 after setPresenting(true). A gatekeeper can therefore open the picker while presented and remain visible and pointer-interactive above the backdrop everywhere outside the trusted popup; iframe clicks do not bubble to the host dialog. Please put the whole dialog portal/backdrop above the iframe or make the iframe inert/non-interactive while the trusted picker is open.
|
Posted 3 actionable inline findings. CI build, tests, and lint are passing. |
713a581 to
b273b22
Compare
c0e8659 to
59ac84d
Compare
59ac84d to
58ed2f9
Compare
| search={search} | ||
| onValueChange={(value) => { setQuery(value); setNotice(null) }} | ||
| onSelect={pick} | ||
| onSubmit={() => {}} |
There was a problem hiding this comment.
When user search is disabled, AuthenticatedApi.searchUsers() always returns [], and this no-op leaves no exact-username/email fallback. That is the default deployment configuration (DEFAULT_ADMIN_CONFIG.userSearchEnabled is false), so this picker cannot share with anyone on a default install. Workspace sharing still permits exact identifiers in this mode; this flow needs the same direct-submit path (and should use ServerConfig.userSearchEnabled to present it).
| role="listbox" | ||
| aria-label="Matching people" | ||
| aria-busy={searchState.status === 'loading'} | ||
| className="themed-floating-shadow-lg absolute left-0 top-full z-30 mt-2 max-h-64 w-full overflow-y-auto rounded-2xl border border-kumo-line/70 bg-kumo-base p-2 sm:w-96" |
There was a problem hiding this comment.
Kumo 2.13.2 gives every Dialog popup overflow-hidden. Because this listbox stays inside that popup and is absolutely positioned below the input, it does not enlarge the dialog and almost all of its 64px/256px content is clipped by the roughly 20px body padding below the input. In practice the search results cannot be clicked. Render the list in a dialog-owned portal/overlay as ShareModal does, or otherwise place it within an unclipped bounded area.
| if (isImeComposing(event)) return | ||
| if (event.key === 'Enter') { | ||
| event.preventDefault() | ||
| const user = open ? results[activeIndex] : undefined |
There was a problem hiding this comment.
results can still belong to the previous query here: changing the controlled value renders the new query immediately, while the effect that clears results and marks the next query loading runs afterward. A rapid Enter can therefore call onSelect() for the old highlighted user and grant the wrong person access. searchState.query already records which query produced the results; only expose/select them when it matches the current query (the existing ShareModal uses this directoryCurrent guard).
|
Posted 3 actionable inline findings. |
58ed2f9 to
5066fd6
Compare
|
| record.vendorId === vendorId && areCredentialsValid(record) && | ||
| (!record.autoProvisioned || ambientGatekeeperMode(config, vendorId) !== "disabled") | ||
| ); | ||
| return matches.length === 1 ? await matches[0].account.getVerifier() : null; |
There was a problem hiding this comment.
In the case of multiple accounts, we could also return multiple matches and let the the user pick between them. This would expose the existence of those accounts to the user doing the picking, which we might not want.
For the initial context gatekeeper use case, users will never have multiple accounts, so we could defer this until there's a concrete need to handle multiple accounts here. This is all self-contained in the workshop, so adding support for multiple accounts in the future would not require changing the gatekeeper-facing API.
There was a problem hiding this comment.
Alternatively, we could also pass multiple accounts to the gatekeeper and let it decide what to do with them.
maxwellpeterson
changed the title
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Kernel and UI concerns are bundled
The PR mixes substantial kernel API work with a new frontend workflow. Repository guidance requires separate PRs or independently reviewable commits for these concerns.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
🔍 Parallel user-search implementations
UserSearchCombobox duplicates ShareModal search state, debouncing, and keyboard behavior. Their differing fallback and dismissal semantics make future fixes likely to drift.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
🔍 Picker bypasses feature organization
The new gatekeeper-sharing workflow lives directly under src/. Frontend rules place coherent new product behavior and colocated tests under src/features/.
Was this helpful? React with 👍 or 👎 to provide feedback.
| search={search} | ||
| onValueChange={(value) => { setQuery(value); setNotice(null) }} | ||
| onSelect={pick} | ||
| onSubmit={() => {}} |
There was a problem hiding this comment.
🔴 Disabled search blocks every selection
When user search is disabled, onSubmit discards every exact identifier. searchUsers also returns no selectable results. No person can be selected on default-configured deployments.
Learn more
The picker relies exclusively on directory results. The authenticated search endpoint returns [] whenever the deployment policy disables user search, and the default admin configuration disables it. The existing workspace-sharing flow treats typed text as a direct user identifier when search is disabled, so sharing remains available without directory discovery.
Example: On a default deployment, Alice types bob@example.com. The directory returns no rows, Enter calls the empty onSubmit, and Bob is never delivered to the gatekeeper. The expected fallback submits bob@example.com directly.
Recommended fix: Read userSearchEnabled from ServerConfigContext and implement the same exact-identifier fallback as ShareModal. Prefer extracting and reusing the established user-selection behavior rather than maintaining a second search implementation.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (closing && pending.length === 0) onClose() | ||
| }, [closing, onClose, pending.length]) | ||
|
|
||
| const excludeIds = useMemo(() => [...picked.map(({ id }) => id), ...pending], [picked, pending]) |
There was a problem hiding this comment.
🟡 Large pick lists break searches
After 1,000 picks, excludeIds makes the next search exceed the directory limit. searchUsers also prepends the caller. The picker then cannot find another person.
Learn more
The directory rejects more than 1,000 distinct exclusions. The picker accumulates every successful pick, then the authenticated API adds the current user before forwarding the list. A session with 1,000 successful picks therefore sends 1,001 exclusions on its next query.
Example: Alice adds users 1 through 1,000 to a collection without closing the picker. Her next query fails with the exclusion-limit error and renders “User search is temporarily unavailable,” even though more users exist.
Recommended fix: Keep the request within the directory limit without allowing already-picked users to be selected again. This likely requires server-side eligibility or pagination state rather than truncating excludeIds, because truncation would reintroduce old picks into results.
Was this helpful? React with 👍 or 👎 to provide feedback.
This seems a bit round-about. This seems to mean gatekeeper needs to keep state server-side related to a UI flow that is ongoing on the client. E.g. the client-side UI may be a form that isn't complete yet and isn't intended to have any effect until the user clicks "sumbit". Now there needs to be a server-side DO holding the form state, so that it can be updated when How about this alternative: When the user selects a contact, some sort of token or ticket is created and returned as the result of the original |
|
@kentonv I was modeling the UX here on the current workspace sharing modal, where users are added as soon as they are selected and there is no "submit" action, you just close the modal when you're done adding people. The API you're describing makes sense, but would mean the gatekeeper user picker has different UX from the workspace user picker. If we implemented the ticket-based API I think we should also update the workspace user picker to follow the same deferred submission pattern. I don't feel strongly about either approach, but I think we should be consistent across both of these. What do you think? |
2 participants
This PR adds a generic user selection API that gatekeepers can use to support resource sharing and other features that require interacting with workshop user accounts. The initial use case for this API is supporting collection sharing in the context gatekeeper. The end to end flow for the context gatekeeper would look like this:
GatekeeperAppHost.selectUsers(collectionId). This request is delivered to the workshop frontend over the MessagePort RPC session.collectionIdis an opaque string that is passed back to the gatekeeper backend later in the process to identify the context in which a user was selected. If the Workers runtime supported RPC stub unwrapping, this string could be replaced by an opaque stub that was unwrapped by the gatekeeper backend instead.AuthenticatedApi.selectGatekeeperUser(contextGatekeeperId, selectedUserId, collectionId)which callsUserDurableObject.deliverSelectedUser(accountId, selectedUserId, collectionId)on the DO of the user who made the selection.GatekeeperUserVerifierfor the selected user and passes that to the gatekeeper backend by callingreceiveSelectedUser(collectionId, selectedUserVerifier, profile)on the gatekeeper app account responsible for the UI. The gatekeeper backend would then verify that the user can sharecollectionId, and useselectedUserVerifierto deliver the share to the selected user.profileis a read-only capability that the gatekeeper can use to display profile information about the selected user in the gatekeeper app UI.GatekeeperAppHost.selectUsers()returnsPromise<void>. This promise resolves when the user closes the user picker modal, and indicates that the gatekeeper app UI should refresh to pull in any server-side state that was updated while the modal was open.Caveats
User selection eligibility isn't checked until the user is selected from search results, so users that do not have a corresponding context gatekeeper account will still appear in search results and fail with a helpful error message when selected. This lets us re-use the same user search API used for workspace sharing. If we wanted to scope search results to just eligible users, the user directory would need to become aware of each user's connected accounts and connection state, which I'm not sure we want.