fix: tx prices linking, dashboard load, initial sync errors#1132
Open
chedieck wants to merge 30 commits into
Open
fix: tx prices linking, dashboard load, initial sync errors#1132chedieck wants to merge 30 commits into
chedieck wants to merge 30 commits into
Conversation
…onnection exhaustion)
- executeCall coalesces concurrent calls (waits) instead of throwing - cacheAddressesInBackground deduplicates per user (skip if already running) - Dashboard not cached when partial (background rebuild active) - Dashboard API returns cacheRebuilding flag; frontend shows banner + auto-refreshes - Payments count falls back to DB count during cache rebuild
- payments/count: always use direct DB count instead of dashboard cache path - cache rebuild: use cursor-based pagination instead of offset (O(1) vs O(n)) - cache rebuild: exclude inputs JOIN (unused by cache) - dashboard: cache partial data with 30s TTL during rebuild - log dedup: only log 'starting background rebuild' when actually starting
Root causes: - initPaymentCache triggered full cache build (200k+ tx scan) on every websocket message for uncached addresses, saturating MariaDB I/O - payments/wallets queries used EXISTS subquery pattern, forcing MySQL to scan all rows before sorting - generateAddressPaymentInfo loaded ALL transactions into memory just to sum amounts (replaced with SQL aggregate) - background cache rebuild shared connection pool with user queries Fixes: - SKIP_CACHE_REBUILD env var to disable cache rebuild in dev-from-dump - Separate backgroundPrisma client with 3-connection pool for cache rebuild - Replace EXISTS subquery with addressId IN (...) for payments/count queries - Replace full-table-load with prisma.transaction.aggregate for balance - 200ms delay between background cache batches to reduce I/O pressure - Reduce cache rebuild page size from 5000 to 1000 Results (dev-from-dump, 223k payments user): - /api/payments: 120s -> 4s - /api/payments/count: hanging -> 89ms - /api/wallets: hanging -> 23ms
The separate 3-connection pool caused timeouts for txCreation calls from websocket handlers (initPaymentCache → addressCreation path). The real fix for dev-from-dump I/O saturation is SKIP_CACHE_REBUILD, not a separate pool.
Contributor
📝 WalkthroughWalkthroughAdds background cache rebuilding for payment/address data with a concurrency guard and UI polling; introduces Chronik WebSocket reconnection with exponential backoff and bounded confirmed-transaction concurrency; refactors ChangesBackground Cache Rebuild, WebSocket Resilience, and Query Optimization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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 (3)
tests/integration-tests/api.test.ts (1)
1348-1366:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid hard-coding a transient rebuild state in the dashboard integration test.
Line 1366 asserts
cacheRebuilding: true, but this test never sets up an active background rebuild. Since the flag reflects runtime state, this makes the suite environment-dependent and it will fail whenever rebuilds are already complete or skipped. Assert the field shape here, or explicitly mock rebuild-in-progress state in a dedicated test.Proposed fix
expect(responseData).toEqual({ thirtyDays: expectedPeriodData, sevenDays: expectedPeriodData, year: expectedPeriodData, all: expectedPeriodData, total: { revenue: { usd: expect.any(String), cad: expect.any(String) }, payments: expect.any(Number), buttons: expect.any(Number) }, filtered: expect.any(Boolean), - cacheRebuilding: true + cacheRebuilding: expect.any(Boolean) } )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration-tests/api.test.ts` around lines 1348 - 1366, The test currently asserts a runtime-specific boolean by expecting cacheRebuilding: true in the 'Should return HTTP 200' case; change the assertion to validate the field shape instead (e.g., expect.any(Boolean)) or remove the explicit value check so the test does not depend on a transient rebuild state; locate the block in the it('Should return HTTP 200', ...) test where responseData is compared to the expected object and replace the cacheRebuilding: true entry with a shape assertion (using expect.any(Boolean)) or omit it and create a separate test that mocks an active rebuild if you need to assert true.redis/index.ts (1)
80-131:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInclude all result-shaping arguments in the dedup key.
executeCall()now coalesces only byuserId+methodName, butdashboardData()also varies bytimezoneandbuttonIds, andpaymentsCount()varies bytimezone. Concurrent requests for the same user with different filters will reuse the first in-flight promise and return the wrong payload to one caller.Proposed fix
- private static readonly pendingPromises = new Map<string, Map<MethodName, Promise<any>>>() + private static readonly pendingPromises = new Map<string, Promise<unknown>>() private static async executeCall<T>( - userId: string, - methodName: MethodName, + dedupeKey: string, fn: () => Promise<T> ): Promise<T> { - let userMap = this.pendingPromises.get(userId) - if (userMap === undefined) { - userMap = new Map() - this.pendingPromises.set(userId, userMap) - } - - const existing = userMap.get(methodName) + const existing = this.pendingPromises.get(dedupeKey) if (existing !== undefined) { return await existing as T } const promise = fn() - userMap.set(methodName, promise) + this.pendingPromises.set(dedupeKey, promise) try { return await promise } finally { - userMap.delete(methodName) - if (userMap.size === 0) { - this.pendingPromises.delete(userId) - } + this.pendingPromises.delete(dedupeKey) } } static async dashboardData (userId: string, timezone: string, buttonIds?: string[]): Promise<DashboardData> { - return await this.executeCall(userId, 'dashboardData', async () => { + const key = `dashboardData:${userId}:${timezone}:${[...(buttonIds ?? [])].sort().join(',')}` + return await this.executeCall(key, async () => { return await getUserDashboardData(userId, timezone, buttonIds) }) } static async paymentsCount (userId: string, timezone: string): Promise<number> { - return await this.executeCall(userId, 'paymentsCount', async () => { + return await this.executeCall(`paymentsCount:${userId}:${timezone}`, async () => { return await getCachedPaymentsCountForUser(userId, timezone) }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@redis/index.ts` around lines 80 - 131, The dedup key currently used by executeCall (userId + methodName) is missing result-shaping arguments, causing dashboardData and paymentsCount callers with different timezone/buttonIds to receive the wrong in-flight result; fix by changing executeCall to accept an explicit dedupKey (string) or an array of key parts and use that composite key in pendingPromises instead of just userId, then update callers dashboardData to include timezone and buttonIds in the dedup key and paymentsCount to include timezone (e.g., pass `${userId}|dashboardData|${timezone}|${buttonIds?.join(',')}` or equivalent via an array-of-params approach) so in-flight promises are coalesced only when all shaping args match.redis/dashboardCache.ts (1)
365-391:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSurface rebuild state on cached dashboard responses too.
Right now
cacheRebuildingis only attached on a cache miss. If a stale dashboard summary is already cached whencacheAddressesInBackground()starts, this path returns it unchanged, so the client never sees the rebuild banner or polling state until the rebuild finishes and clears the cache. Please checkisBackgroundRebuildActive(userId)before the early cached return and overlay the flag there as well.Suggested fix
export const getUserDashboardData = async function (userId: string, timezone: string, paybuttonIds?: string[]): Promise<DashboardData> { let dashboardData = await getCachedDashboardData(userId) + const rebuilding = isBackgroundRebuildActive(userId) if ((paybuttonIds !== undefined && paybuttonIds.length > 0) || dashboardData?.filtered === true) { dashboardData = null } + if (dashboardData !== null) { + return rebuilding ? { ...dashboardData, cacheRebuilding: true } : dashboardData + } - if (dashboardData === null) { - console.log('[CACHE]: Recreating dashboard for user', userId) - const nMonthsTotal = await getNumberOfMonths(userId) - const paymentStream = getPaymentStream(userId) + console.log('[CACHE]: Recreating dashboard for user', userId) + const nMonthsTotal = await getNumberOfMonths(userId) + const paymentStream = getPaymentStream(userId) - const dashboardData = await generateDashboardDataFromStream( - paymentStream, - nMonthsTotal, - { revenue: '`#66fe91`', payments: '`#669cfe`' }, - timezone, - paybuttonIds - ) + const rebuiltDashboardData = await generateDashboardDataFromStream( + paymentStream, + nMonthsTotal, + { revenue: '`#66fe91`', payments: '`#669cfe`' }, + timezone, + paybuttonIds + ) - const rebuilding = isBackgroundRebuildActive(userId) - if (rebuilding) { - dashboardData.cacheRebuilding = true - } else { - await cacheDashboardData(userId, dashboardData) - } - return dashboardData + if (rebuilding) { + rebuiltDashboardData.cacheRebuilding = true + } else { + await cacheDashboardData(userId, rebuiltDashboardData) } - return dashboardData + return rebuiltDashboardData }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@redis/dashboardCache.ts` around lines 365 - 391, The cached-return path should surface background rebuild state: after retrieving dashboardData via getCachedDashboardData(userId) and before returning it early, call isBackgroundRebuildActive(userId) and if true set dashboardData.cacheRebuilding = true so the client sees the rebuild banner; ensure you modify the existing dashboardData object (don't shadow the variable name) so the flag is preserved, and leave the existing cache-miss path (generateDashboardDataFromStream / cacheDashboardData) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pages/dashboard/index.tsx`:
- Around line 143-156: The current useEffect uses a single setTimeout based on
dashboardData?.cacheRebuilding and selectedButtonIds, but if the fetched
response still has cacheRebuilding: true the effect’s dependency value doesn’t
change and no further poll is scheduled; change to an explicit polling loop:
implement a local async function (e.g., pollDashboard) inside the effect that
fetches via the same URL logic (using selectedButtonIds and moment.tz.guess()),
calls setDashboardData(json), and if json.cacheRebuilding === true schedules
itself with setTimeout to run again (or immediately re-invoke after await) so
polling continues until cacheRebuilding becomes false; ensure you clear the
pending timeout on cleanup and reference the same symbols (useEffect,
selectedButtonIds, pollDashboard, setDashboardData,
dashboardData?.cacheRebuilding, timer) so the loop respects selection changes
and is cancelled properly.
In `@services/chronikService.ts`:
- Around line 204-220: The reconnect logic reopens Chronik during an intentional
teardown; add a wsShuttingDown boolean flag on ChronikBlockchainClient, set it
to true in a new shutdown() method (call shutdown() from
MultiBlockchainClient.destroy() before closing chronikWSEndpoint), and guard the
onEnd handler and reconnectWs() (which currently uses wsReconnecting and
reconnectWs) to return immediately if wsShuttingDown is true so the socket isn't
reopened during shutdown; apply the same guard for the other onEnd/reconnect
path around the code at the 705-714 area.
- Around line 186-202: connectWsWithRetry currently stops trying after
CHRONIK_WS_MAX_RETRIES and leaves chronikWSEndpoint offline since autoReconnect
is disabled; change it so that after the final failed attempt it schedules a
future reconnect (using setTimeout) to call this.connectWsWithRetry again with
exponential backoff rather than just logging and returning, and guard scheduling
with a flag/property (e.g., this.reconnectTimerId or this.isReconnectScheduled)
to avoid multiple concurrent timers; ensure the scheduled attempt still uses
chronikWSEndpoint.waitForOpen and honors baseDelay/backoff so outages recover
automatically without a process restart.
---
Outside diff comments:
In `@redis/dashboardCache.ts`:
- Around line 365-391: The cached-return path should surface background rebuild
state: after retrieving dashboardData via getCachedDashboardData(userId) and
before returning it early, call isBackgroundRebuildActive(userId) and if true
set dashboardData.cacheRebuilding = true so the client sees the rebuild banner;
ensure you modify the existing dashboardData object (don't shadow the variable
name) so the flag is preserved, and leave the existing cache-miss path
(generateDashboardDataFromStream / cacheDashboardData) unchanged.
In `@redis/index.ts`:
- Around line 80-131: The dedup key currently used by executeCall (userId +
methodName) is missing result-shaping arguments, causing dashboardData and
paymentsCount callers with different timezone/buttonIds to receive the wrong
in-flight result; fix by changing executeCall to accept an explicit dedupKey
(string) or an array of key parts and use that composite key in pendingPromises
instead of just userId, then update callers dashboardData to include timezone
and buttonIds in the dedup key and paymentsCount to include timezone (e.g., pass
`${userId}|dashboardData|${timezone}|${buttonIds?.join(',')}` or equivalent via
an array-of-params approach) so in-flight promises are coalesced only when all
shaping args match.
In `@tests/integration-tests/api.test.ts`:
- Around line 1348-1366: The test currently asserts a runtime-specific boolean
by expecting cacheRebuilding: true in the 'Should return HTTP 200' case; change
the assertion to validate the field shape instead (e.g., expect.any(Boolean)) or
remove the explicit value check so the test does not depend on a transient
rebuild state; locate the block in the it('Should return HTTP 200', ...) test
where responseData is compared to the expected object and replace the
cacheRebuilding: true entry with a shape assertion (using expect.any(Boolean))
or omit it and create a separate test that mocks an active rebuild if you need
to assert true.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0036b6ff-4960-41ad-b7a6-ab5c16477bed
📒 Files selected for processing (16)
.env.from-dump.gitignoreconstants/index.tspages/api/payments/count/index.tspages/dashboard/dashboard.module.csspages/dashboard/index.tsxredis/dashboardCache.tsredis/index.tsredis/paymentCache.tsredis/types.tsservices/addressService.tsservices/chronikService.tsservices/transactionService.tstests/integration-tests/api.test.tstests/unittests/addressService.test.tstests/unittests/transactionService.test.ts
Comment on lines
+143
to
+156
| useEffect(() => { | ||
| if (dashboardData?.cacheRebuilding !== true) return | ||
| const timer = setTimeout(() => { | ||
| let url = 'api/dashboard' | ||
| if (selectedButtonIds.length > 0) { | ||
| url += `?buttonIds=${selectedButtonIds.join(',')}` | ||
| } | ||
| fetch(url, { headers: { Timezone: moment.tz.guess() } }) | ||
| .then(async res => await res.json()) | ||
| .then(json => { setDashboardData(json) }) | ||
| .catch(console.error) | ||
| }, 15000) | ||
| return () => clearTimeout(timer) | ||
| }, [dashboardData?.cacheRebuilding, selectedButtonIds]) |
Contributor
There was a problem hiding this comment.
Polling stops after the first refresh.
This effect only depends on the boolean dashboardData?.cacheRebuilding. If the first refresh still returns cacheRebuilding: true, the dependencies do not change, so no second timer is scheduled. Long-running rebuilds will therefore update once and then stall.
Suggested fix
useEffect(() => {
if (dashboardData?.cacheRebuilding !== true) return
- const timer = setTimeout(() => {
- let url = 'api/dashboard'
- if (selectedButtonIds.length > 0) {
- url += `?buttonIds=${selectedButtonIds.join(',')}`
- }
- fetch(url, { headers: { Timezone: moment.tz.guess() } })
- .then(async res => await res.json())
- .then(json => { setDashboardData(json) })
- .catch(console.error)
- }, 15000)
- return () => clearTimeout(timer)
- }, [dashboardData?.cacheRebuilding, selectedButtonIds])
+ const interval = setInterval(() => {
+ let url = 'api/dashboard'
+ if (selectedButtonIds.length > 0) {
+ url += `?buttonIds=${selectedButtonIds.join(',')}`
+ }
+ fetch(url, { headers: { Timezone: moment.tz.guess() } })
+ .then(async res => await res.json())
+ .then(json => { setDashboardData(json) })
+ .catch(console.error)
+ }, 15000)
+ return () => clearInterval(interval)
+ }, [dashboardData, selectedButtonIds])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pages/dashboard/index.tsx` around lines 143 - 156, The current useEffect uses
a single setTimeout based on dashboardData?.cacheRebuilding and
selectedButtonIds, but if the fetched response still has cacheRebuilding: true
the effect’s dependency value doesn’t change and no further poll is scheduled;
change to an explicit polling loop: implement a local async function (e.g.,
pollDashboard) inside the effect that fetches via the same URL logic (using
selectedButtonIds and moment.tz.guess()), calls setDashboardData(json), and if
json.cacheRebuilding === true schedules itself with setTimeout to run again (or
immediately re-invoke after await) so polling continues until cacheRebuilding
becomes false; ensure you clear the pending timeout on cleanup and reference the
same symbols (useEffect, selectedButtonIds, pollDashboard, setDashboardData,
dashboardData?.cacheRebuilding, timer) so the loop respects selection changes
and is cancelled properly.
Comment on lines
+186
to
+202
| private async connectWsWithRetry (maxRetries = CHRONIK_WS_MAX_RETRIES, baseDelay = CHRONIK_WS_BASE_DELAY_MS): Promise<void> { | ||
| for (let attempt = 1; attempt <= maxRetries; attempt++) { | ||
| try { | ||
| await this.chronikWSEndpoint.waitForOpen() | ||
| console.log(`${this.CHRONIK_MSG_PREFIX}: WebSocket connected.`) | ||
| return | ||
| } catch (err: any) { | ||
| console.error(`${this.CHRONIK_MSG_PREFIX}: WebSocket connection attempt ${attempt}/${maxRetries} failed: ${err.message as string}`) | ||
| if (attempt < maxRetries) { | ||
| const delay = Math.min(baseDelay * Math.pow(2, attempt - 1), 60000) | ||
| console.log(`${this.CHRONIK_MSG_PREFIX}: Retrying WebSocket in ${delay / 1000}s...`) | ||
| await new Promise(resolve => setTimeout(resolve, delay)) | ||
| } | ||
| } | ||
| } | ||
| console.error(`${this.CHRONIK_MSG_PREFIX}: WebSocket failed after ${maxRetries} attempts. Continuing without real-time updates.`) | ||
| } |
Contributor
There was a problem hiding this comment.
Retry exhaustion leaves the client permanently offline.
When connectWsWithRetry() hits CHRONIK_WS_MAX_RETRIES, it only logs and returns. Since autoReconnect is disabled, nothing schedules another reconnect cycle after that point, so a long Chronik outage becomes a permanent loss of real-time updates until the process restarts.
Proposed fix
private async connectWsWithRetry (maxRetries = CHRONIK_WS_MAX_RETRIES, baseDelay = CHRONIK_WS_BASE_DELAY_MS): Promise<void> {
for (let attempt = 1; attempt <= maxRetries; attempt++) {
@@
}
- console.error(`${this.CHRONIK_MSG_PREFIX}: WebSocket failed after ${maxRetries} attempts. Continuing without real-time updates.`)
+ console.error(`${this.CHRONIK_MSG_PREFIX}: WebSocket failed after ${maxRetries} attempts. Scheduling another reconnect cycle.`)
+ if (!this.wsShuttingDown) {
+ await new Promise(resolve => setTimeout(resolve, 60000))
+ await this.reconnectWs()
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/chronikService.ts` around lines 186 - 202, connectWsWithRetry
currently stops trying after CHRONIK_WS_MAX_RETRIES and leaves chronikWSEndpoint
offline since autoReconnect is disabled; change it so that after the final
failed attempt it schedules a future reconnect (using setTimeout) to call
this.connectWsWithRetry again with exponential backoff rather than just logging
and returning, and guard scheduling with a flag/property (e.g.,
this.reconnectTimerId or this.isReconnectScheduled) to avoid multiple concurrent
timers; ensure the scheduled attempt still uses chronikWSEndpoint.waitForOpen
and honors baseDelay/backoff so outages recover automatically without a process
restart.
Comment on lines
+204
to
+220
| private async reconnectWs (): Promise<void> { | ||
| if (this.wsReconnecting) return | ||
| this.wsReconnecting = true | ||
| try { | ||
| const addresses = this.getSubscribedAddresses() | ||
| this.chronikWSEndpoint = this.chronik.ws(this.getWsConfig()) | ||
| this.chronikWSEndpoint.subscribeToBlocks() | ||
| for (const addr of addresses) { | ||
| this.chronikWSEndpoint.subscribeToAddress(addr) | ||
| } | ||
| await this.connectWsWithRetry() | ||
| } catch (err: any) { | ||
| console.error(`${this.CHRONIK_MSG_PREFIX}: WebSocket reconnection error: ${err.message as string}`) | ||
| } finally { | ||
| this.wsReconnecting = false | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Prevent reconnects during intentional shutdown.
destroy() closes chronikWSEndpoint at Lines 1460-1465, and this new onEnd handler immediately calls reconnectWs(). Without a shutdown flag, teardown now reopens the Chronik socket you are trying to close, which leaks background connections during process shutdown and test cleanup.
Proposed fix
export class ChronikBlockchainClient {
chronik!: ChronikClient
@@
private latencyTestFinished: boolean
private wsReconnecting = false
private confirmedTxsBeingProcessed = 0
+ private wsShuttingDown = false
@@
private async reconnectWs (): Promise<void> {
- if (this.wsReconnecting) return
+ if (this.wsReconnecting || this.wsShuttingDown) return
this.wsReconnecting = true
@@
onReconnect: (_: ws.Event) => {
+ if (this.wsShuttingDown) return
console.log(`${this.CHRONIK_MSG_PREFIX}: Chronik webSocket unexpectedly closed. Attempting reconnection...`)
void this.reconnectWs()
},
@@
onEnd: (e: ws.Event) => {
+ if (this.wsShuttingDown) return
console.log(`${this.CHRONIK_MSG_PREFIX}: Chronik WebSocket ended, type: ${e.type}. Attempting reconnection...`)
void this.reconnectWs()
},Add a small shutdown() method on ChronikBlockchainClient that sets wsShuttingDown = true before closing sockets, and call that from MultiBlockchainClient.destroy().
Also applies to: 705-714
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/chronikService.ts` around lines 204 - 220, The reconnect logic
reopens Chronik during an intentional teardown; add a wsShuttingDown boolean
flag on ChronikBlockchainClient, set it to true in a new shutdown() method (call
shutdown() from MultiBlockchainClient.destroy() before closing
chronikWSEndpoint), and guard the onEnd handler and reconnectWs() (which
currently uses wsReconnecting and reconnectWs) to return immediately if
wsShuttingDown is true so the socket isn't reopened during shutdown; apply the
same guard for the other onEnd/reconnect path around the code at the 705-714
area.
Fabcien
reviewed
Jun 3, 2026
Fabcien
requested changes
Jun 22, 2026
| console.warn(`[PRICES] Skipped ${skippedTxIds.length} txs due to missing price data.`) | ||
| } | ||
|
|
||
| const txIdsToConnect = txList.filter(t => !skippedTxIds.includes(t.id)).map(t => t.id) |
Collaborator
There was a problem hiding this comment.
This is very inefficient, you can build txIdsToConnect on the fly (instead of the skipped ones) and check if txList.length > txIdsToConnect.length to determine if there is any skipped tx. This avoids the O(n²) search and map building + saves some memory from the temporary filter vector.
| } | ||
|
|
||
| private async connectWsWithRetry (maxRetries = 10, baseDelay = 5000): Promise<void> { | ||
| for (let attempt = 1; attempt <= maxRetries; attempt++) { |
Collaborator
Author
There was a problem hiding this comment.
I renamed maxRetries to maxAttempts and CHRONIK_WS_MAX_TRIES to CHRONIK_WS_MAX_CONNECTION_ATTEMPTS. Now it should be good
| autoReconnect: true | ||
| onEnd: (e: ws.Event) => { | ||
| console.log(`${this.CHRONIK_MSG_PREFIX}: Chronik WebSocket ended, type: ${e.type}. Attempting reconnection...`) | ||
| void this.reconnectWs() |
Collaborator
There was a problem hiding this comment.
I'm not sure that's what you want, this could prevent you from stopping the app as it will keep trying to reconnect ?
Collaborator
Author
There was a problem hiding this comment.
Indeed, I think the reconnection logic only at onReconnect should fix the issue.
| export const BCH_TX_EXPLORER_URL = 'https://blockchair.com/bitcoin-cash/transaction/' | ||
|
|
||
| export const MAX_MEMPOOL_TXS_TO_PROCESS_AT_A_TIME = 2 | ||
| export const MAX_CONFIRMED_TXS_TO_PROCESS_AT_A_TIME = 6 |
Collaborator
There was a problem hiding this comment.
OTOH this default seems very low to me. A block full will take ~10 minutes of total delay, not accounting for the tx processing itself
Collaborator
Author
There was a problem hiding this comment.
I bumped it to 20, but in the end we have to try: Since we have to connect new txs to address & prices, this would sometimes try to do too much at once and end up timing out the DB connection
| if (uncachedAddresses.length > 0 && process.env.SKIP_CACHE_REBUILD === undefined) { | ||
| console.log(`[CACHE] ${uncachedAddresses.length}/${allAddresses.length} uncached addresses for user ${userId}, starting background rebuild`) | ||
| void cacheAddressesInBackground(uncachedAddresses, userId) | ||
| } |
Collaborator
There was a problem hiding this comment.
This is duplicated from getPaymentList, you should call that instead
Collaborator
Author
There was a problem hiding this comment.
Refactored both to use same auxiliary function.
|
|
||
| const backgroundDbUrl = (process.env.DATABASE_URL ?? '').replace( | ||
| /connection_limit=\d+/, | ||
| 'connection_limit=3' |
Collaborator
There was a problem hiding this comment.
Maybe make it a constant?
Collaborator
Author
There was a problem hiding this comment.
I took it out, we can change that in .env already
| paymentCount++ | ||
| } | ||
| } | ||
| await new Promise(resolve => setTimeout(resolve, 200)) |
Collaborator
There was a problem hiding this comment.
You should add a comment on this one. I'm not sure it's really helping anything but at least if somebody else looks at the code there should be an explanation about why there is a seemingly random delay in the processing workflow
| balance, | ||
| const address = await fetchAddressBySubstring(addressString) | ||
| const [balanceResult, paymentCount] = await Promise.all([ | ||
| prisma.transaction.aggregate({ |
| where: { userProfiles: { some: { userId } } }, | ||
| select: { id: true } | ||
| }) | ||
| const userAddressIds = userAddresses.map(a => a.id) |
Collaborator
There was a problem hiding this comment.
too bad prisma can't flatten the result directly! It's rebuilding the object from the SQL, unflattening the result with that single attribute so you can flatten it again, what a lost
Fabcien
reviewed
Jun 22, 2026
| command: sh -c "sed -i 's/\\r//g' ./scripts/redis-start.sh && sh ./scripts/redis-start.sh" | ||
|
|
||
| volumes: | ||
| paybutton-dump-db-data: |
Collaborator
There was a problem hiding this comment.
Can you explain why this commit exists ? These changes where beneficial I don't see the point reverting them
Collaborator
Author
There was a problem hiding this comment.
I don't understand what commit in particular you are referring to, nor how you got that diff. I am guessing is because this branch was behind master after we merged #1131 (where these lines were added to the file) to it, but I am not sure. I haven't changed this remote branch since this comment, but it says Outdated already
| await redis.set(key, JSON.stringify(cachable)) | ||
| } | ||
|
|
||
| const cacheDashboardDataWithTTL = async (userId: string, dashboardData: DashboardData, ttlSeconds: number): Promise<void> => { |
Collaborator
There was a problem hiding this comment.
Same why the revert ? What's the problem ? These reverts make the PR impossible to review, you really need to squash the fixes so the actual changes can be reviewed
Collaborator
Author
There was a problem hiding this comment.
I removed the changes because I thought maybe not ideal to be serving out-of-date data for 30s, and thought we don't gain much with the partial caches.
About the reverts, usually I review the latest version. If I would squash this revert to the commit I added the changes, that would've been an empty commit, no? I don't get why it would make it better, but sure I can improve my workflow in order to make it easier for reviewers once I understand it.
…ild, use getUserUncachedAddresses)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@redis/paymentCache.ts`:
- Around line 33-49: Keep rebuild discovery off the payments hot path by making
getPaymentList and getPaymentStream stop awaiting
triggerBackgroundRebuildIfNeeded. Update triggerBackgroundRebuildIfNeeded to
return immediately when isBackgroundRebuildActive(userId) is true, and avoid
fully draining getUserUncachedAddresses before deciding to rebuild; instead kick
off discovery/rebuild asynchronously and handle errors inside that background
flow so failures cannot delay the request or surface as unhandled rejections.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b33b7425-1f80-4985-959b-eed7d574ab0f
📒 Files selected for processing (5)
constants/index.tsredis/paymentCache.tsservices/chronikService.tsservices/transactionService.tstsconfig.tsbuildinfo
🚧 Files skipped from review as they are similar to previous changes (1)
- services/chronikService.ts
Comment on lines
+33
to
+49
| const triggerBackgroundRebuildIfNeeded = async (userId: string): Promise<void> => { | ||
| if (process.env.SKIP_CACHE_REBUILD !== undefined) return | ||
| const uncachedAddresses: Address[] = [] | ||
| const uncachedAddressStream = getUserUncachedAddresses(userId) | ||
| for await (const address of uncachedAddressStream) { | ||
| uncachedAddresses.push(address) | ||
| } | ||
| if (uncachedAddresses.length > 0) { | ||
| if (!isBackgroundRebuildActive(userId)) { | ||
| console.log(`[CACHE] ${uncachedAddresses.length} uncached addresses for user ${userId}, starting background rebuild`) | ||
| } | ||
| void cacheAddressesInBackground(uncachedAddresses, userId) | ||
| } | ||
| } | ||
|
|
||
| export const getPaymentList = async (userId: string): Promise<Payment[]> => { | ||
| await triggerBackgroundRebuildIfNeeded(userId) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep rebuild discovery off the payments hot path.
getPaymentList/getPaymentStream still wait for triggerBackgroundRebuildIfNeeded, and that function fully scans getUserUncachedAddresses before returning—even when a rebuild is already active. Large users can still see request latency, and discovery/rebuild failures can still affect the request or become unhandled rejections.
Suggested direction
+const backgroundRebuildDiscoveryInProgress = new Set<string>()
+
-const triggerBackgroundRebuildIfNeeded = async (userId: string): Promise<void> => {
+const triggerBackgroundRebuildIfNeeded = (userId: string): void => {
if (process.env.SKIP_CACHE_REBUILD !== undefined) return
+ if (isBackgroundRebuildActive(userId) || backgroundRebuildDiscoveryInProgress.has(userId)) return
+
+ backgroundRebuildDiscoveryInProgress.add(userId)
+ void (async () => {
+ try {
const uncachedAddresses: Address[] = []
const uncachedAddressStream = getUserUncachedAddresses(userId)
for await (const address of uncachedAddressStream) {
uncachedAddresses.push(address)
}
if (uncachedAddresses.length > 0) {
if (!isBackgroundRebuildActive(userId)) {
- console.log(`[CACHE] ${uncachedAddresses.length} uncached addresses for user ${userId}, starting background rebuild`)
+ console.log(`[CACHE] ${uncachedAddresses.length} uncached addresses, starting background rebuild`)
}
- void cacheAddressesInBackground(uncachedAddresses, userId)
+ await cacheAddressesInBackground(uncachedAddresses, userId)
}
+ } catch (err) {
+ console.warn('[CACHE] Failed to trigger background rebuild', err)
+ } finally {
+ backgroundRebuildDiscoveryInProgress.delete(userId)
+ }
+ })()
}
export const getPaymentList = async (userId: string): Promise<Payment[]> => {
- await triggerBackgroundRebuildIfNeeded(userId)
+ triggerBackgroundRebuildIfNeeded(userId)
return await getCachedPaymentsForUser(userId)
}Also applies to: 374-374
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@redis/paymentCache.ts` around lines 33 - 49, Keep rebuild discovery off the
payments hot path by making getPaymentList and getPaymentStream stop awaiting
triggerBackgroundRebuildIfNeeded. Update triggerBackgroundRebuildIfNeeded to
return immediately when isBackgroundRebuildActive(userId) is true, and avoid
fully draining getUserUncachedAddresses before deciding to rebuild; instead kick
off discovery/rebuild asynchronously and handle errors inside that background
flow so failures cannot delay the request or surface as unhandled rejections.
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
2 participants
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.
Depends On
Description
Performance
/api/paymentsusesaddressId IN (...)instead of EXISTS subquery (120s → 4s for large users)/api/payments/countuses direct DB count instead of dashboard cache recomputationinputsrelationTest Plan
This is running on prod rn.
To test locally, preferrably use prod's DB locally according to the workflow detailed in #1131
Summary by CodeRabbit
New Features
Bug Fixes