fix(share): align user identity and display with the v1beta3 user construct by hortison · Pull Request #1715 · layer5io/sistent

hortison · 2026-07-15T22:41:21Z

Symptom

The Share Design people-picker shows no email address and no first/last name while searching for and selecting existing users (Layer5 Cloud, Meshery, and Kanvas all embed these components).

Root cause

The components hard-coded a pre-cutover user shape. The v1beta3 user construct in meshery/schemas retired the wire userId (canonical identifier is id), and meshery-cloud's hardened user endpoints serve reduced projections that can omit names/email. Concretely:

Option rows rendered firstName lastName / email directly - blank when a projection omits them, with no username fallback.
isOptionEqualToValue compared emails; with emails absent, undefined === undefined made every option "equal" once one user was selected, emptying the suggestion list.
grant_access payloads sent actor_id: user.userId - undefined for canonical v1beta3 records.
The ownership check in utils/permissions.ts compared userId on both sides, so two canonical records never matched (and two records missing the field always matched).

Fix

New src/utils/user.ts is the single place that absorbs wire shapes, aligned with the v1beta3 user construct:

getUserIdentifier - id with deprecated userId fallback
getUserDisplayName - firstName lastName -> username -> email
getUserContactLabel - email -> username
isSameUser - identifier match with email fallback, never matches empty records

UserShareSearch, ShareModal, UserSearchFieldInput, and the permissions ownership check now route through these helpers. Revocation passes the actor record instead of round-tripping through email.

Out-of-scope fixes included

Removed leftover debug console.logs in ShareModal/UserShareSearch (including personal-name debug tags).
User-facing message typos: "revokke", double spaces in revoke notifications.

Watch for

UsersTable, CollaboratorAvatarGroup, and catalog tables still read userId from other constructs (org-users projection, Kanvas awareness state, design-owner FK). Those wires still carry userId today and are unaffected by this defect, but will need the same treatment if/when their contracts move to v1beta3 identity.

Coordinated changes

feat(user): add PublicUser/SearchableUser projections and search paths to v1beta3 meshery/schemas#1060 adds the canonical SearchableUser projection + search operations these components consume.
Follow-up PRs point meshery-cloud's share search at the authenticated searchable projection and bump sistent in meshery and meshery-extensions.

Test plan

npx jest - 369/369 pass, including new userIdentity.test.ts (helper matrix across v1beta3/legacy/public shapes) and UserShareSearch.test.tsx (name+email render, username fallback, id-based selection/grant, access-list dedup)
npm run build (tsup + DTS)
eslint on touched paths

…struct Symptom: the Share Design people-picker shows no email address and no first/last name while searching for and selecting existing users. Root cause: the components hard-coded a pre-cutover user shape. The v1beta3 user construct in meshery/schemas retired the wire userId (the canonical identifier is id) and the hardened cloud endpoints serve reduced projections that may omit names or email, so records keyed and rendered via userId/firstName/lastName/email came up empty: options rendered blank, isOptionEqualToValue compared undefined emails (making every option 'equal' after one selection), and grant_access payloads carried an undefined actor_id. Fix: introduce utils/user.ts as the single place that absorbs wire shapes - getUserIdentifier (id with deprecated userId fallback), getUserDisplayName (first/last -> username -> email), getUserContactLabel (email -> username), and isSameUser (identifier match with email fallback) - and route UserShareSearch, ShareModal, UserSearchFieldInput, and the permissions ownership check through it. Revocation now passes the actor record instead of round-tripping through email. Also swept while here: debug console.log leftovers in ShareModal and UserShareSearch, and user-facing message typos (revokke, stray spacing). Watch for: UsersTable, CollaboratorAvatarGroup, and the catalog tables still read userId from other constructs (org-users projection, Kanvas awareness state, design-owner FK); those wires still carry userId and are unaffected by this defect. Signed-off-by: jamieplu <179417684+jamieplu@users.noreply.github.com>

hortison · 2026-07-15T22:42:51Z

/gemini review

Copilot

Pull request overview

This PR updates Sistent’s share/people-picker and related permission checks to align user identity and display logic with the v1beta3 user construct (canonical id, deprecated userId fallback), including safer display fallbacks for reduced user projections.

Changes:

Introduces centralized user identity/display helpers (getUserIdentifier, getUserDisplayName, getUserContactLabel, isSameUser) and routes sharing/selection logic through them.
Updates ShareModal / UserShareSearch / UserSearchFieldInput to use id-based identity matching and username/email/name fallbacks, removing debug logs and fixing user-facing revoke messages.
Adds regression tests covering v1beta3/legacy/public user shapes and id-based selection/dedup behavior.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
src/utils/user.ts	Adds v1beta3-aligned user type plus identity/display helper utilities.
src/utils/permissions.ts	Switches ownership checks to canonical identifier comparison via helpers.
src/utils/index.ts	Re-exports the new user utilities through the utils barrel.
src/custom/UserSearchField/UserSearchFieldInput.tsx	Updates user search field selection, equality, and rendering to use helper fallbacks.
src/custom/UserSearchField/UserSearchField.tsx	Updates share-search Autocomplete identity logic and option rendering with helper fallbacks.
src/custom/ShareModal/ShareModal.tsx	Updates grant/revoke payload identity sourcing and access list rendering to v1beta3 semantics.
src/testing/UserShareSearch.test.tsx	Adds regression coverage for v1beta3 display, fallback, selection, and dedup.
src/testing/UserSearchFieldInput.test.tsx	Adds regression coverage for v1beta3 id/userId identity matching in the user picker.
src/testing/userIdentity.test.ts	Adds a helper-matrix unit test suite for identity/display fallbacks.

