Merged
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:
📝 WalkthroughWalkthroughAdds an Issue domain: DB enums/tables and relations; new constants and permissions; DB client includes relations; a TRPC issues router (create/get/getAll/update/delete) with permission and visibility logic; and the router is exposed from the API root. Changes
Sequence DiagramsequenceDiagram
actor Client
participant API as "API / Issues Router"
participant Auth as "Auth / Permissions"
participant DB as "Database"
Client->>API: createIssue(payload)
API->>Auth: verify EDIT_ISSUES
Auth-->>API: allowed / denied
alt allowed
API->>DB: INSERT Issue
DB-->>API: Issue created
API->>DB: INSERT IssuesToTeamsVisibility / IssuesToUsersAssignment
DB-->>API: junctions created
API-->>Client: return Issue
else denied
API-->>Client: Unauthorized error
end
Client->>API: getIssue(id)
API->>Auth: verify READ_ISSUES
Auth-->>API: allowed / denied
alt allowed
API->>DB: SELECT Issue + apply visibility filters (officer/team/assignee/creator)
DB-->>API: Issue + relations
API-->>Client: return Issue
else denied
API-->>Client: Unauthorized error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 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 |
DGoel1602
added
Database
Spyderma9
force-pushed
the
blade/issues-backend
branch
from
7468125 to
888314e
Compare
Implements createIssue, getAllIssues, updateIssue, createSubIssue, and deleteIssue procedures with team visibility and user assignment junction handling, and adds READ_ISSUES/EDIT_ISSUES permissions to gate access.
DGoel1602
force-pushed
the
blade/issues-backend
branch
from
942219a to
6207046
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 57-72: The insert flow currently calls
db.insert(Issue).values(...).returning() and then separately calls
insertJunctions(issue.id, teamVisibilityIds, assigneeIds), which can leave the
DB in an inconsistent state if a later statement fails; change this to run the
insert and all related junction operations inside a single database transaction
(use your DB client's transaction API) so that the creation of Issue and the
calls performed by insertJunctions are committed or rolled back together; locate
the code using symbols Issue, db.insert(...).returning(), and insertJunctions
and wrap those calls in a transaction callback, moving any subsequent awaits
(including the TRPCError check) into the transaction scope and ensuring
insertJunctions uses the transactional connection/context.
- Around line 86-107: The visibility logic for non-officers only checks
IssuesToTeamsVisibility and omits the issue's owning team, so members of the
owning team may be denied; update the non-officer branch that computes
visibilityFilter (the code using db.query.Permissions.findMany,
visibilityFilter, exists(...) and IssuesToTeamsVisibility) to also allow access
when Issue.team is one of the user's roles — e.g. when userRoles is non-empty,
combine the exists(...) predicate with an OR that checks Issue.team is in
userRoles (use the existing inArray helper against Issue.team), or alternatively
ensure the owning team is seeded into IssuesToTeamsVisibility; adjust the
visibilityFilter expression accordingly so Issue.team is considered in the
authorization check.
- Around line 194-197: The query currently hydrates full auth rows via
teamVisibility.with.team: true and userAssignments.with.user: true, exposing
sensitive auth fields; change these to project only the public fields needed by
the client (e.g., team: { select: { id: true, name: true, slug: true } } and
user: { select: { id: true, name: true, username: true, avatarUrl: true } }) and
apply the same change for the other occurrence around lines 258-260 so assignee
and visible-team objects return only non-sensitive properties.
- Around line 19-22: CreateIssueInputSchema currently reuses IssueSchema and
leaks DB constraints; replace it with an explicit input schema (update
CreateIssueInputSchema) that omits any client-provided creator field and
enforces the same validations used in updateIssue: require name and description
with .min(1), validate links elements with .url(), validate status with
z.enum(ISSUE.ISSUE_STATUS), and require team (teamId or team field as per Issue
shape); keep assigneeIds and teamVisibilityIds as optional
z.array(z.string().uuid()). Also update the createIssue logic to perform the
multi-table writes (insert issue, then visibility/assignment inserts) inside a
single DB transaction so the operations are atomic.
In `@packages/db/src/schemas/knight-hacks.ts`:
- Around line 579-611: The junction tables IssuesToTeamsVisibility and
IssuesToUsersAssignment must declare referential delete behavior: update the
issueId foreign-key definitions (in IssuesToTeamsVisibility and
IssuesToUsersAssignment) that reference Issue.id to include .onDelete("CASCADE")
so rows are removed automatically when an Issue is deleted; leave or explicitly
set the teamId/userId foreign keys (references to Roles.id and User.id) to the
intended behavior if different (e.g., .onDelete("RESTRICT") or
.onDelete("CASCADE")) to make the constraint explicit.
- Around line 548-575: The Issue table lacks secondary indexes for the fields
used in read paths (team, creator, status, date, parent) causing full table
scans; add indexes in the same module where Issue is defined by creating indexes
on Issue.team, Issue.creator, Issue.status, Issue.date (and a composite index if
you expect common multi-column filters like (team, status) or (team, date)), and
add an index on Issue.parent (in addition to the existing foreign key) using
Drizzle’s index creation helpers so queries in
packages/api/src/routers/issues.ts that filter by team, creator, status, date,
or parent use those indexes for efficient lookups.
- Around line 29-30: This change added the new enums/tables (issue_status via
issueStatus, issue, issues_to_teams_visibility, issues_to_users_assignment) but
used db:push; update drizzle to generate migrations by adding an out property in
drizzle.config.ts (e.g., out: "./drizzle") and run drizzle-kit generate
postgresql to create and check in SQL migration files; in those migrations add
secondary indexes on issue(status), issue(team), issue(creator), issue(date) and
issue(parent) to support common queries; and modify the table
definitions/migrations to set explicit onDelete behavior (onDelete: "CASCADE" or
"RESTRICT") for the junction table foreign keys in issues_to_teams_visibility
and issues_to_users_assignment and for the self-referential parent FK on issue
to ensure referential integrity rather than relying on application ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 5b27364f-2d97-4846-9406-d36d43220918
📒 Files selected for processing (9)
packages/api/src/root.tspackages/api/src/routers/issues.tspackages/consts/src/index.tspackages/consts/src/issue.tspackages/consts/src/permissions.tspackages/db/src/client.tspackages/db/src/schemas/auth.tspackages/db/src/schemas/knight-hacks.tspackages/db/src/schemas/relations.ts
💤 Files with no reviewable changes (1)
- packages/db/src/schemas/auth.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/db/src/schemas/relations.ts (1)
41-44: Consider adding direct FK relations for eager-loading.
IssueRelationsonly wires the junction tables. Addingonerelations forteam,creator,event, andparentwould enable Drizzle'swithclause to load them in a single query:💡 Optional enhancement
export const IssueRelations = relations(Issue, ({ many, one }) => ({ teamVisibility: many(IssuesToTeamsVisibility), userAssignments: many(IssuesToUsersAssignment), + team: one(Roles, { fields: [Issue.team], references: [Roles.id] }), + creator: one(User, { fields: [Issue.creator], references: [User.id] }), + event: one(Event, { fields: [Issue.event], references: [Event.id] }), + parentIssue: one(Issue, { fields: [Issue.parent], references: [Issue.id] }), }));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/schemas/relations.ts` around lines 41 - 44, IssueRelations currently only exposes junction-table many relations (IssuesToTeamsVisibility, IssuesToUsersAssignment); add direct one relations so Drizzle's .with eager-loading can pull related rows in one query. Update the relations(Issue, ...) call to include one mappings (e.g., team: one(Team), creator: one(User), event: one(Event), parent: one(Issue)) alongside the existing many(...) entries and ensure the corresponding FK columns on the Issue model exist and match those relation targets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/db/src/schemas/relations.ts`:
- Around line 41-44: IssueRelations currently only exposes junction-table many
relations (IssuesToTeamsVisibility, IssuesToUsersAssignment); add direct one
relations so Drizzle's .with eager-loading can pull related rows in one query.
Update the relations(Issue, ...) call to include one mappings (e.g., team:
one(Team), creator: one(User), event: one(Event), parent: one(Issue)) alongside
the existing many(...) entries and ensure the corresponding FK columns on the
Issue model exist and match those relation targets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 32f4d1f9-589b-4123-8389-4cd17271b240
📒 Files selected for processing (9)
packages/api/src/root.tspackages/api/src/routers/issues.tspackages/consts/src/index.tspackages/consts/src/issue.tspackages/consts/src/permissions.tspackages/db/src/client.tspackages/db/src/schemas/auth.tspackages/db/src/schemas/knight-hacks.tspackages/db/src/schemas/relations.ts
💤 Files with no reviewable changes (1)
- packages/db/src/schemas/auth.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/consts/src/permissions.ts
- packages/api/src/root.ts
- packages/api/src/routers/issues.ts
- packages/consts/src/issue.ts
- packages/db/src/client.ts
- packages/consts/src/index.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/db/src/schemas/knight-hacks.ts (1)
557-574: Consider adding explicitonDeletebehavior to the Issue table's foreign keys.The FKs for
event,team,creator, andparentReferencecurrently use the defaultNO ACTION. While this is safe (prevents orphaned records), being explicit makes the referential integrity contract clear to future maintainers.Suggested behavior based on typical use cases:
event:onDelete: "set null"(issue survives if event is deleted)team/creator:onDelete: "restrict"(explicit: require reassignment before deletion)parentReference:onDelete: "set null"or"restrict"depending on whether child issues should become top-level or block parent deletion💡 Example for the event FK
- event: t.uuid().references(() => Event.id), + event: t.uuid().references(() => Event.id, { onDelete: "set null" }),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/schemas/knight-hacks.ts` around lines 557 - 574, Update the Issue table foreign key definitions to explicitly set onDelete behavior: for the column and foreign key symbols event (t.uuid().references(() => Event.id)) set onDelete: "set null"; for team (t.uuid().notNull().references(() => Roles.id)) and creator (t.uuid().notNull().references(() => User.id)) set onDelete: "restrict"; and for parentReference (foreignKey({ columns: [table.parent], foreignColumns: [table.id], name: "issue_parent_fk" })) set onDelete to either "set null" or "restrict" per chosen policy; modify the references()/foreignKey(...) calls accordingly to include the onDelete option so the schema expresses these explicit referential actions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/db/src/schemas/knight-hacks.ts`:
- Around line 557-574: Update the Issue table foreign key definitions to
explicitly set onDelete behavior: for the column and foreign key symbols event
(t.uuid().references(() => Event.id)) set onDelete: "set null"; for team
(t.uuid().notNull().references(() => Roles.id)) and creator
(t.uuid().notNull().references(() => User.id)) set onDelete: "restrict"; and for
parentReference (foreignKey({ columns: [table.parent], foreignColumns:
[table.id], name: "issue_parent_fk" })) set onDelete to either "set null" or
"restrict" per chosen policy; modify the references()/foreignKey(...) calls
accordingly to include the onDelete option so the schema expresses these
explicit referential actions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 54bd41b1-62c7-46b9-8bac-507bd42a7230
📒 Files selected for processing (2)
packages/api/src/routers/issues.tspackages/db/src/schemas/knight-hacks.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/api/src/routers/issues.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/db/src/schemas/knight-hacks.ts (1)
559-566: Consider adding explicitonDeletebehavior onteamandcreatorforeign keys.These FKs currently default to
NO ACTION, meaning you cannot delete a Role or User if they have associated issues. This might be intentional (forces reassignment before deletion), but it's inconsistent with the rest of this file where similar references use explicitonDelete: "cascade".Since both columns are
notNull, your options are:
cascade— Deleting the team/user deletes their issues (matches codebase pattern)restrict— Explicitly prevents deletion if issues exist (current implicit behavior, made explicit)♻️ Suggested change (if cascade is desired)
team: t .uuid() .notNull() - .references(() => Roles.id), + .references(() => Roles.id, { onDelete: "cascade" }), creator: t .uuid() .notNull() - .references(() => User.id), + .references(() => User.id, { onDelete: "cascade" }),As per coding guidelines: "Foreign key constraints and referential integrity" — explicit behavior prevents surprises when deleting parent records.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/schemas/knight-hacks.ts` around lines 559 - 566, The foreign keys for team and creator (references to Roles.id and User.id in knight-hacks schema) lack explicit onDelete behavior; update both references to include an explicit onDelete (preferably onDelete: "cascade" to match the rest of the file) so deletions of a Role or User will cascade to their issues, e.g. modify the team references(() => Roles.id) and creator references(() => User.id) declarations to include the onDelete option.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/db/src/schemas/knight-hacks.ts`:
- Around line 559-566: The foreign keys for team and creator (references to
Roles.id and User.id in knight-hacks schema) lack explicit onDelete behavior;
update both references to include an explicit onDelete (preferably onDelete:
"cascade" to match the rest of the file) so deletions of a Role or User will
cascade to their issues, e.g. modify the team references(() => Roles.id) and
creator references(() => User.id) declarations to include the onDelete option.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: fd44f227-f577-48f7-808a-0a175b26f780
📒 Files selected for processing (2)
packages/api/src/routers/issues.tspackages/db/src/schemas/knight-hacks.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/api/src/routers/issues.ts
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 `@packages/db/src/schemas/knight-hacks.ts`:
- Around line 559-566: The current foreign keys for the Issue columns team and
creator reference Roles.id and User.id with onDelete: "cascade", which causes
Issues to be deleted when a Role or User is removed; change these constraints to
avoid destructive cascades—update the team foreign key (identifier: team) to use
onDelete: "restrict" (prevent role deletion while issues reference it) and
update the creator foreign key (identifier: creator) to either use onDelete:
"restrict" as well or make creator nullable and use onDelete: "set null" if you
want issues to survive account cleanup; adjust the column nullability
accordingly if you choose the set-null approach and run migrations to apply the
constraint changes.
- Around line 567-579: The current single nullable parent column (parent) with
its foreign key (parentReference / issue_parent_fk) only supports one-to-many
sub-issues; instead add a self-join table to model many-to-many links. Create a
new table (e.g., IssuesToSubIssues via createTable) with columns issue_id and
sub_issue_id that reference Issue.id (onDelete cascade) and a composite primary
key on [issue_id, sub_issue_id]; remove or deprecate the parent FK/indexes
(parent, parentReference, parentIdx, issue_parent_fk) and update any code that
writes/reads parent to use the new join table. Ensure migrations are written to
migrate existing parent values into the join table and to drop the old
column/index once consumers are updated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 0e45cd01-8fb7-4993-a08a-64ade84e035c
📒 Files selected for processing (1)
packages/db/src/schemas/knight-hacks.ts
…sue table to restrict
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
We need to be able to store and query issues for the new feature!
What
Adds schemas and the full router to make, delete, get, update schemas with proper authentication and considerations for future features. Schema includes main issues table and 2 join tables joining issues with both assignees and the teams that are allowed to see the issue (issue is only assigned to one team, but can be visible by multiple).
Also moves all relations in the database to a seperate file for cleaner navigation for future DB schema changes. (We should probably move these to more than 3 files anyway but it's fine for now)
Closes: #390
Closes: #391
Test Plan
Checklist
db:pushbefore mergingLots of schema changes please db push!
Summary by CodeRabbit
New Features
Chores