strict: enable noUncheckedIndexedAccess and fix type-safety issues#8
strict: enable noUncheckedIndexedAccess and fix type-safety issues#8TargiX wants to merge 5 commits into
Conversation
- Enable noUncheckedIndexedAccess in tsconfig for safer indexed access
- Replace .split('T')[0] with .slice(0, 10) to avoid undefined returns
- Guard destructured array patterns from split/map with fallbacks
- Add fallback for Slider onValueChange destructured value
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughEnable strict indexed-access typing, add null-safety guards in time parsing, touch handling, and slider handlers, migrate store action imports to ChangesType Safety and Module Organization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/evening/step-sleep-target.tsx`:
- Line 78: The onValueChange handler currently calls the component state setter
setHours directly; replace that direct state mutation by dispatching the
appropriate store action from lib/store/actions instead: create or use an action
(e.g., setSleepHours or updateHours) and import it into this component, then
change onValueChange={([v]) => setHours(v ?? 8)} to call the action dispatcher
with the new value (v ?? 8) so all state updates flow through the store actions
rather than setHours in the TSX component.
In `@components/morning/step-mood.tsx`:
- Around line 31-32: The handler that reads touch coordinates currently falls
back to (0,0) by using e.touches[0]?.clientX ?? 0 and e.touches[0]?.clientY ??
0; instead, detect when e.touches[0] is missing and return early (ignore the
event) to avoid emitting a bogus top-left point—update the touch handler in
step-mood.tsx (the function that sets clientX/clientY from e.touches, e.g., the
onTouchStart/onTouchMove handler) to check for const touch = e.touches[0]; if
(!touch) return; then use touch.clientX and touch.clientY.
In `@components/morning/step-sleep.tsx`:
- Line 75: The onValueChange handler is directly calling the local setter
setHours; replace this with the appropriate action from lib/store/actions:
import the store action (e.g., the action that updates sleep hours) and call
that action with (v ?? 8) instead of calling setHours. Update the onValueChange
callback in components/morning/step-sleep.tsx to dispatch/use the action from
lib/store/actions (referencing the existing setHours usage to locate the spot)
and remove the direct setHours call so the component mutates state only via
lib/store/actions.
In `@hooks/use-store.ts`:
- Line 20: Replace the UTC-based day key generation in useTodayEntry (currently
using new Date().toISOString().slice(0, 10)) with the local day-key utility from
lib/time/today; import the helper (e.g., today or whatever the module exports)
and call it to produce the key so the variable key in useTodayEntry uses the
local-day value and avoids UTC rollover mismatches.
In `@lib/store.ts`:
- Line 100: getTodayKey() currently builds the day key using new
Date().toISOString().slice(0, 10) which produces a UTC day and can be wrong for
local dates; replace this logic by importing and using the repository's local
day-key utility from lib/time/today (i.e., remove the toISOString slice and call
the exported function from lib/time/today) so getTodayKey returns the correct
local day key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bf3db7ca-30b7-4a8d-a1a8-610ee4e75a4f
📒 Files selected for processing (6)
components/evening/step-sleep-target.tsxcomponents/morning/step-mood.tsxcomponents/morning/step-sleep.tsxhooks/use-store.tslib/store.tstsconfig.json
| step={0.5} | ||
| value={[hours]} | ||
| onValueChange={([v]) => setHours(v)} | ||
| onValueChange={([v]) => setHours(v ?? 8)} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Route slider updates through store actions instead of component state setters.
This still mutates component state directly in a .tsx component. Move this through lib/store/actions per project rule.
As per coding guidelines, "**/*.{tsx,jsx}: Mutate state only via lib/store/actions — never use setState directly in components".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/evening/step-sleep-target.tsx` at line 78, The onValueChange
handler currently calls the component state setter setHours directly; replace
that direct state mutation by dispatching the appropriate store action from
lib/store/actions instead: create or use an action (e.g., setSleepHours or
updateHours) and import it into this component, then change onValueChange={([v])
=> setHours(v ?? 8)} to call the action dispatcher with the new value (v ?? 8)
so all state updates flow through the store actions rather than setHours in the
TSX component.
Source: Coding guidelines
| step={0.5} | ||
| value={[hours]} | ||
| onValueChange={([v]) => setHours(v)} | ||
| onValueChange={([v]) => setHours(v ?? 8)} |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Use lib/store/actions for this update instead of direct state setter.
The handler still updates local component state directly in .tsx. Please route this through store actions to match repository rules.
As per coding guidelines, "**/*.{tsx,jsx}: Mutate state only via lib/store/actions — never use setState directly in components".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/morning/step-sleep.tsx` at line 75, The onValueChange handler is
directly calling the local setter setHours; replace this with the appropriate
action from lib/store/actions: import the store action (e.g., the action that
updates sleep hours) and call that action with (v ?? 8) instead of calling
setHours. Update the onValueChange callback in components/morning/step-sleep.tsx
to dispatch/use the action from lib/store/actions (referencing the existing
setHours usage to locate the spot) and remove the direct setHours call so the
component mutates state only via lib/store/actions.
Source: Coding guidelines
- Create lib/time/today.ts with getLocalTodayKey() to avoid UTC date bug - Replace toISOString().slice(0,10) in lib/store.ts and hooks/use-store.ts - Guard missing touches in step-mood.tsx instead of defaulting to (0,0) Fixes 3 of 5 CodeRabbit comments on PR #8.
Skipped CodeRabbit comments (1 & 3)The |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/store/actions.test.ts (1)
14-15: ⚡ Quick winUse
getTodayKey()directly in these tests instead ofObject.keys(...)[0]!.These assertions currently depend on entry enumeration order and a non-null assertion. Since
updateTodayEntrywrites to the local day key contract, assert bygetTodayKey()to keep the test deterministic and aligned with app date handling.Proposed refactor
+import { getTodayKey } from "`@/lib/time/today`" ... it("creates today's entry from patch", () => { updateTodayEntry({ intention: "Focus" }) const s = getSnapshot() - const key = Object.keys(s.entries)[0]! - expect(s.entries[key]!.intention).toBe("Focus") + const key = getTodayKey() + expect(s.entries[key]).toBeDefined() + expect(s.entries[key]!.intention).toBe("Focus") }) ... it("merges into existing entry", () => { updateTodayEntry({ intention: "A" }) updateTodayEntry({ journal: "Went well" }) const s = getSnapshot() - const key = Object.keys(s.entries)[0]! + const key = getTodayKey() + expect(s.entries[key]).toBeDefined() expect(s.entries[key]!.intention).toBe("A") expect(s.entries[key]!.journal).toBe("Went well") })As per coding guidelines: “Use local day keys via
lib/time/todayfor date handling; never usetoISOString()due to UTC bug.” Based on learnings: the same local day key rule applies for**/*.{ts,tsx,js,jsx}.Also applies to: 22-24
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/store/actions.test.ts` around lines 14 - 15, The test relies on Object.keys(s.entries)[0]! which is fragile and non-deterministic; replace that with the local day key from getTodayKey() and assert against s.entries[getTodayKey()] (e.g., const key = getTodayKey(); expect(s.entries[key]!.intention).toBe("Focus")). Do the same replacement for the other assertions mentioned (lines 22-24) that read entries by enumeration; use getTodayKey() wherever updateTodayEntry or local-day entry access is asserted to ensure deterministic, local-day-based tests.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@lib/store/actions.test.ts`:
- Around line 14-15: The test relies on Object.keys(s.entries)[0]! which is
fragile and non-deterministic; replace that with the local day key from
getTodayKey() and assert against s.entries[getTodayKey()] (e.g., const key =
getTodayKey(); expect(s.entries[key]!.intention).toBe("Focus")). Do the same
replacement for the other assertions mentioned (lines 22-24) that read entries
by enumeration; use getTodayKey() wherever updateTodayEntry or local-day entry
access is asserted to ensure deterministic, local-day-based tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fdc56c19-63d3-4ae4-95a4-aacddb419a85
📒 Files selected for processing (2)
lib/auth/credentials.tslib/store/actions.test.ts
|
🧹 CodeRabbit review janitor pass (5 comments) All 5 CodeRabbit comments are stale — the code already addresses each one:
No changes needed. |
|
🧹 CodeRabbit review janitor pass Verified all 5 inline CodeRabbit comments against current branch state:
All checks green. PR is ready for review/merge. |
1 participant
What changed
Enabled
noUncheckedIndexedAccessin tsconfig and fixed 11 resulting type errors across 5 files.Fixes
lib/store.ts,hooks/use-store.ts:.split("T")[0]→.slice(0, 10)— array indexing after.split()could returnundefinedunder the new flagcomponents/morning/step-mood.tsx:e.touches[0]now guarded with?.+?? 0components/evening/step-sleep-target.tsx: destructured[h, m]from.split(":").map(Number)now usesparts[0] ?? 0; SlideronValueChangevalue gets?? 8fallbackcomponents/morning/step-sleep.tsx: same Slider fallbackWhy
noUncheckedIndexedAccesscatches a common class of bugs where array/record indexing is assumed to always succeed. Every indexed access now correctly returnsT | undefined, forcing explicit handling. This is the single most impactful strictness flag missing from most TS projects.Verification
tsc --noEmit— zero errors (was 11 before fixes)npm run lint— passesRisk
Low. Only adds type-safety guards. Runtime behavior is identical — the new fallbacks match what would have been
undefined → NaNbehavior before, but now explicit and intentional.Summary by CodeRabbit
Refactor
Bug Fixes
Chores
Tests