[Refactor] Update freestyle wrapper to use latest version and simplify code#1086
[Refactor] Update freestyle wrapper to use latest version and simplify code#1086
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Older cmux preview screenshots (latest comment is below)Preview Screenshots⏳ Preview screenshots are being captured... Workspace and dev browser links will appear here once the preview environment is ready. Generated by cmux preview system |
📝 WalkthroughWalkthroughFreestyle client usage moves from the sandboxes API to serverless.runs.create; dependency Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Email as EmailRendering
participant Wrapper as FreestyleWrapper
participant API as FreestyleServerlessAPI
Email->>Wrapper: executeScript(code, { config: { nodeModules } })
Wrapper->>API: serverless.runs.create({ code, config, ...options })
activate API
API-->>Wrapper: { status, data, error }
deactivate API
Note right of Wrapper `#DDFFDD`: Wrapper maps response to standardized {_status,_data} or {_status,_error}
Wrapper-->>Email: Result.ok(data.result) or Result.error(errorMessage)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (6)**/*.{tsx,ts,jsx,js}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/e2e/**/*.{test,spec}.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{test,spec}.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{tsx,ts}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (1)📚 Learning: 2026-01-07T00:55:19.856ZApplied to files:
🧬 Code graph analysis (1)apps/e2e/tests/backend/endpoints/api/v1/internal/failed-emails-digest.test.ts (1)
⏰ 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). (13)
🔇 Additional comments (1)
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 |
Greptile SummaryThis PR successfully migrates the freestyle library from version 0.0.92 to 0.1.5 and refactors the wrapper code to use the new
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant App as Email Rendering
participant Wrapper as Freestyle Wrapper
participant SDK as Freestyle SDK
App->>Wrapper: executeScript(code, {config: {nodeModules}})
Note over Wrapper: New v0.1.5 API
Wrapper->>Wrapper: Map options to API params
Wrapper->>Wrapper: Extract code and config
Wrapper->>SDK: serverless.runs.create({code, config, ...options})
SDK->>SDK: Execute script in sandbox
SDK-->>Wrapper: {result, logs}
Wrapper->>Wrapper: Handle response
Wrapper->>Wrapper: Retry on timeout
Wrapper-->>App: Result.ok({result, logs})
App->>App: Extract result data
App->>App: Return HTML/text
|
There was a problem hiding this comment.
Pull request overview
This PR migrates the freestyle-sandboxes dependency from version 0.0.92 to 0.1.5 and refactors the wrapper code to use the new serverless.runs.create API instead of the deprecated executeScript method. The wrapper maintains backward compatibility by keeping the same executeScript method signature.
Key Changes:
- Updated freestyle-sandboxes from v0.0.92 to v0.1.5
- Refactored internal API call from
executeScript()toserverless.runs.create() - Updated parameter structure:
nodeModulesis now nested under aconfigobject
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/backend/package.json | Bumps freestyle-sandboxes dependency from ^0.0.92 to ^0.1.5 |
| apps/backend/src/lib/freestyle.tsx | Refactors wrapper to use new API (serverless.runs.create), updates imports and types, changes parameter structure to nest options under config |
| apps/backend/src/lib/email-rendering.tsx | Updates all three call sites to pass nodeModules within a config object to match the new API structure |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/backend/src/lib/freestyle.tsx (1)
46-50: Consider simplifying the redundant config assignment.The explicit
config: options?.config ?? {}on line 48 is overwritten by the spread...optionson line 49 whenoptionscontains aconfigfield. While functionally correct, this pattern is redundant and could be simplified.🔎 Proposed simplification
- const response = await this.freestyle.serverless.runs.create({ - code: script, - config: options?.config ?? {}, - ...options, - }); + const response = await this.freestyle.serverless.runs.create({ + code: script, + config: {}, + ...options, + });This achieves the same result: if
optionscontainsconfig, the spread overwrites the default empty object; if not, the default is used.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
apps/backend/package.jsonapps/backend/src/lib/email-rendering.tsxapps/backend/src/lib/freestyle.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{tsx,ts,jsx,js}
📄 CodeRabbit inference engine (AGENTS.md)
For blocking alerts and errors, never use
toast; instead, use alerts as toasts are easily missed by the user
Files:
apps/backend/src/lib/freestyle.tsxapps/backend/src/lib/email-rendering.tsx
**/*.{tsx,css}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{tsx,css}: Keep hover/click animations snappy and fast; don't delay actions with pre-transitions (e.g., no fade-in on button hover) as it makes UI feel sluggish; instead apply transitions after the action like smooth fade-out when hover ends
When creating hover transitions, avoid hover-enter transitions and use only hover-exit transitions (e.g.,transition-colors hover:transition-none)
Files:
apps/backend/src/lib/freestyle.tsxapps/backend/src/lib/email-rendering.tsx
**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
NEVER use Next.js dynamic functions if avoidable; prefer using client components instead to keep pages static (e.g., use
usePathnameinstead ofawait params)
Files:
apps/backend/src/lib/freestyle.tsxapps/backend/src/lib/email-rendering.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: NEVER try-catch-all, NEVER void a promise, and NEVER use .catch(console.error) or similar; use loading indicators instead; if asynchronous handling is necessary, userunAsynchronouslyorrunAsynchronouslyWithAlertinstead
Use ES6 maps instead of records wherever possible
Files:
apps/backend/src/lib/freestyle.tsxapps/backend/src/lib/email-rendering.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Code defensively; prefer?? throwErr(...)over non-null assertions with good error messages explicitly stating violated assumptions
Avoid theanytype; when necessary, leave a comment explaining why it's used, why the type system fails, and how errors would be caught at compile-, test-, or runtime
Files:
apps/backend/src/lib/freestyle.tsxapps/backend/src/lib/email-rendering.tsx
🧠 Learnings (1)
📚 Learning: 2026-01-07T00:55:19.856Z
Learnt from: CR
Repo: stack-auth/stack-auth PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-07T00:55:19.856Z
Learning: Applies to **/config/schema.ts,**/config/**/*.{ts,tsx} : Whenever making backwards-incompatible changes to the config schema, update the migration functions in `packages/stack-shared/src/config/schema.ts`
Applied to files:
apps/backend/src/lib/email-rendering.tsx
⏰ 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). (15)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (javascript-typescript)
- GitHub Check: Vercel Agent Review
- GitHub Check: build (22.x)
- GitHub Check: lint_and_build (latest)
- GitHub Check: E2E Tests (Node 22.x, Freestyle prod)
- GitHub Check: E2E Tests (Node 22.x, Freestyle mock)
- GitHub Check: setup-tests-with-custom-base-port
- GitHub Check: restart-dev-and-test-with-custom-base-port
- GitHub Check: docker
- GitHub Check: setup-tests
- GitHub Check: restart-dev-and-test
- GitHub Check: build (22.x)
- GitHub Check: check_prisma_migrations (22.x)
- GitHub Check: all-good
🔇 Additional comments (9)
apps/backend/src/lib/freestyle.tsx (5)
6-11: LGTM: Type extraction and definitions.The type extraction using
Parameters<...>[0]correctly captures the serverless runs API signature, and making itPartial<Omit<..., 'code'>>appropriately allows callers to pass optional configuration while ensuringcodeis handled separately.
14-14: LGTM: Client type updated consistently.
26-30: LGTM: Constructor correctly instantiates FreestyleClient.
32-32: LGTM: Function signature updated to accept new options type.
38-38: LGTM: Tracing attributes updated for new config structure.Correctly reads
nodeModulescount from the nestedoptions.config.nodeModulespath.apps/backend/src/lib/email-rendering.tsx (3)
139-139: LGTM: Call site correctly updated for new API shape.The options are now passed as
{ config: { nodeModules } }, which correctly matches the newExecuteScriptOptionstype defined infreestyle.tsx.
243-243: LGTM: Call site correctly updated for new API shape.Consistent with the other call sites, options are passed as
{ config: { nodeModules } }.
365-365: LGTM: Call site correctly updated for new API shape.All three call sites in this file have been consistently updated to use the new nested config structure.
apps/backend/package.json (1)
81-81: Version is valid and has no known security advisories.The library version 0.1.5 is the latest release on npm, published a week ago by legitimate maintainers, with no security deprecations or known vulnerabilities in the registry.
Note that with the old version, the options and the code were different arguments. Now they are one argument. This is why we have to extract the params differently. There is no change in function with this commit.
nams1570
force-pushed
the
bump-freestyle-error-handling
branch
from
d083fc9 to
0b550a1
Compare
Older cmux preview screenshots (latest comment is below)Preview ScreenshotsOpen Workspace (1 hr expiry) · Open Dev Browser (1 hr expiry) · Open Diff Heatmap Screenshot capture was skipped.
Generated by cmux preview system |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/backend/src/lib/freestyle.tsx (2)
38-38: Consider consistent optional chaining for TypeScript narrowing.The logic is correct, but TypeScript may not narrow the type properly when the condition uses
options?.config?.nodeModulesand the consequence usesoptions.config.nodeModules(without?). For better type safety and consistency, consider using optional chaining in both places.🔎 Proposed refinement
- 'freestyle.nodeModules.count': options?.config?.nodeModules ? Object.keys(options.config.nodeModules).length.toString() : '0', + 'freestyle.nodeModules.count': options?.config?.nodeModules ? Object.keys(options?.config?.nodeModules).length.toString() : '0',
53-53: Add explanatory comment foranyusage.Per coding guidelines, when using the
anytype, leave a comment explaining why it's used and why the type system fails. In this case, the standardErrortype doesn't include thecodeproperty, which is a Node.js-specific extension for network errors.As per coding guidelines, which require comments explaining
anyusage.🔎 Suggested comment
+ // Cast to any because Node.js errors have a non-standard 'code' property + // that's not included in the standard Error type if (e instanceof Error && (e as any).code === "ETIMEDOUT") {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
apps/backend/package.jsonapps/backend/src/lib/email-rendering.tsxapps/backend/src/lib/freestyle.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/backend/package.json
- apps/backend/src/lib/email-rendering.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{tsx,ts,jsx,js}
📄 CodeRabbit inference engine (AGENTS.md)
For blocking alerts and errors, never use
toast; instead, use alerts as toasts are easily missed by the user
Files:
apps/backend/src/lib/freestyle.tsx
**/*.{tsx,css}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{tsx,css}: Keep hover/click animations snappy and fast; don't delay actions with pre-transitions (e.g., no fade-in on button hover) as it makes UI feel sluggish; instead apply transitions after the action like smooth fade-out when hover ends
When creating hover transitions, avoid hover-enter transitions and use only hover-exit transitions (e.g.,transition-colors hover:transition-none)
Files:
apps/backend/src/lib/freestyle.tsx
**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
NEVER use Next.js dynamic functions if avoidable; prefer using client components instead to keep pages static (e.g., use
usePathnameinstead ofawait params)
Files:
apps/backend/src/lib/freestyle.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: NEVER try-catch-all, NEVER void a promise, and NEVER use .catch(console.error) or similar; use loading indicators instead; if asynchronous handling is necessary, userunAsynchronouslyorrunAsynchronouslyWithAlertinstead
Use ES6 maps instead of records wherever possible
Files:
apps/backend/src/lib/freestyle.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Code defensively; prefer?? throwErr(...)over non-null assertions with good error messages explicitly stating violated assumptions
Avoid theanytype; when necessary, leave a comment explaining why it's used, why the type system fails, and how errors would be caught at compile-, test-, or runtime
Files:
apps/backend/src/lib/freestyle.tsx
🧬 Code graph analysis (1)
apps/backend/src/lib/freestyle.tsx (2)
apps/backend/src/utils/telemetry.tsx (1)
traceSpan(13-27)packages/stack-shared/src/utils/telemetry.tsx (1)
traceSpan(13-27)
⏰ 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). (13)
- GitHub Check: build (22.x)
- GitHub Check: restart-dev-and-test-with-custom-base-port
- GitHub Check: restart-dev-and-test
- GitHub Check: setup-tests-with-custom-base-port
- GitHub Check: setup-tests
- GitHub Check: lint_and_build (latest)
- GitHub Check: all-good
- GitHub Check: E2E Tests (Node 22.x, Freestyle prod)
- GitHub Check: E2E Tests (Node 22.x, Freestyle mock)
- GitHub Check: build (22.x)
- GitHub Check: Vercel Agent Review
- GitHub Check: check_prisma_migrations (22.x)
- GitHub Check: docker
🔇 Additional comments (3)
apps/backend/src/lib/freestyle.tsx (3)
8-11: Well-structured type extraction from the SDK.The approach of deriving
ExecuteScriptOptionsfrom the SDK's method signature ensures type safety and stays in sync with the library. Making all fieldsPartialis appropriate sincecodeis the only required parameter and is provided separately.
46-50: Object construction and response handling are correct; no changes needed.The object construction order is correct. Since
configis set AFTER spreadingoptions, it properly overrides anyundefinedconfig value with an empty object. All call sites inemail-rendering.tsx(lines 139, 243, 365) correctly handle the response structure by accessingexecuteResult.data.resultand checking the error status first, confirming compatibility with the new API response shape.
52-58: Verify error handling compatibility with the new API, and add explanation foranytype usage.The error handling logic has been preserved from the old implementation. However, I cannot definitively confirm timeout behavior without Freestyle SDK documentation or test cases.
Two concerns:
ETIMEDOUT detection: The
(e as any).code === "ETIMEDOUT"check uses a standard Node.js error pattern that socket/HTTP libraries follow, so this should remain compatible. However, the code violates the guideline against using theanytype without explanation. Add a comment explaining why type narrowing cannot be used here and how errors would be caught at runtime.Nested error unwrapping: The outer catch block's multi-level unwrapping (e.error.error) aligns with the documented workaround in line 60's comment. Without Freestyle SDK documentation or integration tests covering error scenarios, verification of continued compatibility cannot be conclusively determined.
Consider adding integration tests that exercise error paths with the new API to confirm the error format has not changed.
apps/backend/src/lib/freestyle.tsx
Outdated
| return Result.ok(Result.orThrow(await Result.retry(async () => { | ||
| try { | ||
| return Result.ok(await this.freestyle.executeScript(script, options)); | ||
| // New API: freestyle.serverless.runs.create({ code, config, ... }) |
There was a problem hiding this comment.
| // New API: freestyle.serverless.runs.create({ code, config, ... }) |
apps/backend/src/lib/freestyle.tsx
Outdated
| try { | ||
| return Result.ok(await this.freestyle.executeScript(script, options)); | ||
| // New API: freestyle.serverless.runs.create({ code, config, ... }) | ||
| // Returns { result, logs } on success |
There was a problem hiding this comment.
| // Returns { result, logs } on success |
| } catch (e: unknown) { | ||
| // for whatever reason, Freestyle's errors are sometimes returned in JSON.parse(e.error.error).error (lol) | ||
| const wrap1 = e && typeof e === "object" && "error" in e ? e.error : e; | ||
| const wrap2 = wrap1 && typeof wrap1 === "object" && "error" in wrap1 ? wrap1.error : wrap1; | ||
| const wrap3 = wrap2 && typeof wrap2 === "string" ? Result.or(parseJson(wrap2), wrap2) : wrap2; | ||
| const wrap4 = wrap3 && typeof wrap3 === "object" && "error" in wrap3 ? wrap3.error : wrap3; | ||
| return Result.error(`${wrap4}`); | ||
| } | ||
| }); |
There are retries further up in the stack. An additional retry here does not accomplish much. Thanks to the freestyle updates, we get the error message directly so there isn't a need to hack it.
Older cmux preview screenshots (latest comment is below)Preview Screenshots⏳ Preview screenshots are being captured... Workspace and dev browser links will appear here once the preview environment is ready. Generated by cmux preview system |
nams1570
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @apps/backend/src/lib/freestyle.tsx:
- Around line 49-51: The existing misleading comment about nested Freestyle
errors should be removed or updated: delete the line "// Freestyle's errors are
sometimes nested in JSON.parse(e.error.error).error" and either remove the
comment entirely or replace it with a short note that Freestyle >=0.1.5 returns
top-level error and message fields; adjust around the error-handling block that
constructs message from e (the e instanceof Error ? e.message : String(e) line)
and returns Result.error(message) to reflect the simplified error shape.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/backend/src/lib/freestyle.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{tsx,ts,jsx,js}
📄 CodeRabbit inference engine (AGENTS.md)
For blocking alerts and errors, never use
toast; instead, use alerts as toasts are easily missed by the user
Files:
apps/backend/src/lib/freestyle.tsx
**/*.{tsx,css}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{tsx,css}: Keep hover/click animations snappy and fast; don't delay actions with pre-transitions (e.g., no fade-in on button hover) as it makes UI feel sluggish; instead apply transitions after the action like smooth fade-out when hover ends
When creating hover transitions, avoid hover-enter transitions and use only hover-exit transitions (e.g.,transition-colors hover:transition-none)
Files:
apps/backend/src/lib/freestyle.tsx
**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
NEVER use Next.js dynamic functions if avoidable; prefer using client components instead to keep pages static (e.g., use
usePathnameinstead ofawait params)
Files:
apps/backend/src/lib/freestyle.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: NEVER try-catch-all, NEVER void a promise, and NEVER use .catch(console.error) or similar; use loading indicators instead; if asynchronous handling is necessary, userunAsynchronouslyorrunAsynchronouslyWithAlertinstead
Use ES6 maps instead of records wherever possible
Files:
apps/backend/src/lib/freestyle.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Code defensively; prefer?? throwErr(...)over non-null assertions with good error messages explicitly stating violated assumptions
Avoid theanytype; when necessary, leave a comment explaining why it's used, why the type system fails, and how errors would be caught at compile-, test-, or runtime
Files:
apps/backend/src/lib/freestyle.tsx
🧬 Code graph analysis (1)
apps/backend/src/lib/freestyle.tsx (2)
apps/backend/src/utils/telemetry.tsx (1)
traceSpan(13-27)packages/stack-shared/src/utils/telemetry.tsx (1)
traceSpan(13-27)
⏰ 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). (13)
- GitHub Check: Vercel Agent Review
- GitHub Check: docker
- GitHub Check: restart-dev-and-test
- GitHub Check: all-good
- GitHub Check: restart-dev-and-test-with-custom-base-port
- GitHub Check: check_prisma_migrations (22.x)
- GitHub Check: setup-tests-with-custom-base-port
- GitHub Check: build (22.x)
- GitHub Check: build (22.x)
- GitHub Check: E2E Tests (Node 22.x, Freestyle mock)
- GitHub Check: E2E Tests (Node 22.x, Freestyle prod)
- GitHub Check: setup-tests
- GitHub Check: lint_and_build (latest)
🔇 Additional comments (3)
apps/backend/src/lib/freestyle.tsx (3)
8-11: LGTM! Well-structured type definition.The type derivation is clean and well-documented. Excluding 'code' and making fields optional provides good flexibility for callers.
42-46: Object construction is correct; Copilot's concern doesn't apply.The concern from the past review about
config: undefinedoverwriting the default is not valid for this code. Since theconfig: options?.config ?? {}assignment on line 45 occurs after the spread on line 43, it correctly overwrites anyundefinedvalue from the spread. JavaScript object literals apply properties in order, so the last assignment wins.
38-38: Correct update for nested config structure.The access pattern correctly reflects the new API structure where
nodeModulesis nested underconfig. The optional chaining properly handles cases where config is undefined.
Freestyle runtime errors make it return status ok, but data.result undefined. Since it isn't easy to distinguish these from cases where the executed code just returns nothing, we make changes. We wrap the script sent to freestyle in a try catch. This way, we can force it to return an error if there is a runtime/compile time error. We remove the old code to deal with the result undefined as it is superfluous now.
Older cmux preview screenshots (latest comment is below)Preview Screenshots⏳ Preview screenshots are being captured... Workspace and dev browser links will appear here once the preview environment is ready. Generated by cmux preview system |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @apps/backend/src/lib/email-rendering.tsx:
- Around line 367-371: The return is casting execution.data to an array without
validation; add defensive checks similar to the earlier validation at line 251:
verify execution.data is an Array and every item is an object with required
string properties (html and text, optional subject and notificationCategory)
before returning Result.ok; if the check fails return Result.error with a clear
message (e.g., "Invalid email rendering data") so callers don’t receive an
unsafe cast.
- Around line 147-150: The code returns Result.ok by blindly casting
executeResult.data to the expected shape; add defensive runtime validation of
executeResult.data before casting: check that executeResult.data exists and that
html, text, subject, and notificationCategory are present and of the expected
types (strings), and if any check fails return Result.error with a clear message
indicating which field is missing/invalid rather than casting; update the branch
that currently does Result.ok(executeResult.data as { html: string, text:
string, subject: string, notificationCategory: string }) to perform these checks
and only call Result.ok with the validated object.
- Around line 248-251: The code currently casts executeResult.data to the
expected array shape without validation; add defensive checks (e.g.,
Array.isArray(executeResult.data) and that every item has string properties html
and text and optional string subject/notificationCategory) before calling
Result.ok in place of the direct cast used around executeResult and Result.ok;
if validation fails return Result.error with a helpful message. Implement this
as a small type-guard/validator function (e.g., isRenderedEmailArray or
validateRenderedEmails) and use it where executeResult is handled so you never
assume the shape when calling Result.ok.
🧹 Nitpick comments (3)
apps/backend/src/lib/email-rendering.tsx (3)
118-128: Consider preserving error stack traces.The error serialization using
String(e)may lose valuable debugging information like stack traces. Consider usinge.stack || String(e)or a more detailed error serialization to aid in debugging runtime failures in the sandboxed environment.💡 Proposed enhancement for error serialization
export default async () => { try { const result = await renderAll(); return { _status: "ok", _data: result }; } catch (e) { - return { _status: "error", _error: String(e) }; + return { _status: "error", _error: e instanceof Error ? (e.stack || e.message) : String(e) }; } };
219-228: Consider preserving error stack traces.Same as the earlier entry.js wrapper, using
String(e)may lose valuable debugging information. Consider usinge.stack || String(e)or more detailed error serialization.💡 Proposed enhancement for error serialization
export default async () => { try { const result = await renderAll(); return { _status: "ok", _data: result }; } catch (e) { - return { _status: "error", _error: String(e) }; + return { _status: "error", _error: e instanceof Error ? (e.stack || e.message) : String(e) }; } };
337-344: Consider preserving error stack traces.Same recommendation as the previous entry.js wrappers: consider using
e.stack || String(e)for better debugging information.💡 Proposed enhancement for error serialization
export default async function() { try { const result = await renderAll(); return { _status: "ok", _data: result }; } catch (e) { - return { _status: "error", _error: String(e) }; + return { _status: "error", _error: e instanceof Error ? (e.stack || e.message) : String(e) }; } };
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/backend/src/lib/email-rendering.tsxapps/backend/src/lib/freestyle.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/backend/src/lib/freestyle.tsx
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{tsx,ts,jsx,js}
📄 CodeRabbit inference engine (AGENTS.md)
For blocking alerts and errors, never use
toast; instead, use alerts as toasts are easily missed by the user
Files:
apps/backend/src/lib/email-rendering.tsx
**/*.{tsx,css}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{tsx,css}: Keep hover/click animations snappy and fast; don't delay actions with pre-transitions (e.g., no fade-in on button hover) as it makes UI feel sluggish; instead apply transitions after the action like smooth fade-out when hover ends
When creating hover transitions, avoid hover-enter transitions and use only hover-exit transitions (e.g.,transition-colors hover:transition-none)
Files:
apps/backend/src/lib/email-rendering.tsx
**/*.{tsx,ts}
📄 CodeRabbit inference engine (AGENTS.md)
NEVER use Next.js dynamic functions if avoidable; prefer using client components instead to keep pages static (e.g., use
usePathnameinstead ofawait params)
Files:
apps/backend/src/lib/email-rendering.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: NEVER try-catch-all, NEVER void a promise, and NEVER use .catch(console.error) or similar; use loading indicators instead; if asynchronous handling is necessary, userunAsynchronouslyorrunAsynchronouslyWithAlertinstead
Use ES6 maps instead of records wherever possible
Files:
apps/backend/src/lib/email-rendering.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Code defensively; prefer?? throwErr(...)over non-null assertions with good error messages explicitly stating violated assumptions
Avoid theanytype; when necessary, leave a comment explaining why it's used, why the type system fails, and how errors would be caught at compile-, test-, or runtime
Files:
apps/backend/src/lib/email-rendering.tsx
🧠 Learnings (1)
📚 Learning: 2025-10-11T04:13:19.308Z
Learnt from: N2D4
Repo: stack-auth/stack-auth PR: 943
File: examples/convex/app/action/page.tsx:23-28
Timestamp: 2025-10-11T04:13:19.308Z
Learning: In the stack-auth codebase, use `runAsynchronouslyWithAlert` from `stackframe/stack-shared/dist/utils/promises` for async button click handlers and form submissions instead of manual try/catch blocks. This utility automatically handles errors and shows alerts to users.
Applied to files:
apps/backend/src/lib/email-rendering.tsx
⏰ 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). (13)
- GitHub Check: Vercel Agent Review
- GitHub Check: restart-dev-and-test
- GitHub Check: docker
- GitHub Check: lint_and_build (latest)
- GitHub Check: E2E Tests (Node 22.x, Freestyle prod)
- GitHub Check: E2E Tests (Node 22.x, Freestyle mock)
- GitHub Check: all-good
- GitHub Check: setup-tests-with-custom-base-port
- GitHub Check: check_prisma_migrations (22.x)
- GitHub Check: build (22.x)
- GitHub Check: build (22.x)
- GitHub Check: setup-tests
- GitHub Check: restart-dev-and-test-with-custom-base-port
🔇 Additional comments (4)
apps/backend/src/lib/email-rendering.tsx (4)
247-247: Consistent config structure.This follows the same config nesting pattern as line 146. The earlier verification comment applies here as well.
366-366: Consistent config structure.This follows the same config nesting pattern as the earlier changes. Good consistency across all three rendering functions.
146-146: Config structure correctly matches Freestyle API.The options structure
{ config: { nodeModules } }aligns with the Freestyleserverless.runs.createAPI, which expectsnodeModulesto be nested within aconfigobject.
4-4: Error tracking is preserved through structured error handling, not removed.The
captureErrorimport was removed fromemail-rendering.tsx, but error tracking is properly handled in the calling code. TherenderEmailsForTenancyBatchedfunction returns aResulttype withstatusanderrorfields. Inemail-queue-step.tsx, all calls checkif (result.status === "error")and invokecaptureError("email-rendering-failed", result.error)to track errors. This is a cleaner separation of concerns—the rendering library returns structured errors, and the caller is responsible for error tracking based on the result.
| if (executeResult.status === "error") { | ||
| return Result.error(`${executeResult.error}`); | ||
| } | ||
| if (!executeResult.data.result) { | ||
| const noResultError = new StackAssertionError("No result from Freestyle", { | ||
| executeResult, | ||
| templateOrDraftComponent, | ||
| themeComponent, | ||
| options, | ||
| }); | ||
| captureError("freestyle-no-result", noResultError); | ||
| throw noResultError; | ||
| } | ||
| return Result.ok(executeResult.data.result as { html: string, text: string, subject: string, notificationCategory: string }); | ||
| return Result.ok(executeResult.data as { html: string, text: string, subject: string, notificationCategory: string }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add defensive validation before type casting.
The code checks for error status but doesn't validate that executeResult.data actually contains the expected { html, text, subject, notificationCategory } structure before casting. According to coding guidelines, prefer defensive coding with explicit error messages.
🛡️ Proposed defensive validation
if (executeResult.status === "error") {
return Result.error(`${executeResult.error}`);
}
- return Result.ok(executeResult.data as { html: string, text: string, subject: string, notificationCategory: string });
+ const data = executeResult.data as any;
+ if (!data || typeof data.html !== 'string' || typeof data.text !== 'string') {
+ throw new StackAssertionError("Freestyle returned invalid email rendering result", { data });
+ }
+ return Result.ok(data as { html: string, text: string, subject?: string, notificationCategory?: string });Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @apps/backend/src/lib/email-rendering.tsx around lines 147 - 150, The code
returns Result.ok by blindly casting executeResult.data to the expected shape;
add defensive runtime validation of executeResult.data before casting: check
that executeResult.data exists and that html, text, subject, and
notificationCategory are present and of the expected types (strings), and if any
check fails return Result.error with a clear message indicating which field is
missing/invalid rather than casting; update the branch that currently does
Result.ok(executeResult.data as { html: string, text: string, subject: string,
notificationCategory: string }) to perform these checks and only call Result.ok
with the validated object.
| if (executeResult.status === "error") { | ||
| return Result.error(executeResult.error); | ||
| } | ||
| if (!executeResult.data.result) { | ||
| const noResultError = new StackAssertionError("No result from Freestyle", { | ||
| executeResult, | ||
| templateOrDraftComponent, | ||
| themeComponent, | ||
| inputs, | ||
| }); | ||
| captureError("freestyle-no-result", noResultError); | ||
| throw noResultError; | ||
| } | ||
| return Result.ok(executeResult.data.result as Array<{ html: string, text: string, subject?: string, notificationCategory?: string }>); | ||
| return Result.ok(executeResult.data as Array<{ html: string, text: string, subject?: string, notificationCategory?: string }>); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add defensive validation before type casting.
Similar to line 150, this casts the result without validating the data structure. Add defensive checks to ensure executeResult.data is an array with the expected shape.
🛡️ Proposed defensive validation
if (executeResult.status === "error") {
return Result.error(executeResult.error);
}
- return Result.ok(executeResult.data as Array<{ html: string, text: string, subject?: string, notificationCategory?: string }>);
+ const data = executeResult.data as any;
+ if (!Array.isArray(data)) {
+ throw new StackAssertionError("Freestyle returned non-array result for batched rendering", { data });
+ }
+ return Result.ok(data as Array<{ html: string, text: string, subject?: string, notificationCategory?: string }>);📝 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.
| if (executeResult.status === "error") { | |
| return Result.error(executeResult.error); | |
| } | |
| if (!executeResult.data.result) { | |
| const noResultError = new StackAssertionError("No result from Freestyle", { | |
| executeResult, | |
| templateOrDraftComponent, | |
| themeComponent, | |
| inputs, | |
| }); | |
| captureError("freestyle-no-result", noResultError); | |
| throw noResultError; | |
| } | |
| return Result.ok(executeResult.data.result as Array<{ html: string, text: string, subject?: string, notificationCategory?: string }>); | |
| return Result.ok(executeResult.data as Array<{ html: string, text: string, subject?: string, notificationCategory?: string }>); | |
| if (executeResult.status === "error") { | |
| return Result.error(executeResult.error); | |
| } | |
| // Cast to any for runtime validation; the type system cannot verify executeResult.data structure at compile time | |
| const data = executeResult.data as any; | |
| if (!Array.isArray(data)) { | |
| throw new StackAssertionError("Freestyle returned non-array result for batched rendering", { data }); | |
| } | |
| return Result.ok(data as Array<{ html: string, text: string, subject?: string, notificationCategory?: string }>); |
🤖 Prompt for AI Agents
In @apps/backend/src/lib/email-rendering.tsx around lines 248 - 251, The code
currently casts executeResult.data to the expected array shape without
validation; add defensive checks (e.g., Array.isArray(executeResult.data) and
that every item has string properties html and text and optional string
subject/notificationCategory) before calling Result.ok in place of the direct
cast used around executeResult and Result.ok; if validation fails return
Result.error with a helpful message. Implement this as a small
type-guard/validator function (e.g., isRenderedEmailArray or
validateRenderedEmails) and use it where executeResult is handled so you never
assume the shape when calling Result.ok.
| if (execution.status === "error") { | ||
| return Result.error(execution.error); | ||
| } | ||
| if (!execution.data.result) { | ||
| const noResultError = new StackAssertionError("No result from Freestyle", { | ||
| execution, | ||
| requests, | ||
| }); | ||
| captureError("freestyle-no-result", noResultError); | ||
| throw noResultError; | ||
| } | ||
|
|
||
| return Result.ok(execution.data.result as Array<{ html: string, text: string, subject?: string, notificationCategory?: string }>); | ||
| return Result.ok(execution.data as Array<{ html: string, text: string, subject?: string, notificationCategory?: string }>); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add defensive validation before type casting.
Same as line 251, add defensive checks to ensure the result is a valid array before casting.
🛡️ Proposed defensive validation
if (execution.status === "error") {
return Result.error(execution.error);
}
- return Result.ok(execution.data as Array<{ html: string, text: string, subject?: string, notificationCategory?: string }>);
+ const data = execution.data as any;
+ if (!Array.isArray(data)) {
+ throw new StackAssertionError("Freestyle returned non-array result for tenancy batched rendering", { data });
+ }
+ return Result.ok(data as Array<{ html: string, text: string, subject?: string, notificationCategory?: string }>);📝 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.
| if (execution.status === "error") { | |
| return Result.error(execution.error); | |
| } | |
| if (!execution.data.result) { | |
| const noResultError = new StackAssertionError("No result from Freestyle", { | |
| execution, | |
| requests, | |
| }); | |
| captureError("freestyle-no-result", noResultError); | |
| throw noResultError; | |
| } | |
| return Result.ok(execution.data.result as Array<{ html: string, text: string, subject?: string, notificationCategory?: string }>); | |
| return Result.ok(execution.data as Array<{ html: string, text: string, subject?: string, notificationCategory?: string }>); | |
| if (execution.status === "error") { | |
| return Result.error(execution.error); | |
| } | |
| const data = execution.data as any; | |
| if (!Array.isArray(data)) { | |
| throw new StackAssertionError("Freestyle returned non-array result for tenancy batched rendering", { data }); | |
| } | |
| return Result.ok(data as Array<{ html: string, text: string, subject?: string, notificationCategory?: string }>); |
🤖 Prompt for AI Agents
In @apps/backend/src/lib/email-rendering.tsx around lines 367 - 371, The return
is casting execution.data to an array without validation; add defensive checks
similar to the earlier validation at line 251: verify execution.data is an Array
and every item is an object with required string properties (html and text,
optional subject and notificationCategory) before returning Result.ok; if the
check fails return Result.error with a clear message (e.g., "Invalid email
rendering data") so callers don’t receive an unsafe cast.
Preview ScreenshotsOpen Workspace (1 hr expiry) · Open Dev Browser (1 hr expiry) · Open Diff Heatmap Screenshot capture was skipped.
Generated by cmux preview system |
2 participants
In this PR, we bump freestyle to the latest version. Then, we refactor our freestyle wrapper code to use the
serverless.runs.createapi since theexecuteScriptwas deprecated. This PR does not change the functionality of our freestyle wrapper.We also remove the retry mechanism since the retries further up in the stack handle the need for one. We simplify the error handling since the latest version of freestyle provides us the message directly.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.