Seperate Exec membership from Org admin #343
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds an SQS-driven Exec Council sync handler and payload, replaces dynamic per-org lead queries with parallel ConsistentRead queries, renames ExecGithubTeam → OrgAdminGithubParentTeam in config, makes GitHub team Changes
Sequence Diagram(s)sequenceDiagram
participant Route as Org Route
participant SQS as SQS
participant Handler as syncExecCouncilHandler
participant DDB as DynamoDB
participant Entra as Entra ID
Route->>SQS: Queue SyncExecCouncil
SQS->>Handler: Dequeue & invoke
Handler->>DDB: Parallel per-org ConsistentRead queries
DDB-->>Handler: Voting leads array (incl. nonVotingMember)
Handler->>Entra: Fetch current group members
Entra-->>Handler: Current members list
Handler->>Handler: Compute adds / removes
alt add members
Handler->>Entra: Add missing members (iterative)
end
alt remove members
Handler->>Entra: Remove non-voting members (iterative)
end
Handler-->>SQS: Log summary / metrics
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration 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 📒 Files selected for processing (1)
Comment |
💰 Infracost reportMonthly estimate generatedThis comment will be updated when code changes. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/api/functions/github.ts (1)
6-15: Make parent_team_id conditional when parentTeamId is undefined.Now that parentTeamId is optional, avoid sending
parent_team_id: undefinedwhich can 4xx.- const response = await octokit.request("POST /orgs/{org}/teams", { - org: orgId, - name, - description: `[Managed by Core API]${description ? ` ${description}` : ""}`, - privacy: privacy || "closed", - notification_setting: "notifications_enabled", - parent_team_id: parentTeamId, - }); + const body: Record<string, any> = { + org: orgId, + name, + description: `[Managed by Core API]${description ? ` ${description}` : ""}`, + privacy: privacy || "closed", + notification_setting: "notifications_enabled", + }; + if (parentTeamId != null) body.parent_team_id = parentTeamId; + const response = await octokit.request("POST /orgs/{org}/teams", body);
🧹 Nitpick comments (2)
src/api/sqs/handlers/syncExecCouncil.ts (1)
1-20: import/extensions lints on “.js” paths.If ESM with NodeNext, keep “.js” and relax ESLint; otherwise drop extensions.
src/api/functions/organizations.ts (1)
588-630: Prefer GetItem (or BatchGetItem) over Query for exact PK+SK; lower latency/RCUsYou’re querying by full primary key (partition + sort). GetItem with ConsistentRead is the direct, cheaper call and simplifies the code. BatchGetItem could fetch all org keys in one request if desired.
Add import (outside this hunk):
+import { GetItemCommand } from "@aws-sdk/client-dynamodb";Refactor body:
for (const orgName of AllOrganizationNameList) { - const leadsQuery = new QueryCommand({ - TableName: genericConfig.SigInfoTableName, - KeyConditionExpression: "primaryKey = :leadName AND entryId = :username", - ExpressionAttributeValues: { - ":leadName": { S: `LEAD#${orgName}` }, - ":username": { S: username }, - }, - ConsistentRead: true, - }); - - try { - const responseMarshall = await dynamoClient.send(leadsQuery); - if (responseMarshall.Items && responseMarshall.Items.length > 0) { - const lead = unmarshall(responseMarshall.Items[0]); - // If they're a lead and not a non-voting member, they should be in exec - if (!lead.nonVotingMember) { - return true; - } - } - } catch (e) { + try { + const res = await dynamoClient.send( + new GetItemCommand({ + TableName: genericConfig.SigInfoTableName, + Key: marshall({ + primaryKey: `LEAD#${orgName}`, + entryId: username, + }), + ConsistentRead: true, + ProjectionExpression: "nonVotingMember", + }), + ); + if (res.Item) { + const lead = unmarshall(res.Item); + if (!lead.nonVotingMember) return true; + } + } catch (e) { if (e instanceof BaseError) { throw e; } logger.error(e); throw new DatabaseFetchError({ message: `Failed to check lead status for ${username} in org ${orgName}.`, }); } }Optional: replace the loop with a single BatchGetItem across all orgs for a single network round trip.
📜 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
📒 Files selected for processing (9)
src/api/functions/github.ts(1 hunks)src/api/functions/organizations.ts(1 hunks)src/api/routes/organizations.ts(4 hunks)src/api/sqs/handlers/createOrgGithubTeam.ts(1 hunks)src/api/sqs/handlers/index.ts(1 hunks)src/api/sqs/handlers/syncExecCouncil.ts(1 hunks)src/api/sqs/index.ts(2 hunks)src/common/config.ts(3 hunks)src/common/types/sqsMessage.ts(3 hunks)
🧰 Additional context used
🪛 ESLint
src/api/sqs/handlers/index.ts
[error] 7-7: Unexpected use of file extension "js" for "./syncExecCouncil.js"
(import/extensions)
src/api/sqs/handlers/syncExecCouncil.ts
[error] 1-1: Resolve error: EACCES: permission denied, open '/ySwLhLIcEr'
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] 1-1: Unexpected use of file extension "js" for "common/types/sqsMessage.js"
(import/extensions)
[error] 6-6: Unexpected use of file extension "js" for "../index.js"
(import/extensions)
[error] 12-12: Unexpected use of file extension "js" for "common/config.js"
(import/extensions)
[error] 13-13: Unexpected use of file extension "js" for "api/functions/organizations.js"
(import/extensions)
[error] 18-18: Unexpected use of file extension "js" for "api/functions/entraId.js"
(import/extensions)
[error] 20-20: Unexpected use of file extension "js" for "common/types/iam.js"
(import/extensions)
src/common/types/sqsMessage.ts
[error] 83-83: Replace ·z.object({}) with ⏎····z.object({}),
(prettier/prettier)
[error] 102-102: Insert ,
(prettier/prettier)
src/api/routes/organizations.ts
[error] 1-1: Resolve error: EACCES: permission denied, open '/iDtwctVzgQ'
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] 42-42: Unexpected use of file extension "js" for "api/functions/entraId.js"
(import/extensions)
[error] 44-44: Unexpected use of file extension "js" for "api/functions/sts.js"
(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 (7)
src/common/config.ts (2)
144-146: Confirm parent team IDs are correct in both envs.If the parent team is ever not set for an env, callers should omit the parent instead of passing an undefined number. See suggested guard in github.ts comment.
Also applies to: 176-177
33-36: Verified: no stale ExecGithubTeam references found.The rename to
OrgAdminGithubParentTeamis complete—no orphaned references to the old key exist in the codebase.src/common/types/sqsMessage.ts (1)
11-12: Enum addition is fine; ensure queue-level gating if needed.If SyncExecCouncil should be limited to a specific SQS queue, add it to restrictedQueues in src/api/sqs/index.ts.
src/api/sqs/handlers/createOrgGithubTeam.ts (1)
92-96: Switch to OrgAdminGithubParentTeam looks correct.No behavior change. Ensure this value exists for all envs; if an env intentionally omits a parent, the GitHub request should skip parent_team_id (see github.ts suggestion).
src/api/sqs/index.ts (1)
21-23: Wiring LGTM.Dispatch table correctly maps CreateOrgGithubTeam and SyncExecCouncil. Consider queue gating for SyncExecCouncil if appropriate (restrictedQueues).
Also applies to: 45-46
src/api/routes/organizations.ts (2)
487-488: LGTM: Clear documentation of architectural change.The comment effectively explains that exec council membership management has been moved from dynamic membership rules to an SQS-based sync handler, making the code easier to understand for future maintainers.
565-577: Exec council sync implementation is correct and properly handles the sync workflow.The unconditional queuing ensures voting member changes are consistently reflected in the exec council. The handler correctly queries the source of truth (DynamoDB for voting leads, Entra ID for current members), performs selective adds/removes, and handles individual operation failures gracefully.
| export async function getAllVotingLeads({ | ||
| dynamoClient, | ||
| includeGroupIds, | ||
| logger, | ||
| }: { | ||
| dynamoClient: DynamoDBClient; | ||
| includeGroupIds?: string[]; | ||
| }): Promise<string | null> { | ||
| const command = new ScanCommand({ | ||
| TableName: genericConfig.SigInfoTableName, | ||
| IndexName: "LeadsGroupIdIndex", | ||
| logger: ValidLoggers; | ||
| }): Promise< | ||
| Array<{ username: string; org: string; name: string; title: string }> | ||
| > { | ||
| // Query all organizations in parallel for better performance | ||
| const queryPromises = AllOrganizationNameList.map(async (orgName) => { | ||
| const leadsQuery = new QueryCommand({ | ||
| TableName: genericConfig.SigInfoTableName, | ||
| KeyConditionExpression: "primaryKey = :leadName", | ||
| ExpressionAttributeValues: { | ||
| ":leadName": { S: `LEAD#${orgName}` }, | ||
| }, | ||
| ConsistentRead: true, | ||
| }); | ||
|
|
||
| try { | ||
| const responseMarshall = await dynamoClient.send(leadsQuery); | ||
| if (responseMarshall.Items) { | ||
| return responseMarshall.Items.map((x) => unmarshall(x)) | ||
| .filter((x) => x.username && !x.nonVotingMember) | ||
| .map((x) => ({ | ||
| username: x.username as string, | ||
| org: orgName, | ||
| name: x.name as string, | ||
| title: x.title as string, | ||
| })); | ||
| } | ||
| return []; | ||
| } catch (e) { | ||
| if (e instanceof BaseError) { | ||
| throw e; | ||
| } | ||
| logger.error(e); | ||
| throw new DatabaseFetchError({ | ||
| message: `Failed to get leads for org ${orgName}.`, | ||
| }); | ||
| } | ||
| }); | ||
| const results = await dynamoClient.send(command); | ||
| if (!results || !results.Items || results.Items.length === 0) { | ||
| return null; | ||
| } | ||
| const entries = results.Items.map((x) => unmarshall(x)) as { | ||
| primaryKey: string; | ||
| leadsEntraGroupId: string; | ||
| }[]; | ||
| const groupIds = entries | ||
| .filter((x) => x.primaryKey.startsWith("DEFINE#")) | ||
| .map((x) => x.leadsEntraGroupId); | ||
|
|
||
| if (groupIds.length === 0) { | ||
| return null; | ||
|
|
||
| const results = await Promise.all(queryPromises); | ||
| return results.flat(); | ||
| } |
There was a problem hiding this comment.
Fix return type for title (can be undefined) + trim payload + optional resiliency
- Title may be missing on some lead items; current return type promises string. Make it optional to avoid misleading consumers and potential runtime issues. Also project only needed attributes to cut payload. Optionally, return partial results if one org read fails.
Apply:
export async function getAllVotingLeads({
dynamoClient,
logger,
}: {
dynamoClient: DynamoDBClient;
- logger: ValidLoggers;
-}): Promise<
- Array<{ username: string; org: string; name: string; title: string }>
-> {
+ logger: ValidLoggers;
+}): Promise<
+ Array<{ username: string; org: string; name: string; title?: string }>
+> {
// Query all organizations in parallel for better performance
const queryPromises = AllOrganizationNameList.map(async (orgName) => {
const leadsQuery = new QueryCommand({
TableName: genericConfig.SigInfoTableName,
KeyConditionExpression: "primaryKey = :leadName",
ExpressionAttributeValues: {
":leadName": { S: `LEAD#${orgName}` },
},
+ ProjectionExpression: "#u, #n, title, nonVotingMember",
+ ExpressionAttributeNames: { "#u": "username", "#n": "name" },
ConsistentRead: true,
});
try {
const responseMarshall = await dynamoClient.send(leadsQuery);
if (responseMarshall.Items) {
return responseMarshall.Items.map((x) => unmarshall(x))
.filter((x) => x.username && !x.nonVotingMember)
.map((x) => ({
username: x.username as string,
org: orgName,
- name: x.name as string,
- title: x.title as string,
+ name: x.name as string,
+ title: x.title,
}));
}
return [];
} catch (e) {
if (e instanceof BaseError) {
throw e;
}
logger.error(e);
throw new DatabaseFetchError({
message: `Failed to get leads for org ${orgName}.`,
});
}
});
- const results = await Promise.all(queryPromises);
- return results.flat();
+ // Option B (resiliency): tolerate one-org failures and return partial results
+ // const settled = await Promise.allSettled(queryPromises);
+ // const results: Array<{ username: string; org: string; name: string; title?: string }> = [];
+ // for (const s of settled) {
+ // if (s.status === "fulfilled") results.push(...s.value);
+ // else logger.error(s.reason);
+ // }
+ // return results;
+ const results = await Promise.all(queryPromises);
+ return results.flat();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function getAllVotingLeads({ | |
| dynamoClient, | |
| includeGroupIds, | |
| logger, | |
| }: { | |
| dynamoClient: DynamoDBClient; | |
| includeGroupIds?: string[]; | |
| }): Promise<string | null> { | |
| const command = new ScanCommand({ | |
| TableName: genericConfig.SigInfoTableName, | |
| IndexName: "LeadsGroupIdIndex", | |
| logger: ValidLoggers; | |
| }): Promise< | |
| Array<{ username: string; org: string; name: string; title: string }> | |
| > { | |
| // Query all organizations in parallel for better performance | |
| const queryPromises = AllOrganizationNameList.map(async (orgName) => { | |
| const leadsQuery = new QueryCommand({ | |
| TableName: genericConfig.SigInfoTableName, | |
| KeyConditionExpression: "primaryKey = :leadName", | |
| ExpressionAttributeValues: { | |
| ":leadName": { S: `LEAD#${orgName}` }, | |
| }, | |
| ConsistentRead: true, | |
| }); | |
| try { | |
| const responseMarshall = await dynamoClient.send(leadsQuery); | |
| if (responseMarshall.Items) { | |
| return responseMarshall.Items.map((x) => unmarshall(x)) | |
| .filter((x) => x.username && !x.nonVotingMember) | |
| .map((x) => ({ | |
| username: x.username as string, | |
| org: orgName, | |
| name: x.name as string, | |
| title: x.title as string, | |
| })); | |
| } | |
| return []; | |
| } catch (e) { | |
| if (e instanceof BaseError) { | |
| throw e; | |
| } | |
| logger.error(e); | |
| throw new DatabaseFetchError({ | |
| message: `Failed to get leads for org ${orgName}.`, | |
| }); | |
| } | |
| }); | |
| const results = await dynamoClient.send(command); | |
| if (!results || !results.Items || results.Items.length === 0) { | |
| return null; | |
| } | |
| const entries = results.Items.map((x) => unmarshall(x)) as { | |
| primaryKey: string; | |
| leadsEntraGroupId: string; | |
| }[]; | |
| const groupIds = entries | |
| .filter((x) => x.primaryKey.startsWith("DEFINE#")) | |
| .map((x) => x.leadsEntraGroupId); | |
| if (groupIds.length === 0) { | |
| return null; | |
| const results = await Promise.all(queryPromises); | |
| return results.flat(); | |
| } | |
| export async function getAllVotingLeads({ | |
| dynamoClient, | |
| logger, | |
| }: { | |
| dynamoClient: DynamoDBClient; | |
| logger: ValidLoggers; | |
| }): Promise< | |
| Array<{ username: string; org: string; name: string; title?: string }> | |
| > { | |
| // Query all organizations in parallel for better performance | |
| const queryPromises = AllOrganizationNameList.map(async (orgName) => { | |
| const leadsQuery = new QueryCommand({ | |
| TableName: genericConfig.SigInfoTableName, | |
| KeyConditionExpression: "primaryKey = :leadName", | |
| ExpressionAttributeValues: { | |
| ":leadName": { S: `LEAD#${orgName}` }, | |
| }, | |
| ProjectionExpression: "#u, #n, title, nonVotingMember", | |
| ExpressionAttributeNames: { "#u": "username", "#n": "name" }, | |
| ConsistentRead: true, | |
| }); | |
| try { | |
| const responseMarshall = await dynamoClient.send(leadsQuery); | |
| if (responseMarshall.Items) { | |
| return responseMarshall.Items.map((x) => unmarshall(x)) | |
| .filter((x) => x.username && !x.nonVotingMember) | |
| .map((x) => ({ | |
| username: x.username as string, | |
| org: orgName, | |
| name: x.name as string, | |
| title: x.title, | |
| })); | |
| } | |
| return []; | |
| } catch (e) { | |
| if (e instanceof BaseError) { | |
| throw e; | |
| } | |
| logger.error(e); | |
| throw new DatabaseFetchError({ | |
| message: `Failed to get leads for org ${orgName}.`, | |
| }); | |
| } | |
| }); | |
| // Option B (resiliency): tolerate one-org failures and return partial results | |
| // const settled = await Promise.allSettled(queryPromises); | |
| // const results: Array<{ username: string; org: string; name: string; title?: string }> = []; | |
| // for (const s of settled) { | |
| // if (s.status === "fulfilled") results.push(...s.value); | |
| // else logger.error(s.reason); | |
| // } | |
| // return results; | |
| const results = await Promise.all(queryPromises); | |
| return results.flat(); | |
| } |
🤖 Prompt for AI Agents
In src/api/functions/organizations.ts around lines 533 to 579, the function
currently declares title as string, fetches full items, and throws if any org
query fails; change the return type so title is optional (title?: string),
restrict the DynamoDB query to only the needed attributes using
ProjectionExpression (e.g., username, name, title) and ExpressionAttributeNames
to reduce payload, and make the per-org catch block resilient by logging the
error and returning an empty array for that org instead of throwing (still
rethrow BaseError). Ensure callers/platform types are updated to accept title as
possibly undefined.
| import { AllOrganizationNameList, getOrgByName } from "@acm-uiuc/js-shared"; | ||
| import rateLimiter from "api/plugins/rateLimiter.js"; | ||
| import { withRoles, withTags } from "api/components/index.js"; | ||
| import { z } from "zod/v4"; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify Zod v4 compatibility across the codebase.
The import has been updated to use zod/v4, which introduces breaking changes. While the usage in this file appears compatible, ensure that other files using Zod schemas have been reviewed for:
- Default value handling changes (
.default()now short-circuits onundefined) - Number validation changes (no infinite values,
.int()only allows safe integers) - Record schema changes (now requires both key and value schemas)
- Function schema redesign
- Enum handling changes
🏁 Script executed:
#!/bin/bash
# Description: Search for Zod patterns that may be affected by v4 breaking changes
echo "=== Checking for .default() usage with transforms ==="
rg -n "\.transform\(" --type=ts -A 5 | rg "\.default\("
echo -e "\n=== Checking for z.record() without explicit schemas ==="
rg -n "z\.record\([^,)]*\)" --type=ts
echo -e "\n=== Checking for z.function() usage ==="
rg -n "z\.function\(" --type=ts
echo -e "\n=== Checking for number validation with potential infinity ==="
rg -n "z\.number\(\)" --type=ts -A 2 | rg -i "infinite|infinity"Length of output: 309
🏁 Script executed:
#!/bin/bash
# Get the full line from events.ts for the z.record() call
echo "=== Full z.record() line from events.ts ==="
sed -n '8p' src/common/types/events.ts
echo -e "\n=== Verify z.record() context (lines 5-12) ==="
sed -n '5,12p' src/common/types/events.ts
echo -e "\n=== Check all Zod version imports in the codebase ==="
rg -n "from\s+[\"']zod" --type=ts
echo -e "\n=== Check for .merge() chains or other schema composition ==="
rg -n "\.merge\(" --type=ts | head -20
echo -e "\n=== Check for .pick() or .omit() usage ==="
rg -n "\.pick\(|\.omit\(" --type=ts | head -20Length of output: 3811
🏁 Script executed:
#!/bin/bash
# Check if api/functions/organizations.ts is related to the reviewed file
echo "=== Check what's imported from api/functions/organizations.ts ==="
rg -n "from.*api/functions/organizations" --type=ts
echo -e "\n=== Check what api/functions/organizations.ts exports ==="
head -50 src/api/functions/organizations.ts | grep -E "export|^import"
echo -e "\n=== Check if api/functions/organizations.ts was modified in this PR ==="
rg -n "import.*organizations" src/api/routes/organizations.ts
echo -e "\n=== Verify Zod import resolution - check if 'zod' resolves to v4 ==="
rg -n "\"zod\"[^/]" --type=ts | head -10Length of output: 1825
Update src/api/functions/organizations.ts to use Zod v4.
The reviewed file imports from api/functions/organizations.ts, which still uses import z from "zod" instead of import z from "zod/v4". This creates a Zod version mismatch that can cause validation and type compatibility issues. Update line 20 of src/api/functions/organizations.ts to:
import z from "zod/v4";Additionally, verify all other files importing from this module are compatible with the updated import.
🤖 Prompt for AI Agents
In src/api/functions/organizations.ts around line 20, the file currently imports
Zod using the old path; change the import to use Zod v4 by replacing the
existing import with: import z from "zod/v4"; then run a quick grep across the
repo for imports of this module and ensure callers remain compatible with the v4
export (update any imports or type usages if necessary) and run the test/compile
step to confirm no type or runtime errors.
| export { sendSaleEmailHandler } from "./sendSaleEmailHandler.js"; | ||
| export { emailNotificationsHandler } from "./emailNotifications.js"; | ||
| export { createOrgGithubTeamHandler } from "./createOrgGithubTeam.js"; | ||
| export { syncExecCouncilHandler } from "./syncExecCouncil.js"; |
There was a problem hiding this comment.
🧩 Analysis chain
ESLint import/extensions warning.
If using TS + ESM with NodeNext and emitted .js paths, keep the extension and relax the rule; otherwise drop the extension for local exports.
🏁 Script executed:
#!/bin/bash
# Show existing pattern in this repo
rg -nP '^export\s+\{.*\}\s+from\s+"\.\/.*\.js";' src/api/sqs/handlers | wc -lLength of output: 1
---
🏁 Script executed:
```shell
#!/bin/bash
# Check tsconfig.json for module resolution strategy
cat tsconfig.json 2>/dev/null | grep -A5 -B5 "moduleResolution\|module"
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check for ESLint configuration files and import/extensions rule
find . -maxdepth 2 -type f \( -name ".eslintrc*" -o -name "eslint.config.*" \) 2>/dev/null | head -5Length of output: 19
🏁 Script executed:
#!/bin/bash
# Check package.json for ESLint config and import/extensions rule mention
cat package.json 2>/dev/null | grep -A20 '"eslintConfig"' | head -30Length of output: 0
🏁 Script executed:
#!/bin/bash
# Show actual exports with .js in src/api/sqs/handlers to confirm pattern
rg -nP '^export\s+\{.*\}\s+from\s+"\.\/.*\.js";' src/api/sqs/handlersLength of output: 680
---
🏁 Script executed:
```shell
#!/bin/bash
# Read eslint.config.mjs to see import/extensions rule
cat eslint.config.mjs | head -100
Length of output: 2565
🏁 Script executed:
#!/bin/bash
# Read full tsconfig.json
cat tsconfig.jsonLength of output: 45
🏁 Script executed:
#!/bin/bash
# Check if .js extensions are used elsewhere in codebase (not just handlers)
rg -nP '^export\s+\{.*\}\s+from\s+"\.\/.*\.js";' --type=ts --max-count=20 | head -20Length of output: 680
Remove .js extensions from all export statements—they violate the ESLint rule.
The project's ESLint configuration explicitly forbids .js extensions on local imports and exports ("import/extensions": ["error", "ignorePackages", { js: "never", ... }] at eslint.config.mjs). All 7 exports at lines 1–7 must drop the .js extension to comply:
export { syncExecCouncilHandler } from "./syncExecCouncil";
🧰 Tools
🪛 ESLint
[error] 7-7: Unexpected use of file extension "js" for "./syncExecCouncil.js"
(import/extensions)
🤖 Prompt for AI Agents
In src/api/sqs/handlers/index.ts around line 7, the export statement includes a
.js extension which violates the project's ESLint import/extensions rule; remove
the .js extension from this and the other local export statements so they read
e.g. export { syncExecCouncilHandler } from "./syncExecCouncil" and repeat for
all seven exports to comply with the lint rule.
| // Convert to set of usernames (without @illinois.edu) | ||
| const votingLeadUsernames = new Set( | ||
| votingLeads.map((lead) => lead.username), | ||
| ); | ||
|
|
||
| logger.info( | ||
| `Found ${votingLeadUsernames.size} voting leads across all organizations.`, | ||
| ); | ||
|
|
||
| // Get current exec council members from Entra ID | ||
| const currentMembers = await listGroupMembers( | ||
| entraIdToken, | ||
| execCouncilGroup, | ||
| ); | ||
|
|
||
| // Convert to set of emails | ||
| const currentMemberEmails = new Set( | ||
| currentMembers | ||
| .map((member) => member.email) | ||
| .filter((email) => email && email.endsWith("@illinois.edu")), | ||
| ); | ||
|
|
||
| logger.info( | ||
| `Current exec council has ${currentMemberEmails.size} members from @illinois.edu domain.`, | ||
| ); | ||
|
|
||
| // Determine who to add and who to remove | ||
| const toAdd = Array.from(votingLeadUsernames).filter( | ||
| (username) => !currentMemberEmails.has(username), | ||
| ); | ||
| const toRemove = Array.from(currentMemberEmails).filter( | ||
| (email) => !votingLeadUsernames.has(email), | ||
| ); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Normalize identifiers: comparing usernames to emails will produce incorrect add/remove sets.
You build a Set of usernames then compare against email addresses, leading to false diffs and unintended membership churn.
- // Convert to set of usernames (without @illinois.edu)
- const votingLeadUsernames = new Set(
- votingLeads.map((lead) => lead.username),
- );
+ // Normalize voting leads to lowercased @illinois.edu emails
+ const EMAIL_DOMAIN = "@illinois.edu";
+ const votingLeadEmails = new Set(
+ votingLeads
+ .map((lead) => lead.username?.toLowerCase())
+ .filter((u): u is string => !!u && u.length > 0)
+ .map((u) => (u.includes("@") ? u : `${u}${EMAIL_DOMAIN}`))
+ );
@@
- const currentMemberEmails = new Set(
- currentMembers
- .map((member) => member.email)
- .filter((email) => email && email.endsWith("@illinois.edu")),
- );
+ const currentMemberEmails = new Set(
+ currentMembers
+ .map((member) => member.email?.toLowerCase())
+ .filter((email): email is string => !!email && email.endsWith("@illinois.edu")),
+ );
@@
- const toAdd = Array.from(votingLeadUsernames).filter(
- (username) => !currentMemberEmails.has(username),
- );
- const toRemove = Array.from(currentMemberEmails).filter(
- (email) => !votingLeadUsernames.has(email),
- );
+ const toAdd = Array.from(votingLeadEmails).filter(
+ (email) => !currentMemberEmails.has(email),
+ );
+ const toRemove = Array.from(currentMemberEmails).filter(
+ (email) => !votingLeadEmails.has(email),
+ );
@@
- for (const username of toAdd) {
+ for (const email of toAdd) {
try {
- logger.info(`Adding ${username} to exec council...`);
+ logger.info(`Adding ${email} to exec council...`);
await modifyGroup(
entraIdToken,
- username,
+ email,
execCouncilGroup,
EntraGroupActions.ADD,
dynamo,
);
- logger.info(`Successfully added ${username} to exec council.`);
+ logger.info(`Successfully added ${email} to exec council.`);
} catch (error) {
logger.error(
error,
- `Failed to add ${username} to exec council. Continuing with other members...`,
+ `Failed to add ${email} to exec council. Continuing with other members...`,
);
}
}🏁 Script executed:
#!/bin/bash
# Inspect getAllVotingLeads return shape/usages to confirm username semantics
rg -nP --type=ts -C2 '\bgetAllVotingLeads\s*\('
rg -nP --type=ts -C2 '\busername\b' api/functions | head -n 50Length of output: 731
I need to gather more information about the return type of getAllVotingLeads and the modifyGroup function signature to verify the review comment's claim. Let me inspect those now.
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the full implementation of getAllVotingLeads to see return type
rg -nP --type=ts -A30 'export async function getAllVotingLeads' src/api/functions/organizations.ts
# Get modifyGroup function signature and implementation
rg -nP --type=ts -A10 'function modifyGroup|export.*modifyGroup' src/
Length of output: 1857
Now I need to see the complete implementation in syncExecCouncil.ts to verify how usernames are being passed to modifyGroup. Let me check the full code around the add/remove loops:
---
🏁 Script executed:
```shell
#!/bin/bash
# Get the complete implementation of the sync handler, particularly the toAdd/toRemove loops
rg -nP --type=ts -A20 'for \(const username of toAdd\)|for \(const.*of toAdd\)' src/api/sqs/handlers/syncExecCouncil.ts
Length of output: 686
Normalize identifiers: comparing usernames to emails will produce incorrect add/remove sets.
You build a Set of usernames then compare against email addresses, leading to false diffs and unintended membership churn. The modifyGroup function expects an email (line 262 of src/api/functions/entraId.ts) and validates it ends with @illinois.edu (line 268), but the code passes raw usernames without domain.
- // Convert to set of usernames (without @illinois.edu)
- const votingLeadUsernames = new Set(
- votingLeads.map((lead) => lead.username),
- );
+ // Normalize voting leads to lowercased @illinois.edu emails
+ const EMAIL_DOMAIN = "@illinois.edu";
+ const votingLeadEmails = new Set(
+ votingLeads
+ .map((lead) => lead.username?.toLowerCase())
+ .filter((u): u is string => !!u && u.length > 0)
+ .map((u) => (u.includes("@") ? u : `${u}${EMAIL_DOMAIN}`))
+ );
@@
- const currentMemberEmails = new Set(
- currentMembers
- .map((member) => member.email)
- .filter((email) => email && email.endsWith("@illinois.edu")),
- );
+ const currentMemberEmails = new Set(
+ currentMembers
+ .map((member) => member.email?.toLowerCase())
+ .filter((email): email is string => !!email && email.endsWith("@illinois.edu")),
+ );
@@
- const toAdd = Array.from(votingLeadUsernames).filter(
- (username) => !currentMemberEmails.has(username),
- );
- const toRemove = Array.from(currentMemberEmails).filter(
- (email) => !votingLeadUsernames.has(email),
- );
+ const toAdd = Array.from(votingLeadEmails).filter(
+ (email) => !currentMemberEmails.has(email),
+ );
+ const toRemove = Array.from(currentMemberEmails).filter(
+ (email) => !votingLeadEmails.has(email),
+ );
@@
- for (const username of toAdd) {
+ for (const email of toAdd) {
try {
- logger.info(`Adding ${username} to exec council...`);
+ logger.info(`Adding ${email} to exec council...`);
await modifyGroup(
entraIdToken,
- username,
+ email,
execCouncilGroup,
EntraGroupActions.ADD,
dynamo,
);
- logger.info(`Successfully added ${username} to exec council.`);
+ logger.info(`Successfully added ${email} to exec council.`);
} catch (error) {
logger.error(
error,
- `Failed to add ${username} to exec council. Continuing with other members...`,
+ `Failed to add ${email} to exec council. Continuing with other members...`,
);
}
}🤖 Prompt for AI Agents
In src/api/sqs/handlers/syncExecCouncil.ts around lines 58 to 91, the code
builds a Set of bare usernames and compares it to a Set of @illinois.edu emails
which produces incorrect add/remove lists; change the normalization to use the
same identifier for both sides (emails): convert votingLeads' usernames into
full emails (append @illinois.edu, validate/trim/lowercase the username first)
before creating votingLeadEmails Set, ensure currentMemberEmails are lowercased
and filtered for the @illinois.edu domain, then compute toAdd and toRemove by
comparing these email Sets so the values passed to modifyGroup are valid
@illinois.edu addresses.
| ), | ||
| [AvailableSQSFunctions.SyncExecCouncil]: createSQSSchema( | ||
| AvailableSQSFunctions.SyncExecCouncil, z.object({}) | ||
| ) |
There was a problem hiding this comment.
Fix Prettier violations and add trailing comma in union.
Tidy formatting to satisfy prettier/prettier.
[AvailableSQSFunctions.CreateOrgGithubTeam]: createSQSSchema(
AvailableSQSFunctions.CreateOrgGithubTeam, z.object({
orgName: z.enum(AllOrganizationNameList),
githubTeamName: z.string().min(1),
githubTeamDescription: z.string().min(1)
})
- ),
- [AvailableSQSFunctions.SyncExecCouncil]: createSQSSchema(
- AvailableSQSFunctions.SyncExecCouncil, z.object({})
- )
+ ),
+ [AvailableSQSFunctions.SyncExecCouncil]: createSQSSchema(
+ AvailableSQSFunctions.SyncExecCouncil,
+ z.object({}),
+ ),
} as const;
...
export const sqsPayloadSchema = z.discriminatedUnion("function", [
sqsPayloadSchemas[AvailableSQSFunctions.Ping],
sqsPayloadSchemas[AvailableSQSFunctions.EmailMembershipPass],
sqsPayloadSchemas[AvailableSQSFunctions.ProvisionNewMember],
sqsPayloadSchemas[AvailableSQSFunctions.SendSaleEmail],
sqsPayloadSchemas[AvailableSQSFunctions.EmailNotifications],
- sqsPayloadSchemas[AvailableSQSFunctions.CreateOrgGithubTeam],
- sqsPayloadSchemas[AvailableSQSFunctions.SyncExecCouncil]
+ sqsPayloadSchemas[AvailableSQSFunctions.CreateOrgGithubTeam],
+ sqsPayloadSchemas[AvailableSQSFunctions.SyncExecCouncil],
] as
const);Also applies to: 101-103
🧰 Tools
🪛 ESLint
[error] 83-83: Replace ·z.object({}) with ⏎····z.object({}),
(prettier/prettier)
[error] 84-84: Insert ,
(prettier/prettier)
🤖 Prompt for AI Agents
In src/common/types/sqsMessage.ts around lines 81-84 (and also apply same fix to
101-103), the block defining the schema has Prettier violations and is missing a
trailing comma in the union; reformat the multi-line createSQSSchema call to
satisfy Prettier (proper indentation, line breaks) and add a trailing comma
after the last array entry/argument so the union and object literal end with a
comma; ensure the same formatting and trailing-comma fix is applied to lines
101-103 as well.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/api/sqs/handlers/syncExecCouncil.ts (1)
88-121: Identifier normalization issue persists from previous review.The username-to-email comparison bug identified in the previous review remains unaddressed. Bare usernames are compared against
@illinois.eduemails, causing incorrect add/remove operations and failures when passing usernames tomodifyGroup(which expects emails).Please apply the normalization fix proposed in the previous review comment.
📜 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
📒 Files selected for processing (1)
src/api/sqs/handlers/syncExecCouncil.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
src/api/sqs/handlers/syncExecCouncil.ts
[error] 1-1: Resolve error: EACCES: permission denied, open '/WDDlCXpPkc'
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] 1-1: Unexpected use of file extension "js" for "common/types/sqsMessage.js"
(import/extensions)
[error] 6-6: Unexpected use of file extension "js" for "../index.js"
(import/extensions)
[error] 13-13: Unexpected use of file extension "js" for "common/config.js"
(import/extensions)
[error] 14-14: Unexpected use of file extension "js" for "api/functions/organizations.js"
(import/extensions)
[error] 19-19: Unexpected use of file extension "js" for "api/functions/entraId.js"
(import/extensions)
[error] 21-21: Unexpected use of file extension "js" for "common/types/iam.js"
(import/extensions)
[error] 22-22: Unexpected use of file extension "js" for "api/functions/sts.js"
(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: Run Unit Tests
- GitHub Check: Build Application
🔇 Additional comments (1)
src/api/sqs/handlers/syncExecCouncil.ts (1)
127-144: Good error handling for individual member additions.The per-member try-catch pattern allows the sync to continue even if individual additions fail, which is appropriate for bulk operations.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/ui/pages/organization/ManageOrganizationForm.tsx (1)
573-573: Good clarity improvement with organizationId interpolation.The interpolation makes it explicit which organization's voting rights are being discussed, aligning well with the PR's objective to separate exec membership from org admin privileges.
If
organizationIdis a technical identifier (UUID or hash) rather than a human-readable slug, consider using an organization display name for better UX. However, for admin interfaces, showing the org ID is often appropriate and helpful.
📜 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
📒 Files selected for processing (1)
src/ui/pages/organization/ManageOrganizationForm.tsx(2 hunks)
⏰ 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 (1)
src/ui/pages/organization/ManageOrganizationForm.tsx (1)
590-591: LGTM! Clear organization-specific messaging.The updated alert text properly clarifies that voting rights are organization-specific, consistent with the checkbox description change on line 573. The wording is clear and aligns well with the PR's objective.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/api/functions/organizations.ts (1)
539-585: Address the unresolved issues from previous review.The past review comment on this function remains unaddressed:
- Type safety:
titleis declared asstringbut can beundefined. Line 546 should declaretitle?: stringand line 568 should not cast tostring.- Payload optimization: Add
ProjectionExpressionto fetch only needed attributes (username,name,title,nonVotingMember) rather than full items.- Resiliency:
Promise.allon line 583 fails the entire operation if any single org query fails. Consider usingPromise.allSettledto tolerate partial failures.Apply this diff to address all three issues:
export async function getAllVotingLeads({ dynamoClient, logger, }: { dynamoClient: DynamoDBClient; logger: ValidLoggers; }): Promise< - Array<{ username: string; org: string; name: string; title: string }> + Array<{ username: string; org: string; name: string; title?: string }> > { // Query all organizations in parallel for better performance const queryPromises = AllOrganizationNameList.map(async (orgName) => { const leadsQuery = new QueryCommand({ TableName: genericConfig.SigInfoTableName, KeyConditionExpression: "primaryKey = :leadName", ExpressionAttributeValues: { ":leadName": { S: `LEAD#${orgName}` }, }, + ProjectionExpression: "#u, #n, title, nonVotingMember", + ExpressionAttributeNames: { "#u": "username", "#n": "name" }, ConsistentRead: true, }); try { const responseMarshall = await dynamoClient.send(leadsQuery); if (responseMarshall.Items) { return responseMarshall.Items.map((x) => unmarshall(x)) .filter((x) => x.username && !x.nonVotingMember) .map((x) => ({ username: x.username as string, org: orgName, name: x.name as string, - title: x.title as string, + title: x.title, })); } return []; } catch (e) { if (e instanceof BaseError) { throw e; } logger.error(e); - throw new DatabaseFetchError({ - message: `Failed to get leads for org ${orgName}.`, - }); + // Log error but continue with other orgs for resiliency + logger.error(`Failed to get leads for org ${orgName}, continuing with other orgs`); + return []; } }); const results = await Promise.all(queryPromises); return results.flat(); }Note: The resiliency change catches errors per-org and returns an empty array, allowing other orgs to succeed. If you prefer
Promise.allSettledas suggested in the past review, that's also valid.
🧹 Nitpick comments (1)
src/api/functions/organizations.ts (1)
587-636: Consider adding ProjectionExpression to reduce payload.The sequential query pattern with early return is appropriate for an existence check. However, you can optimize payload size by fetching only the
nonVotingMemberattribute.Apply this diff:
const leadsQuery = new QueryCommand({ TableName: genericConfig.SigInfoTableName, KeyConditionExpression: "primaryKey = :leadName AND entryId = :username", ExpressionAttributeValues: { ":leadName": { S: `LEAD#${orgName}` }, ":username": { S: username }, }, + ProjectionExpression: "nonVotingMember", ConsistentRead: true, });
📜 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
📒 Files selected for processing (1)
src/api/functions/organizations.ts(2 hunks)
⏰ 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: Run Unit Tests
- GitHub Check: Build Application
🔇 Additional comments (1)
src/api/functions/organizations.ts (1)
104-110: LGTM: Clean addition of nonVotingMember field.The implementation correctly defaults
nonVotingMembertofalsewhen undefined and includes explicit type annotations.
This allows us to have some people who are not exec representatives but do have org admin privileges.
Summary by CodeRabbit
New Features
Refactor
Style
Tests