-
Notifications
You must be signed in to change notification settings - Fork 1.7k
fix: update prompts #2910
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: update prompts #2910
Conversation
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
WalkthroughConsolidates imports around a new constants entry point, updates prompt text in context and system constants, adds a constants barrel export, adjusts the prompt package’s public exports, and updates dependent import paths. One web API router merges imports. No runtime control-flow changes. Changes
Sequence Diagram(s)Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 (2)
apps/web/client/src/server/api/routers/chat/suggestion.ts (2)
13-19: Block user-supplied system-role messages (prompt injection vector).Allowing clients to send role: "system" after your own system prompt can override it. Restrict roles to user/assistant and filter out any incoming system messages.
Apply these diffs:
- messages: z.array(z.object({ - role: z.enum(['user', 'assistant', 'system']), + messages: z.array(z.object({ + role: z.enum(['user', 'assistant']), content: z.string(), })),- ...convertToModelMessages(input.messages.map((m) => ({ + ...convertToModelMessages( + input.messages + .filter((m) => m.role !== 'system') + .map((m) => ({ role: m.role, parts: [{ type: 'text', text: m.content }], - }))), + }))),Also applies to: 34-37
47-49: Restrict suggestion update to current project
Add a predicate on conversations.projectId to match ctx.session.projectId and import and:-import { eq } from 'drizzle-orm'; +import { and, eq } from 'drizzle-orm'; … - await ctx.db.update(conversations).set({ - suggestions, - }).where(eq(conversations.id, input.conversationId)); + await ctx.db.update(conversations).set({ suggestions }).where( + and( + eq(conversations.id, input.conversationId), + eq(conversations.projectId, ctx.session.projectId) + ) + );
🧹 Nitpick comments (2)
packages/ai/src/prompt/constants/context.ts (2)
1-1: Minor copy edit (optional).Consider making the prefix more direct; current copy is fine if intentionally conversational.
24-24: Brand capitalization nit (optional).Use “Next.js” and “Tailwind CSS” for consistency.
-const projectContextPrefix = `This is a Nextjs project with TailwindCSS`; +const projectContextPrefix = `This is a Next.js project with Tailwind CSS`;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/web/client/src/server/api/routers/chat/suggestion.ts(1 hunks)packages/ai/src/prompt/constants/context.ts(1 hunks)packages/ai/src/prompt/constants/index.ts(1 hunks)packages/ai/src/prompt/constants/system.ts(1 hunks)packages/ai/src/prompt/index.ts(1 hunks)packages/ai/src/prompt/provider.ts(1 hunks)packages/ai/src/tools/classes/onlook-instructions.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
packages/ai/src/prompt/index.tspackages/ai/src/prompt/constants/index.tsapps/web/client/src/server/api/routers/chat/suggestion.tspackages/ai/src/prompt/constants/context.tspackages/ai/src/prompt/constants/system.tspackages/ai/src/prompt/provider.tspackages/ai/src/tools/classes/onlook-instructions.ts
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
packages/ai/src/prompt/index.tspackages/ai/src/prompt/constants/index.tsapps/web/client/src/server/api/routers/chat/suggestion.tspackages/ai/src/prompt/constants/context.tspackages/ai/src/prompt/constants/system.tspackages/ai/src/prompt/provider.tspackages/ai/src/tools/classes/onlook-instructions.ts
apps/web/client/src/server/api/routers/**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/server/api/routers/**/*.ts: Place tRPC routers under apps/web/client/src/server/api/routers/**
Use publicProcedure/protectedProcedure from apps/web/client/src/server/api/trpc.ts and validate inputs with Zod
Return plain objects/arrays; rely on SuperJSON for serialization in tRPC procedures
apps/web/client/src/server/api/routers/**/*.ts: Place tRPC routers under src/server/api/routers/**
Use publicProcedure/protectedProcedure from src/server/api/trpc.ts and validate inputs with Zod
Return plain objects/arrays; rely on SuperJSON for serialization
Files:
apps/web/client/src/server/api/routers/chat/suggestion.ts
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/server/api/routers/chat/suggestion.ts
🔇 Additional comments (7)
apps/web/client/src/server/api/routers/chat/suggestion.ts (2)
1-1: Import consolidation LGTMSingle-source import from @onlook/ai improves clarity and aligns with the new public surface.
43-44: Verify token limit option name and right-sizing.For OpenRouter via the ai SDK, the option is typically maxTokens, not maxOutputTokens. 10k is excessive for 3 brief suggestions.
Consider:
- maxOutputTokens: 10000, + maxTokens: 512,If your ai package expects maxOutputTokens, keep it; otherwise switch to maxTokens. Please confirm against your installed ai SDK.
packages/ai/src/tools/classes/onlook-instructions.ts (1)
4-4: Import path update LGTMSwitching ONLOOK_INSTRUCTIONS to the constants barrel matches the new public surface.
packages/ai/src/prompt/constants/system.ts (1)
14-14: Directive tightening LGTMStronger guidance around data-oid handling is clear and unambiguous.
packages/ai/src/prompt/provider.ts (1)
10-10: Barrel import consolidation LGTMReduces import churn and centralizes prompt sourcing.
packages/ai/src/prompt/constants/index.ts (1)
1-11: New constants barrel LGTMCentralized re-exports simplify consumption and align with related import changes.
packages/ai/src/prompt/index.ts (1)
1-1: No stale imports remain. Search across the codebase found no references toprompt/create,prompt/onlook, orprompt/summary. Public surface change is safe.
Description
Related Issues
Type of Change
Testing
Screenshots (if applicable)
Additional Notes
Important
Refactor prompt files into a
constantsdirectory and update related imports, with minor prompt content updates for clarity.constantsdirectory inpackages/ai/src/prompt/.provider.ts,onlook-instructions.ts, andsuggestion.tsto reflect new paths.filesContentPrefixandhighlightPrefixincontext.tsfor clarity.SYSTEM_PROMPTinsystem.tsto emphasize not alteringdata-oidattributes.index.tsinconstantsdirectory to export all constants.This description was created by
for 700c721. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Documentation
Refactor