Merge pull request #208 from richardthe3rd/claude/review-flutter-app-u7YmM

Reorganize docs: archive planning, add ADRs, update implementation status

Author: richardthe3rd

.gitignore

@@ -150,5 +150,6 @@ app.*.symbols
 !/dev/ci/**/Gemfile.lock
 
 # Keep manually created test mocks (build_runner has version compatibility issues)
-!test/*.mocks.dartscreenshots/
+!test/*.mocks.dart
+screenshots/
 test/failures/

AGENTS.md

@@ -351,12 +351,12 @@ void main() {
 
 ### API Documentation
 
-Full API documentation and JSON schemas are in `docs/api/`:
+Full API documentation and JSON schemas are in `docs/code/api/`:
 
-- **[docs/api/README.md](docs/api/README.md)** - Overview and quick reference
-- **[docs/api/data-api-reference.md](docs/api/data-api-reference.md)** - Complete API reference
-- **[docs/api/beer-list-schema.json](docs/api/beer-list-schema.json)** - JSON Schema for beverage data
-- **[docs/api/festival-registry-schema.json](docs/api/festival-registry-schema.json)** - JSON Schema for festival config
+- **[docs/code/api/README.md](docs/code/api/README.md)** - Overview and quick reference
+- **[docs/code/api/data-api-reference.md](docs/code/api/data-api-reference.md)** - Complete API reference
+- **[docs/code/api/beer-list-schema.json](docs/code/api/beer-list-schema.json)** - JSON Schema for beverage data
+- **[docs/code/api/festival-registry-schema.json](docs/code/api/festival-registry-schema.json)** - JSON Schema for festival config
 
 ### Validating festivals.json
 

README.md

@@ -128,10 +128,9 @@ Coverage fails if it drops below 70% overall, helping maintain code quality.
 
 ### E2E Testing
 
-- **Web E2E Tests**: Playwright tests for Flutter web builds - [Testing Flutter Web Guide](docs/tooling/flutter-web-testing.md)
-- **Mobile E2E Tests**: Patrol + Firebase Test Lab integration (planned) - [Testing Plan](docs/planning/patrol-firebase-testing/plan.md)
+- **Web E2E Tests**: Playwright tests for URL routing and accessibility smoke tests - [Testing Flutter Web Guide](docs/tooling/flutter-web-testing.md)
 
-See the [Patrol Firebase Testing Summary](docs/planning/patrol-firebase-testing/summary.md) for implementation status and plan overview.
+See [ADR 0005](docs/adr/0005-e2e-testing-strategy.md) for the rationale behind this approach.
 
 ## Data API
 
@@ -159,8 +158,6 @@ Technical documentation is available in the [docs](docs/) directory - see [docs/
 
 ### Testing & Quality
 - [Testing Flutter Web](docs/tooling/flutter-web-testing.md) - E2E testing with Playwright
-- [Patrol Firebase Testing Plan](docs/planning/patrol-firebase-testing/plan.md) - Mobile E2E testing strategy
-- [Patrol Testing Summary](docs/planning/patrol-firebase-testing/summary.md) - Quick overview and status
 
 ### Architecture & Deployment
 - [URL Routing](docs/code/routing.md) - Path-based routing implementation

docs/README.md

@@ -0,0 +1,79 @@
+# ADR 0004: Path-Based URL Strategy for Deep Linking
+
+**Status**: Accepted
+
+**Date**: 2025-12-21
+
+**Deciders**: Engineering Team
+
+**Context**: The app needed shareable, bookmarkable URLs for festival drinks, breweries, and styles. Two approaches were considered: hash-based URLs (`/#/drink/123`) and path-based URLs (`/drink/123`). The app was pre-release with no existing shared URLs or search engine indexing, so there were no backward-compatibility constraints.
+
+---
+
+## Decision
+
+We adopted **festival-scoped, path-based URLs** with GoRouter and `usePathUrlStrategy()`.
+
+### URL Structure
+
+```
+/{festivalId}                    → Festival home (drinks list)
+/{festivalId}/favorites          → Favorites for this festival
+/{festivalId}/drink/{drinkId}    → Drink detail
+/{festivalId}/brewery/{id}       → Brewery detail
+/{festivalId}/style/{styleName}  → Style detail (lowercase canonical)
+/{festivalId}/info               → Festival info
+/about                           → About (global, not festival-scoped)
+```
+
+### Key Design Choices
+
+1. **Festival ID as URL root** -- every drink/brewery/style URL is scoped to a festival, enabling cross-festival deep links
+2. **Lowercase canonical style URLs** -- `buildStylePath()` lowercases style names for consistent URLs
+3. **URL encoding** -- all user-provided IDs are encoded via `Uri.encodeComponent()`
+4. **Pre-release advantage** -- no redirect logic or legacy URL support needed
+
+---
+
+## Alternatives Considered
+
+### Hash-Based URLs (`/#/drink/123`)
+
+- Simpler: no server-side SPA routing config needed
+- Rejected because: poor SEO, unprofessional appearance, not shareable on social media
+
+### Flat URLs without festival scoping (`/drink/123`)
+
+- Simpler routing, fewer path segments
+- Rejected because: can't distinguish same drink ID across different festivals; can't share a link to "this year's festival"
+
+---
+
+## Consequences
+
+### Positive
+
+- Clean, shareable URLs that work on social media
+- Festival context is always visible in the URL
+- Browser back/forward works correctly
+- Bookmarks and shared links are self-contained
+
+### Negative
+
+- Requires SPA fallback routing on the server (Cloudflare Pages `_redirects` or `--proxy` flag on http-server)
+- Detail routes currently lack festival ID validation (documented as known limitation, see todos.md H3)
+- Festival selector UI doesn't update the URL when switching festivals (see todos.md C3)
+
+---
+
+## Implementation
+
+- **Router**: `lib/router.dart` (GoRouter configuration)
+- **URL builders**: `lib/utils/navigation_helpers.dart`
+- **E2E tests**: `test-e2e/routing.spec.ts` (Playwright URL smoke tests)
+- **Server config**: `--proxy` flag on http-server for SPA fallback
+
+## Related Documents
+
+- `docs/code/routing.md` -- current routing implementation details
+- `docs/code/navigation.md` -- navigation helper API reference

docs/adr/0004-path-based-url-strategy.md

@@ -0,0 +1,88 @@
+# ADR 0005: E2E Testing Strategy -- Playwright for URL Smoke Tests
+
+**Status**: Accepted
+
+**Date**: 2025-12-21
+
+**Deciders**: Engineering Team
+
+**Context**: The app needed automated end-to-end testing to validate routing and deep linking. Two approaches were evaluated: Patrol with Firebase Test Lab (native Flutter E2E on real Android devices) and Playwright (browser-based testing). Flutter web renders to a `<canvas>` element, which makes traditional DOM-based testing largely ineffective for UI interactions.
+
+---
+
+## Decision
+
+We adopted **Playwright for URL and routing smoke tests only**, and deferred native E2E testing.
+
+Playwright tests verify:
+- URL routing works (correct URLs after navigation)
+- Browser back/forward/refresh preserves routes
+- No critical console errors on page load
+- Basic ARIA label presence (accessibility smoke test)
+
+Playwright tests **do not** verify:
+- Visual appearance, layout, or rendered text
+- Widget interactions (tapping buttons, filling forms)
+- User flows (search, filter, favorite)
+- Canvas-rendered content
+
+Widget interactions are covered by Flutter's own `testWidgets` framework in `test/`.
+
+---
+
+## Alternatives Considered
+
+### Patrol + Firebase Test Lab
+
+A detailed plan was created (see `docs/planning/archive/patrol-firebase-testing/`) proposing:
+- Native Flutter E2E tests using the Patrol framework
+- Execution on real Android devices via Firebase Test Lab free tier (15 tests/day)
+- 4-5 week implementation timeline across 5 phases
+
+**Why it was not implemented:**
+- Significant setup complexity (Firebase Test Lab, GCP service accounts, Android instrumentation builds)
+- 4-5 week implementation investment for a pre-release app
+- Free tier limit (15 tests/day) constrains CI usage
+- Flutter widget tests already cover interaction flows effectively
+- The immediate need was validating URL routing for the deep linking feature, not full native E2E
+
+**When to reconsider:**
+- If the app ships on Android/iOS and needs device-specific testing (permissions, system dialogs, push notifications)
+- If visual regression testing becomes important
+- If Flutter widget tests prove insufficient for catching real-world bugs
+
+### Flutter Integration Tests
+
+Flutter's built-in `integration_test` package was considered but not prioritised. It would run the full app in a test harness and can interact with widgets directly. This remains a valid option for future investment (tracked in todos.md item #1).
+
+---
+
+## Consequences
+
+### Positive
+
+- Fast to implement (2 test files, ~440 lines)
+- Validates the most critical web concern: URL routing works correctly
+- Runs in CI without special infrastructure
+- ARIA label checks enforce accessibility as a side effect
+- No ongoing cost or quota limits
+
+### Negative
+
+- Cannot test actual user flows through the UI
+- Cannot verify that the correct screen renders for a given URL
+- Flutter canvas rendering means Playwright can never do meaningful UI testing for this app
+- Gap between "URL works" and "screen works" -- a route could return 200 but render an error state
+
+---
+
+## Implementation
+
+- **Config**: `playwright.config.ts`
+- **Tests**: `test-e2e/app.spec.ts` (loading, console errors, ARIA), `test-e2e/routing.spec.ts` (URL routing, browser history)
+- **Approach doc**: `docs/tooling/flutter-web-testing.md`
+
+## Related Documents
+
+- `docs/tooling/flutter-web-testing.md` -- how Playwright works with Flutter's canvas renderer
+- `docs/planning/archive/patrol-firebase-testing/` -- the Patrol evaluation that was not implemented

docs/adr/0005-e2e-testing-strategy.md

@@ -30,6 +30,8 @@ Each ADR follows this structure:
 | [0001](0001-github-actions-caching-strategy.md) | GitHub Actions Caching Strategy | Accepted | 2025-12-27 |
 | [0002](0002-composite-actions-and-test-deduplication.md) | Composite Actions and Test Deduplication | Accepted | 2025-12-27 |
 | [0003](0003-parallel-build-strategy.md) | Parallel Build Strategy for Android Releases | Accepted | 2025-12-27 |
+| [0004](0004-path-based-url-strategy.md) | Path-Based URL Strategy for Deep Linking | Accepted | 2025-12-21 |
+| [0005](0005-e2e-testing-strategy.md) | E2E Testing Strategy (Playwright for URL Smoke Tests) | Accepted | 2025-12-21 |
 
 ## Creating a New ADR
 

docs/adr/README.md

@@ -68,29 +68,37 @@ Comprehensive accessibility guidelines for the Cambridge Beer Festival app.
 
 ## Implementation Status
 
-### Current Status: ❌ NOT IMPLEMENTED
-
-**No accessibility features currently exist in the codebase.**
-
-Zero `Semantics` widgets found in:
-- ❌ `lib/widgets/drink_card.dart`
-- ❌ `lib/screens/drinks_screen.dart`
-- ❌ `lib/screens/festival_info_screen.dart`
-- ❌ `lib/main.dart`
-- ❌ `lib/widgets/star_rating.dart`
-
-**Impact:** App is currently unusable for screen reader users.
-
-### What Needs Implementation
-
-See [todos.md](../todos.md) item #6 for full details:
-- Favorite buttons need labels
-- Filter chips need state announcements
-- Navigation bar needs descriptive labels
-- Search interface needs proper semantics
-- Drink cards need summaries
-- Star ratings need value announcements
-- Action buttons need clear descriptions
+### Current Status: ✅ Implemented
+
+**53+ `Semantics` widgets** are implemented across the app, with **9 dedicated accessibility tests** in `test/accessibility_test.dart`.
+
+Coverage by file:
+- ✅ `lib/widgets/drink_card.dart` -- card semantic labels, favorite button semantics
+- ✅ `lib/screens/drinks_screen.dart` -- search clear button, filter chips
+- ✅ `lib/screens/festival_info_screen.dart` -- map, website, and GitHub buttons
+- ✅ `lib/main.dart` -- bottom navigation bar with descriptive labels for both tabs
+- ✅ `lib/widgets/star_rating.dart` -- parent rating label, individual star semantics
+- ✅ `lib/widgets/bottom_action_bar.dart` -- action button semantics
+- ✅ `lib/widgets/breadcrumb_bar.dart` -- back navigation semantics
+- ✅ `lib/widgets/overflow_menu.dart` -- menu button with `ExcludeSemantics` on decorative icons
+- ✅ `lib/widgets/info_chip.dart` -- chip semantics
+- ✅ `lib/widgets/festival_menu_sheets.dart` -- festival selector, settings, theme selector
+- ✅ `lib/widgets/environment_badge.dart` -- environment indicator semantics
+- ✅ `lib/screens/about_screen.dart` -- theme, GitHub, issues, licenses buttons
+- ✅ `lib/screens/drink_detail_screen.dart` -- action buttons, brewery link
+
+### Automated Tests
+
+`test/accessibility_test.dart` verifies:
+- Favorite button semantic labels (add/remove states)
+- ABV chip `ExcludeSemantics` for decorative elements
+- Card semantic structure and labels
+- Environment badge semantics
+- Button property (`button: true`) on interactive elements
+- Hint instructions for screen reader users
+- `ExcludeSemantics` usage on decorative icons
+- Filter chip selection state announcements
+- Retry button semantics on error states
 
 ---