Skip to content

Conversation

@devksingh4
Copy link
Member

@devksingh4 devksingh4 commented Nov 15, 2025

Summary by CodeRabbit

  • New Features

    • Batch email resolution with server-side batching, retries, and graceful partial results; app-wide provider enables caching and fallbacks for display names.
    • New user-resolution card used across tables, logs, membership lists, room requests, and links for consistent avatar/name display.
    • Click-to-copy ticket ID via purchaser email cell with user notifications; minor UI spacing and timeline sizing tweaks.
  • Tests

    • Added live and e2e test updates covering batch user-resolution and adjusted render expectations.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 15, 2025

Walkthrough

Adds a batched user-info resolver: backend POST /api/v1/users/batchResolveInfo that queries DynamoDB with batching and retry/backoff; frontend introduces UserResolverProvider and NameOptionalUserCard to batch, cache, and display resolved names across multiple UI pages and tests.

Changes

Cohort / File(s) Change Summary
Backend API - Batch User Info Resolution
src/api/functions/uin.ts, src/api/routes/user.ts, src/common/types/user.ts
New batchGetUserInfo using BatchGetItemCommand (batch size 100) with retryDynamoTransactionWithBackoff, detection of UnprocessedKeys to trigger retries, partial-result logging on failure; new POST /batchResolveInfo route; Zod request/response schemas and exported TS type for batch responses.
Frontend - User Resolver Context & Component
src/ui/components/NameOptionalCard/index.tsx, src/ui/App.tsx
New UserResolverProvider implementing batched requests, debounce, caching (including NO_NAME_FOUND sentinel), resolutionDisabled mode, and hooks; new NameOptionalUserCard component; provider wrapped around Router in App.tsx.
Tables & Lists Updated to Use Resolver
src/ui/pages/apiKeys/ManageKeys.page.tsx, src/ui/pages/apiKeys/ManageKeysTable.tsx, src/ui/pages/apiKeys/ManageKeysTable.test.tsx, src/ui/pages/iam/GroupMemberManagement.tsx, src/ui/pages/iam/GroupMemberManagement.test.tsx, src/ui/pages/logs/LogRenderer.tsx, src/ui/pages/logs/LogRenderer.test.tsx, src/ui/pages/membershipLists/ExternalMemberListManagement.tsx, src/ui/pages/stripe/CurrentLinks.tsx, src/ui/pages/stripe/CurrentLinks.test.tsx
Replaced inline email/name cells with NameOptionalUserCard rendering; tests wrapped with UserResolverProvider (often with resolutionDisabled) and updated expectations; minor layout change in ManageKeys.page.tsx (container size).
Pages with UI Adjustments / Interactions
src/ui/pages/roomRequest/ViewRoomRequest.page.tsx, src/ui/pages/tickets/ViewTickets.page.tsx
ViewRoomRequest: timeline bulletSize and spacing tweaks; CreatedBy now uses NameOptionalUserCard; spacing adjustments. ViewTickets: purchaser cell now renders NameOptionalUserCard inside a clickable Box with Tooltip to copy ticket ID; Ticket ID column removed.
Tests: Live & E2E Updates
tests/live/user.test.ts, tests/e2e/auditLogs.spec.ts, tests/e2e/membershipLists.spec.ts
New live tests for /batchResolveInfo (invalid 400, valid 200 verifying firstName/lastName); small timing insertion (500ms) in audit logs e2e; membership lists e2e updated assertions to account for duplicated/name-card rendering and selector adjustments.

Sequence Diagram

