ENG-1090 Refactor error handling: add username, fix styling, handle local cors, etc #576
Conversation
…error context, add username to error email payload, and enhance EmailTemplate to display username. Update ErrorEmailProps type to require username. (#575)
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThe PR refactors the error reporting system by centralizing user identification in the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Pre-merge checks✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/roam/src/components/settings/AdminPanel.tsx (1)
31-31: Send Error Email test button looks good; consider tightening error handling and Checkbox stateThe admin-only test button wiring is straightforward and correctly calls
sendErrorEmailwith a synthetic error. Two small cleanups you might consider:
- For the Checkbox, you're tracking
useReifiedRelationsin state but also usingdefaultChecked. Either drop the state (uncontrolled, usedefaultChecked={getSetting(...)}directly) or make it controlled withchecked={useReifiedRelations}to avoid the hybrid pattern.- For the test button,
.catch(() => {})silently swallows failures. Even a simpleconsole.error(or reusing whatever logging you prefer) would help debugging if the endpoint or CORS misbehaves, while keeping UX lightweight for this admin/testing control.Also applies to: 330-363
packages/types/index.ts (1)
7-9: ErrorEmailProps stricter contract – ensure all callers now populate the new required fieldsMaking
version,buildDate, andusernamerequired is consistent with the new email template and Roam sender logic, but the API handler currently does a blind cast fromrequest.json()toErrorEmailProps. Older clients (or other apps like Obsidian) that haven’t been updated could still POST payloads missing these fields, leading toundefinedat runtime despite the static type.I’d suggest either:
- Verifying all existing producers of
ErrorEmailPropsnow always includeversion,buildDate, andusername, or- Adding lightweight runtime validation/defaulting in the API route before calling
EmailTemplateso the handler is robust against partial payloads.apps/website/app/api/errors/route.tsx (1)
10-25: CORS helper + handlers are solid; consider explicit return types and revisiting the eslint-disableThe refactor to
createCorsResponseand consistent CORS handling forPOST/OPTIONSlooks good and should make local Roam dev friendlier. A few focused improvements:
Add explicit return types to match the TS guidelines, e.g.:
export const POST = async (request: Request): Promise<Response> => { … }; export const OPTIONS = (request: Request): Response => { … };
OPTIONScurrently re-implements the same CORS header logic ascreateCorsResponse. You could optionally delegate tocreateCorsResponse(null, 204, origin)to keep behavior centralized (and reduce chances of future drift).The
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignmentonusernamesuggests the linter considers the request body unsafe. A more robust pattern is to treatawait request.json()asunknownand validate/narrow toErrorEmailProps(e.g. via a small type guard or schema) before destructuring and passing intoEmailTemplate, which should remove the need for the suppression and improve safety for untrusted input.Also applies to: 27-82, 86-102
apps/roam/src/utils/sendErrorEmail.ts (1)
4-4: sendErrorEmail payload updates look good; add explicit return type and minor typing polishThe added
usernameand theversion/buildDatefallbacks align well with the updatedErrorEmailProps, and the improved catch block makes failures much easier to debug.Two small TS/guideline tweaks you might consider:
Give
sendErrorEmailan explicit return type to match the repo rules:const sendErrorEmail = async (…): Promise<void> => { … };Since
ErrorEmailPropsis only used for typing, you can import it as a type to avoid any chance of runtime import overhead:import type { ErrorEmailProps } from "@repo/types";Also applies to: 6-52
apps/website/app/api/errors/EmailTemplate.tsx (1)
4-9: EmailTemplate refactor improves clarity; consider more flexible props and explicit return typesThe
ErrorFieldextraction and the newUsernamefield make the template much cleaner and align with the updatedErrorEmailProps.Two minor refinements:
If you ever want these fields to render richer content (e.g. fallbacks, code formatting), you could widen
ErrorFieldto acceptReact.ReactNodeinstead of juststring:const ErrorField = ({ label, value }: { label: string; value: React.ReactNode }) => (…);To match the TS guidelines, you may want to add explicit return types to
ErrorFieldandEmailTemplate, e.g.(): JSX.Element.Using inline styles here is reasonable given this is email HTML and not styled by Tailwind at runtime.
Also applies to: 19-30
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/roam/src/components/canvas/Tldraw.tsx(0 hunks)apps/roam/src/components/canvas/useRoamStore.ts(0 hunks)apps/roam/src/components/settings/AdminPanel.tsx(2 hunks)apps/roam/src/utils/sendErrorEmail.ts(3 hunks)apps/website/app/api/errors/EmailTemplate.tsx(2 hunks)apps/website/app/api/errors/route.tsx(3 hunks)packages/types/index.ts(1 hunks)
💤 Files with no reviewable changes (2)
- apps/roam/src/components/canvas/useRoamStore.ts
- apps/roam/src/components/canvas/Tldraw.tsx
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/main.mdc)
**/*.{ts,tsx}: Use Tailwind CSS for styling where possible
When refactoring inline styles, use tailwind classes
Prefertypeoverinterfacein TypeScript
Use explicit return types for functions
Avoidanytypes when possible
Prefer arrow functions over regular function declarations
Use named parameters (object destructuring) when a function has more than 2 parameters
Use PascalCase for components and types
Use camelCase for variables and functions
Use UPPERCASE for constants
Function names should describe their purpose clearly
Prefer early returns over nested conditionals for better readability
Files:
packages/types/index.tsapps/roam/src/components/settings/AdminPanel.tsxapps/website/app/api/errors/route.tsxapps/roam/src/utils/sendErrorEmail.tsapps/website/app/api/errors/EmailTemplate.tsx
apps/roam/**/*.{js,ts,tsx,jsx,json}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Prefer existing dependencies from package.json when working on the Roam Research extension
Files:
apps/roam/src/components/settings/AdminPanel.tsxapps/roam/src/utils/sendErrorEmail.ts
apps/roam/**/*.{ts,tsx,jsx,js,css,scss}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Use BlueprintJS 3 components and Tailwind CSS for platform-native UI in the Roam Research extension
Files:
apps/roam/src/components/settings/AdminPanel.tsxapps/roam/src/utils/sendErrorEmail.ts
apps/roam/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
apps/roam/**/*.{ts,tsx,js,jsx}: Use the roamAlphaApi docs from https://roamresearch.com/#/app/developer-documentation/page/tIaOPdXCj when implementing Roam functionality
Use Roam Depot/Extension API docs from https://roamresearch.com/#/app/developer-documentation/page/y31lhjIqU when implementing extension functionality
Files:
apps/roam/src/components/settings/AdminPanel.tsxapps/roam/src/utils/sendErrorEmail.ts
apps/roam/**
📄 CodeRabbit inference engine (.cursor/rules/roam.mdc)
Implement the Discourse Graph protocol in the Roam Research extension
Files:
apps/roam/src/components/settings/AdminPanel.tsxapps/roam/src/utils/sendErrorEmail.ts
apps/website/**
📄 CodeRabbit inference engine (.cursor/rules/website.mdc)
Prefer existing dependencies from apps/website/package.json when adding dependencies to the website application
Files:
apps/website/app/api/errors/route.tsxapps/website/app/api/errors/EmailTemplate.tsx
🧠 Learnings (2)
📚 Learning: 2025-06-19T19:43:43.380Z
Learnt from: sid597
Repo: DiscourseGraphs/discourse-graph PR: 226
File: apps/roam/src/components/settings/HomePersonalSettings.tsx:123-149
Timestamp: 2025-06-19T19:43:43.380Z
Learning: The "Fetch Embeddings for nodes" button in HomePersonalSettings.tsx is for testing purposes only, so it doesn't require production-level error handling or user feedback improvements.
Applied to files:
apps/roam/src/components/settings/AdminPanel.tsx
📚 Learning: 2025-11-25T00:52:27.779Z
Learnt from: CR
Repo: DiscourseGraphs/discourse-graph PR: 0
File: .cursor/rules/main.mdc:0-0
Timestamp: 2025-11-25T00:52:27.779Z
Learning: Applies to **/*.{ts,tsx} : Prefer arrow functions over regular function declarations
Applied to files:
apps/website/app/api/errors/route.tsx
🧬 Code graph analysis (4)
apps/roam/src/components/settings/AdminPanel.tsx (1)
apps/roam/src/utils/extensionSettings.ts (1)
setSetting(13-16)
apps/website/app/api/errors/route.tsx (1)
packages/types/index.ts (1)
ErrorEmailProps(1-11)
apps/roam/src/utils/sendErrorEmail.ts (1)
packages/types/index.ts (1)
ErrorEmailProps(1-11)
apps/website/app/api/errors/EmailTemplate.tsx (1)
packages/types/index.ts (1)
ErrorEmailProps(1-11)
#575
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.