adaptive-cards-code-review

name	adaptive-cards-code-review
description	Use when finalizing a change in the Flutter-AdaptiveCards monorepo — before you propose, commit, or merge — as the manual or AI-assisted review gate. Checks monorepo hygiene, Adaptive Cards spec compliance, theming, widget keys, accessibility, and testing.

Code Review Protocol

Use this skill as a "Final Gate" for any PR or significant change. Cross-reference with specialized skills (adaptive-cards-spec-compliance, adaptive-cards-element-registry, adaptive-cards-public-api-docs, adaptive-cards-testing) as needed.

---

1. Monorepo Hygiene & Consistency

• FVM usage: Are all commands (flutter, dart) executed via fvm?
• Analysis: Does the code pass fvm flutter analyze? (Compliance with very_good_analysis).
• Changelog: Has CHANGELOG.md been updated in the affected packages following Keep a Changelog?
• Release / post-publish (if applicable): Follow adaptive-cards-release-engineer — all six version: fields match, six changelogs have matching top ## [<version>] sections, and flutter_adaptive_charts_fs and flutter_adaptive_cards_host_fs use flutter_adaptive_cards_fs: ^<version>.
• Formatting: Has dart_format been run on all modified files?
• Public /// docs: Do exported API changes in library packages explain why and how to use the API (not implementation steps)? See adaptive-cards-public-api-docs.

Documentation impact (canonical docs/)

Architecture docs drift silently. If this change touches any of the triggers below, git grep the affected symbols in docs/ and confirm the canonical docs were updated in this change (see the "Architecture documentation sync gate" in AGENTS.md).