sequenceDiagram
    autonumber
    participant UI as NameOptionalUserCard
    participant Provider as UserResolverProvider
    participant Cache as Local Cache
    participant API as /api/v1/users/batchResolveInfo
    participant Backend as batchGetUserInfo (DynamoDB)

    UI->>Provider: requestUser(email)
    Provider->>Cache: lookup(email)
    alt cached
        Cache-->>Provider: name or NO_NAME_FOUND
        Provider-->>UI: return result
    else not cached
        Provider->>Provider: enqueue email (debounce window)
        Note over Provider: batch formed after delay
        Provider->>API: POST { emails: [...] }
        API->>Backend: batchGetUserInfo(emails)
        Backend->>Backend: BatchGetItemCommand (<=100) with projection
        alt UnprocessedKeys present
            Backend->>Backend: emit TransactionCanceledException → retryDynamoTransactionWithBackoff
        end
        Backend-->>API: mapping { email: { firstName?, lastName? } }
        API-->>Provider: mapping
        Provider->>Cache: store results (including NO_NAME_FOUND)
        Provider-->>UI: resolve pending requests
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

  • Focus review on:
    • src/api/functions/uin.ts: correctness of batching logic, handling/detection of UnprocessedKeys, retry/backoff integration, and logging of partial failures.
    • src/ui/components/NameOptionalCard/index.tsx: debounce/batching correctness, cache lifecycle and sentinel handling, concurrency/race conditions, memory/leak risks.
    • Integration touchpoints: ensure all replaced renderers pass expected email strings, provider wrapping in App.tsx covers intended subtree, and updated tests reflect new rendering semantics.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Implement batch name resolution for emails' accurately describes the main change: a new batch resolution system that fetches and resolves user names for multiple emails.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch dsingh14/setup-name-resolution

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 15, 2025

💰 Infracost report

Monthly estimate generated

This comment will be updated when code changes.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/ui/pages/roomRequest/ViewRoomRequest.page.tsx (1)

448-510: Add a stable React key on Timeline.Item in the map

data.updates.map((x) => (<Timeline.Item ...>)) currently renders list items without a key, which React (and Biome) warn against and can cause reconciliation glitches.

You can fix this by adding a deterministic key, e.g.:

