mobile: habit submit validation + no-comments lint rule + native bottom-sheet (TrueSheet) - #144
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean parity fix. Both mobile modals now gate the submit button on isPending || !formState.isValid, exactly matching the web counterparts (verified at apps/web/components/habits/create-habit-modal.tsx:375 and edit-habit-modal.tsx:199). isPending is still live — used for the cancel button and the loading spinner in each modal — so no dead code. The WHY comment is correct (non-obvious cross-platform invariant). Diff is minimal, scoped, and correct.
Parity with web: the habit create/edit submit button stayed enabled with an empty title on mobile (only `isPending` gated it), so users could fire a doomed submit that the title field's `min(1)` rule rejects. Web already gated on `formState.isValid`. Both platforms use the same react-hook-form + zodResolver config and the same shared `habitFormSchema`, so reading `formState.isValid` is reactive on mobile too. Now mobile gates the submit button (and its disabled style) on `isPending || !form.formState.isValid`, matching web. Cancel stays gated on `isPending` only. Audited every shared form for the same gap — goals (create/edit), tags, auth, and checklist items are already at parity (both platforms behave identically); only the habit create/edit submit differed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
62dac95 to
9c4fba1
Compare
Add a local ESLint rule `local/no-comments` (wired into apps/web and apps/mobile flat configs) that errors on any comment except: `/** */` JSDoc, tooling directives (eslint-disable, @ts-expect-error, /// <reference>, coverage/bundler pragmas), and a WHY note that links an upstream URL. It is autofixable and AST-based, so `//` inside strings, URLs, and regex literals is never touched. Ran the autofix across both apps to remove ~1.2k existing narration comments. Type-check, lint and tests stay green on web and mobile. Tighten CLAUDE.md rule 5 to match: drop the vague "WHY for non-obvious decisions" exception (the loophole that let narration slip in) — a WHY comment now requires a URL, and everything else is banned and lint-enforced. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thomasluizon
changed the title
There was a problem hiding this comment.
Two high-signal issues block this.
1. Autofix bug: empty JSX containers left behind (eslint-rules/no-comments.cjs)
The fixer removes only the comment text from JSX expression container comments, leaving the curly-brace shell. This PR introduces ~47 empty expression containers across both apps — dead code, violating rule 2. Fix the fixer to detect the JSX container pattern and expand the removal range to include the surrounding braces, then re-run --fix in both apps to clean up. See inline suggestion on the fixer.
2. Bare empty catches in preferences.tsx (lines 142, 212)
Stripping the documenting comments turns these into invisible rule-8 violations. The language-preference catch silently drops API mutation errors while the UI has already updated optimistically — real user-facing data loss. Prefer surfacing the error (toast) or add a URL-linked WHY comment if silent failure is genuinely intentional.
…ue-sheet The custom RN Modal + PanResponder sheet could not drag-to-resize: JS gesture and scroll coordination is unreliable on the New Architecture (the same root cause as gorhom's present() no-op). Swap the BottomSheetModal internals to a native sheet so drag-to-resize between snap points, scroll coordination, the dimmed backdrop, the keyboard, and the Android back button are handled on the native thread. Public props are unchanged, so all 18 callers keep working: snapPoints map to detents, and the dirty guard sets dismissible=false and routes the close button + back press to onAttemptDismiss. Removes the now-unused custom machinery (bottom-sheet-snap, bottom-sheet-overlay-controller, and their tests). Native module: needs `expo prebuild` + a fresh APK; cannot be verified headlessly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
thomasluizon
changed the title
There was a problem hiding this comment.
The new commit (78e4426) is clean: the react-native-true-sheet swap removes all the old PanResponder/Animated machinery without introducing any new dead code, workarounds, or contract changes. requestDismiss is properly defined and wired, the dirty-guard semantics are preserved, and the JSDoc on the component is accurate.
That said, the two issues from my prior REQUEST_CHANGES are still open — this PR can't be approved until both are addressed:
1. Empty JSX containers (dead code — rule 2)
The no-comments.cjs fixer only removes the comment text from {/* … */} JSX expression containers, leaving the bare {} shell. Those empty containers are still in the tree:
apps/mobile/components/ui/app-date-picker.tsx— lines 118, 142, 158, 179, 188apps/mobile/components/onboarding/onboarding-welcome.tsx— lines 86, 96, 112apps/mobile/components/ui/app-select.tsx— lines 51, 71
Fix: expand the fixer's removal range to cover the surrounding { and } when the container holds only a comment, then re-run --fix in both apps.
2. Bare empty catches in apps/mobile/app/preferences.tsx (rule 8 — swallowed errors)
- Line 142: the language-mutation catch is empty. The optimistic update (
setSelectedLanguage,i18n.changeLanguage) has already committed in the UI — if the API call fails, the local state and remote state silently diverge. Show an error toast or roll back. - Line 212: the
AsyncStorage.setItemcatch is empty. Arguably lower stakes (local preference), but still a swallowed error. A WHY comment linking the reason is acceptable if silent failure is genuinely intentional here.
…rrors Addresses PR review on #144: - no-comments.cjs: the autofix left empty `{}` shells for `{/* ... */}` JSX expression containers (the comment range excludes the braces). Detect the container pattern and expand the removal to the surrounding braces; re-stripped the ~48 leftover `{}` across both apps. - preferences.tsx: the strip turned two documented intentional catches into bare empty catches (rule 8). Roll back the optimistic update on failure (matching the weekStartMutation pattern) so a failed language change or preference write no longer silently diverges from the persisted/UI state. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Both blocking issues from the prior review are resolved. The no-comments.cjs fixer now correctly detects JSX expression containers and expands the removal range to include the surrounding braces — all ~48 leftover {} shells are gone. The preferences.tsx catches now roll back optimistic state on failure: language change reverts selectedLanguage + i18n.changeLanguage; the AsyncStorage catch reverts showGeneralOnToday. Both patterns match the existing weekStartMutation rollback in the same file.
The react-native-true-sheet swap is clean: public props are unchanged, the dirty-guard semantics are preserved via dismissible + onAttemptDismiss, requestDismiss is correctly wired to both the close button and the back-press handler, and the deleted bottom-sheet-snap / bottom-sheet-overlay-controller machinery and their tests are fully removed with no dangling references.
Minor suggestion (non-blocking): apps/mobile/CLAUDE.md line 31 still says "Bottom-sheet modals use RN Modal + Animated" — worth updating to react-native-true-sheet so future sessions get correct architectural guidance.
|
1 participant
Three things bundled into this branch (per request). The first two are mergeable as-is; the third needs a native rebuild.
1. fix(mobile): Create/Edit Habit submit gated on validity (parity with web)
Submit stayed enabled with an empty title on mobile (only
isPending); web already gated onformState.isValid. Now mobile gates the submit button + its disabled style onisPending || !form.formState.isValid; cancel stays onisPending. Audited all shared forms — goals/tags/auth/checklist were already at parity.2. chore(lint): forbid narration comments + strip every existing one
local/no-commentsESLint rule (both apps' flat configs,eslint-rules/no-comments.cjs). Errors on any comment except/** */JSDoc, tooling directives (eslint-disable,@ts-expect-error,/// <reference>), and a WHY note linking an upstream URL. Autofixable + AST-based (never touches//in strings/URLs/regex).A narration comment now fails
npm run lint, which gates PRs.3. feat(mobile): native bottom-sheet via react-native-true-sheet⚠️ needs prebuild
The custom RN
Modal+PanRespondersheet could not drag-to-resize — JS gesture/scroll coordination is unreliable on the New Architecture (same root cause as gorhom'spresent()no-op).BottomSheetModal's internals now use@lodev09/react-native-true-sheet(native Fabric sheet): drag-to-resize between snap points, scroll coordination, dimmed backdrop, keyboard, and Android back are handled on the native thread.snapPoints→detents; dirty guard setsdismissible=falseand routes close/back toonAttemptDismiss.bottom-sheet-snap,bottom-sheet-overlay-controller+ tests).npx expo prebuild+ a fresh dev/APK build. This part cannot be verified headlessly — needs an on-device check of: open/close, drag-to-resize, scroll-to-footer, keyboard in the create/edit forms, and the dirty-guard confirm.Verification
🤖 Generated with Claude Code