fix: client id creation, fetching of routes and dark mode issues

[bassgeta]

Problems

1. You couldn't create client IDs without setting feeAddress and feePercentage.
2. If Easy Invoice didn't have the environment variables for the fee address and fee percentage, it was impossible to get LiFi routes.
3. Some dark mode issues :)

Solutions

1. Explicitly send undefined instead of null when creating a client ID.
2. If the variables are undefined, don't send them as strings of undefined but do not add them as query params at all
3. Use proper colours instead of hardcoding shades of gray and such.

Changes

• the create function in routers/ecommerce.ts now fallbacks to undefined explicitly in case of falsey values
• when fetching payment routes, we conditionally append the fee address and percentage from process.env
• fix Tailwind colour variables where applicable

Summary by CodeRabbit

• Bug Fixes
  • Improved handling of optional fee address and fee percentage fields. When left blank, they are now treated as unset rather than empty or null, preventing validation issues and reducing errors during client ID creation.
  • Increased robustness of input normalization for these fields, ensuring smoother submissions and more consistent behavior across the app.

[coderabbitai]

Walkthrough

Schema normalization now returns undefined for empty/missing feeAddress/feePercentage; router create flow sends undefined (not null) to external v2/client-ids. Payment route lookup conditionally includes platform fee query params based on env vars. Several UI/theme token and styling updates applied.

Changes

Cohort / File(s)	Summary
Ecommerce schema src/lib/schemas/ecommerce.ts	Normalize feeAddress and feePercentage to return undefined for empty/undefined inputs; remove redundant .optional() after refine; minor formatting.
Router: ecommerce create src/server/routers/ecommerce.ts	Send feeAddress/feePercentage as undefined (instead of null) to external v2/client-ids in create flow; DB storage unchanged.
Router: invoice payment routes src/server/routers/invoice.ts	Conditionally append feePercentage and feeAddress query params to payment routes URL only if env vars are set.
UI / global styles src/app/globals.css, src/app/not-found.tsx, src/components/payment-route.tsx, src/lib/invoice-status.ts	Swap popover color tokens for light/dark themes and fix missing semicolons in CSS; update NotFound page and button/card token classes for dark mode and semantic tokens; adjust direct-payment badge text color; replace concrete color classes with semantic token classes in payment status mapping.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Client
  participant Server (Ecommerce Router)
  participant Schema
  participant External API (v2/client-ids)
  participant DB

  Client->>Server (Ecommerce Router): POST /create (feeAddress?, feePercentage?)
  Server (Ecommerce Router)->>Schema: validate & normalize input
  note right of Schema: empty/undefined -> undefined
  Schema-->>Server (Ecommerce Router): normalized payload
  Server (Ecommerce Router)->>External API (v2/client-ids): POST payload (feeAddress: undefined, feePercentage: undefined when absent)
  External API (v2/client-ids)-->>Server (Ecommerce Router): Response
  Server (Ecommerce Router)->>DB: Persist record (DB may store null)
  DB-->>Server (Ecommerce Router): Ack
  Server (Ecommerce Router)-->>Client: Response

Loading

sequenceDiagram
  autonumber
  participant Client
  participant Server (Invoice Router)
  participant PaymentService

  Client->>Server (Invoice Router): GET /payment-routes?invoiceId=...
  Server (Invoice Router)->>Server (Invoice Router): check env for FEE_PERCENTAGE & FEE_ADDRESS
  alt env vars set
    Server (Invoice Router)->>PaymentService: GET /payment-routes?...&feePercentage=...&feeAddress=...
  else env vars unset
    Server (Invoice Router)->>PaymentService: GET /payment-routes?...
  end
  PaymentService-->>Server (Invoice Router): Routes
  Server (Invoice Router)-->>Client: Routes

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• feat: add platform fee support to payment routes and sections #77 — Related changes to src/server/routers/invoice.ts involving how feePercentage/feeAddress are passed to payment route requests.
• feat: integrate cross-chain payment into EasyInvoice #42 — Related payment-related changes touching invoice router and payment-route component.

Suggested reviewers

