Added anomaly type #1012
Added anomaly type #1012
Conversation
Based on the Cognitect Labs anomalies library, this should be a generic, simple set of errors that are both actionable and easy to translate to HTTP errors when needed.
WalkthroughA new TypeScript module introduces a structured error handling system based on an 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (1)
✨ 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
🧹 Nitpick comments (3)
src/core/anomaly.ts (1)
1-140: Consider exporting types for better extensibility.The base
Anomalyclass and type definitions are not exported, which limits extensibility. Users might want to create custom anomaly types or use the option types in their own code.-type AnomalyCategory = +export type AnomalyCategory = | 'unavailable' | 'interrupted' | 'busy' | 'incorrect' | 'forbidden' | 'unsupported' | 'not-found' | 'conflict' | 'fault'; -type AnomalyOptions = { +export type AnomalyOptions = { message?: string; metadata?: Record<string, unknown>; }; -type MaybeRetryableAnomalyOptions = AnomalyOptions & { +export type MaybeRetryableAnomalyOptions = AnomalyOptions & { retryable?: boolean; }; -class Anomaly extends Error { +export class Anomaly extends Error {src/core/anomaly.unit.test.ts (2)
16-91: Inconsistent test assertions across anomaly types.Some tests check different properties. For example, the BusyAnomaly test (lines 36-42) doesn't verify the default message, while others do. Consider standardizing the assertions.
it('should create BusyAnomaly correctly', () => { const anomaly = new BusyAnomaly(); expect(anomaly).toMatchObject({ category: 'busy', + message: 'Service is busy', retryable: true, }); });Apply similar changes to other tests missing message assertions.
1-152: Consider adding tests for additional properties.The tests don't verify that anomalies are instances of Error, have the correct
nameproperty, or test themetadataproperty functionality.+ it('should create anomalies as Error instances with correct name', () => { + const anomaly = new UnavailableAnomaly(); + expect(anomaly).toBeInstanceOf(Error); + expect(anomaly.name).toBe('Anomaly'); + }); + + it('should handle metadata correctly', () => { + const metadata = { userId: 123, action: 'login' }; + const anomaly = new UnavailableAnomaly({ metadata }); + expect(anomaly.metadata).toEqual(metadata); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/anomaly.ts(1 hunks)src/core/anomaly.unit.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: allouis
PR: TryGhost/ActivityPub#1010
File: src/app.ts:744-744
Timestamp: 2025-07-09T06:47:50.040Z
Learning: In the TryGhost/ActivityPub repository, allouis prefers to keep refactoring PRs focused on code restructuring only, without adding new functionality or changing behavior. When moving existing code that has issues, the issues should be preserved in the refactoring and addressed separately.
🧬 Code Graph Analysis (2)
src/core/anomaly.unit.test.ts (1)
src/core/anomaly.ts (9)
UnavailableAnomaly(42-46)InterruptedAnomaly(48-57)BusyAnomaly(59-63)IncorrectAnomaly(65-69)ForbiddenAnomaly(71-75)UnsupportedAnomaly(77-81)NotFoundAnomaly(83-87)ConflictAnomaly(89-93)FaultAnomaly(95-99)
src/core/anomaly.ts (1)
src/core/result.ts (1)
Error(2-2)
⏰ 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). (1)
- GitHub Check: check-yarn-lock
🔇 Additional comments (6)
src/core/anomaly.ts (3)
1-20: Well-structured type definitions with good naming conventions.The union type for anomaly categories is comprehensive and the option types are clearly defined. The distinction between
AnomalyOptionsandMaybeRetryableAnomalyOptionseffectively communicates which anomalies support retryable overrides.
42-99: Consistent implementation across all anomaly subclasses.The specialized anomaly classes follow a consistent pattern and correctly use the appropriate option types. The distinction between classes that accept
AnomalyOptionsand those that acceptMaybeRetryableAnomalyOptionsis well-implemented.
101-140: Helper functions provide clear default behavior.The default messages are descriptive and the retryable categorization follows logical patterns (transient issues like 'unavailable' and 'busy' are retryable, while client errors like 'incorrect' and 'forbidden' are not).
src/core/anomaly.unit.test.ts (3)
1-13: Well-organized imports and test structure.The imports are clean and the test file follows good practices with clear describe blocks and descriptive test names.
93-117: Excellent edge case coverage with efficient loop testing.The test that iterates through all anomaly types with custom messages is a great approach to ensure consistent behavior across all implementations.
118-151: Comprehensive retryable override testing.The tests thoroughly cover all scenarios for the two anomaly types that support retryable overrides, including default behavior and explicit true/false values.
|
Gah I merged a copilot suggested fix and it fails lint 🙈 Will manually fix that, thank you copilot <3 |
Shouldn't be an issue, since it's only possible to set this boolean when the anomaly is a "maybe retryable" one. In those cases, the default is false, so passing false will get the default. But it's still better to actually use the user-input instead of falling back to the `getRetryable` method. Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
This is looking good, though I wonder, what are the benefits of having this extend Error vs just being a map, with an optional error property, e.g.
type Anomoly = {
category: AnomolyType
message?: string
error?: Error
}
Generally prefer simpler, but happy to go either way here - curious on your thoughts.
In terms of the API, I think we're gonna want to have a functional interface for construction so we can just do return busy() or return fault('message here')
But maybe that lives in the result.ts file and under the hoods returns an Err<Anomoly>
Based on the anomalies package by Cognitect Labs, this TypeScript version is designed to be simple to use and provide a great deal of context simply by the error type.
The categories are generic enough to cover all errors, but can also map nicely to HTTP errors.
The semantics of the errors include whether or not they are retryable, based on the category rather than being left up to the callee to decide for each error.