fix(payments): show specific error messages and retry button#200
fix(payments): show specific error messages and retry button#200kumudranjan6127-debug wants to merge 6 commits into
Conversation
Closes hrx01-dev#198 Map Firestore error codes (permission-denied, unauthenticated, unavailable) to user-friendly messages instead of surfacing raw err.message. Add a 'Try again' button to the error state so the client can reload without navigating away.
|
@kumudranjan6127-debug is attempting to deploy a commit to the hrx01-dev's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughFirestore billing errors now map to code-based messages in the client dashboard, the payment error view shows that message with a reload action, the estimate API uses a configured CORS origin, and admin lists gain load-more pagination. ChangesClient payment error handling
Estimate API origin restriction
Admin list pagination
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Export paymentErrorMessage so it is testable, then cover all four branches: permission-denied, unauthenticated, unavailable, and the generic fallback. Also assert that no raw Firebase strings are ever surfaced to the user and that each known code maps to a distinct message.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/dashboard/hooks/useClientPayments.test.ts (1)
15-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the exact user-facing copy here.
These checks are too permissive for a string that
PaymentManagementrenders verbatim. A regression to a different generic message would still pass as long as it contains"permission","sign in", or"unavailable". Pinning the exact strings fromsrc/dashboard/hooks/useClientPayments.ts:7-18will give this PR the contract coverage it claims.Proposed tightening
it("returns a permission hint for permission-denied", () => { - const msg = paymentErrorMessage("permission-denied"); - expect(msg).toContain("permission"); - expect(msg).not.toBe(""); + expect(paymentErrorMessage("permission-denied")).toBe( + "You don't have permission to view payment data. Try signing out and back in.", + ); }); it("returns a sign-in prompt for unauthenticated", () => { - const msg = paymentErrorMessage("unauthenticated"); - expect(msg.toLowerCase()).toContain("sign in"); + expect(paymentErrorMessage("unauthenticated")).toBe( + "Your session has expired. Please sign in again.", + ); }); it("returns a temporary-outage message for unavailable", () => { - const msg = paymentErrorMessage("unavailable"); - expect(msg.toLowerCase()).toContain("unavailable"); + expect(paymentErrorMessage("unavailable")).toBe( + "Payment service is temporarily unavailable. Please try again in a moment.", + ); }); it("returns a generic fallback for any unknown code", () => { - const msg = paymentErrorMessage("internal"); - expect(msg).toBeTruthy(); - expect(msg.toLowerCase()).toContain("couldn't load"); + expect(paymentErrorMessage("internal")).toBe( + "Couldn't load your payments. Please check your connection and try again.", + ); });🤖 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/dashboard/hooks/useClientPayments.test.ts` around lines 15 - 35, The `paymentErrorMessage` tests are too loose for copy that `PaymentManagement` renders verbatim. Update the assertions in `useClientPayments.test.ts` to match the exact user-facing strings returned by `paymentErrorMessage` for each supported code (`permission-denied`, `unauthenticated`, `unavailable`, and the fallback), using the implementation in `useClientPayments.ts` as the source of truth. Keep the checks tied to the specific `paymentErrorMessage` helper so the test fails if the rendered copy changes.
🤖 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/dashboard/hooks/useClientPayments.test.ts`:
- Around line 15-35: The `paymentErrorMessage` tests are too loose for copy that
`PaymentManagement` renders verbatim. Update the assertions in
`useClientPayments.test.ts` to match the exact user-facing strings returned by
`paymentErrorMessage` for each supported code (`permission-denied`,
`unauthenticated`, `unavailable`, and the fallback), using the implementation in
`useClientPayments.ts` as the source of truth. Keep the checks tied to the
specific `paymentErrorMessage` helper so the test fails if the rendered copy
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8efb210b-d78a-4fc8-b19d-98e1433637e7
📒 Files selected for processing (2)
src/dashboard/hooks/useClientPayments.test.tssrc/dashboard/hooks/useClientPayments.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/dashboard/hooks/useClientPayments.ts
Replace the wildcard Access-Control-Allow-Origin header with the Firebase Hosting domain so external sites cannot call the endpoint and drain the Gemini API quota. Falls back to the production URL if ALLOWED_ORIGIN is not set. Document the env var in .env.example.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.env.example:
- Around line 23-26: The example value for ALLOWED_ORIGIN includes a trailing
slash, which will not match the browser Origin header. Update the .env.example
entry for ALLOWED_ORIGIN to use the Firebase Hosting domain without the trailing
slash so the CORS check in the Gemini estimation endpoint matches real requests.
In `@api/estimate.ts`:
- Line 125: The CORS handling in the estimate endpoint currently falls back to a
hardcoded origin when ALLOWED_ORIGIN is missing, which hides misconfiguration;
update the logic around res.setHeader in api/estimate.ts to fail closed by
omitting the Access-Control-Allow-Origin header or rejecting the request when
process.env.ALLOWED_ORIGIN is unset, and only set the header when a configured
origin is actually present.
🪄 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 Plus
Run ID: 3b2f8af0-ba20-47b4-a593-a3e1e4989017
📒 Files selected for processing (2)
.env.exampleapi/estimate.ts
| res.setHeader('Access-Control-Allow-Credentials', 'true'); | ||
| // In production, you can replace '*' with your Firebase hosting URL for better security | ||
| res.setHeader('Access-Control-Allow-Origin', '*'); | ||
| res.setHeader('Access-Control-Allow-Origin', process.env.ALLOWED_ORIGIN || 'https://servio-0.web.app/'); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Fail closed if ALLOWED_ORIGIN is unset.
The hardcoded fallback hides misconfiguration and still advertises a production origin from any deployment that forgot to set this variable. Since this endpoint is supposed to be restricted by configuration, it should reject the request (or omit the CORS header) when ALLOWED_ORIGIN is missing instead of silently allowing a default origin.
Suggested fix
- res.setHeader('Access-Control-Allow-Origin', process.env.ALLOWED_ORIGIN || 'https://servio-0.web.app/');
+ if (!process.env.ALLOWED_ORIGIN) {
+ return res.status(500).json({ error: 'ALLOWED_ORIGIN is not configured on the server.' });
+ }
+ res.setHeader('Access-Control-Allow-Origin', process.env.ALLOWED_ORIGIN);📝 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.
| res.setHeader('Access-Control-Allow-Origin', process.env.ALLOWED_ORIGIN || 'https://servio-0.web.app/'); | |
| if (!process.env.ALLOWED_ORIGIN) { | |
| return res.status(500).json({ error: 'ALLOWED_ORIGIN is not configured on the server.' }); | |
| } | |
| res.setHeader('Access-Control-Allow-Origin', process.env.ALLOWED_ORIGIN); |
🤖 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 `@api/estimate.ts` at line 125, The CORS handling in the estimate endpoint
currently falls back to a hardcoded origin when ALLOWED_ORIGIN is missing, which
hides misconfiguration; update the logic around res.setHeader in api/estimate.ts
to fail closed by omitting the Access-Control-Allow-Origin header or rejecting
the request when process.env.ALLOWED_ORIGIN is unset, and only set the header
when a configured origin is actually present.
…ers — closes hrx01-dev#209 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/admin/hooks/useAdminData.ts (1)
110-124: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoff
loadMorere-reads the entire expanded window on each call.Bumping
pageLimittears down and recreates theonSnapshotsubscription with a largerlimit, so every "Load more" re-reads all previously loaded docs plus the new page (billable reads grow quadratically with pages). For a long-lived admin list this is wasteful. Consider cursor-based pagination (startAfter(lastDoc)) appending to existing data, if read volume becomes a concern.🤖 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/admin/hooks/useAdminData.ts` around lines 110 - 124, The current `useAdminData` pagination strategy recreates the `onSnapshot` query with a larger `firestoreLimit(pageLimit)` on every `loadMore`, causing previously loaded documents to be re-read each time. Update the pagination flow in `useAdminData`/`loadMore` to use cursor-based paging with `startAfter(lastDoc)` (or an equivalent append strategy) so new pages are fetched without re-reading the full expanded window. Keep the existing `onSnapshot`, `setState`, and `parse` flow, but track the last document cursor and append newly fetched items to the existing state instead of replacing the whole list.src/admin/pages/Audit.tsx (1)
78-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: extract the repeated “Load more” block into a shared component.
This exact pattern (
{x.hasMore && <div className="flex justify-center pt-…"><Button variant="outline" size="sm" onClick={x.loadMore}>Load more</Button></div>}) is duplicated across all seven admin pages. A small<LoadMore hasMore onClick />helper would centralize markup/labels and reduce drift.🤖 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/admin/pages/Audit.tsx` around lines 78 - 84, The repeated Load more UI block should be extracted into a shared helper component to avoid duplication across admin pages. Create a reusable component for the hasMore/onClick/button markup and use it in Audit.tsx instead of inlining the div/Button pattern; reference the existing auditLogs.hasMore and auditLogs.loadMore usage so the new shared component preserves the same behavior and label.
🤖 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/admin/hooks/useAdminData.ts`:
- Around line 110-120: The paginated query in useAdminData/useCollectionData is
only applying firestoreLimit(pageLimit) and then re-sorting client-side, which
can drop items across pages for collections ordered by createdAt or clientEmail.
Update useCollectionData to accept and apply a matching server-side orderBy in
the Firestore query for the sorted call sites, using the existing
compare/parsing flow in useAdminData to locate the pagination logic, and keep
the client-side compare sort only when it matches the same field as the server
order.
In `@src/admin/pages/Messages.tsx`:
- Around line 304-322: The Messages view is paging the unfiltered Firestore
results while rendering a client-side filtered `visible` list, so `hasMore` and
`loadMore` can advance through non-matching documents and produce arbitrary
ordering. Update the data flow in `Messages` so the status filter is applied in
the query that feeds the list, and make `hasMore` reflect that same filtered
query rather than the raw `messages` collection.
---
Nitpick comments:
In `@src/admin/hooks/useAdminData.ts`:
- Around line 110-124: The current `useAdminData` pagination strategy recreates
the `onSnapshot` query with a larger `firestoreLimit(pageLimit)` on every
`loadMore`, causing previously loaded documents to be re-read each time. Update
the pagination flow in `useAdminData`/`loadMore` to use cursor-based paging with
`startAfter(lastDoc)` (or an equivalent append strategy) so new pages are
fetched without re-reading the full expanded window. Keep the existing
`onSnapshot`, `setState`, and `parse` flow, but track the last document cursor
and append newly fetched items to the existing state instead of replacing the
whole list.
In `@src/admin/pages/Audit.tsx`:
- Around line 78-84: The repeated Load more UI block should be extracted into a
shared helper component to avoid duplication across admin pages. Create a
reusable component for the hasMore/onClick/button markup and use it in Audit.tsx
instead of inlining the div/Button pattern; reference the existing
auditLogs.hasMore and auditLogs.loadMore usage so the new shared component
preserves the same behavior and label.
🪄 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 Plus
Run ID: 3a9ff8ca-8309-4c55-b59e-8a3dde33f7f2
📒 Files selected for processing (8)
src/admin/hooks/useAdminData.tssrc/admin/pages/Audit.tsxsrc/admin/pages/Clients.tsxsrc/admin/pages/Messages.tsxsrc/admin/pages/ProjectBilling.tsxsrc/admin/pages/ProjectUpdates.tsxsrc/admin/pages/Projects.tsxsrc/admin/pages/Settings.tsx
| const q = query(ref, firestoreLimit(pageLimit)); | ||
| const unsubscribe = onSnapshot( | ||
| ref, | ||
| q, | ||
| (snapshot) => { | ||
| const data: T[] = []; | ||
| snapshot.forEach((docSnap) => { | ||
| const item = parse(docSnap.id, docSnap.data()); | ||
| if (item) data.push(item); | ||
| }); | ||
| if (compare) data.sort(compare); | ||
| setState({ data, loading: false, error: null }); | ||
| setState({ data, loading: false, error: null, hasMore: snapshot.size >= pageLimit }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
Firestore query limit without orderBy default ordering by document name
💡 Result:
In Firestore, if you do not specify an explicit order using the orderBy clause, the query by default retrieves documents in ascending order by their document ID (the document name) [1][2][3]. However, the behavior regarding order stability depends on your Firestore database edition: 1. Standard Edition: Firestore guarantees a stable ordering [4][5]. If you do not provide an explicit order, Firestore automatically appends an ascending order by document name (name) to your query [4][6][5]. This ensures that your query results remain consistent across executions [4][7]. 2. Enterprise Edition: Firestore does not guarantee a stable ordering for queries without a full, unique order [4][5]. Instead, it selects the most efficient ordering based on the indexes available at the time of execution, which may lead to different document orderings for identical queries [4][5]. To ensure a stable and predictable order in Enterprise edition, you should always explicitly include a unique field—such as the document name (name)—in your orderBy clause [4][5]. If you use a limit clause without an explicit orderBy, Firestore will apply the default ascending sort by document ID before limiting the result set [1][2][3]. Because the default sorting uses lexicographical order, document IDs are compared as strings [8]. This means that numeric strings will be sorted lexicographically (e.g., "10" comes before "2") rather than numerically [8]. To achieve numeric sorting, you must store the values in a numeric field type and use orderBy on that field [8].
Citations:
- 1: https://cloud.google.com/firestore/docs/query-data/order-limit-data
- 2: https://firebase.google.com/docs/firestore/query-data/order-limit-data
- 3: https://docs.cloud.google.com/firestore/native/docs/query-data/order-limit-data
- 4: https://cloud.google.com/firestore/docs/reference/rest/v1/StructuredQuery
- 5: https://firebase.google.com/docs/firestore/reference/rest/v1/StructuredQuery
- 6: https://cloud.google.com/python/docs/reference/firestore/latest/google.cloud.firestore_v1.types.StructuredQuery
- 7: https://stackoverflow.com/questions/51570106/firestore-query-with-order-stability
- 8: https://stackoverflow.com/questions/72229057/firestore-query-default-order
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files 'src/admin/hooks/useAdminData.ts' 'src/admin/hooks/*.ts' 'src/admin/**/*.ts' | sed -n '1,200p'
echo '--- useAdminData outline ---'
ast-grep outline src/admin/hooks/useAdminData.ts --view expanded || true
echo '--- useAdminData excerpt ---'
sed -n '1,220p' src/admin/hooks/useAdminData.ts
echo '--- search useCollectionData / compare / loadMore ---'
rg -n "useCollectionData|compare\\b|loadMore\\b|pageLimit\\b|firestoreLimit\\(" src/admin -g '*.ts' -g '*.tsx'Repository: hrx01-dev/Servio
Length of output: 9030
Add a matching orderBy for the paginated collections
query(ref, firestoreLimit(pageLimit)) returns a prefix of Firestore’s default doc-id order, then compare re-sorts only that subset. For collections displayed by createdAt or clientEmail, loadMore can skip items that should appear in the current page.
Thread an explicit server-side orderBy through useCollectionData for the sorted call sites, and keep the client sort only if it matches that same field.
🤖 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/admin/hooks/useAdminData.ts` around lines 110 - 120, The paginated query
in useAdminData/useCollectionData is only applying firestoreLimit(pageLimit) and
then re-sorting client-side, which can drop items across pages for collections
ordered by createdAt or clientEmail. Update useCollectionData to accept and
apply a matching server-side orderBy in the Firestore query for the sorted call
sites, using the existing compare/parsing flow in useAdminData to locate the
pagination logic, and keep the client-side compare sort only when it matches the
same field as the server order.
…in has no slash Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…re pages the filtered set Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
Closes #198
What changed
useClientPayments.ts: AddedpaymentErrorMessage()to map Firestoreerror codes to user-friendly messages instead of surfacing the raw
err.messagestring.PaymentManagement.tsx: Error state now renders the specific messagefrom the hook, and a "Try again" button lets the client reload without
navigating away.
Files changed
src/dashboard/hooks/useClientPayments.tssrc/dashboard/pages/PaymentManagement.tsxSummary by CodeRabbit