Repository navigation
Conversation
📝 WalkthroughWalkthroughLinear team member retrieval now looks up and caches the viewer ID per workspace credential revision, handles viewer lookup failures with a warning and fallback, and sorts members with the viewer first followed by display name and ID ordering. Both retrieval functions use the new sorting behavior. Tests cover pagination fixtures, viewer ordering, non-member and lookup-failure fallbacks, credential rotation cache invalidation, and deterministic duplicate-name ordering. Merge Risk: 🔵 Low · up to If Linear credentials change while the assignee list is loading, the logged-in user could appear in the wrong position because the viewer identity and member list may come from different sessions. This is a bounded ordering issue that should receive explicit owner follow-up. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/linear/teams.ts (1)
22-41: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConcurrent cold-cache calls trigger redundant viewer lookups; persistent failures re-hit the API on every call.
getCachedViewerIdhas two related gaps:
- If multiple calls for the same workspace race before the cache is populated (e.g. several UI panels requesting team members on startup), each misses the cache and issues its own
entry.client.viewercall instead of sharing one in-flight lookup.- On failure the result isn't cached at all, so every subsequent
getTeamMembers/getTeamMembersOrThrowcall for that revision re-attempts the viewer lookup — if the endpoint is down/rate-limited, this adds a redundant failing call (plus aconsole.warn) to every member-list fetch until it recovers.Neither breaks correctness (fallback to alpha order still works), but both add avoidable load on the Linear API.
♻️ Dedup concurrent lookups with an in-flight promise map
+const viewerLookupInFlight = new Map<string, Promise<string | null>>() + async function getCachedViewerId(entry: LinearClientForWorkspace): Promise<string | null> { const revision = entry.workspace.credentialRevision ?? 0 const cached = viewerIdCache.get(entry.workspace.id) if (cached && cached.revision === revision) { return cached.viewerId } - await acquire() - try { - const viewer = await entry.client.viewer - viewerIdCache.set(entry.workspace.id, { revision, viewerId: viewer.id }) - return viewer.id - } catch (error) { - // Member lists must not fail because the viewer lookup did. - console.warn('[linear] viewer lookup for member ordering failed:', error) - return null - } finally { - release() - } + const inFlight = viewerLookupInFlight.get(entry.workspace.id) + if (inFlight) { + return inFlight + } + + const lookup = (async () => { + await acquire() + try { + const viewer = await entry.client.viewer + viewerIdCache.set(entry.workspace.id, { revision, viewerId: viewer.id }) + return viewer.id + } catch (error) { + // Member lists must not fail because the viewer lookup did. + console.warn('[linear] viewer lookup for member ordering failed:', error) + return null + } finally { + release() + viewerLookupInFlight.delete(entry.workspace.id) + } + })() + viewerLookupInFlight.set(entry.workspace.id, lookup) + return lookup }src/main/linear/teams.test.ts (1)
345-361: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the warning was actually logged.
The test spies on
console.warnbut never asserts it was called, so a regression that silently drops the warning (or changes its message) wouldn't be caught — only the fallback ordering is verified.✅ Add a warning assertion
await expect(getTeamMembersOrThrow('team-1', 'workspace-1')).resolves.toMatchObject([ { id: 'user-3' }, { id: 'user-1' }, { id: 'user-2' }, { id: 'user-4' } ]) + expect(warn).toHaveBeenCalledWith( + expect.stringContaining('viewer lookup'), + expect.any(Error) + )
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8178c88e-c3c1-4a2e-b182-0e2f986fd15b
📥 Commits
Reviewing files that changed from the base of the PR and between 2cf91b8 and 41710431d6349d6eb2e76ad48fc9fbed5014e370.
📒 Files selected for processing (2)
src/main/linear/teams.test.tssrc/main/linear/teams.ts
|
Addressed both review nitpicks in f3346d4: concurrent cold-cache member fetches now share one in-flight viewer lookup (keyed by workspace + credential revision), and the fallback test asserts the warning is logged. The ordering logic moved to |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 03350cef-18f3-410a-930e-a07a5aba4db1
📥 Commits
Reviewing files that changed from the base of the PR and between 41710431d6349d6eb2e76ad48fc9fbed5014e370 and f3346d4a376531044b43424837ad3eedab1ddd6d.
📒 Files selected for processing (3)
src/main/linear/team-member-order.tssrc/main/linear/teams.test.tssrc/main/linear/teams.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/linear/teams.test.ts
Linear team members were returned in arbitrary API order, so the current user was buried in assignee pickers. Sort members in getTeamMembers with the workspace viewer first and the rest alphabetized, cached per workspace credential revision. All member consumers (drawer, composer, filters) inherit the order, including remote runtimes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Share one in-flight viewer promise per workspace credential revision so a cold cache doesn't issue redundant API calls when several dropdowns load at once, and assert the fallback warning in tests. Viewer-first ordering moves to team-member-order.ts to keep teams.ts within the line budget. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
localeCompare with the host default collator can order the same member list differently across macOS, Linux, and remote hosts. Use a pinned Intl.Collator plus an ordinal id tie-break so ordering is identical everywhere Orca runs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
840de6a to
094e6a4
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Rebased onto latest
async function readTeamMembersViewerFirst(teamId, workspaceId, fallbackWarning?) {
const entry = getClients(workspaceId)[0]
if (!entry) return []
const viewerId = await getCachedViewerId(entry)
const members = await readTeamResource(teamId, workspaceId, fetchAllTeamMembers, fallbackWarning)
return sortMembersViewerFirst(members, viewerId)
}so Re-verified after the rebase: |
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7d86798c-306d-4f89-8629-b227f2d45807
📒 Files selected for processing (3)
src/main/linear/team-member-order.tssrc/main/linear/teams.test.tssrc/main/linear/teams.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/linear/teams.test.ts
- src/main/linear/team-member-order.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.
| const entry = getClients(workspaceId)[0] | ||
| if (!entry) { | ||
| return [] | ||
| } | ||
| const viewerId = await getCachedViewerId(entry) | ||
| const members = await readTeamResource(teamId, workspaceId, fetchAllTeamMembers, fallbackWarning) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep the viewer lookup and member fetch on one client snapshot.
entry is resolved before getCachedViewerId, but readTeamResource resolves getClients(workspaceId)[0] again after the viewer lookup awaits. If credentials rotate between these calls, the member list can use the new credential while viewerId belongs to the old revision. The current viewer can then be sorted incorrectly. Pass entry into the member read, or revalidate the credential revision before sorting.
Summary
Linear assignee dropdowns (issue drawer/page properties, new-issue composer, and assignee filters) previously listed team members in arbitrary Linear API order, so the logged-in user was buried mid-list. Team members are now sorted with the current Linear user (workspace viewer) first, and the remaining members alphabetized by display name.
The sort happens in the main-process
getTeamMembers/getTeamMembersOrThrow, so every consumer — including remote/SSH runtimes, whose RPC path calls the same function on the token-owning host — inherits the order with no renderer or protocol changes. The viewer id is fetched once per workspace and cached keyed bycredentialRevision, so reconnecting as a different user refreshes it. A failed viewer lookup degrades to plain alphabetical order and never fails the members fetch.Screenshots
Assignee dropdown with the logged-in Linear user (andrew) pinned first after Unassigned:
Testing
pnpm lintpnpm typecheckpnpm test— full suite; 2 pre-existing failures insrc/relay/agent-exec-handler.test.tsalso fail on unmodifiedmainin this local environment (env-dependent codex spawn expectations), unrelated to this changepnpm buildAI Review Report
Reviewed with Claude Code and cross-checked by a second agent (Codex):
linear.teamMembers) executes the samegetTeamMemberson the remote host, so remote runtimes get identical ordering; no protocol/capability changes (result shape unchanged).client.viewercall per workspace per credential revision (cached in-memory; fetched outside the members semaphore window to avoid nested-acquire starvation). Sorting is O(n log n) on ≤100-member pages already held in memory.Security Audit
viewer.idcomes from the authenticated Linear SDK client already in use.nulland logs via the existingconsole.warnconvention without including token material.Notes
useIssueMetadata), so the union is not globally alphabetical; the viewer still surfaces at the top of the first team block after dedup.