fix(app): normalize cloud core RPC URLs#2480
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 (4)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughNormalize RPC URLs everywhere: persistence, client resolution, state hydration, and user input validation now rewrite base URLs (including trailing‑slash Cloudflare domains) to consistently target the JSON‑RPC ChangesCloud RPC URL Normalization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
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 |
![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
🧹 Nitpick comments (1)
app/src/utils/configPersistence.ts (1)
87-89: ⚡ Quick winUse the frontend debug helper and redact the URL.
This new log bypasses the repo’s namespaced
debugpattern, and logging the full user-entered URL can leak embedded credentials or query tokens. Please switch to the shared debug logger and log a redacted value instead.As per coding guidelines, "Use namespaced
debugfunction in TypeScript frontend with dev-only detail logging." and "Never log secrets, raw JWTs, API keys, or full PII — redact or omit sensitive 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 `@app/src/utils/configPersistence.ts` around lines 87 - 89, Replace the console.debug call in the block that stores the RPC URL (around normalizeRpcUrl and localStorage.setItem using RPC_URL_STORAGE_KEY) with the repo’s shared frontend debug helper (the namespaced debug import used across the frontend), and log a redacted form of the URL instead of the raw value; specifically, strip any userinfo (user:pass@) and remove query string parameters from the normalized value before passing it to the debug logger (e.g., debug('configPersistence: Stored RPC URL: %s', redactedUrl)), ensuring you import the shared debug helper the file already uses elsewhere and do not emit the full URL or any tokens.
🤖 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/configPersistence.ts`:
- Around line 185-194: The helper currently trims trailing slashes from the
whole input (variable normalized) before parsing, which strips meaningful
search/hash values (e.g., https://host?next=/); change the flow to parse first
with new URL(input.trim()), then operate on parsed.pathname (remove trailing
slashes only from parsed.pathname) and rebuild the returned string using
parsed.origin + (adjusted pathname or '/rpc' logic) + parsed.search +
parsed.hash; update the code paths that reference normalized/parsed accordingly
and add regression tests covering inputs like "https://host?next=/" and
"https://host/#/" to ensure search/hash are preserved.
---
Nitpick comments:
In `@app/src/utils/configPersistence.ts`:
- Around line 87-89: Replace the console.debug call in the block that stores the
RPC URL (around normalizeRpcUrl and localStorage.setItem using
RPC_URL_STORAGE_KEY) with the repo’s shared frontend debug helper (the
namespaced debug import used across the frontend), and log a redacted form of
the URL instead of the raw value; specifically, strip any userinfo (user:pass@)
and remove query string parameters from the normalized value before passing it
to the debug logger (e.g., debug('configPersistence: Stored RPC URL: %s',
redactedUrl)), ensuring you import the shared debug helper the file already uses
elsewhere and do not emit the full URL or any tokens.
🪄 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: e1d072a0-91d7-47ef-9bc2-0cd496b91af6
📒 Files selected for processing (8)
app/src/components/BootCheckGate/BootCheckGate.tsxapp/src/components/BootCheckGate/__tests__/BootCheckGate.test.tsxapp/src/services/__tests__/coreRpcClient.test.tsapp/src/services/coreRpcClient.tsapp/src/store/coreModeSlice.test.tsapp/src/store/coreModeSlice.tsapp/src/utils/__tests__/configPersistence.test.tsapp/src/utils/configPersistence.ts
…-cloudflare-2467 # Conflicts: # app/src/lib/i18n/chunks/de-5.ts
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
URL normalization for Cloudflare Tunnel self-hosted cores
Solid fix. The root cause — users pasting the tunnel base URL instead of the /rpc endpoint — is correctly addressed, and normalization is applied at every relevant entry point: picker validation, test-connection probe, getCoreRpcUrl, storeRpcUrl/getStoredRpcUrl/peekStoredRpcUrl, and coreModeSlice localStorage restoration. The CodeRabbit query/hash edge case is handled and tested.
Two minor things worth a look:
Issue–PR alignment
Issue #2467 describes a 405. The reporter's working hypothesis was "GET sent to /rpc", but the actual bug is a POST sent to / (the root endpoint). The fix is correct; the issue diagnosis was slightly off. Not blocking, just noting in case someone searches the issue later and is confused by the mismatch.
normalizeRpcUrl discards the parsed URL object
The function calls new URL(trimmed) only to validate, then re-parses the raw string by hand. The manual logic is correct for standard RPC URLs, but it diverges from the URL spec in one observable way: hostname casing is NOT normalized. new URL('HTTP://EXAMPLE.COM/') lowercases the host; the manual path returns HTTP://EXAMPLE.COM/rpc. Not a real-world issue for RPC URLs, but using the parsed object directly would eliminate this class of drift:
export function normalizeRpcUrl(url: string): string {
const trimmed = url.trim();
let parsed: URL;
try { parsed = new URL(trimmed); } catch { return trimmed.replace(/\/+$/, ''); }
// Remove trailing slashes from pathname only; append /rpc when path is root.
parsed.pathname = parsed.pathname.replace(/\/+$/, '') || '/rpc';
return parsed.href;
}URL#href preserves search and hash natively, which makes firstUrlSuffixIndex unnecessary. Something to consider for a follow-up if the manual approach ever causes trouble.
Test mock diverges from real normalizeRpcUrl
See inline comment below.
| Area | Files | Status |
|---|---|---|
| Core logic | configPersistence.ts, coreRpcClient.ts |
✓ Correct |
| Redux slice | coreModeSlice.ts |
✓ Correct |
| UI validation | BootCheckGate.tsx |
✓ Correct |
| New utility | redactRpcUrlForLog.ts |
✓ Clean |
| Test coverage | All four touched modules have regression cases | ✓ Good |
| CodeRabbit finding | Query/hash preservation — addressed in a89c207 | ✓ Addressed |
| describe('getCoreRpcUrl', () => { | ||
| const normalizeMockRpcUrl = (url: string) => { | ||
| const trimmed = url.replace(/\/+$/, ''); | ||
| return trimmed.endsWith('/rpc') ? trimmed : `${trimmed}/rpc`; |
There was a problem hiding this comment.
[minor] normalizeMockRpcUrl is a simplified stub that doesn't match normalizeRpcUrl's behavior for URLs with query strings or leading whitespace. For example, 'https://example.com?next=/' → mock produces 'https://example.com?next=//rpc' (appends /rpc to the whole string including ?next=/), whereas the real function returns 'https://example.com/rpc?next=/'.
The test cases that use this mock all pass pre-normalized RPC URLs through, so it works today. But the test 'in web mode normalizes a stored core base URL' is checking the mock's normalization, not the real module's — if normalizeRpcUrl broke for query-bearing URLs, that test would still pass.
An easy fix: import normalizeRpcUrl from the actual module in the vi.doMock factory (using vi.importActual) rather than re-implementing it. Same pattern applies to the duplicate at line ~1085 in the getCoreRpcToken describe block.
|
thanks @YUHAO-corn, the url normalization is such a quality of life fix, no more broken rpc connections when users paste a bare base url 🔥 great to see the regression coverage in there too, really solid work as always! |
2 participants
Summary
https://example.trycloudflare.comand still reach the JSON-RPC endpoint./rpcURLs, and previously persisted base URLs.Problem
/rpcendpoint./rpc; using the base URL can make the connection flow fail even though the tunnel itself is healthy.Solution
normalizeRpcUrlto append/rpcwhen the input URL has no path, while preserving existing/rpcURLs and non-root paths.normalizeRpcUrlacrossBootCheckGate,coreRpcClient,configPersistence, andcoreModeSliceso test connection, boot check, cached URL resolution, and localStorage restoration all agree.Submission Checklist
## Related— N/A: no coverage-matrix feature ID touched.docs/RELEASE-MANUAL-SMOKE.md) — N/A: no release-cut smoke checklist surface changed.Closes #NNNin the## RelatedsectionImpact
/rpcURLs continue to resolve unchanged; previously stored base URLs now self-heal on read.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
yuhao/fix-remote-core-cloudflare-24675e95aeed8a97acee5823d73b6dc8e92f04af00fbValidation Run
pnpm --filter openhuman-app format:checkpnpm typecheckpnpm --dir app exec vitest run --config test/vitest.config.ts src/services/__tests__/coreRpcClient.test.ts src/utils/__tests__/configPersistence.test.ts src/store/coreModeSlice.test.ts src/components/BootCheckGate/__tests__/BootCheckGate.test.tsx— 200 passedValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
/rpc./rpc.Parity Contract
/rpcURLs, auth token handling, RPC POST envelopes, and public-HTTP rejection behavior are unchanged.Duplicate / Superseded PR Handling
Summary by CodeRabbit
Bug Fixes
Tests
Localization