fix(restate): use /fetch subpath and fix env handling#1974
Conversation
- Update Restate SDK imports to use @restatedev/restate-sdk-cloudflare-workers/fetch (matching official template) - Remove Node SDK dependency and dev:local script (no longer needed) - Add VITE_RESTATE_INGRESS_URL to client env schema with default localhost:8080 - Update apps/web/src/utils/restate.ts to use validated env instead of process.env - Add build and start scripts to match official template Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
✅ Deploy Preview for hyprnote-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for hyprnote ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughUpdated Restate SDK imports to the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
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: 0
🧹 Nitpick comments (3)
apps/restate/package.json (1)
6-8: Clarifybuildvstypechecktsc flags
buildrunstsc --noEmitOnErrorwhiletypecheckrunstsc --noEmit. IfnoEmitisn’t set intsconfig,buildwill emit JS into the source tree, which may not be what you want for a Workers-only bundle.If the goal is “typecheck-only” for both, consider aligning them; otherwise, documenting that
buildis the emit step would avoid confusion.apps/web/src/utils/restate.ts (1)
1-4: Web Restate ingress now correctly flows through typed envSwitching to
env.VITE_RESTATE_INGRESS_URLis a nice improvement over ad‑hocprocess.envaccess and keeps this aligned withapps/web/src/env.ts.One thing to watch: because
VITE_RESTATE_INGRESS_URLhas a default in the env schema, a missing value in staging/prod will silently fall back to that default. Make sure your deploys always setVITE_RESTATE_INGRESS_URLexplicitly so the web app hits the right Restate ingress.apps/web/src/env.ts (1)
33-33: Consider makingVITE_RESTATE_INGRESS_URLrequired outside developmentHaving
default("http://localhost:8080")is great for local dev, but in staging/prod a missingVITE_RESTATE_INGRESS_URLwill silently point the web app at localhost instead of failing fast.You could mirror the
VITE_POSTHOG_API_KEYpattern so it’s only optional in dev:- VITE_RESTATE_INGRESS_URL: z.string().default("http://localhost:8080"), + VITE_RESTATE_INGRESS_URL: isDev + ? z.string().default("http://localhost:8080") + : z.string().min(1),That keeps DX in development while enforcing an explicit, non-empty value in other environments.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/restate/package.json(1 hunks)apps/restate/src/audioPipeline.ts(1 hunks)apps/restate/src/index.ts(1 hunks)apps/restate/src/userRateLimiter.ts(1 hunks)apps/web/src/env.ts(1 hunks)apps/web/src/utils/restate.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Agent implementations should use TypeScript and follow the established architectural patterns defined in the agent framework
Agent communication should use defined message protocols and interfaces
Files:
apps/restate/src/audioPipeline.tsapps/web/src/env.tsapps/web/src/utils/restate.tsapps/restate/src/userRateLimiter.tsapps/restate/src/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props, just inline them instead.
Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
If there are many classNames with conditional logic, usecn(import from@hypr/utils). It is similar toclsx. Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/restate/src/audioPipeline.tsapps/web/src/env.tsapps/web/src/utils/restate.tsapps/restate/src/userRateLimiter.tsapps/restate/src/index.ts
🧬 Code graph analysis (1)
apps/web/src/utils/restate.ts (1)
apps/web/src/env.ts (1)
env(6-38)
⏰ 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). (4)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: fmt
🔇 Additional comments (3)
apps/restate/src/index.ts (1)
1-1: Switch to/fetchRestate SDK entrypoint looks consistentImporting from
@restatedev/restate-sdk-cloudflare-workers/fetchlines up with the other Restate modules in this PR, and the rest of the handler wiring stays the same.Just make sure the
/fetchsubpath in the version you’re using re-exportscreateEndpointHandlerwith the same types sowrangler dev/deploy builds cleanly.apps/restate/src/userRateLimiter.ts (1)
1-1: Import update to/fetchmatches the rest of the Restate appThe rate limiter still uses the same Restate APIs; only the import source moved to
@restatedev/restate-sdk-cloudflare-workers/fetch, which is consistent withaudioPipelineandindex.ts.As long as this subpath in your SDK version exports
object,Context,TerminalError, etc., this change is good.apps/restate/src/audioPipeline.ts (1)
2-2: Restate/fetchimport aligns with workflow usageThe workflow still calls
restate.workflowandrestate.handlers.workflow.*; only the import source changed to@restatedev/restate-sdk-cloudflare-workers/fetch, which matches the rest of the Restate app in this PR.Confirm that this entrypoint in your SDK version exposes the workflow APIs you’re using.
Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/src/functions/transcription.ts (1)
131-136: TypeScript error:workflowAttachnot defined onAudioPipelinetype.Line 136 calls
workflowAttach()on a client typed asAudioPipeline, but theAudioPipelinetype (lines 34-37) doesn't include this method. This should cause a TypeScript compilation error.This is inconsistent with
startAudioPipeline(lines 62-69), which casts toanybefore callingworkflowSubmit. The inconsistency suggests the type definitions are incomplete rather than a deliberate design choice.This will be resolved by updating the
AudioPipelinetype definition to include all workflow client methods, as suggested in the earlier comment on lines 29-37.
🧹 Nitpick comments (1)
apps/web/src/functions/transcription.ts (1)
23-23: Type safety reduction: Consider whetherz.any()is necessary.Changing
llmResultfromz.unknown().optional()toz.any().optional()removes type safety by allowing theanytype to bypass TypeScript's type checking. Whilez.unknown()requires explicit type narrowing, it prevents accidental misuse of the LLM result.Unless there's a specific reason why type narrowing is problematic in consumer code, consider keeping
z.unknown()for better type safety.Apply this diff to restore type safety:
- llmResult: z.any().optional(), + llmResult: z.unknown().optional(),If
z.any()is required due to downstream code complexity, please document why type narrowing isn't feasible.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/functions/transcription.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Agent implementations should use TypeScript and follow the established architectural patterns defined in the agent framework
Agent communication should use defined message protocols and interfaces
Files:
apps/web/src/functions/transcription.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props, just inline them instead.
Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
If there are many classNames with conditional logic, usecn(import from@hypr/utils). It is similar toclsx. Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/web/src/functions/transcription.ts
🧬 Code graph analysis (1)
apps/web/src/functions/transcription.ts (2)
apps/web/src/utils/restate.ts (1)
StatusState(5-16)apps/restate/src/audioPipeline.ts (2)
StartAudioPipelineInput(13-13)AudioPipeline(260-260)
⏰ 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). (5)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: ci (macos, macos-14)
- GitHub Check: fmt
- Add AudioPipelineDefinition type with ctx parameter matching server-side handler signatures - Add AudioPipelineClient type alias using IngressWorkflowClient<AudioPipelineDefinition> - Remove unsafe 'as any' cast from workflowSubmit call - Update all workflowClient calls to use the new types Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/web/src/functions/transcription.ts (2)
24-24:llmResult: z.any()is pragmatic but loosens validation.Switching from
z.unknown()toz.any()removes both runtime and compile‑time constraints onllmResult. That’s fine as a quick way to unblock typing, but if you know even a partial shape (e.g. top‑level fields), consider tightening this later with a structured schema to catch regressions.
30-47: Audio pipeline definition/client typing is solid and fixes earlier gaps.Defining
StartAudioPipelineInput,AudioPipelineDefinition, andAudioPipelineClient = IngressWorkflowClient<AudioPipelineDefinition>cleanly models the workflow contract and removes the need for unsafe casts called out in earlier reviews. The comment aboutctxbeing stripped on the ingress client is accurate and helpful.If the server‑side workflow definition lives in another package, you might eventually want to centralize these types in a shared module to avoid drift between client and worker, but that’s optional and can be deferred.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/web/src/functions/transcription.ts(5 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.ts: Agent implementations should use TypeScript and follow the established architectural patterns defined in the agent framework
Agent communication should use defined message protocols and interfaces
Files:
apps/web/src/functions/transcription.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Avoid creating a bunch of types/interfaces if they are not shared. Especially for function props, just inline them instead.
Never do manual state management for form/mutation. Use useForm (from tanstack-form) and useQuery/useMutation (from tanstack-query) instead for 99% of cases. Avoid patterns like setError.
If there are many classNames with conditional logic, usecn(import from@hypr/utils). It is similar toclsx. Always pass an array and split by logical grouping.
Usemotion/reactinstead offramer-motion.
Files:
apps/web/src/functions/transcription.ts
⏰ 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). (5)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: fmt
- GitHub Check: ci (macos, macos-14)
🔇 Additional comments (4)
apps/web/src/functions/transcription.ts (4)
2-2: Typed Restate client import looks appropriate.Using a type-only import for
IngressWorkflowClientmatches the newAudioPipelineClientalias and keeps runtime bundle clean. Just ensure your installed@restatedev/restate-sdk-clientsversion exposes this type and thatconnect().workflowClientis generic over the same definition.
71-79: startAudioPipeline: typed workflowSubmit usage looks correct.Constructing a typed
AudioPipelineClientand callingworkflowSubmit({ userId, audioUrl })with the pipelineId as the workflow identity matches the new definition and resolves the previousas anyissue. Returninghandle.invocationIdalongsidepipelineIdgives callers everything they need to attach or inspect status later.
108-113: getAudioPipelineStatus: consistent, strongly typed status fetch.Reusing
AudioPipelineClienthere and callingworkflowClient.getStatus()bound to the providedpipelineIdaligns with theAudioPipelineDefinitionand keeps status shape (StatusStateType) consistent with the rest of the flow.
141-145: getAudioPipelineResult: workflowAttach usage matches the workflow client pattern.Creating an
AudioPipelineClientfor the givenpipelineIdand callingworkflowAttach()is the expected Restate pattern for retrieving the workflow result; wiring is consistent withstartAudioPipeline/getAudioPipelineStatusand should behave well end‑to‑end once the worker side is aligned.
Resolved merge conflict in apps/web/src/functions/transcription.ts: - Kept AudioPipelineDefinition and AudioPipelineClient types with IngressWorkflowClient - Updated StartAudioPipelineInput to use fileId instead of audioUrl (from main) - Updated llmResult to z.string() (from main) - Kept properly typed workflowClient calls without 'as any' cast Co-Authored-By: yujonglee <yujonglee.dev@gmail.com>
|
Devin is archived and cannot be woken up. Please unarchive Devin if you want to continue using it. |
1 participant
fix(restate): use /fetch subpath and fix env handling
Summary
This PR fixes two issues with the Restate Cloudflare Workers integration:
SDK import path: Updated all Restate SDK imports to use
@restatedev/restate-sdk-cloudflare-workers/fetchinstead of the root export, matching the official Restate Cloudflare Workers template. This addresses theset_log_levelerror referenced in sdk-typescript#622.Environment variable handling: The
RESTATE_INGRESS_URLinapps/web/src/utils/restate.tswas reading directly fromprocess.env, which doesn't work reliably in Vite browser builds. AddedVITE_RESTATE_INGRESS_URLto the client env schema with a sensible default (http://localhost:8080) and updated the utils file to use the validated env.Also added
buildandstartscripts toapps/restate/package.jsonto match the official template.Updates since last revision
Properly typed the AudioPipeline workflow client (addressing reviewer feedback):
AudioPipelineDefinitiontype withctx: unknownparameter matching the server-side handler signatures (the context parameter is stripped byIngressWorkflowClient)AudioPipelineClienttype alias usingIngressWorkflowClient<AudioPipelineDefinition>as anycast -workflowSubmit,workflowAttach, andgetStatusare now properly typedz.any()forllmResultto work around TanStack Start's server function type inferenceReview & Testing Checklist for Human
AudioPipelineDefinitiontype pattern (withctx: unknownas first parameter) is based on reading the SDK'sIngressWorkflowClientconditional types - confirmworkflowSubmitandworkflowAttachwork correctly at runtime/fetchimport path works correctly withwrangler dev --port 9080(may require Cloudflare login)VITE_RESTATE_INGRESS_URLis correctly available in the browser buildRecommended test plan:
pnpm -F @hypr/restate devand verify the worker starts without theset_log_levelerrorpnpm -F web devand verify the Restate client can connect using the env variableNotes
The known issue sdk-typescript#622 describes a WASM-related error that may still occur in certain environments. If the
set_log_levelerror persists after these changes, it's likely an upstream SDK bug rather than a configuration issue.Link to Devin run: https://app.devin.ai/sessions/3c61dfe0b167467daf2d2c4c02ec4a2d
Requested by: yujonglee (@yujonglee)