feat: add structured error codes and context to ActionExecutionResult#459
Open
rjmunro wants to merge 4 commits into
Open
feat: add structured error codes and context to ActionExecutionResult#459rjmunro wants to merge 4 commits into
rjmunro wants to merge 4 commits into
Conversation
WalkthroughThis PR adds structured error reporting to device action execution results. The ChangesStructured error support for device actions
Dependencies
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/timeline-state-resolver/src/integrations/casparCG/index.ts (1)
744-764:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
RESTART_BAD_REPLYreachability for non-2xx responses
Withgotv11 defaultingthrowHttpErrors: true, 3xx/4xx/5xx responses reject before this.then, so theelsebranch only runs for 2xx responses wherestatusCode !== 200(e.g., 201/204). IfRESTART_BAD_REPLYshould cover non-2xx as well, setthrowHttpErrors: false(or mapgot.HTTPErrorin.catch).Proposed fix
return got .post(url, { timeout: { request: 5000, // Arbitary, long enough for realistic scenarios }, + throwHttpErrors: false, })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/timeline-state-resolver/src/integrations/casparCG/index.ts` around lines 744 - 764, The current got.post(...) call relies on a .then(...) to map non-200 replies to CasparCGActionErrorCode.RESTART_BAD_REPLY, but got v11 throws on non-2xx by default so those responses never reach the .then; either disable throwing by adding throwHttpErrors: false to the options object passed to got.post, or keep the default and add a .catch that detects got.HTTPError and maps its response (statusCode and body) to the same error shape (using ActionExecutionResultCode.Error and CasparCGActionErrorCode.RESTART_BAD_REPLY) so that non-2xx replies are handled consistently instead of being swallowed by the rejected promise.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/timeline-state-resolver-types/src/integrations/casparcg.ts`:
- Around line 46-49: The docstring currently refers to the obsolete config key
deviceActionMessages; update the comment to use the correct blueprint config key
deviceActionErrorMessages everywhere in this file (e.g., in the top-level
comment describing "Action error codes for CasparCG device actions") so the
documentation matches the PR and related docs and avoid any mismatches for
CasparCG integration consumers.
In `@packages/timeline-state-resolver/src/integrations/casparCG/index.ts`:
- Around line 645-657: The two early error returns in clearAllChannels
(returning ActionExecutionResultCode.Error with
CasparCGActionErrorCode.CLEAR_INFO_FAILED and
CasparCGActionErrorCode.CLEAR_NO_CHANNELS) omit the legacy fallback "response"
field; update those returned objects to include the same fallback response value
used elsewhere (e.g., the variable response or its fallback message) so callers
relying on the legacy response still receive it—modify the return statements in
clearAllChannels that reference CLEAR_INFO_FAILED and CLEAR_NO_CHANNELS to
include a response property mirroring the successful-path response fallback.
In `@packages/timeline-state-resolver/src/integrations/httpSend/index.ts`:
- Around line 117-123: The validation mistakenly checks cmd.type instead of
cmd.paramsType; update the guard in the HTTP send handler so it verifies that
cmd.paramsType exists and is a valid key in TimelineContentTypeHTTPParamType
(i.e., replace the check "!(cmd.type in TimelineContentTypeHTTPParamType)" with
a check against cmd.paramsType) so the error return (using
ActionExecutionResultCode.Error and HttpSendActionErrorCode.INVALID_PARAMS_TYPE)
only fires for truly invalid paramsType values.
---
Outside diff comments:
In `@packages/timeline-state-resolver/src/integrations/casparCG/index.ts`:
- Around line 744-764: The current got.post(...) call relies on a .then(...) to
map non-200 replies to CasparCGActionErrorCode.RESTART_BAD_REPLY, but got v11
throws on non-2xx by default so those responses never reach the .then; either
disable throwing by adding throwHttpErrors: false to the options object passed
to got.post, or keep the default and add a .catch that detects got.HTTPError and
maps its response (statusCode and body) to the same error shape (using
ActionExecutionResultCode.Error and CasparCGActionErrorCode.RESTART_BAD_REPLY)
so that non-2xx replies are handled consistently instead of being swallowed by
the rejected promise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb315daa-3c7f-4122-91a4-64c08559d0b1
⛔ Files ignored due to path filters (3)
packages/timeline-state-resolver-types/src/__tests__/__snapshots__/index.spec.ts.snapis excluded by!**/*.snappackages/timeline-state-resolver/src/__tests__/__snapshots__/index.spec.ts.snapis excluded by!**/*.snapyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
packages/timeline-state-resolver-types/src/actions.tspackages/timeline-state-resolver-types/src/integrations/casparcg.tspackages/timeline-state-resolver-types/src/integrations/httpSend.tspackages/timeline-state-resolver/package.jsonpackages/timeline-state-resolver/src/integrations/casparCG/index.tspackages/timeline-state-resolver/src/integrations/httpSend/index.ts
Comment on lines
+46
to
+49
| /** | ||
| * Action error codes for CasparCG device actions. | ||
| * These codes can be customized in blueprints via deviceActionMessages. | ||
| * |
There was a problem hiding this comment.
Fix blueprint config key name in the docstring.
The comment references deviceActionMessages, but this PR and related docs use deviceActionErrorMessages. Please align the term to avoid integration confusion.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/timeline-state-resolver-types/src/integrations/casparcg.ts` around
lines 46 - 49, The docstring currently refers to the obsolete config key
deviceActionMessages; update the comment to use the correct blueprint config key
deviceActionErrorMessages everywhere in this file (e.g., in the top-level
comment describing "Action error codes for CasparCG device actions") so the
documentation matches the PR and related docs and avoid any mismatches for
CasparCG integration consumers.
Comment on lines
+645
to
+657
| return { | ||
| result: ActionExecutionResultCode.Error, | ||
| code: CasparCGActionErrorCode.CLEAR_INFO_FAILED, | ||
| context: {}, | ||
| } | ||
| } | ||
| const response = await request | ||
| if (!response?.data[0]) { | ||
| return { result: ActionExecutionResultCode.Error } | ||
| return { | ||
| result: ActionExecutionResultCode.Error, | ||
| code: CasparCGActionErrorCode.CLEAR_NO_CHANNELS, | ||
| context: {}, | ||
| } |
There was a problem hiding this comment.
Return fallback response on all error paths in clearAllChannels.
Two structured error returns omit response, which weakens backward compatibility for consumers still relying on the legacy fallback message.
Proposed fix
if (error) {
return {
result: ActionExecutionResultCode.Error,
code: CasparCGActionErrorCode.CLEAR_INFO_FAILED,
context: {},
+ response: t('Cannot clear CasparCG channels: failed to retrieve channel info'),
}
}
@@
if (!response?.data[0]) {
return {
result: ActionExecutionResultCode.Error,
code: CasparCGActionErrorCode.CLEAR_NO_CHANNELS,
context: {},
+ response: t('Cannot clear CasparCG channels: no channels found'),
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/timeline-state-resolver/src/integrations/casparCG/index.ts` around
lines 645 - 657, The two early error returns in clearAllChannels (returning
ActionExecutionResultCode.Error with CasparCGActionErrorCode.CLEAR_INFO_FAILED
and CasparCGActionErrorCode.CLEAR_NO_CHANNELS) omit the legacy fallback
"response" field; update those returned objects to include the same fallback
response value used elsewhere (e.g., the variable response or its fallback
message) so callers relying on the legacy response still receive it—modify the
return statements in clearAllChannels that reference CLEAR_INFO_FAILED and
CLEAR_NO_CHANNELS to include a response property mirroring the successful-path
response fallback.
Comment on lines
117
to
123
| if (cmd.paramsType && !(cmd.type in TimelineContentTypeHTTPParamType)) { | ||
| return { | ||
| result: ActionExecutionResultCode.Error, | ||
| response: t('Failed to send command: params type is invalid'), | ||
| code: HttpSendActionErrorCode.INVALID_PARAMS_TYPE, | ||
| context: { paramsType: cmd.paramsType }, | ||
| } |
There was a problem hiding this comment.
Fix params-type validation condition (wrong field checked).
The guard checks cmd.type instead of cmd.paramsType, so valid paramsType values can be incorrectly rejected.
Proposed fix
- if (cmd.paramsType && !(cmd.type in TimelineContentTypeHTTPParamType)) {
+ if (
+ cmd.paramsType &&
+ !Object.values<TimelineContentTypeHTTPParamType>(TimelineContentTypeHTTPParamType).includes(cmd.paramsType)
+ ) {
return {
result: ActionExecutionResultCode.Error,
response: t('Failed to send command: params type is invalid'),
code: HttpSendActionErrorCode.INVALID_PARAMS_TYPE,
context: { paramsType: cmd.paramsType },
}
}📝 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.
Suggested change
| if (cmd.paramsType && !(cmd.type in TimelineContentTypeHTTPParamType)) { | |
| return { | |
| result: ActionExecutionResultCode.Error, | |
| response: t('Failed to send command: params type is invalid'), | |
| code: HttpSendActionErrorCode.INVALID_PARAMS_TYPE, | |
| context: { paramsType: cmd.paramsType }, | |
| } | |
| if ( | |
| cmd.paramsType && | |
| !Object.values<TimelineContentTypeHTTPParamType>(TimelineContentTypeHTTPParamType).includes(cmd.paramsType) | |
| ) { | |
| return { | |
| result: ActionExecutionResultCode.Error, | |
| response: t('Failed to send command: params type is invalid'), | |
| code: HttpSendActionErrorCode.INVALID_PARAMS_TYPE, | |
| context: { paramsType: cmd.paramsType }, | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/timeline-state-resolver/src/integrations/httpSend/index.ts` around
lines 117 - 123, The validation mistakenly checks cmd.type instead of
cmd.paramsType; update the guard in the HTTP send handler so it verifies that
cmd.paramsType exists and is a valid key in TimelineContentTypeHTTPParamType
(i.e., replace the check "!(cmd.type in TimelineContentTypeHTTPParamType)" with
a check against cmd.paramsType) so the error return (using
ActionExecutionResultCode.Error and HttpSendActionErrorCode.INVALID_PARAMS_TYPE)
only fires for truly invalid paramsType values.
Saftret
added
the
contribution from BBC
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
About the Contributor
This pull request is posted on behalf of the BBC.
Type of Contribution
This is a: Feature / Code improvement
Current Behavior
When a device action fails (e.g. CasparCG or HTTP Send), the
ActionExecutionResultreturned to the caller contains only a pre-rendered human-readable message. There is no structured way for consumers (e.g. blueprints) to identify the specific error type or to customise the error message for their context.New Behavior
ActionExecutionResultgains two optional fields:code— a structured error code following the patternACTION_{DEVICETYPE}_{REASON}(e.g.ACTION_CASPARCG_LAUNCHER_HOST_NOT_SET)context— aRecord<string, unknown>providing values for custom message interpolation viainterpolateTemplateString()The
responsefield continues to carry a pre-rendered fallback message, so existing consumers are unaffected. Consumers that want to customise messages can match oncodeand interpolate usingcontext.Both CasparCG and HTTP Send device integrations have been updated to return structured error codes and context on their action errors.
Additionally, the
wspackage has been bumped to address a vulnerability report (GHSA).Testing Instructions
ActionExecutionResultincludes acodeandcontextalongside the existingresponsemessage.responsecontinue to work unchanged.yarn unitOther Information
The
code/contextfields are purely additive and optional, making this change fully backwards-compatible. A blueprint can opt in to richer error messages by looking upcodein adeviceActionErrorMessagesmap and interpolating withcontext.Status