Sync v1.4.0 - #5
Conversation
📝 WalkthroughWalkthroughThis PR adds configurable driver schedule generation and a Flutter interface. It also updates container-photo handling, optimistic rollback, cache clearing, Firestore validation, accessibility labels, input limits, documentation, tests, and the application version. ChangesDriver scheduling
Operational reliability
Release and interface polish
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🔴 Critical · up to The change adds schedule generation and photo persistence updates, but the current head still has a build-blocking type error, can strand or duplicate existing photo records, can race photo replacements, and can freeze the app for large schedules. These are concrete correctness and availability risks, so the PR is not merge-ready until the blocking issues are fixed; smaller validation and accessibility follow-ups also remain. Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 `@lib/src/services/container_photo_sync_service.dart`:
- Around line 8-11: Update readKey to fall back to the legacy slug-only document
ID when the hash-suffixed ID is absent, and migrate the legacy document to the
new ID before returning it. Preserve existing behavior for current documents,
and add a regression test covering reads from a legacy document.
- Around line 8-11: Update containerPhotoDocId so unlabeledDocId is returned
only when trimmed is empty. For non-empty locations whose _readableSlug result
is empty, generate a document ID using a fixed readable fallback prefix plus
_hash8(trimmed), preserving the existing slug-hash format otherwise; add
coverage for a Unicode-only location.
In `@lib/src/services/driver_schedule_generator.dart`:
- Around line 75-80: Bound the repair work in the schedule-generation flow,
especially the loop around _resolveBackToBack and _resolveConflicts and their
internal pass loops. Replace fixed high iteration limits with a budget scaled to
slots or an overall work/time limit so large schedules cannot monopolize the UI
thread; preserve early termination when no moves occur. Reduce repeated per-slot
allocations in _namesIn and _othersIn where practical by reusing hoisted name
sets during each repair pass.
In `@lib/src/state/pit_controller_mixin.dart`:
- Around line 98-100: Convert the clamped indices passed to List.insert in both
affected sites of lib/src/state/pit_controller_mixin.dart (lines 98-100 and
129-135) to int with toInt(), including the calls in the restoration logic
around _pitConfirmed and its sibling insertion site.
In `@lib/src/ui/borrow_tab.dart`:
- Around line 774-775: Ensure _save or BorrowRecord.toJson validates _contact
before writing, enforcing the Firestore limit of 256 characters by rejecting or
truncating overlong values. Preserve valid contact values unchanged, and add a
regression test covering an overlong loaded contact during save.
In `@lib/src/ui/driver_schedule_screen.dart`:
- Around line 457-479: Update the flagged-cell build logic to render a small
non-color marker for non-null cellFlag values, while preserving the existing
background and semantics label; use the flag’s existing identity to choose the
marker. Add the same marker mapping and visual treatment to _LegendItem so the
legend communicates each flag without relying on color.
In `@lib/src/ui/packing_tab.dart`:
- Around line 240-244: Update _handleContainerPhoto to return immediately when
_containerPhotoChecks already contains location, preventing replacement actions
while the initial readKey request is pending. Preserve existing busy and
failed-state handling, and add a widget test covering an initial read completing
after a successful replacement without overwriting the new key.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1dc3a54f-dfe8-4441-a059-7b2406f7957d
📒 Files selected for processing (21)
README.mdfirestore.ruleslib/src/models/driver_schedule.dartlib/src/services/container_photo_sync_service.dartlib/src/services/driver_schedule_generator.dartlib/src/services/photo_service.dartlib/src/state/pit_controller_mixin.dartlib/src/ui/app_shell.dartlib/src/ui/borrow_tab.dartlib/src/ui/driver_schedule_screen.dartlib/src/ui/packing_tab.dartlib/src/ui/schedule_tab.dartpubspec.yamltest/container_photo_sync_service_test.darttest/driver_schedule_generator_test.darttest/driver_schedule_screen_test.darttest/inventory_controller_test.darttest/packing_tab_test.darttest/photo_service_test.darttest/support/fake_container_photo_sync_service.darttest/support/fake_inventory_sync_service.dart
| String containerPhotoDocId(String location) { | ||
| final slug = location | ||
| .trim() | ||
| .toLowerCase() | ||
| .replaceAll(RegExp('[^a-z0-9]+'), '-') | ||
| .replaceAll(RegExp('^-+|-+\$'), ''); | ||
| return slug.isEmpty ? unlabeledDocId : slug; | ||
| final trimmed = location.trim(); | ||
| final slug = _readableSlug(trimmed); | ||
| return slug.isEmpty ? unlabeledDocId : '$slug-${_hash8(trimmed)}'; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve access to existing slug-only photo documents.
readKey now reads only the hash-suffixed document ID. Photos saved before this change remain at the previous slug-only ID, so the UI treats them as missing and can create a second document for the same location.
Read the legacy ID as a fallback, then migrate it, or deploy a Firestore migration before this code. Add a regression test that reads a legacy document.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/src/services/container_photo_sync_service.dart` around lines 8 - 11,
Update readKey to fall back to the legacy slug-only document ID when the
hash-suffixed ID is absent, and migrate the legacy document to the new ID before
returning it. Preserve existing behavior for current documents, and add a
regression test covering reads from a legacy document.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not map non-empty locations to unlabeled.
_readableSlug removes all non-[a-z0-9] characters. Locations such as "工具箱" then map to unlabeled, even though Firestore accepts them. Distinct locations can read the same photo. The first write also blocks later writes because location is immutable in firestore.rules.
Return unlabeledDocId only when trimmed.isEmpty. Use a fixed readable fallback prefix plus _hash8(trimmed) when the slug is empty. Add coverage for a Unicode-only location.
Proposed fix
String containerPhotoDocId(String location) {
final trimmed = location.trim();
+ if (trimmed.isEmpty) return unlabeledDocId;
final slug = _readableSlug(trimmed);
- return slug.isEmpty ? unlabeledDocId : '$slug-${_hash8(trimmed)}';
+ return '${slug.isEmpty ? 'location' : slug}-${_hash8(trimmed)}';
}📝 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.
| String containerPhotoDocId(String location) { | |
| final slug = location | |
| .trim() | |
| .toLowerCase() | |
| .replaceAll(RegExp('[^a-z0-9]+'), '-') | |
| .replaceAll(RegExp('^-+|-+\$'), ''); | |
| return slug.isEmpty ? unlabeledDocId : slug; | |
| final trimmed = location.trim(); | |
| final slug = _readableSlug(trimmed); | |
| return slug.isEmpty ? unlabeledDocId : '$slug-${_hash8(trimmed)}'; | |
| String containerPhotoDocId(String location) { | |
| final trimmed = location.trim(); | |
| if (trimmed.isEmpty) return unlabeledDocId; | |
| final slug = _readableSlug(trimmed); | |
| return '${slug.isEmpty ? 'location' : slug}-${_hash8(trimmed)}'; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/src/services/container_photo_sync_service.dart` around lines 8 - 11,
Update containerPhotoDocId so unlabeledDocId is returned only when trimmed is
empty. For non-empty locations whose _readableSlug result is empty, generate a
document ID using a fixed readable fallback prefix plus _hash8(trimmed),
preserving the existing slug-hash format otherwise; add coverage for a
Unicode-only location.
| _resolveConflicts(columns, slots, roleKeys); | ||
| for (var round = 0; round < 20; round++) { | ||
| final movedAdjacent = _resolveBackToBack(columns, slots, roleKeys); | ||
| final movedConflict = _resolveConflicts(columns, slots, roleKeys); | ||
| if (!movedAdjacent && !movedConflict) break; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Bound the repair work by slot count to protect the UI thread.
maxScheduleSlots is 500, and both repair passes are quadratic in slots. _resolveBackToBack runs slots × roleKeys × slots iterations per pass, and each inner iteration calls _namesIn and _othersIn, which allocate a new Set and call _at twice per role. One pass at 500 slots and 8 roles is about 16M inner steps. Each function allows up to 300 passes, and Line 76 repeats both up to 20 times.
DriverScheduleScreen._generate calls generate synchronously inside setState, so this work runs on the UI thread. A large match count can freeze the app for seconds.
Reduce the pass budget, scale it by slots, or add an overall work/time budget. Alternatively lower maxScheduleSlots to a realistic competition size.
⚡ Example: scale the pass budget and hoist the per-slot name set
- _resolveConflicts(columns, slots, roleKeys);
- for (var round = 0; round < 20; round++) {
+ final rounds = slots > 100 ? 4 : 20;
+ _resolveConflicts(columns, slots, roleKeys);
+ for (var round = 0; round < rounds; round++) {
final movedAdjacent = _resolveBackToBack(columns, slots, roleKeys);
final movedConflict = _resolveConflicts(columns, slots, roleKeys);
if (!movedAdjacent && !movedConflict) break;
} ) => {
for (final key in roleKeys)
- if (_at(columns, key, slot).isNotEmpty) _at(columns, key, slot),
+ if (_at(columns, key, slot) case final name when name.isNotEmpty) name,
};Also applies to: 183-275
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/src/services/driver_schedule_generator.dart` around lines 75 - 80, Bound
the repair work in the schedule-generation flow, especially the loop around
_resolveBackToBack and _resolveConflicts and their internal pass loops. Replace
fixed high iteration limits with a budget scaled to slots or an overall
work/time limit so large schedules cannot monopolize the UI thread; preserve
early termination when no moves occur. Reduce repeated per-slot allocations in
_namesIn and _othersIn where practical by reusing hoisted name sets during each
repair pass.
| final confirmed = _pitConfirmed[item.id]; | ||
| if (confirmed != null) { | ||
| restored.insert(previousIndex.clamp(0, restored.length), confirmed); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 2 '\.insert\(\s*.*\.clamp\(' lib testRepository: Spectrum3847/spectrum-pit
Length of output: 967
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- target source ---'
sed -n '80,145p' lib/src/state/pit_controller_mixin.dart
printf '%s\n' '--- Dart configuration ---'
fd -HI -t f '(pubspec\.yaml|analysis_options\.yaml|.*\.dart-version$)' . -x sh -c 'echo "--- $1"; sed -n "1,180p" "$1"' sh {}Repository: Spectrum3847/spectrum-pit
Length of output: 7275
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if command -v dart >/dev/null 2>&1; then
cat >/tmp/clamp_insert_check.dart <<'DART'
void main() {
final previousIndex = 1;
final restored = <Object>[];
restored.insert(previousIndex.clamp(0, restored.length), Object());
}
DART
dart analyze /tmp/clamp_insert_check.dart 2>&1 || true
else
echo "Dart SDK is not available"
fi
python3 - <<'PY'
from pathlib import Path
text = Path("lib/src/state/pit_controller_mixin.dart").read_text()
needles = [
"restored.insert(previousIndex.clamp(0, restored.length), confirmed);",
]
print("matching call count:", text.count(needles[0]))
print("all matching calls use .toInt():", all(
".toInt()" in line for line in text.splitlines()
if "restored.insert(previousIndex.clamp(0, restored.length), confirmed);" in line
))
PYRepository: Spectrum3847/spectrum-pit
Length of output: 251
Convert each clamped index to int.
int.clamp returns num, but List.insert requires an int index. Convert both calls with .toInt().
📍 Affects 1 file
lib/src/state/pit_controller_mixin.dart#L98-L100(this comment)lib/src/state/pit_controller_mixin.dart#L129-L135
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/src/state/pit_controller_mixin.dart` around lines 98 - 100, Convert the
clamped indices passed to List.insert in both affected sites of
lib/src/state/pit_controller_mixin.dart (lines 98-100 and 129-135) to int with
toInt(), including the calls in the restoration logic around _pitConfirmed and
its sibling insertion site.
| maxLength: 256, | ||
| keyboardType: TextInputType.text, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'maxLength: 256|_contact|contact:|toJson|LengthLimitingTextInputFormatter' \
lib/src/ui/borrow_tab.dart \
lib/src/models/borrow_record.dart \
testRepository: Spectrum3847/spectrum-pit
Length of output: 33525
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- BorrowRecord model ---'
cat -n lib/src/models/borrow_record.dart
printf '%s\n' '--- Borrow editor save and contact field ---'
sed -n '560,635p' lib/src/ui/borrow_tab.dart
sed -n '750,785p' lib/src/ui/borrow_tab.dart
printf '%s\n' '--- BorrowRecord persistence and validation references ---'
rg -n -C 5 \
'BorrowRecord|borrow_records|contact|maxLength|256|length|validate|validation|withConverter|toJson|fromJson' \
lib test firestore.rules firebase.json README.md 2>/dev/null || trueRepository: Spectrum3847/spectrum-pit
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Borrow controller write path ---'
ast-grep outline lib/src/state/borrow_controller.dart
rg -n -C 12 \
'upsert|onSubmit|save|add|update|catch|error|isValidBorrowRecord' \
lib/src/state/borrow_controller.dart \
lib/src/ui/borrow_tab.dart \
lib/src/services \
test/support/fake_borrow_sync_service.dart
printf '%s\n' '--- Relevant Firestore rule block ---'
sed -n '221,246p' firestore.rules
printf '%s\n' '--- Existing editor save tests ---'
rg -n -C 10 \
'open.*Borrow|edit.*borrow|Contact|contact|maxLength|Check out|Save|save' \
test/borrow_tab_test.dartRepository: Spectrum3847/spectrum-pit
Length of output: 50382
Validate the contact length before saving.
firestore.rules requires contact.size() <= 256, but TextField.maxLength does not constrain an overlong value loaded into _contact. Saving such a record sends the unchanged value through BorrowRecord.toJson(), and Firestore rejects the write. Validate or truncate the value in _save or the model, and add a regression test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/src/ui/borrow_tab.dart` around lines 774 - 775, Ensure _save or
BorrowRecord.toJson validates _contact before writing, enforcing the Firestore
limit of 256 characters by rejecting or truncating overlong values. Preserve
valid contact values unchanged, and add a regression test covering an overlong
loaded contact during save.
Source: MCP tools
| @override | ||
| Widget build(BuildContext context) { | ||
| final accent = PitPalette.accentOf(context); | ||
|
|
||
| final cellFlag = flag; | ||
| final label = name.isEmpty ? 'Nobody' : name; | ||
|
|
||
| return Semantics( | ||
| label: cellFlag == null ? label : '$label, ${cellFlag.label}', | ||
| excludeSemantics: true, | ||
| child: Container( | ||
| decoration: BoxDecoration( | ||
| color: cellFlag?.colorOf(context).withValues(alpha: 0.20), | ||
| border: selected ? Border.all(color: accent, width: 2) : null, | ||
| ), | ||
| padding: const EdgeInsets.symmetric(horizontal: 12, vertical: 12), | ||
| child: Text( | ||
| name.isEmpty ? '-' : name, | ||
| style: Theme.of(context).textTheme.bodyMedium, | ||
| ), | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a non-color marker for the cell flag.
The cell communicates conflict, backToBack, and handoff only through a background fill at 20% alpha. Users who cannot distinguish those hues get no visual cue, and the legend is also color-keyed. The Semantics label covers screen-reader users only.
Add a small icon or text marker inside the flagged cell, and repeat the same marker in _LegendItem.
♿ Example: add a per-flag icon
padding: const EdgeInsets.symmetric(horizontal: 12, vertical: 12),
- child: Text(
- name.isEmpty ? '-' : name,
- style: Theme.of(context).textTheme.bodyMedium,
+ child: Row(
+ mainAxisSize: MainAxisSize.min,
+ children: [
+ if (cellFlag != null) ...[
+ Icon(cellFlag.icon, size: 14, color: cellFlag.colorOf(context)),
+ const SizedBox(width: 6),
+ ],
+ Text(
+ name.isEmpty ? '-' : name,
+ style: Theme.of(context).textTheme.bodyMedium,
+ ),
+ ],
),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/src/ui/driver_schedule_screen.dart` around lines 457 - 479, Update the
flagged-cell build logic to render a small non-color marker for non-null
cellFlag values, while preserving the existing background and semantics label;
use the flag’s existing identity to choose the marker. Add the same marker
mapping and visual treatment to _LegendItem so the legend communicates each flag
without relying on color.
| Future<void> _handleContainerPhoto(String location) { | ||
| if (_busyContainerPhotos.contains(location)) return Future<void>.value(); | ||
| if (_containerPhotoLoadFailed.contains(location)) { | ||
| _containerPhotoChecks.add(location); | ||
| return _loadContainerPhoto(location); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Block photo actions while the initial read is pending.
A user can start a replacement before the automatic readKey call completes. If that read returns afterward, _loadContainerPhoto overwrites _containerPhotoKeys[location] with the stale key. The new key remains persisted, and the old key can remain undeleted.
Return early while _containerPhotoChecks contains location, or version reads and ignore obsolete results. Add a widget test that completes an initial read after a replacement succeeds.
Proposed fix
Future<void> _handleContainerPhoto(String location) {
- if (_busyContainerPhotos.contains(location)) return Future<void>.value();
+ if (_busyContainerPhotos.contains(location) ||
+ _containerPhotoChecks.contains(location)) {
+ return Future<void>.value();
+ }
if (_containerPhotoLoadFailed.contains(location)) {📝 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.
| Future<void> _handleContainerPhoto(String location) { | |
| if (_busyContainerPhotos.contains(location)) return Future<void>.value(); | |
| if (_containerPhotoLoadFailed.contains(location)) { | |
| _containerPhotoChecks.add(location); | |
| return _loadContainerPhoto(location); | |
| Future<void> _handleContainerPhoto(String location) { | |
| if (_busyContainerPhotos.contains(location) || | |
| _containerPhotoChecks.contains(location)) { | |
| return Future<void>.value(); | |
| } | |
| if (_containerPhotoLoadFailed.contains(location)) { | |
| _containerPhotoChecks.add(location); | |
| return _loadContainerPhoto(location); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/src/ui/packing_tab.dart` around lines 240 - 244, Update
_handleContainerPhoto to return immediately when _containerPhotoChecks already
contains location, preventing replacement actions while the initial readKey
request is pending. Preserve existing busy and failed-state handling, and add a
widget test covering an initial read completing after a successful replacement
without overwriting the new key.
2 participants
Driver match rotation, now inside the app. Enter a name list per role, set how many matches to cover, and generate a balanced rotation from the Schedule tab. Tapping a person highlights every match they are in, and the chart copies out as pasteable rows.
Also in this release: photos work on preview builds again. Uploads and existing photos were both failing on preview deploys because the photo Worker rejected the generated per-channel domain.
Summary by CodeRabbit