Replace ad-hoc parsing with Zod schemas; dedupe shared helpers#4
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis pull request refactors the Pear application to use Zod for runtime data validation, consolidating ad-hoc normalization logic across main and renderer processes. It establishes shared validation schemas, improves IPC type safety, removes the non-fast broker input method, and centralizes timestamp formatting utilities. ChangesZod Schema Validation & IPC Type Safety Refactor
🎯 4 (Complex) | ⏱️ ~45 minutes
Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15505c7d14
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const user = UserInfoSchema.parse(candidate) | ||
| return Object.keys(user).length > 0 ? user : undefined |
There was a problem hiding this comment.
normalizeUserInfo now parses a candidate object that always includes every user key (often with undefined values), so Object.keys(user).length > 0 is effectively always true and this function no longer returns undefined for empty inputs. In paths like mergeUserInfo, that means a sparse/empty whoami response can overwrite previously stored fields with undefined (e.g., clearing cached avatar/identity metadata) instead of leaving prior values intact.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 608a7b7. Zod preserves keys whose value is undefined, so the Object.keys(user).length > 0 guard would always be truthy and mergeUserInfo would shadow previously cached fields (e.g. wipe cachedAvatarUrl on every whoami refresh).
The fix copies only defined entries off the parsed result before the emptiness check, restoring the prior semantics:
const parsed = UserInfoSchema.parse(candidate)
const user: UserInfo = {}
for (const [key, value] of Object.entries(parsed) as Array<[keyof UserInfo, string | undefined]>) {
if (value !== undefined) user[key] = value
}
return Object.keys(user).length > 0 ? user : undefinedGenerated by Claude Code
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| const user = UserInfoSchema.parse(candidate) | ||
| return Object.keys(user).length > 0 ? user : undefined |
There was a problem hiding this comment.
🟡 normalizeUserInfo never returns undefined for record inputs due to Zod preserving keys with undefined values
The new normalizeUserInfo constructs a candidate object with all 7 keys explicitly set (to string | undefined from firstString). When UserInfoSchema.parse(candidate) processes this, Zod preserves all keys in the output even when their values are undefined. This means Object.keys(user).length is always 7 (never 0), so the > 0 check always passes and the function never returns undefined for any record input.
Verified Zod behavior with installed version
UserInfoSchema.parse({ name: undefined, email: undefined, ... }) produces an object with 7 keys (all undefined), confirmed by test. Object.keys(result).length is 7.
The old code built the object conditionally (if (name) user.name = name), so empty inputs produced {} with 0 keys → returned undefined.
This changes behavior of downstream callers like mergeUserInfo and withCachedAvatar (src/main/auth.ts:107-108, src/main/auth.ts:151-152): they'll process/propagate empty-but-truthy user objects instead of undefined, potentially storing user: {} in auth tokens and returning empty user info in auth status.
| const user = UserInfoSchema.parse(candidate) | |
| return Object.keys(user).length > 0 ? user : undefined | |
| const user = UserInfoSchema.parse(candidate) | |
| const hasContent = Object.values(user).some((v) => v !== undefined) | |
| return hasContent ? user : undefined |
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Good catch — fixed in 608a7b7. Took the same approach as your suggestion but materialised a fresh object so the result is Partial<UserInfo>-shaped (no undefined keys leaking into the merge spread in mergeUserInfo / the JSON we write to auth-meta.json):
const parsed = UserInfoSchema.parse(candidate)
const user: UserInfo = {}
for (const [key, value] of Object.entries(parsed) as Array<[keyof UserInfo, string | undefined]>) {
if (value !== undefined) user[key] = value
}
return Object.keys(user).length > 0 ? user : undefinedGenerated by Claude Code
Centralizes validation of disk + IPC payloads in Zod schemas, shares
project/broker-event helpers between the main and renderer processes, and
removes dead code and unsafe casts.
Highlights:
- src/shared/schemas/project.ts: single source of truth for Project /
ProjectRoot / ProjectIntegration shape, used by both the main process
store and the renderer project store (was duplicated ~110 lines x2).
- src/main/schemas.ts: Zod schemas for StoredTokens, AuthMeta, UserInfo,
broker connection.json, generated commit drafts, and the avatar cache
manifest. Replaces `JSON.parse(...) as Record<string, unknown>` plus
hand-rolled type guards.
- ui-store: validate localStorage theme/layout with z.enum instead of
casting `as Theme | null` / `as TerminalLayout | null`.
- git-store: narrow `pear.git.status` return type so the `as FileStatus[]`
cast is gone.
- preload: collapse 4 `as Promise<T>` casts into one generic `invoke<T>`
helper; drop dead `broker:send-input` IPC handler / type / wrapper that
no caller used.
- broker.ts: replace two `(err as { status?: unknown }).status` casts with
a tiny `getErrorStatus` helper; narrow BrokerEvent with `in` operator
instead of accessing fields that don't exist on every variant.
- shared/lib/broker-events.ts: single `compactBrokerEvent` /
`normalizeEventTimestamp` for both main and renderer (was duplicated).
- format.ts: extract `formatClockTime` / `formatRelativeShort` shared by
ChatMessage and ThreadPanel.
normalizeUserInfo fix (review feedback): Zod preserves keys with
`undefined` values from the input candidate, so the previous
`Object.keys(user).length > 0` check would always be true and an empty
whoami payload would shadow previously cached fields via mergeUserInfo
(clearing the cached avatar URL on every refresh). Now strips undefined
entries before the emptiness check, preserving the prior semantics.
Net effect on `tsc -b`: 5 baseline errors fewer than main, 0 new errors,
0 new `as any` / `@ts-ignore` / `@ts-expect-error`. `npm run build`
succeeds.
https://claude.ai/code/session_013TyabW37ec75uPpdFhYkS5
willwashburn
force-pushed
the
claude/refactor-pear-codebase-Talve
branch
from
15505c7 to
608a7b7
Compare
|
@copilot resolve the merge conflicts in this pull request |
Co-authored-by: willwashburn <957608+willwashburn@users.noreply.github.com>
Summary
Centralises validation of disk + IPC payloads in Zod schemas, shares project / broker‑event helpers between main and renderer, removes dead code and unsafe casts. Net ‑350 LOC with zero new
as any/@ts-ignore, and 4 baselinetscerrors fixed.Why
A deep read of
pearsurfaced repeated patterns that hurt readability and safety:src/main/store.tsandsrc/renderer/src/stores/project-store.tshad ~110 lines each of nearly‑identicalnormalizeRoot/normalizeIntegration/normalizeProjecthelpers, all built onas Record<string, unknown>casts.JSON.parse(...) as Teverywhere inauth.ts,avatar-cache.ts,broker.ts— disk and network payloads getting trusted by type assertion.localStorage.getItem(...) as Theme | nullinui-store.tswith no actual validation.(await pear.git.status(path)) as FileStatus[]ingit-store.tsbecause the IPC return type was loose.as Promise<T>sprinkled acrosspreload/index.ts.pear.broker.sendInput— preload exposed it, IPC handler existed, renderer never called it (all input goes throughsendInputFast).formatTimeinChatMessage.tsxandThreadPanel.tsx;compactBrokerEventandnormalizeEventTimestampin bothmain/broker.tsandrenderer/stores/agent-store.ts.What changed
New shared modules
src/shared/schemas/project.ts— single source of truth forProject/ProjectRoot/ProjectIntegration.makeProjectSchema(rootSchema)lets each process specialize the per‑root shape (main stores{id,name,path}; renderer enriches withpathExists).src/main/schemas.ts— Zod schemas forStoredTokens,AuthMeta,UserInfo, brokerconnection.json, generated commit drafts, and the avatar cache manifest.src/shared/lib/broker-events.ts— sharedcompactBrokerEvent+normalizeEventTimestamp.format.ts—formatClockTime,formatRelativeShort(replaces duplicated formatters in chat components).Both tsconfigs and
electron.vite.config.tswere updated with a@shared/*path so main and renderer can both import the shared code.Unsafe casts removed
ui-store.ts:as Theme | null→z.enum([...]).safeParse(localStorage.getItem(...)).git-store.ts: narrowed the IPC return type via a newGitFileStatusinterface inlib/ipc.ts; theas FileStatus[]cast is gone.preload/index.ts: 4×as Promise<T>collapsed into one genericinvoke<T>(channel, ...args)helper.broker.ts: two(err as { status?: unknown }).statuscasts → onegetErrorStatushelper;event.name/event.fromaccess on a discriminated union narrowed with theinoperator instead of casting.Dead code removed
pear.broker.sendInput(preload + IPC handler +PearAPItype entry) — never called.normalizeGeneratedCommitDraft,compactEventText, plus dup helpers folded into the shared module.Bonus baseline cleanups
AppTopBar.tsx: added missing'dm'branch so theAppTabKindswitch is exhaustive (fixes TS2366).broker.ts: narrowedthis.sessions.values().next().valuesogetSessionForAgentreturnsBrokerSession, notBrokerSession | undefined(fixes TS2322)..gitignore: ignore*.tsbuildinfo.Verification
npx tsc -b→ 4 baseline errors fewer thanmain, 0 new errors introduced.npm run build→ clean (✓ built in 7.24s).@ts-ignore/@ts-expect-error/as anyexist in the tree (was already true; preserved).'café','#general', etc.Test plan
npm run dev— open app, load an existing project, confirm projects still parse fromprojects.json.auth.jsondecrypts andwhoamipopulates the avatar.🤖 Generated with Claude Code
https://claude.ai/code/session_013TyabW37ec75uPpdFhYkS5
Generated by Claude Code