• rodrigopavezi
• aimensahnoun
• MantisClone

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.
Title Check	❓ Inconclusive	The title references client ID creation, route fetching, and dark mode but uses broad, generic phrases that don’t clearly convey the specific fixes or the primary change in this pull request.	Please update the title to a concise, specific summary of the actual fixes—for example “fix: send undefined for fee fields in client ID creation and invoice route calls; update dark mode popover colors”—or consider splitting unrelated UI and API fixes into separate pull requests.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch hotfix/client-id-creation

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/server/routers/ecommerce.ts (1)

95-96: Critical: Edit procedure still sends null to external API.

The edit procedure still uses ?? null when calling the external API, which likely suffers from the same issue that affected the create procedure. If the external v2/client-ids API rejects null values for feePercentage and feeAddress during creation, it may also reject them during updates.

Apply this diff to maintain consistency with the create procedure:

-            feePercentage: input.feePercentage ?? null,
-            feeAddress: input.feeAddress ?? null,
+            feePercentage: input.feePercentage ?? undefined,
+            feeAddress: input.feeAddress ?? undefined,

🧹 Nitpick comments (1)

src/lib/schemas/ecommerce.ts (1)

31-53: Consider extracting the transform logic to reduce duplication.

Both feeAddress and feePercentage use identical transform logic. Consider extracting it to a shared helper:

const normalizeToUndefined = (val: string | undefined) => {
  if (val === undefined || val === "") return undefined;
  return val;
};

// Then use:
.transform(normalizeToUndefined)

This improves maintainability and ensures consistency if the normalization logic needs to change in the future.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a038333 and de93678.

📒 Files selected for processing (2)

• src/lib/schemas/ecommerce.ts (1 hunks)
• src/server/routers/ecommerce.ts (1 hunks)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Build

🔇 Additional comments (4)

src/lib/schemas/ecommerce.ts (2)

31-41: LGTM! Schema normalization is correct.

The refactored validation chain properly handles optional fee fields by:

• Placing .optional() before the transform to allow undefined values through the chain
• Normalizing empty strings to undefined in the transform
• Removing the redundant trailing .optional() after refine (since undefined is already permitted)

This ensures consistent undefined values flow through to the router, where they can be properly handled for the external API.

---

42-53: LGTM! Consistent with feeAddress validation.

The validation chain follows the same pattern as feeAddress, ensuring both fee fields are normalized consistently.

src/server/routers/ecommerce.ts (2)

35-36: LGTM! Correctly sends undefined to external API.

The change from ?? null to ?? undefined ensures that when these fields are not provided, they are omitted from the JSON payload sent to the external API (since undefined properties are typically not serialized), rather than being explicitly set to null. This fixes the client ID creation issue described in the PR.

---

50-51: LGTM! Database writes correctly use null.

The database writes for both create (lines 50-51) and edit (lines 105-106) correctly use ?? null, which is appropriate for database storage where NULL is the standard way to represent absent values.

Also applies to: 105-106

[MantisClone]

⚠️ Outside diff range comments (1)

src/server/routers/ecommerce.ts (1)> 95-96: Critical: Edit procedure still sends null to external API.

The edit procedure still uses ?? null when calling the external API, which likely suffers from the same issue that affected the create procedure. If the external v2/client-ids API rejects null values for feePercentage and feeAddress during creation, it may also reject them during updates.
Apply this diff to maintain consistency with the create procedure:

-            feePercentage: input.feePercentage ?? null,
-            feeAddress: input.feeAddress ?? null,
+            feePercentage: input.feePercentage ?? undefined,
+            feeAddress: input.feeAddress ?? undefined,

@coderabbitai - The edit procedure is actually correct as-is. The backend API's update endpoint uses .nullable().optional() (not just .optional() like create), which explicitly allows null values to support unsetting both fees together. This is intentional behavior, not a bug. Only the create endpoint rejects null.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Examine the edit procedure in the ecommerce router
echo "=== Edit procedure in ecommerce.ts ==="
rg -n -A 20 "\.mutation\(\"edit\"" src/server/routers/ecommerce.ts

echo -e "\n=== Search for backend schema definitions that might show .nullable().optional() ==="
rg -n "nullable\(\)\.optional\(\)|optional\(\)\.nullable\(\)" --type=ts

echo -e "\n=== Look for client-id related schemas ==="
fd -e ts -e js schema | head -20

