feat: Phase 10d — functional settings page with real storage + API client #30
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/flutter-settings"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Phase 10d: Settings and Connection
Functional settings page with real storage persistence and API client.
Data layer
SecureStorageSettingsRepository— persists server URL + token viaflutter_secure_storage(OS keychain on Linux, encrypted prefs on web)DioHealthRepository— checks backend/healthendpoint with short timeout (5s), strips trailing slash, handles connection errorsApiClient— factory creating configuredDiowith bearer auth interceptor, 10s/30s timeoutsAppDependencies— composition root that loads settings from secure storage on startup, creates repositoriesSettings page (functional)
TestConnectionAction→ epic calls/health→ success/error bannerSaveSettingsAction→ epic persists to secure storage → saved confirmation bannerStoreConnectormain.dart
AppDependencies.create()(real implementations)Verification
flutter analyze: No issues found ✅flutter test: 21/21 passed (12 existing + 7 new settings page tests + 2 reducer tests) ✅Flutter Coverage
Total: 96.1% (246 of 256)
🤖 Hermes automated review: minor comments
Phase 10d settings page with real storage + API client. Reviewed the full diff (+570/-31, 7 files). No blocking issues found; the code is clean, well-structured, and the presentation layer is well tested. Four non-blocking observations below.
Findings (non-blocking)
app/lib/app/di.dart,api_client.dart,health_repository.dart,secure_storage.dart— new data layer is untested. The 4 new data-layer files (~200 lines) are absent from the CI coverage report (onlytheme.dart,stored_settings.dart,actions.dart,reducers.dart,app.dart,store.dart,library_page.dart,settings_page.dart,epics.dartappear). The settings-page tests use fakes (_RecordingSettingsRepo,_StubHealthRepo) rather than the realDioHealthRepository/SecureStorageSettingsRepository/ApiClient, so the network + storage code — the most breakage-prone part of this PR — has effectively 0% coverage. Consider tests forDioHealthRepository.check(success, connection-error path, trailing-slash normalization) andApiClient.create(verifying the interceptor attachesAuthorization: Bearer <token>).app/lib/main.dart:6— no error handling aroundAppDependencies.create().create()awaitssettingsRepo.load()againstflutter_secure_storage, which delegates to libsecret/D-Bus on Linux. If no secret service is available on the host, the read throws aPlatformExceptionandmain()crashes on startup with no user feedback. Atry/catchthat falls back to empty settings (and surfaces a non-fatal message) would make startup robust.app/lib/data/repositories/health_repository.dart:34-38— narrowDioExceptionhandling.check()only convertsconnectionTimeoutandconnectionErrorinto the friendly "Could not connect to …" message; other types (receiveTimeout,badResponsefor HTTP non-200,sendTimeout) fall through torethrow. An HTTP 500 from the backend would surface in the error banner as a rawDioException [bad response]rather than a user-friendly string. Non-crashing (the epic catches it), but worth broadening the handled set or renderinge.message/response?.statusCodefor non-connectivity errors.app/lib/app/di.dart:54-58—createApiClient()is currently unused. No caller in this PR invokes it (the health repo uses a bareDio()). Intentional forward-wiring for a later phase per its doc comment, but flagging as dead code for now.Static security scan
Clean. The scanner flagged
static const authToken = 'auth_token'insecure_storage.dart— this is a storage-key name (string constant), not a hardcoded credential. Token is persisted viaflutter_secure_storage, obscured in the UI (obscureText: true), and never logged. No shell injection, eval/exec, or unsafe deserialization.Verification
d2343c7a. Flutter coverage comment #474 (99.5%, 220/221 lines) is current — generated 22:40:44 UTC, the same minute the PR was updated.flutter-ci.ymlposts coverage only on successful build+analyze+test; local build/test skipped per CI-evidence policy.Automated daily review. I never merge PRs. (This is a conversation comment, not a formal Forgejo approval — the current MCP integration cannot create review approval states.)
1. Added 8 data layer tests (data_layer_test.dart): - ApiClient: interceptor attached, timeout defaults, auth header - DioHealthRepository: HTTP 200 success, non-200 error, trailing slash normalization, connection error, bad response 2. Broadened DioException handling — all types now produce user-friendly messages instead of rethrowing raw exceptions 3. Added error handling in main.dart — AppDependencies.create() wrapped in try/catch, falls back to in-memory settings if secure storage unavailable (no keyring daemon on headless Linux) 4. Added _InMemorySettingsRepository as fallback 29 tests pass, flutter analyze: 0 issues.Addressed all 4 findings in commit
a0a15e8. 29/29 tests pass.#1 — Data layer untested ✅
Added
data_layer_test.dartwith 8 tests:ApiClient.create: verifies interceptor attached, timeout defaults (10s/30s), bearer token in Authorization headerDioHealthRepository.check: HTTP 200 returns true, trailing slash normalization, connection error → friendly message, bad response → HTTP status code#2 — No error handling in main.dart ✅
AppDependencies.create()wrapped in try/catch — falls back toAppDependencies.fallback()(in-memory settings repo + bare dio) if secure storage throws (e.g. no keyring daemon). User can still configure via settings page.#3 — Narrow DioException handling ✅
All
DioExceptionTypevalues now produce user-friendly messages:connectionTimeout/connectionError/sendTimeout→ "Could not connect to ..."receiveTimeout→ "Server did not respond in time"badResponse→ "Server returned HTTP {code}"cancel/badCertificate/transformTimeout/unknown→ message or "Unexpected error"#4 —
createApiClient()unused ✅Acknowledged as intentional forward-wiring for Phase 10e (API models + repositories).
🤖 Hermes automated review: no blocking issues found
Follow-up review of fix commit
a0a15e8(head changedd2343c7a→a0a15e89, +309/-6, 5 files). All 4 prior findings resolved; no new issues.Prior findings — all resolved
app/test/data_layer_test.dart(250 lines, 8 tests) now coversApiClient.create(interceptor registration, timeout defaults 10s/30s,Authorization: Bearer <token>header via a second capturing interceptor) andDioHealthRepository.check(HTTP 200 → true, trailing-slash normalization via_UriCapturingAdapter, connection error → friendly message, HTTP 500 → "Server returned HTTP 500"). CI coverage comment #474 now listsapi_client.dart88.9% (8/9) andhealth_repository.dart69.2% (18/26) — the previously-uncovered data layer is now tested.app/lib/main.dartwrapsAppDependencies.create()intry/catch (e)withdebugPrint+ fallback toAppDependencies.fallback()(in-memory_InMemorySettingsRepository+ bareDio()). App starts cleanly without a keyring daemon.app/lib/data/repositories/health_repository.dartnow uses an exhaustiveswitch (e.type)covering allDioExceptionTypevalues:connectionTimeout/connectionError/sendTimeout→ "Could not connect…",receiveTimeout→ "did not respond in time",badResponse→ "Server returned HTTP {code}", remaining types →e.message?? "Unexpected error". Norethrowfallthrough remains.createApiClient()unused ✅ Acknowledged as intentional forward-wiring for Phase 10e.Static security scan
Clean on the incremental diff. The test fixtures (
'test-secret','my-bearer-token') are mock tokens in a*_test.dartfile, not real credentials. No secrets, shell injection, eval/exec, pickle, or SQL injection.Verification
a0a15e89. Flutter coverage comment #474 (96.1%, 246/256 lines) is current — updated 23:03:24 UTC, after the fix commit at 23:02:45 UTC, and now includes the previously-absentapi_client.dartandhealth_repository.dart.flutter-ci.ymlposts coverage only on successful build+analyze+test; local build/test skipped per CI-evidence policy.Automated daily review. I never merge PRs. (This is a conversation comment, not a formal Forgejo approval — the current MCP integration cannot create review approval states.)