refactor(quality): thermo-nuclear review — dedup celebration setters, lift habit helpers to shared - #307
Conversation
… lift habit helpers to shared
Behavior-preserving structural cleanup; shared (1013), web (1619), and mobile (620) suites stay green.
- ui-store: collapse the three near-identical celebration setters (~120 LOC) into shared enqueue/clear
helpers + a createCelebrationSetter factory (~50 LOC); read-then-increment sequence ordering preserved.
- Lift the byte-identical use-habits tree helpers (getTomorrowDateString, findHabitInTree, findHabitInList,
buildOptimisticSkipPatch + HabitTreeNode type) into packages/shared (habit-optimistic.ts); web + mobile
import them instead of duplicating. Mobile's distinct habit-mutation-helpers left untouched.
- mobile use-habit-form: replace 7 per-field useWatch calls + manual assembly with the single
useWatch({ control }) pattern web already uses (identical derived flags).
- calendar-sync: narrow formatCalendarAutoSyncLastSynced's translate param to its real shape, removing the
two `as never` casts at the call site.
No behavior, UI, or contract changes. The two mega-files (habit-form-fields ~1580, habit-list ~1278/1888)
are documented as follow-up decomposition PRs - too large to split safely while keeping web/mobile parity
and every interaction test green in one pass.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
Paired nuclear-review PR: thomasluizon/orbit-api#250 (same thermo-nuclear pass, backend side). |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #307
Scope: PR #307 — refactor(quality): thermo-nuclear review — dedup celebration setters, lift habit helpers to shared
Recommendation: APPROVE
Summary
Behavior-preserving refactoring across 8 files. Deduplicates the three celebration setter methods into a createCelebrationSetter factory, lifts four byte-identical pure helpers from both use-habits.ts files into packages/shared/src/utils/habit-optimistic.ts, aligns mobile useHabitForm watch pattern with web, and removes two as never casts in the calendar-sync call site. No UI, behavior, or API contract is changed. All four subagents returned clean verdicts.
Findings
Critical
None
High
None
Medium
None
Low / Info
None
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | PAIRED |
| i18n-syncer | IN SYNC |
| contract-aligner | N/A (no Zod schema / DTO changes; only client-side utility helpers moved) |
| security-reviewer | N/A (no orbit-api changes) |
Validation
| Check | Result | Details |
|---|---|---|
| Lint | N/A | npm commands unavailable in CI runner; PR author reports lint clean |
| Type check | N/A | npm commands unavailable in CI runner; PR author reports type-check clean |
| Tests | N/A | npm commands unavailable in CI runner; PR author reports shared 1013 · web 1619 · mobile 620 all green |
| Build (api) | N/A | No orbit-api changes |
What's good
createCelebrationSetter factory is correct and tight. The semantics are preserved: clearCelebrationKind delegates to activateNextCelebration (which runs deriveLegacyCelebrationState on the next queued item) when the active celebration is being cleared — and only spreads clearedLegacyState when the cleared kind was queued but not active. This is the right behavior in both branches.
Lifting helpers to shared hits the right DRY level (CLAUDE.md rule 10): getTomorrowDateString, findHabitInTree, findHabitInList, buildOptimisticSkipPatch were byte-identical across both apps. Mobile's habit-mutation-helpers.ts was correctly left alone — its local HabitTreeNode helpers are not identical to the shared ones.
Comment policy is clean: new habit-optimistic.ts exports carry correct /** */ JSDoc; no narration // comments appear in any changed file.
as HabitFormInput cast on useWatch({ control }) is the established pattern (already present in the web hook before this PR) and is the correct approach — without a name param, react-hook-form cannot narrow the return type to the full form shape. No as any or as unknown as X escape hatches anywhere in the diff.
as never removal in calendar-sync/page.tsx is a genuine root-cause fix (CLAUDE.md rule 1): narrowing translate's values param from Record<string, unknown> to Record<string, string | number> matches what next-intl's t() actually accepts, eliminating both casts.
Net −93 LOC with zero behavior change and full suite retained.
Recommendation
Clean diff, tight scope, zero findings. APPROVE.
🤖 Generated with Claude Code
|
There was a problem hiding this comment.
PR Review — #307 refactor(quality): thermo-nuclear review
Decision: APPROVE — no Critical or High findings. Two Medium items flagged as follow-up candidates.
Findings
[MEDIUM] Over-exported helpers in packages/shared
getTomorrowDateString(habit-optimistic.ts:12) andfindHabitInTree(habit-optimistic.ts:19) are re-exported throughpackages/shared/src/utils/index.tsbut have no external callers inapps/weborapps/mobile(grep-confirmed). Both are only called by sibling functions in the same file.- Risk: they widen the
@orbit/sharedpublic API surface unnecessarily and could invite single-platform callers that break parity — the exact problem this lift was meant to prevent. - Fix (non-blocking): remove both from
index.ts; keep them unexported insidehabit-optimistic.ts. The real cross-app exports (buildOptimisticSkipPatch,findHabitInList) stay. - Ref: CLAUDE.md rule 2 — "No just-in-case exports"
[MEDIUM] as HabitFormInput cast replicated into mobile
apps/mobile/hooks/use-habit-form.ts:80usesas HabitFormInputto castDeepPartial<HabitFormInput>fromuseWatch. This mirrors the pre-existing web pattern and doesn't widen toany, so today's tests pass cleanly.- Risk: if a field without a
.default()coercion is ever added tohabitFormSchema,normalizeHabitFormDatasilently receivesundefined. Low blast radius now; medium as the schema grows. - Fix (non-blocking): make
normalizeHabitFormDataacceptDeepPartial<HabitFormInput>in a follow-up shared-utils cleanup. - Ref: CLAUDE.md rule 3
Subagent checks
| Agent | Verdict |
|---|---|
| parity-checker | PAIRED — use-habits.ts mirrors, use-habit-form.ts mirrors, calendar-sync page correct |
| i18n-syncer | IN SYNC — no new strings; en.json / pt-BR.json both at 1,545 keys |
| contract-aligner | N/A — no shared types, endpoint constants, or DTO changes |
| security-reviewer | N/A — no orbit-api changes |
What's well done
- The celebration-setter factory collapses three ~40-line setters into a 12-line factory + two one-liner call sites — correct extraction point, independently testable.
nextCelebrationSequence()cleanly isolates the mutable counter and preserves ordering guarantees identically.formatCalendarAutoSyncLastSyncedfixes the upstream signature instead of papering overas nevercasts — CLAUDE.md rule 1 followed.- Lifting skip-patch helpers to shared is architecturally correct;
habit-mutation-helpers.tscorrectly left alone. - PR body clearly documents what was investigated and intentionally deferred.
1 participant
A thermo-nuclear code-quality review of orbit-ui-mobile (strict maintainability/structure standard). Everything here is behavior-preserving — no UI, behavior, or contract changes — and every suite stays green (shared 1013 · web 1619 · mobile 620). Net −93 LOC across 8 files.
Applied
packages/shared/src/stores/ui-store.ts)setStreakCelebration/setGoalCompletedCelebration/setAllDoneCelebration, ~120 LOC) repeated the same queue → dedup → activate / clear flow. ExtractedenqueueCelebrationItem+clearCelebrationKindhelpers and acreateCelebrationSetter(kind, clearedLegacyState)factory (read-then-increment sequence ordering preserved). ~120 → ~50 LOC.use-habitstree helpers to shared (packages/shared/src/utils/habit-optimistic.ts+ both apps'use-habits.ts)getTomorrowDateString,findHabitInTree,findHabitInList,buildOptimisticSkipPatch(+HabitTreeNode) — into shared; web + mobile import them instead of each keeping a copy. Removed the orphaned imports the deletions left.habit-mutation-helpers.tswas correctly left alone (not truly identical).apps/mobile/hooks/use-habit-form.ts)useWatchcalls + manual object assembly with the singleuseWatch({ control })pattern web already uses.packages/shared/src/utils/calendar-sync.ts+ web call site)formatCalendarAutoSyncLastSynced'stranslateparam to its real shape (Record<string, string | number>), so the web call site drops botht(key as never, values as never)casts.as neverwas papering over an over-strict signature, not a real type gap.Two findings were investigated and correctly left unchanged: the "dead" legacy celebration setters are still used by both platforms, and the emoji options are already single-sourced from one definition (a prior commit fixed it).
Verification:
type-check+lintclean on all 8 changed files;npm testgreen in all three workspaces (the only failing items are the pre-existing local@sentry/*/@playwright/testinstall gap, reproducible onmainand untouched here).Deferred (documented follow-ups — too large to split safely in one pass)
habit-form-fields.tsx(~1580 web / similar mobile) → split per form section (HabitTypeSelector,FrequencySection,ScheduleSection,ReminderSection,ChecklistSection,GoalTagLinkSection,EmojiPickerField), each taking the shareduseHabitFormhelpers as props; keep the file as the orchestrator. Mirror on both platforms.habit-list.tsx(~1278 web / ~1888 mobile) → extractHabitTreeRows(recursion),HabitSelectionBar(bulk actions), auseHabitRowActionshook (optimistic glue), and the empty/loading/filtered state components; isolate mobile's gesture/reanimated wiring in auseHabitRowGesturesso the shared list-shaping logic can converge. Behind the existing behavior tests (no new E2E).Each warrants its own PR with careful web↔mobile parity verification.
🤖 Generated with Claude Code