Merged
Conversation
WalkthroughSeveral account pages were converted from client components to server-side async pages; server pages now fetch data and delegate interactive flows to new client components (AddressManagement, CreditCardList, GiftCardList, OrderList, OrderDetail), plus shared formatting utilities were added. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant ServerPage
participant ClientComponent
participant SDK_API
participant DB
Browser->>ServerPage: Request account page (SSR)
ServerPage->>SDK_API: get*() calls (addresses/cards/gift-cards/orders) (parallel)
SDK_API->>DB: query data
DB-->>SDK_API: data
SDK_API-->>ServerPage: response
ServerPage-->>Browser: render page with initial props (e.g., initialAddresses)
Note right of Browser: Interactive flows handled client-side
Browser->>ClientComponent: user action (add/edit/delete)
ClientComponent->>SDK_API: create/update/delete API call
SDK_API->>DB: persist change
DB-->>SDK_API: result
SDK_API-->>ClientComponent: result
ClientComponent-->>Browser: update local UI state
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![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
🧹 Nitpick comments (1)
src/app/[country]/[locale]/(storefront)/account/addresses/page.tsx (1)
6-6: Consider adding page metadata for consistency.As per coding guidelines, page files should use the
generateMetadatafunction for SEO. While this account page may not need search indexing, adding basic metadata improves consistency and browser tab titles.📝 Example metadata
import type { Metadata } from "next"; export const metadata: Metadata = { title: "Addresses", description: "Manage your saved addresses", };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[country]/[locale]/(storefront)/account/addresses/page.tsx at line 6, Add basic page metadata for consistency by exporting a Metadata object or implementing generateMetadata alongside the existing AddressesPage component; declare and export either export const metadata: Metadata = { title: "Addresses", description: "Manage your saved addresses" } or an async function generateMetadata() that returns equivalent metadata so the browser tab title and SEO fields are set for the AddressesPage export.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/components/addresses/AddressManagement.tsx`:
- Around line 35-39: The handleDelete function sets deleting state before
awaiting onDelete but doesn’t reset it if onDelete throws; wrap the await call
in a try/finally so setDeleting(false) always runs (i.e., keep setDeleting(true)
at start, then try { await onDelete(); } finally { setDeleting(false); }) to
ensure the deleting state is cleared even on errors—update the handleDelete
function to use this pattern.
- Around line 147-153: The handleDelete function currently ignores failures from
deleteAddress and gives no user feedback; update handleDelete to check the
returned result when deleteAddress(id) resolves and, if result.success is false,
surface the error to the user (e.g., call your toast/alert API or at minimum
console.error) instead of silently doing nothing; ensure you still only call
setAddresses((prev)=>prev.filter(...)) when result.success is true, and
optionally wrap the await deleteAddress(id) in a try/catch to handle unexpected
exceptions and report them via the same notification mechanism.
---
Nitpick comments:
In `@src/app/`[country]/[locale]/(storefront)/account/addresses/page.tsx:
- Line 6: Add basic page metadata for consistency by exporting a Metadata object
or implementing generateMetadata alongside the existing AddressesPage component;
declare and export either export const metadata: Metadata = { title:
"Addresses", description: "Manage your saved addresses" } or an async function
generateMetadata() that returns equivalent metadata so the browser tab title and
SEO fields are set for the AddressesPage export.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e4b92659-2f4e-4fb7-8e8a-316e8a5577b4
📒 Files selected for processing (2)
src/app/[country]/[locale]/(storefront)/account/addresses/page.tsxsrc/components/addresses/AddressManagement.tsx
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/components/account/CreditCardList.tsx (1)
107-107: Prefer non-blocking error feedback overalert().
alert()works, but it blocks interaction and is jarring. Prefer your toast/inline notification pattern for delete failures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/account/CreditCardList.tsx` at line 107, The alert() call in CreditCardList.tsx (on failed delete: alert(`Failed to remove card: ${result.error}`)) is blocking and should be replaced with the app's non-blocking notification pattern; update the failure branch in the remove-card handler (e.g., the function handling the deletion in CreditCardList or its handleRemoveCard/removeCard code path) to call the project's toast/notification API (for example toast.error(...) or the useNotification/setNotification helper used elsewhere) and include the result.error text in the message so the user sees a non-blocking, contextual error notification.src/app/[country]/[locale]/(storefront)/account/orders/page.tsx (1)
11-16: StartgetOrders()before awaitingparams.These two awaits are independent here, so doing them serially adds avoidable latency to every request.
As per coding guidelines, "Use Promise.all() for parallel data fetching in server components instead of sequential awaits."⚡ Suggested refactor
export default async function OrdersPage({ params }: OrdersPageProps) { - const { country, locale } = await params; - const basePath = `/${country}/${locale}`; - - const response = await getOrders({ limit: 50 }); + const [{ country, locale }, response] = await Promise.all([ + params, + getOrders({ limit: 50 }), + ]); + const basePath = `/${country}/${locale}`; const orders = response.data.filter((o) => o.completed_at !== null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/app/`[country]/[locale]/(storefront)/account/orders/page.tsx around lines 11 - 16, OrdersPage currently awaits params and getOrders sequentially causing extra latency; start the getOrders call before awaiting params and then await both in parallel (e.g., initiate const ordersPromise = getOrders({ limit: 50 }); then await params and ordersPromise with Promise.all or await params then await ordersPromise) so you still build basePath from params and compute orders = response.data.filter(...) after the response resolves; update references to getOrders, params, basePath, OrdersPage, and orders accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/`[country]/[locale]/(storefront)/account/orders/[id]/page.tsx:
- Around line 18-20: The order detail page calls getOrder(id) but does not
enforce the same "completed" filter used by the history list, allowing
draft/abandoned checkouts to render; update the page (page.tsx) after the
getOrder(id) call to verify the order matches the history-list predicate (e.g.
order.status === 'completed' or the exact condition used by the sibling history
code) and if it does not, return a proper non-found/redirect response (e.g. call
notFound() or redirect to the orders list) so only completed orders render on
/account/orders/[id].
In `@src/app/`[country]/[locale]/(storefront)/account/orders/page.tsx:
- Around line 15-16: The current call to getOrders({ limit: 50 }) applies the
limit before you filter out incomplete orders, causing completed history to be
truncated; update the fetching logic in page.tsx so completed orders are
selected at query time (e.g., pass a filter that requires completed_at != null
to getOrders) or implement pagination that continues requesting pages until you
have the desired count of completed orders before slicing; reference getOrders,
the limit: 50 argument, and the completed_at filter when making this change.
In `@src/lib/utils/format.ts`:
- Around line 10-18: The function formatDateTime currently calls
toLocaleDateString with time options which will throw a TypeError; change the
call to use toLocaleString instead and keep the same locale "en-US" and the
options object (year, month, day, hour, minute) so date and time are formatted
correctly; update the formatDateTime function to return new
Date(dateString).toLocaleString("en-US", { year: "numeric", month: "long", day:
"numeric", hour: "2-digit", minute: "2-digit" }) when dateString is present and
still return "-" when null/empty.
---
Nitpick comments:
In `@src/app/`[country]/[locale]/(storefront)/account/orders/page.tsx:
- Around line 11-16: OrdersPage currently awaits params and getOrders
sequentially causing extra latency; start the getOrders call before awaiting
params and then await both in parallel (e.g., initiate const ordersPromise =
getOrders({ limit: 50 }); then await params and ordersPromise with Promise.all
or await params then await ordersPromise) so you still build basePath from
params and compute orders = response.data.filter(...) after the response
resolves; update references to getOrders, params, basePath, OrdersPage, and
orders accordingly.
In `@src/components/account/CreditCardList.tsx`:
- Line 107: The alert() call in CreditCardList.tsx (on failed delete:
alert(`Failed to remove card: ${result.error}`)) is blocking and should be
replaced with the app's non-blocking notification pattern; update the failure
branch in the remove-card handler (e.g., the function handling the deletion in
CreditCardList or its handleRemoveCard/removeCard code path) to call the
project's toast/notification API (for example toast.error(...) or the
useNotification/setNotification helper used elsewhere) and include the
result.error text in the message so the user sees a non-blocking, contextual
error notification.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fe59f0f4-cb80-464f-9a54-c1e54675b080
📒 Files selected for processing (10)
src/app/[country]/[locale]/(storefront)/account/credit-cards/page.tsxsrc/app/[country]/[locale]/(storefront)/account/gift-cards/page.tsxsrc/app/[country]/[locale]/(storefront)/account/orders/[id]/page.tsxsrc/app/[country]/[locale]/(storefront)/account/orders/page.tsxsrc/components/account/CreditCardList.tsxsrc/components/account/GiftCardList.tsxsrc/components/account/OrderDetail.tsxsrc/components/account/OrderList.tsxsrc/components/addresses/AddressManagement.tsxsrc/lib/utils/format.ts
src/app/[country]/[locale]/(storefront)/account/orders/[id]/page.tsx
Outdated
Show resolved
Hide resolved
src/app/[country]/[locale]/(storefront)/account/orders/page.tsx
Outdated
Show resolved
Hide resolved
damianlegawiec
changed the title
useEffect from account addressesuseEffect from account
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/addresses/AddressManagement.tsx (1)
32-32: Add explicit return types to functions/handlers.Please annotate return types for
AddressCard,fetchStates,handleEdit,handleAdd,handleSave, andhandleDeleteinstead of relying on inference.🧩 Example
-function AddressCard({ address, onEdit, onDelete }: AddressCardProps) { +function AddressCard({ address, onEdit, onDelete }: AddressCardProps): JSX.Element { @@ - const fetchStates = useCallback(async (countryIso: string) => { + const fetchStates = useCallback(async (countryIso: string): Promise<Country["states"]> => { @@ - const handleEdit = (address: Address) => { + const handleEdit = (address: Address): void => { @@ - const handleAdd = () => { + const handleAdd = (): void => { @@ - const handleSave = async (data: AddressParams, id?: string) => { + const handleSave = async (data: AddressParams, id?: string): Promise<void> => { @@ - const handleDelete = async (id: string) => { + const handleDelete = async (id: string): Promise<void> => {As per coding guidelines: "Use strict TypeScript type checking. Always define explicit return types for functions..."
Also applies to: 107-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/addresses/AddressManagement.tsx` at line 32, Add explicit return type annotations for AddressCard, fetchStates, handleEdit, handleAdd, handleSave, and handleDelete: annotate the React component AddressCard as returning JSX.Element; annotate fetchStates with the correct Promise return (e.g., Promise<State[]> if it returns data or Promise<void> if it only sets state); and annotate all handlers (handleEdit, handleAdd, handleSave, handleDelete) as returning void or Promise<void> as appropriate. Update the function signatures for these symbols to include the explicit return types to satisfy strict TypeScript checking.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/app/`[country]/[locale]/(storefront)/account/orders/page.tsx:
- Around line 15-16: The order list is including incomplete orders (completed_at
=== null) which leads to "Order not found" on details; fix by ensuring only
completed orders are shown: either update the API call getOrders to accept and
send a filter for completed_at (e.g., pass a param to only fetch orders with
completed_at !== null) or restore the client-side filtering before rendering
(filter the orders array returned from getOrders by completed_at !== null) and
then pass the filtered array to OrderList so the detail guard no longer
triggers.
In `@src/components/addresses/AddressManagement.tsx`:
- Around line 159-174: The empty-state rendering currently relies solely on the
immutable prop emptyState which can leave the UI stuck after creating an
address; update the render gating to use the live addresses collection instead
(e.g. check addresses.length === 0) so the component exits the empty state as
soon as addresses contains items. Locate the conditional that returns the Add
Your First Address button and AddressEditModal (the block referencing
emptyState, handleAdd, modalOpen, AddressEditModal and editingAddress) and
replace its predicate with a runtime check against addresses.length (or combine
both: emptyState && addresses.length === 0) so the UI immediately shows the
address grid after a successful create.
---
Nitpick comments:
In `@src/components/addresses/AddressManagement.tsx`:
- Line 32: Add explicit return type annotations for AddressCard, fetchStates,
handleEdit, handleAdd, handleSave, and handleDelete: annotate the React
component AddressCard as returning JSX.Element; annotate fetchStates with the
correct Promise return (e.g., Promise<State[]> if it returns data or
Promise<void> if it only sets state); and annotate all handlers (handleEdit,
handleAdd, handleSave, handleDelete) as returning void or Promise<void> as
appropriate. Update the function signatures for these symbols to include the
explicit return types to satisfy strict TypeScript checking.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c16092c2-7d07-4056-a1b3-5d69dcd11ece
📒 Files selected for processing (4)
src/app/[country]/[locale]/(storefront)/account/orders/[id]/page.tsxsrc/app/[country]/[locale]/(storefront)/account/orders/page.tsxsrc/components/addresses/AddressManagement.tsxsrc/lib/utils/format.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/utils/format.ts
src/app/[country]/[locale]/(storefront)/account/orders/page.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/components/addresses/AddressManagement.tsx (2)
26-30: Type signature foronDeleteshould reflect async nature.The
onDeletecallback is awaited on line 38, and the actual function passed (line 193) returnsPromise<void>. Update the type to accurately reflect this.✏️ Suggested fix
interface AddressCardProps { address: Address; onEdit: () => void; - onDelete: () => void; + onDelete: () => Promise<void>; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/addresses/AddressManagement.tsx` around lines 26 - 30, Update the AddressCardProps type so the onDelete callback reflects that it returns a Promise; change its signature from onDelete: () => void to onDelete: () => Promise<void> (and update any related usages if necessary) so the await on onDelete in the AddressCard component matches the actual async function being passed.
126-148: Non-null assertions can be avoided by restructuring.The
result.address!assertions (lines 135, 145) are safe since they're insideif (result.address)blocks, but the pattern can be simplified.✏️ Suggested refactor
if (id) { const result = await updateAddress(id, data); if (!result.success) { throw new Error(result.error); } - // Update just the one address in state - if (result.address) { - setAddresses((prev) => - prev.map((addr) => (addr.id === id ? result.address! : addr)), - ); - } + // Update just the one address in state + const { address } = result; + if (address) { + setAddresses((prev) => + prev.map((addr) => (addr.id === id ? address : addr)), + ); + } } else { const result = await createAddress(data); if (!result.success) { throw new Error(result.error); } - // Append the new address - if (result.address) { - setAddresses((prev) => [...prev, result.address!]); - } + // Append the new address + const { address } = result; + if (address) { + setAddresses((prev) => [...prev, address]); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/addresses/AddressManagement.tsx` around lines 126 - 148, The code in handleSave uses non-null assertions result.address! inside branches that already check if (result.address); remove the assertions by restructuring to use the narrowed variable directly — for example, within the if (result.address) blocks, store const addr = result.address and pass addr to setAddresses (in the update branch map replacement for addr.id and in the create branch appending addr) so you avoid the ! operator and keep type safety; references: handleSave, updateAddress, createAddress, setAddresses, result.address.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/components/addresses/AddressManagement.tsx`:
- Around line 26-30: Update the AddressCardProps type so the onDelete callback
reflects that it returns a Promise; change its signature from onDelete: () =>
void to onDelete: () => Promise<void> (and update any related usages if
necessary) so the await on onDelete in the AddressCard component matches the
actual async function being passed.
- Around line 126-148: The code in handleSave uses non-null assertions
result.address! inside branches that already check if (result.address); remove
the assertions by restructuring to use the narrowed variable directly — for
example, within the if (result.address) blocks, store const addr =
result.address and pass addr to setAddresses (in the update branch map
replacement for addr.id and in the create branch appending addr) so you avoid
the ! operator and keep type safety; references: handleSave, updateAddress,
createAddress, setAddresses, result.address.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 88fc5e12-4d7e-4c50-8c47-7ae4d62ea7f0
📒 Files selected for processing (2)
src/app/[country]/[locale]/(storefront)/account/orders/page.tsxsrc/components/addresses/AddressManagement.tsx
✅ Files skipped from review due to trivial changes (1)
- src/app/[country]/[locale]/(storefront)/account/orders/page.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor