feat(cards): action card family for scenes, scripts and buttons - #252
Merged
Conversation
Register one card for `scene`, `script`, `button` and `input_button`, replacing the generic fallback all four resolve to today. This is a bugfix before it is a feature. The fallback `ButtonCard` dispatches `<domain>.toggle`, and three of these four domains have no such service: `scene.toggle`, `button.toggle` and `input_button.toggle` all answer HTTP 400 against a live Home Assistant, so every tap on one of those cards is a call the backend rejects. The per-domain action map is the fix — `scene.turn_on`, `script.turn_on` / `script.turn_off`, `button.press`, `input_button.press` — each checked against a running instance's service registry rather than against documentation. One family card, not three: the domains diverge only in the service name and in the script-only running state, so splitting them would split the option surface, the stories and the tests with them. Behaviour from the option doc: - Activation feedback is intrinsic, not an option. These entities expose no state change to observe, so the icon swaps to a spinner in flight and to a check held ~1.5s on success. Reduced motion drops the rotation and the swap transition in the stylesheet but keeps the check, which is evidence rather than decoration. - `unknown` is inert only for `script`. For the other three the state IS the last-activation timestamp, so a never-activated entity reports `unknown` forever and must stay activatable. - A running script's tap means stop, and the card says so. - `confirm` supplies the family's own gate through `confirmRoute`, replacing the shell's generic on/off rule — so it also covers the `homeassistant.*` aliases that rule caught, and fails toward asking. - `showLastActivated` reads the state for three domains and `last_triggered` for `script`, rendering "Never" for anything unset or unparseable. Dispatch goes through the non-retrying guarded path throughout: every service here is non-idempotent, and a retried press presses twice. The card renders the icon circle itself rather than through `GridCard.Icon`, whose `icon` override wins over its children — a configured icon must not suppress the only evidence a tap did anything. Adds a `pending` service-call mode to the Storybook mock so the in-flight state has a story that can observe it.
The grid already publishes an `onConfigure` on the card item context and owns the configuration modal, which is how the cover and sensor families reach their options. A second modal inside the card added a save path that only the grid's own can reach. Resolving the primary command once in render also removes two guards that could never fire: the handler's "no command" check and the confirm route's missing-prompt fallback were both dead behind `isUnavailable`, which is now derived from the same value.
Two probes survived the mutation run, and both were real gaps. Nothing asserted the registry mapping, so unmapping `scene` back to the fallback broke no test — while being exactly the regression this change exists to prevent. The sequencing cases also read the hold out of the same constant they were checking, so shortening it moved the code and the expectation together; the interval is now pinned as a literal.
The defect this change fixes lives exactly at the boundary a unit test stubs out: the fallback card dispatches `button.toggle`, which is not a registered service, so Home Assistant answers 400 and the entity never moves. A mocked dispatch layer agrees with the card either way. `button.push` carries its last press as its own state, so the proof is that the state advances.
📝 WalkthroughWalkthroughAdds a unified ActionCard family for scene, script, button, and input_button entities, including domain routing, persisted options, guarded activation feedback, confirmation prompts, Storybook scenarios, unit tests, and an end-to-end button interaction. ChangesAction card family
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ActionCard
participant dispatchGuarded
participant HomeAssistant
User->>ActionCard: Tap action card
ActionCard->>dispatchGuarded: Resolve and dispatch primary command
dispatchGuarded->>HomeAssistant: callService(domain, service, target)
HomeAssistant-->>dispatchGuarded: Return service result
dispatchGuarded-->>ActionCard: Update pending, success, or error feedback
ActionCard-->>User: Render updated icon and state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Introduces a new ActionCard family to correctly support Home Assistant scene, script, button, and input_button entities (previously falling through to the fallback ButtonCard and incorrectly dispatching <domain>.toggle), including guarded non-retrying dispatch, activation feedback UI, confirmation gating, and “last activated” display.
Changes:
- Add the
ActionCardcomponent family (UI, per-domain action map, activation feedback, script running state) plus Storybook stories and unit tests. - Wire the family into the app: registry/domain lists, configuration modal definitions, config schema validation + option reader for
confirm/showLastActivated. - Add boundary coverage: new Playwright E2E spec against a real Home Assistant demo button.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/e2e/helpers.ts | Add demo button entity + dedicated seed config for action-card E2E screen. |
| tests/e2e/action-card.spec.ts | New E2E spec asserting button.press works against real Home Assistant by observing state change. |
| src/test/fixtures/storyParameters.ts | Extend Storybook service-call simulation to include a pending state. |
| src/test/fixtures/entities.ts | Add fixture factories for scene, script, button, input_button. |
| src/store/configSchema.ts | Merge action-card option schema into persisted config validation. |
| src/store/actionOptions.ts | New persisted option contract + reader for confirm and showLastActivated. |
| src/store/tests/actionOptions.test.ts | Unit tests for action options reader + schema import gate behavior. |
| src/components/EntitiesBrowserTab.tsx | Add action-family domains to supported domains and friendly labels. |
| src/components/configurations/cardConfigurations.ts | Add per-domain entries for the action family’s config UI (icon + confirm + last activated). |
| src/components/cardRegistry.ts | Register scene/script/button/input_button to render via ActionCard. |
| src/components/cardDomains.ts | Add the four new mapped domains to the domain type list. |
| src/components/ActionCard/index.tsx | New card component implementing per-domain activation + guarded dispatch and tier layouts. |
| src/components/ActionCard/hooks.ts | New hooks for activation feedback sequencing and last-activated ticker. |
| src/components/ActionCard/actions.ts | Per-domain action map + confirm-route classification + activation timestamp parsing/formatting. |
| src/components/ActionCard/ActionCard.stories.tsx | Full Storybook matrix for domains/states/options/tiers, including pending and confirm flows. |
| src/components/ActionCard/ActionCard.css | Layered styles for spinner + motion transitions with prefers-reduced-motion handling. |
| src/components/ActionCard/tests/activationStyles.test.ts | CSS-level tests asserting reduced-motion suppression and layering/animation invariants. |
| src/components/ActionCard/tests/actions.test.ts | Unit tests for the per-domain action map, inertness rules, confirm routing, and timestamps. |
| src/components/ActionCard/tests/ActionCard.test.tsx | Integration-style RTL tests exercising real hooks/guarded dispatch via mocked hass.callService. |
| src/components/tests/cardRegistry.test.ts | Assert the registry routes the four domains to ActionCard. |
| docs/changes/0027-scene-cards.md | Mark PR1 task as completed for change 0027. |
| .storybook/mockHass.ts | Add pending option to simulate never-settling service calls for in-flight UI stories. |
| .storybook/decorators.tsx | Thread new pending story parameter into the mock hass factory. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
# Conflicts: # src/components/configurations/cardConfigurations.ts # src/store/configSchema.ts
`confirm` was declared by both `switchOptionsConfigSchema` and `actionOptionsConfigSchema`, and `configSchema.ts` merges every family fragment into one item schema where `zod.merge()` is last-one-wins. The switch fragment therefore governed `confirm` for action cards too. Nothing misbehaved, because both declared it identically — which is what made it worth fixing: tightening the switch card's gate would have changed what action cards accept, with no diff touching them and no test failing. Both fragments now merge one shared object from `store/confirmOption.ts`, so the merge order cannot change what `confirm` accepts. Not the universal fragment, deliberately: `confirm` is a per-card option in both the switch and scene documents, and the universal set is closed at the five display keys and the three action keys — declaring it universal would say every family accepts it, which is a spec change rather than a refactor. `readCardConfirm` moves there with it. The shell applies the gate and had been importing a *switch* module to read an option three families define. Tests assert schema identity rather than equivalence — two structurally identical declarations would pass an equivalence check and still be free to drift — and pin `confirm` through the whole merged item schema, which is the level a fragment-level test cannot see.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #252 +/- ##
==========================================
+ Coverage 84.13% 84.47% +0.34%
==========================================
Files 192 197 +5
Lines 6927 7080 +153
Branches 2274 2333 +59
==========================================
+ Hits 5828 5981 +153
Misses 874 874
Partials 225 225 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/components/ActionCard/__tests__/activationStyles.test.ts (1)
81-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnimation-name extraction is order-sensitive.
/animation:\s*([a-z-]+)/captures the first token after the colon, so a legal shorthand reorder (animation: 1s linear infinite action-spin) makes this fail with a misleading message. Matching the name anywhere in the declaration would be more robust.🤖 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 `@src/components/ActionCard/__tests__/activationStyles.test.ts` around lines 81 - 88, Update the animation-name extraction in the reduced-motion test so it identifies animation names regardless of their position within a legal animation shorthand declaration, including declarations where duration, timing, or iteration values precede the name. Preserve the existing filtering of “none” and the expected “action-spin” assertion.src/components/ActionCard/index.tsx (1)
144-172: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
feedbackobject is a fresh reference every render, defeatinguseMemo.
useActivationFeedback()returns a new{ phase, run }literal each render, so listingfeedbackwhole in the dependency array (Line 172) meanshandlePrimaryis recreated on every render, not just when its actionable inputs change.♻️ Proposed dependency fix
- const handlePrimary = useMemo(() => { + const handlePrimary = useMemo(() => { if (!command) return undefined return () => { - if (!isRunning && feedback.phase !== 'idle') return + if (!isRunning && feedback.phase !== 'idle') return if (error) clearError() void feedback.run(() => dispatchGuarded({ domain: command.domain, service: command.service, entityId }) ) } - }, [clearError, command, dispatchGuarded, entityId, error, feedback, isRunning]) + }, [clearError, command, dispatchGuarded, entityId, error, feedback.phase, feedback.run, isRunning])🤖 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 `@src/components/ActionCard/index.tsx` around lines 144 - 172, Update the handlePrimary useMemo dependency list to depend on the stable, actionable members of feedback rather than the whole feedback object: reference feedback.phase and feedback.run separately in the callback and dependency array. Preserve the existing behavior and all other dependencies.
🤖 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 `@src/components/ActionCard/__tests__/ActionCard.test.tsx`:
- Around line 878-900: Update the test around “offers a reload as the way out of
a disconnection” to capture window.location’s original property descriptor
before overriding it, then restore that descriptor after the test completes,
including on assertion or interaction failure. Avoid reconstructing location via
object spread; preserve the original descriptor while replacing only reload for
the test.
In `@src/store/__tests__/actionOptions.test.ts`:
- Around line 82-86: Remove the misleading test case around
actionOptionsConfigSchema.shape.confirm, including its contradictory title and
comments, since confirm is intentionally shared with confirmOptionsConfigSchema
and the toBeDefined assertion does not validate that contract.
---
Nitpick comments:
In `@src/components/ActionCard/__tests__/activationStyles.test.ts`:
- Around line 81-88: Update the animation-name extraction in the reduced-motion
test so it identifies animation names regardless of their position within a
legal animation shorthand declaration, including declarations where duration,
timing, or iteration values precede the name. Preserve the existing filtering of
“none” and the expected “action-spin” assertion.
In `@src/components/ActionCard/index.tsx`:
- Around line 144-172: Update the handlePrimary useMemo dependency list to
depend on the stable, actionable members of feedback rather than the whole
feedback object: reference feedback.phase and feedback.run separately in the
callback and dependency array. Preserve the existing behavior and all other
dependencies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d5fe2143-d395-4804-8e6a-7ca00dd2b0f1
📒 Files selected for processing (28)
.storybook/decorators.tsx.storybook/mockHass.tsdocs/changes/0027-scene-cards.mdsrc/components/ActionCard/ActionCard.csssrc/components/ActionCard/ActionCard.stories.tsxsrc/components/ActionCard/__tests__/ActionCard.test.tsxsrc/components/ActionCard/__tests__/actions.test.tssrc/components/ActionCard/__tests__/activationStyles.test.tssrc/components/ActionCard/actions.tssrc/components/ActionCard/hooks.tssrc/components/ActionCard/index.tsxsrc/components/EntitiesBrowserTab.tsxsrc/components/__tests__/cardRegistry.test.tssrc/components/cardDomains.tssrc/components/cardRegistry.tssrc/components/configurations/cardConfigurations.tssrc/hooks/useCardActions.tssrc/store/__tests__/actionOptions.test.tssrc/store/__tests__/confirmOption.test.tssrc/store/__tests__/switchOptions.test.tssrc/store/actionOptions.tssrc/store/configSchema.tssrc/store/confirmOption.tssrc/store/switchOptions.tssrc/test/fixtures/entities.tssrc/test/fixtures/storyParameters.tstests/e2e/action-card.spec.tstests/e2e/helpers.ts
💤 Files with no reviewable changes (1)
- src/store/tests/switchOptions.test.ts
Comment on lines
+878
to
+900
| it('offers a reload as the way out of a disconnection', async () => { | ||
| const reload = vi.fn() | ||
| // jsdom's own `location.reload` throws "not implemented", so the retry can | ||
| // only be exercised against a replaced one. | ||
| Object.defineProperty(window, 'location', { | ||
| configurable: true, | ||
| value: { ...window.location, reload }, | ||
| }) | ||
|
|
||
| entityStore.setState((state) => ({ | ||
| ...state, | ||
| isConnected: false, | ||
| isInitialLoading: false, | ||
| entities: {}, | ||
| })) | ||
| renderCard(<ActionCard entityId="scene.movie_night" />) | ||
|
|
||
| // The card-variant error tile opens a detail modal; Retry lives inside it. | ||
| fireEvent.click(screen.getByLabelText('Disconnected: Disconnected from Home Assistant')) | ||
| fireEvent.click(await screen.findByRole('button', { name: 'Retry' })) | ||
|
|
||
| expect(reload).toHaveBeenCalledTimes(1) | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore the original window.location descriptor after this test.
The override is installed but never reverted, so every later test in this file (and any sharing the environment) sees the replaced object. Also, { ...window.location } copies only own enumerable properties; jsdom's Location exposes href/origin/etc. as prototype accessors, so the replacement is likely missing them.
Based on learnings, when simulating window.location you should capture the existing property descriptor and restore it afterward.
🧪 Proposed fix
it('offers a reload as the way out of a disconnection', async () => {
const reload = vi.fn()
// jsdom's own `location.reload` throws "not implemented", so the retry can
// only be exercised against a replaced one.
+ const originalLocation = Object.getOwnPropertyDescriptor(window, 'location')!
Object.defineProperty(window, 'location', {
configurable: true,
- value: { ...window.location, reload },
+ value: Object.assign(Object.create(window.location), { reload }),
})
+ try { expect(reload).toHaveBeenCalledTimes(1)
+ } finally {
+ Object.defineProperty(window, 'location', originalLocation)
+ }
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('offers a reload as the way out of a disconnection', async () => { | |
| const reload = vi.fn() | |
| // jsdom's own `location.reload` throws "not implemented", so the retry can | |
| // only be exercised against a replaced one. | |
| Object.defineProperty(window, 'location', { | |
| configurable: true, | |
| value: { ...window.location, reload }, | |
| }) | |
| entityStore.setState((state) => ({ | |
| ...state, | |
| isConnected: false, | |
| isInitialLoading: false, | |
| entities: {}, | |
| })) | |
| renderCard(<ActionCard entityId="scene.movie_night" />) | |
| // The card-variant error tile opens a detail modal; Retry lives inside it. | |
| fireEvent.click(screen.getByLabelText('Disconnected: Disconnected from Home Assistant')) | |
| fireEvent.click(await screen.findByRole('button', { name: 'Retry' })) | |
| expect(reload).toHaveBeenCalledTimes(1) | |
| }) | |
| it('offers a reload as the way out of a disconnection', async () => { | |
| const reload = vi.fn() | |
| // jsdom's own `location.reload` throws "not implemented", so the retry can | |
| // only be exercised against a replaced one. | |
| const originalLocation = Object.getOwnPropertyDescriptor(window, 'location')! | |
| Object.defineProperty(window, 'location', { | |
| configurable: true, | |
| value: Object.assign(Object.create(window.location), { reload }), | |
| }) | |
| try { | |
| entityStore.setState((state) => ({ | |
| ...state, | |
| isConnected: false, | |
| isInitialLoading: false, | |
| entities: {}, | |
| })) | |
| renderCard(<ActionCard entityId="scene.movie_night" />) | |
| // The card-variant error tile opens a detail modal; Retry lives inside it. | |
| fireEvent.click(screen.getByLabelText('Disconnected: Disconnected from Home Assistant')) | |
| fireEvent.click(await screen.findByRole('button', { name: 'Retry' })) | |
| expect(reload).toHaveBeenCalledTimes(1) | |
| } finally { | |
| Object.defineProperty(window, 'location', originalLocation) | |
| } | |
| }) |
🤖 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 `@src/components/ActionCard/__tests__/ActionCard.test.tsx` around lines 878 -
900, Update the test around “offers a reload as the way out of a disconnection”
to capture window.location’s original property descriptor before overriding it,
then restore that descriptor after the test completes, including on assertion or
interaction failure. Avoid reconstructing location via object spread; preserve
the original descriptor while replacing only reload for the test.
Source: Learnings
Comment on lines
+82
to
+86
| it('declares confirm itself rather than leaning on the switch card’s schema', () => { | ||
| // The two families name the option independently; a later change to the | ||
| // switch surface must not take this one's validation with it. | ||
| expect(actionOptionsConfigSchema.shape.confirm).toBeDefined() | ||
| }) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Test title/comment contradicts the actual shared-schema design (and confirmOption.test.ts).
This test's name and comment claim the action family "declares confirm itself rather than leaning on the switch card's schema" and that the two "name the option independently." But actionOptionsConfigSchema merges the shared confirmOptionsConfigSchema (line 55 of actionOptions.ts), and confirmOption.test.ts's "one definition of the gate" suite explicitly asserts actionOptionsConfigSchema.shape.confirm and confirmOptionsConfigSchema.shape.confirm are the same object reference — the opposite of independent declaration. The toBeDefined() assertion here also wouldn't catch a regression either way, since it passes whether confirm is shared or duplicated.
🧹 Suggested fix: remove the stale/misleading test
- it('declares confirm itself rather than leaning on the switch card's schema', () => {
- // The two families name the option independently; a later change to the
- // switch surface must not take this one's validation with it.
- expect(actionOptionsConfigSchema.shape.confirm).toBeDefined()
- })📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| it('declares confirm itself rather than leaning on the switch card’s schema', () => { | |
| // The two families name the option independently; a later change to the | |
| // switch surface must not take this one's validation with it. | |
| expect(actionOptionsConfigSchema.shape.confirm).toBeDefined() | |
| }) |
🤖 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 `@src/store/__tests__/actionOptions.test.ts` around lines 82 - 86, Remove the
misleading test case around actionOptionsConfigSchema.shape.confirm, including
its contradictory title and comments, since confirm is intentionally shared with
confirmOptionsConfigSchema and the toBeDefined assertion does not validate that
contract.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
First PR of
docs/changes/0027-scene-cards.md: oneActionCardfamily servingscene,script,buttonandinput_button, with a per-domain action map, activation feedback, script running state, theconfirmgate,showLastActivated, tier layouts and registry entries. The spec update is PR 2.This fixes a defect that was live on main
ButtonCardis the registry fallback for every unmapped domain and dispatchestoggle. All four of these domains reached it, and Home Assistant refusesturn_on/turn_off/toggleon the stateless ones — so every tap on a scene, button or input-button card was a call the backend rejected. Recorded on #230 as this change's to own.The mapping was verified against the running Home Assistant's own service registry rather than taken from the change document:
sceneoffersapply/create/delete/reload/turn_on,scriptoffersreload/toggle/turn_off/turn_on,buttonofferspress, andinput_buttonofferspress/reload.Two reviewer claims about Home Assistant's API were confidently wrong earlier in this wave — the fan
set_preset_modegate and the climatetemperature_unitattribute — and both would have shipped defects if implemented literally. Reading the live registry is now the habit for anything touching an HA contract.Dispatch is guarded from birth
These cards go through
dispatchGuardedrather than the retrying path, and were never on it. That is not a migration but the point of the guard: a retriedscript.turn_onruns the script twice, and a retriedbutton.pressfires whatever it is wired to twice — the exact harm#230names.Registry
Registered through
domainToCardwith the four domains added toSUPPORTED_DOMAINSandgetFriendlyDomain. The card does not importcardRegistry— that closes a cycle which fails with a temporal-dead-zone error depending on bundle entry order, and it broke the Storybook build once already.Testing
npm test— 3419 passednpm run lint,npm run typechecknpm run test:coverage— patch clean across all 9 changed source files, verified againstBRDAbranch entriesnpm run build-storybookandnpm run build:ha:prodSummary by CodeRabbit