fix: resolve test suite failures in backend and frontend#472
Open
rizwandev99 wants to merge 3 commits into
Open
fix: resolve test suite failures in backend and frontend#472rizwandev99 wants to merge 3 commits into
rizwandev99 wants to merge 3 commits into
Conversation
Resolves database foreign key constraint errors during test tear-down, configures correct status codes/errors for challenge auth failures, and handles indexer/webhookWorker database/network promise rejections.
…/contract mocking Mock API calls, WebSocket connections, component props, and Soroban contract addresses to resolve crashes and unhandled exceptions across all frontend unit and integration tests.
|
@rizwandev99 is attempting to deploy a commit to the ritik4ever's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR introduces stream pause/resume functionality with on-chain integration, refactors auth error handling, persists indexer ledger progress, implements webhook dead-letter routing, and significantly enhances frontend UI with bulk stream operations, component improvements, and hook test reliability. Approximately 45 files span backend services, tests, and frontend components. ChangesStream Management and Webhook System
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
backend/src/webhooks.integration.test.ts (3)
116-117:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing parent stream rows may cause foreign key violations.
This test inserts 2 dead letters with
stream_idvalues "s1" and "s2" without first inserting corresponding rows into thestreamstable.🛠️ Proposed fix
it("should return correct total count", async () => { const db = getDb(); + + // Insert parent streams to satisfy FK constraint + const streamStmt = db.prepare(` + INSERT INTO streams (id, sender, recipient, asset_code, total_amount, duration_seconds, start_at, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) + `); + streamStmt.run("s1", "sender", "recipient", "USDC", 100, 3600, 0, 0); + streamStmt.run("s2", "sender", "recipient", "USDC", 100, 3600, 0, 0); + db.prepare(`INSERT INTO webhook_dead_letters (stream_id, event, url, payload, last_error, failed_at) VALUES (?, ?, ?, ?, ?, ?)`).run("s1", "e", "u", "p", "err", 100);🤖 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/webhooks.integration.test.ts` around lines 116 - 117, The test inserts rows into webhook_dead_letters for stream_id "s1" and "s2" without creating the parent rows in the streams table, causing foreign key violations; before the two db.prepare(...).run("s1", ...) and db.prepare(...).run("s2", ...) calls, insert minimal parent rows into the streams table (e.g. insert the required columns for ids "s1" and "s2") so the foreign key constraint is satisfied, ensuring the webhook_dead_letters inserts succeed.
65-68:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing parent stream row may cause foreign key violation.
This test inserts into
webhook_dead_letterswithstream_id = "s1"without first inserting a corresponding row into thestreamstable. Ifwebhook_dead_letters.stream_idhas a foreign key constraint referencingstreams.id, this INSERT will fail.The requeue test at lines 132-135 demonstrates the correct pattern by inserting a mock stream before the dead letter.
🛠️ Proposed fix
it("should return dead letters with correct fields", async () => { const db = getDb(); const now = Math.floor(Date.now() / 1000); + + // Insert parent stream to satisfy FK constraint + db.prepare(` + INSERT INTO streams (id, sender, recipient, asset_code, total_amount, duration_seconds, start_at, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) + `).run("s1", "sender", "recipient", "USDC", 100, 3600, 0, 0); + db.prepare(` INSERT INTO webhook_dead_letters (stream_id, event, url, payload, last_error, failed_at)🤖 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/webhooks.integration.test.ts` around lines 65 - 68, The test inserts into webhook_dead_letters with stream_id "s1" but never creates the corresponding streams row, which can trigger a FK violation; before the INSERT into webhook_dead_letters add a matching INSERT into the streams table (e.g., create a mock stream with id "s1", stream type and any required columns) similar to the pattern used in the requeue test, so the webhook_dead_letters INSERT (in this test function) succeeds without violating the streams -> webhook_dead_letters foreign key constraint.
86-93:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winMissing parent stream rows may cause foreign key violations.
This test inserts 5 dead letters with
stream_idvalues "s1" through "s5" without first inserting corresponding rows into thestreamstable. Apply the same fix pattern as the requeue test.🛠️ Proposed fix
it("should respect pagination (limit/offset)", async () => { const db = getDb(); + + // Insert parent streams to satisfy FK constraint + const streamStmt = db.prepare(` + INSERT INTO streams (id, sender, recipient, asset_code, total_amount, duration_seconds, start_at, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?) + `); + for (let i = 1; i <= 5; i++) { + streamStmt.run(`s${i}`, "sender", "recipient", "USDC", 100, 3600, 0, 0); + } + for (let i = 1; i <= 5; i++) { db.prepare(`🤖 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/webhooks.integration.test.ts` around lines 86 - 93, The test "should respect pagination (limit/offset)" inserts rows into webhook_dead_letters with stream_ids s1..s5 but doesn't create corresponding parent rows in the streams table, causing foreign key violations; update the test (using getDb()) to insert matching entries into the streams table for each stream id (e.g., s1..s5) before inserting into webhook_dead_letters—follow the same pattern used in the requeue test to create the parent stream rows so the foreign key constraint is satisfied.backend/src/services/streamStore.ts (1)
693-703:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not persist pause/resume locally unless on-chain tx succeeds.
Both pause/resume flows continue to DB writes and webhook dispatch without validating final transaction success. If Soroban rejects/fails, local state diverges from chain state.
Proposed fix pattern
const sendRes = await retryWithBackoff(() => rpcServer!.sendTransaction(built)); -if (sendRes.status === "PENDING") { +if (sendRes.status !== "PENDING") { + throw new Error(`pause/resume tx not accepted: ${JSON.stringify(sendRes)}`); +} +{ let txResult; let attempts = 0; while (attempts < 10) { txResult = await retryWithBackoff(() => rpcServer!.getTransaction(sendRes.hash)); if (txResult.status !== "NOT_FOUND") break; await new Promise((r) => setTimeout(r, 1000)); attempts++; } + if (txResult?.status !== "SUCCESS") { + throw new Error(`pause/resume tx failed: ${JSON.stringify(txResult)}`); + } }Also applies to: 751-761
🤖 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/services/streamStore.ts` around lines 693 - 703, The pause/resume flow currently proceeds to DB writes and webhooks after sending the Soroban transaction without confirming final on-chain success; update the logic around retryWithBackoff(()=>rpcServer!.sendTransaction(built)) and the follow-up polling of rpcServer!.getTransaction(sendRes.hash) so that you wait until the transaction's final status indicates success (e.g., "SUCCESS"/confirmed) before persisting local state or dispatching webhooks, and if the final status is failure or still NOT_FOUND after retries, abort the local DB update and return/throw an error (do the same for the analogous block using retryWithBackoff/rpcServer!.getTransaction at the other pause/resume location).package.json (1)
13-13:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeclare
conventional-changelog-cliin rootpackage.jsonand syncpackage-lock.json.
package.jsondefineschangelog:updateusingconventional-changelog, but rootdependencies/devDependenciesare empty.package-lock.json(lockfile v3) nonetheless includes rootpackages[""].devDependencies.conventional-changelog-cli, so a clean install/CI will either fail due to lockfile mismatch or won’t install the CLI—breakingnpm run changelog:update.Fix: add
conventional-changelog-clitodevDependenciesinpackage.jsonand regeneratepackage-lock.json.🤖 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 `@package.json` at line 13, The root package.json defines the npm script "changelog:update" that runs conventional-changelog but doesn't declare the dependency; add "conventional-changelog-cli" to devDependencies in package.json (matching the version used in the existing package-lock.json if possible) and then regenerate the lockfile (run npm install or npm install --package-lock-only) so package-lock.json and package.json stay in sync; ensure the added package name exactly matches "conventional-changelog-cli" and update the lockfile committed changes.frontend/src/hooks/useClaimStream.ts (1)
54-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent same-tick double claims from bypassing the in-flight guard.
Line 57 reads
claimState.statusfrom render state, so two back-to-backclaim()calls before rerender can both pass and callclaimStream(...), violating the single-flight contract.💡 Proposed fix
export function useClaimStream( @@ const claimIdRef = useRef(0); + const inFlightRef = useRef(false); @@ const claim = useCallback( async ({ streamId, recipientAddress, amount }: ClaimInput) => { - // Block concurrent claims - if (claimState.status === "pending") return; + // Block concurrent claims (same-tick safe) + if (inFlightRef.current) return; // Early return for zero amount to prevent unnecessary API calls if (amount === 0) return; + inFlightRef.current = true; const claimId = ++claimIdRef.current; @@ } catch (err) { @@ setClaimState({ streamId, status: "failed", error: message }); onFailure(streamId, message); + } finally { + if (claimIdRef.current === claimId) { + inFlightRef.current = false; + } } }, - [claimState.status, onSuccess, onFailure], + [onSuccess, onFailure], );Also applies to: 95-96
🤖 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/hooks/useClaimStream.ts` around lines 54 - 58, The guard reads claimState.status (render state) so two same-tick calls to claim() can both pass and call claimStream; fix by adding an in-flight mutable ref (e.g., inFlightRef via useRef(false)) and in claim (and the other function that currently checks claimState.status) do an atomic check-and-set: if (inFlightRef.current) return; inFlightRef.current = true; then try { await claimStream(...) } finally { inFlightRef.current = false; } also keep setClaimState updates for UI, but rely on the ref for the single-flight protection.
🧹 Nitpick comments (1)
frontend/src/components/StreamsTable.test.tsx (1)
18-21: ⚡ Quick winBulk-selection tests should include
pausedstreams.The component now treats
pausedas selectable, but the factory/type and eligibility test don’t cover that path.Proposed fix
const createMockStream = ( id: string, - status: 'active' | 'scheduled' | 'completed' | 'canceled' + status: 'active' | 'paused' | 'scheduled' | 'completed' | 'canceled' ): Stream => ({ @@ - it('renders checkboxes only for active and scheduled streams', () => { + it('renders checkboxes only for selectable streams (active, paused, scheduled)', () => { const streams = [ createMockStream('1', 'active'), + createMockStream('2', 'paused'), - createMockStream('2', 'scheduled'), - createMockStream('3', 'completed'), - createMockStream('4', 'canceled'), + createMockStream('3', 'scheduled'), + createMockStream('4', 'completed'), + createMockStream('5', 'canceled'), ]; @@ - // Should have 2 row checkboxes (active + scheduled) + 1 header checkbox + // Should have 3 row checkboxes (active + paused + scheduled) + 1 header checkbox const checkboxes = screen.getAllByRole('checkbox'); - expect(checkboxes).toHaveLength(3); + expect(checkboxes).toHaveLength(4); });Also applies to: 82-103
🤖 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/StreamsTable.test.tsx` around lines 18 - 21, The bulk-selection tests and the Stream factory omit the new 'paused' status so selectable-paused streams aren't covered; update the Stream type/factory used in StreamsTable.test.tsx (the createMockStream function and any Stream type definition it relies on) to accept 'paused' as a valid status, and add test cases in the bulk-selection suite (the tests around lines where createMockStream is used, previously 82-103) that create at least one stream with status 'paused' and assert it is treated as selectable by the bulk-selection logic in StreamsTable.
🤖 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/src/services/auth.ts`:
- Around line 168-176: In verifyChallengeAndIssueToken, replace the uppercase
error code "UNAUTHORIZED" with the lowercase "unauthorized" for both thrown
errors (the expired-challenge error and the challenge verification-failed error)
so the auth API uses consistent lowercase error codes; update the (err as
any).code assignments in those two throw paths to "unauthorized".
In `@backend/src/services/indexer.ts`:
- Around line 162-168: The current startup cursor logic uses a plain SELECT and
an INSERT that can race (duplicate id=1) and later UPDATEs that silently no-op
if the row is missing; replace the INSERT and any later UPDATEs with an
idempotent UPSERT so checkpoint writes are durable. Locate the code using
db.prepare on the indexer_cursor table (queries referencing "SELECT
last_ledger_sequence FROM indexer_cursor WHERE id = 1", the INSERT into
indexer_cursor, and any UPDATE ... WHERE id=1 around indexEvents / indexer
logic) and change those writes to a single UPSERT statement (INSERT ... ON
CONFLICT(id) DO UPDATE SET last_ledger_sequence = excluded.last_ledger_sequence)
so both initial insert and subsequent checkpoint updates are atomic and
race-safe; apply the same change to the other occurrence noted (around line
~198).
In `@backend/src/services/streamStore.ts`:
- Line 369: Summary: Pause time is being double-counted because resumeStream
both adds pausedDuration into durationSeconds and keeps pausedDuration, while
computeStatus/calculateProgress also add pausedDuration when checking
completion/progress. Fix: remove the double-count by choosing one source of
truth—prefer keeping pausedDuration separate and stop mutating durationSeconds
in resumeStream (remove the line that adds stream.pausedDuration to
stream.durationSeconds in resumeStream); ensure computeStatus and
calculateProgress continue using stream.pausedDuration together with
stream.durationSeconds and stream.startAt (references: resumeStream,
computeStatus, calculateProgress, stream.startAt, stream.durationSeconds,
stream.pausedDuration, variable at) and add/adjust unit tests to cover
resume/pause behavior so completion/vesting times are correct.
In `@frontend/src/components/StreamsTable.tsx`:
- Line 3: The bulk-cancel flow currently calls cancelStream directly and
bypasses the component's cancel contract; update the bulk cancel logic in
StreamsTable (the code around the individual cancel path that uses onCancel and
the bulk code at the block handling lines ~187-205) to invoke the same onCancel
callback for each stream (or to call a single onCancel that accepts multiple
ids) instead of calling cancelStream directly so parent-level side effects and
error handling are preserved; ensure you reuse the existing onCancel signature
(or add a bulk variant onCancelBulk) and remove direct cancelStream calls from
the bulk path so both single-row and bulk cancellations follow the same
contract.
- Around line 227-237: The current useEffect that calls setSelectedStreamIds
only retains ids that still exist in streams but doesn't remove ones whose
status became non-selectable (e.g., "completed" or "canceled"); update the
filter inside setSelectedStreamIds to also check each stream's status and only
keep ids for streams that both exist in streams and have a selectable status
(e.g., not "completed" or "canceled"). Locate the effect referencing useEffect
and setSelectedStreamIds and the streams array, and add a status check against
the stream object (stream.status) when building validIds so stale selections are
removed.
In `@frontend/src/services/api.ts`:
- Around line 297-300: getConfig currently treats the entire parsed response
body as AppConfig but parseResponse returns an envelope like { data: AppConfig
}, so change getConfig to call parseResponse<AppConfig>(response) and then
return the inner data (e.g. const body = await
parseResponse<AppConfig>(response); return body.data;) so callers receive the
actual AppConfig (including allowedAssets) rather than the envelope; update the
function signature/comments if needed to reflect that it returns AppConfig.
---
Outside diff comments:
In `@backend/src/services/streamStore.ts`:
- Around line 693-703: The pause/resume flow currently proceeds to DB writes and
webhooks after sending the Soroban transaction without confirming final on-chain
success; update the logic around
retryWithBackoff(()=>rpcServer!.sendTransaction(built)) and the follow-up
polling of rpcServer!.getTransaction(sendRes.hash) so that you wait until the
transaction's final status indicates success (e.g., "SUCCESS"/confirmed) before
persisting local state or dispatching webhooks, and if the final status is
failure or still NOT_FOUND after retries, abort the local DB update and
return/throw an error (do the same for the analogous block using
retryWithBackoff/rpcServer!.getTransaction at the other pause/resume location).
In `@backend/src/webhooks.integration.test.ts`:
- Around line 116-117: The test inserts rows into webhook_dead_letters for
stream_id "s1" and "s2" without creating the parent rows in the streams table,
causing foreign key violations; before the two db.prepare(...).run("s1", ...)
and db.prepare(...).run("s2", ...) calls, insert minimal parent rows into the
streams table (e.g. insert the required columns for ids "s1" and "s2") so the
foreign key constraint is satisfied, ensuring the webhook_dead_letters inserts
succeed.
- Around line 65-68: The test inserts into webhook_dead_letters with stream_id
"s1" but never creates the corresponding streams row, which can trigger a FK
violation; before the INSERT into webhook_dead_letters add a matching INSERT
into the streams table (e.g., create a mock stream with id "s1", stream type and
any required columns) similar to the pattern used in the requeue test, so the
webhook_dead_letters INSERT (in this test function) succeeds without violating
the streams -> webhook_dead_letters foreign key constraint.
- Around line 86-93: The test "should respect pagination (limit/offset)" inserts
rows into webhook_dead_letters with stream_ids s1..s5 but doesn't create
corresponding parent rows in the streams table, causing foreign key violations;
update the test (using getDb()) to insert matching entries into the streams
table for each stream id (e.g., s1..s5) before inserting into
webhook_dead_letters—follow the same pattern used in the requeue test to create
the parent stream rows so the foreign key constraint is satisfied.
In `@frontend/src/hooks/useClaimStream.ts`:
- Around line 54-58: The guard reads claimState.status (render state) so two
same-tick calls to claim() can both pass and call claimStream; fix by adding an
in-flight mutable ref (e.g., inFlightRef via useRef(false)) and in claim (and
the other function that currently checks claimState.status) do an atomic
check-and-set: if (inFlightRef.current) return; inFlightRef.current = true; then
try { await claimStream(...) } finally { inFlightRef.current = false; } also
keep setClaimState updates for UI, but rely on the ref for the single-flight
protection.
In `@package.json`:
- Line 13: The root package.json defines the npm script "changelog:update" that
runs conventional-changelog but doesn't declare the dependency; add
"conventional-changelog-cli" to devDependencies in package.json (matching the
version used in the existing package-lock.json if possible) and then regenerate
the lockfile (run npm install or npm install --package-lock-only) so
package-lock.json and package.json stay in sync; ensure the added package name
exactly matches "conventional-changelog-cli" and update the lockfile committed
changes.
---
Nitpick comments:
In `@frontend/src/components/StreamsTable.test.tsx`:
- Around line 18-21: The bulk-selection tests and the Stream factory omit the
new 'paused' status so selectable-paused streams aren't covered; update the
Stream type/factory used in StreamsTable.test.tsx (the createMockStream function
and any Stream type definition it relies on) to accept 'paused' as a valid
status, and add test cases in the bulk-selection suite (the tests around lines
where createMockStream is used, previously 82-103) that create at least one
stream with status 'paused' and assert it is treated as selectable by the
bulk-selection logic in StreamsTable.
🪄 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: f2883b61-d01b-4931-b3ae-f308ea0b9646
⛔ 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 (37)
backend/package.jsonbackend/src/auth.test.tsbackend/src/index.test.tsbackend/src/index.tsbackend/src/integration.test.tsbackend/src/services/auth.tsbackend/src/services/eventHistory.test.tsbackend/src/services/indexer.test.tsbackend/src/services/indexer.tsbackend/src/services/streamStore.cancel.integration.test.tsbackend/src/services/streamStore.progress.test.tsbackend/src/services/streamStore.tsbackend/src/services/streamStore.updateStartAt.test.tsbackend/src/services/webhook.test.tsbackend/src/services/webhookWorker.test.tsbackend/src/services/webhookWorker.tsbackend/src/webhooks.integration.test.tsfrontend/src/components/CopyableAddress.test.tsxfrontend/src/components/CopyableAddress.tsxfrontend/src/components/CreateStreamForm.tsxfrontend/src/components/FilterBar.test.tsxfrontend/src/components/RecipientDashboard.test.tsxfrontend/src/components/StreamDetailDrawer.test.tsxfrontend/src/components/StreamMetricsChart.test.tsxfrontend/src/components/StreamTimeline.filterbar.test.tsfrontend/src/components/StreamTimeline.test.tsxfrontend/src/components/StreamsTable.test.tsxfrontend/src/components/StreamsTable.tsxfrontend/src/components/WalletButton.test.tsxfrontend/src/hooks/useClaimStream.test.tsfrontend/src/hooks/useClaimStream.tsfrontend/src/hooks/useMetricsHistory.test.tsfrontend/src/hooks/useWebSocket.test.tsfrontend/src/hooks/useWebSocket.tsfrontend/src/services/api.tsfrontend/src/services/soroban.tspackage.json
Comment on lines
+168
to
+176
| const err = new Error("Challenge has expired. Please request a new one."); | ||
| (err as any).statusCode = 401; | ||
| (err as any).code = "UNAUTHORIZED"; | ||
| throw err; | ||
| } | ||
| throw new Error(`Challenge verification failed: ${error.message}`); | ||
| const err = new Error(`Challenge verification failed: ${error.message}`); | ||
| (err as any).statusCode = 401; | ||
| (err as any).code = "UNAUTHORIZED"; | ||
| throw err; |
There was a problem hiding this comment.
Use lowercase auth error codes for challenge failures.
verifyChallengeAndIssueToken now emits "UNAUTHORIZED" while the rest of auth paths use lowercase codes. This creates an inconsistent API contract (Line 170, Line 175).
Proposed fix
- (err as any).code = "UNAUTHORIZED";
+ (err as any).code = "unauthorized";
...
- (err as any).code = "UNAUTHORIZED";
+ (err as any).code = "unauthorized";📝 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
| const err = new Error("Challenge has expired. Please request a new one."); | |
| (err as any).statusCode = 401; | |
| (err as any).code = "UNAUTHORIZED"; | |
| throw err; | |
| } | |
| throw new Error(`Challenge verification failed: ${error.message}`); | |
| const err = new Error(`Challenge verification failed: ${error.message}`); | |
| (err as any).statusCode = 401; | |
| (err as any).code = "UNAUTHORIZED"; | |
| throw err; | |
| const err = new Error("Challenge has expired. Please request a new one."); | |
| (err as any).statusCode = 401; | |
| (err as any).code = "unauthorized"; | |
| throw err; | |
| } | |
| const err = new Error(`Challenge verification failed: ${error.message}`); | |
| (err as any).statusCode = 401; | |
| (err as any).code = "unauthorized"; | |
| throw err; |
🤖 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/services/auth.ts` around lines 168 - 176, In
verifyChallengeAndIssueToken, replace the uppercase error code "UNAUTHORIZED"
with the lowercase "unauthorized" for both thrown errors (the expired-challenge
error and the challenge verification-failed error) so the auth API uses
consistent lowercase error codes; update the (err as any).code assignments in
those two throw paths to "unauthorized".
Comment on lines
+162
to
168
| const cursor = db.prepare("SELECT last_ledger_sequence FROM indexer_cursor WHERE id = 1").get() as any; | ||
| if (cursor) { | ||
| lastProcessedLedger = cursor.last_ledger_sequence; | ||
| } else { | ||
| lastProcessedLedger = indexerStartLedger !== null ? indexerStartLedger : currentLedger - 1; | ||
| db.prepare("INSERT INTO indexer_cursor (id, last_ledger_sequence) VALUES (1, ?)").run(lastProcessedLedger); | ||
| } |
There was a problem hiding this comment.
Make cursor writes idempotent to avoid startup race failures and silent checkpoint loss.
indexEvents() can overlap on startup, so the first-run INSERT can throw on duplicate id=1. Also, UPDATE ... WHERE id=1 silently does nothing if the row is missing. Use UPSERT for both paths so checkpointing stays durable.
Suggested fix
- const cursor = db.prepare("SELECT last_ledger_sequence FROM indexer_cursor WHERE id = 1").get() as any;
+ const cursor = db.prepare("SELECT last_ledger_sequence FROM indexer_cursor WHERE id = 1").get() as any;
if (cursor) {
lastProcessedLedger = cursor.last_ledger_sequence;
} else {
lastProcessedLedger = indexerStartLedger !== null ? indexerStartLedger : currentLedger - 1;
- db.prepare("INSERT INTO indexer_cursor (id, last_ledger_sequence) VALUES (1, ?)").run(lastProcessedLedger);
+ db.prepare(`
+ INSERT INTO indexer_cursor (id, last_ledger_sequence)
+ VALUES (1, ?)
+ ON CONFLICT(id) DO UPDATE SET last_ledger_sequence = excluded.last_ledger_sequence
+ `).run(lastProcessedLedger);
}
@@
- db.prepare("UPDATE indexer_cursor SET last_ledger_sequence = ? WHERE id = 1").run(currentLedger);
+ db.prepare(`
+ INSERT INTO indexer_cursor (id, last_ledger_sequence)
+ VALUES (1, ?)
+ ON CONFLICT(id) DO UPDATE SET last_ledger_sequence = excluded.last_ledger_sequence
+ `).run(currentLedger);Also applies to: 198-198
🤖 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/services/indexer.ts` around lines 162 - 168, The current startup
cursor logic uses a plain SELECT and an INSERT that can race (duplicate id=1)
and later UPDATEs that silently no-op if the row is missing; replace the INSERT
and any later UPDATEs with an idempotent UPSERT so checkpoint writes are
durable. Locate the code using db.prepare on the indexer_cursor table (queries
referencing "SELECT last_ledger_sequence FROM indexer_cursor WHERE id = 1", the
INSERT into indexer_cursor, and any UPDATE ... WHERE id=1 around indexEvents /
indexer logic) and change those writes to a single UPSERT statement (INSERT ...
ON CONFLICT(id) DO UPDATE SET last_ledger_sequence =
excluded.last_ledger_sequence) so both initial insert and subsequent checkpoint
updates are atomic and race-safe; apply the same change to the other occurrence
noted (around line ~198).
| return "scheduled"; | ||
| } | ||
| if (at >= stream.startAt + stream.durationSeconds) { | ||
| if (at >= stream.startAt + stream.durationSeconds + stream.pausedDuration) { |
There was a problem hiding this comment.
Pause time is double-counted after resume.
resumeStream adds elapsed pause to durationSeconds (Line 768) and keeps accumulating pausedDuration (Line 766), while computeStatus/calculateProgress still apply pausedDuration again (Line 369, Line 398, Line 401). This stretches vesting/completion longer than intended.
Also applies to: 390-401, 766-769
🤖 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/services/streamStore.ts` at line 369, Summary: Pause time is
being double-counted because resumeStream both adds pausedDuration into
durationSeconds and keeps pausedDuration, while computeStatus/calculateProgress
also add pausedDuration when checking completion/progress. Fix: remove the
double-count by choosing one source of truth—prefer keeping pausedDuration
separate and stop mutating durationSeconds in resumeStream (remove the line that
adds stream.pausedDuration to stream.durationSeconds in resumeStream); ensure
computeStatus and calculateProgress continue using stream.pausedDuration
together with stream.durationSeconds and stream.startAt (references:
resumeStream, computeStatus, calculateProgress, stream.startAt,
stream.durationSeconds, stream.pausedDuration, variable at) and add/adjust unit
tests to cover resume/pause behavior so completion/vesting times are correct.
|
|
||
| import { useState, useMemo, useRef, useEffect, type RefObject } from "react"; | ||
| import { Stream } from "../types/stream"; | ||
| import { getExportCsvUrl, ListStreamsFilters, cancelStream } from "../services/api"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Bulk cancel bypasses the component’s cancel contract.
Bulk flow calls cancelStream directly instead of onCancel, so it can skip parent-level side effects/error handling and diverge from single-row cancellation behavior.
Proposed fix
-import { getExportCsvUrl, ListStreamsFilters, cancelStream } from "../services/api";
+import { getExportCsvUrl, ListStreamsFilters } from "../services/api";
@@
- await cancelStream(streamId);
+ await onCancel(streamId);Also applies to: 187-205
🤖 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/StreamsTable.tsx` at line 3, The bulk-cancel flow
currently calls cancelStream directly and bypasses the component's cancel
contract; update the bulk cancel logic in StreamsTable (the code around the
individual cancel path that uses onCancel and the bulk code at the block
handling lines ~187-205) to invoke the same onCancel callback for each stream
(or to call a single onCancel that accepts multiple ids) instead of calling
cancelStream directly so parent-level side effects and error handling are
preserved; ensure you reuse the existing onCancel signature (or add a bulk
variant onCancelBulk) and remove direct cancelStream calls from the bulk path so
both single-row and bulk cancellations follow the same contract.
Comment on lines
+227
to
+237
| // Clear selections when streams change (e.g., after filter change) | ||
| useEffect(() => { | ||
| setSelectedStreamIds((prev) => { | ||
| const validIds = new Set(streams.map((s) => s.id)); | ||
| const next = new Set<string>(); | ||
| prev.forEach((id) => { | ||
| if (validIds.has(id)) next.add(id); | ||
| }); | ||
| return next; | ||
| }); | ||
| }, [streams]); |
There was a problem hiding this comment.
Selection cleanup should also drop no-longer-selectable streams.
Current cleanup only checks ID existence. If a selected stream transitions to completed/canceled, it can remain selected with no row checkbox, leaving stale bulk-selection state.
Proposed fix
useEffect(() => {
setSelectedStreamIds((prev) => {
- const validIds = new Set(streams.map((s) => s.id));
+ const validIds = new Set(
+ streams.filter(isStreamSelectable).map((s) => s.id)
+ );
const next = new Set<string>();
prev.forEach((id) => {
if (validIds.has(id)) next.add(id);
});
return next;
});
}, [streams]);🤖 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/StreamsTable.tsx` around lines 227 - 237, The current
useEffect that calls setSelectedStreamIds only retains ids that still exist in
streams but doesn't remove ones whose status became non-selectable (e.g.,
"completed" or "canceled"); update the filter inside setSelectedStreamIds to
also check each stream's status and only keep ids for streams that both exist in
streams and have a selectable status (e.g., not "completed" or "canceled").
Locate the effect referencing useEffect and setSelectedStreamIds and the streams
array, and add a status check against the stream object (stream.status) when
building validIds so stale selections are removed.
Comment on lines
+297
to
+300
| export async function getConfig(): Promise<AppConfig> { | ||
| const response = await fetch(`${API_BASE}/config`); | ||
| return parseResponse<AppConfig>(response); | ||
| } |
There was a problem hiding this comment.
getConfig() unwraps the wrong payload shape.
parseResponse returns the full response body; this currently returns { data: ... } as AppConfig, so allowedAssets can be undefined at runtime for callers expecting AppConfig directly.
Proposed fix
export async function getConfig(): Promise<AppConfig> {
const response = await fetch(`${API_BASE}/config`);
- return parseResponse<AppConfig>(response);
+ const body = await parseResponse<{ data: AppConfig }>(response);
+ return body.data;
}📝 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
| export async function getConfig(): Promise<AppConfig> { | |
| const response = await fetch(`${API_BASE}/config`); | |
| return parseResponse<AppConfig>(response); | |
| } | |
| export async function getConfig(): Promise<AppConfig> { | |
| const response = await fetch(`${API_BASE}/config`); | |
| const body = await parseResponse<{ data: AppConfig }>(response); | |
| return body.data; | |
| } |
🤖 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/api.ts` around lines 297 - 300, getConfig currently
treats the entire parsed response body as AppConfig but parseResponse returns an
envelope like { data: AppConfig }, so change getConfig to call
parseResponse<AppConfig>(response) and then return the inner data (e.g. const
body = await parseResponse<AppConfig>(response); return body.data;) so callers
receive the actual AppConfig (including allowedAssets) rather than the envelope;
update the function signature/comments if needed to reflect that it returns
AppConfig.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR resolves the test suite failures across both the backend and frontend components:
Backend
indexer.tsandwebhookWorker.tsduring database connection and startup errors.stream_events) before deleting parent entries (streams).statusCode(401) and error payload.auth.test.tsto align with actual error messages and status codes.Frontend
useWebSockettests to prevent network connection errors.api.tsand mock contract configurations for Soroban.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests