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.
![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 e81ca75:
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.
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