Add keyboard shortcut for creating tasks #172
Conversation
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughReworks permission helpers to accept a PermissionContext and short-circuit org admins; updates TRPC routers to call the new signatures and return TRPCError on denials. Adds a Kbd component and an "N" global shortcut to open the inline task form; expands CLAUDE.md with strict build/test restrictions. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Browser
participant TaskForm
User->>Browser: Press "N"
Browser->>TaskForm: keydown (N) handler (ignores inputs/contentEditable)
TaskForm->>TaskForm: setIsCreating(true)
User->>TaskForm: Submit task
TaskForm->>TaskForm: save -> setIsCreating(false), clear input
sequenceDiagram
participant Client
participant TRPCRouter
participant Permissions
participant DB
Client->>TRPCRouter: Project action (e.g., update)
TRPCRouter->>Permissions: canEditProject(ctx, projectId)
Permissions->>Permissions: if ctx.isOrgAdmin then return true
Permissions->>DB: checkProjectPermission(ctx.db, ctx.userId)
DB-->>Permissions: permission result
Permissions-->>TRPCRouter: allow / deny
TRPCRouter-->>Client: proceed or throw TRPCError FORBIDDEN
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
components/form/task.tsx (1)
40-44: Close-on-submit without error handling risks data loss and double-submit.If action() throws, the dialog still closes and input clears. Also no submit in-flight guard. Recommend adding isSaving + try/catch and disabling Save/Enter until completion.
Outside the changed lines, adapt as follows:
// state const [isSaving, setIsSaving] = useState(false); // handler const handleSubmit = useCallback(async () => { if (isSaving) return; const name = value.trim(); if (!name) return; setIsSaving(true); try { await action(name); setValue(""); setIsCreating(false); } catch (err) { // TODO: surface a toast or error state console.error("Failed to create task", err); } finally { setIsSaving(false); } }, [action, value, isSaving]); // input Enter handler onKeyDown={(e) => { if (e.key === "Enter" && !e.shiftKey) { e.preventDefault(); handleSubmit(); } }} // Save button <Button type="button" onClick={handleSubmit} disabled={isSaving || !value.trim()}> {isSaving ? "Saving..." : "Save"} </Button>trpc/routers/events.ts (1)
126-136: Missing permission check in getByMonth exposes events across projectsUnlike getByDate/getByWeek, getByMonth performs no access check. This can leak event data to unauthorized users.
Apply this diff to enforce view permissions:
.query(async ({ ctx, input }) => { const { date, projectId } = input; + // Check if user has permission to view this project + const hasAccess = await canViewProject(ctx, projectId); + if (!hasAccess) { + throw new Error("Project access denied"); + } + const { start, end } = getStartEndMonthRangeInUtc(ctx.timezone, date);If you adopt TRPCError (see separate comment), switch the throw to TRPCError with FORBIDDEN.
🧹 Nitpick comments (5)
CLAUDE.md (1)
150-226: Clarify audience and scope; otherwise solid guidance.The new “CRITICAL BUILD AND TEST RESTRICTIONS” section is clear and aligns with the repository’s long-term learning about not rebuilding unless asked. Two small improvements:
- Make it explicit this policy applies to assistants/automation (not human contributors), to avoid misinterpretation by new devs scanning docs.
- If this guidance appears elsewhere in the file, deduplicate and link to a single canonical section to reduce drift.
This aligns with the retrieved learning: “Do not build the app after every change… only rebuild when asked or after large changes.”
components/ui/kbd.tsx (1)
4-16: Type the component to the intrinsic "kbd" element; minor class cleanup.Using HTMLElement works, but typing to the intrinsic element improves correctness and IntelliSense. Also, “opacity-100” is redundant.
Apply:
-const Kbd = React.forwardRef< - HTMLElement, - React.HTMLAttributes<HTMLElement> ->(({ className, ...props }, ref) => ( +const Kbd = React.forwardRef< + React.ElementRef<"kbd">, + React.ComponentPropsWithoutRef<"kbd"> +>(({ className, ...props }, ref) => ( <kbd ref={ref} className={cn( - "pointer-events-none inline-flex h-5 select-none items-center gap-1 rounded border bg-muted px-1.5 font-mono text-[10px] font-medium text-muted-foreground opacity-100", + "pointer-events-none inline-flex h-5 select-none items-center gap-1 rounded border bg-muted px-1.5 font-mono text-[10px] font-medium text-muted-foreground", className, )} {...props} /> ))components/form/task.tsx (1)
49-61: Expose the shortcut to assistive tech.Add aria-keyshortcuts so screen readers can discover the “N” shortcut. Keep the visual Kbd.
- <Button + <Button type="button" size="sm" + aria-keyshortcuts="N" onClick={(e) => { e.preventDefault(); setIsCreating(true); }} className="flex items-center gap-2" > Add task <Kbd className="hidden md:inline-flex">N</Kbd> </Button>trpc/routers/events.ts (1)
104-112: Use computed start-of-week for full weekly coverageCurrently start is now, which omits earlier days in the same week. Use the helper’s computed start for the entire week window.
- const now = new Date(); - const { end } = getStartEndWeekRangeInUtc( - ctx.timezone, - now, - ); + const { start, end } = getStartEndWeekRangeInUtc( + ctx.timezone, + new Date(), + ); const events = await ctx.db.query.calendarEvent - .findMany(buildEventsQuery(projectId, now, end)) + .findMany(buildEventsQuery(projectId, start, end)) .execute();lib/permissions/index.ts (1)
48-66: Add per-request memoization to cut duplicate DB callsRouters often call canViewProject/canEditProject multiple times in a single request. Memoize results per (userId, projectId) to avoid redundant queries.
Example approach (conceptual):
- Add a Map<string, boolean> on ctx (e.g., ctx.permissionCache).
- Key format:
${ctx.userId}:${projectId}:view|edit.- Check cache before queries; write-through on miss.
This keeps behavior the same while reducing DB overhead under repeated checks.
Also applies to: 68-86
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
CLAUDE.md(1 hunks)components/form/task.tsx(3 hunks)components/ui/kbd.tsx(1 hunks)lib/permissions/index.ts(2 hunks)trpc/routers/events.ts(4 hunks)trpc/routers/projects.ts(5 hunks)trpc/routers/tasks.ts(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-07T13:08:07.363Z
Learnt from: CR
PR: techulus/manage#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-07T13:08:07.363Z
Learning: Do not build the app after every change you make, only rebuild the app when asked or after making large changes spanning many files
Applied to files:
CLAUDE.md
🧬 Code Graph Analysis (5)
trpc/routers/tasks.ts (1)
lib/permissions/index.ts (2)
canViewProject(68-86)canEditProject(48-66)
trpc/routers/events.ts (1)
lib/permissions/index.ts (2)
canViewProject(68-86)canEditProject(48-66)
trpc/routers/projects.ts (1)
lib/permissions/index.ts (2)
canEditProject(48-66)canViewProject(68-86)
lib/permissions/index.ts (2)
drizzle/types.ts (1)
Database(15-15)drizzle/schema.ts (1)
project(34-45)
components/form/task.tsx (1)
components/ui/kbd.tsx (1)
Kbd(19-19)
🔇 Additional comments (8)
trpc/routers/tasks.ts (1)
31-31: Permission API Migration Complete — No Legacy Calls DetectedAll
canViewProject/canEditProjectinvocations now use the new two-argument signature (ctx, projectId), and imports reference@/lib/permissions. A search found no remaining three-argument usages. Confirm that your TRPC context still populatesisOrgAdminas expected; otherwise, everything’s ready to merge.trpc/routers/events.ts (4)
73-76: Switched to ctx-based view permission check — good alignment with new APIUsing canViewProject(ctx, projectId) matches the new PermissionContext-based helpers and enables org-admin short-circuiting.
99-102: Consistent ctx-based permission in getByWeek — looks goodSame as above; the migration is correct and semantically equivalent with the new permission model.
156-156: Edit permission check switched to ctx — correctDeletion now benefits from org-admin bypass and consistent permission semantics.
213-213: Edit permission check switched to ctx — correctUpsert now uses the new helper signature; this is consistent with other routers.
lib/permissions/index.ts (3)
48-54: Org-admin short-circuit is correct and well-placedEarly return for admins reduces DB work and expresses intent clearly.
56-66: Permission check + creator fallback LGTMcheckProjectPermission first, then creator fallback preserves backward compatibility. Good sequencing.
69-86: View permission mirrors edit semantics correctlyAdmin bypass, explicit permission, then creator fallback — consistent and predictable.
|
@coderabbitai Please review |
|
@arjunkomath I'll conduct a comprehensive review of your PR that adds keyboard shortcuts for creating tasks. This includes the new Kbd component, keyboard shortcut implementation, and the significant permission system refactoring. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (4)
trpc/routers/tasks.ts (1)
253-286: Missing authorization check in getListById leaks datagetListById returns task list details (with tasks and user info) without verifying that the caller can view the project. This is an authorization gap.
Add a canViewProject(ctx, data.projectId) check with TRPCError:
.query(async ({ ctx, input }) => { const data = await ctx.db.query.taskList .findFirst({ where: eq(taskList.id, +input.id), with: { tasks: { orderBy: [asc(task.position)], with: { creator: { columns: { firstName: true, imageUrl: true, }, }, assignee: { columns: { firstName: true, imageUrl: true, }, }, }, }, }, }) .execute(); if (!data) { throw new Error(`TaskList with id ${input.id} not found`); } + const hasAccess = await canViewProject(ctx, data.projectId); + if (!hasAccess) { + throw new TRPCError({ + code: "FORBIDDEN", + message: "Project access denied", + }); + } return data; }),trpc/routers/events.ts (1)
131-141: Missing authorization check in getByMonth leaks calendar eventsgetByMonth does not verify view permission before returning events. Add canViewProject(ctx, projectId) and throw TRPCError on denial, mirroring getByDate/getByWeek.
.query(async ({ ctx, input }) => { const { date, projectId } = input; + const hasAccess = await canViewProject(ctx, projectId); + if (!hasAccess) { + throw new TRPCError({ + code: "FORBIDDEN", + message: "Project access denied", + }); + } + const { start, end } = getStartEndMonthRangeInUtc(ctx.timezone, date); const events = await ctx.db.query.calendarEvent .findMany(buildEventsQuery(projectId, start, end)) .execute(); return events; }),trpc/routers/projects.ts (2)
250-258: getComments lacks authorization; add view check (and consider aligning input with addComment)Anyone authenticated can fetch comments for any roomId. Add canViewProject(ctx, …) to prevent data leaks. Since addComment requires projectId already, consider accepting projectId here too for consistent checks.
- getComments: protectedProcedure - .input( - z.object({ - roomId: z.string(), - }), - ) + getComments: protectedProcedure + .input( + z.object({ + roomId: z.string(), + projectId: z.number(), + }), + ) .query(async ({ ctx, input }) => { + const hasAccess = await canViewProject(ctx, input.projectId); + if (!hasAccess) { + throw new TRPCError({ + code: "FORBIDDEN", + message: "Project access denied", + }); + } const comments = await ctx.db.query.comment.findMany({ where: eq(comment.roomId, input.roomId), orderBy: desc(comment.createdAt), with: { creator: true, }, }); return comments; }),
335-355: getActivities lacks authorization; add view checkPrevent activity feed leaks by ensuring the caller can view the project.
.query(async ({ ctx, input }) => { + const hasAccess = await canViewProject(ctx, input.projectId); + if (!hasAccess) { + throw new TRPCError({ + code: "FORBIDDEN", + message: "Project access denied", + }); + } const activities = await ctx.db.query.activity .findMany({ with: { actor: { columns: { id: true, firstName: true, imageUrl: true, }, }, }, where: eq(activity.projectId, input.projectId), orderBy: [desc(activity.createdAt)], limit: 25, offset: input.offset, }) .execute(); return activities; }),
🧹 Nitpick comments (9)
trpc/routers/tasks.ts (7)
2-2: Import looks good; consider applying TRPCError consistently across this routerYou’ve introduced TRPCError; several permission-denial paths below still throw generic Error. Follow-up comments include targeted diffs to standardize.
174-179: Prefer TRPCError for authorization failuresReturn proper error codes to clients instead of generic Error.
Apply this diff:
- throw new Error( - "You don't have permission to update task lists in this project", - ); + throw new TRPCError({ + code: "FORBIDDEN", + message: "Project edit access denied", + });
227-232: Prefer TRPCError for authorization failuresKeep error semantics consistent with the rest of the router.
- throw new Error( - "You don't have permission to delete task lists in this project", - ); + throw new TRPCError({ + code: "FORBIDDEN", + message: "Project edit access denied", + });
316-321: Prefer TRPCError for authorization failuresAlign createTask with the TRPCError usage adopted elsewhere.
- throw new Error( - "You don't have permission to add tasks to this project", - ); + throw new TRPCError({ + code: "FORBIDDEN", + message: "Project edit access denied", + });
437-442: Prefer TRPCError for authorization failuresUse FORBIDDEN for consistent client behavior.
- throw new Error( - "You don't have permission to update tasks in this project", - ); + throw new TRPCError({ + code: "FORBIDDEN", + message: "Project edit access denied", + });
508-513: Prefer TRPCError for authorization failuresConsistent error semantics across endpoints.
- throw new Error( - "You don't have permission to delete tasks in this project", - ); + throw new TRPCError({ + code: "FORBIDDEN", + message: "Project edit access denied", + });
545-550: Prefer TRPCError for authorization failuresStandardize to TRPCError(FORBIDDEN).
- throw new Error( - "You don't have permission to modify tasks in this project", - ); + throw new TRPCError({ + code: "FORBIDDEN", + message: "Project edit access denied", + });trpc/routers/projects.ts (2)
271-276: Prefer TRPCError for authorization failures in addCommentStandardize error semantics across the router.
- throw new Error( - "You don't have permission to add comments to this project", - ); + throw new TRPCError({ + code: "FORBIDDEN", + message: "Project edit access denied", + });
320-327: Ensure Authorization on Comment DeletionThe
deleteCommentmutation currently trusts the caller’sinput.projectId, allowing any authenticated user to delete arbitrary comments by supplying a project they can edit. Instead, we must:
- Fetch the comment by
input.id, including itscreatedByUserandroomId.- Derive the actual
projectIdfrom the associatedRoom.- Enforce “author OR project-editor” authorization.
- Delete the comment and log activity with the derived
projectId..mutation(async ({ ctx, input }) => { - await ctx.db.delete(comment).where(eq(comment.id, input.id)); + // 1) Load comment, including its owner and room + const existing = await ctx.db.query.comment.findFirst({ + where: eq(comment.id, input.id), + columns: { createdByUser: true, roomId: true }, + }); + if (!existing) { + throw new TRPCError({ code: "NOT_FOUND", message: "Comment not found" }); + } + + // 2) Derive actual projectId from the room + const { projectId: actualProjectId } = await ctx.db.query.room.findFirstOrThrow({ + where: eq(room.id, existing.roomId), + columns: { projectId: true }, + }); + + // 3) Author‐or‐editor check + if ( + existing.createdByUser !== ctx.userId && + !(await canEditProject(ctx, actualProjectId)) + ) { + throw new TRPCError({ + code: "FORBIDDEN", + message: "Insufficient permissions to delete this comment", + }); + } + + // 4) Safe delete + await ctx.db.delete(comment).where(eq(comment.id, input.id)); await logActivity({ action: "deleted", type: "comment", - projectId: input.projectId, + projectId: actualProjectId, }); }),I wasn’t able to locate any
roomId→projectIdmapping in the codebase automatically. Please confirm which field oncommentlinks it to aRoomand how that room maps to a project so we can fully lock down this mutation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
components/form/task.tsx(3 hunks)lib/permissions/index.ts(2 hunks)trpc/routers/events.ts(5 hunks)trpc/routers/projects.ts(7 hunks)trpc/routers/tasks.ts(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/permissions/index.ts
- components/form/task.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-07T13:08:07.393Z
Learnt from: CR
PR: techulus/manage#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-07T13:08:07.393Z
Learning: Applies to trpc/routers/**/*.ts : tRPC procedures that require authentication must use `protectedProcedure`
Applied to files:
trpc/routers/events.ts
🧬 Code Graph Analysis (3)
trpc/routers/events.ts (1)
lib/permissions/index.ts (2)
canViewProject(68-86)canEditProject(48-66)
trpc/routers/tasks.ts (1)
lib/permissions/index.ts (2)
canViewProject(68-86)canEditProject(48-66)
trpc/routers/projects.ts (1)
lib/permissions/index.ts (2)
canEditProject(48-66)canViewProject(68-86)
🔇 Additional comments (12)
trpc/routers/tasks.ts (2)
31-37: Good: standardized permission check and TRPCError on view denialUsing canViewProject(ctx, …) and TRPCError(FORBIDDEN) aligns with the new PermissionContext and tRPC semantics.
74-80: Good: standardized edit check and TRPCError on denialThis matches the ctx-based permission helpers and returns structured errors to clients.
trpc/routers/events.ts (4)
1-1: Import looks good; aligns with structured error handlingTRPCError is properly imported for standardized error responses.
73-79: Good: permission check with TRPCError(FORBIDDEN) in getByDateThis matches the ctx-based helpers and consistent error semantics.
101-107: Good: permission check with TRPCError(FORBIDDEN) in getByWeekConsistent with the rest of the router.
160-166: Good: delete enforces edit permission with TRPCErrorChecks against the event’s projectId and returns FORBIDDEN on denial.
trpc/routers/projects.ts (6)
2-2: TRPCError import aligns with the new error handling approachImport is used consistently below.
36-39: Good: createProject enforces org-admin requirement with TRPCErrorClear semantics and consistent error code.
115-121: Good: updateProject uses ctx-based permission and TRPCErrorMatches the new PermissionContext helpers and standard error semantics.
175-181: Good: deleteProject uses ctx-based permission and TRPCErrorConsistent with the rest of the codebase.
210-216: Good: getProjectById uses view-permission with TRPCError(FORBIDDEN)Consistent with other routers.
235-236: Nice optimization: compute canEdit in-memory using fetched data and userRoleAvoids an extra DB round-trip. Good call.
Summary by CodeRabbit
New Features
Documentation
Refactor