Repository navigation
#105 Browse and pick a Home Assistant entity during season-sync setup - #107
Conversation
…d on Q1–Q6 and ten strings Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SsCifH5iWH1JdtAsUcbKnW
…ten strings in ten packs Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SsCifH5iWH1JdtAsUcbKnW
…ded read - listStates maps up to 8 MiB of JSON on the client's io, not the sheet's main thread. - The browser's read is held as a job; a new read, a pick, manual entry and a close cancel it (the client disconnects), so a late answer never lands in a browser opened after it. New row-14 case fails without the cancel. - The empty-scope flag (P105-8) is computed once per list, not per keystroke. - Plan §9a records the review. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0133GBN5r2nyJM4teL6rziSk
GonzRon
left a comment
There was a problem hiding this comment.
Reviewed at head 206a00e32ba85eab9a805289985611a1f42385f3.
CI run 385 is green (build, MCP, schedules, bundle), and the implementation is generally well aligned with #105: exact entity IDs remain canonical, REST discovery reuses #16's transport policy, the 8 MiB cap is isolated to the foreground list call, filtering/search/sort are pure, failed refreshes preserve the last list/selection, and the late-answer cancellation fix is good for the in-sheet paths.
Disposition: two blocking findings before merge
1. BLOCKING — whole-sheet dismissal can orphan an in-flight 8 MiB list request
The cancellation fix covers closeBrowse(), pick, manual entry and starting a new read, but the actual ModalBottomSheet.onDismissRequest bypasses the ViewModel and calls onDone() directly.
That matters because this ViewModel is created with viewModel(key = "season-sync-sheet:$assetId:$opening") under the Asset-detail ViewModelStoreOwner. Removing the conditional sheet from composition does not clear that ViewModel; each reopen increments opening and creates another keyed ViewModel while the old one remains alive until the Asset-detail owner is destroyed.
So this sequence is possible:
- open Choose entity →
GET /api/statesstarts; - press Android Back / tap scrim / swipe the sheet away;
- the UI disappears, but the old request keeps running (up to the 30 s call bound, potentially reading/parsing 8 MiB);
- reopen and start another list read;
- repeat → multiple abandoned foreground reads can overlap.
That contradicts the #105 contract that the inventory read is an explicit foreground action and that the sheet's list/read goes away when the sheet closes. Route every whole-sheet dismissal through a ViewModel cancellation method before onDone() (and pin that dismissal path with a test), rather than only cancelling the browser's own Cancel button.
2. BLOCKING — Save is enabled with no entity solely to preserve a stale #16 test
The approved #105 plan changed the form contract to: Save is disabled until a picked entity or a manual ID exists. The implementation instead leaves canSave independent of entity presence, and the execution record says this was done so the old #16 device assertion would remain true.
That test describes the old manual-ID form; it is not a product invariant. With the new browse-first UI, an owner can tap Save while the only entity affordance says Choose entity, and gets P16-49 telling them to enter an entity ID. That is internally inconsistent with the new primary flow.
Please restore the planned predicate for LINK: require chosen != null || entityId.trim().isNotEmpty() (RESUME remains unaffected), and update the old device assertion to the new contract. Invalid typed IDs should still flow through P16-49 exactly as before.
Proof gate before merge
The new/edited androidTest code is not exercised by CI and the PR says the connected classes have not been run. Because #105 changes the actual sheet/navigation surface and acceptance explicitly calls for Android coverage, I would run at least SeasonSyncScreensTest on emulator-5554 before merging, then the planned broader connected gate. The real-Home-Assistant smoke proof can remain the controller/release proof if needed, but the instrumented code should at least compile and run once before merge.
What I checked and am accepting
mapHaStatesAnswer: bounded body contract, status map, strict IDs, first-wins duplicate handling, bounded/control-scrubbed friendly names.pickerRows: exactinput_booleandomain, name/id substring search, deterministic ordering, 2,000-row cap.HomeAssistantStateClient.listStates: same address/network/host/header/no-redirect policy as #16, separate 8 MiB cap, parsing moved off main.- ViewModel selection/manual/refresh semantics: exact ID only; last good list survives failure; a picked entity never retargets by friendly name.
- Localization: new copy is resource-backed across the shipped packs and existing guards are green.
- No schema/backup/API/MCP/manifest expansion.
I found no other code blocker in this pass.
…ts for an entity - Back, scrim, swipe and the form's Cancel go through the model's dismiss(), which cancels a list read still running: each opening's model is keyed under the asset page and outlives its sheet. - Link's Save is held until there is an entity to send (a pick or non-blank typed text), as row 16 specifies; Resume is unchanged. - The two shipped device rows that typed into the field (S55, #78's YEAR_ROUND link) open Enter entity ID manually first; the S55 row asserts Save held, then enabled. - Plan §9a records the review. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0133GBN5r2nyJM4teL6rziSk
|
Review 5416922609, finding 2 and the proof gate. Both findings are fixed in 2. Save waits for an entity. For Link, Two more connected tests were broken, and the fix covers them. While updating the old Save assertion, I found two #16 device tests that typed into the Entity ID field right after opening the sheet: the S55 test and #78's YEAR_ROUND link test. The browse-first sheet doesn't show that field until Enter entity ID manually is tapped, so both would have failed on Proof gate: agreed, still open. Generated by Claude Code |
Implements #105 (1.8.0 content, coordinator #104) under the owner rulings of 2026-10-05. Plan:
docs/superpowers/plans/2026-10-05-issue-105-home-assistant-entity-picker.md.What changes
The setup sheet (Link to Home Assistant on an Asset's season card) now opens on a Choose entity row instead of a bare Entity ID field. Tapping it turns the sheet into a searchable list of Home Assistant's
input_booleanhelpers, showing each one's friendly name over its exact entity ID, sorted by name. A tap picks one and returns to the form. Enter entity ID manually keeps the typed path. Save is held on Link until there is an entity to send: a pick, or non-blank typed text. Save then sends the exact entity ID through the shipped #16LinkSeasonSync, unchanged. As #16 shipped it, Home Assistant is checked after the link is saved; no extra per-entity request is added.HaEntityList.ktmaps aGET /api/statesanswer to candidates. Ids are checked withisValidEntityId, names bounded at 128 characters with control characters replaced, and duplicates dropped. Failures use Test connection's status map.EntityPicker.kt(pickerRows) filters to theinput_booleandomain, searches name and id by case-insensitive substring, sorts by name then id, and stops at 2,000 rows with a truncation flag.HomeAssistantStateClient.listStatesmakes one foregroundGET <base>/api/statesthrough the sameexchangeas the poll: address rule, permission, home-network gate, headers, timeouts and failure map. It has its own 8 MiB cap; the poll stays at 64 KiB. The answer is parsed on the client's IO context.AppGraph.listHaEntitieswires it over the stored connection and token.values/strings_asset_edit.xmland all nine packs. The translations are drafts until a native speaker reviews them.docs/home-assistant-season-sync.md(linking section and Limits) anddocs/capabilities.md.Unchanged: schema, backup format, API, MCP, manifest, permissions, the poll, the applier,
LinkSeasonSync, and every #16 string.Rulings applied
/api/statesonly; device grouping and the entity/device registries are deferred (Q1). The acceptance bullet about device context is deferred, not met.input_boolean.*only, with manual entry as the escape hatch (Q3).Departures from the plan (recorded in §9a)
HaStateMapper.kt'sstringOrNullandboundedbecameinternal, so the list mapper uses the same bounding code (no behaviour change).An earlier departure, Save staying enabled with nothing chosen, was reversed in review (
eaad0cf). Row 16 holds as the plan wrote it.Review history
Task review (
206a00e):PR review 5416922609 (
eaad0cf):dismiss(), which cancels it.The fix also repaired two shipped #16 device tests (S55, and #78's year-round link). They typed into a field the browse-first sheet no longer shows, so they now open Enter entity ID manually first. The S55 test asserts Save is disabled before an entity is given and enabled after.
Noted and left as is: a key-store or database failure before the read shows no message, as Save already does (none is ratified for it).
Tests and gates
HaStatesMapperTest,EntityPickerTest;HomeAssistantStateClientTest;LinkSeasonSyncViewModelTest, plus the superseded-read, sheet-dismissal and Save-waits-for-an-entity cases;SeasonSyncScreensTest, plus the two repaired [SHIPPED][FEATURE] Home Assistant operating-season synchronization #16 tests.eaad0cf(run 37333710584), including:app:testDebugUnitTestwithLocalizationCoverageTestandUiLiteralGuardTest.SeasonSyncScreensTest, then the connected suite, onemulator-5554(ANDROID_SERIAL=emulator-5554 ./gradlew :app:connectedDebugAndroidTest). CI does not compile or run androidTest, so this is also the only check of the sheet's dismissal wiring. The real-Home-Assistant proof (§10) stays the controller's release step.🤖 Generated with Claude Code
https://claude.ai/code/session_0133GBN5r2nyJM4teL6rziSk