-
Notifications
You must be signed in to change notification settings - Fork 11
Refactor/workspace api routes #22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
5b7bbe9
5b0b40b
04470bd
4ff1059
f530d17
f2edd15
f83e771
16a01fa
6ff442a
482d2fc
d0e88c9
f5537f6
70f9d7d
db0cd57
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,56 +1,36 @@ | ||
| import { NextRequest, NextResponse } from "next/server"; | ||
| import { headers } from "next/headers"; | ||
| import { auth } from "@/lib/auth"; | ||
| import type { WorkspaceEvent, EventResponse } from "@/lib/workspace/events"; | ||
| import { checkAndCreateSnapshot } from "@/lib/workspace/snapshot-manager"; | ||
| import { db, workspaces, workspaceEvents, workspaceSnapshots } from "@/lib/db/client"; | ||
| import { eq, gt, desc, asc, sql, and } from "drizzle-orm"; | ||
| import { db, workspaceEvents } from "@/lib/db/client"; | ||
| import { eq, gt, asc, sql, and } from "drizzle-orm"; | ||
| import { requireAuth, verifyWorkspaceOwnership, withErrorHandling } from "@/lib/api/workspace-helpers"; | ||
|
|
||
| /** | ||
| * GET /api/workspaces/[id]/events | ||
| * Fetch all events for a workspace (owner only) | ||
| */ | ||
| export async function GET( | ||
| async function handleGET( | ||
| request: NextRequest, | ||
| { params }: { params: Promise<{ id: string }> } | ||
| ) { | ||
| const startTime = Date.now(); | ||
| const timings: Record<string, number> = {}; | ||
| let id: string | undefined; | ||
|
|
||
| try { | ||
| const paramsResolved = await params; | ||
| id = paramsResolved.id; | ||
|
|
||
| const authStart = Date.now(); | ||
| const session = await auth.api.getSession({ | ||
| headers: await headers(), | ||
| }); | ||
| timings.auth = Date.now() - authStart; | ||
|
|
||
| if (!session) { | ||
| return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); | ||
| } | ||
|
|
||
| const userId = session.user.id; | ||
|
|
||
| // Check if user is workspace owner | ||
| const workspaceCheckStart = Date.now(); | ||
| const workspace = await db | ||
| .select({ userId: workspaces.userId }) | ||
| .from(workspaces) | ||
| .where(eq(workspaces.id, id)) | ||
| .limit(1); | ||
| timings.workspaceCheck = Date.now() - workspaceCheckStart; | ||
|
|
||
| if (!workspace[0]) { | ||
| return NextResponse.json({ error: "Workspace not found" }, { status: 404 }); | ||
| } | ||
|
|
||
| // Enforce strict ownership (sharing is fork-based) | ||
| if (workspace[0].userId !== userId) { | ||
| return NextResponse.json({ error: "Access denied" }, { status: 403 }); | ||
| } | ||
|
|
||
| // Start independent operations in parallel | ||
| const paramsPromise = params; | ||
| const authPromise = requireAuth(); | ||
|
|
||
| const paramsResolved = await paramsPromise; | ||
| const id = paramsResolved.id; | ||
|
|
||
| const authStart = Date.now(); | ||
| const userId = await authPromise; | ||
| timings.auth = Date.now() - authStart; | ||
|
Comment on lines
+26
to
+28
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. logic: timing measurement incorrect - measures only promise wait time, not actual auth duration since auth already started at line 21 Prompt To Fix With AIThis is a comment left during a code review.
Path: src/app/api/workspaces/[id]/events/route.ts
Line: 26:28
Comment:
**logic:** timing measurement incorrect - measures only promise wait time, not actual auth duration since auth already started at line 21
How can I resolve this? If you propose a fix, please make it concise. |
||
|
|
||
| // Check if user is workspace owner | ||
| const workspaceCheckStart = Date.now(); | ||
| await verifyWorkspaceOwnership(id, userId); | ||
| timings.workspaceCheck = Date.now() - workspaceCheckStart; | ||
|
|
||
| // Get only the latest snapshot (not all snapshots - loaded on demand for version history) | ||
| // Use optimized function that bypasses RLS (access already verified above) | ||
|
|
@@ -207,71 +187,50 @@ export async function GET( | |
| const totalTime = Date.now() - startTime; | ||
| timings.total = totalTime; | ||
|
|
||
| return NextResponse.json(response); | ||
| } catch (error) { | ||
| const totalTime = Date.now() - startTime; | ||
| console.error(`[GET /api/workspaces/${id || '[unknown]'}/events] Error after ${totalTime}ms:`, error); | ||
| return NextResponse.json({ error: "Internal server error" }, { status: 500 }); | ||
| } | ||
| return NextResponse.json(response); | ||
| } | ||
|
|
||
| export const GET = withErrorHandling(handleGET, "GET /api/workspaces/[id]/events"); | ||
|
|
||
| /** | ||
| * POST /api/workspaces/[id]/events | ||
| * Append new event(s) to workspace event log (owner only) | ||
| */ | ||
| export async function POST( | ||
| async function handlePOST( | ||
| request: NextRequest, | ||
| { params }: { params: Promise<{ id: string }> } | ||
| ) { | ||
| const startTime = Date.now(); | ||
| const timings: Record<string, number> = {}; | ||
| let id: string | undefined; | ||
|
|
||
| try { | ||
| const paramsResolved = await params; | ||
| id = paramsResolved.id; | ||
|
|
||
| const authStart = Date.now(); | ||
| const session = await auth.api.getSession({ | ||
| headers: await headers(), | ||
| }); | ||
| timings.auth = Date.now() - authStart; | ||
|
|
||
| if (!session) { | ||
| return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); | ||
| } | ||
|
|
||
| const userId = session.user.id; | ||
|
|
||
| const bodyStart = Date.now(); | ||
| const body = await request.json(); | ||
| timings.bodyParse = Date.now() - bodyStart; | ||
| const { event, baseVersion } = body; | ||
|
|
||
| if (!event || baseVersion === undefined || isNaN(baseVersion)) { | ||
| return NextResponse.json( | ||
| { error: "Event and valid baseVersion are required" }, | ||
| { status: 400 } | ||
| ); | ||
| } | ||
|
|
||
| // Check if user is workspace owner | ||
| const workspaceCheckStart = Date.now(); | ||
| const workspace = await db | ||
| .select({ userId: workspaces.userId }) | ||
| .from(workspaces) | ||
| .where(eq(workspaces.id, id)) | ||
| .limit(1); | ||
| timings.workspaceCheck = Date.now() - workspaceCheckStart; | ||
|
|
||
| if (!workspace[0]) { | ||
| return NextResponse.json({ error: "Workspace not found" }, { status: 404 }); | ||
| } | ||
|
|
||
| // Start independent operations in parallel | ||
| const paramsPromise = params; | ||
| const authPromise = requireAuth(); | ||
| const bodyPromise = request.json(); | ||
|
|
||
| const paramsResolved = await paramsPromise; | ||
| const id = paramsResolved.id; | ||
|
|
||
| const authStart = Date.now(); | ||
| const userId = await authPromise; | ||
| timings.auth = Date.now() - authStart; | ||
|
|
||
| const bodyStart = Date.now(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P2: Timing measurement is inaccurate. Prompt for AI agents |
||
| const body = await bodyPromise; | ||
| timings.bodyParse = Date.now() - bodyStart; | ||
| const { event, baseVersion } = body; | ||
|
|
||
| if (!event || baseVersion === undefined || isNaN(baseVersion)) { | ||
| return NextResponse.json( | ||
| { error: "Event and valid baseVersion are required" }, | ||
| { status: 400 } | ||
| ); | ||
| } | ||
|
|
||
| // Enforce strict ownership (sharing is fork-based) | ||
| if (workspace[0].userId !== userId) { | ||
| return NextResponse.json({ error: "Access denied" }, { status: 403 }); | ||
| } | ||
| // Check if user is workspace owner | ||
| const workspaceCheckStart = Date.now(); | ||
| await verifyWorkspaceOwnership(id, userId); | ||
| timings.workspaceCheck = Date.now() - workspaceCheckStart; | ||
|
|
||
| // Use the append function to handle versioning and conflicts | ||
| const appendStart = Date.now(); | ||
|
|
@@ -353,15 +312,12 @@ export async function POST( | |
| const totalTime = Date.now() - startTime; | ||
| timings.total = totalTime; | ||
|
|
||
| return NextResponse.json({ | ||
| success: true, | ||
| version: appendResult.version, | ||
| conflict: false, | ||
| }); | ||
| } catch (error) { | ||
| const totalTime = Date.now() - startTime; | ||
| console.error(`[POST /api/workspaces/${id || '[unknown]'}/events] Error after ${totalTime}ms:`, error); | ||
| return NextResponse.json({ error: "Internal server error" }, { status: 500 }); | ||
| } | ||
| return NextResponse.json({ | ||
| success: true, | ||
| version: appendResult.version, | ||
| conflict: false, | ||
| }); | ||
| } | ||
|
|
||
| export const POST = withErrorHandling(handlePOST, "POST /api/workspaces/[id]/events"); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Timing measurement is inaccurate.
authStartis set afterrequireAuth()has already started executing (it's a promise that begins immediately). This will underreport auth time since it only measures remaining wait time, not total execution time. Moveconst authStart = Date.now();back to beforeconst authPromise = requireAuth();.Prompt for AI agents