Length of output: 192

---

🏁 Script executed:

#!/bin/bash
# Search for edit procedure more broadly
echo "=== Search for edit procedure ==="
rg -n "edit.*procedure\|procedure.*edit" src/server/routers/ecommerce.ts

echo -e "\n=== Show lines 80-120 of ecommerce.ts to see edit procedure ==="
sed -n '80,120p' src/server/routers/ecommerce.ts | cat -n

echo -e "\n=== Examine the database schema ==="
cat src/server/db/schema.ts | head -100

echo -e "\n=== Search for any backend API client-id schemas or references ==="
rg -n "client-id|clientId" --type=ts -C 2 | grep -i "schema\|nullable\|optional" | head -30

Length of output: 4599

---

@MantisClone, thank you for the clarification! You're absolutely right. I misunderstood the intentional design difference between the two endpoints:

• Create endpoint: Only accepts undefined (using .optional()) - hence the fix in this PR
• Edit endpoint: Explicitly accepts null (using .nullable().optional()) to allow unsetting fees

This design makes sense because during updates, you need a way to explicitly clear/unset fee values, whereas during creation, you simply omit them. I appreciate you taking the time to explain this distinction!

🐇✨
(Understanding the design nuances)

---

✏️ Learnings added

Learnt from: MantisClone
URL: https://github.com/RequestNetwork/easy-invoice/pull/160

Timestamp: 2025-10-13T19:12:33.805Z
Learning: In `src/server/routers/ecommerce.ts`, the `create` procedure for client IDs should use `?? undefined` for `feePercentage` and `feeAddress` when calling the external API, because the backend create endpoint uses `.optional()` and rejects `null`. However, the `edit` procedure should use `?? null` for these fields because the backend update endpoint uses `.nullable().optional()`, which allows `null` values to support explicitly unsetting fees.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

[MantisClone]

Nice one @bassgeta 💪

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/server/routers/invoice.ts (1)

450-458: Verify handling of partial fee configuration.

The condition requires both FEE_PERCENTAGE_FOR_PAYMENT and FEE_ADDRESS_FOR_PAYMENT to be defined. If only one environment variable is set, fee parameters are silently omitted from the request, which could lead to unintended behavior or misconfiguration going unnoticed.

Consider whether partial configuration should:

• Log a warning when only one env var is set
• Throw an error if the configuration is incomplete
• Or document that both must be set or neither

Additionally, this logic is duplicated in the payRequest procedure (lines 377-382). Consider extracting this into a helper function to maintain consistency.

// Helper function to add fee params if both are configured
const addFeeParamsIfConfigured = (params: URLSearchParams) => {
  if (
    process.env.FEE_PERCENTAGE_FOR_PAYMENT &&
    process.env.FEE_ADDRESS_FOR_PAYMENT
  ) {
    params.append("feePercentage", process.env.FEE_PERCENTAGE_FOR_PAYMENT);
    params.append("feeAddress", process.env.FEE_ADDRESS_FOR_PAYMENT);
  }
};

Then use it in both procedures:

// In getPaymentRoutes:
let url = `/v2/request/${requestId}/routes?wallet=${walletAddress}`;
if (
  process.env.FEE_PERCENTAGE_FOR_PAYMENT &&
  process.env.FEE_ADDRESS_FOR_PAYMENT
) {
  url += `&feePercentage=${process.env.FEE_PERCENTAGE_FOR_PAYMENT}&feeAddress=${process.env.FEE_ADDRESS_FOR_PAYMENT}`;
}

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between de93678 and b1313a2.

📒 Files selected for processing (5)

• src/app/globals.css (3 hunks)
• src/app/not-found.tsx (3 hunks)
• src/components/payment-route.tsx (1 hunks)
• src/lib/invoice-status.ts (1 hunks)
• src/server/routers/invoice.ts (1 hunks)

🧰 Additional context used

🧠 Learnings (4)

📓 Common learnings

Learnt from: MantisClone
PR: RequestNetwork/easy-invoice#0
File: :0-0
Timestamp: 2025-10-13T19:12:33.805Z
Learning: In `src/server/routers/ecommerce.ts`, the `create` procedure for client IDs should use `?? undefined` for `feePercentage` and `feeAddress` when calling the external API, because the backend create endpoint uses `.optional()` and rejects `null`. However, the `edit` procedure should use `?? null` for these fields because the backend update endpoint uses `.nullable().optional()`, which allows `null` values to support explicitly unsetting fees.

