Fix/quotas 404 bootstrap#99
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (4)
📝 WalkthroughWalkthroughAdds ChangesQuotas 404 Fallback, UI, and Tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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.
Pull request overview
Improves the tenant quotas feature by making quota/usage fetching resilient to missing backend records (404), enhancing the quotas UI feedback (loading skeleton, enforcement-off banner, icons, dirty indicator), and substantially expanding unit test coverage for both hooks and the quotas page.
Changes:
- Add 404 fallbacks in the quotas API layer to return sensible defaults instead of failing.
- Update the quotas page UI with clearer states (loading skeleton, enforcement disabled banner, improved dirty indicator).
- Add/expand Vitest + MSW test suites for quota hooks and the quotas page, covering interactions and error scenarios.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/pages/quotas.tsx | UI polish: loading skeleton, enforcement-off banner, icons, dirty indicator test id, and reorganized config/usage layout. |
| src/pages/tests/quotas.test.tsx | Expanded page-level tests: loading, rendering, interactions (toggle/save/reset), 404 fallbacks, and server-error resilience. |
| src/lib/api/quotas.ts | Add 404-specific handling to return defaultQuota / emptyUsage instead of throwing, enabling UI to render for “no record yet” tenants. |
| src/hooks/tests/use-quotas.test.tsx | New hook test suite covering list/detail/usage queries, 404 fallbacks, and mutation hooks. |
💡 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.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/__tests__/quotas.test.tsx (1)
12-28: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winUse the
renderPage(type)helper instead of a custom render function.The coding guidelines require using a shared
renderPage(type)helper withMemoryRouter,QueryClient, andThemeProviderin unit tests. This ensures consistency across test files and reduces duplication.As per coding guidelines: "
**/*.test.{ts,tsx}: UserenderPage(type)helper withMemoryRouter,QueryClient, andThemeProviderin unit tests"♻️ Refactor to use the shared helper
Replace the
renderQuotas()function with the project'srenderPagehelper. You may need to adjust the helper to accept customQueryClientoptions for test-specific configuration likegcTime: 0.🤖 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/pages/__tests__/quotas.test.tsx` around lines 12 - 28, Replace the custom renderQuotas() function with the shared renderPage(type) helper from the project's test utilities. The renderPage helper should be called with the appropriate page type for the QuotasPage component, which will handle the setup of MemoryRouter, QueryClient, and ThemeProvider automatically. If the helper does not support custom QueryClient options like gcTime: 0, extend it to accept optional configuration parameters that can be merged with the default QueryClient settings.Source: Coding guidelines
🧹 Nitpick comments (1)
src/pages/__tests__/quotas.test.tsx (1)
215-216: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider adding a comment explaining the
"count"guard.The conditional check
if (params.tenantId === "count") return;appears to prevent overriding a different endpoint, but the intent isn't immediately clear.📝 Suggested clarification
http.get("*/administration/quotas/:tenantId/usage", ({ params }) => { + // Skip override for the quotas count endpoint at /administration/quotas/count if (params.tenantId === "count") return; return HttpResponse.json({🤖 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/pages/__tests__/quotas.test.tsx` around lines 215 - 216, The conditional guard `if (params.tenantId === "count") return;` in the http.get handler for the quotas endpoint lacks clarity on its purpose. Add a clarifying comment above this guard condition that explains why the check for "count" is needed and what endpoint it's protecting against. The comment should indicate that this guard prevents the handler from overriding a different endpoint that uses "count" as part of its path.
🤖 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 `@src/hooks/__tests__/use-quotas.test.tsx`:
- Around line 19-31: The test file is using a custom createWrapper function
instead of the repository's shared test harness. Replace the createWrapper
function and its usage with the renderPage(type) helper, which already provides
all required providers including MemoryRouter, QueryClient, and ThemeProvider.
This ensures provider setup consistency across all unit tests and aligns with
the coding guidelines for test files matching the *.test.{ts,tsx} pattern.
In `@src/lib/api/quotas.ts`:
- Around line 44-45: The emptyUsage object is calling new Date().toISOString()
separately for both minuteWindowStart and dayStart fields, which creates two
different timestamps instead of one consistent value. Capture the result of new
Date().toISOString() into a single variable (e.g., currentTimestamp) before
defining the emptyUsage object, then use that variable for both the
minuteWindowStart and dayStart fields to ensure they have the same timestamp
value.
---
Outside diff comments:
In `@src/pages/__tests__/quotas.test.tsx`:
- Around line 12-28: Replace the custom renderQuotas() function with the shared
renderPage(type) helper from the project's test utilities. The renderPage helper
should be called with the appropriate page type for the QuotasPage component,
which will handle the setup of MemoryRouter, QueryClient, and ThemeProvider
automatically. If the helper does not support custom QueryClient options like
gcTime: 0, extend it to accept optional configuration parameters that can be
merged with the default QueryClient settings.
---
Nitpick comments:
In `@src/pages/__tests__/quotas.test.tsx`:
- Around line 215-216: The conditional guard `if (params.tenantId === "count")
return;` in the http.get handler for the quotas endpoint lacks clarity on its
purpose. Add a clarifying comment above this guard condition that explains why
the check for "count" is needed and what endpoint it's protecting against. The
comment should indicate that this guard prevents the handler from overriding a
different endpoint that uses "count" as part of its path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21153646-9bce-4dc0-b810-1c720ba250cf
📒 Files selected for processing (4)
src/hooks/__tests__/use-quotas.test.tsxsrc/lib/api/quotas.tssrc/pages/__tests__/quotas.test.tsxsrc/pages/quotas.tsx
ginccc
force-pushed
the
fix/quotas-404-bootstrap
branch
from
510ff42 to
51dac29
Compare
This pull request significantly improves the test coverage and robustness of the quotas feature, and enhances the user experience for handling tenant quotas. The main changes include comprehensive tests for quotas hooks and page, improved API error handling with sensible fallbacks, and UI improvements for quota configuration and enforcement state.
The most important changes are:
Testing Improvements:
src/hooks/__tests__/use-quotas.test.tsx, covering normal fetches, error handling, and 404 fallback logic.src/pages/__tests__/quotas.test.tsx, including rendering, field values, user interactions (form changes, toggling, saving, reset usage), and error states. [1] [2] [3] [4] [5] [6]API Robustness:
src/lib/api/quotas.tsto return default quota values or zeroed usage when the backend returns 404 (no record), preventing UI crashes and allowing the UI to render with sensible defaults. [1] [2] [3]defaultQuotaandemptyUsagefor these fallback values.UI/UX Improvements:
Minor Enhancements:
These changes make the quotas feature more reliable, user-friendly, and maintainable.
Summary by CodeRabbit