fix: Fetching _User crashes when authData remains after auth provider is removed from config#10535
Conversation
… is removed from config
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. Tip
Note Please respond to review comments from AI agents just like you would to comments from a human reviewer. Let the reviewer resolve their own comments, unless they have reviewed and accepted your commit, or agreed with your explanation for why the feedback was incorrect. Caution Pull requests must be written using an AI agent with human supervision. Pull requests written entirely by a human will likely be rejected, because of lower code quality, higher review effort and the higher risk of introducing bugs. Please note that AI review comments on this pull request alone do not satisfy this requirement. Our CI and AI review are safeguards, not development tools. If many issues are flagged, rethink your development approach. Invest more effort in planning and design rather than using review cycles to fix low-quality code. |
📝 WalkthroughWalkthroughAdds a try/catch around per-provider adapter resolution in ChangesStale authData crash fix
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #10535 +/- ##
==========================================
+ Coverage 92.66% 92.68% +0.01%
==========================================
Files 193 193
Lines 16981 16985 +4
Branches 248 248
==========================================
+ Hits 15736 15742 +6
+ Misses 1224 1222 -2
Partials 21 21 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Adapters/Auth/index.js (1)
4-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSolid, minimal fix consistent with existing pattern.
The try/catch mirrors the existing error-swallowing pattern already used in
getProviders()(Line 279-283) for the sameloadAuthAdapterfailure mode, and correctly prevents a single stale/removed provider from crashing the whole_UserafterFind processing for all providers in thePromise.all.One minor note: this will log a warning on every single fetch of an affected
_Userrow until the staleauthDatais cleaned up or the provider reconfigured, which could be noisy for high-traffic affected accounts. Consider whetherlogger.warn(vs.logger.verbose/debug) is the right level long-term, but this is a reasonable default for surfacing the issue.Also applies to: 232-245
🤖 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 `@src/Adapters/Auth/index.js` at line 4, The new try/catch around loadAuthAdapter in Auth processing is fine, but logger.warn will emit on every _User afterFind fetch for the same stale provider and may be too noisy under load. Update the logging in the affected path to use a lower-noise level such as logger.verbose or logger.debug, or otherwise gate repeated warnings, while keeping the existing error-swallowing behavior consistent with getProviders() and the Promise.all flow.
🤖 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.
Nitpick comments:
In `@src/Adapters/Auth/index.js`:
- Line 4: The new try/catch around loadAuthAdapter in Auth processing is fine,
but logger.warn will emit on every _User afterFind fetch for the same stale
provider and may be too noisy under load. Update the logging in the affected
path to use a lower-noise level such as logger.verbose or logger.debug, or
otherwise gate repeated warnings, while keeping the existing error-swallowing
behavior consistent with getProviders() and the Promise.all flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d58ab1e2-34ca-4160-8d79-73b83e930169
📒 Files selected for processing (2)
spec/AuthenticationAdaptersV2.spec.jssrc/Adapters/Auth/index.js
Closes #10526
Summary by CodeRabbit