feat: restructure into Turborepo monorepo with shared packages#395
feat: restructure into Turborepo monorepo with shared packages#395williamluke4 wants to merge 8 commits intomainfrom
Conversation
Move the Next.js app into apps/web/, extract shared server code into @sarafu/api, @sarafu/contracts, @sarafu/core, and @sarafu/schemas packages, add a skeletal Expo mobile app at apps/mobile/, and configure Turborepo for build orchestration. Adds Bearer token auth support so the mobile client can authenticate via sealed iron-session tokens. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds pnpm overrides for @types/react and @types/react-dom to prevent duplicate type definitions across the monorepo, which caused TypeScript errors in icons.tsx and other files with React ref types. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Claude finished @williamluke4's task in 4m 10s —— View job Code Review: Turborepo Monorepo RestructureThis is a substantial and well-executed restructure. The package boundaries are sensible, the Turborepo pipeline is configured correctly, and the Bearer token auth is a clean addition. Below are the findings grouped by severity. Security[Medium] Bearer token always returned in SIWE verify response
const token = await createAuthToken(address, chainId);
return NextResponse.json({ ok: true, token });The sealed token is returned unconditionally — web clients don't need it and it needlessly exposes the credential. Consider returning it only when the request signals it's a non-cookie client (e.g. via an [Low] No token revocation Bearer tokens are sealed with a 30-day TTL but there's no revocation mechanism. If a token is stolen, the only recourse is rotating Bugs / Type Safety[Medium] IP extraction may be
const ip = (opts.req.headers.get("x-forwarded-for") ?? "127.0.0.1").split(",")[0];With const ip = (opts.req.headers.get("x-forwarded-for") ?? "127.0.0.1").split(",")[0] ?? "127.0.0.1";[Low] Unstable tRPC internal import
import { createCallerFactory } from "@trpc/server/unstable-core-do-not-import";
Architecture[Medium]
[Low] Mobile app depends on a Next.js-coupled package
Testing[Medium] Missing test:
The success test only checks it("returns ok on valid SIWE verification", async () => {
// ...
expect(body.ok).toBe(true);
// Missing: expect(typeof body.token).toBe("string");
});[Low] No tests for
Mobile App[Low]
isAuthenticated: !!state.token,A stored token that has expired server-side will still make [Info] Login screen is a placeholder
[Info] No production API URL configured
CI / Deployment[Low] CI runs only
- name: Run tests
run: pnpm run testTypeScript errors and lint failures won't be caught on CI. Adding [Info] Vercel root directory The Vercel deployment comment shows Minor Observations
SummaryThe core restructure is solid. The package boundaries make sense, the Turborepo pipeline is well-configured, and the Bearer token auth implementation is clean and tested for the happy/unhappy paths. The main items to address before merge are:
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
The verify route now imports createAuthToken which uses sealData from iron-session. The test mock was missing this export, causing CI failure. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @williamluke4's task in 2m 42s —— View job Code Review: Turborepo Monorepo RestructureTasks
The restructure is well-executed — package boundaries are clean, the Turborepo pipeline is configured correctly, and Bearer token auth is a solid addition for the mobile app. The main things that need attention before merge are listed below. Security[Medium] Bearer token returned unconditionally
The sealed token is returned to every client, including web browsers that already have a cookie. It's encrypted, but there's no reason to expose it to clients that don't need it. Consider gating on a request header: // Only return the token for non-cookie clients
const wantToken = req.headers.get("x-client-type") === "mobile";
return NextResponse.json({ ok: true, ...(wantToken ? { token } : {}) });[Low] No token revocation path
Bugs / Type Safety[Medium] IP extraction undefined with
const ip = (opts.req.headers.get("x-forwarded-for") ?? "127.0.0.1").split(",")[0];With const ip = (opts.req.headers.get("x-forwarded-for") ?? "127.0.0.1").split(",")[0] ?? "127.0.0.1";[Low] Unstable tRPC internal import
import { createCallerFactory } from "@trpc/server/unstable-core-do-not-import";
Testing[Medium] New commit mocks
The most recent commit ( sealData: vi.fn().mockResolvedValue("mock-sealed-token"),But the success test case still only checks: expect(body.ok).toBe(true);
expect(response.status).toBe(200);The expect(body.token).toBe("mock-sealed-token");[Low] No unit tests for
A round-trip test (seal then unseal) would validate the token flow and guard against accidental password config changes. These are pure async functions that are straightforward to test. Architecture[Medium] Beyond
This means [Low] Mobile
[Low] Unused font asset
CI[Low] No type-checking or linting step in CI
The workflow now runs tests but has no - name: Type check
run: pnpm check-typesMinor Observations
SummaryThe core restructure is solid. The four items worth addressing before merge:
|
Vercel needs framework and build settings in the app directory since Next.js moved from repo root to apps/web/. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @williamluke4's task in 3m 3s —— View job Code Review: Turborepo Monorepo RestructureTasks
The restructure is well-executed. Package boundaries are sensible, the Turborepo pipeline is correct, and the Bearer token auth is a clean addition for the mobile client. Previous reviews identified several issues; here's the current status plus three fixes I've prepared locally (push blocked by repo permissions — apply them manually or grant push access to the GitHub App). Prepared Fixes (ready to apply)The following three fixes are trivial but blocked from being pushed. Apply them to close out the review: 1. Missing The expect(body.ok).toBe(true);
expect(response.status).toBe(200);
+ expect(body.token).toBe("mock-sealed-token");2. IP extraction undefined with
- const ip = (opts.req.headers.get("x-forwarded-for") ?? "127.0.0.1").split(
- ","
- )[0];
+ const ip =
+ (opts.req.headers.get("x-forwarded-for") ?? "127.0.0.1").split(",")[0] ??
+ "127.0.0.1";3. Unstable tRPC internal import —
- import { createCallerFactory } from "@trpc/server/unstable-core-do-not-import";
+ import { createCallerFactory } from "@sarafu/api/trpc";Remaining Issues (not yet addressed)[Medium] Bearer token returned unconditionally — The sealed token is sent to every client, including web browsers that don't need it. While the token is encrypted (iron-session seal), returning it unnecessarily widens the exposure surface. Gate it on a request header: const wantToken = req.headers.get("x-client-type") === "mobile";
return NextResponse.json({ ok: true, ...(wantToken ? { token } : {}) });[Low] No type-checking or lint step in CI — The workflow runs - name: Type check
run: pnpm check-types[Low] No token revocation —
[Low] Mobile: isAuthenticated: !!state.token,A token expired on the server still returns // In httpBatchLink options or a terminating link:
onError(error) {
if (error.data?.code === "UNAUTHORIZED") auth.signOut();
}[Low]
The [Info]
[Info] Unused font asset —
[Info] Login screen is a placeholder — Only contains UI text describing what will be implemented. Fine for a scaffold, but worth tracking as a follow-up issue before any user-facing deployment. What's Good
|
Vercel reads config from the Root Directory, which is still `.` (repo root). The previous `apps/web/vercel.json` was ignored. Moving it to the root with `outputDirectory: "apps/web/.next"` and a turbo-filtered build command fixes framework detection without changing the project's Root Directory setting. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @williamluke4's task in 2m 19s —— View job Code Review: Turborepo Monorepo RestructureTasks
Three previous reviews identified the same set of issues. None of the four actionable items have been addressed in the subsequent commits ( Blocking Issues (unchanged from prior reviews)[Medium]
it("returns ok on valid SIWE verification", async () => {
const response = await POST(makeRequest());
const body = await response.json();
expect(body.ok).toBe(true);
expect(response.status).toBe(200);
// Missing: expect(body.token).toBe("mock-sealed-token");
});The mock is already in place — this is a one-line fix. [Medium] IP extraction is
// get-ip.ts (correct)
return forwarded.split(",")[0]?.trim() ?? "127.0.0.1";
// context.ts (broken)
const ip = (opts.req.headers.get("x-forwarded-for") ?? "127.0.0.1").split(",")[0];
// ^ string | undefinedFix: const ip =
(opts.req.headers.get("x-forwarded-for") ?? "127.0.0.1").split(",")[0] ??
"127.0.0.1";Non-Blocking Issues (unchanged from prior reviews)[Low] Unstable tRPC internal import
import { createCallerFactory } from "@trpc/server/unstable-core-do-not-import";
[Low] No
The workflow only runs - name: Type check
run: pnpm check-types[Medium] Bearer token returned unconditionally
The sealed token is returned to every client. Web browsers using cookie auth don't need it and shouldn't receive it. While the token is encrypted (iron-session seal), reducing unnecessary exposure is good hygiene: const wantToken = req.headers.get("x-client-type") === "mobile";
return NextResponse.json({ ok: true, ...(wantToken ? { token } : {}) });New Observations
Mobile: no global UNAUTHORIZED handler in tRPC client
The tRPC client has no error handler. When a stored token expires server-side, // In the links array, before httpBatchLink:
() => (op) =>
observable((observer) =>
next(op).subscribe({
...observer,
error(err) {
if (err.data?.code === "UNAUTHORIZED") signOut();
observer.error(err);
},
})
),
SummaryThe monorepo restructure itself is solid — Turborepo pipeline, package boundaries, Bearer token auth implementation, and the Vercel configuration are all correct. The four outstanding items from prior reviews remain unaddressed. The two most important:
|
Vercel's Next.js builder requires `next` in the Root Directory's package.json to detect the framework version, even when the actual app lives in apps/web/. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @williamluke4's task in 3m 39s —— View job Code Review: Turborepo Monorepo RestructureTasks
The monorepo restructure is well-executed: package boundaries are sensible, Turborepo pipeline is correct, and the Vercel configuration commits ( Prepared Fixes (apply manually — 3 files, ~6 lines)1. expect(body.ok).toBe(true);
expect(response.status).toBe(200);
+ expect(body.token).toBe("mock-sealed-token");2. - const ip = (opts.req.headers.get("x-forwarded-for") ?? "127.0.0.1").split(
- ","
- )[0];
+ const ip =
+ (opts.req.headers.get("x-forwarded-for") ?? "127.0.0.1").split(",")[0] ??
+ "127.0.0.1";3. - import { createCallerFactory } from "@trpc/server/unstable-core-do-not-import";
+ import { createCallerFactory } from "@sarafu/api/trpc";
// ...
- export const caller = createCallerFactory()(appRouter)(createCallerContext);
+ export const caller = createCallerFactory(appRouter)(createCallerContext);Remaining Issues[Low] No The workflow only runs - name: Type check
run: pnpm check-types[Medium] Bearer token returned unconditionally — The comment says "Return a sealed token for non-cookie clients (e.g. mobile)" but the implementation returns it to every client including web browsers. The token is encrypted (iron-session seal), but there's no reason to send it to clients that don't need it: // Gate on a request header
const wantToken = req.headers.get("x-client-type") === "mobile";
return NextResponse.json({ ok: true, ...(wantToken ? { token } : {}) });[Low] No token revocation —
Architecture[Medium] Despite
The peer dep documents this, but a comment block in [Low] Mobile: no global UNAUTHORIZED handler
// Before httpBatchLink in the links array:
() => (op) =>
observable((observer) =>
next(op).subscribe({
...observer,
error(err) {
if (err.data?.code === "UNAUTHORIZED") signOut();
observer.error(err);
},
})
),[Low]
[Low] Unused font asset
Minor Observations
SummaryThe restructure itself is solid. The four items to resolve before merge:
|
- Add missing token assertion in verify test - Fix IP extraction type safety for noUncheckedIndexedAccess - Replace unstable tRPC import with stable @sarafu/api/trpc export - Add pnpm check-types step to CI workflow Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @williamluke4's task in 1m 36s —— View job Claude finished @williamluke4's task —— View job Code Review: Turborepo Monorepo Restructure (Updated)Tasks
The latest commit resolves all four blocking items from prior reviews. The PR is close to merge-ready. One medium-severity issue remains open. Resolved in
|
| Issue | File | Status |
|---|---|---|
Missing body.token assertion |
verify.test.ts:109 |
Fixed — expect(body.token).toBe("mock-sealed-token") added |
IP extraction string | undefined |
context.ts:11-13 |
Fixed — ?? "127.0.0.1" fallback added |
| Unstable tRPC internal import | _app.ts:5,25 |
Fixed — imports from @sarafu/api/trpc, call site also corrected |
No pnpm check-types in CI |
.github/workflows/test.yaml:38-39 |
Fixed — step added |
Remaining Issues
[Medium] Bearer token returned unconditionally
apps/web/src/app/api/auth/verify/route.ts:88-91
The comment says "Return a sealed token for non-cookie clients (e.g. mobile)" but the implementation returns it to every client:
// Comment says non-cookie clients only — implementation sends to all clients
const token = await createAuthToken(address, chainId);
return NextResponse.json({ ok: true, token });Web browsers authenticating via SIWE receive the sealed Bearer token even though they use cookie auth and have no use for it. The token is encrypted (iron-session seal), but unnecessary exposure is poor hygiene. The test at line 109 also now asserts the token is always present, which will need updating if this is gated:
// Gate on a request header
const wantToken = req.headers.get("x-client-type") === "mobile";
return NextResponse.json({ ok: true, ...(wantToken ? { token } : {}) });Low-Priority / Pre-merge Optional
[Low] No token revocation documentation
packages/api/src/auth/index.ts:97-107
createAuthToken seals with a 30-day TTL. If a token is compromised, rotating NEXT_IRON_PASSWORD is the only recourse (which invalidates all sessions globally). A comment on the function would make this limitation explicit for future maintainers.
[Low] Mobile: isAuthenticated doesn't detect server-side token expiry
apps/mobile/lib/auth.tsx:62
isAuthenticated: !!state.token,A stored token that has expired server-side still returns true until a tRPC call fails with UNAUTHORIZED. Since trpc.tsx has no error handler, users won't be redirected to login automatically. A link-level error handler would address this gracefully:
// In the links array (trpc.tsx), before httpBatchLink:
() => (op) =>
observable((observer) =>
next(op).subscribe({
...observer,
error(err) {
if (err.data?.code === "UNAUTHORIZED") signOut();
observer.error(err);
},
})
),Info / Follow-up
app.jsonmissingextra.apiBaseUrl—apps/mobile/lib/api-url.tsfalls back tohttp://localhost:3000when noextra.apiBaseUrlis configured.apps/mobile/app.jsonhas noextrafield, so any EAS/TestFlight build will hit localhost. Must be configured before production deployment.- Unused font asset —
apps/mobile/assets/fonts/SpaceMono-Regular.ttfis bundled but_layout.tsxonly loadsFontAwesome.font. Either register it inuseFontsor remove the file. - Login screen is a placeholder —
apps/mobile/app/(auth)/login.tsxcontains only descriptive text. Fine for a scaffold; track as a follow-up issue before any user-facing deployment. @sarafu/apihas Next.js couplings —auth/index.ts,auth/check-rate-limit.ts, andauth/get-ip.tsall import fromnext/headersornext/server. ThepeerDependencies: { next: ">= 15" }documents this, but a note in the affected files or package README would make the constraint clear to future contributors adding non-Next.js consumers.
What's Good
- All four items identified as blocking across multiple prior reviews are now resolved.
- CI now runs both tests and type checking — TypeScript errors will be caught on PRs.
- The tRPC caller factory is now using the stable public API via
@sarafu/api/trpc. - The IP extraction is now safe under
noUncheckedIndexedAccess. - Turborepo pipeline, package boundaries, and the Vercel monorepo configuration are all correct.
expo-secure-storefor token persistence andtokenRefpattern intrpc.tsxto keep the auth header fresh without re-creating the client are both good implementation choices.
1 participant
Summary
apps/web/, shared server code extracted into@sarafu/api,@sarafu/contracts,@sarafu/core, and@sarafu/schemaspackagesapps/mobile/with tRPC integration and auth scaffoldingtsconfig.base.json, and deduplicates@types/reactvia pnpm overridespnpm/action-setup@v4Test plan
pnpm buildcompletes successfully across all packagespnpm testpasses for all packagespnpm check-typespasses with no errorstokenfield in its response🤖 Generated with Claude Code