Sync v1.0.0 - #1
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe pull request adds the initial Spectrum Pit Flutter application. It includes Firebase configuration, cross-platform runners, data models, synchronization services, role-based state, feature screens, security rules, documentation, CI, and tests. ChangesApplication and data foundation
User interface
Platform and repository tooling
Estimated code review effort: 5 (Critical) | ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 107
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/ci-workflows.md (1)
1-5: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winSeven doc files contain identical placeholder text instead of topic-specific content. Each file's body is byte-for-byte the same generic "internal document" disclaimer used in
docs/README.md. The filenames promise distinct technical content, but none of them deliver it yet.
docs/ci-workflows.md#L1-L5: replace the placeholder with actual documentation of the CI pipeline defined in.github/workflows/ci.yml.docs/database-plan.md#L1-L5: replace the placeholder with the Firestore schema/collections plan (matchingfirestore.rulesandfirestore.indexes.json).docs/developer-manual.md#L1-L5: replace the placeholder with developer onboarding and setup instructions.docs/external-dependencies.md#L1-L5: replace the placeholder with the list of external dependencies and their purpose.docs/regen-flutterfire-config.md#L1-L5: replace the placeholder with steps to regeneratelib/firebase_options.dart.docs/release-process.md#L1-L5: replace the placeholder with the actual release steps, relevant given this PR ships version 1.0.0.docs/upstream.md#L1-L5: replace the placeholder with the upstream-tracking process description.🤖 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 `@docs/ci-workflows.md` around lines 1 - 5, Replace the identical placeholder bodies with topic-specific documentation: in docs/ci-workflows.md:1-5 document the CI pipeline from .github/workflows/ci.yml; in docs/database-plan.md:1-5 document the Firestore schema and collections consistent with firestore.rules and firestore.indexes.json; in docs/developer-manual.md:1-5 add onboarding and setup instructions; in docs/external-dependencies.md:1-5 list dependencies and purposes; in docs/regen-flutterfire-config.md:1-5 explain regenerating lib/firebase_options.dart; in docs/release-process.md:1-5 document the release steps, including the 1.0.0 release; and in docs/upstream.md:1-5 describe the upstream-tracking process.
🤖 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 @.github/workflows/ci.yml:
- Around line 21-28: Update the Checkout step to use the immutable
actions/checkout commit 3d3c42e5aac5ba805825da76410c181273ba90b1 and set
persist-credentials to false. Update the Install Flutter step to use the
immutable subosito/flutter-action commit
1a449444c387b1966244ae4d4f8c696479add0b2, preserving its existing Flutter
version and cache settings.
In `@android/app/build.gradle.kts`:
- Around line 54-67: Update the release signing configuration in
buildTypes.release so that missing hasReleaseSigning leaves the release build
unsigned instead of selecting signingConfigs.getByName("debug"). Preserve the
release signingConfigs.getByName("release") path when credentials are available,
and require CI to use the debug variant separately when an installable
unsigned-free artifact is needed.
In `@android/app/src/main/AndroidManifest.xml`:
- Around line 2-7: Add an explicit camera hardware feature declaration alongside
the existing CAMERA permission, marking android.hardware.camera as not required
so Google Play continues listing the app for devices without cameras.
- Around line 8-11: Update the main AndroidManifest.xml by declaring
android.permission.INTERNET directly under the root <manifest> element,
alongside the existing application declaration. Do not rely on debug/profile
manifests or transitive libraries, ensuring release builds have the required
network access.
In `@firebase.json`:
- Around line 26-27: Add a valid reporting endpoint directive, using report-uri
or report-to, to the Content-Security-Policy-Report-Only value and apply the
same change to the corresponding spectrumpit-staging policy entry. Keep the
existing policy directives unchanged.
In `@firestore.rules`:
- Around line 95-100: Update the telemetry create rule in match
/telemetry/{eventId} to require isMember() instead of only isAuthed(), and
additionally require request.resource.data.id to equal eventId. Preserve the
existing validation and deny read, update, and delete behavior.
- Around line 15-17: Add an isValidProfileUpdate helper next to
isValidNewProfile that restricts profile update keys, preserves
resource.data.uid, and validates roles as a 1–4 item list containing only the
allowed role values; also enforce displayName and optional email string length
limits. Update the userProfiles update rule to require
isValidProfileUpdate(request.resource.data) alongside its existing
authentication and admin checks.
In `@ios/Runner/Assets.xcassets/LaunchImage.imageset/README.md`:
- Line 5: Add a single trailing newline to the end of README.md so markdownlint
MD047 passes; do not change the documented content or other Flutter template
files.
In `@lib/main.dart`:
- Around line 163-176: Consolidate the duplicated SyncedMapImageStore
construction by combining the firebaseReady && !_isDesktop and _isDesktop &&
_oauthClientId.isNotEmpty conditions into one condition. Keep the shared
constructor arguments unchanged and retain LocalMapImageStore as the fallback.
In `@lib/src/models/inventory_item.dart`:
- Around line 63-80: Update InventoryItem.copyWith so mapRef distinguishes an
omitted argument from an explicitly provided null, using a sentinel default and
resolving the value accordingly. Preserve the existing fallback to this.mapRef
when mapRef is not provided, while allowing copyWith(mapRef: null) to clear the
field.
In `@lib/src/models/packing_record.dart`:
- Around line 56-69: Update PackingRecord.copyWith to accept an explicit
clearPhotoRef flag and use it to set photoRef to null when requested, while
preserving the current value by default. Refactor _removePhoto to call
copyWith(clearPhotoRef: true, updatedAt: DateTime.now().toUtc()) instead of
constructing PackingRecord directly, preserving all other fields automatically.
In `@lib/src/models/pit_shift.dart`:
- Around line 77-96: Update PitShift.fromJson to parse startsAt and endsAt
through a tolerant date helper instead of casting them directly to String?. Add
the proposed _dateTime helper near _stringList, accepting DateTime and parseable
String values while returning null for other types, then use it for both date
fields so malformed document values do not abort collection loading.
In `@lib/src/models/user_profile.dart`:
- Around line 27-29: Update the legacy role handling in the UserProfile parsing
logic to verify that data['role'] is a String before passing it to
UserRole.fromString; otherwise use the safe fallback behavior without throwing.
Preserve tryParse’s handling of unknown string values.
In `@lib/src/models/user_role.dart`:
- Around line 39-53: Update the visibleTabIndices implementation in UserRole to
replace numeric literals with a shared named tab identifier, preserving the
current viewer, pit/developer, and admin visibility mappings. Define or reuse
the shared tab identifiers for feature tabs, Docs, Users, and Settings so
visibility remains aligned with shell routing when tabs are inserted or
reordered.
In `@lib/src/services/debug_info.dart`:
- Around line 1-5: Prevent dart:io from being resolved during web compilation
across the affected services: debug_info.dart, desktop_launcher_service.dart,
desktop_self_update_service.dart, desktop_auth_service.dart,
map_image_store.dart, and synced_map_image_store.dart. Replace unconditional
platform imports with conditional exports or web-safe implementations, update
the affected settings_tab.dart integration as needed, and verify the web target
compiles while preserving _isDesktopPlatform as the runtime widget guard.
In `@lib/src/services/desktop_auth_service.dart`:
- Line 89: Update the persistence flow around _session.toJson() so the Firebase
refresh token is never written through SharedPreferences. Store the refresh
token using the platform keystore via the project’s secure-storage mechanism,
while persisting only non-secret session/profile fields under _prefsKey; update
the corresponding restore logic to combine both sources and preserve existing
authentication behavior.
- Around line 60-79: Update initialize() so that when _session.restore throws
while restoring the stored session, it removes _prefsKey from the loaded
preferences before swallowing the error. Preserve the existing removal when
restore returns null and keep successful restoration behavior unchanged.
In `@lib/src/services/desktop_inventory_sync_service.dart`:
- Around line 34-43: Update streamAll in
lib/src/services/desktop_inventory_sync_service.dart (lines 34-43) and
lib/src/services/desktop_map_location_sync_service.dart (lines 37-46) so each
catch block logs the caught exception with debugPrint, following the existing
pattern in FirestoreMapLocationSyncService.fetchAll; do not discard
listDocuments failures silently.
In `@lib/src/services/desktop_launcher_service.dart`:
- Around line 48-56: Update desktopEntry so both appImagePath in Exec and
iconPath in Icon are escaped according to Desktop Entry rules: escape
backslashes, quotes, backticks, and dollar signs for Exec, and backslashes plus
newline characters for Icon using the required escape sequences. Preserve the
existing default icon value and output line structure, including encoded newline
content rather than removing it.
In `@lib/src/services/desktop_packing_sync_service.dart`:
- Around line 42-46: Update the catch block in the desktop packing fetch flow to
log Firestore polling failures with debugPrint, matching the behavior of
FirestorePackingSyncService.fetchAll. Preserve the existing
last-successful-items behavior while including the caught error details in the
log.
- Around line 19-22: Update DesktopPackingSyncService.fetchAll to handle
listDocuments failures consistently with streamAll, and sort the mapped
PackingRecord results by id before returning them. Preserve the existing
document-to-record conversion while aligning fetchAll’s error handling and
ordering with streamAll.
In `@lib/src/services/desktop_pit_shift_sync_service.dart`:
- Around line 55-61: Update _fingerprint to derive its value from
PitShift.toJson rather than manually concatenating fields, and add the
dart:convert import to encode that serialized map deterministically. Remove the
hand-written field list so future PitShift fields are automatically included in
change detection.
- Around line 37-52: The desktop polling loop currently swallows failures and
retries at a fixed interval. Update the service containing the polling loop and
the other desktop polling services to accept and invoke an injected error
callback for Firestore/parsing failures, track consecutive failures, and apply a
bounded backoff that resets after a successful poll while preserving normal
fingerprint-based yielding.
In `@lib/src/services/desktop_self_update_service.dart`:
- Around line 45-47: Update _defaultMakeExecutable to retain the Process.run
result and validate its exitCode after running chmod. Propagate a failure when
chmod does not succeed so the update flow stops at the executable-permission
step instead of continuing to relaunch.
- Line 30: Update the download request in the desktop self-update flow around
_client.get to enforce the repository’s standard HTTP timeout, preferably by
wrapping the injected client with HttpTimeoutClient or applying the established
timeout configuration to the request, so stalled release hosts cannot leave the
install action pending.
- Around line 30-40: Update the desktop self-update flow around update to reject
URLs unless their scheme is https, then verify the downloaded bytes against a
caller-supplied trusted checksum before writing, renaming, or executing the
staged AppImage. Extend update’s contract and its call site in settings_tab.dart
so the expected digest comes from the same trusted release metadata as the URL,
and abort on any mismatch.
In `@lib/src/services/desktop_update_service.dart`:
- Around line 65-76: Update lib/src/services/desktop_update_service.dart lines
65-76 in _loadLatestRelease to wrap both _client.get and jsonDecode in
try/catch, returning null for transport or decoding failures; update lines 24-30
to use the repository’s HttpTimeoutClient instead of the bare http.Client
fallback so stalled requests time out.
- Around line 127-144: Update _parseVersion to use pub_semver’s Version.parse
instead of manually parsing components, preserving prerelease metadata for
correct version precedence. Add pub_semver as a direct dependency, catch
FormatException from Version.parse, and return null for invalid input while
retaining the existing nullable behavior.
In `@lib/src/services/map_image_store.dart`:
- Around line 99-102: Update _extensionOf to return only extensions from an
explicit allowed list (such as the supported image formats), and fall back to
.png for missing, malformed, or unsupported values. Ensure the returned
extension cannot contain path separators before pickDiagram interpolates it into
destination paths.
- Around line 90-95: Update _decodeSize to release all native image resources:
dispose the decoded frame.image after capturing its dimensions and dispose the
codec in a finally block, ensuring both cleanup paths run even when dimension
extraction fails. Preserve the returned Size behavior for successful decoding.
In `@lib/src/services/pit_shift_sync_service.dart`:
- Around line 26-42: Update fetchAll so only the server _collection.get() call
is covered by the connectivity fallback; perform _itemsFrom(snapshot) outside
that catch path, or restrict the catch to FirebaseException. Preserve cache
fallback for Firebase fetch failures while allowing parsing errors from
_itemsFrom to propagate without being labeled as server or cache failures.
In `@lib/src/services/spectrum_auth_service.dart`:
- Around line 102-118: Update the service’s initialize() method to await
_googleSignIn.initialize() before signIn() can call authenticate(), supplying
the required platform OAuth configuration. Preserve the existing web popup flow
and ensure native authentication uses the initialized _googleSignIn instance.
In `@lib/src/services/synced_map_image_store.dart`:
- Around line 137-151: Update _decodeSize to dispose both the Codec returned by
ui.instantiateImageCodec and the image from the retrieved frame after extracting
its dimensions, ensuring disposal also occurs when decoding or size extraction
fails.
- Around line 78-101: Update clearDiagram so photoService.delete and
diagramSync.clearKey are each best-effort operations, matching the existing
file.delete handling. Wrap each remote call independently so one failure does
not prevent subsequent remote, cache, and preference cleanup from executing.
In `@lib/src/services/telemetry_service.dart`:
- Around line 31-34: Change the telemetry opt-in flow centered on isEnabled and
logEvent so the absent preference defaults to false, preventing startup events
such as app_open from transmitting identifiers before consent. Add the required
first-launch consent prompt and persist the user’s choice through the existing
preference mechanism, while preserving explicit opt-in behavior.
- Around line 41-47: Update _deviceId to use a shared single-flight Future for
initial ID generation and persistence, so concurrent callers await the same
in-progress operation and receive one device ID. Retain the existing stored
non-empty ID fast path, and clear or reset the shared future if generation or
setString fails so later calls can retry.
In `@lib/src/services/user_role_service.dart`:
- Around line 1-16: Move the UserRoleService abstraction into a dedicated
interface file such as user_role_service_interface.dart, containing only the
abstract class and its model imports without cloud_firestore. Keep
FirestoreUserRoleService in a separate Firestore-specific file, update its
imports to reference the new interface, and update DesktopUserRoleService and
LocalUserRoleService to import the interface file directly.
- Around line 70-81: Update streamAllProfiles to remove the Firestore
orderBy('displayName') query and sort the mapped UserProfile list client-side
using displayName.toLowerCase(), matching DesktopUserRoleService; ensure
profiles without displayName are retained according to the existing model
behavior.
- Around line 63-68: Update UserRoleService.updateRoles to use a merged set
operation for the target userProfiles document, matching
DesktopUserRoleService.updateRoles and creating the document when absent.
Confirm firestore.rules permits admins to create userProfiles; otherwise retain
this update behavior and make the desktop implementation strict instead.
In `@lib/src/state/pit_controller_mixin.dart`:
- Around line 40-59: Update the upsert and delete methods in
StatePitControllerMixin so failed pitUpsertRemote or pitDeleteRemote calls do
not leave optimistic local state and cache changes silently applied: either
track and retry pending mutations or roll back _pitItems and the cache on
failure. Also verify every caller handles the propagated exception and informs
the user.
- Line 91: Serialize all cache writes in _pitSaveCache by chaining each save
operation through a shared future, ensuring concurrent invocations execute
sequentially and the most recently enqueued state is persisted last. Update both
the fire-and-forget call site and the method’s
SharedPreferences/jsonEncode/setString flow without changing the existing cache
format.
In `@lib/src/state/theme_controller.dart`:
- Around line 14-16: Update bootstrap() and the _bootstrap() failure path so a
failed bootstrap clears _bootstrapFuture, allowing a later “Try again” action to
start a fresh load while preserving the cached future for successful
initialization.
In `@lib/src/state/user_role_controller.dart`:
- Around line 48-68: Update _onAuthSnapshot to catch failures from
fetchOrCreateRoles, set _roles to {UserRole.viewer}, store the caught error in a
new _rolesError field exposed through a rolesError getter, and notify listeners
so the UI can report it. Clear _rolesError on successful role loading and
signed-out transitions, while preserving the existing generation check to avoid
applying stale fetch results.
In `@lib/src/theme/app_theme.dart`:
- Line 7: Update the theme configuration around GoogleFonts.ibmPlexMono to
bundle the IBM Plex font assets declared in pubspec.yaml, and disable
GoogleFonts.config.allowRuntimeFetching so font resolution never depends on
network access.
In `@lib/src/theme/pit_palette.dart`:
- Around line 27-37: In the theme palette class containing statusPacking through
lightStatusOverdue, add context-based accessors for each status color that
select the dark token for dark themes and the corresponding light token
otherwise. Update the listed consumers, including _overdueOf and the status
chip/border sites, to use these accessors and remove their duplicated brightness
conditionals.
In `@lib/src/ui/app_shell.dart`:
- Around line 329-354: Update the feature-tab body in the app shell so
IndexedStack builds only the tabs listed in features, using _navIndex as the
navigation-position index and _featureBody to map each visible full tab index to
its widget. Replace the current all-tabs list near _secondaryBody while
preserving each tab’s existing controller and dependency wiring, and verify
widget tests still resolve the expected visible tab.
In `@lib/src/ui/borrow_tab.dart`:
- Around line 605-612: Update _pickEstimatedReturn so the date passed as
initialDate is converted to local time and clamped within the picker’s allowed
range, ensuring overdue _estimatedReturn values are raised to firstDate and
values beyond lastDate are lowered to lastDate. Also update the related
date-picker initialization around _checkedOutAt to pass its local calendar date
rather than the UTC instant.
- Around line 504-510: Update _shortDateTime to convert its DateTime input with
toLocal() before reading month, day, hour, and minute, and apply the same
conversion when seeding TimeOfDay.fromDateTime in the picker flows around the
borrow timestamp handlers. Preserve UTC storage while ensuring displayed labels
and picker initial values use the device’s local time.
In `@lib/src/ui/inventory_tab.dart`:
- Around line 483-499: Replace the timestamp-based fallback ID in _save with
const Uuid().v4() for new InventoryItem records, while preserving existing IDs
for edits. Apply the same UUID generation to timestamp-derived IDs in the
packing, borrow, maps, and schedule save flows, using the existing uuid
dependency.
In `@lib/src/ui/maps_tab.dart`:
- Around line 395-412: Guard the onPanUpdate handler in _buildDiagram against an
empty dest rect before dividing by dest.width or dest.height. Skip the drag
update when either dimension is zero, while preserving the existing position
clamping and onPanEnd behavior for valid destination sizes.
In `@lib/src/ui/packing_tab.dart`:
- Around line 100-113: Ensure uploaded photo keys are reclaimed whenever the
record update is not committed. In lib/src/ui/packing_tab.dart lines 100-113,
delete the key returned by the upload when the widget is unmounted or upsert
throws. In lines 774-804, track every key uploaded by the sheet, discard unused
keys after _save succeeds or when the sheet is dismissed, and include the
original record.photoRef whenever a replacement is saved rather than relying
only on previousWasCaptured; reuse a shared upload commit/discard helper if
practical.
- Around line 344-360: Wrap the tappable _StatusChip widget in a Tooltip, using
an accessible message that includes the current stage and explains that tapping
advances the packing stage. Ensure the Tooltip is correctly closed around the
existing Material/InkWell tree, following the pattern used by _PhotoSlot.
- Around line 196-206: Update the delete flow around controller.delete and
_deleteKey so a failed record deletion does not continue to photo deletion.
Preserve the existing failure reporting through _showFailure, but propagate or
otherwise track the delete failure and invoke _deleteKey only after
widget.controller.delete(record.id) completes successfully.
In `@lib/src/ui/schedule_tab.dart`:
- Around line 789-823: The shift editor’s _canSave validation must reject
inverted ranges: require the parsed match start not to exceed the end, and
require the time start not to follow the end. In test/pit_shift_test.dart lines
156-318, add a PitShift.conflictsWith case covering an inverted range such as
startMatch 20 and endMatch 5, pinning the intended conflict result.
- Around line 73-87: Remove the earlier unfiltered conflictIds construction and
derive conflictIds from the same shownConflicts collection used by
_ConflictPanel. Update the relevant logic around _mineOnly, shownConflicts, and
conflictIds so Mine mode badges only reference conflicts the panel can explain,
while preserving all-conflicts behavior outside Mine mode.
In `@lib/src/ui/settings_tab.dart`:
- Around line 396-400: Update _openDownload to capture the boolean returned by
launchUrl and set the appropriate error status when it is false. Preserve the
existing releaseUrl and external-application launch behavior, and leave the
current flow unchanged when the URL opens successfully.
- Around line 134-136: Update the catch block around IssueReportService.submit
so the exception is logged for diagnostics without exposing it in the snackbar.
Replace the interpolated $e text passed to _showReportSnack with a fixed
user-facing failure message while preserving isError: true.
- Around line 493-504: Update initState to handle failures from
_service.isEnabled(), preventing an unhandled asynchronous error while
preserving the mounted state update on success. In _toggle, retain the previous
enabled value, await _service.setEnabled(value), and restore the previous value
if the write fails so the switch reflects the persisted preference.
In `@lib/src/ui/user_management_screen.dart`:
- Around line 127-143: Update the save flow around the roles-change callback so
`_expanded` is set to false only after a successful
`widget.onRolesChanged!(_pendingRoles)` call. Keep `_expanded` unchanged in the
failure path, while retaining the `finally` cleanup that resets `_saving` and
preserving the existing error SnackBar behavior.
In `@linux/runner/my_application.cc`:
- Around line 45-53: Update both title assignments in the use_header_bar
branches of the window setup to use the display name “Spectrum Pit” instead of
the lowercase package name, covering gtk_header_bar_set_title and
gtk_window_set_title.
In `@macos/Flutter/GeneratedPluginRegistrant.swift`:
- Around line 8-27: Regenerate GeneratedPluginRegistrant.swift after running
flutter pub get so the resolved path_provider_foundation dependency is imported
and PathProviderPlugin is registered in RegisterGeneratedPlugins alongside the
existing plugins. Commit the generated changes without manually altering
unrelated registrations.
In `@macos/Runner/AppDelegate.swift`:
- Line 5: Leave AppDelegate unchanged and update the SwiftLint configuration’s
excluded list to include both macos/Runner and ios/Runner, preventing
required_deinit warnings from generated Flutter runner files.
In `@macos/Runner/Configs/AppInfo.xcconfig`:
- Around line 7-8: Update the macOS application display name from “spectrumpit”
to the product name “Spectrum Pit” by either changing PRODUCT_NAME or setting
CFBundleDisplayName in the existing configuration. If changing PRODUCT_NAME,
update every Runner.xcscheme BuildableName occurrence from spectrumpit.app to
the matching display-name app bundle.
In `@macos/Runner/DebugProfile.entitlements`:
- Around line 4-11: Add the com.apple.security.network.client entitlement with a
true value to both DebugProfile.entitlements and Release.entitlements, alongside
the existing network.server entitlement, while preserving all current sandbox
and JIT settings.
In `@macos/Runner/Info.plist`:
- Around line 4-31: Add the com.apple.security.files.user-selected.read-only
entitlement to both DebugProfile.entitlements and Release.entitlements alongside
the existing App Sandbox settings, enabling PhotoService's macOS file_selector
access without adding camera or photo-library usage keys.
In `@macos/Runner/Release.entitlements`:
- Around line 4-7: Add the required App Sandbox keys to both
Release.entitlements and DebugProfile.entitlements: enable
com.apple.security.network.client for outbound connections and
com.apple.security.files.user-selected.read-only for file_selector access.
Preserve the existing sandbox settings, including DebugProfile’s network.server
entitlement.
In `@pubspec.yaml`:
- Around line 43-46: Update the firestore_client dependency’s git ref from the
mutable v0.1.0 tag to the full commit SHA corresponding to that tag, preserving
the existing repository URL and dependency configuration.
In `@test/borrow_tab_test.dart`:
- Around line 180-189: Update the delete confirmation interaction in the test
around the delete flow to use an unambiguous finder that matches the “Delete”
text specifically on the dialog button, following the button-type matching
approach used by the checkout test. Keep the existing tap and settlement
behavior unchanged.
- Around line 34-46: Expose the sync fakes used by the test helpers so mutation
tests can verify persistence calls. In test/borrow_tab_test.dart lines 34-46,
store FakeBorrowSyncService in a top-level variable and assert sync.upserts in
check-in and checkout tests and sync.deletes in the delete test. Apply the same
pattern in test/packing_tab_test.dart lines 32-44 with FakePackingSyncService,
asserting sync.upserts for add, status-advance, and photo tests and sync.deletes
for the delete test.
- Around line 58-64: Update controller setup in the affected tests, including
_makeController and the corresponding packing-tab tests, to register
controller.dispose with addTearDown immediately after each controller is
created. Remove trailing explicit controller.dispose() calls from test bodies so
cleanup still runs when expectations fail.
In `@test/dark_theme_tokens_test.dart`:
- Around line 58-65: Convert the seven pure theme assertion cases, including
“dark theme wires the Shadow Board tokens” and the tests around the referenced
ranges, from testWidgets to test; remove the unused tester parameter while
preserving their ThemeData construction and assertions. Leave the two
widget-pumping tests unchanged as testWidgets.
- Around line 96-139: Extend the four existing theme test groups—secondary
button colors, minimum button height, type-scale ceiling, and focused input
border—to validate both buildDarkAppTheme() and buildAppTheme(). Use a
parameterized loop or equivalent shared assertions over both theme builders,
preserving the current expected values for each property.
In `@test/desktop_auth_service_test.dart`:
- Around line 43-53: Update the _service test helper to register each created
DesktopAuthService for teardown so its dispose() method is always called. Add a
focused test covering snapshotStream that verifies it emits signingIn followed
by signedIn during the successful sign-in flow, using the existing service setup
and assertions.
In `@test/desktop_launcher_service_test.dart`:
- Line 15: Register cleanup immediately after each createTempSync call in the
affected tests using addTearDown, so the temporary directory is deleted
regardless of assertion failures; remove the corresponding final deleteSync
statements while preserving the existing test behavior.
- Around line 6-12: Add tests for DesktopLauncherService.isSupported covering
empty and non-empty APPIMAGE values, while accounting for Platform.isLinux so
assertions remain host-independent or isolate the appImagePath check through a
testable member. Keep the existing desktopEntry test unchanged and verify that a
non-empty path enables support only on Linux, while an empty path is
unsupported.
In `@test/desktop_pit_shift_sync_service_test.dart`:
- Around line 155-190: Add a test alongside the reassignment test covering an
unchanged poll: use a fixed Firestore response with the existing
DesktopPitShiftSyncService and a short poll interval, collect emissions briefly,
cancel the subscription, and assert exactly one result. This should verify the
fingerprint check suppresses re-emission when the schedule is unchanged.
In `@test/desktop_self_update_service_test.dart`:
- Around line 9-45: Update both existing self-update tests to register
temporary-directory deletion with addTearDown immediately after creating each
directory, removing the direct cleanup calls. Add a non-200 response test for
DesktopSelfUpdateService.update using a sufficiently large payload and status
500, asserting it throws StateError and leaves the target file unchanged.
In `@test/desktop_update_service_test.dart`:
- Around line 63-90: Extend the DesktopUpdateService tests around checkForUpdate
to cover multiple repositories: configure repositories with a primary and
fallback, verify a null result from the primary causes iteration to continue,
and assert the first newer release from the fallback is returned with both
repositories requested.
In `@test/docs_assets_exist_test.dart`:
- Around line 12-25: Update the asset-reference check around the RegExp and
missing-file assertions to match both single- and double-quoted docs/*.md
literals. Then read pubspec.yaml and add an expectation that every referenced
document is covered by the declared docs asset configuration, reporting any
unbundled paths in sorted order while preserving the existing existence check.
In `@test/docs_viewer_test.dart`:
- Around line 63-69: Strengthen the loading assertion in the test around the
DocPage navigation expectations by verifying the error state is absent or,
preferably, asserting that rendered Markdown content is present. Add the
flutter_markdown_plus Markdown import if using a widget-type assertion, while
preserving the existing app-bar and navigation checks.
In `@test/inventory_tab_test.dart`:
- Around line 33-41: Update the test lifecycle around the late controller field:
remove the unconditional tearDown disposal and register controller.dispose with
addTearDown inside pumpTab immediately after controller is assigned, so cleanup
only runs when initialization succeeds.
In `@test/issue_report_service_test.dart`:
- Around line 62-67: Update the assertions in the test around the bugReports
document to compare the complete data.keys set against the expected whitelisted
keys, following the key-set comparison used by test 1, rather than only checking
that roles exists; otherwise revise the comment to match the weaker assertion.
In `@test/local_only_services_test.dart`:
- Around line 26-46: Extend the existing local auth and role wiring test around
UserRoleController to call updateUserRoles as the non-administrator session and
assert that it throws StateError. Keep the current role, visibility, and
canManageUsers assertions unchanged.
In `@test/map_location_controller_test.dart`:
- Around line 209-237: Add emitError to the shared FakeMapLocationSyncService,
preserving its ability to publish stream errors, then remove the local
_FakeSyncService from the test and update references to use the shared fake.
Keep the existing fetch, upsert, delete, and stream behavior unchanged.
In `@test/map_location_test.dart`:
- Around line 34-51: The MapLocation roundtrip test should also assert that
updatedAt survives toJson/fromJson unchanged. Add coverage for
MapLocation.fromJson with both an unparseable timestamp and a missing updatedAt
value, verifying each falls back to the Unix epoch.
In `@test/maps_tab_test.dart`:
- Around line 87-90: Move disposal registration from the shared tearDown block
into pumpTab, immediately after constructing mapController and
inventoryController. Register each controller’s dispose method with addTearDown
so cleanup occurs only when that controller was initialized, and remove their
unconditional disposal from the global tearDown.
- Around line 344-352: Add store-level assertions in the test covering the gated
removal, verifying that the lab map still contains the expected pit diagram
while the pending lab removal is applied after gate.complete(). Keep the
existing UI assertions and assert both state outcomes after the final
pumpAndSettle.
In `@test/packing_controller_test.dart`:
- Around line 138-151: Update the “bootstrap is idempotent” test to observe
notification count rather than only the resulting items list. Attach a listener
to the controller before emitting through sync, count each notification, and
assert one notification for the single emission while preserving the existing
item assertion and cleanup.
In `@test/packing_tab_test.dart`:
- Around line 243-270: Update the affected widget tests, including the test
containing the add-photo flow and the two other platform-override cases, to wrap
all operations after setting debugDefaultTargetPlatformOverride in try/finally.
Move the reset to the finally block so it executes on failures and clears the
override before teardown, while preserving each test’s existing assertions and
cleanup.
In `@test/photo_service_test.dart`:
- Around line 192-228: Register debugDefaultTargetPlatformOverride cleanup with
addTearDown at the start of both tests, before assigning any platform override.
Remove the end-of-test manual resets in the pickImage and
sources-follow-the-platform tests so the override is restored even when an
expectation fails.
In `@test/pit_shift_test.dart`:
- Around line 59-71: Add an assertion in the “defaults for missing fields” test
for PitShift.fromJson that verifies updatedAt falls back to the Unix epoch in
UTC when absent, matching the model’s existing fallback behavior.
- Around line 84-107: Extend the roundtrip test around PitShift.fromJson and
original.toJson to initialize non-null startsAt and endsAt values on original,
then assert restored.startsAt and restored.endsAt equal the originals, covering
timestamp serialization and deserialization.
In `@test/schedule_tab_test.dart`:
- Around line 234-311: Add widget-test coverage in the schedule tab tests for
_openEditor’s onDelete flow: verify cancelling the confirmation leaves
sync.deletes empty, confirming a successful deletion records the shift ID, and a
failed deletion keeps the editor sheet open. Reuse the existing pumpTab setup
and FakePitShiftSyncService delete behavior.
- Around line 48-56: Make the controller field nullable, update its assignment
and both controller references in pumpTab to handle the nullable type, and
change tearDown to dispose it only when initialized. Preserve disposal behavior
while preventing LateInitializationError when pumpTab is not reached.
In `@test/support/fake_map_image_store.dart`:
- Around line 19-38: Update FakeMapImageStore methods diagramFor, pickDiagram,
and clearDiagram to honor isSupported like LocalMapImageStore: return null from
the diagram lookup and pick operations when unsupported, and return immediately
from clearDiagram before applying failure, gate, or removal behavior.
In `@test/support/fake_user_role_service.dart`:
- Around line 35-48: Update the fake service’s streamAllProfiles to use a
broadcast stream controller that emits the current profile list immediately and
re-emits after updateRoles and setRoles mutate _roles or related state. Ensure
the controller remains available to existing subscribers and preserves the
profile mapping used by the current Stream.value implementation.
In `@test/support/photo_test_support.dart`:
- Around line 43-71: Update the MockClient handler in fakePhotoService to
validate the request’s Authorization header against the expected Bearer token
before invoking the respond override. Return an HTTP 401 response for missing or
mismatched credentials, while preserving the existing respond and
method-specific behavior for authenticated requests.
In `@test/synced_map_image_store_test.dart`:
- Around line 163-185: Update FakeMapDiagramSyncService.clearKey to clear the
stored key returned by readKey, while preserving its existing clearCalls
tracking. Ensure readKey reflects the cleared state so the diagramFor assertion
verifies the remote pointer was removed rather than relying on the deleted
object producing a 404.
- Around line 84-89: Update the offline fallback test around SyncedMapImageStore
and diagramFor to capture requests made by the photo service, then assert that
the captured request list is empty alongside the existing non-null fallback
assertion, proving the read used the local cache without network access.
In `@test/telemetry_service_test.dart`:
- Around line 36-58: Add a test case for TelemetryService.logEvent that submits
over-length type and detail values, then reads the stored telemetry document and
asserts type is limited to 64 characters and detail to 128 characters. Use the
existing FakeFirebaseFirestore setup and verify the persisted values rather than
only the input values.
- Around line 7-18: Update the telemetry payload assertion in the test around
_allowedKeys to require exact set equality, ensuring no allowed key is missing
or unexpected rather than checking only that written keys are a subset. Add a
concise comment referencing firestore.rules and isValidTelemetry, explicitly
noting that changes to the rules’ required fields must update this test and its
expected set.
In `@test/user_management_screen_test.dart`:
- Around line 33-36: Dispose each UserRoleController created in both tests
immediately after construction, registering cleanup at the creation site so the
auth snapshot subscription is released when each test completes. Update both
controller instantiations, including the second test’s instance, without
changing the test behavior.
In `@test/user_role_controller_test.dart`:
- Around line 40-90: Replace the hard-coded visibleTabIndices lists in the
UserRoleSetPermissions tests with named tab-index constants or a labelled
accessor that identifies each tab, including the omitted Users tab and Settings
tab. Update the assertions for pit, admin, developer, and multi-role cases to
use those names while preserving their current tab membership and ordering.
- Around line 251-254: Update the tests for the async updateUserRoles method to
await its rejected Future using await expectLater, including both the shown
assertion and the corresponding assertion around lines 271-274. Preserve the
expected StateError matcher.
In `@tool/generate_icons.py`:
- Around line 56-59: Update redraw to open the source image within a with block,
read its full size before calling maker, and close it before saving back to the
same path. Preserve the existing redraw and reporting flow while avoiding the
square-only size assumption by handling both image dimensions.
In `@windows/runner/flutter_window.cpp`:
- Around line 64-68: Move the WM_FONTCHANGE switch handling in the window
message procedure inside the existing flutter_controller_ null guard, so
ReloadSystemFonts is only called when the controller is valid. Preserve the
current early-return behavior and avoid dereferencing the controller after
OnDestroy has cleared it.
In `@windows/runner/Runner.rc`:
- Around line 92-99: Update the Windows VERSIONINFO fields FileDescription and
ProductName from “spectrumpit” to “Spectrum Pit”, while preserving InternalName
and OriginalFilename. Also update macOS PRODUCT_NAME and the Linux GTK window
titles to “Spectrum Pit”, leaving their internal identifiers unchanged.
---
Outside diff comments:
In `@docs/ci-workflows.md`:
- Around line 1-5: Replace the identical placeholder bodies with topic-specific
documentation: in docs/ci-workflows.md:1-5 document the CI pipeline from
.github/workflows/ci.yml; in docs/database-plan.md:1-5 document the Firestore
schema and collections consistent with firestore.rules and
firestore.indexes.json; in docs/developer-manual.md:1-5 add onboarding and setup
instructions; in docs/external-dependencies.md:1-5 list dependencies and
purposes; in docs/regen-flutterfire-config.md:1-5 explain regenerating
lib/firebase_options.dart; in docs/release-process.md:1-5 document the release
steps, including the 1.0.0 release; and in docs/upstream.md:1-5 describe the
upstream-tracking process.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 282acf4f-6b4f-4778-926c-a7ed54b145c5
⛔ Files ignored due to path filters (58)
android/app/src/main/res/drawable-hdpi/ic_launcher_foreground.pngis excluded by!**/*.pngandroid/app/src/main/res/drawable-hdpi/ic_launcher_monochrome.pngis excluded by!**/*.pngandroid/app/src/main/res/drawable-mdpi/ic_launcher_foreground.pngis excluded by!**/*.pngandroid/app/src/main/res/drawable-mdpi/ic_launcher_monochrome.pngis excluded by!**/*.pngandroid/app/src/main/res/drawable-xhdpi/ic_launcher_foreground.pngis excluded by!**/*.pngandroid/app/src/main/res/drawable-xhdpi/ic_launcher_monochrome.pngis excluded by!**/*.pngandroid/app/src/main/res/drawable-xxhdpi/ic_launcher_foreground.pngis excluded by!**/*.pngandroid/app/src/main/res/drawable-xxhdpi/ic_launcher_monochrome.pngis excluded by!**/*.pngandroid/app/src/main/res/drawable-xxxhdpi/ic_launcher_foreground.pngis excluded by!**/*.pngandroid/app/src/main/res/drawable-xxxhdpi/ic_launcher_monochrome.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-hdpi/ic_launcher.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-mdpi/ic_launcher.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xhdpi/ic_launcher.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xxhdpi/ic_launcher.pngis excluded by!**/*.pngandroid/app/src/main/res/mipmap-xxxhdpi/ic_launcher.pngis excluded by!**/*.pngassets/icon/icon.pngis excluded by!**/*.pngassets/icon/icon_foreground.pngis excluded by!**/*.pngios/Runner.xcodeproj/project.xcworkspace/contents.xcworkspacedatais excluded by!**/*.xcworkspace/contents.xcworkspacedataios/Runner.xcworkspace/contents.xcworkspacedatais excluded by!**/*.xcworkspace/contents.xcworkspacedataios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-1024x1024@1x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-20x20@1x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-20x20@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-20x20@3x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-29x29@1x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-29x29@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-29x29@3x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-40x40@1x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-40x40@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-40x40@3x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-50x50@1x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-50x50@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-57x57@1x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-57x57@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-60x60@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-60x60@3x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-72x72@1x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-72x72@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-76x76@1x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-76x76@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/AppIcon.appiconset/Icon-App-83.5x83.5@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/LaunchImage.imageset/LaunchImage.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/LaunchImage.imageset/LaunchImage@2x.pngis excluded by!**/*.pngios/Runner/Assets.xcassets/LaunchImage.imageset/LaunchImage@3x.pngis excluded by!**/*.pngmacos/Runner.xcworkspace/contents.xcworkspacedatais excluded by!**/*.xcworkspace/contents.xcworkspacedatamacos/Runner/Assets.xcassets/AppIcon.appiconset/app_icon_1024.pngis excluded by!**/*.pngmacos/Runner/Assets.xcassets/AppIcon.appiconset/app_icon_128.pngis excluded by!**/*.pngmacos/Runner/Assets.xcassets/AppIcon.appiconset/app_icon_16.pngis excluded by!**/*.pngmacos/Runner/Assets.xcassets/AppIcon.appiconset/app_icon_256.pngis excluded by!**/*.pngmacos/Runner/Assets.xcassets/AppIcon.appiconset/app_icon_32.pngis excluded by!**/*.pngmacos/Runner/Assets.xcassets/AppIcon.appiconset/app_icon_512.pngis excluded by!**/*.pngmacos/Runner/Assets.xcassets/AppIcon.appiconset/app_icon_64.pngis excluded by!**/*.pngpubspec.lockis excluded by!**/*.lockweb/favicon.pngis excluded by!**/*.pngweb/icons/Icon-192.pngis excluded by!**/*.pngweb/icons/Icon-512.pngis excluded by!**/*.pngweb/icons/Icon-maskable-192.pngis excluded by!**/*.pngweb/icons/Icon-maskable-512.pngis excluded by!**/*.pngwindows/runner/resources/app_icon.icois excluded by!**/*.ico
📒 Files selected for processing (214)
.firebaserc.github/CODEOWNERS.github/workflows/ci.yml.gitignore.metadataCONTRIBUTING.mdanalysis_options.yamlandroid/.gitignoreandroid/app/build.gradle.ktsandroid/app/src/debug/AndroidManifest.xmlandroid/app/src/main/AndroidManifest.xmlandroid/app/src/main/kotlin/org/spectrum3847/spectrumpit/MainActivity.ktandroid/app/src/main/res/drawable-v21/launch_background.xmlandroid/app/src/main/res/drawable/launch_background.xmlandroid/app/src/main/res/mipmap-anydpi-v26/ic_launcher.xmlandroid/app/src/main/res/values-night/styles.xmlandroid/app/src/main/res/values/colors.xmlandroid/app/src/main/res/values/styles.xmlandroid/app/src/profile/AndroidManifest.xmlandroid/build.gradle.ktsandroid/gradle.propertiesandroid/gradle/wrapper/gradle-wrapper.propertiesandroid/settings.gradle.ktsdevtools_options.yamldocs/README.mddocs/ci-workflows.mddocs/database-plan.mddocs/developer-manual.mddocs/external-dependencies.mddocs/regen-flutterfire-config.mddocs/release-process.mddocs/upstream.mdfirebase.jsonfirestore.indexes.jsonfirestore.rulesios/.gitignoreios/Flutter/AppFrameworkInfo.plistios/Flutter/Debug.xcconfigios/Flutter/Release.xcconfigios/Runner.xcodeproj/project.pbxprojios/Runner.xcodeproj/project.xcworkspace/xcshareddata/IDEWorkspaceChecks.plistios/Runner.xcodeproj/project.xcworkspace/xcshareddata/WorkspaceSettings.xcsettingsios/Runner.xcodeproj/xcshareddata/xcschemes/Runner.xcschemeios/Runner.xcworkspace/xcshareddata/IDEWorkspaceChecks.plistios/Runner.xcworkspace/xcshareddata/WorkspaceSettings.xcsettingsios/Runner/AppDelegate.swiftios/Runner/Assets.xcassets/AppIcon.appiconset/Contents.jsonios/Runner/Assets.xcassets/LaunchImage.imageset/Contents.jsonios/Runner/Assets.xcassets/LaunchImage.imageset/README.mdios/Runner/Base.lproj/LaunchScreen.storyboardios/Runner/Base.lproj/Main.storyboardios/Runner/Info.plistios/Runner/Runner-Bridging-Header.hios/Runner/SceneDelegate.swiftios/RunnerTests/RunnerTests.swiftlib/firebase_options.dartlib/main.dartlib/src/app.dartlib/src/models/borrow_record.dartlib/src/models/inventory_item.dartlib/src/models/map_location.dartlib/src/models/packing_record.dartlib/src/models/pit_model.dartlib/src/models/pit_shift.dartlib/src/models/user_profile.dartlib/src/models/user_role.dartlib/src/services/borrow_sync_service.dartlib/src/services/debug_info.dartlib/src/services/desktop_auth_service.dartlib/src/services/desktop_borrow_sync_service.dartlib/src/services/desktop_inventory_sync_service.dartlib/src/services/desktop_launcher_service.dartlib/src/services/desktop_map_diagram_sync_service.dartlib/src/services/desktop_map_location_sync_service.dartlib/src/services/desktop_packing_sync_service.dartlib/src/services/desktop_pit_shift_sync_service.dartlib/src/services/desktop_self_update_service.dartlib/src/services/desktop_update_service.dartlib/src/services/desktop_user_role_service.dartlib/src/services/http_timeout_client.dartlib/src/services/inventory_sync_service.dartlib/src/services/issue_report_service.dartlib/src/services/local_only_services.dartlib/src/services/map_diagram_sync_service.dartlib/src/services/map_image_store.dartlib/src/services/map_location_sync_service.dartlib/src/services/packing_sync_service.dartlib/src/services/photo_service.dartlib/src/services/pit_shift_sync_service.dartlib/src/services/spectrum_auth_service.dartlib/src/services/synced_map_image_store.dartlib/src/services/telemetry_service.dartlib/src/services/user_role_service.dartlib/src/state/borrow_controller.dartlib/src/state/inventory_controller.dartlib/src/state/map_location_controller.dartlib/src/state/packing_controller.dartlib/src/state/pit_controller_mixin.dartlib/src/state/pit_shift_controller.dartlib/src/state/theme_controller.dartlib/src/state/user_role_controller.dartlib/src/theme/app_theme.dartlib/src/theme/pit_palette.dartlib/src/ui/app_shell.dartlib/src/ui/borrow_tab.dartlib/src/ui/docs_viewer_screen.dartlib/src/ui/inventory_tab.dartlib/src/ui/location_code.dartlib/src/ui/maps_tab.dartlib/src/ui/packing_photo.dartlib/src/ui/packing_tab.dartlib/src/ui/schedule_tab.dartlib/src/ui/settings_tab.dartlib/src/ui/sign_in_screen.dartlib/src/ui/user_management_screen.dartlinux/.gitignorelinux/CMakeLists.txtlinux/flutter/CMakeLists.txtlinux/flutter/generated_plugin_registrant.cclinux/flutter/generated_plugin_registrant.hlinux/flutter/generated_plugins.cmakelinux/runner/CMakeLists.txtlinux/runner/main.cclinux/runner/my_application.cclinux/runner/my_application.hmacos/.gitignoremacos/Flutter/Flutter-Debug.xcconfigmacos/Flutter/Flutter-Release.xcconfigmacos/Flutter/GeneratedPluginRegistrant.swiftmacos/Runner.xcodeproj/project.pbxprojmacos/Runner.xcodeproj/project.xcworkspace/xcshareddata/IDEWorkspaceChecks.plistmacos/Runner.xcodeproj/xcshareddata/xcschemes/Runner.xcschememacos/Runner.xcworkspace/xcshareddata/IDEWorkspaceChecks.plistmacos/Runner/AppDelegate.swiftmacos/Runner/Assets.xcassets/AppIcon.appiconset/Contents.jsonmacos/Runner/Base.lproj/MainMenu.xibmacos/Runner/Configs/AppInfo.xcconfigmacos/Runner/Configs/Debug.xcconfigmacos/Runner/Configs/Release.xcconfigmacos/Runner/Configs/Warnings.xcconfigmacos/Runner/DebugProfile.entitlementsmacos/Runner/Info.plistmacos/Runner/MainFlutterWindow.swiftmacos/Runner/Release.entitlementsmacos/RunnerTests/RunnerTests.swiftpubspec.yamltest/app_bootstrap_error_test.darttest/borrow_controller_test.darttest/borrow_record_test.darttest/borrow_tab_test.darttest/dark_theme_tokens_test.darttest/debug_info_test.darttest/desktop_auth_service_test.darttest/desktop_launcher_service_test.darttest/desktop_map_location_sync_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_item_test.darttest/inventory_tab_test.darttest/issue_report_service_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_service_test.darttest/pit_shift_controller_test.darttest/pit_shift_test.darttest/schedule_tab_test.darttest/support/fake_borrow_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/widget_test.darttool/generate_icons.pyweb/index.htmlweb/manifest.jsonwindows/.gitignorewindows/CMakeLists.txtwindows/flutter/CMakeLists.txtwindows/flutter/generated_plugin_registrant.ccwindows/flutter/generated_plugin_registrant.hwindows/flutter/generated_plugins.cmakewindows/runner/CMakeLists.txtwindows/runner/Runner.rcwindows/runner/flutter_window.cppwindows/runner/flutter_window.hwindows/runner/main.cppwindows/runner/resource.hwindows/runner/runner.exe.manifestwindows/runner/utils.cppwindows/runner/utils.hwindows/runner/win32_window.cppwindows/runner/win32_window.h
Comment on lines
+21
to
+28
| - name: Checkout | ||
| uses: actions/checkout@v7 | ||
|
|
||
| - name: Install Flutter | ||
| uses: subosito/flutter-action@v2 | ||
| with: | ||
| flutter-version: 3.44.4 | ||
| cache: true |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-remote --refs https://github.com/actions/checkout.git refs/tags/v7
git ls-remote --refs https://github.com/subosito/flutter-action.git refs/tags/v2Repository: Spectrum3847/spectrum-pit
Length of output: 271
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- workflow ---'
cat -n .github/workflows/ci.yml
printf '%s\n' '--- checkout tag resolution ---'
git ls-remote https://github.com/actions/checkout.git \
refs/tags/v7 'refs/tags/v7^{}'
printf '%s\n' '--- flutter-action tag resolution ---'
git ls-remote https://github.com/subosito/flutter-action.git \
refs/tags/v2 'refs/tags/v2^{}'Repository: Spectrum3847/spectrum-pit
Length of output: 1351
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for spec in \
"actions/checkout 3d3c42e5aac5ba805825da76410c181273ba90b1" \
"subosito/flutter-action 1a449444c387b1966244ae4d4f8c696479add0b2"
do
repo="${spec% *}"
sha="${spec#* }"
printf '%s\n' "--- $repo@$sha ---"
if command -v gh >/dev/null 2>&1; then
gh api "repos/$repo/commits/$sha" --jq '{sha: .sha, message: .commit.message}'
else
curl --fail --silent --show-error \
-H 'Accept: application/vnd.github+json' \
"https://api.github.com/repos/$repo/commits/$sha" |
jq '{sha: .sha, message: .commit.message}'
fi
doneRepository: Spectrum3847/spectrum-pit
Length of output: 1075
Pin both actions to immutable commits and disable checkout credentials.
Use actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 and subosito/flutter-action@1a449444c387b1966244ae4d4f8c696479add0b2. Because no later step requires Git authentication, set persist-credentials: false for checkout.
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 21-22: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 22-22: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 25-25: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/ci.yml around lines 21 - 28, Update the Checkout step to
use the immutable actions/checkout commit
3d3c42e5aac5ba805825da76410c181273ba90b1 and set persist-credentials to false.
Update the Install Flutter step to use the immutable subosito/flutter-action
commit 1a449444c387b1966244ae4d4f8c696479add0b2, preserving its existing Flutter
version and cache settings.
Source: Linters/SAST tools
Comment on lines
+54
to
+67
| buildTypes { | ||
| release { | ||
| // Release builds use the keystore declared in android/key.properties | ||
| // (gitignored). When that file is absent (e.g. CI without secrets | ||
| // wired up), fall back to the debug keystore so the build still | ||
| // produces an installable APK -- the artifact just won't match the | ||
| // release SHA-1 registered in Firebase. | ||
| signingConfig = if (hasReleaseSigning) { | ||
| signingConfigs.getByName("release") | ||
| } else { | ||
| signingConfigs.getByName("debug") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not sign release builds with the debug keystore.
The debug keystore uses a publicly known key and password. An APK signed with it can be forged by anyone, and it can never be upgraded to a real release-signed build on the same device. If key.properties is absent, leave the release build unsigned instead. An unsigned artifact fails loudly and cannot be mistaken for a shippable build.
🔒 Proposed change
buildTypes {
release {
// Release builds use the keystore declared in android/key.properties
- // (gitignored). When that file is absent (e.g. CI without secrets
- // wired up), fall back to the debug keystore so the build still
- // produces an installable APK -- the artifact just won't match the
- // release SHA-1 registered in Firebase.
- signingConfig = if (hasReleaseSigning) {
- signingConfigs.getByName("release")
- } else {
- signingConfigs.getByName("debug")
- }
+ // (gitignored). When that file is absent (e.g. CI without secrets
+ // wired up), the build stays unsigned so a debug-signed artifact is
+ // never mistaken for a release build.
+ signingConfig = if (hasReleaseSigning) {
+ signingConfigs.getByName("release")
+ } else {
+ null
+ }
}
}If CI must produce an installable artifact without secrets, build the debug variant in CI instead.
📝 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
| buildTypes { | |
| release { | |
| // Release builds use the keystore declared in android/key.properties | |
| // (gitignored). When that file is absent (e.g. CI without secrets | |
| // wired up), fall back to the debug keystore so the build still | |
| // produces an installable APK -- the artifact just won't match the | |
| // release SHA-1 registered in Firebase. | |
| signingConfig = if (hasReleaseSigning) { | |
| signingConfigs.getByName("release") | |
| } else { | |
| signingConfigs.getByName("debug") | |
| } | |
| } | |
| } | |
| buildTypes { | |
| release { | |
| // Release builds use the keystore declared in android/key.properties | |
| // (gitignored). When that file is absent (e.g. CI without secrets | |
| // wired up), the build stays unsigned so a debug-signed artifact is | |
| // never mistaken for a release build. | |
| signingConfig = if (hasReleaseSigning) { | |
| signingConfigs.getByName("release") | |
| } else { | |
| 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 `@android/app/build.gradle.kts` around lines 54 - 67, Update the release
signing configuration in buildTypes.release so that missing hasReleaseSigning
leaves the release build unsigned instead of selecting
signingConfigs.getByName("debug"). Preserve the release
signingConfigs.getByName("release") path when credentials are available, and
require CI to use the debug variant separately when an installable unsigned-free
artifact is needed.
Comment on lines
+2
to
+7
| <!-- Packing photos (#63). image_picker needs no manifest entries of its | ||
| own, but when CAMERA is declared it asks for it at runtime before | ||
| launching the capture intent, so the declaration and the runtime | ||
| prompt have to stay together. Reading the photo library needs no | ||
| permission on the API levels this app supports. --> | ||
| <uses-permission android:name="android.permission.CAMERA" /> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Declare the camera hardware feature as optional.
A CAMERA permission declaration makes Google Play imply <uses-feature android:name="android.hardware.camera" android:required="true" />. Play then hides the app from devices without a camera, including many Chromebooks and emulator targets. Add an explicit optional feature declaration.
🔧 Proposed change
<uses-permission android:name="android.permission.CAMERA" />
+ <!-- The camera is optional: image_picker can also read from the gallery. -->
+ <uses-feature android:name="android.hardware.camera" android:required="false" />
+ <uses-feature android:name="android.hardware.camera.autofocus" android:required="false" />📝 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
| <!-- Packing photos (#63). image_picker needs no manifest entries of its | |
| own, but when CAMERA is declared it asks for it at runtime before | |
| launching the capture intent, so the declaration and the runtime | |
| prompt have to stay together. Reading the photo library needs no | |
| permission on the API levels this app supports. --> | |
| <uses-permission android:name="android.permission.CAMERA" /> | |
| <!-- Packing photos (`#63`). image_picker needs no manifest entries of its | |
| own, but when CAMERA is declared it asks for it at runtime before | |
| launching the capture intent, so the declaration and the runtime | |
| prompt have to stay together. Reading the photo library needs no | |
| permission on the API levels this app supports. --> | |
| <uses-permission android:name="android.permission.CAMERA" /> | |
| <!-- The camera is optional: image_picker can also read from the gallery. --> | |
| <uses-feature android:name="android.hardware.camera" android:required="false" /> | |
| <uses-feature android:name="android.hardware.camera.autofocus" android:required="false" /> |
🤖 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 `@android/app/src/main/AndroidManifest.xml` around lines 2 - 7, Add an explicit
camera hardware feature declaration alongside the existing CAMERA permission,
marking android.hardware.camera as not required so Google Play continues listing
the app for devices without cameras.
Comment on lines
+8
to
+11
| <application | ||
| android:label="Spectrum Pit" | ||
| android:name="${applicationName}" | ||
| android:icon="@mipmap/ic_launcher"> |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find every AndroidManifest.xml in the repo and show INTERNET declarations.
fd -H 'AndroidManifest.xml' --exec sh -c 'echo "== $1"; rg -n "uses-permission|INTERNET" "$1" || echo " (no uses-permission)"' _ {}Repository: Spectrum3847/spectrum-pit
Length of output: 664
🏁 Script executed:
#!/bin/bash
printf '%s\n' '== manifests'
fd -H 'AndroidManifest.xml' --exec sh -c 'echo "== $1"; cat -n "$1"' _ {}
printf '%s\n' '== Android build configuration'
fd -H -e gradle -e gradle.kts -e properties -e xml android | sort | while read -r f; do
case "$f" in
*build.gradle|*build.gradle.kts|*gradle.properties|*settings.gradle|*settings.gradle.kts)
echo "== $f"; cat -n "$f" ;;
esac
done
printf '%s\n' '== INTERNET and networking references'
rg -n -i 'INTERNET|firebase|http|https|networkSecurityConfig' android pubspec.yaml pubspec.lock 2>/dev/null || trueRepository: Spectrum3847/spectrum-pit
Length of output: 12982
🏁 Script executed:
#!/bin/bash
set -eu
echo '== Android files'
git ls-files 'android/**' | sed -n '1,160p'
echo '== Manifest declarations'
for f in $(fd -H 'AndroidManifest.xml' android); do
echo "== $f"
cat -n "$f"
done
echo '== Gradle files'
for f in $(git ls-files 'android/**' | grep -E '(^|/)(build.gradle|build.gradle.kts|settings.gradle|settings.gradle.kts|gradle.properties)$'); do
echo "== $f"
cat -n "$f"
done
echo '== Relevant dependency and network references'
rg -n -i 'INTERNET|firebase|http|https|network' android pubspec.yaml pubspec.lock 2>/dev/null || trueRepository: Spectrum3847/spectrum-pit
Length of output: 19920
🏁 Script executed:
#!/bin/bash
set -eu
echo '== application networking call sites'
rg -n -i 'Firebase|FirebaseAuth|http\.|Client\(|Uri\.|https?://' --glob '!pubspec.lock' --glob '!*.md' --glob '!android/**' . || true
echo '== Android source-set manifest declarations'
python3 - <<'PY'
from pathlib import Path
for p in sorted(Path("android/app/src").rglob("AndroidManifest.xml")):
text = p.read_text()
print(f"{p}: INTERNET={'android.permission.INTERNET' in text}, CAMERA={'android.permission.CAMERA' in text}")
PY
echo '== tracked release-specific Android inputs'
git ls-files 'android/app/src/release/**' 'android/app/src/main/**' 'android/app/build.gradle.kts'Repository: Spectrum3847/spectrum-pit
Length of output: 30760
🌐 Web query:
FlutterFire firebase_core AndroidManifest.xml android.permission.INTERNET firebase_auth plugin manifest
💡 Result:
To use Firebase services in your Flutter application on Android, you must explicitly declare the internet permission in your Android manifest [1][2]. While some Android permissions are automatically included in debug and profile builds to facilitate features like hot reload, they are not automatically included for release builds [3]. Because Firebase services (such as Firebase Auth, Firestore, and others) require network access to communicate with Firebase servers, your application will fail to function in production if this permission is missing [4][5][6]. To fix or prevent network-related errors in your Flutter app, add the following line to your android/app/src/main/AndroidManifest.xml file, placed within the tag but outside the tag [1][6][2]: Key points regarding this requirement: 1. Consistency: Even if you are using multiple Firebase plugins (e.g., firebase_core and firebase_auth), this single permission declaration covers network access for all of them [7][8]. 2. Debug vs. Release: It is common for developers to overlook this because network access may appear to function during development (debug mode) due to automatic configuration, only to have the app fail when building for release [4][6]. 3. Additional Network Settings: If you are using the Firebase Emulator Suite for local development, you may additionally need to allow cleartext traffic in your debug manifest (android/app/src/debug/AndroidManifest.xml) because emulators may use unencrypted connections [9]. This is typically done by adding android:usesCleartextTraffic="true" to the element in the debug manifest [9][10]. Always ensure your Android manifest is configured correctly for the production environment to avoid network-request-failed or similar connectivity exceptions [10][6].
Citations:
- 1: https://docs.flutter.dev/data-and-backend/networking
- 2: https://stackoverflow.com/questions/72254836/no-internet-connection-android-firebase-flutter-rtd
- 3: android.permission.INTERNET permission flutter/flutter#31968
- 4: https://stackoverflow.com/questions/75519639/firebase-auth-email-password-signin-not-working-in-release-mode-with-flutter
- 5: https://stackoverflow.com/questions/67501843/flutter-google-and-firebase-login-not-working-in-production
- 6: Firebase database as a back-end server for flutter app, the app is working in debug mode for android and IOS. but it is not working in release mode for android only. firebase/flutterfire#6941
- 7: https://firebase.google.com/docs/flutter/setup
- 8: https://firebase.flutter.dev/docs/overview/
- 9: https://firebase.flutter.dev/docs/manual-installation/android/
- 10: 🐛 [firebase_auth] [firebase_auth/network-request-failed] A network error firebase/flutterfire#7966
Declare android.permission.INTERNET in the main manifest.
Only debug and profile manifests declare it. Add the permission under <manifest> so Android release builds do not depend on a transitive library manifest for Firebase and HTTP access.
🤖 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 `@android/app/src/main/AndroidManifest.xml` around lines 8 - 11, Update the
main AndroidManifest.xml by declaring android.permission.INTERNET directly under
the root <manifest> element, alongside the existing application declaration. Do
not rely on debug/profile manifests or transitive libraries, ensuring release
builds have the required network access.
Comment on lines
+26
to
+27
| { "key": "Content-Security-Policy", "value": "frame-ancestors 'self'" }, | ||
| { "key": "Content-Security-Policy-Report-Only", "value": "default-src 'self'; script-src 'self' 'wasm-unsafe-eval' 'unsafe-inline' https://apis.google.com https://www.gstatic.com; style-src 'self' 'unsafe-inline' https://fonts.googleapis.com; font-src 'self' data: https://fonts.gstatic.com; img-src 'self' data: blob: https://www.thebluealliance.com https://*.googleusercontent.com https://*.gstatic.com; connect-src 'self' https://*.googleapis.com https://api.statbotics.io https://www.thebluealliance.com wss://*.firebaseio.com; frame-src 'self' https://*.firebaseapp.com https://accounts.google.com https://apis.google.com; frame-ancestors 'self'; base-uri 'self'; object-src 'none'" } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Add a reporting endpoint to the Report-Only policy.
Content-Security-Policy-Report-Only without report-uri or report-to sends no reports. Violations appear only in the browser console of each user. You cannot collect data to promote this policy to enforcement. The same gap exists at Line 56 for the spectrumpit-staging site.
Add a reporting endpoint, or accept that the policy is console-only.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@firebase.json` around lines 26 - 27, Add a valid reporting endpoint
directive, using report-uri or report-to, to the
Content-Security-Policy-Report-Only value and apply the same change to the
corresponding spectrumpit-staging policy entry. Keep the existing policy
directives unchanged.
Comment on lines
+19
to
+38
| @override | ||
| bool isSupported = true; | ||
|
|
||
| @override | ||
| Future<MapDiagram?> diagramFor(MapType mapType) async => images[mapType]; | ||
|
|
||
| @override | ||
| Future<MapDiagram?> pickDiagram(MapType mapType) async { | ||
| final diagram = nextPick; | ||
| if (diagram != null) images[mapType] = diagram; | ||
| return diagram; | ||
| } | ||
|
|
||
| @override | ||
| Future<void> clearDiagram(MapType mapType) async { | ||
| if (clearFailure != null) throw clearFailure!; | ||
| final gate = clearGate; | ||
| if (gate != null) await gate; | ||
| images.remove(mapType); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
The fake ignores isSupported, so it diverges from LocalMapImageStore.
LocalMapImageStore returns null from diagramFor and pickDiagram, and returns early from clearDiagram, when isSupported is false (lib/src/services/map_image_store.dart Lines 48, 60, 80). This fake honors isSupported in no method. A widget test that sets isSupported = false and then drives a "choose diagram" tap still receives a diagram. The test then passes against behavior that production code does not produce.
Mirror the guard so the fake matches the contract it stands in for.
♻️ Proposed fix
`@override`
- Future<MapDiagram?> diagramFor(MapType mapType) async => images[mapType];
+ Future<MapDiagram?> diagramFor(MapType mapType) async =>
+ isSupported ? images[mapType] : null;
`@override`
Future<MapDiagram?> pickDiagram(MapType mapType) async {
+ if (!isSupported) return null;
final diagram = nextPick;
if (diagram != null) images[mapType] = diagram;
return diagram;
}
`@override`
Future<void> clearDiagram(MapType mapType) async {
+ if (!isSupported) return;
if (clearFailure != null) throw clearFailure!;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @override | |
| bool isSupported = true; | |
| @override | |
| Future<MapDiagram?> diagramFor(MapType mapType) async => images[mapType]; | |
| @override | |
| Future<MapDiagram?> pickDiagram(MapType mapType) async { | |
| final diagram = nextPick; | |
| if (diagram != null) images[mapType] = diagram; | |
| return diagram; | |
| } | |
| @override | |
| Future<void> clearDiagram(MapType mapType) async { | |
| if (clearFailure != null) throw clearFailure!; | |
| final gate = clearGate; | |
| if (gate != null) await gate; | |
| images.remove(mapType); | |
| } | |
| `@override` | |
| bool isSupported = true; | |
| `@override` | |
| Future<MapDiagram?> diagramFor(MapType mapType) async => | |
| isSupported ? images[mapType] : null; | |
| `@override` | |
| Future<MapDiagram?> pickDiagram(MapType mapType) async { | |
| if (!isSupported) return null; | |
| final diagram = nextPick; | |
| if (diagram != null) images[mapType] = diagram; | |
| return diagram; | |
| } | |
| `@override` | |
| Future<void> clearDiagram(MapType mapType) async { | |
| if (!isSupported) return; | |
| if (clearFailure != null) throw clearFailure!; | |
| final gate = clearGate; | |
| if (gate != null) await gate; | |
| images.remove(mapType); | |
| } |
🤖 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/support/fake_map_image_store.dart` around lines 19 - 38, Update
FakeMapImageStore methods diagramFor, pickDiagram, and clearDiagram to honor
isSupported like LocalMapImageStore: return null from the diagram lookup and
pick operations when unsupported, and return immediately from clearDiagram
before applying failure, gate, or removal behavior.
Comment on lines
+43
to
+71
| httpClient: MockClient((request) async { | ||
| requests?.add(request); | ||
| if (respond != null) return respond(request); | ||
| final key = request.url.pathSegments.length > 1 | ||
| ? request.url.pathSegments.last | ||
| : null; | ||
| switch (request.method) { | ||
| case 'POST': | ||
| final generated = 'key-${next++}.jpg'; | ||
| bucket[generated] = request.bodyBytes; | ||
| return http.Response( | ||
| jsonEncode({'key': generated}), | ||
| 201, | ||
| headers: {'content-type': 'application/json'}, | ||
| ); | ||
| case 'GET': | ||
| final bytes = bucket[key]; | ||
| if (bytes == null) return http.Response('{"error":"Not found"}', 404); | ||
| return http.Response.bytes( | ||
| bytes, | ||
| 200, | ||
| headers: {'content-type': 'image/jpeg'}, | ||
| ); | ||
| case 'DELETE': | ||
| return http.Response('', bucket.remove(key) == null ? 404 : 204); | ||
| default: | ||
| return http.Response('', 405); | ||
| } | ||
| }), |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect how PhotoService attaches the ID token to outbound requests.
fd -t f 'photo_service.dart' -x rg -n -C3 'Authorization|Bearer|idToken|headers' {}Repository: Spectrum3847/spectrum-pit
Length of output: 1417
🏁 Script executed:
#!/bin/bash
set -eu
support="$(fd -t f 'photo_test_support.dart' | head -n1)"
printf '%s\n' "== $support =="
cat -n "$support" | sed -n '1,95p'
printf '%s\n' '== fakePhotoService usages =='
rg -n -C2 'fakePhotoService|unavailablePhotoService' --glob '*.dart' .Repository: Spectrum3847/spectrum-pit
Length of output: 14295
Enforce authentication in fakePhotoService.
PhotoService sends Authorization: Bearer $token, but this handler never checks it. A missing-header regression can therefore pass. Check the header before the respond override and return 401 when it does not match.
🤖 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/support/photo_test_support.dart` around lines 43 - 71, Update the
MockClient handler in fakePhotoService to validate the request’s Authorization
header against the expected Bearer token before invoking the respond override.
Return an HTTP 401 response for missing or mismatched credentials, while
preserving the existing respond and method-specific behavior for authenticated
requests.
Comment on lines
+84
to
+89
| final offlineStore = SyncedMapImageStore( | ||
| photoService: photoServiceReturningPng(), | ||
| diagramSync: offlineSync, | ||
| ); | ||
| final fallback = await offlineStore.diagramFor(MapType.lab); | ||
| expect(fallback, isNotNull, reason: 'offline read should use the cache'); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Assert that the fallback read reached no network.
The test title states that the read falls back to the local cache. expect(fallback, isNotNull) does not distinguish a cache hit from a successful remote fetch. The distinction currently holds only because photoServiceReturningPng() returns a fresh empty bucket, which is indirect.
Capture the requests and assert the list is empty.
♻️ Proposed fix
+ final offlineRequests = <http.BaseRequest>[];
final offlineStore = SyncedMapImageStore(
- photoService: photoServiceReturningPng(),
+ photoService: fakePhotoService(
+ requests: offlineRequests,
+ picker: (_) async =>
+ PickedPhoto(bytes: _png, contentType: 'image/png'),
+ ),
diagramSync: offlineSync,
);
final fallback = await offlineStore.diagramFor(MapType.lab);
expect(fallback, isNotNull, reason: 'offline read should use the cache');
+ expect(
+ offlineRequests,
+ isEmpty,
+ reason: 'the cached file must satisfy the read with no fetch',
+ );📝 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
| final offlineStore = SyncedMapImageStore( | |
| photoService: photoServiceReturningPng(), | |
| diagramSync: offlineSync, | |
| ); | |
| final fallback = await offlineStore.diagramFor(MapType.lab); | |
| expect(fallback, isNotNull, reason: 'offline read should use the cache'); | |
| final offlineRequests = <http.BaseRequest>[]; | |
| final offlineStore = SyncedMapImageStore( | |
| photoService: fakePhotoService( | |
| requests: offlineRequests, | |
| picker: (_) async => | |
| PickedPhoto(bytes: _png, contentType: 'image/png'), | |
| ), | |
| diagramSync: offlineSync, | |
| ); | |
| final fallback = await offlineStore.diagramFor(MapType.lab); | |
| expect(fallback, isNotNull, reason: 'offline read should use the cache'); | |
| expect( | |
| offlineRequests, | |
| isEmpty, | |
| reason: 'the cached file must satisfy the read with no fetch', | |
| ); |
🤖 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/synced_map_image_store_test.dart` around lines 84 - 89, Update the
offline fallback test around SyncedMapImageStore and diagramFor to capture
requests made by the photo service, then assert that the captured request list
is empty alongside the existing non-null fallback assertion, proving the read
used the local cache without network access.
Comment on lines
+163
to
+185
| test('deletes the R2 object, pointer, and local cache', () async { | ||
| final requests = <http.BaseRequest>[]; | ||
| final sync = FakeMapDiagramSyncService(readKeyValue: 'key-0.jpg'); | ||
| final store = SyncedMapImageStore( | ||
| photoService: fakePhotoService( | ||
| requests: requests, | ||
| picker: (_) async => | ||
| PickedPhoto(bytes: _png, contentType: 'image/png'), | ||
| ), | ||
| diagramSync: sync, | ||
| ); | ||
| await store.pickDiagram(MapType.lab); | ||
| expect(_appSupport.listSync(), isNotEmpty); | ||
|
|
||
| await store.clearDiagram(MapType.lab); | ||
|
|
||
| expect(requests.where((r) => r.method == 'DELETE'), hasLength(1)); | ||
| expect(sync.clearCalls, [MapType.lab]); | ||
| final prefs = await SharedPreferences.getInstance(); | ||
| expect(prefs.getString('$_r2KeyPref${MapType.lab.name}'), isNull); | ||
| expect(_appSupport.listSync(), isEmpty); | ||
| expect(await store.diagramFor(MapType.lab), isNull); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Line 184 can pass for the wrong reason.
FakeMapDiagramSyncService.readKey returns _readKeyValue unconditionally and does not react to clearKey (test/support/fake_map_diagram_sync_service.dart Lines 40-43). After clearDiagram runs, readKey still returns 'key-0.jpg'. The diagramFor call at Line 184 therefore still sees a remote pointer and attempts a fetch. That fetch returns 404 only because the DELETE removed the object from the fake bucket.
The assertion holds, but it does not prove that the remote pointer was cleared. If a future change stopped calling clearKey, Line 184 would still pass.
Make the fake clear its stored key so the assertion tests the pointer.
♻️ Proposed fix in `test/support/fake_map_diagram_sync_service.dart`
- final String? _readKeyValue;
+ String? _readKeyValue;
`@override`
Future<void> clearKey(MapType mapType) async {
clearCalls.add(mapType);
if (_clearFailure != null) throw _clearFailure;
+ _readKeyValue = null;
}The existing expect(sync.clearCalls, [MapType.lab]) at Line 180 already covers the call itself, so this change strengthens Line 184 rather than duplicating Line 180.
🤖 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/synced_map_image_store_test.dart` around lines 163 - 185, Update
FakeMapDiagramSyncService.clearKey to clear the stored key returned by readKey,
while preserving its existing clearCalls tracking. Ensure readKey reflects the
cleared state so the diagramFor assertion verifies the remote pointer was
removed rather than relying on the deleted object producing a 404.
Comment on lines
+64
to
+68
| switch (message) { | ||
| case WM_FONTCHANGE: | ||
| flutter_controller_->engine()->ReloadSystemFonts(); | ||
| break; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the WM_FONTCHANGE handler against a null controller.
Line 55 checks flutter_controller_, but that check only guards the early return. When flutter_controller_ is null, control reaches the switch and Line 66 dereferences a null unique_ptr. OnDestroy sets flutter_controller_ = nullptr while the window can still receive broadcast messages, and WM_FONTCHANGE is broadcast to all top-level windows. Move the handler inside the existing null check.
🛡️ Proposed fix
switch (message) {
case WM_FONTCHANGE:
- flutter_controller_->engine()->ReloadSystemFonts();
+ if (flutter_controller_) {
+ flutter_controller_->engine()->ReloadSystemFonts();
+ }
break;
}Note: this pattern comes from the upstream Flutter Windows runner template. Confirm whether you want to diverge from the template before applying the fix.
📝 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
| switch (message) { | |
| case WM_FONTCHANGE: | |
| flutter_controller_->engine()->ReloadSystemFonts(); | |
| break; | |
| } | |
| switch (message) { | |
| case WM_FONTCHANGE: | |
| if (flutter_controller_) { | |
| flutter_controller_->engine()->ReloadSystemFonts(); | |
| } | |
| break; | |
| } |
🤖 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 `@windows/runner/flutter_window.cpp` around lines 64 - 68, Move the
WM_FONTCHANGE switch handling in the window message procedure inside the
existing flutter_controller_ null guard, so ReloadSystemFonts is only called
when the controller is valid. Preserve the current early-return behavior and
avoid dereferencing the controller after OnDestroy has cleared it.
Comment on lines
+77
to
+96
| factory PitShift.fromJson(String id, Map<String, dynamic> data) { | ||
| final startsAtRaw = data['startsAt'] as String?; | ||
| final endsAtRaw = data['endsAt'] as String?; | ||
| return PitShift( | ||
| id: id, | ||
| label: data['label'] as String? ?? '', | ||
| kind: ShiftKind.fromString(data['kind'] as String?), | ||
| competition: data['competition'] as String? ?? '', | ||
| assignedUids: _stringList(data['assignedUids']), | ||
| assignedNames: _stringList(data['assignedNames']), | ||
| startMatch: (data['startMatch'] as num?)?.toInt(), | ||
| endMatch: (data['endMatch'] as num?)?.toInt(), | ||
| startsAt: startsAtRaw == null ? null : DateTime.tryParse(startsAtRaw), | ||
| endsAt: endsAtRaw == null ? null : DateTime.tryParse(endsAtRaw), | ||
| notes: data['notes'] as String?, | ||
| updatedAt: | ||
| DateTime.tryParse(data['updatedAt'] as String? ?? '') ?? | ||
| DateTime.fromMillisecondsSinceEpoch(0, isUtc: true), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use a tolerant cast for the date fields.
Lines 78-79 cast startsAt and endsAt with as String?. The numeric fields at lines 87-88 use the tolerant as num? form. If a document stores either date as a non-string value, for example a Firestore timestamp written from the console or a Cloud Function, the cast throws. fetchAll and streamAll in lib/src/services/desktop_pit_shift_sync_service.dart map over all documents, so one bad field fails the whole list load.
🛡️ Proposed defensive parse
- final startsAtRaw = data['startsAt'] as String?;
- final endsAtRaw = data['endsAt'] as String?;
+ final startsAtRaw = data['startsAt'];
+ final endsAtRaw = data['endsAt'];
@@
- startsAt: startsAtRaw == null ? null : DateTime.tryParse(startsAtRaw),
- endsAt: endsAtRaw == null ? null : DateTime.tryParse(endsAtRaw),
+ startsAt: _dateTime(startsAtRaw),
+ endsAt: _dateTime(endsAtRaw),
@@
- updatedAt:
- DateTime.tryParse(data['updatedAt'] as String? ?? '') ??
- DateTime.fromMillisecondsSinceEpoch(0, isUtc: true),
+ updatedAt:
+ _dateTime(data['updatedAt']) ??
+ DateTime.fromMillisecondsSinceEpoch(0, isUtc: true),Add the helper next to _stringList:
static DateTime? _dateTime(Object? value) => switch (value) {
DateTime v => v,
String v => DateTime.tryParse(v),
_ => 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 `@lib/src/models/pit_shift.dart` around lines 77 - 96, Update PitShift.fromJson
to parse startsAt and endsAt through a tolerant date helper instead of casting
them directly to String?. Add the proposed _dateTime helper near _stringList,
accepting DateTime and parseable String values while returning null for other
types, then use it for both date fields so malformed document values do not
abort collection loading.
Comment on lines
+27
to
+29
| } else { | ||
| roles = {UserRole.fromString(data['role'] as String?)}; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Guard the legacy role cast against non-string values.
Line 28 casts data['role'] to String?. If a stored document holds a non-string role value, the cast throws and the whole profile parse fails. tryParse already handles unknown strings, so a type check keeps the fallback safe.
🛡️ Proposed fix
} else {
- roles = {UserRole.fromString(data['role'] as String?)};
+ final legacyRole = data['role'];
+ roles = {
+ UserRole.fromString(legacyRole is String ? legacyRole : 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
| } else { | |
| roles = {UserRole.fromString(data['role'] as String?)}; | |
| } | |
| } else { | |
| final legacyRole = data['role']; | |
| roles = { | |
| UserRole.fromString(legacyRole is String ? legacyRole : 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 `@lib/src/models/user_profile.dart` around lines 27 - 29, Update the legacy
role handling in the UserProfile parsing logic to verify that data['role'] is a
String before passing it to UserRole.fromString; otherwise use the safe fallback
behavior without throwing. Preserve tryParse’s handling of unknown string
values.
Comment on lines
+39
to
+53
| List<int> get visibleTabIndices { | ||
| final tabs = <int>{}; | ||
| for (final role in this) { | ||
| switch (role) { | ||
| case UserRole.viewer: | ||
| break; | ||
| case UserRole.pit: | ||
| case UserRole.developer: | ||
| tabs.addAll(const [0, 1, 2, 3, 4, 5, 7]); | ||
| case UserRole.admin: | ||
| tabs.addAll(const [0, 1, 2, 3, 4, 5, 6, 7]); | ||
| } | ||
| } | ||
| return tabs.toList()..sort(); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the tab list and any index-based visibility use.
fd -t f 'app_shell.dart' | xargs -r rg -n -C4 'Tab|visibleTabIndices|index'
rg -n -C3 'visibleTabIndices' --glob '*.dart'Repository: Spectrum3847/spectrum-pit
Length of output: 5113
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- user_role.dart ---'
cat -n lib/src/models/user_role.dart | sed -n '1,90p'
printf '%s\n' '--- app_shell.dart metadata and tab construction ---'
cat -n lib/src/app_shell.dart | sed -n '55,105p'
cat -n lib/src/app_shell.dart | sed -n '320,370p'
printf '%s\n' '--- role visibility tests and all relevant references ---'
rg -n -C4 'visibleTabIndices|UserRole\.(viewer|pit|developer|admin)|_kFirstSecondaryTab|_kTabMeta|_secondaryBody' --glob '*.dart' .Repository: Spectrum3847/spectrum-pit
Length of output: 46599
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- app_shell.dart metadata ---'
cat -n lib/src/ui/app_shell.dart | sed -n '60,105p'
printf '%s\n' '--- app_shell.dart tab list and index dispatch ---'
cat -n lib/src/ui/app_shell.dart | sed -n '195,215p'
cat -n lib/src/ui/app_shell.dart | sed -n '320,365p'
printf '%s\n' '--- read-only index/order consistency probe ---'
python3 - <<'PY'
from pathlib import Path
import re
shell = Path("lib/src/ui/app_shell.dart").read_text()
role = Path("lib/src/models/user_role.dart").read_text()
meta = re.search(r"const _kTabMeta = \[(.*?)\n\];", shell, re.S).group(1)
labels = re.findall(r"label:\s*'([^']+)'", meta)
tabs = re.search(r"final tabs = <Widget>\[(.*?)\n \];", shell, re.S).group(1)
constructors = re.findall(r"^\s*([A-Z]\w+Tab)\b", tabs, re.M)
pit = re.search(r"UserRole\.pit:\s*.*?tabs\.addAll\(const \[([^\]]+)\]", role, re.S).group(1)
admin = re.search(r"UserRole\.admin:\s*.*?tabs\.addAll\(const \[([^\]]+)\]", role, re.S).group(1)
print("metadata_count =", len(labels))
print("metadata =", list(enumerate(labels)))
print("shell_tab_count =", len(constructors))
print("shell_tabs =", list(enumerate(constructors)))
print("pit_indices =", [int(x.strip()) for x in pit.split(",")])
print("admin_indices =", [int(x.strip()) for x in admin.split(",")])
print("metadata_and_shell_counts_match =", len(labels) == len(constructors))
PYRepository: Spectrum3847/spectrum-pit
Length of output: 4200
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
import re
shell = Path("lib/src/ui/app_shell.dart").read_text()
role = Path("lib/src/models/user_role.dart").read_text()
meta = re.search(r"const _kTabMeta = \[(.*?)\n\];", shell, re.S).group(1)
labels = re.findall(r"label:\s*'([^']+)'", meta)
tabs = re.search(r"final tabs = <Widget>\[(.*?)\n \];", shell, re.S).group(1)
constructors = re.findall(r"^\s*([A-Z]\w+Tab)\b", tabs, re.M)
def indices_for(case):
match = re.search(
rf"case UserRole\.{case}:(.*?)(?=\n\s*case UserRole\.|\n\s*\}})",
role,
re.S,
)
return [int(x) for x in re.search(r"\[([^\]]+)\]", match.group(1)).group(1).split(",")]
print("tab_metadata =", list(enumerate(labels)))
print("feature_tabs =", list(enumerate(constructors)))
print("pit_indices =", indices_for("pit"))
print("developer_indices =", indices_for("developer"))
print("admin_indices =", indices_for("admin"))
print("secondary_boundary =", re.search(r"_kFirstSecondaryTab = (\d+)", shell).group(1))
print("secondary_dispatch =", re.findall(r"case (\d+):\s*\n\s*return (\w+)", shell))
PY
printf '%s\n' '--- named tab abstractions ---'
rg -n -i 'enum .*tab|class .*tab|typedef .*tab|TabId|TabType|PitTab' lib test --glob '*.dart' || trueRepository: Spectrum3847/spectrum-pit
Length of output: 589
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
import re
role = Path("lib/src/models/user_role.dart").read_text()
for name in ("pit", "developer", "admin"):
matches = re.findall(
rf"case UserRole\.{name}:.*?tabs\.addAll\(const \[([^\]]+)\]",
role,
re.S,
)
print(name, [[int(x.strip()) for x in m.split(",")] for m in matches])
shell = Path("lib/src/ui/app_shell.dart").read_text()
print("secondary_boundary", re.search(r"_kFirstSecondaryTab = (\d+)", shell).group(1))
print("secondary_cases", re.findall(r"case (\d+):", shell[shell.index("Widget _secondaryBody"):shell.index("List<Widget> _buildAppBarActions")]))
PY
printf '%s\n' '--- named tab abstractions ---'
rg -n -i 'enum .*tab|class .*tab|typedef .*tab|TabId|TabType|PitTab' lib test --glob '*.dart' || trueRepository: Spectrum3847/spectrum-pit
Length of output: 1368
Replace hard-coded tab indices with named tabs. A tab insertion or reorder can desynchronize role visibility from shell routing. Define a shared tab identifier and derive both from it. The current mapping is 0–4 for feature tabs, 5 for Docs, 6 for Users, and 7 for Settings.
🤖 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/models/user_role.dart` around lines 39 - 53, Update the
visibleTabIndices implementation in UserRole to replace numeric literals with a
shared named tab identifier, preserving the current viewer, pit/developer, and
admin visibility mappings. Define or reuse the shared tab identifiers for
feature tabs, Docs, Users, and Settings so visibility remains aligned with shell
routing when tabs are inserted or reordered.
Comment on lines
+1
to
+5
| import 'dart:io' show Platform; | ||
|
|
||
| import 'package:device_info_plus/device_info_plus.dart'; | ||
| import 'package:flutter/foundation.dart' show kIsWeb; | ||
| import 'package:package_info_plus/package_info_plus.dart'; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Establish whether web is a supported build target and map every dart:io import in lib/.
set -euo pipefail
echo "=== CI build targets ==="
fd -e yml -e yaml . .github --exec rg -n 'flutter build|flutter test|chrome|web' {} \;
echo "=== dart:io imports under lib/ ==="
rg -n "import 'dart:io'" lib
echo "=== files that import the two affected services ==="
rg -n "debug_info.dart|desktop_launcher_service.dart" lib
echo "=== web scaffold present? ==="
fd -t f . web -d 1Repository: Spectrum3847/spectrum-pit
Length of output: 1064
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== relevant file outlines ==="
ast-grep outline lib/src/services/debug_info.dart
ast-grep outline lib/src/services/desktop_launcher_service.dart
ast-grep outline lib/src/ui/settings_tab.dart
echo "=== relevant source sections ==="
cat -n lib/src/services/debug_info.dart | sed -n '1,130p'
cat -n lib/src/services/desktop_launcher_service.dart | sed -n '1,220p'
cat -n lib/src/ui/settings_tab.dart | sed -n '1,330p'
echo "=== package metadata and web references ==="
cat -n pubspec.yaml | sed -n '1,220p'
rg -n -i 'web|kIsWeb|dart:io|debug_info|desktop_launcher|flutter build' README.md .github pubspec.yaml lib test 2>/dev/null || trueRepository: Spectrum3847/spectrum-pit
Length of output: 28142
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
from collections import defaultdict, deque
root = Path("lib")
files = {p.as_posix() for p in root.rglob("*.dart")}
imports = defaultdict(list)
for name in files:
text = Path(name).read_text()
for spec in re.findall(r"""^\s*import\s+['"]([^'"]+)['"]""", text, re.M):
if spec.startswith("dart:"):
imports[name].append((spec, None))
elif spec.startswith("package:spectrumpit/"):
target = spec.removeprefix("package:spectrumpit/")
imports[name].append((spec, target if target in files else None))
elif spec.startswith("../") or spec.startswith("./"):
target = (Path(name).parent / spec).resolve().relative_to(Path.cwd())
target = target.as_posix()
imports[name].append((spec, target if target in files else None))
reachable = set()
queue = deque(["lib/main.dart"])
while queue:
name = queue.popleft()
if name in reachable:
continue
reachable.add(name)
for _, target in imports[name]:
if target and target not in reachable:
queue.append(target)
print("=== dart:io files reachable from lib/main.dart ===")
for name in sorted(reachable):
if any(spec == "dart:io" for spec, _ in imports[name]):
print(name)
print("=== import paths from lib/main.dart to reachable dart:io files ===")
def paths(start, predicate):
todo = [(start, [start])]
seen = set()
while todo:
name, path = todo.pop(0)
if name in seen:
continue
seen.add(name)
if name != start and predicate(name):
print(" -> ".join(path))
for _, target in imports[name]:
if target and target not in seen:
todo.append((target, path + [target]))
paths("lib/main.dart",
lambda name: any(spec == "dart:io" for spec, _ in imports[name]))
print("=== all direct dart:io imports under lib/ ===")
for name in sorted(files):
if any(spec == "dart:io" for spec, _ in imports[name]):
print(name)
PY
echo "=== main and desktop service imports ==="
cat -n lib/main.dart | sed -n '1,125p'
cat -n lib/src/services/desktop_self_update_service.dart | sed -n '1,180p'
cat -n lib/src/services/desktop_update_service.dart | sed -n '1,180p'
cat -n lib/src/services/issue_report_service.dart | sed -n '1,80p'
cat -n lib/src/services/telemetry_service.dart | sed -n '1,80p'Repository: Spectrum3847/spectrum-pit
Length of output: 19939
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
from collections import defaultdict, deque
root = Path("lib")
files = {p.as_posix() for p in root.rglob("*.dart")}
imports = defaultdict(list)
for name in files:
source = Path(name).read_text()
for spec in re.findall(r"""^\s*import\s+['"]([^'"]+)['"]""", source, re.M):
if spec.startswith("dart:"):
imports[name].append((spec, None))
continue
if spec.startswith("package:"):
prefix = "package:spectrumpit/"
target = spec[len(prefix):] if spec.startswith(prefix) else None
else:
target = (Path(name).parent / spec).as_posix()
imports[name].append((spec, target if target in files else None))
reachable = set()
queue = deque(["lib/main.dart"])
while queue:
name = queue.popleft()
if name in reachable:
continue
reachable.add(name)
for _, target in imports[name]:
if target and target not in reachable:
queue.append(target)
io_files = sorted(
name for name in reachable
if any(spec == "dart:io" for spec, _ in imports[name])
)
print("=== all files reachable from lib/main.dart that import dart:io ===")
for name in io_files:
print(name)
print("=== shortest import paths from lib/main.dart ===")
queue = deque([("lib/main.dart", ["lib/main.dart"])])
seen = {"lib/main.dart"}
while queue:
name, path = queue.popleft()
if name in io_files:
print(" -> ".join(path))
for _, target in imports[name]:
if target and target not in seen:
seen.add(target)
queue.append((target, path + [target]))
PY
echo "=== direct imports in lib/main.dart ==="
cat -n lib/main.dart | sed -n '1,42p'
echo "=== all reachable dart:io import declarations ==="
for file in \
lib/src/services/debug_info.dart \
lib/src/services/desktop_auth_service.dart \
lib/src/services/desktop_launcher_service.dart \
lib/src/services/desktop_self_update_service.dart \
lib/src/services/map_image_store.dart \
lib/src/services/synced_map_image_store.dart
do
if test -f "$file"; then
echo "--- $file"
rg -n -C 2 "import 'dart:io'|kIsWeb|Platform\.|File\(|Directory\(|Process" "$file"
fi
done
echo "=== documented target references ==="
rg -n -i 'web|browser|chrome|flutter build' README.md docs .github pubspec.yaml web 2>/dev/null || trueRepository: Spectrum3847/spectrum-pit
Length of output: 12231
Prevent dart:io from entering web compilation.
Web is an intended target. Unconditional dart:io imports remain in debug_info.dart, desktop_launcher_service.dart, desktop_self_update_service.dart, desktop_auth_service.dart, map_image_store.dart, and synced_map_image_store.dart. Runtime kIsWeb checks do not prevent the compiler from resolving these imports. Use conditional exports or web-safe implementations for all affected services, then compile the web target. Preserve _isDesktopPlatform as the runtime widget guard.
📍 Affects 3 files
lib/src/services/debug_info.dart#L1-L5(this comment)lib/src/services/desktop_launcher_service.dart#L1-L3lib/src/ui/settings_tab.dart#L7-L8
🤖 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/debug_info.dart` around lines 1 - 5, Prevent dart:io from
being resolved during web compilation across the affected services:
debug_info.dart, desktop_launcher_service.dart,
desktop_self_update_service.dart, desktop_auth_service.dart,
map_image_store.dart, and synced_map_image_store.dart. Replace unconditional
platform imports with conditional exports or web-safe implementations, update
the affected settings_tab.dart integration as needed, and verify the web target
compiles while preserving _isDesktopPlatform as the runtime widget guard.
Comment on lines
+60
to
+79
| Future<void> initialize() async { | ||
| try { | ||
| final prefs = await _prefsLoader(); | ||
| final stored = prefs.getString(_prefsKey); | ||
| if (stored == null) return; | ||
| final user = await _session.restore( | ||
| (jsonDecode(stored) as Map).cast<String, dynamic>(), | ||
| ); | ||
| if (user != null) { | ||
| _emit( | ||
| SpectrumAuthSnapshot( | ||
| state: SpectrumAuthState.signedIn, | ||
| user: _toSpectrumUser(user), | ||
| ), | ||
| ); | ||
| } else { | ||
| await prefs.remove(_prefsKey); | ||
| } | ||
| } catch (_) {} | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Remove the persisted session when restore throws.
The method deletes _prefsKey only when _session.restore returns null (line 76). If restore throws, for example on a malformed payload or a revoked token that surfaces as an exception, the catch block at line 78 swallows the error and the invalid entry stays in SharedPreferences. Every later launch then repeats the same failing restore.
🛠️ Proposed fix
Future<void> initialize() async {
+ SharedPreferences? prefs;
try {
- final prefs = await _prefsLoader();
+ prefs = await _prefsLoader();
final stored = prefs.getString(_prefsKey);
if (stored == null) return;
final user = await _session.restore(
(jsonDecode(stored) as Map).cast<String, dynamic>(),
);
if (user != null) {
_emit(
SpectrumAuthSnapshot(
state: SpectrumAuthState.signedIn,
user: _toSpectrumUser(user),
),
);
} else {
await prefs.remove(_prefsKey);
}
- } catch (_) {}
+ } catch (error) {
+ debugPrint('Desktop session restore failed: $error');
+ try {
+ await prefs?.remove(_prefsKey);
+ } catch (_) {}
+ }
}📝 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
| Future<void> initialize() async { | |
| try { | |
| final prefs = await _prefsLoader(); | |
| final stored = prefs.getString(_prefsKey); | |
| if (stored == null) return; | |
| final user = await _session.restore( | |
| (jsonDecode(stored) as Map).cast<String, dynamic>(), | |
| ); | |
| if (user != null) { | |
| _emit( | |
| SpectrumAuthSnapshot( | |
| state: SpectrumAuthState.signedIn, | |
| user: _toSpectrumUser(user), | |
| ), | |
| ); | |
| } else { | |
| await prefs.remove(_prefsKey); | |
| } | |
| } catch (_) {} | |
| } | |
| Future<void> initialize() async { | |
| SharedPreferences? prefs; | |
| try { | |
| prefs = await _prefsLoader(); | |
| final stored = prefs.getString(_prefsKey); | |
| if (stored == null) return; | |
| final user = await _session.restore( | |
| (jsonDecode(stored) as Map).cast<String, dynamic>(), | |
| ); | |
| if (user != null) { | |
| _emit( | |
| SpectrumAuthSnapshot( | |
| state: SpectrumAuthState.signedIn, | |
| user: _toSpectrumUser(user), | |
| ), | |
| ); | |
| } else { | |
| await prefs.remove(_prefsKey); | |
| } | |
| } catch (error) { | |
| debugPrint('Desktop session restore failed: $error'); | |
| try { | |
| await prefs?.remove(_prefsKey); | |
| } catch (_) {} | |
| } | |
| } |
🤖 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/desktop_auth_service.dart` around lines 60 - 79, Update
initialize() so that when _session.restore throws while restoring the stored
session, it removes _prefsKey from the loaded preferences before swallowing the
error. Preserve the existing removal when restore returns null and keep
successful restoration behavior unchanged.
Comment on lines
+48
to
+56
| late FakePitShiftSyncService sync; | ||
| late PitShiftController controller; | ||
|
|
||
| setUp(() { | ||
| sync = FakePitShiftSyncService(); | ||
| SharedPreferences.setMockInitialValues(<String, Object>{}); | ||
| }); | ||
|
|
||
| tearDown(() => controller.dispose()); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
tearDown throws when a test does not reach pumpTab.
controller is late and is assigned at line 86, inside pumpTab. The tearDown at line 56 calls controller.dispose() unconditionally. If a test fails before line 86, or if a later test never calls pumpTab, the teardown throws LateInitializationError. That error replaces the real failure in the report.
Make the field nullable and dispose it conditionally.
💚 Proposed fix
late FakePitShiftSyncService sync;
- late PitShiftController controller;
+ PitShiftController? controller;
setUp(() {
sync = FakePitShiftSyncService();
SharedPreferences.setMockInitialValues(<String, Object>{});
});
- tearDown(() => controller.dispose());
+ tearDown(() {
+ controller?.dispose();
+ controller = null;
+ });Update the assignment at line 86 and the two controller: uses at line 95 accordingly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/schedule_tab_test.dart` around lines 48 - 56, Make the controller field
nullable, update its assignment and both controller references in pumpTab to
handle the nullable type, and change tearDown to dispose it only when
initialized. Preserve disposal behavior while preventing LateInitializationError
when pumpTab is not reached.
Comment on lines
+234
to
+311
| testWidgets('marking yourself unavailable writes an unavailable block', ( | ||
| tester, | ||
| ) async { | ||
| await pumpTab( | ||
| tester, | ||
| shifts: [_shift('a', label: 'Qual block', startMatch: 18, endMatch: 34)], | ||
| ); | ||
|
|
||
| await tester.tap(find.text('Mark unavailable')); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| // The form talks about the person, not about a kind of shift. | ||
| expect(find.text('Mark yourself unavailable'), findsOneWidget); | ||
| expect(find.widgetWithText(TextField, 'Reason'), findsOneWidget); | ||
| expect(find.text('Type'), findsNothing); | ||
|
|
||
| await tester.enterText( | ||
| find.widgetWithText(TextField, 'Reason'), | ||
| 'Drivers meeting', | ||
| ); | ||
| await tester.tap(find.text('Match')); | ||
| await tester.pumpAndSettle(); | ||
| await tester.enterText(find.widgetWithText(TextField, 'First match'), '40'); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| await tester.tap(find.widgetWithText(FilledButton, 'Mark unavailable')); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| final saved = sync.upserts.single; | ||
| expect(saved.kind, ShiftKind.unavailable); | ||
| expect(saved.label, 'Drivers meeting'); | ||
| expect(saved.competition, 'Houston'); | ||
| expect(saved.assignedUids, ['uid-me']); | ||
| expect(saved.assignedNames, ['Alex Reyes']); | ||
| expect(saved.startMatch, 40); | ||
| expect(saved.startsAt, isNull); | ||
| }); | ||
|
|
||
| testWidgets('the shift form offers the roster and saves both name lists', ( | ||
| tester, | ||
| ) async { | ||
| await pumpTab( | ||
| tester, | ||
| shifts: [ | ||
| _shift( | ||
| 'a', | ||
| label: 'Qual block', | ||
| startMatch: 18, | ||
| endMatch: 34, | ||
| uids: const ['uid-other'], | ||
| names: const ['Sam Ito'], | ||
| ), | ||
| ], | ||
| ); | ||
|
|
||
| await tester.tap(find.byType(FloatingActionButton)); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| // Self plus everyone already on the schedule, no roster read required. | ||
| expect(find.widgetWithText(FilterChip, 'Alex Reyes'), findsOneWidget); | ||
| expect(find.widgetWithText(FilterChip, 'Sam Ito'), findsOneWidget); | ||
|
|
||
| await tester.enterText( | ||
| find.widgetWithText(TextField, 'Shift name'), | ||
| 'Load out crew', | ||
| ); | ||
| await tester.enterText(find.widgetWithText(TextField, 'First match'), '80'); | ||
| await tester.tap(find.widgetWithText(FilterChip, 'Sam Ito')); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| await tester.tap(find.widgetWithText(FilledButton, 'Add shift')); | ||
| await tester.pumpAndSettle(); | ||
|
|
||
| final saved = sync.upserts.single; | ||
| expect(saved.label, 'Load out crew'); | ||
| expect(saved.assignedUids, containsAll(<String>['uid-me', 'uid-other'])); | ||
| expect(saved.assignedNames, containsAll(<String>['Alex Reyes', 'Sam Ito'])); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add coverage for the delete flow.
No test exercises _openEditor's onDelete at lib/src/ui/schedule_tab.dart lines 189-201. That path is destructive and has three branches: the user cancels, the delete succeeds, and the delete fails and keeps the sheet open. FakePitShiftSyncService already records deletes, so the fake supports the assertion.
💚 Proposed test
testWidgets('deleting a shift asks first and then writes the delete', (
tester,
) async {
await pumpTab(
tester,
shifts: [_shift('a', label: 'Qual block', startMatch: 18, endMatch: 34)],
);
await tester.tap(find.text('Qual block'));
await tester.pumpAndSettle();
await tester.tap(find.byTooltip('Delete'));
await tester.pumpAndSettle();
expect(find.text('Delete shift?'), findsOneWidget);
await tester.tap(find.widgetWithText(TextButton, 'Cancel'));
await tester.pumpAndSettle();
expect(sync.deletes, isEmpty);
await tester.tap(find.byTooltip('Delete'));
await tester.pumpAndSettle();
await tester.tap(find.widgetWithText(FilledButton, 'Delete'));
await tester.pumpAndSettle();
expect(sync.deletes, ['a']);
});🤖 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/schedule_tab_test.dart` around lines 234 - 311, Add widget-test coverage
in the schedule tab tests for _openEditor’s onDelete flow: verify cancelling the
confirmation leaves sync.deletes empty, confirming a successful deletion records
the shift ID, and a failed deletion keeps the editor sheet open. Reuse the
existing pumpTab setup and FakePitShiftSyncService delete behavior.
Comment on lines
+7
to
+18
| // Every field the Firestore rules' isValidTelemetry allows. | ||
| const _allowedKeys = { | ||
| 'id', | ||
| 'type', | ||
| 'deviceId', | ||
| 'appVersion', | ||
| 'platform', | ||
| 'osVersion', | ||
| 'locale', | ||
| 'detail', | ||
| 'createdAt', | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
The whitelist assertion checks only a subset, and it duplicates the rules by hand.
Line 49 asserts that every written key is in _allowedKeys. It does not assert that the required keys are present. A regression that drops deviceId or createdAt still passes this check, while real Firestore rejects the write.
_allowedKeys is a hand-copied mirror of isValidTelemetry in firestore.rules. If the rules gain or lose a field, this test does not fail, so the two drift apart silently.
Assert the exact required set, and add a comment that links the two files so a rules change prompts a test update.
♻️ Proposed fix
+const _requiredKeys = {
+ 'id',
+ 'type',
+ 'deviceId',
+ 'appVersion',
+ 'platform',
+ 'osVersion',
+ 'createdAt',
+}; expect(data.keys.every(_allowedKeys.contains), isTrue);
+ expect(data.keys.toSet(), containsAll(_requiredKeys));Also applies to: 49-49
🤖 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/telemetry_service_test.dart` around lines 7 - 18, Update the telemetry
payload assertion in the test around _allowedKeys to require exact set equality,
ensuring no allowed key is missing or unexpected rather than checking only that
written keys are a subset. Add a concise comment referencing firestore.rules and
isValidTelemetry, explicitly noting that changes to the rules’ required fields
must update this test and its expected set.
Comment on lines
+36
to
+58
| test('logEvent writes a telemetry doc within the rules whitelist', () async { | ||
| final firestore = FakeFirebaseFirestore(); | ||
| final service = TelemetryService( | ||
| firestore: firestore, | ||
| debugInfo: () async => _info, | ||
| ); | ||
|
|
||
| await service.logEvent('app_open'); | ||
|
|
||
| final snap = await firestore.collection('telemetry').get(); | ||
| expect(snap.docs, hasLength(1)); | ||
| final data = snap.docs.single.data(); | ||
|
|
||
| expect(data.keys.every(_allowedKeys.contains), isTrue); | ||
| expect(data['type'], 'app_open'); | ||
| expect(data['deviceId'], isNotEmpty); | ||
| expect(data['platform'], 'linux'); | ||
| expect(data['appVersion'], contains('1.2.3')); | ||
| expect(data['id'], snap.docs.single.id); | ||
| expect(data.containsKey('detail'), isFalse); | ||
| final createdAt = data['createdAt'] as String; | ||
| expect(DateTime.tryParse(createdAt), isNotNull); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a test for the _clamp limits.
TelemetryService.logEvent clamps type to 64 characters, appVersion to 64, osVersion to 128, locale to 32, and detail to 128. These limits exist to satisfy the Firestore rules. No test exercises them, so a change to any limit passes silently and the rules reject the write in production.
Add a case that logs an over-length type and detail, then asserts the stored lengths.
🤖 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/telemetry_service_test.dart` around lines 36 - 58, Add a test case for
TelemetryService.logEvent that submits over-length type and detail values, then
reads the stored telemetry document and asserts type is limited to 64 characters
and detail to 128 characters. Use the existing FakeFirebaseFirestore setup and
verify the persisted values rather than only the input values.
Comment on lines
+92
to
+99
| VALUE "CompanyName", "Spectrum 3847" "\0" | ||
| VALUE "FileDescription", "spectrumpit" "\0" | ||
| VALUE "FileVersion", VERSION_AS_STRING "\0" | ||
| VALUE "InternalName", "spectrumpit" "\0" | ||
| VALUE "LegalCopyright", "Copyright (C) 2026 Spectrum 3847. All rights reserved." "\0" | ||
| VALUE "OriginalFilename", "spectrumpit.exe" "\0" | ||
| VALUE "ProductName", "spectrumpit" "\0" | ||
| VALUE "ProductVersion", VERSION_AS_STRING "\0" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare the product display names across the desktop platform manifests.
rg -n 'PRODUCT_NAME|CFBundleName|CFBundleDisplayName|APPLICATION_ID' macos/Runner/Configs/AppInfo.xcconfig macos/Runner/Info.plist
rg -n 'application_id|set_title|APPLICATION_ID' linux/runner/my_application.cc linux/CMakeLists.txt
rg -n 'BINARY_NAME' windows/CMakeLists.txtRepository: Spectrum3847/spectrum-pit
Length of output: 1044
🏁 Script executed:
#!/bin/bash
# Inspect the relevant resource and desktop metadata values without executing repository code.
printf '%s\n' '--- windows/runner/Runner.rc ---'
sed -n '48,105p' windows/runner/Runner.rc
printf '%s\n' '--- macOS metadata ---'
sed -n '1,24p' macos/Runner/Configs/AppInfo.xcconfig
sed -n '1,24p' macos/Runner/Info.plist
printf '%s\n' '--- Linux title metadata ---'
sed -n '35,58p' linux/runner/my_application.cc
sed -n '1,18p' linux/CMakeLists.txt
printf '%s\n' '--- Windows build name ---'
sed -n '1,14p' windows/CMakeLists.txtRepository: Spectrum3847/spectrum-pit
Length of output: 5479
🏁 Script executed:
#!/bin/bash
# Determine whether “Spectrum Pit” is an established display name in the repository
# and distinguish it from package, executable, and identifier names.
printf '%s\n' '--- display-name occurrences ---'
rg -n -i --hidden --glob '!.git/**' 'Spectrum[[:space:]]+Pit|spectrumpit' .
printf '%s\n' '--- package metadata files ---'
fd -i 'pubspec.yaml|README.*|.*manifest.*' . -t f | head -80Repository: Spectrum3847/spectrum-pit
Length of output: 22836
Set the desktop display names to “Spectrum Pit”.
FileDescription and ProductName use spectrumpit, while the app UI and mobile/web metadata use “Spectrum Pit”. Update both Windows fields, macOS PRODUCT_NAME, and the Linux GTK window titles. Keep InternalName and OriginalFilename unchanged.
🤖 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 `@windows/runner/Runner.rc` around lines 92 - 99, Update the Windows
VERSIONINFO fields FileDescription and ProductName from “spectrumpit” to
“Spectrum Pit”, while preserving InternalName and OriginalFilename. Also update
macOS PRODUCT_NAME and the Linux GTK window titles to “Spectrum Pit”, leaving
their internal identifiers unchanged.
| if (path == null || path.isEmpty) { | ||
| throw StateError('Not running as an AppImage'); | ||
| } | ||
| final response = await _client.get(url); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Apply a timeout to the download request.
_client.get has no timeout. If the release host stalls, the future never completes and the install action in lib/src/ui/settings_tab.dart stays pending. The repository already provides lib/src/services/http_timeout_client.dart. Wrap the injected client with it, or apply .timeout(...) to the request.
⏱️ Proposed fix
- final response = await _client.get(url);
+ final response = await _client
+ .get(url)
+ .timeout(const Duration(minutes: 5));📝 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
| final response = await _client.get(url); | |
| final response = await _client | |
| .get(url) | |
| .timeout(const Duration(minutes: 5)); |
🤖 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/desktop_self_update_service.dart` at line 30, Update the
download request in the desktop self-update flow around _client.get to enforce
the repository’s standard HTTP timeout, preferably by wrapping the injected
client with HttpTimeoutClient or applying the established timeout configuration
to the request, so stalled release hosts cannot leave the install action
pending.
Comment on lines
+30
to
+40
| final response = await _client.get(url); | ||
|
|
||
| if (response.statusCode != 200 || response.bodyBytes.length < 100000) { | ||
| throw StateError('Download failed (status ${response.statusCode})'); | ||
| } | ||
|
|
||
| final staged = File('$path.new'); | ||
| await staged.writeAsBytes(response.bodyBytes, flush: true); | ||
| await staged.rename(path); | ||
| await _makeExecutable(path); | ||
| await _relaunch(path); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Verify the downloaded AppImage before you execute it.
update accepts any payload that returns status 200 and exceeds 100000 bytes. It then writes that payload over the current executable and starts it. This is remote code execution if the URL, the release host, or any network hop is attacker-controlled. A size floor is not an integrity check.
Add two controls:
- Reject any
urlwhose scheme is nothttps. - Verify the payload against a checksum or a signature that the caller supplies from a trusted source.
🔒️ Proposed fix: enforce HTTPS and verify a checksum
- Future<void> update(Uri url) async {
+ Future<void> update(Uri url, {required String sha256Hex}) async {
+ if (url.scheme != 'https') {
+ throw StateError('Refusing to download an update over ${url.scheme}');
+ }
final path = _appImagePath();
if (path == null || path.isEmpty) {
throw StateError('Not running as an AppImage');
}
final response = await _client.get(url);
if (response.statusCode != 200 || response.bodyBytes.length < 100000) {
throw StateError('Download failed (status ${response.statusCode})');
}
+
+ final digest = sha256.convert(response.bodyBytes).toString();
+ if (digest != sha256Hex.toLowerCase()) {
+ throw StateError('Update checksum mismatch');
+ }
final staged = File('$path.new');Add the import:
+import 'package:crypto/crypto.dart';The caller in lib/src/ui/settings_tab.dart must pass the expected digest from the same release metadata that supplies the URL.
🤖 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/desktop_self_update_service.dart` around lines 30 - 40,
Update the desktop self-update flow around update to reject URLs unless their
scheme is https, then verify the downloaded bytes against a caller-supplied
trusted checksum before writing, renaming, or executing the staged AppImage.
Extend update’s contract and its call site in settings_tab.dart so the expected
digest comes from the same trusted release metadata as the URL, and abort on any
mismatch.
Comment on lines
+63
to
+68
| @override | ||
| Future<void> updateRoles(String targetUid, Set<UserRole> roles) async { | ||
| await _firestore.collection('userProfiles').doc(targetUid).update({ | ||
| 'roles': roles.map((r) => r.name).toList(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Align updateRoles with the desktop implementation.
update() throws not-found when the profile document is absent. DesktopUserRoleService.updateRoles performs a masked setDocument, which creates the document instead. The same admin action then behaves differently per platform. Use a merged set to match.
🔁 Proposed fix
`@override`
Future<void> updateRoles(String targetUid, Set<UserRole> roles) async {
- await _firestore.collection('userProfiles').doc(targetUid).update({
- 'roles': roles.map((r) => r.name).toList(),
- });
+ await _firestore.collection('userProfiles').doc(targetUid).set(
+ {'roles': roles.map((r) => r.name).toList()},
+ SetOptions(merge: true),
+ );
}Confirm that firestore.rules permits create on userProfiles for admins, otherwise keep update() and make the desktop implementation strict instead.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @override | |
| Future<void> updateRoles(String targetUid, Set<UserRole> roles) async { | |
| await _firestore.collection('userProfiles').doc(targetUid).update({ | |
| 'roles': roles.map((r) => r.name).toList(), | |
| }); | |
| } | |
| `@override` | |
| Future<void> updateRoles(String targetUid, Set<UserRole> roles) async { | |
| await _firestore.collection('userProfiles').doc(targetUid).set( | |
| {'roles': roles.map((r) => r.name).toList()}, | |
| SetOptions(merge: true), | |
| ); | |
| } |
🤖 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/user_role_service.dart` around lines 63 - 68, Update
UserRoleService.updateRoles to use a merged set operation for the target
userProfiles document, matching DesktopUserRoleService.updateRoles and creating
the document when absent. Confirm firestore.rules permits admins to create
userProfiles; otherwise retain this update behavior and make the desktop
implementation strict instead.
Comment on lines
+70
to
+81
| @override | ||
| Stream<List<UserProfile>> streamAllProfiles() { | ||
| return _firestore | ||
| .collection('userProfiles') | ||
| .orderBy('displayName') | ||
| .snapshots() | ||
| .map( | ||
| (snapshot) => snapshot.docs | ||
| .map((doc) => UserProfile.fromJson(doc.id, doc.data())) | ||
| .toList(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Roster ordering differs from the desktop implementation.
orderBy('displayName') sorts case-sensitively, so Zoe precedes adam. DesktopUserRoleService.streamAllProfiles sorts with toLowerCase(). The same roster appears in a different order per platform. orderBy also omits any document that lacks a displayName field.
Sort client-side to match the desktop behavior.
♻️ Proposed fix
Stream<List<UserProfile>> streamAllProfiles() {
return _firestore
.collection('userProfiles')
- .orderBy('displayName')
.snapshots()
.map(
(snapshot) => snapshot.docs
.map((doc) => UserProfile.fromJson(doc.id, doc.data()))
- .toList(),
+ .toList()
+ ..sort(
+ (a, b) => a.displayName.toLowerCase().compareTo(
+ b.displayName.toLowerCase(),
+ ),
+ ),
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @override | |
| Stream<List<UserProfile>> streamAllProfiles() { | |
| return _firestore | |
| .collection('userProfiles') | |
| .orderBy('displayName') | |
| .snapshots() | |
| .map( | |
| (snapshot) => snapshot.docs | |
| .map((doc) => UserProfile.fromJson(doc.id, doc.data())) | |
| .toList(), | |
| ); | |
| } | |
| `@override` | |
| Stream<List<UserProfile>> streamAllProfiles() { | |
| return _firestore | |
| .collection('userProfiles') | |
| .snapshots() | |
| .map( | |
| (snapshot) => snapshot.docs | |
| .map((doc) => UserProfile.fromJson(doc.id, doc.data())) | |
| .toList() | |
| ..sort( | |
| (a, b) => a.displayName.toLowerCase().compareTo( | |
| b.displayName.toLowerCase(), | |
| ), | |
| ), | |
| ); | |
| } |
🤖 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/user_role_service.dart` around lines 70 - 81, Update
streamAllProfiles to remove the Firestore orderBy('displayName') query and sort
the mapped UserProfile list client-side using displayName.toLowerCase(),
matching DesktopUserRoleService; ensure profiles without displayName are
retained according to the existing model behavior.
| import 'pit_palette.dart'; | ||
|
|
||
| TextStyle pitCodeStyle(BuildContext context, {Color? color}) { | ||
| return GoogleFonts.ibmPlexMono( |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether IBM Plex fonts are bundled and whether runtime fetching is disabled.
fd -t f 'pubspec.yaml' --max-depth 2 --exec cat -n {}
echo '--- font asset files ---'
fd -i -e ttf -e otf . | head -50
echo '--- allowRuntimeFetching usage ---'
rg -n 'allowRuntimeFetching|GoogleFonts.config' --glob '*.dart'Repository: Spectrum3847/spectrum-pit
Length of output: 4094
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- theme references ---'
rg -n -C 3 'GoogleFonts|pitCodeStyle|_themeFrom|_textThemeFor' lib/src/theme/app_theme.dart
printf '%s\n' '--- GoogleFonts configuration and font references ---'
rg -n -C 2 'GoogleFonts|allowRuntimeFetching|fontFamily|assets:' --glob '*.dart' --glob 'pubspec.yaml' .
printf '%s\n' '--- repository font and asset files ---'
git ls-files | rg -i '(^|/)(assets?|fonts?)/|\\.(ttf|otf)$' || trueRepository: Spectrum3847/spectrum-pit
Length of output: 5000
Bundle the IBM Plex fonts.
pubspec.yaml contains no IBM Plex font assets, and GoogleFonts.config.allowRuntimeFetching is not disabled. Without network access, the theme can fall back to platform fonts. Add the font assets and disable runtime fetching.
🤖 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/theme/app_theme.dart` at line 7, Update the theme configuration
around GoogleFonts.ibmPlexMono to bundle the IBM Plex font assets declared in
pubspec.yaml, and disable GoogleFonts.config.allowRuntimeFetching so font
resolution never depends on network access.
Comment on lines
+35
to
+48
| @override | ||
| Stream<List<UserProfile>> streamAllProfiles() { | ||
| return Stream.value( | ||
| _roles.entries | ||
| .map( | ||
| (e) => UserProfile( | ||
| uid: e.key, | ||
| displayName: _displayNames[e.key] ?? e.key, | ||
| roles: e.value, | ||
| ), | ||
| ) | ||
| .toList(), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
Emit live updates from streamAllProfiles.
Stream.value captures the profile list at call time and then closes. updateRoles and setRoles never reach an existing subscriber. The real UserRoleService streams Firestore snapshots, so the fake does not model the contract it stands in for. Tests cannot assert that the user list refreshes after a role change; test/user_management_screen_test.dart line 88 has to read the roles back through fetchOrCreateRoles instead of through the UI.
Back the fake with a broadcast controller and re-emit after each mutation.
♻️ Proposed refactor
+import 'dart:async';
+
import 'package:spectrumpit/src/models/user_profile.dart';
import 'package:spectrumpit/src/models/user_role.dart';
import 'package:spectrumpit/src/services/user_role_service.dart';
class FakeUserRoleService implements UserRoleService {
final Map<String, Set<UserRole>> _roles = {};
final Map<String, String> _displayNames = {};
+ final StreamController<List<UserProfile>> _profiles =
+ StreamController<List<UserProfile>>.broadcast();
- void setRoles(String uid, Set<UserRole> roles) => _roles[uid] = roles;
+ void setRoles(String uid, Set<UserRole> roles) {
+ _roles[uid] = roles;
+ _publish();
+ }
- void setRole(String uid, UserRole role) => _roles[uid] = {role};
+ void setRole(String uid, UserRole role) => setRoles(uid, {role});
+
+ List<UserProfile> _snapshot() => _roles.entries
+ .map(
+ (e) => UserProfile(
+ uid: e.key,
+ displayName: _displayNames[e.key] ?? e.key,
+ roles: e.value,
+ ),
+ )
+ .toList();
+
+ void _publish() {
+ if (!_profiles.isClosed) _profiles.add(_snapshot());
+ }
`@override`
Future<void> updateRoles(String uid, Set<UserRole> roles) async {
_roles[uid] = roles;
+ _publish();
}
`@override`
- Stream<List<UserProfile>> streamAllProfiles() {
- return Stream.value(
- _roles.entries
- .map(
- (e) => UserProfile(
- uid: e.key,
- displayName: _displayNames[e.key] ?? e.key,
- roles: e.value,
- ),
- )
- .toList(),
- );
- }
+ Stream<List<UserProfile>> streamAllProfiles() async* {
+ yield _snapshot();
+ yield* _profiles.stream;
+ }
}🤖 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/support/fake_user_role_service.dart` around lines 35 - 48, Update the
fake service’s streamAllProfiles to use a broadcast stream controller that emits
the current profile list immediately and re-emits after updateRoles and setRoles
mutate _roles or related state. Ensure the controller remains available to
existing subscribers and preserves the profile mapping used by the current
Stream.value implementation.
Comment on lines
+33
to
+36
| final controller = UserRoleController( | ||
| authService: FakeSpectrumAuthService(), | ||
| roleService: roleService, | ||
| ); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Dispose the UserRoleController in both tests.
Neither test disposes the controller. UserRoleController subscribes to the auth snapshot stream, so the subscription outlives the test. Later tests in the same file then run with a live listener attached to a dead controller, which can surface as an unrelated failure. Every other controller test in this PR disposes. Register disposal at the creation site.
The second test at lines 65-70 has the same gap.
🛠️ Proposed fix
final controller = UserRoleController(
authService: FakeSpectrumAuthService(),
roleService: roleService,
);
+ addTearDown(controller.dispose);🤖 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/user_management_screen_test.dart` around lines 33 - 36, Dispose each
UserRoleController created in both tests immediately after construction,
registering cleanup at the creation site so the auth snapshot subscription is
released when each test completes. Update both controller instantiations,
including the second test’s instance, without changing the test behavior.
Comment on lines
+40
to
+90
| group('UserRoleSetPermissions', () { | ||
| test('viewer only: no tabs, no debug, no manage', () { | ||
| final roles = {UserRole.viewer}; | ||
| expect(roles.visibleTabIndices, isEmpty); | ||
| expect(roles.canManageUsers, isFalse); | ||
| expect(roles.isDebug, isFalse); | ||
| }); | ||
|
|
||
| test('pit: feature tabs + Docs + Settings', () { | ||
| expect({UserRole.pit}.visibleTabIndices, [0, 1, 2, 3, 4, 5, 7]); | ||
| }); | ||
|
|
||
| // isMember gates the in-app problem-reporting surface (#438): everyone | ||
| // except viewer-only gets it. | ||
| test('isMember: true for any non-viewer role, false for viewer only', () { | ||
| expect({UserRole.viewer}.isMember, isFalse); | ||
| expect({UserRole.pit}.isMember, isTrue); | ||
| expect({UserRole.admin}.isMember, isTrue); | ||
| expect({UserRole.developer}.isMember, isTrue); | ||
| expect({UserRole.viewer, UserRole.pit}.isMember, isTrue); | ||
| }); | ||
|
|
||
| test('admin: all tabs including Users, canManageUsers', () { | ||
| final roles = {UserRole.admin}; | ||
| expect(roles.visibleTabIndices, [0, 1, 2, 3, 4, 5, 6, 7]); | ||
| expect(roles.canManageUsers, isTrue); | ||
| }); | ||
|
|
||
| test('developer: feature tabs + Docs + Settings, no Users, isDebug', () { | ||
| final roles = {UserRole.developer}; | ||
| expect(roles.visibleTabIndices, [0, 1, 2, 3, 4, 5, 7]); | ||
| expect(roles.isDebug, isTrue); | ||
| expect(roles.canManageUsers, isFalse); | ||
| }); | ||
|
|
||
| test('multi-role union: pit + admin = all tabs', () { | ||
| final roles = {UserRole.pit, UserRole.admin}; | ||
| expect(roles.visibleTabIndices, [0, 1, 2, 3, 4, 5, 6, 7]); | ||
| expect(roles.canManageUsers, isTrue); | ||
| }); | ||
|
|
||
| test( | ||
| 'multi-role union: admin + developer = all tabs + isDebug + canManage', | ||
| () { | ||
| final roles = {UserRole.admin, UserRole.developer}; | ||
| expect(roles.visibleTabIndices, [0, 1, 2, 3, 4, 5, 6, 7]); | ||
| expect(roles.canManageUsers, isTrue); | ||
| expect(roles.isDebug, isTrue); | ||
| }, | ||
| ); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Name the tab indices instead of hard-coding literal lists.
Lines 49, 64, 70, 77, and 85 assert raw index lists such as [0, 1, 2, 3, 4, 5, 7]. A reader cannot tell which tab index 6 or 7 is. When a tab is inserted, every expectation shifts and the diff gives no signal about intent. Define named constants for the tab indices, or expose a labelled accessor, and assert against those.
🤖 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/user_role_controller_test.dart` around lines 40 - 90, Replace the
hard-coded visibleTabIndices lists in the UserRoleSetPermissions tests with
named tab-index constants or a labelled accessor that identifies each tab,
including the omitted Users tab and Settings tab. Update the assertions for pit,
admin, developer, and multi-role cases to use those names while preserving their
current tab membership and ordering.
Comment on lines
+251
to
+254
| expect( | ||
| () => controller.updateUserRoles('uid-x', {UserRole.pit}), | ||
| throwsStateError, | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether updateUserRoles is async and where the guard throws.
rg -n -A20 'updateUserRoles' lib/src/state/user_role_controller.dartRepository: Spectrum3847/spectrum-pit
Length of output: 769
Await the updateUserRoles failure
updateUserRoles is an async method. Its StateError is returned through the Future, not thrown by the callback. Use await expectLater here and at lines 271-274.
🛠️ Proposed fix
- expect(
- () => controller.updateUserRoles('uid-x', {UserRole.pit}),
- throwsStateError,
- );
+ await expectLater(
+ controller.updateUserRoles('uid-x', {UserRole.pit}),
+ throwsStateError,
+ );📝 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
| expect( | |
| () => controller.updateUserRoles('uid-x', {UserRole.pit}), | |
| throwsStateError, | |
| ); | |
| await expectLater( | |
| controller.updateUserRoles('uid-x', {UserRole.pit}), | |
| throwsStateError, | |
| ); |
🤖 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/user_role_controller_test.dart` around lines 251 - 254, Update the tests
for the async updateUserRoles method to await its rejected Future using await
expectLater, including both the shown assertion and the corresponding assertion
around lines 271-274. Preserve the expected StateError matcher.
Comment on lines
+56
to
+59
| def redraw(path, maker): | ||
| size = Image.open(path).size[0] | ||
| maker(size).save(path) | ||
| print(f"{path.relative_to(ROOT)} ({size})") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Close the source image before writing to the same path.
Image.open(path) is lazy. It keeps the file open until the image is closed or garbage collected. Line 58 then writes to that same path while the handle is still open. On Windows this raises a sharing violation. On every platform the script leaks one file descriptor per icon, and the script processes many icons in one run. Read the size inside a with block.
Line 57 also assumes a square source. If a non-square icon file exists, the height is discarded silently and the icon is redrawn square.
🛠️ Proposed fix
def redraw(path, maker):
- size = Image.open(path).size[0]
+ with Image.open(path) as src:
+ width, height = src.size
+ if width != height:
+ raise ValueError(f"{path} is not square: {width}x{height}")
+ size = width
maker(size).save(path)
print(f"{path.relative_to(ROOT)} ({size})")📝 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
| def redraw(path, maker): | |
| size = Image.open(path).size[0] | |
| maker(size).save(path) | |
| print(f"{path.relative_to(ROOT)} ({size})") | |
| def redraw(path, maker): | |
| with Image.open(path) as src: | |
| width, height = src.size | |
| if width != height: | |
| raise ValueError(f"{path} is not square: {width}x{height}") | |
| size = width | |
| maker(size).save(path) | |
| print(f"{path.relative_to(ROOT)} ({size})") |
🤖 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 `@tool/generate_icons.py` around lines 56 - 59, Update redraw to open the
source image within a with block, read its full size before calling maker, and
close it before saving back to the same path. Preserve the existing redraw and
reporting flow while avoiding the square-only size assumption by handling both
image dimensions.
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Spectrum Pit 1.0.0: FRC pit logistics — inventory with lab and pit maps, four-stage event packing with optional photos, and a borrowed-tools tracker. iOS, Android, and desktop.
Summary by CodeRabbit