fix(config): log RPC URL and core mode as strings, not object wrappers#2459
fix(config): log RPC URL and core mode as strings, not object wrappers#2459M3gA-Mind wants to merge 4 commits into
Conversation
storeRpcUrl logged { url: '...' } and storeCoreMode logged { mode }
causing "[object Object]" in diagnostic output and hiding the actual
value in user-reported logs. Log the string values directly.
Adds a regression test asserting no object argument appears in the
debug log call for storeRpcUrl.
Refs tinyhumansai#2437 (sub-issue B)
|
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 (1)
📝 WalkthroughWalkthroughConsole debug calls in config persistence now log primitive/string values directly for RPC URL and core mode; tests were added to verify the direct-string logging and absence of object wrappers. ChangesDebug Logging Simplification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Clean, focused fix — nice work. Both storeRpcUrl and storeCoreMode changes are correct; the object-wrapper { url: url.trim() } / { mode } was indeed printing [object Object] in non-DevTools contexts (CEF logs, production consoles). Test covers the storeRpcUrl path well.
[minor] The storeCoreMode fix (line 258) is identical in nature but has no corresponding test. Not blocking since it's a trivial one-line change and the pattern is proven by the existing test, but worth adding for completeness if you're doing a follow-up.
| File | Change |
|---|---|
configPersistence.ts:88 |
{ url: url.trim() } → url.trim() |
configPersistence.ts:258 |
{ mode } → mode |
configPersistence.test.ts |
+1 regression test for object-wrapper logging |
LGTM — moving to approved queue.
…ycyrus review) Add regression test verifying storeCoreMode logs the mode string directly rather than an object wrapper, mirroring the existing storeRpcUrl test. Requested in reviewer comment: "not blocking since it's a trivial one-line change and the pattern is proven by the existing test, but worth adding."
|
Thanks @graycyrus! Added the |
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 `@app/src/utils/__tests__/configPersistence.test.ts`:
- Around line 433-443: The test "logs the mode string directly, not an object
wrapper" currently spies on console.debug but only calls spy.mockRestore() at
the end, which can leak the spy if an assertion throws; update this test to wrap
the assertions and any call to spy.mock.calls in a try/finally block so that
spy.mockRestore() is always executed. Specifically, inside the test that calls
storeCoreMode('cloud') and inspects spy.mock.calls, move the assertions into a
try block and put spy.mockRestore() in the finally block to guarantee cleanup
even on failure.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2d76c217-bd05-4c56-a481-2123e4adcee4
📒 Files selected for processing (1)
app/src/utils/__tests__/configPersistence.test.ts
…bit) Ensure mockRestore() always runs even when an assertion throws, preventing console.debug spy from leaking into subsequent tests. Applied to both the storeRpcUrl and storeCoreMode log-format tests.
2 participants
Summary
storeRpcUrl: changeconsole.debug('[configPersistence] Stored RPC URL:', { url: url.trim() })→ logurl.trim()directly so the value appears as a readable string rather than[object Object].storeCoreMode: same fix —{ mode }→mode.storeRpcUrldebug call.Problem
configPersistence.tslogged wrapper objects{ url: '...' }and{ mode }in its debug lines. In browser DevTools (and in CEF renderer logs) these print as[object Object], actively hiding the stored value from diagnostic output. This was flagged as sub-issue B in #2437 as the cause of masked root-cause investigation: the log lineStored RPC URL: [object Object]hid the fact that no cloud RPC URL had been stored, making the remote-core connection failure harder to triage.Solution
Log the scalar values directly. One-line fix per site; no behaviour change.
Submission Checklist
'logs the URL string directly, not an object wrapper'inconfigPersistence.test.tsRefs #2437(sub-issue B)Impact
Related
[configPersistence] Stored RPC URL: [object Object]printer bug)AI Authored PR Metadata
Linear Issue
Commit & Branch
Validation Run
pnpm --filter openhuman-app format:check— cleanpnpm --filter openhuman-app compile— 0 errorspnpm --filter openhuman-app lint— 0 new errorspnpm debug unit configPersistence— 68 tests pass (1 new)Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Parity Contract
localStorageread/write semantics unchanged; only theconsole.debugargument format changedDuplicate / Superseded PR Handling
Summary by CodeRabbit
Tests
Refactor