Repository navigation
[feature/db] Implement data persistence layer for creating and editing alarms - #2
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesThe PR replaces mock alarm data with a Drizzle/Expo SQLite database, generated migrations, and a Zustand store. Alarm conversion helpers, form persistence, screen CRUD actions, lifecycle loading, and empty and error states are added. Alarm persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RootLayout
participant useAlarmStore
participant dbReady
participant SQLiteAlarms
RootLayout->>useAlarmStore: loadAlarms()
useAlarmStore->>dbReady: await database migrations
useAlarmStore->>SQLiteAlarms: fetch ordered alarms
SQLiteAlarms-->>useAlarmStore: alarm rows
useAlarmStore-->>RootLayout: populated alarm state
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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
🧹 Nitpick comments (2)
src/hooks/useCreateAlarmForm.ts (1)
165-166: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNew comments added to a
.tsfile.Per coding guidelines, comments shouldn't be added unless explicitly requested. These two "Seam" markers appear to be new additions in this diff.
As per coding guidelines, "Do not add comments unless explicitly requested; match the existing code style" (
**/*.{ts,tsx}).Also applies to: 192-192
🤖 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 `@src/hooks/useCreateAlarmForm.ts` around lines 165 - 166, Remove the newly added “Seam 2” and corresponding “Seam” marker comments from the useCreateAlarmForm hook, leaving the surrounding implementation and dependency array unchanged.Source: Coding guidelines
src/hooks/useAlarmById.ts (1)
5-16: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueNarrow
useAlarmByIdto string ids
Alarm.idis always a string, and the only caller passesalarmIdas a string route param, so thenumberunion member is unnecessary.🤖 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 `@src/hooks/useAlarmById.ts` around lines 5 - 16, Update the useAlarmById hook’s id parameter to accept only string (plus null/undefined), and remove the number union from its type while preserving the existing lookup and null handling.
🤖 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 `@AGENTS.md`:
- Line 28: Update the Persistence guidance in AGENTS.md to retain the npm run
db:generate requirement while removing the instruction to commit its output;
instead, state that generated artifacts should be included when requested,
consistent with the repository-wide no-commit rule.
In `@src/app/`(main)/index.tsx:
- Around line 49-56: Update both turnAllOnOff and performDelete to handle bulk
store-operation failures: start the per-alarm update/delete calls in parallel,
await them with failure handling, and prevent rejected promises from escaping
their onPress/void callers. Ensure performDelete resets selectedIds and
editEnabled regardless of whether an operation fails, while preserving the
existing success behavior and reporting failures appropriately.
- Around line 43-47: Update toggleEnabled to handle promise rejection from
useAlarmStore.getState().updateAlarm: attach error handling that reports the
failure to the user through the existing UI feedback mechanism, while preserving
the current enabled-state toggle behavior.
In `@src/hooks/useCreateAlarmForm.ts`:
- Around line 183-200: Update the persist function in the alarm save flow so
insertAlarm/updateAlarm failures are surfaced to the user instead of only
logged, and move router.back() out of finally so navigation occurs only after
persistence succeeds. Preserve the existing success behavior and error details
while keeping the failure path on the current form.
In `@src/store/alarmStore.ts`:
- Around line 43-67: Prevent concurrent initialization in loadAlarms by adding a
synchronous in-flight guard that is set before starting the asynchronous init
work and cleared when initialization completes or fails. Update init/loadAlarms
around the existing _disposer check so only one dbReady/fetchAlarms chain can
create the SQLite listener, while preserving disposer cleanup and allowing
retries after failure.
---
Nitpick comments:
In `@src/hooks/useAlarmById.ts`:
- Around line 5-16: Update the useAlarmById hook’s id parameter to accept only
string (plus null/undefined), and remove the number union from its type while
preserving the existing lookup and null handling.
In `@src/hooks/useCreateAlarmForm.ts`:
- Around line 165-166: Remove the newly added “Seam 2” and corresponding “Seam”
marker comments from the useCreateAlarmForm hook, leaving the surrounding
implementation and dependency array unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1d5169f4-cacf-4a9a-b3a0-2216020c6665
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
AGENTS.mdapp.jsonbabel.config.jsdrizzle.config.tsdrizzle/0000_large_vulture.sqldrizzle/meta/0000_snapshot.jsondrizzle/meta/_journal.jsondrizzle/migrations.jsmetro.config.jspackage.jsonsrc/app/(main)/index.tsxsrc/app/_layout.tsxsrc/data/constants.tssrc/data/conversions.tssrc/data/db.tssrc/data/mockAlarms.tssrc/data/schema.tssrc/data/types.tssrc/hooks/useAlarmById.tssrc/hooks/useCreateAlarmForm.tssrc/store/alarmStore.ts
💤 Files with no reviewable changes (1)
- src/data/mockAlarms.ts
Add a synchronous _initializing guard in init/loadAlarms so only one dbReady/fetchAlarms chain can register the SQLite change listener, while preserving disposer cleanup and allowing retries after failure. Also align AGENTS.md persistence guidance with the no-commit rule.
Home screen: run turnAllOnOff/performDelete updates in parallel via Promise.allSettled, report failures via Alert, and reset edit state regardless of delete errors. toggleEnabled now catches rejections. useCreateAlarmForm: report persist failures to the user and only navigate back on success.
Drop the unused number union from useAlarmById's id parameter (id is always string from the route). Remove Seam 1/2 marker comments from useCreateAlarmForm per the no-comments guideline.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/useAlarmById.ts (1)
18-19: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winExpose initialization failure instead of reporting permanent loading.
If
dbReadyorfetchAlarms()fails,useAlarmStoresetsloadingtofalsebut leavesloadedasfalse. This hook then returnsloading: trueforever while it remains mounted.Expose an initialization error from
useAlarmStoreand consume it here, or define a terminal failure state that the UI can handle. Add a failure-path test.🤖 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 `@src/hooks/useAlarmById.ts` around lines 18 - 19, Update useAlarmById and useAlarmStore so initialization failures from dbReady or fetchAlarms are exposed as a terminal error state instead of leaving loaded false and reporting permanent loading. Have the hook return that error alongside alarm/loading, and add a test covering the failed initialization path.
🤖 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 `@src/hooks/useAlarmById.ts`:
- Line 5: Update useAlarmById so alarm-store initialization failures cannot
leave loaded false while loading remains true; add an explicit initialization
error state or mark loading complete in the failure path, and expose the
resulting state consistently so the edit screen exits “Loading alarm…” instead
of hanging.
---
Outside diff comments:
In `@src/hooks/useAlarmById.ts`:
- Around line 18-19: Update useAlarmById and useAlarmStore so initialization
failures from dbReady or fetchAlarms are exposed as a terminal error state
instead of leaving loaded false and reporting permanent loading. Have the hook
return that error alongside alarm/loading, and add a test covering the failed
initialization path.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f2984487-746f-446d-8a99-10a5a5806e3d
📒 Files selected for processing (5)
AGENTS.mdsrc/app/(main)/index.tsxsrc/hooks/useAlarmById.tssrc/hooks/useCreateAlarmForm.tssrc/store/alarmStore.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- AGENTS.md
- src/app/(main)/index.tsx
- src/hooks/useCreateAlarmForm.ts
Add an initError field to alarmStore, set it in the init failure path and clear it on retry/success. useAlarmById now derives loading as !loaded && !error and exposes error, so the edit screen renders an error view with retry instead of hanging on "Loading alarm…".
This was referenced Aug 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit