Fix core-state poll logs and rewards timeout UX#1321
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRate-limit bootstrap/poll warnings to specific retry checkpoints and add a 15s timeout plus structured error mapping for the rewards API; tests updated to assert warning checkpoints and timeout behavior. ChangesBootstrap Warning Rate-Limiting
Rewards API Timeout Handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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. 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 |
|
Tested locally on a merge of Verdict: works as intended
Repro for the timeout path: monkey-patched Action item before mergeBranch needs (Tried to push the merge commit on your behalf via maintainer-edits but lacked write access to your fork — leaving it to you.) |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/src/providers/__tests__/CoreStateProvider.test.tsx (1)
222-228: ⚡ Quick winConsider extending test to cover attempt 15 for completeness.
The test correctly validates checkpoints [1, 5, 10] within a 12-attempt sequence. Per the PR comment from oxoxDev, warnings also fire at attempt 15. Extending the test range to
{ length: 16 }and expecting[1, 5, 10, 15]would provide stronger validation of the "every 5" recurring pattern.📋 Proposed extension
- it('rate-limits repeated bootstrap poll warnings to useful checkpoints', () => { - const warnedAttempts = Array.from({ length: 12 }, (_, index) => index + 1).filter( - shouldWarnForBootstrapFailure - ); - - expect(warnedAttempts).toEqual([1, 5, 10]); - }); + it('rate-limits repeated bootstrap poll warnings to useful checkpoints', () => { + const warnedAttempts = Array.from({ length: 16 }, (_, index) => index + 1).filter( + shouldWarnForBootstrapFailure + ); + + expect(warnedAttempts).toEqual([1, 5, 10, 15]); + });🤖 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 `@app/src/providers/__tests__/CoreStateProvider.test.tsx` around lines 222 - 228, Update the test that computes warnedAttempts to include attempt 15: change the Array.from length from 12 to 16 in the test inside CoreStateProvider.test.tsx and update the expected array in the expect(...) assertion to [1, 5, 10, 15]; this ensures the shouldWarnForBootstrapFailure logic (the function under test) is validated for the recurring "every 5" checkpoint including attempt 15.app/src/services/api/rewardsApi.ts (2)
41-46: ⚖️ Poor tradeoffConsider centralizing error extraction utilities.
The
errorTexthelper is similar tosanitizeErrorinCoreStateProvider.tsx(lines 57-70). While both serve slightly different purposes, extracting error messages is a common pattern that could benefit from a shared utility inapp/src/utils/. This is not blocking, but worth considering for future refactoring to reduce duplication.🤖 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 `@app/src/services/api/rewardsApi.ts` around lines 41 - 46, The helper function errorText duplicates logic already in sanitizeError; extract the common error-extraction logic into a new shared utility (e.g., create a function like extractErrorMessage in app/src/utils/) and replace the local errorText and usages of sanitizeError to call that utility; update the exported names (keep errorText and sanitizeError wrappers if needed) so callers (including the function errorText in rewardsApi.ts and sanitizeError in CoreStateProvider.tsx) import the single implementation to avoid duplication.
131-148: ⚡ Quick winTimeout implementation is correct; consider using namespaced debug logger.
The timeout and error handling logic correctly implements the PR objectives: 15s timeout, user-friendly timeout message, and backend error preservation. However, per coding guidelines for
app/src/**/*.ts, frontend logging should use namespaceddebug(from thedebugpackage) rather thanconsole.debug. Other files in this PR (e.g.,CoreStateProvider.tsxline 43) follow this pattern withdebugFactory('core-state').♻️ Proposed refactor to use namespaced debug
Add the import at the top:
+import debugFactory from 'debug'; + import type { ApiResponse } from '../../types/api'; import type { RewardsAchievement, RewardsSnapshot } from '../../types/rewards'; import { apiClient } from '../apiClient'; +const log = debugFactory('rewards-api'); + const REWARDS_REQUEST_TIMEOUT_MS = 15_000;Then update the debug calls:
} catch (error) { const message = isTimeoutError(error) ? REWARDS_TIMEOUT_MESSAGE : errorText(error) || 'Unable to load rewards'; - console.debug('[rewards] backend snapshot unavailable', { message }); + log('backend snapshot unavailable message=%s', message); throw { success: false, error: message }; } - console.debug('[rewards] loaded backend snapshot', { - achievementCount: Array.isArray((response.data as { achievements?: unknown[] })?.achievements) - ? (response.data as { achievements: unknown[] }).achievements.length - : 0, - }); + const achievementCount = Array.isArray((response.data as { achievements?: unknown[] })?.achievements) + ? (response.data as { achievements: unknown[] }).achievements.length + : 0; + log('loaded backend snapshot achievementCount=%d', achievementCount); return normalizeRewardsSnapshot(response.data);As per coding guidelines: "Use namespaced
debugfor frontend logging with dev-only detail; avoid logging secrets or full PII."🤖 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 `@app/src/services/api/rewardsApi.ts` around lines 131 - 148, Replace the console.debug call with the project’s namespaced debug logger: add an import for the debug factory and create a module-scoped logger (e.g., const debug = debugFactory('rewards-api')) at the top of this module, then change the console.debug('[rewards] backend snapshot unavailable', { message }) call inside the catch to use debug (e.g., debug('backend snapshot unavailable %O', { message })) so logging follows the frontend namespaced debug pattern used elsewhere like CoreStateProvider.
🤖 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.
Nitpick comments:
In `@app/src/providers/__tests__/CoreStateProvider.test.tsx`:
- Around line 222-228: Update the test that computes warnedAttempts to include
attempt 15: change the Array.from length from 12 to 16 in the test inside
CoreStateProvider.test.tsx and update the expected array in the expect(...)
assertion to [1, 5, 10, 15]; this ensures the shouldWarnForBootstrapFailure
logic (the function under test) is validated for the recurring "every 5"
checkpoint including attempt 15.
In `@app/src/services/api/rewardsApi.ts`:
- Around line 41-46: The helper function errorText duplicates logic already in
sanitizeError; extract the common error-extraction logic into a new shared
utility (e.g., create a function like extractErrorMessage in app/src/utils/) and
replace the local errorText and usages of sanitizeError to call that utility;
update the exported names (keep errorText and sanitizeError wrappers if needed)
so callers (including the function errorText in rewardsApi.ts and sanitizeError
in CoreStateProvider.tsx) import the single implementation to avoid duplication.
- Around line 131-148: Replace the console.debug call with the project’s
namespaced debug logger: add an import for the debug factory and create a
module-scoped logger (e.g., const debug = debugFactory('rewards-api')) at the
top of this module, then change the console.debug('[rewards] backend snapshot
unavailable', { message }) call inside the catch to use debug (e.g.,
debug('backend snapshot unavailable %O', { message })) so logging follows the
frontend namespaced debug pattern used elsewhere like CoreStateProvider.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 153f0bef-eceb-4b84-8d13-a6daa2c8f188
📒 Files selected for processing (4)
app/src/providers/CoreStateProvider.tsxapp/src/providers/__tests__/CoreStateProvider.test.tsxapp/src/services/api/__tests__/rewardsApi.test.tsapp/src/services/api/rewardsApi.ts
… and extended checkpoint test
- Replace console.debug calls in rewardsApi.ts with debugFactory('rewards-api')
logger per frontend logging guidelines (addresses @coderabbitai on rewardsApi.ts:131-148)
- Extend shouldWarnForBootstrapFailure test from length 12 to 16, expect [1,5,10,15]
to validate the recurring every-5 pattern including attempt 15
(addresses @coderabbitai on CoreStateProvider.test.tsx:222-228)
- Drop misleading /${MAX_BOOTSTRAP_RETRIES} denominator from rate-limited warn message
(addresses @senamakel review)
4 participants
Summary
debug('core-state')failure diagnostics for development without console warning spam./rewards/merequest timeout and normalize timeout-style failures into an actionable retry message.Problem
[core-state] poll failedwarnings while the core state endpoint is unavailable./rewards/metimeout-style failures can surface as low-level network/client errors instead of a quiet, recoverable Rewards state.Solution
shouldWarnForBootstrapFailure()and use it to warn on attempts 1, 5, then every 5 failures thereafter while debug logs still include every failed attempt.rewardsApi.getMyRewards()request/failure handling to apply a 15s timeout and map timeout/abort/ERR_TIMED_OUTmessages to: “Rewards request timed out. Please check your connection and try again.”Submission Checklist
docs/TESTING-STRATEGY.md## Related— N/A: no feature ID row changed.docs/TESTING-STRATEGY.md)Closes #NNNin the## RelatedsectionImpact
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
codex/SYM-209-core-state-rewards-timeoutf113824af973abb8977bbd92cd8ae45ed69da3bbValidation Run
pnpm --filter openhuman-app format:check(via pre-push hook; passed)pnpm typecheck/pnpm --filter openhuman-app compile(passed)pnpm --dir app exec vitest run src/providers/__tests__/CoreStateProvider.test.tsx src/services/api/__tests__/rewardsApi.test.ts --config test/vitest.config.ts(13 passed)cargo fmt --manifest-path ../Cargo.toml --all --check(via pre-push hook; passed);cargo check --manifest-path src-tauri/Cargo.toml(via pre-push hook; passed with pre-existing warnings)cargo fmt --manifest-path src-tauri/Cargo.toml --all --checkandcargo check --manifest-path src-tauri/Cargo.toml(via pre-push hook; passed with pre-existing warnings)Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
/rewards/metimes out.Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit
Bug Fixes
Tests