Conversation
The teams table and the members dialog each hardcoded bg-yellow-500. The invitations dialog needs the same square, so give it a token named for the domain rather than shipping a third copy. Value is #ffd200 from the design, not yellow-500's #eab308, so the existing squares change shade slightly. No .dark override, for the reason the other icon-bg tokens already state. Signed-off-by: Anna Effort <anna.effort@ibm.com>
Transport for the invitee's own invitations, for #5536. Two of the three routes do not exist yet and are the subject of #6010, which also leaves the inbox path undecided. Both unknowns are confined to this module, so settling them is a one-line change here. TeamInvitation re-exports the generated TeamInvitationResponse rather than restating it, so regenerating the client turns any drift between what this assumed and what the backend ships into a compile error. Signed-off-by: Anna Effort <anna.effort@ibm.com>
Its own bundle rather than keys in teams.json, since the components consuming it sit outside components/teams and a later surface elsewhere in the app should not reach into the teams bundle. The invitation sentence is one message with named placeholders so translators control word order, and the count uses ICU plural. Signed-off-by: Anna Effort <anna.effort@ibm.com>
One invitation as a sentence plus either its two actions or the outcome that replaced them. Accepted and declined are deliberately asymmetric, per node 5966:29865: accepted gets a check and full contrast, declined is muted with no icon. Roles map to display names with a fallback to the raw string, so an unrecognised role stays readable. Renders a div rather than an li, so a surface showing one invitation inline is not forced into list markup. Signed-off-by: Anna Effort <anna.effort@ibm.com>
Presentational: it takes the list and callbacks and owns no fetching. Once nothing is left to act on it holds the confirmation for 4000ms and closes itself, reusing sonner's default duration so the app has one dwell rather than two. Pointer entry cancels the timer outright rather than pausing it, since a timer that resumes on pointer exit closes at an unpredictable moment. Radix's usual pause-while-focused signal is unusable because the dialog traps focus. Scroll is scoped to the list rather than the dialog. DialogContent's own overflow puts the close button inside the scroll container, so past roughly seven invitations the X scrolls out of reach. Resolving a row unmounts the focused button, so focus moves to the next actionable row or to the close button, which is what the new data-slot on the primitive is for. Signed-off-by: Anna Effort <anna.effort@ibm.com>
Resolutions are committed after the server confirms, not optimistically: both actions are irreversible and the dialog stays open, so a rollback would visibly flip a row back. Named Data to keep it distinct from the context consumer of the same idea. An accidental second instance holds its own resolutions, which is the divergent count the provider exists to prevent, so the two should not be one import typo apart. Signed-off-by: Anna Effort <anna.effort@ibm.com>
One provider, one dialog, N triggers. Two mounted copies of a self-contained component would hold separate resolutions, so resolving in one would leave the other's count stale with nothing logged. Mounted in AppShell, which an app-wide fetch would not justify on its own. The fetch is gated on a mounted consumer instead, so a page with no trigger issues no request. That gating is also what lets this land ahead of #6010 with no feature flag: nothing calls the missing routes until a trigger is rendered. Focus return is handled here rather than by a DialogTrigger, which would force every trigger into one dialog's subtree. The element that opened the dialog is commonly gone by close, since resolving the last invitation unmounts every trigger. Signed-off-by: Anna Effort <anna.effort@ibm.com>
The Settings > Teams trigger, hidden at zero pending. Not yet rendered anywhere. Mounting it registers the consumer that starts the fetch, so the Teams page wiring waits on #6010. Takes the height and padding of the Create team button it will sit beside rather than the design's standalone 36px chip. Signed-off-by: Anna Effort <anna.effort@ibm.com>
Measured against the Figma frame: 8px padding on all four sides with a content-driven height, 4px corners, and an explicit foreground on the label, which was inheriting its colour. The icon square is 17.5px rather than the drawn 20px. At 20px the chip stands 38px tall, and the Settings tab row reserves 32px for its toolbar contents, so the drawn size pushes the whole row taller. The three outline buttons opt out of the variant's shadow-xs, which reads as an unwanted shadow in light mode. Removing it at the variant belongs to #35, so these join the dead overrides that PR already sweeps up. Signed-off-by: Anna Effort <anna.effort@ibm.com>
The step this feature could not land earlier. Mounting the chip is what registers a consumer, which is what enables the fetch, so this is the only change that calls the routes IBM/mcp-context-forge#6010 has yet to add. Everything before it merges inert. Teams reacts to an accepted invitation through a counter in context rather than a callback prop, so a later surface elsewhere in the app can react independently of whichever one rendered the trigger. e2e/invitations.spec.ts stubs the three routes through the api-mock fixture, one test per visual state. Under E2E_REAL_API=true it becomes the integration test, once e2e/seed/seed.ts can seed a pending invitation for the test user. Signed-off-by: Anna Effort <anna.effort@ibm.com>
Cut the build-time narrative: which routes did not exist yet, which alternatives were weighed, why a structure was chosen over another. That belongs in the PR and the issues, and goes stale in the code. What is left says what a reader needs to work with it: mount one hook instance, the row is not an <li>, the dwell never resumes, focus return is manual because the opener is usually gone. Signed-off-by: Anna Effort <anna.effort@ibm.com>
Corners at 12px. The sentence and the action row lose their 16px inset, so both sit flush with the header against the dialog's own padding. Roles render lowercase, matching teams.json, with the English article inside the value: "an owner", "a member". pt-BR and es-ES carry no article, since "como proprietário" is right and "como um proprietário" is not. The resolved row keeps the height of the buttons it replaces, so accepting or declining no longer shifts the invitation below it, and the accepted check uses the success token. The list no longer scrolls. scrollbar-gutter-stable goes on DialogContent, which is a scroll container through the primitive; the property does not inherit, so it needs its own. Signed-off-by: Anna Effort <anna.effort@ibm.com>
The chip renders when invitations are pending and not otherwise, and the teams list refetches when acceptedCount increments. The refetch is what puts a newly joined team in the table, and it had no coverage at all. Signed-off-by: Anna Effort <anna.effort@ibm.com>
It was debug instrumentation for the HeaderProfileMenu scroll lock, with no assertions, and it failed both lint and format:check. Signed-off-by: Anna Effort <anna.effort@ibm.com>
The dwell now cancels on pointer movement or a key press during it, rather than on pointer entry: resolving the last invitation means clicking inside the dialog, so entry alone cancelled the dwell for every mouse user. The accept button's accessible name now contains its visible label in all three locales, so voice control can address it. The load error gets a localised headline over the sanitised detail, the pattern the Teams page already uses. sanitizeError only returns English. The chip moves out of the has-teams branch, so someone invited to a first team can still reach the invitation. Signed-off-by: Anna Effort <anna.effort@ibm.com>
Expired invitations are filtered as they arrive. The inbox filters on is_active, which stays true past the expiry date, so they would otherwise be counted and offered a Join that cannot succeed. The dialog no longer stays open with nothing in it: a refetch that comes back empty closes it, and open() no-ops at a count of zero. An error keeps it open so the retry stays reachable. The sr-only region now names the row in flight and each outcome as it lands, rather than only reporting once everything is resolved. The toolbar no longer renders empty scaffolding, and the chip drops its trailing margin when it has no siblings. Signed-off-by: Anna Effort <anna.effort@ibm.com>
A role=status count of zero can never hold: HeaderQuickNav mounts a permanent announcer on every page. An open modal puts the chip in an aria-hidden subtree, so the role query has to opt into it. Outcome text now also appears in the announcer, so the row assertions are scoped to the row. Signed-off-by: Anna Effort <anna.effort@ibm.com>
There was a problem hiding this comment.
🟡 Changes recommended
Non-admin invitees cannot reach the feature, and refresh races can bypass the required confirmation dwell.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds invitee-facing pending team invitation management, including accept/decline actions, shared state, localization, accessibility, and tests.
Changes:
- Adds invitation API, state provider, count chip, and resolution dialog.
- Integrates invitations into Teams and refreshes memberships after acceptance.
- Adds localized strings, styling tokens, and unit/E2E coverage.
File summaries
| File | Description |
|---|---|
src/types/team.ts |
Re-exports invitation API type. |
src/types/invitation.ts |
Defines invitation action and resolution types. |
src/pages/Teams.tsx |
Adds invitation trigger and team refresh behavior. |
src/pages/Teams.test.tsx |
Tests Teams integration. |
src/index.css |
Adds the team icon color token. |
src/i18n/locales/pt-BR/invitations.json |
Adds Portuguese translations. |
src/i18n/locales/pt-BR/index.ts |
Registers Portuguese translations. |
src/i18n/locales/es-ES/invitations.json |
Adds Spanish translations. |
src/i18n/locales/es-ES/index.ts |
Registers Spanish translations. |
src/i18n/locales/en-US/invitations.json |
Adds English invitation messages. |
src/i18n/locales/en-US/index.ts |
Registers English messages. |
src/hooks/usePendingInvitationsData.ts |
Implements invitation queries and mutations. |
src/hooks/usePendingInvitationsData.test.ts |
Tests invitation data behavior. |
src/components/ui/dialog.tsx |
Identifies the dialog close control. |
src/components/teams/TeamsTable.tsx |
Applies the team icon token. |
src/components/teams/ManageTeamMembersDialog.tsx |
Applies the team icon token. |
src/components/layout/AppShell.tsx |
Mounts the shared invitation provider. |
src/components/invitations/PendingInvitationsProvider.tsx |
Coordinates shared invitation state and dialog. |
src/components/invitations/PendingInvitationsProvider.test.tsx |
Tests provider lifecycle and focus behavior. |
src/components/invitations/PendingInvitationsDialog.tsx |
Renders invitation states and auto-close behavior. |
src/components/invitations/PendingInvitationsDialog.test.tsx |
Tests dialog behavior and announcements. |
src/components/invitations/PendingInvitationsChip.tsx |
Adds the pending-count trigger. |
src/components/invitations/PendingInvitationsChip.test.tsx |
Tests trigger rendering and behavior. |
src/components/invitations/PendingInvitationItem.tsx |
Renders individual invitations and actions. |
src/components/invitations/PendingInvitationItem.test.tsx |
Tests invitation rows. |
src/api/invitations.ts |
Adds invitation API functions. |
src/api/invitations.test.ts |
Tests invitation API paths. |
e2e/invitations.spec.ts |
Covers invitation UI states end-to-end. |
Review details
- Files reviewed: 28/28 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A refetch answered after the user acted no longer publishes: it would drop the row they just resolved along with its confirmation. The dialog also stays open while a request is in flight, so its outcome can be shown. The sr-only summary counts what is still pending rather than the original total, which it was calling pending after rows resolved. The post-accept teams refresh catches its rejection. useQuery.execute rethrows after recording the error, so the bare call leaked an unhandled rejection; the other refresh paths in the page already catch. Signed-off-by: Anna Effort <anna.effort@ibm.com>
Settings lists the Teams tab only for is_admin and redirects everyone else off /app/settings/teams, so the badge was unreachable by the invitees it is for. The header renders on every authenticated page for every role, and the provider already wraps it. The badge itself is unchanged. The Teams toolbar goes back to what it was, and createTeamRef goes with it, since it existed only as the badge's focus fallback. The e2e suite missed this because its default user is an admin, so the new case signs in as a non-admin. Signed-off-by: Anna Effort <anna.effort@ibm.com>
Drops the box from 38px to 28px: the h-auto override is gone, so size="sm" applies, with 6px of vertical padding and a 16px icon square to match. The half-pixel sizes go with it. Team icon chips move to the brand cyan, which --server-icon-bg already carries. That recolours the Teams table and the manage-members dialog too, both of which moved onto this token earlier in the branch. The count names the entity, since the badge is now app-wide rather than on the Teams page. Signed-off-by: Anna Effort <anna.effort@ibm.com>
marekdano
left a comment
marekdano
left a comment
There was a problem hiding this comment.
Findings
🔴 High
1. Focus refetch can clobber accept confirmation UX
File: src/hooks/usePendingInvitationsData.ts:78
A background staleness refetch (tab focus/visibility) that lands after a resolution already committed is not treated as "acted since," so it silently overwrites the invitations list and drops the resolved row's confirmation entry, closing the dialog before the dwell/confirmation UX the PR explicitly designed can show.
Failure scenario: Dialog is open for >60s (past STALE_AFTER_MS) before the user accepts; the accept commits a resolution. If the tab loses and regains focus within the 4s auto-close dwell, refreshIfStale() fires load(); actedSince is false because resolvedBefore already contained the id, so the fresh server list (which no longer includes the accepted invitation) replaces state and the resolutions filter drops the "accepted" entry too. PendingInvitationsProvider's invitations.length === 0 effect then closes the dialog immediately, skipping the "Invite accepted" confirmation.
2. resolve() lacks a reentrancy guard
File: src/hooks/usePendingInvitationsData.ts:128
resolve() has no reentrancy guard, so it can be invoked twice for the same invitation before React commits the disabled state from the first setInFlight call.
Failure scenario: A fast double-click or a held Enter key on "Join team"/"Decline" fires onAccept/onDecline twice before the button's disabled attribute re-renders. Both calls run resolve(), issuing two concurrent POST /teams/invitations/{token}/accept (or decline) requests for one token; the second likely 400s, producing a confusing "Failed to join team" toast even though the first request already succeeded.
3. Teams.tsx refetch effect re-fires on every remount
File: src/pages/Teams.tsx:60
The acceptedCount-triggered refetch effect has no "seen" tracking, so it fires on every fresh mount of Teams.tsx once any invitation has ever been accepted this session, not only right after a new acceptance.
Failure scenario: User accepts one invitation anywhere in the app (acceptedCount becomes 1, never resets since PendingInvitationsProvider lives at
the AppShell level for the whole session). Every subsequent navigation to Settings > Teams remounts this effect; since React runs effects on mount regardless of dependency history, it re-fires refetch() on top of useQuery's own initial fetch, causing a redundant /teams request and a visible spinner flash on every visit for the rest of the session.
4. Focus restoration can target a disabled button
File: src/components/invitations/PendingInvitationsDialog.tsx:151
The focus-restoration effect moves focus to the next pending invitation's Accept button without checking whether that row itself has a request in flight (and thus disabled), and .focus() on a disabled button is a no-op.
Failure scenario: Two invitations are open at once (accept on A, decline on B in flight together — a supported state per the dialog's own tests). A resolves first; the effect picks pending[0] = B and calls its Accept button's .focus(), but B's buttons are disabled because its own decline is still in flight, so focus silently fails to move and falls back to <body>, dropping keyboard/screen-reader focus.
5. Sidebar team list stale after accepting an invite
File: src/components/layout/TeamSwitcher.tsx:25
The sidebar team switcher fetches its own team list via useQuery("/teams") and never consumes usePendingInvitations()/acceptedCount, unlike src/pages/Teams.tsx which explicitly refetches when an invitation is accepted.
Failure scenario: A user accepts a team invitation from the header chip on any page; PendingInvitationsProvider's acceptedCount increments, but the sidebar TeamSwitcher (mounted app-wide) keeps showing the pre-accept team list until a full page reload, even though Teams.tsx would show the new team correctly.
6. Team icon color now identical to server icon color
File: src/index.css:164
The new --team-icon-bg token (#70f9ff) is set to the exact same hex value as the pre-existing --server-icon-bg, so the team and server icon chips — previously distinguishable (team was yellow-500) — are now visually identical.
Failure scenario: Anywhere team and server icon chips appear together or are switched between in the same session (e.g. Teams vs. Servers pages, or search/dashboard views), users lose the color cue that used to distinguish the two entity types at a glance.
🟡 Medium
7. TeamForm missed the icon color token migration
File: src/components/teams/TeamForm.tsx:83
The team create/edit form's icon swatch still hardcodes bg-yellow-400, while this same PR migrated TeamsTable.tsx and ManageTeamMembersDialog.tsx to the new bg-team-icon-bg token.
Failure scenario: A user views the Teams table (new cyan icon), clicks Edit on a team, and the edit form's header shows the old yellow icon — a visible color inconsistency within one navigation flow that this PR's own color migration should have caught.
8. Shared Dialog primitive coupled to one caller's DOM scraping
File: src/components/ui/dialog.tsx:46
data-slot="dialog-close" was added to the shared Dialog primitive solely so PendingInvitationsDialog can querySelector for it to move focus onto the close button, coupling shared UI markup to one feature's DOM-scraping need instead of exposing a ref/callback API.
Failure scenario: If DialogContent's internal structure changes later (close button wrapped, moved, or Radix's Close renders differently), the querySelector in PendingInvitationsDialog.tsx silently stops matching and focus falls through to nothing/body on the "last invitation resolved" path — a regression invisible at the dialog.tsx call site since nothing types this contract.
🟢 Low
9. Refetch-on-focus logic duplicated across 3 hooks
File: src/hooks/usePendingInvitationsData.ts:108
The pause-while-hidden / refetch-on-focus-when-stale pattern (visibilitychange + focus listeners gated on a staleness timestamp) reimplements logic already present near-identically in src/hooks/useMcpServers.ts and src/hooks/useSystemHealth.ts, which both wire the same visibilitychange listener.
Failure scenario: Three independent copies of the same staleness/refetch-on-focus wiring now have to be kept in sync by hand; a fix to one (e.g. a Safari bfcache edge case) won't propagate to the others since none share a common useRefetchOnFocus/usePolling hook.
10. open() silently no-ops with no feedback
File: src/components/invitations/PendingInvitationsProvider.tsx:73
open() silently no-ops when pendingCount is 0, giving no feedback if a trigger fires in the brief window after the count has dropped to 0 but before the calling element has unmounted.
Failure scenario: The last invitation resolves, count reaches 0, and the chip is scheduled to unmount; if a queued interaction (e.g. a same-tick Enter keypress) still calls open() before unmount completes, nothing happens and no dialog appears — unlike every other path in this file, which explains its behavior, this one fails silently with no user-visible feedback.
Pause the staleness refresh while the dialog is open, so a list landing behind it cannot drop a committed resolution and its confirmation. Skip busy rows when moving focus after a row resolves: their buttons are disabled, so focusing one leaves focus on the body. Seed the post-accept team refetch on mount, in Teams and now also in TeamSwitcher, which was never refreshed at all. Move the TeamForm icon onto the team token the rest of the page uses. Signed-off-by: Anna Effort <anna.effort@ibm.com>
|
Thanks for the review @marekdano! Details on what's included/not in 9074075:
Still blocked on IBM/mcp-context-forge#6010. |
marekdano
left a comment
marekdano
left a comment
There was a problem hiding this comment.
Findings
🔴 High
1. isLoading can get stuck true forever
File: src/hooks/usePendingInvitationsData.ts:96
If the last consumer unmounts while a fetch is still in flight, the mount effect's cleanup bumps latestRequestId.current. When the response lands, both the early-return check (line 77) and the finally guard (line 96) see a stale requestId and skip setIsLoading(false). Since enabled=false prevents any future load() from running, isLoading stays true indefinitely.
Failure scenario: All consumers of usePendingInvitations() unregister (e.g. consumerCount drops to 0 on some route) while listMyInvitations() is still pending.
2. Aria-live can drop an announcement (possible regression)
File: src/components/invitations/PendingInvitationsDialog.tsx:92
setLastResolvedIds(resolved) overwrites instead of accumulates. This is downstream of the 5922ae1 fix for Copilot's "only first in-flight entry announced" comment — the mechanism changed (now lists every in-flight/resolved-since-last-announcement row), but a narrower version of the same class of bug may still be possible.
Failure scenario: Invitation A is accepted, updating the live region. Before a screen reader finishes reading it, invitation B is declined in a separate async completion (not batched with A's). The effect diffs only against the immediately-prior snapshot, so lastResolvedIds becomes [B] only - A's announcement disappears.
🟡 Medium
3. Teams.tsx staleness fix is narrow in scope
File: src/pages/Teams.tsx:61
The new acceptedCount signal only refreshes the team list on invite-accept. TeamSwitcher's sidebar list still won't reflect team create/delete/edit actions taken on the Teams page — a narrower fix than the underlying staleness problem.
🟢 Low
4. pending array not memoized
File: src/components/invitations/PendingInvitationsDialog.tsx:60
pending is recomputed with .filter() on every render and is a dependency of the focus-restoration effect, so that effect re-runs on every render instead of only when resolvedCount changes. No visible bug — just wasted work on a hot path during the dialog's lifetime.
Signed-off-by: Anna Effort <anna.effort@ibm.com>
|
Thanks for the additional review round @marekdano. 🙌 Fixed 1, 2 and 4 in abee073. No changes added for feedback item 3. Details:
Still blocked on IBM/mcp-context-forge#6010. @jonpspri |
marekdano
left a comment
marekdano
left a comment
There was a problem hiding this comment.
Not blocking, but good to fix
🟡 Medium — Stale invitation stays actionable while dialog is open
File: src/hooks/usePendingInvitationsData.ts:113
The staleness refresh (focus/visibilitychange) pauses while the invitations dialog is open. If an invitation's expires_at passes while the dialog sits open without the tab losing focus, the row still renders with an enabled "Join team" button. Clicking it calls acceptInvitation(), which the server rejects with a 400 — the exact failure the PR's expiry filtering was meant to prevent, just reached via a different path.
🟢 Low — Pending-count badge can go stale indefinitely
File: src/hooks/usePendingInvitationsData.ts:113
In a tab that's never blurred, nothing re-triggers a load() (only mount, focus/visibilitychange, or opening the dialog do). The header badge can keep showing an already-expired invitation as pending indefinitely. Display-only — no incorrect action results.
The expiry sweep runs while the dialog is closed, so a tab that is never blurred stops reporting lapsed invitations as pending. It refilters state rather than refetching. Accept rechecks expiry at the click, covering the open dialog where the sweep is paused. Decline stays unguarded. Signed-off-by: Anna Effort <anna.effort@ibm.com>
|
@marekdano, I confirmed the non-blocking feedback from your third review round: Feedback marked as low priority: The interval does not run while paused. Removing a row from the open dialog would unmount a focused button, discard the confirmation for an accept still in flight, and empty the list, which triggers the auto-close at Feedback marked as medium priority: Decline is deliberately not guarded. Declining an expired invitation should succeed, since it is the only way the invitee can clear the row, and I have proposed that behaviour for the decline route on IBM/mcp-context-forge#6010. Both endpoints this PR calls are still unimplemented on backend main. Four tests added, one of which asserts the interval does not run while paused. |
marekdano
left a comment
marekdano
left a comment
There was a problem hiding this comment.
🔴 High — expiry-click load() bypasses the paused guard and can clobber a sibling's accept confirmation
File: src/hooks/usePendingInvitationsData.ts:149-154
resolve()'s new early-return branch calls void load() unconditionally when accept is clicked on an expired invitation. Every other load() trigger in this file is gated by paused (the interval sweep at line 126, and refreshIfStale at line 102 both live inside if (!enabled || paused) return;) specifically because, as the author's own PR comment states: "Removing a row from the open dialog would... discard the confirmation for an accept still in flight, and empty the list, which triggers the auto-close at PendingInvitationsProvider.tsx:89." This new call sidesteps that guard by design ("covering the open dialog where the sweep is paused"), but load()'s actedSince check (line 79-83) only protects against a resolution that lands during the fetch — not one already committed and still on-screen dwelling before the fetch starts.
Failure scenario: Dialog is open with invitation A (valid) and B (about to expire). User accepts A — resolutions = {A: "accepted"}, A's row shows its confirmation check, dialog stays open because B is still pending. B then lapses while the dialog is open (sweep paused, row correctly stays visible per the new fix) and the user clicks "Join team" on it. isActionable(B) is false, so resolve() shows the toast and calls load(). Since nothing else is in-flight and A's resolution predates this fetch, actedSince is false, so the fresh server list (which the code's own "drop resolutions for invitations no longer published" comment at line 88-91 shows is expected to omit an already-accepted invitation) overwrites invitations, dropping A. PendingInvitationsProvider's invitations.length === 0 effect then closes the dialog immediately, cutting off A's confirmation before its 4s dwell — the same bug as review-round-1, reached through a new path.
None of the four new tests cover this: each exercises the expired invitation in isolation, with no prior resolution on a sibling invitation still displayed.
Suggested fix: skip the load() call (or skip applying its result) while paused/the dialog is open — the toast alone is enough feedback for the click; let the existing pause-aware sweep or the next open-triggered refetch() clear the row once it's safe to.
Accept on a lapsed row showed the toast and refetched. The server drops an invitation already accepted in the same dialog, so the fresh list took that row and its confirmation with it, emptying the dialog and triggering the auto-close. The toast alone is the response now. Signed-off-by: Anna Effort <anna.effort@ibm.com>
|
Thanks for the additional feedback, @marekdano ! To help keep track of prior rounds of feedback and fixes:
-> Fix applied: remove the call. The toast is the whole response now, and the row clears on the sweep or the next refetch on open. Gating The row stays on screen and still looks actionable until then, and a second click just re-toasts. Disabling it live would mutate the open dialog. Tests: the existing lapsed-accept case now asserts that no request is issued, and a new case covers the sibling scenario. Both tests fail against the old hook. |
There was a problem hiding this comment.
🟡 Changes recommended
The retry path can bypass the confirmation dwell, and in-flight controls have accessible names inconsistent with their visible labels.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
src/components/invitations/PendingInvitationItem.tsx:101
- When an accept is in flight, the visible label changes to “Joining...” (and its localized equivalents), but this unchanged
aria-labelremains “Join team...”. The visible text is therefore absent from the control's accessible name, contrary to the label-in-name requirement this UI is intended to satisfy. Let the busy-state text provide the accessible name; the live region already announces the team.
aria-label={intl.formatMessage(
{ id: "invitations.action.accept.aria" },
{ team: invitation.team_name },
)}
- Files reviewed: 33/33 changed files
- Comments generated: 2
- Review effort level: Balanced
Signed-off-by: Anna Effort <anna.effort@ibm.com>
marekdano
left a comment
marekdano
left a comment
There was a problem hiding this comment.
Happy with the changes!
The PR can be merged
- after implementation of GET /users/me/invitations and the decline route from IBM/mcp-context-forge#6010
- after the current conflicts are resolved
Signed-off-by: Anna Effort <anna.effort@ibm.com>
Summary
Adds the invitee-facing UI for IBM/mcp-context-forge#5536. A count badge in the app header opens a dialog listing every pending invitation. Accepting decrements the count behind the dialog. Once nothing is left to act on, the dialog holds the confirmation for 4 seconds and closes itself.
V2-updatedSept14.mp4
PendingInvitationsProvider mounts in AppShell and owns the data; triggers are consumers of usePendingInvitations(). The fetch is gated on a mounted consumer, so a surface with no trigger issues no request.
The header renders inside AppShell inside AuthGuard on every authenticated page for every role, and the provider already wraps it, so this needs no new route and no change to the admin gate.
Blocked on IBM/mcp-context-forge#6010 and not mergeable until it lands: 2 of 3 routes do not exist yet: nothing lists the invitations addressed to the caller, and there is no decline route. The only DELETE on an invitation is cancel_team_invitation, which is gated on teams.manage_members and owner-or-inviter, so it is revoke rather than decline. Without the list there is no token, so even the working accept route is unreachable.
Resolutions are committed after the server confirms rather than optimistically, since both actions are irreversible and the dialog stays open. The inviter renders as an email address, because TeamInvitationResponse carries no name field.
The dwell that closes the dialog is cancelled by pointer movement or a key press while it is running, not by pointer entry. Entry cannot be the signal: resolving the last invitation means clicking inside the dialog, so entry would cancel the dwell for every mouse user and leave the self-close reachable only by keyboard. Arming clears any cancel from before the confirmation appeared, so the click that produced it does not count against it.
Expired invitations are filtered out as they arrive. The inbox filters on is_active, which stays true past the expiry date, so an expired invitation would otherwise be counted by the chip and offered a Join that returns 400. The dialog also never shows with nothing in it: a refetch that comes back empty closes it, and open() no-ops at a count of zero. A failed refetch keeps it open, since that is the only path on which the error and retry are reachable.
The accept button's accessible name contains its visible label in all three locales, so voice control can address it. The load error carries a localised headline over the sanitised detail, matching the Teams page, because sanitizeError only ever returns English. The sr-only region names the row in flight and each outcome as it lands, rather than only reporting once everything is resolved.
Also updated the Teams color and used it for the badge as the previous yellow option might have read as a warning.
Follow-up, not in scope: IBM/mcp-context-forge#6796 covers the untranslated role picker.