chore: enhance error handling and improve PR template#7237
chore: enhance error handling and improve PR template#7237dipanshurdev wants to merge 2 commits intousebruno:mainfrom
Conversation
…with error message
WalkthroughThe PR updates the GitHub PR template with new guided sections, improves error handling and logging in Redux collection actions to pass explicit error objects, and enhances error messages in schema validation by appending original error details. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js (1)
3064-3077:⚠️ Potential issue | 🟡 MinorTwo issues in
scanForBrunoFiles: debug log artifact + error swallowing incatch.
Line 3070 — debug log:
console.log('scan done', res)is a debug artifact. This PR's only functional need here wasresolve(res)(the previous.then(resolve)already did that). Leaving it in adds noise to production devtools output.Line 3074 —
reject()withouterr: This is the exact same error-swallowing pattern that was fixed inopenMultipleCollections. Callers ofscanForBrunoFileswill receive anundefinedrejection reason on failure, which the PR explicitly aimed to address.🐛 Proposed fix
- .then((res) => { - console.log('scan done', res); - resolve(res); - }) + .then(resolve) .catch((err) => { - reject(); + reject(err); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js` around lines 3064 - 3077, In scanForBrunoFiles, remove the debug console.log('scan done', res) and ensure the catch forwards the real error instead of swallowing it; specifically, in the promise returned by ipcRenderer.invoke('renderer:scan-for-bruno-files', dir) (inside scanForBrunoFiles) stop logging the result and call reject(err) (or let the promise chain propagate the error) so callers receive the actual error object..github/PULL_REQUEST_TEMPLATE.md (1)
26-26:⚠️ Potential issue | 🟡 MinorChecklist item phrasing is inconsistent — reads as an instruction, not a confirmation.
Every other item is phrased in the first person affirming something already done (
"I've used AI…","I have read…"). This item reads as a future action, making it unclear whether contributors should check it before or after they've done it.✏️ Suggested reword
-- [ ] **Create an issue and link to the pull request.** +- [ ] **I have created/linked a related issue in the PR description.**🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/PULL_REQUEST_TEMPLATE.md at line 26, Replace the checklist item text that currently reads "[ ] **Create an issue and link to the pull request.**" with an affirmation phrased like the other items, e.g. "[ ] **I created an issue and linked this pull request.**" to match the first-person confirmation style used elsewhere in the template; update the exact string in the .github/PULL_REQUEST_TEMPLATE.md checklist so the wording is consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/PULL_REQUEST_TEMPLATE.md:
- Line 26: Replace the checklist item text that currently reads "[ ] **Create an
issue and link to the pull request.**" with an affirmation phrased like the
other items, e.g. "[ ] **I created an issue and linked this pull request.**" to
match the first-person confirmation style used elsewhere in the template; update
the exact string in the .github/PULL_REQUEST_TEMPLATE.md checklist so the
wording is consistent.
In `@packages/bruno-app/src/providers/ReduxStore/slices/collections/actions.js`:
- Around line 3064-3077: In scanForBrunoFiles, remove the debug
console.log('scan done', res) and ensure the catch forwards the real error
instead of swallowing it; specifically, in the promise returned by
ipcRenderer.invoke('renderer:scan-for-bruno-files', dir) (inside
scanForBrunoFiles) stop logging the result and call reject(err) (or let the
promise chain propagate the error) so callers receive the actual error object.
|
Hey @dipanshurdev, Another thing, I'd like to understand why the changes were made and what value do you think they add to the app. |
Hi @sid-bruno, sorry for the delay. Thanks for the feedback! I totally understand. I will split these into two separate PRs: one for the template and one for the error handling. Rationale for the changes: PR Template: I noticed some PRs lack context or categorization. Adding "Related Issue" and "Type of Change" prompts contributors to provide this upfront, which help you triage and review faster. I'll go ahead and split these now. |
|
Hi @sid-bruno, thanks for the feedback! I've split these into two separate PRs. Rationale for the changes: PR Template: Prompts contributors for context (issues, type of change) upfront, helping triage. |
Description
This PR addresses swallowed errors and generic error messages in the collection import/open workflows, making debugging much easier for end users.
Specific Improvements:
catch(() => reject())). Now, the error is properly propagated to the caller (reject(err)), allowing the UI to handle it correctly."Field 'name' is required") instead of a generic"The Collection file is corrupted"error. This helps users immediately identify what's wrong with their file.Type of Change
How Has This Been Tested?
Contribution Checklist:
Summary by CodeRabbit
Bug Fixes
Documentation