fix: honor explicit msg in FgaError constructor#325
Conversation
|
Related Documentation Checked 8 published document(s) in 1 knowledge base(s). No updates required. How did I do? Any feedback? |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe FgaError constructor is fixed to use nullish coalescing (\?\?) instead of logical OR (||), resolving an operator precedence bug that prevented explicit error messages from being used. A test is added to verify the corrected behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #325 +/- ##
=======================================
Coverage 89.67% 89.67%
=======================================
Files 25 25
Lines 1492 1492
Branches 279 279
=======================================
Hits 1338 1338
Misses 94 94
Partials 60 60 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes an operator precedence bug in the FgaError constructor where a provided msg parameter was being ignored due to incorrect operator precedence with the logical OR (||) operator.
Changes:
- Fixed
FgaErrorconstructor to use nullish coalescing (??) with proper grouping instead of logical OR - Added regression test to verify explicit message parameter is honored
- Removed unnecessary type assertion (
as string) that became redundant after the fix
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| errors.ts | Fixed operator precedence bug in FgaError constructor by switching from ` |
| tests/errors.test.ts | Added regression test asserting that explicit msg parameter takes precedence over derived error message |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const err = new FgaError(new Error("wrapped-error"), "explicit-message"); | ||
|
|
||
| expect(err.message).toBe("explicit-message"); | ||
| }); |
There was a problem hiding this comment.
Consider adding additional test cases to ensure comprehensive coverage of the FgaError constructor behavior. Suggested scenarios:
- When msg is undefined/not provided, verify the error message is derived from err parameter
- When err is a string, verify the message uses that string
- When err is an Error object without msg, verify the default "FGA Error: ..." format is used
- Edge case: when msg is an empty string, verify it's used (since ?? only treats null/undefined as nullish)
These additional tests would help prevent future regressions and document the expected behavior more thoroughly.
| }); | |
| }); | |
| test("should derive message from err when msg is not provided and err is a string", () => { | |
| const err = new FgaError("string-error"); | |
| // Expect the message to be derived from the string err parameter | |
| expect(err.message).toBe("string-error"); | |
| }); | |
| test('should use default "FGA Error: ..." format when err is an Error and msg is not provided', () => { | |
| const underlying = new Error("inner-error"); | |
| const err = new FgaError(underlying); | |
| // Expect the default FGA Error formatting to be used | |
| expect(err.message).toBe("FGA Error: inner-error"); | |
| }); | |
| test("should use empty string msg when provided (edge case for nullish coalescing)", () => { | |
| const err = new FgaError("ignored-error", ""); | |
| // Empty string should be respected, not replaced by a derived or default message | |
| expect(err.message).toBe(""); | |
| }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/errors.test.ts (1)
1-11: Good regression test for the reported bug.The test correctly validates the core fix. Consider adding a few more edge cases to lock down the full behavior matrix:
💡 Suggested additional test cases
+ test("should derive message from string err when msg is not provided", () => { + const err = new FgaError("string-error"); + expect(err.message).toBe("string-error"); + }); + + test("should derive message from Error err when msg is not provided", () => { + const err = new FgaError(new Error("inner")); + expect(err.message).toBe("FGA Error: inner"); + }); + + test("should preserve empty string msg", () => { + const err = new FgaError(new Error("inner"), ""); + expect(err.message).toBe(""); + });
|
Addressed the review suggestion about expanding constructor behavior coverage. Implemented in commit
All |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2 participants
Summary
Fixes operator precedence in
FgaErrorconstructor so a providedmsgis actually used.Changes
Potential Breaking Change
new FgaError(err, msg)now usesmsgwhenever it is notnull/undefined(including empty string).Fixes #324