Harden Tabby transaction persistence failures#314
Conversation
📝 WalkthroughWalkthroughThe Tabby model replaces debug-output-based error handling with exception throwing in ChangesTransaction Error Handling Refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![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 (1)
common/models/Tabby.php (1)
90-93: ⚡ Quick winConsider using a more specific exception type.
While
\yii\base\Exceptionworks, Yii2 provides more specific exception types for database operations. Consider usingyii\db\Exceptionor creating a custom exception class for Tabby-specific failures. This improves error handling granularity and makes it easier for callers to distinguish between different failure types.♻️ Suggested improvement
if (!$tt->save()) { TabbyTransaction::ddlog('error', 'addTransaction', null, $tt->errors); - throw new \yii\base\Exception('Unable to persist Tabby transaction.'); + throw new \yii\db\Exception('Unable to persist Tabby transaction.'); }🤖 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 `@common/models/Tabby.php` around lines 90 - 93, The current catch when $tt->save() fails throws a generic \yii\base\Exception; change this to throw a more specific exception (either yii\db\Exception or a custom Tabby-specific exception class like TabbyException) so callers can distinguish DB failures; update the throw in the failure block where TabbyTransaction::ddlog('error', 'addTransaction', null, $tt->errors) is called to instantiate and throw the chosen specific exception and, if creating a custom class, add it to the common\models namespace and use it here.
🤖 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 `@common/models/Tabby.php`:
- Around line 131-134: The log context is incorrect: inside updateTransaction()
the TabbyTransaction::ddlog call uses the string 'addTransaction' which is a
copy-paste error; update the ddlog invocation in updateTransaction() to use the
correct context string (e.g., 'updateTransaction') so logs reflect the right
method, and while here consider throwing a more specific exception class instead
of \yii\base\Exception (for example \RuntimeException or your domain-specific
exception) to keep error handling consistent.
---
Nitpick comments:
In `@common/models/Tabby.php`:
- Around line 90-93: The current catch when $tt->save() fails throws a generic
\yii\base\Exception; change this to throw a more specific exception (either
yii\db\Exception or a custom Tabby-specific exception class like TabbyException)
so callers can distinguish DB failures; update the throw in the failure block
where TabbyTransaction::ddlog('error', 'addTransaction', null, $tt->errors) is
called to instantiate and throw the chosen specific exception and, if creating a
custom class, add it to the common\models namespace and use it here.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| if (!$tt->save()) { | ||
| TabbyTransaction::ddlog('error', 'addTransaction', null, $tt->errors); | ||
| echo "<pre />"; | ||
| print_r($tt->errors); | ||
| die(); | ||
| throw new \yii\base\Exception('Unable to update Tabby transaction.'); | ||
| } |
There was a problem hiding this comment.
Fix copy-paste error in log context.
The ddlog call on line 132 uses 'addTransaction' as the context, but this code is inside the updateTransaction() method. This inconsistency will make log analysis and debugging more difficult.
🐛 Proposed fix
if (!$tt->save()) {
- TabbyTransaction::ddlog('error', 'addTransaction', null, $tt->errors);
+ TabbyTransaction::ddlog('error', 'updateTransaction', null, $tt->errors);
throw new \yii\base\Exception('Unable to update Tabby transaction.');
}Additionally, consider using a more specific exception type for consistency:
if (!$tt->save()) {
TabbyTransaction::ddlog('error', 'updateTransaction', null, $tt->errors);
- throw new \yii\base\Exception('Unable to update Tabby transaction.');
+ throw new \yii\db\Exception('Unable to update Tabby transaction.');
}📝 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.
| if (!$tt->save()) { | |
| TabbyTransaction::ddlog('error', 'addTransaction', null, $tt->errors); | |
| echo "<pre />"; | |
| print_r($tt->errors); | |
| die(); | |
| throw new \yii\base\Exception('Unable to update Tabby transaction.'); | |
| } | |
| if (!$tt->save()) { | |
| TabbyTransaction::ddlog('error', 'updateTransaction', null, $tt->errors); | |
| throw new \yii\base\Exception('Unable to update Tabby transaction.'); | |
| } |
🤖 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 `@common/models/Tabby.php` around lines 131 - 134, The log context is
incorrect: inside updateTransaction() the TabbyTransaction::ddlog call uses the
string 'addTransaction' which is a copy-paste error; update the ddlog invocation
in updateTransaction() to use the correct context string (e.g.,
'updateTransaction') so logs reflect the right method, and while here consider
throwing a more specific exception class instead of \yii\base\Exception (for
example \RuntimeException or your domain-specific exception) to keep error
handling consistent.
2 participants
Summary\n- remove raw print_r/die handling when Tabby transactions fail to save\n- keep structured Tabby error logging and raise Yii exceptions instead\n- avoid leaking model validation errors to payment callback callers\n\n/claim #59\n\n## Validation\n- git diff --check\n- php -l common/models/Tabby.php could not run because php is not installed in this environment
Summary by CodeRabbit