feat: add branching #2763
feat: add branching #2763
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughUnified framing and branch model: removed FrameType enum and WebFrame-specific paths in favor of a single Frame with branchId; added Branch schema, BranchManager, branch TRPC router, renamed many DB mappers to directional fromDb*/toDb*, and propagated type/signature updates across client, server, and DB layers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as Client UI
participant Engine as EditorEngine
participant BranchMgr as BranchManager
participant API as TRPC Router
participant Mappers as DB Mappers
participant DB as Database
UI->>Engine: request switch/create Frame (includes branchId)
Engine->>BranchMgr: switchToBranch(branchId)
BranchMgr-->>Engine: active branch set
Engine->>API: call project/frame create/update (Frame payload)
API->>Mappers: toDbFrame(payload)
Mappers-->>API: DbFrame (x/y serialized, branchId)
API->>DB: insert/update frames (branch_id FK)
DB-->>API: DbFrame
API->>Mappers: fromDbFrame(DbFrame)
Mappers-->>API: Frame (no type, with branchId)
API-->>Engine: Frame
Engine-->>UI: render updated Frame
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 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 (
|
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| @@ -87,6 +82,6 @@ export function fromPreviewImg(previewImg: PreviewImg | null): { | |||
| res.previewImgPath = previewImg.storagePath.path; | |||
| res.previewImgBucket = previewImg.storagePath.bucket; | |||
| } | |||
| res.updatedPreviewImgAt = new Date(); | |||
| res.updatedPreviewImgAt = previewImg.updatedAt ?? new Date(); | |||
There was a problem hiding this comment.
In fromPreviewImg, updatedPreviewImgAt defaults to new Date() when missing. This may inadvertently indicate a new timestamp when none exists. Consider returning null if no update timestamp is provided.
| res.updatedPreviewImgAt = previewImg.updatedAt ?? new Date(); | |
| res.updatedPreviewImgAt = previewImg.updatedAt ?? null; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
packages/db/src/defaults/frame.ts (1)
15-15: Prevent overrides from clobbering id/canvasId/branchIdBecause ...overrides is applied last, a caller could accidentally override id, canvasId, or branchId. Filter those keys out of overrides.
Apply:
export const createDefaultFrame = (canvasId: string, branchId: string, url: string, overrides?: Partial<DbFrame>): DbFrame => { - return { + // Prevent overrides from mutating identity/ownership fields + const { id: _id, canvasId: _canvasId, branchId: _branchId, ...safeOverrides } = overrides ?? {}; + return { id: uuidv4(), canvasId, branchId, url, x: DefaultSettings.FRAME_POSITION.x.toString(), y: DefaultSettings.FRAME_POSITION.y.toString(), width: DefaultSettings.FRAME_DIMENSION.width.toString(), height: DefaultSettings.FRAME_DIMENSION.height.toString(), - ...overrides, + ...safeOverrides, }; };packages/db/src/dto/project/frame.ts (1)
34-44: Fix unsafe optional chaining and include branchId in partial mapper
- Potential runtime exception: expressions like
frame.position?.x.toString()can throw whenpositionexists butxisundefined. Same fory,width,height.- Missing
branchIdprevents partial updates from moving a frame between branches.Apply this diff:
export const fromPartialFrame = (frame: Partial<Frame>): Partial<DbFrame> => { return { id: frame.id, url: frame.url, - x: frame.position?.x.toString(), - y: frame.position?.y.toString(), - canvasId: frame.canvasId, - width: frame.dimension?.width.toString(), - height: frame.dimension?.height.toString(), + branchId: frame.branchId, + canvasId: frame.canvasId, + x: frame.position?.x?.toString(), + y: frame.position?.y?.toString(), + width: frame.dimension?.width?.toString(), + height: frame.dimension?.height?.toString(), }; };packages/db/src/dto/project/project.ts (1)
79-86: Avoid unintentionally bumping updatedPreviewImgAtSetting
res.updatedPreviewImgAt = previewImg.updatedAt ?? new Date()will stamp “now” even when the preview image hasn’t changed and the client omittedupdatedAt. This can cause noisy writes and cache busting.Use a pass-through instead (no defaulting), and stamp at the service layer when a real change is detected:
- res.updatedPreviewImgAt = previewImg.updatedAt ?? new Date(); + res.updatedPreviewImgAt = previewImg.updatedAt ?? null;If you need to skip updating this column when
updatedAtis absent, consider making this property optional and omitting it from the update payload.apps/web/client/src/components/store/editor/frames/manager.ts (4)
214-222: Persist and store the same rounded frame object to avoid UI/server driftYou round before sending to the API, but keep the unrounded object in memory. Store the rounded one locally for consistency.
Apply:
- async create(frame: Frame) { - const success = await api.frame.create.mutate(fromFrame(roundDimensions(frame))); + async create(frame: Frame) { + const rounded = roundDimensions(frame); + const success = await api.frame.create.mutate(fromFrame(rounded)); if (success) { - this._frameIdToData.set(frame.id, { frame, view: null, selected: false }); + this._frameIdToData.set(frame.id, { frame: rounded, view: null, selected: false }); } else { console.error('Failed to create frame'); } }
224-242: Duplicate/delete should not require a live view; check frame presence instead (semantics + bug)Both duplicate() and delete() abort if view is missing, even though they only need the frame data and id. This blocks valid operations (e.g., when the view is not mounted yet) and is a functional bug.
Apply:
async duplicate(id: string) { const frameData = this.get(id); - if (!frameData?.view) { - console.error('Frame view not found for duplicate', id); + if (!frameData) { + console.error('Frame not found for duplicate', id); return; } - - const frame = frameData.frame + const frame = frameData.frame; const newFrame: Frame = { ...frame, id: uuid(), position: { x: frame.position.x + frame.dimension.width + 100, y: frame.position.y, }, }; await this.create(newFrame); }And for delete():
- const frameData = this.get(id); - if (!frameData?.view) { - console.error('Frame not found for delete', id); + const frameData = this.get(id); + if (!frameData) { + console.error('Frame not found for delete', id); return; } const success = await api.frame.delete.mutate({ - frameId: frameData.frame.id, + frameId: frameData.frame.id, });
158-176: Build the next URL via URL resolution, not string concat; handle null originsConcatenating
${baseUrl}${path}can double-slash or produce "null/path" for about:blank. Resolve robustly.Apply:
- await this.updateAndSaveToStorage(frameId, { url: `${baseUrl}${path}` }); + let nextUrl: string | null = null; + try { + nextUrl = new URL(path, new URL(currentUrl).origin).toString(); + } catch { + console.warn('Failed to resolve next URL from', { currentUrl, path }); + return; + } + await this.updateAndSaveToStorage(frameId, { url: nextUrl });
244-255: Don’t await a lodash.debounce’d function; add flush for lifecycle boundariesAwaiting a debounced function does not await the underlying async work and suggests false ordering. Also, pending updates can be dropped during clear/dispose without an explicit flush.
Apply:
- async updateAndSaveToStorage(frameId: string, frame: Partial<Frame>) { + async updateAndSaveToStorage(frameId: string, frame: Partial<Frame>) { const existingFrame = this.get(frameId); if (existingFrame) { const newFrame = { ...existingFrame.frame, ...frame }; this._frameIdToData.set(frameId, { ...existingFrame, frame: newFrame, selected: existingFrame.selected, }); } - await this.saveToStorage(frameId, frame); + // Fire-and-forget; persistence is debounced. Call this.saveToStorage.flush() on boundaries. + this.saveToStorage(frameId, frame); }And update the debounced declaration and lifecycle (outside this hunk):
- saveToStorage = debounce(this.undebouncedSaveToStorage.bind(this), 1000); + saveToStorage = debounce( + (frameId: string, frame: Partial<Frame>) => { + void this.undebouncedSaveToStorage(frameId, frame); + }, + 1000, + );Flush at boundaries:
clear() { + this.saveToStorage.flush?.(); this.deregisterAll(); this._disposers.forEach((dispose) => dispose()); this._disposers = []; this._navigation.clearAllHistory(); }And when disposing a single frame:
disposeFrame(frameId: string) { + this.saveToStorage.flush?.(); this._frameIdToData.delete(frameId); this.editorEngine?.ast?.mappings?.remove(frameId); this._navigation.removeFrame(frameId); }apps/web/client/src/app/project/[id]/_components/canvas/frame/web-frame.tsx (2)
168-181: promisifyMethod doesn’t catch async rejections (missing await)Returning the promise without awaiting prevents try/catch from handling rejections, so errors won’t trigger reloadIframe or proper logging.
Apply:
const promisifyMethod = <T extends (...args: any[]) => any>( method: T | undefined, ): ((...args: Parameters<T>) => Promise<ReturnType<T>>) => { return async (...args: Parameters<T>) => { try { if (!method) throw new Error('Method not initialized'); - return method(...args); + return await method(...args); } catch (error) { console.error(`${PENPAL_PARENT_CHANNEL} (${frame.id}) - Method failed:`, error); reloadIframe(); throw error; } }; };
107-114: Restrict allowedOrigins for WindowMessenger; '*' is riskyUsing ['*'] allows any origin to handshake if the iframe navigates, expanding the attack surface. Constrain to the origin of frame.url.
Apply:
- const messenger = new WindowMessenger({ - remoteWindow: iframeRef.current.contentWindow, - allowedOrigins: ['*'], - }); + const allowedOrigin = (() => { + try { + return new URL(frame.url).origin; + } catch { + return '*'; + } + })(); + const messenger = new WindowMessenger({ + remoteWindow: iframeRef.current.contentWindow, + allowedOrigins: [allowedOrigin], + });Optionally update when frame.url changes substantially (different origin) by tearing down and reconnecting.
🧹 Nitpick comments (25)
apps/web/client/src/server/api/routers/project/project.ts (1)
146-146: Use a single timestamp to avoid subtle clock skew between fieldsYou’re setting updatedAt twice (once passed into fromPreviewImg and once in the DB update) via separate new Date() calls. Capture a single now to keep these consistent.
Apply this diff within the changed area:
- updatedAt: new Date(), + updatedAt: now,And also update the project update below:
- updatedAt: new Date(), + updatedAt: now,Add this once right before calling fromPreviewImg (or at the start of the mutation block):
const now = new Date();packages/db/src/schema/chat/index.ts (1)
1-2: Barrel file looks good; clean consolidation point for chat schema.Simple, focused re-exports improve ergonomics. Consider documenting intended public surface in a follow-up to prevent accidental exports creeping in as the module grows.
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx (1)
40-45: Hook deps nit: ensureeditorEngineis stable or include it in deps.If
editorEngineis a stable store singleton, keeping deps as-is is fine. Otherwise, add it to the dependency array to satisfyexhaustive-depsand prevent stale closures.apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar.tsx (2)
2-2: Type migration toFramealigns with the new model.Looks consistent with the unified Frame API. To avoid ambiguity with DB-side
Frame, consider a local alias (purely a readability nicety).Example:
-import type { Frame } from '@onlook/models'; +import type { Frame as ModelFrame } from '@onlook/models'; … -({ frame }: { frame: Frame }) => { +({ frame }: { frame: ModelFrame }) => {Also applies to: 13-13
143-147: Avoid unnecessary prefetch for an external, new-tab link.When opening in a new tab, prefetching is wasted work. Disable to save bandwidth/CPU.
- <Link + <Link + prefetch={false} className="absolute right-1 top-1/2 -translate-y-1/2 transition-opacity duration-300" href={frame.url.replace(/\[([^\]]+)\]/g, 'temp-$1')} // Dynamic routes are not supported so we replace them with a temporary value target="_blank"apps/web/client/src/app/project/[id]/_components/canvas/frame/page-selector.tsx (1)
47-51: Guardnew URL(frame.url)to handle relative/invalid URLs gracefully.
new URL()throws on invalid inputs in some edge cases. Using a base or a try/catch avoids runtime errors while pages are scanning or URLs are transient.- const currentPage = useMemo(() => { - const framePathname = new URL(frame.url).pathname; + const currentPage = useMemo(() => { + let framePathname = '/'; + try { + framePathname = new URL(frame.url, window.location.origin).pathname; + } catch { + // fall back to inferred path if url is not absolute/valid yet + framePathname = inferredCurrentPage.path; + } return allPages.find(page => { const pagePath = page.path === '/' ? '' : page.path; return framePathname === pagePath || framePathname === page.path; }); }, [frame.url, allPages]);packages/db/src/dto/index.ts (1)
1-1: Optional: Add temporary compatibility re-exports for DTOsWe’ve reshaped the top-level DTO barrel to only expose
./chat. A quick scan of the repo shows no internal imports of the old DTO barrel, so this change won’t break any in-repo consumers. However, if external consumers still importFrameorCanvasfrom@onlook/db/dto, you may wish to provide a deprecation path:export * from './chat'; export * from './domain'; export * from './project'; export * from './subscription'; export * from './user'; + +// Temporary compatibility re-exports (to be removed in a future major/minor) +// Allow existing imports from '@onlook/db/dto' to continue functioning. +export * from './project/frame'; +export * from './project/canvas';apps/web/client/src/app/projects/_components/select/index.tsx (1)
115-120: Avoid double fetch: choose invalidate or refetch, not bothAfter
removeTag, you bothinvalidate()andrefetch(). In tRPC/react-query,invalidate()is typically sufficient to trigger a fresh fetch. Callingrefetch()immediately afterward often causes two back-to-back network requests.Consider removing
refetch()and relying oninvalidate()(or vice versa), and optionally move the cache invalidation into the mutation’sonSuccessfor better cohesion.apps/web/client/src/components/store/editor/text/index.ts (1)
20-71: start(): make error path restore state to avoid “stuck editing”/open history transactionIf an error is thrown after setting
targetDomEl/shouldNotStartEditingand starting a history transaction, the catch path only logs, leaving flags and possibly a pending transaction. Reset local state and remove any text editor overlay in the catch path.Apply this minimal diff:
async start(el: DomElement, frameView: FrameView): Promise<void> { try { const isEditable = (await frameView.isChildTextEditable(el.oid ?? '')) as | boolean | null; if (isEditable !== true) { throw new Error( isEditable === null ? "Can't determine if text is editable" : "Can't edit text because it's not plain text. Edit in code or use AI.", ); } const res = (await frameView.startEditingText(el.domId)) as EditTextResult | null; if (!res) { throw new Error('Failed to start editing text, no result returned'); } const computedStyles = (await frameView.getComputedStyleByDomId(el.domId)) as Record< string, string > | null; if (!computedStyles) { throw new Error('Failed to get computed styles for text editing'); } const { originalContent } = res; this.targetDomEl = el; this.originalContent = originalContent; this.shouldNotStartEditing = true; this.editorEngine.history.startTransaction(); @@ } catch (error) { console.error('Error starting text edit:', error); + // Best-effort cleanup to avoid leaving the editor in an inconsistent state + try { + this.editorEngine.overlay.state.removeTextEditor(); + } catch {} + this.targetDomEl = null; + this.shouldNotStartEditing = false; } }Note: If the history API exposes an explicit rollback/abort, prefer calling it here.
apps/web/client/src/app/project/[id]/_components/canvas/frame/gesture.tsx (1)
97-109: Mouse move handling condition reflow — LGTMNo functional differences; reads clearer with explicit grouping.
Optional: short-circuit early when
isResizingto avoid unnecessary DOM queries during resize interactions.apps/web/client/src/server/api/routers/project/frame.ts (1)
37-41: Create mutation trusts input shape; consider returning the created id (or row) for better UXThe insert now pipes input straight through (validated by frameInsertSchema). That’s fine. Consider returning the created id (or toFrame(row)) so the client can optimistically navigate/select without a follow-up fetch.
Example:
- await ctx.db.insert(frames).values(input); - return true; + const [row] = await ctx.db.insert(frames).values(input).returning(); + // optionally: return toFrame(row) + return !!row?.id;apps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx (1)
8-8: Naming nit: FrameComponent imported from './web-frame'The file is still named web-frame despite the generic role. Consider renaming to frame-view.tsx (or similar) to prevent confusion during future maintenance.
apps/web/client/src/components/store/editor/insert/index.ts (2)
1-1: Type import source is still “web-frame”Minor naming drift: FrameView now represents the generic frame view. Consider moving it to a neutral filename and updating imports to avoid “web-” terminology in the new model.
80-98: Normalize end() return type to voidend currently returns null in one branch, making its return type Promise<void | null>. If callers don’t use the return value, prefer a consistent void.
Apply:
- async end(e: React.MouseEvent<HTMLDivElement>, frameView: FrameView | null) { + async end(e: React.MouseEvent<HTMLDivElement>, frameView: FrameView | null): Promise<void> { if (!this.isDrawing || !this.drawOrigin) { - return null; + return; }apps/web/client/src/components/store/editor/overlay/utils.ts (2)
1-1: Type import rename looks consistent with the Frame/FrameView migrationImporting
FrameViewfromweb-framealigns with the broader refactor and keeps this util decoupled from the deprecated WebFrame types. Consider extracting theFrameViewtype into a colocatedtypes.tsto avoid importing from a React component file, but not blocking.
43-69: Coordinate transform assumes scale/translate only; consider full matrix application
adaptRectToCanvasderives scale fromcanvasTransform.aand linearly appliescanvasTransform.e/f. This breaks if the canvas ever introduces rotation/skew or non-uniform scaling. If that’s a valid invariant (scale+translate only), add a brief comment. Otherwise, consider using DOMMatrix to transform all four rect corners and rebuild the AABB for correctness.Example refactor sketch (not a drop-in):
- const scale = inverse ? 1 / canvasTransform.a : canvasTransform.a; - const sourceOffset = getRelativeOffset(frameView, canvasContainer); - return { - width: rect.width * scale, - height: rect.height * scale, - top: (rect.top + sourceOffset.top + canvasTransform.f / scale) * scale, - left: (rect.left + sourceOffset.left + canvasTransform.e / scale) * scale, - }; + const sourceOffset = getRelativeOffset(frameView, canvasContainer); + const toCanvas = inverse ? canvasTransform.inverse() : canvasTransform; + const p1 = toCanvas.transformPoint(new DOMPoint(rect.left + sourceOffset.left, rect.top + sourceOffset.top)); + const p2 = toCanvas.transformPoint(new DOMPoint(rect.left + sourceOffset.left + rect.width, rect.top + sourceOffset.top + rect.height)); + return { + top: Math.min(p1.y, p2.y), + left: Math.min(p1.x, p2.x), + width: Math.abs(p2.x - p1.x), + height: Math.abs(p2.y - p1.y), + };packages/db/src/schema/canvas/frame.ts (1)
12-15: Consider indexing by (canvasId, branchId) for common access pathsGiven frames are accessed by canvas and branch, add a composite index to avoid sequential scans as data grows.
Apply this diff:
-export const frames = pgTable("frames", { +export const frames = pgTable("frames", { id: uuid("id").primaryKey().defaultRandom(), canvasId: uuid("canvas_id") .notNull() .references(() => canvases.id, { onDelete: "cascade", onUpdate: "cascade" }), branchId: uuid("branch_id") .notNull() .references(() => branches.id, { onDelete: "cascade", onUpdate: "cascade" }), url: varchar("url").notNull(), // display data x: numeric("x").notNull(), y: numeric("y").notNull(), width: numeric("width").notNull(), height: numeric("height").notNull(), -}).enableRLS(); +}, (t) => ({ + byCanvasBranch: index("frames_canvas_branch_idx").on(t.canvasId, t.branchId), +})).enableRLS();packages/db/src/schema/project/branch.ts (2)
7-12: Unused enum: syncStatus is defined but not used in the tableEither add a
syncStatuscolumn tobranchesor drop the enum for now to avoid dead code and migrations drift.Example if you intend to keep it:
export const branches = pgTable('branches', { id: uuid('id').primaryKey().defaultRandom(), projectId: uuid('project_id').notNull().references(() => projects.id, { onDelete: 'cascade', onUpdate: 'cascade' }), + syncStatus: syncStatus('sync_status').default('synced').notNull(),
14-33: Add practical constraints and indexes (unique name per project, projectId index)To prevent duplicate branch names within a project and improve lookup performance:
Apply this diff:
-export const branches = pgTable('branches', { +export const branches = pgTable('branches', { id: uuid('id').primaryKey().defaultRandom(), projectId: uuid('project_id') .notNull() .references(() => projects.id, { onDelete: 'cascade', onUpdate: 'cascade' }), @@ sandboxId: varchar('sandbox_id').notNull(), -}).enableRLS(); +}, (t) => ({ + byProject: index('branches_project_id_idx').on(t.projectId), + namePerProject: uniqueIndex('branches_project_name_uk').on(t.projectId, t.name), +})).enableRLS();apps/web/client/src/components/store/editor/frames/manager.ts (2)
70-75: Defensive parsing for iframe.src to avoid invalid URL and about:blank pitfallsnew URL(view.src) can throw for invalid/empty values, and about:blank yields origin "null". Default a safe pathname and guard errors.
Apply:
- const framePathname = new URL(view.src).pathname; - this._navigation.registerFrame(frame.id, framePathname); + let framePathname = '/'; + try { + // window.location.href as base supports relative src values + framePathname = new URL(view.src, window.location.href).pathname || '/'; + } catch { + console.warn('Invalid iframe src when registering navigation; defaulting to "/"', view.src); + } + this._navigation.registerFrame(frame.id, framePathname);
259-273: Optional: short-circuit no-op updates to reduce API chatterIf fromPartialFrame(frame) yields an empty object, skip the network call.
Example:
async undebouncedSaveToStorage(frameId: string, frame: Partial<Frame>) { try { const frameToUpdate = fromPartialFrame(frame); + if (!frameToUpdate || Object.keys(frameToUpdate).length === 0) { + return; + } const success = await api.frame.update.mutate({ frameId, frame: frameToUpdate, });apps/web/client/src/app/project/[id]/_components/canvas/frame/web-frame.tsx (4)
232-237: Avoid returning an empty object as FrameView when iframe is missingReturning {} as FrameView is unsound and can crash callers. Prefer a nullable ref or throw early.
Minimal change (keep non-nullable external API):
- if (!iframe) { - console.error(`${PENPAL_PARENT_CHANNEL} (${frame.id}) - Iframe - Not found`); - return {} as FrameView; - } + if (!iframe) { + console.error(`${PENPAL_PARENT_CHANNEL} (${frame.id}) - Iframe - Not found`); + // Delay: let React mount the iframe before exposing the handle + throw new Error('FrameView not ready'); + }Or, better (requires updating ref type):
-export const FrameComponent = observer( - forwardRef<FrameView, FrameViewProps>(({ frame, ...props }, ref) => { +export const FrameComponent = observer( + forwardRef<FrameView | null, FrameViewProps>(({ frame, ...props }, ref) => { ... - useImperativeHandle(ref, (): FrameView => { + useImperativeHandle(ref, (): FrameView | null => { const iframe = iframeRef.current; - if (!iframe) { - console.error(`${PENPAL_PARENT_CHANNEL} (${frame.id}) - Iframe - Not found`); - return {} as FrameView; - } + if (!iframe) return null;
255-261: Move registerView side-effect out of useImperativeHandle and deregister on cleanupImperative handle should avoid side effects to prevent double registration on re-renders. Register once when penpalChild is ready and iframe exists; deregister on unmount.
Apply:
- // Register the iframe with the editor engine - editorEngine.frames.registerView(frame, iframe as FrameView); + // registration moved to effect belowAnd add/update the effect:
- useEffect(() => { - return () => { + useEffect(() => { + if (penpalChild && iframeRef.current) { + editorEngine.frames.registerView(frame, iframeRef.current as FrameView); + } + return () => { if (connectionRef.current) { connectionRef.current.destroy(); connectionRef.current = null; } + // ensure view is deregistered to prevent stale references + editorEngine.frames.deregister(frame); setPenpalChild(null); isConnecting.current = false; }; - }, []); + }, [penpalChild, frame]);
239-249: isLoading check always true for cross-origin iframescontentDocument is null cross-origin, so readyState is inaccessible; your check returns true indefinitely. Consider a heuristic like navigation timing via messages from child, or fall back to the load event and a local flag.
Example minimal tweak:
- isLoading: () => iframe.contentDocument?.readyState !== 'complete', + isLoading: () => { + // Best effort: complete if iframe has loaded at least once. + // For cross-origin, rely on a data- attribute we set on load. + return iframe.dataset.loaded !== '1'; + },And set it:
- onLoad={setupPenpalConnection} + onLoad={(e) => { + (e.currentTarget as HTMLIFrameElement).dataset.loaded = '1'; + setupPenpalConnection(); + }}
284-286: Re-evaluate powerful features in allow attributeallow enables geolocation/microphone/camera for whatever URL users load. If not strictly required, consider removing or gating behind a settings flag.
📜 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 (33)
apps/web/client/src/app/project/[id]/_components/canvas/frame/gesture.tsx(4 hunks)apps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/canvas/frame/page-selector.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/canvas/frame/web-frame.tsx(4 hunks)apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx(1 hunks)apps/web/client/src/app/projects/_components/select/index.tsx(1 hunks)apps/web/client/src/app/projects/_components/settings.tsx(1 hunks)apps/web/client/src/components/store/editor/frames/manager.ts(7 hunks)apps/web/client/src/components/store/editor/insert/index.ts(4 hunks)apps/web/client/src/components/store/editor/overlay/utils.ts(3 hunks)apps/web/client/src/components/store/editor/text/index.ts(4 hunks)apps/web/client/src/server/api/routers/project/frame.ts(2 hunks)apps/web/client/src/server/api/routers/project/project.ts(1 hunks)apps/web/client/src/server/api/routers/publish/helpers/publish.ts(1 hunks)packages/db/src/defaults/frame.ts(1 hunks)packages/db/src/dto/index.ts(1 hunks)packages/db/src/dto/project/canvas.ts(1 hunks)packages/db/src/dto/project/frame.ts(2 hunks)packages/db/src/dto/project/index.ts(1 hunks)packages/db/src/dto/project/project.ts(5 hunks)packages/db/src/schema/canvas/canvas.ts(1 hunks)packages/db/src/schema/canvas/frame.ts(2 hunks)packages/db/src/schema/canvas/index.ts(0 hunks)packages/db/src/schema/chat/index.ts(1 hunks)packages/db/src/schema/domain/deployment.ts(1 hunks)packages/db/src/schema/domain/index.ts(1 hunks)packages/db/src/schema/index.ts(1 hunks)packages/db/src/schema/project/branch.ts(1 hunks)packages/db/src/schema/project/index.ts(1 hunks)packages/db/src/schema/project/project.ts(1 hunks)packages/models/src/project/frame.ts(1 hunks)packages/models/src/project/project.ts(2 hunks)
💤 Files with no reviewable changes (1)
- packages/db/src/schema/canvas/index.ts
🧰 Additional context used
🧬 Code graph analysis (14)
apps/web/client/src/app/project/[id]/_components/canvas/frame/page-selector.tsx (2)
packages/db/src/schema/canvas/frame.ts (1)
Frame(27-27)packages/models/src/project/frame.ts (1)
Frame(4-16)
apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar.tsx (2)
packages/db/src/schema/canvas/frame.ts (1)
Frame(27-27)packages/models/src/project/frame.ts (1)
Frame(4-16)
apps/web/client/src/app/projects/_components/select/index.tsx (1)
packages/db/src/schema/project/project.ts (1)
projects(11-26)
apps/web/client/src/components/store/editor/text/index.ts (3)
packages/models/src/element/element.ts (2)
DomElement(11-15)ElementPosition(22-25)apps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx (1)
FrameView(10-28)apps/web/client/src/app/project/[id]/_components/canvas/frame/web-frame.tsx (1)
FrameView(25-30)
apps/web/client/src/components/store/editor/frames/manager.ts (2)
apps/web/client/src/app/project/[id]/_components/canvas/frame/web-frame.tsx (1)
FrameView(25-30)packages/models/src/project/frame.ts (1)
Frame(4-16)
packages/db/src/schema/canvas/frame.ts (2)
packages/db/src/schema/canvas/canvas.ts (1)
canvases(8-13)packages/db/src/schema/project/branch.ts (1)
branches(14-33)
apps/web/client/src/components/store/editor/overlay/utils.ts (2)
apps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx (1)
FrameView(10-28)apps/web/client/src/app/project/[id]/_components/canvas/frame/web-frame.tsx (1)
FrameView(25-30)
apps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx (3)
apps/web/client/src/app/project/[id]/_components/canvas/frame/top-bar.tsx (1)
TopBar(12-161)apps/web/client/src/app/project/[id]/_components/canvas/frame/web-frame.tsx (1)
FrameComponent(36-292)apps/web/client/src/app/project/[id]/_components/canvas/frame/gesture.tsx (1)
GestureScreen(13-233)
apps/web/client/src/app/project/[id]/_components/canvas/frame/gesture.tsx (2)
packages/db/src/schema/canvas/frame.ts (1)
Frame(27-27)packages/models/src/project/frame.ts (1)
Frame(4-16)
apps/web/client/src/server/api/routers/project/frame.ts (1)
packages/db/src/schema/canvas/frame.ts (1)
frames(7-22)
apps/web/client/src/components/store/editor/insert/index.ts (2)
apps/web/client/src/app/project/[id]/_components/canvas/frame/web-frame.tsx (1)
FrameView(25-30)packages/models/src/element/element.ts (2)
RectDimensions(33-38)ElementPosition(22-25)
packages/db/src/schema/project/branch.ts (2)
packages/db/src/schema/project/project.ts (1)
projects(11-26)packages/db/src/schema/canvas/frame.ts (1)
frames(7-22)
packages/db/src/dto/project/frame.ts (2)
packages/db/src/schema/canvas/frame.ts (1)
Frame(27-27)packages/models/src/project/frame.ts (1)
Frame(4-16)
apps/web/client/src/app/project/[id]/_components/canvas/frame/web-frame.tsx (3)
apps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx (1)
FrameView(10-28)packages/db/src/schema/canvas/frame.ts (1)
Frame(27-27)packages/models/src/project/frame.ts (1)
Frame(4-16)
🔇 Additional comments (32)
packages/models/src/project/frame.ts (1)
14-16: FrameType and frame.type fully removedNo residual references to the old FrameType enum or any frame.type property were found anywhere in the codebase. Great cleanup!
• packages/models/src/project/frame.ts now only defines
url: string;with no stale type checks or enums.
• A full ripgrep sweep forFrameTypeandframe.typereturned zero matches.If you anticipate supporting frame sources beyond URLs in the future, you might later rename
urltosrcand introduce asourceTypeunion for extensibility—no immediate action required.packages/db/src/schema/domain/deployment.ts (1)
5-5: Import path verified and safe
I’ve confirmed thatpackages/db/src/schema/project/index.tsre-exportsprojectsfromproject.ts, soimport { projects } from '../project'resolves correctly.
LGTM.packages/db/src/dto/project/canvas.ts (1)
2-2: LGTM: Canvas import path correctedI ran the provided ripgrep check across packages/db/src/dto and confirmed that all
schemaimports align with each file’s directory depth. Files in nested folders correctly use../../schema, while those directly underdtouse../schema. No further updates are needed.packages/db/src/schema/domain/index.ts (1)
2-2: No conflicting re-exports found—re-export is safe.The grep search for any duplicate or conflicting
export * from '../domain/(deployment|custom|preview)'inpackages/db/src/schemareturned no results, confirming thatexport * from './deployment'indomain/index.tsdoes not collide with any other barrel exports.apps/web/client/src/server/api/routers/publish/helpers/publish.ts (1)
2-2: Barrel import switch for Deployment looks goodUsing a type-only import from '@onlook/db' removes deep-path coupling and carries no runtime cost. No issues spotted with the change.
packages/db/src/dto/project/index.ts (1)
1-2: No circular dependencies or duplicate exports detectedI ran the provided checks against
packages/db/src/dto/project/index.ts:
- No imports of
./indexanywhere underpackages/db/src/dto, indicating no obvious cycles.- All exported symbols (
Canvas,fromCanvas,toCanvas,Frame,fromFrame,toFrame, etc.) are unique across the DTO project surface.Everything looks clean—no further action needed here.
packages/db/src/schema/canvas/canvas.ts (1)
3-6: Import path correction and minor import reorder LGTMSwitching userCanvases to '../user' matches the file’s location under schema/canvas and removes the fragile '../../user' hop. No functional concerns with the drizzle-zod import reorder.
packages/db/src/schema/index.ts (1)
1-2: No duplicate exports detected; safe to re-exportI ran a focused duplicate‐symbol check across the
canvasandchatschema modules and found no overlapping export names, so the top-levelexport *from each directory introduces no collisions.– Verified that
packages/db/src/schema/canvasexports (e.g.Canvas,Frame, etc.) do not collide with those inpackages/db/src/schema/chat(e.g.Conversation,Message, etc.).
– No duplicate symbols were reported when sorting and diff-checking the two sets of exports.As such, these re-exports can be safely approved.
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx (1)
40-45: No manual date normalization needed; tRPC is already using SuperJSONI’ve confirmed that both server and client tRPC configurations include
transformer: superjson(inapps/web/server/src/router/trpc.tsandapps/web/client/src/server/api/trpc.ts), which ensures allDatefields (includingpreviewImg.updatedAt) are deserialized asDateinstances on the client. Therefore, the suggested manual normalization isn’t necessary.Likely an incorrect or invalid review comment.
apps/web/client/src/app/project/[id]/_components/canvas/frame/page-selector.tsx (1)
2-2: Prop/type switch toFrameis consistent with the refactor.No behavioral changes; import and prop typing are aligned with the new model.
Also applies to: 19-19
packages/db/src/schema/project/index.ts (1)
1-1: Verification complete: new branch export and downstream imports are correctAll requested checks have passed:
- No remaining imports of
schema/project/canvas,schema/project/chat, orschema/project/deploymentwere found (rgsearch returned no matches).- The file
packages/db/src/schema/project/branch.tsexists.- The barrel file
packages/db/src/schema/project/index.tscorrectly containsexport * from './branch';.No further changes needed—this addition is safe to merge.
apps/web/client/src/components/store/editor/text/index.ts (3)
135-151: handleEditedText(): type alignment with FrameView — LGTMParameter type updated to
FrameViewand usage offrameView.id(from HTMLIFrameElement) remains valid. No functional concerns.
197-209: editElementAtLoc signature migrated to FrameView — LGTMType-only change; control flow is unchanged and consistent with the rest of the module.
1-1: Type migration complete — no remainingWebFrameViewreferencesAll occurrences of the old
WebFrameViewidentifier have been removed or replaced withFrameView, and a project-wide search confirms no stale references remain.apps/web/client/src/app/project/[id]/_components/canvas/frame/gesture.tsx (3)
4-4: Import types migration (Frame) — LGTMShifts the component to the new
Framemodel without behavior changes.
13-13: Prop typing updated to Frame — LGTMThe
GestureScreenprop now usesFrame, aligning with the wider refactor.
151-156: Drag end flow — LGTM
cancelDragPreparation()beforeend(e)is sensible; keeps the state machine consistent.packages/db/src/defaults/frame.ts (1)
11-14: DbFrame numeric columns map to strings by default
Drizzle’snumericcolumns infer astringtype for both selects and inserts (the Node/Postgres driver casts numerics to strings for precision), so your.toString()calls forx,y,width, andheightalign perfectly with the inferredNewFrametype. No changes are required. (orm.drizzle.team, github.com)apps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx (2)
1-9: Unified Frame view — simplification looks goodDropping WebFrame/WebFrameView conditionals and using a single FrameComponent streamlines the render path and state.
18-20: Right-click menu scope: verify you still get context menu on the iframe/content areaRightClickMenu now wraps only TopBar. If the intention was a canvas-wide menu, this may reduce the activation area.
Try right-clicking on the frame content (not the top bar). If it no longer shows, consider wrapping the entire “relative” div or the outer container instead.
apps/web/client/src/components/store/editor/insert/index.ts (2)
148-155: LGTM: insertElement signature tightened to FrameViewNarrowing to FrameView clarifies contract and aids tooling. The internal flow (createInsertAction -> action.run) remains unchanged.
157-161: LGTM: createInsertAction now requires FrameViewA search for all
createInsertAction(call sites shows it’s only invoked in insertElement within apps/web/client/src/components/store/editor/insert/index.ts; no external references exist. Theend(e, frameView)usage in gesture.tsx also matches the updated signature. No further updates are needed—approving these changes.packages/db/src/dto/project/frame.ts (2)
4-19: Mapping to model looks correct and includes branchId/url
toFramenow includesbranchIdand keepsurl, with numeric-to-number conversion for position/dimension. This aligns with the schema.
21-32: Outbound mapping is consistent with schema numeric typesStringifying numeric fields for Drizzle
numericcolumns is correct. No issues found here.packages/db/src/schema/canvas/frame.ts (1)
35-38: Cyclic import betweencanvas/frame.tsandproject/branch.ts– please verify your build/tooling tolerates itThere’s a direct module cycle:
packages/db/src/schema/canvas/frame.tsimportsbranchesfrom../project/branchpackages/db/src/schema/project/branch.tsimportsframesfrom../canvas/frameDrizzle-ORM’s relation helpers use functions to defer resolution, so at runtime this usually works fine. However, some bundlers (e.g. Webpack, Rollup) can choke on circular ESM imports. Please ensure in your build pipeline that:
- The code compiles without errors or warnings related to these files
- Any schema generation or migration scripts still run correctly
- Your frontend or Lambda bundles don’t break when importing these modules
If you do encounter bundler issues, consider one of the following mitigations:
- Move your relation definitions into a third shared file (e.g.
schema/relations.ts) to break the cycle- Use dynamic imports or lazy references (
async () => import(...)) for one side of the relation- Switch to require-style imports in one file as a last resort to sidestep ESM cycle checks
[tag: verify_review_comment]
packages/db/src/dto/project/project.ts (2)
15-16: Tags moved under metadata: migration looks goodReading/writing
tagsviaproject.metadata.tagsmatches the schema’sprojects.tagscolumn.If there’s existing data, ensure a migration moved top-level tags into
projects.tagsto avoid losing them.
42-43: Confirm PreviewImg.updatedAt type consistency across layers
toPreviewImgassignsdbProject.updatedPreviewImgAt(a Date | null) directly topreviewImg.updatedAt. Elsewhere,createdAt/updatedAtin metadata are serialized to ISO strings. Ensure thePreviewImg.updatedAttype in@onlook/modelsmatches this (Date vs string). If the frontend expects strings, convert with.toISOString().Apply this minimal change if strings are required:
- updatedAt: dbProject.updatedPreviewImgAt, + updatedAt: dbProject.updatedPreviewImgAt ?? null,or
- updatedAt: dbProject.updatedPreviewImgAt, + updatedAt: dbProject.updatedPreviewImgAt ? dbProject.updatedPreviewImgAt.toISOString() : null,Also applies to: 51-52
packages/db/src/schema/project/branch.ts (1)
38-44: Module dependency cycle detectedThe
branchandframeschema modules import each other, creating a circular dependency that may lead to uninitialized exports at import time:
- packages/db/src/schema/project/branch.ts imports
framesfrom../canvas/frameand definesbranchRelationsreferencingframes- packages/db/src/schema/canvas/frame.ts imports
branchesfrom../project/branchand definesframeRelationsreferencingbranchesPlease verify that your bundler/runtime (ESM or CJS) correctly handles this cycle without producing
undefinedexports during module initialization. If you encounter any errors or broken relations, consider consolidating your relation definitions into a single module (or a third “relations” module) to break the cycle.apps/web/client/src/components/store/editor/frames/manager.ts (2)
11-15: LGTM: FrameData now tracks FrameView | nullThe type aligns with the unified Frame/FrameView model. No concerns.
17-29: LGTM: roundDimensions(Frame) returns an immutable, rounded copyGood defensive copy and rounding semantics.
apps/web/client/src/app/project/[id]/_components/canvas/frame/web-frame.tsx (2)
4-4: LGTM: unified Frame importMatches the project-wide migration. No issues.
25-31: LGTM: FrameView type matches expected imperative APIThe surface looks consistent with usages in the store and gestures.
| import { v4 as uuidv4 } from 'uuid'; | ||
|
|
||
| export const createDefaultFrame = (canvasId: string, url: string, overrides?: Partial<DbFrame>): DbFrame => { | ||
| export const createDefaultFrame = (canvasId: string, branchId: string, url: string, overrides?: Partial<DbFrame>): DbFrame => { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Branch association made explicit — good call
Making branchId a required parameter aligns the defaults factory with the new branching model and prevents silent creation on the wrong branch.
If helpful, run a quick sweep to ensure all call sites were updated to pass branchId:
🏁 Script executed:
#!/bin/bash
# Verify all createDefaultFrame usages specify branchId (3 params before overrides)
rg -nP "createDefaultFrame\s*\(" -C2Length of output: 3950
Inconsistent createDefaultFrame Invocations — Critical Fixes Required
Several call sites still use the old two- or three-parameter form, omitting the newly required branchId argument. These must be updated to pass (canvasId, branchId, url[, overrides]) or the code will not compile.
Please update the following locations:
-
packages/db/src/seed/db.ts
• Line 84:
const frame0 = createDefaultFrame(canvas0.id, project0.sandboxUrl);
→ should be
const frame0 = createDefaultFrame(canvas0.id, project0.mainBranchId, project0.sandboxUrl);• Line 88:
const frame1 = createDefaultFrame(canvas1.id, project1.sandboxUrl);
→ should be
const frame1 = createDefaultFrame(canvas1.id, project1.mainBranchId, project1.sandboxUrl); -
apps/web/client/src/server/api/routers/project/template.ts
• Lines 127 & 134:const desktopFrame = createDefaultFrame(newCanvas.id, newSandboxUrl, { x: '5', y: '0' }); const mobileFrame = createDefaultFrame(newCanvas.id, newSandboxUrl, { x: '1600', y: '0' });
→ should be
const desktopFrame = createDefaultFrame(newCanvas.id, project.branchId, newSandboxUrl, { x: '5', y: '0' }); const mobileFrame = createDefaultFrame(newCanvas.id, project.branchId, newSandboxUrl, { x: '1600', y: '0' });
-
apps/web/client/src/server/api/routers/project/project.ts
• Lines 261 & 268:const desktopFrame = createDefaultFrame(newCanvas.id, input.project.sandboxUrl, { x: '5', y: '0' }); const mobileFrame = createDefaultFrame(newCanvas.id, input.project.sandboxUrl, { x: '1600', y: '0' });
→ should be
const desktopFrame = createDefaultFrame(newCanvas.id, input.project.branchId, input.project.sandboxUrl, { x: '5', y: '0' }); const mobileFrame = createDefaultFrame(newCanvas.id, input.project.branchId, input.project.sandboxUrl, { x: '1600', y: '0' });
After these updates, run a sweep to confirm all invocations match the new signature:
rg -nP "createDefaultFrame\s*\(" -C2🤖 Prompt for AI Agents
In packages/db/src/defaults/frame.ts around line 5 and the call sites listed,
the createDefaultFrame signature now requires (canvasId, branchId, url[,
overrides]) but several call sites still pass only two or three args; update
each call to pass the branchId as the second argument: in
packages/db/src/seed/db.ts lines ~84 and ~88 change to
createDefaultFrame(canvasId, project.mainBranchId, project.sandboxUrl), in
apps/web/client/src/server/api/routers/project/template.ts lines ~127 and ~134
change to createDefaultFrame(newCanvas.id, project.branchId, newSandboxUrl, {
... }), and in apps/web/client/src/server/api/routers/project/project.ts lines
~261 and ~268 change to createDefaultFrame(newCanvas.id, input.project.branchId,
input.project.sandboxUrl, { ... }); after applying these fixes run rg -nP
"createDefaultFrame\s*\(" -C2 to verify all invocations match the new signature.
| import { canvases } from '../canvas'; | ||
| import { conversations, PROJECT_CONVERSATION_RELATION_NAME } from '../chat'; | ||
| import { PREVIEW_DOMAIN_PROJECT_RELATION_NAME, previewDomains, PROJECT_CUSTOM_DOMAIN_PROJECT_RELATION_NAME, projectCustomDomains } from '../domain'; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Import reorg matches the new schema layout — please confirm migrations and dead code cleanup for sandbox removal
Since sandboxId/sandboxUrl were removed from projects, ensure a corresponding migration drops these columns and that RLS/policies, DTOs, and any code paths referencing them are cleaned up.
Run:
If you’d like, I can help draft the migration and a small codemod to remove stale references.
🏁 Script executed:
#!/bin/bash
# Find any lingering references
rg -n "sandbox(Id|URL|Url)|sandbox_id|sandbox_url" -S
# Sanity-check there is a migration removing sandbox fields
fd -t f -e sql -e ts --search-path packages/db "migration|migrate|sandbox|project" | sed -n '1,120p'Length of output: 17701
Missing Migration and Dead Code Cleanup for sandbox fields
It looks like the schema reorg removed sandboxId/sandboxUrl from projects, but there’s currently no migration dropping those columns and a large number of code paths and seeds still reference them. Please address the following:
-
Create a SQL migration to drop
sandbox_idandsandbox_urlfrom the projects table.
• No*.sqlmigration was found under packages/db or apps/backend/supabase/migrations that removes these columns. -
Update DB seeds in packages/db/src/seed/db.ts (around lines 56–57, 71–72, and frame creation at 84–88), removing or replacing references to sandboxId/sandboxUrl.
-
Prune schema definitions:
• RemovesandboxIdfrom packages/db/src/schema/project/branch.ts (line 32).
• RemovesandboxIdfrom packages/db/src/schema/domain/deployment.ts (line 15). -
Clean up DTOs and defaults:
• Remove sandbox fields from packages/db/src/dto/project/project.ts
• Update packages/db/src/defaults/project-settings.ts -
Remove or refactor code references throughout the repo, including but not limited to:
• packages/constants/src/csb.ts
• packages/code-provider/src/providers/codesandbox/*
• apps/web/client components (template-modal.tsx, import contexts, hero/start-blank.tsx, etc.)
• apps/web/server API routers (publish, project, sandbox)
• client store managers and sessions (create/manager.ts, editor/sandbox/session.ts)
Let me know if you’d like help drafting the migration or a codemod to clean up stale references.
| // IDs | ||
| id: string; | ||
| branchId: string; | ||
| canvasId: string; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
branchId and canvasId addition: confirm DB/DTO parity and backfill strategy
New required IDs look good, but ensure DB schema and DTOs already carry branchId and canvasId and that existing frames are backfilled to a valid branch. Otherwise, runtime nulls will creep in.
Run:
🏁 Script executed:
#!/bin/bash
# DTO/schema presence
rg -nP -C3 'branchId|canvasId' packages/db/src
# Find any lingering optional assumptions
rg -nP -C2 --type ts '\bframe\.(branchId|canvasId)\b(?!\s*\??\.)'Length of output: 8484
🏁 Script executed:
#!/bin/bash
# Locate any migrations directories in the repo
find . -type d | rg -i migrations
# List all migration files
find . -type f | rg -i migrationsLength of output: 2302
🏁 Script executed:
#!/bin/bash
# Search for initial creation of frames table
rg -n -C5 'CREATE TABLE.*frames' apps/backend/supabase/migrations
# Search for any ALTER operations on frames table
rg -n -C5 'ALTER TABLE.*frames' apps/backend/supabase/migrations
# Display context for branch_id additions
rg -n -C5 'branch_id' apps/backend/supabase/migrations
# Display context for canvas_id additions
rg -n -C5 'canvas_id' apps/backend/supabase/migrationsLength of output: 107483
branchId requires a new Supabase migration and backfill
The Drizzle schema in packages/db/src/schema/canvas/frame.ts and the DTO mappings already include branchId and enforce it as NOT NULL, but I don’t see any Supabase migration SQL that adds branch_id to the frames table—or that backfills it for existing rows. Without that:
- The production database won’t have a branch_id column on frames, breaking queries.
- Even if you add the column, it must be populated for legacy frames (e.g. via a default or a batch UPDATE) to avoid runtime null errors.
Please add a migration under apps/backend/supabase/migrations/ that:
- Alters frames to add a non-nullable branch_id UUID column with a FK to branches(id).
- Backfills existing records (e.g. join through canvases → default branch per canvas).
- Updates the FK constraint and RLS policies if they reference branch_id.
Once that migration (and any necessary branch-table migrations) is in place, the DB/DTO parity will be complete.
| sandbox: { | ||
| id: string; | ||
| url: string; | ||
| tags: string[]; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Tags relocation LGTM; verify consumers moved off project.tags
Moving tags under metadata improves cohesion. Please double-check all usages of project.tags are updated to project.metadata.tags.
Run to find lingering references:
🏁 Script executed:
#!/bin/bash
rg -nP -C2 --glob '!**/node_modules/**' '\bproject\.tags\b|\bProject\.tags\b'Length of output: 973
Update lingering project.tags references to project.metadata.tags
The relocation of tags to metadata.tags is on the right track, but there are still direct project.tags usages in the API router that need updating. Please refactor these to reference project.metadata.tags (and preserve any existing null-coalescing or filtering logic).
• apps/web/client/src/server/api/routers/project/project.ts
– Line 373: const currentTags = project.tags ?? [];
– Line 397: const currentTags = project.tags ?? [];
After updating, rerun the ripgrep search to ensure no remaining project.tags references.
🤖 Prompt for AI Agents
In apps/web/client/src/server/api/routers/project/project.ts around lines 373
and 397, update the direct accesses of project.tags to project.metadata.tags
while preserving the existing null-coalescing and filtering logic (e.g., change
const currentTags = project.tags ?? []; to const currentTags =
project.metadata.tags ?? [];), then run ripgrep across the repo to verify no
remaining project.tags references; also remove or update the tags: string[]
declaration in packages/models/src/project/project.ts (line 9) if it is now
duplicated by metadata.tags.
| @@ -22,4 +17,5 @@ export interface PreviewImg { | |||
| path: string; | |||
| }; | |||
| url?: string; | |||
| updatedAt: Date | null; | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unify time types: prefer string (ISO) over Date for PreviewImg.updatedAt
createdAt/updatedAt in metadata are strings, but previewImg.updatedAt is Date | null. Mixing Date and string time types at the model boundary is error-prone for serialization and API contracts. Recommend making updatedAt a string | null (ISO 8601) to align with the rest of Project.
Apply:
export interface PreviewImg {
type: 'storage' | 'url';
storagePath?: {
bucket: string;
path: string;
};
url?: string;
- updatedAt: Date | null;
+ updatedAt: string | null;
}Follow-up sanity check:
#!/bin/bash
# Locate usage sites to confirm types and parsing remain correct
rg -nP -C2 --type ts 'previewImg\.updatedAt'🤖 Prompt for AI Agents
In packages/models/src/project/project.ts around line 20, the
PreviewImg.updatedAt type is Date | null but Project metadata uses string
timestamps; change the type to string | null (ISO 8601) to unify time types.
Update any factory/parser/serialization code in this file to accept and emit ISO
strings (use toISOString where converting Date -> string) and ensure callers
that previously passed/expected Date are updated to pass/parse ISO strings; then
run a repository search for "previewImg.updatedAt" and update usages to parse
ISO strings to Date only where runtime Date operations are required.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (13)
packages/db/src/mappers/chat/conversation.ts (2)
4-12: Mapper leaks DB-only fields and omits title mapping
- Object spread
...conversationbrings DB-only fields (projectId,suggestions, possiblydisplayName) into the returned ChatConversation.titleis not set fromdisplayName, breaking round-trip symmetry.Prefer stripping DB-only fields and explicitly mapping the divergent names.
Apply:
-export const fromDbConversation = (conversation: DbConversation): ChatConversation => { - return { - ...conversation, - resourceId: conversation.projectId, - metadata: { - suggestions: conversation.suggestions || [], - } - } -} +export const fromDbConversation = (conversation: DbConversation): ChatConversation => { + const { projectId, suggestions, displayName, ...rest } = conversation as any; + return { + // retain common thread fields without DB-only keys + ...rest, + title: displayName ?? null, + resourceId: projectId, + metadata: { suggestions: suggestions ?? [] }, + }; +}Optional follow-up: add a unit test asserting a fromDb→toDb→fromDb round-trip preserves id, title/displayName, suggestions length, and resourceId/projectId consistency.
14-21: toDbConversation must exclude Chat-only fields
The current implementation spreads the entireChatConversationinto theDbConversation, unintentionally leakingresourceIdandmetadata—neither of which belong on the DB entity. Additionally, usingtitle || nullwill convert an empty string tonull; if you only want to normalizeundefinedornull, use the??operator instead.Please update
packages/db/src/mappers/chat/conversation.ts(around lines 14–21) as follows:-export const toDbConversation = (conversation: ChatConversation): DbConversation => { - return { - ...conversation, - projectId: conversation.resourceId, - displayName: conversation.title || null, - suggestions: conversation.metadata?.suggestions || [], - } -} +export const toDbConversation = (conversation: ChatConversation): DbConversation => { + // Remove Chat-only keys before constructing the Db entity + const { resourceId, metadata, title, ...commonFields } = conversation as any; + + return { + ...commonFields, // e.g. id, createdAt, updatedAt + projectId: resourceId, + displayName: title ?? null, // preserves '' if desired; null only on undefined + suggestions: metadata?.suggestions ?? [], // empty array if no metadata + }; +}
- If you do intend to treat an empty
titlestring asnull, reverttitle ?? nullback totitle || nulland add a comment explaining that normalization.- Confirm that
commonFieldscovers exactly the properties required byDbConversation(id, timestamps, etc.) and no others.apps/web/client/src/server/api/routers/chat/message.ts (3)
44-49: Upsert may overwrite createdAt and misses updatedAt; exclude createdAt from SET and stamp updatedAtCurrently the conflict SET spreads
normalizedMessage, which can inadvertently overwritecreatedAt. Also,updatedAtis not set. Prefer omittingcreatedAtfrom updates and stampingupdatedAt.Apply:
- .onConflictDoUpdate({ - target: [messages.id], - set: { - ...normalizedMessage, - }, - }); + .onConflictDoUpdate({ + target: [messages.id], + set: { + // only update mutable fields + updatedAt + ...updateSet, + updatedAt: new Date(), + }, + });And just above the
insert(...).values(normalizedMessage)line, computeupdateSetby excludingcreatedAt:// place before the db call inside the same scope const { createdAt: _createdAt, ...updateSet } = normalizedMessage;
60-70: Update mutation should stamp updatedAtUpdating a message should also refresh
updatedAtfor accurate ordering and cache invalidation.Apply:
- await ctx.db.update(messages).set({ - ...input.message, - role: input.message.role as ChatMessageRole, - parts: input.message.parts as Message['parts'], - }).where(eq(messages.id, input.messageId)); + await ctx.db.update(messages).set({ + ...input.message, + role: input.message.role as ChatMessageRole, + parts: input.message.parts as Message['parts'], + updatedAt: new Date(), + }).where(eq(messages.id, input.messageId));
72-79: Accept ISO strings for checkpoint.createdAt to avoid zod Date parsing issues over JSONJSON transports dates as strings.
z.date()requires Date instances and will fail without a transformer. Usez.coerce.date()to accept strings and coerce to Date.Apply:
- checkpoints: z.array(z.object({ - type: z.nativeEnum(MessageCheckpointType), - oid: z.string(), - createdAt: z.date(), - })), + checkpoints: z.array(z.object({ + type: z.nativeEnum(MessageCheckpointType), + oid: z.string(), + createdAt: z.coerce.date(), + })),apps/web/client/src/server/api/routers/chat/conversation.ts (1)
51-55: Bug: misuse of drizzle update(table) API; you’re passing an object instead of the table and not setting updatedAt
update({ ...conversations, updatedAt: new Date() })mutates the table argument and won’t setupdatedAt. SetupdatedAtinside.set(...)and pass the table directly.Apply:
- const [conversation] = await ctx.db.update({ - ...conversations, - updatedAt: new Date(), - }).set(input.conversation) + const [conversation] = await ctx.db + .update(conversations) + .set({ ...input.conversation, updatedAt: new Date() }) .where(eq(conversations.id, input.conversationId)).returning();apps/web/client/src/server/api/routers/user/user.ts (1)
95-96: Return domain-mapped user from upsert for consistency
upsertcurrently returns the raw DB row, whilegetreturns a mapped domain user. Map here too to keep the API surface uniform.Apply:
- return user ?? null; + return user ? fromDbUser(user) : null;apps/web/client/src/server/api/routers/project/settings.ts (1)
35-41: Critical Fix Required: Correct.values()to avoid inserting a non-existent “settings” columnThe current call
.values(input)will attempt to insert two keys—
projectIdandsettings—into theprojectSettingstable. Since there is nosettingscolumn, this will fail at runtime. You must explicitly pass each column instead of the nestedsettingsobject.Please update as follows in
• apps/web/client/src/server/api/routers/project/settings.ts (upsert mutation)- const [updatedSettings] = await ctx.db - .insert(projectSettings) - .values(input) + const [updatedSettings] = await ctx.db + .insert(projectSettings) + .values({ + projectId: input.projectId, + ...input.settings, + }) .onConflictDoUpdate({ target: [projectSettings.projectId], - set: input.settings, + set: { ...input.settings }, }) .returning();Follow-up: verify that
projectSettingsInsertSchemaexports exactly the column names inprojectSettings(e.g.buildCommand,runCommand,installCommand, etc.) so the spread will match 1:1.apps/web/client/src/server/api/routers/project/member.ts (2)
13-21: Authorization gap: any authenticated user can list members of arbitrary projectsThe route doesn’t verify that
ctx.user.idbelongs to the targetprojectIdbefore returning the member list. This leaks team composition and user emails across projects.Apply a pre-check and fail fast if the caller isn’t a member:
.query(async ({ ctx, input }) => { - const members = await ctx.db.query.userProjects.findMany({ + // Ensure caller has access to this project + const hasAccess = await ctx.db.query.userProjects.findFirst({ + where: and( + eq(userProjects.projectId, input.projectId), + eq(userProjects.userId, ctx.user.id), + ), + }); + if (!hasAccess) { + throw new TRPCError({ code: 'FORBIDDEN', message: 'Not a member of this project' }); + } + + const members = await ctx.db.query.userProjects.findMany({ where: eq(userProjects.projectId, input.projectId), with: { user: true, }, });Add the missing import at the top of the file:
-import { fromDbUser, userProjects } from '@onlook/db'; +import { fromDbUser, userProjects } from '@onlook/db'; +import { TRPCError } from '@trpc/server';
21-40: Stop fabricating DbUser; remove ts-expect-error and avoid leaking billing identifiers
- You’re constructing a pseudo-
DbUserwithnew Date()forcreatedAt/updatedAtand sprinkling@ts-expect-errorto bypass type checks. This corrupts metadata and masks typing issues.stripeCustomerIdis being returned to clients; that’s unnecessary for a “member list” surface and increases exposure of billing identifiers.Use the actual joined
member.userand strip sensitive fields:- return members.map((member) => ({ - role: member.role, - user: fromDbUser({ - id: member.user.id, - email: member.user.email, - createdAt: new Date(), - updatedAt: new Date(), - - // @ts-expect-error - TODO: Fix this later - firstName: member.user.firstName ?? '', - // @ts-expect-error - TODO: Fix this later - lastName: member.user.lastName ?? '', - // @ts-expect-error - TODO: Fix this later - displayName: member.user.displayName ?? '', - // @ts-expect-error - TODO: Fix this later - avatarUrl: member.user.avatarUrl ?? '', - // @ts-expect-error - TODO: Fix this later - stripeCustomerId: member.user.stripeCustomerId ?? null, - }), - })); + return members.map(({ role, user }) => { + const { stripeCustomerId: _omit, ...publicUser } = fromDbUser(user); + return { role, user: publicUser }; + });If
fromDbUserisn’t strictly typed against the drizzle shape here, prefer adding/aligning types instead of@ts-expect-error.apps/web/client/src/server/api/routers/project/project.ts (2)
341-351: getPreviewProjects leaks other users’ projects; constrain to the authenticated userThe endpoint takes
userIdas input and uses it verbatim in the query. Any logged-in user can enumerate another user’s projects.Minimal fix: ignore the input and use
ctx.user.idin the query (keep the input for backward compatibility), or reject mismatches.- getPreviewProjects: protectedProcedure - .input(z.object({ userId: z.string() })) + getPreviewProjects: protectedProcedure + .input(z.object({ userId: z.string() })) .query(async ({ ctx, input }) => { - const projects = await ctx.db.query.userProjects.findMany({ - where: eq(userProjects.userId, input.userId), + // Do not allow enumerating other users' projects + if (input.userId && input.userId !== ctx.user.id) { + throw new TRPCError({ code: 'FORBIDDEN', message: 'Cannot access other users’ projects' }); + } + const projects = await ctx.db.query.userProjects.findMany({ + where: eq(userProjects.userId, ctx.user.id), with: { project: true, }, }); return projects.map((project) => fromDbProject(project.project)); }),Add the import once at the top of this file:
+import { TRPCError } from '@trpc/server';
260-275: Fix createDefaultFrame calls to include the required branchIdThe signature of
createDefaultFrameis now(canvasId: string, branchId: string, url: string, overrides?: Partial<DbFrame>) => DbFramebut all existing call sites are still passing only
(canvasId, url, overrides)and will fail to compile.Please update each of these to supply the proper
branchId(for example the newly created branch’s ID orinput.project.defaultBranchId), or adjustcreateDefaultFrameto resolve a default branch if none is provided:• apps/web/client/src/server/api/routers/project/project.ts
– lines 261, 268• apps/web/client/src/server/api/routers/project/template.ts
– lines 127–136, 154–163• packages/db/src/seed/db.ts
– lines 84, 88Either pass the branch ID explicitly or extend
createDefaultFrameto look up the default branch internally.packages/db/src/mappers/project/frame.ts (1)
34-43: toDbPartialFrame can throw when fields are omitted; also omits branchId updatesCalling
.toString()on possibly-undefined nested fields will throw at runtime (e.g., updating onlyurl). Additionally, branch changes cannot be persisted becausebranchIdisn’t mapped.Make the mapping conditional and include
branchId:-export const toDbPartialFrame = (frame: Partial<Frame>): Partial<DbFrame> => { - return { - id: frame.id, - url: frame.url, - x: frame.position?.x.toString(), - y: frame.position?.y.toString(), - canvasId: frame.canvasId, - width: frame.dimension?.width.toString(), - height: frame.dimension?.height.toString(), - }; -}; +export const toDbPartialFrame = (frame: Partial<Frame>): Partial<DbFrame> => { + const res: Partial<DbFrame> = {}; + if (frame.id !== undefined) res.id = frame.id; + if (frame.url !== undefined) res.url = frame.url; + if (frame.canvasId !== undefined) res.canvasId = frame.canvasId; + if (frame.branchId !== undefined) res.branchId = frame.branchId; + if (frame.position?.x !== undefined) res.x = String(frame.position.x); + if (frame.position?.y !== undefined) res.y = String(frame.position.y); + if (frame.dimension?.width !== undefined) res.width = String(frame.dimension.width); + if (frame.dimension?.height !== undefined) res.height = String(frame.dimension.height); + return res; +};Consider adding unit tests covering url-only, position-only, and dimension-only updates.
♻️ Duplicate comments (1)
apps/web/client/src/components/store/editor/frames/manager.ts (1)
1-1: Type-only import uses .tsx extension — prefer extensionless import or enable TS flagThis repeats a prior finding: importing with
.tsxrequiresallowImportingTsExtensions=true. Use an extensionless path (recommended) or enable the flag.-import type { FrameView } from '@/app/project/[id]/_components/canvas/frame/web-frame.tsx'; +import type { FrameView } from '@/app/project/[id]/_components/canvas/frame/web-frame';
🧹 Nitpick comments (30)
packages/models/src/project/branch.ts (4)
1-15: Consider exposing the project relationship in the domain model (align with DB’s required projectId).DB schema requires
projectId(non-null). If domainBranchinstances are passed around without a project reference, APIs/mappers will need external context to persist/update them. Recommend adding a lightweightproject: { id: string }(orprojectId: string) to avoid hidden coupling.Proposed change:
export interface Branch { id: string; + project: { id: string }; name: string; description: string | null; createdAt: string; updatedAt: string; git: {
5-6: Type timestamps asDate | stringto preserve fidelity across layers.Frontends often receive ISO strings, while server/domain may work with
Date. Allow both to reduce casting churn and accidental string operations on dates.- createdAt: string; - updatedAt: string; + createdAt: string | Date; + updatedAt: string | Date;
12-14: Verifysandboxshape consistency across models.Other models sometimes use
fooId: stringprimitives; here it’s{ id: string }. Either is fine—ensure consistency to avoid mapper/typing friction. If most models use primitives, considersandboxId: stringinstead.
1-15: Name collision with DB typeBranchmay cause import ambiguity.Both models and DB packages export a
Branchtype. This can lead to confusing imports in consumers. Consider aliasing in the DB package (e.g.,BranchRow) or re-exporting aliases in barrel files.packages/db/src/schema/project/branch.ts (6)
1-3: Add constraints and indexes; tighten column definitions.
- Enforce unique branch names per project.
- Index common predicates (
projectId,sandboxId) for joins.- Constrain lengths for
name,gitBranch,gitCommitSha,gitRepoUrl.- Add a check that if
gitBranchis set,gitRepoUrlmust be set.These are low-risk improvements that prevent bad data and improve query performance.
-import { relations } from 'drizzle-orm'; -import { pgTable, text, timestamp, uuid, varchar } from 'drizzle-orm/pg-core'; +import { relations, sql } from 'drizzle-orm'; +import { pgTable, text, timestamp, uuid, varchar, index, uniqueIndex, check } from 'drizzle-orm/pg-core'; import { createInsertSchema, createUpdateSchema } from 'drizzle-zod'; import { frames } from '../canvas/frame'; import { projects } from './project'; -export const branches = pgTable('branches', { +export const branches = pgTable('branches', { id: uuid('id').primaryKey().defaultRandom(), projectId: uuid('project_id') .notNull() .references(() => projects.id, { onDelete: 'cascade', onUpdate: 'cascade' }), // branch metadata - name: varchar('name').notNull(), + name: varchar('name', { length: 128 }).notNull(), description: text('description'), createdAt: timestamp('created_at', { withTimezone: true }).defaultNow().notNull(), updatedAt: timestamp('updated_at', { withTimezone: true }).defaultNow().notNull(), // git - gitBranch: varchar('git_branch'), - gitCommitSha: varchar('git_commit_sha'), - gitRepoUrl: varchar('git_repo_url'), + gitBranch: varchar('git_branch', { length: 255 }), + gitCommitSha: varchar('git_commit_sha', { length: 64 }), + gitRepoUrl: varchar('git_repo_url', { length: 2048 }), // sandbox sandboxId: varchar('sandbox_id').notNull(), -}).enableRLS(); +}, (t) => ({ + projectIdx: index('branches_project_idx').on(t.projectId), + sandboxIdx: index('branches_sandbox_idx').on(t.sandboxId), + uniqNamePerProject: uniqueIndex('branches_project_name_uniq').on(t.projectId, t.name), + requireRepoIfGit: check( + 'branches_git_branch_requires_repo', + sql`${t.gitBranch} is null or ${t.gitRepoUrl} is not null`, + ), +})).enableRLS();Also applies to: 7-26
28-30: Harden Zod schemas: validate names and restrict updates to mutable fields.
- Validate branch names (length + safe charset).
- Disallow updates to
id,projectId, andcreatedAtvia the generic update schema.-import { createInsertSchema, createUpdateSchema } from 'drizzle-zod'; +import { createInsertSchema, createUpdateSchema } from 'drizzle-zod'; +import { z } from 'zod'; @@ -export const branchInsertSchema = createInsertSchema(branches); -export const branchUpdateSchema = createUpdateSchema(branches); +export const branchInsertSchema = createInsertSchema(branches, { + name: z.string().min(1).max(128).regex(/^[A-Za-z0-9._/-]+$/, 'Invalid branch name'), + gitCommitSha: z.string().length(64).optional().nullable(), + gitRepoUrl: z.string().url().max(2048).optional().nullable(), +}); +export const branchUpdateSchema = createUpdateSchema(branches) + .omit({ id: true, projectId: true, createdAt: true });
16-17:updatedAtwill not auto-bump on UPDATE by default—add a DB trigger or handle in code.Postgres
defaultNow()only applies on INSERT. Ensure application code always setsupdated_at = now()on updates, or add a trigger to make this automatic.Example migration snippet:
CREATE OR REPLACE FUNCTION set_updated_at() RETURNS trigger AS $$ BEGIN NEW.updated_at = NOW(); RETURN NEW; END; $$ LANGUAGE plpgsql; DROP TRIGGER IF EXISTS branches_set_updated_at ON branches; CREATE TRIGGER branches_set_updated_at BEFORE UPDATE ON branches FOR EACH ROW EXECUTE FUNCTION set_updated_at();
31-37: Potential circular import betweenframesandbranches.
framesreferencesbranches(FK), and this file importsframesfor relations. Depending on module resolution, this can produce partially initialized bindings. Consider moving relations into a separate module (e.g.,schema/relations.ts) to break cycles, or keep FK references on one side and relations on the other.Example of decoupled relations file:
// packages/db/src/schema/relations.ts import { relations } from 'drizzle-orm'; import { frames } from './canvas/frame'; import { branches } from './project/branch'; import { projects } from './project/project'; export const branchRelations = relations(branches, ({ one, many }) => ({ project: one(projects, { fields: [branches.projectId], references: [projects.id] }), frames: many(frames), }));
26-26: RLS is enabled—verify policies exist for select/insert/update/delete.With RLS on and no policies, all access is denied. Ensure policies are defined (and tested) for your access model.
39-40: Avoid type name collision with domainBranch.To reduce confusion across packages, consider renaming these exports to
BranchRow/NewBranchRow(or at least provide aliases in barrel exports).-export type Branch = typeof branches.$inferSelect; -export type NewBranch = typeof branches.$inferInsert; +export type BranchRow = typeof branches.$inferSelect; +export type NewBranchRow = typeof branches.$inferInsert;packages/db/src/mappers/user/settings.ts (1)
22-32: Harden toDbUserSettings against partial inputsIf a partial
UserSettingsever slips through, direct property access can yieldundefinedinto not-null DB columns. Consider defensive defaults to mirror read-side behavior.Apply:
export const toDbUserSettings = (userId: string, settings: UserSettings): DbUserSettings => { return { id: settings.id, userId, - autoApplyCode: settings.chat.autoApplyCode, - expandCodeBlocks: settings.chat.expandCodeBlocks, - showSuggestions: settings.chat.showSuggestions, - showMiniChat: settings.chat.showMiniChat, - shouldWarnDelete: settings.editor.shouldWarnDelete, + autoApplyCode: settings.chat?.autoApplyCode ?? DefaultSettings.CHAT_SETTINGS.autoApplyCode, + expandCodeBlocks: settings.chat?.expandCodeBlocks ?? DefaultSettings.CHAT_SETTINGS.expandCodeBlocks, + showSuggestions: settings.chat?.showSuggestions ?? DefaultSettings.CHAT_SETTINGS.showSuggestions, + showMiniChat: settings.chat?.showMiniChat ?? DefaultSettings.CHAT_SETTINGS.showMiniChat, + shouldWarnDelete: settings.editor?.shouldWarnDelete ?? DefaultSettings.EDITOR_SETTINGS.shouldWarnDelete, }; };packages/db/src/mappers/project/settings.ts (1)
14-21: Write-side: consider optional chaining for robustnessIf callers ever pass a partial
ProjectSettings, direct access can throw or yieldundefined. Optional chaining with defaults is safer, matching DB defaults.export const toDbProjectSettings = (projectId: string, projectSettings: ProjectSettings): DbProjectSettings => { return { projectId, - buildCommand: projectSettings.commands.build ?? '', - runCommand: projectSettings.commands.run ?? '', - installCommand: projectSettings.commands.install ?? '' + buildCommand: projectSettings.commands?.build ?? '', + runCommand: projectSettings.commands?.run ?? '', + installCommand: projectSettings.commands?.install ?? '' }; };apps/web/client/src/components/ui/settings-modal/project/index.tsx (3)
5-5: Avoid leaking DB mappers into the client UIImporting toDbProjectSettings from @onlook/db couples the UI to DB-layer shapes. Prefer sending an API-level DTO and doing the DB mapping server-side in the TRPC router. This reduces churn when DB schema changes and keeps the client thinner.
If you want to keep the client-side mapper for now, at least wrap it locally (e.g., a ui→api mapper) so the import surface doesn’t expose DB concerns directly.
66-70: Invalidate settings query after updating the project nameAfter changing the name, you invalidate project.list and project.get, but not the settings query. If other tabs/components rely on settings.get, they can remain stale.
Apply this diff:
- await Promise.all([ - utils.project.list.invalidate(), - utils.project.get.invalidate({ projectId: editorEngine.projectId }), - ]); + await Promise.all([ + utils.project.list.invalidate(), + utils.project.get.invalidate({ projectId: editorEngine.projectId }), + utils.settings.get.invalidate({ projectId: editorEngine.projectId }), + ]);
61-65: Nit: trim name before saving to avoid accidental trailing spacesMinor UX polish: trim whitespace so name changes aren’t triggered by stray spaces.
- project: { name: formData.name } + project: { name: formData.name.trim() }apps/web/client/src/components/store/editor/chat/conversation.ts (1)
2-2: UI depending on DB mappers leaks persistence concernsImporting toDbMessage in the client ties chat UI to the DB schema. Prefer passing ChatMessage to the API and performing the mapping server-side (router) to control schema drift and keep client agnostic of DB.
If changing now is costly, consider localizing the dependency behind a thin client-side adapter so the import path isn’t @onlook/db directly.
apps/web/client/src/server/api/routers/project/invitation.ts (2)
45-49: Mapper rename to fromDbUser is correct; consider removing the ts-expect-errorThe switch is good. The lingering // @ts-expect-error hints at a missing Drizzle typing. If feasible, tighten the typing to avoid suppression.
Example approach:
- Define an explicit type for the query result (with inviter populated).
- Or cast invitation.inviter to the concrete DbUser type returned by Drizzle before mapping.
Minimal adjustment:
- // @ts-expect-error - Drizzle is not typed correctly - inviter: fromDbUser(invitation.inviter), + inviter: fromDbUser(invitation.inviter as any),Longer-term: fix the Drizzle schema typings so with: { inviter: true } is inferred correctly.
73-78: Same as above for getWithoutToken: mapper rename is good; revisit the ts-expect-errorMirrors the prior comment; keep types precise if possible.
apps/web/client/src/server/api/routers/chat/message.ts (2)
94-101: Normalize timestamps consistently; consider defaulting/normalizing updatedAt
normalizeMessagesmartly handlescreatedAtwhen provided as a string. Mirror that forupdatedAtso inserts/updates have consistent types. With the separateupdateSetin upsert, this won’t mutatecreatedAton conflicts.Apply:
return { ...message, role: message.role as ChatMessageRole, parts: message.parts as Message['parts'], - createdAt: typeof message.createdAt === 'string' ? new Date(message.createdAt) : message.createdAt, + createdAt: typeof message.createdAt === 'string' ? new Date(message.createdAt) : message.createdAt, + updatedAt: typeof (message as any).updatedAt === 'string' ? new Date((message as any).updatedAt) : (message as any).updatedAt, };
52-58: Consider onConflict handling for bulk upsert to avoid duplicate key errorsBulk insert without conflict handling can fail if any
idalready exists. If the intended behavior is idempotent sync, addonConflictDoNothing()oronConflictDoUpdate(...).Example:
await ctx.db .insert(messages) .values(normalizedMessages) .onConflictDoNothing({ target: [messages.id] });apps/web/client/src/server/api/routers/chat/conversation.ts (1)
98-103: Optional: also bump updatedAt when generating a titleKeeping
updatedAtin sync helps ordering by recency.Apply:
- await ctx.db.update(conversations).set({ - displayName: generatedName, - }).where(eq(conversations.id, input.conversationId)); + await ctx.db.update(conversations).set({ + displayName: generatedName, + updatedAt: new Date(), + }).where(eq(conversations.id, input.conversationId));apps/web/client/src/server/api/routers/user/user.ts (1)
25-26: Optional hardening: guard access to avatarUrl from auth metadataSupabase
user_metadatashape can vary. Optional chaining avoids runtime errors ifavatarUrlis missing.Apply:
- avatarUrl: user.avatarUrl ?? authUser.user_metadata.avatarUrl, + avatarUrl: user.avatarUrl ?? authUser.user_metadata?.avatarUrl,- avatarUrl: input.avatarUrl ?? authUser.user_metadata.avatarUrl, + avatarUrl: input.avatarUrl ?? authUser.user_metadata?.avatarUrl,Also applies to: 59-60
apps/web/client/src/server/api/routers/user/user-settings.ts (2)
11-12: Heads-up: GET returns an ephemeral default ID when row is missing.
fromDbUserSettings(settings ?? createDefaultUserSettings(user.id))fabricates a new UUID on every cache miss but doesn’t persist it. If the client relies on a stable settingsid, this can cause churn and odd diffs.Optional: create-on-read so the ID is stable.
Proposed change:
- return fromDbUserSettings(settings ?? createDefaultUserSettings(user.id)); + if (!settings) { + const [inserted] = await ctx.db + .insert(userSettings) + .values(createDefaultUserSettings(user.id)) + .returning(); + return fromDbUserSettings(inserted); + } + return fromDbUserSettings(settings);
27-31: Consider standardized TRPC errors for consistency.Throwing a raw
Errorhere diverges from other routers usingTRPCError. Optional, but improves client error handling consistency.Suggested tweak:
- throw new Error('Failed to update user settings'); + throw new TRPCError({ + code: 'INTERNAL_SERVER_ERROR', + message: 'Failed to update user settings', + });Add the import:
+import { TRPCError } from '@trpc/server';packages/db/src/mappers/chat/message.ts (2)
7-23: Avoid leaking DB-only fields into ChatMessage; map only what you need.
const baseMessage = { ...message, ... }pulls DB fields (e.g.,conversationId,applied,commitOid,snapshots) into the domain object. Not harmful at runtime, but it couples layers and risks accidental serialization later.Apply this refactor to construct a minimal base shape:
-export const fromDbMessage = (message: DbMessage): ChatMessage => { +export const fromDbMessage = (message: DbMessage): ChatMessage => { const content = { format: 2 as const, parts: message.parts ?? [], metadata: { vercelId: message.id, context: message.context ?? [], checkpoints: message.checkpoints ?? [], } } - const baseMessage = { - ...message, - content, - threadId: message.conversationId, - } + const baseMessage = { + id: message.id, + createdAt: message.createdAt, + threadId: message.conversationId, + role: message.role as ChatMessageRole, + content, + }
40-59: Preserving DB extension fields (applied/commitOid/snapshots) is optional but consider metadata passthrough.You currently hard-set these to
null. If callers ever populate them (e.g., code-application checkpoints), they’ll be dropped.Possible improvement:
export const toDbMessage = (message: ChatMessage): DbMessage => { return { id: message.id, createdAt: message.createdAt, conversationId: message.threadId, context: message.content.metadata?.context ?? [], parts: message.content.parts, content: message.content.parts.map((part) => { if (part.type === 'text') { return part.text; } return ''; }).join(''), role: message.role as DbMessage['role'], checkpoints: message.content.metadata?.checkpoints ?? [], - applied: null, - commitOid: null, - snapshots: null, + applied: (message as any).applied ?? null, + commitOid: (message as any).commitOid ?? null, + snapshots: (message as any).snapshots ?? null, } satisfies DbMessage; }Also verify that reducing non-text parts to
contenttext is acceptable for search/logging and won’t lose critical info (e.g., code/tool parts).packages/db/src/mappers/project/project.ts (1)
20-33: Minor: consider guarding unintended updatedPreviewImgAt bumps on generic writes
toDbPreviewImgdefaultsupdatedPreviewImgAttonew Date()whenpreviewImgis provided withoutupdatedAt. If reused in generic update flows, this could update the “last updated” timestamp even when the preview didn’t change.If that’s not desired, require callers to explicitly set
updatedAtwhen they intend to bump the timestamp, and otherwise leave itnullto avoid touching the DB column.apps/web/client/src/components/store/editor/frames/manager.ts (3)
70-75: Guard URL parsing in registerView to avoid crashes on invalid/blank src
new URL(view.src)can throw for relative or special URLs (e.g., about:blank). Wrap in try/catch and register a safe default.registerView(frame: Frame, view: FrameView) { const isSelected = this.isSelected(frame.id); this._frameIdToData.set(frame.id, { frame, view, selected: isSelected }); - const framePathname = new URL(view.src).pathname; - this._navigation.registerFrame(frame.id, framePathname); + try { + const framePathname = new URL(view.src).pathname; + this._navigation.registerFrame(frame.id, framePathname); + } catch { + this._navigation.registerFrame(frame.id, '/'); + } }
165-176: Navigation on non-http origins can generate invalid URLsWhen the iframe
srcisabout:blank,new URL(currentUrl).originyields'null'; concatenating'null' + pathproduces an invalid URL. Consider validating the scheme.- const currentUrl = frameData.view.src; - const baseUrl = currentUrl ? new URL(currentUrl).origin : null; + const currentUrl = frameData.view.src; + let baseUrl: string | null = null; + try { + const u = new URL(currentUrl); + baseUrl = /^https?:$/.test(u.protocol) ? u.origin : null; + } catch { /* ignore */ }
259-266: Debounced save doesn’t return a promise; no need to await
lodash.debouncereturns a void-returning function.await this.saveToStorage(...)is a no-op and suggests false synchronicity.- await this.saveToStorage(frameId, frame); + this.saveToStorage(frameId, frame);
📜 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 (28)
apps/web/client/src/components/store/editor/chat/conversation.ts(2 hunks)apps/web/client/src/components/store/editor/frames/manager.ts(7 hunks)apps/web/client/src/components/ui/settings-modal/project/index.tsx(2 hunks)apps/web/client/src/server/api/routers/chat/conversation.ts(5 hunks)apps/web/client/src/server/api/routers/chat/message.ts(2 hunks)apps/web/client/src/server/api/routers/project/frame.ts(4 hunks)apps/web/client/src/server/api/routers/project/invitation.ts(3 hunks)apps/web/client/src/server/api/routers/project/member.ts(2 hunks)apps/web/client/src/server/api/routers/project/project.ts(6 hunks)apps/web/client/src/server/api/routers/project/settings.ts(3 hunks)apps/web/client/src/server/api/routers/subscription/subscription.ts(2 hunks)apps/web/client/src/server/api/routers/user/user-canvas.ts(3 hunks)apps/web/client/src/server/api/routers/user/user-settings.ts(3 hunks)apps/web/client/src/server/api/routers/user/user.ts(2 hunks)packages/db/src/index.ts(1 hunks)packages/db/src/mappers/chat/conversation.ts(2 hunks)packages/db/src/mappers/chat/message.ts(3 hunks)packages/db/src/mappers/index.ts(1 hunks)packages/db/src/mappers/project/canvas.ts(2 hunks)packages/db/src/mappers/project/frame.ts(2 hunks)packages/db/src/mappers/project/index.ts(1 hunks)packages/db/src/mappers/project/project.ts(4 hunks)packages/db/src/mappers/project/settings.ts(2 hunks)packages/db/src/mappers/subscription.ts(4 hunks)packages/db/src/mappers/user/settings.ts(2 hunks)packages/db/src/mappers/user/user.ts(2 hunks)packages/db/src/schema/project/branch.ts(1 hunks)packages/models/src/project/branch.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/client/src/server/api/routers/project/frame.ts
🧰 Additional context used
🧬 Code graph analysis (19)
apps/web/client/src/server/api/routers/project/invitation.ts (1)
packages/db/src/mappers/user/user.ts (1)
fromDbUser(18-30)
apps/web/client/src/components/store/editor/chat/conversation.ts (2)
apps/web/client/src/trpc/react.tsx (1)
api(23-23)packages/db/src/mappers/chat/message.ts (1)
toDbMessage(40-59)
packages/models/src/project/branch.ts (1)
packages/db/src/schema/project/branch.ts (1)
Branch(39-39)
apps/web/client/src/server/api/routers/project/settings.ts (1)
packages/db/src/mappers/project/settings.ts (1)
fromDbProjectSettings(4-12)
apps/web/client/src/components/ui/settings-modal/project/index.tsx (1)
packages/db/src/mappers/project/settings.ts (1)
toDbProjectSettings(14-21)
apps/web/client/src/server/api/routers/chat/conversation.ts (1)
packages/db/src/mappers/chat/conversation.ts (1)
fromDbConversation(4-12)
packages/db/src/mappers/project/settings.ts (1)
packages/db/src/schema/project/settings.ts (1)
projectSettings(6-14)
apps/web/client/src/server/api/routers/chat/message.ts (1)
packages/db/src/mappers/chat/message.ts (1)
fromDbMessage(7-38)
packages/db/src/mappers/chat/conversation.ts (1)
packages/models/src/chat/conversation/index.ts (1)
ChatConversation(4-8)
apps/web/client/src/server/api/routers/user/user.ts (1)
packages/db/src/mappers/user/user.ts (1)
fromDbUser(18-30)
apps/web/client/src/server/api/routers/user/user-canvas.ts (2)
packages/db/src/mappers/project/canvas.ts (1)
fromDbCanvas(4-13)packages/db/src/mappers/project/frame.ts (1)
fromDbFrame(4-19)
packages/db/src/schema/project/branch.ts (3)
packages/db/src/schema/project/project.ts (1)
projects(11-26)packages/db/src/schema/canvas/frame.ts (1)
frames(7-22)packages/models/src/project/branch.ts (1)
Branch(1-15)
apps/web/client/src/server/api/routers/subscription/subscription.ts (1)
packages/db/src/mappers/subscription.ts (1)
fromDbSubscription(4-24)
apps/web/client/src/server/api/routers/user/user-settings.ts (3)
packages/db/src/mappers/user/settings.ts (1)
fromDbUserSettings(5-20)packages/db/src/defaults/user-settings.ts (1)
createDefaultUserSettings(5-15)packages/db/src/schema/user/settings.ts (1)
userSettings(6-18)
apps/web/client/src/server/api/routers/project/member.ts (1)
packages/db/src/mappers/user/user.ts (1)
fromDbUser(18-30)
apps/web/client/src/components/store/editor/frames/manager.ts (4)
apps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx (1)
FrameView(10-28)apps/web/client/src/app/project/[id]/_components/canvas/frame/web-frame.tsx (1)
FrameView(25-30)packages/models/src/project/frame.ts (1)
Frame(4-16)packages/db/src/mappers/project/frame.ts (2)
toDbFrame(21-32)toDbPartialFrame(34-44)
packages/db/src/mappers/project/project.ts (1)
packages/models/src/project/project.ts (1)
PreviewImg(13-21)
packages/db/src/mappers/chat/message.ts (1)
packages/models/src/chat/message/message.ts (1)
ChatMessage(34-34)
apps/web/client/src/server/api/routers/project/project.ts (5)
packages/db/src/mappers/project/project.ts (2)
toDbPreviewImg(57-87)fromDbProject(4-18)packages/constants/src/storage.ts (1)
STORAGE_BUCKETS(1-4)packages/db/src/mappers/project/canvas.ts (1)
fromDbCanvas(4-13)packages/db/src/mappers/project/frame.ts (1)
fromDbFrame(4-19)packages/db/src/schema/project/project.ts (1)
projects(11-26)
🔇 Additional comments (35)
packages/db/src/mappers/user/user.ts (2)
4-16: Rename toDbUser verified – no lingeringfromUserreferences
ThetoDbUsermapping remains a 1:1 transformation and aligns with thefromDbX/toDbXconvention. A grep check for any residualfromUser(calls returned no hits, confirming there are no old references to update.
18-30: Rename fromDbUser: no residualtoUsercalls foundRound-trip mapping is lossless for all fields. Confirmed no references to the old
toUseridentifier remain in the codebase. Code changes are approved.packages/db/src/mappers/user/settings.ts (1)
5-20: Defaults applied with nullish coalescing are correctUsing
??preserves explicit false values. The shape matchesUserSettings. LGTM.packages/db/src/mappers/project/settings.ts (1)
4-12: Rename maintains behavior; mapping is straightforwardRead-side mapper cleanly projects flat DB fields into nested
commands. Looks good.apps/web/client/src/components/ui/settings-modal/project/index.tsx (1)
72-84: No mismatch in TRPC input schema for settings.upsert
I’ve verified that in apps/web/client/src/server/api/routers/project/settings.ts, the upsert procedure is defined as:.input( z.object({ projectId: z.string(), settings: projectSettingsInsertSchema, }), )and
projectSettingsInsertSchema(fromcreateInsertSchema(projectSettings)) requires exactly the four fields{ projectId, runCommand, buildCommand, installCommand }. The helpertoDbProjectSettings(editorEngine.projectId, { commands: { … } })indeed returns an object with those four keys, so passingprojectIdboth at the top level and insidesettingsis intentional and satisfies the Zod schema.No breaking schema mismatch will occur, and Zod will accept the nested
projectId. If you’d rather remove the redundancy, you can optionally refactor to flatten the input—either by:
- Dropping
projectIdfrom the nested settings and merging it into.values({ projectId, …settings })in the mutation, or- Removing the top-level
projectIdfrom the.input(...)and having clients pass only the fullNewProjectSettingsobject.However, as written, the code aligns with the TRPC/Zod definitions and requires no urgent changes.
packages/db/src/mappers/subscription.ts (4)
4-24: Rename to fromDbSubscription is consistent and the mapper looks correctThe renaming aligns with the repo-wide convention. Field mapping (id/status/dates/product/price/scheduledChange/stripe fields) is straightforward and correct.
26-42: fromDbProduct/fromDbPrice mappings LGTMMapping names and fields match domain expectations; no behavioral changes introduced.
44-61: Scheduled change mapper: sensible null-guard and reuse of fromDbPriceNull return when scheduledAction or scheduledChangeAt is missing is appropriate. Reusing fromDbPrice for the nested price keeps consistency.
1-61: No stale mapper references found; please verify external consumersThe grep search for the old names (
toSubscription,toProduct,toPrice,toScheduledChange) returned no matches in the repository, indicating all in-repo imports have been renamed to the newfromDb*functions.Before merging, please manually confirm that:
- Any external packages or services consuming this module have updated their imports to
fromDbSubscription,fromDbProduct,fromDbPrice, andfromDbScheduledChange.- All documentation, example code, tests, and CI workflows referencing the old names have been updated.
- The build completes successfully in downstream repositories or applications that depend on these mappers.
apps/web/client/src/server/api/routers/subscription/subscription.ts (2)
2-2: Import switch to fromDbSubscription matches DB mapper renameGood alignment with the new mapper naming; no other control flow changes.
46-47: Return mapping with fromDbSubscription: ensure scheduled price is loaded as intendedscheduledPrice is conditionally fetched and passed; with: { product: true, price: true } ensures nested entities are present for the mapper. Looks correct.
apps/web/client/src/components/store/editor/chat/conversation.ts (1)
213-215: Confirmchat.message.upsertZod input schema expects fullDbMessage
I don’t see a localchatrouter underapps/web/server/src/router—it’s likely pulled in from the@onlook/web-serverpackage. Before landing this, please:
- Open the TRPC router definition for
chat.message.upsert(e.g. in@onlook/web-server/src/router/chat.tsor wherever thechatrouter is defined).- Verify its Zod
input(...)schema requires all fields of yourDbMessage(id, conversationId, parts, content, role, context, checkpoints, etc.).- If the router expects a client-side
ChatMessageinstead, move thetoDbMessage(message)mapping into the server implementation.File of concern:
- apps/web/client/src/components/store/editor/chat/conversation.ts
Lines 213–215:async upsertMessageInStorage(message: ChatMessage) { await api.chat.message.upsert.mutate({ message: toDbMessage(message) }); }packages/db/src/mappers/project/index.ts (1)
1-2: Verified barrel exports for project mappersAll expected files and symbols are present; no broken imports will occur at build time.
• packages/db/src/mappers/project/canvas.ts exists and exports
fromDbCanvasandtoDbCanvas
• packages/db/src/mappers/project/frame.ts exists and exportsfromDbFrameandtoDbFrameThe lines in
packages/db/src/mappers/project/index.ts:export * from './canvas'; export * from './frame';correctly re-export these mapper functions. No further action required.
apps/web/client/src/server/api/routers/chat/message.ts (2)
6-8: Mapper rename import is correct and consistent with the new fromDb/toDb convention**Switching to
fromDbMessagealigns with the repository-wide mapper renames.
24-25: Return mapping with fromDbMessage is correctMapping the DB rows before returning keeps the router’s contract stable.
apps/web/client/src/server/api/routers/chat/conversation.ts (1)
6-7: Mapper rename import is correctUsing
fromDbConversationmatches the new mapper naming strategy.apps/web/client/src/server/api/routers/user/user.ts (2)
3-3: Mapper rename import looks correct
fromDbUserimport matches the mapper refactor.
19-26: Good: mapping DB row to domain with merged defaultsMerging Supabase metadata defaults into the DB record before
fromDbUser(...)keeps the returned shape consistent.packages/db/src/mappers/index.ts (2)
1-5: LGTM on the consolidated mapper exportsExporting
chat,domain,project,subscription, anduserkeeps the public API tidy and matches the refactor.
1-1: Top-level mapper barrel migration confirmed; no stale direct imports detectedI ran the suggested searches and verified:
- No occurrences of imports from
@onlook/db/.../mappers/(canvas|frame|conversation|message)remain.- The new
packages/db/src/mappers/index.tscleanly exports thechatandprojectbarrels (alongsidedomain,subscription, anduser).- Inside
chat/index.ts, bothconversation.tsandmessage.tsare re-exported.- Inside
project/index.ts,canvas.ts,frame.ts,project.ts, andsettings.tsare re-exported.All import sites are now correctly using the new barrel structure. No action is needed.
apps/web/client/src/server/api/routers/project/settings.ts (3)
4-5: Mapper rename is consistent with the new fromDb convention.*Importing
fromDbProjectSettingsaligns with the mapper rename across the PR.
25-26: Return mapper usage looks correct.
fromDbProjectSettings(setting)is the right direction-of-data transformation for the GET path.
49-50: Upsert return mapping is correct.Returning
fromDbProjectSettings(updatedSettings)matches the DB→API direction.apps/web/client/src/server/api/routers/user/user-settings.ts (3)
1-1: Import rename to fromDbUserSettings is consistent with the project-wide mapper changes.
23-24: Insert path mapping LGTM.
fromDbUserSettings(insertedSettings ?? newSettings)preserves behavior even ifreturning()comes back empty.
31-32: Update path mapping LGTM.apps/web/client/src/server/api/routers/user/user-canvas.ts (3)
5-7: Mapper imports migrated cleanly (to ➜ fromDb).**This aligns with the rest of the PR’s mapper API.
36-37: Return mapping for GET is correct.
fromDbCanvas(userCanvas)uses the user_canvas row fields and ignores the joinedcanvasrelation—works fine with structural typing.
59-61: Frames mapping LGTM; now includes branchId via fromDbFrame.Verify downstream UI expects
branchIdonFrameand handles null for legacy data.packages/db/src/mappers/chat/message.ts (1)
96-98: Rename usage LGTM.
toDbMessageFromVercelnow correctly callstoDbMessage(...).packages/db/src/mappers/project/canvas.ts (3)
2-3: Import path switch to schema index is fine.
4-13: DB→API canvas mapper looks correct (types and numeric coercions).
15-22: Verification Complete: No lingering old mapper usages found
- Executed a ripgrep search for
toCanvas(andfromCanvas(across the codebase (excludingdist/andbuild/) and found no remaining occurrences.API→DB mapper rename and serialization LGTM. Casting numbers to strings for numeric columns matches prior conventions in this codebase.
apps/web/client/src/server/api/routers/project/project.ts (1)
135-148: Preview image write path looks solidGood use of
toDbPreviewImgto keep storage-path andupdatedPreviewImgAtin sync, and explicit content-type on upload. The fallback to compressed JPEG is sensible for preview thumbnails.packages/db/src/mappers/project/project.ts (1)
36-55: Preview image mappers are consistent and correctly carry updatedAt
fromDbPreviewImgandtoDbPreviewImgmirror each other well, includingupdatedPreviewImgAtplumbed throughPreviewImg.updatedAt. This aligns with the server’s screenshot write path.
| @@ -1,4 +1,4 @@ | |||
| export * from './defaults'; | |||
| export * from './dto'; | |||
| export * from './mappers'; | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Public API surface change: confirm downstream impact or provide a migration path
Switching the root export from './dto' to './mappers' is a breaking change for external consumers relying on the old surface. Either (a) ship a semver-major with release notes and a migration guide, or (b) keep a transitional alias to avoid immediate breakage.
Option (b) example (only if './dto' still exists and doesn’t conflict):
export * from './defaults';
-export * from './mappers';
+export * from './mappers';
+// Optional: transitional aliases (remove in next major)
+export * as mappers from './mappers';
+export * as dto from './dto';
export * from './schema';
export * from './seed/constants';Run this to find internal/dependent imports of legacy dto or legacy function names before merging:
🏁 Script executed:
#!/bin/bash
# Find legacy imports/symbols likely affected by this change
rg -nP -C2 "(from\s+['\"]@onlook/db/dto['\"]|toUserSettings\s*\(|fromUserSettings\s*\(|toConversation\s*\(|fromConversationToDb\s*\(|toUser\s*\(|fromUser\s*\()" --glob '!**/node_modules/**'Length of output: 195
🏁 Script executed:
#!/bin/bash
# Verify existence of legacy dto directory
if [ -d "packages/db/src/dto" ]; then
echo "dto directory exists at packages/db/src/dto"
else
echo "dto directory does not exist at packages/db/src/dto"
fi
# Search for any import of sub-path "@onlook/db/dto"
rg -nP "from\s+['\"].*@onlook/db/dto" --glob '!**/node_modules/**' -C2
# Search for any import from sub-directory "dto" (relative or package)
rg -nP "from\s+['\"].*(?:/dto|dto)(?:['\"/])" --glob '!**/node_modules/**' -C2Length of output: 305
Public API surface change requires action
Switching the root export in packages/db/src/index.ts from ./dto to ./mappers removes all of the old DTO exports. Since the packages/db/src/dto directory no longer exists, a transitional alias isn’t in place and external consumers depending on those DTO symbols will break.
Please choose one of the following:
- Ship a semver-major release and include detailed release notes with a migration guide that shows how to update imports (e.g. replace
@onlook/dbDTO imports with the new mapper exports). - (Optional) Add a temporary DTO stub to preserve backward compatibility, then remove it in the next major. For example:
- Re-create
packages/db/src/dto/index.ts:// packages/db/src/dto/index.ts export * from '../mappers';
- Update
packages/db/src/index.tsto include the transitional alias:export * from './defaults'; -export * from './mappers'; +export * from './mappers'; +// Transitional alias—remove in next major +export * from './dto'; export * from './schema'; export * from './seed/constants';
- Re-create
File to update:
packages/db/src/index.ts(around line 2)
🤖 Prompt for AI Agents
In packages/db/src/index.ts around line 2, the root export was switched from
./dto to ./mappers which removed the old DTO exports and breaks external
consumers; to preserve backward compatibility, recreate
packages/db/src/dto/index.ts as a temporary alias that re-exports everything
from ../mappers, then update packages/db/src/index.ts to export both from
'./mappers' and './dto' (so existing imports continue to work); alternatively,
if you intend a breaking change, prepare a semver-major release and add clear
migration notes showing how to replace DTO imports with the new mapper exports.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
apps/web/client/src/app/project/[id]/_components/canvas/frame/view.tsx (2)
107-111: Wildcard allowedOrigins is a security riskallowedOrigins: ['*'] accepts messages from any origin, defeating postMessage origin checks. Restrict it to the iframe’s origin.
Apply:
- const messenger = new WindowMessenger({ - remoteWindow: iframeRef.current.contentWindow, - allowedOrigins: ['*'], - }); + // Restrict messages to the child iframe’s origin + const childOrigin = (() => { + try { + const src = iframeRef.current?.src ?? frame.url; + return new URL(src).origin; + } catch { + // Fallback: current app origin (prefer over '*') + return location.origin; + } + })(); + const messenger = new WindowMessenger({ + remoteWindow: iframeRef.current.contentWindow, + allowedOrigins: [childOrigin], + });If multiple origins are legitimately needed, gate them behind configuration and list them explicitly.
284-286: Iframe permissions are overly permissive (camera/mic/geolocation enabled)Sandbox/allow grant powerful capabilities to untrusted content (camera, mic, geolocation, downloads). This is a high-risk posture. Minimize by default and enable per feature flag.
Apply:
- sandbox="allow-modals allow-forms allow-same-origin allow-scripts allow-popups allow-downloads" - allow="geolocation; microphone; camera; midi; encrypted-media" + sandbox="allow-scripts allow-same-origin allow-popups" + allow="encrypted-media" + referrerPolicy="no-referrer" + loading="lazy"If your UX needs additional capabilities, gate them via explicit settings and only for trusted origins.
apps/web/client/src/components/store/editor/frames/manager.ts (4)
158-189: Robust URL composition and immediate navigationString concatenation with origin + path breaks on missing leading “/”, query-only paths, and hashes. Also, the iframe doesn’t navigate immediately.
Apply:
- try { - const currentUrl = frameData.view.src; - const baseUrl = currentUrl ? new URL(currentUrl).origin : null; - - if (!baseUrl) { - console.warn('No base URL found'); - return; - } - - await this.updateAndSaveToStorage(frameId, { url: `${baseUrl}${path}` }); + try { + const currentUrl = frameData.view.src; + const newUrl = new URL(path, currentUrl).toString(); + + // Navigate immediately + frameData.view.src = newUrl; + + // Persist + await this.updateAndSaveToStorage(frameId, { url: newUrl });This keeps the iframe, DB, and navigation history consistent.
191-201: Don’t require a loaded view to delete a frameDeletion shouldn’t depend on view availability. This blocks deletes for frames not yet mounted.
Apply:
- const frameData = this.get(id); - if (!frameData?.view) { - console.error('Frame not found for delete', id); - return; - } + const frameData = this.get(id); + if (!frameData) { + console.error('Frame not found for delete', id); + return; + }
224-242: Duplicate shouldn’t depend on view; also fix missing semicolonThere’s an unnecessary view gate, and Line 231 is missing a semicolon, causing a TS/JS syntax error.
Apply:
- const frameData = this.get(id); - if (!frameData?.view) { - console.error('Frame view not found for duplicate', id); - return; - } - - const frame = frameData.frame + const frameData = this.get(id); + if (!frameData) { + console.error('Frame not found for duplicate', id); + return; + } + + const frame = frameData.frame; const newFrame: Frame = { ...frame, id: uuid(), position: { x: frame.position.x + frame.dimension.width + 100, y: frame.position.y, }, };
259-266: AddbranchIdto the partial‐frame mapperThe
toDbPartialFramefunction inpackages/db/src/mappers/project/frame.tsonly mapsidandurl, so anybranchIdpassed through a partial update (e.g. viaundebouncedSaveToStorage) will be dropped. To support moving frames across branches, you must:
- In
packages/db/src/mappers/project/frame.ts, updatetoDbPartialFrameto includebranchId:export const toDbPartialFrame = (frame: Partial<Frame>): Partial<DbFrame> => { return { id: frame.id, url: frame.url,
branchId: frame.branchId, // …other optional fields
};
- Verify the server’s frame‐update endpoint accepts and applies a partial
branchIdchange.
♻️ Duplicate comments (1)
apps/web/client/src/components/store/editor/frames/manager.ts (1)
1-1: Thanks for switching to extensionless importThis addresses the prior tsconfig import extension issue flagged earlier.
🧹 Nitpick comments (6)
apps/web/client/src/app/project/[id]/_components/canvas/frame/view.tsx (3)
59-61: Debounced reload should be canceled on unmountThe debounced reloadIframe can fire after unmount. Cancel it in the cleanup to avoid stray timers.
Apply:
useEffect(() => { return () => { + // cancel pending debounced reloads + reloadIframe.cancel(); if (connectionRef.current) { connectionRef.current.destroy(); connectionRef.current = null; } setPenpalChild(null); isConnecting.current = false; }; }, []);Also applies to: 267-276
146-152: Handle child init promises to avoid unhandled rejectionsPenpal methods typically return promises. Fire-and-forget calls without handlers risk unhandled rejections and silent failures.
Apply:
- setPenpalChild(remote); - remote.setFrameId(frame.id); - remote.handleBodyReady(); - remote.processDom(); + setPenpalChild(remote); + Promise.allSettled([ + remote.setFrameId(frame.id), + remote.handleBodyReady(), + remote.processDom(), + ]).then(() => { + console.log(`${PENPAL_PARENT_CHANNEL} (${frame.id}) - Child initialized`); + }).catch((e) => { + console.error(`${PENPAL_PARENT_CHANNEL} (${frame.id}) - Child init failed`, e); + retrySetupPenpalConnection(e as Error); + });
232-237: Avoid returning a dummy {} as FrameViewReturning {} as FrameView can cause hard-to-debug runtime errors when callers dereference properties. Prefer a concrete fallback with no-op methods.
Apply:
- if (!iframe) { - console.error(`${PENPAL_PARENT_CHANNEL} (${frame.id}) - Iframe - Not found`); - return {} as FrameView; - } + if (!iframe) { + console.error(`${PENPAL_PARENT_CHANNEL} (${frame.id}) - Iframe - Not found`); + const fallback = Object.assign(document.createElement('iframe'), { + supportsOpenDevTools: () => false, + setZoomLevel: () => {}, + reload: () => {}, + isLoading: () => true, + ...remoteMethods, + }) as FrameView; + return fallback; + }And keep the registerView path unchanged once the real iframe is available.
Also applies to: 255-260
apps/web/client/src/components/store/editor/frames/manager.ts (3)
48-52: Consider clearing stale frames before applyingapplyFrames adds/updates entries but won’t remove frames that no longer exist, leading to stale state. Optionally reset first when this method is intended to replace the set.
Example:
applyFrames(frames: Frame[]) { - for (const frame of frames) { - this._frameIdToData.set(frame.id, { frame, view: null, selected: false }); - } + const next = new Map<string, FrameData>(); + for (const frame of frames) { + const prev = this._frameIdToData.get(frame.id); + next.set(frame.id, { frame, view: prev?.view ?? null, selected: prev?.selected ?? false }); + } + this._frameIdToData = next; }
70-75: new URL(view.src) can throw; guard for invalid/opaque URLsAbout:blank or malformed URLs will throw. Use a safe parse with fallback to '/'.
Apply:
- const framePathname = new URL(view.src).pathname; - this._navigation.registerFrame(frame.id, framePathname); + try { + const { pathname } = new URL(view.src); + this._navigation.registerFrame(frame.id, pathname); + } catch { + console.warn('Unable to parse view src for navigation history', view.src); + this._navigation.registerFrame(frame.id, '/'); + }
244-256: Round dimensions before persisting partial updates (optional)If callers pass fractional values in frame, you’ll persist unrounded values. Consider rounding provided fields before save.
You can add a helper:
function roundPartialFrame(p: Partial<Frame>): Partial<Frame> { const r = { ...p }; if (p.position) { r.position = { x: Math.round(p.position.x ?? 0), y: Math.round(p.position.y ?? 0), }; } if (p.dimension) { r.dimension = { width: Math.round(p.dimension.width ?? 0), height: Math.round(p.dimension.height ?? 0), }; } return r; }And use it in updateAndSaveToStorage and undebouncedSaveToStorage.
📜 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 (7)
apps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/canvas/frame/view.tsx(4 hunks)apps/web/client/src/components/store/editor/frames/manager.ts(7 hunks)apps/web/client/src/components/store/editor/insert/index.ts(4 hunks)apps/web/client/src/components/store/editor/overlay/utils.ts(4 hunks)apps/web/client/src/components/store/editor/text/index.ts(4 hunks)packages/db/src/schema/canvas/frame.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/web/client/src/components/store/editor/overlay/utils.ts
- apps/web/client/src/components/store/editor/insert/index.ts
- apps/web/client/src/components/store/editor/text/index.ts
- apps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx
- packages/db/src/schema/canvas/frame.ts
🧰 Additional context used
🧬 Code graph analysis (2)
apps/web/client/src/app/project/[id]/_components/canvas/frame/view.tsx (3)
apps/web/client/src/app/project/[id]/_components/canvas/frame/index.tsx (1)
FrameView(10-28)packages/db/src/schema/canvas/frame.ts (1)
Frame(30-30)packages/penpal/src/parent.ts (1)
PENPAL_PARENT_CHANNEL(20-20)
apps/web/client/src/components/store/editor/frames/manager.ts (3)
apps/web/client/src/app/project/[id]/_components/canvas/frame/view.tsx (1)
FrameView(25-30)packages/models/src/project/frame.ts (1)
Frame(4-16)packages/db/src/mappers/project/frame.ts (2)
toDbFrame(21-32)toDbPartialFrame(34-44)
🔇 Additional comments (4)
apps/web/client/src/app/project/[id]/_components/canvas/frame/view.tsx (3)
4-4: Good switch to unified Frame typeImporting the Frame type from @onlook/models keeps the UI aligned with the new model schema (branchId/canvasId, etc.).
259-259: Right place to register the viewRegistering after a successful child connection ensures the manager sees a ready view. Good sequencing.
25-30: No action required – type spelling is correct
ThePromisifiedPendpalChildMethodstype is indeed exported with that exact spelling inpackages/penpal/src/child.tsand matches every import inview.tsx. You can safely ignore the original spelling concern.Likely an incorrect or invalid review comment.
apps/web/client/src/components/store/editor/frames/manager.ts (1)
89-97: Selection toggles when called; verify intended UXselect() toggles current state for each frame. If the intent is “select these frames,” you likely want to set selected = true (not toggle) after optional deselectAll().
If toggle is desired for multiselect only, adjust:
- for (const frame of frames) { - this.updateFrameSelection(frame.id, !this.isSelected(frame.id)); - } + for (const frame of frames) { + this.updateFrameSelection(frame.id, true); + }Or gate the toggle behavior behind multiselect=true.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
apps/web/client/src/server/api/routers/publish/helpers/helpers.ts (1)
42-53: Fix sandbox lookup to use the default branch’s sandboxIdThe helper currently still reads
project.sandboxId, which no longer exists and will break at compile time and runtime. Update the import to includebranchesand fetch the sandboxId from the project’s default branch instead.• File:
apps/web/client/src/server/api/routers/publish/helpers/helpers.ts(lines 42–53)
• Replace theproject.sandboxIdlookup with a query againstbranchesfiltered byprojectIdandisDefault-import { deployments, deploymentUpdateSchema, previewDomains, projectCustomDomains, projects, type Deployment } from '@onlook/db'; +import { deployments, deploymentUpdateSchema, previewDomains, projectCustomDomains, projects, branches, type Deployment } from '@onlook/db'; @@ -export async function getSandboxId(db: DrizzleDb, projectId: string): Promise<string> { - const project = await db.query.projects.findFirst({ - where: eq(projects.id, projectId), - }); - if (!project) { - throw new TRPCError({ - code: 'BAD_REQUEST', - message: 'Project not found', - }); - } - return project.sandboxId; -} +export async function getSandboxId(db: DrizzleDb, projectId: string): Promise<string> { + const defaultBranch = await db.query.branches.findFirst({ + where: and( + eq(branches.projectId, projectId), + eq(branches.isDefault, true) + ), + }); + if (!defaultBranch) { + throw new TRPCError({ + code: 'BAD_REQUEST', + message: 'Default branch not found for project', + }); + } + return defaultBranch.sandboxId; +}apps/web/client/src/server/api/routers/publish/deployment.ts (2)
11-11: Drop.tsextension in import path.ESM/CJS resolution and many bundlers will emit
.js, so importing./helpers/index.tscan break at runtime.-import { createDeployment, publish } from './helpers/index.ts'; +import { createDeployment, publish } from './helpers';
75-84: Fix unreachable CANCELLED check and clarify run preconditions.Filtering by status in the query makes the later status checks inconsistent (e.g., CANCELLED path is unreachable). Query by
idonly, then validate status.- const existingDeployment = await ctx.db.query.deployments.findFirst({ - where: and( - eq(deployments.id, deploymentId), - or( - eq(deployments.status, DeploymentStatus.IN_PROGRESS), - eq(deployments.status, DeploymentStatus.PENDING), - ), - ), - }); + const existingDeployment = await ctx.db.query.deployments.findFirst({ + where: eq(deployments.id, deploymentId), + }); @@ - if (existingDeployment.status === DeploymentStatus.IN_PROGRESS) { + if (existingDeployment.status === DeploymentStatus.IN_PROGRESS) { throw new TRPCError({ code: 'BAD_REQUEST', message: 'Deployment in progress', }); } - if (existingDeployment.status === DeploymentStatus.CANCELLED) { + if (existingDeployment.status === DeploymentStatus.CANCELLED) { throw new TRPCError({ code: 'BAD_REQUEST', message: 'Deployment cancelled', }); } + // Optional: restrict to pending only + // if (existingDeployment.status !== DeploymentStatus.PENDING) { + // throw new TRPCError({ code: 'BAD_REQUEST', message: 'Deployment not pending' }); + // }Also applies to: 90-101
apps/web/client/src/server/api/routers/project/project.ts (2)
68-75: Refactor allsandboxUrlusages to derive preview URLs from each project’s default branchProjects no longer expose
sandboxUrlon their root model, so every reference will fail once the migration lands. We need to fetch the default branch’ssandboxIdand build the preview URL everywhereproject.sandboxUrlis used.Key locations to update:
- apps/web/client/src/server/api/routers/project/project.ts
• Lines 68–75 (and later at lines 261, 268)- apps/web/client/src/server/api/routers/project/template.ts (around line 76)
- apps/web/client/src/app/_components/hero/start-blank.tsx (line 50)
- apps/web/client/src/components/store/create/manager.ts (line 80)
- apps/web/client/src/app/projects/_components/templates/template-modal.tsx (lines 101–102)
- apps/web/client/src/app/projects/import/local/_context/index.tsx (around line 159)
- apps/web/client/src/app/projects/import/github/_context/context.tsx (around line 237)
- DB seed scripts in packages/db/src/seed/db.ts (lines 57, 72, 84, 88)
- Any
createDefaultFrame(…, project.sandboxUrl, …)callsExample refactor in project router:
- if (!project.sandboxUrl) { - throw new Error('No sandbox URL found'); - } - - const app = new FirecrawlApp({ apiKey: env.FIRECRAWL_API_KEY }); - - const result = await app.scrapeUrl(project.sandboxUrl, { + // Derive the preview URL from the project’s default branch + const defaultBranch = await ctx.db.query.branches.findFirst({ + where: and( + eq(branches.projectId, project.id), + eq(branches.isDefault, true), + ), + }); + if (!defaultBranch) { + throw new Error('Default branch not found'); + } + const previewUrl = getBranchPreviewUrl(defaultBranch.sandboxId); + const app = new FirecrawlApp({ apiKey: env.FIRECRAWL_API_KEY }); + const result = await app.scrapeUrl(previewUrl, {Add (near the bottom of the file):
function getBranchPreviewUrl(sandboxId: string): string { // TODO: replace with your canonical preview URL builder return `https://codesandbox.io/p/devbox/${sandboxId}`; }And at the top:
import { frames } from '…'; +import { branches } from '…';Don’t forget to update all other instances (seeds, components, template router,
createDefaultFramecalls, etc.) so noproject.sandboxUrlremains.
261-274: Create a default branch and pass its ID (notsandboxUrl) intocreateDefaultFrameIn the
createmutation (apps/web/client/src/server/api/routers/project/project.ts, around lines 259–274), you’re currently callingcreateDefaultFrame(newCanvas.id, input.project.sandboxUrl, { … })but the real signature is
createDefaultFrame(canvasId: string, branchId: string, url: string, overrides?: Partial<DbFrame>) ``` ([]()) This means you’re treating `sandboxUrl` as `branchId` and passing the overrides object in place of `url`. You must: - Insert a default branch (e.g. “main”) and mark it as default. - Use its `id` for the second argument and its preview URL (or `sandboxId`) for the third. For example: ```diff @@ apps/web/client/src/server/api/routers/project/project.ts - // 4. Create the default frame + // 4. Create the default branch + const [defaultBranch] = await tx.insert(branches).values({ + projectId: newProject.id, + name: 'main', + description: 'Default main branch', + isDefault: true, + sandboxId: `sandbox-${newProject.id}`, + }).returning(); + if (!defaultBranch) { + throw new Error('Failed to create default branch'); + } + + const defaultUrl = getBranchPreviewUrl(defaultBranch.sandboxId); + // 5. Create the default frames on this branch const desktopFrame = createDefaultFrame( - newCanvas.id, - input.project.sandboxUrl, - { + newCanvas.id, + defaultBranch.id, + defaultUrl, + { x: '5', y: '0', width: '1536', height: '960', } ); await tx.insert(frames).values(desktopFrame); const mobileFrame = createDefaultFrame( - newCanvas.id, - input.project.sandboxUrl, - { + newCanvas.id, + defaultBranch.id, + defaultUrl, + { x: '1600', y: '0', width: '440', height: '956', } ); await tx.insert(frames).values(mobileFrame);Please update the mutation accordingly—without this fix, frame creation will break as soon as
sandboxUrlis removed from theprojectinput.
♻️ Duplicate comments (1)
packages/db/src/schema/project/project.ts (1)
12-27: Confirm migrations and downstream cleanup for sandbox field removal.Given
sandboxId/sandboxUrlwere removed here, verify a migration drops these columns and that seeds/DTOs/routers no longer reference them (there’s at least one usage in helpers today).#!/bin/bash # Check for lingering references rg -n "sandbox(Id|URL|Url)|sandbox_id|sandbox_url" -S # Sanity-check migrations mention dropping these columns fd -t f -e sql -e ts --search-path packages/db "migration|migrate|sandbox|project" | xargs -I{} sh -c 'echo "== {} =="; sed -n "1,160p" "{}"'
🧹 Nitpick comments (20)
apps/web/client/src/components/ui/settings-modal/site/page.tsx (2)
45-61: Don’t call createSecureUrl with undefined; conditionally include openGraph.urlPrevent passing
undefinedintocreateSecureUrland avoid serializingurl: undefinedinto the Open Graph payload.Apply:
- const url = createSecureUrl(baseUrl); + const url = baseUrl ? createSecureUrl(baseUrl) : undefined; const finalTitle = getFinalTitleMetadata(); const siteTitle = typeof finalTitle === 'string' ? finalTitle : finalTitle.absolute ?? finalTitle.default ?? ''; const updatedMetadata: PageMetadata = { ...metadata, title: finalTitle, description, - openGraph: { - ...metadata?.openGraph, - title: siteTitle, - description: description, - url: url, - siteName: siteTitle, - type: 'website', - }, + openGraph: { + ...metadata?.openGraph, + title: siteTitle, + description, + siteName: siteTitle, + type: 'website', + ...(url ? { url } : {}), + }, };
69-90: Avoid early return on image upload failure; notify the user and continue saving metadata without the imageThe early
returnsilently aborts the whole save flow, leaving the user without feedback beyond a console log. Prefer surfacing a toast error and proceeding withoutimages. Guard the assignment to ensureimagePathis defined.Apply:
- if (uploadedImage) { - let imagePath; - try { - await editorEngine.image.upload(uploadedImage, DefaultSettings.IMAGE_FOLDER); - imagePath = `/${uploadedImage.name}`; - } catch (error) { - console.log(error); - return; - } - updatedMetadata.openGraph = { - ...updatedMetadata.openGraph, - images: [ - { - url: imagePath, - width: 1200, - height: 630, - alt: siteTitle, - }, - ], - type: 'website', - }; - } + if (uploadedImage) { + let imagePath: string | undefined; + try { + await editorEngine.image.upload(uploadedImage, DefaultSettings.IMAGE_FOLDER); + imagePath = `/${uploadedImage.name}`; + } catch (error) { + console.error('Failed to upload image', error); + toast.error('Failed to upload the image. Saving metadata without an image.'); + } + if (imagePath) { + updatedMetadata.openGraph = { + ...updatedMetadata.openGraph, + images: [ + { + url: imagePath, + width: 1200, + height: 630, + alt: siteTitle, + }, + ], + type: 'website', + }; + } + }If you prefer to hard-fail the entire save when the image upload fails, keep the early return but add a toast error for clear UX.
packages/db/src/schema/domain/deployment.ts (1)
49-51: Good: requiringidon updates; consider strict schema and avoid settingiddownstream.
- Requiring a UUID
idtightens update contracts—nice.- To fail-fast on unexpected fields, consider making this schema strict.
- Ensure update callers don’t pass
idinto.set(...)(see router fix inapps/web/.../conversation.ts).Suggested tweak (keeps behavior; rejects unknown keys):
-export const deploymentUpdateSchema = createUpdateSchema(deployments, { - id: z.string().uuid(), -}); +export const deploymentUpdateSchema = createUpdateSchema(deployments, { + id: z.string().uuid(), +}).strict();packages/db/src/schema/chat/conversation.ts (2)
23-25: Requireidon updates: add.strict()and don’t propagateidto.set(...)in callers.
- +1 on making
idmandatory. To prevent silent acceptance of unintended fields, consider.strict()on the schema.- In callers, avoid
.set(input)which would attempt to update the primary key; instead.omit({ id: true })or destructure.Minimal schema hardening:
-export const conversationUpdateSchema = createUpdateSchema(conversations, { - id: z.string().uuid(), -}); +export const conversationUpdateSchema = createUpdateSchema(conversations, { + id: z.string().uuid(), +}).strict();
7-7: Typo in constant name (CONVERSATION_MESSAGe_RELATION_NAME).The trailing lowercase
einMESSAGelooks accidental. If you decide to correct it, rename it in both this file and./messageto avoid drift.I can generate a repo-wide rename script to update both sides atomically.
apps/web/client/src/server/api/routers/chat/conversation.ts (2)
17-18: Tighten input validation to UUIDs for identifiers.Use UUID validation for all ID inputs for consistency with DB schemas.
- .input(z.object({ projectId: z.string() })) + .input(z.object({ projectId: z.string().uuid() }))- .input(z.object({ conversationId: z.string() })) + .input(z.object({ conversationId: z.string().uuid() }))- .input(z.object({ - conversationId: z.string() - })) + .input(z.object({ + conversationId: z.string().uuid() + }))- .input(z.object({ - conversationId: z.string(), - content: z.string(), - })) + .input(z.object({ + conversationId: z.string().uuid(), + content: z.string(), + }))Also applies to: 26-27, 59-61, 66-69
76-101: Name generation: consider bounding by words as well as length.You cap at 50 chars, but prompt requests 2–4 words. If titles sometimes exceed 4 words, enforce a word cap post-generation to keep UX consistent. Optional, since the prompt already nudges behavior.
packages/db/src/schema/project/branch.ts (2)
8-33: Schema looks solid; partial unique index correctly enforces “at most one default” per project.
- Table definition and relations are consistent with the new frames FK and project FK.
- RLS enabled—good.
Two optional enhancements:
- Add an index on
projectIdto speed up listing branches by project.- Optionally ensure branch names are unique per project.
Proposed indexes:
-}, - (table) => [ - uniqueIndex('idx_project_default_branch').on(table.projectId).where(eq(table.isDefault, true)), - ] +}, + (table) => [ + uniqueIndex('idx_project_default_branch') + .on(table.projectId) + .where(eq(table.isDefault, true)), + // Speeds up lookups by project + // Note: use `index` from pg-core once available in your version, or create via migration if needed. + // index('idx_branches_project_id').on(table.projectId), + // Prevent duplicate branch names within a project + // uniqueIndex('u_project_branch_name').on(table.projectId, table.name), + ] ).enableRLS();If you need “at least one default branch per project,” that requires a trigger/check outside simple indexes. I can draft one if desired.
34-37: Update schema: mirror other modules with.strict()and keepidout of.set(...)in services.Same guidance as other schemas—strict object and ensure services don’t attempt to update PKs.
apps/web/client/src/server/api/root.ts (1)
20-20: Keep router imports consistent — prefer importingbranchRoutervia./routersbarrelYou import most routers from
./routersbutbranchRouterfrom a deep path. For consistency and maintainability, re-exportbranchRouterfrom./routers/index.tsand import it alongside others.Apply once
./routersre-exportsbranchRouter:-import { branchRouter } from './routers/project/branch'; +// Ensure ./routers/index.ts exports branchRouter +// then pull it in with the rest: +// (no separate deep import needed)And update the top import block:
import { chatRouter, + branchRouter, codeRouter, domainRouter, feedbackRouter, frameRouter, githubRouter, invitationRouter, memberRouter, projectRouter, publishRouter, sandboxRouter, settingsRouter, subscriptionRouter, usageRouter, userCanvasRouter, userRouter, } from './routers';packages/db/src/mappers/project/branch.ts (1)
24-38: Separate insert/update mappers to avoid over-posting timestampsPassing
createdAt/updatedAtthrough a singletoDbBranchrisks accidentally overwritingcreatedAtor leavingupdatedAtstale. Prefer dedicated insert/update shapes.You can keep
toDbBranchfor compatibility and add granular helpers:export const toDbBranch = (branch: Branch): DbBranch => { return { id: branch.id, name: branch.name, projectId: branch.projectId, description: branch.description, createdAt: branch.createdAt, updatedAt: branch.updatedAt, isDefault: branch.isDefault, gitBranch: branch.git?.branch ?? null, gitCommitSha: branch.git?.commitSha ?? null, gitRepoUrl: branch.git?.repoUrl ?? null, sandboxId: branch.sandbox.id, }; }; + +// More precise helpers (use in routers/services): +export const toDbBranchInsert = (branch: Branch): Omit<DbBranch, 'updatedAt' | 'createdAt'> & { createdAt?: Date; updatedAt?: Date } => ({ + id: branch.id, + name: branch.name, + projectId: branch.projectId, + description: branch.description, + isDefault: branch.isDefault, + gitBranch: branch.git?.branch ?? null, + gitCommitSha: branch.git?.commitSha ?? null, + gitRepoUrl: branch.git?.repoUrl ?? null, + sandboxId: branch.sandbox.id, +}); + +export const toDbBranchUpdate = (branch: Partial<Branch> & { id: string }): Partial<DbBranch> => ({ + id: branch.id, + name: branch.name, + projectId: branch.projectId, + description: branch.description ?? null, + isDefault: branch.isDefault, + gitBranch: branch.git?.branch ?? null, + gitCommitSha: branch.git?.commitSha ?? null, + gitRepoUrl: branch.git?.repoUrl ?? null, + sandboxId: branch.sandbox?.id, + // let service set updatedAt = new Date() just-in-time +});apps/web/client/src/server/api/routers/project/branch.ts (4)
10-13: ValidateprojectIdas UUID.Stronger input validation prevents noisy 500s downstream.
- z.object({ - projectId: z.string(), - }), + z.object({ + projectId: z.string().uuid(), + }),
15-17: Ambiguous selection: clarify default vs. any branch.
findFirstwithout ordering/criteria is nondeterministic when multiple branches exist. If the intent is the default branch, filter byisDefault. If the intent is “list branches”, return an array.- const dbBranch = await ctx.db.query.branches.findFirst({ - where: eq(branches.projectId, input.projectId), - }); + const dbBranch = await ctx.db.query.branches.findFirst({ + where: eq(branches.projectId, input.projectId), + // If default desired: + // where: and(eq(branches.projectId, input.projectId), eq(branches.isDefault, true)), + });
27-36: Return the created branch rather than a boolean; fix log message.Returning the created entity simplifies clients and aligns with other routers. Also, the log mentions “frame” instead of “branch”.
- .mutation(async ({ ctx, input }) => { + .mutation(async ({ ctx, input }) => { try { - await ctx.db.insert(branches).values(input); - return true; + const [inserted] = await ctx.db.insert(branches).values(input).returning(); + return fromDbBranch(inserted); } catch (error) { - console.error('Error creating frame', error); - return false; + console.error('Error creating branch', error); + return null; } }),
51-65: Fix copy-paste log labels.These errors mention “frame”; rename to “branch” for clarity.
- console.error('Error deleting frame', error); + console.error('Error deleting branch', error);apps/web/client/src/components/store/editor/engine.ts (1)
79-104: Clear order: consider clearing branches before per-branch dependents to avoid accidental sandbox access during teardownA few manager clear() implementations can, now or in the future, call back into
editorEngine.sandbox. Clearingbranchesearlier reduces the chance of accidentally touching a disposed provider. Not a blocker, but safer sequencing would putthis.branches.clear()before managers that might indirectly use the sandbox.this.code.clear(); this.ide.clear(); this.error.clear(); - this.branches.clear(); this.frameEvent.clear(); this.screenshot.clear(); + // Clear branches after UI/state but before anything that might call into the sandbox again + this.branches.clear();packages/db/src/schema/canvas/frame.ts (2)
13-23: Branch association is correct; add indexes for common access patternsBranch FK looks good. Frames are typically fetched by canvas and often filtered by branch. Add indexes to keep queries snappy.
-export const frames = pgTable("frames", { +export const frames = pgTable("frames", { @@ height: numeric("height").notNull(), -}).enableRLS(); +}, (t) => ({ + idx_canvas: index('idx_frames_canvas').on(t.canvasId), + idx_canvas_branch: index('idx_frames_canvas_branch').on(t.canvasId, t.branchId), +})).enableRLS();
18-23: Constrain numeric precision/scale for coordinates and dimensionsUnless you intend arbitrary precision, set precision/scale to keep storage and comparisons predictable (e.g., precision 12, scale 3). This is optional and schema-wide.
- x: numeric("x").notNull(), - y: numeric("y").notNull(), - width: numeric("width").notNull(), - height: numeric("height").notNull(), + x: numeric("x", { precision: 12, scale: 3 }).notNull(), + y: numeric("y", { precision: 12, scale: 3 }).notNull(), + width: numeric("width", { precision: 12, scale: 3 }).notNull(), + height: numeric("height", { precision: 12, scale: 3 }).notNull(),apps/web/client/src/components/store/editor/branch/manager.ts (2)
20-31: Lazy sandbox creation is fine; consider a non-throwing accessor or default selection pathThrowing when no branch is selected is strict and can produce noisy logs in code paths that probe availability. Two alternatives:
- Return
SandboxManager | nulland let callers branch.- Ensure default branch selection during engine init (preferred; see engine comment).
126-128: listBranches returns an empty list — wire to server APIAs a follow-up, connect this to your TRPC branch router so UI can reflect actual branches. Not a blocker for this PR’s core schema changes.
Happy to wire this to
project.branch.listif you share the intended API shape.
📜 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 (25)
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx(3 hunks)apps/web/client/src/components/store/editor/branch/manager.ts(1 hunks)apps/web/client/src/components/store/editor/chat/conversation.ts(3 hunks)apps/web/client/src/components/store/editor/engine.ts(4 hunks)apps/web/client/src/components/ui/settings-modal/project/index.tsx(3 hunks)apps/web/client/src/components/ui/settings-modal/site/page.tsx(1 hunks)apps/web/client/src/server/api/root.ts(2 hunks)apps/web/client/src/server/api/routers/chat/conversation.ts(4 hunks)apps/web/client/src/server/api/routers/project/branch.ts(1 hunks)apps/web/client/src/server/api/routers/project/frame.ts(3 hunks)apps/web/client/src/server/api/routers/project/project.ts(6 hunks)apps/web/client/src/server/api/routers/publish/deployment.ts(3 hunks)apps/web/client/src/server/api/routers/publish/helpers/helpers.ts(1 hunks)packages/db/src/mappers/project/branch.ts(1 hunks)packages/db/src/mappers/project/canvas.ts(1 hunks)packages/db/src/mappers/project/index.ts(1 hunks)packages/db/src/schema/canvas/frame.ts(2 hunks)packages/db/src/schema/chat/conversation.ts(2 hunks)packages/db/src/schema/domain/deployment.ts(2 hunks)packages/db/src/schema/project/branch.ts(1 hunks)packages/db/src/schema/project/project.ts(2 hunks)packages/db/src/schema/user/settings.ts(1 hunks)packages/models/src/project/branch.ts(1 hunks)packages/models/src/project/canvas.ts(1 hunks)packages/models/src/project/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/db/src/schema/user/settings.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web/client/src/components/store/editor/chat/conversation.ts
- apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx
- apps/web/client/src/server/api/routers/project/frame.ts
- apps/web/client/src/components/ui/settings-modal/project/index.tsx
🧰 Additional context used
🧬 Code graph analysis (13)
packages/db/src/mappers/project/branch.ts (1)
packages/models/src/project/branch.ts (1)
Branch(1-17)
apps/web/client/src/server/api/root.ts (1)
apps/web/client/src/server/api/routers/project/branch.ts (1)
branchRouter(7-66)
apps/web/client/src/server/api/routers/publish/helpers/helpers.ts (2)
packages/db/src/client.ts (2)
db(16-16)DrizzleDb(17-17)packages/db/src/schema/domain/deployment.ts (3)
deploymentUpdateSchema(49-51)Deployment(53-53)deployments(12-34)
apps/web/client/src/server/api/routers/project/branch.ts (3)
apps/web/client/src/server/api/trpc.ts (1)
protectedProcedure(130-149)packages/db/src/schema/project/branch.ts (3)
branches(8-32)branchInsertSchema(34-34)branchUpdateSchema(35-37)packages/db/src/mappers/project/branch.ts (1)
fromDbBranch(4-22)
packages/models/src/project/branch.ts (1)
packages/db/src/schema/project/branch.ts (1)
Branch(47-47)
apps/web/client/src/components/store/editor/branch/manager.ts (3)
apps/web/client/src/components/store/editor/engine.ts (1)
EditorEngine(32-121)apps/web/client/src/components/store/editor/sandbox/index.ts (1)
SandboxManager(35-800)packages/models/src/project/branch.ts (1)
Branch(1-17)
apps/web/client/src/server/api/routers/publish/deployment.ts (3)
apps/web/client/src/server/api/trpc.ts (1)
protectedProcedure(130-149)packages/db/src/schema/domain/deployment.ts (1)
deploymentUpdateSchema(49-51)apps/web/client/src/server/api/routers/publish/helpers/helpers.ts (1)
updateDeployment(55-72)
packages/db/src/schema/project/branch.ts (3)
packages/db/src/schema/project/project.ts (1)
projects(12-27)packages/db/src/schema/canvas/frame.ts (1)
frames(8-23)packages/models/src/project/branch.ts (1)
Branch(1-17)
apps/web/client/src/server/api/routers/chat/conversation.ts (2)
packages/db/src/mappers/chat/conversation.ts (1)
fromDbConversation(4-12)packages/db/src/schema/chat/conversation.ts (2)
conversationUpdateSchema(23-25)conversations(11-20)
apps/web/client/src/server/api/routers/project/project.ts (5)
packages/db/src/mappers/project/project.ts (2)
toDbPreviewImg(57-87)fromDbProject(4-18)packages/constants/src/storage.ts (1)
STORAGE_BUCKETS(1-4)packages/db/src/mappers/project/canvas.ts (1)
fromDbCanvas(4-14)packages/db/src/mappers/project/frame.ts (1)
fromDbFrame(4-19)packages/db/src/schema/project/project.ts (2)
projects(12-27)projectUpdateSchema(30-32)
packages/db/src/schema/canvas/frame.ts (2)
packages/db/src/schema/canvas/canvas.ts (1)
canvases(8-13)packages/db/src/schema/project/branch.ts (1)
branches(8-32)
apps/web/client/src/components/store/editor/engine.ts (4)
apps/web/client/src/components/store/editor/preload/index.ts (1)
PreloadScriptManager(7-88)apps/web/client/src/components/store/editor/screenshot/index.tsx (1)
ScreenshotManager(7-57)apps/web/client/src/components/store/editor/branch/manager.ts (1)
BranchManager(6-137)apps/web/client/src/components/store/editor/sandbox/index.ts (1)
SandboxManager(35-800)
packages/db/src/mappers/project/canvas.ts (1)
packages/models/src/project/canvas.ts (1)
Canvas(3-8)
🔇 Additional comments (19)
apps/web/client/src/components/ui/settings-modal/site/page.tsx (2)
122-122: Confirm MetadataForm.projectUrl accepts undefined and/or expects a fully-qualified URLWith sandbox fallback removed,
projectUrlmay now beundefined. Verify the prop typing and any assumptions the form makes (e.g., protocol present).If the form expects a sanitized absolute URL, consider passing the secured value:
- projectUrl={baseUrl} + projectUrl={baseUrl ? createSecureUrl(baseUrl) : undefined}Otherwise, leave as-is and ensure the form handles
undefinedgracefully (e.g., show guidance to configure a domain).
16-16: No guard needed forundefinedURLs—function and prop types are already safeOn inspection,
createSecureUrlis defined to acceptstring | undefined | nulland will return an empty string for any falsy or invalid input—includingundefined—so it never produces"https://undefined"at runtime. Likewise, theMetadataFormProps.projectUrlprop is declared as optional (projectUrl?: string), so passingundefinedis explicitly allowed and renders as an empty preview without error.You can safely drop the sandbox fallback without adding an extra guard:
•
createSecureUrl(undefined)⇒''
•projectUrl={undefined}(valid byprojectUrl?: string)No changes required here—please disregard the previous guard suggestion.
packages/db/src/schema/domain/deployment.ts (1)
5-6: Import path change looks correct; confirm no circular deps via domain index.Switching to
../projectmatches the location ofprojectsunderschema/project. Please double-check there’s no circular import via anyindex.tsre-exports that also pulldomain/*back intoproject/*.If helpful, I can provide a quick script to scan for cycles across
packages/db/src/schema/**.packages/db/src/schema/chat/conversation.ts (1)
5-5: Zod import addition is appropriate.This enables explicit UUID validation on updates; no concerns here.
packages/models/src/project/branch.ts (1)
1-17: Model shape aligns with DB schema; confirm Date serialization across API boundary.
- Fields map cleanly to
schema/project/branch.ts(includingsandboxId -> sandbox.idandgit* -> git.*). LGTM.- Since
createdAt/updatedAtareDate, ensure your API layer (tRPC) uses a transformer (e.g., superjson) so the client receivesDateobjects rather than ISO strings.Would you like a quick check script to confirm the tRPC transformer is configured and that dates round-trip as
Dateon the client?packages/models/src/project/index.ts (1)
1-1: ✅ Branch export is safe—bump minor versionAll checks confirm that re-exporting
./branchintroduces no circular dependencies and correctly exposes theBranchtype to downstream consumers. Treat this as a minor semver bump.• No circular imports found between
packages/models/src/project/index.tsandpackages/models/src/project/branch.ts; thebranch.tsfile has no imports from the models barrel and only defines theBranchinterface.
• Downstream packages importingBranchfrom@onlook/modelswill now resolve correctly via the public barrel.
• This change is backwards-compatible (non-breaking), so increment the minor version.No further action required.
packages/models/src/project/canvas.ts (1)
7-7: Audit Canvas constructors for the new requireduserIdI’ve tightened the Canvas contract by making
userIdmandatory—this will break any code paths that omit it. Here’s what’s been confirmed so far:
Mappers
fromDbCanvas(packages/db/src/mappers/project/canvas.ts:4–13) correctly mapsuserIdfrom the DB record.toDbCanvasis defined (packages/db/src/mappers/project/canvas.ts:16–18) but needs review (see below).Factories & Defaults
createDefaultUserCanvas(packages/db/src/defaults/user-canvas.ts:4–6) always suppliesuserId.- Seed data (packages/db/src/seed/db.ts:83–90) uses
createDefaultUserCanvas⇒ covers project seeds.Server-side Routers
- User‐canvas routes (apps/web/client/src/server/api/routers/user/user-canvas.ts:5–7 & :57–60) use
fromDbCanvas/createDefaultUserCanvas.- Project routers (…/project/project.ts and …/project/template.ts) likewise call
createDefaultUserCanvas.Please verify the remaining spots to avoid any runtime
undefined:
toDbCanvasimplementation (packages/db/src/mappers/project/canvas.ts:16–…): ensure it includesuserIdin its returned object or remove it if unused.- Any manual object literals (e.g. in tests or fixtures) constructing
Canvas—they must setuserId.- Migration scripts or other builders that may instantiate
Canvasdirectly.Optional refactor: introduce a branded
UserIdtype to prevent ID mix-ups:// models/src/types/id.ts export type Brand<T, B extends string> = T & { readonly __brand: B }; export type UserId = Brand<string, 'UserId'>; export interface Canvas { /*…*/ userId: UserId; }apps/web/client/src/server/api/root.ts (1)
32-32: The newbranchroute has been correctly wired up on the client and the server:
- Client types regenerated:
api.branch.get.useQueryis already in use (e.g. inuse-start-project.tsx:30), andAppRouter = typeof appRouterremains in sync in both server and client roots.branchRouteris defined inapps/web/server/src/router/branch.ts, and all operations useprotectedProcedure, ensuring that only authenticated users can call any branch endpoints.No further changes are needed here.
packages/db/src/mappers/project/index.ts (1)
1-6: Aggregator of project mappers looks goodRe-export set aligns with the new mapper modules; keeps import sites clean.
packages/db/src/schema/project/project.ts (2)
30-32: Requiringidin update schema is correct and consistent with deployment updates.This tightens API contracts and simplifies router inputs. Ensure upstream callers have been adjusted.
4-7: Import reorg aligns with new schema layout.Pulling in
z, canvas/chat/domain pieces here looks good and reflects the consolidated exports.packages/db/src/mappers/project/canvas.ts (2)
4-14: Rename to directional mappers looks good; mapping reads correctly.
fromDbCanvascorrectly mapscanvasId→id, numeric coercions, and includesuserId.
16-24: Manual verification required: integratetoDbCanvasfor write operationsI ran searches across the codebase and did not find any call sites for the new
toDbCanvasmapper. Please manually confirm that:
- All insert/update operations that persist canvases now call
toDbCanvas(which returns aDbUserCanvasincludinguserId) instead of any outdated helpers.- Any legacy uses of
fromCanvas/toCanvashave been removed or replaced.- Database API routes and service layers (e.g., in
apps/webunder project/user canvas routers) invoketoDbCanvaswhen saving canvas records.Nit: If incoming numbers can be non-finite, guard against persisting
"NaN":const toStr = (n: number) => Number.isFinite(n) ? n.toString() : '0';apps/web/client/src/server/api/routers/publish/deployment.ts (2)
107-118: Ensure helper tolerates partial payloads (notype).These calls intentionally omit
type. With the current helper, coercingtype as DeploymentTypecan set it toundefined. After the helper fix, this path is safe.Run tests for run/cancel flows after applying the helper change.
Also applies to: 126-131
28-30: Ensure helper filters out undefined payload fieldsThe update route and schema alignment look good, but it relies on
updateDeploymentomittingundefinedproperties rather than setting them tonull. Please verify that in your helper implementation:
- In the file where
updateDeploymentis defined (e.g. underapps/web/client/src/server/api/services/deployment.ts), confirm you strip out any keys whose values areundefinedbefore constructing the ORM updatedataobject.- If not already in place, apply a utility (such as Lodash’s
omitBy(input, isUndefined)or a manual filter) so that partial payloads don’t inadvertently null out existing columns.packages/db/src/schema/canvas/frame.ts (3)
26-28: Update schema passes id override — good
createUpdateSchema(frames, { id: z.string().uuid() })is the right pattern to require id in updates.
33-42: Relations look correctCanvas and branch relations are properly wired.
2-7: Import Path forbranchesVerifiedConfirmed that
packages/db/src/schema/project/index.tsre-exports everything frombranch.ts(includingbranches) viaexport * from './branch';and
branch.tsindeed declaresexport const branches = …. Therefore,import { branches } from "../project";will resolve correctly and no changes are needed.
apps/web/client/src/server/api/routers/project/project.ts (1)
181-182: Mapping + sort looks goodUsing
fromDbProjectthen sorting bymetadata.updatedAtis consistent with the mapper layer.
| readonly branches: BranchManager = new BranchManager(this); | ||
| // Sandbox getter - returns branch-specific sandbox | ||
| get sandbox(): SandboxManager { | ||
| return this.branches.getCurrentSandbox(); | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Sandbox getter throws before any branch is selected — add explicit engine initialization to prevent runtime errors
Right now, get sandbox() throws if no branch has been selected. Several managers (e.g., PreloadScriptManager via this.editorEngine.sandbox) may access this early and end up in try/catch fallbacks or log errors. Provide an explicit initialization path on the engine that selects a branch and starts its sandbox, then require callers to await it.
Proposed addition (engine API) to be called immediately after constructing EditorEngine:
export class EditorEngine {
@@
constructor(projectId: string, posthog: PostHog) {
this.projectId = projectId;
this.posthog = posthog;
makeAutoObservable(this);
}
+
+ /**
+ * Initialize the engine by selecting a branch and starting its sandbox.
+ * Must be awaited by callers before interacting with per-branch managers.
+ */
+ async init(branchId?: string) {
+ const id = branchId ?? (await this.branches.getDefaultBranch()).id;
+ await this.branches.switchToBranch(id);
+ await this.branches.startCurrentBranchSandbox();
+ }Verification script to spot potential unsafe early access patterns:
🏁 Script executed:
#!/bin/bash
# Find direct sandbox access that may happen before init/branch selection
rg -n -C3 --type=ts --type=tsx '\beditorEngine\.sandbox\b|this\.editorEngine\.sandbox\b' apps/web | sed -n '1,200p'Length of output: 143
🏁 Script executed:
#!/usr/bin/env bash
# Find direct sandbox access patterns in TypeScript files
grep -R -n -C3 'editorEngine\.sandbox' apps/web --include '*.ts' --include '*.tsx'
grep -R -n -C3 'this\.editorEngine\.sandbox' apps/web --include '*.ts' --include '*.tsx'Length of output: 165373
Ensure EditorEngine is initialized before any sandbox access
We verified that the sandbox getter in src/components/store/editor/engine.ts calls branches.getCurrentSandbox() without any default branch selected. Any early use of editorEngine.sandbox (over 100 occurrences across the codebase—from terminal and canvas components to image, page, chat, theme/font, and dev-tool managers) will throw at runtime.
To prevent these errors, introduce an explicit initialization step on EditorEngine and require callers to await it before using branch-specific managers:
- In
src/components/store/editor/engine.ts, add:export class EditorEngine { constructor(projectId: string, posthog: PostHog) { this.projectId = projectId; this.posthog = posthog; makeAutoObservable(this); }
-
/** -
* Selects (or takes the provided) branch, switches to it, -
* and starts its sandbox session. Must be awaited -
* before any `editorEngine.sandbox` access. -
*/ -
async init(branchId?: string): Promise<void> { -
const id = branchId ?? (await this.branches.getDefaultBranch()).id; -
await this.branches.switchToBranch(id); -
await this.branches.startCurrentBranchSandbox(); -
}
- Update the engine’s consumer (e.g.,
useEditorEngineoruse-start-project) to call and awaitengine.init()immediately after instantiation. - Add a clear error or guard in the
get sandbox()accessor to surface “Engine not initialized” if called too early. - Audit and adjust any existing code paths that rely on
editorEngine.sandboxto ensure they occur after initialization.
Representative places requiring this guard or updated to await initialization:
apps/web/client/src/app/project/[id]/_components/bottom-bar/terminal-area.tsxapps/web/client/src/app/project/[id]/_components/canvas/frame/page-selector.tsxapps/web/client/src/app/project/[id]/_components/left-panel/image-tab/...apps/web/client/src/components/store/editor/preload/index.tsapps/web/client/src/components/store/editor/ast/index.tsapps/web/client/src/components/store/editor/chat/context.tsapps/web/client/src/components/store/editor/dev/index.tsapps/web/client/src/components/store/editor/theme/index.tsapps/web/client/src/components/store/editor/font/font-config-manager.tsapps/web/client/src/components/tools/handlers/cli.ts
📝 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.
| readonly branches: BranchManager = new BranchManager(this); | |
| // Sandbox getter - returns branch-specific sandbox | |
| get sandbox(): SandboxManager { | |
| return this.branches.getCurrentSandbox(); | |
| } | |
| export class EditorEngine { | |
| constructor(projectId: string, posthog: PostHog) { | |
| this.projectId = projectId; | |
| this.posthog = posthog; | |
| makeAutoObservable(this); | |
| } | |
| /** | |
| * Selects (or takes the provided) branch, switches to it, | |
| * and starts its sandbox session. Must be awaited | |
| * before any `editorEngine.sandbox` access. | |
| */ | |
| async init(branchId?: string): Promise<void> { | |
| const id = branchId ?? (await this.branches.getDefaultBranch()).id; | |
| await this.branches.switchToBranch(id); | |
| await this.branches.startCurrentBranchSandbox(); | |
| } | |
| readonly branches: BranchManager = new BranchManager(this); | |
| // Sandbox getter - returns branch-specific sandbox | |
| get sandbox(): SandboxManager { | |
| return this.branches.getCurrentSandbox(); | |
| } | |
| } |
🤖 Prompt for AI Agents
In apps/web/client/src/components/store/editor/engine.ts around lines 67-71, the
sandbox getter calls branches.getCurrentSandbox() before the engine is
guaranteed to be initialized which causes runtime throws; add an async init():
Promise<void> on EditorEngine that selects/creates a default branch and sets an
"initialized" flag, update the sandbox getter to throw a clear "Engine not
initialized" Error if initialized is false, and then update engine consumers
(e.g., useEditorEngine or use-start-project and the listed files like
terminal-area.tsx, page-selector.tsx, preload/index.ts, ast/index.ts,
chat/context.ts, dev/index.ts, theme/index.ts, font-config-manager.ts, cli
handler, etc.) to call and await engine.init() immediately after instantiation
before accessing editorEngine.sandbox or other branch-specific managers; audit
and fix any early access paths to ensure they await init().
| .input(conversationUpdateSchema) | ||
| .mutation(async ({ ctx, input }) => { | ||
| const [conversation] = await ctx.db.update({ | ||
| ...conversations, | ||
| updatedAt: new Date(), | ||
| }).set(input.conversation) | ||
| .where(eq(conversations.id, input.conversationId)).returning(); | ||
| }).set(input) | ||
| .where(eq(conversations.id, input.id)).returning(); | ||
| if (!conversation) { |
There was a problem hiding this comment.
Bug: invalid ctx.db.update target and accidental primary key updates.
ctx.db.update({...conversations, updatedAt: new Date()})is incorrect; the update target must be the table object, not a spread object.- Passing
.set(input)will attempt to updateid, which is a PK. This can error or cause unintended writes.
Fix:
- update: protectedProcedure
- .input(conversationUpdateSchema)
- .mutation(async ({ ctx, input }) => {
- const [conversation] = await ctx.db.update({
- ...conversations,
- updatedAt: new Date(),
- }).set(input)
- .where(eq(conversations.id, input.id)).returning();
+ update: protectedProcedure
+ .input(conversationUpdateSchema)
+ .mutation(async ({ ctx, input }) => {
+ const { id, ...updates } = input;
+ const [conversation] = await ctx.db
+ .update(conversations)
+ .set({ ...updates, updatedAt: new Date() })
+ .where(eq(conversations.id, id))
+ .returning();
if (!conversation) {
throw new Error('Conversation not updated');
}
return fromDbConversation(conversation);
}),📝 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.
| .input(conversationUpdateSchema) | |
| .mutation(async ({ ctx, input }) => { | |
| const [conversation] = await ctx.db.update({ | |
| ...conversations, | |
| updatedAt: new Date(), | |
| }).set(input.conversation) | |
| .where(eq(conversations.id, input.conversationId)).returning(); | |
| }).set(input) | |
| .where(eq(conversations.id, input.id)).returning(); | |
| if (!conversation) { | |
| update: protectedProcedure | |
| .input(conversationUpdateSchema) | |
| .mutation(async ({ ctx, input }) => { | |
| const { id, ...updates } = input; | |
| const [conversation] = await ctx.db | |
| .update(conversations) | |
| .set({ ...updates, updatedAt: new Date() }) | |
| .where(eq(conversations.id, id)) | |
| .returning(); | |
| if (!conversation) { | |
| throw new Error('Conversation not updated'); | |
| } | |
| return fromDbConversation(conversation); | |
| }), |
🤖 Prompt for AI Agents
In apps/web/client/src/server/api/routers/chat/conversation.ts around lines 46
to 53, the update call is using a spread object as the target and passing the
whole input to .set which can attempt to update the primary key; change the
update target to the table object (conversations), exclude id from the fields
passed to .set (e.g. destructure input to separate id), include updatedAt: new
Date() in the set payload, and use the extracted id in the where clause so only
non-PK fields are updated.
| update: protectedProcedure.input(branchUpdateSchema).mutation(async ({ ctx, input }) => { | ||
| try { | ||
| await ctx.db | ||
| .update(branches) | ||
| .set(input) | ||
| .where( | ||
| eq(branches.id, input.id) | ||
| ); | ||
| return true; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Omit id from SET and update updatedAt.
Passing the whole input to .set() tries to update the PK; also keep timestamps fresh.
- update: protectedProcedure.input(branchUpdateSchema).mutation(async ({ ctx, input }) => {
+ update: protectedProcedure.input(branchUpdateSchema).mutation(async ({ ctx, input }) => {
try {
- await ctx.db
- .update(branches)
- .set(input)
- .where(
- eq(branches.id, input.id)
- );
+ const { id, ...rest } = input;
+ await ctx.db
+ .update(branches)
+ .set({ ...rest, updatedAt: new Date() })
+ .where(eq(branches.id, id));
return true;
} catch (error) {
- console.error('Error updating frame', error);
+ console.error('Error updating branch', error);
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.
| update: protectedProcedure.input(branchUpdateSchema).mutation(async ({ ctx, input }) => { | |
| try { | |
| await ctx.db | |
| .update(branches) | |
| .set(input) | |
| .where( | |
| eq(branches.id, input.id) | |
| ); | |
| return true; | |
| update: protectedProcedure.input(branchUpdateSchema).mutation(async ({ ctx, input }) => { | |
| try { | |
| const { id, ...rest } = input; | |
| await ctx.db | |
| .update(branches) | |
| .set({ ...rest, updatedAt: new Date() }) | |
| .where(eq(branches.id, id)); | |
| return true; | |
| } catch (error) { | |
| console.error('Error updating branch', error); | |
| return false; | |
| } | |
| }), |
🤖 Prompt for AI Agents
In apps/web/client/src/server/api/routers/project/branch.ts around lines 37 to
45, the mutation currently passes the whole input to .set(), which attempts to
update the primary key; change it to exclude input.id from the SET payload and
explicitly set updatedAt to the current timestamp. Extract id for the WHERE
clause (e.g., const { id, ...rest } = input), use .where(eq(branches.id, id)),
and call .set({ ...rest, updatedAt: new Date() }) so the PK is not modified and
the timestamp stays fresh.
| update: protectedProcedure.input(projectUpdateSchema).mutation(async ({ ctx, input }) => { | ||
| const [updatedProject] = await ctx.db.update(projects).set({ | ||
| ...input, | ||
| updatedAt: new Date(), | ||
| }).where(eq(projects.id, input.id)); | ||
| }).where( | ||
| eq(projects.id, input.id) | ||
| ).returning(); | ||
| if (!updatedProject) { | ||
| throw new Error('Project not found'); | ||
| } | ||
| return fromDbProject(updatedProject); | ||
| }), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Do not set primary key in updates; exclude id from the SET clause
Passing the entire input into .set({ ...input }) attempts to update projects.id. It’s unnecessary and can cause issues. Destructure id and set the rest.
- update: protectedProcedure.input(projectUpdateSchema).mutation(async ({ ctx, input }) => {
- const [updatedProject] = await ctx.db.update(projects).set({
- ...input,
- updatedAt: new Date(),
- }).where(
- eq(projects.id, input.id)
- ).returning();
+ update: protectedProcedure.input(projectUpdateSchema).mutation(async ({ ctx, input }) => {
+ const { id, ...patch } = input;
+ const [updatedProject] = await ctx.db.update(projects).set({
+ ...patch,
+ updatedAt: new Date(),
+ }).where(eq(projects.id, id)).returning();
if (!updatedProject) {
throw new Error('Project not found');
}
return fromDbProject(updatedProject);
}),📝 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.
| update: protectedProcedure.input(projectUpdateSchema).mutation(async ({ ctx, input }) => { | |
| const [updatedProject] = await ctx.db.update(projects).set({ | |
| ...input, | |
| updatedAt: new Date(), | |
| }).where(eq(projects.id, input.id)); | |
| }).where( | |
| eq(projects.id, input.id) | |
| ).returning(); | |
| if (!updatedProject) { | |
| throw new Error('Project not found'); | |
| } | |
| return fromDbProject(updatedProject); | |
| }), | |
| update: protectedProcedure.input(projectUpdateSchema).mutation(async ({ ctx, input }) => { | |
| const { id, ...patch } = input; | |
| const [updatedProject] = await ctx.db.update(projects).set({ | |
| ...patch, | |
| updatedAt: new Date(), | |
| }).where(eq(projects.id, id)).returning(); | |
| if (!updatedProject) { | |
| throw new Error('Project not found'); | |
| } | |
| return fromDbProject(updatedProject); | |
| }), |
🤖 Prompt for AI Agents
In apps/web/client/src/server/api/routers/project/project.ts around lines 352 to
363, the mutation currently spreads the whole input into .set(...) which
attempts to update the primary key (id); destructure id out of input (e.g. const
{ id, ...data } = input) and pass only the remaining fields plus updatedAt to
.set({ ...data, updatedAt: new Date() }) so the id is only used in the where
clause and not included in the SET clause.
| export async function updateDeployment(db: DrizzleDb, deployment: z.infer<typeof deploymentUpdateSchema>): Promise<Deployment | null> { | ||
| try { | ||
| const [result] = await db.update(deployments).set({ | ||
| ...deployment, | ||
| type: deployment.type as DeploymentType, | ||
| status: deployment.status as DeploymentStatus | ||
| }).where( | ||
| and( | ||
| eq(deployments.id, deploymentId), | ||
| eq(deployments.id, deployment.id), | ||
| ne(deployments.status, DeploymentStatus.CANCELLED) | ||
| ) | ||
| ).returning(); | ||
| return result ?? null; | ||
| } catch (error) { | ||
| console.error(`Failed to update deployment ${deploymentId}:`, error); | ||
| console.error(`Failed to update deployment ${deployment.id}:`, error); | ||
| return null; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix partial-update handling: avoid setting primary key and undefined fields; also bump updatedAt.
Spreading the entire payload into SET includes id and may set type/status to undefined when callers send partial updates (see deployment router). That can violate NOT NULL constraints or attempt a no-op PK write. Build the SET object explicitly, omit id, conditionally include type/status, and update updatedAt.
Apply:
export async function updateDeployment(db: DrizzleDb, deployment: z.infer<typeof deploymentUpdateSchema>): Promise<Deployment | null> {
try {
- const [result] = await db.update(deployments).set({
- ...deployment,
- type: deployment.type as DeploymentType,
- status: deployment.status as DeploymentStatus
- }).where(
- and(
- eq(deployments.id, deployment.id),
- ne(deployments.status, DeploymentStatus.CANCELLED)
- )
- ).returning();
+ // Build a SET payload that excludes immutable keys and omits undefined fields
+ const { id, type, status, ...rest } = deployment;
+ const setData: Record<string, unknown> = { ...rest, updatedAt: new Date() };
+ if (type !== undefined) setData.type = type as DeploymentType;
+ if (status !== undefined) setData.status = status as DeploymentStatus;
+
+ const [result] = await db
+ .update(deployments)
+ .set(setData)
+ .where(
+ and(
+ eq(deployments.id, id),
+ ne(deployments.status, DeploymentStatus.CANCELLED)
+ )
+ )
+ .returning();
return result ?? null;
} catch (error) {
console.error(`Failed to update deployment ${deployment.id}:`, error);
return null;
}
}Run to confirm no callsite passes type unless intended:
🏁 Script executed:
#!/bin/bash
rg -n -C2 "updateDeployment\(" --glob '!**/dist/**' --glob '!**/build/**'Length of output: 9510
🏁 Script executed:
#!/bin/bash
# Show the definition and signature of updateDeployment to confirm its parameters
rg -nP "export async function updateDeployment" -n apps/web/client/src/server/api/routers/publish/helpers/helpers.ts -C5Length of output: 550
🏁 Script executed:
#!/bin/bash
# Locate the definition of deploymentUpdateSchema to inspect its fields and optionality
rg -nP "deploymentUpdateSchema\\s*=" -n apps/web/client/src -C3Length of output: 64
🏁 Script executed:
#!/bin/bash
# List files in the publish router directory
ls apps/web/client/src/server/api/routers/publish
echo "---- Top 20 lines of deployment.ts for import inspection ----"
sed -n '1,20p' apps/web/client/src/server/api/routers/publish/deployment.tsLength of output: 1093
🏁 Script executed:
#!/bin/bash
# Recursively search for deploymentUpdateSchema across the repository
rg -n "deploymentUpdateSchema" -C3 .Length of output: 4426
🏁 Script executed:
#!/bin/bash
# Print the createUpdateSchema section to see which fields are optional or required
sed -n '45,65p' packages/db/src/schema/domain/deployment.tsLength of output: 360
🏁 Script executed:
#!/bin/bash
# Inspect the beginning of manager.ts for updateDeployment usage and import
sed -n '1,50p' apps/web/client/src/server/api/routers/publish/manager.tsLength of output: 1718
🏁 Script executed:
#!/bin/bash
# List helper files
ls apps/web/client/src/server/api/routers/publish/helpers
# Print the first 200 lines of publish.ts to inspect updateDeployment usage
sed -n '1,200p' apps/web/client/src/server/api/routers/publish/helpers/publish.tsLength of output: 3897
🏁 Script executed:
#!/bin/bash
sed -n '1,200p' packages/db/src/schema/domain/deployment.tsLength of output: 2150
Refactor updateDeployment signature and payload handling
The current implementation in helpers.ts spreads the entire deployment payload—including the primary key (id) and any undefined fields—into the .set() call, risking PK writes and NOT NULL violations, and it never updates the updatedAt timestamp. We should:
• Change the function signature to accept id separately from the update data
• Destructure and omit id from the payload, conditionally include only defined fields
• Add an updatedAt: new Date() assignment
• Adjust all callsites to pass (db, id, data) instead of a single object
Files to update:
- apps/web/client/src/server/api/routers/publish/helpers/helpers.ts
- apps/web/client/src/server/api/routers/publish/deployment.ts
- apps/web/client/src/server/api/routers/publish/helpers/publish.ts
- apps/web/client/src/server/api/routers/publish/helpers/unpublish.ts
- apps/web/client/src/server/api/routers/publish/manager.ts
Suggested refactor in helpers.ts:
-export async function updateDeployment(db: DrizzleDb, deployment: z.infer<typeof deploymentUpdateSchema>): Promise<Deployment | null> {
+export async function updateDeployment(
+ db: DrizzleDb,
+ id: string,
+ changes: Omit<Partial<z.infer<typeof deploymentUpdateSchema>>, 'id'>
+): Promise<Deployment | null> {
try {
- const [result] = await db.update(deployments).set({
- ...deployment,
- type: deployment.type as DeploymentType,
- status: deployment.status as DeploymentStatus
- }).where(
- and(
- eq(deployments.id, deployment.id),
- ne(deployments.status, DeploymentStatus.CANCELLED)
- )
- ).returning();
+ // Exclude PK, only set defined fields, and bump updatedAt
+ const { type, status, ...rest } = changes;
+ const setData: Record<string, unknown> = { ...rest, updatedAt: new Date() };
+ if (type !== undefined) setData.type = type as DeploymentType;
+ if (status !== undefined) setData.status = status as DeploymentStatus;
+
+ const [result] = await db
+ .update(deployments)
+ .set(setData)
+ .where(
+ and(
+ eq(deployments.id, id),
+ ne(deployments.status, DeploymentStatus.CANCELLED)
+ )
+ )
+ .returning();
return result ?? null;
} catch (error) {
console.error(`Failed to update deployment ${id}:`, error);
return null;
}
}Then update callsites (e.g. in deployment.ts, publish.ts, unpublish.ts, manager.ts) to:
// old
await updateDeployment(db, { id, status: ..., message: ... });
// new
await updateDeployment(db, id, { status: ..., message: ... });This ensures no PK or undefined fields get written and correctly refreshes updatedAt.
🤖 Prompt for AI Agents
In apps/web/client/src/server/api/routers/publish/helpers/helpers.ts around
lines 55 to 71, change updateDeployment to accept (db, id, data) instead of a
single deployment object; inside the function destructure/omit id, build the
.set() payload by conditionally adding only fields from data that are !==
undefined (to avoid writing PK or NULLs), always include updatedAt: new Date(),
cast type/status as needed, and use the separate id in the WHERE clause (and !=
CANCELLED); then update all callsites in deployment.ts, helpers/publish.ts,
helpers/unpublish.ts, and manager.ts to call updateDeployment(db, id, {
...fields }) instead of passing the whole object.
| git: { | ||
| branch: dbBranch.gitBranch, | ||
| commitSha: dbBranch.gitCommitSha, | ||
| repoUrl: dbBranch.gitRepoUrl, | ||
| }, |
There was a problem hiding this comment.
fromDbBranch always returns a truthy git object — return null when all git fields are null
Downstream code often uses if (branch.git) { ... }. Returning an object with all-null fields breaks that contract and can cause logic errors.
Apply:
- git: {
- branch: dbBranch.gitBranch,
- commitSha: dbBranch.gitCommitSha,
- repoUrl: dbBranch.gitRepoUrl,
- },
+ git:
+ dbBranch.gitBranch ||
+ dbBranch.gitCommitSha ||
+ dbBranch.gitRepoUrl
+ ? {
+ branch: dbBranch.gitBranch,
+ commitSha: dbBranch.gitCommitSha,
+ repoUrl: dbBranch.gitRepoUrl,
+ }
+ : null,📝 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.
| git: { | |
| branch: dbBranch.gitBranch, | |
| commitSha: dbBranch.gitCommitSha, | |
| repoUrl: dbBranch.gitRepoUrl, | |
| }, | |
| git: | |
| dbBranch.gitBranch || | |
| dbBranch.gitCommitSha || | |
| dbBranch.gitRepoUrl | |
| ? { | |
| branch: dbBranch.gitBranch, | |
| commitSha: dbBranch.gitCommitSha, | |
| repoUrl: dbBranch.gitRepoUrl, | |
| } | |
| : null, |
🤖 Prompt for AI Agents
In packages/db/src/mappers/project/branch.ts around lines 13-17, fromDbBranch
currently always returns a git object with branch/commitSha/repoUrl possibly set
to null which breaks callers that check `if (branch.git)`. Change the mapper to
detect when dbBranch.gitBranch, dbBranch.gitCommitSha and dbBranch.gitRepoUrl
are all null/undefined (or empty) and set git to null in that case; otherwise
return the git object with those fields. Ensure the null check covers undefined
and empty-string cases if applicable and keep the return shape/type consistent
with the branch DTO.
| sandbox: { | ||
| id: dbBranch.sandboxId, | ||
| }, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm sandboxId nullability and FK guarantees
Branch.sandbox is required in the model, but this mapper assumes dbBranch.sandboxId is always non-null. If the DB allows nulls or delayed sandbox assignment, this will violate the domain type.
Run to inspect the schema and constraints:
If nullable is possible, either:
- make
sandboxoptional in the model for transient states, or - enforce NOT NULL + FK at the DB and add creation-time assignment.
🏁 Script executed:
#!/bin/bash
# Inspect the branches schema for sandboxId definition and nullability
rg -nP -C3 'sandboxId' packages/db/src/schema/project/branch.ts
# Also check any migrations that alter sandboxId
fd -t f -a packages/db -g '*branch*' | xargs -I{} rg -nP -C3 'sandboxId' {}Length of output: 490
Enforce Referential Integrity on sandboxId
The schema at packages/db/src/schema/project/branch.ts already declares
sandboxId: varchar('sandbox_id').notNull(),so null values are disallowed. However, there is no .references() call to enforce a foreign-key relationship with the sandbox table—meaning referential integrity isn’t guaranteed.
• File: packages/db/src/schema/project/branch.ts
– At the sandboxId column definition, add a .references() clause.
• Migration: create a new migration to alter the branch table and add the FK constraint.
Suggested diff:
// sandbox
- sandboxId: varchar('sandbox_id').notNull(),
+ sandboxId: varchar('sandbox_id').notNull()
+ .references(() => sandbox.id),Then generate/apply a migration so the database enforces the foreign key.
🤖 Prompt for AI Agents
In packages/db/src/mappers/project/branch.ts around lines 18-20 and in
packages/db/src/schema/project/branch.ts, the mapper is setting sandbox.id from
dbBranch.sandboxId but the schema's sandboxId lacks a .references() FK; update
the schema column to sandboxId:
varchar('sandbox_id').notNull().references('sandbox.id') (choose ON
DELETE/UPDATE behavior consistent with your data model, e.g.,
.references('sandbox.id').onDelete('CASCADE') or .onDelete('RESTRICT'), and keep
the notNull), then create a new migration that ALTER TABLE project_branch (or
the actual table name) ADD CONSTRAINT fk_branch_sandbox FOREIGN KEY (sandbox_id)
REFERENCES sandbox(id) with the same ON DELETE/UPDATE rules, and ensure the
migration handles existing data (clean or backfill/null-check) before applying
the constraint.
| const dbBranches = await ctx.db.query.branches.findMany({ | ||
| where: eq(branches.projectId, input.projectId), | ||
| }); | ||
| if (!dbBranches) { |
There was a problem hiding this comment.
findMany always returns an array. Replace the check with 'if (dbBranches.length === 0)' to properly detect absence of branches.
| if (!dbBranches) { | |
| if (dbBranches.length === 0) { |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/web/client/src/server/api/routers/publish/helpers/unpublish.ts (2)
17-22: Await and guard updateDeployment(IN_PROGRESS) to ensure state consistencyThis call is not awaited and its result isn’t checked. publish.ts awaits and verifies updateDeployment; unpublish should mirror that to avoid racing ahead or silently losing the state transition.
Apply this diff:
- updateDeployment(db, { + const updateDeploymentResult1 = await updateDeployment(db, { id: deployment.id, status: DeploymentStatus.IN_PROGRESS, message: 'Unpublishing project...', progress: 20, - }); + }); + if (!updateDeploymentResult1) { + throw new TRPCError({ + code: 'BAD_REQUEST', + message: 'Update deployment failed', + }); + }
38-43: Await updateDeployment(FAILED) inside catch to avoid background racesOn failure, ensure the FAILED status is persisted before rethrowing. Awaiting matches publish.ts and reduces chances of the error escaping before the status update is stored.
- updateDeployment(db, { + await updateDeployment(db, { id: deployment.id, status: DeploymentStatus.FAILED, error: error instanceof Error ? error.message : 'Unknown error', progress: 100, });apps/web/client/src/server/api/routers/publish/helpers/publish.ts (1)
68-74: Add final COMPLETED status update after deploymentThe
publishfunction currently marks the deployment as IN_PROGRESS up through the deploy step but never sets it to COMPLETED on success—so if no other process does so, the deployment will remain stuck in progress. We should append a final update to mark the status as COMPLETED (100%) immediately afterdeployFreestylesucceeds.• File:
apps/web/client/src/server/api/routers/publish/helpers/publish.ts
Insert after line 96 (right after theawait deployFreestyle(...)call)await deployFreestyle({ files, urls: deploymentUrls, envVars: mergedEnvVars, }); + // Mark deployment as completed + const finalizeResult = await updateDeployment(db, { + id: deploymentId, + status: DeploymentStatus.COMPLETED, + message: 'Project published!', + progress: 100, + }); + if (!finalizeResult) { + throw new TRPCError({ + code: 'BAD_REQUEST', + message: 'Finalizing deployment failed', + }); + }With this change, successful deployments will be explicitly closed out with a COMPLETED status and progress at 100%.
🧹 Nitpick comments (4)
apps/web/client/src/server/api/routers/publish/helpers/publish.ts (1)
26-32: Optional: DRY the guarded updateDeployment patternYou repeat the “call updateDeployment + null check + TRPCError” pattern. Consider a small helper to reduce duplication in both publish and unpublish.
Example helper (in helpers.ts):
export async function guardedUpdateDeployment(db: DrizzleDb, patch: Parameters<typeof updateDeployment>[1]) { const res = await updateDeployment(db, patch); if (!res) { throw new TRPCError({ code: 'BAD_REQUEST', message: 'Update deployment failed' }); } return res; }Then replace call sites with guardedUpdateDeployment(db, { ... }).
Also applies to: 45-53, 68-74
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx (3)
48-53: Prevent redundant branch switches and potential thrashThis effect will run on every new branch object reference and call switchToBranch every time. If switchToBranch triggers state that re-fetches branch, you can create a loop. Guard with a ref against repeat switches for the same id. Also include editorEngine in deps to avoid stale closures.
Apply this diff:
- useEffect(() => { - if (branch) { - editorEngine.branches.switchToBranch(branch.id); - } - }, [branch]); + useEffect(() => { + if (!branch) return; + if (lastSwitchedBranchId.current === branch.id) return; + // If async, consider: void editorEngine.branches.switchToBranch(branch.id) + editorEngine.branches.switchToBranch(branch.id); + lastSwitchedBranchId.current = branch.id; + }, [branch, editorEngine]);And add useRef to imports (outside this hunk):
- import { useEffect, useState } from 'react'; + import { useEffect, useState, useRef } from 'react';Plus declare the ref once (near other hooks, outside this hunk):
const lastSwitchedBranchId = useRef<string | null>(null);If switchToBranch returns a Promise, either await it in an async effect or use void to avoid unhandled rejections and add error handling.
132-144: Ready flag should also consider branch success/failure; add dependencies accordinglyCurrently, readiness ignores whether a branch was actually resolved or failed; it only checks that the query is not loading. This can flip the app to “ready” before the active branch is applied. Also, the effect doesn’t depend on branch/branchError values.
Apply this diff to ensure readiness requires either a resolved branch or a surfaced error, and track those dependencies:
const allQueriesResolved = !isUserLoading && !isProjectLoading && !isCanvasLoading && !isConversationsLoading && !isCreationRequestLoading && - !isSandboxLoading && - !isBranchLoading; + !isSandboxLoading && + !isBranchLoading && + (!!branch || !!branchError); setIsProjectReady(allQueriesResolved); - }, [isUserLoading, isProjectLoading, isCanvasLoading, isConversationsLoading, isCreationRequestLoading, isSandboxLoading, isBranchLoading]); + }, [isUserLoading, isProjectLoading, isCanvasLoading, isConversationsLoading, isCreationRequestLoading, isSandboxLoading, isBranchLoading, branch, branchError]);Note: This pairs with surfacing branchError in the aggregated error setter (see earlier comment).
30-30: SurfacebranchErrorin aggregated error state – Since this hook lives under the/project/[id]dynamic route andeditorEngine.projectIdis always defined, gating the branch query onprojectIdisn’t required (all other project‐scoped queries likewise run unconditionally). However, we should includebranchErrorin our error aggregator so any TRPC failures here aren’t swallowed:• In your final
useEffect, update the call tosetError:useEffect(() => { - setError( - userError?.message - ?? projectError?.message - ?? canvasError?.message - ?? conversationsError?.message - ?? creationRequestError?.message - ?? null - ); + setError( + userError?.message + ?? projectError?.message + ?? canvasError?.message + ?? conversationsError?.message + ?? creationRequestError?.message + ?? branchError?.message + ?? null + ); }, [userError, projectError, canvasError, conversationsError, creationRequestError, branchError]);(Optional) If you’d prefer to guard all project‐scoped queries against a missing
projectId, apply a sharedenabled: Boolean(editorEngine.projectId)option to eachuseQuery.
📜 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 (4)
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx(3 hunks)apps/web/client/src/server/api/routers/project/branch.ts(1 hunks)apps/web/client/src/server/api/routers/publish/helpers/publish.ts(5 hunks)apps/web/client/src/server/api/routers/publish/helpers/unpublish.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/client/src/server/api/routers/project/branch.ts
🧰 Additional context used
🧬 Code graph analysis (3)
apps/web/client/src/server/api/routers/publish/helpers/unpublish.ts (1)
apps/web/client/src/server/api/routers/publish/helpers/helpers.ts (1)
updateDeployment(55-72)
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx (1)
apps/web/client/src/trpc/react.tsx (1)
api(23-23)
apps/web/client/src/server/api/routers/publish/helpers/publish.ts (1)
apps/web/client/src/server/api/routers/publish/helpers/helpers.ts (1)
updateDeployment(55-72)
🔇 Additional comments (4)
apps/web/client/src/server/api/routers/publish/helpers/publish.ts (4)
26-32: LGTM: payload-based updateDeployment usage with null-guardYou’ve updated the call site to pass an object including id and you verify the returned row. This is consistent and robust.
45-53: LGTM: propagating sandboxId and guarding resultPassing sandboxId in the patch and checking for null mirrors the first guard. Looks good.
95-100: LGTM: failure path updates status to FAILED with awaitConsistent with the new API and awaited to ensure persistence before rethrowing.
65-66: No changes needed: updateDeployment callback type matches schemaVerified that
PublishManager.publishexpects anupdateDeploymentcallback with signature
(deployment: z.infer<typeof deploymentUpdateSchema>) => Promise<Deployment | null>, and the helper in
apps/web/client/src/server/api/routers/publish/helpers/helpers.tsis defined asexport async function updateDeployment( db: DrizzleDb, deployment: z.infer<typeof deploymentUpdateSchema>, ): Promise<Deployment | null> { … }Passing it via
(deployment) => updateDeployment(db, deployment)in
apps/web/client/src/server/api/routers/publish/helpers/publish.tstherefore exactly matches the required type.
| editorEngine.screenshot.lastScreenshotAt = project.metadata.previewImg?.updatedAt ?? null; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Confirm lastScreenshotAt type; updatedAt may be Date/string vs previous numeric timestamp
The old field (updatedPreviewImgAt) was likely a number (epoch ms). The new chain uses previewImg?.updatedAt, which may be a string/Date. If consumers of editorEngine.screenshot.lastScreenshotAt expect a number, this will silently break comparisons/sorts.
Run this to verify both types before we normalize:
If lastScreenshotAt should remain a number, normalize:
- editorEngine.screenshot.lastScreenshotAt = project.metadata.previewImg?.updatedAt ?? null;
+ editorEngine.screenshot.lastScreenshotAt = project.metadata.previewImg?.updatedAt
+ ? new Date(project.metadata.previewImg.updatedAt as unknown as string | number | Date).getTime()
+ : null;If it's allowed to be a string/Date, consider renaming the field or adding a JSDoc to prevent type drift.
🏁 Script executed:
#!/bin/bash
# Where is lastScreenshotAt defined/used?
rg -nP -C3 "\blastScreenshotAt\b" apps packages -g '!**/node_modules/**'
# Inspect the type of Project.metadata.previewImg.updatedAt
rg -nP -C3 "previewImg\b" packages -g '!**/node_modules/**'Length of output: 8912
Fix mismatched types for lastScreenshotAt
The screenshot.lastScreenshotAt setter is typed as Date | null, but project.metadata.previewImg?.updatedAt is a string (per packages/models/src/project/project.ts), so assigning it directly will store a string instead of a Date.
• In apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx (line 44)
• Normalize the string to a Date:
- editorEngine.screenshot.lastScreenshotAt = project.metadata.previewImg?.updatedAt ?? null;
+ editorEngine.screenshot.lastScreenshotAt = project.metadata.previewImg?.updatedAt
+ ? new Date(project.metadata.previewImg.updatedAt)
+ : null;Consider adding a JSDoc or strengthening the type for previewImg.updatedAt if you ever intend to accept other formats.
📝 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.
| editorEngine.screenshot.lastScreenshotAt = project.metadata.previewImg?.updatedAt ?? null; | |
| } | |
| editorEngine.screenshot.lastScreenshotAt = project.metadata.previewImg?.updatedAt | |
| ? new Date(project.metadata.previewImg.updatedAt) | |
| : null; | |
| } |
🤖 Prompt for AI Agents
In apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx around
lines 44-45, the setter editorEngine.screenshot.lastScreenshotAt is typed Date |
null but project.metadata.previewImg?.updatedAt is a string, so
convert/normalize that string to a Date (or null) before assignment: if
previewImg?.updatedAt exists, construct a Date from it and check it’s valid (not
NaN) then assign that Date, otherwise assign null; consider adding a short
inline comment/JSDoc noting the normalization and optionally strengthen the
previewImg.updatedAt type to Date in models if you accept non-string values.
| updateDeployment(db, { | ||
| id: deployment.id, | ||
| status: DeploymentStatus.COMPLETED, | ||
| message: 'Project unpublished!', | ||
| progress: 100, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Await and guard updateDeployment(COMPLETED) after successful unpublish
Same issue here: not awaiting and not checking the result. Align this with publish.ts to fail fast if the DB update doesn’t land.
- updateDeployment(db, {
+ const updateDeploymentResult2 = await updateDeployment(db, {
id: deployment.id,
status: DeploymentStatus.COMPLETED,
message: 'Project unpublished!',
progress: 100,
- });
+ });
+ if (!updateDeploymentResult2) {
+ throw new TRPCError({
+ code: 'BAD_REQUEST',
+ message: 'Update deployment failed',
+ });
+ }📝 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.
| updateDeployment(db, { | |
| id: deployment.id, | |
| status: DeploymentStatus.COMPLETED, | |
| message: 'Project unpublished!', | |
| progress: 100, | |
| }); | |
| const updateDeploymentResult2 = await updateDeployment(db, { | |
| id: deployment.id, | |
| status: DeploymentStatus.COMPLETED, | |
| message: 'Project unpublished!', | |
| progress: 100, | |
| }); | |
| if (!updateDeploymentResult2) { | |
| throw new TRPCError({ | |
| code: 'BAD_REQUEST', | |
| message: 'Update deployment failed', | |
| }); | |
| } |
🤖 Prompt for AI Agents
In apps/web/client/src/server/api/routers/publish/helpers/unpublish.ts around
lines 31 to 36, the call to updateDeployment when marking the deployment
COMPLETED is neither awaited nor its result checked; change it to await the
updateDeployment call and verify the returned value (or throw/return an error)
just like publish.ts does so the handler fails fast if the DB update did not
persist. Ensure you await the promise, handle any thrown errors, and guard on a
falsy result by logging/throwing a suitable error to stop further execution.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx (1)
57-68: Fix indefinite loading on failure; always toggle sandbox loading.
If branch switching/sandbox start throws,isSandboxLoadingremains true forever, blocking readiness.- const setBranch = async (branch: Branch) => { - try { - await editorEngine.branches.switchToBranch(branch.id); - await editorEngine.branches.startCurrentBranchSandbox(); - setIsSandboxLoading(false); - } catch (error) { - console.error('Failed to set branch', error); - toast.error('Failed to set branch', { - description: error instanceof Error ? error.message : 'Unknown error', - }); - } - } + const setBranch = async (selectedBranch: Branch) => { + setIsSandboxLoading(true); + try { + await editorEngine.branches.switchToBranch(selectedBranch.id); + await editorEngine.branches.startCurrentBranchSandbox(); + } catch (error) { + console.error('Failed to set branch', error); + toast.error('Failed to set branch', { + description: error instanceof Error ? error.message : 'Unknown error', + }); + } finally { + setIsSandboxLoading(false); + } + }
♻️ Duplicate comments (1)
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx (1)
44-44: Normalize lastScreenshotAt to a Date.
This assigns a potentially string value to a Date-typed field.- editorEngine.screenshot.lastScreenshotAt = project.metadata.previewImg?.updatedAt ?? null; + editorEngine.screenshot.lastScreenshotAt = project.metadata.previewImg?.updatedAt + ? new Date(project.metadata.previewImg.updatedAt as unknown as string) + : null;If lastScreenshotAt is supposed to be a number (epoch), use
new Date(...).getTime()instead.
🧹 Nitpick comments (3)
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx (3)
31-31: Rename “branch” to “branches” and gate the query with enabled for safety.
- Improves clarity (API returns an array).
- Avoids firing before projectId is available.
Apply:
- const { data: branch, isLoading: isBranchLoading, error: branchError } = api.branch.getByProjectId.useQuery({ projectId: editorEngine.projectId }); + const { data: branches, isLoading: isBranchLoading, error: branchError } = + api.branch.getByProjectId.useQuery( + { projectId: editorEngine.projectId }, + { enabled: !!editorEngine.projectId } + );Note: Update references below (lines 49-55, 147, and readiness calc) from
branch→branches.
48-55: Guard default-branch selection; clear loading when no branches.
- Prevents repeated switching if data refetches.
- Unblocks readiness when a project has zero branches.
- useEffect(() => { - if (branch && branch.length > 0) { - const defaultBranch = branch.find(b => b.isDefault) || branch[0]; - if (defaultBranch) { - setBranch(defaultBranch); - } - } - }, [branch]); + useEffect(() => { + if (branches && branches.length > 0 && isSandboxLoading) { + const defaultBranch = branches.find(b => b.isDefault) || branches[0]; + if (defaultBranch) { + setBranch(defaultBranch); + } + } else if (branches && branches.length === 0) { + // Nothing to start; don't block readiness on sandbox. + setIsSandboxLoading(false); + } + }, [branches, isSandboxLoading]);
143-147: Don’t block readiness on sandbox when there’s no branch or branch query errored.
Keeps the app usable even if branching is unavailable for a project.- !isSandboxLoading && - !isBranchLoading; + ( + !isSandboxLoading || + branchError || + (Array.isArray(branches) && branches.length === 0) + ) && + !isBranchLoading; @@ - }, [isUserLoading, isProjectLoading, isCanvasLoading, isConversationsLoading, isCreationRequestLoading, isSandboxLoading, isBranchLoading]); + }, [isUserLoading, isProjectLoading, isCanvasLoading, isConversationsLoading, isCreationRequestLoading, isSandboxLoading, isBranchLoading, branchError, branches]);
📜 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 (2)
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx(4 hunks)apps/web/client/src/components/store/editor/branch/manager.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/client/src/components/store/editor/branch/manager.ts
🧰 Additional context used
🧬 Code graph analysis (1)
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx (2)
packages/models/src/project/branch.ts (1)
Branch(1-17)packages/db/src/schema/project/branch.ts (1)
Branch(47-47)
🔇 Additional comments (2)
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx (2)
12-12: LGTM: correct Branch typing import.
150-152: LGTM: consolidated error aggregation includes branchError.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Adds branching support with new models, API routes, and schema changes, refactoring existing code for branch-aware operations.
BranchManagerinmanager.tsfor branch management, including switching and creating branches.branchRouterinbranch.tsfor branch-related API operations.Frame,Project, andBranchmodels to support branching.branchestable inbranch.tswith metadata and sandbox information.framestable inframe.tsto includebranchId.projectstable inproject.tsto remove sandbox fields.toFramewithfromDbFrameinframe.tsand similar changes in other mappers.EditorEngineinengine.tsto use branch-specific sandbox logic.conversation.ts,deployment.ts.This description was created by
for b4d6f19. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes