Sync v1.3.0 - #4
Conversation
📝 WalkthroughWalkthroughThe release adds container-photo synchronization, inventory-aware packing, vehicle maps, borrow contacts, overdue indicators, and new packing states. It also serializes selected remote and disk operations, improves desktop authentication cleanup, updates Firestore validation, and expands related tests. ChangesApplication features and reliability
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 10
🧹 Nitpick comments (5)
test/borrow_tab_test.dart (1)
141-149: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise contact submission in the checkout test.
The test leaves the new fifth field empty. The contact display tests use prebuilt records. They do not verify editor-to-
BorrowRecordmapping.Enter a contact value and assert
controller.items.single.contactafter checkout.Proposed test update
await tester.enterText(fields.at(2), '254'); await tester.enterText(fields.at(3), 'Texas States'); + await tester.enterText(fields.at(4), 'pit@team.org'); final checkOutButton = find.widgetWithText(FilledButton, 'Check out'); await tester.ensureVisible(checkOutButton); await tester.tap(checkOutButton); await tester.pumpAndSettle(); expect(controller.items.length, 1); expect(controller.items.single.toolName, 'Wrench'); expect(controller.items.single.teamNumber, 254); + expect(controller.items.single.contact, 'pit@team.org');🤖 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 `@test/borrow_tab_test.dart` around lines 141 - 149, Update the checkout test around the existing TextField entries and checkOutButton to fill the new fifth contact field, then assert that controller.items.single.contact matches the entered value after checkout. Preserve the existing input values and checkout flow while validating the editor-to-BorrowRecord mapping.lib/src/ui/packing_tab.dart (2)
292-316: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueReplace the nested merge loop with a map lookup.
_mergedRowsscans every record for every inventory item, which is O(items × records).buildcalls it on every controller notification, andAppShellnow rebuilds every minute. Index the records byitemIdfirst.♻️ Proposed refactor
final rows = <_PackingRowData>[]; + final byItemId = <String, PackingRecord>{}; + for (final record in records) { + byItemId.putIfAbsent(record.itemId, () => record); + } for (final item in inventoryItems) { - PackingRecord? match; - for (final record in records) { - if (record.itemId == item.id) { - match = record; - break; - } - } + final match = byItemId[item.id]; rows.add(🤖 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 292 - 316, Update _mergedRows to index records by itemId before iterating inventoryItems, then retrieve matches through map lookup instead of the nested records scan. Preserve the existing virtual-record creation and unmatched-record append behavior while reducing the merge to linear-time processing.
62-76: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMove the container-photo load out of
itemBuilder.
itemBuildermutates_containerPhotoChecksand starts_loadContainerPhoto, which callssetState. The call is safe today only because_loadContainerPhotoawaits before it callssetState. If that method later reads a cached value synchronously, Flutter throws "setState() or markNeedsBuild() called during build".Trigger the loads from a post-frame callback, or prefetch the distinct locations once after
_mergedRowsis computed.♻️ Proposed change
if (location != null) { if (!_containerPhotoChecks.contains(location)) { _containerPhotoChecks.add(location); - _loadContainerPhoto(location); + WidgetsBinding.instance.addPostFrameCallback( + (_) => unawaited(_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 62 - 76, Move container-photo prefetching out of the itemBuilder callback: after _mergedRows or entries are computed, collect distinct non-null locations and schedule _loadContainerPhoto for them via a post-frame callback, while preserving _containerPhotoChecks deduplication. Keep itemBuilder limited to rendering _LocationHeader and avoid any setState-triggering mutation during build.lib/src/ui/borrow_tab.dart (1)
615-615: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim once and reuse the value.
_contact.text.trim()runs twice on the same line. Compute it before the record is built.♻️ Proposed refactor
final teamNumber = int.tryParse(_teamNumber.text.trim()) ?? 0; + final contact = _contact.text.trim(); final existing = widget.record; @@ - contact: _contact.text.trim().isEmpty ? null : _contact.text.trim(), + contact: contact.isEmpty ? null : contact,🤖 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` at line 615, In the record-building flow containing the contact field, compute the trimmed _contact.text value once before constructing the record, then reuse that local value for both the empty check and the assigned contact value.lib/src/ui/app_shell.dart (1)
111-122: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider narrowing the periodic rebuild.
Timer.periodiccallssetStateon_AppShellStateevery minute. This rebuilds the app bar, theIndexedStack, and every tab body, only to refresh the overdue badge. Move the tick into a small widget that wraps theNavigationBardestinations, or rebuild only whenoverdueCountchanges.♻️ Sketch: rebuild only when the count changes
_overdueTick = Timer.periodic(const Duration(minutes: 1), (_) { - if (mounted) setState(() {}); + if (!mounted) return; + final count = widget.borrowController.overdueCount; + if (count == _overdueCount) return; + setState(() => _overdueCount = count); });🤖 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/app_shell.dart` around lines 111 - 122, Refactor the _overdueTick handling in _AppShellState so the one-minute timer no longer calls setState on the entire app shell; isolate the periodic refresh in a small widget around the NavigationBar destinations, or otherwise rebuild only when overdueCount changes. Keep the existing overdue badge behavior while leaving the app bar, IndexedStack, and tab bodies outside the timer-driven rebuild.
🤖 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 `@firestore.rules`:
- Around line 78-86: Update the containerPhotos rules to require docId to equal
the normalized request.resource.data.location using the existing
containerPhotoDocId normalization logic. Apply this binding to both create and
update alongside isValidContainerPhoto and isMonotonicUpdate, rejecting writes
whose document ID and location do not match.
In `@lib/src/services/container_photo_sync_service.dart`:
- Around line 5-11: The containerPhotoDocId function currently produces
colliding, lossy document IDs; replace the slug-based mapping with an immutable
container identifier or reversible encoding of the canonical location, while
preserving the unlabeled behavior as needed. Update both Firestore
implementations to use the collision-resistant ID consistently, and add tests
covering distinct locations such as “Drawer A/B” versus “Drawer A B” and empty
versus “unlabeled” locations.
In `@lib/src/services/photo_service.dart`:
- Around line 195-209: Update the disk-operation coordination around
_queueDiskOp and clearCache so clearCache acts as a barrier: await all in-flight
or previously queued disk actions, prevent those pre-clear actions from writing
after cache removal, then clear the disk cache. Add a test that gates a disk
write, invokes clearCache while it is pending, and verifies the write cannot
recreate data after cleanup.
In `@lib/src/state/pit_controller_mixin.dart`:
- Line 85: Update the queued upsert flow around _pitQueueRemote and
pitUpsertRemote so a failed queued chain restores only the last remotely
confirmed item, not the failed operation’s stale previousItem; track confirmed
state per ID or reload the remote record after failure. Preserve stale-failure
handling while ensuring consecutive failures for one ID leave the cache at the
confirmed remote value, and add coverage for two failing queued upserts.
In `@lib/src/ui/app_shell.dart`:
- Around line 197-205: Update the withBadge helper in the navigation destination
construction to wrap the badged icon in Semantics with an explicit label
describing the overdue count, while leaving icons without badges unchanged.
In `@lib/src/ui/borrow_tab.dart`:
- Around line 771-778: Add maxLength: 256 to the TextField controlled by
_contact, keeping the existing optional contact label and hint unchanged so its
input limit matches isValidBorrowRecord.
In `@lib/src/ui/packing_tab.dart`:
- Around line 352-357: Ensure virtual packing rows use one stable persistence
ID: update _virtualRecord to be the sole place that assigns it, then in _advance
reuse row.record.id instead of creating a new UUID. Add an itemId-keyed busy
guard around the in-flight upsert so repeated taps cannot create duplicate
records. Apply these changes at lib/src/ui/packing_tab.dart#L352-L357 and
lib/src/ui/packing_tab.dart#L105-L124.
- Around line 208-215: Update _loadContainerPhoto to track failures from readKey
instead of silently treating them as a missing key; when the read fails,
preserve a non-actionable header state or report the failure through
_showFailure, and prevent _containerPhotoKeys[location] from being set as a
normal null result that allows _captureContainerPhoto/writeKey to overwrite an
existing remote photo.
- Around line 225-254: Update _captureContainerPhoto so _busyContainerPhotos
retains location through the entire capture and writeKey flow, removing it only
after persistence or failure completes. Before overwriting the location’s key,
retain the previous _containerPhotoKeys value and delete that blob after
writeKey succeeds, matching the cleanup behavior used by _capture.
In `@README.md`:
- Around line 17-18: Update the checksum guidance in the README so it
distinguishes corruption detection from authenticity: a colocated .sha256 file
only verifies the downloaded artifact against that checksum and cannot detect an
attacker replacing both files. State that tamper detection requires an expected
checksum obtained through a separate trusted channel or a signed
artifact/checksum.
---
Nitpick comments:
In `@lib/src/ui/app_shell.dart`:
- Around line 111-122: Refactor the _overdueTick handling in _AppShellState so
the one-minute timer no longer calls setState on the entire app shell; isolate
the periodic refresh in a small widget around the NavigationBar destinations, or
otherwise rebuild only when overdueCount changes. Keep the existing overdue
badge behavior while leaving the app bar, IndexedStack, and tab bodies outside
the timer-driven rebuild.
In `@lib/src/ui/borrow_tab.dart`:
- Line 615: In the record-building flow containing the contact field, compute
the trimmed _contact.text value once before constructing the record, then reuse
that local value for both the empty check and the assigned contact value.
In `@lib/src/ui/packing_tab.dart`:
- Around line 292-316: Update _mergedRows to index records by itemId before
iterating inventoryItems, then retrieve matches through map lookup instead of
the nested records scan. Preserve the existing virtual-record creation and
unmatched-record append behavior while reducing the merge to linear-time
processing.
- Around line 62-76: Move container-photo prefetching out of the itemBuilder
callback: after _mergedRows or entries are computed, collect distinct non-null
locations and schedule _loadContainerPhoto for them via a post-frame callback,
while preserving _containerPhotoChecks deduplication. Keep itemBuilder limited
to rendering _LocationHeader and avoid any setState-triggering mutation during
build.
In `@test/borrow_tab_test.dart`:
- Around line 141-149: Update the checkout test around the existing TextField
entries and checkOutButton to fill the new fifth contact field, then assert that
controller.items.single.contact matches the entered value after checkout.
Preserve the existing input values and checkout flow while validating the
editor-to-BorrowRecord mapping.
🪄 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: eb60c0e8-f207-4f6f-aa41-5a0572cbf8ef
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (74)
.pr_agent.tomlREADME.mdfirestore.ruleslib/main.dartlib/src/app.dartlib/src/models/borrow_record.dartlib/src/models/map_location.dartlib/src/models/packing_record.dartlib/src/services/container_photo_sync_service.dartlib/src/services/desktop_auth_service.dartlib/src/services/desktop_container_photo_sync_service.dartlib/src/services/desktop_firestore_cache_io.dartlib/src/services/photo_service.dartlib/src/services/user_scoped_firestore_cache.dartlib/src/state/borrow_controller.dartlib/src/state/pit_controller_mixin.dartlib/src/ui/app_shell.dartlib/src/ui/borrow_tab.dartlib/src/ui/maps_tab.dartlib/src/ui/packing_tab.dartlib/src/widgets/keyboard_shortcuts.dartpubspec.yamltest/app_bootstrap_error_test.darttest/borrow_controller_test.darttest/borrow_record_test.darttest/borrow_tab_test.darttest/container_photo_sync_service_test.darttest/dark_theme_tokens_test.darttest/debug_info_test.darttest/desktop_auth_service_test.darttest/desktop_launcher_service_test.darttest/desktop_pit_shift_sync_service_test.darttest/desktop_self_update_service_test.darttest/desktop_sync_services_test.darttest/desktop_update_service_test.darttest/desktop_user_role_service_test.darttest/docs_assets_exist_test.darttest/docs_viewer_test.darttest/http_timeout_client_test.darttest/inventory_controller_test.darttest/inventory_tab_test.darttest/issue_report_service_test.darttest/keyboard_shortcuts_test.darttest/large_text_layout_test.darttest/local_only_services_test.darttest/map_location_controller_test.darttest/map_location_test.darttest/maps_tab_test.darttest/packing_controller_test.darttest/packing_record_test.darttest/packing_tab_test.darttest/photo_disk_cache_test.darttest/photo_service_test.darttest/pit_shift_controller_test.darttest/pit_shift_test.darttest/schedule_tab_test.darttest/support/fake_borrow_sync_service.darttest/support/fake_container_photo_sync_service.darttest/support/fake_inventory_sync_service.darttest/support/fake_map_diagram_sync_service.darttest/support/fake_map_image_store.darttest/support/fake_map_location_sync_service.darttest/support/fake_packing_sync_service.darttest/support/fake_pit_shift_sync_service.darttest/support/fake_spectrum_auth_service.darttest/support/fake_user_role_service.darttest/support/photo_test_support.darttest/synced_map_image_store_test.darttest/telemetry_service_test.darttest/theme_controller_test.darttest/user_management_screen_test.darttest/user_role_controller_test.darttest/user_scoped_firestore_cache_test.darttest/widget_test.dart
💤 Files with no reviewable changes (17)
- test/support/fake_map_location_sync_service.dart
- test/keyboard_shortcuts_test.dart
- test/pit_shift_controller_test.dart
- test/support/fake_map_image_store.dart
- test/map_location_controller_test.dart
- test/user_role_controller_test.dart
- test/support/fake_borrow_sync_service.dart
- test/support/fake_pit_shift_sync_service.dart
- .pr_agent.toml
- test/theme_controller_test.dart
- test/desktop_user_role_service_test.dart
- test/support/fake_packing_sync_service.dart
- lib/src/widgets/keyboard_shortcuts.dart
- test/http_timeout_client_test.dart
- test/support/fake_spectrum_auth_service.dart
- test/photo_disk_cache_test.dart
- test/packing_controller_test.dart
| match /containerPhotos/{docId} { | ||
| allow read: if isAuthed() && isMember(); | ||
| allow create: if isAuthed() && isMember() | ||
| && isValidContainerPhoto(request.resource.data); | ||
| allow update: if isAuthed() && isMember() | ||
| && isValidContainerPhoto(request.resource.data) | ||
| && isMonotonicUpdate(resource.data, request.resource.data); | ||
| allow delete: if isAuthed() && isMember(); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect how container photo document ids are derived from location.
fd -t f 'container_photo' | xargs -r rg -n -C5 'doc\(|location|docId|collection\('Repository: Spectrum3847/spectrum-pit
Length of output: 12682
🌐 Web query:
Firebase Firestore query "contains multiple documents" no query results
💡 Result:
When a Cloud Firestore query returns no results, it is rarely due to a technical error. Instead, it typically indicates that the query constraints are not matching any documents in your database [1]. Use the following troubleshooting checklist to diagnose the issue: 1. Check for Missing Indexes Complex queries, especially those combining inequality filters or sorting by multiple fields, often require composite indexes [2][3]. If your query requires an index and it is missing, Firestore will return an error [2][4]. Check your console or application logs for a URL provided by Firestore that will automatically create the required index [2][3]. 2. Verify Security Rules Security rules are not filters; they act as an "all or nothing" gatekeeper [5]. If your security rules are configured in a way that the query could potentially access a document you do not have permission to read, the entire request will fail with a "Missing or insufficient permissions" error, rather than returning a partial result [5]. Ensure your security rules align with your query constraints [5]. 3. Understand Field Existence Firestore indexes only include documents that contain the field being queried or ordered [6][7]. If a document lacks a field entirely, it will be excluded from any query that uses an orderBy clause or a range filter (e.g., <, <=, >, >=) on that field [6][7]. 4. Validate Query Logic and Path - Inequality Limitations: You can only use a range/inequality filter on a single field per query [8][9]. Attempting to filter on multiple fields using range comparisons (e.g., age > 20 AND price < 100) is not supported [8][10]. - Data Pathing: If querying subcollections, ensure the path is correct [11]. If data is nested within a Map field, you must use dot notation (e.g., parentMap.childField) to access it [11]. - Empty Result Handling: A query returning zero results is a successful operation [1]. Your code should explicitly check for this state (e.g., checking if the snapshot is empty) rather than assuming an error occurred [1][2][4]. 5. Debugging - Console Comparison: Test your query directly in the Firebase Console's Firestore interface to verify the logic and ensure the documents you expect to retrieve actually exist and match your criteria [12]. - Check for Errors: Always wrap your query execution in a try/catch block (or use.catch) to handle permissions or index-related errors that might otherwise go unnoticed [2][13].
Citations:
- 1: https://stackoverflow.com/questions/60593077/firestore-why-is-my-code-not-calling-if-document-exists
- 2: https://www.rapidevelopers.com/firebase-tutorial/how-to-query-documents-in-firestore
- 3: https://cloud.google.com/firestore/docs/concepts/index-overview
- 4: https://firebase.google.com/codelabs/firestore-web
- 5: https://medium.com/firebase-developers/what-does-it-mean-that-firestore-security-rules-are-not-filters-68ec14f3d003
- 6: https://firebase.google.com/docs/firestore/query-data/order-limit-data
- 7: https://stackoverflow.com/questions/49579693/how-do-i-get-documents-where-a-specific-field-exists-does-not-exists-in-firebase
- 8: https://estuary.dev/firestore-limitations
- 9: https://stackoverflow.com/questions/57947567/firestore-compound-query-with-order-and-limit
- 10: https://stackoverflow.com/questions/56131733/firestore-how-to-merge-indexes-for-queries-containing-multiple-range-comparison
- 11: https://stackoverflow.com/questions/64376879/firestore-query-on-sub-collection-doesnt-return-data
- 12: https://stackoverflow.com/questions/76450619/why-is-firestore-wherearraycontains-not-fetching-expected-data-in-project-but-fe
- 13: https://stackoverflow.com/questions/70481627/fetching-documents-in-a-subcollection-of-a-cloud-firestore-database-does-not-ret
🌐 Web query:
location field Firestore duplicate document id
💡 Result:
In Cloud Firestore, document IDs are unique within their specific collection [1][2][3]. It is impossible to have two documents with the same ID in the same collection; any attempt to create a document with an ID already in use will either overwrite the existing document (if using a set operation) or fail (if using specific constraints or transaction logic) [4][5][6]. Key points regarding document IDs and uniqueness: Scope of Uniqueness: The uniqueness of a document ID is scoped strictly to the collection that contains it [1][3]. You can have documents with the same ID if they reside in different collections or subcollections [1][3]. The true unique identifier for any document in Firestore is its full path, which includes the collection name and the document ID [1][3]. Auto-Generated IDs: When you allow Firestore to auto-generate a document ID, it uses a random string (20 characters long) that is statistically guaranteed to be unique within the target collection [7][8][9]. The probability of a collision is considered extremely low, making these IDs effectively unique [7][10]. Common Causes of Apparent Duplicates: If you perceive a "duplicate" document ID in your collection, it is typically due to one of the following: * Hidden Characters: An ID might appear identical but contain hidden whitespace or invisible characters [11][6]. * Different Collections: The documents exist in different collections or subcollections [1][3]. * Overwriting: If you use set to create a document with an ID that already exists, you will overwrite the data in that document unless you use options like merge [5][6]. If you are encountering errors regarding "document already exists," verify the exact path you are targeting, including any potential leading/trailing spaces or unintended path segments [11][6].
Citations:
- 1: https://www.codestudy.net/blog/firestore-are-auto-generated-ids-unique-in-the-collection-or-globally/
- 2: https://firebase.google.com/docs/firestore/data-model
- 3: https://stackoverflow.com/questions/78226968/same-document-id-in-a-collection-group-but-in-different-parents
- 4: FirebaseError: Document already exists firebase/firebase-js-sdk#5549
- 5: https://firebase.google.com/docs/firestore/manage-data/add-data
- 6: https://stackoverflow.com/questions/63564799/firestore-allows-the-creation-of-two-separate-documents-with-the-same-id-in-a-co
- 7: https://stackoverflow.com/questions/55350061/firebase-firestore-does-creating-a-doc-reference-and-using-it-later-overwrite
- 8: https://stackoverflow.com/questions/46618719/firestore-are-auto-generated-ids-unique-in-the-collection-or-globally
- 9: https://stackoverflow.com/questions/61145596/how-unique-are-document-id-of-firestore-considering-more-than-million-document
- 10: https://stackoverflow.com/questions/54268257/what-are-the-chances-for-firestore-to-generate-two-identical-random-keys
- 11: https://discuss.google.dev/t/firestore-document-id-duplication-error-error-for-id-to-be-duplicated-in-firestore/129936
🌐 Web query:
Firebase Firestore set document non existing document rules document data
💡 Result:
To manage security rules for Cloud Firestore operations concerning the existence of documents, you should leverage the built-in resource variable, the exists() function, and the distinction between create and update rules [1][2][3]. Core Concepts: 1. The resource variable refers to the document currently in the database [3]. If you are performing a create operation, resource will be null because the document does not yet exist [1][4]. If you are performing an update or delete operation, resource will contain the existing document's data [3]. 2. The request.resource variable refers to the state of the document after the requested operation is completed [3]. 3. The exists() function allows you to check for the presence of a document at a specific path, returning a boolean value [5][6]. Common Patterns: Restricting Creation to Non-Existing Documents If you want to ensure a document is created only if it does not already exist, you can use the create rule [2]. Since the create rule only applies to operations that result in a new document, you do not technically need to check for existence within that rule itself [1]. However, you can explicitly enforce it if needed: service cloud.firestore { match /databases/{database}/documents { match /users/{uid} { allow create: if request.auth!= null &&!exists(/databases/$(database)/documents/users/$(uid)); } } } Handling Set Operations The set() method in client libraries can behave as either a create or an update depending on whether the document already exists [7]. Security rules differentiate this automatically: - If the document does not exist, Firestore evaluates the create rule [2]. - If the document already exists, Firestore evaluates the update rule [7][2]. Because of this, it is recommended to split your write rules into specific create and update rules to ensure that logic for new documents (e.g., initial field validation) does not clash with logic for existing documents (e.g., preventing modification of immutable fields) [7][4]. Accessing Data When checking existing data, always ensure the document exists to avoid runtime errors [8][9]. For instance, accessing resource.data on a create operation will result in an error because resource is null [4]. Using logical operators can help safely chain these checks: - exists(path) && get(path).data.field == value ensures that get() is only called if the document exists, preventing potential errors [9]. For further guidance, consult the official documentation on writing conditions and understanding rule behavior [5][10].
Citations:
- 1: https://stackoverflow.com/questions/46888701/create-a-document-only-if-it-doesnt-exist-in-firebase-firestore
- 2: https://cloud.google.com/firestore/native/docs/security/rules-structure
- 3: https://firebase.google.com/docs/rules/data-validation
- 4: https://stackoverflow.com/questions/55951645/how-to-set-firestore-security-rules-resource-data-null-value-error
- 5: https://firebase.google.com/docs/firestore/security/rules-conditions
- 6: https://firebase.google.com/docs/reference/rules/rules.firestore
- 7: https://stackoverflow.com/questions/62816249/firestore-security-rules-not-working-as-expected
- 8: https://stackoverflow.com/questions/62078385/firestore-rules-get-throws-error-instead-of-returning-null-when-document-does
- 9: https://stackoverflow.com/questions/63439961/inherited-access-firebase-security-rules
- 10: https://firebase.google.com/docs/rules/rules-behavior
Bind containerPhotos IDs to the normalized location field.
Firestore clients use containerPhotoDocId(location) as the document id, but the rule allows any docId. A member can set a different raw location value through the rule, so the id and location can diverge. Add a validation check that maps docId back to request.resource.data.location using the same slug normalization, and reject mismatched writes.
🤖 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 `@firestore.rules` around lines 78 - 86, Update the containerPhotos rules to
require docId to equal the normalized request.resource.data.location using the
existing containerPhotoDocId normalization logic. Apply this binding to both
create and update alongside isValidContainerPhoto and isMonotonicUpdate,
rejecting writes whose document ID and location do not match.
| String containerPhotoDocId(String location) { | ||
| final slug = location | ||
| .trim() | ||
| .toLowerCase() | ||
| .replaceAll(RegExp('[^a-z0-9]+'), '-') | ||
| .replaceAll(RegExp('^-+|-+\$'), ''); | ||
| return slug.isEmpty ? unlabeledDocId : slug; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use a collision-resistant container identifier.
containerPhotoDocId maps distinct locations to the same document ID. For example, "Drawer A/B" and "Drawer A B" both map to drawer-a-b. An empty location and "unlabeled" also map to unlabeled.
Both Firestore implementations use this ID. A write or clear for one container can overwrite or delete the other container photo. Use an immutable container ID, or a reversible encoding of the canonical location. Add collision tests.
🤖 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 5 - 11, The
containerPhotoDocId function currently produces colliding, lossy document IDs;
replace the slug-based mapping with an immutable container identifier or
reversible encoding of the canonical location, while preserving the unlabeled
behavior as needed. Update both Firestore implementations to use the
collision-resistant ID consistently, and add tests covering distinct locations
such as “Drawer A/B” versus “Drawer A B” and empty versus “unlabeled” locations.
| Future<void> _queueDiskOp(String key, Future<void> Function() action) { | ||
| final previous = _diskOps[key]; | ||
| late final Future<void> next; | ||
| next = () async { | ||
| try { | ||
| await previous; | ||
| } catch (_) {} | ||
| try { | ||
| await action(); | ||
| } finally { | ||
| if (identical(_diskOps[key], next)) _diskOps.remove(key); | ||
| } | ||
| }(); | ||
| _diskOps[key] = next; | ||
| return next; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Serialize clearCache with queued disk operations.
A queued write can complete after clearCache removes the disk cache. This leaves photos from a signed-out session on disk. StrategyApp starts this cleanup on sign-out.
Make clearCache a barrier. It must wait for in-flight actions and prevent actions queued before the clear from writing afterward. Add a test that clears the cache while a disk write is gated.
🤖 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/photo_service.dart` around lines 195 - 209, Update the
disk-operation coordination around _queueDiskOp and clearCache so clearCache
acts as a barrier: await all in-flight or previously queued disk actions,
prevent those pre-clear actions from writing after cache removal, then clear the
disk cache. Add a test that gates a disk write, invokes clearCache while it is
pending, and verifies the write cannot recreate data after cleanup.
| notifyListeners(); | ||
| try { | ||
| await pitUpsertRemote(item); | ||
| await _pitQueueRemote(item.id, () => pitUpsertRemote(item)); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not restore an unconfirmed optimistic item after queued failures.
If updates A → B → C queue for one ID and both remote updates fail, the first failure does not roll back because it is stale. The second failure restores B from previousItem, although the remote record is still A. The cache then persists B.
Track the last confirmed item per ID, or reload the record after a failed chain. Add coverage for two failing queued upserts for the same ID.
🤖 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` at line 85, Update the queued upsert
flow around _pitQueueRemote and pitUpsertRemote so a failed queued chain
restores only the last remotely confirmed item, not the failed operation’s stale
previousItem; track confirmed state per ID or reload the remote record after
failure. Preserve stale-failure handling while ensuring consecutive failures for
one ID leave the cache at the confirmed remote value, and add coverage for two
failing queued upserts.
| final overdueCount = widget.borrowController.overdueCount; | ||
| return _featureTabIndices.map((i) { | ||
| final m = _kTabMeta[i]; | ||
| final showBadge = i == AppTabs.borrowed && overdueCount > 0; | ||
| Widget withBadge(Icon icon) => | ||
| showBadge ? Badge(label: Text('$overdueCount'), child: icon) : icon; | ||
| return NavigationDestination( | ||
| icon: Icon(m.icon), | ||
| selectedIcon: Icon(m.selectedIcon), | ||
| icon: withBadge(Icon(m.icon)), | ||
| selectedIcon: withBadge(Icon(m.selectedIcon)), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add an accessible label for the overdue badge.
The badge exposes only the number. A screen reader announces "Borrowed, 3", which does not state what the number counts. Wrap the badged icon in Semantics with an explicit label.
♿ Proposed change
- Widget withBadge(Icon icon) =>
- showBadge ? Badge(label: Text('$overdueCount'), child: icon) : icon;
+ Widget withBadge(Icon icon) => showBadge
+ ? Semantics(
+ label: '$overdueCount overdue ${overdueCount == 1 ? 'loan' : 'loans'}',
+ child: Badge(label: Text('$overdueCount'), child: icon),
+ )
+ : icon;📝 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.
| final overdueCount = widget.borrowController.overdueCount; | |
| return _featureTabIndices.map((i) { | |
| final m = _kTabMeta[i]; | |
| final showBadge = i == AppTabs.borrowed && overdueCount > 0; | |
| Widget withBadge(Icon icon) => | |
| showBadge ? Badge(label: Text('$overdueCount'), child: icon) : icon; | |
| return NavigationDestination( | |
| icon: Icon(m.icon), | |
| selectedIcon: Icon(m.selectedIcon), | |
| icon: withBadge(Icon(m.icon)), | |
| selectedIcon: withBadge(Icon(m.selectedIcon)), | |
| final overdueCount = widget.borrowController.overdueCount; | |
| return _featureTabIndices.map((i) { | |
| final m = _kTabMeta[i]; | |
| final showBadge = i == AppTabs.borrowed && overdueCount > 0; | |
| Widget withBadge(Icon icon) => showBadge | |
| ? Semantics( | |
| label: '$overdueCount overdue ${overdueCount == 1 ? 'loan' : 'loans'}', | |
| child: Badge(label: Text('$overdueCount'), child: icon), | |
| ) | |
| : icon; | |
| return NavigationDestination( | |
| icon: withBadge(Icon(m.icon)), | |
| selectedIcon: withBadge(Icon(m.selectedIcon)), |
🤖 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/app_shell.dart` around lines 197 - 205, Update the withBadge
helper in the navigation destination construction to wrap the badged icon in
Semantics with an explicit label describing the overdue count, while leaving
icons without badges unchanged.
| const SizedBox(height: 12), | ||
| TextField( | ||
| controller: _contact, | ||
| decoration: const InputDecoration( | ||
| labelText: 'Contact (optional)', | ||
| hintText: 'Phone or email', | ||
| ), | ||
| ), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Cap the contact field to match the Firestore rule.
isValidBorrowRecord in firestore.rules rejects a contact value longer than 256 characters. This field accepts unlimited text. A long value therefore fails the remote write with a permission error after the user saves. Add maxLength: 256.
🛡️ Proposed fix
TextField(
controller: _contact,
+ maxLength: 256,
+ keyboardType: TextInputType.text,
decoration: const InputDecoration(
labelText: 'Contact (optional)',
hintText: 'Phone or email',
),
),📝 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.
| const SizedBox(height: 12), | |
| TextField( | |
| controller: _contact, | |
| decoration: const InputDecoration( | |
| labelText: 'Contact (optional)', | |
| hintText: 'Phone or email', | |
| ), | |
| ), | |
| const SizedBox(height: 12), | |
| TextField( | |
| controller: _contact, | |
| maxLength: 256, | |
| keyboardType: TextInputType.text, | |
| decoration: const InputDecoration( | |
| labelText: 'Contact (optional)', | |
| hintText: 'Phone or email', | |
| ), | |
| ), |
🤖 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 771 - 778, Add maxLength: 256 to the
TextField controlled by _contact, keeping the existing optional contact label
and hint unchanged so its input limit matches isValidBorrowRecord.
| Future<void> _loadContainerPhoto(String location) async { | ||
| String? key; | ||
| try { | ||
| key = await widget.containerPhotoSyncService.readKey(location); | ||
| } catch (_) {} | ||
| if (!mounted) return; | ||
| setState(() => _containerPhotoKeys[location] = key); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not swallow the container-photo read failure silently.
catch (_) {} discards every error from readKey. On failure, key stays null and _containerPhotoKeys[location] is set to null. The header then shows "Add a container photo" even when a photo exists remotely. If the user taps it, _captureContainerPhoto runs and writeKey overwrites the existing container photo reference. The previous photo becomes unreachable.
Track the failure and keep the header in a non-actionable state, or report the error with _showFailure.
🤖 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 208 - 215, Update
_loadContainerPhoto to track failures from readKey instead of silently treating
them as a missing key; when the read fails, preserve a non-actionable header
state or report the failure through _showFailure, and prevent
_containerPhotoKeys[location] from being set as a normal null result that allows
_captureContainerPhoto/writeKey to overwrite an existing remote photo.
| Future<void> _captureContainerPhoto(String location) async { | ||
| final source = await choosePhotoSource( | ||
| context, | ||
| widget.photoService.sources, | ||
| ); | ||
| if (source == null || !mounted) return; | ||
| setState(() => _busyContainerPhotos.add(location)); | ||
| String? key; | ||
| try { | ||
| key = await widget.photoService.capture(source); | ||
| } catch (error) { | ||
| _showFailure('add a photo to "$location"', error); | ||
| } finally { | ||
| if (mounted) setState(() => _busyContainerPhotos.remove(location)); | ||
| } | ||
| if (key == null) return; | ||
| if (!mounted) { | ||
| await _deleteKey(key, location); | ||
| return; | ||
| } | ||
| try { | ||
| await widget.containerPhotoSyncService.writeKey(location, key); | ||
| } catch (error) { | ||
| _showFailure('save the photo for "$location"', error); | ||
| await _deleteKey(key, location); | ||
| return; | ||
| } | ||
| if (!mounted) return; | ||
| setState(() => _containerPhotoKeys[location] = key); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clear the busy flag after the write, and delete the replaced photo.
Two problems exist in _captureContainerPhoto.
- The
finallyblock removeslocationfrom_busyContainerPhotosimmediately aftercapturereturns, beforewriteKeyruns. During thewriteKeyawait,_LocationHeaderre-enables the button and_containerPhotoKeys[location]still holds the old value. A second tap therefore starts another capture for the same location. - On replace, the previous key is never deleted.
_openContainerPhotocalls_captureContainerPhotoforPackingPhotoAction.replace, andwriteKeyoverwrites the stored key. The previous blob stays on disk forever. The per-item path in_capturedeletespreviousafter a successful upsert; this path does not.
🐛 Proposed fix
if (source == null || !mounted) return;
setState(() => _busyContainerPhotos.add(location));
+ final previous = _containerPhotoKeys[location];
String? key;
try {
key = await widget.photoService.capture(source);
- } catch (error) {
- _showFailure('add a photo to "$location"', error);
- } finally {
- if (mounted) setState(() => _busyContainerPhotos.remove(location));
- }
- if (key == null) return;
- if (!mounted) {
- await _deleteKey(key, location);
- return;
- }
- try {
- await widget.containerPhotoSyncService.writeKey(location, key);
- } catch (error) {
- _showFailure('save the photo for "$location"', error);
- await _deleteKey(key, location);
- return;
- }
- if (!mounted) return;
- setState(() => _containerPhotoKeys[location] = key);
+ if (key == null) return;
+ if (!mounted) {
+ await _deleteKey(key, location);
+ return;
+ }
+ await widget.containerPhotoSyncService.writeKey(location, key);
+ if (!mounted) return;
+ setState(() => _containerPhotoKeys[location] = key);
+ if (previous != null && previous != key) {
+ await _deleteKey(previous, location);
+ }
+ } catch (error) {
+ _showFailure('add a photo to "$location"', error);
+ if (key != null) await _deleteKey(key, location);
+ } finally {
+ if (mounted) setState(() => _busyContainerPhotos.remove(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 225 - 254, Update
_captureContainerPhoto so _busyContainerPhotos retains location through the
entire capture and writeKey flow, removing it only after persistence or failure
completes. Before overwriting the location’s key, retain the previous
_containerPhotoKeys value and delete that blob after writeKey succeeds, matching
the cleanup behavior used by _capture.
| PackingRecord _virtualRecord(InventoryItem item) => PackingRecord( | ||
| id: item.id, | ||
| itemId: item.id, | ||
| packingStatus: PackingStatus.notStarted, | ||
| updatedAt: item.updatedAt, | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Virtual packing rows have no stable identity, so each persistence path creates its own record. _virtualRecord assigns id: item.id, but _advance and _RecordEditorSheetState._save both discard that id and generate const Uuid().v4(), while the photo path (_handlePhoto → _capture → _current) persists the virtual record with id == item.id. Two paths on the same not-started row therefore write two PackingRecord documents for one itemId. _mergedRows matches only the first, and the second loop drops the rest, so the extra documents persist remotely and are unreachable from the UI.
lib/src/ui/packing_tab.dart#L352-L357: decide the single id used when a virtual row is first persisted, and make_virtualRecordthe only place that produces it.lib/src/ui/packing_tab.dart#L105-L124: reuserow.record.idinstead of generating a new UUID, and add a busy guard keyed byitemIdso a second tap during the in-flight upsert cannot create another record.
📍 Affects 1 file
lib/src/ui/packing_tab.dart#L352-L357(this comment)lib/src/ui/packing_tab.dart#L105-L124
🤖 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 352 - 357, Ensure virtual packing
rows use one stable persistence ID: update _virtualRecord to be the sole place
that assigns it, then in _advance reuse row.record.id instead of creating a new
UUID. Add an itemId-keyed busy guard around the in-flight upsert so repeated
taps cannot create duplicate records. Apply these changes at
lib/src/ui/packing_tab.dart#L352-L357 and lib/src/ui/packing_tab.dart#L105-L124.
| Every artifact ships with a `.sha256` file next to it. Because these builds are unsigned, the instructions below ask you to click past your platform's own integrity check, so this is the only thing left that tells a good download from a corrupted or tampered one. Download both files into the same folder and run: | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not claim that a colocated checksum detects tampering.
A .sha256 file downloaded from the same release detects corruption. It does not prove authenticity. An attacker who replaces both files can provide a matching checksum.
State that tamper detection requires the expected checksum from a separate trusted channel, or a signed artifact or checksum.
Proposed documentation change
-Every artifact ships with a `.sha256` file next to it. Because these builds are unsigned, the instructions below ask you to click past your platform's own integrity check, so this is the only thing left that tells a good download from a corrupted or tampered one.
+Every artifact ships with a `.sha256` file next to it. These commands detect a corrupted download. They do not authenticate the artifact when both files come from the same release.📝 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.
| Every artifact ships with a `.sha256` file next to it. Because these builds are unsigned, the instructions below ask you to click past your platform's own integrity check, so this is the only thing left that tells a good download from a corrupted or tampered one. Download both files into the same folder and run: | |
| Every artifact ships with a `.sha256` file next to it. These commands detect a corrupted download. They do not authenticate the artifact when both files come from the same release. Download both files into the same folder and run: | |
🤖 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 `@README.md` around lines 17 - 18, Update the checksum guidance in the README
so it distinguishes corruption detection from authenticity: a colocated .sha256
file only verifies the downloaded artifact against that checksum and cannot
detect an attacker replacing both files. State that tamper detection requires an
expected checksum obtained through a separate trusted channel or a signed
artifact/checksum.
2 participants
The Maps tab now has a third layout for a bus, van, or box truck, next to the lab and pit diagrams, so a load-out photo lives alongside the rest of the team's maps.
Borrowed tools can carry a contact (phone or email), and the Borrowed tab shows a badge for overdue loans right on the nav bar, so an expensive tool on loan does not get forgotten.
The packing tab now lists every inventory item up front, showing which ones have not been started yet instead of only the ones someone remembered to add by hand.
Packing photos can now cover a whole drawer or container at once, not just one item at a time, for confirming everything went in together.
Summary by CodeRabbit