fix(settings): suppress permission-denied banner on Purchasing policies for non-admins - #981
Conversation
…es for non-admins (closes #979) When GET /api/config returns HTTP 403 with a "permission denied" error, loadGlobalSettings now returns early instead of rendering the red error banner. The section stays hidden and the page degrades gracefully, matching the existing Exchange Automation hide-for-non-admins pattern. Non-permission 403s, 5xx, and network failures still surface the banner. Four new tests in settings-permissions.test.ts cover: the 403+permission path (no banner), unrelated 403s (banner shown), 500s (banner shown), and network errors (banner shown).
|
Need an answer fast? Review this PR in Change Stack to ask focused questions about the PR or a changed range. Warning Review limit reached
More reviews will be available in 4 minutes and 7 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. 📝 WalkthroughWalkthroughDetects backend 403 "permission denied" responses during global settings load and hides the loading UI without showing the generic error banner; other failures (other 403s, 500s, network errors) continue to show the error banner. ChangesGraceful permission-denied error handling
Sequence Diagram(s)sequenceDiagram
participant loadGlobalSettings
participant BackendAPI
participant DOM
loadGlobalSettings->>BackendAPI: GET /api/config
BackendAPI-->>loadGlobalSettings: 403 "permission denied" or other error
alt permission denied (403)
loadGlobalSettings->>DOM: hide `#settings-loading`
loadGlobalSettings-->>DOM: do not show `#settings-error`
else other error (500, network, 403 other)
loadGlobalSettings->>DOM: hide `#settings-loading`
loadGlobalSettings->>DOM: show `#settings-error` with message
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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)
Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
frontend/src/settings.ts (1)
2984-2986: 💤 Low valueConsider extracting the permission-denied check to a helper function.
The inline boolean expression is correct, but extracting it would improve readability and make the intent clearer. For example:
function isPermissionDeniedError(error: unknown): boolean { return (error as { status?: number }).status === 403 && error instanceof Error && error.message.startsWith('permission denied'); }Then the catch block would read:
if (isPermissionDeniedError(error)) { if (loadingEl) loadingEl.classList.add('hidden'); return; }🤖 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 `@frontend/src/settings.ts` around lines 2984 - 2986, Extract the inline permission-check into a small helper named isPermissionDeniedError(error: unknown): boolean that implements the current logic ((error as { status?: number }).status === 403 && error instanceof Error && error.message.startsWith('permission denied')), then replace the inline boolean in the catch block with a call to isPermissionDeniedError(error) so the catch reads: if (isPermissionDeniedError(error)) { if (loadingEl) loadingEl.classList.add('hidden'); return; } to improve readability and intent.
🤖 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 `@frontend/src/settings.ts`:
- Around line 2984-2986: Extract the inline permission-check into a small helper
named isPermissionDeniedError(error: unknown): boolean that implements the
current logic ((error as { status?: number }).status === 403 && error instanceof
Error && error.message.startsWith('permission denied')), then replace the inline
boolean in the catch block with a call to isPermissionDeniedError(error) so the
catch reads: if (isPermissionDeniedError(error)) { if (loadingEl)
loadingEl.classList.add('hidden'); return; } to improve readability and intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 325a5689-83b6-4c69-88c1-aaa24772a880
📒 Files selected for processing (2)
frontend/src/__tests__/settings-permissions.test.tsfrontend/src/settings.ts
Pull the inline 403 + "permission denied" check out of the catch block into a small helper so the catch reads as a single intent-revealing line. Behavior unchanged; existing settings-permissions tests cover the contract.
|
Addressed CR pass-1 finding:
@coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Read the current state of settings.ts to verify the isPermissionDeniedError helper
cat frontend/src/settings.tsLength of output: 145107
function isPermissionDeniedError(error: unknown): boolean {
return error instanceof Error
&& (error as { status?: number }).status === 403
&& error.message.startsWith('permission denied');
}That said, this is purely cosmetic — happy to trigger the review as-is. 🧠 Learnings used✅ Action performedReview finished.
|
…nDeniedError (#981 CR) Reorder the guard so the `instanceof Error` narrowing precedes the `.status` type assertion, matching the conventional TypeScript idiom (narrow first, then access properties). Behavior is unchanged thanks to short-circuit evaluation; this addresses the CodeRabbit style nitpick.
|
Addressed the remaining CR style nitpick: reordered the guard in @coderabbitai review |
|
✅ Action performedReview finished.
|
Summary
Failed to load settings: permission denied: requires view on configwheneverGET /api/configreturned 403.loadGlobalSettingsdetect 403 +"permission denied"errors and return early, leaving the section hidden. Unrelated 403s, 5xx, and network errors still surface the banner.Closes #979
Changes
frontend/src/settings.ts: in thecatchblock ofloadGlobalSettings, detect permission-denied errors (HTTP 403 AND message starts with"permission denied") and return early without showing#settings-error.frontend/src/__tests__/settings-permissions.test.ts: 4 new tests covering the 403+permission path (no banner), unrelated 403s (banner shown), 500s (banner shown), and network errors (banner shown).Test plan
npx jest --testPathPattern="settings-permissions"- 12 tests pass (4 new)npx tsc --noEmit- no type errorsSKIP=terraform_validate pre-commit run --all-files- all hooks passSummary by CodeRabbit
Bug Fixes
Tests