Sync v1.1.0 - #2
Conversation
📝 WalkthroughWalkthroughThis release updates platform settings, Firestore rules, desktop synchronization and updates, photo caching, state recovery, UI navigation, data validation, and test coverage across Android, macOS, Linux, Windows, and Flutter. ChangesApplication hardening and release updates
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
lib/src/services/photo_service.dart (1)
121-156: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor
Reachability: External · Exploitability: Moderate
Reachability path
● Entry lib/src/services/desktop_auth_service.dart:11 DesktopAuthService │ ▼ ● Hop lib/src/services/synced_map_image_store.dart │ ▼ ● Sink lib/src/services/photo_service.dartScope photo caches to the active session.
When sign-out starts
clearCache(), an in-flightfetchcan finish afterward and repopulate both caches. A later session can then receive those bytes from_cachebefore_sendruns. Add a session generation or identity check to discard stale reads and writes, and add an interleaving test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/services/photo_service.dart` around lines 121 - 156, The photo cache must be scoped to the active session so an in-flight fetch cannot repopulate caches after clearCache(). Update fetch, clearCache, and the related cache read/write flow to track a session generation or identity, discard stale in-flight results, and prevent stale writes to both _cache and _diskCache; ensure later sessions do not receive prior-session bytes before _send runs. Add a test covering clearCache interleaved with an in-flight fetch.lib/src/services/desktop_borrow_sync_service.dart (1)
44-78: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winStop desktop borrow and packing pollers on disposal. Both
async*loops usewhile (true). During repeatedlistDocumentsfailures, they reach noyield, so stream cancellation does not stop the generator or its polling. Add_disposedanddispose(), usewhile (!_disposed), and calldispose()from the application lifecycle.lib/main.dartstores these services behind interfaces, whilelib/src/app.dartdisposes only their controllers; make service disposal reachable there. Add sustained-failure teardown tests for both services.🤖 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_borrow_sync_service.dart` around lines 44 - 78, Update DesktopBorrowSyncService in lib/src/services/desktop_borrow_sync_service.dart:44-78 and DesktopPackingSyncService in lib/src/services/desktop_packing_sync_service.dart:53-83 to add an _disposed state, expose dispose(), and replace each unconditional async* while (true) loop with a condition that exits after disposal, including during sustained listDocuments failures and polling delays. Make service disposal reachable through the interfaces stored by lib/main.dart, invoke it from the application lifecycle in lib/src/app.dart alongside controller disposal, and add teardown tests covering repeated failures for both services.
🧹 Nitpick comments (5)
lib/src/models/pit_shift.dart (1)
87-92: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winApply
_dateTimetoupdatedAtfor consistent parsing.
startsAtandendsAtnow tolerate bothDateTimeandStringinputs.updatedAtat Line 91 still usesdata['updatedAt'] as String?. That cast throws aTypeErrorif the backend supplies a non-string value, which would make the wholefromJsoncall fail instead of falling back to the epoch default. RouteupdatedAtthrough the same helper to keep one parsing rule for all time fields.♻️ Proposed refactor
notes: data['notes'] as String?, - updatedAt: - DateTime.tryParse(data['updatedAt'] as String? ?? '') ?? - DateTime.fromMillisecondsSinceEpoch(0, isUtc: true), + updatedAt: + _dateTime(data['updatedAt']) ?? + DateTime.fromMillisecondsSinceEpoch(0, isUtc: 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/models/pit_shift.dart` around lines 87 - 92, Update the updatedAt field in the model’s fromJson construction to use the existing _dateTime helper, matching startsAt and endsAt, while preserving the epoch fallback for invalid or absent values.lib/src/services/desktop_update_service.dart (2)
146-156: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider excluding prerelease tags from the update offer.
Moving to
pub_semveris the right change; it replaces hand-rolled comparison logic with the standard implementation.One behavior note.
pub_semverapplies SemVer precedence, soVersion.parse('1.1.1-beta.1').compareTo(Version.parse('1.1.0')) > 0.checkForUpdatetherefore offers a prerelease tag to every user as an installable update. If you publish prerelease tags on this repository, filter them out.♻️ Proposed refactor
final version = _parseVersion(tagName); + if (version.isPreRelease) { + return null; + }Apply this in
_loadLatestReleaseafter the null check onversion.🤖 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_update_service.dart` around lines 146 - 156, Update _loadLatestRelease after its null check on version to exclude prerelease versions from update offers, using the parsed Version’s prerelease indicator before comparing or returning the release. Keep stable-version update behavior unchanged.
108-122: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winSelect the AppImage explicitly.
The GitHub REST API provides the asset
digestfield with asha256:prefix, so the checksum path is valid. If a release contains multiple AppImage architectures, this helper returns the first one and can install an incompatible binary. Match the target architecture or reject ambiguous releases.🤖 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_update_service.dart` around lines 108 - 122, The _appImageAsset helper currently selects the first .AppImage without verifying architecture. Update its asset selection to match the target architecture, using the project’s existing architecture-detection or naming convention, and reject ambiguous releases when no unique compatible asset can be identified; preserve the existing URL and sha256 digest extraction for the selected asset.test/widget_test.dart (1)
255-260: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the macOS Command shortcut.
The helper sends only
LogicalKeyboardKey.controlLeft. The Ctrl/Cmd navigation contract has noLogicalKeyboardKey.metaLeftassertion. A Command-key regression can pass these tests.Add equivalent forward and backward assertions with
metaLeft.🤖 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/widget_test.dart` around lines 255 - 260, Add macOS Command-key coverage alongside the existing press helper in the widget tests: create equivalent forward and backward navigation assertions that invoke press with LogicalKeyboardKey.metaLeft, while preserving the current controlLeft assertions and helper behavior.test/photo_disk_cache_test.dart (1)
80-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise
PhotoDiskCache.readin the eviction test.Lines 80-89 set file times directly. They do not verify that
readrefreshes the timestamp that_trimuses. IfPhotoDiskCache.readstops callingsetLastModified, this test still passes.Proposed test update
- // Times set explicitly rather than by sleeping between writes: wall-clock - // gaps made this flaky, and the eviction order is the only thing under test. - // 'old' is the most recently read, so it must survive. + // Set deterministic stale times, then use the public read path to mark + // `old` as recently used. final dir = Directory('${base.path}/photos'); - await File('${dir.path}/old').setLastModified(DateTime.now()); + final now = DateTime.now(); + await File( + '${dir.path}/old', + ).setLastModified(now.subtract(const Duration(hours: 2))); await File( '${dir.path}/newer', - ).setLastModified(DateTime.now().subtract(const Duration(hours: 1))); + ).setLastModified(now.subtract(const Duration(hours: 1))); + + await cache.read('old'); await cache.write('pushes-over', bytes(400));🤖 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/photo_disk_cache_test.dart` around lines 80 - 89, Update the eviction test to make the “old” file’s timestamp result from calling PhotoDiskCache.read rather than setting it directly, while retaining the explicit older timestamp for “newer.” Ensure the test still verifies that the read-refreshed file survives _trim when cache.write('pushes-over', bytes(400)) triggers eviction.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/src/services/desktop_launcher_service.dart`:
- Around line 49-52: Update the Exec-path escaping logic in the desktop launcher
service to encode each literal percent in appImagePath as %% before adding the
%U field code, while preserving escaping for $ and backslashes. Add or update
tests covering paths containing percent signs, dollar signs, and backslashes.
In `@lib/src/services/desktop_update_service.dart`:
- Around line 103-105: Update checkForUpdate to distinguish a repository with no
newer release from transport failures: log caught errors, do not convert network
or TimeoutException failures into null, and propagate the failure so
_DesktopUpdateTileState._check can display its existing error message. If
iterating multiple repositories, only rethrow after all repositories fail;
preserve null for a successful check with no release.
In `@lib/src/services/photo_disk_cache.dart`:
- Around line 1-4: Make PhotoDiskCache platform-conditional so Web builds never
resolve the dart:io-dependent implementation: split the native and Web
implementations behind conditional imports or exports, preserving the existing
PhotoDiskCache API while providing a Web-safe implementation. Update the
PhotoService integration to reference this conditional abstraction rather than
importing native cache code directly.
In `@lib/src/services/photo_service.dart`:
- Around line 134-136: Update the upload flow to persist the newly uploaded
photo bytes in the disk cache after the server returns its key. In the upload
method, alongside the existing _remember call, invoke the _diskCache write using
the returned key and photo.bytes, preserving the existing behavior when no disk
cache is configured.
In `@lib/src/services/spectrum_auth_service.dart`:
- Around line 75-78: Make the initialization flow around
_StrategyAppState._retryBootstrap and _authStateSubscription idempotent: prevent
repeated initialize() calls from registering duplicate authStateChanges
listeners, either by caching and reusing the initialization future or by
cancelling the existing subscription before replacing it. Ensure dispose() can
cleanly terminate the active listener and failed bootstrap retries do not leave
earlier listeners active.
In `@lib/src/services/synced_map_image_store.dart`:
- Around line 89-98: Update the cleanup flow around diagramSync.clearKey so it
records any clearKey failure, still performs the local photoService.delete
cleanup when applicable, then rethrows the original pointer-clear error after
cleanup completes. Preserve successful completion when clearing the synchronized
pointer succeeds.
In `@lib/src/services/telemetry_service.dart`:
- Around line 46-62: Update the telemetry consent/settings text and
corresponding privacy documentation to accurately describe the persistent
pseudonymous device identifier generated by _deviceId and stored by
_createDeviceId in SharedPreferences, including its purpose and retention
period; do not refer to this telemetry as only “anonymous usage data.”
In `@lib/src/state/pit_controller_mixin.dart`:
- Around line 47-101: Guard the rollback logic in upsert and delete against
overlapping mutations for the same ID by tracking a per-item revision or
serializing operations per ID. Only restore the previous state when the failed
mutation still owns the current item; otherwise preserve the newer mutation and
its cache state. Add an interleaving test covering two updates to the same ID
where the earlier update fails.
In `@lib/src/ui/borrow_tab.dart`:
- Around line 578-579: Update the date picker bounds in the borrow-tab flows
around firstDate and lastDate (including the corresponding second block) to
derive both endpoints from now, allowing selection through the same calendar
date one year later rather than January 1. Add boundary tests covering the
rolling one-year limits.
In `@lib/src/ui/packing_tab.dart`:
- Around line 197-207: Update the deletion flow around widget.controller.delete
so a failed deletion returns immediately when deleted remains false, keeping the
editor open. Only execute the subsequent photo cleanup and editor-closing logic
after successful deletion.
- Around line 841-861: The save completion flow around widget.onSubmit must
clean up captured photos even when the sheet unmounts or the save is not
committed. Delete all _capturedKeys when committed is false; on success, retain
the claimed _photoRef while deleting unclaimed captures and the replaced
_originalPhotoRef. Remove the broad mounted/committed early return, and guard
only Navigator.of(context).pop() with mounted; add deferred-submit tests
covering both outcomes.
In `@lib/src/ui/settings_tab.dart`:
- Around line 510-529: Prevent the asynchronous isEnabled read in the settings
initialization flow from overwriting a value changed by _toggle: either disable
the switch until the read completes or ignore its callback once a toggle begins.
Preserve the existing toggle persistence and rollback behavior, and add a
deferred isEnabled/setEnabled test covering the race.
In `@test/docs_assets_exist_test.dart`:
- Around line 50-55: Update the unbundled asset filter to match file assets only
by exact equality, while allowing prefix matching exclusively for declared
assets whose path ends with “/”. Adjust the predicate in the referenced/declared
comparison so file names such as docs/guide.md-extra.md do not match
docs/guide.md.
In `@test/support/fake_map_diagram_sync_service.dart`:
- Line 22: Update the fake service’s key storage and the read/write/clear
methods in FakeMapDiagramSyncService to maintain an independent key for each
MapType, ensuring readKey, successful writes, and clears only access the
requested map type and cannot affect other maps.
In `@test/support/photo_test_support.dart`:
- Around line 45-50: Update the authentication guard in the request fake so it
immediately returns a 401 response when token is null, before computing or
comparing the expected Authorization header. Preserve the existing Bearer-token
comparison for non-null tokens.
---
Outside diff comments:
In `@lib/src/services/desktop_borrow_sync_service.dart`:
- Around line 44-78: Update DesktopBorrowSyncService in
lib/src/services/desktop_borrow_sync_service.dart:44-78 and
DesktopPackingSyncService in
lib/src/services/desktop_packing_sync_service.dart:53-83 to add an _disposed
state, expose dispose(), and replace each unconditional async* while (true) loop
with a condition that exits after disposal, including during sustained
listDocuments failures and polling delays. Make service disposal reachable
through the interfaces stored by lib/main.dart, invoke it from the application
lifecycle in lib/src/app.dart alongside controller disposal, and add teardown
tests covering repeated failures for both services.
In `@lib/src/services/photo_service.dart`:
- Around line 121-156: The photo cache must be scoped to the active session so
an in-flight fetch cannot repopulate caches after clearCache(). Update fetch,
clearCache, and the related cache read/write flow to track a session generation
or identity, discard stale in-flight results, and prevent stale writes to both
_cache and _diskCache; ensure later sessions do not receive prior-session bytes
before _send runs. Add a test covering clearCache interleaved with an in-flight
fetch.
---
Nitpick comments:
In `@lib/src/models/pit_shift.dart`:
- Around line 87-92: Update the updatedAt field in the model’s fromJson
construction to use the existing _dateTime helper, matching startsAt and endsAt,
while preserving the epoch fallback for invalid or absent values.
In `@lib/src/services/desktop_update_service.dart`:
- Around line 146-156: Update _loadLatestRelease after its null check on version
to exclude prerelease versions from update offers, using the parsed Version’s
prerelease indicator before comparing or returning the release. Keep
stable-version update behavior unchanged.
- Around line 108-122: The _appImageAsset helper currently selects the first
.AppImage without verifying architecture. Update its asset selection to match
the target architecture, using the project’s existing architecture-detection or
naming convention, and reject ambiguous releases when no unique compatible asset
can be identified; preserve the existing URL and sha256 digest extraction for
the selected asset.
In `@test/photo_disk_cache_test.dart`:
- Around line 80-89: Update the eviction test to make the “old” file’s timestamp
result from calling PhotoDiskCache.read rather than setting it directly, while
retaining the explicit older timestamp for “newer.” Ensure the test still
verifies that the read-refreshed file survives _trim when
cache.write('pushes-over', bytes(400)) triggers eviction.
In `@test/widget_test.dart`:
- Around line 255-260: Add macOS Command-key coverage alongside the existing
press helper in the widget tests: create equivalent forward and backward
navigation assertions that invoke press with LogicalKeyboardKey.metaLeft, while
preserving the current controlLeft assertions and helper behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a27c93b-9aef-4811-9d5d-bb0e6e6233bd
⛔ Files ignored due to path filters (5)
assets/fonts/IBMPlexMono-Medium.ttfis excluded by!**/*.ttfassets/fonts/IBMPlexSans-Bold.ttfis excluded by!**/*.ttfassets/fonts/IBMPlexSans-Regular.ttfis excluded by!**/*.ttfassets/fonts/IBMPlexSans-SemiBold.ttfis excluded by!**/*.ttfpubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (87)
android/app/build.gradle.ktsandroid/app/src/main/AndroidManifest.xmlfirestore.rulesios/Runner/Assets.xcassets/LaunchImage.imageset/README.mdlib/main.dartlib/src/app.dartlib/src/models/inventory_item.dartlib/src/models/packing_record.dartlib/src/models/pit_shift.dartlib/src/models/user_profile.dartlib/src/models/user_role.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_location_sync_service.dartlib/src/services/desktop_packing_sync_service.dartlib/src/services/desktop_pit_shift_sync_service.dartlib/src/services/desktop_polling.dartlib/src/services/desktop_self_update_service.dartlib/src/services/desktop_update_service.dartlib/src/services/desktop_user_role_service.dartlib/src/services/local_only_services.dartlib/src/services/map_image_store.dartlib/src/services/photo_disk_cache.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/services/user_role_service_interface.dartlib/src/state/pit_controller_mixin.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/inventory_tab.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/user_management_screen.dartlinux/runner/my_application.ccmacos/Runner/DebugProfile.entitlementsmacos/Runner/Info.plistmacos/Runner/Release.entitlementspubspec.yamltest/borrow_tab_test.darttest/dark_theme_tokens_test.darttest/desktop_auth_service_test.darttest/desktop_launcher_service_test.darttest/desktop_pit_shift_sync_service_test.darttest/desktop_self_update_service_test.darttest/desktop_update_service_test.darttest/docs_assets_exist_test.darttest/docs_viewer_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_tab_test.darttest/photo_disk_cache_test.darttest/photo_service_test.darttest/pit_shift_test.darttest/schedule_tab_test.darttest/support/fake_map_diagram_sync_service.darttest/support/fake_map_image_store.darttest/support/fake_map_location_sync_service.darttest/support/fake_pit_shift_sync_service.darttest/support/fake_user_role_service.darttest/support/photo_test_support.darttest/synced_map_image_store_test.darttest/telemetry_service_test.darttest/user_management_screen_test.darttest/user_profile_test.darttest/user_role_controller_test.darttest/widget_test.darttool/generate_icons.pywindows/runner/Runner.rcwindows/runner/flutter_window.cpp
| final exec = appImagePath.replaceAllMapped( | ||
| RegExp(r'["`$\\]'), | ||
| (m) => '\\${m[0]}', | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file=$(git ls-files | rg '(^|/)lib/src/services/desktop_launcher_service\.dart$' | head -n 1)
printf '%s\n' "FILE=$file"
wc -l "$file"
sed -n '1,130p' "$file"
printf '%s\n' '--- related symbols and tests ---'
rg -n -C 3 'desktopEntry|appImagePath|APPIMAGE|%U|Exec' . --glob '!build/**' --glob '!dist/**' --glob '!node_modules/**'Repository: Spectrum3847/spectrum-pit
Length of output: 23229
🌐 Web query:
Freedesktop Desktop Entry Specification latest Exec key literal percent sign %% quoted argument field codes
💡 Result:
According to the Freedesktop Desktop Entry Specification, the Exec key handles literal percent signs by using the escape sequence %% [1][2]. Regarding the interaction between field codes and quoted arguments, the specification explicitly states that field codes (such as %f, %u, %F, or %U) must not be used inside a quoted argument [2]. The results of field code expansion when placed inside a quoted argument are defined as undefined behavior [2][3]. To handle complex command lines that might otherwise require field codes within quotes, developers typically use shell wrappers or pass the field codes as separate arguments to a shell command (e.g., sh -c "..." sh %u) [3]. Implementations are required to perform quote removal before expanding field codes and before passing the arguments to the executable [1][2].
Citations:
- 1: https://specifications.freedesktop.org/desktop-entry/1.0/exec-variables.html
- 2: https://specifications.freedesktop.org/desktop-entry/1.3/exec-variables.html
- 3: https://lists.libreplanet.org/archive/html/emacs-diffs/2022-12/msg00722.html
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
# Model the exact character transformations in the Dart source and in the
# proposed replacement. String.replaceAll uses literal replacement strings.
def current(value):
return ''.join(('\\' + c) if c in '"`$\\' else c for c in value)
def proposed(value):
return (value
.replace('\\', '\\\\\\\\') # Dart raw r'\\\\' => four backslashes
.replace('$', '\\\\$') # Dart raw r'\\$' => two backslashes + $
.replace('"', '\\"')
.replace('`', '\\`')
.replace('%', '%%'))
for value in [
'/tmp/App%20.AppImage',
r'/tmp/App$foo\App"Image`',
r'/tmp/App%U.AppImage',
]:
print(f'input: {value!r}')
print(f'current: {current(value)!r}')
print(f'proposed: {proposed(value)!r}')
PY
printf '%s\n' '--- available desktop-entry validators/parsers ---'
command -v desktop-file-validate || true
command -v gio || true
command -v gdbus || trueRepository: Spectrum3847/spectrum-pit
Length of output: 544
🏁 Script executed:
#!/bin/bash
set -eu
url='https://specifications.freedesktop.org/desktop-entry/latest-single/'
if command -v curl >/dev/null 2>&1; then
curl -L --fail --silent --show-error "$url" |
rg -n -C 4 'literal percent|field code|quoted argument|Exec key|%%'
else
printf '%s\n' 'curl is unavailable'
fiRepository: Spectrum3847/spectrum-pit
Length of output: 8578
Escape literal percent signs in Exec.
When appImagePath contains %, encode it as %% before appending the %U field code. Also preserve the required Desktop Entry escaping for $ and backslashes. Add tests for paths containing %, $, and backslashes.
🤖 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_launcher_service.dart` around lines 49 - 52, Update
the Exec-path escaping logic in the desktop launcher service to encode each
literal percent in appImagePath as %% before adding the %U field code, while
preserving escaping for $ and backslashes. Add or update tests covering paths
containing percent signs, dollar signs, and backslashes.
| } catch (_) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The blanket catch reports a failed update check as "up to date".
catch (_) { return null; } swallows every error, including the TimeoutException that TimeoutHttpClient raises and any network failure. checkForUpdate then treats the repository as having no release, skips it, and returns null. The consumer in lib/src/ui/settings_tab.dart maps a null result to "You are on the latest version."
The user therefore sees a success message after a failed check. That branch also makes the existing "Could not check for updates right now." handler in _check unreachable for network errors, because no exception escapes this method. For an update mechanism that carries security fixes, a false "up to date" state is the wrong default.
Distinguish a transport failure from "no newer release", and log the error so the failure is diagnosable.
🐛 Proposed fix
- } catch (_) {
- return null;
- }
+ } on FormatException catch (error) {
+ debugPrint('release metadata for $repository was malformed: $error');
+ return null;
+ } catch (error) {
+ debugPrint('release check for $repository failed: $error');
+ rethrow;
+ }checkForUpdate then propagates the failure, and the existing catch in _DesktopUpdateTileState._check shows "Could not check for updates right now." If you keep more than one entry in _repositories, rethrow only after every repository fails. debugPrint comes from package:flutter/foundation.dart.
📝 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.
| } catch (_) { | |
| return null; | |
| } | |
| } on FormatException catch (error) { | |
| debugPrint('release metadata for $repository was malformed: $error'); | |
| return null; | |
| } catch (error) { | |
| debugPrint('release check for $repository failed: $error'); | |
| rethrow; | |
| } |
🤖 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_update_service.dart` around lines 103 - 105, Update
checkForUpdate to distinguish a repository with no newer release from transport
failures: log caught errors, do not convert network or TimeoutException failures
into null, and propagate the failure so _DesktopUpdateTileState._check can
display its existing error message. If iterating multiple repositories, only
rethrow after all repositories fail; preserve null for a successful check with
no release.
| import 'dart:io'; | ||
|
|
||
| import 'package:flutter/foundation.dart' show Uint8List, debugPrint, kIsWeb; | ||
| import 'package:path_provider/path_provider.dart'; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 2 "import 'dart:io'|class PhotoDiskCache|PhotoDiskCache" lib
rg -n -i -C 2 "flutter build web|run -d chrome|web" README.md pubspec.yaml 2>/dev/null || trueRepository: Spectrum3847/spectrum-pit
Length of output: 4852
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- tracked platform/config files ---'
git ls-files | rg -i '(^|/)(readme|pubspec|analysis_options|web|android|ios|macos|windows|linux)(\.|/|$)' | head -200
printf '%s\n' '--- pubspec.yaml ---'
cat -n pubspec.yaml | sed -n '1,180p'
printf '%s\n' '--- photo_disk_cache.dart ---'
cat -n lib/src/services/photo_disk_cache.dart
printf '%s\n' '--- photo_service.dart imports and constructor ---'
cat -n lib/src/services/photo_service.dart | sed -n '1,90p'
printf '%s\n' '--- main.dart context ---'
cat -n lib/main.dart | sed -n '145,175p'
printf '%s\n' '--- platform guards and conditional imports ---'
rg -n -C 3 "kIsWeb|Platform\.|if \(dart\.library|if \(dart\.|conditional import|conditional export|dart:io" lib test pubspec.yaml README.md 2>/dev/null | head -300Repository: Spectrum3847/spectrum-pit
Length of output: 39706
Hide the native cache behind a conditional implementation for Web builds.
Web is an application target, but PhotoDiskCache imports dart:io through PhotoService. A runtime kIsWeb check cannot prevent browser compilation from resolving dart:io.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/src/services/photo_disk_cache.dart` around lines 1 - 4, Make
PhotoDiskCache platform-conditional so Web builds never resolve the
dart:io-dependent implementation: split the native and Web implementations
behind conditional imports or exports, preserving the existing PhotoDiskCache
API while providing a Web-safe implementation. Update the PhotoService
integration to reference this conditional abstraction rather than importing
native cache code directly.
| final disk = _diskCache; | ||
| if (disk != null) unawaited(disk.write(key, bytes)); | ||
| return bytes; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Persist newly uploaded photo bytes.
The disk tier only stores GET responses. upload only calls _remember, so a newly captured photo is unavailable after an app restart until another fetch succeeds. Store photo.bytes in the disk cache after the upload returns its key.
Proposed fix
_remember(key, photo.bytes);
+ final disk = _diskCache;
+ if (disk != null) unawaited(disk.write(key, photo.bytes));
return key;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/src/services/photo_service.dart` around lines 134 - 136, Update the
upload flow to persist the newly uploaded photo bytes in the disk cache after
the server returns its key. In the upload method, alongside the existing
_remember call, invoke the _diskCache write using the returned key and
photo.bytes, preserving the existing behavior when no disk cache is configured.
| if (!kIsWeb) { | ||
| await _googleSignIn.initialize(); | ||
| } | ||
| _authStateSubscription = _appAuth.authStateChanges().listen((user) { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make initialize() idempotent before startup retry.
_StrategyAppState._retryBootstrap() calls initialize() again after any bootstrap failure. If the first call already subscribed, Line 78 overwrites _authStateSubscription without cancelling the old listener. Auth events then emit twice, and dispose() leaves the old listener active.
Cache the initialization future, or cancel and replace the existing subscription before registering a new listener.
🤖 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/spectrum_auth_service.dart` around lines 75 - 78, Make the
initialization flow around _StrategyAppState._retryBootstrap and
_authStateSubscription idempotent: prevent repeated initialize() calls from
registering duplicate authStateChanges listeners, either by caching and reusing
the initialization future or by cancelling the existing subscription before
replacing it. Ensure dispose() can cleanly terminate the active listener and
failed bootstrap retries do not leave earlier listeners active.
| } finally { | ||
| _saving = false; | ||
| } | ||
| if (!mounted || !committed) return; | ||
| _saved = true; | ||
|
|
||
| final claimed = _photoRef; | ||
| for (final key in _capturedKeys) { | ||
| if (key != claimed) { | ||
| try { | ||
| await widget.photoService.delete(key); | ||
| } catch (_) {} | ||
| } | ||
| } | ||
| final original = _originalPhotoRef; | ||
| if (original != null && original != claimed) { | ||
| try { | ||
| await widget.photoService.delete(original); | ||
| } catch (_) {} | ||
| } | ||
| if (mounted) Navigator.of(context).pop(); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate symbols ---'
ast-grep outline lib/src/ui/packing_tab.dart --match 'PackingTab' --view expanded || true
printf '%s\n' '--- lifecycle/save/cleanup references ---'
rg -n -C 5 '_saving|_capturedKeys|_photoRef|_originalPhotoRef|onSubmit|dispose|committed|delete\\(' lib/src/ui/packing_tab.dart
printf '%s\n' '--- related tests ---'
rg -n -C 3 'PackingTab|capturedKeys|photoRef|onSubmit|photoService|delete\\(' test lib 2>/dev/null | head -n 500Repository: Spectrum3847/spectrum-pit
Length of output: 419
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file symbols ---'
ast-grep outline lib/src/ui/packing_tab.dart --match 'PackingTab' --view expanded || true
printf '%s\n' '--- lifecycle, save, and cleanup references ---'
rg -n -C 5 '_saving|_capturedKeys|_photoRef|_originalPhotoRef|onSubmit|dispose|committed|delete\\(' lib/src/ui/packing_tab.dart
printf '%s\n' '--- related tests and usages ---'
rg -n -C 3 'PackingTab|capturedKeys|photoRef|onSubmit|photoService|delete\\(' test lib 2>/dev/null | head -n 500Repository: Spectrum3847/spectrum-pit
Length of output: 427
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file size ---'
wc -l lib/src/ui/packing_tab.dart
printf '%s\n' '--- relevant identifiers ---'
for term in _saving _capturedKeys _photoRef _originalPhotoRef onSubmit dispose committed delete; do
printf '\n[%s]\n' "$term"
rg -n -F -C 4 "$term" lib/src/ui/packing_tab.dart || true
done
printf '%s\n' '--- target section ---'
sed -n '650,900p' lib/src/ui/packing_tab.dart
printf '%s\n' '--- test files ---'
git ls-files | rg '(^|/)(test|tests)/|packing_tab'Repository: Spectrum3847/spectrum-pit
Length of output: 18993
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- sheet-opening and submit callback ---'
sed -n '130,225p' lib/src/ui/packing_tab.dart
printf '%s\n' '--- packing tab test outline and relevant matches ---'
ast-grep outline test/packing_tab_test.dart --view compact || true
rg -n -F -C 6 'onSubmit' test/packing_tab_test.dart
rg -n -F -C 6 'PhotoService' test/packing_tab_test.dart
rg -n -F -C 6 'delete' test/packing_tab_test.dart
printf '%s\n' '--- photo service contract ---'
rg -n -F -C 8 'abstract class PhotoService' lib test
rg -n -F -C 8 'class PhotoService' lib/src testRepository: Spectrum3847/spectrum-pit
Length of output: 3400
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- packing tab test matches ---'
rg -n -F -C 8 'onSubmit' test/packing_tab_test.dart || true
rg -n -F -C 8 'PhotoService' test/packing_tab_test.dart || true
rg -n -F -C 8 'delete' test/packing_tab_test.dart || true
rg -n -F -C 5 'showModalBottomSheet' test/packing_tab_test.dart || true
printf '%s\n' '--- photo service definitions ---'
rg -n -F -C 10 'abstract class PhotoService' lib test || true
rg -n -F -C 10 'class PhotoService' lib/src test || true
printf '%s\n' '--- photo support helpers ---'
rg -n -F -C 8 'FakePhoto' test lib || true
rg -n -F -C 8 'delete(' test/support/photo_test_support.dart lib/src/services lib/src 2>/dev/null | head -n 400 || trueRepository: Spectrum3847/spectrum-pit
Length of output: 35263
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
source = Path("lib/src/ui/packing_tab.dart").read_text()
required = [
"if (!_saved && !_saving) {",
"committed = await widget.onSubmit(",
"if (!mounted || !committed) return;",
"for (final key in _capturedKeys) {",
"if (key != claimed) {",
]
for fragment in required:
assert fragment in source, fragment
def current_cleanup(mounted, committed, captured, claimed, original):
# This mirrors the control flow in _save after onSubmit resolves.
if not mounted or not committed:
return []
deleted = [key for key in captured if key != claimed]
if original is not None and original != claimed:
deleted.append(original)
return deleted
captured = ["new-1", "new-2"]
assert current_cleanup(True, False, captured, "new-2", "old") == []
assert current_cleanup(False, False, captured, "new-2", "old") == []
assert current_cleanup(False, True, captured, "new-2", "old") == []
assert current_cleanup(True, True, captured, "new-2", "old") == ["new-1", "old"]
print("current _save flow:")
print(" mounted + failure: no cleanup")
print(" unmounted + failure: no cleanup")
print(" unmounted + success: no cleanup")
print(" mounted + success: delete unclaimed capture and replaced original")
PY
printf '%s\n' '--- exact PhotoService.delete behavior ---'
sed -n '133,154p' lib/src/services/photo_service.dart
printf '%s\n' '--- remaining packing tab tests ---'
sed -n '290,390p' test/packing_tab_test.dartRepository: Spectrum3847/spectrum-pit
Length of output: 2950
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
source = Path("lib/src/ui/packing_tab.dart").read_text()
required = [
"if (!_saved && !_saving) {",
"committed = await widget.onSubmit(",
"if (!mounted || !committed) return;",
"for (final key in _capturedKeys) {",
"if (key != claimed) {",
]
for fragment in required:
assert fragment in source, fragment
def current_cleanup(mounted, committed, captured, claimed, original):
if not mounted or not committed:
return []
deleted = [key for key in captured if key != claimed]
if original is not None and original != claimed:
deleted.append(original)
return deleted
captured = ["new-1", "new-2"]
assert current_cleanup(True, False, captured, "new-2", "old") == []
assert current_cleanup(False, False, captured, "new-2", "old") == []
assert current_cleanup(False, True, captured, "new-2", "old") == []
assert current_cleanup(True, True, captured, "new-2", "old") == ["new-1", "old"]
print("current _save flow:")
print(" mounted + failure: no cleanup")
print(" unmounted + failure: no cleanup")
print(" unmounted + success: no cleanup")
print(" mounted + success: delete unclaimed capture and replaced original")
PY
printf '%s\n' '--- exact PhotoService.delete behavior ---'
sed -n '133,154p' lib/src/services/photo_service.dart
printf '%s\n' '--- remaining packing tab tests ---'
sed -n '290,390p' test/packing_tab_test.dartRepository: Spectrum3847/spectrum-pit
Length of output: 2950
Run photo cleanup after an in-flight save completes.
When the sheet unmounts during widget.onSubmit, _saving prevents dispose() from cleaning _capturedKeys, and the mounted guard skips post-save cleanup. Delete all captured keys when committed is false. On success, delete unclaimed captures and the replaced _originalPhotoRef. Guard only Navigator.of(context).pop() with mounted. Add deferred-submit tests for both outcomes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/src/ui/packing_tab.dart` around lines 841 - 861, The save completion flow
around widget.onSubmit must clean up captured photos even when the sheet
unmounts or the save is not committed. Delete all _capturedKeys when committed
is false; on success, retain the claimed _photoRef while deleting unclaimed
captures and the replaced _originalPhotoRef. Remove the broad mounted/committed
early return, and guard only Navigator.of(context).pop() with mounted; add
deferred-submit tests covering both outcomes.
| _service | ||
| .isEnabled() | ||
| .then((value) { | ||
| if (mounted) setState(() => _enabled = value); | ||
| }) | ||
| .catchError((Object error) { | ||
| debugPrint('Telemetry preference read failed: $error'); | ||
| }); | ||
| } | ||
|
|
||
| Future<void> _toggle(bool value) async { | ||
| if (_busy) return; | ||
| _busy = true; | ||
| final previous = _enabled; | ||
| setState(() => _enabled = value); | ||
| await _service.setEnabled(value); | ||
| try { | ||
| await _service.setEnabled(value); | ||
| } catch (error) { | ||
| debugPrint('Telemetry preference write failed: $error'); | ||
| if (mounted) setState(() => _enabled = previous); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- file outline ---'
ast-grep outline lib/src/ui/settings_tab.dart --view compact
printf '%s\n' '--- target section ---'
sed -n '450,560p' lib/src/ui/settings_tab.dart
printf '%s\n' '--- related symbols and tests ---'
rg -n --glob '*.dart' '_enabled|_toggle|isEnabled\\(|setEnabled\\(' lib test . 2>/dev/null | head -240Repository: Spectrum3847/spectrum-pit
Length of output: 335
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- file outline ---'
ast-grep outline lib/src/ui/settings_tab.dart --view signatures
printf '%s\n' '--- target section ---'
sed -n '450,560p' lib/src/ui/settings_tab.dart
printf '%s\n' '--- related symbols and tests ---'
rg -n --glob '*.dart' '_enabled|_toggle|isEnabled\\(|setEnabled\\(' lib test . 2>/dev/null | head -240Repository: Spectrum3847/spectrum-pit
Length of output: 3915
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- telemetry declarations and implementations ---'
rg -n -C 8 --glob '*.dart' 'class TelemetryService|abstract class TelemetryService|TelemetryService|isEnabled|setEnabled' .
printf '%s\n' '--- settings widget tests ---'
find . -type f \( -name '*test.dart' -o -name '*widget_test.dart' \) -print | sort | head -200
printf '%s\n' '--- project test configuration ---'
find . -maxdepth 3 -type f \( -name 'pubspec.yaml' -o -name 'analysis_options.yaml' \) -print -exec sed -n '1,180p' {} \;Repository: Spectrum3847/spectrum-pit
Length of output: 32055
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
from collections import deque
source = Path("lib/src/ui/settings_tab.dart").read_text()
required = [
".isEnabled()",
"if (mounted) setState(() => _enabled = value);",
"Future<void> _toggle(bool value) async",
"setState(() => _enabled = value);",
"await _service.setEnabled(value);",
"onChanged: _toggle,",
]
missing = [item for item in required if item not in source]
if missing:
raise SystemExit(f"missing expected source fragments: {missing}")
# Model the two independent completions. The read captures the persisted
# value before the write completes, then its UI callback runs after the toggle.
enabled = True
persisted = False
events = deque([
("toggle", True),
("read_complete", persisted),
("write_complete", True),
])
for event, value in events:
if event == "toggle":
enabled = value
elif event == "read_complete":
enabled = value
elif event == "write_complete":
persisted = value
print({
"source_fragments_present": True,
"persisted_after_write": persisted,
"ui_after_late_read": enabled,
"ui_differs_from_persisted": enabled != persisted,
})
assert persisted is True
assert enabled is False
assert enabled != persisted
PY
printf '%s\n' '--- settings references in tests ---'
rg -n --glob '*.dart' 'SettingsTab|_TelemetryTile|Share anonymous usage data' test lib || trueRepository: Spectrum3847/spectrum-pit
Length of output: 1335
Prevent the initial read from overwriting a user toggle.
If isEnabled() completes after _toggle() starts, its callback can restore the old value after setEnabled(value) persists the new value. Disable the switch until the initial read completes, or ignore the read after a toggle starts. Add a deferred isEnabled()/setEnabled() test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/src/ui/settings_tab.dart` around lines 510 - 529, Prevent the
asynchronous isEnabled read in the settings initialization flow from overwriting
a value changed by _toggle: either disable the switch until the read completes
or ignore its callback once a toggle begins. Preserve the existing toggle
persistence and rollback behavior, and add a deferred isEnabled/setEnabled test
covering the race.
| final unbundled = | ||
| referenced | ||
| .where( | ||
| (p) => | ||
| !declared.any((asset) => p == asset || p.startsWith(asset)), | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file="test/docs_assets_exist_test.dart"
printf '%s\n' '--- target file outline ---'
ast-grep outline "$file" --lang dart 2>/dev/null || true
printf '%s\n' '--- target lines ---'
sed -n '1,120p' "$file"
printf '%s\n' '--- related symbols and fixtures ---'
rg -n -S "docs_assets_exist|declared|referenced|unbundled|asset.endsWith|startsWith" . \
-g '*.dart' -g '*.md' -g '*.yaml' -g '*.yml' | head -200Repository: Spectrum3847/spectrum-pit
Length of output: 4561
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- asset configuration ---'
rg -n -A25 -B5 '^[[:space:]]*assets:' pubspec.yaml
printf '%s\n' '--- referenced and declared documentation paths ---'
python3 - <<'PY'
from pathlib import Path
import re
pattern = re.compile(r"""['"](docs/[\w./-]+\.md)['"]""")
referenced = set()
for path in Path("lib").rglob("*.dart"):
referenced.update(m.group(1) for m in pattern.finditer(path.read_text()))
declared = []
in_assets = False
for line in Path("pubspec.yaml").read_text().splitlines():
trimmed = line.strip()
if trimmed == "assets:":
in_assets = True
elif in_assets:
if trimmed.startswith("- "):
declared.append(trimmed[2:].strip())
elif trimmed and not trimmed.startswith("#"):
in_assets = False
print("referenced:", sorted(referenced))
print("declared:", declared)
print("current unbundled:", sorted(
p for p in referenced
if not any(p == asset or p.startswith(asset) for asset in declared)
))
print("false-positive candidates:", sorted(
(p, asset) for p in referenced for asset in declared
if p != asset and p.startswith(asset) and not asset.endswith("/")
))
PY
printf '%s\n' '--- documentation files ---'
find docs -type f -name '*.md' -print | sortRepository: Spectrum3847/spectrum-pit
Length of output: 1292
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
def covered_current(path, assets):
return any(path == asset or path.startswith(asset) for asset in assets)
def covered_proposed(path, assets):
return any(
path == asset or (asset.endswith("/") and path.startswith(asset))
for asset in assets
)
cases = [
("docs/guide.md", ["docs/guide.md"]),
("docs/guide.md-extra.md", ["docs/guide.md"]),
("docs/guide.md", ["docs/"]),
("docs/guide.md-extra.md", ["docs/"]),
]
for path, assets in cases:
print(path, assets, covered_current(path, assets), covered_proposed(path, assets))
PYRepository: Spectrum3847/spectrum-pit
Length of output: 334
Restrict prefix matching to directory assets. Use exact equality for file assets, and use startsWith only when asset.endsWith('/'). Otherwise, docs/guide.md-extra.md can match the declared file docs/guide.md.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/docs_assets_exist_test.dart` around lines 50 - 55, Update the unbundled
asset filter to match file assets only by exact equality, while allowing prefix
matching exclusively for declared assets whose path ends with “/”. Adjust the
predicate in the referenced/declared comparison so file names such as
docs/guide.md-extra.md do not match docs/guide.md.
| _clearFailure = clearFailure; | ||
|
|
||
| final String? _readKeyValue; | ||
| String? _readKeyValue; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Keep fake diagram keys isolated by MapType.
readKey ignores mapType, and successful writes and clears mutate one shared value. A lab write can change a pit read. A lab clear can remove the pit key. This fake can hide cross-map pointer isolation defects in SyncedMapImageStore.
Store a separate key for each MapType.
Also applies to: 49-62
🤖 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_diagram_sync_service.dart` at line 22, Update the fake
service’s key storage and the read/write/clear methods in
FakeMapDiagramSyncService to maintain an independent key for each MapType,
ensuring readKey, successful writes, and clears only access the requested map
type and cannot affect other maps.
| // The Worker authenticates every request with the caller's Bearer token; | ||
| // simulate that gate so an unauthenticated request cannot hit the | ||
| // storage logic. | ||
| final expected = token == null ? null : 'Bearer $token'; | ||
| if (request.headers['Authorization'] != expected) { | ||
| return http.Response('{"error":"Unauthorized"}', 401); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject requests when the fake has no token.
When token is null, expected is also null. A missing Authorization header then passes the comparison and reaches the fake storage logic. This contradicts the stated authentication contract and can hide a missing-header regression.
Return 401 when token == null before comparing the header.
Proposed fix
- final expected = token == null ? null : 'Bearer $token';
- if (request.headers['Authorization'] != expected) {
+ if (
+ token == null ||
+ request.headers['Authorization'] != 'Bearer $token'
+ ) {
return http.Response('{"error":"Unauthorized"}', 401);
}📝 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.
| // The Worker authenticates every request with the caller's Bearer token; | |
| // simulate that gate so an unauthenticated request cannot hit the | |
| // storage logic. | |
| final expected = token == null ? null : 'Bearer $token'; | |
| if (request.headers['Authorization'] != expected) { | |
| return http.Response('{"error":"Unauthorized"}', 401); | |
| // The Worker authenticates every request with the caller's Bearer token; | |
| // simulate that gate so an unauthenticated request cannot hit the | |
| // storage logic. | |
| if ( | |
| token == null || | |
| request.headers['Authorization'] != 'Bearer $token' | |
| ) { | |
| return http.Response('{"error":"Unauthorized"}', 401); |
🤖 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 45 - 50, Update the
authentication guard in the request fake so it immediately returns a 401
response when token is null, before computing or comparing the expected
Authorization header. Preserve the existing Bearer-token comparison for non-null
tokens.
2 participants
Pit photos now survive a relaunch. They are cached on the device, so opening the
packing list on venue wifi no longer refetches every image, and signing out wipes
them so a shared tablet does not keep the previous user's photos.
On desktop you can move between tabs from the keyboard, with Ctrl or Cmd and the
bracket keys.
Usage data is now on by default, matching Spectrum Strategy. It stays anonymous
(app version, platform, and which tabs get opened) and Settings has a switch to
turn it off.
Fixes: the startup "Try again" button can now actually recover from a failed
launch, the security headers that were reporting into a dead endpoint are gone,
and a map image no longer leaks memory while its size is read.
Downloads now live on the public spectrum-pit repository. If you install through
AltStore your existing source keeps working; it picks up the new location on its
next refresh.
Summary by CodeRabbit
New Features
Bug Fixes