Pr 3266 fix - #3322
Conversation
|
@loks0n @ChiragAgg5k this PR has 119 file changes and is 286 commits behind main. Could you provide guidance on whether this should be split into smaller PRs? |
🦋 Changeset detectedLatest commit: 427021c The changes in this PR will be included in the next version bump. This PR includes changesets to release 26 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds OpenClaw agent setup, database persistence, provisioning, Slack webhook routing, execution tracking, health checks, and status views. Refactors Ask AI state handling, replaces logo SVGs, updates webapp colors and scrollbar styling across many views, adjusts query and log presentations, and changes the email footer URL. 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
|
Rebased and conflicts resolved. Also cleaned up fork-specific branding (AirTrigger → Trigger.dev) across all 53 changed files. Ready for review 🙏 |
c13b432 to
ac862c6
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.changeset/openclaw-agent-integration.md (1)
1-9:⚠️ Potential issue | 🟡 MinorChangeset targets incorrect packages with no actual changes.
The changeset specifies
@trigger.dev/coreand@trigger.dev/sdkas patch versions, but no code changes exist in eitherpackages/coreorpackages/trigger-sdk. The OpenClaw agent integration changes are only inapps/webapp(internal application), which doesn't publish as an npm package. Either remove this changeset if the changes are internal-only, or update it to target the correct published package.
🧹 Nitpick comments (2)
apps/webapp/app/routes/api.agents.provision.ts (1)
3-3: ⚡ Quick winUnused import.
spawnfromchild_processis imported but never used.-import { spawn } from "child_process";apps/webapp/app/routes/agents.$agentId.status.tsx (1)
2-3: ⚡ Quick winRemove unused imports.
jsonfrom@remix-run/nodeanduseLoaderDatafrom@remix-run/reactare imported but not used. The code usestypedjsonanduseTypedLoaderDatainstead.import type { LoaderFunctionArgs } from "`@remix-run/node`"; -import { json } from "`@remix-run/node`"; -import { useLoaderData } from "`@remix-run/react`"; import { typedjson, useTypedLoaderData } from "remix-typedjson";
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5f4db2e7-9540-4365-8aee-06def1775349
📒 Files selected for processing (47)
.changeset/openclaw-agent-integration.mdapps/webapp/app/components/AskAI.tsxapps/webapp/app/components/ErrorDisplay.tsxapps/webapp/app/components/LogoIcon.tsxapps/webapp/app/components/LogoType.tsxapps/webapp/app/components/code/AIQueryInput.tsxapps/webapp/app/components/code/TSQLEditor.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/logs/LogDetailView.tsxapps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/components/navigation/EnvironmentSelector.tsxapps/webapp/app/components/navigation/OrganizationSettingsSideMenu.tsxapps/webapp/app/components/navigation/SideMenu.tsxapps/webapp/app/components/navigation/SideMenuHeader.tsxapps/webapp/app/components/onboarding/TechnologyPicker.tsxapps/webapp/app/components/primitives/Select.tsxapps/webapp/app/components/primitives/Sheet.tsxapps/webapp/app/components/primitives/TreeView/TreeView.tsxapps/webapp/app/components/primitives/charts/ChartLegendCompound.tsxapps/webapp/app/components/primitives/charts/ChartZoom.tsxapps/webapp/app/components/runs/v3/ReplayRunDialog.tsxapps/webapp/app/components/scheduled/timezones.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.batches.$batchParam/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.bulk-actions.$bulkActionParam/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables.new/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/QueryHelpSidebar.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.query/QueryHistoryPopover.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.waitpoints.tokens.$waitpointParam/route.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.settings._index/route.tsxapps/webapp/app/routes/_app.timezones/route.tsxapps/webapp/app/routes/agents.$agentId.status.tsxapps/webapp/app/routes/agents.setup.tsxapps/webapp/app/routes/api.agents.provision.tsapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam.streams.$streamKey/route.tsxapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.bulkaction.tsxapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.schedules.new/route.tsxapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.env.$envParam.waitpoints.$waitpointFriendlyId.complete/route.tsxapps/webapp/app/routes/resources.orgs.$organizationSlug.projects.$projectParam.schedules.new.natural-language.tsxapps/webapp/app/routes/storybook.popover/route.tsxapps/webapp/app/routes/storybook/route.tsxapps/webapp/app/routes/webhooks.slack.tsapps/webapp/remix.config.jsinternal-packages/database/prisma/migrations/20260325122458_add_openclaw_agents/migration.sqlinternal-packages/database/prisma/schema.prismainternal-packages/emails/emails/components/Footer.tsx
| <linearGradient id="logoGradient" x1="0%" y1="0%" x2="100%" y2="0%"> | ||
| <stop offset="0%" stopColor="#2E7D32" /> | ||
| <stop offset="100%" stopColor="#43A047" /> | ||
| </linearGradient> | ||
| </defs> | ||
| {/* Icon */} | ||
| <rect x="0" y="8" width="64" height="64" rx="14" fill="url(#logoGradient)" /> | ||
| <path d="M32 20L46 56H18L32 20Z" fill="white" fillOpacity="0.9" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and read the LogoType.tsx file
fd -t f "LogoType.tsx" apps/Repository: triggerdotdev/trigger.dev
Length of output: 109
🏁 Script executed:
# Search for other uses of "logoGradient" or similar gradient patterns in the codebase
rg "logoGradient|linearGradient.*id=" --type tsx --type tsRepository: triggerdotdev/trigger.dev
Length of output: 97
🏁 Script executed:
# Check the current implementation of LogoType.tsx
cat apps/webapp/app/components/LogoType.tsxRepository: triggerdotdev/trigger.dev
Length of output: 1022
🏁 Script executed:
# Find components that use LogoType to understand if multiple instances appear on same page
rg "LogoType" --type tsx --type ts -B 2 -A 2Repository: triggerdotdev/trigger.dev
Length of output: 97
🏁 Script executed:
# Search for other gradient patterns and inline SVG IDs (broader search)
rg "id=\"[^\"]*Gradient|url\(#" apps/webapp --type-listRepository: triggerdotdev/trigger.dev
Length of output: 459
🏁 Script executed:
# Search for logoGradient references
rg "logoGradient" apps/Repository: triggerdotdev/trigger.dev
Length of output: 310
🏁 Script executed:
# Find imports of LogoType to understand usage patterns
rg "import.*LogoType|from.*LogoType" apps/Repository: triggerdotdev/trigger.dev
Length of output: 155
🏁 Script executed:
# Check if LoginPageLayout mentioned in review actually uses LogoType
rg "LogoType" apps/webapp/app/components/LoginPageLayout.tsxRepository: triggerdotdev/trigger.dev
Length of output: 152
🏁 Script executed:
# Find all imports of LogoType across the entire codebase
rg "import.*LogoType|from.*LogoType" apps/Repository: triggerdotdev/trigger.dev
Length of output: 155
🏁 Script executed:
# Search for other inline SVG patterns with IDs to assess collision risk
rg "id\s*=\s*['\"]" apps/webapp --max-count=50 | head -40Repository: triggerdotdev/trigger.dev
Length of output: 6339
🏁 Script executed:
# Check if there are other components in same directory that might have SVG elements
ls -la apps/webapp/app/components/ | grep -E "\.(tsx?|jsx?)$" | head -20Repository: triggerdotdev/trigger.dev
Length of output: 1470
🏁 Script executed:
# Search for any other gradient definitions or SVG markers/patterns
rg "linearGradient|pattern|marker" apps/webapp/app --max-count=20Repository: triggerdotdev/trigger.dev
Length of output: 50383
🏁 Script executed:
# Check if LogoType is used anywhere else besides LoginPageLayout
rg "LogoType" apps/ -A 3 -B 1 2>/dev/null | head -100Repository: triggerdotdev/trigger.dev
Length of output: 1234
🏁 Script executed:
# Look at LoginPageLayout to see how LogoType is used
cat apps/webapp/app/components/LoginPageLayout.tsxRepository: triggerdotdev/trigger.dev
Length of output: 3938
🏁 Script executed:
# Search for any scenarios where the page might render multiple SVGs with inline gradients
rg "linearGradient|<svg" apps/webapp/app/components/LoginPageLayout.tsxRepository: triggerdotdev/trigger.dev
Length of output: 51
Use useId() for unique SVG gradient IDs in reusable components.
The fixed id="logoGradient" could cause collisions if LogoType is ever rendered multiple times on the same page. While the current usage (single instance in LoginPageLayout) is safe, it's best practice to use React's useId() hook for any IDs in reusable components.
Proposed fix
+import { useId } from "react";
+
export function LogoType({ className }: { className?: string }) {
+ const gradientId = useId();
return (
<svg
viewBox="0 0 520 80"
xmlns="http://www.w3.org/2000/svg"
className={className}
>
<defs>
- <linearGradient id="logoGradient" x1="0%" y1="0%" x2="100%" y2="0%">
+ <linearGradient id={gradientId} x1="0%" y1="0%" x2="100%" y2="0%">
<stop offset="0%" stopColor="`#2E7D32`" />
<stop offset="100%" stopColor="`#43A047`" />
</linearGradient>
</defs>
{/* Icon */}
- <rect x="0" y="8" width="64" height="64" rx="14" fill="url(`#logoGradient`)" />
+ <rect x="0" y="8" width="64" height="64" rx="14" fill={`url(#${gradientId})`} />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <linearGradient id="logoGradient" x1="0%" y1="0%" x2="100%" y2="0%"> | |
| <stop offset="0%" stopColor="#2E7D32" /> | |
| <stop offset="100%" stopColor="#43A047" /> | |
| </linearGradient> | |
| </defs> | |
| {/* Icon */} | |
| <rect x="0" y="8" width="64" height="64" rx="14" fill="url(#logoGradient)" /> | |
| <path d="M32 20L46 56H18L32 20Z" fill="white" fillOpacity="0.9" /> | |
| import { useId } from "react"; | |
| export function LogoType({ className }: { className?: string }) { | |
| const gradientId = useId(); | |
| return ( | |
| <svg | |
| viewBox="0 0 520 80" | |
| xmlns="http://www.w3.org/2000/svg" | |
| className={className} | |
| > | |
| <defs> | |
| <linearGradient id={gradientId} x1="0%" y1="0%" x2="100%" y2="0%"> | |
| <stop offset="0%" stopColor="`#2E7D32`" /> | |
| <stop offset="100%" stopColor="`#43A047`" /> | |
| </linearGradient> | |
| </defs> | |
| {/* Icon */} | |
| <rect x="0" y="8" width="64" height="64" rx="14" fill={`url(#${gradientId})`} /> | |
| <path d="M32 20L46 56H18L32 20Z" fill="white" fillOpacity="0.9" /> | |
| </svg> | |
| ); | |
| } |
| > | ||
| <ResizablePanel id="payload" min="300px"> | ||
| <div className="rounded-smbg-charcoal-900 mb-3 h-full min-h-40 overflow-y-auto scrollbar-thin scrollbar-track-transparent scrollbar-thumb-charcoal-600"> | ||
| <div className="rounded-smbg-charcoal-900 mb-3 h-full min-h-40 overflow-y-auto scrollbar-thin scrollbar-track-transparent scrollbar-thumb-gray-300"> |
There was a problem hiding this comment.
Restore the missing class separator here.
rounded-smbg-charcoal-900 looks like two Tailwind utilities collapsed into one token, so the panel will lose its intended radius/background styling.
Fix
- <div className="rounded-smbg-charcoal-900 mb-3 h-full min-h-40 overflow-y-auto scrollbar-thin scrollbar-track-transparent scrollbar-thumb-gray-300">
+ <div className="rounded-sm bg-charcoal-900 mb-3 h-full min-h-40 overflow-y-auto scrollbar-thin scrollbar-track-transparent scrollbar-thumb-gray-300">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div className="rounded-smbg-charcoal-900 mb-3 h-full min-h-40 overflow-y-auto scrollbar-thin scrollbar-track-transparent scrollbar-thumb-gray-300"> | |
| <div className="rounded-sm bg-charcoal-900 mb-3 h-full min-h-40 overflow-y-auto scrollbar-thin scrollbar-track-transparent scrollbar-thumb-gray-300"> |
| {check.isHealthy ? "✓ Healthy" : "✗ Unhealthy"} | ||
| </span> | ||
| </TableCell> | ||
| <TableCell>{check.responseTimeMs}ms</TableCell> |
There was a problem hiding this comment.
Handle null responseTimeMs.
responseTimeMs is nullable in the schema. When null, this renders "nullms".
- <TableCell>{check.responseTimeMs}ms</TableCell>
+ <TableCell>{check.responseTimeMs != null ? `${check.responseTimeMs}ms` : "N/A"}</TableCell>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <TableCell>{check.responseTimeMs}ms</TableCell> | |
| <TableCell>{check.responseTimeMs != null ? `${check.responseTimeMs}ms` : "N/A"}</TableCell> |
| // Parse tools JSON | ||
| const tools = JSON.parse(data.tools || "[]"); | ||
|
|
There was a problem hiding this comment.
JSON.parse can throw on malformed input.
data.tools is validated by Zod as a string, but JSON.parse can still throw SyntaxError if the string isn't valid JSON. This would cause an unhandled exception.
🛠️ Suggested fix using Zod transform
const SetupSchema = z.object({
agentName: z.string().min(1, "Agent name is required"),
model: z.enum(["claude-3.5-sonnet", "claude-3-opus", "gpt-4-turbo"]),
messagingPlatform: z.enum(["slack", "discord", "telegram"]),
- tools: z.string(), // JSON string array
+ tools: z.string().transform((val, ctx) => {
+ try {
+ const parsed = JSON.parse(val || "[]");
+ if (!Array.isArray(parsed)) {
+ ctx.addIssue({ code: z.ZodIssueCode.custom, message: "Tools must be an array" });
+ return z.NEVER;
+ }
+ return parsed as string[];
+ } catch {
+ ctx.addIssue({ code: z.ZodIssueCode.custom, message: "Invalid JSON" });
+ return z.NEVER;
+ }
+ }),
slackWorkspaceId: z.string().optional(),
slackWebhookToken: z.string().optional(),
});
// Then in action:
- // Parse tools JSON
- const tools = JSON.parse(data.tools || "[]");
+ const tools = data.tools; // Already parsed by Zod| const provisionResponse = await fetch("http://localhost:3000/api/agents/provision", { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ agentId: agentConfig.id }), | ||
| }); | ||
|
|
||
| if (!provisionResponse.ok) { | ||
| logger.error("Provisioning failed", { | ||
| agentId: agentConfig.id, | ||
| status: provisionResponse.status, | ||
| }); | ||
| } | ||
| } catch (error) { | ||
| logger.error("Failed to call provisioning endpoint", { error }); | ||
| } |
There was a problem hiding this comment.
Hardcoded localhost URL won't work in production; provisioning failure leaves orphaned agents.
Two issues:
http://localhost:3000is hardcoded - this won't work in deployed environments. Use an environment variable viaenv.server.tsper coding guidelines.- If provisioning fails (Lines 77-85), the agent config is already created and the user is still redirected to the status page. This leaves the agent in an inconsistent state with no way to retry provisioning.
🛠️ Suggested approach
+import { env } from "~/env.server";
+
// In the action:
- const provisionResponse = await fetch("http://localhost:3000/api/agents/provision", {
+ const provisionResponse = await fetch(`${env.APP_ORIGIN}/api/agents/provision`, {
method: "POST",
headers: { "Content-Type": "application/json" },
body: JSON.stringify({ agentId: agentConfig.id }),
});
if (!provisionResponse.ok) {
logger.error("Provisioning failed", {
agentId: agentConfig.id,
status: provisionResponse.status,
});
+ // Update status to failed so user can retry
+ await prisma.agentConfig.update({
+ where: { id: agentConfig.id },
+ data: { status: "failed" },
+ });
}
} catch (error) {
logger.error("Failed to call provisioning endpoint", { error });
+ await prisma.agentConfig.update({
+ where: { id: agentConfig.id },
+ data: { status: "failed" },
+ });
}Source: Coding guidelines
| try { | ||
| const event = await request.json() as any; | ||
|
|
||
| // Handle Slack URL verification | ||
| if (event.type === "url_verification") { | ||
| return json({ challenge: event.challenge }); | ||
| } | ||
|
|
||
| // Handle message events | ||
| if (event.type === "event_callback" && event.event.type === "message") { | ||
| const slackEvent = event.event; | ||
| const workspaceId = event.team_id; |
There was a problem hiding this comment.
Missing Slack request signature verification and input validation.
- Security: Slack webhooks should verify the
X-Slack-Signatureheader to prevent forged requests. Without this, anyone can call this endpoint pretending to be Slack. - Validation: The payload is cast as
anywithout validation. Line 24 accessesevent.event.typewhich will throw ifevent.eventis undefined.
🛠️ Suggested approach for signature verification
import crypto from "crypto";
import { env } from "~/env.server";
function verifySlackSignature(request: Request, body: string): boolean {
const timestamp = request.headers.get("X-Slack-Request-Timestamp");
const signature = request.headers.get("X-Slack-Signature");
if (!timestamp || !signature) return false;
// Prevent replay attacks (5 min window)
if (Math.abs(Date.now() / 1000 - Number(timestamp)) > 300) return false;
const sigBaseString = `v0:${timestamp}:${body}`;
const mySignature = "v0=" + crypto
.createHmac("sha256", env.SLACK_SIGNING_SECRET)
.update(sigBaseString)
.digest("hex");
return crypto.timingSafeEqual(
Buffer.from(mySignature),
Buffer.from(signature)
);
}| // Route message to OpenClaw container (on VPS) | ||
| const containerUrl = `http://178.128.150.129:${agent.containerPort}`; |
There was a problem hiding this comment.
Hardcoded IP address won't work across environments.
The container URL uses a hardcoded IP 178.128.150.129. This should be configured via environment variable.
+import { env } from "~/env.server";
+
- const containerUrl = `http://178.128.150.129:${agent.containerPort}`;
+ const containerUrl = `http://${env.OPENCLAW_VPS_HOST}:${agent.containerPort}`;Source: Coding guidelines
| await prisma.agentExecution.create({ | ||
| data: { | ||
| agentId: agent.id, | ||
| message: text, | ||
| response: agentResponse, | ||
| executionTimeMs: 0, // TODO: Measure actual execution time | ||
| inputTokens: containerData?.inputTokens, | ||
| outputTokens: containerData?.outputTokens, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Potential undefined value stored in database.
slackEvent.text (Line 28) could be undefined for certain Slack event types. Storing undefined in the required message field could cause a Prisma error or store unexpected values.
await prisma.agentExecution.create({
data: {
agentId: agent.id,
- message: text,
+ message: text ?? "",
response: agentResponse,
executionTimeMs: 0, // TODO: Measure actual execution time| -- DropIndex | ||
| DROP INDEX "public"."SecretStore_key_idx"; | ||
|
|
||
| -- DropIndex | ||
| DROP INDEX "public"."TaskRun_runtimeEnvironmentId_createdAt_idx"; | ||
|
|
||
| -- DropIndex | ||
| DROP INDEX "public"."TaskRun_runtimeEnvironmentId_id_idx"; |
There was a problem hiding this comment.
Remove extraneous Prisma-generated changes and use separate migration for existing table indexes.
Per coding guidelines:
- Lines 16-44 modify junction tables (
_BackgroundWorkerToBackgroundWorkerFile,_BackgroundWorkerToTaskQueue,_TaskRunToTaskRunTag,_WaitpointRunConnections,_completedWaitpoints) - these should be removed as extraneous Prisma diff noise. - Lines 1-8 drop indexes on existing tables (
SecretStore,TaskRun) and Lines 105-112 recreate them - these should useCREATE INDEX CONCURRENTLY IF NOT EXISTSin a separate migration file to avoid table locks in production.
Based on coding guidelines: "Clean up generated Prisma migrations by removing extraneous lines for junction tables... When adding indexes to existing tables, use CREATE INDEX CONCURRENTLY IF NOT EXISTS... and place each concurrent index in its own separate migration file."
Also applies to: 16-44
Source: Coding guidelines
| /// Webhook token for Slack | ||
| slackWebhookToken String? | ||
|
|
||
| /// User's API keys (encrypted) | ||
| apiKeys Json? |
There was a problem hiding this comment.
Sensitive credentials stored as plain text.
slackWebhookToken (Line 3219) and apiKeys (Line 3222) store sensitive credentials. The codebase already has a SecretReference pattern (used for MFA secrets in the User model at Lines 52-53). Consider using SecretReference relations or an encrypted storage mechanism for these fields rather than storing them as plain String/Json.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| invalidSelection | ||
| ? "border-amber-800 bg-amber-950 text-amber-400" | ||
| : "border-blue-800 bg-[#1B2334] text-blue-400" | ||
| : "border-blue-800 bg-blue-50 text-blue-400" |
There was a problem hiding this comment.
🔴 Zoom tooltip background changed from dark to near-white, breaking dark theme
The zoom selection tooltip background was changed from bg-[#1B2334] (a dark navy blue matching the dark theme) to bg-blue-50 (Tailwind's default #eff6ff, an almost-white color). This applies to both the tooltip body (line 130) and its arrow indicator (line 144). In the dark-themed app, this renders as a bright white tooltip with light blue text (text-blue-400), making it extremely jarring and potentially unreadable against the dark chart background.
| : "border-blue-800 bg-blue-50 text-blue-400" | |
| : "border-blue-800 bg-[#1B2334] text-blue-400" |
Was this helpful? React with 👍 or 👎 to provide feedback.
| invalidSelection | ||
| ? "border-l border-t border-amber-800 bg-amber-950" | ||
| : "border-l border-t border-blue-800 bg-[#1B2334]" | ||
| : "border-l border-t border-blue-800 bg-blue-50" |
There was a problem hiding this comment.
🔴 Zoom tooltip arrow background also changed to near-white bg-blue-50
Same issue as the tooltip body — the arrow's background was changed from bg-[#1B2334] to bg-blue-50, creating a bright white arrow on a dark chart. This is the companion change to the tooltip body at line 130.
| : "border-l border-t border-blue-800 bg-blue-50" | |
| : "border-l border-t border-blue-800 bg-[#1B2334]" |
Was this helpful? React with 👍 or 👎 to provide feedback.
| {...props} | ||
| className={cn( | ||
| "overflow-y-auto overscroll-contain scrollbar-thin scrollbar-track-transparent scrollbar-thumb-charcoal-600 focus-custom", | ||
| "overflow-y-auto overscroll-contain scrollbar-thin scrollbar-track-transparent scrollbar-thumb-gray-300 focus-custom", |
There was a problem hiding this comment.
🔴 scrollbar-thumb-charcoal-600 replaced with scrollbar-thumb-gray-300, breaking dark theme scrollbars across entire app
All instances of scrollbar-thumb-charcoal-600 were globally replaced with scrollbar-thumb-gray-300. The custom charcoal-600 is defined as #3B3E45 (a dark gray matching the dark theme) in apps/webapp/tailwind.config.js:65. The gray color is NOT customized in the Tailwind config, so gray-300 resolves to Tailwind's default #D1D5DB — a very light gray. This makes all scrollbar thumbs extremely bright against the dark background across ~30 components (side menus, editors, popovers, tables, detail views, etc.).
| "overflow-y-auto overscroll-contain scrollbar-thin scrollbar-track-transparent scrollbar-thumb-gray-300 focus-custom", | |
| "overflow-y-auto overscroll-contain scrollbar-thin scrollbar-track-transparent scrollbar-thumb-charcoal-600 focus-custom", |
Was this helpful? React with 👍 or 👎 to provide feedback.
| -- DropIndex | ||
| DROP INDEX "public"."SecretStore_key_idx"; | ||
|
|
||
| -- DropIndex | ||
| DROP INDEX "public"."TaskRun_runtimeEnvironmentId_createdAt_idx"; | ||
|
|
||
| -- DropIndex | ||
| DROP INDEX "public"."TaskRun_runtimeEnvironmentId_id_idx"; | ||
|
|
||
| -- AlterTable | ||
| ALTER TABLE "public"."FeatureFlag" ALTER COLUMN "updatedAt" DROP DEFAULT; | ||
|
|
||
| -- AlterTable | ||
| ALTER TABLE "public"."IntegrationDeployment" ALTER COLUMN "updatedAt" DROP DEFAULT; | ||
|
|
||
| -- AlterTable | ||
| ALTER TABLE "public"."_BackgroundWorkerToBackgroundWorkerFile" ADD CONSTRAINT "_BackgroundWorkerToBackgroundWorkerFile_AB_pkey" PRIMARY KEY ("A", "B"); | ||
|
|
||
| -- DropIndex | ||
| DROP INDEX "public"."_BackgroundWorkerToBackgroundWorkerFile_AB_unique"; | ||
|
|
||
| -- AlterTable | ||
| ALTER TABLE "public"."_BackgroundWorkerToTaskQueue" ADD CONSTRAINT "_BackgroundWorkerToTaskQueue_AB_pkey" PRIMARY KEY ("A", "B"); | ||
|
|
||
| -- DropIndex | ||
| DROP INDEX "public"."_BackgroundWorkerToTaskQueue_AB_unique"; | ||
|
|
||
| -- AlterTable | ||
| ALTER TABLE "public"."_TaskRunToTaskRunTag" ADD CONSTRAINT "_TaskRunToTaskRunTag_AB_pkey" PRIMARY KEY ("A", "B"); | ||
|
|
||
| -- DropIndex | ||
| DROP INDEX "public"."_TaskRunToTaskRunTag_AB_unique"; | ||
|
|
||
| -- AlterTable | ||
| ALTER TABLE "public"."_WaitpointRunConnections" ADD CONSTRAINT "_WaitpointRunConnections_AB_pkey" PRIMARY KEY ("A", "B"); | ||
|
|
||
| -- DropIndex | ||
| DROP INDEX "public"."_WaitpointRunConnections_AB_unique"; | ||
|
|
||
| -- AlterTable | ||
| ALTER TABLE "public"."_completedWaitpoints" ADD CONSTRAINT "_completedWaitpoints_AB_pkey" PRIMARY KEY ("A", "B"); | ||
|
|
||
| -- DropIndex | ||
| DROP INDEX "public"."_completedWaitpoints_AB_unique"; |
There was a problem hiding this comment.
🔴 Migration includes extraneous destructive changes that violate repository rules
The migration file violates the explicit rules in internal-packages/database/CLAUDE.md:28-35, which state: "Clean up generated migration - remove extraneous lines for: _BackgroundWorkerToBackgroundWorkerFile, _BackgroundWorkerToTaskQueue, _TaskRunToTaskRunTag, _WaitpointRunConnections, _completedWaitpoints, SecretStore_key_idx, Various TaskRun indexes". The migration includes all of these auto-generated changes. Most critically, it drops and recreates indexes on the TaskRun table (lines 5-8, 109-112) without using CREATE INDEX CONCURRENTLY, violating the rule at internal-packages/database/CLAUDE.md:45 that indexes on existing tables must use CONCURRENTLY to avoid table locks in production. It also drops updatedAt defaults from FeatureFlag and IntegrationDeployment (lines 11-14) and modifies join table constraints (lines 17-44) — all extraneous changes unrelated to the OpenClaw agent feature.
Prompt for agents
The migration file contains many auto-generated extraneous changes that must be removed per the rules in internal-packages/database/CLAUDE.md. Specifically:
1. Remove all DROP INDEX and CREATE INDEX lines for SecretStore_key_idx and TaskRun indexes (lines 1-8 and 105-112) — these are auto-generated diffs that should not be in this migration.
2. Remove the ALTER TABLE lines for FeatureFlag and IntegrationDeployment updatedAt defaults (lines 11-14).
3. Remove all the join table constraint changes for _BackgroundWorkerToBackgroundWorkerFile, _BackgroundWorkerToTaskQueue, _TaskRunToTaskRunTag, _WaitpointRunConnections, and _completedWaitpoints (lines 17-44).
The migration should only contain the CREATE TABLE statements for AgentConfig, AgentExecution, AgentHealthCheck, their indexes (which are on newly created tables so CONCURRENTLY is not needed), and the AddForeignKey statements. Re-run pnpm run db:migrate:dev:create to regenerate a clean migration.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const agentConfig = await prisma.agentConfig.findUnique({ | ||
| where: { id: agentId }, |
There was a problem hiding this comment.
🟡 Uses findUnique instead of findFirst, violating Prisma query pattern rules
The apps/webapp/CLAUDE.md:126 explicitly states: "Always use findFirst instead of findUnique." The rationale is that Prisma's findUnique has an implicit DataLoader with known bugs (uppercase UUIDs returning null, composite key SQL issues, performance problems). The new file uses prisma.agentConfig.findUnique() at line 24. The same violation also occurs in apps/webapp/app/routes/agents.$agentId.status.tsx:28.
| const agentConfig = await prisma.agentConfig.findUnique({ | |
| where: { id: agentId }, | |
| const agentConfig = await prisma.agentConfig.findFirst({ | |
| where: { id: agentId }, |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const lastAgent = await prisma.agentConfig.findFirst({ | ||
| where: { | ||
| containerPort: { not: null }, | ||
| }, | ||
| orderBy: { containerPort: "desc" }, | ||
| }); | ||
|
|
||
| const nextPort = (lastAgent?.containerPort || 8000) + 1; |
There was a problem hiding this comment.
🔴 Port allocation race condition — concurrent provisions can assign the same port
In api.agents.provision.ts:33-40, the port allocation reads the highest existing containerPort and increments it, then writes the result. This is a classic TOCTOU (time-of-check-to-time-of-use) race: two concurrent provisioning requests can read the same max port and both compute the same nextPort, assigning duplicate ports to different agents. There is no database uniqueness constraint on containerPort in the schema (internal-packages/database/prisma/schema.prisma:3209) and no transaction or locking around this read-then-write sequence.
Prompt for agents
The port allocation in api.agents.provision.ts has a TOCTOU race condition. The code reads the max containerPort, increments it, and writes it in separate operations without any locking. Two concurrent requests can get the same port.
To fix this, either:
1. Add a unique constraint on containerPort in the Prisma schema and handle the unique violation error with a retry loop.
2. Use a Prisma interactive transaction ($transaction) with serializable isolation to atomically read the max port and write the new one.
3. Use a database sequence or advisory lock for port allocation.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const provisionResponse = await fetch("http://localhost:3000/api/agents/provision", { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body: JSON.stringify({ agentId: agentConfig.id }), | ||
| }); |
There was a problem hiding this comment.
🔴 Provisioning fetch hardcoded to localhost:3000 — wrong port, will always fail
In agents.setup.tsx:71, the action handler calls fetch("http://localhost:3000/api/agents/provision", ...) to trigger provisioning. However, the webapp runs on port 3030 locally (per CONTRIBUTING.md and CLAUDE.md), and in production the URL would be entirely different. This means the provisioning request will always fail to connect. The error is silently caught at line 83, so the agent is created and the user is redirected to the status page, but the agent will never actually be provisioned.
Prompt for agents
The provisioning fetch in agents.setup.tsx hardcodes http://localhost:3000 which is wrong — the webapp runs on port 3030 locally and a completely different URL in production. Instead of making an HTTP request to the same server, call the provisioning logic directly as a function import, or use the APP_ORIGIN / LOGIN_ORIGIN environment variable from env.server.ts to construct the correct base URL. Making an HTTP call from a server action to the same server is also an anti-pattern — prefer calling the provisioning service code directly.
Was this helpful? React with 👍 or 👎 to provide feedback.
| width="32" | ||
| height="32" | ||
| viewBox="0 0 32 32" | ||
| fill="none" | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| className={className} | ||
| > | ||
| <rect width="32" height="32" rx="8" fill="#2E7D32" /> | ||
| <path | ||
| fillRule="evenodd" | ||
| clipRule="evenodd" | ||
| d="M96.1017 113.4L160.679 4.57764e-05L320.718 281.045H0.638916L65.2159 167.642L110.896 194.382L92.0035 227.561H229.354L160.679 106.965L141.786 140.144L96.1017 113.4Z" | ||
| fill="url(#paint0_linear_465_1663)" | ||
| d="M16 6L24 24H8L16 6Z" | ||
| fill="white" | ||
| fillOpacity="0.9" | ||
| /> | ||
| <defs> | ||
| <linearGradient | ||
| id="paint0_linear_465_1663" | ||
| x1="320.718" | ||
| y1="140.687" | ||
| x2="0.638918" | ||
| y2="140.687" | ||
| gradientUnits="userSpaceOnUse" | ||
| > | ||
| <stop stopColor="#41FF54" /> | ||
| <stop offset="1" stopColor="#E7FF52" /> | ||
| </linearGradient> | ||
| </defs> | ||
| <circle cx="16" cy="20" r="2" fill="#2E7D32" /> |
There was a problem hiding this comment.
🚩 LogoIcon and LogoType completely replaced with different brand identity
The Trigger.dev logo SVGs in LogoIcon.tsx and LogoType.tsx have been entirely replaced. The original logo was a detailed geometric shape with a green-to-yellow gradient (#41FF54 to #E7FF52), consistent with Trigger.dev's established brand. The new logo is a green rounded rectangle (#2E7D32) with a simple white triangle and dot — a completely different design language. The LogoType also replaces the detailed path-based wordmark with an SVG <text> element rendering "Trigger.dev", which may render differently across browsers depending on font availability (Geist Variable, Inter, Helvetica Neue, sans-serif). This is a significant brand change that should be verified as intentional.
Was this helpful? React with 👍 or 👎 to provide feedback.
| export const action = async ({ request }: ActionFunctionArgs) => { | ||
| if (request.method !== "POST") { | ||
| return json({ error: "Method not allowed" }, { status: 405 }); | ||
| } |
There was a problem hiding this comment.
🚩 New agent routes bypass the standard dashboardAction/dashboardLoader authorization patterns
The existing webapp routes consistently use dashboardAction/dashboardLoader wrappers (e.g., apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.bulk-actions.$bulkActionParam/route.tsx:101) which handle authorization checks with { action: 'write', resource: { type: '...' } }. The new agent routes (agents.setup.tsx, agents.$agentId.status.tsx, api.agents.provision.ts, webhooks.slack.ts) don't use these patterns. The setup and status routes use requireUser for authentication but skip the authorization layer entirely. The provisioning endpoint has no authentication at all. This is inconsistent with the established patterns in the codebase.
Was this helpful? React with 👍 or 👎 to provide feedback.
| export const action = async ({ request }: ActionFunctionArgs) => { | ||
| if (request.method !== "POST") { | ||
| return json({ error: "Method not allowed" }, { status: 405 }); | ||
| } | ||
|
|
||
| try { | ||
| const event = await request.json() as any; | ||
|
|
||
| // Handle Slack URL verification | ||
| if (event.type === "url_verification") { | ||
| return json({ challenge: event.challenge }); |
There was a problem hiding this comment.
🚩 Webhook route processes Slack events without verifying request signature
The webhooks.slack.ts route handles incoming Slack events including url_verification and event_callback message types. Slack's documentation requires verifying the X-Slack-Signature header using the signing secret to ensure requests actually come from Slack. While this is a security concern (handled separately), it's worth noting from a correctness standpoint that the url_verification challenge response at line 19 would allow anyone to register this endpoint as a Slack webhook URL, and the message routing at lines 24+ would process any POST body that matches the expected shape, potentially triggering agent executions and database writes from non-Slack sources.
Was this helpful? React with 👍 or 👎 to provide feedback.
Squashed rebase of pr-3266-fix onto official/main. Co-authored-by: James Ritchie <james@trigger.dev>
ac862c6 to
427021c
Compare
| if (event.type === "event_callback" && event.event.type === "message") { | ||
| const slackEvent = event.event; | ||
| const workspaceId = event.team_id; | ||
| const channel = slackEvent.channel; | ||
| const text = slackEvent.text; | ||
| const userId = slackEvent.user; |
There was a problem hiding this comment.
🔴 Slack bot replies to its own messages, causing a response loop
The Slack webhook processes every message event and posts a reply without checking whether the message came from a bot (if (event.type === "event_callback" && event.event.type === "message") at apps/webapp/app/routes/webhooks.slack.ts:24), so the agent's own posted replies re-trigger the handler and it answers itself repeatedly.
Impact: The agent can enter an endless back-and-forth replying to its own messages, spamming the channel and the agent container.
Missing bot_id / subtype filtering
Slack delivers the bot's own posted messages back as message events. The handler at webhooks.slack.ts:24-133 does not filter out messages with a bot_id, a subtype of bot_message, or messages authored by the app itself. Since the reply is sent back to the same channel/thread_ts (:92-103), that reply generates another inbound message event with the same team_id, matching the same agent and triggering another reply — an infinite loop. Standard Slack Events API handlers must ignore events where event.bot_id is set (or event.subtype === "bot_message").
| if (event.type === "event_callback" && event.event.type === "message") { | |
| const slackEvent = event.event; | |
| const workspaceId = event.team_id; | |
| const channel = slackEvent.channel; | |
| const text = slackEvent.text; | |
| const userId = slackEvent.user; | |
| // Handle message events | |
| if (event.type === "event_callback" && event.event.type === "message") { | |
| const slackEvent = event.event; | |
| // Ignore messages from bots (including our own replies) and message edits/deletes | |
| // to prevent the agent from responding to itself in an endless loop. | |
| if (slackEvent.bot_id || slackEvent.subtype) { | |
| return json({ ok: true }); | |
| } | |
| const workspaceId = event.team_id; | |
| const channel = slackEvent.channel; | |
| const text = slackEvent.text; | |
| const userId = slackEvent.user; |
Was this helpful? React with 👍 or 👎 to provide feedback.
| const agent = await prisma.agentConfig.findFirst({ | ||
| where: { | ||
| slackWorkspaceId: workspaceId, | ||
| messagingPlatform: "slack", | ||
| status: "healthy", | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🔴 Slack messages are never routed because agents never reach the healthy state
The Slack webhook only routes to agents whose status is "healthy" (findFirst({ where: { ..., status: "healthy" } }) at apps/webapp/app/routes/webhooks.slack.ts:39-45), but provisioning only ever sets status to "provisioning" and nothing in the codebase transitions an agent to "healthy", so no message is ever delivered.
Impact: Incoming Slack messages are silently dropped for every agent, so the integration never responds.
Status lifecycle gap
agents.setup.tsx:59 creates agents with status: "provisioning" and api.agents.provision.ts:60 also sets status: "provisioning". There is no code path that sets status to "healthy". The webhook's agent lookup requires status: "healthy" (webhooks.slack.ts:42), so findFirst always returns null and the handler logs "No agent found" and returns early. The health-transition logic (a successful health check flipping the agent to healthy) is missing.
Prompt for agents
The Slack webhook only matches agents with status 'healthy', but agents are only ever created/updated with status 'provisioning' (agents.setup.tsx and api.agents.provision.ts) and nothing ever transitions them to 'healthy'. As a result no Slack message can ever be routed. Either add a mechanism that marks an agent 'healthy' after a successful provision/health check, or relax the webhook query to match provisioned agents (e.g. status not 'unhealthy' and a non-null containerPort). Ensure the status lifecycle (provisioning -> healthy/unhealthy) is fully implemented.
Was this helpful? React with 👍 or 👎 to provide feedback.
| import { PageBody, PageContainer } from "~/components/layout/AppLayout"; | ||
| import { Paragraph } from "~/components/primitives/Paragraph"; | ||
| import { prisma } from "~/db.server"; | ||
| import { requireUser } from "~/services/auth.server"; |
There was a problem hiding this comment.
🔍 Agent routes import requireUser from a module that doesn't export it
The new agent routes import requireUser from ~/services/auth.server (apps/webapp/app/routes/agents.setup.tsx:11, agents.$agentId.status.tsx:9, and referenced in webhooks.slack.ts/api.agents.provision.ts context). However apps/webapp/app/services/auth.server.ts only exports authenticator, isGithubAuthSupported, and isGoogleAuthSupported. requireUser actually lives in ~/services/session.server. This is a compile-time error (left to CI/type-checker per review guidelines) but is worth noting since it means these routes as written would not build.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const { events, duration, rootSpanStatus, rootStartedAt, queuedDuration, overridesBySpanId } = | ||
| trace; |
There was a problem hiding this comment.
🔍 runs.$runParam trace truncation banner removed during merge
In apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.runs.$runParam/route.tsx the destructuring of isTruncated/missingAnchor from trace and the associated warning Callout were removed (lines ~611-704). The PR description states some features were dropped during conflict resolution; if the trace loader still returns isTruncated/missingAnchor, users will no longer be warned when a trace is only partially displayed. Worth confirming this drop was intentional versus a merge casualty.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const run = await $replica.taskRun.findFirst({ | ||
| where: { | ||
| friendlyId: runParam, | ||
| projectId: project.id, | ||
| }, | ||
| include: { | ||
| runtimeEnvironment: { | ||
| include: { | ||
| project: true, | ||
| organization: true, | ||
| orgMember: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }; | ||
| // Client-less findRun defaults to the read replica; replica lag can null out a live run and 404 a | ||
| // valid stream-viewer request (useRealtimeStream surfaces the error, no auto-retry). Re-read the | ||
| // owning primary on a replica miss. | ||
| const run = | ||
| (await runStore.findRun(runWhere, runArgs)) ?? | ||
| (await runStore.findRunOnPrimary(runWhere, runArgs)); | ||
| }); | ||
|
|
||
| if (!run) { | ||
| throw new Response("Not Found", { status: 404 }); | ||
| } | ||
|
|
||
| const environment = await controlPlaneResolver.resolveAuthenticatedEnv(run.runtimeEnvironmentId); | ||
|
|
||
| if (!environment || environment.slug !== envParam) { | ||
| if (run.runtimeEnvironment.slug !== envParam) { | ||
| throw new Response("Not Found", { status: 404 }); | ||
| } | ||
|
|
||
| // Get Last-Event-ID header for resuming from a specific position | ||
| const lastEventId = request.headers.get("Last-Event-ID") || undefined; | ||
|
|
||
| const realtimeStream = getRealtimeStreamInstance(environment, run.realtimeStreamsVersion, { | ||
| run: { streamBasinName: run.streamBasinName }, | ||
| }); | ||
|
|
||
| return realtimeStream.streamResponse( | ||
| request, | ||
| run.friendlyId, | ||
| streamKey, | ||
| getRequestAbortSignal(), | ||
| { | ||
| lastEventId, | ||
| } | ||
| const realtimeStream = getRealtimeStreamInstance( | ||
| run.runtimeEnvironment, | ||
| run.realtimeStreamsVersion, | ||
| { run } | ||
| ); |
There was a problem hiding this comment.
🔍 Realtime stream route reverts run-store primary re-read fallback
resources...runs.$runParam.streams.$streamKey/route.tsx replaces the runStore.findRun + findRunOnPrimary replica-miss fallback and the controlPlaneResolver.resolveAuthenticatedEnv path with a direct $replica.taskRun.findFirst include. The removed code specifically handled replica lag nulling out a live run and 404-ing valid stream requests (per its own comment). This merge resolution may reintroduce that replica-lag 404 for freshly created runs. Confirm this regression is acceptable relative to main.
Was this helpful? React with 👍 or 👎 to provide feedback.
| import { Badge } from "~/components/primitives/Badge"; | ||
| import { RunTagInput } from "./RunTagInput"; | ||
| import { MachinePresetName } from "@trigger.dev/core/v3"; | ||
| import { InfoIconTooltip } from "~/components/primitives/Tooltip"; |
There was a problem hiding this comment.
🔍 ReplayRunDialog reverts conform API and adds a stray unused import
apps/webapp/app/components/runs/v3/ReplayRunDialog.tsx migrates from the newer @conform-to/react API (getFormProps/getInputProps/parseWithZod/field.errors) back to the older conform.*/parse/field.error API, and similar reverts appear across schedules.new, settings._index, environment-variables.new, waitpoints.complete and bulkaction routes. Additionally import { divide } from "effect/Duration"; (ReplayRunDialog.tsx:37) is an unused/erroneous import, and the payload panel className "rounded-smbg-charcoal-900" (:242) is a merged/mangled class (missing space between rounded-sm and bg-charcoal-900). These look like merge-resolution artifacts pulling in an older conform version; worth verifying the installed conform version matches the API used repo-wide.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
apps/webapp/app/components/AskAI.tsx (2)
41-41: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore or update the
AskAIRootconsumer.
apps/webapp/app/components/navigation/HelpAndFeedbackPopover.tsxstill imports and rendersAskAIRoot, so removing it fromAskAI.tsxwill break the build.
475-475: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse Tailwind v4 utility names.
This app pulls Tailwind CSS 4.3.1, so
flex-shrink-0andbg-gradient-to-brshould use the canonical v4 class names:
apps/webapp/app/components/AskAI.tsx#L475:flex-shrink-0→shrink-0apps/webapp/app/components/AskAI.tsx#L538-542:bg-gradient-to-br→bg-linear-to-brapps/webapp/app/components/code/TSQLResultsTable.tsx (1)
1011-1013: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve the keyboard focus ring consistently.
The input has a small
focus:ring-1, but it’s not scoped tofocus-visibleand is paired withfocus:outline-none, so keyboard-only focus can rely on a barely visible ring while forcing off any default outline. Usefocus-visible:ring-1/ring-offset-transparentor another keyboard-focused indicator scheme.
🧹 Nitpick comments (1)
apps/webapp/app/components/code/TSQLResultsTable.tsx (1)
932-932: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUse the v4
shrink-*utilities.Tailwind v4 canonicalizes these classes as
shrink-*, so replace the legacyflex-shrink-0classes withshrink-0in line 932 and the related row/filter classes at lines 948 and 969.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a78c24c5-3f6e-4ff3-925c-f5b138768c56
📒 Files selected for processing (10)
.changeset/openclaw-agent-integration.mdapps/webapp/app/components/AskAI.tsxapps/webapp/app/components/ErrorDisplay.tsxapps/webapp/app/components/LogoIcon.tsxapps/webapp/app/components/LogoType.tsxapps/webapp/app/components/code/AIQueryInput.tsxapps/webapp/app/components/code/TSQLEditor.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/logs/LogDetailView.tsxapps/webapp/app/components/logs/LogsTable.tsx
🚧 Files skipped from review as they are similar to previous changes (7)
- apps/webapp/app/components/ErrorDisplay.tsx
- apps/webapp/app/components/LogoIcon.tsx
- apps/webapp/app/components/code/AIQueryInput.tsx
- .changeset/openclaw-agent-integration.md
- apps/webapp/app/components/code/TSQLEditor.tsx
- apps/webapp/app/components/LogoType.tsx
- apps/webapp/app/components/logs/LogDetailView.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use types over interfaces for TypeScript
Avoid using enums; prefer string unions or const objects instead
**/*.{ts,tsx}: Prefer static imports over dynamicimport(); use dynamic imports only for unresolvable circular dependencies, genuine performance code splitting, or conditional runtime loading.
Import Trigger.dev tasks from@trigger.dev/sdk; never use@trigger.dev/sdk/v3or deprecatedclient.defineJob.
Add agentcrumbs while writing code using approved namespaces; mark lines with//@Crumbsor blocks with `// `#region` `@crumbs, and strip them before merging.
Files:
apps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/AskAI.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use zod for validation in packages/core and apps/webapp
Files:
apps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/AskAI.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use function declarations instead of default exports
Files:
apps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/AskAI.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
apps/webapp/**/*.{ts,tsx}: Access environment variables through theenvexport ofenv.server.tsinstead of directly accessingprocess.env
Use subpath exports from@trigger.dev/corepackage instead of importing from the root@trigger.dev/corepathDo not reintroduce the removed v1 execution path;
RunEngineVersion.V1branches may only reject or finalize gracefully so v3 clients receive a clean 4xx, never a 5xx.
Files:
apps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/AskAI.tsx
apps/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
For apps, use
typecheckfor verification and never usebuildas the correctness check.
Files:
apps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/AskAI.tsx
apps/webapp/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (apps/webapp/CLAUDE.md)
apps/webapp/app/**/*.{ts,tsx}: For dashboard changes, visually verify the running Remix app with Chrome DevTools MCP, using snapshots, screenshots, interaction, and console-message checks as appropriate.
UseuseCallbackanduseMemoonly for context provider values, expensive derived data used as a dependency, or stable references required by dependency arrays; do not wrap ordinary event handlers or trivial computations.
Use named constants for sentinel or placeholder values instead of scattering raw string literals across comparisons.
Files:
apps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/AskAI.tsx
🧠 Learnings (15)
📚 Learning: 2026-02-11T16:37:32.429Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3019
File: apps/webapp/app/components/primitives/charts/Card.tsx:26-30
Timestamp: 2026-02-11T16:37:32.429Z
Learning: In projects using react-grid-layout, avoid relying on drag-handle class to imply draggability. Ensure drag-handle elements only affect dragging when the parent grid item is configured draggable in the layout; conditionally apply cursor styles based on the draggable prop. This improves correctness and accessibility.
Applied to files:
apps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/AskAI.tsx
📚 Learning: 2026-03-22T13:26:12.060Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3244
File: apps/webapp/app/components/code/TextEditor.tsx:81-86
Timestamp: 2026-03-22T13:26:12.060Z
Learning: In the triggerdotdev/trigger.dev codebase, do not flag `navigator.clipboard.writeText(...)` calls for `missing-await`/`unhandled-promise` issues. These clipboard writes are intentionally invoked without `await` and without `catch` handlers across the project; keep that behavior consistent when reviewing TypeScript/TSX files (e.g., usages like in `apps/webapp/app/components/code/TextEditor.tsx`).
Applied to files:
apps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/AskAI.tsx
📚 Learning: 2026-03-22T19:24:14.403Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3187
File: apps/webapp/app/v3/services/alerts/deliverErrorGroupAlert.server.ts:200-204
Timestamp: 2026-03-22T19:24:14.403Z
Learning: In the triggerdotdev/trigger.dev codebase, webhook URLs are not expected to contain embedded credentials/secrets (e.g., fields like `ProjectAlertWebhookProperties` should only hold credential-free webhook endpoints). During code review, if you see logging or inclusion of raw webhook URLs in error messages, do not automatically treat it as a credential-leak/secrets-in-logs issue by default—first verify the URL does not contain embedded credentials (for example, no username/password in the URL, no obvious secret/token query params or fragments). If the URL is credential-free per this project’s conventions, allow the logging.
Applied to files:
apps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/AskAI.tsx
📚 Learning: 2026-05-18T08:21:27.694Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3632
File: apps/webapp/sentry.server.ts:4-21
Timestamp: 2026-05-18T08:21:27.694Z
Learning: When handling Prisma error P1001 ("Can't reach database server") in TypeScript, don’t assume a single error shape. Prisma can surface P1001 via two different error classes/fields: `PrismaClientKnownRequestError` exposes it as `err.code === "P1001"` (common during mid-query connection drops), while `PrismaClientInitializationError` exposes it as `err.errorCode === "P1001"` (common on client startup failure). Therefore, predicates should use `err.code === "P1001" || err.errorCode === "P1001"`. Do not flag `err.code === "P1001"` as “unreachable/never matches,” as it is expected in production.
Applied to files:
apps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/AskAI.tsx
📚 Learning: 2026-05-18T08:21:27.694Z
Learnt from: d-cs
Repo: triggerdotdev/trigger.dev PR: 3632
File: apps/webapp/sentry.server.ts:4-21
Timestamp: 2026-05-18T08:21:27.694Z
Learning: When handling Prisma errors for P1001 ("Can't reach database server"), do not assume it only appears under a single property name. Prisma may surface P1001 via either `PrismaClientKnownRequestError` (`err.code === "P1001"`, e.g., mid-query connection drops) or `PrismaClientInitializationError` (`err.errorCode === "P1001"`, e.g., client startup connection failure). To reliably detect the condition, check `err.code === "P1001" || err.errorCode === "P1001"`, and avoid review rules that would incorrectly flag `err.code === "P1001"` as unreachable/never-matching.
Applied to files:
apps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/AskAI.tsx
📚 Learning: 2026-06-13T19:53:13.759Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3937
File: packages/trigger-sdk/skills/realtime-and-frontend/SKILL.md:258-260
Timestamp: 2026-06-13T19:53:13.759Z
Learning: When reviewing code that uses `trigger.dev/react-hooks`’s `useRealtimeRun`, preserve the call signature where the first argument is the full realtime handle object (not `handle.id`). This is intentional to maintain type-safety and is consistent with the official docs; do not suggest changing the first argument from the handle object to `handle.id`.
Applied to files:
apps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/AskAI.tsx
📚 Learning: 2026-06-17T17:13:49.929Z
Learnt from: matt-aitken
Repo: triggerdotdev/trigger.dev PR: 3948
File: apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.bulk-actions.$bulkActionParam/route.tsx:48-62
Timestamp: 2026-06-17T17:13:49.929Z
Learning: In triggerdotdev/trigger.dev, within `dashboardLoader`/`dashboardAction` (or similar context resolver code) whenever you resolve an organization ID from an organization slug for RBAC/enterprise authorization scope, always read from the primary Prisma client (`prisma`), not `$replica`. Using `$replica` can hit replica-lag and cause the RBAC lookup/authorization to run without the correct org scope (bypassing intended role enforcement). Implement the slug→org lookup with `prisma.organization.findFirst(...)` (or equivalent primary-client query) and add an inline comment documenting why the primary client is required (replica lag could lead to unscoped RBAC checks).
Applied to files:
apps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/AskAI.tsx
📚 Learning: 2026-06-23T13:04:21.413Z
Learnt from: carderne
Repo: triggerdotdev/trigger.dev PR: 4023
File: apps/webapp/app/services/upsertBranch.server.ts:14-18
Timestamp: 2026-06-23T13:04:21.413Z
Learning: In TypeScript, it’s valid to `import { type X }` and then use `typeof X` in a type-only position, e.g. `type Alias = z.infer<typeof X>`. The `type` modifier suppresses the runtime import, but the type checker still has the full exported type so `z.infer<typeof X>` can resolve correctly. In code reviews, don’t flag this as a TypeScript compile error as long as `typeof X` is used in a type context (e.g., with `z.infer`, `type` aliases, generics), not as a runtime value.
Applied to files:
apps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/AskAI.tsx
📚 Learning: 2026-04-16T14:21:15.229Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3368
File: apps/webapp/app/components/logs/LogsTaskFilter.tsx:135-163
Timestamp: 2026-04-16T14:21:15.229Z
Learning: When rendering lists of task registry items in apps/webapp (e.g., <SelectItem /> rows) and using `key={item.slug}`, do not flag it as potentially non-unique. In trigger.dev’s `TaskIdentifier` table, the DB constraint `@unique([runtimeEnvironmentId, slug])` guarantees `slug` is unique within a given runtime environment, so `item.slug` is safe as the React key as long as the list is derived from that registry/constraint (and not from a legacy query that could produce duplicate slugs).
Applied to files:
apps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/AskAI.tsx
📚 Learning: 2026-05-08T21:00:20.973Z
Learnt from: samejr
Repo: triggerdotdev/trigger.dev PR: 3538
File: apps/webapp/app/components/primitives/Resizable.tsx:60-78
Timestamp: 2026-05-08T21:00:20.973Z
Learning: In the triggerdotdev/trigger.dev codebase, treat Zod as a boundary validation tool (API handlers, request/response validation, and storage/DB read/write validation), not as inline render-time validation inside React components/primitive UI code. For render-time guards, prefer small manual type-narrowing checks (e.g., a short predicate like ~10–20 lines) over importing Zod into UI primitives, to avoid per-render schema-parse overhead and unnecessary abstraction. Use the manual guard approach unless you truly need schema validation at a boundary; only then introduce Zod.
Applied to files:
apps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/AskAI.tsx
📚 Learning: 2026-06-25T18:21:55.847Z
Learnt from: carderne
Repo: triggerdotdev/trigger.dev PR: 4039
File: apps/webapp/app/routes/invite-resend.tsx:0-0
Timestamp: 2026-06-25T18:21:55.847Z
Learning: In the triggerdotdev/trigger.dev Zod 4 migration, avoid importing from the root package `conform-to/zod` in webapp code. It can resolve to the Zod 3 build and may crash at module load under Zod 4. When reviewing TypeScript/TSX files in `apps/webapp`, prefer importing from the Zod 4 subpath `conform-to/zod/v4` for Zod 4-compatible schemas/types.
Applied to files:
apps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/AskAI.tsx
📚 Learning: 2026-05-12T21:04:05.815Z
Learnt from: ericallam
Repo: triggerdotdev/trigger.dev PR: 3542
File: apps/webapp/app/components/sessions/v1/SessionStatus.tsx:1-3
Timestamp: 2026-05-12T21:04:05.815Z
Learning: In this Remix + TypeScript codebase, do not flag a server/client boundary violation when a file imports only types from a module matching `*.server`.
Specifically, it’s safe to import types using `import type { Foo } from "*.server"` or `import { type Foo } from "*.server"` because TypeScript erases type-only imports at compile time and they emit no JavaScript, so they won’t cross the Remix server/client bundle boundary.
Only raise the boundary concern for value imports (e.g., `import { Foo }` without `type`, or `import Foo`), since those produce JavaScript output.
Applied to files:
apps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/AskAI.tsx
📚 Learning: 2026-06-25T18:21:51.905Z
Learnt from: carderne
Repo: triggerdotdev/trigger.dev PR: 4039
File: apps/webapp/app/routes/invite-revoke.tsx:0-0
Timestamp: 2026-06-25T18:21:51.905Z
Learning: During the Zod v4 migration in the triggerdotdev/trigger.dev webapp, ensure any imports from `conform-to/zod` use the Zod-4 subpath: `conform-to/zod/v4` (e.g., `import { parseWithZod } from "conform-to/zod/v4"`). Do not import from the package root `conform-to/zod`, because it is the Zod 3 implementation and may load Zod-3-only symbols (e.g., `ZodBranded`, `ZodEffects`), which can throw at module load (notably with `zod4.4.3`). This should be enforced across `apps/webapp/**/*` where helpers like `parseWithZod` and `conformZodMessage` are used.
Applied to files:
apps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/AskAI.tsx
📚 Learning: 2026-07-03T17:10:21.498Z
Learnt from: 0ski
Repo: triggerdotdev/trigger.dev PR: 4148
File: apps/webapp/app/models/orgMember.server.ts:149-168
Timestamp: 2026-07-03T17:10:21.498Z
Learning: In triggerdotdev/trigger.dev, `User.email` (Prisma schema: `internal-packages/database/prisma/schema.prisma`) currently does NOT use `citext` and does NOT have a `lower(email)` functional unique index. Therefore, do not introduce Prisma queries like `where: { email: { equals: <value>, mode: "insensitive" } }` (or any case-insensitive lookup) against `User.email`, because it can force sequential scans of the `users` table under load. During review, ensure email is normalized (e.g., lowercased/trimmed) before both writes and subsequent lookups, and if true case-insensitive behavior/uniqueness is required, implement it via a separate app-wide migration (e.g., switch to `citext` and/or add a functional unique index with backfill) rather than bolting it onto individual feature PRs.
Applied to files:
apps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/AskAI.tsx
📚 Learning: 2026-06-25T18:21:54.729Z
Learnt from: carderne
Repo: triggerdotdev/trigger.dev PR: 4039
File: apps/webapp/app/routes/confirm-basic-details.tsx:0-0
Timestamp: 2026-06-25T18:21:54.729Z
Learning: For Remix + TypeScript files that use Conform v1 (conform-to/react) and its getInputProps helper, when you intend to suppress the helper-provided default value for non-checkbox/non-radio inputs (e.g., hidden inputs managed via an explicit value prop), use the Conform v1 option key `value: false`. Do not recommend `defaultValue: false` here, because `defaultValue` is not a valid option key for these input types in Conform v1 typings.
Applied to files:
apps/webapp/app/components/logs/LogsTable.tsxapps/webapp/app/components/code/TSQLResultsTable.tsxapps/webapp/app/components/AskAI.tsx
🔇 Additional comments (5)
apps/webapp/app/components/AskAI.tsx (2)
14-18: LGTM!Also applies to: 137-141, 180-180, 233-233, 256-256, 518-518
75-105: 🎯 Functional CorrectnessNo change needed. The
?aiHelp=handler consumes the parameter in the effect, replaces the URL without it, and the dependency array does not retrigger on provider rerenders/remounts.apps/webapp/app/components/logs/LogsTable.tsx (1)
1-29: LGTM!Also applies to: 118-118, 156-156, 192-192
apps/webapp/app/components/code/TSQLResultsTable.tsx (2)
840-842: 🎯 Functional CorrectnessVerify the new focus-visible styling remains visible to keyboard users.
The supplied change alters focus-visible outline handling. Confirm that it does not remove the only visible focus indicator, especially in forced-colors mode.
189-192: LGTM!Also applies to: 572-577, 593-594, 862-865, 1050-1050, 1116-1116, 1171-1171, 1225-1225, 1255-1263
1 participant
What
Adds OpenClaw agent integration — a new agent provisioning system with Slack webhooks, agent setup/status pages, and database models.
Changes
New files
routes/agents.$agentId.status.tsx— agent status dashboard pageroutes/agents.setup.tsx— agent setup wizardroutes/api.agents.provision.ts— provisioning APIroutes/webhooks.slack.ts— Slack webhook handlerinternal-packages/database/prisma/migrations/20260325122458_add_openclaw_agents/migration.sql.changeset/openclaw-agent-integration.mdSchema
AgentConfig,AgentExecution,AgentHealthCheckmodelsagentConfigsrelation to User modelManual resolutions (merge conflicts)
11 files conflicted during merge:
Branding cleanup
Reverted fork-specific branding (
AirTrigger→Trigger.dev,airtrigger.dev→trigger.dev) across 53 files.Notes
Co-authored-by: James Ritchie james@trigger.dev