-                {data.updates.map((x) => (
-                  <Timeline.Item
+                {data.updates.map((x) => (
+                  <Timeline.Item
+                    key={`${x.createdAt}-${x.status}`}
                     bullet={getStatusIcon(x.status)}
                     title={<Text size="md">{formatStatus(x.status)}</Text>}
                   >
                     {x.createdBy && (
                       <NameOptionalUserCard email={x.createdBy} />
                     )}

The rest of the changes here (NameOptionalUserCard usage, spacing tweaks, and Text wrappers) look good.

🧹 Nitpick comments (2)
src/ui/pages/tickets/ViewTickets.page.tsx (1)

363-379: Verify resolver provider wrapping and consider keyboard accessibility

Using <NameOptionalUserCard> in the Email column is a nice enhancement, but it depends on UserResolverProvider. Please confirm that ViewTicketsPage is rendered under that provider (e.g. via App.tsx), otherwise useUserResolver will throw at runtime.

Also, the clickable area for copying the ticket ID is a Box with onClick and pointer cursor. For better accessibility, consider using a <button>/ActionIcon or adding keyboard handlers (onKeyDown for Enter/Space and tabIndex={0}) so keyboard and screen‑reader users can trigger the copy behavior as well.

src/ui/components/NameOptionalCard/index.tsx (1)

51-106: Consider cleaning up batch timeout on unmount and differentiating transient failures

The batching / caching logic looks solid overall, but two small points:

  • batchTimeout.current is never cleared on provider unmount, so a pending timeout may still fire and call setUserCache after the component is gone, which can trigger “setState on unmounted component” warnings.
  • In the catch block you permanently mark all emailsToFetch as NO_NAME_FOUND. That makes sense for protecting the API, but it also means transient network/Dynamo issues can prevent any future resolution for those emails until a full reload.

If this provider is long‑lived (e.g. app‑wide), consider:

  • Adding a useEffect cleanup that clears batchTimeout.current on unmount.
  • Optionally distinguishing “hard” failures (e.g. 4xx) from transient ones and allowing retry on the latter (e.g. by not caching NO_NAME_FOUND in the error case, or by expiring those entries after some time).
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between f076739 and 2ccd435.

📒 Files selected for processing (18)
  • src/api/functions/uin.ts (2 hunks)
  • src/api/routes/user.ts (2 hunks)
  • src/common/types/user.ts (1 hunks)
  • src/ui/App.tsx (2 hunks)
  • src/ui/components/NameOptionalCard/index.tsx (1 hunks)
  • src/ui/pages/apiKeys/ManageKeys.page.tsx (1 hunks)
  • src/ui/pages/apiKeys/ManageKeysTable.test.tsx (3 hunks)
  • src/ui/pages/apiKeys/ManageKeysTable.tsx (2 hunks)
  • src/ui/pages/iam/GroupMemberManagement.test.tsx (2 hunks)
  • src/ui/pages/iam/GroupMemberManagement.tsx (2 hunks)
  • src/ui/pages/logs/LogRenderer.test.tsx (3 hunks)
  • src/ui/pages/logs/LogRenderer.tsx (3 hunks)
  • src/ui/pages/membershipLists/ExternalMemberListManagement.tsx (3 hunks)
  • src/ui/pages/roomRequest/ViewRoomRequest.page.tsx (3 hunks)
  • src/ui/pages/stripe/CurrentLinks.test.tsx (2 hunks)
  • src/ui/pages/stripe/CurrentLinks.tsx (2 hunks)
  • src/ui/pages/tickets/ViewTickets.page.tsx (4 hunks)
  • tests/live/user.test.ts (1 hunks)
🧰 Additional context used
🪛 Biome (2.1.2)
src/ui/pages/roomRequest/ViewRoomRequest.page.tsx

[error] 454-457: Missing key property for this element in iterable.

The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.

(lint/correctness/useJsxKeyInIterable)

🪛 ESLint
src/api/functions/uin.ts

[error] 9-9: Unexpected use of file extension "js" for "api/types.js"

(import/extensions)


[error] 10-10: Unexpected use of file extension "js" for "api/utils.js"

(import/extensions)

src/ui/components/NameOptionalCard/index.tsx

[error] 1-1: Resolve error: EACCES: permission denied, open '/kLAsoYtwGa'
at Object.writeFileSync (node:fs:2409:20)
at l (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:13685)
at createFilesMatcher (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:14437)
at Object.resolve (/home/jailuser/git/node_modules/eslint-import-resolver-typescript/lib/index.cjs:298:107)
at withResolver (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:180:23)
at fullResolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:201:22)
at relative (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:217:10)
at resolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:233:12)
at checkFileExtension (/home/jailuser/git/node_modules/eslint-plugin-import/lib/rules/extensions.js:205:53)
at checkSourceValue (/home/jailuser/git/node_modules/eslint-module-utils/moduleVisitor.js:32:5)

(import/extensions)


[error] 12-12: Missing file extension for "../AuthContext"

(import/extensions)

src/common/types/user.ts

[error] 13-13: Insert ,

(prettier/prettier)


[error] 14-15: Replace with ;

(prettier/prettier)


[error] 17-18: Delete

(prettier/prettier)


[error] 21-21: Insert ,

(prettier/prettier)


[error] 22-22: Insert ,

(prettier/prettier)


[error] 25-25: Replace typeof·batchResolveUserInfoResponse with ⏎··typeof·batchResolveUserInfoResponse⏎

(prettier/prettier)

src/api/routes/user.ts

[error] 16-16: Unexpected use of file extension "js" for "common/types/user.js"

(import/extensions)


[error] 21-21: Unexpected use of file extension "js" for "api/functions/uin.js"

(import/extensions)

tests/live/user.test.ts

[error] 1-1: Resolve error: EACCES: permission denied, open '/VVuTkMHkWc'
at Object.writeFileSync (node:fs:2409:20)
at l (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:13685)
at createFilesMatcher (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:14437)
at Object.resolve (/home/jailuser/git/node_modules/eslint-import-resolver-typescript/lib/index.cjs:298:107)
at withResolver (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:180:23)
at fullResolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:201:22)
at relative (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:217:10)
at resolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:233:12)
at checkFileExtension (/home/jailuser/git/node_modules/eslint-plugin-import/lib/rules/extensions.js:205:53)
at checkSourceValue (/home/jailuser/git/node_modules/eslint-module-utils/moduleVisitor.js:32:5)

(import/extensions)


[error] 2-2: Unexpected use of file extension "js" for "./utils.js"

(import/extensions)


[error] 3-3: './utils.js' import is duplicated.

(no-duplicate-imports)


[error] 3-3: Unexpected use of file extension "js" for "./utils.js"

(import/extensions)

src/ui/App.tsx

[error] 11-11: Missing file extension for "./components/NameOptionalCard"

(import/extensions)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Build Application
  • GitHub Check: Run Unit Tests
🔇 Additional comments (15)
src/ui/pages/apiKeys/ManageKeys.page.tsx (1)

19-19: LGTM!

The container size adjustment aligns with the card-based user rendering introduced in related components.

src/ui/pages/membershipLists/ExternalMemberListManagement.tsx (1)

26-26: LGTM!

The integration of NameOptionalUserCard for member display is consistent with the PR's batch name resolution feature. The column header change from "Member NetID" to "Member" appropriately reflects the card-based rendering.

Also applies to: 331-331, 378-378

src/ui/pages/iam/GroupMemberManagement.tsx (1)

25-25: LGTM!

The replacement of inline avatar/name/email rendering with NameOptionalUserCard provides a consistent user display pattern across the application.

Also applies to: 192-192

src/ui/pages/iam/GroupMemberManagement.test.tsx (1)

8-8: LGTM!

Wrapping the component with UserResolverProvider (with resolutionDisabled) ensures the test environment provides the necessary context for NameOptionalUserCard without triggering API calls.

Also applies to: 23-28

src/ui/pages/apiKeys/ManageKeysTable.tsx (1)

41-41: LGTM!

The use of NameOptionalUserCard with size="sm" in the table context is appropriate and standardizes owner display across the application.

Also applies to: 212-212

src/ui/App.tsx (1)

29-31: LGTM!

Wrapping the Router with UserResolverProvider appropriately establishes the user resolution context for all child components that use NameOptionalUserCard.

src/ui/pages/stripe/CurrentLinks.test.tsx (1)

8-8: LGTM!

The test infrastructure correctly provides UserResolverProvider context with resolutionDisabled, aligning with the new standardized user rendering approach.

Also applies to: 31-36

src/ui/pages/stripe/CurrentLinks.tsx (1)

21-21: LGTM!

The replacement of conditional "You" display logic with NameOptionalUserCard standardizes user rendering and removes the dependency on useAuth for display purposes.

Also applies to: 187-187

src/ui/pages/apiKeys/ManageKeysTable.test.tsx (1)

78-93: Good use of UserResolverProvider in the test harness

Wrapping OrgApiKeyTable with UserResolverProvider (with resolutionDisabled) is a clean way to keep tests independent of network-based name resolution while still exercising the new owner rendering path.

src/ui/pages/logs/LogRenderer.test.tsx (1)

40-51: User resolver provider wrapping looks appropriate

Wrapping LogRenderer with UserResolverProvider (using resolutionDisabled in tests) aligns the test harness with the new NameOptionalUserCard-based rendering without introducing network dependencies.

src/ui/pages/logs/LogRenderer.tsx (1)

213-245: Actor/target rendering changes align well with the new user resolver

The new use of NameOptionalUserCard for the actor and target columns looks consistent with the batch name-resolution design:

  • actor now benefits from name resolution while still taking a plain string.
  • target cleanly distinguishes between module-valued targets (AUDIT_LOG + Modules.*) and user/email targets, with a fallback that preserves the original email/text.

No functional issues spotted here; just ensure the parent tree always includes UserResolverProvider (as your updated tests already do).

src/api/functions/uin.ts (1)

242-317: Robust DynamoDB batch get implementation with sensible retry behavior

batchGetUserInfo looks solid:

  • Respects DynamoDB’s 100-item BatchGetItem limit via BATCH_SIZE.
  • Uses ProjectionExpression to only retrieve id, firstName, lastName, and safely builds a mapping from email → { firstName?, lastName? }.
  • Reuses retryDynamoTransactionWithBackoff, triggering retries when UnprocessedKeys is present and logging a warning while returning partial results if the batch still fails.

I don’t see correctness issues here given the surrounding utilities; the function should integrate cleanly with the new batchResolveInfo route.

src/api/routes/user.ts (2)

12-21: Check import path style vs ESLint import/extensions expectations

The new imports from common/types/user.js and api/functions/uin.js follow the same .js-suffixed pattern as the rest of this file, but the static analysis hints are flagging them as violations of import/extensions.

If your TS/ESM setup prefers extension-less imports here (e.g. common/types/user / api/functions/uin), it may be worth aligning these two new imports with that convention to keep lint green. Otherwise, consider adjusting the ESLint config for this backend package so it matches your intended module resolution rules.


67-98: Confirm authorization level for /batchResolveInfo

The new /batchResolveInfo route mirrors the existing /findUserByUin shape, but it’s registered with withRoles([]), whereas /findUserByUin requires AppRoles.VIEW_USER_INFO.

Since this endpoint resolves emails to user info (names) in batch, please double‑check that exposing it to “any authenticated caller allowed by authorizeFromSchema” is intentional, and that no additional role gating is needed (e.g. reusing VIEW_USER_INFO or a similar permission).

src/ui/components/NameOptionalCard/index.tsx (1)

10-13: Verify local import path vs import/extensions lint rule

ESLint is flagging the ../AuthContext import as missing a file extension. Other UI files may be using extension‑less local imports as well, so this could be a resolver configuration issue rather than a code issue.

Please verify whether your front‑end lint rules expect .tsx / .js here (e.g. ../AuthContext/index) and adjust either the import path or the ESLint import/extensions config so this file is consistent with the rest of the UI.


import ColorSchemeContext from "./ColorSchemeContext";
import { Router } from "./Router";
import { UserResolverProvider } from "./components/NameOptionalCard";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Fix the ESLint import/extensions violation.

The import statement is missing a file extension. While this may work with your current build configuration, it violates the ESLint rule and could cause issues in some environments.

Based on coding guidelines.

Apply this diff to add the file extension:

-import { UserResolverProvider } from "./components/NameOptionalCard";
+import { UserResolverProvider } from "./components/NameOptionalCard/index";

Alternatively, if there's an index.ts or index.tsx file that re-exports from the module, you could be more explicit:

-import { UserResolverProvider } from "./components/NameOptionalCard";
+import { UserResolverProvider } from "./components/NameOptionalCard/index.tsx";

Committable suggestion skipped: line range outside the PR's diff.

🧰 Tools
🪛 ESLint

[error] 11-11: Missing file extension for "./components/NameOptionalCard"

(import/extensions)

🤖 Prompt for AI Agents
In src/ui/App.tsx around line 11, the import for UserResolverProvider lacks a
file extension which violates the ESLint import/extensions rule; update the
import to include the explicit file extension (e.g. import from
"./components/NameOptionalCard.tsx") or, if you intend to import from a barrel
index, point to that file explicitly (e.g.
"./components/NameOptionalCard/index.ts" or ".tsx") so the resolver and linter
both accept it.

Comment on lines 115 to 118
await screen.findByText("User created");
expect(screen.getByText("admin")).toBeInTheDocument();
expect(screen.getByText("user@example.com")).toBeInTheDocument();
expect(screen.getAllByText("user@example.com")).length(2);
expect(screen.getByText("req-123")).toBeInTheDocument();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Correct the array length assertion for target email

Same issue as in the API keys tests: expect(screen.getAllByText("user@example.com")).length(2); is invalid and will throw.

Suggested fix:

-    expect(screen.getAllByText("user@example.com")).length(2);
+    expect(screen.getAllByText("user@example.com")).toHaveLength(2);
🤖 Prompt for AI Agents
In src/ui/pages/logs/LogRenderer.test.tsx around lines 115 to 118, the assertion
expect(screen.getAllByText("user@example.com")).length(2); is invalid and will
throw; replace it with a proper Jest assertion such as
expect(screen.getAllByText("user@example.com")).toHaveLength(2) (or
expect(screen.getAllByText("user@example.com").length).toBe(2)) so the test
correctly asserts there are two matching elements.

devksingh4 and others added 3 commits November 15, 2025 15:13
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (2)
src/ui/pages/tickets/ViewTickets.page.tsx (1)

301-314: Async clipboard handling is now correct.

The copyTicketId function properly handles the async navigator.clipboard.writeText call by making the function async and awaiting the operation. Error handling will now correctly catch permission or other async failures.

This addresses the previous review comment about synchronous try/catch not catching Promise rejections.

Based on learnings

tests/live/user.test.ts (1)

1-52: Address remaining test structure issues (Authorization header is now fixed).

The Authorization header has been correctly added (lines 19, 36), but three issues from the previous review remain:

  1. Async describe callback (line 7)

    • describe blocks should be synchronous; move const token = await createJwt() to a beforeAll hook
  2. Duplicate imports (lines 2-3)

    • Both createJwt and getBaseEndpoint are imported from ./utils.js in separate statements
  3. Wrong response type (line 44)

    • Typed as string[] but used as Record<string, { firstName?: string; lastName?: string }>

Apply this refactor:

-import { expect, test, describe } from "vitest";
-import { createJwt } from "./utils.js";
-import { getBaseEndpoint } from "./utils.js";
+import { expect, test, describe, beforeAll } from "vitest";
+import { createJwt, getBaseEndpoint } from "./utils.js";

 const baseEndpoint = getBaseEndpoint();

-describe("User info live tests", async () => {
-  const token = await createJwt();
+describe("User info live tests", () => {
+  let token: string;
+
+  beforeAll(async () => {
+    token = await createJwt();
+  });

   test(
     "Test that invalid emails are rejected",
     { timeout: 10000 },
     async () => {
       const response = await fetch(
         `${baseEndpoint}/api/v1/users/batchResolveInfo`,
         {
           method: "POST",
           headers: {
             "Content-Type": "application/json",
             Authorization: `Bearer ${token}`,
           },
           body: JSON.stringify({
             emails: ["invalid"],
           }),
         },
       );
       expect(response.status).toBe(400);
     },
   );
   test("Test that valid emails are resolved", { timeout: 10000 }, async () => {
     const response = await fetch(
       `${baseEndpoint}/api/v1/users/batchResolveInfo`,
       {
         method: "POST",
         headers: {
           "Content-Type": "application/json",
           Authorization: `Bearer ${token}`,
         },
         body: JSON.stringify({
           emails: ["testinguser@illinois.edu"],
         }),
       },
     );
     expect(response.status).toBe(200);
-    const responseJson = (await response.json()) as string[];
+    const responseJson = (await response.json()) as Record<
+      string,
+      { firstName?: string; lastName?: string }
+    >;
     expect(responseJson).toEqual({
       "testinguser@illinois.edu": {
         firstName: "Testing",
         lastName: "User",
       },
     });
   });
 });

Based on learnings

🧹 Nitpick comments (3)
tests/e2e/auditLogs.spec.ts (1)

23-23: Timing guard for async name resolution looks reasonable.

The 500ms delay accommodates the batched user resolution introduced by NameOptionalUserCard. This is a pragmatic fix for e2e test stability.

For better resilience under load, you could wait for a specific UI state instead of a fixed timeout:

-await page.waitForTimeout(500);
+// Wait for skeleton loaders to disappear (indicating resolution complete)
+await page.waitForSelector('[data-testid="user-skeleton"]', { state: 'detached', timeout: 2000 }).catch(() => {});

However, the fixed delay is acceptable for e2e tests.

src/ui/components/NameOptionalCard/index.tsx (1)

223-241: Past review about polling is no longer applicable; current implementation uses cacheVersion reactivity.

The previous review comment about infinite polling with setInterval is outdated. The current implementation uses cacheVersion as a dependency (line 238) to reactively update when the cache changes, which is a cleaner approach.

One performance consideration: cacheVersion increments on every cache update (lines 100, 110, 120, 134), causing all mounted NameOptionalUserCard instances to re-run their effects even when an unrelated email resolves. For pages with many cards, this creates unnecessary work.

A more granular approach would be to track per-email resolution status, but the current implementation is functionally correct and acceptable for typical use cases.

Based on learnings

src/ui/pages/tickets/ViewTickets.page.tsx (1)

363-379: Click-to-copy UX is clear and functional.

The tooltip "Click to copy ticket ID" provides clear affordance for the interaction. The implementation correctly calls copyTicketId on click.

One minor UX consideration: clicking the user card (which shows email/name) to copy the ticket ID might be slightly unintuitive. An alternative would be to add a small copy icon button:

<Table.Td>
  <Group gap="sm" wrap="nowrap">
    <NameOptionalUserCard
      email={ticket.purchaserData.email}
      size="sm"
    />
    <Tooltip label="Copy ticket ID" position="top" withArrow>
      <ActionIcon onClick={() => copyTicketId(ticket.ticketId)}>
        <IconCopy size={16} />
      </ActionIcon>
    </Tooltip>
  </Group>
</Table.Td>

However, the current approach is acceptable with the clear tooltip.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to data retention organization setting

📥 Commits

Reviewing files that changed from the base of the PR and between 84cf6f8 and feab435.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (5)
  • src/ui/components/NameOptionalCard/index.tsx (1 hunks)
  • src/ui/pages/tickets/ViewTickets.page.tsx (4 hunks)
  • tests/e2e/auditLogs.spec.ts (1 hunks)
  • tests/e2e/membershipLists.spec.ts (2 hunks)
  • tests/live/user.test.ts (1 hunks)
🧰 Additional context used
🪛 ESLint
tests/live/user.test.ts

[error] 1-1: Resolve error: EACCES: permission denied, open '/mhvZRjVCNg'
at Object.writeFileSync (node:fs:2409:20)
at l (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:13685)
at createFilesMatcher (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:14437)
at Object.resolve (/home/jailuser/git/node_modules/eslint-import-resolver-typescript/lib/index.cjs:298:107)
at withResolver (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:180:23)
at fullResolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:201:22)
at relative (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:217:10)
at resolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:233:12)
at checkFileExtension (/home/jailuser/git/node_modules/eslint-plugin-import/lib/rules/extensions.js:205:53)
at checkSourceValue (/home/jailuser/git/node_modules/eslint-module-utils/moduleVisitor.js:32:5)

(import/extensions)


[error] 2-2: Unexpected use of file extension "js" for "./utils.js"

(import/extensions)


[error] 3-3: './utils.js' import is duplicated.

(no-duplicate-imports)


[error] 3-3: Unexpected use of file extension "js" for "./utils.js"

(import/extensions)

src/ui/components/NameOptionalCard/index.tsx

[error] 1-1: Resolve error: EACCES: permission denied, open '/YGsqPelKDq'
at Object.writeFileSync (node:fs:2409:20)
at l (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:13685)
at createFilesMatcher (/home/jailuser/git/node_modules/get-tsconfig/dist/index.cjs:7:14437)
at Object.resolve (/home/jailuser/git/node_modules/eslint-import-resolver-typescript/lib/index.cjs:298:107)
at withResolver (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:180:23)
at fullResolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:201:22)
at relative (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:217:10)
at resolve (/home/jailuser/git/node_modules/eslint-module-utils/resolve.js:233:12)
at checkFileExtension (/home/jailuser/git/node_modules/eslint-plugin-import/lib/rules/extensions.js:205:53)
at checkSourceValue (/home/jailuser/git/node_modules/eslint-module-utils/moduleVisitor.js:32:5)

(import/extensions)


[error] 12-12: Missing file extension for "../AuthContext"

(import/extensions)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Run Unit Tests
🔇 Additional comments (2)
tests/e2e/membershipLists.spec.ts (2)

40-40: Count-based assertions align with NameOptionalUserCard structure.

The updated assertions expect exactly 2 occurrences of each NetID, which aligns with the NameOptionalUserCard rendering both the display name and email address as separate text elements.

Also applies to: 48-48


57-59: More specific row selector improves test reliability.

The updated selector uses the full rendered display from NameOptionalUserCard, making the test less likely to match unintended rows.

@devksingh4 devksingh4 merged commit a30528e into main Nov 15, 2025
13 of 15 checks passed
@devksingh4 devksingh4 deleted the dsingh14/setup-name-resolution branch November 15, 2025 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants