Repository navigation
[feature/alarm-core] Implement core alarm system - #3
Conversation
📝 WalkthroughWalkthroughThis PR adds persistent, cross-platform alarm scheduling. It adds Android and iOS native modules, Expo integration, notifications, conflict detection, alarm UI flows, configurable task parameters, database migrations, and unit tests. ChangesAlarm scheduler
Estimated code review effort: 5 (Critical) | ~120 minutes 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: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (25)
native/ios/AlarmSchedulerModule.swift-35-38 (1)
35-38: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReject stale non-snoozed one-shot alarms.
A past
triggerAtvalue is converted to a one-second trigger. A restored or delayed regular alarm can therefore fire immediately. Apply the stale-alarm guard before creating the trigger, and preserve only theisSnoozedexception.As per coding guidelines, “Reuse the
${alarmId}${weekday}hash-code scheduling identifier scheme and replicate the stale-alarm guard, including theisSnoozedexception.”🤖 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 `@native/ios/AlarmSchedulerModule.swift` around lines 35 - 38, Update the one-shot scheduling flow around UNTimeIntervalNotificationTrigger to reject alarms whose triggerAt is in the past unless opts.isSnoozed is true, before creating the trigger. Reuse the existing `${alarmId}${weekday}` hash-code scheduling identifier scheme and replicate its stale-alarm guard behavior, preserving normal future-alarm scheduling.Source: Coding guidelines
native/ios/AlarmSchedulerModule.swift-51-54 (1)
51-54: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReturn the actual notification authorization result.
requestAuthorizationreturnsfalsewhen the user declines permission, butrequestExactAlarmPermissionalways returnstrue. The JavaScript layer can record permission as granted and schedule alarms that iOS will not deliver.Proposed fix
AsyncFunction("requestExactAlarmPermission") { () -> Bool in - try await Self.ensureNotificationAuthorization() - return true + return try await Self.ensureNotificationAuthorization() } - private static func ensureNotificationAuthorization() async throws { + private static func ensureNotificationAuthorization() async throws -> Bool { let center = UNUserNotificationCenter.current() let granted = try await center.notificationSettings() if granted.authorizationStatus != .authorized && granted.authorizationStatus != .provisional { - try await center.requestAuthorization(options: [.alert, .sound, .badge]) + return try await center.requestAuthorization(options: [.alert, .sound, .badge]) } + return true }As per coding guidelines, “Request exact-alarm, notification, and audio-picker permissions at runtime as specified by the platform documentation.”
Also applies to: 91-97
🤖 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 `@native/ios/AlarmSchedulerModule.swift` around lines 51 - 54, Update requestExactAlarmPermission in AlarmSchedulerModule to return the actual Bool result from Self.ensureNotificationAuthorization instead of always returning true, so the JavaScript layer reflects whether notification permission was granted. Keep the existing authorization flow in ensureNotificationAuthorization and propagate its result through any related permission call sites referenced by the same behavior.Source: Coding guidelines
native/ios/AlarmSchedulerModule.swift-21-21 (1)
21-21: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftImplement the notification-to-alarm event lifecycle.
The module declares
onAlarmFired, but no path callssendEvent("onAlarmFired", ...).forceDismissFiringcan emitonAlarmDismissedonly whenplayerSnapshotexists, but the publicplayAlarmSoundcall always passesnilas the snapshot. A scheduled iOS notification therefore cannot activate the alarm task flow or establish dismissal state through this module.Add notification delivery and response handling that loads the alarm snapshot, emits
onAlarmFired, and initializes playback state before dismissal.Also applies to: 56-70
🤖 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 `@native/ios/AlarmSchedulerModule.swift` at line 21, Implement notification delivery and response handling in the alarm module so scheduled notifications load the associated alarm snapshot, emit onAlarmFired through sendEvent, and initialize playerSnapshot/playback state before any dismissal action. Update playAlarmSound and forceDismissFiring as needed so notification-triggered alarms establish dismissal state even when the public call provides no snapshot, while preserving the existing onAlarmDismissed event behavior.native/ios/AlarmSchedulerModule.swift-46-48 (1)
46-48: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUse the shared weekly scheduling identifiers for cancellation.
scheduleWeeklyreceives its identifier from the caller, but this method invents colon-based identifiers. This does not match the required${alarmId}${weekday}hash-code scheme. Active weekly notification requests can remain scheduled after an alarm is disabled.As per coding guidelines, “Reuse the
${alarmId}${weekday}hash-code scheduling identifier scheme and replicate the stale-alarm guard, including theisSnoozedexception.”🤖 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 `@native/ios/AlarmSchedulerModule.swift` around lines 46 - 48, Update cancelAllForAlarm in AlarmSchedulerModule so it removes pending requests using the same shared weekly scheduling identifiers generated by scheduleWeekly, not the current colon-based strings. Reuse the ${alarmId}${weekday} hash-code scheme for the weekly identifiers, and make the cancellation logic mirror the stale-alarm guard used in scheduling, including preserving the isSnoozed exception when deciding what to cancel.Source: Coding guidelines
AGENTS.md-29-29 (1)
29-29: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftCompile the iOS module after
pod install.
buildIos()only runspod install; it does not compileAlarmSchedulerModule.swift. Add anxcodebuildstep for the generated workspace and app scheme afterpod install, and ensure its failure causesbuild:nativeto fail. UpdateAGENTS.mdto describe the actual verification.🤖 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 `@AGENTS.md` at line 29, The native iOS build flow described for buildIos() currently stops after pod install; update the script documentation and implementation to run xcodebuild against the generated workspace and app scheme afterward, propagating any xcodebuild failure so build:native fails. Revise the Native module build section in AGENTS.md to describe this compilation and failure behavior accurately.Source: Coding guidelines
scripts/build-native-module.js-50-50 (1)
50-50: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winInjection (CWE-78): Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection')
Reachability: Internal
Use argument-array process execution instead of shell command string.
Line 50 interpolates
gradlew(derived from the working directory path) into a shell command string passed toexecSync(). If a developer runs this script from a checkout path containing shell metacharacters (for example, backticks or semicolons), the shell parses them as command syntax and executes injected commands with the developer's permissions.Replace
execSync()withexecFileSync()to pass the Gradle executable and task as separate arguments. This avoids shell interpretation and works on Windows without the need for a shell command string.🤖 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 `@scripts/build-native-module.js` at line 50, Update the Gradle invocation in the build script to use argument-array process execution via execFileSync instead of the shell command string passed to run/execSync. Pass gradlew as the executable and :${MODULE_GROUP}:assembleRelease as a separate argument, preserving androidDir as the working directory and avoiding shell interpretation across platforms.Source: Linters/SAST tools
native/android/src/main/java/expo/modules/alarmscheduler/AlarmReceiver.kt-13-16 (1)
13-16: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd a tolerance window to the stale-alarm guard.
The guard requires an exact match on
HOUR_OF_DAYandMINUTE.scheduleAlarmAtfalls back tosetAndAllowWhileIdlewhen exact alarms are not permitted or when aSecurityExceptionoccurs. That API is inexact, and Doze throttles delivery, so the broadcast can arrive several minutes late. In that case the guard fails, the receiver returns, and the alarm never rings for that occurrence.Compare the delivery time against the scheduled instant with a bounded tolerance. This keeps the documented guard and the
isSnoozedexception.🐛 Proposed fix
private fun staleGuardPasses(snapshot: AlarmSnapshotData): Boolean { if (snapshot.isSnoozed) return true - val now = Calendar.getInstance() - return now.get(Calendar.DAY_OF_WEEK) == weekdayIndexToCalendarDay(snapshot.weekday) && - now.get(Calendar.HOUR_OF_DAY) == snapshot.hour && - now.get(Calendar.MINUTE) == snapshot.minute + val now = System.currentTimeMillis() + val scheduled = nextWeeklyTrigger( + snapshot.weekday, + snapshot.hour, + snapshot.minute, + now - TOLERANCE_MS, + ) + return now - scheduled in 0..TOLERANCE_MS } + + private companion object { + const val TOLERANCE_MS = 15 * 60 * 1000L + }Based on learnings and the coding guideline "Reuse the
${alarmId}${weekday}hash-code scheduling identifier scheme and replicate the stale-alarm guard, including theisSnoozedexception": confirm the tolerance value against the authoritativedocs/section before you change the guard.Also applies to: 31-37
🤖 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 `@native/android/src/main/java/expo/modules/alarmscheduler/AlarmReceiver.kt` around lines 13 - 16, Update staleGuardPasses to compare the current delivery time with the scheduled instant using the bounded tolerance specified in the authoritative docs/ section, rather than requiring exact HOUR_OF_DAY and MINUTE equality. Preserve the existing isSnoozed exception and the stale-alarm logging/return behavior in AlarmReceiver.Sources: Coding guidelines, Learnings
native/android/src/main/java/expo/modules/alarmscheduler/AlarmSoundService.kt-34-40 (1)
34-40: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAdd a fallback to post the notification directly when
startForegroundServicefails on Android 12 and later.On Android 12 and later,
startForegroundServicethrowsForegroundServiceStartNotAllowedExceptionwhen called outside a permitted exemption window. Exact alarms (viasetExactAndAllowWhileIdle) are exempt from this restriction, but whenscheduleAlarmAtfalls back tosetAndAllowWhileIdledue to missing permissions, the app loses the exemption. The current code catches this exception and logs only; the alarm produces no sound and no notification, leaving the user without feedback.Post the notification directly to
NotificationManagerwhen the service start fails. The notification channel atALARM_CHANNEL_IDis already configured withIMPORTANCE_HIGHandsetBypassDnd(true), so it will deliver the alarm to the user even when the foreground service cannot start.This applies to both locations: the
runCatchingblock at lines 34–40 (instart()) and the block at lines 67–69 (inonStartCommand()). After making changes, runnpm run build:nativeto verify the module compiles.Code locations
Lines 34–40 (companion object
start()):runCatching { if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { context.startForegroundService(intent) } else { context.startService(intent) } }.onFailure { Log.e("AlarmSoundService", "Failed to start service", it) }Lines 67–69 (in
onStartCommand()):runCatching { ServiceCompat.startForeground(this, ALARM_NOTIFICATION_ID, notification, type) }.onFailure { Log.e("AlarmSoundService", "startForeground failed", it) }🤖 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 `@native/android/src/main/java/expo/modules/alarmscheduler/AlarmSoundService.kt` around lines 34 - 40, Update both failure paths in AlarmSoundService: in the companion object start() block and in onStartCommand(), keep the existing runCatching flow but, inside onFailure, fall back to posting the alarm notification directly through NotificationManager when startForegroundService or ServiceCompat.startForeground fails on Android 12+. Use the existing ALARM_CHANNEL_ID and ALARM_NOTIFICATION_ID symbols so the notification still reaches the user even when the foreground service cannot start.native/android/src/main/java/expo/modules/alarmscheduler/AlarmStore.kt-21-36 (1)
21-36: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winOpen the Expo SQLite database from its actual Android path.
Expo SQLite 57.0.1 stores
"alarm_database"at${context.filesDir}/SQLite/alarm_databasewithout a.dbsuffix.getDatabasePath()resolves to a different directory, soAlarmStorereturns no alarms andBootReceiverdoes not re-arm them after reboot. Use the Expo path and log missing or failed database opens.🤖 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 `@native/android/src/main/java/expo/modules/alarmscheduler/AlarmStore.kt` around lines 21 - 36, Update AlarmStore’s init path resolution so it opens expo-sqlite’s actual database location under context.filesDir/SQLite for the "alarm_database" file instead of relying on getDatabasePath(). Keep the existing read-only SQLiteDatabase.openDatabase flow in AlarmStore, but point it at the Expo SQLite path and preserve the null fallback when the database is missing or cannot be opened. Add logging in the AlarmStore open path for the missing-database and open-failure cases so BootReceiver failures are diagnosable.native/android/src/main/java/expo/modules/alarmscheduler/AlarmReceiver.kt-10-27 (1)
10-27: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRe-arm the next weekly occurrence inside AlarmReceiver instead of depending on the JS layer.
AlarmManager.setExactAndAllowWhileIdleandsetAndAllowWhileIdleare one-shot. The current implementation emitsonAlarmFiredto notify the JS layer, which then callsresetAlarm()to schedule the next occurrence. This depends on the app process starting and the JS listener executing, which is not guaranteed when the app remains backgrounded after the alarm fires.Add a call to
scheduleAlarmAt(context, snapshot, nextWeeklyTrigger(...))in AlarmReceiver to schedule the next weekly trigger directly from the native layer. This ensures recurrence survives even if the app process does not start. BootReceiver already uses this pattern to re-arm alarms after boot; apply the same approach here for event-driven re-arming.🤖 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 `@native/android/src/main/java/expo/modules/alarmscheduler/AlarmReceiver.kt` around lines 10 - 27, Update AlarmReceiver.onReceive to re-arm the weekly alarm natively after a successful fire instead of relying on AlarmSchedulerModule.instance?.emitAlarmFired(snapshot) and JS resetAlarm() flow. After the existing sound/deep-link handling, compute the next weekly trigger with nextWeeklyTrigger(...) and call scheduleAlarmAt(context, snapshot, ...) so the next occurrence is scheduled even if the app process never starts. Keep the current staleGuardPasses and deep-link behavior unchanged, and mirror the re-arming approach already used by BootReceiver.native/android/src/main/java/expo/modules/alarmscheduler/AlarmSchedulerModule.kt-159-176 (1)
159-176: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSynchronize snapshot time fields with computed trigger time.
The
toData()method reads snapshot fields fromopts.payload, not from top-leveloptsfields. WhenscheduleWeeklycomputestriggerAtfromopts.weekday,opts.hour, andopts.minute, these values do not flow into the snapshot. If a caller populates only the top-level fields and omits payload fields, the snapshot retains default values (weekday=0,hour=0,minute=0), andstaleGuardPasses()silently drops every fire unless the current time is exactly 00:00 on Sunday.
scheduleOneShothas the same risk: the snapshot carries notriggerAt, onlypayload.isSnoozed. The guard only passes whenisSnoozedis true; otherwise, it requires the current time to match the storedweekday,hour, andminute. For a one-shot timer with unset payload fields, the guard will fail.Copy the top-level options into the snapshot after calling
toData()to ensure the snapshot contains the values used to compute the alarm trigger time:Proposed fix
AsyncFunction("scheduleWeekly") { opts: ScheduleWeeklyOptsRecord -> - val snapshot = opts.payload.toData(opts.identifier, opts.soundUri) + val snapshot = opts.payload.toData(opts.identifier, opts.soundUri).copy( + alarmId = opts.alarmId, + weekday = opts.weekday, + hour = opts.hour, + minute = opts.minute, + ) val triggerAt = nextWeeklyTrigger(Apply the same fix to
scheduleOneShot.Current JS call sites populate payloads correctly, but the module contract must not depend on caller discipline.
🤖 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 `@native/android/src/main/java/expo/modules/alarmscheduler/AlarmSchedulerModule.kt` around lines 159 - 176, Update both scheduleWeekly and scheduleOneShot after payload.toData() creates the snapshot: copy the corresponding top-level scheduling fields from opts into the snapshot before calling scheduleAlarmAt. Ensure the snapshot’s weekday, hour, minute, and triggerAt values match those used to compute each alarm’s trigger time, while preserving the existing return values and scheduling flow.src/app/_layout.tsx-87-112 (1)
87-112: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftSurface a store initialization failure so reconciliation is not skipped silently.
reconcileSchedulesruns only after both stores setloaded: true.useAlarmStore.initanduseAlarmRegistrationsStore.initsetinitErrorand leaveloadedasfalsewhen the database fails to open (src/store/alarmStore.ts:30-116 and src/store/alarmRegistrationsStore.ts:31-131). In that casetryReconcilenever fires, no alarm is re-armed after a reboot, and the user receives no signal.Subscribe to
initErrorin addition toloaded, and log or surface the failure.🤖 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/app/_layout.tsx` around lines 87 - 112, The reconciliation gate in _layout currently only watches loaded, so init failures in useAlarmStore or useAlarmRegistrationsStore can block reconcileSchedules without any signal. Update the tryReconcile flow to also track initError from both stores, and when either store reports an init error, log or surface that failure instead of waiting forever on loaded. Keep the existing loaded-based success path intact, and anchor the change around tryReconcile, runOnceLoaded, and the two store getters.src/app/_layout.tsx-72-85 (1)
72-85: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle rejection from
getInitialURL()and frominit().Line 72 starts a promise chain with no
.catch. The async callback also awaitsuseAlarmFiringStore.getState().init()on line 77. If either rejects, the result is an unhandled promise rejection, and the persisted active alarm is never restored. Add a rejection handler so the failure is logged.🐛 Proposed fix
- Linking.getInitialURL().then(async (url) => { - if (url) { - handleAlarmUrl(url, router); - return; - } - await useAlarmFiringStore.getState().init(); - const persisted = useAlarmFiringStore.getState().activeSnapshot; - if (persisted) { - router.replace({ - pathname: "/alarm", - params: snapshotToQueryParams(persisted), - }); - } - }); + Linking.getInitialURL() + .then(async (url) => { + if (url) { + handleAlarmUrl(url, router); + return; + } + await useAlarmFiringStore.getState().init(); + const persisted = useAlarmFiringStore.getState().activeSnapshot; + if (persisted) { + router.replace({ + pathname: "/alarm", + params: snapshotToQueryParams(persisted), + }); + } + }) + .catch((e) => console.warn("initial alarm URL handling failed", e));🤖 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/app/_layout.tsx` around lines 72 - 85, Update the Linking.getInitialURL promise flow in the layout initialization to catch rejections from both getInitialURL() and the awaited useAlarmFiringStore.getState().init(). Add a rejection handler that logs the failure, while preserving the existing URL handling and persisted alarm restoration behavior on success.src/hooks/useAlarmDismissal.ts-9-14 (1)
9-14: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winIsolate each dismissal step so one failure cannot block the rest.
The four steps run sequentially with no error handling. If
stopAlarmSound()rejects, for example because the native module is missing,forceDismissFiring(),clearActive(), androuter.dismissTonever run. The user then stays on the task screen while the alarm keeps ringing, andactiveSnapshotstays set, so a laterdismissOldAlarmIfActiveacts on stale state.Both call sites use
void dismiss()(src/app/(alarm)/tasks/phone-shaking.tsx:13 and the math task screen), so the rejection is dropped without a log.Wrap the native calls so the state clear and the navigation always run.
🐛 Proposed fix
return useCallback(async () => { - await stopAlarmSound(); - await getAlarmScheduler().forceDismissFiring(); - useAlarmFiringStore.getState().clearActive(); - router.dismissTo("/(main)"); + try { + await stopAlarmSound(); + } catch (e) { + console.warn("stopAlarmSound failed during dismissal", e); + } + try { + await getAlarmScheduler().forceDismissFiring(); + } catch (e) { + console.warn("forceDismissFiring failed during dismissal", e); + } + useAlarmFiringStore.getState().clearActive(); + router.dismissTo("/(main)"); }, [router]);As per coding guidelines: "Use one
useAlarmDismissal()hook that wrapsstopAlarmSound()and the native dismiss callback; every dismissal task must call it." The hook satisfies that rule; this comment addresses only its failure behavior.🤖 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/useAlarmDismissal.ts` around lines 9 - 14, Update the dismissal callback returned by useAlarmDismissal so stopAlarmSound and forceDismissFiring each fail independently without preventing subsequent steps. Ensure clearActive and router.dismissTo always execute, while preserving the existing dismissal order and callback dependencies.Source: Coding guidelines
src/hooks/useAlarmNotifications.ts-64-73 (1)
64-73: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass
snapshottodismissOldAlarmIfActivein theonAlarmFiredhandler.Line 68 calls
dismissOldAlarmIfActive()with no argument. The other three call sites pass the incoming snapshot. Without the argument, theincoming.alarmId === oldSnapshot.alarmIdguard on line 52 cannot apply. If the same alarm fires again, for example after a snooze re-trigger, the handler runsstopAlarmSound(),forceDismissFiring(), andresetAlarm(oldSnapshot)against the alarm that is starting. It then callssetActive(snapshot)for that same alarm. The reset can cancel or clear scheduling state for the alarm that is currently ringing.🐛 Proposed fix
(snapshot) => { void (async () => { - await dismissOldAlarmIfActive(); + await dismissOldAlarmIfActive(snapshot); useAlarmFiringStore.getState().setActive(snapshot); navigateToAlarm(router, snapshot); })(); },🤖 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/useAlarmNotifications.ts` around lines 64 - 73, Update the onAlarmFired handler in useAlarmNotifications so the async callback passes the incoming snapshot into dismissOldAlarmIfActive, matching the other call sites. Keep the existing setActive(snapshot) and navigateToAlarm(router, snapshot) flow unchanged, and use the snapshot guard path in dismissOldAlarmIfActive to avoid dismissing the alarm that just fired.src/alarms/sound.ts-12-14 (1)
12-14: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winApply
soundUriFromSnapshotto the weekly scheduling path.The helper converts the
"Default"sentinel tonull, as required by the native contract (native/index.ts:7-15). Two calls tonative.scheduleWeeklyinsrc/alarms/scheduling.tspasssoundUri: alarm.sounddirectly:
- Line 48 (weekly scheduling loop)
- Line 147 (in
reconcileSchedules)When an alarm stores
"Default", the native layer receives the literal string instead ofnulland does not select the device default sound.🤖 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/alarms/sound.ts` around lines 12 - 14, Update the weekly scheduling path in scheduling.ts so both native.scheduleWeekly call sites use soundUriFromSnapshot(alarm) instead of passing alarm.sound directly. This affects the weekly scheduling loop and the reconcileSchedules flow, and it should preserve existing behavior for non-default sounds while converting the "Default" sentinel to null before reaching the native contract.src/alarms/sound.ts-4-6 (1)
4-6: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winInput Validation Bypass (CWE-20): Improper Input Validation
Reachability: External · Exploitability: Moderate
Reachability path
● Entry src/app/_layout.tsx:41 handleAlarmUrl │ ▼ ● Hop src/hooks/useAlarmNotifications.ts │ ▼ ● Hop src/alarms/scheduling.ts:66 resetAlarm │ ▼ ● Sink src/alarms/sound.tsValidate the sound URI scheme before passing it to the native layer.
parseAlarmSnapshotaccepts arbitrarysoundvalues, allowing external deep links and notification payloads to reach Android’s unvalidatedMediaPlayer.setDataSource(). Restrictsoundto"Default"or sandboxfile://URIs before playback.🤖 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/alarms/sound.ts` around lines 4 - 6, Update playAlarmSound to validate soundUri before calling getAlarmScheduler().playAlarmSound: allow only the literal "Default" or file:// URIs, and reject all other values, including null or external URI schemes, without passing them to the native layer.Source: Coding guidelines
src/app/_layout.tsx-41-60 (1)
41-60: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winOther (CWE-346): Origin Validation Error
Reachability: External
Reachability path
● Entry src/app/_layout.tsx:41 handleAlarmUrl │ ▼ ● Hop src/hooks/useAlarmNotifications.ts │ ▼ ● Hop src/alarms/scheduling.ts:66 resetAlarm │ ▼ ● Sink src/alarms/sound.tsAdd path and alarm-persistence validation to
handleAlarmUrl.
handleAlarmUrlprocesses every incoming deep-link URL without validating the path or confirming the alarm exists. Native code constructs deep-links asbrainlyalarmexpo://alarm?alarmId=...&task=...&difficulty=...&sound=.... However,Linking.parsedoes not enforce the path; an external app can sendbrainlyalarmexpo://any-path?alarmId=<attacker-chosen-id>andhandleAlarmUrlwill process it.
parseAlarmSnapshotvalidates only that analarmIdparameter exists. It does not check whether that ID is persisted in the database. The function applies defaults to task, difficulty, and other fields, so an attacker can construct a complete spoofed alarm state.An external attacker can exploit this in two ways:
- Send a deep-link with a fake
alarmIdto present a full-screen modal alarm with attacker-chosen task, difficulty, and sound.- Send a deep-link with a different
alarmIdto triggerdismissOldAlarmIfActive, which silences the currently-firing alarm viastopAlarmSound()andforceDismissFiring().Check
parsed.pathequals"alarm"before parsing the snapshot. Then callawait useAlarmStore.getState().getAlarmById(snapshot.alarmId)and return if the ID does not exist.Proposed guard
function handleAlarmUrl( url: string, router: ReturnType<typeof useRouter>, ): void { const parsed = Linking.parse(url); + const path = (parsed.path ?? "").replace(/^\/+/, ""); + if (path !== "alarm") return; const queryParams = (parsed.queryParams ?? {}) as Record< string, string | string[] | undefined >; const snapshot = parseAlarmSnapshot(queryParams); if (!snapshot) return; void (async () => { + const known = await useAlarmStore.getState().getAlarmById(snapshot.alarmId); + if (!known) return; await dismissOldAlarmIfActive(snapshot);🤖 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/app/_layout.tsx` around lines 41 - 60, Update handleAlarmUrl to reject deep links unless Linking.parse(url).path is exactly "alarm" before calling parseAlarmSnapshot, and then validate the parsed snapshot against persistence by awaiting useAlarmStore.getState().getAlarmById(snapshot.alarmId) and returning early when no alarm is found. Keep the existing dismissOldAlarmIfActive, useAlarmFiringStore.getState().setActive, and router.replace flow unchanged for valid alarm links only.src/store/alarmRegistrationsStore.ts-74-82 (1)
74-82: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnblock reconciliation when
initfails.The error path sets
initErrorand clearsloading, but leavesloadedatfalse.src/app/_layout.tsx:62-120runsreconcileSchedules()only after both stores reportloaded. A single transient database failure therefore disables schedule reconciliation for the whole app session, andload()retries only if it is called again.Mark the load attempt as settled and let reconciliation proceed with the current record set, or expose a retry that
_layoutcan call after aninitError.🤖 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/store/alarmRegistrationsStore.ts` around lines 74 - 82, Update the `alarmRegistrationsStore.init` failure path so a failed load still marks the attempt as settled by setting `loaded` to true or otherwise exposing a retry path that `_layout` can use after `initError`. Keep `loading` and `_initializing` cleanup as-is, and ensure the `reconcileSchedules()` gate in `_layout` can proceed with the current record set instead of being blocked indefinitely by a transient `init` failure.src/store/alarmRegistrationsStore.ts-91-122 (1)
91-122: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUpdate
recordsin the mutators instead of relying only on the change listener.
upsert,remove, andremoveForAlarmwrite to SQLite but never touchrecords.getAll,getForAlarm, andhasreadrecordsonly, andreconcileSchedulesinsrc/alarms/scheduling.tsusesregistry.getAll()to find orphaned registrations. Two gaps follow:
- If
initfailed, no change listener exists, sorecordsnever reflects any write.- The listener callback is asynchronous relative to the awaited mutation, so a read after
await registry.upsert(...)can still observe the old set.Apply the same change to
recordsinside each mutator, and keep the listener as a reconciliation path.🛡️ Proposed fix to keep local state in sync
upsert: async (alarmId, type) => { await dbReady; await db .insert(alarmRegistrationsTable) .values({ alarmId, type }) - .onConflictDoUpdate({ - target: [ - alarmRegistrationsTable.alarmId, - alarmRegistrationsTable.type, - ], - set: { alarmId, type }, - }); + .onConflictDoNothing({ + target: [ + alarmRegistrationsTable.alarmId, + alarmRegistrationsTable.type, + ], + }); + set((s) => + s.records.some((r) => r.alarmId === alarmId && r.type === type) + ? s + : { records: [...s.records, { alarmId, type }] }, + ); }, remove: async (alarmId, type) => { await dbReady; await db .delete(alarmRegistrationsTable) .where( and( eq(alarmRegistrationsTable.alarmId, alarmId), eq(alarmRegistrationsTable.type, type), ), ); + set((s) => ({ + records: s.records.filter( + (r) => !(r.alarmId === alarmId && r.type === type), + ), + })); }, removeForAlarm: async (alarmId) => { await dbReady; await db .delete(alarmRegistrationsTable) .where(eq(alarmRegistrationsTable.alarmId, alarmId)); + set((s) => ({ + records: s.records.filter((r) => r.alarmId !== alarmId), + })); },Run the following script to confirm the
onConflictDoNothingsignature for the pinned Drizzle version and that no consumer depends on an update-on-conflict:#!/bin/bash # Description: Verify drizzle-orm version and registration-store consumers. set -euo pipefail fd -t f 'package.json' -d 2 --exec jq -r '.dependencies["drizzle-orm"] // empty' {} rg -nP --type=ts --type=tsx -C3 'useAlarmRegistrationsStore\.getState\(\)\.\w+' src🤖 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/store/alarmRegistrationsStore.ts` around lines 91 - 122, Update records directly within the upsert, remove, and removeForAlarm mutators so reads immediately reflect each awaited database change, including when initialization failed; preserve the change listener as a reconciliation path. In upsert, add the registration only when the insert succeeds without an existing conflict, and remove matching entries in remove and removeForAlarm using the existing alarmId/type keys.src/alarms/scheduling.ts-127-134 (1)
127-134: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winCancel the pending snooze when an alarm is disabled.
snoozeAlarmschedules a one-shot undersnoozeIdentifierFor(alarm.id)and records a"snooze"registration. The disabled branch here cancels only the seven weekly identifiers and removes only the"weekly"registration.setAlarmhas the same gap at Line 34. A snooze scheduled before the user disabled the alarm therefore stays registered and can still fire after a restart.Cancel the snooze identifier and remove the
"snooze"registration in both disabled paths.🐛 Proposed fix for the disabled branch
if (!alarm.enabled) { for (let w = 0; w <= 6; w++) { await native.cancel(identifierFor(alarm.id, w)); } + await native.cancel(snoozeIdentifierFor(alarm.id)); await registry.remove(alarm.id, "weekly"); + await registry.remove(alarm.id, "snooze"); continue; }Apply the same removal in
setAlarm:if (!alarm.enabled) { await registry.remove(alarm.id, "weekly"); + await registry.remove(alarm.id, "snooze"); return; }🤖 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/alarms/scheduling.ts` around lines 127 - 134, Update the disabled-alarm branches in both setAlarm and the alarm loop to cancel snoozeIdentifierFor(alarm.id) and remove the corresponding "snooze" registry entry, in addition to the existing weekly cancellation and removal.src/app/(alarm)/tasks/memory-game/[rounds]/[difficulty].tsx-23-25 (1)
23-25: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate the
difficultyparam instead of casting it.The cast asserts a type that the route cannot guarantee.
src/app/(alarm)/alarm.tsxbuilds this route from a validated snapshot, but a deep link can supply any string, for example/tasks/memory-game/3/easy. That value reaches the difficulty table inuseMemoryGameas a missing key, and the task screen becomes unusable. The user must complete the task to dismiss the alarm, so the screen has no safe fallback.Check the value against the allowed set, as
roundsalready does with its numeric fallback.🛡️ Proposed fix to validate the param
const rounds = Number(roundsParam) || 3; - const difficulty = (difficultyParam ?? "Normal") as - "Easy" | "Normal" | "Hard"; + const DIFFICULTIES = ["Easy", "Normal", "Hard"] as const; + type Difficulty = (typeof DIFFICULTIES)[number]; + const difficulty: Difficulty = DIFFICULTIES.includes( + difficultyParam as Difficulty, + ) + ? (difficultyParam as Difficulty) + : "Normal";
Difficultyis already exported from@/data/types. Import it and reuse the shared constant list fromsrc/data/constants.tsinstead of declaring the tuple in the screen.As per coding guidelines: "Preserve the documented difficulty tables."
🤖 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/app/`(alarm)/tasks/memory-game/[rounds]/[difficulty].tsx around lines 23 - 25, Validate difficultyParam in the memory-game screen instead of asserting its type: import the shared Difficulty type and difficulty values from the existing data modules, then fall back to "Normal" when the route value is not an allowed difficulty. Reuse the shared constant list and preserve the documented difficulty tables while keeping valid Easy, Normal, and Hard values unchanged.Source: Coding guidelines
src/alarms/scheduling.ts-27-36 (1)
27-36: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftScope
forceDismissFiring()to the scheduled alarm. WhenactiveSnapshot?.alarmId === alarm.id, callforceDismissFiring(); otherwise skip it so scheduling another alarm does not stop the firing alarm.🤖 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/alarms/scheduling.ts` around lines 27 - 36, Update setAlarm so native.forceDismissFiring() is called only when activeSnapshot?.alarmId matches alarm.id; skip it for different or absent active alarms while preserving the existing cancellation and disabled-alarm handling.src/app/(alarm)/alarm.tsx-74-100 (1)
74-100: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPass
effectiveSnapshottoresetAlarmandsnoozeAlarm.Lines 28-40 build
effectiveSnapshotto replace staletask,difficulty,roundCount, andsoundvalues on a snoozed payload. The three handlers then pass the rawsnapshotinstead.snoozeAlarmcopies that payload into the next snooze schedule and derives the sound from it, so the stale task, difficulty, round count, and sound survive every re-snooze and reach the rescheduled one-shot inresetAlarm.The
snapshot!assertions are also unnecessary. Reaching Line 74 already proveseffectiveSnapshotis set.🐛 Proposed fix to use the resolved snapshot
const handleBegin = () => { - void resetAlarm(snapshot!); + void resetAlarm(effectiveSnapshot); if (effectiveSnapshot.task === "None") { void dismiss(); return; } @@ const handleOff = () => { - void resetAlarm(snapshot!); + void resetAlarm(effectiveSnapshot); void dismiss(); }; const handleSnooze = () => { - void snoozeAlarm(snapshot!); + void snoozeAlarm(effectiveSnapshot); useAlarmFiringStore.getState().clearActive(); router.dismissTo("/(main)"); };🤖 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/app/`(alarm)/alarm.tsx around lines 74 - 100, Update handleBegin, handleOff, and handleSnooze to pass effectiveSnapshot to resetAlarm and snoozeAlarm instead of snapshot!, removing the unnecessary non-null assertions. Preserve the existing navigation, dismissal, and active-alarm clearing behavior while ensuring resolved task, difficulty, round count, and sound values are used.src/alarms/weeklyTrigger.ts-28-39 (1)
28-39: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCompare against the unrounded current time.
At
10:00:30, a10:00candidate equals the minute-roundednowDayjs. The function then returns a timestamp that is already 30 seconds old instead of advancing to next week.Keep seconds and milliseconds on the comparison value. Add a regression test with
nowat10:00:30.Proposed fix
- const nowDayjs = dayjs(now).second(0).millisecond(0); + const nowDayjs = dayjs(now);🤖 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/alarms/weeklyTrigger.ts` around lines 28 - 39, Update the comparison in weeklyTrigger.ts inside the weekly trigger calculation so the candidate is checked against the unrounded current time instead of nowDayjs, while still rounding the returned candidate to minute precision. Preserve the existing reset handling and the dayjs candidate construction, but compare against the original now value (with seconds and milliseconds intact) to decide whether to add 7 days; add a regression test in the weekly trigger path using now at 10:00:30 to confirm it advances to next week.
🟡 Minor comments (4)
native/android/src/main/java/expo/modules/alarmscheduler/AlarmStore.kt-77-84 (1)
77-84: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLog the
daysparse failure.
parseDaysreturns an empty list and drops the exception.BootReceivertreats an empty day list as every weekday, so a malformed value silently re-arms seven alarms. Log the error so a schema mismatch is visible. detekt also flags the swallowed exception on line 81.🐛 Proposed fix
private fun parseDays(raw: String): List<Int> { return try { val arr = JSONArray(raw) (0 until arr.length()).map { arr.getInt(it) } } catch (e: Exception) { + Log.w("AlarmStore", "Could not parse alarm days", e) emptyList() } }Add
import android.util.Logat the top of the file.🤖 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 `@native/android/src/main/java/expo/modules/alarmscheduler/AlarmStore.kt` around lines 77 - 84, Update AlarmStore.parseDays to stop swallowing the JSON parse exception silently; keep the emptyList fallback, but log the failure when JSONArray(raw) or arr.getInt(it) throws so malformed days values are visible. Use the existing parseDays method as the fix point, add the Android Log import if needed, and preserve the current return behavior for valid inputs and fallback cases.Source: Linters/SAST tools
native/android/src/main/java/expo/modules/alarmscheduler/BootReceiver.kt-35-36 (1)
35-36: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSet
soundUrionly when the stored value is a URI.
alarm.soundsupplies both the display name andsoundUri. If the column stores a label such as"Default",AlarmSoundService.startPlaybackcallsUri.parse("Default"),setDataSourcethrows, and the retry path plays the default alarm sound. The alarm still rings, so the effect is a misleading snapshot and an avoidable failure path.🐛 Proposed fix
- sound = alarm.sound ?: "Default", - soundUri = alarm.sound, + sound = alarm.sound ?: "Default", + soundUri = alarm.sound?.takeIf { it.startsWith("file://") || it.startsWith("content://") },Based on the coding guideline "Copy picked audio into the sandbox and persist its
file://URI rather than the document-picker URI": confirm which form thesoundcolumn stores.🤖 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 `@native/android/src/main/java/expo/modules/alarmscheduler/BootReceiver.kt` around lines 35 - 36, Update the alarm construction in BootReceiver so soundUri is populated only when alarm.sound contains a stored URI, leaving it null for display labels such as "Default"; preserve alarm.sound as the display value and ensure the resulting value matches the persisted sandbox file:// URI format used by AlarmSoundService.startPlayback.Source: Coding guidelines
src/alarms/types.ts-1-7 (1)
1-7: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove
getAlarmSchedulerfrom theexport typeblock in src/alarms/types.ts.
getAlarmScheduleris a function value, not a type. It is declared asexport function getAlarmScheduler(): AlarmSchedulerin src/alarms/AlarmScheduler.ts and imported there from the native module. When placed in anexport type { ... }block, TypeScript re-exports the binding as type-only. If code importsgetAlarmSchedulerfrom@/alarms/typesand attempts to call it, the build fails. The re-export currently has no consumers.Proposed fix
export type { AlarmEventName, AlarmScheduler, ScheduleOneShotOpts, ScheduleWeeklyOpts, - getAlarmScheduler, } from "alarm-scheduler";🤖 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/alarms/types.ts` around lines 1 - 7, Remove getAlarmScheduler from the export type block in the alarms types module; retain the existing type-only exports and leave the function’s value export in AlarmScheduler.ts unchanged.src/hooks/useAlarmNotifications.ts-82-113 (1)
82-113: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDeduplicate notification activation by
alarmId.When the app receives a foreground notification and the user taps it, both listeners can activate the same alarm. The native implementations replace the current player, but both handlers can still call
setActiveandrouter.replace. Use a synchronous in-flightalarmIdguard or serialize activation before the firstawait. Extract the shared activation sequence and keepclearDeliveredAlarmNotificationsresponse-only.🤖 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/useAlarmNotifications.ts` around lines 82 - 113, Deduplicate the foreground and response handlers by extracting their shared alarm activation sequence into a helper guarded synchronously by in-flight alarmId state before the first await. Ensure duplicate notifications skip setActive, sound playback, and navigateToAlarm, while retaining clearDeliveredAlarmNotifications only in the response handler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f7d702c-5023-4395-9eb2-1fe8e80ad170
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (59)
.gitattributesAGENTS.mdapp.jsondrizzle/0001_equal_jetstream.sqldrizzle/0002_bent_zemo.sqldrizzle/0003_misty_moon.sqldrizzle/meta/0001_snapshot.jsondrizzle/meta/0002_snapshot.jsondrizzle/meta/0003_snapshot.jsondrizzle/meta/_journal.jsondrizzle/migrations.jseslint.config.jsjest.config.jsnative/.gitignorenative/android/build.gradlenative/android/src/main/AndroidManifest.xmlnative/android/src/main/java/expo/modules/alarmscheduler/AlarmIntents.ktnative/android/src/main/java/expo/modules/alarmscheduler/AlarmReceiver.ktnative/android/src/main/java/expo/modules/alarmscheduler/AlarmSchedulerModule.ktnative/android/src/main/java/expo/modules/alarmscheduler/AlarmScheduling.ktnative/android/src/main/java/expo/modules/alarmscheduler/AlarmSoundService.ktnative/android/src/main/java/expo/modules/alarmscheduler/AlarmStore.ktnative/android/src/main/java/expo/modules/alarmscheduler/BootReceiver.ktnative/app.plugin.jsnative/expo-module.config.jsonnative/index.tsnative/ios/AlarmScheduler.podspecnative/ios/AlarmSchedulerModule.swiftnative/package.jsonpackage.jsonscripts/build-native-module.jssrc/alarms/AlarmScheduler.tssrc/alarms/conflicts.tssrc/alarms/scheduling.tssrc/alarms/sound.tssrc/alarms/types.tssrc/alarms/weeklyTrigger.tssrc/app/(alarm)/_layout.tsxsrc/app/(alarm)/alarm.tsxsrc/app/(alarm)/tasks/math-equation/[rounds]/[difficulty].tsxsrc/app/(alarm)/tasks/memory-game/[rounds]/[difficulty].tsxsrc/app/(alarm)/tasks/phone-shaking.tsxsrc/app/(main)/index.tsxsrc/app/_layout.tsxsrc/data/activeAlarm.tssrc/data/conversions.tssrc/data/schema.tssrc/data/types.tssrc/hooks/useAlarmDismissal.tssrc/hooks/useAlarmNotifications.tssrc/hooks/useCreateAlarmForm.tssrc/notifications/AlarmNotifications.tssrc/store/alarmFiringStore.tssrc/store/alarmRegistrationsStore.tssrc/tasks/useMathEquation.tssrc/tasks/useMemoryGame.tstest/unit/alarms/conflicts.test.tstest/unit/alarms/weeklyTrigger.test.tstsconfig.json
| let components = DateComponents() | ||
| components.weekday = Self.iosWeekday(fromJsWeekday: opts.weekday) | ||
| components.hour = opts.hour | ||
| components.minute = opts.minute |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect a match until `components` is declared with `var`.
rg -n -U 'let components = DateComponents\(\)\n\s*components\.(weekday|hour|minute)\s*=' \
native/ios/AlarmSchedulerModule.swiftRepository: HoweiAY/brainly_alarm_expo
Length of output: 282
🏁 Script executed:
sed -n '20,32p' native/ios/AlarmSchedulerModule.swiftRepository: HoweiAY/brainly_alarm_expo
Length of output: 734
🏁 Script executed:
# Verify that DateComponents is a struct and that assigning to properties
# of a let-bound struct is indeed a Swift compile error
python3 - <<'PYEOF'
# We can verify Swift behavior by checking Apple documentation or testing logic:
# In Swift, DateComponents is a struct from Foundation
# Structs are value types, and you cannot mutate properties of value types bound with `let`
# This is a fundamental Swift language rule
# The error message would be something like:
# "Cannot assign to property: 'components' is a 'let' constant"
# Let's confirm by understanding the Swift semantics:
print("Swift Semantics Verification:")
print("=" * 50)
print("1. DateComponents is a struct (value type) from Foundation")
print("2. Structs are value types in Swift")
print("3. Properties of value types bound by 'let' are immutable")
print("4. Attempting to assign to properties of a 'let' struct causes:")
print(" Compile error: Cannot assign to property")
print()
print("Therefore:")
print("- Line 25: 'let components = DateComponents()' creates an immutable binding")
print("- Lines 26-28: Attempting to mutate properties will fail at compile time")
print("- Fix: Change 'let' to 'var' to allow mutation")
print()
print("Conclusion: The review comment is CORRECT")
PYEOFRepository: HoweiAY/brainly_alarm_expo
Length of output: 757
Change let to var for components declaration on line 25.
In Swift, DateComponents is a struct (value type). You cannot assign to properties of a struct bound by let. Lines 26–28 mutate components.weekday, components.hour, and components.minute, which causes a compile error. Change line 25 to declare components with var to allow these mutations.
Proposed fix
- let components = DateComponents()
+ var components = DateComponents()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let components = DateComponents() | |
| components.weekday = Self.iosWeekday(fromJsWeekday: opts.weekday) | |
| components.hour = opts.hour | |
| components.minute = opts.minute | |
| var components = DateComponents() | |
| components.weekday = Self.iosWeekday(fromJsWeekday: opts.weekday) | |
| components.hour = opts.hour | |
| components.minute = opts.minute |
🤖 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 `@native/ios/AlarmSchedulerModule.swift` around lines 25 - 28, Change the
DateComponents declaration in the alarm scheduling setup from let to var so the
subsequent weekday, hour, and minute property assignments compile.
…tError before reconcile
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/tasks/params.ts (1)
4-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd unit tests for task parameter normalization.
Test missing, invalid, fractional, negative, and non-finite
roundsParamvalues. Test accepted and rejected difficulty values.As per coding guidelines, “prefer unit tests for pure math, memory, and time logic.”
🤖 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/tasks/params.ts` around lines 4 - 15, Add unit tests for the pure parseTaskParams function covering missing, invalid, fractional, negative, and non-finite roundsParam values, verifying normalization to the expected minimum/default rounds. Also cover accepted taskDifficulties values and rejected or missing difficultyParam values, verifying the "Normal" fallback.Source: Coding guidelines
🤖 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/useAlarmNotifications.ts`:
- Line 23: Replace the single activeActivationId guard in the activation flow
with serialized execution or a monotonically increasing activation token so
different alarm IDs cannot overlap. Update the activation logic around the
hook’s await points to validate the token before mutating the firing store,
playing sound, or navigating, and ensure cleanup only affects the activation
that owns the token rather than clearing a newer activation’s state.
In `@src/tasks/params.ts`:
- Line 8: Update the rounds parsing expression in params.ts to validate the
numeric value with Number.isFinite before applying Math.floor, so values such as
roundsParam="Infinity" fall back to the existing default instead of producing an
infinite completion target.
---
Nitpick comments:
In `@src/tasks/params.ts`:
- Around line 4-15: Add unit tests for the pure parseTaskParams function
covering missing, invalid, fractional, negative, and non-finite roundsParam
values, verifying normalization to the expected minimum/default rounds. Also
cover accepted taskDifficulties values and rejected or missing difficultyParam
values, verifying the "Normal" fallback.
🪄 Autofix
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: e07b165f-8740-4eb5-9092-86cf36873535
📒 Files selected for processing (12)
src/alarms/scheduling.tssrc/alarms/types.tssrc/alarms/weeklyTrigger.tssrc/app/(alarm)/alarm.tsxsrc/app/(alarm)/tasks/math-equation/[rounds]/[difficulty].tsxsrc/app/(alarm)/tasks/memory-game/[rounds]/[difficulty].tsxsrc/app/_layout.tsxsrc/hooks/useAlarmDismissal.tssrc/hooks/useAlarmNotifications.tssrc/store/alarmRegistrationsStore.tssrc/tasks/params.tstest/unit/alarms/weeklyTrigger.test.ts
💤 Files with no reviewable changes (1)
- src/alarms/types.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/app/(alarm)/tasks/memory-game/[rounds]/[difficulty].tsx
- test/unit/alarms/weeklyTrigger.test.ts
- src/alarms/weeklyTrigger.ts
- src/hooks/useAlarmDismissal.ts
- src/app/(alarm)/tasks/math-equation/[rounds]/[difficulty].tsx
- src/store/alarmRegistrationsStore.ts
- src/app/(alarm)/alarm.tsx
- src/app/_layout.tsx
- src/alarms/scheduling.ts
| import { useEffect } from "react"; | ||
| import { AppState } from "react-native"; | ||
|
|
||
| let activeActivationId: string | null = null; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Serialize activation for different alarm IDs.
activeActivationId only suppresses duplicate IDs. It does not prevent two different alarms from running concurrently.
If alarm A pauses at Line 69, alarm B can replace the guard at Line 67. Alarm A can then resume, set itself active, and navigate after alarm B. Its finally block also clears alarm B's guard at Line 79.
Use a serialized activation queue or a monotonically increasing activation token. Check the token after each await before changing the firing store, playing sound, or navigating.
Also applies to: 66-80
🤖 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/useAlarmNotifications.ts` at line 23, Replace the single
activeActivationId guard in the activation flow with serialized execution or a
monotonically increasing activation token so different alarm IDs cannot overlap.
Update the activation logic around the hook’s await points to validate the token
before mutating the firing store, playing sound, or navigating, and ensure
cleanup only affects the activation that owns the token rather than clearing a
newer activation’s state.
…alarm with dismissOldAlarmIfActive
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/useAlarmNotifications.ts`:
- Around line 55-58: Update the incoming alarm replacement flow in the hook
around resetAlarm so a changed incoming.alarmId first stops the previous alarm
sound and dismisses the previous firing state before resetting the snapshot. Use
the owner-aware native dismissal API for that previous firing state, while
preserving the existing no-incoming cleanup behavior.
🪄 Autofix
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: 5fbf5010-85f6-46e7-a13d-88320bf945ff
📒 Files selected for processing (2)
src/hooks/useAlarmNotifications.tssrc/tasks/params.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tasks/params.ts
| if (!incoming) { | ||
| await stopAlarmSound(); | ||
| await getAlarmScheduler().forceDismissFiring(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Stop the previous alarm before replacing its snapshot.
When incoming has a different alarmId, this branch skips stopAlarmSound() and forceDismissFiring(). Line 59 then resets the previous snapshot. If the replacement activation does not play sound, the previous alarm can continue playing while the firing store represents the new alarm.
Stop the previous sound before resetAlarm(). Also use an owner-aware native dismissal for the previous firing state.
Proposed minimum fix
if (incoming && incoming.alarmId === oldSnapshot.alarmId) return;
+ await stopAlarmSound();
if (!incoming) {
- await stopAlarmSound();
await getAlarmScheduler().forceDismissFiring();
}🤖 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/useAlarmNotifications.ts` around lines 55 - 58, Update the incoming
alarm replacement flow in the hook around resetAlarm so a changed
incoming.alarmId first stops the previous alarm sound and dismisses the previous
firing state before resetting the snapshot. Use the owner-aware native dismissal
API for that previous firing state, while preserving the existing no-incoming
cleanup behavior.
1 participant
Summary by CodeRabbit