Conversation
|
Thanks for opening this, but we'd appreciate a little more information. Could you update it with more details? |
Contributor
📝 WalkthroughWalkthroughThe PR adds offline delta sync for incident command and ad-hoc resources, plus idempotent replay handling for check-in requests. ChangesOffline Sync Change Tracking
Check-in Replay Idempotency
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
Providers/Resgrid.Providers.MigrationsPg/Migrations/M0081_AddIncidentCommandChangeTrackingPg.cs (1)
25-40: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winSame
NULLModifiedOnbackfill gap as the SQL Server migration.This PostgreSQL migration mirrors
M0081_AddIncidentCommandChangeTrackingand shares the same issue: existing rows receiveNULLModifiedOnand will be excluded from the initial delta/full pull. Apply the same backfill here.🤖 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 `@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0081_AddIncidentCommandChangeTrackingPg.cs` around lines 25 - 40, The PostgreSQL migration Up method in M0081_AddIncidentCommandChangeTrackingPg adds ModifiedOn as nullable but does not backfill existing rows, so they stay NULL and miss the initial sync. Update the migration to set ModifiedOn for all pre-existing rows in each table from ChangeTrackedTables, using the same backfill approach as the SQL Server migration before or after adding the column. Keep the existing CommandStructureNodes DeletedOn addition unchanged, and make sure the backfill is applied consistently within Up.
🧹 Nitpick comments (2)
Providers/Resgrid.Providers.Migrations/Migrations/M0082_AddCheckInRecordIdempotencyKey.cs (1)
12-18: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftConsider a partial unique index to enforce idempotency at the DB level.
A plain nullable column does not prevent duplicate inserts under concurrent replay (see
PerformCheckInAsync). A filtered unique index onIdempotencyKey(scoped per department, where non-null) would make the dedup guarantee robust and let the service catch the violation deterministically.🤖 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 `@Providers/Resgrid.Providers.Migrations/Migrations/M0082_AddCheckInRecordIdempotencyKey.cs` around lines 12 - 18, Add a DB-level idempotency guard in M0082_AddCheckInRecordIdempotencyKey instead of only adding the nullable IdempotencyKey column. Update the migration to create a filtered unique index on CheckInRecords.IdempotencyKey, scoped with DepartmentId (or the same tenant scope used by PerformCheckInAsync) and excluding null values, so concurrent replays cannot insert duplicates. Make sure the index is created alongside the existing AddColumn change and that the service layer can deterministically handle the unique-constraint violation.Core/Resgrid.Services/IncidentCommandService.cs (1)
365-396: 🚀 Performance & Scalability | 🔵 TrivialDelta read fans out to 8 full department-scoped loads + in-memory filtering on every poll.
Each
GetChangesSinceAsynccall pulls every command/node/assignment/objective/timer/annotation/role/timeline row for the department and filters in memory. Since soft-deleted/closed/released rows are intentionally never purged, these sets grow unbounded over the life of the department, so polling clients pay an ever-increasing cost. The class header already flags per-CallId/predicate repository queries as a follow-up — consider aModifiedOn >@since`` (indexed) repository query before this ships to high-traffic departments.🤖 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 `@Core/Resgrid.Services/IncidentCommandService.cs` around lines 365 - 396, GetChangesSinceAsync in IncidentCommandService is loading entire department-scoped collections and filtering them in memory, which will scale poorly as data grows. Update the repository calls for commands, nodes, assignments, objectives, timers, annotations, roles, and timeline entries to query by sinceUtc/ModifiedOn (or OccurredOn for timeline) at the repository level instead of using GetAllByDepartmentIdAsync plus Changed filtering. Keep the existing change-shaping logic, but move the predicate into the repository methods or add new indexed query methods so the method only returns delta rows.
🤖 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 `@Core/Resgrid.Model/IncidentCommand/IncidentCommandChanges.cs`:
- Around line 10-11: The XML doc comment on IncidentCommandChanges contradicts
the actual sync behavior exposed by AdHocUnits, AdHocPersonnel, and
GetAdHocChangesSinceAsync. Update the remark to describe these ad-hoc resources
as delta-tracked by ModifiedOn and returned through the incremental changes
payload, rather than saying they are not change-tracked or require a full
refetch. Keep the wording aligned with the model fields and the filtering logic
so clients can rely on the sync contract.
In `@Core/Resgrid.Services/CheckInTimerService.cs`:
- Around line 326-332: The idempotency guard in CheckInTimerService is only
doing a read-before-write check, so concurrent replays can still insert
duplicates. Add a filtered unique index for the check-in record idempotency key
on DepartmentId and IdempotencyKey with IdempotencyKey IS NOT NULL in the
M0082_AddCheckInRecordIdempotencyKey migration, then update the
CheckInTimerService insert flow to catch the unique-violation and return the
existing record instead of failing.
In `@Core/Resgrid.Services/IncidentCommandService.cs`:
- Around line 363-396: The delta-pull logic in IncidentCommandService currently
filters everything through the Changed(IChangeTracked) predicate, so rows with
null ModifiedOn are skipped even during the initial full sync. Update this flow
so the sinceUtc initial-cursor case (DateTime.MinValue) returns all rows or
otherwise includes legacy null-ModifiedOn records, and keep the filtering
behavior for normal incremental syncs; use the existing Changed helper and the
repository fetch blocks for commands, nodes, assignments, objectives, timers,
annotations, and roles to apply the fix consistently.
In
`@Providers/Resgrid.Providers.Migrations/Migrations/M0081_AddIncidentCommandChangeTracking.cs`:
- Around line 26-40: Backfill the new ModifiedOn column in
M0081_AddIncidentCommandChangeTracking so existing rows are visible to
GetChangesSinceAsync on the first sync. Update the Up() migration logic around
ChangeTrackedTables and the CommandStructureNodes alter to seed ModifiedOn for
pre-existing records, not just add it as nullable, so current
commands/nodes/assignments/objectives/timers/annotations/roles are included in
the initial delta pull.
In
`@Providers/Resgrid.Providers.Migrations/Migrations/M0083_AddAdHocResourceChangeTracking.cs`:
- Around line 14-23: The migration that adds `ModifiedOn` to the ad-hoc tables
leaves existing rows with null timestamps, so they never satisfy the
`GetAdHocChangesSinceAsync` change filter and won’t sync on reconnect or first
pull. Update `M0083_AddAdHocResourceChangeTracking.Up()` to backfill
`ModifiedOn` for pre-existing rows in the affected tables (`IncidentAdHocUnits`
and `IncidentAdHocPersonnel`) after adding the column, using an appropriate
existing timestamp source such as `CreatedOn`, `ReleasedOn`, or the current UTC
time so legacy records participate in delta sync.
In
`@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0083_AddAdHocResourceChangeTrackingPg.cs`:
- Around line 19-21: The M0083_AddAdHocResourceChangeTrackingPg migration adds
the nullable ModifiedOn column in the table loop but does not populate it for
existing records, so pre-existing rows will remain NULL and be skipped by
/Sync/Changes. Update the migration logic around the Schema.Table(t).Exists()
and Alter.Table(t).AddColumn("ModifiedOn") flow to backfill a sensible timestamp
for existing rows after adding the column, using the same migration path and
table iteration that already handles the column creation.
---
Duplicate comments:
In
`@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0081_AddIncidentCommandChangeTrackingPg.cs`:
- Around line 25-40: The PostgreSQL migration Up method in
M0081_AddIncidentCommandChangeTrackingPg adds ModifiedOn as nullable but does
not backfill existing rows, so they stay NULL and miss the initial sync. Update
the migration to set ModifiedOn for all pre-existing rows in each table from
ChangeTrackedTables, using the same backfill approach as the SQL Server
migration before or after adding the column. Keep the existing
CommandStructureNodes DeletedOn addition unchanged, and make sure the backfill
is applied consistently within Up.
---
Nitpick comments:
In `@Core/Resgrid.Services/IncidentCommandService.cs`:
- Around line 365-396: GetChangesSinceAsync in IncidentCommandService is loading
entire department-scoped collections and filtering them in memory, which will
scale poorly as data grows. Update the repository calls for commands, nodes,
assignments, objectives, timers, annotations, roles, and timeline entries to
query by sinceUtc/ModifiedOn (or OccurredOn for timeline) at the repository
level instead of using GetAllByDepartmentIdAsync plus Changed filtering. Keep
the existing change-shaping logic, but move the predicate into the repository
methods or add new indexed query methods so the method only returns delta rows.
In
`@Providers/Resgrid.Providers.Migrations/Migrations/M0082_AddCheckInRecordIdempotencyKey.cs`:
- Around line 12-18: Add a DB-level idempotency guard in
M0082_AddCheckInRecordIdempotencyKey instead of only adding the nullable
IdempotencyKey column. Update the migration to create a filtered unique index on
CheckInRecords.IdempotencyKey, scoped with DepartmentId (or the same tenant
scope used by PerformCheckInAsync) and excluding null values, so concurrent
replays cannot insert duplicates. Make sure the index is created alongside the
existing AddColumn change and that the service layer can deterministically
handle the unique-constraint violation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3951bf55-d737-4e8a-a00d-815e42c39750
⛔ Files ignored due to path filters (5)
Tests/Resgrid.Tests/Services/CheckInTimerServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/IncidentCommandServiceParTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/IncidentResourcesServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/IncidentVoiceServiceTests.csis excluded by!**/Tests/**docs/architecture/offline-first-architecture.mdis excluded by!**/*.md
📒 Files selected for processing (25)
Core/Resgrid.Model/CheckInRecord.csCore/Resgrid.Model/IChangeTracked.csCore/Resgrid.Model/IncidentCommand/CommandStructureNode.csCore/Resgrid.Model/IncidentCommand/IncidentAdHocResources.csCore/Resgrid.Model/IncidentCommand/IncidentCommand.csCore/Resgrid.Model/IncidentCommand/IncidentCommandChanges.csCore/Resgrid.Model/IncidentCommand/IncidentRole.csCore/Resgrid.Model/IncidentCommand/IncidentTacticals.csCore/Resgrid.Model/Services/IIncidentCommandService.csCore/Resgrid.Model/Services/IIncidentResourcesService.csCore/Resgrid.Services/CheckInTimerService.csCore/Resgrid.Services/IncidentCommandService.csCore/Resgrid.Services/IncidentResourcesService.csCore/Resgrid.Services/IncidentVoiceService.csProviders/Resgrid.Providers.Migrations/Migrations/M0081_AddIncidentCommandChangeTracking.csProviders/Resgrid.Providers.Migrations/Migrations/M0082_AddCheckInRecordIdempotencyKey.csProviders/Resgrid.Providers.Migrations/Migrations/M0083_AddAdHocResourceChangeTracking.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0081_AddIncidentCommandChangeTrackingPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0082_AddCheckInRecordIdempotencyKeyPg.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0083_AddAdHocResourceChangeTrackingPg.csWeb/Resgrid.Web.Services/Controllers/v4/CheckInTimersController.csWeb/Resgrid.Web.Services/Controllers/v4/SyncController.csWeb/Resgrid.Web.Services/Models/v4/CheckInTimers/CheckInTimerModels.csWeb/Resgrid.Web.Services/Models/v4/Sync/SyncModels.csWeb/Resgrid.Web.Services/Resgrid.Web.Services.xml
Comment on lines
+10
to
+11
| /// Ad-hoc resources are not change-tracked and are pulled separately (full refetch). See | ||
| /// docs/architecture/offline-first-architecture.md. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Doc comment contradicts the model and the actual delta behavior.
The remark says ad-hoc resources "are not change-tracked and are pulled separately (full refetch)", yet this payload exposes AdHocUnits/AdHocPersonnel (Lines 32-34) and GetAdHocChangesSinceAsync filters them by ModifiedOn (delta, not full refetch). This will mislead clients implementing the sync contract.
📝 Suggested doc fix
- /// Soft-deleted / closed / released rows ARE included (with their state columns set) so the client can remove or
- /// update them locally. The client stores <see cref="ServerTimestampMs"/> and passes it back as the next `since`.
- /// Ad-hoc resources are not change-tracked and are pulled separately (full refetch). See
- /// docs/architecture/offline-first-architecture.md.
+ /// Soft-deleted / closed / released rows ARE included (with their state columns set) so the client can remove or
+ /// update them locally. The client stores <see cref="ServerTimestampMs"/> and passes it back as the next `since`.
+ /// Ad-hoc resources are change-tracked via ModifiedOn and delta-pulled alongside command rows (aggregated by
+ /// SyncController). See docs/architecture/offline-first-architecture.md.📝 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
| /// Ad-hoc resources are not change-tracked and are pulled separately (full refetch). See | |
| /// docs/architecture/offline-first-architecture.md. | |
| /// Soft-deleted / closed / released rows ARE included (with their state columns set) so the client can remove or | |
| /// update them locally. The client stores <see cref="ServerTimestampMs"/> and passes it back as the next `since`. | |
| /// Ad-hoc resources are change-tracked via ModifiedOn and delta-pulled alongside command rows (aggregated by | |
| /// SyncController). See docs/architecture/offline-first-architecture.md. |
🤖 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 `@Core/Resgrid.Model/IncidentCommand/IncidentCommandChanges.cs` around lines 10
- 11, The XML doc comment on IncidentCommandChanges contradicts the actual sync
behavior exposed by AdHocUnits, AdHocPersonnel, and GetAdHocChangesSinceAsync.
Update the remark to describe these ad-hoc resources as delta-tracked by
ModifiedOn and returned through the incremental changes payload, rather than
saying they are not change-tracked or require a full refetch. Keep the wording
aligned with the model fields and the filtering logic so clients can rely on the
sync contract.
Comment on lines
+26
to
+40
| public override void Up() | ||
| { | ||
| foreach (var table in ChangeTrackedTables) | ||
| { | ||
| if (Schema.Table(table).Exists() && !Schema.Table(table).Column("ModifiedOn").Exists()) | ||
| { | ||
| Alter.Table(table).AddColumn("ModifiedOn").AsDateTime2().Nullable(); | ||
| } | ||
| } | ||
|
|
||
| if (Schema.Table("CommandStructureNodes").Exists() && !Schema.Table("CommandStructureNodes").Column("DeletedOn").Exists()) | ||
| { | ||
| Alter.Table("CommandStructureNodes").AddColumn("DeletedOn").AsDateTime2().Nullable(); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the command-side delta reader to see how null ModifiedOn is handled
fd -t f 'IncidentCommandService.cs' Core/Resgrid.Services --exec sed -n '320,400p' {}Repository: Resgrid/Core
Length of output: 3732
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the sync controller cursor handling and the migration context
fd -t f 'SyncController.cs' Core/Resgrid.* --exec sed -n '1,240p' {}
printf '\n---\n'
fd -t f 'M0081_AddIncidentCommandChangeTracking.cs' Providers/Resgrid.Providers.Migrations --exec sed -n '1,220p' {}Repository: Resgrid/Core
Length of output: 2018
Backfill ModifiedOn for existing rows in M0081_AddIncidentCommandChangeTracking.cs.
GetChangesSinceAsync only surfaces rows with ModifiedOn.HasValue && ModifiedOn > sinceUtc, so adding the column as nullable leaves all pre-existing commands/nodes/assignments/objectives/timers/annotations/roles invisible to the initial delta pull. Seed the new column during Up() so current board state is included on first sync.
🤖 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
`@Providers/Resgrid.Providers.Migrations/Migrations/M0081_AddIncidentCommandChangeTracking.cs`
around lines 26 - 40, Backfill the new ModifiedOn column in
M0081_AddIncidentCommandChangeTracking so existing rows are visible to
GetChangesSinceAsync on the first sync. Update the Up() migration logic around
ChangeTrackedTables and the CommandStructureNodes alter to seed ModifiedOn for
pre-existing records, not just add it as nullable, so current
commands/nodes/assignments/objectives/timers/annotations/roles are included in
the initial delta pull.
Comment on lines
+14
to
+23
| public override void Up() | ||
| { | ||
| foreach (var table in Tables) | ||
| { | ||
| if (Schema.Table(table).Exists() && !Schema.Table(table).Column("ModifiedOn").Exists()) | ||
| { | ||
| Alter.Table(table).AddColumn("ModifiedOn").AsDateTime2().Nullable(); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Pre-existing ad-hoc rows will never appear in delta sync.
ModifiedOn is added as nullable with no backfill, so any IncidentAdHocUnits/IncidentAdHocPersonnel rows created before this migration keep ModifiedOn = NULL. The downstream Changed predicate in GetAdHocChangesSinceAsync requires e.ModifiedOn.HasValue && e.ModifiedOn.Value > sinceUtc, so those rows are excluded even on an initial full pull (since <= 0 → DateTime.MinValue). A reconnecting client would silently miss all pre-migration ad-hoc resources until they are touched again.
Consider backfilling ModifiedOn (e.g. from CreatedOn/ReleasedOn, or GETUTCDATE()) in Up() so existing rows participate in the first sync.
🛠️ Possible backfill
if (Schema.Table(table).Exists() && !Schema.Table(table).Column("ModifiedOn").Exists())
{
Alter.Table(table).AddColumn("ModifiedOn").AsDateTime2().Nullable();
+ Execute.Sql($"UPDATE [{table}] SET [ModifiedOn] = COALESCE([ReleasedOn], [CreatedOn]) WHERE [ModifiedOn] IS NULL");
}📝 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
| public override void Up() | |
| { | |
| foreach (var table in Tables) | |
| { | |
| if (Schema.Table(table).Exists() && !Schema.Table(table).Column("ModifiedOn").Exists()) | |
| { | |
| Alter.Table(table).AddColumn("ModifiedOn").AsDateTime2().Nullable(); | |
| } | |
| } | |
| } | |
| public override void Up() | |
| { | |
| foreach (var table in Tables) | |
| { | |
| if (Schema.Table(table).Exists() && !Schema.Table(table).Column("ModifiedOn").Exists()) | |
| { | |
| Alter.Table(table).AddColumn("ModifiedOn").AsDateTime2().Nullable(); | |
| Execute.Sql($"UPDATE [{table}] SET [ModifiedOn] = COALESCE([ReleasedOn], [CreatedOn]) WHERE [ModifiedOn] IS NULL"); | |
| } | |
| } | |
| } |
🤖 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
`@Providers/Resgrid.Providers.Migrations/Migrations/M0083_AddAdHocResourceChangeTracking.cs`
around lines 14 - 23, The migration that adds `ModifiedOn` to the ad-hoc tables
leaves existing rows with null timestamps, so they never satisfy the
`GetAdHocChangesSinceAsync` change filter and won’t sync on reconnect or first
pull. Update `M0083_AddAdHocResourceChangeTracking.Up()` to backfill
`ModifiedOn` for pre-existing rows in the affected tables (`IncidentAdHocUnits`
and `IncidentAdHocPersonnel`) after adding the column, using an appropriate
existing timestamp source such as `CreatedOn`, `ReleasedOn`, or the current UTC
time so legacy records participate in delta sync.
Comment on lines
+19
to
+21
| if (Schema.Table(t).Exists() && !Schema.Table(t).Column("ModifiedOn".ToLower()).Exists()) | ||
| { | ||
| Alter.Table(t).AddColumn("ModifiedOn".ToLower()).AsDateTime2().Nullable(); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor
🧩 Analysis chain
🌐 Web query:
FluentMigrator 7.0.0 Postgres generator AsDateTime2 column type mapping
💡 Result:
In FluentMigrator 7.0.0, the AsDateTime2() column type extension is supported and maps to the timestamp data type when using the PostgreSQL generator [1][2]. While AsDateTime2() is explicitly available in the FluentMigrator API [1][3], the PostgreSQL generator maps DbType.DateTime2 to the standard PostgreSQL timestamp type [2]. This behavior is consistent across recent versions of FluentMigrator, as PostgreSQL's timestamp type effectively covers the functional requirements typically associated with DateTime2 in other SQL dialects [4][2]. If you require specific PostgreSQL timestamp behaviors—such as time zone awareness—it is recommended to use AsDateTimeOffset(), which maps to the PostgreSQL timestamptz (timestamp with time zone) type [2]. For scenarios requiring database-specific precision or custom timestamp definitions that the built-in generator does not provide, you can use the AsCustom("type_name") method to explicitly define the column type in your migration [1][5]. Top results: [1], [3], [2]
Citations:
- 1: https://fluentmigrator.github.io/basics/columns.html
- 2: https://github.com/fluentmigrator/fluentmigrator/blob/master/src/FluentMigrator.Runner.Postgres/Generators/Postgres/PostgresTypeMap.cs
- 3: https://fluentmigrator.github.io/operations/create-tables.html
- 4: Support datetime2 type fluentmigrator/fluentmigrator#974
- 5: https://fluentmigrator.github.io/advanced/custom-extensions.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target migration and the related SQL Server migration for the same version.
git ls-files 'Providers/Resgrid.Providers.MigrationsPg/Migrations/M0083_AddAdHocResourceChangeTrackingPg.cs' \
'Providers/Resgrid.Providers.MigrationsSql/Migrations/M0083_AddAdHocResourceChangeTracking.cs' \
'Providers/Resgrid.Providers.Migrations*/Migrations/M0083_AddAdHocResourceChangeTracking*.cs'
echo '--- Pg migration outline ---'
ast-grep outline Providers/Resgrid.Providers.MigrationsPg/Migrations/M0083_AddAdHocResourceChangeTrackingPg.cs --view expanded || true
echo '--- Sql migration outline ---'
ast-grep outline Providers/Resgrid.Providers.MigrationsSql/Migrations/M0083_AddAdHocResourceChangeTracking.cs --view expanded || true
echo '--- Pg migration lines ---'
sed -n '1,220p' Providers/Resgrid.Providers.MigrationsPg/Migrations/M0083_AddAdHocResourceChangeTrackingPg.cs
echo '--- Sql migration lines ---'
sed -n '1,240p' Providers/Resgrid.Providers.MigrationsSql/Migrations/M0083_AddAdHocResourceChangeTracking.csRepository: Resgrid/Core
Length of output: 1923
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If the migrations differ, check whether the Pg migration also backfills the new column.
rg -n "ModifiedOn|Update.*ModifiedOn|Set.*ModifiedOn|Backfill|Execute\.Sql|Update\(" Providers/Resgrid.Providers.MigrationsPg/Migrations/M0083_AddAdHocResourceChangeTrackingPg.cs Providers/Resgrid.Providers.MigrationsSql/Migrations/M0083_AddAdHocResourceChangeTracking.csRepository: Resgrid/Core
Length of output: 1203
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the shared migration counterpart to see whether it performs a backfill or uses a different type.
sed -n '1,240p' Providers/Resgrid.Providers.Migrations/Migrations/M0083_AddAdHocResourceChangeTracking.csRepository: Resgrid/Core
Length of output: 1174
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the PostgreSQL generator maps DateTime2 to timestamp in the bundled FluentMigrator sources, if present.
rg -n "DateTime2|timestamp" -g '*/PostgresTypeMap.cs' -g '*/Postgres*.cs' .Repository: Resgrid/Core
Length of output: 150
Add a backfill for existing rows
Providers/Resgrid.Providers.MigrationsPg/Migrations/M0083_AddAdHocResourceChangeTrackingPg.cs:19-21 adds a nullable ModifiedOn column, but leaves existing rows NULL. Seed the column for pre-existing records if they should participate in /Sync/Changes.
🤖 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
`@Providers/Resgrid.Providers.MigrationsPg/Migrations/M0083_AddAdHocResourceChangeTrackingPg.cs`
around lines 19 - 21, The M0083_AddAdHocResourceChangeTrackingPg migration adds
the nullable ModifiedOn column in the table loop but does not populate it for
existing records, so pre-existing rows will remain NULL and be skipped by
/Sync/Changes. Update the migration logic around the Schema.Table(t).Exists()
and Alter.Table(t).AddColumn("ModifiedOn") flow to backfill a sensible timestamp
for existing rows after adding the column, using the same migration path and
table iteration that already handles the column creation.
Contributor
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 `@Core/Resgrid.Services/CheckInTimerService.cs`:
- Around line 341-350: The replay-adoption catch in CheckInTimerService’s save
flow is too broad because it catches every exception and can mask real failures
as successful idempotent replays. Narrow the exception filter around the
SaveOrUpdateAsync path to only handle PostgreSQL unique-constraint violations by
catching Npgsql.PostgresException and checking SqlState == "23505" together with
a non-empty record.IdempotencyKey; keep the existing winner lookup logic, but
let all other exceptions propagate immediately so non-unique-key errors are not
swallowed.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7a28cb24-6850-4249-bcd8-4a404bd2208e
⛔ Files ignored due to path filters (3)
Tests/Resgrid.Tests/Services/CheckInTimerServiceTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/IncidentCommandServiceParTests.csis excluded by!**/Tests/**Tests/Resgrid.Tests/Services/IncidentResourcesServiceTests.csis excluded by!**/Tests/**
📒 Files selected for processing (5)
Core/Resgrid.Services/CheckInTimerService.csCore/Resgrid.Services/IncidentCommandService.csCore/Resgrid.Services/IncidentResourcesService.csProviders/Resgrid.Providers.Migrations/Migrations/M0082_AddCheckInRecordIdempotencyKey.csProviders/Resgrid.Providers.MigrationsPg/Migrations/M0082_AddCheckInRecordIdempotencyKeyPg.cs
🚧 Files skipped from review as they are similar to previous changes (3)
- Providers/Resgrid.Providers.MigrationsPg/Migrations/M0082_AddCheckInRecordIdempotencyKeyPg.cs
- Core/Resgrid.Services/IncidentResourcesService.cs
- Core/Resgrid.Services/IncidentCommandService.cs
Comment on lines
+341
to
+350
| catch (Exception) when (!string.IsNullOrWhiteSpace(record.IdempotencyKey)) | ||
| { | ||
| // The in-memory pre-check above is check-then-insert and races under concurrent replays; the filtered | ||
| // unique index UX_CheckInRecords_Department_IdempotencyKey is the real guard. If we lost the race, | ||
| // adopt the winner — same idempotent result as the pre-check — rather than surfacing a 500 (which would | ||
| // just trigger another retry). | ||
| var existing = await _recordRepository.GetByCallIdAsync(record.CallId); | ||
| var winner = existing?.FirstOrDefault(r => r.IdempotencyKey == record.IdempotencyKey); | ||
| if (winner != null) | ||
| return winner; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify which provider/repository exception types represent unique constraint violations
# so the catch filter can be narrowed without guessing.
rg -nP -C3 '(SaveOrUpdateAsync|SqlException|PostgresException|DbUpdateException|unique constraint|duplicate key|2601|2627|23505)' Core Providers -g '*.cs'Repository: Resgrid/Core
Length of output: 50369
🏁 Script executed:
python3 - <<'PY'
code_snippet = """
try
{
saved = await _recordRepository.SaveOrUpdateAsync(record, cancellationToken);
}
catch (Exception) when (!string.IsNullOrWhiteSpace(record.IdempotencyKey))
{
var existing = await _recordRepository.GetByCallIdAsync(record.CallId);
var winner = existing?.FirstOrDefault(r => r.IdempotencyKey == record.IdempotencyKey);
if (winner != null)
return winner;
throw;
}
"""
# Analyze the behavior of `throw` in this specific context
analysis = """
The snippet uses `catch (Exception)` without specifying a variable name.
Inside the block:
1. `var existing = await ...` is an I/O call. If this call fails, an *new* exception is awaited.
- If it throws an `OperationCanceledException`, that exception bubbles up immediately. The original `Exception` is effectively lost/swapped for the new failure if the await fails, or the new one propagates.
- If it succeeds but `winner` is null, the code hits `throw;`.
2. C# `throw;` with no arguments re-throws the *original* exception caught by the `catch` block (the one from `SaveOrUpdateAsync`).
3. Therefore, if `winner` is null, the original exception propagates correctly.
However, the risk remains if `SaveOrUpdateAsync` throws an exception that is NOT a unique violation (e.g., FK constraint violation "23503", Serialization error, or Timeout), and `existing` query *successfully* returns a record (perhaps from a different idempotency key or just a stale read).
If `winner` is found (even if it's the wrong idempotency key? No, the code checks `r.IdempotencyKey == record.IdempotencyKey`), then it returns the winner.
The logic `if (winner != null)` only returns if a record matches the key.
If the original exception was a Timeout, but `winner` happens to exist in the DB with the matching key (from a previous retry that partially completed), the current code treats it as a success.
CRITICAL ISSUE: The exception filter `when (!string.IsNullOrWhiteSpace(record.IdempotencyKey))` applies to the ENTRY of the catch block.
It does NOT guard against:
1. Non-unique exceptions (like Foreign Keys) where a matching record happens to exist.
2. It is safer to check the exception type explicitly.
"""
print(analysis)
PYRepository: Resgrid/Core
Length of output: 1853
Limit retry adoption strictly to unique-key violations.
The current retry logic catches all exceptions regardless of type, risking the masking of valid failures (e.g., timeouts, foreign key violations) as successful replays if a matching row happens to exist. For example, if a transaction times out but a partial write created the record, the code returns success even though the original operation failed with a non-unique-key error.
Refine the exception handling to filter only for PostgreSQL unique constraint violations (SqlState == "23505") to ensure other errors propagate immediately.
Proposed Fix
try
{
saved = await _recordRepository.SaveOrUpdateAsync(record, cancellationToken);
}
catch (Npgsql.PostgresException ex) when (ex.SqlState == "23505" && !string.IsNullOrWhiteSpace(record.IdempotencyKey))
{
var existing = await _recordRepository.GetByCallIdAsync(record.CallId, cancellationToken);
var winner = existing?.FirstOrDefault(r => r.IdempotencyKey == record.IdempotencyKey);
if (winner != null)
return winner;
throw;
}🤖 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 `@Core/Resgrid.Services/CheckInTimerService.cs` around lines 341 - 350, The
replay-adoption catch in CheckInTimerService’s save flow is too broad because it
catches every exception and can mask real failures as successful idempotent
replays. Narrow the exception filter around the SaveOrUpdateAsync path to only
handle PostgreSQL unique-constraint violations by catching
Npgsql.PostgresException and checking SqlState == "23505" together with a
non-empty record.IdempotencyKey; keep the existing winner lookup logic, but let
all other exceptions propagate immediately so non-unique-key errors are not
swallowed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
api/v{version}/Sync/Changes) that returns incident command updates and related ad-hoc resource changes using a cursor.Bug Fixes