Merged
Conversation
📝 WalkthroughWalkthroughThis PR introduces a comprehensive issue and template creation system by refactoring the create-edit dialog to use Forge UI components and a hierarchical Changes
Sequence DiagramsequenceDiagram
actor User
participant Dialog as Create/Edit Dialog
participant Form as Form Validation<br/>(Zod + Tree)
participant API as API Router<br/>(Issues/Events)
participant DB as Database
User->>Dialog: Fill form & add sub-issues
User->>Dialog: Submit
Dialog->>Form: Validate IssueEditNode tree
alt Validation fails
Form-->>Dialog: Invalid
Dialog-->>User: Show error
else Validation succeeds
Dialog->>API: createIssue({ name, team, ..., children: [...] })
API->>API: For each child: create event (if isEvent)
API->>DB: Insert parent issue
API->>DB: Insert child issues recursively
API->>DB: Insert assignments & visibility
DB-->>API: Success
API-->>Dialog: Return issue + children
Dialog->>Dialog: Invalidate cache
Dialog-->>User: Show success toast
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (9)
apps/blade/src/app/_components/issues/issue-form-fields.tsx (1)
236-248: Consider adding accessible labels to checkboxes.The
Checkboxcomponents lack explicitidand associatedhtmlForon the Label. While clicking the label may work via parent click handling, explicit association improves accessibility.💡 Optional: Add explicit label association
<div key={`${keyPrefix}${role.id}`} className="flex flex-row items-start space-x-3 space-y-0" > <Checkbox + id={`${keyPrefix}checkbox-${role.id}`} checked={selectedIds.includes(role.id)} onCheckedChange={(checked) => onChange( checked ? Array.from(new Set([...selectedIds, role.id])) : selectedIds.filter((id) => id !== role.id), ) } /> - <Label className="cursor-pointer font-normal">{role.name}</Label> + <Label htmlFor={`${keyPrefix}checkbox-${role.id}`} className="cursor-pointer font-normal">{role.name}</Label> </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/blade/src/app/_components/issues/issue-form-fields.tsx` around lines 236 - 248, The Checkbox and Label lack an explicit association—add an id to the Checkbox and set the Label's htmlFor to that same id so screen readers and forms can associate them; locate the Checkbox and Label inside the issue row (where Checkbox is using checked={selectedIds.includes(role.id)} and onCheckedChange calling onChange with role.id, and Label renders role.name) and generate a stable id (e.g., using role.id like `role-checkbox-${role.id}`) and apply it to Checkbox and Label.htmlFor, ensuring you don’t break existing click handlers or selectedIds logic.apps/blade/src/app/_components/issues/create-edit-dialog.tsx (2)
423-428: Silent return when datetime parsing fails—consider user feedback.If
startDatetimeorendDatetimeis null, the function returns without creating the issue or showing an error. While validation should prevent this, a defensive toast would improve debuggability.💡 Optional: Add error feedback
if (!startDatetime || !endDatetime) return; + toast.error("Invalid event start or end time"); + return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/blade/src/app/_components/issues/create-edit-dialog.tsx` around lines 423 - 428, The code silently returns if parseEventDateTime fails for startDatetime or endDatetime inside the formValues.isEvent branch; instead, detect when startDatetime or endDatetime is null and surface a user-facing error (use the app's toast/snackbar helper) before returning so the user gets feedback. Update the block that calls parseEventDateTime (referencing parseEventDateTime, defaultEventForm, formValues.isEvent, startDatetime, endDatetime) to show a descriptive error toast like "Invalid event start/end datetime" (or a localized string) and then abort the create flow.
341-395: Sequential event creation may be slow for large trees.
buildIssueNodescreates events sequentially (for...ofwithawait). For templates with many event nodes, this could be slow. Consider if parallel creation (Promise.all) is feasible, though you'd need to handle partial failures.For now, this is acceptable—templates typically have limited depth/breadth.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/blade/src/app/_components/issues/create-edit-dialog.tsx` around lines 341 - 395, buildIssueNodes currently creates events sequentially using a for...of loop and awaiting createEventMutation.mutateAsync for each node, which can be slow for large trees; refactor buildIssueNodes to launch event creation in parallel where safe by mapping event nodes to promises (using createEventMutation.mutateAsync for nodes with node.isEvent && node.eventData) and awaiting Promise.all for those creations, then continue assembling the IssueSubmitNode results (ensure you correlate created event IDs back to their respective nodes, handle/propagate individual mutation failures or collect errors, and keep recursion for children by calling await buildIssueNodes(node.children) as needed); refer to buildIssueNodes, createEventMutation.mutateAsync, node.isEvent, node.eventData, and node.children when implementing.packages/api/src/routers/event.ts (1)
321-347: Good improvement: returning the created event enables downstream usage.The
.returning()pattern is correct. One small note: the error code at line 336 (BAD_REQUEST) might be more accurate asINTERNAL_SERVER_ERRORsince this represents a database failure, not invalid input. However, this is a minor consistency point.💡 Optional: Use INTERNAL_SERVER_ERROR for DB failures
if (!createdEvent) { throw new TRPCError({ message: "Failed to retrieve created event", - code: "BAD_REQUEST", + code: "INTERNAL_SERVER_ERROR", }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routers/event.ts` around lines 321 - 347, Change the error code used when failing to retrieve the created DB row to reflect a server/database failure: in the block that calls db.insert(Event).values(...).returning() and checks if (!createdEvent), update the TRPCError instantiation (currently using code: "BAD_REQUEST") to use code: "INTERNAL_SERVER_ERROR" so the error better represents a DB/server-side failure for the createdEvent path.apps/blade/src/app/admin/issues/page.tsx (1)
23-32: Dev page renders issue dialog components.The
[DEV]prefix in the button text suggests this is a development/testing page. Consider adding a comment or removing from production builds if not intended for end users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/blade/src/app/admin/issues/page.tsx` around lines 23 - 32, The page currently renders a dev-only trigger button ("[DEV] Open Issue Dialog") inside CreateEditDialog which exposes test UI in production; either remove that button or guard it behind an environment/feature flag check (e.g., process.env.NODE_ENV !== 'production' or a NEXT_PUBLIC_SHOW_DEV_UI flag) and add a brief inline comment explaining it's dev-only; update the JSX around CreateEditDialog and the button text (and/or reference IssueTemplate) accordingly so production builds do not show the dev button.apps/blade/src/app/_components/issues/issue-dialog-utils.ts (1)
98-115: Minor edge case: month value of 0 passes validation.The check
!monthwould fail if the parsed month is0(which could happen with malformed input like"2024-00-15"). While unlikely in practice (the date input type prevents this), a more explicit check could bemonth === undefined || month < 1 || month > 12.This is a minor edge case since the HTML date input provides valid strings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/blade/src/app/_components/issues/issue-dialog-utils.ts` around lines 98 - 115, The validation in parseEventDateTime currently treats falsy month values as invalid but lets a parsed month of 0 through (or out-of-range values) slip through; update the validation to explicitly check that year, month and day are numbers and that month is between 1 and 12 (e.g., month === undefined || month < 1 || month > 12) and similarly validate day is within 1..31 (or better, rely on parsed.getTime after range checks) and that hour/minute are not NaN before constructing the Date; adjust the conditional that currently reads `if (!year || !month || !day || Number.isNaN(hour) || Number.isNaN(minute))` to use explicit numeric-range checks for month (and day) so inputs like "2024-00-15" return null.apps/blade/src/app/_components/issues/issue-template-dialog.tsx (1)
273-281: Consider adding delete confirmation.The delete button immediately calls
deleteTemplate.mutate({ id: t.id })without confirmation. A misclick could permanently delete a template. Consider adding a confirmation dialog or using a double-click pattern.💡 Optional: Add confirmation before delete
<Button variant="ghost" size="sm" className="text-destructive hover:text-destructive" - onClick={() => deleteTemplate.mutate({ id: t.id })} + onClick={() => { + if (confirm(`Delete template "${t.name}"?`)) { + deleteTemplate.mutate({ id: t.id }); + } + }} disabled={deleteTemplate.isPending} >Or implement a more polished confirmation dialog component.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/blade/src/app/_components/issues/issue-template-dialog.tsx` around lines 273 - 281, The delete button currently calls deleteTemplate.mutate({ id: t.id }) immediately; wrap that call in a user confirmation step (e.g., show a modal confirmation or use window.confirm) inside the Button's onClick handler in issue-template-dialog.tsx so a user must explicitly confirm before delete; ensure you still respect deleteTemplate.isPending (keep the disabled state while pending) and only call deleteTemplate.mutate when the user confirms, and surface any error/success feedback from the mutation in the dialog flow.apps/blade/src/app/_components/issues/issue-node.tsx (2)
436-442: Same accessibility improvement applies here.Add
aria-label="Sub-task name"for screen reader support, consistent with the suggestion forIssueNode.♿ Suggested accessibility improvement
<Input className="h-7 flex-1 border-0 bg-transparent px-0 py-0 text-sm shadow-none focus-visible:ring-0" placeholder="Sub-task name (required)" + aria-label="Sub-task name" value={node.name} onChange={(e) => update("name", e.target.value)} onClick={(e) => e.stopPropagation()} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/blade/src/app/_components/issues/issue-node.tsx` around lines 436 - 442, The Input in the IssueNode sub-task block doesn't provide an accessible name for screen readers; add aria-label="Sub-task name" to the Input element (the same place where value={node.name}, onChange={(e) => update("name", e.target.value)}, and onClick={(e) => e.stopPropagation()} are defined) so assistive tech can announce the field; ensure the attribute is added to the <Input ... /> JSX for the sub-task name input.
149-155: Consider adding an accessible label to the name input.The name input has a placeholder, but screen readers work best with explicit labels. Adding an
aria-labelprovides the missing accessible name without changing the visual layout.♿ Suggested accessibility improvement
<Input className="h-7 flex-1 border-0 bg-transparent px-0 py-0 text-sm shadow-none focus-visible:ring-0" placeholder="Sub-issue name (required)" + aria-label="Sub-issue name" value={node.name} onChange={(e) => update("name", e.target.value)} onClick={(e) => e.stopPropagation()} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/blade/src/app/_components/issues/issue-node.tsx` around lines 149 - 155, The name Input in issue-node.tsx lacks an accessible label—add an aria-label to the Input (the same element that uses value={node.name} and onChange={(e) => update("name", e.target.value)}) to provide screen readers with a descriptive name (e.g., aria-label="Sub-issue name") and optionally aria-required="true" if the field is required; ensure the attribute is added to the Input props so visual layout remains unchanged.
🤖 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/issue-node.tsx`:
- Around line 467-476: The number input's onChange handler can set daysOffset to
NaN; in the Input onChange for the daysOffset field, validate and sanitize
e.target.value before calling update: trim the string, treat empty string as
undefined, attempt to parse to a number (e.g., parseFloat/parseInt), and only
call update("daysOffset", parsedNumber) if the parsed result is a finite number;
otherwise call update("daysOffset", undefined) to avoid storing NaN. Keep
references to the Input element, the onChange handler, the update function, and
the daysOffset property when making the change.
---
Nitpick comments:
In `@apps/blade/src/app/_components/issues/create-edit-dialog.tsx`:
- Around line 423-428: The code silently returns if parseEventDateTime fails for
startDatetime or endDatetime inside the formValues.isEvent branch; instead,
detect when startDatetime or endDatetime is null and surface a user-facing error
(use the app's toast/snackbar helper) before returning so the user gets
feedback. Update the block that calls parseEventDateTime (referencing
parseEventDateTime, defaultEventForm, formValues.isEvent, startDatetime,
endDatetime) to show a descriptive error toast like "Invalid event start/end
datetime" (or a localized string) and then abort the create flow.
- Around line 341-395: buildIssueNodes currently creates events sequentially
using a for...of loop and awaiting createEventMutation.mutateAsync for each
node, which can be slow for large trees; refactor buildIssueNodes to launch
event creation in parallel where safe by mapping event nodes to promises (using
createEventMutation.mutateAsync for nodes with node.isEvent && node.eventData)
and awaiting Promise.all for those creations, then continue assembling the
IssueSubmitNode results (ensure you correlate created event IDs back to their
respective nodes, handle/propagate individual mutation failures or collect
errors, and keep recursion for children by calling await
buildIssueNodes(node.children) as needed); refer to buildIssueNodes,
createEventMutation.mutateAsync, node.isEvent, node.eventData, and node.children
when implementing.
In `@apps/blade/src/app/_components/issues/issue-dialog-utils.ts`:
- Around line 98-115: The validation in parseEventDateTime currently treats
falsy month values as invalid but lets a parsed month of 0 through (or
out-of-range values) slip through; update the validation to explicitly check
that year, month and day are numbers and that month is between 1 and 12 (e.g.,
month === undefined || month < 1 || month > 12) and similarly validate day is
within 1..31 (or better, rely on parsed.getTime after range checks) and that
hour/minute are not NaN before constructing the Date; adjust the conditional
that currently reads `if (!year || !month || !day || Number.isNaN(hour) ||
Number.isNaN(minute))` to use explicit numeric-range checks for month (and day)
so inputs like "2024-00-15" return null.
In `@apps/blade/src/app/_components/issues/issue-form-fields.tsx`:
- Around line 236-248: The Checkbox and Label lack an explicit association—add
an id to the Checkbox and set the Label's htmlFor to that same id so screen
readers and forms can associate them; locate the Checkbox and Label inside the
issue row (where Checkbox is using checked={selectedIds.includes(role.id)} and
onCheckedChange calling onChange with role.id, and Label renders role.name) and
generate a stable id (e.g., using role.id like `role-checkbox-${role.id}`) and
apply it to Checkbox and Label.htmlFor, ensuring you don’t break existing click
handlers or selectedIds logic.
In `@apps/blade/src/app/_components/issues/issue-node.tsx`:
- Around line 436-442: The Input in the IssueNode sub-task block doesn't provide
an accessible name for screen readers; add aria-label="Sub-task name" to the
Input element (the same place where value={node.name}, onChange={(e) =>
update("name", e.target.value)}, and onClick={(e) => e.stopPropagation()} are
defined) so assistive tech can announce the field; ensure the attribute is added
to the <Input ... /> JSX for the sub-task name input.
- Around line 149-155: The name Input in issue-node.tsx lacks an accessible
label—add an aria-label to the Input (the same element that uses
value={node.name} and onChange={(e) => update("name", e.target.value)}) to
provide screen readers with a descriptive name (e.g., aria-label="Sub-issue
name") and optionally aria-required="true" if the field is required; ensure the
attribute is added to the Input props so visual layout remains unchanged.
In `@apps/blade/src/app/_components/issues/issue-template-dialog.tsx`:
- Around line 273-281: The delete button currently calls deleteTemplate.mutate({
id: t.id }) immediately; wrap that call in a user confirmation step (e.g., show
a modal confirmation or use window.confirm) inside the Button's onClick handler
in issue-template-dialog.tsx so a user must explicitly confirm before delete;
ensure you still respect deleteTemplate.isPending (keep the disabled state while
pending) and only call deleteTemplate.mutate when the user confirms, and surface
any error/success feedback from the mutation in the dialog flow.
In `@apps/blade/src/app/admin/issues/page.tsx`:
- Around line 23-32: The page currently renders a dev-only trigger button
("[DEV] Open Issue Dialog") inside CreateEditDialog which exposes test UI in
production; either remove that button or guard it behind an environment/feature
flag check (e.g., process.env.NODE_ENV !== 'production' or a
NEXT_PUBLIC_SHOW_DEV_UI flag) and add a brief inline comment explaining it's
dev-only; update the JSX around CreateEditDialog and the button text (and/or
reference IssueTemplate) accordingly so production builds do not show the dev
button.
In `@packages/api/src/routers/event.ts`:
- Around line 321-347: Change the error code used when failing to retrieve the
created DB row to reflect a server/database failure: in the block that calls
db.insert(Event).values(...).returning() and checks if (!createdEvent), update
the TRPCError instantiation (currently using code: "BAD_REQUEST") to use code:
"INTERNAL_SERVER_ERROR" so the error better represents a DB/server-side failure
for the createdEvent path.
🪄 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: f851da03-035b-44d7-b131-b2ad01942a9b
📒 Files selected for processing (9)
apps/blade/src/app/_components/issues/create-edit-dialog.tsxapps/blade/src/app/_components/issues/issue-dialog-utils.tsapps/blade/src/app/_components/issues/issue-form-fields.tsxapps/blade/src/app/_components/issues/issue-node.tsxapps/blade/src/app/_components/issues/issue-template-dialog.tsxapps/blade/src/app/admin/issues/page.tsxpackages/api/src/routers/event.tspackages/api/src/routers/issues.tspackages/consts/src/issue.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/blade/src/app/_components/issues/issue-node.tsx (2)
149-155: Consider adding an accessible label for the name input.The input relies on
placeholderfor context, but screen readers don't consistently announce placeholder text as a label. For better accessibility, consider adding a visually hidden label oraria-label.♿ Suggested fix
<Input className="h-7 flex-1 border-0 bg-transparent px-0 py-0 text-sm shadow-none focus-visible:ring-0" placeholder="Sub-issue name (required)" + aria-label="Sub-issue name" value={node.name} onChange={(e) => update("name", e.target.value)} onClick={(e) => e.stopPropagation()} />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/blade/src/app/_components/issues/issue-node.tsx` around lines 149 - 155, The name input uses only a placeholder which is not reliably announced by screen readers; add an accessible label by providing either an aria-label (e.g., aria-label="Sub-issue name") or rendering a visually hidden <label> tied to the Input's id, and keep the existing props (value={node.name}, onChange={e => update("name", e.target.value)}, onClick={e => e.stopPropagation()}) untouched; update the Input usage in issue-node.tsx (the Input component instance) to include the chosen accessibility attribute so screen readers receive a proper label.
436-442: Same accessibility improvement applies here.Like the
IssueNodeinput, consider addingaria-label="Sub-task name"for screen reader users.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/blade/src/app/_components/issues/issue-node.tsx` around lines 436 - 442, The Input for sub-tasks lacks an accessible label for screen readers; add aria-label="Sub-task name" to the Input element used in the IssueNode sub-task UI (the same Input that uses value={node.name}, onChange={(e) => update("name", e.target.value)} and onClick={(e) => e.stopPropagation()}) so assistive technologies announce the field purpose.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@apps/blade/src/app/_components/issues/issue-node.tsx`:
- Around line 149-155: The name input uses only a placeholder which is not
reliably announced by screen readers; add an accessible label by providing
either an aria-label (e.g., aria-label="Sub-issue name") or rendering a visually
hidden <label> tied to the Input's id, and keep the existing props
(value={node.name}, onChange={e => update("name", e.target.value)}, onClick={e
=> e.stopPropagation()}) untouched; update the Input usage in issue-node.tsx
(the Input component instance) to include the chosen accessibility attribute so
screen readers receive a proper label.
- Around line 436-442: The Input for sub-tasks lacks an accessible label for
screen readers; add aria-label="Sub-task name" to the Input element used in the
IssueNode sub-task UI (the same Input that uses value={node.name}, onChange={(e)
=> update("name", e.target.value)} and onClick={(e) => e.stopPropagation()}) so
assistive technologies announce the field purpose.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 62a05ccd-aa44-41f6-8d18-f56e6acdceca
📒 Files selected for processing (2)
apps/blade/src/app/_components/issues/issue-node.tsxapps/blade/src/app/_components/issues/issue-template-dialog.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/blade/src/app/_components/issues/issue-template-dialog.tsx
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
2 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 Issue templates to create multiple issue tasks easier to create
What
Template creation/edit modal
Addresses: #398
Test Plan
Checklist
db:pushbefore mergingSummary by CodeRabbit
New Features
Improvements