SpeziScheduler - #7
Open
eldcn wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis change adds the Sequence Diagram(s)sequenceDiagram
participant SchedulerClient
participant SchedulerImpl
participant SchedulerDatabase
participant NotificationPlanner
participant NotificationScheduler
SchedulerClient->>SchedulerImpl: create or update task
SchedulerImpl->>SchedulerDatabase: persist task version and outcomes
SchedulerImpl->>SchedulerClient: emit task events
SchedulerClient->>NotificationPlanner: plan upcoming notifications
NotificationPlanner->>NotificationScheduler: schedule alarms
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
eldcn
force-pushed
the
task/spezi-scheduler
branch
from
July 22, 2026 17:16
98116f4 to
d301b59
Compare
eldcn
force-pushed
the
task/spezi-scheduler
branch
2 times, most recently
from
July 27, 2026 14:51
e8b1ec7 to
7683015
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
eldcn
force-pushed
the
task/spezi-scheduler
branch
from
August 1, 2026 15:25
7683015 to
a5db34b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerImpl.kt (1)
122-131: 🚀 Performance & Scalability | 🔵 Trivial
queryEvents(range, predicate)loads the entire task-version and outcome tables on every emission.
combine(dao.observeTaskVersions(), dao.observeOutcomes())(Line 123) loads every task version and every outcome row on every emission, then filters byrangein Kotlin (Lines 124-129). Since this store is explicitly append-only and versioned, both tables only grow over time, so this reactive query's cost increases with total history rather than with the size of the requestedrange.Consider filtering by
range(and, where applicable, by task predicate) at the DAO/SQL level instead of loading and filtering the full tables in memory on each recomposition.🤖 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 `@scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerImpl.kt` around lines 122 - 131, The queryEvents flow currently loads all task versions and outcomes before applying range and predicate filters; move applicable filtering into the DAO/SQL queries used by observeTaskVersions and observeOutcomes. Update the DAO and SchedulerImpl queryEvents integration so only records relevant to range, and task predicate where supported, are emitted, while preserving effectiveVersions and assembleEvents behavior for the filtered results.
🤖 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
`@scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerDatabase.kt`:
- Around line 17-21: Update SchedulerDatabase and SchedulerConfiguration to use
explicit Room Migration objects for every schema version change instead of
fallbackToDestructiveMigration(dropAllTables = true), preserving TaskEntity and
OutcomeEntity data. Enable exportSchema in the `@Database` declaration when schema
exports are required, and register the migrations with the database builder.
In
`@scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerImpl.kt`:
- Around line 47-81: Wrap the multi-step bodies of createOrUpdateTask,
deleteTask, and deleteAllVersions in database.withTransaction { ... }, including
their DAO reads, outcome deletions, version upserts, and task deletions.
Preserve each method’s existing return values and control flow while ensuring
the read-then-write and multi-table delete operations execute atomically.
- Around line 47-81: The createOrUpdateTask flow must reject an effectiveFrom
that is earlier than or equal to the latest task version. After retrieving
latest in createOrUpdateTask, validate effectiveFrom against
latest.effectiveFromMillis and fail before outcome handling or dao.upsertTask;
preserve the existing unchanged-draft return path as appropriate.
- Around line 162-181: In SchedulerImpl.complete, replace
outcomesFlow.tryEmit(outcome) with the suspending emit operation so newly
created outcomes are not dropped when the flow buffer is full; keep the existing
existing == null condition and persistence behavior unchanged.
- Around line 142-160: Update the task-specific queryEvents overload in
SchedulerImpl to start occurrence generation at the version’s effectiveFrom when
it is later than range.start, matching assembleEvents behavior; retain
range.start for versions effective earlier or without a later bound, while
preserving the existing upper-bound and outcome mapping logic.
In `@scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Schedule.kt`:
- Around line 124-131: Update the Recurrence constructor to validate that
interval is strictly positive, rejecting zero and negative values while
preserving the existing default of 1 and all other fields.
---
Nitpick comments:
In
`@scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerImpl.kt`:
- Around line 122-131: The queryEvents flow currently loads all task versions
and outcomes before applying range and predicate filters; move applicable
filtering into the DAO/SQL queries used by observeTaskVersions and
observeOutcomes. Update the DAO and SchedulerImpl queryEvents integration so
only records relevant to range, and task predicate where supported, are emitted,
while preserving effectiveVersions and assembleEvents behavior for the filtered
results.
🪄 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: 8ed1f49a-7866-4e0e-b1de-61ef41cc5031
📒 Files selected for processing (61)
.gitignoreaccount-firebase/src/main/kotlin/edu/stanford/spezi/account/firebase/internal/FirestoreAccountDetailsCodec.ktaccount/src/main/kotlin/edu/stanford/spezi/account/AccountKeys.ktaccount/src/main/kotlin/edu/stanford/spezi/account/internal/AccountDetailsCodec.ktbuild.gradle.ktscore-time/build.gradle.ktscore-time/src/main/kotlin/edu/stanford/spezi/core/time/TimeProvider.ktcore-time/src/testFixtures/kotlin/edu/stanford/spezi/core/time/FakeTimeProvider.ktfoundation/build.gradle.ktsfoundation/src/main/kotlin/edu/stanford/spezi/foundation/DurationSerializer.ktfoundation/src/main/kotlin/edu/stanford/spezi/foundation/InstantSerializer.ktfoundation/src/main/kotlin/edu/stanford/spezi/foundation/JsonSerializer.ktfoundation/src/main/kotlin/edu/stanford/spezi/foundation/UUIDSerializer.ktfoundation/src/test/kotlin/edu/stanford/spezi/foundation/JsonSerializerTest.ktfoundation/src/testFixtures/kotlin/edu/stanford/spezi/foundation/fixtures/InstantFixtures.ktfoundation/src/testFixtures/kotlin/edu/stanford/spezi/foundation/fixtures/UUIDFixtures.ktgradle.propertiesgradle/libs.versions.tomlmyheartcounts/src/main/kotlin/edu/stanford/myheartcounts/account/MHCAccountKeys.ktmyheartcounts/src/main/kotlin/edu/stanford/myheartcounts/notification/NotificationPermissionHandler.ktscheduler/build.gradle.ktsscheduler/src/androidTest/kotlin/edu/stanford/spezi/scheduler/SchedulerImplTest.ktscheduler/src/main/AndroidManifest.xmlscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/AllowedCompletionPolicy.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Event.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/NotificationThread.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/NotificationTime.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Occurrence.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Outcome.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Schedule.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/ScheduleCalculator.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Scheduler.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/SchedulerConfiguration.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/SchedulerNotifications.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/SchedulerNotificationsConfiguration.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Task.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/TaskCategory.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/TaskContext.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/CompletionPolicyEvaluator.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/JsonContextStore.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/NotificationPlanner.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/NotificationScheduler.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/OccurrenceGenerator.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/ScheduleCalculatorImpl.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerBootReceiver.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerDao.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerDatabase.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerEntities.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerImpl.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerMapper.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerNotificationReceiver.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerNotificationsImpl.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerTypeConverters.ktscheduler/src/test/kotlin/edu/stanford/spezi/scheduler/ScheduleTest.ktscheduler/src/test/kotlin/edu/stanford/spezi/scheduler/internal/NotificationPlannerTest.ktscheduler/src/testFixtures/kotlin/edu/stanford/spezi/scheduler/FakeScheduleCalculator.ktscheduler/src/testFixtures/kotlin/edu/stanford/spezi/scheduler/FakeScheduler.ktscheduler/src/testFixtures/kotlin/edu/stanford/spezi/scheduler/fixtures/TaskDraftFixtures.ktsettings.gradle.ktsstorage-local/build.gradle.ktsstorage-local/src/main/kotlin/edu/stanford/spezi/storage/local/KeyValueStorage.kt
🚧 Files skipped from review as they are similar to previous changes (52)
- .gitignore
- myheartcounts/src/main/kotlin/edu/stanford/myheartcounts/notification/NotificationPermissionHandler.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/NotificationThread.kt
- myheartcounts/src/main/kotlin/edu/stanford/myheartcounts/account/MHCAccountKeys.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Event.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/SchedulerNotifications.kt
- settings.gradle.kts
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/SchedulerNotificationsConfiguration.kt
- foundation/build.gradle.kts
- foundation/src/testFixtures/kotlin/edu/stanford/spezi/foundation/fixtures/UUIDFixtures.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/AllowedCompletionPolicy.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Outcome.kt
- gradle.properties
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/TaskCategory.kt
- core-time/build.gradle.kts
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerBootReceiver.kt
- foundation/src/main/kotlin/edu/stanford/spezi/foundation/JsonSerializer.kt
- core-time/src/main/kotlin/edu/stanford/spezi/core/time/TimeProvider.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/NotificationTime.kt
- storage-local/src/main/kotlin/edu/stanford/spezi/storage/local/KeyValueStorage.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Task.kt
- scheduler/src/testFixtures/kotlin/edu/stanford/spezi/scheduler/fixtures/TaskDraftFixtures.kt
- foundation/src/testFixtures/kotlin/edu/stanford/spezi/foundation/fixtures/InstantFixtures.kt
- foundation/src/main/kotlin/edu/stanford/spezi/foundation/InstantSerializer.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/SchedulerConfiguration.kt
- storage-local/build.gradle.kts
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerDao.kt
- foundation/src/main/kotlin/edu/stanford/spezi/foundation/UUIDSerializer.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Occurrence.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/ScheduleCalculator.kt
- scheduler/src/main/AndroidManifest.xml
- scheduler/build.gradle.kts
- account/src/main/kotlin/edu/stanford/spezi/account/internal/AccountDetailsCodec.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerTypeConverters.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/CompletionPolicyEvaluator.kt
- account-firebase/src/main/kotlin/edu/stanford/spezi/account/firebase/internal/FirestoreAccountDetailsCodec.kt
- scheduler/src/testFixtures/kotlin/edu/stanford/spezi/scheduler/FakeScheduleCalculator.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/ScheduleCalculatorImpl.kt
- scheduler/src/testFixtures/kotlin/edu/stanford/spezi/scheduler/FakeScheduler.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerNotificationReceiver.kt
- scheduler/src/androidTest/kotlin/edu/stanford/spezi/scheduler/SchedulerImplTest.kt
- scheduler/src/test/kotlin/edu/stanford/spezi/scheduler/ScheduleTest.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/NotificationPlanner.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerMapper.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/TaskContext.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/OccurrenceGenerator.kt
- scheduler/src/test/kotlin/edu/stanford/spezi/scheduler/internal/NotificationPlannerTest.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerNotificationsImpl.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/NotificationScheduler.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Scheduler.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerEntities.kt
- account/src/main/kotlin/edu/stanford/spezi/account/AccountKeys.kt
Comment on lines
+17
to
+21
| @Database( | ||
| entities = [TaskEntity::class, OutcomeEntity::class], | ||
| version = 1, | ||
| exportSchema = false, | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the module already configures a Room schema export location.
fd -a build.gradle.kts scheduler --exec cat -n {}Repository: SchmiedmayerLab/MyHeartCounts-Android
Length of output: 1823
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate relevant files =="
fd -a 'SchedulerDatabase\.kt|Scheduler.*Configuration|RoomDatabase|fallbackToDestructiveMigration|scheduler/build.gradle.kts|settings.gradle.kts|gradle/libs.version.toml|libs.versions.toml' . | sed 's#^\./##'
echo
echo "== SchedulerDatabase.kt =="
sched_db=$(fd 'SchedulerDatabase\.kt' . | head -n1 || true)
if [ -n "$sched_db" ]; then
cat -n "$sched_db"
fi
echo
echo "== fallbackToDestructiveMigration usages =="
rg -n "fallbackToDestructiveMigration|databaseBuilder|SchedulerDatabase|RoomDatabaseBuilder|exportSchema|room.schemaLocation" .
echo
echo "== check Gradle schema config text =="
rg -n "room\.schemaLocation|schemas|Room\.databaseBuilder|fallbackToDestructiveMigration" --glob '*.gradle.kts' --glob '*.gradle' --glob '*.kt' .Repository: SchmiedmayerLab/MyHeartCounts-Android
Length of output: 3509
Plan real migrations instead of relying on destructive fallback.
SchedulerDatabase keeps exportSchema = false, and SchedulerConfiguration builds the database with fallbackToDestructiveMigration(dropAllTables = true). The first future version bump will drop the current TaskEntity and OutcomeEntity data instead of preserving persisted task history and completion outcomes. Use explicit Migration objects for required schema changes and enable schema export when exports are required.
🤖 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
`@scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerDatabase.kt`
around lines 17 - 21, Update SchedulerDatabase and SchedulerConfiguration to use
explicit Room Migration objects for every schema version change instead of
fallbackToDestructiveMigration(dropAllTables = true), preserving TaskEntity and
OutcomeEntity data. Enable exportSchema in the `@Database` declaration when schema
exports are required, and register the migrations with the database builder.
Comment on lines
+47
to
+81
| override suspend fun createOrUpdateTask( | ||
| draft: TaskDraft, | ||
| effectiveFrom: Instant, | ||
| shadowedOutcomesHandling: ShadowedOutcomesHandling, | ||
| ): Result<TaskUpdateResult> = runCatching { | ||
| val latest = dao.versions(draft.id).maxByOrNull { it.effectiveFromMillis } | ||
|
|
||
| if (latest != null && !wouldChange(latest, draft)) { | ||
| return@runCatching TaskUpdateResult( | ||
| task = mapper.mapTask(latest, nextVersionEffectiveFrom = null), | ||
| didChange = false, | ||
| ) | ||
| } | ||
|
|
||
| if (latest != null) { | ||
| val shadowed = dao.outcomesForTask(draft.id).filter { it.occurrenceStartMillis >= effectiveFrom.toEpochMilli() } | ||
| when (shadowedOutcomesHandling) { | ||
| ShadowedOutcomesHandling.THROW_ERROR -> | ||
| if (shadowed.isNotEmpty()) error("An updated task cannot shadow outcomes of a previous version.") | ||
| ShadowedOutcomesHandling.DELETE -> | ||
| if (shadowed.isNotEmpty()) dao.deleteOutcomesFrom(draft.id, effectiveFrom.toEpochMilli()) | ||
| } | ||
| } | ||
|
|
||
| val entity = mapper.mapTaskEntity( | ||
| versionId = UUID.randomUUID().toString(), | ||
| draft = draft, | ||
| effectiveFrom = effectiveFrom, | ||
| ) | ||
| dao.upsertTask(entity) | ||
| TaskUpdateResult( | ||
| task = mapper.mapTask(entity, nextVersionEffectiveFrom = null), | ||
| didChange = true, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm room-ktx (withTransaction) is available on the scheduler module classpath.
fd -a build.gradle.kts scheduler --exec grep -n 'room' {}Repository: SchmiedmayerLab/MyHeartCounts-Android
Length of output: 311
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
fd -a 'SchedulerImpl\.kt|TaskDao|Room|Database' scheduler || true
echo "== SchedulerImpl outline =="
file="$(fd 'SchedulerImpl\.kt' scheduler | head -n 1)"
if [ -n "${file:-}" ]; then
wc -l "$file"
ast-grep outline "$file" --view expanded || true
echo "== relevant scheduler impl sections =="
sed -n '1,230p' "$file" | cat -n
fi
echo "== DAOs =="
for f in $(fd 'Dao\.kt$|Dao\.kt$' scheduler); do
echo "--- $f"
wc -l "$f"
rg -n "fun versions|fun outcomesForTask|fun deleteOutcomesFrom|fun upsertTask|fun deleteTask|fun deleteAllVersions|Transaction|transaction|`@Dao`" "$f" -C 3
done
echo "== Room database definitions =="
for f in $(fd 'Database\.kt$|Database\.ktor$|RoomDatabase\.kt$' scheduler); do
echo "--- $f"
wc -l "$f"
rg -n "abstract class .*Database|open class .*Database|class .*Database|`@Database`|withTransaction|RoomDatabase" "$f" -C 3
doneRepository: SchmiedmayerLab/MyHeartCounts-Android
Length of output: 15893
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== dependency catalog references =="
fd -a 'libs\.versions\.toml|gradle\.libs\.versions\.toml' backend . --exec sh -c 'echo "--- $1"; rg -n "room|androidx-room|ktx|ksp" "$1" -C 2' sh {}
echo "== scheduler build file relevant lines =="
fd -a 'build\.gradle\.kts' scheduler --exec sh -c 'echo "--- $1"; sed -n "1,120p" "$1" | cat -n' sh {}Repository: SchmiedmayerLab/MyHeartCounts-Android
Length of output: 6957
Wrap multi-step writes in a transaction.
createOrUpdateTask, deleteTask, and deleteAllVersions each call multiple DAO methods without a transaction. A concurrent update can see stale latest versions before the new version is inserted, and interrupted writes can leave task versions and outcomes partially deleted.
Wrap each method body in database.withTransaction { ... } for the read-then-upsert path and for the multi-table deletes.
🤖 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
`@scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerImpl.kt`
around lines 47 - 81, Wrap the multi-step bodies of createOrUpdateTask,
deleteTask, and deleteAllVersions in database.withTransaction { ... }, including
their DAO reads, outcome deletions, version upserts, and task deletions.
Preserve each method’s existing return values and control flow while ensuring
the read-then-write and multi-table delete operations execute atomically.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the referenced file and related DAO/entity behavior without executing repository code.
git ls-files | rg 'SchedulerImpl\.kt|Dao|Task' | head -200
printf '\n--- SchedulerImpl outline ---\n'
ast-grep outline scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerImpl.kt --view expanded || true
printf '\n--- Relevant SchedulerImpl sections ---\n'
sed -n '1,260p' scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerImpl.kt
printf '\n--- Search versions/effectiveFrom usage ---\n'
rg -n "versions\\(|effectiveFromMillis|effectiveVersions|allVersions|createOrUpdateTask|upsertTask|deleteOutcomesFrom|outcomesForTask" scheduler/src/main/kotlin -SRepository: SchmiedmayerLab/MyHeartCounts-Android
Length of output: 18412
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Behavioural source probe: parse the Kotlin file and check whether createOrUpdateTask contains
# a latest != null guard requiring effectiveFrom > latest.effectiveFromMillis before dao.upsertTask.
# This is a read-only structural/equivalence check over source text.
python3 - <<'PY'
from pathlib import Path
import re
p = Path('scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerImpl.kt')
s = p.read_text()
start = s.index('override suspend fun createOrUpdateTask(')
end = s.index('} // SchedulerImpl', start) + len('} // SchedulerImpl')
text = s[start:end]
print('function_found=', True)
print('has_latest_var=', bool(re.search(r'val\s+latest\s*=\s*dao\.versions', text)))
checks = []
for name, pat in [
('require_latest_guard', re.compile(r'if\s*\(\s*latest\s*!=\s*null\s*\)\s*\{\s*require\s*\(\s*effectiveFrom\.toEpochMilli\s*\(\s*\)\s*>\s*latest\.effectiveFromMillis')),
('require_latest_guard_strict', re.compile(r'effectiveFrom\.toEpochMilli\s*\(\s*\)\s*>\s*latest\.effectiveFromMillis')),
('require_non_strict', re.compile(r'effectiveFrom\.toEpochMilli\s*\(\s*\)\s*>=\s*latest\.effectiveFromMillis')),
]:
checks.append((name, bool(pat.search(text))))
for name, ok in checks:
print(f'{name}={ok}')
# Print compact context around latest/use of effectiveFrom/upsert to show assumptions.
print('\n--- compact context ---\n')
for pat in [
r'val \s*latest\s*=[\s\S]{0,220}',
r'if\s*\(\s*latest\s*!=\s*null\s*\}\s*[\s\S]{0,350}',
r'suspend\s+[\w<>\s?]+\seffectiveVersions[\s\S]{0,550}',
r'allVersions[\s\S]{0,400}',
]:
m = re.search(pat, text)
if m:
print(f'--- {pat[:40]} ---')
print(m.group(0).replace('\n', '\n'))
PYRepository: SchmiedmayerLab/MyHeartCounts-Android
Length of output: 280
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,140p' scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerEntities.kt
printf '\n--- SchedulerDao ---\n'
sed -n '1,100p' scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerDao.kt
printf '\n--- All call sites of createOrUpdateTask ---\n'
rg -n "createOrUpdateTask\\(" myheartcounts scheduler -SRepository: SchmiedmayerLab/MyHeartCounts-Android
Length of output: 5599
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '70,280p' scheduler/src/androidTest/kotlin/edu/stanford/spezi/scheduler/SchedulerImplTest.kt
printf '\n--- Upcoming usage ---\n'
rg -n "createOrUpdateTask\\(|effectiveFrom|Instant\\.now|Instant" myheartCounts src/main/kotlin/edu/stanford/myheartcounts up -S || true
printf '\n--- API contract section ---\n'
sed -n '30,90p' scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Scheduler.ktRepository: SchmiedmayerLab/MyHeartCounts-Android
Length of output: 9824
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "createOrUpdateTask\\(|Scheduler\\." -S . \
--glob '!**/.gradle/**' --glob '!**/build/**' --glob '!**/node_modules/**' \
| head -200Repository: SchmiedmayerLab/MyHeartCounts-Android
Length of output: 1966
Validate that effectiveFrom advances past the latest task version.
createOrUpdateTask() builds the next effectiveFrom from the latest existing version, but the current guard only handles unchanged drafts and shadowed outcomes. An effectiveFrom value earlier than or equal to latest.effectiveFromMillis can create an out-of-order task_versions row, and allVersions() / effectiveVersions() then compute wrong boundaries by pairing each version with the next sorted effectiveFromMillis. Reject that input before dao.upsertTask(entity).
🤖 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
`@scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerImpl.kt`
around lines 47 - 81, The createOrUpdateTask flow must reject an effectiveFrom
that is earlier than or equal to the latest task version. After retrieving
latest in createOrUpdateTask, validate effectiveFrom against
latest.effectiveFromMillis and fail before outcome handling or dao.upsertTask;
preserve the existing unchanged-draft return path as appropriate.
Comment on lines
+142
to
+160
| override fun queryEvents(task: Task, range: InstantRange): Flow<List<Event>> = | ||
| dao.observeOutcomes().map { outcomes -> | ||
| val upper = task.nextVersionEffectiveFrom?.let { minOf(it, range.endExclusive) } ?: range.endExclusive | ||
| val outcomesByStart = outcomes | ||
| .filter { it.taskId == task.id } | ||
| .associateBy { it.occurrenceStartMillis } | ||
| val occurrenceRange = InstantRange( | ||
| start = range.start, | ||
| endExclusive = upper, | ||
| ) | ||
| scheduleCalculator.occurrences(schedule = task.schedule, range = occurrenceRange).map { occurrence -> | ||
| val outcome = outcomesByStart[occurrence.start.toEpochMilli()]?.let(mapper::mapOutcome) | ||
| Event( | ||
| task = task, | ||
| occurrence = occurrence, | ||
| outcome = outcome, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
fd -a 'SchedulerImpl.kt' . || true
echo "== file outline =="
file="$(fd 'SchedulerImpl.kt' . | head -n 1)"
if [ -n "${file:-}" ]; then
ast-grep outline "$file" --view condensed || true
echo "== relevant lines 120-170 and 210-265 =="
sed -n '120,265p' "$file" | nl -ba -v120
fi
echo "== search relevant symbols =="
rg -n "queryEvents|observeOutcomes|effectiveFrom|hasPreviousVersion|assembleEvents|nextVersionEffectiveFrom|occurrenceRange" .Repository: SchmiedmayerLab/MyHeartCounts-Android
Length of output: 547
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="/home/jailuser/git/scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerImpl.kt"
echo "== relevant lines =="
sed -n '120,265p' "$file" | awk '{printf "%4d %s\n", NR+119, $0}'
echo "== relevant usages =="
rg -n "queryEvents|observeOutcomes|effectiveFrom|hasPreviousVersion|assembleEvents|nextVersionEffectiveFrom|occurrenceRange" .Repository: SchmiedmayerLab/MyHeartCounts-Android
Length of output: 21454
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== scheduler files =="
git ls-files scheduler/src/main kotlin 2>/dev/null | grep -E 'Scheduler\.(kt|kts)$|Task\.kt|SchedulerEntities\.kt|InternalScheduler|Event\.kt' || git ls-files scheduler/src/main | grep -E 'Scheduler\.(kt|kts)$|Task\.kt|SchedulerEntities\.kt|Event\.kt'
file="$(git ls-files | grep -E 'scheduler/src/main/.*Scheduler\.kt$' | head -n 1)"
task="$(git ls-files | grep -E 'scheduler/src/main/.*Task\.kt$' | head -n 1)"
event="$(git ls-files | grep -E 'scheduler/src/main/.*Event\.kt$' | head -n 1)"
echo "== Task, Scheduler, Event snippets =="
for f in "$task" "$file" "$event"; do
if [ -n "${f:-}" ]; then
echo "--- $f ---"
sed -n '1,140p' "$f" | awk '{printf "%4d %s\n", NR, $0}'
fi
done
echo "== read-only behavioral probe for overlap between overload and assembled query =="
python3 - <<'PY'
from datetime import datetime, timedelta
def occurrences_daily(start, range_start, range_end):
# daily occurrences at fixed local date boundary: include dates in [range_start, range_end)
out = []
day = (range_start.date() or datetime.min.date()).toordinal()
end = range_end
while True:
d = datetime.fromordinal(day)
if range_start <= d < range_end:
out.append(d)
if d >= end:
break
day += 1
return out
task_effective_from = datetime(2026, 6, 18, 9, 0)
range_start = datetime(2026, 6, 14, 0, 0)
range_end = datetime(2026, 6, 22, 0, 0)
# same lower bound as queryEvents(task, range)
lower = range_start
overload = occurrences_daily(task_effective_from, lower, range_end)
# same lower bound as assembleEvents when hasPreviousVersion=True
lower_assembled = max(task_effective_from, range_start)
assembled = occurrences_daily(task_effective_from, lower_assembled, range_end)
print("overload_lower:", lower.isoformat())
print("assembled_lower:", lower_assembled.isoformat())
print("overload_events:", [t.isoformat() for t in overload])
print("assembled_events:", [t.isoformat() for t in assembled])
print("overload_extra_before_effective:", sorted(t.isoformat() for t in overload if t < task_effective_from))
PYRepository: SchmiedmayerLab/MyHeartCounts-Android
Length of output: 11798
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="/home/jailuser/git/scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerImpl.kt"
task_file="/home/jailuser/git/scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Task.kt"
echo "== exact lower-bound expressions =="
sed -n '139,152p' "$file" | awk '{printf "%4d %s\n", NR+138, $0}'
sed -n '227,241p' "$file" | awk '{printf "%4d %s\n", NR+226, $0}'
echo "== task API has previous current? =="
sed -n '22,55p' "$task_file" | awk '{printf "%4d %s\n", NR, $0}'
echo "== deterministic probe for same version queried both ways =="
python3 - <<'PY'
from datetime import datetime
task_effective_from = datetime(2026, 6, 18, 9, 0)
range_start = datetime(2026, 6, 14, 0, 0)
range_end = datetime(2026, 6, 22, 0, 0)
schedule_start = task_effective_from
schedule_interval = datetime.timedelta(days=1)
def daterange_first(start, limit):
out = []
while True:
if start >= range_end:
break
out.append(start)
start += schedule_interval
if len(out) >= limit:
break
return out
start_overload = range_start
overload = daterange_first(start_overload, 20)
start_assembled = max(task_effective_from, range_start)
assembled = daterange_first(start_assembled, 20)
print("same_version_overlap:", set(overload).issubset(set(assembled)))
print("overload_before_effective_count:", sum(1 for t in overload if t < task_effective_from))
PYRepository: SchmiedmayerLab/MyHeartCounts-Android
Length of output: 3541
Clamp queryEvents(task, range) against the version’s effective start.
queryEvents(task, range) starts occurrence generation from range.start, while queryEvents(range, predicate) maps this same task version through assembleEvents, where non-most-recent versions use maxOf(task.effectiveFrom, range.start). When a version is queried directly and an earlier version exists, the overload can return occurrences before that version can produce them. Change the overload to preserve the same lower bound as assembleEvents for this task version, or document the version-overload semantics and update the jdoc in Scheduler.kt accordingly.
🤖 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
`@scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerImpl.kt`
around lines 142 - 160, Update the task-specific queryEvents overload in
SchedulerImpl to start occurrence generation at the version’s effectiveFrom when
it is later than range.start, matching assembleEvents behavior; retain
range.start for versions effective earlier or without a later bound, while
preserving the existing upper-bound and outcome mapping logic.
Comment on lines
+162
to
+181
| override suspend fun complete( | ||
| event: Event, | ||
| ignoreCompletionPolicy: Boolean, | ||
| context: OutcomeContext, | ||
| ): Result<Outcome> = runCatching { | ||
| check(ignoreCompletionPolicy || scheduleCalculator.isAllowedToComplete(event = event)) { | ||
| "The completion policy does not allow completing this event now." | ||
| } | ||
| val existing = event.outcome | ||
| val outcome = existing?.copy(context = context) ?: Outcome( | ||
| id = UUID.randomUUID(), | ||
| taskId = event.task.id, | ||
| occurrenceStartDate = event.occurrence.start, | ||
| completionDate = timeProvider.nowInstant(), | ||
| context = context, | ||
| ) | ||
| dao.upsertOutcome(mapper.mapOutcomeEntity(outcome)) | ||
| if (existing == null) outcomesFlow.tryEmit(outcome) | ||
| outcome | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
tryEmit can silently drop outcomes from newOutcomes on buffer overflow.
complete is a suspend function but uses outcomesFlow.tryEmit(outcome) (Line 179) instead of emit. outcomesFlow has extraBufferCapacity = 64 (Line 43, OUTCOME_BUFFER). If 64 outcomes are emitted while no collector is actively draining the flow (for example a burst of completions with a slow or temporarily unsubscribed notification consumer), tryEmit returns false and the outcome is silently dropped from newOutcomes, even though it is correctly persisted via dao.upsertOutcome (Line 178).
Since complete already suspends, replace tryEmit with emit, which suspends until buffer space is available instead of dropping data.
🔁 Proposed fix
- if (existing == null) outcomesFlow.tryEmit(outcome)
+ if (existing == null) outcomesFlow.emit(outcome)📝 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.
Suggested change
| override suspend fun complete( | |
| event: Event, | |
| ignoreCompletionPolicy: Boolean, | |
| context: OutcomeContext, | |
| ): Result<Outcome> = runCatching { | |
| check(ignoreCompletionPolicy || scheduleCalculator.isAllowedToComplete(event = event)) { | |
| "The completion policy does not allow completing this event now." | |
| } | |
| val existing = event.outcome | |
| val outcome = existing?.copy(context = context) ?: Outcome( | |
| id = UUID.randomUUID(), | |
| taskId = event.task.id, | |
| occurrenceStartDate = event.occurrence.start, | |
| completionDate = timeProvider.nowInstant(), | |
| context = context, | |
| ) | |
| dao.upsertOutcome(mapper.mapOutcomeEntity(outcome)) | |
| if (existing == null) outcomesFlow.tryEmit(outcome) | |
| outcome | |
| } | |
| override suspend fun complete( | |
| event: Event, | |
| ignoreCompletionPolicy: Boolean, | |
| context: OutcomeContext, | |
| ): Result<Outcome> = runCatching { | |
| check(ignoreCompletionPolicy || scheduleCalculator.isAllowedToComplete(event = event)) { | |
| "The completion policy does not allow completing this event now." | |
| } | |
| val existing = event.outcome | |
| val outcome = existing?.copy(context = context) ?: Outcome( | |
| id = UUID.randomUUID(), | |
| taskId = event.task.id, | |
| occurrenceStartDate = event.occurrence.start, | |
| completionDate = timeProvider.nowInstant(), | |
| context = context, | |
| ) | |
| dao.upsertOutcome(mapper.mapOutcomeEntity(outcome)) | |
| if (existing == null) outcomesFlow.emit(outcome) | |
| outcome | |
| } |
🤖 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
`@scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerImpl.kt`
around lines 162 - 181, In SchedulerImpl.complete, replace
outcomesFlow.tryEmit(outcome) with the suspending emit operation so newly
created outcomes are not dropped when the flow buffer is full; keep the existing
existing == null condition and persistence behavior unchanged.
Comment on lines
+124
to
+131
| data class Recurrence( | ||
| val frequency: RecurrenceFrequency, | ||
| val interval: Int = 1, | ||
| val weekday: DayOfWeek? = null, | ||
| val dayOfMonth: Int? = null, | ||
| val month: Int? = null, | ||
| val end: RecurrenceEnd = RecurrenceEnd.Never, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject non-positive recurrence intervals.
Line 126 accepts 0 and negative values. A recurrence generator cannot advance when interval is 0. This can cause an unbounded occurrence-generation loop.
Validate interval > 0 when constructing Recurrence.
Proposed fix
data class Recurrence(
val frequency: RecurrenceFrequency,
val interval: Int = 1,
val weekday: DayOfWeek? = null,
val dayOfMonth: Int? = null,
val month: Int? = null,
val end: RecurrenceEnd = RecurrenceEnd.Never,
-)
+) {
+ init {
+ require(interval > 0) { "interval must be positive" }
+ }
+}📝 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.
Suggested change
| data class Recurrence( | |
| val frequency: RecurrenceFrequency, | |
| val interval: Int = 1, | |
| val weekday: DayOfWeek? = null, | |
| val dayOfMonth: Int? = null, | |
| val month: Int? = null, | |
| val end: RecurrenceEnd = RecurrenceEnd.Never, | |
| ) | |
| data class Recurrence( | |
| val frequency: RecurrenceFrequency, | |
| val interval: Int = 1, | |
| val weekday: DayOfWeek? = null, | |
| val dayOfMonth: Int? = null, | |
| val month: Int? = null, | |
| val end: RecurrenceEnd = RecurrenceEnd.Never, | |
| ) { | |
| init { | |
| require(interval > 0) { "interval must be positive" } | |
| } | |
| } |
🤖 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 `@scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Schedule.kt` around
lines 124 - 131, Update the Recurrence constructor to validate that interval is
strictly positive, rejecting zero and negative values while preserving the
existing default of 1 and all other fields.
eldcn
force-pushed
the
task/spezi-scheduler
branch
from
August 3, 2026 15:11
1fce04f to
013a44d
Compare
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 `@scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Schedule.kt`:
- Around line 52-83: Update ScheduleDuration.Fixed construction to reject
negative Duration values, including direct calls and the seconds, minutes, and
hours factory methods. Validate the duration at construction time while
preserving zero and positive durations.
🪄 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: 64ebdce2-1e4f-4c3a-83fa-7ddb42714cb7
📒 Files selected for processing (62)
.gitignoreaccount-firebase/src/main/kotlin/edu/stanford/spezi/account/firebase/internal/FirestoreAccountDetailsCodec.ktaccount/src/main/kotlin/edu/stanford/spezi/account/AccountKeys.ktaccount/src/main/kotlin/edu/stanford/spezi/account/internal/AccountDetailsCodec.ktbuild.gradle.ktscore-time/build.gradle.ktscore-time/src/main/kotlin/edu/stanford/spezi/core/time/TimeProvider.ktcore-time/src/testFixtures/kotlin/edu/stanford/spezi/core/time/FakeTimeProvider.ktfoundation/build.gradle.ktsfoundation/src/main/kotlin/edu/stanford/spezi/foundation/DurationSerializer.ktfoundation/src/main/kotlin/edu/stanford/spezi/foundation/InstantSerializer.ktfoundation/src/main/kotlin/edu/stanford/spezi/foundation/JsonSerializer.ktfoundation/src/main/kotlin/edu/stanford/spezi/foundation/UUIDSerializer.ktfoundation/src/test/kotlin/edu/stanford/spezi/foundation/JsonSerializerTest.ktfoundation/src/testFixtures/kotlin/edu/stanford/spezi/foundation/fixtures/InstantFixtures.ktfoundation/src/testFixtures/kotlin/edu/stanford/spezi/foundation/fixtures/UUIDFixtures.ktgradle.propertiesgradle/libs.versions.tomlmyheartcounts/src/main/kotlin/edu/stanford/myheartcounts/account/MHCAccountKeys.ktmyheartcounts/src/main/kotlin/edu/stanford/myheartcounts/notification/NotificationPermissionHandler.ktscheduler/build.gradle.ktsscheduler/src/androidTest/kotlin/edu/stanford/spezi/scheduler/SchedulerImplTest.ktscheduler/src/main/AndroidManifest.xmlscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/AllowedCompletionPolicy.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Event.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/NotificationThread.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/NotificationTime.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Occurrence.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Outcome.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Schedule.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/ScheduleCalculator.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Scheduler.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/SchedulerConfiguration.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/SchedulerNotifications.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/SchedulerNotificationsConfiguration.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Task.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/TaskCategory.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/TaskContext.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/CompletionPolicyEvaluator.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/JsonContextStore.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/NotificationPlanner.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/NotificationScheduler.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/OccurrenceGenerator.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/ScheduleCalculatorImpl.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerBootReceiver.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerDao.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerDatabase.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerEntities.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerImpl.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerMapper.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerNotificationReceiver.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerNotificationsImpl.ktscheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerTypeConverters.ktscheduler/src/test/kotlin/edu/stanford/spezi/scheduler/ScheduleTest.ktscheduler/src/test/kotlin/edu/stanford/spezi/scheduler/internal/NotificationPlannerTest.ktscheduler/src/testFixtures/kotlin/edu/stanford/spezi/scheduler/FakeScheduleCalculator.ktscheduler/src/testFixtures/kotlin/edu/stanford/spezi/scheduler/FakeScheduler.ktscheduler/src/testFixtures/kotlin/edu/stanford/spezi/scheduler/fixtures/EventFixtures.ktscheduler/src/testFixtures/kotlin/edu/stanford/spezi/scheduler/fixtures/TaskDraftFixtures.ktsettings.gradle.ktsstorage-local/build.gradle.ktsstorage-local/src/main/kotlin/edu/stanford/spezi/storage/local/KeyValueStorage.kt
🚧 Files skipped from review as they are similar to previous changes (54)
- myheartcounts/src/main/kotlin/edu/stanford/myheartcounts/account/MHCAccountKeys.kt
- settings.gradle.kts
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerDao.kt
- build.gradle.kts
- account/src/main/kotlin/edu/stanford/spezi/account/AccountKeys.kt
- .gitignore
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerBootReceiver.kt
- scheduler/src/main/AndroidManifest.xml
- foundation/src/main/kotlin/edu/stanford/spezi/foundation/InstantSerializer.kt
- foundation/build.gradle.kts
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerTypeConverters.kt
- myheartcounts/src/main/kotlin/edu/stanford/myheartcounts/notification/NotificationPermissionHandler.kt
- storage-local/src/main/kotlin/edu/stanford/spezi/storage/local/KeyValueStorage.kt
- foundation/src/main/kotlin/edu/stanford/spezi/foundation/UUIDSerializer.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerDatabase.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerEntities.kt
- gradle.properties
- core-time/src/testFixtures/kotlin/edu/stanford/spezi/core/time/FakeTimeProvider.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/TaskContext.kt
- foundation/src/main/kotlin/edu/stanford/spezi/foundation/JsonSerializer.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Outcome.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/TaskCategory.kt
- core-time/src/main/kotlin/edu/stanford/spezi/core/time/TimeProvider.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/JsonContextStore.kt
- foundation/src/test/kotlin/edu/stanford/spezi/foundation/JsonSerializerTest.kt
- core-time/build.gradle.kts
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Event.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/ScheduleCalculatorImpl.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Occurrence.kt
- account/src/main/kotlin/edu/stanford/spezi/account/internal/AccountDetailsCodec.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/OccurrenceGenerator.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/SchedulerConfiguration.kt
- foundation/src/main/kotlin/edu/stanford/spezi/foundation/DurationSerializer.kt
- foundation/src/testFixtures/kotlin/edu/stanford/spezi/foundation/fixtures/InstantFixtures.kt
- scheduler/src/testFixtures/kotlin/edu/stanford/spezi/scheduler/FakeScheduleCalculator.kt
- scheduler/src/test/kotlin/edu/stanford/spezi/scheduler/ScheduleTest.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Task.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/CompletionPolicyEvaluator.kt
- storage-local/build.gradle.kts
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerNotificationsImpl.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/ScheduleCalculator.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/NotificationScheduler.kt
- scheduler/build.gradle.kts
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/NotificationTime.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/NotificationPlanner.kt
- scheduler/src/test/kotlin/edu/stanford/spezi/scheduler/internal/NotificationPlannerTest.kt
- gradle/libs.versions.toml
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/internal/SchedulerImpl.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/SchedulerNotifications.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/AllowedCompletionPolicy.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/NotificationThread.kt
- scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Scheduler.kt
- foundation/src/testFixtures/kotlin/edu/stanford/spezi/foundation/fixtures/UUIDFixtures.kt
- account-firebase/src/main/kotlin/edu/stanford/spezi/account/firebase/internal/FirestoreAccountDetailsCodec.kt
Comment on lines
+52
to
+83
| sealed interface ScheduleDuration { | ||
| /** | ||
| * The occurrence spans a whole calendar day, with its start pinned to the start of that day. | ||
| */ | ||
| data object AllDay : ScheduleDuration | ||
|
|
||
| /** | ||
| * The occurrence starts at its scheduled time and ends at the end of that calendar day. | ||
| */ | ||
| data object TillEndOfDay : ScheduleDuration | ||
|
|
||
| /** | ||
| * The occurrence starts at its scheduled time and lasts for a fixed [duration]. | ||
| */ | ||
| data class Fixed(val duration: Duration) : ScheduleDuration | ||
|
|
||
| companion object { | ||
| /** | ||
| * A fixed-length duration of [seconds]. | ||
| */ | ||
| fun seconds(seconds: Long): Fixed = Fixed(duration = Duration.ofSeconds(seconds)) | ||
|
|
||
| /** | ||
| * A fixed-length duration of [minutes]. | ||
| */ | ||
| fun minutes(minutes: Long): Fixed = Fixed(duration = Duration.ofMinutes(minutes)) | ||
|
|
||
| /** | ||
| * A fixed-length duration of [hours]. | ||
| */ | ||
| fun hours(hours: Long): Fixed = Fixed(duration = Duration.ofHours(hours)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject negative fixed durations.
ScheduleDuration.Fixed(Duration.ofSeconds(-1)) is valid. It can produce an occurrence whose end precedes its start. Reject negative durations at construction.
Proposed fix
- data class Fixed(val duration: Duration) : ScheduleDuration
+ data class Fixed(val duration: Duration) : ScheduleDuration {
+ init {
+ require(!duration.isNegative) { "duration must not be negative" }
+ }
+ }📝 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.
Suggested change
| sealed interface ScheduleDuration { | |
| /** | |
| * The occurrence spans a whole calendar day, with its start pinned to the start of that day. | |
| */ | |
| data object AllDay : ScheduleDuration | |
| /** | |
| * The occurrence starts at its scheduled time and ends at the end of that calendar day. | |
| */ | |
| data object TillEndOfDay : ScheduleDuration | |
| /** | |
| * The occurrence starts at its scheduled time and lasts for a fixed [duration]. | |
| */ | |
| data class Fixed(val duration: Duration) : ScheduleDuration | |
| companion object { | |
| /** | |
| * A fixed-length duration of [seconds]. | |
| */ | |
| fun seconds(seconds: Long): Fixed = Fixed(duration = Duration.ofSeconds(seconds)) | |
| /** | |
| * A fixed-length duration of [minutes]. | |
| */ | |
| fun minutes(minutes: Long): Fixed = Fixed(duration = Duration.ofMinutes(minutes)) | |
| /** | |
| * A fixed-length duration of [hours]. | |
| */ | |
| fun hours(hours: Long): Fixed = Fixed(duration = Duration.ofHours(hours)) | |
| } | |
| sealed interface ScheduleDuration { | |
| /** | |
| * The occurrence spans a whole calendar day, with its start pinned to the start of that day. | |
| */ | |
| data object AllDay : ScheduleDuration | |
| /** | |
| * The occurrence starts at its scheduled time and ends at the end of that calendar day. | |
| */ | |
| data object TillEndOfDay : ScheduleDuration | |
| /** | |
| * The occurrence starts at its scheduled time and lasts for a fixed [duration]. | |
| */ | |
| data class Fixed(val duration: Duration) : ScheduleDuration { | |
| init { | |
| require(!duration.isNegative) { "duration must not be negative" } | |
| } | |
| } | |
| companion object { | |
| /** | |
| * A fixed-length duration of [seconds]. | |
| */ | |
| fun seconds(seconds: Long): Fixed = Fixed(duration = Duration.ofSeconds(seconds)) | |
| /** | |
| * A fixed-length duration of [minutes]. | |
| */ | |
| fun minutes(minutes: Long): Fixed = Fixed(duration = Duration.ofMinutes(minutes)) | |
| /** | |
| * A fixed-length duration of [hours]. | |
| */ | |
| fun hours(hours: Long): Fixed = Fixed(duration = Duration.ofHours(hours)) | |
| } |
🤖 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 `@scheduler/src/main/kotlin/edu/stanford/spezi/scheduler/Schedule.kt` around
lines 52 - 83, Update ScheduleDuration.Fixed construction to reject negative
Duration values, including direct calls and the seconds, minutes, and hours
factory methods. Validate the duration at construction time while preserving
zero and positive durations.
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
2 participants
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.
♻️ Current situation & Problem
scheduler(notifications = …)⚙️ Release Notes
Add a bullet point list summary of the feature and possible migration guides if this is a breaking change so this section can be added to the release notes.
Include code snippets that provide examples of the feature implemented or links to the documentation if it adds to or changes the public interface.
📚 Documentation
Please ensure that you properly document any additions in conformance with the project's documentation guidelines.
You can use this section to describe your solution, but we encourage contributors to document your reasoning and changes using inline documentation.
✅ Testing
Please ensure that the PR meets the testing requirements set by Codecov and that new functionality is appropriately tested.
This section describes important information about the tests and why some elements might not be testable.
Code of Conduct & Contributing Guidelines
By creating and submitting this pull request, you agree to follow our Code of Conduct and Contributing Guidelines: