Add captureWarning for warnings#1506
Conversation
…corder flush Co-authored-by: Konsti <n2d4xc@gmail.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Requesting review from @N2D4 who has experience with the following files modified in this PR:
|
|
|
Greptile SummaryThis PR adds a
Confidence Score: 4/5Safe to merge; the change is additive and the new warning path is functionally correct in all sinks. The core logic in errors.tsx is clean and preserves existing behaviour. The main open question is whether Sentry warnings should be exception-type events or message-type events — the current choice means engineers will see warning entries in the Sentry Issues queue alongside real exceptions, which could create noise. The inline type annotations in the polyfills instead of the exported CaptureLevel are a minor coupling risk. The three Sentry sink files (apps/backend/src/polyfills.tsx, apps/dashboard/src/polyfills.tsx, packages/stack-cli/src/lib/sentry.ts) are worth a second look for the captureException-vs-captureMessage decision. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[captureError / captureWarning] -->|level: error / warning| B[dispatchToSinks]
B --> C[Console Sink\nconsole.error / console.warn\nwith ANSI color]
B --> D[GlobalVar Sink\nstackCapturedErrors.push]
B --> E[Sentry Sink\npolyfills / stack-cli]
E --> F[Sentry.captureException\nwith level field]
G[SessionRecorder.flush] -->|flush error| A
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/backend/src/polyfills.tsx:19
**Sentry exception type used for warnings**
Both the backend and dashboard polyfills (and the CLI sink) call `Sentry.captureException` regardless of `level`. When `level` is `"warning"`, this creates an *Exception* event in Sentry rather than a *Message* event, so warnings land in the Issues section alongside real exceptions. For non-Error values passed to `captureWarning`, Sentry will still try to parse them as exception objects. Using `Sentry.captureMessage` for the `"warning"` branch would keep warnings as message events and errors as exception events, matching Sentry's intended semantic split. The same pattern applies in `apps/dashboard/src/polyfills.tsx` and `packages/stack-cli/src/lib/sentry.ts`.
### Issue 2 of 2
apps/backend/src/polyfills.tsx:13
The `level` parameter is typed inline as `"error" | "warning"` rather than using the exported `CaptureLevel` type from `errors.tsx`. If `CaptureLevel` gains a new variant in the future, this sink silently drops those events without a compile error. Importing and using `CaptureLevel` keeps the sink in sync with the shared contract.
```suggestion
const sentryErrorSink = (location: string, error: unknown, level: import("@stackframe/stack-shared/dist/utils/errors").CaptureLevel) => {
```
Reviews (1): Last reviewed commit: "feat(errors): add captureWarning for war..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return; | ||
| } | ||
| Sentry.captureException(error, { extra: { location } }); | ||
| Sentry.captureException(error, { extra: { location }, level }); |
There was a problem hiding this comment.
Both the backend and dashboard polyfills (and the CLI sink) call Sentry.captureException regardless of level. When level is "warning", this creates an Exception event in Sentry rather than a Message event, so warnings land in the Issues section alongside real exceptions. For non-Error values passed to captureWarning, Sentry will still try to parse them as exception objects. Using Sentry.captureMessage for the "warning" branch would keep warnings as message events and errors as exception events, matching Sentry's intended semantic split. The same pattern applies in apps/dashboard/src/polyfills.tsx and packages/stack-cli/src/lib/sentry.ts.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/backend/src/polyfills.tsx
Line: 19
Comment:
**Sentry exception type used for warnings**
Both the backend and dashboard polyfills (and the CLI sink) call `Sentry.captureException` regardless of `level`. When `level` is `"warning"`, this creates an *Exception* event in Sentry rather than a *Message* event, so warnings land in the Issues section alongside real exceptions. For non-Error values passed to `captureWarning`, Sentry will still try to parse them as exception objects. Using `Sentry.captureMessage` for the `"warning"` branch would keep warnings as message events and errors as exception events, matching Sentry's intended semantic split. The same pattern applies in `apps/dashboard/src/polyfills.tsx` and `packages/stack-cli/src/lib/sentry.ts`.
How can I resolve this? If you propose a fix, please make it concise.| } | ||
|
|
||
| const sentryErrorSink = (location: string, error: unknown) => { | ||
| const sentryErrorSink = (location: string, error: unknown, level: "error" | "warning") => { |
There was a problem hiding this comment.
level parameter is typed inline as "error" | "warning" rather than using the exported CaptureLevel type from errors.tsx. If CaptureLevel gains a new variant in the future, this sink silently drops those events without a compile error. Importing and using CaptureLevel keeps the sink in sync with the shared contract.
| const sentryErrorSink = (location: string, error: unknown, level: "error" | "warning") => { | |
| const sentryErrorSink = (location: string, error: unknown, level: import("@stackframe/stack-shared/dist/utils/errors").CaptureLevel) => { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/backend/src/polyfills.tsx
Line: 13
Comment:
The `level` parameter is typed inline as `"error" | "warning"` rather than using the exported `CaptureLevel` type from `errors.tsx`. If `CaptureLevel` gains a new variant in the future, this sink silently drops those events without a compile error. Importing and using `CaptureLevel` keeps the sink in sync with the shared contract.
```suggestion
const sentryErrorSink = (location: string, error: unknown, level: import("@stackframe/stack-shared/dist/utils/errors").CaptureLevel) => {
```
How can I resolve this? If you propose a fix, please make it concise.
2 participants
Summary
Added captureWarning function similar to captureError but logs warnings with console.warn and sends warnings to Sentry with level 'warning'.
This improves warning handling by integrating with Sentry and consistent logging.
Summary by cubic
Add captureWarning to capture and send warnings to Sentry with the correct level and consistent logging. Error sinks now support levels, and SessionRecorder.flush uses the new warning capture.
captureWarning(location, error)in@stackframe/stack-shared.apps/backend,apps/dashboard, andpackages/stack-cliforwards it toSentry.captureException.console.warninSessionRecorder.flushwithcaptureWarning, wrapping non-OK responses in an Error.Written for commit 181aa7e. Summary will update on new commits. Review in cubic