Fix/monorepo dependency consolidation#312
Conversation
Stabilize date rendering across server/client, avoid navigator access during SSR initialization, add error boundary for pages router, and add SSR-focused tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Keep SSR hydration-safe rendering while retaining EmptyState/BellOff UI from main. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adopt pnpm workspaces as the monorepo dependency source of truth, add shared tooling configs, and update CI/scripts to run consistently across workspace packages. Made-with: Cursor
Resolve duplicate imports and conflicting type declarations, fix error boundary fallback rendering, and remove duplicated Starknet env helpers to keep validation behavior consistent. Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
This PR migrates the repo toward pnpm workspaces with centralized tooling config, while also addressing some SSR/hydration stability concerns and wiring global error handling.
Changes:
- Introduce pnpm workspaces + shared tooling package (
@teachlink/tooling) and update root TypeScript/ESLint configs to consume it. - Update CI and docs to use pnpm; add workspace-wide lint/test/typecheck scripts.
- Improve SSR determinism (search date formatting + SSR test) and add global error boundary; adjust client components for hydration/Next.js compatibility.
Reviewed changes
Copilot reviewed 17 out of 20 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Switch root TS config to extend shared base config from @teachlink/tooling. |
| src/utils/web3/envValidation.ts | Reorder imports and remove duplicated exported helpers; keeps Web3 env validation logic. |
| src/pages/_app.tsx | Wrap app with ErrorBoundary. |
| src/components/search/tests/SearchResultsVisualizer.ssr.test.tsx | Add SSR-focused test for deterministic date formatting. |
| src/components/search/SearchResultsVisualizer.tsx | Introduce formatSearchResultDate() and use it in render for UTC-stable dates. |
| src/components/notificationcenter.tsx | Mark as client component; swap empty state UI; add hydration-safe timestamp rendering; switch avatar rendering to next/image. |
| src/components/errors/ErrorBoundarySystem.tsx | Adjust UserFriendlyErrorDisplay import style and update fallback rendering. |
| src/app/store/messagingStore.ts | Fix socket.io-client import to use named io. |
| src/app/mobile/components/OfflineContentManager.tsx | Avoid navigator.onLine in initial state (SSR-safe); set online state in effect. |
| src/app/mobile/components/MobileProgressTracker.tsx | Avoid navigator.onLine in initial state (SSR-safe); set online state in effect. |
| pnpm-workspace.yaml | Define workspace package globs for pnpm. |
| packages/tooling/tsconfig/base.json | Add shared base tsconfig used by root. |
| packages/tooling/package.json | Define internal tooling workspace package exports (ESLint + tsconfig). |
| packages/tooling/eslint/base.js | Add shared flat ESLint config factory. |
| package.json | Pin pnpm via packageManager, add workspace scripts, add overrides, and depend on @teachlink/tooling. |
| eslint.config.js | Replace inline config with @teachlink/tooling factory. |
| README.md | Update setup instructions to pnpm and document monorepo tooling decision. |
| .github/workflows/ci.yml | Switch CI install/cache/run steps from npm to pnpm. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Set up pnpm | ||
| uses: pnpm/action-setup@v4 | ||
| with: | ||
| version: 10 |
There was a problem hiding this comment.
CI installs pnpm with version: 10, but the repo pins packageManager to pnpm@10.33.2 in package.json. To avoid lockfile/behavior mismatches between local and CI, pin the action to the same exact version (or use corepack to activate the version from packageManager).
| @@ -7,7 +9,7 @@ export function validateStarknetEnv(): { valid: boolean; missing: string[] } { | |||
| export function getStarknetNetwork(): string { | |||
| return process.env.NEXT_PUBLIC_STARKNET_NETWORK ?? 'testnet'; | |||
| } | |||
There was a problem hiding this comment.
validateStarknetEnv()/getStarknetNetwork() are now out of sync with the Zod schema in this same file: the default network is 'testnet', but envSchema only allows mainnet-alpha | goerli-alpha | sepolia-alpha. With EnvGuard calling validateAppEnv(), following the default/README value will trip validation and block the app. Align the default + allowed values (either add a 'testnet' alias to the schema/NETWORKS mapping, or switch defaults/docs back to goerli-alpha/sepolia-alpha).
| Create a `.env.local` with: | ||
|
|
||
| ```ini | ||
| NEXT_PUBLIC_STARKNET_NETWORK=testnet |
There was a problem hiding this comment.
The README now instructs NEXT_PUBLIC_STARKNET_NETWORK=testnet, but validateAppEnv() only accepts mainnet-alpha, goerli-alpha, or sepolia-alpha (and defaults to goerli-alpha). As written, users following these steps will hit a configuration error. Update the example value to one of the accepted networks or update the validation to accept testnet as an alias.
| NEXT_PUBLIC_STARKNET_NETWORK=testnet | |
| NEXT_PUBLIC_STARKNET_NETWORK=goerli-alpha |
| <> | ||
| <div style={{ padding: '20px' }}> | ||
| <h2>Something went wrong.</h2> | ||
| <p>{this.state.error?.message}</p> | ||
|
|
||
| <button onClick={this.resetError}>Try Again</button> | ||
| </div> | ||
| <UserFriendlyErrorDisplay | ||
| error={this.state.error} | ||
| title="Application Error" | ||
| onRetry={this.resetError} | ||
| showDetails={true} | ||
| severity="error" | ||
| /> | ||
| </> |
There was a problem hiding this comment.
The error fallback currently renders both the inline "Something went wrong" block and UserFriendlyErrorDisplay, which will show two error UIs at once. Pick one presentation (preferably the shared UserFriendlyErrorDisplay) or wrap the legacy block behind a condition so only a single fallback is rendered.
| <> | |
| <div style={{ padding: '20px' }}> | |
| <h2>Something went wrong.</h2> | |
| <p>{this.state.error?.message}</p> | |
| <button onClick={this.resetError}>Try Again</button> | |
| </div> | |
| <UserFriendlyErrorDisplay | |
| error={this.state.error} | |
| title="Application Error" | |
| onRetry={this.resetError} | |
| showDetails={true} | |
| severity="error" | |
| /> | |
| </> | |
| <UserFriendlyErrorDisplay | |
| error={this.state.error} | |
| title="Application Error" | |
| onRetry={this.resetError} | |
| showDetails={true} | |
| severity="error" | |
| /> |
| const [isHydrated, setIsHydrated] = useState(false); | ||
|
|
||
| useEffect(() => { | ||
| setIsHydrated(true); | ||
| }, []); |
There was a problem hiding this comment.
NotificationItem sets its own isHydrated state via useEffect, which causes an extra render per notification and duplicates hydration logic across list items. Consider lifting a single isHydrated flag to NotificationCenter (or a small hook used once) and passing it down, so opening the panel only triggers one hydration state change regardless of list size.
| {avatarUrl ? ( | ||
| <img src={avatarUrl} alt="" className="notification-item__avatar" /> | ||
| <Image | ||
| src={avatarUrl} | ||
| alt="" | ||
| width={24} | ||
| height={24} | ||
| className="notification-item__avatar" | ||
| /> |
There was a problem hiding this comment.
Switching from <img> to next/image for avatarUrl can break rendering when the notification payload contains an image host that isn't allowed in next.config.ts (images.remotePatterns is currently limited to a few domains). Since avatarUrl comes from WebSocket payloads and isn't constrained, either keep a plain <img> for this case, or ensure the expected avatar domains are added to remotePatterns / use a safe proxy/loader.
| export function formatSearchResultDate(value: string | Date): string { | ||
| const date = new Date(value); | ||
| if (Number.isNaN(date.getTime())) { | ||
| return ''; | ||
| } | ||
| return new Intl.DateTimeFormat('en-US', { | ||
| year: 'numeric', | ||
| month: '2-digit', | ||
| day: '2-digit', | ||
| timeZone: 'UTC', | ||
| }).format(date); |
There was a problem hiding this comment.
formatSearchResultDate creates a new Intl.DateTimeFormat instance on every call. Since this runs per-result render, consider hoisting a single formatter (module-level constant) and reusing it to reduce allocations during large result renders.
| export function formatSearchResultDate(value: string | Date): string { | |
| const date = new Date(value); | |
| if (Number.isNaN(date.getTime())) { | |
| return ''; | |
| } | |
| return new Intl.DateTimeFormat('en-US', { | |
| year: 'numeric', | |
| month: '2-digit', | |
| day: '2-digit', | |
| timeZone: 'UTC', | |
| }).format(date); | |
| const SEARCH_RESULT_DATE_FORMATTER = new Intl.DateTimeFormat('en-US', { | |
| year: 'numeric', | |
| month: '2-digit', | |
| day: '2-digit', | |
| timeZone: 'UTC', | |
| }); | |
| export function formatSearchResultDate(value: string | Date): string { | |
| const date = new Date(value); | |
| if (Number.isNaN(date.getTime())) { | |
| return ''; | |
| } | |
| return SEARCH_RESULT_DATE_FORMATTER.format(date); |
|
Kindly resolve conflict |
Closes #240
Description
Brief description of changes
Related Issue
Closes #
Type of Change
Checklist