Fix for v4 run failed alerts missing error #2031
Conversation
|
WalkthroughReferences to Changes
Suggested reviewers
Poem
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
apps/webapp/app/v3/services/alerts/deliverAlert.server.ts (1)
978-999: 🛠️ Refactor suggestionFallback
TaskRunErroris missingmessage/stackTrace; may surface asundefineddownstream
#getRunErrornow serialises the unknown data intoraw, but leaves out
message/stackTrace. Later, Slack & e-mail builders call
createJsonErrorObject(taskRunError)and then access
error.stackTrace ?? error.message.
With the current object those properties will beundefined, producing empty
alerts.- return { - type: "CUSTOM_ERROR", - raw: JSON.stringify(alert.taskRun.error ?? "Unknown error"), - }; + const raw = alert.taskRun.error ?? "Unknown error"; + return { + type: "CUSTOM_ERROR", + message: typeof raw === "string" ? raw : "Unknown error", + stackTrace: undefined, + raw: typeof raw === "string" ? raw : JSON.stringify(raw), + };This ensures every consumer has a sensible
messagefield to display.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webapp/app/v3/services/alerts/deliverAlert.server.ts(3 hunks)apps/webapp/app/v3/services/alerts/performTaskRunAlerts.server.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / 🧪 Unit Tests
| import { type prisma } from "~/db.server"; | ||
| import { commonWorker } from "~/v3/commonWorker.server"; |
There was a problem hiding this comment.
import { type prisma } … breaks typeof prisma – import the value, not just the type
FoundRun relies on typeof prisma.taskRun, which is a value query.
When prisma is imported with the type modifier, the symbol only exists in the
type-space; the value symbol is erased, so typeof prisma triggers TS 2749
(“only refers to a type, but is being used as a value here”).
-import { type prisma } from "~/db.server";
+import { prisma } from "~/db.server";If you still want to avoid bringing the runtime value into the bundle, replace
typeof prisma.taskRun with the model–name type from @prisma/client instead
(e.g. Prisma.TaskRunDelegate).
Failing to adjust this will cause the file not to compile.
📝 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.
| import { type prisma } from "~/db.server"; | |
| import { commonWorker } from "~/v3/commonWorker.server"; | |
| import { prisma } from "~/db.server"; | |
| import { commonWorker } from "~/v3/commonWorker.server"; |
About 6 months ago we added the
errorcolumn toTaskRun. Previously they were only on theTaskRunAttempt.So for the first version of run alerts we used the attempt error.
We can just use the
TaskRunnow and attempt database records don't exist in v4… so it meant all v4 run alerts were missing the error on them.This is faster and more compatible.
Summary by CodeRabbit