Refactor apps/api service #2145
Conversation
✅ Deploy Preview for hyprnote-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughConsolidates integrations and middleware, splits routes into modular handlers (health, llm, stt, webhook), introduces an OpenRouter-backed LLM integration, adds a Sentry tracing middleware that stores spans in bindings, centralizes Stripe/Supabase clients, and refactors desktop LLM connection logic into resolver-driven provider handling. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant API as API (Hono)
participant Sentry as sentryMiddleware
participant Verify as verifyStripeWebhook
participant Billing as billing.syncBillingForStripeEvent
participant Stripe
participant Supabase as supabaseAdmin
Client->>API: POST /webhook/stripe (with stripe-signature)
API->>Sentry: pass through sentryMiddleware (continue trace / start span)
Sentry->>API: span stored in bindings (sentrySpan)
API->>Verify: verify signature -> produce stripeEvent
Verify-->>API: stripeEvent
API->>Sentry: set span tag stripe.event_type
API->>Billing: syncBillingForStripeEvent(stripeEvent)
Billing->>Stripe: fetch/process event data
Billing->>Supabase: update billing records
Billing-->>API: success / error
API->>Sentry: record outcome and status
API-->>Client: 200 { ok: true } / 500 { error }
sequenceDiagram
autonumber
participant Client
participant API as API (Hono / /chat/completions)
participant Sentry as sentryMiddleware
participant OpenRouter as openrouter.openai
participant Metrics
Client->>API: POST /chat/completions (messages, tools, streaming?)
API->>Sentry: sentryMiddleware attaches span
API->>API: determine needsToolCalling -> models = getModels(...)
alt streaming=true
API->>OpenRouter: stream request (models, messages, tools...)
OpenRouter-->>API: stream chunks
API->>Client: SSE stream events (including [DONE])
API->>Metrics: record upstream latency + streaming metrics
else streaming=false
API->>OpenRouter: create completion (models, messages...)
OpenRouter-->>API: JSON response
API-->>Client: JSON completion
API->>Metrics: record upstream latency + completion metrics
end
API->>Sentry: annotate span with outcome
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Deploy Preview for hyprnote ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
apps/api/src/routes/constants.ts (1)
1-6: CentralizedAPI_TAGSis good; ensure old definitions are removedDefining
API_TAGSin a dedicatedconstants.tswithas constis a solid move for tag consistency and typing. Just make sure any priorAPI_TAGSdeclarations (e.g., inroutes.ts) are removed or re‑wired to this one so there’s a single source of truth.apps/api/src/middleware/index.ts (1)
1-4: Middleware barrel export is helpful; watch exported surfaceConsolidating middleware exports here will simplify consumption (
import { … } from "./middleware"). Just ensure each of the re-exported modules only exposes the intended middleware symbols so you don’t accidentally widen the public surface or create naming collisions down the line.apps/api/src/routes/health.ts (1)
1-33: Health route looks solid; consider tightening the response schemaThe
healthrouter and its OpenAPI metadata are wired correctly, and returning{ status: "ok" }withAPI_TAGS.INTERNALmakes sense.You could optionally make the schema reflect the concrete response by narrowing it to a literal:
-const HealthResponseSchema = z.object({ - status: z.string(), -}); +const HealthResponseSchema = z.object({ + status: z.literal("ok"), +});This keeps clients’ expectations aligned with the actual behavior.
apps/api/src/integration/openrouter.ts (1)
6-10: Consider renamingopenaito clarify it's an OpenRouter client.The variable
openaiis initialized with the OpenRouter base URL, which could cause confusion. A name likeopenrouteroropenrouterClientwould better reflect its purpose.-export const openai = new PostHogOpenAI({ +export const openrouter = new PostHogOpenAI({ baseURL: "https://openrouter.ai/api/v1", apiKey: env.OPENROUTER_API_KEY, posthog, });apps/api/src/routes/stt.ts (1)
62-65: Dynamic import may be unnecessary here.The lazy import of
listenSocketHandleradds runtime overhead on each request. Unless there's a specific code-splitting or circular dependency reason, consider using a static import at the top of the file.+import { listenSocketHandler } from "../listen"; + const WebSocketErrorSchema = z.object({Then simplify the handler:
async (c, next) => { - const { listenSocketHandler } = await import("../listen"); return listenSocketHandler(c, next); },apps/api/src/routes/webhook.ts (1)
55-60: Header validation duplicates middleware check.The
verifyStripeWebhookmiddleware already validates theStripe-Signatureheader and returns 400 if missing. This validator adds OpenAPI documentation value but creates redundant validation.Consider if the header validator is purely for OpenAPI documentation or if it serves another purpose. If only for docs, the schema could be declared without runtime validation since the middleware handles it.
apps/api/src/routes/llm.ts (1)
12-25: Schema may not cover all OpenAI message types.
ChatCompletionMessageSchemaonly supportssystem,user, andassistantroles. OpenAI's API also supportstoolandfunctionroles for tool call responses. If clients send tool responses, validation will fail.const ChatCompletionMessageSchema = z.object({ - role: z.enum(["system", "user", "assistant"]), - content: z.string(), + role: z.enum(["system", "user", "assistant", "tool", "function"]), + content: z.string().nullable(), + tool_call_id: z.string().optional(), + name: z.string().optional(), });Alternatively, if full OpenAI compatibility isn't needed, document the supported subset.
apps/desktop/src/hooks/useLLMConnection.ts (1)
52-55: Consider deeper memoization for the connection object.The
connobject is recreated on every render inresolveLLMConnection, which limits the effectiveness of thisuseMemo. While not incorrect, you could optimize by usinguseMemowith value-based dependencies (providerId, modelId, baseUrl, apiKey) inresolveLLMConnectionto return stable object references when values haven't changed.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
apps/api/src/billing.ts(1 hunks)apps/api/src/hono-bindings.ts(1 hunks)apps/api/src/index.ts(2 hunks)apps/api/src/integration/index.ts(1 hunks)apps/api/src/integration/openrouter.ts(1 hunks)apps/api/src/integration/posthog.ts(1 hunks)apps/api/src/integration/stripe.ts(1 hunks)apps/api/src/integration/supabase.ts(1 hunks)apps/api/src/listen.ts(1 hunks)apps/api/src/metrics.ts(1 hunks)apps/api/src/middleware/index.ts(1 hunks)apps/api/src/middleware/load-test-auth.ts(1 hunks)apps/api/src/middleware/sentry.ts(1 hunks)apps/api/src/middleware/stripe.ts(1 hunks)apps/api/src/middleware/supabase.ts(1 hunks)apps/api/src/routes.ts(3 hunks)apps/api/src/routes/constants.ts(1 hunks)apps/api/src/routes/health.ts(1 hunks)apps/api/src/routes/index.ts(1 hunks)apps/api/src/routes/llm.ts(1 hunks)apps/api/src/routes/stt.ts(1 hunks)apps/api/src/routes/webhook.ts(1 hunks)apps/api/src/sentry/middleware.ts(0 hunks)apps/desktop/src/hooks/useLLMConnection.ts(3 hunks)
💤 Files with no reviewable changes (1)
- apps/api/src/sentry/middleware.ts
🧰 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/api/src/middleware/index.tsapps/api/src/hono-bindings.tsapps/api/src/routes/constants.tsapps/api/src/integration/supabase.tsapps/api/src/routes/index.tsapps/api/src/routes/stt.tsapps/api/src/middleware/sentry.tsapps/api/src/integration/openrouter.tsapps/api/src/integration/stripe.tsapps/api/src/routes/webhook.tsapps/api/src/routes/health.tsapps/api/src/billing.tsapps/api/src/integration/index.tsapps/api/src/routes/llm.tsapps/api/src/index.tsapps/api/src/metrics.tsapps/api/src/middleware/load-test-auth.tsapps/api/src/listen.tsapps/api/src/integration/posthog.tsapps/api/src/middleware/stripe.tsapps/api/src/middleware/supabase.tsapps/api/src/routes.tsapps/desktop/src/hooks/useLLMConnection.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/api/src/middleware/index.tsapps/api/src/hono-bindings.tsapps/api/src/routes/constants.tsapps/api/src/integration/supabase.tsapps/api/src/routes/index.tsapps/api/src/routes/stt.tsapps/api/src/middleware/sentry.tsapps/api/src/integration/openrouter.tsapps/api/src/integration/stripe.tsapps/api/src/routes/webhook.tsapps/api/src/routes/health.tsapps/api/src/billing.tsapps/api/src/integration/index.tsapps/api/src/routes/llm.tsapps/api/src/index.tsapps/api/src/metrics.tsapps/api/src/middleware/load-test-auth.tsapps/api/src/listen.tsapps/api/src/integration/posthog.tsapps/api/src/middleware/stripe.tsapps/api/src/middleware/supabase.tsapps/api/src/routes.tsapps/desktop/src/hooks/useLLMConnection.ts
🧬 Code graph analysis (9)
apps/api/src/routes/constants.ts (2)
apps/api/src/routes.ts (1)
API_TAGS(11-16)apps/api/src/routes/index.ts (1)
API_TAGS(9-9)
apps/api/src/routes/index.ts (6)
apps/api/src/routes.ts (1)
routes(50-50)apps/api/src/hono-bindings.ts (1)
AppBindings(4-9)apps/api/src/routes/health.ts (1)
health(13-13)apps/api/src/routes/llm.ts (1)
llm(27-27)apps/api/src/routes/stt.ts (1)
stt(14-14)apps/api/src/routes/webhook.ts (1)
webhook(19-19)
apps/api/src/middleware/sentry.ts (1)
apps/api/src/hono-bindings.ts (1)
AppBindings(4-9)
apps/api/src/routes/webhook.ts (5)
apps/api/src/hono-bindings.ts (1)
AppBindings(4-9)apps/api/src/routes.ts (1)
API_TAGS(11-16)apps/api/src/routes/constants.ts (1)
API_TAGS(1-6)apps/api/src/billing.ts (1)
syncBillingForStripeEvent(75-87)apps/api/src/metrics.ts (1)
Metrics(33-43)
apps/api/src/routes/llm.ts (5)
apps/api/src/hono-bindings.ts (1)
AppBindings(4-9)apps/api/src/routes.ts (1)
API_TAGS(11-16)apps/api/src/routes/constants.ts (1)
API_TAGS(1-6)apps/api/src/integration/openrouter.ts (2)
openai(6-10)getModels(21-23)apps/api/src/metrics.ts (1)
Metrics(33-43)
apps/api/src/index.ts (4)
apps/api/src/middleware/load-test-auth.ts (1)
loadTestOverride(5-16)apps/api/src/middleware/supabase.ts (1)
supabaseAuthMiddleware(6-27)apps/api/src/middleware/stripe.ts (1)
verifyStripeWebhook(10-38)apps/api/src/env.ts (1)
env(4-25)
apps/api/src/middleware/supabase.ts (1)
apps/web/src/middleware/supabase.ts (1)
supabaseAuthMiddleware(12-41)
apps/api/src/routes.ts (3)
apps/api/src/integration/openrouter.ts (2)
openai(6-10)getModels(21-23)apps/api/src/metrics.ts (1)
Metrics(33-43)apps/api/src/billing.ts (1)
syncBillingForStripeEvent(75-87)
apps/desktop/src/hooks/useLLMConnection.ts (4)
apps/desktop/src/auth.tsx (1)
useAuth(278-286)apps/desktop/src/billing.tsx (1)
useBillingAccess(122-130)apps/desktop/src/components/settings/ai/stt/shared.tsx (2)
ProviderId(24-24)PROVIDERS(189-197)apps/desktop/src/env.ts (1)
env(4-16)
⏰ 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). (10)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: Redirect rules - hyprnote-storybook
- GitHub Check: Header rules - hyprnote-storybook
- GitHub Check: Pages changed - hyprnote-storybook
- GitHub Check: desktop_ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: desktop_ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: desktop_ci (macos, depot-macos-14)
- GitHub Check: fmt
🔇 Additional comments (33)
apps/api/src/hono-bindings.ts (1)
1-8: ConfirmsentrySpanis always initialized before useExtending
AppBindings.VariableswithsentrySpan: Sentry.Spanis reasonable, but this type shape implies the span is always present whenever accessed viac.get("sentrySpan"). Please double‑check that all code paths that readsentrySpanonly do so after it’s set (or consider making it nullable/optional in the bindings if some routes legitimately don’t create a span).apps/api/src/integration/posthog.ts (1)
3-8: Env import path refactor looks consistentSwitching
envto../envkeeps the PostHog wiring intact and aligns with the shared env module used elsewhere. No behavioral changes here as long as../envexposes the same keys.apps/api/src/listen.ts (1)
5-52: Metrics import redirection looks correctPointing
Metricsto"./metrics"matches the metrics consolidation described elsewhere; existing calls towebsocketConnected/websocketDisconnectedremain unchanged. Just confirm that./metricsstill exposes aMetricsobject with these methods to avoid a silent API shape drift.apps/api/src/middleware/load-test-auth.ts (1)
3-16: Env path refactor keeps middleware behavior intactUsing
../envhere is consistent with the new env layout and doesn’t change the override behavior. Middleware still safely falls back tonext()when the header or override token doesn’t match.apps/api/src/billing.ts (1)
4-109: Using shared Stripe/Supabase integrations is a good consolidationImporting
stripeandsupabaseAdminfrom theintegrationmodules keeps client configuration centralized while preserving the existing billing flow (handled events, customer resolution, andbillingsupsert). Please just confirm there’s no circular dependency (e.g.,integration/stripeimportingbilling.ts) and thatintegration/stripeexports a fully configuredStripeclient instance.apps/api/src/integration/index.ts (1)
1-4: Clean barrel export pattern for integration modules.This consolidates all integration adapters under a single entry point, making imports cleaner throughout the codebase.
apps/api/src/integration/supabase.ts (1)
1-8: Standard Supabase admin client initialization.The singleton pattern is appropriate for server-side admin operations. The service role key usage is correct for server-to-server communication.
apps/api/src/integration/openrouter.ts (1)
21-23: Clean implementation of model selection helper.The spread operator ensures callers receive a mutable copy, preventing accidental mutations to the source array. The
as constassertion onMODELSprovides good type narrowing.apps/api/src/routes/index.ts (2)
13-16: Route mounting order is not a concern.The
sttrouter contains only a specific route (GET /listen), not a catch-all or wildcard pattern. Thewebhookrouter similarly contains specific routes (e.g.,POST /stripe). Reordering the routes as suggested is unnecessary; the current mounting order causes no path conflicts in Hono.Likely an incorrect or invalid review comment.
9-11: Remove the oldapps/api/src/routes.tsfile to eliminate confusion.The codebase now uses
apps/api/src/routes/index.tsfor route aggregation, but the legacyroutes.tsfile still exists at the same level. Although imports correctly resolve to the new directory-based structure, the duplicate file creates ambiguity and should be deleted.⛔ Skipped due to learnings
Learnt from: CR Repo: fastrepl/hyprnote PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-11-24T16:32:23.055Z Learning: Applies to **/*.{ts,tsx} : Use `motion/react` instead of `framer-motion`.Learnt from: CR Repo: fastrepl/hyprnote PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-11-24T16:32:19.706Z Learning: Applies to **/*.{ts,tsx} : Use `motion/react` instead of `framer-motion`.apps/api/src/integration/stripe.ts (1)
5-7: Consider updating to the latest Stripe API version.The current version
"2025-10-29.clover"is valid and supported. However, a newer Clover release2025-11-17.cloveris available. Review whether upgrading aligns with your release schedule and feature requirements (the November version includes additional refinements over the October release).Likely an incorrect or invalid review comment.
apps/api/src/index.ts (2)
14-19: LGTM! Clean middleware consolidation.The consolidated import from
./middlewarebarrel aligns well with the refactoring goal of centralizing middleware exports.
49-54: LGTM! Middleware ordering is correct.The
loadTestOverridemiddleware correctly runs beforesupabaseAuthMiddleware, allowing load test authentication to bypass Supabase auth whenOVERRIDE_AUTHis set. The conditional/listenauth in non-development environments is preserved.apps/api/src/middleware/sentry.ts (1)
6-29: Well-structured Sentry middleware with proper distributed tracing.The implementation correctly continues traces from upstream services and creates appropriate HTTP server spans. Storing the span in context enables downstream route handlers to add custom attributes.
Consider whether errors thrown by
next()should also be captured on the span. Currently, if an error occurs,span.setAttribute("http.status_code", ...)may not execute or may record an incorrect status. The globalapp.onErrorhandler captures exceptions, but the span won't reflect the error state.apps/api/src/metrics.ts (1)
3-43: LGTM! Clean metric organization by domain.The refactoring improves code organization by grouping metrics logically while preserving the external API through object spreading. The
billingSyncmetric appropriately tracks success/failure with the Stripe event type.apps/api/src/middleware/stripe.ts (1)
5-6: LGTM! Import paths updated for integration consolidation.The Stripe instance is now properly sourced from the centralized
integration/stripemodule, aligning with the PR's goal of consolidating integrations.apps/api/src/routes/stt.ts (2)
9-12: LGTM! Well-defined error schema.The
WebSocketErrorSchemaprovides a clear structure for error responses with requirederrorand optionaldetailfields.
16-61: LGTM! Comprehensive OpenAPI documentation.The route description covers all relevant HTTP status codes for a WebSocket upgrade endpoint, including upstream service errors (502, 504). Security requirement is correctly declared.
apps/api/src/middleware/supabase.ts (1)
6-27: LGTM! Correct Supabase authentication middleware.The implementation properly creates a per-request Supabase client with the user's JWT, validates the token via
getUser(), and stores the user ID in context. Using the anon key with the user's Authorization header enables RLS policies.apps/api/src/routes/webhook.ts (1)
61-80: LGTM! Well-structured webhook handler with proper observability.The handler correctly:
- Retrieves the pre-validated Stripe event from context
- Attaches event type to Sentry span for tracing
- Records billing metrics for success/failure
- Captures exceptions with relevant tags
- Returns appropriate HTTP responses
Same note as stt.ts: consider static import for
../billingunless code splitting is intentional.apps/api/src/routes/llm.ts (2)
92-99: Streaming latency metric measures time-to-first-byte, not total duration.
Metrics.upstreamLatencyis recorded immediately afteropenai.chat.completions.createreturns for streaming responses. This captures only the initial connection time, not the full stream duration. Consider if this is the intended behavior or if you want end-to-end latency.If end-to-end latency is desired for streaming, record the metric inside the
ReadableStream.startcallback after the stream completes:controller.enqueue(encoder.encode("data: [DONE]\n\n")); + Metrics.upstreamLatency("openrouter", performance.now() - startTime); controller.close();And remove the early metric call at line 98.
139-148: LGTM! Error handling extracts status and re-throws.The error handler correctly records metrics, extracts HTTP status from OpenAI SDK errors when available, and re-throws to let the global error handler capture it to Sentry.
apps/api/src/routes.ts (2)
303-306: LGTM!The WebSocket route delegation is clean and the dynamic import pattern is appropriate for lazy loading the socket handler.
97-105: The concern about potential null/undefined access onsentrySpanis incorrect. ThesentryMiddlewareis registered globally withapp.use(sentryMiddleware)at line 24 ofapps/api/src/index.tsand runs before all route handlers. The middleware guarantees thatc.set("sentrySpan", span)is called within theSentry.startSpan()callback with a valid span object beforeawait next()allows route handlers to execute. The span value is guaranteed to exist when routes access it.Likely an incorrect or invalid review comment.
apps/desktop/src/hooks/useLLMConnection.ts (9)
26-50: LGTM! Well-structured type definitions.The discriminated union for
LLMConnectionStatusprovides clear, type-safe status tracking with detailed error reasons. The types are appropriately scoped.
57-87: LGTM! Well-composed hook with proper dependencies.The hook correctly aggregates auth, billing, and store state. The memoization dependencies are complete.
89-92: LGTM! Clean convenience hook.
94-136: LGTM! Proper validation flow with safe type handling.The type assertion at Line 116 is mitigated by the subsequent validation that checks if the provider exists in
PROVIDERS(lines 125-136).
164-194: LGTM! Robust configuration validation.The fallback chain for
baseUrland conditionalapiKeyvalidation handle both configured and default provider settings correctly. Themissingarray provides clear error feedback.
196-204: LGTM! Clean middleware abstraction.Centralizing the reasoning extraction middleware ensures consistent behavior across all providers.
206-221: LGTM! Proper provider setup with tracing.The hyprnote case correctly uses
tracedFetchfor observability and sets appropriate headers. Note: This assumes thebaseUrlfrom line 157 is correct (see previous comment).
223-264: LGTM! Provider-specific implementations are correct.Each provider case properly configures the respective SDK with appropriate headers, fetch implementations, and provider-specific options. The consistent use of
tauriFetchfor desktop environment is appropriate.
266-279: LGTM! Flexible default handling for OpenAI-compatible providers.The conditional
apiKeyconfiguration (lines 272-274) correctly handles providers that don't require authentication, such as local LM Studio or Ollama instances.
- Fix incomplete OpenRouter model identifier (gpt-5.1-chat -> gpt-5.1-chat-latest) - Fix metrics recorded before streaming completes (move success metric after stream completes) - Add null safety for sentrySpan in all routes using optional chaining - Use openrouter/auto instead of empty string for model - Fix baseUrl for hyprnote provider to include path suffix Co-Authored-By: yujonglee <[email protected]>
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/api/src/routes.ts (1)
72-194: Major code duplication: entire chat completions route duplicated fromllm.ts.The
/chat/completionsroute handler (lines 72-194) is nearly identical to the implementation inapps/api/src/routes/llm.ts. Based on the AI summary,routes/index.tswires/chatto thellmrouter, which would make this implementation redundant.Either:
- Remove this route if
llm.tsis the intended implementation- Import and mount the
llmrouter here instead of duplicating+import { llm } from "./routes/llm"; + export const routes = new Hono<AppBindings>(); + +routes.route("/chat", llm); -routes.post( - "/chat/completions", - // ... ~120 lines of duplicated code -);
♻️ Duplicate comments (1)
apps/desktop/src/hooks/useLLMConnection.ts (1)
153-161: Previous baseUrl issue has been addressed.The
baseUrlnow correctly includes${providerDefinition.baseUrl || ""}, which will append the/functions/v1/llmpath from the PROVIDERS definition for hyprnote.
🧹 Nitpick comments (5)
apps/api/src/routes/llm.ts (1)
12-15: Consider adding "tool" and "function" roles for complete tool-calling support.The OpenAI-compatible chat API also supports
"tool"and"function"roles for tool response messages. If tool-calling workflows need to support multi-turn conversations with tool results, these roles should be included.const ChatCompletionMessageSchema = z.object({ - role: z.enum(["system", "user", "assistant"]), + role: z.enum(["system", "user", "assistant", "tool", "function"]), content: z.string(), });apps/api/src/routes.ts (2)
11-16: DuplicateAPI_TAGSdefinition detected.
API_TAGSis also defined inapps/api/src/routes/constants.ts(per relevant snippets). This creates duplicate definitions that could drift apart. Consider importing from the constants file instead.+import { API_TAGS } from "./routes/constants"; + -export const API_TAGS = { - INTERNAL: "internal", - APP: "app", - WEBHOOK: "webhook", - PUBLIC: "public", -} as const;
22-35: Duplicate schema definitions withllm.ts.
ChatCompletionMessageSchemaandChatCompletionRequestSchemaare duplicated in bothroutes.tsandroutes/llm.ts. Extract these to a shared schemas file to maintain a single source of truth.apps/desktop/src/hooks/useLLMConnection.ts (2)
116-136: Consider moving the type cast after provider validation.The cast to
ProviderIdat line 116 occurs before verifying the provider exists inPROVIDERS(line 127). While functionally safe since the error path usesrawProviderId, moving the cast after the validation would better reflect the logical flow.- const providerId = rawProviderId as ProviderId; - if (!modelId) { return { conn: null, - status: { status: "pending", reason: "missing_model", providerId }, + status: { status: "pending", reason: "missing_model", providerId: rawProviderId as ProviderId }, }; } const providerDefinition = PROVIDERS.find((p) => p.id === rawProviderId); if (!providerDefinition) { return { conn: null, status: { status: "error", reason: "provider_not_found", providerId: rawProviderId, }, }; } + + const providerId = rawProviderId as ProviderId;
210-221: Consider removing redundant Authorization header.The
apiKeyoption already handles authentication for OpenAI-compatible providers. The explicitAuthorizationheader is redundant unless the server requires a specific header format not provided by the SDK.const provider = createOpenAICompatible({ fetch: tracedFetch, name: "hyprnote", baseURL: conn.baseUrl, apiKey: conn.apiKey, - headers: { - Authorization: `Bearer ${conn.apiKey}`, - }, });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/api/src/integration/openrouter.ts(1 hunks)apps/api/src/routes.ts(3 hunks)apps/api/src/routes/llm.ts(1 hunks)apps/api/src/routes/webhook.ts(1 hunks)apps/desktop/src/hooks/useLLMConnection.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/api/src/integration/openrouter.ts
- apps/api/src/routes/webhook.ts
🧰 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/api/src/routes/llm.tsapps/api/src/routes.tsapps/desktop/src/hooks/useLLMConnection.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/api/src/routes/llm.tsapps/api/src/routes.tsapps/desktop/src/hooks/useLLMConnection.ts
🧠 Learnings (1)
📚 Learning: 2025-11-24T16:32:13.593Z
Learnt from: CR
Repo: fastrepl/hyprnote PR: 0
File: packages/nango/.cursor/rules/nango.mdc:0-0
Timestamp: 2025-11-24T16:32:13.593Z
Learning: Applies to packages/nango/**/models.ts : Do NOT edit the models.ts file - it is automatically generated at compilation time
Applied to files:
apps/api/src/routes/llm.ts
🧬 Code graph analysis (3)
apps/api/src/routes/llm.ts (5)
apps/api/src/hono-bindings.ts (1)
AppBindings(4-9)apps/api/src/routes/constants.ts (1)
API_TAGS(1-6)apps/api/src/routes/index.ts (1)
API_TAGS(9-9)apps/api/src/integration/openrouter.ts (2)
openai(6-10)getModels(21-23)apps/api/src/metrics.ts (1)
Metrics(33-43)
apps/api/src/routes.ts (3)
apps/api/src/integration/openrouter.ts (2)
openai(6-10)getModels(21-23)apps/api/src/metrics.ts (1)
Metrics(33-43)apps/api/src/billing.ts (1)
syncBillingForStripeEvent(75-87)
apps/desktop/src/hooks/useLLMConnection.ts (2)
packages/store/src/schema-internal.ts (1)
AIProviderStorage(46-46)apps/desktop/src/components/settings/ai/stt/shared.tsx (2)
ProviderId(24-24)PROVIDERS(189-197)
⏰ 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). (7)
- GitHub Check: desktop_ci (linux, depot-ubuntu-24.04-8)
- GitHub Check: desktop_ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: desktop_ci (macos, depot-macos-14)
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: fmt
- GitHub Check: Pages changed - hyprnote
🔇 Additional comments (10)
apps/api/src/routes/llm.ts (3)
1-11: LGTM!Imports are well-organized and correctly reference the consolidated integration modules.
27-76: LGTM!Route setup is well-structured with proper OpenAPI documentation, validation, and safe optional chaining for Sentry span attributes.
77-150: LGTM! Past review concerns addressed.Good improvements:
- Success metric
Metrics.chatCompletion(true, 200)is correctly called aftercontroller.close()(line 110)- Streaming error path now records failure metric
Metrics.chatCompletion(true, 500)(line 112)- Using
"openrouter/auto"as the primary model follows OpenRouter's documented patternapps/api/src/routes.ts (3)
52-70: LGTM!Simple and clean health check endpoint.
236-256: LGTM! Past review concern addressed.The
span?.setAttributeon line 241 properly handles the case wheresentrySpanmight be undefined. Error handling is comprehensive with proper Sentry tagging and metric recording.
258-308: LGTM!WebSocket endpoint is cleanly structured with proper OpenAPI documentation and dynamic import for code splitting.
apps/desktop/src/hooks/useLLMConnection.ts (4)
52-55: LGTM!Clean hook composition with correct memoization dependency.
70-86: LGTM!The resolver-driven memoization approach is clean. Minor note: the dependency on
authobject (rather thanauth?.session) may cause extra recomputation if the auth object reference changes but session remains unchanged, though this is unlikely to be a performance concern in practice.
196-204: LGTM!Clean middleware wrapper supporting both
<think>and<thinking>tag formats for reasoning extraction.
266-277: LGTM!Good fallback implementation for OpenAI-compatible providers with conditional
apiKeyhandling. The use oftauriFetchfor external providers andtracedFetchfor the hosted hyprnote provider is a sensible distinction.
Co-Authored-By: yujonglee <[email protected]>
Co-Authored-By: yujonglee <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/api/src/routes.ts (2)
176-195: Streaming metrics now aligned with stream lifecycleMoving
Metrics.chatCompletion(true, 200)to after thefor awaitloop andcontroller.close()ensures success is only recorded once the stream has fully completed, and addingMetrics.chatCompletion(true, 500)in the catch gives you a clear streaming‑error path. This addresses the earlier concern about premature success metrics for streaming responses.
273-278: Null‑safe Sentry span attribute for Stripe webhookUsing
const span = c.get("sentrySpan");withspan?.setAttribute("stripe.event_type", stripeEvent.type);correctly guards against missing spans and prevents a runtime error in cases where the Sentry middleware hasn’t attached one. AwaitingsyncBillingForStripeEventinside the try also ensuresMetrics.billingSync(true, …)only fires on successful completion.
🧹 Nitpick comments (1)
apps/api/src/routes.ts (1)
130-167: Chat completions refactor looks correct; verifyextraBodyhandling and reduce reliance onascastThe overall flow (deriving
needsToolCalling, usinggetModels(needsToolCalling), defaulting toopenrouter/auto, and centralizing metrics in the shared try/catch) is coherent and matches the OpenRouter auto‑routing intent.Two small follow‑ups to consider:
- The
createParamsobject is forced withas Parameters<typeof openai.chat.completions.create>[0]. To keep types honest as the client evolves, you might instead introduce a local alias and assign without a cast:type ChatCreateParams = Parameters<typeof openai.chat.completions.create>[0]; const createParams: ChatCreateParams = { model: "openrouter/auto", messages, tools, tool_choice, temperature, max_tokens, };
- For the second argument, please double‑check that
PostHogOpenAI’s.chat.completions.createactually consumes{ body: extraBody }the way you expect (i.e., forwardingmodelsandproviderthrough to OpenRouter). Ifbodyis not honored in that options object, the OpenRouter‑specific fields (and any loosenedrestBodyfields) may be silently ignored.Also applies to: 207-225
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/src/index.ts(2 hunks)apps/api/src/routes.ts(3 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/api/src/routes.tsapps/api/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/api/src/routes.tsapps/api/src/index.ts
🧬 Code graph analysis (1)
apps/api/src/routes.ts (3)
apps/api/src/integration/openrouter.ts (2)
openai(6-10)getModels(21-23)apps/api/src/metrics.ts (1)
Metrics(33-43)apps/api/src/billing.ts (1)
syncBillingForStripeEvent(75-87)
⏰ 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). (10)
- GitHub Check: Redirect rules - hyprnote-storybook
- GitHub Check: Header rules - hyprnote-storybook
- GitHub Check: Pages changed - hyprnote-storybook
- GitHub Check: Redirect rules - hyprnote
- GitHub Check: Header rules - hyprnote
- GitHub Check: Pages changed - hyprnote
- GitHub Check: fmt
- GitHub Check: desktop_ci (macos, depot-macos-14)
- GitHub Check: desktop_ci (linux, depot-ubuntu-22.04-8)
- GitHub Check: desktop_ci (linux, depot-ubuntu-24.04-8)
🔇 Additional comments (3)
apps/api/src/index.ts (2)
14-19: Middleware barrel import keeps the entrypoint clean and aligned with the new layoutPulling
loadTestOverride,sentryMiddleware,supabaseAuthMiddleware, andverifyStripeWebhookfrom./middlewarematches the refactor goal of consolidating middleware surfaces and makesindex.tsmuch easier to scan. No issues from my side here.
49-55: Auth + load-test coverage is correctly centralized with clean dev bypassVerification confirms the architecture is sound:
- No legacy
requireSupabaseAuth-style per-route wrappers remain in the codebase—middleware is properly centralized inindex.ts.- All three endpoints (
/chat/completions,/listen,/transcribe) have auth/load-test middleware registered in the correct scope:/chat/completionsalways authenticated,/listenand/transcribeauthenticated only in production.- The dev bypass is intentional and documented ("Requires Supabase authentication in production" in route descriptions).
- No double-auth or inconsistent 401 handling patterns detected.
The implementation matches the PR checklist and is ready to merge.
apps/api/src/routes.ts (1)
8-9: Centralized OpenRouter & metrics imports look goodUsing
./integration/openrouterand./metricshere matches the new integration/metrics surface and keeps this route thin. No issues from a routing or dependency standpoint.
Summary
Refactors the API service architecture by reorganizing code into domain-specific directories:
API Changes:
integration/directorymiddleware/directoryDesktop Changes:
useLLMConnectionhook into modular resolver pattern withresolveLLMConnectionandcreateLanguageModelfunctionsuseLLMConnectionStatushook for status-only accessBug Fixes (from CodeRabbit/Claude review):
controller.close())sentrySpanusing optional chainingopenrouter/autofor proper OpenRouter routingbaseUrlto include path suffix (/functions/v1/llm)Review & Testing Checklist for Human
moonshotai/kimi-k2-0905,openai/gpt-5.1-chat, etc.) are valid and accessible${VITE_API_URL}/functions/v1/llm)Recommended test plan:
/chat/completionsendpoint with both streaming and non-streaming requestsNotes
Link to Devin run: https://app.devin.ai/sessions/417a0eb8e7164599b6bade0455eabfc4
Requested by: yujonglee ([email protected]) / @yujonglee