feat(deployments): stream build server logs #2663
Conversation
Needed for using s2 client-side.
|
WalkthroughThis pull request adds S2 (streamstore) support for streaming deployment logs (env schema additions, DeploymentPresenter S2 token retrieval and Redis-backed caching, loader now returns s2Logs, client-side streaming, and a LogsDisplay UI). It also introduces new DateTime props Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
apps/webapp/app/components/primitives/Paragraph.tsx (1)
20-23: Remove duplicate variants and use existing ones.The newly added variants are exact duplicates of existing variants:
"small/dimmed"(lines 20-23) is identical to"small"(lines 12-15)"extra-small/dimmed"(lines 32-35) is identical to"extra-small"(lines 24-27)"extra-small/dimmed/mono"(lines 36-39) is identical to"extra-small/mono"(lines 40-43)Since the base variants already use
text-text-dimmed, the/dimmedsuffix adds no value and creates code duplication. Use the existing variant names directly in the consuming components.</review_comment_end>
Also applies to: 32-39
apps/webapp/app/components/primitives/DateTime.tsx (1)
9-27: Remove unusedhideDateprop from DateTime component's prop type or split prop types.
hideDateis included inDateTimePropsbut is only used byDateTimeAccurate(line 206), not byDateTime(line 27),SmartDateTime(line 127), orDateTimeShort(line 267). Consider either removinghideDatefrom the sharedDateTimePropstype or creating separate, more specific prop types for components that don't use this option.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
apps/webapp/app/components/primitives/DateTime.tsx(7 hunks)apps/webapp/app/components/primitives/Paragraph.tsx(2 hunks)apps/webapp/app/env.server.ts(1 hunks)apps/webapp/app/presenters/v3/DeploymentPresenter.server.ts(3 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx(6 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments/route.tsx(1 hunks)apps/webapp/package.json(1 hunks)apps/webapp/remix.config.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/components/primitives/Paragraph.tsxapps/webapp/app/env.server.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments/route.tsxapps/webapp/app/components/primitives/DateTime.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/components/primitives/Paragraph.tsxapps/webapp/app/env.server.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments/route.tsxapps/webapp/app/components/primitives/DateTime.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/components/primitives/Paragraph.tsxapps/webapp/app/env.server.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments/route.tsxapps/webapp/app/components/primitives/DateTime.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Files:
apps/webapp/app/env.server.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Files:
apps/webapp/app/env.server.tsapps/webapp/app/presenters/v3/DeploymentPresenter.server.ts
🧠 Learnings (6)
📚 Learning: 2025-08-29T10:06:49.293Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-08-29T10:06:49.293Z
Learning: Applies to {apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts} : Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Applied to files:
apps/webapp/app/env.server.ts
📚 Learning: 2025-08-29T10:06:49.293Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/webapp.mdc:0-0
Timestamp: 2025-08-29T10:06:49.293Z
Learning: Applies to apps/webapp/app/**/*.ts : Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Applied to files:
apps/webapp/app/env.server.ts
📚 Learning: 2025-08-14T12:13:20.455Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2392
File: packages/cli-v3/src/utilities/gitMeta.ts:195-218
Timestamp: 2025-08-14T12:13:20.455Z
Learning: In the GitMeta schema (packages/core/src/v3/schemas/common.ts), all fields are intentionally optional to handle partial data from various deployment contexts (local, GitHub Actions, GitHub App). Functions like getGitHubAppMeta() are designed to work with missing environment variables rather than validate their presence.
Applied to files:
apps/webapp/app/env.server.ts
📚 Learning: 2025-08-18T10:07:17.368Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-08-18T10:07:17.368Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Import Trigger.dev APIs from "trigger.dev/sdk/v3" when writing tasks or related utilities
Applied to files:
apps/webapp/app/presenters/v3/DeploymentPresenter.server.ts
📚 Learning: 2025-08-18T10:07:17.368Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-08-18T10:07:17.368Z
Learning: Applies to trigger.config.ts : Provide a valid Trigger.dev configuration using defineConfig with project ref and dirs (e.g., ["./trigger"]; tests/specs auto-excluded)
Applied to files:
apps/webapp/app/presenters/v3/DeploymentPresenter.server.ts
📚 Learning: 2025-10-08T11:48:12.327Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 2593
File: packages/core/src/v3/workers/warmStartClient.ts:168-170
Timestamp: 2025-10-08T11:48:12.327Z
Learning: The trigger.dev runners execute only in Node 21 and 22 environments, so modern Node.js APIs like AbortSignal.any (introduced in v20.3.0) are supported.
Applied to files:
apps/webapp/app/presenters/v3/DeploymentPresenter.server.ts
🧬 Code graph analysis (3)
apps/webapp/app/presenters/v3/DeploymentPresenter.server.ts (1)
apps/webapp/app/env.server.ts (1)
env(1211-1211)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments/route.tsx (1)
apps/webapp/app/components/primitives/Resizable.tsx (1)
ResizablePanel(46-46)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx (4)
apps/webapp/app/utils/cn.ts (1)
cn(77-79)apps/webapp/app/components/primitives/Paragraph.tsx (1)
Paragraph(88-107)apps/webapp/app/components/primitives/Tooltip.tsx (4)
TooltipProvider(128-128)Tooltip(128-128)TooltipTrigger(128-128)TooltipContent(128-128)apps/webapp/app/components/primitives/DateTime.tsx (1)
DateTimeAccurate(200-249)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
apps/webapp/app/components/primitives/DateTime.tsx (4)
65-82: LGTM: Consistent 24-hour format enforcement.Adding
hour12: falseensures consistent 24-hour time formatting across the application.
188-198: Behavioral change: hour format now uses leading zeros.The
hourformat changed from"numeric"to"2-digit"(line 190), which will display times as "09:30" instead of "9:30". This affects all components using time-only formatting (e.g.,SmartDateTimeandDateTimeAccurate). Verify this aligns with the desired UX.
200-249: LGTM: hideDate implementation is correct.The conditional logic properly handles the
hideDateflag by prioritizing it over thepreviousDatecomparison. WhenhideDateis true, time-only formatting is consistently applied.
173-185: LGTM: Comprehensive 24-hour format enforcement.The
hour12: falseaddition acrossformatSmartDateTime,formatDateTimeAccurate, andformatDateTimeShortensures consistent 24-hour time formatting throughout all date-time display components.Also applies to: 251-265, 281-293
...organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx (1)
88-156: Reset isStreaming to true when the effect re-runs.The effect correctly includes
s2Logs?.accessTokenin the dependency array (line 156), but it never setsisStreamingback totruewhen the token rotates and a new stream session begins. This leaves the UI showing "No logs yet" (line 562) instead of "Waiting for logs..." even though streaming is active.Apply this diff to reset the streaming state at the start of each effect run:
useEffect(() => { const abortController = new AbortController(); setLogs([]); setStreamError(null); + setIsStreaming(true); const streamLogs = async () => {
🧹 Nitpick comments (3)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx (1)
446-450: Consider checking scroll position before auto-scrolling.The current implementation always scrolls to the bottom when new logs arrive, which can be disruptive if users scroll up to review earlier entries. A common pattern is to auto-scroll only when the user is already near the bottom.
Example implementation:
useEffect(() => { if (logsContainerRef.current) { const { scrollTop, scrollHeight, clientHeight } = logsContainerRef.current; const isNearBottom = scrollHeight - scrollTop - clientHeight < 50; // 50px threshold if (isNearBottom) { logsContainerRef.current.scrollTop = scrollHeight; } } }, [logs]);apps/webapp/app/presenters/v3/DeploymentPresenter.server.ts (2)
166-179: Consider adding upfront checks for S2 configuration values.While the
tryCatchwrapper prevents the page from failing, the code attempts S2 operations even when required values might be missing:
project.externalRefcould benull(used on lines 168, 236, 244)env.S2_DEPLOYMENT_LOGS_BASIN_NAMEcould beundefined(used on lines 174, 241)env.S2_ACCESS_TOKENcould beundefineddespiteS2_ENABLEDbeing"1"Adding upfront validation avoids unnecessary API calls and produces clearer logs.
Apply this diff:
let s2Logs = undefined; - if (env.S2_ENABLED === "1" && gitMetadata?.source === "trigger_github_app") { + if ( + env.S2_ENABLED === "1" && + env.S2_ACCESS_TOKEN && + env.S2_DEPLOYMENT_LOGS_BASIN_NAME && + project.externalRef && + gitMetadata?.source === "trigger_github_app" + ) { const [error, accessToken] = await tryCatch(this.getS2AccessToken(project.externalRef));Based on learnings
236-236: Use Date.now() for cleaner timestamp generation.- id: `${projectRef}-${new Date().getTime()}`, + id: `${projectRef}-${Date.now()}`,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webapp/app/presenters/v3/DeploymentPresenter.server.ts(4 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx(6 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/presenters/v3/DeploymentPresenter.server.tsapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx
{apps/webapp/app/**/*.server.{ts,tsx},apps/webapp/app/routes/**/*.ts}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Access environment variables only via the env export from app/env.server.ts; do not reference process.env directly
Files:
apps/webapp/app/presenters/v3/DeploymentPresenter.server.ts
apps/webapp/app/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
Modules intended for test consumption under apps/webapp/app/**/*.ts must not read environment variables; accept configuration via options instead
Files:
apps/webapp/app/presenters/v3/DeploymentPresenter.server.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2663
File: apps/webapp/app/env.server.ts:1205-1206
Timestamp: 2025-11-10T09:09:07.392Z
Learning: In the trigger.dev webapp, S2_ACCESS_TOKEN and S2_DEPLOYMENT_LOGS_BASIN_NAME environment variables must remain optional until an OSS version of S2 is available, to avoid breaking environments that don't have S2 provisioned.
📚 Learning: 2025-11-10T09:09:07.392Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2663
File: apps/webapp/app/env.server.ts:1205-1206
Timestamp: 2025-11-10T09:09:07.392Z
Learning: In the trigger.dev webapp, S2_ACCESS_TOKEN and S2_DEPLOYMENT_LOGS_BASIN_NAME environment variables must remain optional until an OSS version of S2 is available, to avoid breaking environments that don't have S2 provisioned.
Applied to files:
apps/webapp/app/presenters/v3/DeploymentPresenter.server.ts
📚 Learning: 2025-08-18T10:07:17.368Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-08-18T10:07:17.368Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Inside tasks, prefer logger.debug/log/info/warn/error over ad-hoc console logging for structured logs
Applied to files:
apps/webapp/app/presenters/v3/DeploymentPresenter.server.ts
📚 Learning: 2025-08-18T10:07:17.368Z
Learnt from: CR
Repo: triggerdotdev/trigger.dev PR: 0
File: .cursor/rules/writing-tasks.mdc:0-0
Timestamp: 2025-08-18T10:07:17.368Z
Learning: Applies to **/trigger/**/*.{ts,tsx,js,jsx} : Import Trigger.dev APIs from "trigger.dev/sdk/v3" when writing tasks or related utilities
Applied to files:
apps/webapp/app/presenters/v3/DeploymentPresenter.server.ts
📚 Learning: 2025-10-08T11:48:12.327Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 2593
File: packages/core/src/v3/workers/warmStartClient.ts:168-170
Timestamp: 2025-10-08T11:48:12.327Z
Learning: The trigger.dev runners execute only in Node 21 and 22 environments, so modern Node.js APIs like AbortSignal.any (introduced in v20.3.0) are supported.
Applied to files:
apps/webapp/app/presenters/v3/DeploymentPresenter.server.ts
📚 Learning: 2025-08-14T12:13:20.455Z
Learnt from: myftija
Repo: triggerdotdev/trigger.dev PR: 2392
File: packages/cli-v3/src/utilities/gitMeta.ts:195-218
Timestamp: 2025-08-14T12:13:20.455Z
Learning: In the GitMeta schema (packages/core/src/v3/schemas/common.ts), all fields are intentionally optional to handle partial data from various deployment contexts (local, GitHub Actions, GitHub App). Functions like getGitHubAppMeta() are designed to work with missing environment variables rather than validate their presence.
Applied to files:
apps/webapp/app/presenters/v3/DeploymentPresenter.server.ts
🧬 Code graph analysis (2)
apps/webapp/app/presenters/v3/DeploymentPresenter.server.ts (2)
apps/webapp/app/env.server.ts (1)
env(1230-1230)apps/webapp/app/presenters/v3/BranchesPresenter.server.ts (1)
processGitMetadata(198-254)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx (2)
apps/webapp/app/components/primitives/Tooltip.tsx (4)
TooltipProvider(128-128)Tooltip(128-128)TooltipTrigger(128-128)TooltipContent(128-128)apps/webapp/app/components/primitives/DateTime.tsx (1)
DateTimeAccurate(200-249)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx (2)
44-67: LGTM! Loader correctly returns s2Logs alongside deployment.The loader properly destructures both
deploymentands2Logsfrom the presenter and returns them viatypedjson. This enables the client-side streaming logic to access S2 credentials when available.
425-609: LGTM! LogsDisplay component is well-structured.The component provides a clean UI for streaming logs with:
- Collapse/expand functionality tied to deployment status
- Copy-to-clipboard with visual feedback
- Error and warning counts with color-coded indicators
- Level-based styling (error/warn/info) with appropriate colors
- Proper timestamp rendering using
DateTimeAccuratewithhideDateapps/webapp/app/presenters/v3/DeploymentPresenter.server.ts (2)
1-31: LGTM! S2 client initialization is properly guarded.The conditional creation of the S2 client (line 31) based on
env.S2_ENABLEDand the Redis-backed token cache setup follow good practices for optional feature integration.
223-256: LGTM! Token caching strategy is sound.The Redis-backed caching with a 59-minute TTL (slightly shorter than the 1-hour token validity) prevents cache/token skew and reduces API calls to S2. The scope restrictions (read-only, basin/stream prefix constraints) follow the principle of least privilege.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx (1)
568-601: Consider using a stable identifier instead of index for the key prop.While using
indexas a key (line 571) is safe for append-only logs, it's generally discouraged as it can cause issues if logs are ever filtered, reordered, or have items removed. Consider using a combination oftimestampandindex, or add a unique identifier to each log entry.Example:
- <div - key={index} + <div + key={`${log.timestamp.getTime()}-${index}`} className={cn(Alternatively, if S2 provides a sequence number in the record, use that:
type LogEntry = { message: string; timestamp: Date; level: "info" | "error" | "warn"; seqNum: number; // Add sequence number from S2 record };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx(6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx
🧬 Code graph analysis (1)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx (4)
apps/webapp/app/utils/cn.ts (1)
cn(77-79)apps/webapp/app/components/primitives/Paragraph.tsx (1)
Paragraph(88-107)apps/webapp/app/components/primitives/Tooltip.tsx (4)
TooltipProvider(128-128)Tooltip(128-128)TooltipTrigger(128-128)TooltipContent(128-128)apps/webapp/app/components/primitives/DateTime.tsx (1)
DateTimeAccurate(200-249)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (17)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: typecheck / typecheck
🔇 Additional comments (1)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx (1)
88-157: LGTM! Past feedback has been addressed.The streaming effect now correctly:
- Includes
s2Logs?.accessTokenin the dependency array (line 157), ensuring the stream restarts when the token rotates- Calls
setIsStreaming(true)at the start (line 95), ensuring the UI reflects an active streamThe implementation properly handles cleanup with
AbortController, uses functional state updates for concurrent safety, and gracefully handles edge cases (stream not found, aborted requests).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/webapp/app/components/primitives/DateTime.tsx (1)
40-47: Propagatehour12into tooltip to keep display consistent.The tooltip always renders 12‑hour (defaults) regardless of the parent’s
hour12, causing mismatched formats between the button and tooltip. Passhour12intoTooltipContentand through itsformatDateTimecalls.Apply these diffs (call sites):
const tooltipContent = ( <TooltipContent realDate={realDate} timeZone={timeZone} localTimeZone={localTimeZone} locales={locales} + hour12={hour12} /> );const tooltipContent = ( <TooltipContent realDate={realDate} timeZone={timeZone} localTimeZone={localTimeZone} locales={locales} + hour12={hour12} /> );And update
TooltipContent:function TooltipContent({ realDate, timeZone, localTimeZone, locales, + hour12, }: { realDate: Date; timeZone?: string; localTimeZone: string; locales: string[]; + hour12?: boolean; }) { return ( <div className="flex flex-col gap-1"> <div className="flex flex-col gap-2.5 pb-1"> {timeZone && timeZone !== "UTC" && ( <DateTimeTooltipContent title={timeZone} - dateTime={formatDateTime(realDate, timeZone, locales, true, true)} + dateTime={formatDateTime(realDate, timeZone, locales, true, true, hour12)} isoDateTime={formatDateTimeISO(realDate, timeZone)} icon={<GlobeAmericasIcon className="size-4 text-purple-500" />} /> )} <DateTimeTooltipContent title="UTC" - dateTime={formatDateTime(realDate, "UTC", locales, true, true)} + dateTime={formatDateTime(realDate, "UTC", locales, true, true, hour12)} isoDateTime={formatDateTimeISO(realDate, "UTC")} icon={<GlobeAltIcon className="size-4 text-blue-500" />} /> <DateTimeTooltipContent title="Local" - dateTime={formatDateTime(realDate, localTimeZone, locales, true, true)} + dateTime={formatDateTime(realDate, localTimeZone, locales, true, true, hour12)} isoDateTime={formatDateTimeISO(realDate, localTimeZone)} icon={<Laptop className="size-4 text-green-500" />} />Also applies to: 239-245
♻️ Duplicate comments (1)
apps/webapp/app/components/primitives/DateTime.tsx (1)
18-19: Makehour12opt‑in; don’t override locale defaults by default.Defaulting
hour12 = trueforces 12‑hour clocks globally (regression for 24h locales). Instead, keephour12optional and only include it inIntl.DateTimeFormatwhen explicitly provided. Also propagate this change consistently across all helpers. This keeps backward compatibility for callers that passhour12, while preserving locale behavior when they don’t.
(Impacts all places timestamps render.)Apply these diffs:
- export const DateTime = ({ + export const DateTime = ({ date, timeZone, includeSeconds = true, includeTime = true, showTimezone = false, showTooltip = true, - hour12 = true, + hour12, }: DateTimeProps) => {-export function formatDateTime( +export function formatDateTime( date: Date, timeZone: string, locales: string[], includeSeconds: boolean, - includeTime: boolean, - hour12: boolean = true + includeTime: boolean, + hour12?: boolean ): string { - return new Intl.DateTimeFormat(locales, { + return new Intl.DateTimeFormat(locales, { year: "numeric", month: "short", day: "numeric", hour: includeTime ? "numeric" : undefined, minute: includeTime ? "numeric" : undefined, second: includeTime && includeSeconds ? "numeric" : undefined, timeZone, - hour12, + ...(hour12 !== undefined ? { hour12 } : {}), }).format(date); }-export const SmartDateTime = ({ date, previousDate = null, timeZone = "UTC", hour12 = true }: DateTimeProps) => { +export const SmartDateTime = ({ date, previousDate = null, timeZone = "UTC", hour12 }: DateTimeProps) => {-function formatSmartDateTime(date: Date, timeZone: string, locales: string[], hour12: boolean = true): string { +function formatSmartDateTime(date: Date, timeZone: string, locales: string[], hour12?: boolean): string { return new Intl.DateTimeFormat(locales, { month: "short", day: "numeric", hour: "numeric", minute: "numeric", second: "numeric", timeZone, // @ts-ignore fractionalSecondDigits works in most modern browsers fractionalSecondDigits: 3, - hour12, + ...(hour12 !== undefined ? { hour12 } : {}), }).format(date); }-function formatTimeOnly(date: Date, timeZone: string, locales: string[], hour12: boolean = true): string { +function formatTimeOnly(date: Date, timeZone: string, locales: string[], hour12?: boolean): string { return new Intl.DateTimeFormat(locales, { hour: "2-digit", minute: "numeric", second: "numeric", timeZone, // @ts-ignore fractionalSecondDigits works in most modern browsers fractionalSecondDigits: 3, - hour12, + ...(hour12 !== undefined ? { hour12 } : {}), }).format(date); }export const DateTimeAccurate = ({ date, timeZone = "UTC", previousDate = null, showTooltip = true, hideDate = false, - hour12 = true, + hour12, }: DateTimeProps) => {-function formatDateTimeAccurate(date: Date, timeZone: string, locales: string[], hour12: boolean = true): string { +function formatDateTimeAccurate(date: Date, timeZone: string, locales: string[], hour12?: boolean): string { const formattedDateTime = new Intl.DateTimeFormat(locales, { month: "short", day: "numeric", hour: "numeric", minute: "numeric", second: "numeric", timeZone, // @ts-ignore fractionalSecondDigits works in most modern browsers fractionalSecondDigits: 3, - hour12, + ...(hour12 !== undefined ? { hour12 } : {}), }).format(date);-export const DateTimeShort = ({ date, timeZone = "UTC", hour12 = true }: DateTimeProps) => { +export const DateTimeShort = ({ date, timeZone = "UTC", hour12 }: DateTimeProps) => {-function formatDateTimeShort(date: Date, timeZone: string, locales: string[], hour12: boolean = true): string { +function formatDateTimeShort(date: Date, timeZone: string, locales: string[], hour12?: boolean): string { const formattedDateTime = new Intl.DateTimeFormat(locales, { hour: "numeric", minute: "numeric", second: "numeric", timeZone, // @ts-ignore fractionalSecondDigits works in most modern browsers fractionalSecondDigits: 3, - hour12, + ...(hour12 !== undefined ? { hour12 } : {}), }).format(date); }Also applies to: 28-29, 74-76, 84-85, 131-131, 177-178, 187-188, 192-193, 200-201, 209-211, 256-258, 266-267, 272-272, 286-287, 294-295
🧹 Nitpick comments (2)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx (2)
105-105: Consider tracking the last sequence number to avoid re-fetching all logs.Starting from
seq_num: 0on every reconnection (e.g., token rotation) means re-reading all logs from the beginning. For short build logs this is acceptable, but you could optimize by tracking the last sequence number read and resuming from there.
571-571: Consider using a more stable key for log entries.Using
indexas the key works for append-only logs, but could cause React to reuse DOM nodes incorrectly when logs are cleared and refilled (line 93). Consider using a composite key like${log.timestamp.getTime()}-${index}for better stability, though the current implementation should function correctly since all content is derived from state.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webapp/app/components/primitives/DateTime.tsx(14 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx(6 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Always prefer using isomorphic code like fetch, ReadableStream, etc. instead of Node.js specific code
For TypeScript, we usually use types over interfaces
Avoid enums
No default exports, use function declarations
Files:
apps/webapp/app/components/primitives/DateTime.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx
{packages/core,apps/webapp}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
We use zod a lot in packages/core and in the webapp
Files:
apps/webapp/app/components/primitives/DateTime.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx
apps/webapp/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/webapp.mdc)
When importing from @trigger.dev/core in the webapp, never import the root package path; always use one of the documented subpath exports from @trigger.dev/core’s package.json
Files:
apps/webapp/app/components/primitives/DateTime.tsxapps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx
🧬 Code graph analysis (1)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx (2)
apps/webapp/app/components/primitives/Tooltip.tsx (4)
TooltipProvider(128-128)Tooltip(128-128)TooltipTrigger(128-128)TooltipContent(128-128)apps/webapp/app/components/primitives/DateTime.tsx (1)
DateTimeAccurate(204-254)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.deployments.$deploymentParam/route.tsx (7)
4-6: LGTM!The new imports support the S2 streaming integration and logs UI. All additions are used appropriately in the streaming logic and LogsDisplay component.
Also applies to: 28-33
44-67: LGTM!The loader correctly extends the return payload to include
s2Logsfrom the presenter, enabling client-side log streaming.
69-73: LGTM!The
LogEntrytype is well-defined with a discriminatedlevelunion, providing type safety for log parsing and rendering. Correctly usestypeoverinterfaceas per guidelines.
88-157: Previous review concerns addressed.The effect now correctly includes
s2Logs.accessTokenin the dependency array (line 157) and setsisStreaming(true)at the start (line 95), ensuring the stream restarts with fresh tokens and the UI reflects the active streaming state.
140-141: Verify silent handling of missing streams is intentional.The code silently ignores
stream_not_founderrors, which may occur when the build hasn't started producing logs yet. Please confirm this is the expected behavior and that users won't be confused by the absence of error feedback in legitimate failure scenarios.
250-262: LGTM!The conditional rendering ensures logs are only displayed when the feature is enabled. The
initialCollapsedlogic provides a sensible default: collapsing logs for successful or pending deployments while expanding them for failed states to highlight errors.
426-610: LGTM!The
LogsDisplaycomponent is well-implemented with thoughtful UX:
- Auto-scrolls to new logs for live streaming
- Copy-to-clipboard with visual feedback
- Color-coded log levels (info/warn/error) with counts
- Collapse/expand controls with smooth transitions
- Appropriate use of
DateTimeAccuratewithhideDateandhour12={false}for 24-hour log timestamps
This PR adds logs streaming for deployments triggered by the GitHub
integration. We use S2 and its typescript sdk to do this. A short-lived
read-only token is used to read the stream directly on the client side.
logs-streaming.mov