Sync v1.2.0 - #3
Conversation
📝 WalkthroughWalkthroughThe PR configures automated reviews and installation documentation, adds desktop authentication cleanup with user-scoped Firestore caching, hardens service and mutation error handling, and adds keyboard shortcuts, accessibility semantics, and large-text layout coverage across Flutter editors. ChangesPlatform, service, and editor updates
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/src/state/pit_controller_mixin.dart (1)
56-71: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winOrder server writes per item ID.
_pitMutationsonly disables stale local rollback;upsertanddeletestill start remote writes concurrently and the sync services always complete deletes and last upserts. If an updated upsert for an item completes after a delete, the server can keep restoring the deleted item. Queue writes per item ID, or include/use a revision the server rejects when stale.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/state/pit_controller_mixin.dart` around lines 56 - 71, Serialize remote writes per item ID in the mutation flow around _pitBeginMutation, pitUpsertRemote, and the corresponding delete operation, ensuring each item’s upsert/delete executes only after its prior write completes. Preserve existing stale-mutation rollback behavior while preventing an older write from completing after a newer operation; use the existing per-item mutation state or a dedicated per-ID queue rather than global serialization.
🧹 Nitpick comments (9)
lib/src/services/desktop_firestore_cache_io.dart (1)
16-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
currentUidclosure here is dead.
clearForUid(uid)calls_cacheFor(uid)directly and never readscurrentUid. ThecurrentUid: () => uidargument at line 21 has no effect. The code reads as if the current-user state matters for the clear, which it does not.Consider exposing a static or top-level clear on
UserScopedFirestoreCachethat takes only the root and the uid, so the caller does not have to supply a meaningless closure.🤖 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_firestore_cache_io.dart` around lines 16 - 23, Remove the unused currentUid dependency from clearDesktopFirestoreCacheFor by exposing and calling a static or top-level UserScopedFirestoreCache clear operation that accepts only root and uid. Preserve the existing cache-root lookup and clearForUid behavior without constructing the instance with a meaningless closure.lib/src/services/desktop_auth_service.dart (3)
181-193: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
_runTeardowndoes not chain onto a pending teardown.
_ensureAuthStateListenerat lines 82-93 chains each setup onto the previous one._runTeardowndoes not use the same pattern. It overwrites_teardownat line 191 without awaiting the previous value.Two concurrent
signOut()calls reach this path.signOut()does not await_teardownfirst, unlikesignIn()at line 140. Both calls then run_forgetStoredSession()and_endSession(uid), soonSessionEndedfires twice for the same uid. The first teardown to finish also resets_endingSessiontofalseat line 188 while the second is still running, which reopens the_handleSessionRevokedguard early.
clearDesktopFirestoreCacheForis idempotent, so the current impact is a duplicate callback rather than data loss. Chaining makes the behavior match the listener-setup path.♻️ Suggested change
Future<void> _runTeardown(String? uid) { _endingSession = true; + final previous = _teardown; final done = () async { + await previous; try { await _forgetStoredSession(); if (uid != null) await _endSession(uid); } finally { _endingSession = false; } }(); _teardown = done; return done; }🤖 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 181 - 193, Update _runTeardown to await and chain after the existing _teardown promise before running _forgetStoredSession and _endSession. Preserve the serialized teardown ordering and ensure _endingSession remains true until the entire queued teardown sequence completes, preventing duplicate session-ending callbacks.
96-136: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd diagnostics to the silent catch at line 135.
The validation and cleanup logic is correct and matches the tests. One gap: the outer
catch (_) {}discards every failure from_prefsLoader(),_session.restore(), and_endSession(). A restore that fails for a non-network reason leaves the app signed out with no signal about the cause.
lib/main.dartline 70 already usesdebugPrintfor the equivalent Firebase bootstrap failure. Consider matching that.♻️ Suggested change
- } catch (_) {} + } catch (error) { + debugPrint('Desktop session restore failed: $error'); + }Note that line 130 re-reads
payload['uid']. The localuidfrom line 110 is already a validated non-emptyStringat that point and can be used directly.🤖 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 96 - 136, Update initialize() so the outer catch logs the caught failure with debugPrint, matching the existing Firebase bootstrap diagnostics in main.dart, while preserving the current silent-recovery behavior. In the restore-failure cleanup branch, reuse the already validated local uid instead of reading payload['uid'] again when calling _endSession.
82-93: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider isolating errors retained by
_listenerSetup.
_listenerSetupstores the setup future permanently. Ifcancel()orlisten()throws, the stored future completes with an error. Every laterawait _listenerSetupthen rethrows. Line 210 indispose()is the important one: the rethrow skips_controller.close()and_session.close(), so the stream controller and the session are never released.The chained
await previousat line 85 has the same problem. One failed setup poisons all laterinitialize()calls.♻️ Suggested guard
Future<void> _ensureAuthStateListener() { final previous = _listenerSetup; final next = () async { - await previous; - await _authStateSub?.cancel(); - _authStateSub = _session.authStateChanges.listen((user) { - if (user == null) unawaited(_handleSessionRevoked()); - }); + try { + await previous; + await _authStateSub?.cancel(); + _authStateSub = _session.authStateChanges.listen((user) { + if (user == null) unawaited(_handleSessionRevoked()); + }); + } catch (_) {} }(); _listenerSetup = next; return next; }🤖 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 82 - 93, Update _ensureAuthStateListener so failures from cancel() or listen() do not permanently poison _listenerSetup or the await previous chain. Isolate or recover setup errors before storing the future, while preserving serialized listener setup; ensure later initialize() calls and dispose() can continue to close _controller and _session even after a setup failure.test/user_scoped_firestore_cache_test.dart (1)
51-58: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider extending the rejected-uid cases.
The test covers the POSIX traversal form
'../elsewhere'. The allowlist regex in_safeUidalso rejects the Windows formr'..\elsewhere', an absolute path such as'/etc/passwd', a drive-qualified path such asr'C:\temp', the empty string, and a uid longer than 64 characters.Because this regex is the security boundary for the cache directory, a table of rejected inputs guards against a future relaxation of the pattern.
♻️ Suggested change
- test('a uid that could escape the root gets no cache', () async { - final subject = cache(); - uid = '../elsewhere'; - await subject.write('doc/teams/1234', '{"a":1}'); - - expect(await subject.read('doc/teams/1234'), isNull); - expect(root.listSync(), isEmpty); - }); + for (final bad in <String>[ + '../elsewhere', + r'..\elsewhere', + '/etc/passwd', + r'C:\temp', + '', + 'a' * 65, + ]) { + test('a uid that could escape the root gets no cache: "$bad"', () async { + final subject = cache(); + uid = bad; + await subject.write('doc/teams/1234', '{"a":1}'); + + expect(await subject.read('doc/teams/1234'), isNull); + expect(root.listSync(), isEmpty); + }); + }🤖 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_scoped_firestore_cache_test.dart` around lines 51 - 58, Extend the cache security test around the existing “uid that could escape the root” case into a table-driven test covering Windows traversal, absolute and drive-qualified paths, the empty uid, and a uid exceeding 64 characters. For every rejected uid, assert that writing produces no readable cache entry and leaves root.listSync() empty, preserving the existing '../elsewhere' coverage.test/desktop_auth_service_test.dart (1)
287-295: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
pumpEventQueue()over a single zero-duration delay.Lines 289 and 343 flush exactly one microtask turn before asserting. The teardown they wait for is a chain: the
authStateChangeslistener calls_handleSessionRevoked, which awaits_runTeardown, which awaits_forgetStoredSession(aSharedPreferencesload plus a remove) and then_endSession.A single
Future<void>.delayed(Duration.zero)is not guaranteed to drain that chain. The risk cuts both ways. The assertion at line 346 checks that a duplicate teardown did not happen, so an under-drained queue makes that test pass even if the dedup guard is broken.
pumpEventQueue()fromflutter_testdrains the queue until it is empty.♻️ Suggested change
- await Future<void>.delayed(Duration.zero); + await pumpEventQueue();The same applies to line 308.
Also applies to: 341-347
🤖 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/desktop_auth_service_test.dart` around lines 287 - 295, Replace the single zero-duration delays in the affected auth teardown tests, including the checks around the authStateChanges listener and duplicate teardown assertion, with Flutter’s pumpEventQueue(). Ensure the queue is fully drained before asserting session state, endedFor, or stored preferences, while preserving the existing assertions and test behavior..pr_agent.toml (3)
58-58: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winKeep
pubspec.lockin automated review scope.
.pr_agent.tomlignores both the explicitpubspec.lockentry and the broad**/*.lockglob. Forfirestore_client,pubspec.yamlpoints to a Git source whilepubspec.lockrecords the resolved Git commit underpackages.firestore_client.description. Removepubspec.lockfrom this ignore list or narrow the lockfile ignores so application Git dependency resolvers stay in the diff.🤖 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 @.pr_agent.toml at line 58, Update the ignore configuration in .pr_agent.toml to stop excluding pubspec.lock, removing its explicit entry and adjusting the broad **/*.lock rule as needed so application lockfiles containing Git dependency resolutions remain in automated review scope.
9-20: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse a distinct fallback model for review availability.
fallback_modelspoints to the sameopenrouter/openrouter/freeidentifier used bymodel, so PR-Agent has no separate secondary model if the router fails. Pin a specific primary model and use a different supported fallback, or remove the duplicate fallback if free-router routing is intentional and document the failure mode.🤖 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 @.pr_agent.toml around lines 9 - 20, Update the PR-Agent model configuration so fallback_models does not duplicate the primary model identifier. Either pin model to a specific supported OpenRouter model and configure a distinct supported fallback, or remove the duplicate fallback when retaining free-router routing and document the expected failure behavior.
22-30: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove PR-Agent automation flags to supported sections.
Put GitHub Action automations under
[github_action_config]: useauto_review,auto_describe,auto_improve, andpr_actionsfor ready/draft behavior. Putignore_bot_pr = trueunder[github]instead ofignore_bot_prsin[config], and removerequire_ready_for_reviewunless a PR-Agent version supports that exact 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 @.pr_agent.toml around lines 22 - 30, Update the PR-Agent configuration by moving the automation settings into [github_action_config] using auto_review, auto_describe, auto_improve, and pr_actions for ready/draft behavior. Move the bot-ignore setting to [github] as ignore_bot, and remove unsupported require_ready_for_review unless the installed PR-Agent version explicitly supports it.
🤖 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_auth_service.dart`:
- Around line 164-171: Update signOut() to guarantee teardown runs even when
_session.signOut() throws, using the existing _runTeardown(departingUid) flow so
_endingSession is reset, persisted session data is removed, and signedOut is
emitted. Add a test covering a SocketException from _session.signOut() that
verifies the signed-out state, onSessionEnded callback, and persisted-key
removal.
In `@lib/src/services/photo_service.dart`:
- Around line 112-113: Update the disk-cache write path in the photo service to
serialize writes with deletion: do not fire-and-forget PhotoDiskCache.write.
Await it and coordinate any subsequent remove/delete so a delayed write cannot
recreate a deleted key, using per-key operation queuing if needed. Add a
regression test that delays write, deletes the same key, and verifies no disk
entry remains.
In `@lib/src/widgets/keyboard_shortcuts.dart`:
- Around line 27-50: Integrate HorizontalStepShortcuts into the relevant desktop
screen widgets, wrapping their content and supplying onPrevious and onNext
callbacks for the screen’s horizontal navigation. Ensure the callbacks are wired
before relying on the existing arrow-key bindings, while preserving the current
screen content behavior.
In `@README.md`:
- Line 13: Update the release artifact documentation around the release links
and platform installation instructions to publish a release-generated SHA-256
manifest and direct users to verify downloaded files against it before bypassing
SmartScreen, Gatekeeper, or other unsigned-artifact warnings.
---
Outside diff comments:
In `@lib/src/state/pit_controller_mixin.dart`:
- Around line 56-71: Serialize remote writes per item ID in the mutation flow
around _pitBeginMutation, pitUpsertRemote, and the corresponding delete
operation, ensuring each item’s upsert/delete executes only after its prior
write completes. Preserve existing stale-mutation rollback behavior while
preventing an older write from completing after a newer operation; use the
existing per-item mutation state or a dedicated per-ID queue rather than global
serialization.
---
Nitpick comments:
In @.pr_agent.toml:
- Line 58: Update the ignore configuration in .pr_agent.toml to stop excluding
pubspec.lock, removing its explicit entry and adjusting the broad **/*.lock rule
as needed so application lockfiles containing Git dependency resolutions remain
in automated review scope.
- Around line 9-20: Update the PR-Agent model configuration so fallback_models
does not duplicate the primary model identifier. Either pin model to a specific
supported OpenRouter model and configure a distinct supported fallback, or
remove the duplicate fallback when retaining free-router routing and document
the expected failure behavior.
- Around line 22-30: Update the PR-Agent configuration by moving the automation
settings into [github_action_config] using auto_review, auto_describe,
auto_improve, and pr_actions for ready/draft behavior. Move the bot-ignore
setting to [github] as ignore_bot, and remove unsupported
require_ready_for_review unless the installed PR-Agent version explicitly
supports it.
In `@lib/src/services/desktop_auth_service.dart`:
- Around line 181-193: Update _runTeardown to await and chain after the existing
_teardown promise before running _forgetStoredSession and _endSession. Preserve
the serialized teardown ordering and ensure _endingSession remains true until
the entire queued teardown sequence completes, preventing duplicate
session-ending callbacks.
- Around line 96-136: Update initialize() so the outer catch logs the caught
failure with debugPrint, matching the existing Firebase bootstrap diagnostics in
main.dart, while preserving the current silent-recovery behavior. In the
restore-failure cleanup branch, reuse the already validated local uid instead of
reading payload['uid'] again when calling _endSession.
- Around line 82-93: Update _ensureAuthStateListener so failures from cancel()
or listen() do not permanently poison _listenerSetup or the await previous
chain. Isolate or recover setup errors before storing the future, while
preserving serialized listener setup; ensure later initialize() calls and
dispose() can continue to close _controller and _session even after a setup
failure.
In `@lib/src/services/desktop_firestore_cache_io.dart`:
- Around line 16-23: Remove the unused currentUid dependency from
clearDesktopFirestoreCacheFor by exposing and calling a static or top-level
UserScopedFirestoreCache clear operation that accepts only root and uid.
Preserve the existing cache-root lookup and clearForUid behavior without
constructing the instance with a meaningless closure.
In `@test/desktop_auth_service_test.dart`:
- Around line 287-295: Replace the single zero-duration delays in the affected
auth teardown tests, including the checks around the authStateChanges listener
and duplicate teardown assertion, with Flutter’s pumpEventQueue(). Ensure the
queue is fully drained before asserting session state, endedFor, or stored
preferences, while preserving the existing assertions and test behavior.
In `@test/user_scoped_firestore_cache_test.dart`:
- Around line 51-58: Extend the cache security test around the existing “uid
that could escape the root” case into a table-driven test covering Windows
traversal, absolute and drive-qualified paths, the empty uid, and a uid
exceeding 64 characters. For every rejected uid, assert that writing produces no
readable cache entry and leaves root.listSync() empty, preserving the existing
'../elsewhere' coverage.
🪄 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: 9b447a28-4293-42bd-883b-02a37b026135
⛔ Files ignored due to path filters (1)
pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (32)
.pr_agent.tomlREADME.mdlib/main.dartlib/src/services/desktop_auth_service.dartlib/src/services/desktop_firestore_cache_io.dartlib/src/services/desktop_firestore_cache_stub.dartlib/src/services/desktop_launcher_service.dartlib/src/services/desktop_update_service.dartlib/src/services/photo_service.dartlib/src/services/spectrum_auth_service.dartlib/src/services/synced_map_image_store.dartlib/src/services/user_scoped_firestore_cache.dartlib/src/state/pit_controller_mixin.dartlib/src/ui/borrow_tab.dartlib/src/ui/inventory_tab.dartlib/src/ui/location_code.dartlib/src/ui/maps_tab.dartlib/src/ui/packing_tab.dartlib/src/ui/schedule_tab.dartlib/src/widgets/keyboard_shortcuts.dartpubspec.yamltest/borrow_tab_test.darttest/desktop_auth_service_test.darttest/desktop_launcher_service_test.darttest/desktop_update_service_test.darttest/keyboard_shortcuts_test.darttest/large_text_layout_test.darttest/maps_tab_test.darttest/support/fake_map_diagram_sync_service.darttest/support/photo_test_support.darttest/synced_map_image_store_test.darttest/user_scoped_firestore_cache_test.dart
| @override | ||
| Future<void> signOut() async { | ||
| _endingSession = true; | ||
| final departingUid = currentUser?.uid; | ||
| await _session.signOut(); | ||
| await _runTeardown(departingUid); | ||
| _emit(const SpectrumAuthSnapshot(state: SpectrumAuthState.signedOut)); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
signOut() leaves the service permanently stuck if _session.signOut() throws.
Line 166 sets _endingSession = true. Line 168 then awaits _session.signOut(). _endingSession is only reset in the finally inside _runTeardown at line 188. If line 168 throws, line 169 never runs, so the flag is never reset.
Two consequences follow:
_handleSessionRevokedreturns early forever at line 174. Mid-session revocation is no longer detected for the life of the service.- No
signedOutsnapshot is emitted, and the stored session is never removed. The UI still shows the user as signed in, and the departing user's cached documents stay on disk.
_session.signOut() performs a network call. A sign-out attempted with no network is a realistic trigger. This is the offline path the PR targets.
🐛 Proposed fix
`@override`
Future<void> signOut() async {
_endingSession = true;
final departingUid = currentUser?.uid;
- await _session.signOut();
+ try {
+ await _session.signOut();
+ } catch (_) {
+ // The local session still has to be torn down when the revoke call
+ // cannot reach the network.
+ }
await _runTeardown(departingUid);
_emit(const SpectrumAuthSnapshot(state: SpectrumAuthState.signedOut));
}Add a test that makes _session.signOut() throw a SocketException and asserts that the state becomes signedOut, that onSessionEnded fires, and that the persisted key is removed.
📝 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.
| @override | |
| Future<void> signOut() async { | |
| _endingSession = true; | |
| final departingUid = currentUser?.uid; | |
| await _session.signOut(); | |
| await _runTeardown(departingUid); | |
| _emit(const SpectrumAuthSnapshot(state: SpectrumAuthState.signedOut)); | |
| } | |
| `@override` | |
| Future<void> signOut() async { | |
| _endingSession = true; | |
| final departingUid = currentUser?.uid; | |
| try { | |
| await _session.signOut(); | |
| } catch (_) { | |
| // The local session still has to be torn down when the revoke call | |
| // cannot reach the network. | |
| } | |
| await _runTeardown(departingUid); | |
| _emit(const SpectrumAuthSnapshot(state: SpectrumAuthState.signedOut)); | |
| } |
🤖 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 164 - 171, Update
signOut() to guarantee teardown runs even when _session.signOut() throws, using
the existing _runTeardown(departingUid) flow so _endingSession is reset,
persisted session data is removed, and signedOut is emitted. Add a test covering
a SocketException from _session.signOut() that verifies the signed-out state,
onSessionEnded callback, and persisted-key removal.
| final disk = _diskCache; | ||
| if (disk != null) unawaited(disk.write(key, photo.bytes)); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repository files matching photo_service.dart:\n'
fd -a 'photo_service\.dart$' . || true
printf '\nRelevant file outline:\n'
for f in $(fd 'photo_service\.dart$' .); do
echo "--- $f"
wc -l "$f"
ast-grep outline "$f" --view compact || true
done
printf '\nRelevant sections:\n'
for f in $(fd 'photo_service\.dart$' .); do
echo "--- $f lines 1-220"
sed -n '1,220p' "$f" | nl -ba
done
printf '\nSearch cache implementations and usages:\n'
rg -n "class PhotoDiskCache|write\(|remove\(|unawaited|delete\\(" . -S --glob '!build/**' --glob '!dist/**' || trueRepository: Spectrum3847/spectrum-pit
Length of output: 638
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f="$(fd 'photo_service\.dart$' . | head -n 1)"
echo "--- $f outline"
ast-grep outline "$f" --view expanded || true
echo "\n--- $f lines 1-220"
awk '{print NR ": " $0}' "$f" | sed -n '1,220p'
echo "\n--- cache definitions/usages"
rg -n "class PhotoDiskCache|PhotoDiskCache|write\\(|remove\\(|unawaited|void upload|upload\\(|void delete|delete\\(" . -S --glob '!build/**' --glob '!dist/**' || trueRepository: Spectrum3847/spectrum-pit
Length of output: 21584
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- lib/src/services/photo_disk_cache.dart"
awk '{print NR ": " $0}' lib/src/services/photo_disk_cache.dart
echo "\n--- test/photo_disk_cache_test.dart"
awk '{print NR ": " $0}' test/photo_disk_cache_test.dart
echo "\n--- test/photo_service_test.dart relevant tests"
awk '{print NR ": " $0}' test/photo_service_test.dart | sed -n '1,110p'
echo "\n--- deterministic call-order probe: write/removes scheduled concurrently without queue"
python3 - <<'PY'
from dataclasses import dataclass
`@dataclass`
class DiskCache:
files: set
async def write(self, key, data):
# async-ish path: enqueue write and complete after caller may have returned
print(f"write scheduled {key}")
print(f"remove ran before write {key}")
self.files.add(key)
print(f"write completed {key}")
async def remove(self, key):
print(f"remove scheduled {key}")
self.files.discard(key)
async def upload_delete_sequence():
files = set()
disk = DiskCache(files)
key = "abc"
disk.files.add(key)
files.add(key)
# simulate unawaited write after upload returns
write_task = disk.write(key, b"data")
# user then deletes immediately
await disk.remove(key)
# pending cache write completes later
await write_task
print(f"final files={disk.files}")
async def upload_delete_seq_awaited_best_effort():
files = set()
disk = DiskCache(files)
key = "abc"
disk.files.add(key)
files.add(key)
try:
await disk.write(key, b"data")
except Exception:
pass
await disk.remove(key)
print(f"final files={disk.files}")
import asyncio
print("unawaited:")
asyncio.run(upload_delete_sequence())
print("awaited:")
asyncio.run(upload_delete_seq_awaited_best_effort())
PYRepository: Spectrum3847/spectrum-pit
Length of output: 13012
Serialize disk-cache writes with deletion.
PhotoDiskCache.write creates and deletes cache files independently. If upload returns before the scheduled write completes, a later delete can remove the file before _trim writes it back, leaving a deleted photo in disk cache. Await the write and retry after remove, or queue per-key writes/removes. Add a regression test that delays PhotoDiskCache.write, deletes the key, then verifies no disk entry remains.
🤖 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 112 - 113, Update the
disk-cache write path in the photo service to serialize writes with deletion: do
not fire-and-forget PhotoDiskCache.write. Await it and coordinate any subsequent
remove/delete so a delayed write cannot recreate a deleted key, using per-key
operation queuing if needed. Add a regression test that delays write, deletes
the same key, and verifies no disk entry remains.
| class HorizontalStepShortcuts extends StatelessWidget { | ||
| const HorizontalStepShortcuts({ | ||
| required this.onPrevious, | ||
| required this.onNext, | ||
| required this.child, | ||
| super.key, | ||
| }); | ||
|
|
||
| final VoidCallback onPrevious; | ||
| final VoidCallback onNext; | ||
| final Widget child; | ||
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| return FocusTraversalGroup( | ||
| child: CallbackShortcuts( | ||
| bindings: <ShortcutActivator, VoidCallback>{ | ||
| const SingleActivator(LogicalKeyboardKey.arrowLeft): onPrevious, | ||
| const SingleActivator(LogicalKeyboardKey.arrowRight): onNext, | ||
| }, | ||
| child: child, | ||
| ), | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Expect production UI call sites for HorizontalStepShortcuts.
rg -n -C 3 --glob '*.dart' \
'HorizontalStepShortcuts\s*\(' lib testRepository: Spectrum3847/spectrum-pit
Length of output: 1122
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- relevant keyboard_shortcuts.dart ---\n'
sed -n '1,120p' lib/src/widgets/keyboard_shortcuts.dart
printf '\n--- test imports and HorizontalStepShortcuts test ---\n'
sed -n '1,120p' test/keyboard_shortcuts_test.dart
printf '\n--- repository-wide HorizontalStepShortcuts references ---\n'
rg -n --glob '*.dart' 'HorizontalStepShortcuts' .Repository: Spectrum3847/spectrum-pit
Length of output: 254
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- relevant keyboard_shortcuts.dart ---'
sed -n '1,120p' lib/src/widgets/keyboard_shortcuts.dart
printf '%s\n' ''
printf '%s\n' '--- test imports and HorizontalStepShortcuts test ---'
sed -n '1,120p' test/keyboard_shortcuts_test.dart
printf '%s\n' ''
printf '%s\n' '--- repository-wide HorizontalStepShortcuts references ---'
rg -n --glob '*.dart' 'HorizontalStepShortcuts' .Repository: Spectrum3847/spectrum-pit
Length of output: 5081
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- candidate UI files by common screen/form naming ---'
git ls-files lib | grep --regexp='lib/src/(screens|features|pages|widgets)/.*\.dart$' | sed -n '1,120p'
printf '%s\n' ''
printf '%s\n' '--- keyboard shortcut imports/usages ---'
rg -n --glob '*.dart' "import .*keyboard_shortcuts|SaveShortcut\(|HorizontalStepShortcuts\(" lib test
printf '%s\n' ''
printf '%s\n' '--- focused widgets that are likely step targets ---'
rg -n --glob '*.dart' "TextButton|ElevatedButton|ButtonBar|Steps|Stepper|FutureBuilder|TextFormField|TextField" lib/src | sed -n '1,240p'Repository: Spectrum3847/spectrum-pit
Length of output: 219
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- lib files ---'
git ls-files lib | sort
printf '%s\n' ''
printf '%s\n' '--- all keyboard shortcut references ---'
rg -n --glob '*.dart' "keyboard_shortcuts|SaveShortcut\(|HorizontalStepShortcuts\(" lib test || true
printf '%s\n' ''
printf '%s\n' '--- focusable interactive widgets in lib ---'
rg -n --glob '*.dart' "TextButton|ElevatedButton|ButtonBar|Stepper|Steps|TextFormField|TextField|FocusNode|autofocus" lib test || trueRepository: Spectrum3847/spectrum-pit
Length of output: 8454
Wire HorizontalStepShortcuts into desktop UI screens.
HorizontalStepShortcuts is only referenced in the tests, so desktop arrow navigation is not integrated. Wrap the relevant screen content and wire onPrevious/onNext before relying on the shortcuts.
🤖 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/widgets/keyboard_shortcuts.dart` around lines 27 - 50, Integrate
HorizontalStepShortcuts into the relevant desktop screen widgets, wrapping their
content and supplying onPrevious and onNext callbacks for the screen’s
horizontal navigation. Ensure the callbacks are wired before relying on the
existing arrow-key bindings, while preserving the current screen content
behavior.
|
|
||
| ## Install | ||
|
|
||
| Builds are attached to [this repo's releases](https://github.com/Spectrum3847/spectrum-pit/releases). They are unsigned, so each platform needs a step or two. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Publish integrity metadata for unsigned artifacts.
Lines 13, 28, and 32-36 tell users to install unsigned files or bypass SmartScreen and Gatekeeper. The installation flow provides no checksum or other artifact-integrity check.
Publish a release-generated SHA-256 manifest and add a verification step before asking users to override platform warnings.
Also applies to: 28-28, 32-36
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 13, Update the release artifact documentation around the
release links and platform installation instructions to publish a
release-generated SHA-256 manifest and direct users to verify downloaded files
against it before bypassing SmartScreen, Gatekeeper, or other unsigned-artifact
warnings.
2 participants
Desktop gets real offline support and a keyboard path.
Firestore reads on desktop now come from a per-account cache, so the app keeps showing event data when the venue wifi drops, including after a relaunch with no connection at all.
Every desktop screen is now reachable from the keyboard, without reaching for a mouse.
Sign-in handles being revoked mid-session instead of leaving the app looking signed in, and a photo upload no longer fails outright when the token refresh cannot reach the network.
Accessibility: the map diagram and its pins are announced to screen readers, and the tab bar stays usable at 200 percent text size.
Fixed a desktop launcher entry that broke when the install path contained a percent sign or a backslash.
Summary by CodeRabbit
New Features
Bug Fixes