hortison · 2026-07-15T22:48:31Z

+          const displayName = getUserDisplayName(option);
+          const contactLabel = getUserContactLabel(option);
+          return (
+            <li {...props} id={getUserIdentifier(option)}>


Fixed in 66e354f - the id override is removed and MUI's renderOption props are spread untouched, so the generated option id keeps backing aria-activedescendant. Added a test asserting the highlighted option resolves from the input's aria-activedescendant. Note this predates the PR (it previously overrode with option.userId), but this was the right place to fix it.

hortison · 2026-07-15T22:48:32Z

  const handleShareWithNewUsers = async (newUsers: User[]) => {
    const grantAccessList = newUsers.map((user) => ({
-      actor_id: user.userId,
+      actor_id: getUserIdentifier(user),
      actor_type: 'user'
    }));


Fixed in 66e354f - handleShareWithNewUsers now fails fast with an error notification and returns { error } when any selected record lacks an identifier, so no grant_access entry is ever built with an empty actor_id.

hortison · 2026-07-15T22:48:33Z

  const handleRevokeAccess = async (revokedUsers: User[]) => {
    const revokeAccessList = revokedUsers.map((user) => ({
-      actor_id: user.id,
+      actor_id: getUserIdentifier(user),
      actor_type: 'user'
    }));


Fixed in 66e354f - handleRevokeAccess has the same fail-fast guard: an error notification plus early return instead of issuing a revoke_access payload with an empty actor_id.

gemini-code-assist

Code Review

This pull request aligns user-related components and utilities with the v1beta3 user construct, transitioning from the deprecated userId to the canonical id and introducing helper functions to handle reduced user projections. The review feedback highlights two main improvements: optimizing performance in UserSearchField by pre-computing a concatenated array outside of a filter loop, and ensuring robust compatibility in UserSearchFieldInput by checking both isSoftDeleted(option.deletedAt) and option.deleted to correctly identify soft-deleted users.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

gemini-code-assist · 2026-07-15T22:44:21Z

  const filteredOptions = suggestions.filter(
-    (option: User) => !usersToShareWith.concat(usersData).find((u) => u.email === option.email)
+    (option: User) => !usersToShareWith.concat(usersData).find((u) => isSameUser(u, option))
  );


The array concatenation usersToShareWith.concat(usersData) is evaluated on every single iteration of suggestions.filter. Pre-computing this combined array once outside the filter loop avoids redundant array allocations and improves performance.

Suggested change

const filteredOptions = suggestions.filter(

(option: User) => !usersToShareWith.concat(usersData).find((u) => u.email === option.email)

(option: User) => !usersToShareWith.concat(usersData).find((u) => isSameUser(u, option))

);

const combinedUsers = usersToShareWith.concat(usersData);

const filteredOptions = suggestions.filter(

(option: User) => !combinedUsers.find((u) => isSameUser(u, option))

);

Done in 66e354f - the combined array is hoisted out of the filter and the lookup now uses .some() instead of .find() for the boolean test.

gemini-code-assist · 2026-07-15T22:44:22Z

+                    </Box>
+                  </Grid2>
+                  <Grid2 size="grow">
+                    {option.deleted ? (


The check for deleted users in renderOption uses option.deleted. However, the new v1beta3 User construct uses deletedAt?: DeletedAt and does not carry a deleted boolean property. This will cause soft-deleted v1beta3 users to not be rendered as deleted in the search options, even though they will be blocked when trying to add them. We should check both isSoftDeleted(option.deletedAt) and option.deleted for robust compatibility.

Suggested change

{option.deleted ? (

{isSoftDeleted(option.deletedAt) || option.deleted ? (

Fixed in 66e354f - the option now renders as deleted when either the component-local deleted flag or isSoftDeleted(option.deletedAt) is set, matching the guard handleAdd already applies. Added a regression test rendering a deletedAt-timestamp record as '(deleted)'.

gemini-code-assist

Code Review

This pull request aligns the user search and sharing components with the v1beta3 user construct by introducing a new set of utility functions in src/utils/user.ts to handle user identity, display names, and contact labels across full and reduced user projections. It refactors ShareModal, UserSearchField, and UserSearchFieldInput to use these helpers, cleans up unused console logs, and adds robust unit and integration tests. The review feedback suggests minor optimizations, such as extracting a repeated array concatenation outside of a filter loop in UserSearchField.tsx to improve performance, and using .charAt(0) instead of bracket notation in UserSearchFieldInput.tsx for safer string access.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

gemini-code-assist · 2026-07-15T22:46:02Z

  const filteredOptions = suggestions.filter(
-    (option: User) => !usersToShareWith.concat(usersData).find((u) => u.email === option.email)
+    (option: User) => !usersToShareWith.concat(usersData).find((u) => isSameUser(u, option))
  );


The expression usersToShareWith.concat(usersData) is evaluated on every single iteration of suggestions.filter. Since suggestions can contain multiple items, computing this concatenation repeatedly is inefficient. Extracting the concatenated array to a variable outside of the filter improves performance.

Suggested change

const filteredOptions = suggestions.filter(

(option: User) => !usersToShareWith.concat(usersData).find((u) => u.email === option.email)

(option: User) => !usersToShareWith.concat(usersData).find((u) => isSameUser(u, option))

);

const excludedUsers = usersToShareWith.concat(usersData);

const filteredOptions = suggestions.filter(

(option: User) => !excludedUsers.find((u) => isSameUser(u, option))

);

Already addressed in 66e354f - the combined array is hoisted to alreadySelectedUsers outside the filter and the lookup uses .some() instead of .find().

gemini-code-assist · 2026-07-15T22:46:02Z

-              <Avatar alt={localUsersData[0].firstName} src={localUsersData[0].avatarUrl}>
-                {!localUsersData[0].avatarUrl && localUsersData[0].firstName?.[0]}
+              <Avatar alt={getUserDisplayName(localUsersData[0])} src={localUsersData[0].avatarUrl}>
+                {!localUsersData[0].avatarUrl && getUserDisplayName(localUsersData[0])[0]}


Using .charAt(0) is safer than bracket notation [0] because it returns an empty string "" instead of undefined if getUserDisplayName returns an empty string. This also maintains consistency with the implementation in UserSearchField.tsx.

Suggested change

{!localUsersData[0].avatarUrl && getUserDisplayName(localUsersData[0])[0]}

{!localUsersData[0].avatarUrl && getUserDisplayName(localUsersData[0]).charAt(0)}

Fixed in ebb869e - switched to charAt(0) for consistency with the pattern in UserSearchField.tsx.

gemini-code-assist · 2026-07-15T22:46:02Z

-                <Avatar alt={user.firstName} src={user.avatarUrl}>
-                  {!user.avatarUrl && user.firstName?.[0]}
+                <Avatar alt={getUserDisplayName(user)} src={user.avatarUrl}>
+                  {!user.avatarUrl && getUserDisplayName(user)[0]}


Using .charAt(0) is safer than bracket notation [0] because it returns an empty string "" instead of undefined if getUserDisplayName returns an empty string. This also maintains consistency with the implementation in UserSearchField.tsx.

Suggested change

{!user.avatarUrl && getUserDisplayName(user)[0]}

{!user.avatarUrl && getUserDisplayName(user).charAt(0)}

Fixed in ebb869e - same charAt(0) change applied to the expanded chip list.

…nd payload guards - Render soft-deleted v1beta3 records (deletedAt) as deleted in the UserSearchFieldInput options, not just the component-local deleted flag - Stop overriding the MUI Autocomplete option id so aria-activedescendant keeps resolving to the highlighted option - Fail fast in ShareModal share/revoke handlers when a user record lacks an identifier instead of sending an empty actor_id - Hoist the selected-users concat out of the suggestion filter loop Signed-off-by: jamieplu <179417684+jamieplu@users.noreply.github.com>

hortison · 2026-07-15T22:48:41Z

/gemini review

gemini-code-assist

Code Review

This pull request refactors user identity and display logic across the application to support the v1beta3 user construct, introducing helper utilities in src/utils/user.ts to handle canonical identifiers and reduced user projections. It updates components like ShareModal, UserSearchField, and UserSearchFieldInput to use these helpers, and adds corresponding unit and integration tests. The review feedback suggests improving runtime safety by guarding against potentially undefined properties (such as usersData and currentUser.roleNames) and ensuring robust UI fallbacks by using getUserIdentifier when both contact labels and display names are absent.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

gemini-code-assist · 2026-07-15T22:50:45Z

    }
  };

+  const alreadySelectedUsers = usersToShareWith.concat(usersData);


Guard usersData with a nullish coalescing operator (?? []) before calling .concat() to prevent potential runtime errors if usersData is null or undefined.

Suggested change

const alreadySelectedUsers = usersToShareWith.concat(usersData);

const alreadySelectedUsers = usersToShareWith.concat(usersData ?? []);

References

Always guard object parameters with a nullish coalescing operator (e.g., filters ?? {}) before calling methods like Object.entries to prevent runtime TypeError when the argument is null or undefined.

Fixed in ebb869e - usersData is now guarded with ?? [] at the published-library boundary, mirroring the nullish-currentUser tolerance in utils/permissions.ts.

gemini-code-assist · 2026-07-15T22:50:45Z

        </Avatar>
      }
-      label={avatarObj.email}
+      label={getUserContactLabel(avatarObj) || getUserDisplayName(avatarObj)}


Fall back to getUserIdentifier(avatarObj) if both getUserContactLabel(avatarObj) and getUserDisplayName(avatarObj) are empty. This ensures the chip always renders a non-empty label even for minimal user projections that only contain an identifier.

Suggested change

label={getUserContactLabel(avatarObj) || getUserDisplayName(avatarObj)}

label={getUserContactLabel(avatarObj) || getUserDisplayName(avatarObj) || getUserIdentifier(avatarObj)}

Addressed in b528110 - the chip label now uses getUserLabel, which consolidates the contact-label, display-name, identifier fallback chain in one helper.

gemini-code-assist · 2026-07-15T22:50:46Z

          loading={searchUserLoading}
          value={usersToShareWith}
-          getOptionLabel={(user) => user.email}
+          getOptionLabel={(user) => getUserContactLabel(user) || getUserDisplayName(user)}


Fall back to getUserIdentifier(user) if both getUserContactLabel(user) and getUserDisplayName(user) are empty to ensure the option label is never an empty string.

Suggested change

getOptionLabel={(user) => getUserContactLabel(user) || getUserDisplayName(user)}

getOptionLabel={(user) => getUserContactLabel(user) || getUserDisplayName(user) || getUserIdentifier(user)}

Addressed in b528110 - getOptionLabel now uses getUserLabel, whose fallback chain ends at the raw identifier.

gemini-code-assist · 2026-07-15T22:50:46Z

      actor_type: 'user'
    }));
-    const emails = newUsers.map((u) => u.email);
+    const recipients = newUsers.map((u) => getUserContactLabel(u) || getUserDisplayName(u));


Fall back to getUserIdentifier(u) if both getUserContactLabel(u) and getUserDisplayName(u) are empty. Since we already validated that all users in newUsers have a valid identifier, this guarantees a non-empty string for the notification message.

Suggested change

const recipients = newUsers.map((u) => getUserContactLabel(u) || getUserDisplayName(u));

const recipients = newUsers.map((u) => getUserContactLabel(u) || getUserDisplayName(u) || getUserIdentifier(u));

Addressed in b528110 - recipients are built with newUsers.map(getUserLabel), which is non-empty for any record that passed the share guard.

…ate nullish currentUser getUserLabel collapses the repeated 'contact label, else display name' chains in chips, option labels, deleted-row text, and share/revoke notification recipients, with a final fallback to the raw identifier so a record that carries only an id never renders an empty label. The permissions ownership check now tolerates a nullish currentUser during auth-state transitions in untyped JS consumers. Signed-off-by: jamieplu <179417684+jamieplu@users.noreply.github.com>

…Data Align the collapsed and expanded chip avatar initials with the charAt(0) pattern already used in UserSearchField, and guard the usersData prop with ?? [] at the published-library boundary, mirroring the nullish-currentUser tolerance in utils/permissions.ts. Signed-off-by: jamieplu <179417684+jamieplu@users.noreply.github.com>

hortison · 2026-07-16T03:23:02Z

/gemini review

gemini-code-assist

Code Review

This pull request updates user identity and display logic to support the v1beta3 user construct, replacing the deprecated userId with id and introducing fallback utilities in src/utils/user.ts to handle reduced user projections. It refactors components like ShareModal and UserSearchField to use these helpers and adds comprehensive tests. The review feedback highlights a critical bug in UserSearchFieldInput where deleting a user with an empty identifier could remove multiple users, suggesting a refactor of handleDelete to use isSameUser. It also recommends updating the TypeScript signature of canUpdateResource to accept nullish values to match its defensive implementation.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

gemini-code-assist · 2026-07-16T03:25:14Z

    (idToDelete: string, event: React.MouseEvent) => {
      event.stopPropagation();

-      const updatedUsers = localUsersData.filter((user) => user.userId !== idToDelete);
+      const updatedUsers = localUsersData.filter((user) => getUserIdentifier(user) !== idToDelete);
      setLocalUsersData(updatedUsers);
      setUsersData(updatedUsers);


When deleting a user from localUsersData, the code currently filters by comparing the user's identifier with idToDelete (which is a string). However, if multiple users do not have an identifier (e.g., they only have an email address, which is common for invited or legacy users), getUserIdentifier will return an empty string "" for all of them. Consequently, deleting one user with an empty identifier will inadvertently delete all other users with empty identifiers from the list.

To prevent this, we should pass the entire User object to handleDelete and use the robust isSameUser helper to filter the list. This correctly falls back to email comparison when identifiers are missing, ensuring only the intended user is removed.

Suggested change

(idToDelete: string, event: React.MouseEvent) => {

event.stopPropagation();

const updatedUsers = localUsersData.filter((user) => user.userId !== idToDelete);

const updatedUsers = localUsersData.filter((user) => getUserIdentifier(user) !== idToDelete);

setLocalUsersData(updatedUsers);

setUsersData(updatedUsers);

(userToDelete: User, event: React.MouseEvent) => {

event.stopPropagation();

const updatedUsers = localUsersData.filter((user) => !isSameUser(user, userToDelete));

setLocalUsersData(updatedUsers);

setUsersData(updatedUsers);

Fixed in eaf7ef0 - handleDelete now receives the User record and filters with isSameUser, using reference equality as the last resort so records carrying neither identifier nor email stay deletable (isSameUser deliberately never matches two empty records). Chip and access-list keys also move off getUserIdentifier to getUserLabel, since duplicate '' keys break React reconciliation for exactly these records. Regression tests cover both deletion cases.

gemini-code-assist · 2026-07-16T03:25:14Z

-            label={localUsersData[0].email}
-            onDelete={(e) => handleDelete(localUsersData[0].userId, e)}
+            label={getUserLabel(localUsersData[0])}
+            onDelete={(e) => handleDelete(getUserIdentifier(localUsersData[0]), e)}


Update the onDelete handler of the first Chip component to pass the entire User object instead of just the identifier string, matching the updated handleDelete signature.

Suggested change

onDelete={(e) => handleDelete(getUserIdentifier(localUsersData[0]), e)}

onDelete={(e) => handleDelete(localUsersData[0], e)}

Fixed in eaf7ef0 - the collapsed chip now passes localUsersData[0] to handleDelete.

gemini-code-assist · 2026-07-16T03:25:14Z

-              label={user.email}
-              onDelete={(e) => handleDelete(user.userId, e)}
+              label={getUserLabel(user)}
+              onDelete={(e) => handleDelete(getUserIdentifier(user), e)}


Update the onDelete handler of the mapped Chip components to pass the entire User object instead of just the identifier string, matching the updated handleDelete signature.

Suggested change

onDelete={(e) => handleDelete(getUserIdentifier(user), e)}

onDelete={(e) => handleDelete(user, e)}

Fixed in eaf7ef0 - the expanded chips now pass the mapped user record to handleDelete.

gemini-code-assist · 2026-07-16T03:25:14Z

 export const canUpdateResource = (
  selectedResource: { visibility: string },
  currentUser: User,
  resourceOwner: User
 ) => {


The implementation of canUpdateResource explicitly handles a nullish currentUser and resourceOwner (e.g., using optional chaining currentUser?.roleNames and checking Boolean(ownerIdentifier)). However, the TypeScript signature still types them as non-nullable User.

To prevent TypeScript compilation errors for consumers and accurately reflect the defensive design, update the parameter types to User | null | undefined.

Suggested change

export const canUpdateResource = (

selectedResource: { visibility: string },

currentUser: User,

resourceOwner: User

) => {

export const canUpdateResource = (

selectedResource: { visibility: string },

currentUser: User | null | undefined,

resourceOwner: User | null | undefined

) => {

Fixed in eaf7ef0 - canUpdateResource now types currentUser and resourceOwner as User | null | undefined, and canShareResourceWithNewUsers gets the same widening since it shares the shape and delegates to it. A new permissions test suite locks in the nullish contract.

…n signatures Review findings from the v1beta3 re-review: - Chip deletion filtered localUsersData by identifier string, so every identifier-less record (email-only invitees) collapsed onto '' and one delete removed them all. handleDelete now takes the User record and filters with isSameUser, with reference equality as the last resort for records carrying neither identifier nor email. Chip and access-list keys move to getUserLabel for the same collapse reason. - canUpdateResource and canShareResourceWithNewUsers tolerated nullish users at runtime but their signatures still demanded User; widened to User | null | undefined so TS consumers can pass auth-transition state without casts. Adds regression tests for both single-record deletion cases and a permissions suite covering the nullish contract. Signed-off-by: jamieplu <179417684+jamieplu@users.noreply.github.com>

hortison · 2026-07-16T04:24:44Z

/gemini review

gemini-code-assist

Code Review

This pull request refactors user identity and display logic to support the v1beta3 user schema and reduced user projections. It introduces centralized utility helpers in src/utils/user.ts and updates components like ShareModal and UserSearchField to use them, backed by comprehensive new tests. Review feedback suggests guarding selectedResource against nullish values in permission checks and the share modal, adding a reference equality check to isSameUser to simplify filtering, and moving the nested UserChip component outside of its parent to avoid React performance anti-patterns.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

gemini-code-assist · 2026-07-16T04:27:21Z

 export const canUpdateResource = (
  selectedResource: { visibility: string },
-  currentUser: User,
-  resourceOwner: User
+  currentUser: User | null | undefined,
+  resourceOwner: User | null | undefined
 ) => {
-  const isOwner = resourceOwner.userId == currentUser.userId;
-  const isAdmin = currentUser.roleNames?.includes('admin');
+  const ownerIdentifier = getUserIdentifier(resourceOwner);
+  const isOwner = Boolean(ownerIdentifier) && ownerIdentifier === getUserIdentifier(currentUser);
+  // Tolerate a nullish currentUser (auth-state transitions in JS consumers):
+  // no user means no permission, consistent with getUserIdentifier above.
+  const isAdmin = currentUser?.roleNames?.includes('admin') ?? false;
  return isOwner || isAdmin;
 };


Guard selectedResource against nullish values to prevent runtime TypeErrors when accessed by JS consumers or during initial load states.

Suggested change

export const canUpdateResource = (

selectedResource: { visibility: string },

currentUser: User,

resourceOwner: User

currentUser: User | null | undefined,

resourceOwner: User | null | undefined

) => {

const isOwner = resourceOwner.userId == currentUser.userId;

const isAdmin = currentUser.roleNames?.includes('admin');

const ownerIdentifier = getUserIdentifier(resourceOwner);

const isOwner = Boolean(ownerIdentifier) && ownerIdentifier === getUserIdentifier(currentUser);

// Tolerate a nullish currentUser (auth-state transitions in JS consumers):

// no user means no permission, consistent with getUserIdentifier above.

const isAdmin = currentUser?.roleNames?.includes('admin') ?? false;

return isOwner || isAdmin;

};

export const canUpdateResource = (

selectedResource: { visibility: string } | null | undefined,

currentUser: User | null | undefined,

resourceOwner: User | null | undefined

) => {

if (!selectedResource) return false;

const ownerIdentifier = getUserIdentifier(resourceOwner);

const isOwner = Boolean(ownerIdentifier) && ownerIdentifier === getUserIdentifier(currentUser);

// Tolerate a nullish currentUser (auth-state transitions in JS consumers):

// no user means no permission, consistent with getUserIdentifier above.

const isAdmin = currentUser?.roleNames?.includes('admin') ?? false;

return isOwner || isAdmin;

};

Fixed in 273a474 - selectedResource is widened to accept nullish in both permission checks; canUpdateResource denies by default when the resource is nullish, with regression tests.

gemini-code-assist · 2026-07-16T04:27:21Z

  const [resourceVisibility, setVisibility] = useState(
    Array.isArray(selectedResource) ? selectedResource[0].visibility : selectedResource.visibility
  );


Use optional chaining when accessing selectedResource properties to prevent runtime crashes if selectedResource is nullish or an empty array.

Suggested change

const [resourceVisibility, setVisibility] = useState(

Array.isArray(selectedResource) ? selectedResource[0].visibility : selectedResource.visibility

);

const [resourceVisibility, setVisibility] = useState(

Array.isArray(selectedResource) ? selectedResource[0]?.visibility : selectedResource?.visibility

);

Fixed in 273a474 - the visibility initializer uses optional chaining for both the array and single-resource shapes.

gemini-code-assist · 2026-07-16T04:27:22Z

+export const isSameUser = (a: User | null | undefined, b: User | null | undefined): boolean => {
+  const idA = getUserIdentifier(a);
+  const idB = getUserIdentifier(b);
+  if (idA && idB) return idA === idB;
+  if (a?.email && b?.email) return a.email === b.email;
+  return false;
+};


Add a reference equality check at the beginning of isSameUser. If two variables point to the exact same object in memory, they are definitely the same user. This also simplifies consumer filtering logic.

Suggested change

export const isSameUser = (a: User | null | undefined, b: User | null | undefined): boolean => {

const idA = getUserIdentifier(a);

const idB = getUserIdentifier(b);

if (idA && idB) return idA === idB;

if (a?.email && b?.email) return a.email === b.email;

return false;

};

export const isSameUser = (a: User | null | undefined, b: User | null | undefined): boolean => {

if (a && b && a === b) return true;

const idA = getUserIdentifier(a);

const idB = getUserIdentifier(b);

if (idA && idB) return idA === idB;

if (a?.email && b?.email) return a.email === b.email;

return false;

};

Fixed in 273a474 - isSameUser short-circuits on reference equality before the identifier and email comparisons, with a test for identifier-less records.

gemini-code-assist · 2026-07-16T04:27:22Z

+      const updatedUsers = localUsersData.filter(
+        (user) => user !== userToDelete && !isSameUser(user, userToDelete)
+      );


With the reference equality check added to isSameUser, the redundant user !== userToDelete check can be simplified.

Suggested change

const updatedUsers = localUsersData.filter(

(user) => user !== userToDelete && !isSameUser(user, userToDelete)

);

const updatedUsers = localUsersData.filter(

(user) => !isSameUser(user, userToDelete)

);

Fixed in 273a474 - with reference equality inside isSameUser, the filter is now just !isSameUser(user, userToDelete).

gemini-code-assist · 2026-07-16T04:27:22Z

+import {
+  getUserContactLabel,
+  getUserDisplayName,
+  getUserLabel,
+  isSameUser,
+  User
+} from '../../utils/user';


Define UserChip outside of the UserShareSearch component. Defining components inside other components is a React anti-pattern that causes the component type to be re-created on every render, leading to unmounting/remounting of DOM nodes, potential focus loss, and performance degradation.

import { getUserContactLabel, getUserDisplayName, getUserLabel, isSameUser, User } from '../../utils/user'; const UserChip = ({ avatarObj, ...props }: { avatarObj: User } & Record<string, any>) => ( <Chip key={getUserLabel(avatarObj)} avatar={ <Avatar alt={getUserDisplayName(avatarObj)} src={avatarObj.avatarUrl}> {avatarObj.avatarUrl ? '' : getUserDisplayName(avatarObj).charAt(0)} </Avatar> } label={getUserLabel(avatarObj)} size="small" {...props} /> );

Fixed in 273a474 - UserChip is hoisted to module scope so its component type stays stable across renders.

isSameUser now short-circuits on reference equality, so identifier-less records compare correctly everywhere and the handleDelete filter needs no local reference check. Permission checks deny-by-default on a nullish resource, and ShareModal's visibility initializer tolerates a nullish or empty resource selection. UserChip moves to module scope so its component type stays stable across renders instead of remounting chips. Signed-off-by: jamieplu <179417684+jamieplu@users.noreply.github.com>

hortison · 2026-07-16T04:39:22Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a new set of user utility functions in src/utils/user.ts to support v1beta3 user records and reduced projections, handling canonical identifiers, display names, contact labels, and identity comparisons. These utilities are integrated into permission checks, the share modal, and user search fields, accompanied by extensive unit and integration tests. A review comment suggests providing a fallback default value when initializing the resourceVisibility state in ShareModal.tsx to prevent React warnings about uncontrolled components when selectedResource is null or undefined.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

gemini-code-assist · 2026-07-16T04:41:12Z

  const [resourceVisibility, setVisibility] = useState(
-    Array.isArray(selectedResource) ? selectedResource[0].visibility : selectedResource.visibility
+    Array.isArray(selectedResource) ? selectedResource[0]?.visibility : selectedResource?.visibility
  );


When initializing the resourceVisibility state, if selectedResource is null, undefined, or an empty array, the state will be initialized to undefined. Passing undefined to the value prop of a Material-UI Select component makes it uncontrolled, which can trigger React warnings when it later becomes defined. It is safer to provide a fallback default value (such as '' or a default visibility constant) to ensure the component remains controlled.

Suggested change

const [resourceVisibility, setVisibility] = useState(

Array.isArray(selectedResource) ? selectedResource[0].visibility : selectedResource.visibility

Array.isArray(selectedResource) ? selectedResource[0]?.visibility : selectedResource?.visibility

);

const [resourceVisibility, setVisibility] = useState(

(Array.isArray(selectedResource) ? selectedResource[0]?.visibility : selectedResource?.visibility) ?? ''

);

Fixed in e49d208 - the initializer now falls back to '' so the Select stays controlled.

…olled A nullish or empty resource selection would initialize the state to undefined, flipping the MUI Select to uncontrolled and warning once a value arrives. Signed-off-by: jamieplu <179417684+jamieplu@users.noreply.github.com>

hortison · 2026-07-16T04:42:41Z

/gemini review

gemini-code-assist

Code Review

This pull request refactors user display and identity logic across the sharing and search components to support the v1beta3 user schema, introducing robust fallback helpers and comprehensive test coverage. Key feedback highlights a critical bug in UserSearchField where the search input is prematurely disabled when no users are selected, and recommends adding fail-fast guards in ShareModal to prevent runtime errors when selectedResource is nullish or empty. Additionally, it is suggested to properly spread getItemProps onto the rendered chips in the Autocomplete component.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

gemini-code-assist · 2026-07-16T04:45:53Z

          onChange={handleAdd}
          onInputChange={handleInputChange}
-          isOptionEqualToValue={(option, value) => option.email === value.email}
+          isOptionEqualToValue={isSameUser}


There is a critical bug in the renderInput prop of the Autocomplete component (starting on line 146). The TextField is configured with disabled={isShareDisabled}.

However, isShareDisabled is defined as:
const isShareDisabled = disabled || isSharing || usersToShareWith.length === 0;

This means that when the component first mounts and no users are selected yet (usersToShareWith.length === 0), isShareDisabled is true, which disables the search input. As a result, users can never type in the input to search and select users.

The TextField should only be disabled when the component is explicitly disabled or currently sharing. The isShareDisabled check should only apply to the "Share" button.

Please update the TextField's disabled prop to:

disabled={disabled || isSharing}

gemini-code-assist · 2026-07-16T04:45:53Z

          loading={searchUserLoading}
          value={usersToShareWith}
-          getOptionLabel={(user) => user.email}
+          getOptionLabel={(user) => getUserLabel(user)}


Ensure that the renderValue prop in the Autocomplete component accepts the getItemProps argument and spreads it onto the rendered UserChip components. This aligns with the API and enables standard tag property injection and deletion handling.

renderValue={(value, { getItemProps }) => value.map((user, index) => <UserChip avatarObj={user} {...getItemProps({ index })} />) }

References

Ensure that renderValue in Autocomplete components accepts the getItemProps argument and spreads it onto the rendered Chip components to align with the API and enable standard tag property injection and deletion handling.

gemini-code-assist · 2026-07-16T04:45:53Z

+    // Fail fast on records without a usable identifier: an empty actor_id
+    // would produce an invalid grant_access payload and a confusing
+    // partial-share result.
+    if (newUsers.some((user) => !getUserIdentifier(user))) {
+      notify({
+        message: `Unable to share ${dataName}: a selected user record is missing its identifier`,
+        event_type: 'error'
+      });
+      return { error: 'missing user identifier' };
+    }


While the component's state initialization is guarded against a nullish or empty selectedResource, the handleShareWithNewUsers function is not. If selectedResource is nullish or an empty array, calling handleShareWithNewUsers will result in a runtime TypeError when trying to access selectedResource.id or selectedResource[0].id.

Adding a fail-fast guard at the beginning of the function prevents this crash and provides a clear error state.

if (!selectedResource || (Array.isArray(selectedResource) && selectedResource.length === 0)) { notify({ message: `Unable to share ${dataName}: no resource selected`, event_type: 'error' }); return { error: 'no resource selected' }; } // Fail fast on records without a usable identifier: an empty actor_id // would produce an invalid grant_access payload and a confusing // partial-share result. if (newUsers.some((user) => !getUserIdentifier(user))) { notify({ message: `Unable to share ${dataName}: a selected user record is missing its identifier`, event_type: 'error' }); return { error: 'missing user identifier' }; }

gemini-code-assist · 2026-07-16T04:45:53Z

+    // Same guard as sharing: never issue a revoke_access entry with an
+    // empty actor_id.
+    if (revokedUsers.some((user) => !getUserIdentifier(user))) {
+      notify({
+        message: `Unable to revoke access to ${dataName}: the user record is missing its identifier`,
+        event_type: 'error'
+      });
+      return { error: 'missing user identifier' };
+    }


Similar to handleShareWithNewUsers, the handleRevokeAccess function is not guarded against a nullish or empty selectedResource. If selectedResource is nullish or an empty array, calling handleRevokeAccess will result in a runtime TypeError when trying to access selectedResource.id or selectedResource[0].id.

Adding a fail-fast guard at the beginning of the function prevents this crash and provides a clear error state.

if (!selectedResource || (Array.isArray(selectedResource) && selectedResource.length === 0)) { notify({ message: `Unable to revoke access to ${dataName}: no resource selected`, event_type: 'error' }); return { error: 'no resource selected' }; } // Same guard as sharing: never issue a revoke_access entry with an // empty actor_id. if (revokedUsers.some((user) => !getUserIdentifier(user))) { notify({ message: `Unable to revoke access to ${dataName}: the user record is missing its identifier`, event_type: 'error' }); return { error: 'missing user identifier' }; }

Copilot AI review requested due to automatic review settings July 15, 2026 22:41

Copilot started reviewing on behalf of hortison July 15, 2026 22:41 View session

Copilot AI reviewed Jul 15, 2026

View reviewed changes

gemini-code-assist Bot reviewed Jul 15, 2026

View reviewed changes

jamieplu added 2 commits July 15, 2026 22:16

gemini-code-assist Bot reviewed Jul 16, 2026

View reviewed changes

	{option.deleted ? (
	{isSoftDeleted(option.deletedAt) \|\| option.deleted ? (

	{!localUsersData[0].avatarUrl && getUserDisplayName(localUsersData[0])[0]}
	{!localUsersData[0].avatarUrl && getUserDisplayName(localUsersData[0]).charAt(0)}

	{!user.avatarUrl && getUserDisplayName(user)[0]}
	{!user.avatarUrl && getUserDisplayName(user).charAt(0)}

	const alreadySelectedUsers = usersToShareWith.concat(usersData);
	const alreadySelectedUsers = usersToShareWith.concat(usersData ?? []);

	label={getUserContactLabel(avatarObj) \|\| getUserDisplayName(avatarObj)}
	label={getUserContactLabel(avatarObj) \|\| getUserDisplayName(avatarObj) \|\| getUserIdentifier(avatarObj)}

	getOptionLabel={(user) => getUserContactLabel(user) \|\| getUserDisplayName(user)}
	getOptionLabel={(user) => getUserContactLabel(user) \|\| getUserDisplayName(user) \|\| getUserIdentifier(user)}

	const recipients = newUsers.map((u) => getUserContactLabel(u) \|\| getUserDisplayName(u));
	const recipients = newUsers.map((u) => getUserContactLabel(u) \|\| getUserDisplayName(u) \|\| getUserIdentifier(u));

	onDelete={(e) => handleDelete(getUserIdentifier(localUsersData[0]), e)}
	onDelete={(e) => handleDelete(localUsersData[0], e)}

	onDelete={(e) => handleDelete(getUserIdentifier(user), e)}
	onDelete={(e) => handleDelete(user, e)}

Uh oh!

Conversation

hortison commented Jul 15, 2026

Symptom

Root cause

Fix

Out-of-scope fixes included

Watch for

Coordinated changes

Test plan

Uh oh!

hortison commented Jul 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hortison commented Jul 15, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 15, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment