test(statsbar): cover 30s auto-refresh and guard against overlapping …#243
Merged
ritik4ever merged 2 commits intoJun 29, 2026
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthrough
StatsBar Auto-Refresh
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 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 `@frontend/components/StatsBar.tsx`:
- Around line 3-27: The in-flight guard in StatsBar is persisting across React
Strict Mode’s mount/replay cycle, which causes the initial fetch to be skipped.
Move the guard off the component-level ref and into the useEffect/load scope in
StatsBar so each effect run starts cleanly, while still preventing overlapping
refreshes within that effect; keep the fetchStats call and active-state check in
load, and ensure the guard is reset during cleanup or completion without relying
on a ref that survives the replay.
🪄 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 Plus
Run ID: b46fec71-2fa2-4d3f-966a-1e40bd7e4881
📒 Files selected for processing (2)
frontend/__tests__/StatsBar.test.tsxfrontend/components/StatsBar.tsx
Contributor
Author
|
Good catch from the CodeRabbit review on the StrictMode replay. The useRef guard stayed set across the mount/cleanup/mount cycle, so the second pass skipped the initial fetch and the bar stayed empty until the first 30s tick. Fixed in 3a790c7 by moving the guard to a local flag inside the effect, so each setup starts clean while overlapping interval fetches are still skipped. Tests still green (4/4). |
Collaborator
|
@PratikshaGayen please fix ci |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/src/routes/agents.js (2)
554-556: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLog policy update failures.
Neighboring routes log
errandaddressbeforehandleContractError; this catch drops the actionable context.Proposed fix
} catch (err) { - + logger.error({ err, address }, 'POST /agents/:address/update-policy failed'); return handleContractError(err, res, 'Policy update failed', 'POLICY_ERROR'); }🤖 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/routes/agents.js` around lines 554 - 556, The policy update catch block in the route handling logic drops the useful debugging context by going straight to handleContractError; update the catch in the agents route to log the caught err along with the relevant address before returning. Follow the same pattern used by neighboring route handlers in this file, and keep the existing handleContractError call with the Policy update failed / POLICY_ERROR arguments after the log.
535-551: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate policy values before passing them on-chain.
This accepts values like
abc,1.5, negative stroops,NaN, or non-string categories. Reject non-integer/non-negative stroops, normalize category strings, and boundminScoreToEarnbefore callingupdatePolicyOnChain.🤖 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/routes/agents.js` around lines 535 - 551, The validation in the policy update route is too permissive and currently lets invalid stroop values and malformed categories reach updatePolicyOnChain. Tighten the checks in the request handler in agents.js so maxPerTxStroops and maxPerDayStroops only accept non-negative integers (reject abc, 1.5, negatives, NaN), ensure allowedCategories is an array of strings and normalize/trim each entry before use, and constrain minScoreToEarn to an acceptable numeric range before forwarding the values to updatePolicyOnChain.
🤖 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 `@agent/agent.js`:
- Around line 253-273: The spend policy checks in agent should not be gated by
scoringEnabled, since that allows paid requests to proceed when
ensureRegistered() fails. Move the MAX_PER_TX, MAX_PER_DAY, and category limit
enforcement into the request path before any payment attempt, and keep
scoringEnabled only for score/outcome recording and related logging. Use the
existing checkSpend, logger.warn/logger.info, and selected/category/serviceId
flow in agent.js to locate and separate the enforcement logic.
- Around line 23-29: Persist the env-driven policy values from AGENT_NAME,
AGENT_DESC, MAX_PER_TX, MAX_PER_DAY, and ALLOWED_CATS during agent setup instead
of only relying on the defaults in agent/agent.js. Update the registration flow
so the backend register route or the post-registration step includes the policy
fields alongside agentAddress, name, and description, or performs an
authenticated policy update immediately after registration. Make sure the
relevant registration handler and any client payload builder use the existing
symbols MAX_PER_TX, MAX_PER_DAY, and ALLOWED_CATS so fresh agents receive the
intended policy values.
- Around line 296-332: The outcome recording calls in agent.js are missing the
required idempotency header, so recordOutcome() posts to
/api/agents/:address/payment are rejected. Update recordOutcome() to generate
and send a stable X-Idempotency-Key for each success/failure submission, and
make sure the calls from the payment flow after selected.id is processed reuse
that key for the same outcome attempt so the server accepts the request.
In `@backend/src/routes/agents.js`:
- Around line 532-533: The direct /agents/:address/update-policy route is
relying on ownerAuth’s spoofable x-caller-address header before calling
updatePolicyOnChain, which allows forged ownership claims. Update the route in
agents.js to require cryptographic verification of the caller or route it
through the existing wallet-signed build/submit flow instead of server-side
signing. Use the existing ownerAuth, updatePolicyOnChain, and any build/submit
policy mutation path symbols to keep the fix aligned with the authenticated
flow.
In `@frontend/app/agents/page.tsx`:
- Line 19: Restore the shared PAGE_SIZE export used by AgentsPage and its test
import. The constant in the agents page module is currently file-local, which
breaks frontend/__tests__/AgentsPage.test.tsx at import time, so either
re-export PAGE_SIZE from frontend/app/agents/page.tsx or move it into a shared
module and update both the page logic and the test to import the same symbol.
---
Outside diff comments:
In `@backend/src/routes/agents.js`:
- Around line 554-556: The policy update catch block in the route handling logic
drops the useful debugging context by going straight to handleContractError;
update the catch in the agents route to log the caught err along with the
relevant address before returning. Follow the same pattern used by neighboring
route handlers in this file, and keep the existing handleContractError call with
the Policy update failed / POLICY_ERROR arguments after the log.
- Around line 535-551: The validation in the policy update route is too
permissive and currently lets invalid stroop values and malformed categories
reach updatePolicyOnChain. Tighten the checks in the request handler in
agents.js so maxPerTxStroops and maxPerDayStroops only accept non-negative
integers (reject abc, 1.5, negatives, NaN), ensure allowedCategories is an array
of strings and normalize/trim each entry before use, and constrain
minScoreToEarn to an acceptable numeric range before forwarding the values to
updatePolicyOnChain.
🪄 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 Plus
Run ID: ddaeabec-f207-4892-946f-74b62e03d496
📒 Files selected for processing (16)
agent/agent.jsbackend/src/lib/contract.jsbackend/src/lib/stellar.jsbackend/src/routes/agents.jsbackend/test/agents.test.jsfrontend/__tests__/RegistryPage.test.tsxfrontend/__tests__/ScoreBadge.test.tsxfrontend/__tests__/WalletPickerModal.test.tsxfrontend/__tests__/wallet.test.tsfrontend/app/agents/page.tsxfrontend/app/registry/page.tsxfrontend/custom.d.tsfrontend/lib/registry.test.tsfrontend/lib/sort.test.tsfrontend/lib/sort.tsfrontend/tsconfig.json
💤 Files with no reviewable changes (2)
- backend/test/agents.test.js
- frontend/lib/registry.test.ts
✅ Files skipped from review due to trivial changes (7)
- frontend/custom.d.ts
- frontend/tests/ScoreBadge.test.tsx
- frontend/tests/WalletPickerModal.test.tsx
- frontend/tsconfig.json
- backend/src/lib/stellar.js
- frontend/lib/sort.test.ts
- backend/src/lib/contract.js
Comment on lines
+23
to
+29
| const AGENT_NAME = process.env.AGENT_NAME ?? 'LodestarAgent'; | ||
| const AGENT_DESC = process.env.AGENT_DESC ?? 'An autonomous Lodestar agent'; | ||
| const MAX_PER_TX = parseFloat(process.env.MAX_PER_TX_USDC ?? '1.00'); | ||
| const MAX_PER_DAY = parseFloat(process.env.MAX_PER_DAY_USDC ?? '10.00'); | ||
| const ALLOWED_CATS = (process.env.ALLOWED_CATEGORIES ?? 'weather,search,data').split(','); | ||
|
|
||
| const agentKeypair = Keypair.fromSecret(AGENT_SECRET); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Persist the env-driven policy values.
MAX_PER_TX, MAX_PER_DAY, and ALLOWED_CATS are posted during registration, but the backend register route only reads agentAddress, name, and description; fresh agents will keep the default on-chain policy. Wire these fields into registration or apply an authenticated policy update after registration.
🤖 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 `@agent/agent.js` around lines 23 - 29, Persist the env-driven policy values
from AGENT_NAME, AGENT_DESC, MAX_PER_TX, MAX_PER_DAY, and ALLOWED_CATS during
agent setup instead of only relying on the defaults in agent/agent.js. Update
the registration flow so the backend register route or the post-registration
step includes the policy fields alongside agentAddress, name, and description,
or performs an authenticated policy update immediately after registration. Make
sure the relevant registration handler and any client payload builder use the
existing symbols MAX_PER_TX, MAX_PER_DAY, and ALLOWED_CATS so fresh agents
receive the intended policy values.
Comment on lines
253
to
273
| if (scoringEnabled) { | ||
| const spendCheck = await checkSpend(selected.price_usdc, category); | ||
| if (!spendCheck.allowed) { | ||
| logger.warn( | ||
| { | ||
| event: EVENT.SPEND_CHECK_BLOCKED, | ||
| category, | ||
| serviceId: selected.id, | ||
| priceUsdc: selected.price_usdc, | ||
| reason: spendCheck.reason, | ||
| }, | ||
| 'Spend blocked by policy' | ||
| ); | ||
| failed.add(selected.id); | ||
| continue; | ||
| } | ||
| logger.info( | ||
| { event: EVENT.SPEND_CHECK_PASSED, category, serviceId: selected.id }, | ||
| 'Spend check passed' | ||
| ); | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not tie spend enforcement to scoring availability.
If ensureRegistered() returns false, this skips policy checks but still signs paid requests. Enforce local MAX_PER_TX, MAX_PER_DAY, and category limits before payment attempts; use scoringEnabled only for score/outcome recording.
🤖 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 `@agent/agent.js` around lines 253 - 273, The spend policy checks in agent
should not be gated by scoringEnabled, since that allows paid requests to
proceed when ensureRegistered() fails. Move the MAX_PER_TX, MAX_PER_DAY, and
category limit enforcement into the request path before any payment attempt, and
keep scoringEnabled only for score/outcome recording and related logging. Use
the existing checkSpend, logger.warn/logger.info, and
selected/category/serviceId flow in agent.js to locate and separate the
enforcement logic.
Comment on lines
+296
to
+332
| if (scoringEnabled) await recordOutcome(selected.price_usdc, true, selected.id); | ||
| await submitReputation(selected.id, true); | ||
| logger.info( | ||
| { | ||
| event: EVENT.PAYMENT_SUCCESS, | ||
| category, | ||
| serviceId: selected.id, | ||
| serviceName: selected.name, | ||
| priceUsdc: selected.price_usdc, | ||
| txHash, | ||
| taskDurationMs, | ||
| }, | ||
| 'Payment successful' | ||
| ); | ||
| return { success: true, priceUsdc: selected.price_usdc }; | ||
| } | ||
|
|
||
| return { success: true, priceUsdc: selected.price_usdc }; | ||
| // Non-2xx response — service endpoint rejected the request | ||
| logger.error( | ||
| { | ||
| event: EVENT.PAYMENT_FAILED, | ||
| category, | ||
| serviceId: selected.id, | ||
| httpStatus: response.status, | ||
| }, | ||
| 'Service returned non-2xx — trying next' | ||
| ); | ||
| if (scoringEnabled) await recordOutcome(selected.price_usdc, false, selected.id); | ||
| await submitReputation(selected.id, false); | ||
| failed.add(selected.id); | ||
| } catch (err) { | ||
| // Network-level failure before payment completed — no reputation penalty | ||
| logger.error( | ||
| { event: EVENT.PAYMENT_FAILED, category, serviceId: selected.id, err }, | ||
| 'Fetch threw — trying next service' | ||
| ); | ||
| if (scoringEnabled) await recordOutcome(selected.price_usdc, false, selected.id); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Send an idempotency key for outcome recording.
recordOutcome() posts to /api/agents/:address/payment, whose route rejects requests without X-Idempotency-Key. These success/failure calls therefore won’t update score or spend state.
Proposed fix
-async function recordOutcome(amountUsdc, success, serviceId) {
+async function recordOutcome(amountUsdc, success, serviceId, idempotencyKey = crypto.randomUUID()) {
try {
const body = JSON.stringify({ amountUsdc, success, serviceId });
- const headers = { 'Content-Type': 'application/json' };
+ const headers = {
+ 'Content-Type': 'application/json',
+ 'X-Idempotency-Key': idempotencyKey,
+ };🤖 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 `@agent/agent.js` around lines 296 - 332, The outcome recording calls in
agent.js are missing the required idempotency header, so recordOutcome() posts
to /api/agents/:address/payment are rejected. Update recordOutcome() to generate
and send a stable X-Idempotency-Key for each success/failure submission, and
make sure the calls from the payment flow after selected.id is processed reuse
that key for the same outcome attempt so the server accepts the request.
Comment on lines
+532
to
+533
| router.post('/agents/:address/update-policy', requireAgentsContract, ownerAuth, async (req, res) => { | ||
| const { address } = req.params; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Do not expose server-signed policy mutation behind a spoofable header.
ownerAuth only checks x-caller-address; this direct route then calls server-side updatePolicyOnChain(...). A client can forge the owner header unless wallet signature/challenge verification is added. Prefer the existing build/submit wallet-signed flow or require cryptographic proof before server-side signing.
🤖 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/routes/agents.js` around lines 532 - 533, The direct
/agents/:address/update-policy route is relying on ownerAuth’s spoofable
x-caller-address header before calling updatePolicyOnChain, which allows forged
ownership claims. Update the route in agents.js to require cryptographic
verification of the caller or route it through the existing wallet-signed
build/submit flow instead of server-side signing. Use the existing ownerAuth,
updatePolicyOnChain, and any build/submit policy mutation path symbols to keep
the fix aligned with the authenticated flow.
Collaborator
|
@PratikshaGayen please resolve conflicts |
…fetches StatsBar already refreshes stats every 30s, but the behavior had no test coverage and a slow fetchStats() could overlap the next interval tick. - Add an in-flight guard so a refresh slower than the interval is skipped rather than stacking another request, and avoid setState after unmount. - Add fake-timer tests covering the mount fetch, the 30s re-fetch and re-render, the in-flight guard, and interval cleanup on unmount.
Keeping the guard in a useRef meant it stayed set across StrictMode's mount/cleanup/mount replay, so the second pass saw it still pending and skipped the initial fetch, leaving the bar empty until the first 30s tick. A local flag inside the effect resets on every setup and still prevents overlapping interval fetches.
PratikshaGayen
force-pushed
the
feat/statsbar-refresh-test-guard
branch
from
cf2153e to
2964722
Compare
Contributor
Author
|
sorry for the churn on this one. rebased onto latest main so it merges clean now, and i trimmed the PR back to just the statsbar test + the in-flight guard since that's what #72 actually needs. the extra cross-package changes i'd pushed earlier were out of scope so i dropped them. checks are green now too. |
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.
StatsBar already does the 30s setInterval refresh this issue asks for (load on mount + setInterval(load, 30_000) cleared on unmount), which I flagged on the issue. What was missing was test coverage for it and a guard so a slow fetchStats() can't overlap the next tick.
Two changes here:
The component now keeps an in-flight ref so a refresh that's still pending when the next 30s tick fires is skipped instead of stacking a second overlapping request, and it no longer calls setState after the component unmounts.
Added fake-timer tests covering the fetch on mount, the 30s re-fetch and re-render with updated totals, the in-flight guard, and interval cleanup on unmount.
No change to the existing fetch/error handling or the rendered output.
Closes #72
One note on CI: the frontend type-check/test job is already failing on main before this change (pre-existing tsc errors in lib/sort.ts and a couple of test files, plus a few unrelated failing suites). This PR doesn't touch any of that. The new StatsBar test passes locally (4/4) and the diff is limited to StatsBar.tsx plus the new test file.
Summary by CodeRabbit