Adding missing features that are not selected on the readme.md #3077
Conversation
This is a fork and we are adding the feature that onlook doesn't have so we can pull request so it does have
|
Someone is attempting to deploy a commit to the Onlook Team on Vercel. A member of the Team first needs to authorize it. |
|
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. 📝 WalkthroughWalkthroughAdds database enums, tables, RLS policies and migrations; introduces Drizzle schemas for assets, comments, figma, github, and mcp; adds TRPC routers for figma/components/comments/assets; adds server env vars for Figma; and introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as TRPC API
participant FigmaService as FigmaIntegrationService
participant FigmaClient as FigmaApiClient
participant Parser as FigmaFileParser
participant Database as Database
rect rgb(220,235,255)
Note over Client,Database: Figma File Import Flow
Client->>API: importFile(fileId, token)
API->>FigmaService: importFile(fileId)
activate FigmaService
FigmaService->>FigmaClient: request file data
FigmaClient-->>FigmaService: file data
FigmaService->>Parser: parseDocument(file data)
Parser-->>FigmaService: components, assets, tokens
FigmaService->>FigmaClient: getFileImages(nodeIds)
FigmaClient-->>FigmaService: image URLs
FigmaService->>Database: persist import (assets/components/tokens)
Database-->>FigmaService: OK
deactivate FigmaService
FigmaService-->>API: FigmaImportResult
API-->>Client: result
end
sequenceDiagram
participant Client as Client
participant API as TRPC API
participant AssetService as AssetManagementService
participant Storage as File Storage
participant Database as Database
rect rgb(235,255,230)
Note over Client,Database: Asset Upload & Optimization Flow
Client->>API: uploadAsset(projectId, fileData)
API->>AssetService: uploadAsset(file, projectId)
activate AssetService
AssetService->>AssetService: validate & buffer file
AssetService->>Storage: store original
Storage-->>AssetService: originalPath
alt image
AssetService->>AssetService: create thumbnail & optimized
AssetService->>Storage: store thumbnail/optimized
Storage-->>AssetService: paths
end
AssetService->>Database: save asset record
Database-->>AssetService: assetId
deactivate AssetService
AssetService-->>API: AssetUploadResult
API-->>Client: { id, url, paths }
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
CodeRabbit configuration file (
|
|
@coderabbitai I made this in 1 hour so just expect to see any errors |
|
Tip For best results, initiate chat on the files or code changes. @DevFlex-AI Thanks for the heads up! No worries at all – quick iterations are part of the development process, and I'm here to help catch any issues. 🚀 I've already reviewed the changes and left some feedback. The main points to consider are:
Feel free to address the feedback at your own pace. If you'd like me to take another look after you make changes, just let me know! Happy to help make this PR ready to merge. 👍 |
|
@coderabbitai so is there any issues with this or can the maintainers build without any problems? |
![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
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (18)
packages/platform-extensions/src/comments/service.ts-238-245 (1)
238-245: Username to userId resolution missing.The
processMentionsmethod extracts usernames from@mentionsand passes them tomentionUser, butmentionUserexpects auserId(UUID). There's no resolution of usernames to user IDs.Add a username lookup step before calling
mentionUser:private async processMentions(commentId: string, mentions: string[]): Promise<void> { for (const username of mentions) { try { const userId = await this.resolveUsername(username); if (userId) { await this.mentionUser(commentId, userId); } } catch (error) { console.error(`Failed to mention user ${username}:`, error); } } }packages/platform-extensions/src/comments/service.ts-7-218 (1)
7-218: Missing authorization checks for all operations.The service lacks any authorization logic. Methods like
resolveComment,updateComment,deleteComment, andaddReplydon't verify that the requesting user has permission to perform these actions. This would allow any user to modify or delete any comment.Consider adding authorization checks that verify:
- User can only update/delete their own comments
- User has appropriate project permissions
- Comment belongs to a project the user can access
packages/platform-extensions/src/comments/service.ts-1-291 (1)
1-291: In-memory storage is not production-ready.The service uses in-memory
Mapstorage (lines 4-5) and all database operations are simulated withconsole.logstatements. This means all comment data will be lost when the service restarts.Consider implementing actual database persistence using the database schemas defined elsewhere in this PR (e.g.,
packages/db/src/schema/comment/comment.ts).packages/platform-extensions/src/comments/service.ts-7-120 (1)
7-120: Add input sanitization for comment content.The
contentparameter increateComment,addReply, andupdateCommentis not sanitized or validated. If this content is rendered as HTML in the UI, it could lead to XSS attacks.Consider:
- Sanitizing HTML content
- Validating maximum length
- Escaping special characters
- Using a content security policy
packages/platform-extensions/src/github/service.ts-28-54 (1)
28-54: Replaceanytypes with proper typing per coding guidelines.Multiple methods use
Promise<any>return types (listRepositories,getRepository,makeRequest). Line 242 also uses(branch: any).🔎 Example typed approach
interface GitHubRepository { id: number; name: string; full_name: string; default_branch: string; // ... other fields } async listRepositories(): Promise<GitHubRepository[]> { // ... return response as GitHubRepository[]; }packages/platform-extensions/src/figma/api-client.ts-55-140 (1)
55-140: Pervasive use ofanyreturn types violates coding guidelines.All public API methods (
getFile,getFileNodes,getFileImages, etc.) returnPromise<any>. Additionally,clientMeta?: anyon line 132 should be typed.Consider defining interfaces for Figma API responses or using
Promise<unknown>with explicit casting at call sites.🔎 Example type definitions
// Define in types.ts or inline interface FigmaFileResponse { document: unknown; components: Record<string, unknown>; // ... other fields } // Then use: async getFile(fileId: string): Promise<FigmaFileResponse> { return this.makeRequest(`/files/${fileId}`) as Promise<FigmaFileResponse>; }packages/platform-extensions/src/figma/api-client.ts-19-50 (1)
19-50: Replaceanyreturn types and add retry limit to prevent infinite loops.
Per coding guidelines, avoid
anytypes. ThemakeRequestmethod and all public methods returnPromise<any>— consider usingPromise<unknown>or defining proper response types.The 429 retry logic has no maximum retry count, risking infinite recursion if the API persistently rate-limits.
The
Retry-Afterheader can be either seconds or an HTTP-date string per RFC 7231;parseIntwill fail silently on dates.🔎 Suggested fix for retry limiting
- private async makeRequest(endpoint: string, options: RequestInit = {}): Promise<any> { + private async makeRequest(endpoint: string, options: RequestInit = {}, retryCount = 0): Promise<unknown> { + const MAX_RETRIES = 3; // Rate limiting const now = Date.now(); const timeSinceLastRequest = now - this.lastRequestTime; if (timeSinceLastRequest < this.rateLimitDelay) { await new Promise(resolve => setTimeout(resolve, this.rateLimitDelay - timeSinceLastRequest)); } this.lastRequestTime = Date.now(); const url = `${this.baseUrl}${endpoint}`; const response = await fetch(url, { ...options, headers: { 'Authorization': `Bearer ${this.accessToken}`, 'Content-Type': 'application/json', ...options.headers, }, }); if (!response.ok) { if (response.status === 429) { + if (retryCount >= MAX_RETRIES) { + throw new Error('Figma API rate limit exceeded after max retries'); + } // Rate limited, wait and retry const retryAfter = response.headers.get('Retry-After'); - const delay = retryAfter ? parseInt(retryAfter) * 1000 : 5000; + const delay = retryAfter ? (parseInt(retryAfter, 10) || 5) * 1000 : 5000; await new Promise(resolve => setTimeout(resolve, delay)); - return this.makeRequest(endpoint, options); + return this.makeRequest(endpoint, options, retryCount + 1); } throw new Error(`Figma API error: ${response.status} ${response.statusText}`); } return response.json(); }packages/platform-extensions/src/github/service.ts-248-260 (1)
248-260: Guard against empty commits array to prevent crash on empty repositories.Accessing
commitsResponse[0]without checking array length will throw if the repository has no commits.🔎 Suggested fix
// Get latest commit const commitsResponse = await this.makeRequest(`/repos/${owner}/${repoName}/commits?per_page=1`); - const lastCommit = commitsResponse[0]; + + if (!commitsResponse.length) { + return { + branches, + lastCommit: undefined, + status: 'synced', + }; + } + const lastCommit = commitsResponse[0]; return { branches, lastCommit: {Note: This requires making
lastCommitoptional inRepositoryStateas suggested in the types file.packages/platform-extensions/src/github/service.ts-68-99 (1)
68-99: Validate repo format to prevent runtime crash on malformed input.The
repo.split('/')pattern assumes the input is always"owner/repo"format. If the input is malformed (e.g., missing/),ownerorrepoNamewill be undefined, causing API calls to fail with confusing errors.🔎 Suggested validation helper
+ private parseRepoString(repo: string): { owner: string; repoName: string } { + const parts = repo.split('/'); + if (parts.length !== 2 || !parts[0] || !parts[1]) { + throw new Error(`Invalid repository format: "${repo}". Expected "owner/repo".`); + } + return { owner: parts[0], repoName: parts[1] }; + } async createBranch(repo: string, branchName: string): Promise<Branch> { // ... - const [owner, repoName] = repo.split('/'); + const { owner, repoName } = this.parseRepoString(repo);Committable suggestion skipped: line range outside the PR's diff.
apps/web/client/src/server/api/routers/comments.ts-37-46 (1)
37-46: Same security issue:userIdshould be derived from context.Apply the same fix to use
ctx.user.idinstead of acceptinguserIdfrom input.🔎 Proposed fix
addReply: protectedProcedure .input(z.object({ commentId: z.string(), content: z.string(), - userId: z.string(), })) - .mutation(async ({ input }) => { + .mutation(async ({ input, ctx }) => { const service = new CommentService(); - return service.addReply(input.commentId, input.content, input.userId); + return service.addReply(input.commentId, input.content, ctx.user.id); }),apps/web/client/src/server/api/routers/comments.ts-6-28 (1)
6-28: Security concern:userIdshould come from authenticated context, not user input.The
protectedProcedurealready ensures an authenticated user exists inctx.user. AcceptinguserIdfrom user input allows callers to impersonate other users. Usectx.user.idinstead.🔎 Proposed fix
createComment: protectedProcedure .input(z.object({ projectId: z.string(), elementId: z.string(), content: z.string(), position: z.object({ x: z.number(), y: z.number(), elementSelector: z.string(), pageUrl: z.string(), }), - userId: z.string(), })) - .mutation(async ({ input }) => { + .mutation(async ({ input, ctx }) => { const service = new CommentService(); return service.createComment( input.projectId, input.elementId, input.content, input.position, - input.userId + ctx.user.id ); }),apps/web/client/src/server/api/routers/assets.ts-6-29 (1)
6-29: Add validation constraints for file uploads.Missing constraints could lead to:
- Memory exhaustion from large base64 payloads
- Accepting disallowed file types
Consider adding limits and type validation.
🔎 Proposed fix
uploadAsset: protectedProcedure .input(z.object({ projectId: z.string(), fileName: z.string(), - fileType: z.string(), - fileSize: z.number(), - fileData: z.string(), // base64 encoded + fileType: z.string().refine( + (type) => ['image/png', 'image/jpeg', 'image/gif', 'image/svg+xml', 'image/webp'].includes(type), + { message: 'Unsupported file type' } + ), + fileSize: z.number().max(10 * 1024 * 1024, 'File size must be under 10MB'), + fileData: z.string().max(14 * 1024 * 1024), // ~10MB after base64 encoding overhead }))packages/platform-extensions/src/components/types.ts-13-20 (1)
13-20: ReplaceanywithunknownfordefaultValue.The coding guidelines specify avoiding
anyunless necessary. Useunknownhere for consistency withPropDefinition.defaultValue(line 46) which correctly usesunknown.Suggested fix
export interface ComponentProp { name: string; type: PropType; required: boolean; - defaultValue?: any; + defaultValue?: unknown; description?: string; options?: string[]; // For enum types }As per coding guidelines, avoid using the
anytype unless absolutely necessary.packages/platform-extensions/src/figma/client.ts-79-86 (1)
79-86: Add maximum retry limit to prevent infinite retry loops.The rate limit retry logic has no maximum retry count. A persistent 429 response could cause infinite retries.
Suggested fix
- private async makeRequest<T>(endpoint: string, options: RequestInit = {}): Promise<T> { + private async makeRequest<T>(endpoint: string, options: RequestInit = {}, retryCount = 0): Promise<T> { + const MAX_RETRIES = 3; const url = endpoint.startsWith('http') ? endpoint : `${this.baseUrl}${endpoint}`; const response = await fetch(url, { ...options, headers: { 'Authorization': `Bearer ${this.accessToken}`, 'Content-Type': 'application/json', ...options.headers, }, }); // Handle rate limiting if (response.status === 429) { + if (retryCount >= MAX_RETRIES) { + throw new FigmaApiError('Rate limit exceeded after max retries', 429, ''); + } const retryAfter = response.headers.get('Retry-After'); - const waitTime = retryAfter ? parseInt(retryAfter) * 1000 : 60000; // Default 60s + const waitTime = retryAfter ? parseInt(retryAfter, 10) * 1000 : 60000; await this.sleep(waitTime); - return this.makeRequest<T>(endpoint, options); // Retry + return this.makeRequest<T>(endpoint, options, retryCount + 1); }apps/web/client/src/server/api/routers/components-panel.ts-44-57 (1)
44-57: MissingprojectIdand service initialization ininsertComponent.Unlike other procedures,
insertComponentdoesn't accept aprojectIdand doesn't callservice.initialize(). The service'scomponentsarray will be empty, causing the component lookup at line 107 of the service to always fail with "Component not found".🔎 Proposed fix
insertComponent: protectedProcedure .input(z.object({ + projectId: z.string(), componentId: z.string(), targetElementId: z.string(), position: z.enum(['before', 'after', 'inside', 'replace']), })) .mutation(async ({ input }) => { const service = new ComponentsPanelService(); + await service.initialize(input.projectId); return service.insertComponent( input.componentId, input.targetElementId, input.position ); }),packages/platform-extensions/src/figma/token-storage.ts-7-8 (1)
7-8: Hardcoded encryption key is a security risk.The
ENCRYPTION_KEYis visible in source code and will be committed to version control. Even though the current implementation uses base64 (not encryption), this pattern sets a dangerous precedent.If this is placeholder code, consider adding a clear runtime error or using environment variables:
- private static readonly ENCRYPTION_KEY = 'onlook_figma_key'; // In production, use proper encryption + private static readonly ENCRYPTION_KEY = process.env.FIGMA_ENCRYPTION_KEY;apps/backend/supabase/migrations/0020_volatile_peter_parker.sql-204-204 (1)
204-204:ON DELETE no actionforparent_idmay leave orphaned comment threads.If a parent comment is deleted, child comments will be orphaned with invalid
parent_idreferences. ConsiderON DELETE CASCADEorON DELETE SET NULLdepending on desired behavior.-ALTER TABLE "comments" ADD CONSTRAINT "comments_parent_id_comments_id_fk" FOREIGN KEY ("parent_id") REFERENCES "public"."comments"("id") ON DELETE no action ON UPDATE no action; +ALTER TABLE "comments" ADD CONSTRAINT "comments_parent_id_comments_id_fk" FOREIGN KEY ("parent_id") REFERENCES "public"."comments"("id") ON DELETE CASCADE ON UPDATE no action;packages/platform-extensions/src/figma/parser.ts-103-106 (1)
103-106:isArtboardcheck is unreliable because parent references are never populated.The Figma REST API doesn't include parent references, and the code never assigns them during traversal. The condition
node.parent?.type === 'PAGE'will always be false. Either add parent references during parsing or revise the artboard detection logic to use a different approach (e.g., check constraints, layout properties, or rely on hierarchical position).Additionally, replace the
anytype with proper typing.
🟡 Minor comments (8)
DONT-CLICK-ON-THIS.HTML-33-52 (1)
33-52: Missing error handling for microphone permission denial.The
getUserMediacall has no rejection handler. If the user denies microphone access, the animation won't start and there will be no feedback to the user.🔎 Proposed fix for error handling
-navigator.mediaDevices.getUserMedia({audio:true}).then(s=>{ +navigator.mediaDevices.getUserMedia({audio:true}).then(s=>{ const a=new AudioContext() const m=a.createMediaStreamSource(s) const n=a.createAnalyser() n.fftSize=512 m.connect(n) const d=new Uint8Array(n.frequencyBinCount) function f(){ n.getByteFrequencyData(d) let v=d.reduce((p,c)=>p+c,0)/d.length let j=(Math.random()-.5)*v*1.5 let k=(Math.random()-.5)*v*1.5 let r=(Math.random()-.5)*v*2 let sc=1+v/60 e.style.transform=`translate(${j}px,${k}px) rotate(${r}deg) scale(${sc})` e.style.filter=`blur(${v/18}px) hue-rotate(${v*4}deg)` requestAnimationFrame(f) } f() +}).catch(err=>{ +console.error('Microphone access denied:', err) +e.textContent='Microphone access required' })Committable suggestion skipped: line range outside the PR's diff.
apps/web/client/src/server/api/routers/figma.ts-35-49 (1)
35-49: Avoidz.any()- violates coding guidelines.The
properties,styles, andchildrenarrays usez.any(), which bypasses type safety and violates the guideline to avoid theanytype. Define proper schemas for these fields.As per coding guidelines, avoid using
anyunless necessary. Consider defining specific schemas:🔎 Proposed fix
convertComponents: protectedProcedure .input(z.object({ components: z.array(z.object({ id: z.string(), name: z.string(), type: z.string(), - properties: z.array(z.any()), - styles: z.array(z.any()), - children: z.array(z.any()), + properties: z.array(z.record(z.string(), z.unknown())), + styles: z.array(z.record(z.string(), z.unknown())), + children: z.array(z.lazy(() => componentSchema)), // or z.unknown() if recursive typing is complex })) }))Committable suggestion skipped: line range outside the PR's diff.
packages/db/src/schema/comment/comment.ts-49-54 (1)
49-54: Self-referential relations require matchingrelationNamefor bidirectional mapping.The
parentandrepliesrelations both reference thecommentstable but don't use a sharedrelationName. Without this, Drizzle may not correctly associate parent comments with their replies.🔎 Proposed fix
+const COMMENT_PARENT_RELATION_NAME = 'commentParentChild'; + parent: one(comments, { fields: [comments.parentId], references: [comments.id], + relationName: COMMENT_PARENT_RELATION_NAME, }), -replies: many(comments), +replies: many(comments, { + relationName: COMMENT_PARENT_RELATION_NAME, +}),packages/platform-extensions/src/figma/client.ts-59-62 (1)
59-62: Await the recursiveprocessQueuecall to prevent stack buildup.The recursive call to
processQueue()is not awaited, which could lead to unexpected behavior or stack growth on high-volume request processing.Suggested fix
// Process next request - this.processQueue(); + await this.processQueue(); }apps/backend/supabase/migrations/0020_volatile_peter_parker.sql-29-29 (1)
29-29:line_numbershould be integer, not varchar.Line numbers are numeric values. Using
varcharprevents range queries and proper sorting.- "line_number" varchar, + "line_number" integer,packages/platform-extensions/src/assets/service.ts-320-322 (1)
320-322:getFileExtensionreturns full filename when no extension exists.If the filename has no
.,lastIndexOfreturns-1, andsubstring(-1)returns the last character of the string, which is incorrect.🔎 Proposed fix
private getFileExtension(name: string): string { - return name.substring(name.lastIndexOf('.')); + const lastDot = name.lastIndexOf('.'); + return lastDot !== -1 ? name.substring(lastDot) : ''; }packages/platform-extensions/src/figma/parser.ts-312-318 (1)
312-318: Potential division by zero inisIcon.If
heightis 0, the aspect ratio calculation will produceInfinity, which will pass theaspectRatio <= 2check unexpectedly.🔎 Proposed fix
private static isIcon(node: any): boolean { const bounds = node.absoluteBoundingBox; if (!bounds) return false; const width = bounds.width; const height = bounds.height; + if (height === 0 || width === 0) return false; const aspectRatio = width / height; return width <= 100 && height <= 100 && aspectRatio >= 0.5 && aspectRatio <= 2; }packages/platform-extensions/src/assets/service.ts-316-318 (1)
316-318:sanitizeFileNamecan produce empty strings or collisions.If a filename consists entirely of special characters (e.g.,
日本語.png), the regex will produce an empty string. Additionally, different filenames likefoo bar.pngandfoo_bar.pngwill collide asfoo-bar.🔎 Proposed fix
private sanitizeFileName(name: string): string { - return name.replace(/[^a-zA-Z0-9.-]/g, '-').toLowerCase(); + const baseName = name.substring(0, name.lastIndexOf('.')) || name; + const sanitized = baseName.replace(/[^a-zA-Z0-9.-]/g, '-').toLowerCase(); + return sanitized || `asset-${Date.now()}`; }Committable suggestion skipped: line range outside the PR's diff.
🧹 Nitpick comments (41)
DONT-CLICK-ON-THIS.HTML (1)
1-61: Consider removing this file from the repository.Per the PR description, this file was "added as a joke and may be deleted later." Non-production code should not be committed to the repository.
packages/db/src/schema/github/github-integration.ts (3)
19-19: Consider adding enum constraint for status field.The comment indicates specific valid values (
'pending','created','merged','closed'), but the schema doesn't enforce them. Using an enum or check constraint would prevent invalid status values.🔎 Proposed fix using Drizzle enum
At the top of the file, define the enum:
+import { pgEnum } from 'drizzle-orm/pg-core'; + +export const integrationStatusEnum = pgEnum('integration_status', ['pending', 'created', 'merged', 'closed']);Then use it in the schema:
- status: varchar('status').notNull().default('pending'), // 'pending', 'created', 'merged', 'closed' + status: integrationStatusEnum('status').notNull().default('pending'),
31-31: Consider adding foreign key constraint for createdBy.The
createdByfield appears to reference a user ID but lacks a foreign key constraint. Adding one would ensure referential integrity.If there's a users table, add the FK:
- createdBy: uuid('created_by').notNull(), // user id + createdBy: uuid('created_by').references(() => users.id, { onDelete: 'cascade' }).notNull(),
22-22: Consider adding schema validation for the changes JSONB field.The
changesfield stores an array of code changes but has no schema validation. Consider using Zod or a similar library to validate the structure at runtime, or document the expected shape in a TypeScript type.packages/db/src/schema/asset/asset-reference.ts (2)
13-13: Consider using integer type for lineNumber.Line numbers are typically integers. Using
varcharallows invalid values and prevents numeric operations. Consider using an integer type or at least validate the format if string representation is required.🔎 Proposed fix
- lineNumber: varchar('line_number'), // line number in file + lineNumber: integer('line_number'), // line number in fileIf you need to allow null, keep it optional:
+import { integer } from 'drizzle-orm/pg-core';
17-17: Consider adding enum constraint for referenceType field.The comment indicates specific valid values (
'import','url','inline'), but the schema doesn't enforce them. Using an enum would prevent invalid reference types.🔎 Proposed fix using Drizzle enum
At the top of the file, define the enum:
+import { pgEnum } from 'drizzle-orm/pg-core'; + +export const referenceTypeEnum = pgEnum('reference_type', ['import', 'url', 'inline']);Then use it in the schema:
- referenceType: varchar('reference_type').notNull(), // 'import', 'url', 'inline' + referenceType: referenceTypeEnum('reference_type').notNull(),packages/platform-extensions/src/comments/service.ts (1)
150-170: Consider optimizing comment queries and adding pagination.Both
getCommentsForElementandgetProjectCommentsperform linear scans through all comments. As the number of comments grows, this will become a performance bottleneck. Additionally, there's no pagination, which could lead to memory issues and slow responses.Consider:
- Using indexed data structures (Map by elementId/projectId)
- Adding pagination parameters (limit, offset or cursor-based)
- Implementing database queries with proper indexes when persistence is added
packages/db/src/schema/asset/asset.ts (1)
37-39: Consider adding a foreign key constraint onuploadedBy.The
uploadedBycolumn stores a user UUID but lacks a foreign key reference to the users table. This could lead to orphaned references if users are deleted.- uploadedBy: uuid('uploaded_by').notNull(), // user id + uploadedBy: uuid('uploaded_by').references(() => users.id, { onDelete: 'set null' }),If users can be deleted while retaining asset history, consider making this nullable with
onDelete: 'set null'.packages/platform-extensions/src/github/types.ts (1)
30-34: Consider makinglastCommitoptional for empty repositories.A newly created repository with no commits would not have a
lastCommit. This could cause runtime issues ifsyncRepositoryis called on an empty repo.export interface RepositoryState { branches: Branch[]; - lastCommit: Commit; + lastCommit?: Commit; status: string; }packages/platform-extensions/src/github/service.ts (1)
120-128: Hardcoded'main'as base branch may fail for repositories using other defaults.Consider fetching the repository's
default_branchinstead of hardcoding'main'.+ // Get the default branch from repo info + const repoInfo = await this.getRepository(owner, repoName); + const baseBranch = repoInfo.default_branch; + const response = await this.makeRequest(`/repos/${owner}/${repoName}/pulls`, { method: 'POST', body: JSON.stringify({ title, body, head: branch, - base: 'main', // or master + base: baseBranch, }), });packages/platform-extensions/package.json (1)
4-4: Placeholder version 0.0.0.The package version is set to
0.0.0, which is a common placeholder. If this package is intended for internal use only within the monorepo, this is acceptable. However, if you plan to publish it to npm, update the version to a proper semantic version (e.g.,0.1.0).packages/platform-extensions/src/universal/project-service.ts (1)
4-29: Stub implementations will throw at runtime if called.All methods throw
Error('Not implemented'). If this service is already wired into production code paths, any call will crash. Ensure these methods are not invoked until implemented, or consider returning safe defaults/empty results instead.Would you like me to open an issue to track the implementation of these methods, or should I generate placeholder implementations that return safe defaults?
packages/db/src/schema/mcp/mcp-config.ts (2)
26-26: Consider adding a foreign key constraint forsetupBy.The
setupByfield stores a user ID but lacks a foreign key reference. This could lead to orphaned references if users are deleted.🔎 Proposed fix
+import { users } from '../user'; + // Setup metadata -setupBy: uuid('setup_by').notNull(), // user id +setupBy: uuid('setup_by').references(() => users.id).notNull(),
28-29:updatedAtwon't auto-update on row modifications.The field has
defaultNow()for inserts but won't automatically update on subsequent writes. Ensure the application layer or a database trigger handles this, or use Drizzle's$onUpdateif available.apps/web/client/src/server/api/routers/assets.ts (1)
31-44: Validate optimization options.The
formatandqualityfields lack constraints. Invalid values could cause unexpected behavior or errors in the service layer.🔎 Proposed fix
optimizeAsset: protectedProcedure .input(z.object({ assetId: z.string(), options: z.object({ - format: z.string(), - quality: z.number().optional(), - width: z.number().optional(), - height: z.number().optional(), + format: z.enum(['png', 'jpg', 'jpeg', 'webp', 'avif']), + quality: z.number().min(1).max(100).optional(), + width: z.number().positive().max(8192).optional(), + height: z.number().positive().max(8192).optional(), }), }))packages/db/src/schema/figma/figma-asset.ts (1)
2-2: Unused import:text.The
texttype is imported but not used in this schema.🔎 Proposed fix
-import { pgTable, text, timestamp, uuid, varchar, integer, jsonb } from 'drizzle-orm/pg-core'; +import { pgTable, timestamp, uuid, varchar, integer, jsonb } from 'drizzle-orm/pg-core';apps/web/client/src/server/api/routers/figma.ts (1)
13-22: Consider server-side token storage instead of passing token per request.Passing the Figma access token in every API request exposes it to logging, network inspection, and request replay. Consider storing the token server-side (encrypted in DB) after initial authentication and retrieving it from the user's session.
This would change the pattern to:
// After authenticate succeeds, store token server-side // In subsequent procedures, retrieve from ctx/session rather than input .mutation(async ({ ctx }) => { const token = await getStoredFigmaToken(ctx.user.id); const service = new FigmaIntegrationService(); await service.authenticate(token); return service.importFile(input.fileId); })packages/db/src/schema/figma/figma-file.ts (1)
22-22: Consider adding a foreign key reference forimportedBy.The
importedByfield stores a user ID but lacks a foreign key constraint to the users table. This could lead to orphaned references if users are deleted.🔎 Proposed fix
+import { users } from '../user'; + // Import metadata importedAt: timestamp('imported_at', { withTimezone: true }).defaultNow().notNull(), -importedBy: uuid('imported_by').notNull(), // user id +importedBy: uuid('imported_by').references(() => users.id, { onDelete: 'set null' }),Note: Adjust the
onDeletebehavior based on your data retention requirements.packages/platform-extensions/src/figma/auth.ts (2)
54-60: Add type annotation to avoid implicitanyfromresponse.json().Per coding guidelines, avoid using
anytype. Theresponse.json()call returnsanywhich propagates through the data access.🔎 Proposed fix
+interface FigmaTokenResponse { + access_token: string; + refresh_token: string; + expires_in?: number; +} + -const data = await response.json(); +const data: FigmaTokenResponse = await response.json();
50-52: Consider including response body in error messages for better debugging.The error messages only include
statusText, but Figma's API often returns detailed error information in the response body that would aid debugging.🔎 Proposed fix
if (!response.ok) { - throw new Error(`OAuth token exchange failed: ${response.statusText}`); + const errorBody = await response.text().catch(() => ''); + throw new Error(`OAuth token exchange failed: ${response.statusText}${errorBody ? ` - ${errorBody}` : ''}`); }Also applies to: 80-82
packages/db/src/schema/comment/comment.ts (2)
27-28: Missing foreign key constraint onparentId.The
parentIdfield references another comment but lacks a foreign key constraint, which could allow invalid parent references. The comment indicates this will be "self-referenced in relations," but relations don't enforce referential integrity at the database level.🔎 Proposed fix
// Thread information -parentId: uuid('parent_id'), // for replies - will be self-referenced in relations +parentId: uuid('parent_id').references((): any => comments.id, { onDelete: 'cascade' }), threadId: uuid('thread_id'), // groups related commentsNote: The
(): any =>pattern is needed for self-referential FKs in Drizzle to avoid circular reference issues during initialization.
31-31: Consider adding foreign key reference foruserId.Similar to
importedByin figma-file.ts,userIdstores a user ID without a foreign key constraint, which could lead to orphaned references.packages/db/src/schema/mcp/mcp-server.ts (1)
22-22: Consider using a PostgreSQL enum forstatus.The
statusfield uses a varchar with valid values documented in a comment. Using a proper enum would provide database-level validation and better type inference.🔎 Proposed fix
+import { pgEnum } from 'drizzle-orm/pg-core'; + +export const mcpServerStatusEnum = pgEnum('mcp_server_status', ['running', 'stopped', 'error']); + // Status enabled: boolean('enabled').notNull().default(true), -status: varchar('status').notNull().default('stopped'), // 'running', 'stopped', 'error' +status: mcpServerStatusEnum('status').notNull().default('stopped'),packages/db/src/schema/github/github-repository.ts (2)
15-15: Consider adding unique constraint ongithubRepoIdscoped to project.A GitHub repository could potentially be connected to the same project twice without a uniqueness constraint. Consider adding a unique constraint on
(projectId, githubRepoId)combination.🔎 Proposed fix using a composite unique index
+import { uniqueIndex } from 'drizzle-orm/pg-core'; + }).enableRLS(); + +export const githubRepositoriesIndex = uniqueIndex('github_repo_project_unique') + .on(githubRepositories.projectId, githubRepositories.githubRepoId);Or inline in the table definition:
// After .enableRLS(), add: // Then create a migration to add: CREATE UNIQUE INDEX ON github_repositories(project_id, github_repo_id);
28-28: Consider adding foreign key reference forconnectedBy.Consistent with feedback on other files,
connectedBystores a user ID without FK constraint.packages/db/src/schema/figma/figma-component.ts (1)
12-12: Consider adding unique constraint onfigmaComponentIdwithin file scope.Similar to GitHub repos, a Figma component could be imported twice to the same file without a uniqueness constraint on
(figmaFileId, figmaComponentId).packages/platform-extensions/src/assets/types.ts (1)
23-28: Consider typinglineNumberasnumberinstead ofstring.Line numbers are typically numeric values. Using
stringhere may cause type inconsistencies when integrating with code analysis tools or IDEs that report line numbers as integers.Suggested fix
export interface AssetReference { filePath: string; - lineNumber?: string; + lineNumber?: number; importStatement?: string; referenceType: 'import' | 'url' | 'inline'; }packages/platform-extensions/src/figma/asset-processor.ts (3)
313-320: Replaceconsole.logwith proper logging or remove placeholder.This debug output will appear in production. Consider using a structured logger or removing this placeholder entirely since the actual storage implementation is pending.
Suggested fix
private async storeAsset(buffer: Buffer, path: string): Promise<void> { // Placeholder for storage implementation // In production, this would: // - Upload to cloud storage (S3, GCS, etc.) // - Store in local file system // - Update database records - console.log(`Storing asset to ${path}, size: ${buffer.length} bytes`); + // TODO: Implement actual storage logic }
164-200: Consider using an image processing library for robust JPEG parsing.The comment on line 165-166 acknowledges this is simplified. The current implementation may fail on edge cases (progressive JPEGs, truncated files, unusual marker ordering). For production use, libraries like
sharporimage-sizeprovide more reliable dimension extraction.
21-29: Silent error handling may mask systematic failures.While continuing on individual asset failures is reasonable, consider accumulating errors and returning them alongside successful results so callers can be informed of partial failures.
Suggested enhancement
- async processAssets(assets: FigmaAsset[]): Promise<ProcessedAsset[]> { + async processAssets(assets: FigmaAsset[]): Promise<{ processed: ProcessedAsset[]; errors: Array<{ assetId: string; error: string }> }> { const processedAssets: ProcessedAsset[] = []; + const errors: Array<{ assetId: string; error: string }> = []; for (const asset of assets) { try { const processed = await this.processAsset(asset); processedAssets.push(processed); } catch (error) { console.error(`Failed to process asset ${asset.id}:`, error); - // Continue with other assets + errors.push({ assetId: asset.id, error: String(error) }); } } - return processedAssets; + return { processed: processedAssets, errors }; }packages/platform-extensions/src/figma/service.ts (1)
159-166: Generated code usesanytype as fallback.While this is for generated component code (not the service itself), consider using a more specific fallback like
unknownor documenting unhandled Figma property types so users know what to expect.Suggested enhancement
private getTypeScriptType(figmaType: string): string { switch (figmaType) { case 'BOOLEAN': return 'boolean'; case 'TEXT': return 'string'; case 'INSTANCE_SWAP': return 'React.ReactNode'; - default: return 'any'; + default: return 'unknown'; // Unhandled Figma type } }packages/platform-extensions/src/figma/client.ts (3)
19-34: Replaceanywith proper generic type constraint orunknown.The method signature uses
anyas the default type parameter, and the request queue is typed asArray<() => Promise<any>>. Per coding guidelines, avoidanyunless necessary.Suggested fix
- private requestQueue: Array<() => Promise<any>> = []; + private requestQueue: Array<() => Promise<unknown>> = []; ... - async request<T = any>(endpoint: string, options: RequestInit = {}): Promise<T> { + async request<T = unknown>(endpoint: string, options: RequestInit = {}): Promise<T> {As per coding guidelines, avoid using the
anytype.
104-164: Add proper return types to API methods instead ofany.All API helper methods return
Promise<any>. Consider defining response interfaces for type safety, or at minimum usePromise<unknown>to enforce type checking at call sites.Example for getMe
+interface FigmaUser { + id: string; + email: string; + handle: string; + img_url: string; +} + - async getMe(): Promise<any> { + async getMe(): Promise<FigmaUser> { return this.request('/me'); }As per coding guidelines, avoid using the
anytype.
201-244: Parameter type should beunknowninstead ofany.Per coding guidelines, use
unknownfor parameters of uncertain type to enforce type checking.Suggested fix
-export function parseFigmaError(error: any): { type: FigmaErrorType; message: string } { +export function parseFigmaError(error: unknown): { type: FigmaErrorType; message: string } { if (error instanceof FigmaApiError) { // ... existing logic } - if (error.message?.includes('fetch')) { + if (error instanceof Error && error.message?.includes('fetch')) { return { type: FigmaErrorType.NETWORK_ERROR, message: 'Network error. Please check your connection.', }; } return { type: FigmaErrorType.UNKNOWN_ERROR, - message: error.message || 'An unknown error occurred.', + message: error instanceof Error ? error.message : 'An unknown error occurred.', }; }apps/web/client/src/server/api/routers/components-panel.ts (2)
70-70: Avoidz.any()fordefaultValue.Per coding guidelines, avoid using
anyunless absolutely necessary. Consider using a union type orz.unknown()with refinement.🔎 Proposed fix
- defaultValue: z.any().optional(), + defaultValue: z.union([z.string(), z.number(), z.boolean(), z.null()]).optional(),
6-12: Service instantiation pattern loses state between requests.Each procedure creates a new
ComponentsPanelServiceinstance. Since the service stores components in instance fields (this.components,this.categories), any state frominitialize()is lost after the request completes. This pattern works only because each procedure re-initializes, but it's inefficient and will cause issues if you expect state persistence (e.g., custom components added viaaddCustomComponentwon't persist).Consider either:
- Using a singleton/cached service per project
- Persisting to database instead of in-memory storage
packages/platform-extensions/src/assets/service.ts (2)
4-4: Avoidanytype forprojectAssetsmap.Per coding guidelines, avoid using
anyunless necessary. Define a properAssetinterface for type safety.+interface Asset { + id: string; + projectId: string; + name: string; + originalName: string; + type: string; + format: string; + size: number; + metadata: Record<string, unknown>; + originalPath: string; + optimizedPath?: string; + thumbnailPath?: string; + url: string; + optimizedUrl?: string; + thumbnailUrl?: string; + uploadedAt: Date; + references: AssetReference[]; + folder?: string; +} export class AssetManagementService { - private projectAssets: Map<string, any[]> = new Map(); + private projectAssets: Map<string, Asset[]> = new Map();
280-286: Remove or guardconsole.logstatements in production code.Multiple
console.logcalls throughout the file (lines 282, 293, 301, 309, 357, 361, 365) will pollute logs in production. These should be removed or replaced with a proper logging framework with appropriate log levels.packages/platform-extensions/src/components/service.ts (1)
298-304: Removeconsole.logstatements from production code.The
simulateComponentInsertionandsaveCustomComponentmethods containconsole.logcalls that should be removed or replaced with proper logging.packages/platform-extensions/src/figma/parser.ts (1)
10-15: Consider using Figma API types instead ofany.The parser uses
anyextensively for Figma document nodes. Consider importing types from@figma/rest-api-specor defining local interfaces to improve type safety and IDE support.// Example: Define minimal interfaces for Figma nodes interface FigmaNode { id: string; name: string; type: string; children?: FigmaNode[]; fills?: FigmaFill[]; strokes?: FigmaStroke[]; // ... other properties }apps/backend/supabase/migrations/0020_volatile_peter_parker.sql (1)
1-219: Consider adding indexes for frequently queried foreign keys.Tables like
assets,comments,figma_files, andgithub_repositorieswill likely be queried byproject_id. Without indexes, these queries will perform full table scans as data grows.CREATE INDEX idx_assets_project_id ON assets(project_id); CREATE INDEX idx_comments_project_id ON comments(project_id); CREATE INDEX idx_comments_element_id ON comments(element_id); CREATE INDEX idx_figma_files_project_id ON figma_files(project_id); CREATE INDEX idx_github_repositories_project_id ON github_repositories(project_id); CREATE INDEX idx_mcp_configs_project_id ON mcp_configs(project_id);
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 8236 🏁 Script executed: Length of output: 5516 🏁 Script executed: Length of output: 4185 🏁 Script executed: Length of output: 4088 🏁 Script executed: Length of output: 897 @DevFlex-AI, I've analyzed your PR and found one critical issue that will prevent maintainers from building, along with several other concerns: 🚨 Blocking Issue - Build Will FailZod Version Problem: In Fix required: "zod": "^3.23.0"
|
|
are you able to change the code to fix or do i have to make make a new PR with the fixed code @coderabbitai and also what should the zod version be? |
|
Tip For best results, initiate chat on the files or code changes. @DevFlex-AI, I'm not able to directly commit code changes to your repository – you'll need to make the changes yourself. But the good news is you don't need a new PR! You can simply push fixes to this same PR branch. 🔧 The FixZod version should be: The latest stable version of Zod is in the 3.23.x range. Here's what you need to change: File: Change line 24 from: "zod": "^4.1.3"To: "zod": "^3.23.0"📝 How to Apply the Fix
This will update the same PR automatically – no need to create a new one! 🎉 Optional but recommended: While you're at it, you might want to remove git rm DONT-CLICK-ON-THIS.HTML
git commit -m "remove joke HTML file"
git pushOnce you push these fixes, the build should work for the maintainers! Let me know if you need any other help. 👍 |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
| await service.authenticate(input.token); | ||
| return service.extractDesignTokens(input.fileId); | ||
| }), | ||
| }); No newline at end of file |
There was a problem hiding this comment.
The file is missing a newline at the end. Consider adding a newline to comply with common coding conventions.
bun.lock
Outdated
| "@onlook/eslint": "*", | ||
| }, | ||
| }, | ||
| <<<<<<< HEAD |
There was a problem hiding this comment.
Unresolved merge conflict marker <<<<<<< HEAD found. Please resolve this conflict before merging the PR.
bun.lock
Outdated
| }, | ||
| }, | ||
| <<<<<<< HEAD | ||
| ======= |
There was a problem hiding this comment.
Unresolved merge conflict marker '=======' found. Please resolve this conflict before merging the PR.
bun.lock
Outdated
| "typescript": "^5.5.4", | ||
| }, | ||
| }, | ||
| >>>>>>> 08a41a9ca19909effa8d5e6b459122bf03547e73 |
There was a problem hiding this comment.
It appears that a merge conflict marker is still present (>>>>>>> 08a41a9ca19909effa8d5e6b459122bf03547e73). Please remove this marker before merging.
bun.lock
Outdated
|
|
||
| "@neoconfetti/react": ["@neoconfetti/[email protected]", "", {}, "sha512-klcSooChXXOzIm+SE5IISIAn3bYzYfPjbX7D7HoqZL84oAfgREeSg5vSIaSFH+DaGzzvImTyWe1OyrJ67vik4A=="], | ||
|
|
||
| <<<<<<< HEAD |
There was a problem hiding this comment.
Unresolved merge conflict marker found (<<<<<<< HEAD). Please resolve the conflict and remove the merge marker.
bun.lock
Outdated
| "@onlook/docs/eslint/strip-ansi/ansi-regex": ["[email protected]", "", {}, "sha512-quJQXlTSUGL2LH9SUXo8VwsY4soanhgo6LNSm84E1LBcE8s3O0wpdiRzyR9z/ZZJMlMWv37qOOb9pdJlMUEKFQ=="], | ||
|
|
||
| <<<<<<< HEAD | ||
| ======= |
There was a problem hiding this comment.
This conflict marker (=======) should be removed after resolving the merge conflict.
bun.lock
Outdated
|
|
||
| "@onlook/web-client/next/sharp/semver": ["[email protected]", "", { "bin": { "semver": "bin/semver.js" } }, "sha512-SdsKMrI9TdgjdweUSR9MweHA4EJ8YxHn8DFaDisvhVlUOe4BF1tLD7GAj0lIqWVl+dPb/rExr0Btby5loQm20Q=="], | ||
|
|
||
| >>>>>>> 08a41a9ca19909effa8d5e6b459122bf03547e73 |
There was a problem hiding this comment.
There appears to be a merge conflict marker left over: >>>>>>> 08a41a9ca19909effa8d5e6b459122bf03547e73. Please remove this marker to avoid build issues.
bun.lock
Outdated
|
|
||
| "@react-email/preview-server/tailwindcss/chokidar/readdirp": ["[email protected]", "", { "dependencies": { "picomatch": "^2.2.1" } }, "sha512-hOS089on8RduqdbhvQ5Z37A0ESjsqz6qnRcffsMU3495FuTdqSm+7bhJ29JvIOsBDEEnan5DPu9t3To9VRlMzA=="], | ||
|
|
||
| <<<<<<< HEAD |
There was a problem hiding this comment.
Unresolved merge conflict marker found ('<<<<<<< HEAD'). Please resolve this conflict and remove all merge markers before merging.
bun.lock
Outdated
|
|
||
| "expo/pretty-format/@jest/schemas/@sinclair/typebox": ["@sinclair/[email protected]", "", {}, "sha512-+Fj43pSMwJs4KRrH/938Uf+uAELIgVBmQzg/q1YG10djyfA3TnrU8N8XzqCh/okZdszqBQTZf96idMfE5lnwTA=="], | ||
|
|
||
| <<<<<<< HEAD |
There was a problem hiding this comment.
Typo/lexical issue: Unresolved merge conflict marker <<<<<<< HEAD detected (and correspondingly, ======= on the following line). Please remove these markers and resolve the conflict before merging.
bun.lock
Outdated
|
|
||
| "fumadocs-ui/next/sharp/semver": ["[email protected]", "", { "bin": { "semver": "bin/semver.js" } }, "sha512-SdsKMrI9TdgjdweUSR9MweHA4EJ8YxHn8DFaDisvhVlUOe4BF1tLD7GAj0lIqWVl+dPb/rExr0Btby5loQm20Q=="], | ||
|
|
||
| >>>>>>> 08a41a9ca19909effa8d5e6b459122bf03547e73 |
There was a problem hiding this comment.
There's a leftover merge conflict marker (>>>>>>> 08a41a9ca19909effa8d5e6b459122bf03547e73) in the file. This should be removed before merging.
|
@coderabbitai o commti some change and can you tell your freind @ellipsis-dev to chill with the amount of conversation he is making |
|
Got it! I'll keep my comments concise going forward. 👍 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 961 @DevFlex-AI, awesome work! 🎉 I can see you've pushed both fixes: ✅ Changes Applied Successfully
The blocking build issue is now resolved! Maintainers should be able to build the project without any problems. 🚀 About @ellipsis-devRegarding @ellipsis-dev – I appreciate the heads up, but I'm not able to control or communicate with other bots. Each tool operates independently. If the conversation volume from other bots is becoming an issue, you could:
Next StepsYour PR is now in much better shape! The main things remaining are:
Great job getting these fixes in quickly! 💪 |
|
@ellipsis-dev , @coderabbitai i have the same question for both if i added all the unselected features in readme would i get added to the orginizatuon on github becuase i addded there missing festures? |
|
Hi @DevFlex-AI! Thanks for your interest in contributing to Onlook. Questions about organization membership and contribution policies are best directed to the project maintainers. I'd recommend:
The maintainers will be able to give you the most accurate information about how contributions are recognized and what qualifies for organization membership. Great work on adding these features! |
|
Tip For best results, initiate chat on the files or code changes. @DevFlex-AI, that's a great question, but unfortunately I'm not able to make decisions about organization membership – that's entirely up to the repository maintainers and owners. 🙏 What I Can Tell YouYour contribution is substantial! You've added:
This is definitely meaningful work that adds value to the project! 🌟 What You Should DoTo find out about organization membership, I recommend:
Important NoteDifferent projects have different policies:
Only the maintainers can tell you what applies to this specific project! Keep up the great work, and good luck! 🚀 |
|
@ellipsis-dev can you just tell me or if not i will or not? |
|
PR #3077 Summary Additions:
Critical Issue:
Fixes Applied:
Current Status:
Organization Membership Question:
|
|
@ellipsis-dev who is the mainter that ca help me with this question? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2573 @DevFlex-AI, based on the repository information, here are the maintainers who can help you: 🎯 Primary MaintainerKiet (GitHub: @Kitenite)
👥 Other Active Maintainers
📬 How to ContactI recommend reaching out through these channels (in order of preference for your question):
💡 My SuggestionSince you've made a substantial contribution, I'd recommend:
The README states they're "actively looking for contributors," which is a good sign! 🌟 Good luck, and thanks for your contribution to the project! 🚀 |
|
Tip For best results, initiate chat on the files or code changes. @DevFlex-AI, I need to clarify something important and give you constructive guidance: 🔍 What My Review Actually SaidYes, I confirmed that your PR:
BUT - I also noted several important caveats:
❗ The Key Distinction"Buildable" ≠ "Mergeable"Buildable means:
Mergeable means:
Your PR passes the first test but fails the second. 📝 The Real Issue (From Kitenite)
This is the problem:
Even perfect code becomes unmergeable if the PR is too large. 💡 About the MD File SuggestionYES - Creating a detailed breakdown document would be VERY helpful! 📄 Here's what to include: Suggested Structure:# PR #3077 - Platform Extensions Breakdown
## Summary
This PR adds multiple platform integrations and features to Onlook.
## Features Added (Can be split into separate PRs)
### 1. Database Schema Extensions
- **Files**: `apps/backend/supabase/migrations/0020_*.sql`, `0021_*.sql`
- **What it does**: Adds tables for assets, comments, figma_files, github_repositories, mcp_configs
- **Dependencies**: None
- **Lines changed**: ~500
- **Standalone?**: Yes ✓
### 2. Figma Integration
- **Files**: `packages/platform-extensions/src/figma/*`, `apps/web/client/src/server/api/routers/figma.ts`
- **What it does**: OAuth, file import, asset extraction, component conversion
- **Dependencies**: Figma API credentials, database schema
- **Lines changed**: ~800
- **Standalone?**: Yes ✓
### 3. GitHub Integration
- **Files**: `packages/platform-extensions/src/github/*`, router files
- **What it does**: Repo management, PR automation
- **Dependencies**: GitHub API token, database schema
- **Lines changed**: ~400
- **Standalone?**: Yes ✓
### 4. Asset Management
- **Files**: `packages/platform-extensions/src/assets/*`, router + schemas
- **What it does**: Upload, optimize, organize assets
- **Dependencies**: Database schema
- **Lines changed**: ~600
- **Standalone?**: Yes ✓
### 5. Comments System
- **Files**: `packages/platform-extensions/src/comments/*`, router + schemas
- **What it does**: Inline comments with mentions, threading, resolution
- **Dependencies**: Database schema
- **Lines changed**: ~400
- **Standalone?**: Yes ✓
### 6. Components Panel
- **Files**: `packages/platform-extensions/src/components/*`, router
- **What it does**: Drag-drop component library with search/filter
- **Dependencies**: None
- **Lines changed**: ~500
- **Standalone?**: Yes ✓
### 7. MCP Integration
- **Files**: `packages/platform-extensions/src/mcp/*`
- **What it does**: Model Context Protocol server management
- **Dependencies**: Database schema
- **Lines changed**: ~200
- **Status**: Stub implementation (TODO items)
- **Standalone?**: Yes ✓
### 8. Universal Services (Incomplete)
- **Files**: `packages/platform-extensions/src/universal/*`
- **Status**: Placeholder methods (9 TODOs)
- **Recommendation**: Complete in separate PR after core features
## Recommended Split Strategy
**Phase 1 - Foundation:**
1. PR: Database schema + migrations
**Phase 2 - Core Features (pick one):**
2. PR: Figma integration (most complete)
3. PR: Asset management (no external deps)
4. PR: Comments system (no external deps)
**Phase 3 - Additional Features:**
5. PR: GitHub integration
6. PR: Components panel
7. PR: MCP integration (complete TODOs first)
**Phase 4 - Future:**
8. PR: Universal services (implement fully)
## Testing Notes
- All features tested locally
- Build passes after zod version fix
- Deployment checks need attention (Vercel)
## Questions for Maintainers
1. Which feature should I submit first?
2. Do you want tests included with each PR?
3. Any specific coding standards to follow?🎯 How to ProceedStep 1: Create the MD File
Step 2: Reach Out to @drfarrellPost a comment like this:
Step 3: Wait for Response
🎓 Learning OpportunityThis is valuable feedback on how open source works:
Smaller PRs get merged faster because they're:
🚀 Bottom LineYour work is valuable and your code is solid. You just need to present it in digestible chunks. Creating that MD file will show @drfarrell:
This is how you turn a "too large" PR into multiple successful merges! 💪 Would you like help drafting the message to @drfarrell or structuring the MD file? |
PR #3077 – Platform Extensions Mega-PR BreakdownThis document explains everything bundled into PR #3077 (“Adding missing features that are not selected on the readme.md”), why it is currently difficult to review, and how it can be split into smaller, reviewer-friendly PRs per @Kitenite’s guidance. 1. High-Level SummaryThe PR attempts to add seven major feature areas plus supporting database migrations and routers in a single change set (~13k additions, 66 files touched). Each feature is valuable, but reviewing all of them at once is burdensome. The goal of this document is to identify clear seams so we can submit focused PRs that are easier to test and merge.
Supporting updates:
2. Recommended Split StrategyTo align with maintainer feedback (“break features out into smaller, separate PRs”), the work can be decomposed as follows. Each PR is self-contained, builds on previously merged pieces, and should be reviewable in ≤30 minutes.
Each follow-up PR can reference this summary for context and explicitly state dependencies (e.g., “requires migrations from PR X”). 3. Current Status / Outstanding Items
4. Proposed Next Steps
5. Maintainer FAQQ: Why not keep everything in one PR? Q: Are there hidden blockers? Q: Do features depend on each other? Q: What about production readiness? @coderabbitai is this md file good enough? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 7044 @DevFlex-AI, YES! This MD file is excellent! 🎉 ✅ What You Did Really Well1. Structure & Organization
2. Addresses Maintainer Concerns
3. Technical Completeness
4. Actionable & Professional
💡 Minor Suggestions (Optional Improvements)1. Sign-off LineCurrent: Prepared by: DevFlex-AI (Cascade assistant)Suggested: Prepared by: @DevFlex-AI(The "Cascade assistant" reference might be confusing) 2. Add LOC Estimates (Optional)Consider adding estimated lines of code for each split PR to show they're manageable: 1. **Foundation – Database Schema & Policies** (~500 LOC)
2. **Figma Integration** (~800 LOC)
3. **Asset Management Service** (~600 LOC)
...3. Highlight Zero-Dependency Features (Optional)Make it clearer which PRs could go first: **Can merge independently (no dependencies):**
- Components Panel ✓
- MCP Integration (with tests) ✓
**Requires schema PR first:**
- All others🎯 Bottom LineThis document is ready to share! 🚀 It's comprehensive, professional, and directly addresses the concerns raised. You've shown:
📝 Next Step: Share with @drfarrellPost this comment on your PR:
🌟 Excellent Work!You've turned what could have been a defensive response into a professional, collaborative document that shows maturity and understanding of the open source process. This significantly increases your chances of successful collaboration with the maintainers. Go ahead and share it with confidence! 💪 |
|
Tip For best results, initiate chat on the files or code changes. @DevFlex-AI, YES! This MD file is excellent and ready to share! 🎉 ✅ What Makes This Document Strong1. Clear Problem Statement
2. Professional Summary Table
3. Actionable Split Strategy
4. Transparency on Status
5. Maintainer FAQ
💡 Very Minor Polish (100% Optional)1. Consider adding LOC estimates per PR: 1. **Foundation – Database Schema & Policies** (~500 LOC)
2. **Figma Integration** (~800 LOC)
3. **Asset Management Service** (~600 LOC)This reinforces that each split PR is manageable. 2. Highlight zero-dependency features: **Can merge independently (after schema):**
- Components Panel ✓
- MCP Integration ✓Shows flexibility in merge order. But honestly? These are nice-to-haves. The document is ready as-is. 🚀 Next Step: Share with @drfarrellPost this on your PR:
🌟 Why This Document WorksYou've successfully demonstrated:
This is exactly how successful open source contributors operate. You've turned potential rejection into a structured collaboration plan. Go ahead and share it with confidence! 🚀 |
|
Hi @drfarrell — per Kitenite’s feedback I put together a full breakdown of this mega-PR (see PR_SUMMARY.md in the repo). It lists every feature that’s bundled here, what’s complete vs. stubbed, and an exact plan for splitting the work into smaller PRs (schema, Figma, assets, comments, components, GitHub, MCP, etc.). CodeRabbit confirms the branch now builds cleanly (zod fix landed, all major warnings addressed), but I understand the scope is too large to review in one go. Could you let me know which feature area you’d like me to submit first so I can open a focused PR right away? I’m happy to close this umbrella PR and re-submit in the order you prefer. Thanks! |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
PR_SUMMARY.mdpackages/platform-extensions/src/figma/service.tspackages/platform-extensions/src/mcp/__tests__/mcp.service.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
packages/platform-extensions/src/mcp/__tests__/mcp.service.test.tspackages/platform-extensions/src/figma/service.ts
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
packages/platform-extensions/src/mcp/__tests__/mcp.service.test.tspackages/platform-extensions/src/figma/service.ts
🧠 Learnings (2)
📚 Learning: 2025-09-16T19:22:52.461Z
Learnt from: CR
Repo: onlook-dev/onlook PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T19:22:52.461Z
Learning: Applies to {apps,packages}/**/*.{ts,tsx} : Avoid using the any type unless absolutely necessary
Applied to files:
packages/platform-extensions/src/figma/service.ts
📚 Learning: 2025-09-14T01:44:21.209Z
Learnt from: CR
Repo: onlook-dev/onlook PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.209Z
Learning: Applies to **/*.{ts,tsx} : Do not use the any type unless necessary
Applied to files:
packages/platform-extensions/src/figma/service.ts
🧬 Code graph analysis (2)
packages/platform-extensions/src/mcp/__tests__/mcp.service.test.ts (1)
packages/platform-extensions/src/mcp/service.ts (1)
MCPIntegrationService(13-118)
packages/platform-extensions/src/figma/service.ts (1)
packages/types/src/design-tokens/index.ts (1)
DesignToken(8-8)
🪛 LanguageTool
PR_SUMMARY.md
[uncategorized] ~13-~13: The official name of this software platform is spelled with a capital “H”.
Context: ...ase schema | Adds assets/comments/figma/github/mcp tables, RLS policies, FK fixes | `a...
(GITHUB)
[uncategorized] ~15-~15: The official name of this software platform is spelled with a capital “H”.
Context: ...Repo management, PR/branch automation | packages/platform-extensions/src/github/*, router wiring | ✅ Complete | | Asse...
(GITHUB)
[uncategorized] ~55-~55: The official name of this software platform is spelled with a capital “H”.
Context: ...itHub Integration Service** * Files: packages/platform-extensions/src/github/*, router wiring. * Repo connection...
(GITHUB)
🔇 Additional comments (2)
packages/platform-extensions/src/mcp/__tests__/mcp.service.test.ts (1)
1-58: LGTM! Well-structured test suite.The test suite provides comprehensive coverage of the MCPIntegrationService:
- Setup and tool exposure validation
- Successful tool execution with metadata verification
- Error handling for unknown tools
- Server restart tracking
The tests are clear, focused, and follow best practices with proper isolation via
beforeEach.packages/platform-extensions/src/figma/service.ts (1)
1-226: LGTM! All previously identified issues have been addressed.Verified that the following corrections from earlier reviews are now in place:
- Line 53: Correctly passes
fileData.documenttoextractDesignTokens- Line 164: Uses
'unknown'instead of'any'as the TypeScript type fallback- Line 174: Correctly accesses
style.propertyandstyle.value(matching theStyleDefinitioninterface)The service implementation is well-structured with proper error handling, type safety, and follows coding guidelines.
|
|
||
| | Feature Area | Description | Key Paths | Status | | ||
| |--------------|-------------|-----------|--------| | ||
| | Database schema | Adds assets/comments/figma/github/mcp tables, RLS policies, FK fixes | `apps/backend/supabase/migrations/0020_*`, `0021_*` | ✅ Complete | |
There was a problem hiding this comment.
Capitalize "GitHub" consistently.
The official platform name is "GitHub" with a capital "H". Update instances on lines 13, 15, and 55 for consistency with the brand name.
🔎 Proposed fix
-| Database schema | Adds assets/comments/figma/github/mcp tables, RLS policies, FK fixes | `apps/backend/supabase/migrations/0020_*`, `0021_*` | ✅ Complete |
+| Database schema | Adds assets/comments/figma/GitHub/mcp tables, RLS policies, FK fixes | `apps/backend/supabase/migrations/0020_*`, `0021_*` | ✅ Complete |-| GitHub integration | Repo management, PR/branch automation | `packages/platform-extensions/src/github/*`, router wiring | ✅ Complete |
+| GitHub integration | Repo management, PR/branch automation | `packages/platform-extensions/src/GitHub/*`, router wiring | ✅ Complete |Wait, the path should remain lowercase (filesystem paths are case-sensitive). Only fix the user-facing text:
-| Database schema | Adds assets/comments/figma/github/mcp tables, RLS policies, FK fixes | `apps/backend/supabase/migrations/0020_*`, `0021_*` | ✅ Complete |
+| Database schema | Adds assets/comments/figma/GitHub/mcp tables, RLS policies, FK fixes | `apps/backend/supabase/migrations/0020_*`, `0021_*` | ✅ Complete |Line 55:
-6. **GitHub Integration Service**
- * Files: `packages/platform-extensions/src/github/*`, router wiring.
+6. **GitHub Integration Service**
+ * Files: `packages/platform-extensions/src/github/*`, router wiring.Actually, line 55 already says "GitHub Integration Service" correctly. Only lines 13 and 15 need the table name "github" changed to "GitHub" in the description column (not in file paths).
Also applies to: 15-15, 55-55
🧰 Tools
🪛 LanguageTool
[uncategorized] ~13-~13: The official name of this software platform is spelled with a capital “H”.
Context: ...ase schema | Adds assets/comments/figma/github/mcp tables, RLS policies, FK fixes | `a...
(GITHUB)
🤖 Prompt for AI Agents
In PR_SUMMARY.md around lines 13, 15, and 55, update the user-facing occurrences
of "github" to the correctly capitalized "GitHub" in the description column on
lines 13 and 15 (leave any filesystem paths or filenames lowercase); do not
change line 55 because it already reads "GitHub" correctly. Ensure only the
display text is modified and file/path casing remains unchanged.
|
@Kitenite the new md file does it fix the probelm that it explains everythign that is being changed? |
|
@coderabbitai with the .md hwo much likely would it make it so that this will pr will pass out of 100 |
|
Tip For best results, initiate chat on the files or code changes. @DevFlex-AI, I need to give you an honest, realistic assessment: 📊 Probability This PR Merges AS-IS: 10-15%Let me break down why, and what the MD file actually accomplishes. ❌ Why the Odds Are Low (Even With the MD File)The Core Problem Hasn't ChangedThe MD file is excellent documentation, but:
Kitenite was explicit:
The MD file explains WHAT you did, but it doesn't solve WHY the PR is problematic (review burden). What Maintainers Actually WantThey want you to physically split the PR, not just document it. The MD file is a roadmap for doing that, not a substitute for it. Think of it this way:
✅ What the MD File DOES AccomplishThe MD file significantly improves your chances if you follow through on the split: With the MD File + Split Strategy:
Why much higher?
🎯 Realistic ScenariosScenario A: Keep Mega-PR As-Is (10-15% chance)
Scenario B: Split Into Focused PRs (70-80% chance per PR)
Scenario C: Hybrid Approach (40-50% chance)
💡 The Reality CheckThe MD file is NOT a workaround for splitting. It's a tool to facilitate splitting. What it proves:
What it doesn't change:
🚀 What You Should Do NextOption 1: Best Path (Highest Success Rate)
Success probability: 70-80% per PR ✅ Option 2: Wait for @drfarrell (Current Path)
Success probability: 60-70% (depends on response time and guidance) Option 3: Keep As-Is (Not Recommended)
Success probability: 10-15% ❌ 🎓 The LessonIn open source:
Your MD file is great prep work. Now use it as the blueprint to actually split the PR, and you'll see much better results. ✨ Bottom LineThe MD file moves you from:
But it doesn't move you to:
Your real next step: Wait for @drfarrell's response (you've already messaged him), then actually create the smaller PRs based on his priorities. Probability of eventual success if you split properly: 70-80% 🚀 The MD file was a smart move. Now follow through on the split strategy, and your chances jump dramatically! 💪 |
|
@coderabbitai is they a very fast way to do this with terminal commands(just kidding i already know you cant) or lieka fats way for me to make multiple prs? |
|
Tip For best results, initiate chat on the files or code changes. @DevFlex-AI, YES! There's definitely a faster way! And you're thinking smart by asking. 🚀 I'll give you a practical Git workflow to efficiently split your mega-PR into multiple focused PRs. Here's the strategy: 🎯 The Fast Split StrategyStep 1: Wait for @drfarrell's Priority ⏳Before splitting everything, wait for Daniel to tell you which feature he wants first. No point creating 7 PRs if he only wants to review one at a time. Step 2: Create Focused Branches (When Ready)Here's the efficient workflow using Git commands: For each feature you want to split out: # 1. Start from main and create a new branch for the feature
git checkout main
git pull origin main
git checkout -b feature/database-schema
# 2. Cherry-pick ONLY the commits related to that feature
# (Find the commit SHAs from your PR branch)
git log main..your-pr-branch --oneline # See all commits
git cherry-pick <commit-sha-1> <commit-sha-2> ...
# 3. Push the focused branch
git push origin feature/database-schema
# 4. Create PR from GitHub CLI or web UI
gh pr create --title "feat: Add database schema for platform extensions" \
--body "Part 1 of #3077. Adds database migrations and schema for assets, comments, figma, github, mcp tables. See [PR_SUMMARY.md](link) for full context."Step 3: Alternative - File-Based Approach (If commits aren't cleanly separated)If your commits mix multiple features, use # 1. Create new branch from main
git checkout main
git pull origin main
git checkout -b feature/database-schema
# 2. Checkout specific files from your mega-PR branch
git checkout your-pr-branch -- apps/backend/supabase/migrations/0020_*.sql
git checkout your-pr-branch -- apps/backend/supabase/migrations/0021_*.sql
git checkout your-pr-branch -- apps/backend/supabase/migrations/meta/_journal.json
git checkout your-pr-branch -- packages/db/src/schema/asset/
git checkout your-pr-branch -- packages/db/src/schema/comment/
git checkout your-pr-branch -- packages/db/src/schema/figma/
git checkout your-pr-branch -- packages/db/src/schema/github/
git checkout your-pr-branch -- packages/db/src/schema/mcp/
git checkout your-pr-branch -- packages/db/src/schema/index.ts
# 3. Commit and push
git add .
git commit -m "feat: Add database schema and migrations for platform extensions"
git push origin feature/database-schema
# 4. Create PR
gh pr create --title "feat: Database schema for platform extensions" \
--body "Foundation PR for #3077. Adds tables for assets, comments, figma, github, mcp with RLS policies."📋 Recommended Split Order (Based on PR_SUMMARY.md)After @drfarrell responds, create branches in this order: # PR 1: Database foundation
feature/database-schema
# PR 2: Asset management (self-contained)
feature/asset-management
# PR 3: Comments system (self-contained)
feature/comments-system
# PR 4: Components panel (self-contained)
feature/components-panel
# PR 5: Figma integration (requires env vars)
feature/figma-integration
# PR 6: GitHub integration
feature/github-integration
# PR 7: MCP integration
feature/mcp-integration
# PR 8: Universal services (implement stubs first)
feature/universal-services⚡ Pro Tips for SpeedUse a Script to Automate:Create #!/bin/bash
FEATURE=$1
FILES=$2
git checkout main
git pull origin main
git checkout -b "feature/$FEATURE"
git checkout your-mega-pr-branch -- $FILES
git add .
git commit -m "feat: Add $FEATURE"
git push origin "feature/$FEATURE"
echo "Branch created! Now create PR at:"
echo "https://github.com/onlook-dev/onlook/compare/feature/$FEATURE"Usage: ./split-pr.sh "database-schema" "apps/backend/supabase/migrations/* packages/db/src/schema/asset/* packages/db/src/schema/comment/*"GitHub CLI for Fast PR Creation:# Install GitHub CLI if needed
brew install gh # or see: https://cli.github.com/
# Authenticate
gh auth login
# Create PR with one command
gh pr create --title "feat: Database schema" \
--body "Part of #3077. See PR_SUMMARY.md" \
--label "enhancement" \
--assignee DevFlex-AIUse GitHub's Web UI Split Feature:
🎯 Recommended Immediate ActionRight now, while waiting for @drfarrell: # 1. Create a backup branch (just in case)
git checkout your-pr-branch
git checkout -b backup/mega-pr-3077
git push origin backup/mega-pr-3077
# 2. Prepare the first PR (database schema) so it's ready to go
git checkout main
git pull origin main
git checkout -b feature/database-schema
# Copy schema files
git checkout your-pr-branch -- apps/backend/supabase/migrations/
git checkout your-pr-branch -- packages/db/src/schema/
# Commit
git add .
git commit -m "feat: Add database schema and migrations for platform extensions
Adds tables for:
- assets & asset_references
- comments & comment_mentions
- figma_files, figma_assets, figma_components
- github_repositories, github_integrations
- mcp_configs, mcp_servers
Includes RLS policies and FK cascade fixes.
Part 1 of #3077. See PR_SUMMARY.md for full context."
git push origin feature/database-schemaThen wait for Daniel's response before creating the PR. ⏱️ Time EstimateOnce you have the workflow down:
Much faster than review waiting time! 🚀 🎯 Next StepWait for @drfarrell's response to your message, then use these commands to quickly create the first focused PR. Once he confirms priority, you can knock out the splits very quickly. You've got this! 💪 |
|
@coderabbitai can you check https://github.com/smack-sh/smack-sh for any coorupt code |
|
Tip For best results, initiate chat on the files or code changes. @DevFlex-AI, I need to politely decline that request and clarify my scope. 🙏 🚫 Why I Can't Review That Repository1. Outside Current Context
2. Security & Privacy
3. Scope Limitations
💡 If You Need to Check That RepositoryFor general code quality:
For security concerns:
For malicious code:
🎯 Let's RefocusYou still have PR #3077 to finalize! You've:
Next step: Wait for @drfarrell's response about which feature to split out first, then use the Git workflow I provided earlier to create focused PRs. Is there anything about your current PR that you'd like help with? 🚀 |
|
@coderabbitai i have been waiting a weke now how much more |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 182188 @DevFlex-AI It’s been 6 days (Dec 24 → Dec 30). Given the holidays, a realistic window is another 1–2 weeks. If you don’t hear back by Monday, January 6, 2026, post a polite ping to @drfarrell; if still quiet by January 13, follow up again and/or open a short Discussion and drop a note in their Discord. Suggested ping: To keep momentum, you can go ahead and open the first small PR (database schema + policies) now, referencing PR_SUMMARY.md. After you push it, tag me with “@coderabbitai review” and I’ll re-run a focused review. If you’d like, I can prep a ready-to-copy bash snippet to scaffold PR #1 for you—just say “generate the split script.” |
|
@coderabbitai It has been over a month now is there evena. chance this could be approved or what |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 197986
Straight answer: approval of this mega‑PR as‑is is unlikely (~10–15%). The quickest way to get movement is to split it. Start with a small, self‑contained PR for “Foundation – Database schema & policies,” then follow with feature PRs. Fast path to PR
Example commands: # From your local clone
git checkout main && git pull
git checkout -b feature/db-schema
# Bring in only the schema/migration files from your mega-PR branch
git checkout <your-mega-pr-branch> -- \
apps/backend/supabase/migrations/ \
packages/db/src/schema/asset/ \
packages/db/src/schema/comment/ \
packages/db/src/schema/figma/ \
packages/db/src/schema/github/ \
packages/db/src/schema/mcp/ \
packages/db/src/schema/index.ts \
packages/db/src/schema/project/project.ts
git add .
git commit -m "feat(db): add platform extension schema + RLS + migrations (part of `#3077`)"
git push -u origin feature/db-schema
# Create the focused PR (GitHub CLI)
gh pr create --title "feat(db): platform extension schema + migrations" \
--body "Part of `#3077`. Foundation-only: tables, RLS, FKs, journal. See PR_SUMMARY.md for context."Then post a concise ping to the maintainer: If you want, I can generate a tailored split script for all follow‑up PRs (assets, comments, components, figma, github, mcp) so you can create them in minutes—just say “generate the split script.” |
|
Здравствуйте,скажите о каком скрипте идёт речь и каком его разделении?спа...
сб, 24 янв. 2026 г., 08:13 coderabbitai[bot] ***@***.***>:
… *coderabbitai[bot]* left a comment (onlook-dev/onlook#3077)
<#3077 (comment)>
Tip
For best results, initiate chat on the files or code changes.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== PR `#3077` Current Status ==="
gh pr view 3077 --repo onlook-dev/onlook --json number,title,state,createdAt,updatedAt,author,comments,isDraft,mergeable
echo ""
echo "=== All Comments Since December 24, 2025 ==="
gh pr view 3077 --repo onlook-dev/onlook --json comments --jq '.comments[] | select(.createdAt >= "2025-12-24") | "\(.author.login) at \(.createdAt):\n\(.body[0:300])\n---"'
echo ""
echo "=== Check ***@***.***` Recent Activity (last 30 days) ==="
gh api repos/onlook-dev/onlook/commits?since=2025-12-24T00:00:00Z --jq '.[] | select(.commit.author.name | contains("Farrell") or contains("Daniel")) | "\(.commit.author.date): \(.commit.message | split("\n")[0])"' | head -10
echo ""
echo "=== Recent Repository Commits (to check if repo is active) ==="
gh api repos/onlook-dev/onlook/commits?per_page=15 --jq '.[] | "\(.commit.author.date) - \(.commit.author.name): \(.commit.message | split("\n")[0])"'
echo ""
echo "=== Check Recent PR Merges (are other PRs getting reviewed?) ==="
gh pr list --repo onlook-dev/onlook --state merged --limit 10 --json number,title,mergedAt,author --jq '.[] | "PR #\(.number) by \(.author.login): merged \(.mergedAt)"'
echo ""
echo "=== Open PRs Waiting for Review ==="
gh pr list --repo onlook-dev/onlook --state open --limit 15 --json number,title,createdAt,author,comments --jq '.[] | "PR #\(.number) by \(.author.login): created \(.createdAt), \(.comments | length) comments"'
echo ""
echo "=== Check if there are any maintainer responses on OTHER recent PRs ==="
gh pr list --repo onlook-dev/onlook --state open --limit 5 --json number,comments --jq '.[] | "PR #\(.number) latest comments:", (.comments[-3:] | .[] | " \(.author.login) at \(.createdAt)")'
Length of output: 197986
------------------------------
@DevFlex-AI Thanks for sticking with it. As of January 24, 2026 (about 31
days since December 24, 2025), PR #3077 is still OPEN and marked
MERGEABLE. Maintainers are active (e.g., PR #3084 merged on January 21,
2026), but this PR hasn’t been reviewed because of its size.
Straight answer: approval of this mega‑PR as‑is is unlikely (~10–15%). The
quickest way to get movement is to split it. Start with a small,
self‑contained PR for “Foundation – Database schema & policies,” then
follow with feature PRs.
Fast path to PR #1 (run locally):
- Branch from main and stage only schema/migrations
- Push and open a focused PR referencing #3077 and PR_SUMMARY.md
Example commands:
# From your local clone
git checkout main && git pull
git checkout -b feature/db-schema
# Bring in only the schema/migration files from your mega-PR branch
git checkout <your-mega-pr-branch> -- \
apps/backend/supabase/migrations/ \
packages/db/src/schema/asset/ \
packages/db/src/schema/comment/ \
packages/db/src/schema/figma/ \
packages/db/src/schema/github/ \
packages/db/src/schema/mcp/ \
packages/db/src/schema/index.ts \
packages/db/src/schema/project/project.ts
git add .
git commit -m "feat(db): add platform extension schema + RLS + migrations (part of `#3077`)"
git push -u origin feature/db-schema
# Create the focused PR (GitHub CLI)
gh pr create --title "feat(db): platform extension schema + migrations" \
--body "Part of `#3077`. Foundation-only: tables, RLS, FKs, journal. See PR_SUMMARY.md for context."
Then post a concise ping to the maintainer:
***@***.*** <https://github.com/drfarrell> friendly nudge: I’ve opened a
small ‘DB schema + policies’ PR that’s independent of other features. If
that lands, I’ll follow with assets/comments/components, etc. in similarly
small PRs. Does that order work for you?”
If you want, I can generate a tailored split script for all follow‑up PRs
(assets, comments, components, figma, github, mcp) so you can create them
in minutes—just say “generate the split script.”
—
Reply to this email directly, view it on GitHub
<#3077 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AXA63AWRM3ETMJJCDNGL3LT4ILPL5AVCNFSM6AAAAACP45IUYGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTOOJTGYZTINZTGE>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
|
Приветствую,что ты имеешь ввиду?
сб, 24 янв. 2026 г., 08:11 Ahmad ***@***.***>:
… *DevFlex-AI* left a comment (onlook-dev/onlook#3077)
<#3077 (comment)>
@coderabbitai <https://github.com/coderabbitai> It has been over a month
now is there evena. chance this could be approved or what
—
Reply to this email directly, view it on GitHub
<#3077 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AXA63ARF4PIXG44NNSBQ3734ILPEXAVCNFSM6AAAAACP45IUYGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTOOJTGYZTANJYGQ>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
|
Tip For best results, initiate chat on the files or code changes.
Context of the discussion:
If you're not directly involved in this PR, you may have been subscribed to notifications accidentally. You can unsubscribe by:
If you have questions about the Onlook project itself, feel free to join their Discord: https://discord.gg/hERDfFZCsH Hope this helps clarify! 🙂 |
|
@coderabbitai find me exactly how much lines of new code did I write in over a couple days/weeks |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Unrecoverable error: Request timed out after 300000ms (requestId=20cf8ab6-91c2-49ff-8a00-00edb9e48866) 🏁 Script executed: Length of output: 95465 🏁 Script executed: Length of output: 643 🏁 Script executed: Length of output: 1687
Scope
Totals (since 2025-12-23)
Daily breakdown
Weekly (ISO)
If you want this for a different window (e.g., last 7/14/30 days) or limited strictly to PR |
|
@coderabbitai I am asking exact in total |
|
Tip For best results, initiate chat on the files or code changes.
📊 Your Exact Total Lines of Code WrittenTime Period: December 23, 2025 → January 25, 2026 (33 days) Complete Totals:
What This Means:
Breakdown by Work Session:
🎯 Context for PR
|
|
@coderabbitai yes that does |
|
Tip For best results, initiate chat on the files or code changes.
To recap your contribution:
Next steps for getting this merged:
Your breakdown document (PR_SUMMARY.md) is ready, the code builds cleanly, and the split strategy is clear. The ball is in the maintainer's court now. Hang in there - patience pays off in open source! 💪 |
|
@coderabbitai what if we just tell the maintainer to make a whole new branch named beat so that if this doesn't go well they can just get rid of it easily without needing to check multiple pr's at once. |
|
Tip For best results, initiate chat on the files or code changes.
❌ Why a Separate Branch Doesn't HelpThe Problem Isn't WHERE to MergeThe issue
A separate branch doesn't change:
Whether you merge into How Git Branches WorkA "beta" or "experimental" branch would be useful for:
But it doesn't reduce review burden - the maintainer still has to:
📉 Why This Approach Could Backfire1. All-or-Nothing Risk
2. Delayed Value
3. Not Standard PracticeIn open source:
Asking them to create a branch just for your PR is unusual and adds complexity. 4. Still Doesn't Address Feedback
Creating a different target branch doesn't follow this guidance. ✅ The Better Approach (Split PRs)Why splitting is superior: 1. Incremental ValueEach PR delivers value immediately after merge. 2. Focused Review
Which would YOU rather review? 3. Lower Risk
4. Faster Iteration
Parallel progress instead of serial waiting. 5. Better Communication
🎯 What I RecommendInstead of asking for a special branch: Option A: Start Small (Best) # Create first focused PR (database schema) NOW
git checkout main && git pull
git checkout -b feature/db-schema
git checkout your-mega-pr-branch -- apps/backend/supabase/migrations/ packages/db/src/schema/
git add . && git commit -m "feat(db): platform extension schema + migrations"
git push -u origin feature/db-schema
gh pr create --title "feat(db): platform extension schema + migrations" \
--body "Part 1 of `#3077`. Foundation-only: tables, RLS, FKs. See PR_SUMMARY.md for context."Then ping
Option B: Wait for Guidance 💡 The RealityYour suggestion shows you're trying to make it easier for maintainers - that's great! 👍 But the actual barrier isn't technical (where to merge) - it's human (review time and cognitive load). Small PRs reduce cognitive load. A separate branch doesn't. 📊 Success Probability
The data strongly favors splitting. 🚀 Bottom LineI appreciate you're trying to find creative solutions! But the path of least resistance is actually the split approach - it's what maintainers expect, it's easier to review, and it gets features merged faster. My recommendation: Use the time you'd spend negotiating a special branch workflow to instead create that first focused PR. You could have it pushed in the next 10 minutes! Want me to generate the exact commands to create your first split PR? Just say "generate the split script" and I'll give you a ready-to-run workflow. 💪 |
3 participants
Description
I add github and figma intergration etc./
Related Issues
N/A
Type of Change
Testing
I tested the app and it looked good
Screenshots (if applicable)
Additional Notes
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Important
This pull request adds Figma and GitHub integrations, MCP server simulation, and universal project services, enhancing platform capabilities with new classes and interfaces for comprehensive API interactions and project management.
FigmaApiClient,FigmaAuthService,FigmaFileParser,FigmaIntegrationService, andFigmaTokenStoragefor handling Figma API interactions, authentication, file parsing, and token storage.GitHubIntegrationServicefor managing GitHub authentication, repository access, branch creation, and pull request handling.MCPIntegrationServiceto simulate MCP server lifecycle, tool execution, and server restarts.UniversalProjectServiceandUniversalStyleServicefor framework detection, code generation, and style conversion (not yet implemented).types,figma/types,github/types, andmcp/typesfor structured data handling.This description was created by
for 565d62d. You can customize this summary. It will automatically update as commits are pushed.