feat(desktop): add task create modal#2656
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a desktop-only task creation flow: a new Changes
sequenceDiagram
participant User
participant TasksTopBar as TasksTopBar (UI)
participant Dialog as CreateTaskDialog (Form)
participant API as TRPC API (createFromUi)
participant DB as Database
participant Sync as TaskSync
User->>TasksTopBar: Click "New task"
TasksTopBar->>Dialog: open = true
Dialog->>Dialog: load statuses, users, org context
User->>Dialog: Enter title/description, pick metadata
User->>Dialog: Cmd/Ctrl+Enter
Dialog->>API: mutate(createFromUi {title, description, statusId, priority, assigneeId})
API->>DB: insert task with generated unique slug
DB-->>API: inserted task
API->>Sync: trigger syncTask(task.id)
Sync-->>Dialog: task appears in live collection
Dialog->>User: close, show success toast
Dialog->>TasksTopBar: navigate to /tasks/$taskId (preserve filters)
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
6 issues found across 14 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/TaskMarkdownRenderer/TaskMarkdownRenderer.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/TaskMarkdownRenderer/TaskMarkdownRenderer.tsx:250">
P2: Only consume Cmd/Ctrl+Enter when `onModEnter` exists; currently the shortcut is swallowed even when no callback is provided.</violation>
<violation number="2" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/TaskMarkdownRenderer/TaskMarkdownRenderer.tsx:266">
P2: External `content` updates can be permanently missed if they arrive while the editor is focused, because blur does not retrigger this effect.</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/components/CreateTaskAssigneePicker/CreateTaskAssigneePicker.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/components/CreateTaskAssigneePicker/CreateTaskAssigneePicker.tsx:101">
P3: `CommandEmpty` will not render here because the list always contains the "No assignee" item. Use a regular element for the manual no-results state so users can actually see "No people found.".</violation>
</file>
<file name="packages/trpc/src/router/task/task.ts">
<violation number="1" location="packages/trpc/src/router/task/task.ts:151">
P2: Slug pre-generation runs outside the insert transaction, so concurrent creates can still collide and fail on the org+slug unique constraint.</violation>
<violation number="2" location="packages/trpc/src/router/task/task.ts:155">
P1: Validate `input.statusId` belongs to the active organization before inserting; currently any valid status UUID can be linked.</violation>
<violation number="3" location="packages/trpc/src/router/task/task.ts:180">
P2: Handle the fire-and-forget `syncTask` promise with `.catch(...)` (or await in `try/catch`) to avoid unhandled promise rejections when sync enqueueing fails.
(Based on your team's feedback about handling async calls with proper await and catch.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Comment on lines
+250
to
+253
| if ((event.metaKey || event.ctrlKey) && event.key === "Enter") { | ||
| onModEnter?.(); | ||
| return true; | ||
| } |
Contributor
There was a problem hiding this comment.
P2: Only consume Cmd/Ctrl+Enter when onModEnter exists; currently the shortcut is swallowed even when no callback is provided.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/TaskMarkdownRenderer/TaskMarkdownRenderer.tsx, line 250:
<comment>Only consume Cmd/Ctrl+Enter when `onModEnter` exists; currently the shortcut is swallowed even when no callback is provided.</comment>
<file context>
@@ -217,21 +244,35 @@ export function TaskMarkdownRenderer({
+ class: cn("focus:outline-none min-h-[100px]", editorClassName),
},
+ handleKeyDown: (_, event) => {
+ if ((event.metaKey || event.ctrlKey) && event.key === "Enter") {
+ onModEnter?.();
+ return true;
</file context>
Suggested change
| if ((event.metaKey || event.ctrlKey) && event.key === "Enter") { | |
| onModEnter?.(); | |
| return true; | |
| } | |
| if ((event.metaKey || event.ctrlKey) && event.key === "Enter") { | |
| if (!onModEnter) return false; | |
| onModEnter(); | |
| return true; | |
| } |
| }); | ||
|
|
||
| useEffect(() => { | ||
| if (!editor || editor.isFocused) return; |
Contributor
There was a problem hiding this comment.
P2: External content updates can be permanently missed if they arrive while the editor is focused, because blur does not retrigger this effect.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/TaskMarkdownRenderer/TaskMarkdownRenderer.tsx, line 266:
<comment>External `content` updates can be permanently missed if they arrive while the editor is focused, because blur does not retrigger this effect.</comment>
<file context>
@@ -217,21 +244,35 @@ export function TaskMarkdownRenderer({
});
+ useEffect(() => {
+ if (!editor || editor.isFocused) return;
+
+ const currentMarkdown = getMarkdown(editor);
</file context>
| }); | ||
|
|
||
| if (result.task) { | ||
| syncTask(result.task.id); |
Contributor
There was a problem hiding this comment.
P2: Handle the fire-and-forget syncTask promise with .catch(...) (or await in try/catch) to avoid unhandled promise rejections when sync enqueueing fails.
(Based on your team's feedback about handling async calls with proper await and catch.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/trpc/src/router/task/task.ts, line 180:
<comment>Handle the fire-and-forget `syncTask` promise with `.catch(...)` (or await in `try/catch`) to avoid unhandled promise rejections when sync enqueueing fails.
(Based on your team's feedback about handling async calls with proper await and catch.) </comment>
<file context>
@@ -88,6 +136,53 @@ export const taskRouter = {
+ });
+
+ if (result.task) {
+ syncTask(result.task.id);
+ }
+
</file context>
Suggested change
| syncTask(result.task.id); | |
| void syncTask(result.task.id).catch((error) => { | |
| console.warn("[task.createFromUi] syncTask failed", error); | |
| }); |
| </CommandGroup> | ||
|
|
||
| {filteredUsers.length === 0 ? ( | ||
| <CommandEmpty>No people found.</CommandEmpty> |
Contributor
There was a problem hiding this comment.
P3: CommandEmpty will not render here because the list always contains the "No assignee" item. Use a regular element for the manual no-results state so users can actually see "No people found.".
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/components/CreateTaskAssigneePicker/CreateTaskAssigneePicker.tsx, line 101:
<comment>`CommandEmpty` will not render here because the list always contains the "No assignee" item. Use a regular element for the manual no-results state so users can actually see "No people found.".</comment>
<file context>
@@ -0,0 +1,126 @@
+ </CommandGroup>
+
+ {filteredUsers.length === 0 ? (
+ <CommandEmpty>No people found.</CommandEmpty>
+ ) : (
+ <CommandGroup>
</file context>
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/CreateTaskDialog.tsx (1)
23-23: Prefer therenderer/...alias for this util import.This file already uses alias imports elsewhere, and the 4-level relative path is brittle to moves and renames. As per coding guidelines,
apps/desktop/**/*.{ts,tsx}: Use alias as defined intsconfig.jsonwhen possible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/CreateTaskDialog.tsx` at line 23, The import for compareStatusesForDropdown uses a brittle 4-level relative path; update the import to use the project alias (renderer) instead of "../../../../utils/sorting". Locate the import statement that brings in compareStatusesForDropdown in CreateTaskDialog.tsx and change it to import from the aliased path (e.g., 'renderer/utils/sorting') so it matches other alias imports and avoids breakage on file moves/renames.
🤖 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/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/`$taskId/components/TaskMarkdownRenderer/TaskMarkdownRenderer.tsx:
- Around line 249-255: The keydown handler in TaskMarkdownRenderer
(handleKeyDown) currently consumes Mod+Enter by returning true even when
onModEnter is undefined, blocking the editor's default behavior; update
handleKeyDown to first check that onModEnter is provided before calling
onModEnter() and returning true (i.e., only call onModEnter?.() and return true
when onModEnter is a function), otherwise let the event fall through by
returning false—this ensures the interception is gated on the onModEnter prop in
TaskMarkdownRenderer/handleKeyDown.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/components/CreateTaskAssigneePicker/CreateTaskAssigneePicker.tsx`:
- Around line 15-19: The users prop is not scoped to the active organization and
can leak cross-org identities; either (A) scope the users list at the call sites
before passing it into CreateTaskAssigneePicker (filter the array to users whose
organizationId matches the current active organization id) or (B) add a
defensive guard inside CreateTaskAssigneePicker: obtain the active org id (from
the same context/hooks the app uses for current org), filter the incoming users
prop (SelectUser[]) to only include users with that organizationId, and
render/select only from that filtered list (or return null/placeholder if no
matching users). Update both usages referenced (the CreateTaskDialog call sites
around the two locations noted) and the logic inside CreateTaskAssigneePicker
(and the similar block at lines ~104-114) so assignee choices are always limited
to the active organization.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/components/CreateTaskStatusPicker/CreateTaskStatusPicker.tsx`:
- Around line 50-53: Replace the unsafe cast at the StatusIcon call by
introducing an explicit mapper or type guard that converts SelectTaskStatus.type
into the safe StatusType union: add a function (e.g., mapTaskStatusToStatusType
or isValidStatusType) that takes currentStatus.type, checks it against a
constant, exhaustively-typed mapping derived from taskStatusEnumValues (use a
typed const object/Record and literal union) and returns the corresponding
StatusType or throws/returns a fallback; then use that function when rendering
StatusIcon instead of "as StatusType" to ensure mismatches between
SelectTaskStatus.type, taskStatusEnumValues, and StatusType are caught at
compile time.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/CreateTaskDialog.tsx`:
- Around line 116-126: The waitForTaskSync helper (waitForTaskSync) currently
always returns after ~2s even if the task never appears; change it to return a
boolean (true on found, false on timeout) or throw an error when the task isn't
found after the retry loop, and update the caller(s) that create and navigate to
the new task so they only close the CreateTaskDialog and call navigateToTask
(the navigation logic) when waitForTaskSync indicates success; if it returns
false/throws, keep the dialog open and surface an error/toast instead of
navigating.
- Around line 105-114: The reset useEffect in CreateTaskDialog currently clears
form state whenever open becomes false, which allows dismissal via overlay/Esc
during an in-flight mutation; change the guard to skip resetting while
isCreating is true (e.g., if (open || isCreating) return;) and add isCreating to
the effect dependencies so the reset only runs when the dialog is closed and not
creating; apply the same change to the other reset effect (the block at
176-184). Also ensure any modal close handler/prop used by this dialog checks
isCreating and no-ops (prevent closing) while isCreating is true so
overlay/Esc/close-button cannot dismiss during a pending create.
- Around line 141-147: The code currently calls
apiTrpcClient.task.createFromUi.mutate with description: description.trim() ||
null which strips meaningful Markdown whitespace; change this so only the
all-whitespace case becomes null and otherwise send the raw editor content. In
the CreateTaskDialog (use the description variable passed to
apiTrpcClient.task.createFromUi.mutate), replace the trim-based conditional with
a check like "if description contains any non-whitespace then send description
as-is else send null" so title, statusId, priority, assigneeId remain unchanged
and only pure-whitespace descriptions become null.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/TasksTopBar.tsx`:
- Around line 153-178: The two icon-only toggle buttons (the Table and Board
buttons using HiOutlineQueueList and HiOutlineViewColumns) need explicit
accessibility semantics: add an accessible name via aria-label (e.g., "Table
view" and "Board view") and expose pressed state by adding
aria-pressed={viewMode === "table"} for the table button and
aria-pressed={viewMode === "board"} for the board button (keep the existing
onClick handlers that call onViewModeChange). This ensures screen readers
recognize the buttons and their active state while preserving the current
styling logic in the className/ cn usage.
In `@packages/trpc/src/router/task/task.ts`:
- Around line 33-42: The slug lookup currently excludes soft-deleted rows,
causing unique-key conflicts; update the query that populates existingTasks (the
db.select(...).from(tasks).where(...) using organizationId, baseSlug and
tasks.slug) to include soft-deleted rows by removing the isNull(tasks.deletedAt)
predicate (or otherwise explicitly including deleted records) so the lookup
matches the DB unique constraint on (organizationId, slug) and prevents false
positives when recreating a title.
- Around line 154-167: The code currently uses input.statusId (or
seedDefaultStatuses result) directly when inserting into tasks, allowing a
caller to supply a UUID that belongs to another organization; before calling
tx.insert(tasks) validate the resolved statusId by querying the statuses table
via tx (e.g. SELECT FROM statuses WHERE id = statusId AND organizationId =
organizationId) and either replace statusId with the found row's id or throw an
error if no matching status exists; update the block around statusId resolution
(the input.statusId check, seedDefaultStatuses call, and the tx.insert into
tasks) to perform this lookup/validation and reject mismatches instead of
blindly using the UUID.
- Around line 27-58: The current generateUniqueTaskSlug approach races because
it reads existing slugs before the insert; fix by moving slug generation/retry
into the same DB transaction or by catching unique-constraint failures on insert
and retrying generation: update the create flow (where generateUniqueTaskSlug is
used) to either perform slug selection inside the insert transaction using the
same connection/transaction scope (so check and insert atomically) or wrap the
insert that uses the slug in a retry loop that catches the tasks_org_slug_unique
violation and regenerates (using generateBaseSlug + suffix logic) until an
insert succeeds; ensure you reference and reuse generateUniqueTaskSlug (or
extract the suffixing logic into a helper) and handle retries with a bounded
attempt count to avoid tight infinite loops.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/CreateTaskDialog.tsx`:
- Line 23: The import for compareStatusesForDropdown uses a brittle 4-level
relative path; update the import to use the project alias (renderer) instead of
"../../../../utils/sorting". Locate the import statement that brings in
compareStatusesForDropdown in CreateTaskDialog.tsx and change it to import from
the aliased path (e.g., 'renderer/utils/sorting') so it matches other alias
imports and avoids breakage on file moves/renames.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc4811e1-a7ee-44d6-9415-c4d78bfc031b
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
apps/desktop/plans/20260320-desktop-task-create-tiptap-plan.mdapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/TaskMarkdownRenderer/TaskMarkdownRenderer.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/TasksTopBar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/CreateTaskDialog.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/components/CreateTaskAssigneePicker/CreateTaskAssigneePicker.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/components/CreateTaskAssigneePicker/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/components/CreateTaskPriorityPicker/CreateTaskPriorityPicker.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/components/CreateTaskPriorityPicker/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/components/CreateTaskStatusPicker/CreateTaskStatusPicker.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/components/CreateTaskStatusPicker/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/index.tspackages/trpc/src/router/task/schema.tspackages/trpc/src/router/task/task.ts
Comment on lines
+249
to
+255
| handleKeyDown: (_, event) => { | ||
| if ((event.metaKey || event.ctrlKey) && event.key === "Enter") { | ||
| onModEnter?.(); | ||
| return true; | ||
| } | ||
| return false; | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the TaskMarkdownRenderer component
fd "TaskMarkdownRenderer" --type fRepository: superset-sh/superset
Length of output: 193
🏁 Script executed:
# Search for the prop interface/type definition and usages
rg -A 15 "interface.*TaskMarkdownRenderer.*Props|type.*TaskMarkdownRenderer.*Props" apps/desktop/srcRepository: superset-sh/superset
Length of output: 2605
🏁 Script executed:
# Search for usages of TaskMarkdownRenderer to see where onModEnter is/isn't provided
rg "TaskMarkdownRenderer" apps/desktop/src --type tsx --type ts -B 2 -A 5Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Search for usages of TaskMarkdownRenderer
rg "TaskMarkdownRenderer" apps/desktop/src -A 5 -B 2Repository: superset-sh/superset
Length of output: 9181
🏁 Script executed:
# Get the full component to understand the current implementation
cat -n apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/components/TaskMarkdownRenderer/TaskMarkdownRenderer.tsx | head -280 | tail -50Repository: superset-sh/superset
Length of output: 218
🏁 Script executed:
# Read the TaskMarkdownRenderer component file
cat -n apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/\$taskId/components/TaskMarkdownRenderer/TaskMarkdownRenderer.tsx | sed -n '240,260p'Repository: superset-sh/superset
Length of output: 654
🏁 Script executed:
# Get more context around the handleKeyDown implementation
cat -n apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/\$taskId/components/TaskMarkdownRenderer/TaskMarkdownRenderer.tsx | sed -n '240,270p'Repository: superset-sh/superset
Length of output: 923
Only consume Mod+Enter when a handler is actually provided.
In the task detail view (page.tsx), onModEnter is omitted, but the current implementation still returns true for Mod+Enter, preventing the editor's default behavior. Gate this interception on onModEnter so only the create dialog overrides it.
Suggested fix
handleKeyDown: (_, event) => {
- if ((event.metaKey || event.ctrlKey) && event.key === "Enter") {
- onModEnter?.();
+ if (
+ (event.metaKey || event.ctrlKey) &&
+ event.key === "Enter" &&
+ onModEnter
+ ) {
+ onModEnter();
return true;
}
return false;
},📝 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.
Suggested change
| handleKeyDown: (_, event) => { | |
| if ((event.metaKey || event.ctrlKey) && event.key === "Enter") { | |
| onModEnter?.(); | |
| return true; | |
| } | |
| return false; | |
| }, | |
| handleKeyDown: (_, event) => { | |
| if ( | |
| (event.metaKey || event.ctrlKey) && | |
| event.key === "Enter" && | |
| onModEnter | |
| ) { | |
| onModEnter(); | |
| return true; | |
| } | |
| return false; | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/`$taskId/components/TaskMarkdownRenderer/TaskMarkdownRenderer.tsx
around lines 249 - 255, The keydown handler in TaskMarkdownRenderer
(handleKeyDown) currently consumes Mod+Enter by returning true even when
onModEnter is undefined, blocking the editor's default behavior; update
handleKeyDown to first check that onModEnter is provided before calling
onModEnter() and returning true (i.e., only call onModEnter?.() and return true
when onModEnter is a function), otherwise let the event fall through by
returning false—this ensures the interception is gated on the onModEnter prop in
TaskMarkdownRenderer/handleKeyDown.
Comment on lines
+15
to
+19
| interface CreateTaskAssigneePickerProps { | ||
| users: SelectUser[]; | ||
| value: string | null; | ||
| onChange: (value: string | null) => void; | ||
| } |
Contributor
There was a problem hiding this comment.
Scope users to the active organization before rendering this picker.
This component displays name/email for every passed user and allows selecting them. The current call site (CreateTaskDialog.tsx, Line 65-81 and Line 250-254) passes an unscoped users collection, which can leak cross-organization identities and permit invalid assignee choices.
Please enforce org scoping before passing users (or add an explicit guard here as a defensive fallback).
Also applies to: 104-114
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/components/CreateTaskAssigneePicker/CreateTaskAssigneePicker.tsx`
around lines 15 - 19, The users prop is not scoped to the active organization
and can leak cross-org identities; either (A) scope the users list at the call
sites before passing it into CreateTaskAssigneePicker (filter the array to users
whose organizationId matches the current active organization id) or (B) add a
defensive guard inside CreateTaskAssigneePicker: obtain the active org id (from
the same context/hooks the app uses for current org), filter the incoming users
prop (SelectUser[]) to only include users with that organizationId, and
render/select only from that filtered list (or return null/placeholder if no
matching users). Update both usages referenced (the CreateTaskDialog call sites
around the two locations noted) and the logic inside CreateTaskAssigneePicker
(and the similar block at lines ~104-114) so assignee choices are always limited
to the active organization.
Comment on lines
+50
to
+53
| <StatusIcon | ||
| type={currentStatus.type as StatusType} | ||
| color={currentStatus.color} | ||
| progress={currentStatus.progressPercent ?? undefined} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate StatusType definition =="
status_icon_file="$(fd 'StatusIcon.tsx$' | head -n 1)"
rg -n "type StatusType|export type StatusType" "$status_icon_file" -A10 -B2
echo
echo "== Locate SelectTaskStatus / task status type definition =="
rg -n "SelectTaskStatus|task_status|status.*type" packages/db/src -g '*.ts' -A6 -B3Repository: superset-sh/superset
Length of output: 3128
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find taskStatusEnumValues definition =="
rg -n "taskStatusEnumValues" packages/db/src -g '*.ts' -A10 -B2
echo
echo "== Read CreateTaskStatusPicker.tsx to see currentStatus type =="
file="apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/components/CreateTaskStatusPicker/CreateTaskStatusPicker.tsx"
wc -l "$file"
cat -n "$file" | head -80Repository: superset-sh/superset
Length of output: 6025
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Check if type mapping/guard exists elsewhere =="
rg "statusEnumToUI|StatusType|TaskStatus" apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks -g '*.ts' -g '*.tsx' -B2 -A2 | grep -E "(map|convert|guard|translate)" || echo "(no existing mapping found)"
echo
echo "== Verify DB schema taskStatuses type field =="
rg -n "type:" packages/db/src/schema/schema.ts -A2 -B2 | grep -A3 -B3 "taskStatuses"Repository: superset-sh/superset
Length of output: 190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Get full taskStatuses table definition =="
rg -n "export const taskStatuses = pgTable" packages/db/src/schema/schema.ts -A40 | head -50Repository: superset-sh/superset
Length of output: 1403
Add explicit type mapping to replace the as StatusType cast at line 51.
The cast bypasses type safety—SelectTaskStatus.type is just text() in the DB schema with no enum constraint, and there's a mismatch between the DB type field comment, the 8-value taskStatusEnumValues, and the 5-value StatusType union. Without an explicit mapper or type guard, invalid or unmapped status values can silently pass to StatusIcon, breaking the type contract. Create a type guard or mapper function that validates the conversion and fails at compile time if the contracts diverge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/components/CreateTaskStatusPicker/CreateTaskStatusPicker.tsx`
around lines 50 - 53, Replace the unsafe cast at the StatusIcon call by
introducing an explicit mapper or type guard that converts SelectTaskStatus.type
into the safe StatusType union: add a function (e.g., mapTaskStatusToStatusType
or isValidStatusType) that takes currentStatus.type, checks it against a
constant, exhaustively-typed mapping derived from taskStatusEnumValues (use a
typed const object/Record and literal union) and returns the corresponding
StatusType or throws/returns a fallback; then use that function when rendering
StatusIcon instead of "as StatusType" to ensure mismatches between
SelectTaskStatus.type, taskStatusEnumValues, and StatusType are caught at
compile time.
Comment on lines
+105
to
+114
| useEffect(() => { | ||
| if (open) return; | ||
|
|
||
| setTitle(""); | ||
| setDescription(""); | ||
| setStatusId(defaultStatusId); | ||
| setPriority("none"); | ||
| setAssigneeId(null); | ||
| setIsCreating(false); | ||
| }, [defaultStatusId, open]); |
Contributor
There was a problem hiding this comment.
Keep the dialog non-dismissible while isCreating is true.
The close button is disabled, but Lines 105-114 reset the form on close and this content block does not block the other dismissal paths. That lets a user click outside or press Esc, reopen the modal, and submit again while the first mutation is still in flight.
Also applies to: 176-184
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/CreateTaskDialog.tsx`
around lines 105 - 114, The reset useEffect in CreateTaskDialog currently clears
form state whenever open becomes false, which allows dismissal via overlay/Esc
during an in-flight mutation; change the guard to skip resetting while
isCreating is true (e.g., if (open || isCreating) return;) and add isCreating to
the effect dependencies so the reset only runs when the dialog is closed and not
creating; apply the same change to the other reset effect (the block at
176-184). Also ensure any modal close handler/prop used by this dialog checks
isCreating and no-ops (prevent closing) while isCreating is true so
overlay/Esc/close-button cannot dismiss during a pending create.
Comment on lines
+116
to
+126
| const waitForTaskSync = async (taskId: string) => { | ||
| for (let attempt = 0; attempt < 20; attempt += 1) { | ||
| if (collections.tasks.get(taskId)) { | ||
| return; | ||
| } | ||
|
|
||
| await new Promise((resolve) => { | ||
| window.setTimeout(resolve, 100); | ||
| }); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
The sync wait doesn't actually guard the navigation path.
waitForTaskSync returns after 2 seconds whether or not the task ever appeared in collections.tasks, and Lines 160-166 navigate unconditionally. On a slow replication, this still closes the dialog and routes to a record the local store does not have yet. Please return a success flag (or throw) on timeout and handle that case explicitly.
Also applies to: 153-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/CreateTaskDialog.tsx`
around lines 116 - 126, The waitForTaskSync helper (waitForTaskSync) currently
always returns after ~2s even if the task never appears; change it to return a
boolean (true on found, false on timeout) or throw an error when the task isn't
found after the retry loop, and update the caller(s) that create and navigate to
the new task so they only close the CreateTaskDialog and call navigateToTask
(the navigation logic) when waitForTaskSync indicates success; if it returns
false/throws, keep the dialog open and surface an error/toast instead of
navigating.
Comment on lines
+141
to
+147
| const result = await apiTrpcClient.task.createFromUi.mutate({ | ||
| title: title.trim(), | ||
| description: description.trim() || null, | ||
| statusId, | ||
| priority, | ||
| assigneeId, | ||
| }); |
Contributor
There was a problem hiding this comment.
Preserve the editor's Markdown verbatim.
Line 143 trims the description before sending it, which strips meaningful leading indentation and trailing two-space hard breaks. Only collapse the all-whitespace case to null; otherwise send the raw editor output.
Suggested fix
const result = await apiTrpcClient.task.createFromUi.mutate({
title: title.trim(),
- description: description.trim() || null,
+ description: description.trim() === "" ? null : description,
statusId,
priority,
assigneeId,
});📝 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.
Suggested change
| const result = await apiTrpcClient.task.createFromUi.mutate({ | |
| title: title.trim(), | |
| description: description.trim() || null, | |
| statusId, | |
| priority, | |
| assigneeId, | |
| }); | |
| const result = await apiTrpcClient.task.createFromUi.mutate({ | |
| title: title.trim(), | |
| description: description.trim() === "" ? null : description, | |
| statusId, | |
| priority, | |
| assigneeId, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/CreateTaskDialog.tsx`
around lines 141 - 147, The code currently calls
apiTrpcClient.task.createFromUi.mutate with description: description.trim() ||
null which strips meaningful Markdown whitespace; change this so only the
all-whitespace case becomes null and otherwise send the raw editor content. In
the CreateTaskDialog (use the description variable passed to
apiTrpcClient.task.createFromUi.mutate), replace the trim-based conditional with
a check like "if description contains any non-whitespace then send description
as-is else send null" so title, statusId, priority, assigneeId remain unchanged
and only pure-whitespace descriptions become null.
Comment on lines
+153
to
+178
| <button | ||
| type="button" | ||
| title="Table view" | ||
| className={cn( | ||
| "flex items-center justify-center size-6 rounded-sm transition-colors", | ||
| viewMode === "table" | ||
| ? "bg-background shadow-sm text-foreground" | ||
| : "text-muted-foreground hover:text-foreground", | ||
| )} | ||
| onClick={() => onViewModeChange("table")} | ||
| > | ||
| <HiOutlineQueueList className="size-3.5" /> | ||
| </button> | ||
| <button | ||
| type="button" | ||
| title="Board view" | ||
| className={cn( | ||
| "flex items-center justify-center size-6 rounded-sm transition-colors", | ||
| viewMode === "board" | ||
| ? "bg-background shadow-sm text-foreground" | ||
| : "text-muted-foreground hover:text-foreground", | ||
| )} | ||
| onClick={() => onViewModeChange("board")} | ||
| > | ||
| <HiOutlineViewColumns className="size-3.5" /> | ||
| </button> |
Contributor
There was a problem hiding this comment.
Add explicit accessibility semantics to the view toggle buttons.
Icon-only buttons should expose an accessible name and pressed state. title is not sufficient for consistent screen-reader behavior.
Suggested fix
<button
type="button"
title="Table view"
+ aria-label="Table view"
+ aria-pressed={viewMode === "table"}
className={cn(
@@
<button
type="button"
title="Board view"
+ aria-label="Board view"
+ aria-pressed={viewMode === "board"}
className={cn(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/TasksTopBar.tsx`
around lines 153 - 178, The two icon-only toggle buttons (the Table and Board
buttons using HiOutlineQueueList and HiOutlineViewColumns) need explicit
accessibility semantics: add an accessible name via aria-label (e.g., "Table
view" and "Board view") and expose pressed state by adding
aria-pressed={viewMode === "table"} for the table button and
aria-pressed={viewMode === "board"} for the board button (keep the existing
onClick handlers that call onViewModeChange). This ensures screen readers
recognize the buttons and their active state while preserving the current
styling logic in the className/ cn usage.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/shared/src/task-slug.ts (2)
15-27: Avoid unconditionalSetcloning ingenerateUniqueTaskSlug.When callers already pass a
Set, reusing it avoids repeated copying overhead.Suggested diff
export function generateUniqueTaskSlug( baseSlug: string, - existingSlugs: Iterable<string>, + existingSlugs: Iterable<string> | ReadonlySet<string>, ): string { - const usedSlugs = new Set(existingSlugs); + const usedSlugs = + existingSlugs instanceof Set ? existingSlugs : new Set(existingSlugs);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/task-slug.ts` around lines 15 - 27, The function generateUniqueTaskSlug unnecessarily clones existingSlugs into a new Set every time; change the usedSlugs initialization to reuse an incoming Set when possible by checking if existingSlugs is a Set (e.g., existingSlugs instanceof Set) and only creating a new Set when it is not (e.g., when it's an array or iterable), leaving the rest of the logic (baseSlug, counter, slug, while loop) unchanged so callers that already pass a Set avoid the copy.
2-8: Consider trimming once more after truncation.Truncation can end on
-; a final trim keeps cleaner slugs.Suggested diff
export function generateBaseTaskSlug(title: string): string { const slug = title .toLowerCase() .replace(/[^a-z0-9]+/g, "-") .replace(/^-|-$/g, "") - .slice(0, 50); + .slice(0, 50) + .replace(/-$/g, ""); return slug || "task"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/shared/src/task-slug.ts` around lines 2 - 8, The slug generation can end with a trailing hyphen after the truncation step; update the pipeline around the slug variable (the chain starting with title.toLowerCase() and using .replace(/[^a-z0-9]+/g, "-").replace(/^-|-$/g, "").slice(0, 50)) to perform a final trim of leading/trailing hyphens after the .slice(0, 50) operation (i.e., run the .replace(/^-|-$/g, "") again or equivalent) so the truncated slug doesn’t end with "-" while preserving the fallback return of "task".apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/page.tsx (2)
101-115: Differentiate fetch failure from true “not found.”If fallback fails (network/server error), this path ends at “Task not found,” which is misleading. Add an explicit error UI branch before the not-found fallback.
💡 Suggested change
if (!task) { if (isTaskLoading || isTaskSyncing) { return ( <div className="flex-1 flex items-center justify-center"> <span className="text-muted-foreground"> {isTaskSyncing ? "Syncing task..." : "Loading task..."} </span> </div> ); } + if (taskFallbackQuery.isError) { + return ( + <div className="flex-1 flex items-center justify-center"> + <span className="text-muted-foreground">Failed to load task</span> + </div> + ); + } return ( <div className="flex-1 flex items-center justify-center"> <span className="text-muted-foreground">Task not found</span> </div> ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/`$taskId/page.tsx around lines 101 - 115, The current render block treats any missing task as "Task not found" even when the fetch failed; update the conditional to check for a fetch error first and show an explicit error UI (e.g., "Failed to load task" with error details and a retry action) before falling back to the "Task not found" message: use or add an error state from your data hook (e.g., isTaskError or taskError) and an associated refetch method (e.g., refetchTask) and place the error branch before the not-found branch in the render logic that currently references task, isTaskLoading, and isTaskSyncing so network/server failures are clearly distinguished from a true 404.
33-36: Use a shared UUID validator from Zod instead of an inline regex.The current regex duplicates validation logic already present in MCP tools (
z.string().uuid().safeParse()) and the server schemas. Maintaining separate validators risks routing errors—if UUID validation fails, requests incorrectly route tobySlug. Extract a sharedisValidUuid()utility from Zod's uuid validator inpackages/sharedand use it consistently across the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/`$taskId/page.tsx around lines 33 - 36, Replace the inline UUID regex check for isUuidTaskId with a shared Zod-based validator: remove the regex usage in the tasks page (variable isUuidTaskId) and call the new shared utility isValidUuid(taskId) (implemented using z.string().uuid().safeParse under packages/shared); ensure the page imports isValidUuid and uses its boolean result to decide routing so UUID validation is consistent with server schemas and other clients.
🤖 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/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/`$taskId/page.tsx:
- Around line 71-79: The fallback query (taskFallbackQuery) is firing while the
primary lookup that sets task is still pending; change the enabled condition to
only run after the primary lookup has conclusively missed by gating on the
primary query's settled/miss state (e.g., use the primary lookup hook that
populates task — the "task" query — and set enabled: primaryQuery.isSettled &&
!task or enabled: primaryQuery.isFetched && !task or enabled:
primaryQuery.isSuccess === false && !task); update the enabled expression for
taskFallbackQuery accordingly and keep using isUuidTaskId to choose
apiTrpcClient.task.byId.query / apiTrpcClient.task.bySlug.query.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/`$taskId/page.tsx:
- Around line 101-115: The current render block treats any missing task as "Task
not found" even when the fetch failed; update the conditional to check for a
fetch error first and show an explicit error UI (e.g., "Failed to load task"
with error details and a retry action) before falling back to the "Task not
found" message: use or add an error state from your data hook (e.g., isTaskError
or taskError) and an associated refetch method (e.g., refetchTask) and place the
error branch before the not-found branch in the render logic that currently
references task, isTaskLoading, and isTaskSyncing so network/server failures are
clearly distinguished from a true 404.
- Around line 33-36: Replace the inline UUID regex check for isUuidTaskId with a
shared Zod-based validator: remove the regex usage in the tasks page (variable
isUuidTaskId) and call the new shared utility isValidUuid(taskId) (implemented
using z.string().uuid().safeParse under packages/shared); ensure the page
imports isValidUuid and uses its boolean result to decide routing so UUID
validation is consistent with server schemas and other clients.
In `@packages/shared/src/task-slug.ts`:
- Around line 15-27: The function generateUniqueTaskSlug unnecessarily clones
existingSlugs into a new Set every time; change the usedSlugs initialization to
reuse an incoming Set when possible by checking if existingSlugs is a Set (e.g.,
existingSlugs instanceof Set) and only creating a new Set when it is not (e.g.,
when it's an array or iterable), leaving the rest of the logic (baseSlug,
counter, slug, while loop) unchanged so callers that already pass a Set avoid
the copy.
- Around line 2-8: The slug generation can end with a trailing hyphen after the
truncation step; update the pipeline around the slug variable (the chain
starting with title.toLowerCase() and using .replace(/[^a-z0-9]+/g,
"-").replace(/^-|-$/g, "").slice(0, 50)) to perform a final trim of
leading/trailing hyphens after the .slice(0, 50) operation (i.e., run the
.replace(/^-|-$/g, "") again or equivalent) so the truncated slug doesn’t end
with "-" while preserving the fallback return of "task".
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6762fdbc-0d65-4db8-bfbf-3737646ee88f
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/$taskId/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/CreateTaskDialog.tsxpackages/mcp/src/tools/tasks/create-task/create-task.tspackages/shared/package.jsonpackages/shared/src/task-slug.test.tspackages/shared/src/task-slug.tspackages/trpc/src/router/task/task.ts
✅ Files skipped from review due to trivial changes (2)
- packages/shared/src/task-slug.test.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/CreateTaskDialog.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/trpc/src/router/task/task.ts
Comment on lines
+71
to
+79
| const taskFallbackQuery = useQuery({ | ||
| queryKey: ["task-detail-fallback", taskId, isUuidTaskId ? "id" : "slug"], | ||
| queryFn: () => | ||
| isUuidTaskId | ||
| ? apiTrpcClient.task.byId.query(taskId) | ||
| : apiTrpcClient.task.bySlug.query(taskId), | ||
| enabled: !task, | ||
| retry: false, | ||
| }); |
Contributor
There was a problem hiding this comment.
Fallback query currently starts before the primary lookup has conclusively missed.
On Line 77, enabled: !task is also true while the live query is still unresolved, so the fallback RPC can fire eagerly. Gate this on an explicit primary “miss” state to avoid avoidable network calls.
💡 Suggested change
- enabled: !task,
+ enabled: taskData !== undefined && taskData.length === 0,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/`$taskId/page.tsx
around lines 71 - 79, The fallback query (taskFallbackQuery) is firing while the
primary lookup that sets task is still pending; change the enabled condition to
only run after the primary lookup has conclusively missed by gating on the
primary query's settled/miss state (e.g., use the primary lookup hook that
populates task — the "task" query — and set enabled: primaryQuery.isSettled &&
!task or enabled: primaryQuery.isFetched && !task or enabled:
primaryQuery.isSuccess === false && !task); update the enabled expression for
taskFallbackQuery accordingly and keep using isUuidTaskId to choose
apiTrpcClient.task.byId.query / apiTrpcClient.task.bySlug.query.
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
1 participant
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.
Summary
task.createFromUimutation with server-owned slug generation, default status fallback, and post-create navigationTesting
bun run --cwd apps/desktop typecheckbunx @biomejs/biome@2.4.2 check --write apps/desktop/src/renderer/routes/_authenticated/_dashboard/tasks/components/TasksView/components/TasksTopBar/components/CreateTaskDialog/CreateTaskDialog.tsxSummary by cubic
Adds a desktop “New task” modal in Tasks with a TipTap editor and a hardened
task.createFromUiflow. After create we close the modal, toast, preserve tab/filter/search, and navigate to the task with a loading/sync fallback.New Features
TaskMarkdownRenderernow supports placeholder override, autofocus, onChange, Mod+Enter callback, className/editorClassName, and external content sync.Bug Fixes
task.createFromUivalidates the active org, seeds a default status, checks assignee membership, generates unique slugs via@superset/shared/task-slugwith retry on conflicts, and triggers sync.Written for commit 04c1542. Summary will update on new commits.
Summary by CodeRabbit