feat: paginate history, add request IDs, supertest integration tests,… - #473
Conversation
… and infinite scroll Adds paginated campaign history, X-Request-ID correlation across the stack, in-process supertest integration tests, and IntersectionObserver-based campaign list loading. Restores corrupted package and app files required for CI. Closes ritik4ever#219 Closes ritik4ever#223 Closes ritik4ever#225 Closes ritik4ever#258 Co-authored-by: Cursor <cursoragent@cursor.com>
|
@devsimze is attempting to deploy a commit to the ritik4ever's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
@Abidoyesimze Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
📝 WalkthroughWalkthroughAdds four features: a paginated ChangesBackend: tracing, paginated history, store fixes, tests
Frontend: HTTP client, API pagination, infinite scroll, routing
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant CampaignsTable
participant apiRequest
participant Axios as apiClient interceptor
participant ViteProxy
participant requestIdMiddleware
participant listCampaignHistory
rect rgba(100, 149, 237, 0.5)
Note over User,CampaignsTable: Infinite scroll triggers next page
User->>CampaignsTable: scrolls to bottom sentinel
CampaignsTable->>App: onLoadMore()
App->>apiRequest: listCampaigns({ page: N, limit: 20 })
apiRequest->>Axios: attach X-Request-ID header
Axios->>ViteProxy: GET /api/campaigns?page=N (X-Request-ID: uuid)
ViteProxy->>requestIdMiddleware: forward X-Request-ID
requestIdMiddleware->>requestIdMiddleware: set AsyncLocalStorage { requestId }
end
rect rgba(144, 238, 144, 0.5)
Note over requestIdMiddleware,listCampaignHistory: Paginated history fetch
App->>apiRequest: getCampaignHistory(campaignId, page, pageSize)
apiRequest->>ViteProxy: GET /api/campaigns/:id/history?page=1&pageSize=20
ViteProxy->>requestIdMiddleware: forward X-Request-ID
requestIdMiddleware->>listCampaignHistory: (campaignId, { page, pageSize })
listCampaignHistory-->>App: { data, total, hasMore, page, pageSize }
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
backend/vitest.config.ts (1)
6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
tests/**/*.integration.tsglob matches no files.The integration suite is named
integration.test.ts(already covered bytests/**/*.test.ts). The*.integration.tspattern appears to be dead config; either rename the file or drop the redundant glob.🤖 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 `@backend/vitest.config.ts` at line 6, The Vitest include list contains a dead glob that matches no files, so update the test discovery config in the vitest config to remove the redundant tests/**/*.integration.ts pattern or rename the integration suite to match it; use the include setting in the config to keep only globs that actually cover existing test file names like integration.test.ts.
🤖 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 `@backend/package.json`:
- Line 12: The dependency versions are mismatched: `express` is on the 4.x
runtime line, but `@types/express` is currently on the 5.x typings line, which
can produce incorrect `Request`/`Response` and router types. Update the
`backend/package.json` dependency entry for `@types/express` to the matching 4.x
typings version so it aligns with the existing `express` package and keeps type
checking consistent.
In `@backend/src/logger.ts`:
- Around line 57-63: The withRequestContext helper in logger.ts is overwriting
the injected requestId when fields.requestId exists as undefined because the
spread order lets fields clobber the context value. Update the merge logic in
withRequestContext so the request-scoped requestId wins whenever it is injected,
and only preserve an explicit non-undefined fields.requestId if that is
intended; use the withRequestContext and getRequestId symbols to locate the fix.
In `@backend/vitest.config.ts`:
- Around line 8-11: Move the Vitest worker settings off the legacy top-level
keys and into the pool configuration. In vitest.config.ts, keep isolate as-is,
but replace the current top-level threads, maxThreads, and minThreads usage with
poolOptions.threads so the worker limits are defined in the modern Vitest config
shape. Use the existing Vitest config object in this file to locate and update
the worker pool settings.
In `@frontend/src/App.tsx`:
- Around line 248-252: The refresh flow is passing campaign IDs into
refreshCampaigns as the searchQuery argument, which can filter the list instead
of selecting the intended campaign. Update the create/pledge/claim/refund call
sites to pass the ID via nextSelectedId and leave searchQuery empty unless an
actual search term is intended. Use refreshCampaigns in App.tsx and its related
post-action handlers to preserve the current campaign list while selecting the
new or updated campaign.
- Around line 255-261: The campaign ID resolution in App.tsx is too page-local
and can incorrectly send valid direct links to not-found when the campaign is
not in the first loaded page. Update the selection flow around the existing
nextSelectedId/selectedCampaignId logic and the getCampaign lookup so that
missing requested ids are resolved via getCampaign before deciding they are
invalid, and only set the not-found path when getCampaign returns an actual
not-found response. Keep the existing setInvalidUrlCampaignId and
setSelectedCampaignId behavior, but make them depend on the resolved campaign
from getCampaign rather than the current page data alone.
- Around line 226-231: The global `handleKeydown` shortcut in `App.tsx` is
firing even when the user is focused in editable fields, which blocks typing
`?`. Update the keyboard handler inside `useEffect` to ignore events originating
from text inputs, textareas, contenteditable elements, or other form controls
before toggling `setIsShortcutsOpen`, and keep the shortcut active only when
focus is not in an editing context. Use the existing `handleKeydown` and
`useEffect` logic to add this guard.
- Around line 452-462: The Freighter account watcher in useEffect only displays
a toast and leaves connectedWallet stale when the account changes, so
pledge/claim flows can keep using the old wallet. Update the state managed by
the Freighter wallet hook when watchFreighterAccount reports a new address, or
explicitly disconnect/reconnect on mismatch, so connectedWallet stays in sync
with the current Freighter account.
In `@frontend/src/components/CampaignsTable.tsx`:
- Around line 76-82: The debounced search effect in CampaignsTable is firing on
mount and again whenever the parent re-renders because onSearchChange is an
inline prop from App, so its changing identity retriggers the effect. Update the
search flow around useDebounce and the useEffect that calls onSearchChange to
depend only on debouncedSearchQuery, and decouple the callback identity by
storing onSearchChange in a ref or equivalent stable holder. Also skip the
initial empty-query emit or seed the search state from the restored query so the
paginated list is not reset on first render.
In `@frontend/src/services/httpClient.ts`:
- Around line 60-90: The retry logic in httpClient’s request/response handling
is too broad because the catch block retries every thrown error, including the
explicit 4xx error created from response.data and mutation requests that may
already have committed side effects. Update the retry policy around the existing
response.status checks and catch path so only retryable failures are retried by
default: limit retries to idempotent/safe methods in the request flow, and only
allow POST/mutation retries when an idempotency key or explicit opt-in is
present. Keep the 4xx path in the same httpClient logic as non-retryable and
make sure network/5xx retries respect the method-based guard.
---
Nitpick comments:
In `@backend/vitest.config.ts`:
- Line 6: The Vitest include list contains a dead glob that matches no files, so
update the test discovery config in the vitest config to remove the redundant
tests/**/*.integration.ts pattern or rename the integration suite to match it;
use the include setting in the config to keep only globs that actually cover
existing test file names like integration.test.ts.
🪄 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: 2df2404d-4d6b-485b-89b7-1c92e624bb3c
⛔ Files ignored due to path filters (2)
backend/package-lock.jsonis excluded by!**/package-lock.jsonfrontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
backend/package.jsonbackend/src/api.test.tsbackend/src/historyEndpoint.test.tsbackend/src/index.tsbackend/src/logger.tsbackend/src/middleware/requestId.tsbackend/src/middleware/types.tsbackend/src/requestContext.tsbackend/src/requestId.test.tsbackend/src/services/campaignStore.tsbackend/src/services/eventHistory.tsbackend/src/validation/schemas.tsbackend/tests/integration.test.tsbackend/vitest.config.tsfrontend/package.jsonfrontend/src/App.tsxfrontend/src/components/CampaignsTable.infiniteScroll.test.tsxfrontend/src/components/CampaignsTable.tsxfrontend/src/hooks/useMediaQuery.tsfrontend/src/main.tsxfrontend/src/services/api.tsfrontend/src/services/httpClient.test.tsfrontend/src/services/httpClient.tsfrontend/vite.config.mts
| "compression": "^1.7.4", | ||
| "cors": "^2.8.5", | ||
| "dotenv": "^17.3.1", | ||
| "express": "^4.21.2", |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🌐 Web query:
What major version of @types/express corresponds to Express 4.x versus Express 5.x?
💡 Result:
The major version of @types/express should correspond to the major version of Express being used in your project [1]. - For Express 4.x, you should use @types/express 4.x [1][2]. - For Express 5.x, you should use @types/express 5.x [1][2]. It is important to explicitly specify the version of @types/express in your package.json (e.g., ^4.17.0 or ^5.0.0) rather than using a wildcard like * or omitting the version, as package managers may otherwise pull the latest version (v5.x), which is incompatible with Express 4.x applications [3][4][5].
Citations:
- 1: Issue with the current [@types/express] upgrade DefinitelyTyped/DefinitelyTyped#71444
- 2: feat: types for
express5 DefinitelyTyped/DefinitelyTyped#70563 - 3: Version 4.21.0 and older now pulls in types for 5.0.0 that are incompatible. expressjs/express#5987
- 4: Type resolution for express ~ 4.21.2 is wrong, pulls @types/express v5 instead of v4 expressjs/session#1007
- 5: Version 4.21.0 and older now pulls in types for 5.0.0 that are incompatible. expressjs/express#5987
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files backend/package.json
echo
cat -n backend/package.json | sed -n '1,80p'Repository: ritik4ever/stellar-goal-vault
Length of output: 1590
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import json, pathlib
p = pathlib.Path('backend/package.json')
data = json.loads(p.read_text())
deps = data.get('dependencies', {})
dev = data.get('devDependencies', {})
for key in ['express', '`@types/express`', 'typescript', '`@typescript-eslint/eslint-plugin`', '`@typescript-eslint/parser`']:
if key in deps:
print(f"dependencies {key}: {deps[key]}")
if key in dev:
print(f"devDependencies {key}: {dev[key]}")
PYRepository: ritik4ever/stellar-goal-vault
Length of output: 378
Align @types/express with Express 4
express is pinned to ^4.21.2, so @types/express should stay on the 4.x typings line. ^5.0.3 targets Express 5 and can mis-type Request/Response and router behavior against the current runtime.
Proposed fix
- "`@types/express`": "^5.0.3",
+ "`@types/express`": "^4.17.21",🤖 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 `@backend/package.json` at line 12, The dependency versions are mismatched:
`express` is on the 4.x runtime line, but `@types/express` is currently on the
5.x typings line, which can produce incorrect `Request`/`Response` and router
types. Update the `backend/package.json` dependency entry for `@types/express`
to the matching 4.x typings version so it aligns with the existing `express`
package and keeps type checking consistent.
| function withRequestContext(fields: LogFields): LogFields { | ||
| const requestId = getRequestId(); | ||
| if (requestId && fields.requestId === undefined) { | ||
| return { requestId, ...fields }; | ||
| } | ||
| return fields; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Spread order clobbers the injected requestId.
The branch is taken whenever fields.requestId === undefined, which is true both when the key is absent and when it is explicitly undefined. In the latter case (e.g. a caller spreading a context object that contains requestId: undefined), { requestId, ...fields } lets the later spread overwrite the context-derived requestId back to undefined, defeating the auto-injection.
🐛 Proposed fix
if (requestId && fields.requestId === undefined) {
- return { requestId, ...fields };
+ return { ...fields, requestId };
}📝 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.
| function withRequestContext(fields: LogFields): LogFields { | |
| const requestId = getRequestId(); | |
| if (requestId && fields.requestId === undefined) { | |
| return { requestId, ...fields }; | |
| } | |
| return fields; | |
| } | |
| function withRequestContext(fields: LogFields): LogFields { | |
| const requestId = getRequestId(); | |
| if (requestId && fields.requestId === undefined) { | |
| return { ...fields, requestId }; | |
| } | |
| return fields; | |
| } |
🤖 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 `@backend/src/logger.ts` around lines 57 - 63, The withRequestContext helper in
logger.ts is overwriting the injected requestId when fields.requestId exists as
undefined because the spread order lets fields clobber the context value. Update
the merge logic in withRequestContext so the request-scoped requestId wins
whenever it is injected, and only preserve an explicit non-undefined
fields.requestId if that is intended; use the withRequestContext and
getRequestId symbols to locate the fix.
| threads: true, | ||
| maxThreads: 4, | ||
| minThreads: 1, | ||
| isolate: true, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor
🧩 Analysis chain
🌐 Web query:
Vitest 1.0 pool poolOptions threads maxThreads config vs deprecated threads option
💡 Result:
In Vitest 1.0, the configuration structure for managing test parallelization was centralized into the pool and poolOptions configuration objects [1][2]. This update replaced several older, disconnected flags with a standardized API [3]. Specifically: - The pool option allows you to select the runner engine, such as 'threads', 'forks', or 'vmThreads' [2][4]. - poolOptions provided a nested object to configure specific parameters for these pools (e.g., poolOptions.threads.maxThreads for thread count) [2]. Important context regarding the lifecycle of these options: - Legacy flags like --threads and --no-threads were deprecated in favor of --pool=threads and --pool=forks respectively [2][3]. - As of Vitest 4.0, the poolOptions object itself has been completely removed [5][6]. All configuration settings previously nested within poolOptions (such as maxThreads, maxForks, isolate, etc.) have been moved to top-level properties under the test configuration object [5][6][7]. - For instance, maxThreads and maxForks are now unified under the top-level property maxWorkers [6][7]. If you are migrating or maintaining a project, you should move away from the poolOptions structure and use the modern, flatter configuration syntax introduced in version 4.0 [6][7].
Citations:
- 1: vitest-dev/vitest@114a993c
- 2: feat!: add support for
poolandpoolOptionsvitest-dev/vitest#4172 - 3: https://v3.vitest.dev/guide/migration
- 4: https://github.com/vitest-dev/vitest/blob/1a4705da/docs/guide/improving-performance.md
- 5: " DEPRECATED
test.poolOptionswas removed in Vitest 4. All previouspoolOptionsare now top-level options. Please, refer to the migration guide: https://vitest.dev/guide/migration#pool-rework" vitest-dev/vitest#9563 - 6: https://vitest.dev/guide/migration.html
- 7: https://github.com/vitest-dev/vitest/blob/main/docs/guide/migration.md
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
git ls-files backend/vitest.config.ts backend/package.json package.json pnpm-lock.yaml package-lock.json yarn.lock 2>/dev/null || true
printf '\nbackend/vitest.config.ts:\n'
sed -n '1,120p' backend/vitest.config.ts
printf '\npackage.json references to vitest:\n'
rg -n '"vitest"|vitest' backend/package.json package.json pnpm-lock.yaml package-lock.json yarn.lock 2>/dev/null || trueRepository: ritik4ever/stellar-goal-vault
Length of output: 1132
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If vitest config is TypeScript, inspect nearby imports and exports too.
ast-grep outline backend/vitest.config.ts --view expanded || trueRepository: ritik4ever/stellar-goal-vault
Length of output: 206
Move the Vitest worker limits into poolOptions.threads
threads, maxThreads, and minThreads are legacy Vitest 0.x keys here. Keep isolate, but configure the worker pool like this:
Proposed fix
- threads: true,
- maxThreads: 4,
- minThreads: 1,
+ pool: "threads",
+ poolOptions: {
+ threads: {
+ minThreads: 1,
+ maxThreads: 4,
+ },
+ },📝 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.
| threads: true, | |
| maxThreads: 4, | |
| minThreads: 1, | |
| isolate: true, | |
| pool: "threads", | |
| poolOptions: { | |
| threads: { | |
| minThreads: 1, | |
| maxThreads: 4, | |
| }, | |
| }, | |
| isolate: true, |
🤖 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 `@backend/vitest.config.ts` around lines 8 - 11, Move the Vitest worker
settings off the legacy top-level keys and into the pool configuration. In
vitest.config.ts, keep isolate as-is, but replace the current top-level threads,
maxThreads, and minThreads usage with poolOptions.threads so the worker limits
are defined in the modern Vitest config shape. Use the existing Vitest config
object in this file to locate and update the worker pool settings.
| useEffect(() => { | ||
| const handleKeydown = (event: KeyboardEvent) => { | ||
| if (event.key === "?" && !event.metaKey && !event.ctrlKey && !event.altKey) { | ||
| event.preventDefault(); | ||
| setIsShortcutsOpen((current) => !current); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Ignore global shortcuts while the user is typing.
Pressing ? inside search, form inputs, or textareas currently opens the shortcuts overlay and prevents entering that character.
Proposed guard
const handleKeydown = (event: KeyboardEvent) => {
+ const target = event.target as HTMLElement | null;
+ const isTyping =
+ target instanceof HTMLInputElement ||
+ target instanceof HTMLTextAreaElement ||
+ target instanceof HTMLSelectElement ||
+ target?.isContentEditable === true;
+
+ if (isTyping) {
+ return;
+ }
+
if (event.key === "?" && !event.metaKey && !event.ctrlKey && !event.altKey) {📝 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.
| useEffect(() => { | |
| const handleKeydown = (event: KeyboardEvent) => { | |
| if (event.key === "?" && !event.metaKey && !event.ctrlKey && !event.altKey) { | |
| event.preventDefault(); | |
| setIsShortcutsOpen((current) => !current); | |
| } | |
| useEffect(() => { | |
| const handleKeydown = (event: KeyboardEvent) => { | |
| const target = event.target as HTMLElement | null; | |
| const isTyping = | |
| target instanceof HTMLInputElement || | |
| target instanceof HTMLTextAreaElement || | |
| target instanceof HTMLSelectElement || | |
| target?.isContentEditable === true; | |
| if (isTyping) { | |
| return; | |
| } | |
| if (event.key === "?" && !event.metaKey && !event.ctrlKey && !event.altKey) { | |
| event.preventDefault(); | |
| setIsShortcutsOpen((current) => !current); | |
| } |
🤖 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 `@frontend/src/App.tsx` around lines 226 - 231, The global `handleKeydown`
shortcut in `App.tsx` is firing even when the user is focused in editable
fields, which blocks typing `?`. Update the keyboard handler inside `useEffect`
to ignore events originating from text inputs, textareas, contenteditable
elements, or other form controls before toggling `setIsShortcutsOpen`, and keep
the shortcut active only when focus is not in an editing context. Use the
existing `handleKeydown` and `useEffect` logic to add this guard.
| async function refreshCampaigns(searchQuery: string = '', nextSelectedId?: string | null): Promise<Campaign[]> { | ||
| setIsCampaignsLoading(true); | ||
| activeSearchRef.current = searchQuery; | ||
| try { | ||
| const response = await fetchCampaignPage(1, searchQuery, false); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pass campaign IDs as nextSelectedId, not as search text.
refreshCampaigns treats its first argument as searchQuery, but these callers pass a campaign id there. After create/pledge/claim/refund, the list refresh searches for the id string and can hide the campaign instead of selecting it.
Proposed call-site fix
- await refreshCampaigns(campaign.id);
+ await refreshCampaigns('', campaign.id);
@@
- const refreshedCampaigns = await refreshCampaigns(campaignId);
+ const refreshedCampaigns = await refreshCampaigns('', campaignId);
@@
- await refreshCampaigns(campaign.id);
+ await refreshCampaigns('', campaign.id);
@@
- await refreshCampaigns(campaignId);
+ await refreshCampaigns('', campaignId);Also applies to: 424-427, 499-500, 553-554, 590-591
🤖 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 `@frontend/src/App.tsx` around lines 248 - 252, The refresh flow is passing
campaign IDs into refreshCampaigns as the searchQuery argument, which can filter
the list instead of selecting the intended campaign. Update the
create/pledge/claim/refund call sites to pass the ID via nextSelectedId and
leave searchQuery empty unless an actual search term is intended. Use
refreshCampaigns in App.tsx and its related post-action handlers to preserve the
current campaign list while selecting the new or updated campaign.
| const requestedId = nextSelectedId ?? selectedCampaignId; | ||
| const nextId = requestedId ?? data[0]?.id ?? null; | ||
| const exists = nextId ? data.some((campaign) => campaign.id === nextId) : false; | ||
| const resolvedId = exists ? nextId : data[0]?.id ?? null; | ||
|
|
||
| setInvalidUrlCampaignId(requestedId && !exists ? requestedId : null); | ||
| setSelectedCampaignId(resolvedId); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don’t 404 valid campaigns that are not on page 1.
Both checks only look in the currently loaded first page. A direct /campaigns/:id link to a valid campaign on page 2+ will navigate to /not-found before getCampaign verifies the id. Resolve requested ids with getCampaign when they are missing from the loaded page, and only navigate to not-found on an actual not-found response.
Also applies to: 360-366
🤖 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 `@frontend/src/App.tsx` around lines 255 - 261, The campaign ID resolution in
App.tsx is too page-local and can incorrectly send valid direct links to
not-found when the campaign is not in the first loaded page. Update the
selection flow around the existing nextSelectedId/selectedCampaignId logic and
the getCampaign lookup so that missing requested ids are resolved via
getCampaign before deciding they are invalid, and only set the not-found path
when getCampaign returns an actual not-found response. Keep the existing
setInvalidUrlCampaignId and setSelectedCampaignId behavior, but make them depend
on the resolved campaign from getCampaign rather than the current page data
alone.
| useEffect(() => { | ||
| if (!connectedWallet) return; | ||
| const stop = watchFreighterAccount((address) => { | ||
| if (address && address !== connectedWallet) { | ||
| addToast(`Switched to ${address.slice(0, 16)}...`, "success"); | ||
| } else if (!address) { | ||
| addToast("Wallet disconnected.", "success"); | ||
| } | ||
| }); | ||
| return stop; | ||
| }, [connectedWallet, addToast]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep wallet state in sync when Freighter switches accounts.
The watcher only shows a toast when Freighter changes address; connectedWallet remains the old key, so subsequent pledge/claim checks and payloads can use stale account state. Either update the Freighter hook state or disconnect and require reconnect on mismatch.
Minimal disconnect-on-switch option
useEffect(() => {
if (!connectedWallet) return;
const stop = watchFreighterAccount((address) => {
if (address && address !== connectedWallet) {
- addToast(`Switched to ${address.slice(0, 16)}...`, "success");
+ freighter.disconnect();
+ addToast(`Freighter switched to ${address.slice(0, 16)}... Please reconnect.`, "error");
} else if (!address) {
+ freighter.disconnect();
addToast("Wallet disconnected.", "success");
}
});
return stop;
- }, [connectedWallet, addToast]);
+ }, [connectedWallet, addToast, freighter]);📝 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.
| useEffect(() => { | |
| if (!connectedWallet) return; | |
| const stop = watchFreighterAccount((address) => { | |
| if (address && address !== connectedWallet) { | |
| addToast(`Switched to ${address.slice(0, 16)}...`, "success"); | |
| } else if (!address) { | |
| addToast("Wallet disconnected.", "success"); | |
| } | |
| }); | |
| return stop; | |
| }, [connectedWallet, addToast]); | |
| useEffect(() => { | |
| if (!connectedWallet) return; | |
| const stop = watchFreighterAccount((address) => { | |
| if (address && address !== connectedWallet) { | |
| freighter.disconnect(); | |
| addToast(`Freighter switched to ${address.slice(0, 16)}... Please reconnect.`, "error"); | |
| } else if (!address) { | |
| freighter.disconnect(); | |
| addToast("Wallet disconnected.", "success"); | |
| } | |
| }); | |
| return stop; | |
| }, [connectedWallet, addToast, freighter]); |
🤖 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 `@frontend/src/App.tsx` around lines 452 - 462, The Freighter account watcher
in useEffect only displays a toast and leaves connectedWallet stale when the
account changes, so pledge/claim flows can keep using the old wallet. Update the
state managed by the Freighter wallet hook when watchFreighterAccount reports a
new address, or explicitly disconnect/reconnect on mismatch, so connectedWallet
stays in sync with the current Freighter account.
| const [searchQuery, setSearchQuery] = useState(""); | ||
| const loadMoreRef = useRef<HTMLDivElement | null>(null); | ||
| const debouncedSearchQuery = useDebounce(searchQuery, 300); | ||
|
|
||
| useEffect(() => { | ||
| onSearchChange?.(debouncedSearchQuery); | ||
| }, [debouncedSearchQuery, onSearchChange]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Debounced search effect currently re-fetches on mount and parent rerenders.
onSearchChange is in the effect deps, but App provides it as an inline function, so identity churn causes repeated refreshes; plus the initial empty-query call can reset restored paginated list state. Decouple callback identity from the debounce trigger (e.g., callback ref + effect keyed only by debounced query, and skip first emit or initialize from restored query).
🤖 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 `@frontend/src/components/CampaignsTable.tsx` around lines 76 - 82, The
debounced search effect in CampaignsTable is firing on mount and again whenever
the parent re-renders because onSearchChange is an inline prop from App, so its
changing identity retriggers the effect. Update the search flow around
useDebounce and the useEffect that calls onSearchChange to depend only on
debouncedSearchQuery, and decouple the callback identity by storing
onSearchChange in a ref or equivalent stable holder. Also skip the initial
empty-query emit or seed the search state from the restored query so the
paginated list is not reset on first render.
| if (response.status >= 500 && attempt < MAX_RETRIES) { | ||
| await sleep(RETRY_DELAY_MS); | ||
| continue; | ||
| } | ||
|
|
||
| if (response.status >= 400) { | ||
| const body = response.data as { | ||
| error?: { | ||
| code?: string; | ||
| message?: string; | ||
| details?: Array<{ field: string; message: string }>; | ||
| requestId?: string; | ||
| }; | ||
| }; | ||
| const error = new Error(body.error?.message ?? 'Unexpected API error'); | ||
| (error as Error & { code?: string }).code = body.error?.code; | ||
| (error as Error & { details?: Array<{ field: string; message: string }> }).details = | ||
| body.error?.details; | ||
| (error as Error & { requestId?: string }).requestId = | ||
| body.error?.requestId ?? requestId; | ||
| throw error; | ||
| } | ||
|
|
||
| return response.data; | ||
| } catch (error) { | ||
| lastError = error; | ||
| if (attempt >= MAX_RETRIES) { | ||
| throw error; | ||
| } | ||
| await sleep(RETRY_DELAY_MS); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Constrain retries to retryable responses and safe methods.
The catch block retries every error thrown inside the try, including the 4xx error built on Lines 74-80. It also retries 5xx/network failures for POST mutations, which can duplicate create/pledge/claim/refund side effects after a committed-but-failed response. Retry only idempotent requests by default, or require an idempotency key/explicit opt-in for mutations.
Proposed retry-policy shape
+const RETRYABLE_METHODS = new Set(['GET', 'HEAD', 'OPTIONS']);
+
+function isRetryableMethod(method: AxiosRequestConfig['method']): boolean {
+ return RETRYABLE_METHODS.has((method ?? 'GET').toUpperCase());
+}
+
export async function apiRequest<T>(config: AxiosRequestConfig): Promise<T> {
let lastError: unknown;
let requestId: string | undefined;
+ const canRetryMethod = isRetryableMethod(config.method);
for (let attempt = 0; attempt <= MAX_RETRIES; attempt += 1) {
+ const canRetryAttempt = canRetryMethod && attempt < MAX_RETRIES;
try {
const response = await apiClient.request<T>({
...config,
@@
- if (response.status >= 500 && attempt < MAX_RETRIES) {
+ if (response.status >= 500 && canRetryAttempt) {
await sleep(RETRY_DELAY_MS);
continue;
}
@@
if (response.status >= 400) {
@@
throw error;
}
return response.data;
} catch (error) {
lastError = error;
- if (attempt >= MAX_RETRIES) {
+ if (!canRetryAttempt) {
throw error;
}
await sleep(RETRY_DELAY_MS);
}
}🤖 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 `@frontend/src/services/httpClient.ts` around lines 60 - 90, The retry logic in
httpClient’s request/response handling is too broad because the catch block
retries every thrown error, including the explicit 4xx error created from
response.data and mutation requests that may already have committed side
effects. Update the retry policy around the existing response.status checks and
catch path so only retryable failures are retried by default: limit retries to
idempotent/safe methods in the request flow, and only allow POST/mutation
retries when an idempotency key or explicit opt-in is present. Keep the 4xx path
in the same httpClient logic as non-retryable and make sure network/5xx retries
respect the method-based guard.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const [selectedCampaignId, setSelectedCampaignId] = useState<string | null>(paramId ?? null); | ||
| const [selectedCampaignDetails, setSelectedCampaignDetails] = useState<Campaign | null>( | ||
| null, | ||
| ); |
| } | ||
|
|
||
| if (event.key === "Escape") { | ||
| setIsShortcutsOpen(false); |
|
|
||
| if (event.key === "Escape") { | ||
| setIsShortcutsOpen(false); | ||
| if (transactionPreview) { |
4 participants
Summary
#219 — Add pagination to GET /api/campaigns/:id/history via ?page and ?pageSize (default 20, max 100), returning { data, total, page, pageSize, hasMore } with events ordered newest-first.
#223 — Read or generate X-Request-ID in backend middleware, attach it to structured logs for the request lifetime, echo it in response headers, and forward it from the frontend Axios client (including retries). Vite proxy forwards the header to the backend.
#225 — Refactor integration tests to run in-process with supertest (no separate HTTP server), restore npm test in the backend, and include integration tests in the Vitest config with an 80% line coverage threshold.
#258 — Replace full-list campaign fetch with IntersectionObserver-based infinite scroll (initial page of 20), loading/end-of-list UI, and scroll position restoration on browser back navigation.
Also restores corrupted empty files (package.json, App.tsx, vitest.config.ts, etc.) and fixes related syntax/SQL issues uncovered during implementation.
Closes #219
Closes #223
Closes #225
Closes #258
Test plan
cd backend && npm test — unit + integration tests run in-process via supertest
cd backend && npm test -- --coverage — coverage report generated (80% line threshold configured)
GET /api/campaigns/:id/history?page=1&pageSize=20 returns paginated response with hasMore
GET /api/health echoes X-Request-ID response header (send custom header and verify echo)
cd frontend && npm test -- --run src/components/CampaignsTable.infiniteScroll.test.tsx src/services/httpClient.test.ts
Campaign board loads 20 campaigns initially; scrolling to bottom fetches the next page
Navigate into a campaign and use browser back — list scroll position and loaded pages are restored
Summary by CodeRabbit
New Features
Bug Fixes