Team selector ux fix - #31
Conversation
d530df8 to
7d79a6b
Compare
gcgoncalves
left a comment
There was a problem hiding this comment.
Nice validation addition. 👏
marekdano
left a comment
There was a problem hiding this comment.
Findings
1. Editing a team-scoped MCP server silently reassigns its team (CRITICAL)
File: src/components/mcp-servers/AdvancedSettings.tsx:151
resolveTeamId(teams, selectedTeamId) is called without the record's current teamId as the explicit third argument, so opening the edit form for a team-scoped MCP server silently overwrites its team as soon as team data loads.
Failure scenario: A caller with a fresh session (selectedTeamId is null — AuthContext resets it on every login) opens the edit form for an MCP server scoped to a shared team that isn't their personal team. On mount, pickedInForm is false, so the effect runs resolveTeamId(teams, null), which falls back to the caller's personal team — differing from the server's actual teamId — and fires onTeamIdChange(personalTeamId) immediately, before the user touches anything. If they submit without noticing, the server is reassigned from the shared team to their personal team. This exact overwrite is even asserted as expected behavior by the existing test "propagates selectedTeamId change after teamId is already set (regression: was ignored by !teamId guard)" in AdvancedSettings.test.tsx.
2. Same defect in the Tools form (CRITICAL)
File: src/components/tools/ToolAdvancedSettings.tsx:93
Identical bug: resolveTeamId(teams, selectedTeamId) omits the tool's current teamId as the explicit override, so editing a team-scoped tool silently reassigns its team on mount.
Failure scenario: Same as above — opening the edit form for a tool scoped to a non-personal team, with the sidebar on its default "All teams," retargets the tool to the caller's personal team before they interact with the selector. Notably, usePromptForm.ts (same PR) avoids this exact bug by seeding chosenTeamId from initialValues.teamId and passing it as resolveTeamId's third argument — that protection was not applied to the Tools/Servers forms.
3. Team-resolution logic duplicated instead of centralized (altitude)
File: src/components/mcp-servers/AdvancedSettings.tsx:144
The pickedInForm state + useEffect calling resolveTeamId is duplicated near-verbatim in ToolAdvancedSettings.tsx, despite this PR's stated goal of centralizing the sidebar-mirroring logic that used to be copy-pasted per form.
Cost: Because this effect lives separately in both files rather than in a shared hook (e.g., alongside resolveTeamId in useTeams.ts), findings #1 and #2 had to be (and were) introduced independently twice, and a fix must likewise be applied twice.
4. Unused requiresSelection field (dead code)
File: src/hooks/useTeams.ts:13
useTeams()'s requiresSelection is computed and unit-tested but never consumed - AdvancedSettings, ToolAdvancedSettings, and usePromptForm all read only teams, and TeamSelect.tsx re-derives its own teams.length < 2 check independently.
Cost: Not a runtime bug, but a maintenance trap — a future call site could wire up requiresSelection while TeamSelect uses its own separate threshold, creating two sources of truth that can drift.
7d79a6b to
6e1efc5
Compare
Team visibility was driven entirely by the sidebar switcher, which starts on "All teams" every session. A caller with one team was told to go pick the only team they have, and the form flagged the field red the moment they chose "Team" — before they had done anything wrong. Resolve the team in the form instead: an explicit choice, else the sidebar's active team, else the caller's personal (or only) team. Callers in more than one team now pick inline via a new TeamSelect rather than being sent to the sidebar, and the requirement is raised on submit rather than on entering team visibility. The sidebar switcher stays authoritative for an open form (#5077) until the caller picks a team in the selector. Signed-off-by: Anna Effort <anna.effort@ibm.com>
Extract the sidebar-mirroring effect the servers and tools forms each had a copy of into useTeamScope(), next to resolveTeamId(). The sidebar switcher is now authoritative while *creating* only (#5077): editing pins the form to the record's own team, so opening the edit form for a record scoped to a team other than the caller's own no longer retargets it before they touch anything. usePromptForm already resolved its team this way; the servers and tools forms did not, and the duplication is why the gap had to be closed twice. - AdvancedSettings / ToolAdvancedSettings take initialTeamId, threaded from the loaded server (useMCPServerForm) and the edited tool (ToolForm). - Drop useTeams()'s unused requiresSelection; TeamSelect owns the threshold. - Drop the mcpServer.advanced.teamScoped/teamNotSelected strings, left over from the hints this branch removes. Signed-off-by: Anna Effort <anna.effort@ibm.com>
Picking a team in the form latched pickedInForm for the life of the mount, so switching visibility away from team and back left teamId empty with nothing to refill it: the latch suppressed the resolve, and a single-team caller has no selector to recover with. Clear the latch when the team is dropped, so returning to team visibility resolves afresh. Also covers the edit-mode fix end to end at the MCPServerForm level, where the sidebar default, the loaded server and the selector actually meet. Signed-off-by: Anna Effort <anna.effort@ibm.com>
useTeamScope tracked whether the caller had picked a team, not which one, so the pick had to be enforced by short-circuiting the resolve. That latch then had to be cleared by hand when the team was dropped, and clearing it lost the pick: returning to team visibility fell back to the default instead. Hold the picked id and hand it to resolveTeamId as its explicit team, the way usePromptForm already does. The pick outranks every default by priority rather than by early return, which makes the resolve idempotent — a round-trip through another visibility now restores the pick, and the latch and its manual reset both go away. Also fixes the auth-context mocks in PromptForm.test.tsx, which the rebase left without completePasswordChangeRequired and broke `tsc -b --noEmit`. Signed-off-by: Anna Effort <anna.effort@ibm.com>
310317f to
0466f62
Compare
|
Thanks @marekdano. All four are addressed. 1 and 2, editing reassigns the team. Both correct. Separating them because the severities differ. Finding 2 (tools) is a regression this PR introduced. The old effect was guarded by if (selectedTeamId && !teamId), which preserved a loaded team. That guard was dropped here. Finding 1 (servers) was flagged in Note 1 as pre-existing. That was true of the root cause but understated the impact. The old code did onTeamIdChange(selectedTeamId ?? ""), so on a fresh session it cleared teamId: isValid went false and submit was visibly disabled. The new code substitutes a valid personal-team id, so the form submits and retargets the record. Broken-and-blocked becoming silently-wrong is a change in impact, so it belonged here rather than in a follow-up. Fix for both: useTeamScope takes the record's own team and passes it as resolveTeamId's explicit argument, so the sidebar is authoritative while creating only. Threaded in from the loaded server (useMCPServerForm.initialTeamId) and the edited tool (ToolForm). usePromptForm already had the shape and served as the reference. 3, duplication. Fixed as framed: the effect now lives in useTeamScope() next to resolveTeamId, and both components call it. The framing that duplication is why the bug appeared twice held up. While consolidating, a third defect surfaced (below), and it only needed fixing once. 4, requiresSelection. Removed. TeamSelect owns the < 2 threshold. One more, found while verifying the above. The in-form pick was tracked as a boolean that latched for the life of the mount. Picking a team, switching visibility away from Team, then switching back left teamId as "" with nothing able to refill it: the latch suppressed the re-resolve, leaving a required field empty behind a disabled submit. Fixed by holding the picked id rather than a "has picked" flag, so it outranks the defaults inside resolveTeamId by priority instead of by short-circuit. That also makes the resolve idempotent, so the round-trip works without special-casing. Verification. npm test: 171 files / 2971 passed. tsc -b --noEmit and lint clean. Every edit-mode and round-trip test was run against a deliberately broken build (explicit team argument removed): 11 fail, so none pass for the wrong reason. format:check is red for an unrelated reason: #2 broadened it to root-level *.md, and README.md/DOCKER.md had never been formatted, so main is red and every open PR inherits it through the merge ref. #50 fixes that; this goes green after it lands and this branch is rebased. |
UX issue
If a user has only one team, an error message appears in the form being filled out, and the user is required to leave the workflow to find the team switcher and make a selection, even when they belong to only one team. For example:
Changes
The sidebar switcher remains authoritative for an open form (#5077) while creating, until the caller picks a team in the selector. Editing is pinned to the record's own team. See "Team resolution" below.
Root cause
Team visibility was driven entirely by the sidebar switcher, and
selectedTeamIdstarts asnull("All teams") every session (AuthContext.tsx, also reset on login). The forms never consulted the actual team list, so they could not distinguish "no team chosen" from "only one team exists."A second, independent defect:
usePromptFormre-validated in an effect whenever visibility wasteam, so the field turned red the moment the user picked "Team," before anything had gone wrong.What was added
src/hooks/useTeams.ts:useTeams(),resolveTeamId(teams, selectedTeamId, explicitTeamId), anduseTeamScope(). Resolution order: explicit choice → sidebar's active team → the caller's personal team → first team. The last two steps prevent a single-team caller from ever being asked, and give multi-team callers a reasonable default instead of an empty required field behind a disabled submit button.useTeamScope()owns the entire visibility→team effect for the forms that hold team state as ateamId/onTeamIdChangepair. It holds the caller's pick as an id rather than a "has picked" flag, and passes it toresolveTeamIdas the explicit team, so the pick outranks the defaults by priority rather than by short-circuiting the resolve.usePromptFormresolves the same way against its own derived state.src/components/common/TeamSelect.tsx: renders nothing below two teams; renders a labeled select above. It still renders an error with no selector, so a failed/teamsload explains an inert submit button rather than failing silently.Both are shared by all three forms, which previously each carried their own copy of the sidebar-mirroring logic: prompts (
usePromptForm+PromptForm), tools (ToolAdvancedSettings), servers (mcp-servers/AdvancedSettings).Team resolution
One table, since three forms depend on it:
Other changes
visibilitytoteamIdin the prompt schema, so the message lands on the selector rather than the visibility dropdown. APIteam_idfield errors now map toteamIdas well (previouslyvisibility).prompts.add.visibility.team.selectedHint,...selectFromSidebarHint, andmcpServer.advanced.teamScoped/...teamNotSelected(the last two arrived on main while this was open, i18n-ing hints this PR deletes); addedcommon.team.label,common.team.placeholder,common.required.teamErrorprop added toToolAdvancedSettingsandmcp-servers/AdvancedSettings.errors.teamIdexisted in both form hooks but was never rendered anywhere.usePromptFormnow owns team state (teamId+setTeamId) instead of deriving it read-only from the sidebar.Testing
npm testgreen: 171 files, 2971 passed / 1 skipped.tsc -b --noEmitand lint clean.New:
useTeams.test.ts(including auseTeamScopeblock),TeamSelect.test.tsx. Updated the team-visibility cases inusePromptForm.test.ts,PromptForm.test.tsx,MCPServerForm.test.tsx,AdvancedSettings.test.tsx,ToolAdvancedSettings.test.tsx.The old cases asserted the sidebar hint text and the pre-submit error, both of which are the behavior being removed.
Every edit-mode and round-trip test was checked against a deliberately broken build (dropping the explicit team argument from
resolveTeamId): 11 fail, so none of them pass for the wrong reason.Changes after review (thanks @marekdano)
useTeamScopetakes the record's own team as the explicit argument, threaded in from the loaded server (useMCPServerForm.initialTeamId) and the edited tool (ToolForm). Covered end-to-end inMCPServerForm.test.tsx(fresh session, sidebar on "All teams", server scoped to a non-personal team), plus component-level cases in both forms.useTeamScopeis the only copy.requiresSelectionremoved (finding 4).TeamSelectowns the< 2threshold.teamIdempty with nothing able to refill it. Holding the picked id instead of the flag removes the latch and the dead end together.Notes
MCPServerForm.test.tsx > team visibility > shows the selector for several teamsasserts only that the selector renders, not that it defaults to the personal team. The defaulting behavior is asserted inAdvancedSettings.test.tsxandPromptForm.test.tsx; edit-mode pinning now has its own composed test in the same file.ToolAuth.test.tsx > shows team scope hint.... The hint no longer exists, and that file has no/teamsmock, so any replacement would have passed for the wrong reason. Equivalent coverage lives inToolAdvancedSettings.test.tsx.format:checkis red here for a reason unrelated to this branch: chore: Pre-commit hooks #2 broadened it to cover root-level*.md, andREADME.md/DOCKER.mdhad never been formatted, somainitself is red and every open PR inherits it through the merge ref. chore: format root markdown with Prettier #50 fixes that; this goes green once it lands and this branch is rebased.