Skip to content

Conversation

@lissavxo
Copy link
Collaborator

@lissavxo lissavxo commented Oct 2, 2025

Related to #818

Description

Added date range filter to payments page
the filter also applies to csv

Test plan

Go to payments page and make sure payments filter works as expected also test csv file

Summary by CodeRabbit

  • New Features

    • Date range filtering added to Payments page with start/end inputs; filters apply to list, pagination, counts, download and CSV export.
    • Payments flows now respect user-preferred timezone (fallbacks handled) and use timezone-aware timestamps.
    • Clear Filters resets date range; selecting a year clears date-range filters.
  • Tests

    • Added unit tests covering date-range and timezone-aware filtering behavior.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 2, 2025

Walkthrough

Adds optional startDate/endDate parsing and timezone propagation across frontend, API endpoints, and service layer. Service methods gain timezone-aware date-range utilities and parameters and apply a timestamp range filter when both dates are provided, otherwise falling back to existing year-based filters.

Changes

Cohort / File(s) Summary
API endpoints: date-range & timezone passthrough
pages/api/payments/index.ts, pages/api/payments/count/index.ts, pages/api/payments/download/index.ts
Parse optional startDate/endDate from req.query, derive/validate timezone (user preference or header fallback), and forward startDate, endDate, timezone to updated transactionService calls; preserve existing error handling.
Frontend: payments UI & requests
pages/payments/index.tsx
Add startDate/endDate state and date inputs; include date params and timezone in API calls (list, count, download, export); clear year selection when date range set and clear date fields when a year is selected; update effects and export/download URL construction.
Service layer: date-range utilities & filters
services/transactionService.ts
Replace moment with moment-timezone; add buildDateRange and getDateRangeFilter; add timezone, startDate?, endDate? parameters to public APIs (fetchAllPaymentsByUserId*, getFilteredTransactionCount); apply timestamp range filter when both dates provided, otherwise use year-based filters.
Tests: timezone & date-range coverage
tests/unittests/transactionService.test.ts
Add tests validating timezone-aware day boundaries, precedence of date-range over year filters, and correct where clauses for pagination and count flows; import moment-timezone and add helper utilities for expected ranges.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor U as User
  participant P as Payments Page (UI)
  participant API as /api/payments (GET)
  participant S as transactionService
  participant DB as Database
  note right of P `#DDEBF7`: UI sends years or date range + timezone
  U->>P: Set filters (years or startDate/endDate)
  P->>API: GET with userId, paging, years?, startDate?, endDate?, timezone?
  API->>S: fetchAllPaymentsByUserIdWithPagination(..., years?, timezone?, startDate?, endDate?)
  alt startDate & endDate provided
    S->>S: buildDateRange(startDate, endDate, timezone)
    S->>DB: Query with timestamp gte/lte filter
  else years provided
    S->>S: getYearFilters(years, timezone)
    S->>DB: Query with year-based filter
  end
  DB-->>S: Results
  S-->>API: Payments data
  API-->>P: JSON response
  P-->>U: Render payments
Loading
sequenceDiagram
  autonumber
  actor U as User
  participant P as Payments Page (UI)
  participant C as /api/payments/count
  participant D as /api/payments/download
  participant S as transactionService
  participant DB as Database
  note right of C `#F7F3D7`: Count/download include date range handling
  P->>C: GET count with years?/startDate?/endDate?/timezone?
  C->>S: getFilteredTransactionCount(..., years?, timezone?, startDate?, endDate?)
  alt startDate & endDate provided
    S->>S: buildDateRange(startDate, endDate, timezone)
    S->>DB: Count with timestamp range filter
  else years provided
    S->>S: getYearFilters(years, timezone)
    S->>DB: Count with year-based filter
  end
  DB-->>S: Count
  S-->>C: Count JSON
  C-->>P: Count response

  U->>P: Export CSV
  P->>D: GET download with years?/startDate?/endDate?/timezone?
  D->>S: fetchAllPaymentsByUserId(..., timezone?, startDate?, endDate?)
  alt startDate & endDate provided
    S->>S: buildDateRange(startDate, endDate, timezone)
    S->>DB: Query with timestamp range filter
  else years provided
    S->>S: getYearFilters(years, timezone)
    S->>DB: Query with year-based filter
  end
  DB-->>S: Rows
  S-->>D: CSV data
  D-->>P: CSV stream
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Review focus:
    • Correctness of timezone-aware bounds in buildDateRange and Prisma where clauses.
    • Consistency of updated service method signatures across all API call sites and frontend usages.
    • Frontend filter interactions and construction of export/download URLs (query separator logic).

Poem

I nibble dates from data rows,
Hop between the start and close.
Timezones guide my carrot-way,
Years step back when ranges play.
CSVs and counts — a rabbit's day 🐇📅

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
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.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: adding date range filtering to the payments functionality.
Description check ✅ Passed The description covers the main feature (date range filter on payments page) and test plan (verify filter works and CSV respects it), though it lacks technical details about implementation approach.
✨ 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 feat/date-range-filter

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.

❤️ Share

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

@lissavxo lissavxo requested a review from chedieck October 2, 2025 20:14
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
pages/payments/index.tsx (1)

140-142: Consider adding startDate to useEffect dependencies.

The useEffect only includes endDate in its dependencies, which means changing startDate alone won't trigger a data refresh. While users typically set startDate first and then endDate (which would trigger the fetch), a user who modifies only startDate after setting both dates would see stale data.

Apply this diff to include startDate in the dependencies:

   useEffect(() => {
     setRefreshCount(prev => prev + 1)
-  }, [selectedButtonIds, selectedTransactionYears, endDate])
+  }, [selectedButtonIds, selectedTransactionYears, startDate, endDate])
services/transactionService.ts (1)

752-763: Consider consistent parameter ordering across functions.

The parameter order differs between functions:

  • fetchAllPaymentsByUserIdWithPagination (lines 752-763): ..., timezone?, startDate?, endDate?
  • fetchAllPaymentsByUserId (lines 889-897): ..., startDate?, endDate?, timezone?
  • getFilteredTransactionCount (lines 949-956): ..., timezone?, startDate?, endDate?

While this doesn't cause bugs (calls pass parameters correctly), consistent ordering improves maintainability.