📚 Learning: 2025-08-08T09:52:43.700Z

Learnt from: bassgeta
PR: RequestNetwork/easy-invoice#117
File: src/app/not-found.tsx:29-67
Timestamp: 2025-08-08T09:52:43.700Z
Learning: Repository: RequestNetwork/easy-invoice
Context: src/app/not-found.tsx (Next.js, TypeScript/React)
Learning: It is acceptable (by current product decision) for the NotFound page to display links to /invoices/create and /subscription-plans even though these routes are auth-gated. Rationale: prevent the 404 from feeling empty; most users are authenticated. No lock icons or conditional rendering required for now; can be revisited later as an enhancement.

Applied to files:

• src/app/not-found.tsx

📚 Learning: 2025-10-13T19:12:33.805Z

Learnt from: MantisClone
PR: RequestNetwork/easy-invoice#0
File: :0-0
Timestamp: 2025-10-13T19:12:33.805Z
Learning: In `src/server/routers/ecommerce.ts`, the `create` procedure for client IDs should use `?? undefined` for `feePercentage` and `feeAddress` when calling the external API, because the backend create endpoint uses `.optional()` and rejects `null`. However, the `edit` procedure should use `?? null` for these fields because the backend update endpoint uses `.nullable().optional()`, which allows `null` values to support explicitly unsetting fees.

Applied to files:

• src/server/routers/invoice.ts

📚 Learning: 2025-02-12T12:40:14.742Z

Learnt from: aimensahnoun
PR: RequestNetwork/easy-invoice#2
File: src/server/routers/invoice.ts:88-109
Timestamp: 2025-02-12T12:40:14.742Z
Learning: The payRequest endpoint in src/server/routers/invoice.ts is intentionally kept public (using publicProcedure) to allow invoice sharing and payment by anyone with the payment reference, similar to how payment links work in other payment systems.

Applied to files:

• src/server/routers/invoice.ts

🧬 Code graph analysis (1)

src/server/routers/invoice.ts (1)

src/lib/axios.ts (1)

• apiClient (3-8)

🔇 Additional comments (5)

src/lib/invoice-status.ts (1)

74-90: LGTM! Good adoption of semantic design tokens.

The migration from hardcoded color classes to semantic design tokens (bg-success/10, text-success-foreground, etc.) improves maintainability and ensures consistent theming across light and dark modes. These changes align with the broader theming updates in src/app/globals.css.

src/app/globals.css (2)

10-11: Verify the popover color swap is intentional.

The popover background and foreground colors have been inverted in the light theme:

• --popover changed from 0 0% 3.9% (dark) to 0 0% 98% (light)
• --popover-foreground changed from 0 0% 98% (light) to 0 0% 3.9% (dark)

This swap significantly changes the visual appearance of popovers in light mode. If intentional, this improves contrast and readability. However, ensure this change is deliberate and doesn't break any existing UI components that rely on the previous color scheme.

---

30-30: LGTM! CSS syntax improvements.

Good catch on adding the missing semicolons to --radius (line 30) and --chart-5 (line 56). This ensures proper CSS validation and prevents potential parsing issues.

Also applies to: 56-56

src/app/not-found.tsx (1)

9-114: LGTM! Comprehensive theming updates.

The file has been thoroughly updated to use semantic design tokens throughout. Key improvements:

• Container backgrounds migrated to bg-background and bg-card
• Text colors standardized to foreground and muted-foreground tokens
• Consistent hover and focus states using ring tokens
• Enhanced dark mode support with explicit dark: variants

These changes align with the broader theming updates in src/app/globals.css and improve maintainability. The conditional rendering logic for authenticated vs. unauthenticated users remains unchanged and correct.

src/components/payment-route.tsx (1)

47-53: LGTM! Badge text color adjustment.

The change from text-primary-foreground to text-primary for the direct payment badge text color is a minor styling refinement that aligns with the semantic token usage across the application. This ensures consistent theming and proper contrast ratios.