Add assignee logic to issue create-edit-component#437
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughLoads team members for the issue create/edit dialog scoped to the selected team, exposes an "Assignees" checkbox UI, ensures Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant User as User
end
rect rgba(200,255,200,0.5)
participant UI as CreateEditDialog (client)
end
rect rgba(255,200,200,0.5)
participant API as Issues API
end
rect rgba(255,255,200,0.5)
participant DB as Server/DB
end
User->>UI: Open dialog
UI->>API: getUsersOnTeam(teamId) [if isOpen && effectiveTeam]
API->>DB: Query Users ⟵ join Permissions where roleId = teamId
DB-->>API: users rows
API-->>UI: usersData / error
UI->>UI: compute assigneesForTeam, safeAssigneeIds
User->>UI: toggle assignee checkbox (update form.assigneeIds)
User->>UI: Submit create/update/delete
UI->>API: create/update/delete issue (assigneeIds: safeAssigneeIds)
API->>DB: validate & persist issue (validate assignees belong to team)
DB-->>API: ack
API-->>UI: success / error
UI-->>User: show result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/blade/src/app/_components/issues/create-edit-dialog.tsx`:
- Around line 742-759: The Checkbox and Label for each assignee are not
programmatically associated, breaking keyboard/screen-reader behavior; give the
Checkbox a stable id (e.g. `assignee-${user.id}`) and set the Label to point to
that id (htmlFor or for prop depending on your Label component) so clicking the
name toggles the checkbox and the checkbox gets an accessible name; update the
JSX where Checkbox, formValues.assigneeIds and updateForm are used to pass the
id to Checkbox and set the Label's htmlFor to the same string.
- Around line 313-320: safeAssigneeIds is being computed from an empty
assigneeIdSet while the users/assignees query is still loading, which causes
existing assignees to be stripped; update the logic in the Create/Edit dialog so
you either (A) keep using formValues.assigneeIds until assigneesForTeam (or
usersQuery) is loaded/successful (e.g. only filter when assigneeIdSet.size > 0),
or (B) prevent submit/rendering until the assignees query succeeds; specifically
modify the safeAssigneeIds computation (which references assigneeIdSet,
assigneesForTeam, and formValues.assigneeIds) or add a guard tied to the
users/assignees query state so existing assignees are preserved until the
options are available.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 2f92d44d-c6f2-4c67-b62c-a41ae3b4212d
📒 Files selected for processing (2)
apps/blade/src/app/_components/issues/create-edit-dialog.tsxpackages/consts/src/issue.ts
DVidal1205
left a comment
There was a problem hiding this comment.
let's make a new endpoint here. currently, on account of raw users, we have about 2,000 rows in the production database. fetching all and processing on the frontend via client component requires all ~2,000 rows to make a round trip.
instead maybe add to the issues router a getUsersOnTeam procedure, that given an effective team id, just performs a join and passes the results back.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/blade/src/app/_components/issues/create-edit-dialog.tsx`:
- Around line 728-755: The assignee checkbox IDs are unscoped and can collide
across multiple dialog instances; update the assigneeCheckboxId generation
inside the assigneesForTeam.map so it includes the component's baseId (e.g.,
`${baseId}-assignee-${user.id}`) and use that id for both the Checkbox id prop
and the Label htmlFor prop to preserve the Label→Checkbox association for
keyboard and screen-reader users (update the code where assigneeCheckboxId is
defined and where Checkbox and Label use it).
In `@packages/api/src/routers/issues.ts`:
- Around line 115-155: The create/update paths must validate that any submitted
assigneeIds belong to the issue's team (the same invariant enforced by
getUsersOnTeam) before writing to IssuesToUsersAssignment; implement a helper
like validateAssigneesBelongToTeam(teamId, assigneeIds) that queries Permissions
(or the same join used in getUsersOnTeam) to ensure all assigneeIds are members,
then call it from createIssue (use incoming input.team) and updateIssue (use
input.team ?? existingIssue.team) and reject the request with a clear error if
any assignee is not on the team rather than inserting invalid rows.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 35048b49-01b7-4b2a-a374-4149bb0b421d
📒 Files selected for processing (2)
apps/blade/src/app/_components/issues/create-edit-dialog.tsxpackages/api/src/routers/issues.ts
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/utils/src/permissions.server.ts (1)
105-113: Batch nested assignee validation by team to reduce query fan-out.
validateIssueNodeAssigneescurrently performs one DB query per node. For deep/wide trees, this becomes many round-trips. Consider collecting assignees byteamfirst, then validating once per team.♻️ Suggested refactor
export const validateIssueNodeAssignees = async ( nodes: IssueAssigneeValidationNode[], ) => { - for (const node of nodes) { - await validateAssigneesBelongToTeam(node.team, node.assigneeIds); - if (node.children?.length) { - await validateIssueNodeAssignees(node.children); - } - } + const assigneesByTeam = new Map<string, Set<string>>(); + const stack = [...nodes]; + + while (stack.length > 0) { + const node = stack.pop()!; + if (node.assigneeIds?.length) { + const set = assigneesByTeam.get(node.team) ?? new Set<string>(); + for (const id of node.assigneeIds) set.add(id); + assigneesByTeam.set(node.team, set); + } + if (node.children?.length) stack.push(...node.children); + } + + for (const [teamId, ids] of assigneesByTeam) { + await validateAssigneesBelongToTeam(teamId, [...ids]); + } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utils/src/permissions.server.ts` around lines 105 - 113, validateIssueNodeAssignees currently calls validateAssigneesBelongToTeam per node causing many DB round-trips; refactor it to first traverse the tree of IssueAssigneeValidationNode (use validateIssueNodeAssignees as entry) and collect assigneeIds grouped by team into a Map<team, Set<assigneeId>> (include children recursively), then iterate that map and call validateAssigneesBelongToTeam once per team with the combined array of assigneeIds; keep the same async signature and handle empty sets by skipping calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/api/src/routers/issues.ts`:
- Around line 167-174: The assignee validations
(permissionsServer.validateAssigneesBelongToTeam and
permissionsServer.validateIssueNodeAssignees) are invoked outside
db.transaction, creating a TOCTOU risk; move these validations into the same
db.transaction block used for the issue inserts/updates so they run under the
same transactional context, and if needed add tx-aware overloads to
permissions.server.ts (e.g., validateAssigneesBelongToTeam(tx, team,
assigneeIds) and validateIssueNodeAssignees(tx, children)) so the permission
reads use the transaction connection rather than a separate non-transactional
query.
- Around line 373-377: The update path can change the issue team without
revalidating current assignees, leaving invalid assignees attached; when calling
permissionsServer.validateAssigneesBelongToTeam, if input.assigneeIds is
undefined but input.team differs from existingIssue.team, pass the
existingIssue.assigneeIds (or explicitly clear them) into
validateAssigneesBelongToTeam so the current assignments are revalidated against
the new assignmentTeamId (assignmentTeamId, input.assigneeIds,
existingIssue.assigneeIds, input.team and existingIssue.team are the relevant
symbols to check and adjust).
---
Nitpick comments:
In `@packages/utils/src/permissions.server.ts`:
- Around line 105-113: validateIssueNodeAssignees currently calls
validateAssigneesBelongToTeam per node causing many DB round-trips; refactor it
to first traverse the tree of IssueAssigneeValidationNode (use
validateIssueNodeAssignees as entry) and collect assigneeIds grouped by team
into a Map<team, Set<assigneeId>> (include children recursively), then iterate
that map and call validateAssigneesBelongToTeam once per team with the combined
array of assigneeIds; keep the same async signature and handle empty sets by
skipping calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: afc6395c-58c4-477c-8b0d-afdbd8309365
📒 Files selected for processing (2)
packages/api/src/routers/issues.tspackages/utils/src/permissions.server.ts
Why
create-edit-component had a missing feature.
What
When you select a team to assign an issue to, you get another menu which allows you to select certain members of that team.
Test Plan
Merge me twin.
Checklist
db:pushbefore mergingSummary by CodeRabbit
New Features
Bug Fixes