Consider standardizing the order (e.g., always place timezone before startDate and endDate) in a follow-up refactor:

 export async function fetchAllPaymentsByUserId (
   userId: string,
   networkIds?: number[],
   buttonIds?: string[],
   years?: string[],
+  timezone?: string,
   startDate?: string,
-  endDate?: string,
-  timezone?: string
+  endDate?: string
 ): Promise<TransactionsWithPaybuttonsAndPrices[]> {

Also applies to: 889-897, 949-956

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 87b146b and e5515fb.

📒 Files selected for processing (5)
  • pages/api/payments/count/index.ts (1 hunks)
  • pages/api/payments/download/index.ts (1 hunks)
  • pages/api/payments/index.ts (2 hunks)
  • pages/payments/index.tsx (8 hunks)
  • services/transactionService.ts (7 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
pages/api/payments/count/index.ts (3)
pages/api/payments/download/index.ts (1)
  • req (13-80)
pages/api/payments/index.ts (1)
  • req (5-48)
services/transactionService.ts (1)
  • getFilteredTransactionCount (949-982)
pages/api/payments/index.ts (2)
pages/api/payments/count/index.ts (1)
  • req (6-43)
pages/api/payments/download/index.ts (1)
  • req (13-80)
pages/api/payments/download/index.ts (3)
pages/api/payments/count/index.ts (1)
  • req (6-43)
pages/api/payments/index.ts (1)
  • req (5-48)
services/transactionService.ts (1)
  • fetchAllPaymentsByUserId (889-935)
⏰ 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: Run Tests
🔇 Additional comments (3)
pages/api/payments/count/index.ts (1)

23-35: LGTM!

The startDate and endDate parameter parsing follows the established pattern for buttonIds and years. The filter condition correctly requires both dates to be present and non-empty before enabling date-range filtering, and the function call properly passes timezone along with the date range parameters.

pages/payments/index.tsx (1)

553-603: Date range and year filter coordination looks good.

The mutual exclusion between date range and year filters is correctly implemented:

  • Selecting years clears the date range (lines 558-561)
  • Setting startDate clears years and resets endDate (lines 582-584)
  • Setting endDate clears years (lines 595-597)

The min attribute on the endDate input (line 599) properly prevents selecting an end date before the start date.

pages/api/payments/index.ts (1)

22-45: LGTM!

The startDate and endDate parameters are correctly parsed and passed to fetchAllPaymentsByUserIdWithPagination with proper timezone handling.

Copy link
Collaborator

@chedieck chedieck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would expect the end date to be included, if nothing is said. Right now the initial date is included and the end date is not.

One option is to add a toggle in the rightside of the selectors saying "Include end date", but if you want just to make it include it and that's it, would be fine too

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (1)
services/transactionService.ts (1)

837-867: Critical: Timezone handling remains broken (duplicate).

The timezone conversion issue flagged in the previous review has not been addressed. The current approach using toLocaleString and re-parsing produces incorrect offsets because:

  1. new Date(startDate) interprets the string in UTC (for ISO format)
  2. toLocaleString formats in the target timezone but returns a string
  3. Re-parsing with new Date() uses the server's timezone, not the target timezone
  4. The offset calculation is therefore meaningless

This will cause date filters to return incorrect results for non-UTC timezones.

As recommended in the previous review, use a proper timezone library like moment-timezone or date-fns-tz to correctly handle timezone conversions.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e5515fb and a7f91c2.

📒 Files selected for processing (1)
  • services/transactionService.ts (7 hunks)
🔇 Additional comments (2)
services/transactionService.ts (2)

760-762: LGTM! Parameter additions are backward compatible.

The new optional parameters for timezone and date range filtering are well-integrated into the function signature.


895-897: Consistent implementation across functions.

The date range filtering logic has been consistently applied to fetchAllPaymentsByUserId and getFilteredTransactionCount with the same parameter additions and control flow.

However, these functions inherit the same issues identified in fetchAllPaymentsByUserIdWithPagination:

  • Critical timezone handling bug (lines 837-867)
  • Critical year filter off-by-one bug (lines 877-888)
  • Missing date validation (lines 802-806)

Please address the issues noted in the other review comments.

Also applies to: 923-926, 954-956, 976-979

Comment on lines 802 to 806
if (startDate !== undefined && endDate !== undefined && startDate !== '' && endDate !== '') {
Object.assign(where, getDateRangeFilter(startDate, endDate, timezone))
} else if (years !== undefined && years.length > 0) {
where.OR = getYearFilters(years, timezone)
}
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Add validation for date range inputs.

The code checks for undefined and empty strings but doesn't validate:

  1. Whether startDate and endDate are valid date strings
  2. Whether startDate <= endDate

Invalid or misordered dates will cause silent failures or unexpected query results.

Add validation before applying the filter:

  if (startDate !== undefined && endDate !== undefined && startDate !== '' && endDate !== '') {
+   const start = new Date(startDate)
+   const end = new Date(endDate)
+   if (isNaN(start.getTime()) || isNaN(end.getTime())) {
+     throw new Error('Invalid date format for startDate or endDate')
+   }
+   if (start > end) {
+     throw new Error('startDate must be less than or equal to endDate')
+   }
    Object.assign(where, getDateRangeFilter(startDate, endDate, timezone))
  } else if (years !== undefined && years.length > 0) {
    where.OR = getYearFilters(years, timezone)
  }
📝 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.

Suggested change
if (startDate !== undefined && endDate !== undefined && startDate !== '' && endDate !== '') {
Object.assign(where, getDateRangeFilter(startDate, endDate, timezone))
} else if (years !== undefined && years.length > 0) {
where.OR = getYearFilters(years, timezone)
}
if (startDate !== undefined && endDate !== undefined && startDate !== '' && endDate !== '') {
const start = new Date(startDate)
const end = new Date(endDate)
if (isNaN(start.getTime()) || isNaN(end.getTime())) {
throw new Error('Invalid date format for startDate or endDate')
}
if (start > end) {
throw new Error('startDate must be less than or equal to endDate')
}
Object.assign(where, getDateRangeFilter(startDate, endDate, timezone))
} else if (years !== undefined && years.length > 0) {
where.OR = getYearFilters(years, timezone)
}
🤖 Prompt for AI Agents
In services/transactionService.ts around lines 802 to 806, the branch applies a
date-range filter without validating the inputs; parse startDate and endDate
into real dates (using Date or your project's date util with timezone), check
both are valid, and ensure start <= end; if either string is not a valid date or
start > end either return/throw a validation error or normalize by swapping the
dates before calling getDateRangeFilter, and only call Object.assign(where,
getDateRangeFilter(...)) when the parsed dates pass these checks.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is relevant

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chedieck, understood! I'll defer to your judgment on this. Feel free to reach out if you need anything else.

Comment on lines 802 to 806
if (startDate !== undefined && endDate !== undefined && startDate !== '' && endDate !== '') {
Object.assign(where, getDateRangeFilter(startDate, endDate, timezone))
} else if (years !== undefined && years.length > 0) {
where.OR = getYearFilters(years, timezone)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is relevant

timezone?: string
): Prisma.TransactionWhereInput[] => {
return years.map((year) => {
const startDate = `${year}-01-01T00:00:00`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like this string formatting. Probably not injectable because I think prisma, in general, avoid injections, but this is bad practice anyway, and would be best written by creating a Date object using the year.

This whole file new additions are kind of weird tbh, why make buildDateRange accept startDate and endDate as string OR Date and then only give string to it? best to actually only accept Date and avoid having to do the conversion in the beginning of the function.

buttonIds?: string[],
years?: string[],
timezone?: string
timezone?: string,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd make timezone a mandatory parameter, we're always passing it at pages/api/payments/index.ts:42 anyways. It will help avoid forgetting it and making mistakes in the future. If we want to call this somewhere else in the future where we want to use UTC (no timezone), we simply pass UTC.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (1)
services/transactionService.ts (1)

838-879: Timezone math in buildDateRange/getYearFilters is still incorrect for non‑UTC timezones

The new implementation fixes the previous toLocaleString re-parse bug, but it still does not interpret dates in the user’s timezone correctly:

  1. User-provided dates (startDate/endDate):

    • Callers do new Date(startDate) and new Date(endDate).
    • For ISO-like strings ('2024-01-15'), new Date() creates an instant at midnight UTC, not “midnight in the user’s timezone”.
    • moment.tz(Date, timezone) treats the Date as a fixed instant and only changes its representation. The .unix() value for start is therefore the UTC midnight instant, not the start-of-day in the user’s timezone.
    • For end, moment.tz(endDateInstant, timezone).endOf('day') computes endOf('day') of the local day that corresponds to that UTC instant. For many timezones this is the previous calendar day, so the end boundary is shifted.

    The resulting range includes transactions that are outside the user’s intended local dates (and may miss some that are inside).

  2. Year filters (getYearFilters):

    • new Date(Number(year), 0, 1, 0, 0, 0) and new Date(Number(year), 11, 31, 23, 59, 59) create Dates in the server’s local timezone.
    • buildDateRange then reinterprets those instants in the user’s timezone, which only happens to be correct when server tz == user tz.
    • On a typical UTC server with a non-UTC user, start/end again do not align with that user’s local calendar year.

To get the correct behavior, you need to construct the date boundaries directly in the target timezone, then convert them to epoch seconds:

  • For a date range: treat startDate/endDate as calendar dates in the user timezone.
  • For a year: build start/end-of-year directly in the user timezone (or UTC if no timezone is provided).

One way to fix this while keeping moment-timezone and avoiding string concatenation is:

-const buildDateRange = (
-  startDate: Date,
-  endDate: Date,
-  timezone?: string
-): { gte: number, lt: number } => {
-  let start: number
-  let end: number
-
-  if (timezone !== undefined && timezone !== null && timezone !== '') {
-    start = moment.tz(startDate, timezone).unix()
-    end = moment.tz(endDate, timezone).endOf('day').unix()
-  } else {
-    start = moment.utc(startDate).unix()
-    end = moment.utc(endDate).endOf('day').unix()
-  }
-
-  return {
-    gte: Math.floor(start),
-    lt: Math.floor(end) + 1
-  }
-}
+const buildDateRange = (
+  startDate: { year: number, month: number, day: number },
+  endDate: { year: number, month: number, day: number },
+  timezone?: string
+): { gte: number, lt: number } => {
+  const { year: sYear, month: sMonth, day: sDay } = startDate
+  const { year: eYear, month: eMonth, day: eDay } = endDate
+
+  const startMoment = (timezone !== undefined && timezone !== null && timezone !== '')
+    ? moment.tz([sYear, sMonth, sDay], timezone).startOf('day')
+    : moment.utc([sYear, sMonth, sDay]).startOf('day')
+
+  const endMoment = (timezone !== undefined && timezone !== null && timezone !== '')
+    ? moment.tz([eYear, eMonth, eDay], timezone).endOf('day')
+    : moment.utc([eYear, eMonth, eDay]).endOf('day')
+
+  const start = startMoment.unix()
+  const end = endMoment.unix()
+
+  return {
+    gte: start,
+    lt: end + 1 // exclusive upper bound, inclusive end-of-day
+  }
+}

Then adapt getDateRangeFilter and getYearFilters to pass structured components instead of Date objects:

-const getDateRangeFilter = (
-  startDate: Date,
-  endDate: Date,
-  timezone?: string
-): Prisma.TransactionWhereInput => ({
-  timestamp: buildDateRange(startDate, endDate, timezone)
-})
+const getDateRangeFilter = (
+  startDate: string,
+  endDate: string,
+  timezone?: string
+): Prisma.TransactionWhereInput => {
+  const [sYear, sMonth, sDay] = startDate.split('-').map(Number)
+  const [eYear, eMonth, eDay] = endDate.split('-').map(Number)
+
+  return {
+    timestamp: buildDateRange(
+      { year: sYear, month: sMonth - 1, day: sDay },
+      { year: eYear, month: eMonth - 1, day: eDay },
+      timezone
+    )
+  }
+}
-const getYearFilters = (
-  years: string[],
-  timezone?: string
-): Prisma.TransactionWhereInput[] => {
-  return years.map((year) => {
-    const startDate = new Date(Number(year), 0, 1, 0, 0, 0) // Jan 1, 00:00:00
-    const endDate = new Date(Number(year), 11, 31, 23, 59, 59) // Dec 31, 23:59:59
-    return {
-      timestamp: buildDateRange(startDate, endDate, timezone)
-    }
-  })
-}
+const getYearFilters = (
+  years: string[],
+  timezone?: string
+): Prisma.TransactionWhereInput[] => {
+  return years.map((year) => {
+    const y = Number(year)
+    return {
+      timestamp: buildDateRange(
+        { year: y, month: 0, day: 1 },     // Jan 1
+        { year: y, month: 11, day: 31 },   // Dec 31
+        timezone
+      )
+    }
+  })
+}

And update call sites (see comments on lines 803–805, 914–916, 967–968) to pass the raw startDate/endDate strings into getDateRangeFilter instead of new Date(...).

This ensures:

  • The start of the range is midnight on startDate in the user’s timezone.
  • The end of the range is inclusive through the end of endDate in that timezone, implemented via the half-open [start, endExclusive) convention.
  • Year filters align with the user’s calendar year regardless of server timezone.

To verify, I’d recommend adding tests that:

  • Set timezone = 'America/New_York' (or another non-UTC zone),
  • Request a range like 2024-01-15 to 2024-01-15,
  • Assert that the generated timestamp range exactly matches midnight-to-midnight for that local day, and that transactions just outside that window are excluded.
🧹 Nitpick comments (1)
services/transactionService.ts (1)

941-948: getFilteredTransactionCount correctly mirrors the payments filters

The count helper now accepts the same timezone/startDate/endDate trio and applies the same branching logic as the list endpoints, ensuring the count aligns with the filtered result set (including date ranges and year fallbacks). That alignment is important for paginated UIs.

After correcting getDateRangeFilter/buildDateRange, please also adjust this call site to pass raw strings instead of new Date(...) in the same way as fetchAllPaymentsByUserId.

Also applies to: 967-971

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a7f91c2 and cde6623.

📒 Files selected for processing (3)
  • pages/api/payments/download/index.ts (1 hunks)
  • pages/api/payments/index.ts (2 hunks)
  • services/transactionService.ts (8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • pages/api/payments/download/index.ts
🧰 Additional context used
🧬 Code graph analysis (1)
services/transactionService.ts (1)
components/Transaction/PaybuttonTransactions.tsx (1)
  • moment (40-124)
⏰ 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: Run Tests
🔇 Additional comments (4)
pages/api/payments/index.ts (1)

22-29: Query parsing and service call wiring for date range look good

The startDate/endDate query parsing mirrors existing patterns (years/buttonIds) and the updated fetchAllPaymentsByUserIdWithPagination call passes arguments in the correct order, including the user timezone. No issues from this layer; any date validation/normalization is correctly delegated to the service.

Also applies to: 34-44

services/transactionService.ts (3)

12-12: moment-timezone import is appropriate but centralize all date math through the helpers

Using moment-timezone is a good choice given the need for timezone-aware ranges; just ensure all date filtering (including future additions) goes through the buildDateRange/getDateRangeFilter helpers so there’s a single, consistent source of truth for timezone behavior.


753-764: Date-range vs year fallback control flow is correct

The new parameters to fetchAllPaymentsByUserIdWithPagination and the where construction behave as intended:

  • Always filter by userProfiles.some({ userId }) and amount > 0.
  • When both startDate and endDate are non-empty, apply getDateRangeFilter(...).
  • Otherwise, fall back to year-based filters via getYearFilters(...).

This matches the API contract described in the PR and keeps the year filter behavior as a fallback.

Also applies to: 796-807


881-889: Let me get more specific information about the function signatures and all occurrences:
<function_calls>

rg "const buildDateRange|function buildDateRange" -A 20 services/transactionService.ts

</function_calls>

<function_calls>

rg "const getDateRangeFilter|function getDateRangeFilter" -A 10 services/transactionService.ts

</function_calls>

<function_calls>

rg "getDateRangeFilter(new Date" services/transactionService.ts -n

</function_calls>

Now let me check the specific line numbers mentioned in the review:
<function_calls>

sed -n '875,920p' services/transactionService.ts

</function_calls>

<function_calls>

sed -n '870,930p' services/transactionService.ts | cat -n

</function_calls>

Let me get context on all the functions involved:
<function_calls>

rg "export async function.*PaymentsByUserId|export async function.*TxCount|export async function.*PaymentCount" -n services/transactionService.ts

</function_calls>

<function_calls>

rg -n "if (startDate !== undefined && endDate !== undefined" services/transactionService.ts

</function_calls>

Perfect! Now let me check if there are any type definitions or overloads:
<function_calls>

rg "buildDateRange|getDateRangeFilter" -B 2 -A 2 services/transactionService.ts | head -100

</function_calls>

Let me check the broader context around those functions to see the complete picture:
<function_calls>

rg -n "^const buildDateRange|^const getDateRangeFilter|^const getYearFilters|^export async function" services/transactionService.ts | head -30

</function_calls>

<function_calls>

wc -l services/transactionService.ts

</function_calls>

Let me read the file to understand the structure better, focusing on the helper functions:
<function_calls>

sed -n '1,100p' services/transactionService.ts

</function_calls>

Let me check if there are any imports or if buildDateRange and getDateRangeFilter could be modified elsewhere:
<function_calls>

grep -n "import.*buildDateRange|import.*getDateRangeFilter|export.*buildDateRange|export.*getDateRangeFilter" services/transactionService.ts

</function_calls>

Now let me check all the places where the date filtering happens to validate the "Also applies to: 914-918" statement:
<function_calls>

sed -n '900,950p' services/transactionService.ts | cat -n

</function_calls>

Let me get the exact function that lines 914-918 belong to:
<function_calls>

sed -n '880,960p' services/transactionService.ts | cat -n | grep -A 50 "fetchAllPaymentsByUserId"

</function_calls>

<function_calls>

sed -n '880,1050p' services/transactionService.ts

</function_calls>

Let me check the git history to see if there are related changes:
<function_calls>

git log --oneline -20 -- services/transactionService.ts

</function_calls>

<function_calls>

git log -p --follow -S "buildDateRange" -- services/transactionService.ts | head -150

</function_calls>

Let me check the current PR or branch information:
<function_calls>

git log -1 --format="%H %s" && git diff HEAD~1 services/transactionService.ts | head -200

</function_calls>

<function_calls>

git show HEAD:services/transactionService.ts | sed -n '800,950p'

</function_calls>

Let me check the complete buildDateRange and getDateRangeFilter implementations:
<function_calls>

cat services/transactionService.ts | sed -n '/^const buildDateRange/,/^}/p'

</function_calls>

<function_calls>

cat services/transactionService.ts | sed -n '/^const getDateRangeFilter/,/^}/p'

</function_calls>

Now let me verify all locations where the refactoring would apply:
<function_calls>

grep -n "getDateRangeFilter(new Date" services/transactionService.ts

</function_calls>

Perfect! Let me now verify by checking the actual line numbers in context:
<function_calls>

sed -n '888,892p' services/transactionService.ts && echo "---" && sed -n '913,917p' services/transactionService.ts && echo "---" && sed -n '938,942p' services/transactionService.ts

</function_calls>

Excellent! Now I have all the information needed. Let me verify one more thing - whether there's any mention of changes to these functions in related commits or comments:
<function_calls>

git log --all --grep="buildDateRange|getDateRangeFilter|date.*filter|date.*range" --oneline 2>/dev/null | head -10

</function_calls>

<function_calls>

git show --name-status HEAD | head -30

</function_calls>

Let me check if there's a buildDateRange update mentioned anywhere or if these functions are designed to accept strings:
<function_calls>

python3 << 'EOF'
import re

Read the file

with open('services/transactionService.ts', 'r') as f:
content = f.read()

Find buildDateRange function

build_match = re.search(r'const buildDateRange\s*=\s*(\sstartDate:\s([^,]+),\sendDate:\s([^,]+)', content)
if build_match:
print(f"buildDateRange startDate type: {build_match.group(1)}")
print(f"buildDateRange endDate type: {build_match.group(2)}")

Find getDateRangeFilter function

filter_match = re.search(r'const getDateRangeFilter\s*=\s*(\sstartDate:\s([^,]+),\sendDate:\s([^,]+)', content)
if filter_match:
print(f"getDateRangeFilter startDate type: {filter_match.group(1)}")
print(f"getDateRangeFilter endDate type: {filter_match.group(2)}")

Count occurrences

count = len(re.findall(r'getDateRangeFilter(new Date', content))
print(f"\nTotal occurrences of getDateRangeFilter(new Date: {count}")

Find line numbers

for match in re.finditer(r'getDateRangeFilter(new Date', content):
line_num = content[:match.start()].count('\n') + 1
print(f" Line {line_num}")
EOF


</function_calls>

Perfect! Now I have comprehensive information. Let me create the final verification:

Service-level date range wiring is consistent but suggestion is incomplete

The review correctly identifies the pattern at lines 881-889 (fetchAllPaymentsByUserId) and 914-918 (fetchTxCountByPaybuttonId), and the suggestion to pass raw strings to getDateRangeFilter is sound if the helper functions are updated to accept strings. However:

  1. Currently, getDateRangeFilter and buildDateRange only accept Date objects, not strings - so this refactoring cannot be applied without first modifying those helper functions to accept string | Date parameters.

  2. A third identical occurrence exists at line 940 in fetchPaymentCountByUser that is not mentioned in the review ("Also applies to: 914-918"):

    Object.assign(where, getDateRangeFilter(new Date(startDate), new Date(endDate), timezone))

The review's conditional phrasing ("Once getDateRangeFilter is updated per the buildDateRange fix") suggests this is awaiting prerequisite changes. Confirm whether the helper functions have been or will be modified to accept strings before applying this refactoring, and ensure all three locations are addressed consistently.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
services/transactionService.ts (2)

963-968: Consider using UTC for year filter date construction.

The new Date(year, month, day, ...) constructor interprets arguments in the server's local timezone, which may vary between environments (UTC in production, local timezone in development). This could cause subtle inconsistencies.

For consistent behavior regardless of server timezone:

 const getYearFilters = (
   years: string[],
   timezone?: string
 ): Prisma.TransactionWhereInput[] => {
   return years.map((year) => {
-    const startDate = new Date(Number(year), 0, 1, 0, 0, 0) // Jan 1, 00:00:00
-    const endDate = new Date(Number(year), 11, 31, 23, 59, 59) // Dec 31, 23:59:59
+    const startDateStr = `${year}-01-01`
+    const endDateStr = `${year}-12-31`
     return {
-      timestamp: buildDateRange(startDate, endDate, timezone)
+      timestamp: buildDateRange(startDateStr, endDateStr, timezone)
     }
   })
 }

This would also require updating buildDateRange to accept strings (see earlier comment).


894-898: Consider extracting date filter application to reduce duplication.

The date filtering logic is duplicated across three functions with the same pattern:

if (startDate && endDate && startDate !== '' && endDate !== '') {
  Object.assign(where, getDateRangeFilter(...))
} else if (years?.length > 0) {
  where.OR = getYearFilters(...)
}

Could extract to a helper:

const applyDateFilters = (
  where: Prisma.TransactionWhereInput,
  startDate?: string,
  endDate?: string,
  years?: string[],
  timezone?: string
): void => {
  if (startDate && endDate && startDate !== '' && endDate !== '') {
    Object.assign(where, getDateRangeFilter(new Date(startDate), new Date(endDate), timezone))
  } else if (years?.length) {
    where.OR = getYearFilters(years, timezone)
  }
}

Also applies to: 1005-1009, 1058-1062

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between cde6623 and 4a7e16c.

📒 Files selected for processing (2)
  • pages/api/payments/download/index.ts (1 hunks)
  • services/transactionService.ts (8 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
pages/api/payments/download/index.ts (3)
pages/api/payments/index.ts (1)
  • req (5-48)
pages/api/payments/count/index.ts (1)
  • req (6-43)
services/transactionService.ts (1)
  • fetchAllPaymentsByUserId (972-1018)
services/transactionService.ts (1)
components/Transaction/PaybuttonTransactions.tsx (1)
  • moment (40-124)
⏰ 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: Run Tests
🔇 Additional comments (7)
pages/api/payments/download/index.ts (1)

54-63: LGTM! Date range filter correctly integrated with timezone support.

The startDate and endDate parsing follows the same pattern used in other payment endpoints, and the fetchAllPaymentsByUserId call now correctly includes all parameters including timezone — addressing the previously flagged critical issue.

services/transactionService.ts (6)

12-12: LGTM! Correct import for timezone-aware date handling.

Using moment-timezone instead of plain moment enables proper timezone interpretation as required for the date range filter feature.


848-854: Good: timezone is now mandatory.

Making timezone a required parameter prevents accidental omissions that could cause incorrect filtering. The optional startDate and endDate parameters align with the existing filter pattern.


929-949: Verify timezone interpretation when dates are passed as Date objects.

When new Date("2024-01-15") is passed to moment.tz(date, timezone), moment uses the Date's internal timestamp (midnight UTC) rather than re-interpreting the date string in the target timezone. This differs from passing a string directly to moment.tz("2024-01-15", timezone), which interprets the date as midnight in that timezone.

If users expect filtering by calendar dates in their timezone (e.g., NYC user selecting Jan 15 wants 2024-01-15T05:00:00Z to 2024-01-16T05:00:00Z), the current approach may be off by the timezone offset.

Please verify the frontend sends dates in a format that accounts for this behavior, or consider passing the date strings directly to moment-timezone before conversion:

 const buildDateRange = (
-  startDate: Date,
-  endDate: Date,
+  startDate: string,
+  endDate: string,
   timezone?: string
 ): { gte: number, lt: number } => {
   let start: number
   let end: number

   if (timezone !== undefined && timezone !== null && timezone !== '') {
-    start = moment.tz(startDate, timezone).unix()
-    end = moment.tz(endDate, timezone).endOf('day').unix()
+    start = moment.tz(startDate, timezone).startOf('day').unix()
+    end = moment.tz(endDate, timezone).endOf('day').unix()
   } else {
-    start = moment.utc(startDate).unix()
-    end = moment.utc(endDate).endOf('day').unix()
+    start = moment.utc(startDate).startOf('day').unix()
+    end = moment.utc(endDate).endOf('day').unix()
   }
   // ...
 }

Then update callers to pass strings instead of Date objects.


951-957: LGTM! Clean filter factory.

The getDateRangeFilter function provides a clean abstraction over buildDateRange for generating Prisma-compatible where clauses.


972-1009: LGTM! Consistent date filtering implementation.

The fetchAllPaymentsByUserId function now supports date range filtering with the same pattern used in fetchAllPaymentsByUserIdWithPagination. The date range filter takes precedence over year filters, which is the expected behavior.


1032-1064: LGTM! Count function updated consistently.

The getFilteredTransactionCount function applies the same date filtering logic as the data-fetching functions, ensuring counts match the filtered results.

@Klakurka Klakurka requested a review from chedieck November 26, 2025 18:54
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

♻️ Duplicate comments (5)
services/transactionService.ts (5)

894-898: Critical: Premature Date conversion breaks timezone handling.

Line 895 converts startDate and endDate strings to Date objects before passing to getDateRangeFilter. This causes new Date("2024-01-15") to interpret the date as midnight UTC, not midnight in the user's timezone. The subsequent moment.tz() call cannot correct this because it receives a Date object representing a UTC instant.

Pass the strings directly to the filter functions instead:

  if (startDate !== undefined && endDate !== undefined && startDate !== '' && endDate !== '') {
-   Object.assign(where, getDateRangeFilter(new Date(startDate), new Date(endDate), timezone))
+   Object.assign(where, getDateRangeFilter(startDate, endDate, timezone))
  } else if (years !== undefined && years.length > 0) {
    where.OR = getYearFilters(years, timezone)
  }

Then update getDateRangeFilter and buildDateRange to accept strings:

-const buildDateRange = (
-  startDate: Date,
-  endDate: Date,
-  timezone?: string
-): { gte: number, lte: number } => {
+const buildDateRange = (
+  startDate: string,
+  endDate: string,
+  timezone: string
+): { gte: number, lte: number } => {
   let start: number
   let end: number
   
-  if (timezone !== undefined && timezone !== null && timezone !== '') {
-    start = moment.tz(startDate, timezone).unix()
-    end = moment.tz(endDate, timezone).endOf('day').unix()
-  } else {
-    start = moment.utc(startDate).unix()
-    end = moment.utc(endDate).endOf('day').unix()
-  }
+  start = moment.tz(startDate, timezone).startOf('day').unix()
+  end = moment.tz(endDate, timezone).endOf('day').unix()
   
   return {
     gte: Math.floor(start),
     lte: Math.floor(end)
   }
 }

This correctly interprets "2024-01-15" as midnight in the user's timezone.


929-949: Critical: Date objects passed to moment.tz do not interpret dates in the target timezone.

The fundamental issue is that moment.tz(dateObject, timezone) where dateObject is a JavaScript Date does NOT interpret that date in the target timezone. Instead, it keeps the same UTC instant and merely changes the display timezone.

Example of the bug:

  1. User in NYC (UTC-5) selects "2024-01-15"
  2. Line 895: new Date("2024-01-15") creates 2024-01-15T00:00:00Z (midnight UTC)
  3. Line 938: moment.tz(dateObj, "America/New_York") converts that UTC instant to display in NYC timezone, yielding 2024-01-14T19:00:00-05:00 (7 PM previous day)
  4. .unix() returns the timestamp for Jan 14 at 7 PM NYC time, not Jan 15 at midnight NYC time

Impact:
Date range filters return incorrect results. Users filtering for "Jan 15" will get transactions from Jan 14 7PM to Jan 16 7PM (NYC example), missing 5 hours at the start and including 5 hours that shouldn't be there.

Based on past review comments, this was supposed to be addressed but the implementation still has the bug. The solution is to pass strings directly to moment.tz():

 const buildDateRange = (
-  startDate: Date,
-  endDate: Date,
-  timezone?: string
+  startDate: string,
+  endDate: string,
+  timezone: string
 ): { gte: number, lte: number } => {
-  let start: number
-  let end: number
-
-  if (timezone !== undefined && timezone !== null && timezone !== '') {
-    start = moment.tz(startDate, timezone).unix()
-    end = moment.tz(endDate, timezone).endOf('day').unix()
-  } else {
-    start = moment.utc(startDate).unix()
-    end = moment.utc(endDate).endOf('day').unix()
-  }
+  const start = moment.tz(startDate, timezone).startOf('day').unix()
+  const end = moment.tz(endDate, timezone).endOf('day').unix()
   
   return {
     gte: Math.floor(start),
     lte: Math.floor(end)
   }
 }

And update the signature of getDateRangeFilter:

 const getDateRangeFilter = (
-  startDate: Date,
-  endDate: Date,
-  timezone?: string
+  startDate: string,
+  endDate: string,
+  timezone: string
 ): Prisma.TransactionWhereInput => ({
   timestamp: buildDateRange(startDate, endDate, timezone)
 })

Based on learnings from past review comments.


959-970: Critical: Year filter construction doesn't account for user timezone.

The Date constructor new Date(Number(year), 0, 1, 0, 0, 0) creates a Date in the server's local timezone (UTC), which then gets passed to buildDateRange. Combined with the bug in buildDateRange, this produces incorrect timestamp ranges for users in non-UTC timezones.

Once buildDateRange is fixed to accept strings, update getYearFilters to construct proper date strings:

 const getYearFilters = (
   years: string[],
-  timezone?: string
+  timezone: string
 ): Prisma.TransactionWhereInput[] => {
   return years.map((year) => {
-    const startDate = new Date(Number(year), 0, 1, 0, 0, 0) // Jan 1, 00:00:00
-    const endDate = new Date(Number(year), 11, 31, 23, 59, 59) // Dec 31, 23:59:59
+    const startDate = `${year}-01-01`
+    const endDate = `${year}-12-31`
     return {
       timestamp: buildDateRange(startDate, endDate, timezone)
     }
   })
 }

This passes ISO date strings to buildDateRange, which will correctly interpret them in the user's timezone using moment.tz().


1005-1009: Critical: Same premature Date conversion issue.

This has the identical bug as lines 894-898: converting date strings to Date objects before passing to getDateRangeFilter breaks timezone interpretation.

Apply the same fix:

  if (startDate !== undefined && endDate !== undefined && startDate !== '' && endDate !== '') {
-   Object.assign(where, getDateRangeFilter(new Date(startDate), new Date(endDate), timezone))
+   Object.assign(where, getDateRangeFilter(startDate, endDate, timezone))
  } else if (years !== undefined && years.length > 0) {
    where.OR = getYearFilters(years, timezone)
  }

1058-1062: Critical: Same premature Date conversion issue (third instance).

This is the third location with the identical bug. Apply the same fix here:

  if (startDate !== undefined && endDate !== undefined && startDate !== '' && endDate !== '') {
-   Object.assign(where, getDateRangeFilter(new Date(startDate), new Date(endDate), timezone))
+   Object.assign(where, getDateRangeFilter(startDate, endDate, timezone))
  } else if (years !== undefined && years.length > 0) {
    where.OR = getYearFilters(years, timezone)
  }
🧹 Nitpick comments (1)
services/transactionService.ts (1)

844-855: Consider making timezone a mandatory parameter.

Based on chedieck's previous feedback: "I'd make timezone a mandatory parameter, we're always passing it... It will help avoid forgetting it and making mistakes in the future." Since the API layer always provides timezone (see pages/api/payments/index.ts:42), making it required would prevent accidental bugs from missing timezone handling.

Apply this change to make timezone required:

 export async function fetchAllPaymentsByUserIdWithPagination (
   userId: string,
   page: number,
   pageSize: number,
-  timezone: string,
+  timezone: string,  // Already required in practice
   orderBy?: string,
   orderDesc = true,
   buttonIds?: string[],
   years?: string[],
   startDate?: string,
   endDate?: string
 ): Promise<Payment[]> {

Then update other similar functions (lines 972-980 and 1032-1039) to also require timezone instead of making it optional.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4a7e16c and f7a40d6.

📒 Files selected for processing (1)
  • services/transactionService.ts (8 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
services/transactionService.ts (1)
components/Transaction/PaybuttonTransactions.tsx (1)
  • moment (40-124)
⏰ 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: Run Tests
🔇 Additional comments (1)
services/transactionService.ts (1)

12-12: LGTM: Import updated to support timezone operations.

The import change from moment to moment-timezone is correct and enables the timezone-aware date filtering functionality.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

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

⚠️ Outside diff range comments (1)
services/transactionService.ts (1)

844-855: timezone parameter can receive undefined despite strict mode; year-end date math uses local timezone, risking off-by-one errors

The type mismatch is real: fetchAllPaymentsByUserIdWithPagination requires timezone: string, but the API layer assigns it conditionally from potentially undefined sources (req.headers.timezone and user?.preferredTimezone). Although tsconfig.json has "strict": true, the mismatch appears to slip through because the API handler is typed as req: any.

Recommend:

  • Either make timezone optional in fetchAllPaymentsByUserIdWithPagination to match fetchAllPaymentsByUserId and getFilteredTransactionCount, or
  • Ensure the API layer always provides a concrete default (e.g., 'UTC') before calling the function.

Additionally, getYearFilters constructs dates using the host local timezone (new Date(year, 11, 31, 23, 59, 59)) then passes them to buildDateRange, which reads UTC components. If the server timezone differs from UTC, the UTC date for Dec 31 23:59:59 local time may spill into the following year, causing the filter to include transactions from Jan 1 of the next year. Use Date.UTC() in getYearFilters to make date construction timezone-invariant.

Also applies to: 887–898, 1050

🧹 Nitpick comments (1)
services/transactionService.ts (1)

990-997: Repeated date-range/year-filter block could be centralized

fetchAllPaymentsByUserId and getFilteredTransactionCount repeat the same conditional:

if (startDate && endDate && startDate !== '' && endDate !== '') {
  Object.assign(where, getDateRangeFilter(new Date(startDate), new Date(endDate), timezone))
} else if (years && years.length > 0) {
  where.OR = getYearFilters(years, timezone)
}

This is functionally fine and consistent with the paginated version, but the duplication across three functions makes future tweaks (e.g., changing how you interpret single-ended ranges) easy to miss in one place.

Consider extracting this into a small helper, e.g.:

function applyDateRangeOrYearFilters(
  where: Prisma.TransactionWhereInput,
  params: { startDate?: string; endDate?: string; years?: string[]; timezone?: string }
) {
  const { startDate, endDate, years, timezone } = params
  if (startDate && endDate) {
    Object.assign(where, getDateRangeFilter(new Date(startDate), new Date(endDate), timezone))
  } else if (years && years.length > 0) {
    where.OR = getYearFilters(years, timezone)
  }
}

and calling it from the three sites. Not urgent, but it will make future maintenance less error-prone.

Also applies to: 1023-1027, 1050-1057, 1076-1080

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f7a40d6 and 5d973f7.

📒 Files selected for processing (3)
  • pages/api/payments/index.ts (2 hunks)
  • pages/payments/index.tsx (11 hunks)
  • services/transactionService.ts (8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • pages/payments/index.tsx
🧰 Additional context used
🧬 Code graph analysis (2)
pages/api/payments/index.ts (3)
services/userService.ts (1)
  • fetchUserProfileFromId (9-13)
pages/api/payments/download/index.ts (1)
  • req (13-80)
services/transactionService.ts (1)
  • fetchAllPaymentsByUserIdWithPagination (844-927)
services/transactionService.ts (1)
components/Transaction/PaybuttonTransactions.tsx (1)
  • moment (40-124)
⏰ 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: Run Tests

if (req.method === 'GET') {
await setSession(req, res)
const userId = req.session.userId
const user = await fetchUserProfileFromId(userId)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Timezone fallback bug and unhandled missing-user case

Two related concerns here:

  1. userPreferredTimezone fallback is wrong for undefined/null:

    • When user?.preferredTimezone is undefined or null, the check userPreferredTimezone !== '' is still true, so timezone becomes undefined/null instead of falling back to userReqTimezone.
    • This can lead to timezone being invalid while downstream code expects a usable string (or at least a well-defined default) and uses it for date-range filtering.

    Consider tightening the condition and adding a final default (e.g. 'UTC'):

  • const userPreferredTimezone = user?.preferredTimezone
  • const timezone = userPreferredTimezone !== '' ? userPreferredTimezone : userReqTimezone
  • const userPreferredTimezone = user?.preferredTimezone
  • const timezone =
  •  (userPreferredTimezone && userPreferredTimezone.trim() !== '')
    
  •    ? userPreferredTimezone
    
  •    : (userReqTimezone || 'UTC')
    
    
    
  1. fetchUserProfileFromId(userId) throws when the profile is missing. If there are edge cases where a session exists without a user profile, this will 500 the request. If that’s possible, consider handling the error (e.g. 404 or 401) or relaxing the dependency on the profile when you only need a timezone.

Also applies to: 31-33

🤖 Prompt for AI Agents
In pages/api/payments/index.ts around line 9 (and similarly lines 31-33),
fetchUserProfileFromId(userId) can throw when no profile exists and
user?.preferredTimezone can be null/undefined but the current check allows those
through; update to catch errors from fetchUserProfileFromId and handle
missing-profile cases (return 401/404 or proceed without profile depending on
intended flow), and change the timezone fallback logic to use a strict presence
check (e.g. user?.preferredTimezone && user.preferredTimezone.trim() !== ''),
then compute timezone = userPreferredTimezone ?? userReqTimezone ?? 'UTC' so
null/undefined/empty strings fall back to the request timezone and finally to
'UTC'.

@chedieck chedieck force-pushed the feat/date-range-filter branch from f4a845d to 2114ffd Compare November 27, 2025 00:17
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (3)
tests/unittests/transactionService.test.ts (2)

209-235: Add explicit return types to helper functions to satisfy ESLint

ESLint is flagging computeExpectedRange and computeYearFilter for missing return types. Adding explicit types will quiet the rule and make intent clearer in these test helpers.

You could do:

-  const computeExpectedRange = (tz: string) => {
+  const computeExpectedRange = (tz: string): { gte: number; lte: number } => {
@@
-  const computeYearFilter = (year: number, tz: string) => {
+  const computeYearFilter = (
+    year: number,
+    tz: string
+  ): { timestamp: { gte: number; lte: number } } => {

Also applies to: 237-265


237-265: Use Date.UTC in computeYearFilter to avoid host‑timezone dependence

computeYearFilter currently builds startDateObj/endDateObj with the local-time Date constructor. That can shift UTC calendar dates depending on the machine timezone, which may make the expected filter diverge from the service’s getYearFilters (which now uses Date.UTC).

Aligning with the implementation and making the tests deterministic across environments:

-  const startDateObj = new Date(year, 0, 1, 0, 0, 0)
-  const endDateObj = new Date(year, 11, 31, 23, 59, 59)
+  const startDateObj = new Date(Date.UTC(year, 0, 1, 0, 0, 0))
+  const endDateObj = new Date(Date.UTC(year, 11, 31, 23, 59, 59))

This should keep the expected timestamp range identical to what getYearFilters produces regardless of the test runner’s timezone.

services/transactionService.ts (1)

929-967: buildDateRange timezone handling and boundaries are sound (minor cleanup possible)

The helper correctly:

  • Uses moment.tz with UTC-derived Y/M/D components when a timezone is provided, giving local‑day [gte, lte] boundaries.
  • Falls back to UTC start/endOf('day') when no timezone is supplied.
  • Returns integer‑second bounds consistent with how timestamps are stored and asserted in tests.

The extra Math.round on startMoment.unix() / endMoment.unix() is redundant since .unix() already returns an integer; you could drop it for clarity, but it’s harmless as‑is.

-  return {
-    gte: Math.round(start),
-    lte: Math.round(end)
-  }
+  return {
+    gte: start,
+    lte: end
+  }
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5d973f7 and 3748fdf.

📒 Files selected for processing (3)
  • pages/api/payments/index.ts (2 hunks)
  • services/transactionService.ts (8 hunks)
  • tests/unittests/transactionService.test.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • pages/api/payments/index.ts
🧰 Additional context used
🧬 Code graph analysis (2)
tests/unittests/transactionService.test.ts (1)
prisma-local/mockedClient.ts (1)
  • prismaMock (19-19)
services/transactionService.ts (1)
components/Transaction/PaybuttonTransactions.tsx (1)
  • moment (40-124)
🪛 ESLint
tests/unittests/transactionService.test.ts

[error] 209-209: Missing return type on function.

(@typescript-eslint/explicit-function-return-type)


[error] 237-237: Missing return type on function.

(@typescript-eslint/explicit-function-return-type)

⏰ 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: Run Tests
🔇 Additional comments (4)
tests/unittests/transactionService.test.ts (1)

199-425: Good coverage of timezone-aware date and year filtering behavior

The new suite exercises:

  • Local-day boundaries across multiple timezones for both pagination and count.
  • Precedence of date range over years.
  • Pure-year filters and the “no dates & no years” fallback.

This lines up cleanly with the service behavior and should protect against regressions in the date/timezone logic.

services/transactionService.ts (3)

844-898: Date range vs. year filter precedence and shapes look correct

The updated fetchAllPaymentsByUserIdWithPagination where-clause:

  • Applies getDateRangeFilter when startDate/endDate are truthy strings, otherwise falls back to getYearFilters.
  • Ensures timestamp is set only for direct date ranges, and OR holds year filters, matching how the tests inspect where.timestamp vs where.OR.
  • Keeps the amount/user/profile constraints untouched.

This matches the new tests’ expectations for precedence and filter structure.


969-975: Helpers cleanly centralize date/year filters and fix prior TZ issues

getDateRangeFilter and getYearFilters now:

  • Encapsulate the timestamp shape so the three call sites (fetchAllPaymentsByUserIdWithPagination, fetchAllPaymentsByUserId, getFilteredTransactionCount) don’t duplicate that structure.
  • Use UTC-based year boundaries via Date.UTC before delegating to buildDateRange, which avoids server‑timezone bleed that earlier versions had and aligns with how the tests derive expected year filters (once updated).

This centralization plus UTC construction should make future adjustments to boundary semantics much safer.

Also applies to: 977-988


991-999: Consistent filter application across list, pagination, and count paths

The same (startDate,endDate) → date range else years → OR year filters pattern is now used in:

  • fetchAllPaymentsByUserId
  • fetchAllPaymentsByUserIdWithPagination
  • getFilteredTransactionCount

This consistency ensures the UI sees matching results between paginated lists, CSV export backing queries, and aggregate counts for identical filter inputs, which is exactly what the new tests are asserting.

Also applies to: 1024-1027, 1051-1057, 1077-1081

@Klakurka Klakurka merged commit e72861c into master Nov 27, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants