Conversation
WalkthroughAdds input validation infrastructure for vault addresses and chain IDs via new utils/validation.ts module. Introduces path parameter parsing in InteractionClient.tsx for vault context from URL. Adds last synced timestamp tracking to vault state. Updates multiple components (CreateForm, FindVaultForm, CardExplorer) to use validation utilities. Adds comprehensive documentation (CONTRIBUTING.md, SECURITY_TEST_REPORT.md) and rebuilds README with structured sections. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/[vaultId]/InteractionClient.tsx (1)
292-297: Use dynamic decimals instead of hardcoded10 ** 18for balance conversions.
readContractreturnsbigint; converting viaNumber()risks precision loss for large values, and hardcoding10 ** 18breaks support for non-18 decimal tokens. The code reads the coin'sdecimalsbut doesn't use it.Fix
coinReserveandcoinBalanceusing the coin's decimals:+ const decimals = vault.decimals ?? 18 setBalances((prev: typeof balances) => ({ ...prev, - coinReserve: Number(coinReserveOnChain) / 10 ** 18, - coinBalance: Number(coinBalanceOnChain) / 10 ** 18, + coinReserve: Number(coinReserveOnChain) / 10 ** decimals, + coinBalance: Number(coinBalanceOnChain) / 10 ** decimals, hodlCoinBalance: Number(hodlCoinBalanceOnChain) / 10 ** 18, }))Alternatively (preferred): import
formatUnitsfrom viem and store rawbigintvalues in state, formatting only for display. This avoidsNumber()conversion losses entirely.
🧹 Nitpick comments (7)
README.md (1)
52-98: Add language specifier to the fenced code block.The file structure code block should specify
textor leave blank for proper rendering and linting compliance.Apply this diff:
## File Structure -``` +```text app/app/[vaultId]/InteractionClient.tsx (3)
10-16: Avoid(params as any); typeuseParamssovaultIdis safe and predictable.
This reduces runtime edge-cases (e.g.,string[]) and removes theanyescape hatch.-import { useSearchParams, useRouter, useParams } from 'next/navigation' +import { useSearchParams, useRouter, useParams } from 'next/navigation' ... - const params = useParams() + const params = useParams<{ vaultId?: string }>() ... - const pv = (params as any)?.vaultId + const pv = params?.vaultIdAlso applies to: 26-30, 84-88
67-67:lastSyncedAtUX is good; consider setting it when serving cached data too.
Right now it updates on fresh fetch and manual sync, but a cache hit leaves it blank, which can look like “never synced”.Also applies to: 216-216, 371-371, 517-520
324-328: Precision risk withNumber(bigint)conversions for supply and price; consider usingformatUnits+ string-based state.The current code converts bigint values to
Numberbefore dividing by decimals. Per viem documentation,Number()loses precision above 2^53-1 (~9 billion). ForhodlCoinSupplywith 18 decimals, any totalSupply exceeding ~9M tokens will silently lose precision. The recommended pattern isformatUnits(value, decimals)which returns a properly formatted string.This would require storing balances as strings instead of numbers, which is a broader architecture change. If you keep numeric state, consider at minimum validating that values remain within safe bounds or document the limitation.
utils/validation.ts (3)
16-32:parseVaultPathParam: trim segments and guard against extra separators.
Small hardening:split(':')/split('@')can yield more than 2 parts; trimming prevents subtle failures with whitespace.if (pv.includes(':')) { - const [a, c] = pv.split(':') - addr = a - cid = Number(c) + const [a, c] = pv.split(':', 2) + addr = a.trim() + cid = Number(c?.trim()) } else if (pv.includes('@')) { - const [a, c] = pv.split('@') - addr = a - cid = Number(c) + const [a, c] = pv.split('@', 2) + addr = a.trim() + cid = Number(c?.trim()) }
34-38:parseChainIdInput: trim and treat empty input as null.
AvoidsNumber('') === 0pitfalls and makes UI validation cleaner.export const parseChainIdInput = (val: string): number | null => { - const num = Number(val) + const v = val?.trim() + if (!v) return null + const num = Number(v) if (!Number.isFinite(num)) return null return isValidChainId(num) ? num : null }
1-14: Use viem's address validation for EIP-55 checksum support.
isAddress(addr, { strict: true })validates EIP-55 checksummed addresses by default, andgetAddress(addr)normalizes to the correct checksum. Replace the regex-only approach to catch invalid checksums and improve safety in form validation (FindVaultForm, CreateForm) and contract interactions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CONTRIBUTING.md(1 hunks)README.md(1 hunks)SECURITY_TEST_REPORT.md(1 hunks)app/[vaultId]/InteractionClient.tsx(8 hunks)components/CreateVault/CreateForm.tsx(4 hunks)components/Explorer/CardExplorer.tsx(4 hunks)components/FindVault/FindVaultForm.tsx(4 hunks)package.json(1 hunks)types/shims.d.ts(1 hunks)utils/validation.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
app/[vaultId]/InteractionClient.tsx (1)
utils/validation.ts (3)
normalizeAddress(5-9)isValidChainId(11-14)parseVaultPathParam(16-32)
utils/validation.ts (1)
utils/chains.ts (1)
getSupportedChainIds(252-254)
components/CreateVault/CreateForm.tsx (1)
utils/validation.ts (2)
normalizeAddress(5-9)isValidChainId(11-14)
components/Explorer/CardExplorer.tsx (1)
utils/validation.ts (2)
normalizeAddress(5-9)isValidChainId(11-14)
🪛 LanguageTool
CONTRIBUTING.md
[style] ~31-~31: Consider using a different verb for a more formal wording.
Context: ...fig.json:25). - Run npm run lint` and fix issues before committing. - Follow Pret...
(FIX_RESOLVE)
🪛 markdownlint-cli2 (0.18.1)
README.md
52-52: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (9)
package.json (1)
10-10: LGTM! Useful addition for build-time type safety.The
typecheckscript enables standalone TypeScript validation without emitting files, which is valuable for CI/CD pipelines and pre-commit checks.SECURITY_TEST_REPORT.md (1)
1-44: Well-structured security documentation.The report provides clear findings and actionable recommendations. The validation utilities referenced (e.g.,
utils/validation.ts) align with security best practices for client-side input handling.CONTRIBUTING.md (1)
1-53: Comprehensive contributing guide with helpful references.The guide provides clear workflows and specific file references, making it easy for contributors to understand the project structure and conventions.
README.md (1)
1-190: Excellent documentation transformation.The README has been restructured from a basic starter template to comprehensive platform documentation with clear sections, specific file references, and helpful setup instructions.
components/Explorer/CardExplorer.tsx (1)
49-55: Good validation before navigation.The address normalization and chain ID validation prevent navigation with invalid parameters, improving robustness.
components/FindVault/FindVaultForm.tsx (2)
28-39: Excellent input validation with user feedback.The real-time validation for both address and chain ID, combined with inline error messages, provides clear feedback and prevents invalid navigation attempts.
71-75: Good UX with immediate validation feedback.Validating inputs on change and displaying errors inline helps users correct issues before submission.
Also applies to: 91-94
components/CreateVault/CreateForm.tsx (2)
107-107: Proper input validation before vault creation.Validating both the ERC20 address format and chain ID support prevents contract interaction errors and improves user feedback.
Also applies to: 115-115
443-443: Security best practice applied.Adding
rel="noreferrer"to external links prevents the Referer header from being sent, which aligns with the security recommendations in the Security Test Report.
| useEffect(() => { | ||
| const vault = searchParams.get('vault') | ||
| const chain = searchParams.get('chainId') | ||
|
|
||
| if (vault && chain) { | ||
| setVaultAddress(vault as `0x${string}`) | ||
| setChainId(Number(chain)) | ||
| const naddr = normalizeAddress(vault) | ||
| const cid = Number(chain) | ||
| if (naddr && isValidChainId(cid)) { | ||
| setVaultAddress(naddr) | ||
| setChainId(cid) | ||
| } else { | ||
| setError('Invalid vault address or chain ID') | ||
| } | ||
| return | ||
| } | ||
| const pv = (params as any)?.vaultId | ||
| if (typeof pv === 'string' && pv.length > 0) { | ||
| const { address, chainId: parsedCid } = parseVaultPathParam(pv) | ||
| const cid = parsedCid ?? activeChainId ?? 0 | ||
| if (address && isValidChainId(cid)) { | ||
| setVaultAddress(address) | ||
| setChainId(cid) | ||
| } else { | ||
| setError('Invalid vault address or chain ID') | ||
| } | ||
| } | ||
| }, [searchParams]) | ||
| }, [searchParams, params, activeChainId]) |
There was a problem hiding this comment.
Fix “stuck loading” when URL params are invalid (error is masked by isLoading).
Right now you can set error without ever clearing isLoading, but the render checks isLoading before error, so the user may never see the error state.
- if (isLoading) {
+ if (error) {
+ return (
+ <div className="relative min-h-screen flex items-center justify-center bg-background overflow-hidden">
+ {/* Matrix background effect */}
+ <div className="absolute inset-0 opacity-10">
+ <div ref={matrixRef} className="absolute inset-0 w-full h-full" />
+ </div>
+
+ <Card className="bg-background/50 backdrop-blur-xl border-red-500/20 p-8 shadow-2xl shadow-red-500/5 max-w-md mx-4">
+ <div className="flex flex-col items-center space-y-4 text-center">
+ <AlertCircle className="h-8 w-8 text-red-500" />
+ <h2 className="text-xl font-semibold text-gradient">Error Loading Vault</h2>
+ <p className="text-muted-foreground">{error}</p>
+ <Link href="/">
+ <Button variant="outline" className="border-primary/50 hover:bg-primary/10">
+ <ArrowLeft className="h-4 w-4 mr-2" />
+ Go Back Home
+ </Button>
+ </Link>
+ </div>
+ </Card>
+ </div>
+ )
+ }
+
+ if (isLoading) {
return (
<div className="relative min-h-screen mt-16 flex items-center justify-center bg-background overflow-hidden">
{/* Matrix background effect */}
<div className="absolute inset-0 opacity-10">
<div ref={matrixRef} className="absolute inset-0 w-full h-full" />
<div className="absolute top-1/4 left-1/4 w-32 h-32 bg-gray-300/15 dark:bg-purple-500/10 rounded-full blur-3xl animate-pulse" />
<div className="absolute bottom-1/3 right-1/4 w-24 h-24 bg-gray-400/10 dark:bg-violet-500/15 rounded-full blur-2xl animate-pulse animation-delay-2000" />
</div>
<Card className="bg-background/50 backdrop-blur-xl border-primary/20 p-8 shadow-2xl shadow-primary/5">
<div className="flex flex-col items-center space-y-4">
<Loader2 className="h-8 w-8 animate-spin text-primary" />
<p className="text-muted-foreground">Loading vault data...</p>
</div>
</Card>
</div>
)
}
-
- if (error) {
- return ( ... )
- }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/[vaultId]/InteractionClient.tsx around lines 69 to 95, the effect can set
error without clearing isLoading so the UI remains stuck showing loading; update
the effect so any branch that sets setError(...) also calls setIsLoading(false),
and ensure successful branches that setVaultAddress/setChainId also call
setIsLoading(false) (or clear isLoading at the end of the effect once validation
is complete); make sure every execution path either clears isLoading or leaves
it intentionally true only while async work is underway.
| </Button> | ||
| </Link> | ||
| <Link href={`/v?chainId=${config.state.chainId}&vault=${vaultAddress}`}> | ||
| <Link href={`/${normalizeAddress(vaultAddress) ?? vaultAddress}?chainId=${config.state.chainId}`}> |
There was a problem hiding this comment.
Potential issue with normalization fallback.
The fallback ?? vaultAddress could still route to an invalid address if normalization fails. Since vault creation already validated the address (lines 107, 115), this scenario shouldn't occur, but it's worth ensuring the fallback never happens.
Consider asserting that normalization succeeds or logging when fallback is used:
-<Link href={`/${normalizeAddress(vaultAddress) ?? vaultAddress}?chainId=${config.state.chainId}`}>
+<Link href={`/${normalizeAddress(vaultAddress) || vaultAddress}?chainId=${config.state.chainId}`}>Or more defensively:
const normalizedAddr = normalizeAddress(vaultAddress);
if (!normalizedAddr) {
console.error('Vault address normalization failed after creation:', vaultAddress);
}
// Use normalizedAddr with proper null handling🤖 Prompt for AI Agents
In components/CreateVault/CreateForm.tsx around line 453, the code uses
normalizeAddress(vaultAddress) with a fallback to vaultAddress which could route
to an invalid address; change this to explicitly normalize once into a variable,
assert or log if normalization returns falsy, and only use the normalized value
in the Link (or redirect to an error/validation path) so the fallback never
silently proceeds — i.e., compute normalizedAddr =
normalizeAddress(vaultAddress), if normalizedAddr is falsy log an error (or
throw/handle) and avoid using vaultAddress as a fallback, then use
normalizedAddr in the Link href.
| onMouseEnter={(e: any) => e.currentTarget.style.color = 'hsl(50 100% 45%)'} | ||
| onMouseLeave={(e: any) => e.currentTarget.style.color = ''} /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Event handler type casts are a symptom of the shims.d.ts issue.
The (e: any) casts for onMouseEnter and onMouseLeave handlers are unnecessary with proper React types. These casts are likely needed because types/shims.d.ts overrides React's type definitions with permissive any types.
Once types/shims.d.ts is removed (as recommended), these handlers should be typed properly:
onMouseEnter={(e: React.MouseEvent<HTMLSpanElement>) => e.currentTarget.style.color = 'hsl(50 100% 45%)'}Also applies to: 164-165, 171-172
🤖 Prompt for AI Agents
In components/Explorer/CardExplorer.tsx around lines 141-142 (and also update
164-165 and 171-172), replace the permissive (e: any) event parameter with the
proper React event typing and remove the casts: use e:
React.MouseEvent<HTMLSpanElement> for onMouseEnter and onMouseLeave so you can
safely access e.currentTarget.style; ensure React is imported for types (import
React from 'react' or enable isolated type import) and, separately, remove or
fix types/shims.d.ts that is overriding React types so these handlers no longer
require any casts.
| declare module 'lucide-react'; | ||
| declare module 'wagmi'; | ||
| declare module '@wagmi/core'; | ||
| declare module 'next/navigation'; | ||
| declare module 'next/link'; | ||
| declare module 'react' { | ||
| export function useState<T>(initialState: T | (() => T)): [T, (value: T | ((prev: T) => T)) => void]; | ||
| export function useEffect(effect: (...args: any[]) => any, deps?: any[]): void; | ||
| export function useCallback<T extends (...args: any[]) => any>(callback: T, deps: any[]): T; | ||
| } | ||
|
|
||
| declare namespace JSX { | ||
| interface IntrinsicElements { | ||
| [elemName: string]: any; | ||
| } | ||
| } | ||
|
|
||
| declare namespace React { | ||
| type MouseEvent = any; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove these temporary type shims and install proper type definitions.
This ambient declaration file uses any extensively, which defeats TypeScript's type safety. The proper types should be available from:
lucide-reactexports its own types@types/react@^18is already in devDependencies (line 50 in package.json)wagmi,@wagmi/core,next/navigation,next/linkexport their own types
As noted in the PR objectives, these temporary shims should be removed after installing real types from node_modules. The types already exist in your dependencies—this file likely masks actual type errors that should be fixed in the consuming code instead.
🤖 Prompt for AI Agents
In types/shims.d.ts around lines 1 to 20, remove this temporary ambient
declaration file that broadly declares many modules and uses any types; instead
install or enable the real type definitions from node_modules (ensure
lucide-react, wagmi, @wagmi/core, next and React types are present), delete the
file, run npm/yarn install to fetch types, and then fix any TypeScript errors
surfaced in the consuming code by updating incorrect typings or imports so the
proper exported types from those packages are used rather than any.
3 participants
Problem
Vault detail: incorrect coin balance scaling and render perf on large state updates #85
Changes
Rationale
Testing
Notes
Next Steps
Install Node 18+ and dependencies to run npm run dev , npm run typecheck , and npm run lint .
After installs, remove types/shims.d.ts and rely on real types from node_modules .
Summary by CodeRabbit
Documentation
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.