Add comprehensive utilities, validation, and developer documentation #25
Conversation
…ation - Add validation utilities for secure input handling (addresses, numbers, timestamps) - Enhance lib/utils.ts with formatting, truncation, and comparison utilities - Improve TypeScript types with TransactionResult, WalletState, and ChainActivity - Create centralized lib/index.ts for organized exports - Add comprehensive JSDoc comments to Header and Countdown components - Add ARIA labels for improved accessibility in countdown component - Create detailed CONTRIBUTING.md guide for future contributors - All changes follow Ultracite code standards and security best practices
📝 WalkthroughWalkthroughThis PR establishes a utility library infrastructure by introducing a central export point, validation utilities, formatting helpers, new type definitions for transactions and wallet state, and enhances component documentation with JSDoc comments and accessibility improvements. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 8
🤖 Fix all issues with AI agents
In `@components/countdown.tsx`:
- Around line 21-22: The per-second updating span (the text variable rendered
inside the Countdown component) currently has aria-live="polite" and will cause
screen readers to announce every second; change that span to aria-hidden="true"
to hide the fast-updating text from AT, then add a separate visually-hidden/live
region element (e.g., minuteAnnouncement or ariaAnnouncement state) that uses
aria-live="polite" and is only updated on less frequent boundaries (minute
changes or throttled via a useEffect that updates when Math.floor(seconds/60)
changes) so screen readers get sparse, meaningful announcements instead of
per-second spam.
In `@CONTRIBUTING.md`:
- Around line 193-212: The fenced project-structure code block in
CONTRIBUTING.md is missing a language specifier which triggers MD040; update the
opening fence for the block that starts with "app/ # Next.js app
router and pages" to include a plaintext specifier (e.g., change "```" to
"```text") so the diagram is treated as plain text by the markdown linter.
- Around line 128-131: Add a blank line immediately before the fenced code block
that contains the git push example in CONTRIBUTING.md so the code block is
separated from the preceding list item (ensure there is an empty line before the
line starting with ```bash and the example line "git push origin
feature/your-feature-name") to satisfy MD031 markdown linting.
In `@lib/index.ts`:
- Around line 1-24: This barrel file re-exports everything and must be removed:
delete lib/index.ts (the re-exporting of "./error-handling", "./validation",
"./utils", "./constants", the type export of SupportedChain/ChainId,
"./ens-utils", "./contracts", and "./fonts") and update all consumers to import
directly from the specific modules (e.g., import { isValidAddress } from
"validation", import { SupportedChain, ChainId } from "constants", import {
createContractHelper } from "contracts", etc.); search the repo for imports
referencing the barrel (e.g., imports from "lib" or "@/lib") and replace them
with direct module imports, then run the TypeScript build/tests to ensure no
broken imports remain.
- Line 15: The file currently does both "export * from \"./constants\"" and an
explicit "export type { SupportedChain, ChainId }" which is redundant; remove
the explicit type re-export (the "export type { SupportedChain, ChainId }" line)
so the barrel relies on the existing "export * from \"./constants\"" export, and
run a quick type-check to confirm nothing else relies on that explicit line.
In `@lib/utils.ts`:
- Around line 109-120: The function memoize currently executes computeFn
immediately and returns a snapshot object rather than caching results; either
rename memoize to something like createMemoEntry or createComputedSnapshot to
reflect that it returns { current, dependencies }, or implement real memoization
inside memoize by storing cachedDependencies and cachedValue and returning a
function (or cached value) that only recomputes computeFn when dependencies
change; update callers expecting memoize to now either call the returned getter
(if implementing a function) or use the new name to avoid semantic confusion
(refer to the memoize export and any call sites using
memoize/current/dependencies to apply the change).
In `@lib/validation.ts`:
- Around line 47-60: The function isValidTimestamp currently enforces a lower
bound of one second ago which rejects all past timestamps; update the code to
match the intended behavior: either (A) if the intent is to only accept
recent-or-future timestamps, rename isValidTimestamp to
isRecentOrFutureTimestamp and update JSDoc/any callers to reflect the new
semantics, or (B) if past timestamps should be allowed, remove the lower-bound
check (timestampSeconds >= oneSecondAgo) so isValidTimestamp only verifies
isNonNegativeInteger(timestampSeconds) and timestampSeconds <= maxAllowed;
ensure you update the JSDoc and any references to the function accordingly and
keep the helper isNonNegativeInteger usage.
In `@types/transaction.ts`:
- Around line 6-12: The TransactionResult<T> type's error field is redundantly
typed as `error?: Error | null`; remove the `| null` and declare it as `error?:
Error` in the TransactionResult<T> definition so the optional marker alone
expresses absence, updating the `TransactionResult` type accordingly.
| <span className="font-mono font-semibold text-xs tabular-nums tracking-tight" aria-live="polite" aria-label={`Time until next reset: ${text}`}> | ||
| {text} |
There was a problem hiding this comment.
Throttle live-region announcements to avoid per-second screen reader spam (Line 21).
With aria-live="polite" on text that updates every second, assistive tech may announce continuously. Consider hiding the fast-updating text from AT and adding a separate, less-frequently changing live region (e.g., minute-level).
♻️ Proposed fix (screen reader-friendly live region)
export function Countdown() {
const { text } = useCountdown();
+ const [hours, minutes] = text.split(":");
+ const a11yText = `${hours}:${minutes}`;
return (
<div className="group mx-auto flex w-fit items-center gap-2.5 rounded-full border border-border/60 bg-linear-to-b from-muted/40 to-muted/60 px-4 py-2.5 shadow-sm backdrop-blur-sm transition-all duration-200 hover:border-border hover:shadow-md">
<div className="flex items-center gap-2">
<ClockIcon className="size-3.5" aria-hidden="true" />
<span className="align-middle font-medium text-muted-foreground text-xs">
Next reset in
</span>
- <span className="font-mono font-semibold text-xs tabular-nums tracking-tight" aria-live="polite" aria-label={`Time until next reset: ${text}`}>
+ <span className="font-mono font-semibold text-xs tabular-nums tracking-tight" aria-hidden="true">
{text}
</span>
+ <span className="sr-only" aria-live="polite" aria-atomic="true">
+ Time until next reset: {a11yText}
+ </span>
</div>
</div>
);
}Based on learnings, avoid rapid ARIA updates that can overwhelm assistive technologies.
🤖 Prompt for AI Agents
In `@components/countdown.tsx` around lines 21 - 22, The per-second updating span
(the text variable rendered inside the Countdown component) currently has
aria-live="polite" and will cause screen readers to announce every second;
change that span to aria-hidden="true" to hide the fast-updating text from AT,
then add a separate visually-hidden/live region element (e.g.,
minuteAnnouncement or ariaAnnouncement state) that uses aria-live="polite" and
is only updated on less frequent boundaries (minute changes or throttled via a
useEffect that updates when Math.floor(seconds/60) changes) so screen readers
get sparse, meaningful announcements instead of per-second spam.
| 1. **Push to your fork** | ||
| ```bash | ||
| git push origin feature/your-feature-name | ||
| ``` |
There was a problem hiding this comment.
Minor markdown formatting issue: missing blank line before fenced code block.
Add a blank line before the code block to comply with markdown lint rules (MD031).
📝 Suggested fix
2. **Create a Pull Request** with:
+
```bash
git push origin feature/your-feature-name
```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 1. **Push to your fork** | |
| ```bash | |
| git push origin feature/your-feature-name | |
| ``` | |
| 1. **Push to your fork** |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
129-129: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
In `@CONTRIBUTING.md` around lines 128 - 131, Add a blank line immediately before
the fenced code block that contains the git push example in CONTRIBUTING.md so
the code block is separated from the preceding list item (ensure there is an
empty line before the line starting with ```bash and the example line "git push
origin feature/your-feature-name") to satisfy MD031 markdown linting.
| ``` | ||
| app/ # Next.js app router and pages | ||
| ├── api/ # API routes | ||
| ├── layout.tsx # Root layout | ||
| └── page.tsx # Home page | ||
| components/ # React components | ||
| ├── gm-chain-card/ # Per-chain GM functionality | ||
| ├── providers/ # Context providers | ||
| └── ui/ # Reusable UI components | ||
| lib/ # Utilities and helpers | ||
| ├── constants.ts # Configuration constants | ||
| ├── error-handling.ts # Error handling utilities | ||
| ├── utils.ts # General utilities | ||
| └── validation.ts # Input validation | ||
| types/ # TypeScript type definitions | ||
| hooks/ # Custom React hooks | ||
| ``` |
There was a problem hiding this comment.
Minor: Add language specifier to the fenced code block.
The project structure diagram should use a language specifier (e.g., text or plaintext) to satisfy markdown lint rules (MD040).
📝 Suggested fix
-```
+```text
app/ # Next.js app router and pages📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| app/ # Next.js app router and pages | |
| ├── api/ # API routes | |
| ├── layout.tsx # Root layout | |
| └── page.tsx # Home page | |
| components/ # React components | |
| ├── gm-chain-card/ # Per-chain GM functionality | |
| ├── providers/ # Context providers | |
| └── ui/ # Reusable UI components | |
| lib/ # Utilities and helpers | |
| ├── constants.ts # Configuration constants | |
| ├── error-handling.ts # Error handling utilities | |
| ├── utils.ts # General utilities | |
| └── validation.ts # Input validation | |
| types/ # TypeScript type definitions | |
| hooks/ # Custom React hooks | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
193-193: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@CONTRIBUTING.md` around lines 193 - 212, The fenced project-structure code
block in CONTRIBUTING.md is missing a language specifier which triggers MD040;
update the opening fence for the block that starts with "app/ #
Next.js app router and pages" to include a plaintext specifier (e.g., change
"```" to "```text") so the diagram is treated as plain text by the markdown
linter.
| /** | ||
| * Central export point for lib utilities | ||
| * Provides organized access to all utility functions and types | ||
| */ | ||
|
|
||
| // Error handling and validation | ||
| export * from "./error-handling"; | ||
| export * from "./validation"; | ||
|
|
||
| // Utility functions | ||
| export * from "./utils"; | ||
| export * from "./constants"; | ||
|
|
||
| // Type utilities | ||
| export type { SupportedChain, ChainId } from "./constants"; | ||
|
|
||
| // ENS utilities | ||
| export * from "./ens-utils"; | ||
|
|
||
| // Smart contract utilities | ||
| export * from "./contracts"; | ||
|
|
||
| // Font utilities | ||
| export * from "./fonts"; |
There was a problem hiding this comment.
Barrel file anti-pattern: violates project coding guidelines.
This file re-exports everything from multiple modules, which the project's coding guidelines explicitly prohibit: "Avoid barrel files (index files that re-export everything)."
Barrel files can cause:
- Poor tree-shaking (larger bundles)
- Circular dependency issues
- Slower IDE performance and type checking
Consider removing this file and having consumers import directly from the specific modules they need (e.g., import { isValidAddress } from "@/lib/validation").
Based on learnings, the project guidelines state to avoid barrel files in **/index.{ts,tsx,js,jsx}.
🤖 Prompt for AI Agents
In `@lib/index.ts` around lines 1 - 24, This barrel file re-exports everything and
must be removed: delete lib/index.ts (the re-exporting of "./error-handling",
"./validation", "./utils", "./constants", the type export of
SupportedChain/ChainId, "./ens-utils", "./contracts", and "./fonts") and update
all consumers to import directly from the specific modules (e.g., import {
isValidAddress } from "validation", import { SupportedChain, ChainId } from
"constants", import { createContractHelper } from "contracts", etc.); search the
repo for imports referencing the barrel (e.g., imports from "lib" or "@/lib")
and replace them with direct module imports, then run the TypeScript build/tests
to ensure no broken imports remain.
| export * from "./constants"; | ||
|
|
||
| // Type utilities | ||
| export type { SupportedChain, ChainId } from "./constants"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Redundant type export.
If this barrel file is kept, the explicit type re-export on line 15 is redundant since line 12 already does export * from "./constants", which includes all exports from that module.
📝 Remove redundant export
// Utility functions
export * from "./utils";
export * from "./constants";
-
-// Type utilities
-export type { SupportedChain, ChainId } from "./constants";🤖 Prompt for AI Agents
In `@lib/index.ts` at line 15, The file currently does both "export * from
\"./constants\"" and an explicit "export type { SupportedChain, ChainId }" which
is redundant; remove the explicit type re-export (the "export type {
SupportedChain, ChainId }" line) so the barrel relies on the existing "export *
from \"./constants\"" export, and run a quick type-check to confirm nothing else
relies on that explicit line.
| export function memoize<T>( | ||
| computeFn: () => T, | ||
| dependencies: readonly unknown[] | ||
| ): { | ||
| current: T; | ||
| dependencies: readonly unknown[]; | ||
| } { | ||
| return { | ||
| current: computeFn(), | ||
| dependencies, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Misleading function name: memoize doesn't actually memoize.
This function computes the value immediately on every call and returns a snapshot object. It lacks any caching mechanism—callers must manually track previous results and compare dependencies, which defeats the purpose of a memoization utility.
Consider either:
- Renaming to
createMemoEntryorcreateComputedSnapshotto reflect its actual behavior - Implementing actual memoization with internal cache storage
♻️ Option 1: Rename to reflect actual behavior
-/**
- * Create a memoized version of a computation result
- * Only recomputes if dependencies change
- */
-export function memoize<T>(
+/**
+ * Create a snapshot of a computed value with its dependencies
+ * Useful for manual comparison in subsequent calls
+ */
+export function createComputedSnapshot<T>(
computeFn: () => T,
dependencies: readonly unknown[]
): {
current: T;
dependencies: readonly unknown[];
} {
return {
current: computeFn(),
dependencies,
};
}♻️ Option 2: Implement actual memoization
/**
* Create a memoized function that caches its result
* Only recomputes when called with different dependencies
*/
export function memoize<T>(
computeFn: () => T,
dependencies: readonly unknown[]
): () => T {
let cachedDependencies: readonly unknown[] = dependencies;
let cachedValue: T = computeFn();
return () => {
if (!arraysEqual([...cachedDependencies], [...dependencies])) {
cachedDependencies = dependencies;
cachedValue = computeFn();
}
return cachedValue;
};
}🤖 Prompt for AI Agents
In `@lib/utils.ts` around lines 109 - 120, The function memoize currently executes
computeFn immediately and returns a snapshot object rather than caching results;
either rename memoize to something like createMemoEntry or
createComputedSnapshot to reflect that it returns { current, dependencies }, or
implement real memoization inside memoize by storing cachedDependencies and
cachedValue and returning a function (or cached value) that only recomputes
computeFn when dependencies change; update callers expecting memoize to now
either call the returned getter (if implementing a function) or use the new name
to avoid semantic confusion (refer to the memoize export and any call sites
using memoize/current/dependencies to apply the change).
| export function isValidTimestamp( | ||
| timestampSeconds: number, | ||
| maxFutureSeconds: number = 365 * 24 * 60 * 60 | ||
| ): boolean { | ||
| const now = Math.floor(Date.now() / 1000); | ||
| const oneSecondAgo = now - 1; | ||
| const maxAllowed = now + maxFutureSeconds; | ||
|
|
||
| return ( | ||
| isNonNegativeInteger(timestampSeconds) && | ||
| timestampSeconds >= oneSecondAgo && | ||
| timestampSeconds <= maxAllowed | ||
| ); | ||
| } |
There was a problem hiding this comment.
Misleading function name and documentation: rejects all past timestamps.
The JSDoc states this validates timestamps "not in the distant future," but the implementation rejects all timestamps older than 1 second ago (timestampSeconds >= oneSecondAgo). This effectively validates "recent-or-future" timestamps, not just "not distant future."
If this is intentional (e.g., for validating incoming event timestamps that should be current), consider renaming to isRecentOrFutureTimestamp and updating the JSDoc. If past timestamps should be valid, remove the lower bound check.
♻️ Option 1: Rename and update docs to match behavior
/**
- * Validates if a timestamp (in seconds) is reasonable (not in the distant future)
- * Allows up to 1 year in the future as a reasonable buffer
+ * Validates if a timestamp (in seconds) is recent or in the near future
+ * Rejects past timestamps (older than 1 second) and distant future timestamps
*/
-export function isValidTimestamp(
+export function isRecentOrFutureTimestamp(
timestampSeconds: number,
maxFutureSeconds: number = 365 * 24 * 60 * 60
): boolean {♻️ Option 2: Allow past timestamps (match docs)
export function isValidTimestamp(
timestampSeconds: number,
maxFutureSeconds: number = 365 * 24 * 60 * 60
): boolean {
const now = Math.floor(Date.now() / 1000);
- const oneSecondAgo = now - 1;
const maxAllowed = now + maxFutureSeconds;
return (
isNonNegativeInteger(timestampSeconds) &&
- timestampSeconds >= oneSecondAgo &&
timestampSeconds <= maxAllowed
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function isValidTimestamp( | |
| timestampSeconds: number, | |
| maxFutureSeconds: number = 365 * 24 * 60 * 60 | |
| ): boolean { | |
| const now = Math.floor(Date.now() / 1000); | |
| const oneSecondAgo = now - 1; | |
| const maxAllowed = now + maxFutureSeconds; | |
| return ( | |
| isNonNegativeInteger(timestampSeconds) && | |
| timestampSeconds >= oneSecondAgo && | |
| timestampSeconds <= maxAllowed | |
| ); | |
| } | |
| export function isValidTimestamp( | |
| timestampSeconds: number, | |
| maxFutureSeconds: number = 365 * 24 * 60 * 60 | |
| ): boolean { | |
| const now = Math.floor(Date.now() / 1000); | |
| const maxAllowed = now + maxFutureSeconds; | |
| return ( | |
| isNonNegativeInteger(timestampSeconds) && | |
| timestampSeconds <= maxAllowed | |
| ); | |
| } |
🤖 Prompt for AI Agents
In `@lib/validation.ts` around lines 47 - 60, The function isValidTimestamp
currently enforces a lower bound of one second ago which rejects all past
timestamps; update the code to match the intended behavior: either (A) if the
intent is to only accept recent-or-future timestamps, rename isValidTimestamp to
isRecentOrFutureTimestamp and update JSDoc/any callers to reflect the new
semantics, or (B) if past timestamps should be allowed, remove the lower-bound
check (timestampSeconds >= oneSecondAgo) so isValidTimestamp only verifies
isNonNegativeInteger(timestampSeconds) and timestampSeconds <= maxAllowed;
ensure you update the JSDoc and any references to the function accordingly and
keep the helper isNonNegativeInteger usage.
| export type TransactionResult<T = unknown> = { | ||
| success: boolean; | ||
| status: TransactionStatus; | ||
| data?: T; | ||
| error?: Error | null; | ||
| hash?: `0x${string}`; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider simplifying the error field type.
The error?: Error | null type is redundant—the optional marker (?) already allows undefined. Unless you specifically need to distinguish between undefined (not set) and null (explicitly no error), consider simplifying.
📝 Suggested simplification
export type TransactionResult<T = unknown> = {
success: boolean;
status: TransactionStatus;
data?: T;
- error?: Error | null;
+ error?: Error;
hash?: `0x${string}`;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type TransactionResult<T = unknown> = { | |
| success: boolean; | |
| status: TransactionStatus; | |
| data?: T; | |
| error?: Error | null; | |
| hash?: `0x${string}`; | |
| }; | |
| export type TransactionResult<T = unknown> = { | |
| success: boolean; | |
| status: TransactionStatus; | |
| data?: T; | |
| error?: Error; | |
| hash?: `0x${string}`; | |
| }; |
🤖 Prompt for AI Agents
In `@types/transaction.ts` around lines 6 - 12, The TransactionResult<T> type's
error field is redundantly typed as `error?: Error | null`; remove the `| null`
and declare it as `error?: Error` in the TransactionResult<T> definition so the
optional marker alone expresses absence, updating the `TransactionResult` type
accordingly.
1 participant
📋 Summary
This PR adds comprehensive improvements to the OnePulse codebase focused on developer experience, code quality, and maintainability.
🎯 Changes
New Utilities
Developer Experience
Documentation
✅ Quality
🚀 Benefits
Summary by CodeRabbit
New Features
Accessibility
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.