• Providers / scopes: Did it add/remove/rename a Riverpod provider or ProviderScope (including nested scopes), or move a provider between scopes? → Update docs/reactive-riverpod.md (provider scopes + diagram) and docs/Architecture-Overview.md (scope diagram).
• Mixin contracts: Did it change what a mixin watches or how effective state is computed (e.g. AdaptiveVisibilityMixin.isVisible, AdaptiveInputMixin)? → Update the relevant section of docs/reactive-riverpod.md.
• HostConfig / element contracts: Did it add/remove/rename a HostConfig section, element/action type, or overlay field, or change a component's implementation/tests status? → Update docs/hostconfig.md, the owning package's README ## Implementation status table (packages/flutter_adaptive_cards_fs/README.md for core; charts/templating in their package READMEs), and docs/overlay-properties-by-type.md as applicable. docs/Implementation-Status.md is an index — edit it only for the project-level roadmap/history or pointers, not per-component status or known gaps (those live in the package READMEs).
• No stale references: git grep -n '<removed-or-renamed-symbol>' docs/ returns nothing pointing at deleted/renamed code (a stale doc reference is a blocker, not a nit).
• Diátaxis mode: If this change adds or edits a governed doc (canonical docs/*.md or a package README), does the doc stay within one Diátaxis quadrant, and does its doc_type: front matter (tutorial / how-to / reference / explanation) match? A newly introduced mode-mix in a previously-pure doc is drift to fix in this change, not a nit. Out-of-scope artifacts (plans/, reviews/, archive/, dated specs) are exempt. See the adaptive-cards-diataxis-docs skill.

---

2. flutter_adaptive_cards_fs Checklist

Registration & Pattern Compliance

• Registry: Is the new element/action added to CardTypeRegistry or ActionTypeRegistry?
• Mixins: Does the element correctly implement AdaptiveElementWidgetMixin (Widget) and AdaptiveElementMixin + AdaptiveVisibilityMixin (State)?
• Inputs: Does the input use AdaptiveInputMixin? On user change, does it call setDocumentInputValue(...)? Does it sync controllers in onDocumentValueChanged when overlays change (reset)?
• Visibility: If visibility can change at runtime, does the element use AdaptiveVisibilityMixin and write via setIsVisible / document notifier (not local-only setState)?
• TextBlock text: If runtime text can change, does AdaptiveTextBlock read display copy from resolvedElementProvider(id) (not only stale adaptiveMap['text'])?
• Actions: Do action buttons respect isEnabled via AdaptiveActionStateMixin / resolvedActionProvider when host-driven enable/disable matters?
• Overlays: Are host/runtime patches written through the document notifier (setInputValue, setText, setInputError, …) rather than mutating the host JSON map?
• Extension boundary: Does flutter_adaptive_cards_fs avoid chart-specific (or other optional-package) types, imports, and overlay fields? Optional behavior belongs in extension packages via addedElements / overlayExtensions, not in core.

Theming & Styling

• ReferenceResolver: Does the element use styleResolver (via ProviderScopeMixin) or ProviderScope.containerOf(context).read(styleReferenceResolverProvider) for all colors, font sizes, and spacing?
• Registries: Does the element use cardTypeRegistry / actionTypeRegistry from ProviderScopeMixin (not from ReferenceResolver)?
• Theme Awareness: Has it been verified in both Light and Dark modes?
• HostConfig: Does it respect spacing, separator, and padding properties from JSON via SeparatorElement?

Keys & Identity

• Deterministic Keys: Is the outer widget key generated via generateAdaptiveWidgetKey(adaptiveMap)?
• Internal Keys: For inputs or sub-elements, are keys generated using the id (e.g., ValueKey(id) or ValueKey('${id}_suffix'))? Crucial for testing stability.

Accessibility

Full contract and testing patterns: adaptive-cards-accessibility.

• Alt-text: Is author altText used as the accessible name — and is an absent altText passed through as null (decorative) rather than a placeholder string?
• Input labels: Is the control wrapped with labelInputSemantics() and the visible label wrapped in ExcludeSemantics? (Both, or the name is announced twice.)
• Live regions: Is transient validation/status text wrapped in Semantics(liveRegion: true)?
• Roles: Do headings use header: true + headingLevel, and do interactive targets expose both a name and a role?
• Test: Is there a semantics test, and does it call tester.ensureSemantics()?
• New strings: No new hardcoded user-visible string — including semanticsLabel: (see adaptive-cards-localization).

Exports

• Public API: Is the new class/widget exported in lib/flutter_adaptive_cards_fs.dart?
• Extension API: Is it exported in lib/flutter_adaptive_cards_extend.dart if intended for customization by consumers?
• Public /// docs: Do new or changed exported members explain why the API exists and how callers use it — not implementation steps? See adaptive-cards-public-api-docs.

---

3. flutter_adaptive_template_fs Checklist

• Expression Evaluation: Are new AST nodes or functions implemented in both the parser and the evaluator?
• Reserved Keywords: Are $data, $root, $index, and $when handled correctly during expansion?
• Scope Integrity: Does nesting $data correctly shift the resolution context?

---

4. flutter_adaptive_cards_host_fs Checklist

• Handlers: AdaptiveCardBackendHandlers shares the same GlobalKey<RawAdaptiveCardState> as RawAdaptiveCard when onSubmit / onExecute / onRefresh need card state.
• Adapters: PlainJson vs Teams invoke shapes match the backend contract; response effects applied via AdaptiveCardInvokeResponse.applyTo.
• Dependencies: pubspec.yaml declares flutter_adaptive_cards_fs: ^<monorepo-version> (sync on release bump).
• Tests: Unit tests under packages/flutter_adaptive_cards_host_fs/test/ (no goldens); run fvm flutter test from that package directory.
• Docs: Public API changes reflected in docs/backend-host-integration.md and package README.md; exported /// comments follow adaptive-cards-public-api-docs.

See adaptive-cards-backend-host skill for file paths and invoke round-trip patterns.

---

5. Testing Protocol

• Key-First Searching: All find.text() or find.byType() calls in tests should be replaced with find.byKey() whenever a key is available.
• JSON Samples: Is there a new file in test/samples/ demonstrating the feature/fix?
• Golden Tests:
  • Have golden tests been added/updated for UI changes?
  • New goldens: generate with --update-goldens on macOS, then copy each new PNG from test/gold_files/macos/ to test/gold_files/linux/ so CI has a baseline (see adaptive-cards-testing skill and test/gold_files/README.md).
  • CI pixel failures: replace linux/ files from CI artifact zips for canonical Linux images.
• Coverage gate: New untested code lowers a package toward its floor in tool/coverage_floors.yaml. The CI coverage gate (golden-excluded line coverage) must stay green — add tests rather than lowering a floor. Raising a floor after landing tests is fine; lowering one to pass is a red flag. See docs/testing-coverage.md.

widgetbook changes (sample app)

• Asset registration: New folders under widgetbook/lib/samples/ are listed in widgetbook/pubspec.yaml under flutter: assets: (required for AdaptiveCardsCanvas.asset).
• Use case / codegen: @widgetbook.UseCase added or updated in adaptive_cards_use_cases.dart; fvm dart run build_runner build run when use cases change.
• Changelog: widgetbook/CHANGELOG.md updated when the demo app changes.
• Docs: Overlay knob pages → registry row in docs/widgetbook-overlay-demos.md; package behavior docs tag widgetbook as Example (widgetbook sample) per documentation-scope.md.

---

Review Output Template

When performing a review, summarize findings using this format:

### Code Review Summary

- **Package**: [e.g. flutter_adaptive_cards_fs]
- **Hygiene**: [PASS/FAIL] (Analysis, Changelog, Formatting)
- **Docs impact**: [PASS/FAIL/N-A] (canonical `docs/` synced; no stale references)
- **Compliance**: [PASS/FAIL] (Registry, Spec, Theming)
- **Testing**: [PASS/FAIL] (Keys, Samples, Goldens)

#### Details

- [Specific feedback regarding keys, accessibility, etc.]