[EPAC-1940]: fix Parliament calendar retap scroll#494
Conversation
|
Symphony expected a reviewer-bot review for the current autonomous PR head within the configured SLA window, but none was found.
|
There was a problem hiding this comment.
ReviewAutonomousPR
- Verdict:
request_changes - Reviewer boundary:
review_only - Acceptance criteria coverage: covered=7, missing=1, unclear=1
Summary
The code changes are directionally correct and scoped to the calendar scroll view, but the PR does not satisfy the required verification/DoD for this UI fix: it explicitly omits before/after screenshots and does not show the required iPad/manual retap verification.
Actionable findings
- required / required_autonomous_fix — Required UI verification artifacts are missing
- The issue Definition of Done requires before/after screenshots for the Parliament retap behavior, Parliament toolbar, and Members toolbar, plus manual verification on iPhone and iPad. The PR body says screenshots were not captured and only documents a simulator launch, so the core user-visible behavior and toolbar de-cluttering are not evidenced before merge. Capture and attach the required artifacts and document the iPad verification result.
- Actionability:
required_autonomous_fix
Acceptance criteria coverage
- unclear — Parliament tab re-tap no longer scrolls to January 2001
- The implementation disables HorizonCalendar's internal CalendarScrollView scrollsToTop, which should address the root cause, but the PR does not include the required retap verification evidence.
- Actionability:
required_autonomous_fix - Evidence: ios/epac/Views/Calendar/CalendarScrollsToTopDisabler.swift; PR body notes screenshots were not captured.
- covered — Prominent Today affordance scrolls to and centers today
- A visible Today button calls scrollToDay(containing: .now, scrollPosition: .centered, animated: true).
- Actionability:
none - Evidence: ios/epac/Views/Calendar/SittingCalendarView.swift:107
- covered — Legend-dot tap target removed so Today has one canonical action
- The old onTapGesture and button accessibility traits were removed from the legend item.
- Actionability:
none - Evidence: ios/epac/Views/Calendar/SittingCalendarView.swift:138
- covered — Today control hidden or disabled while today is visible
- The button is disabled based on the visible day range updated through HorizonCalendar onScroll.
- Actionability:
none - Evidence: ios/epac/Views/Calendar/SittingCalendarView.swift:114
- covered — Parliament toolbar is de-cluttered
- Order Paper, Committees, and Ontario Debates are grouped into one menu, reducing toolbar slots.
- Actionability:
none - Evidence: ios/epac/Views/Calendar/SittingCalendarView.swift:271
- covered — Members toolbar is de-cluttered
- Party, Province, Status, and Cabinet filters are consolidated into one Filters menu with an active-count badge, while the parties menu remains separate.
- Actionability:
none - Evidence: ios/epac/Views/Members/MembersView.swift:47
- covered — Do not blanket-disable scrollsToTop elsewhere
- The opt-out matches only UIScrollView instances whose runtime class name is CalendarScrollView, and the regression test confirms ordinary UIScrollView instances remain opted in.
- Actionability:
none - Evidence: ios/epac/Views/Calendar/CalendarScrollsToTopDisabler.swift:28; ios/epacTests/CalendarScrollsToTopDisablerTests.swift:7
- covered — New scroll behavior test added and existing targeted tests pass
- New CalendarScrollsToTopDisablerTests and MembersViewModelTests coverage were added; the PR body reports the targeted test command passed.
- Actionability:
none - Evidence: ios/epacTests/CalendarScrollsToTopDisablerTests.swift; ios/epacTests/MembersViewModelTests.swift
- missing — Manual verification on iPhone simulator and iPad plus screenshots
- The PR explicitly states screenshots were not captured and does not document iPad verification of the retap behavior or toolbar layouts.
- Actionability:
required_autonomous_fix - Evidence: PR body: 'Screenshots were not captured in this autonomous run.'
There was a problem hiding this comment.
ReviewAutonomousPR
- Verdict:
request_changes - Reviewer boundary:
review_only - Acceptance criteria coverage: covered=7, missing=2, unclear=1
Summary
The implementation is close, but two acceptance-contract gaps remain: the iPad verification still did not pass, and the new scrollsToTop regression test uses a fake CalendarScrollView so it does not protect the actual HorizonCalendar integration.
Actionable findings
- required / required_autonomous_fix — Required iPad verification still has not passed (
docs/review-artifacts/EPAC-1940/README.md:25)- The issue Definition of Done requires manual verification on iPad. The committed review artifact documents only failed iPad launch attempts and explicitly says no valid in-app iPad screenshot was captured. This is not a human-only gate; the PR owner should rerun iPad simulator verification successfully or document a concrete, reproducible environment blocker with command evidence.
- Actionability:
required_autonomous_fix
- required / required_autonomous_fix — Scroll regression test does not exercise the real HorizonCalendar scroll view (
ios/epacTests/CalendarScrollsToTopDisablerTests.swift:31)- The acceptance criteria call for a test that SittingCalendarView does not opt the underlying scroll view into scrollsToTop, and the implementation relies on HorizonCalendar's internal runtime class name. The added test defines its own private class named CalendarScrollView, so it will continue passing even if HorizonCalendar renames its actual internal class or the SwiftUI hierarchy changes and the helper no longer reaches the real calendar scroll view. Add coverage that renders the real calendar/HorizonCalendar view hierarchy or otherwise asserts against the actual dependency used in production.
- Actionability:
required_autonomous_fix
Acceptance criteria coverage
- unclear — Parliament tab re-tap does not scroll to January 2001
- The scoped scrollsToTop opt-out should address the root cause, but iPad verification remains missing and the test does not exercise the real HorizonCalendar scroll view.
- Actionability:
required_autonomous_fix - Evidence: ios/epac/Views/Calendar/CalendarScrollsToTopDisabler.swift:30; ios/epacTests/CalendarScrollsToTopDisablerTests.swift:31
- covered — Prominent Today affordance scrolls to and centers today
- The visible Today button calls scrollToDay(containing: .now, scrollPosition: .centered, animated: true).
- Actionability:
none - Evidence: ios/epac/Views/Calendar/SittingCalendarView.swift:107
- covered — Legend-dot tap target removed so Today has one canonical action
- The hidden tap gesture and button traits were removed from the legend item.
- Actionability:
none - Evidence: ios/epac/Views/Calendar/SittingCalendarView.swift:142
- covered — Today control hidden or disabled when today is already visible
- The button is disabled from isTodayVisible, updated from HorizonCalendar's visible day range.
- Actionability:
none - Evidence: ios/epac/Views/Calendar/SittingCalendarView.swift:98; ios/epac/Views/Calendar/SittingCalendarView.swift:114
- covered — Parliament toolbar is de-cluttered
- Order Paper, Committees, and Ontario Debates are grouped into one menu.
- Actionability:
none - Evidence: ios/epac/Views/Calendar/SittingCalendarView.swift:271
- covered — Members toolbar is de-cluttered
- Party, Province, Status, and Cabinet filters are consolidated into one Filters menu with active count.
- Actionability:
none - Evidence: ios/epac/Views/Members/MembersView.swift:45
- covered — Do not blanket-disable scrollsToTop across the app
- The helper only changes UIScrollView instances whose runtime class name is CalendarScrollView.
- Actionability:
none - Evidence: ios/epac/Views/Calendar/CalendarScrollsToTopDisabler.swift:33
- covered — Tab re-tap may optionally scroll to today or do nothing
- The PR chooses the explicit Today button path and does not add a global tab re-selection interceptor.
- Actionability:
none - Evidence: PR body trade-offs; ios/epac/Views/Calendar/CalendarScrollsToTopDisabler.swift:17
- missing — Existing targeted tests pass and new scroll behavior test added
- A test was added, but it uses a fake local CalendarScrollView and does not verify the actual SittingCalendarView/HorizonCalendar scroll view behavior requested by the AC.
- Actionability:
required_autonomous_fix - Evidence: ios/epacTests/CalendarScrollsToTopDisablerTests.swift:31
- missing — Manual verification on iPhone simulator and iPad plus screenshots
- iPhone artifacts are present, but iPad verification did not pass and no in-app iPad evidence is included.
- Actionability:
required_autonomous_fix - Evidence: docs/review-artifacts/EPAC-1940/README.md:25
Why
The Parliament calendar inherited UIKit
scrollsToTopbehavior from HorizonCalendar's internal scroll view, so re-tapping the active Parliament tab could send users to January 2001. That range is intentionally configured but not useful for House sitting data, and the previous Today action was hidden on the legend dot.What changed
CalendarScrollViewby runtime class name and sets only that scroll view'sscrollsToToptofalse..centered; it stays visible from the default state and disables itself while today is visible.scrollsToTopopt-out and Members active filter count.docs/review-artifacts/EPAC-1940/for the requested before/after toolbar comparisons and fixed Parliament post-retap state.Trade-offs not taken
Test plan
cd ios && xcodebuild test -quiet -project epac.xcodeproj -scheme epac -destination 'platform=iOS Simulator,name=iPhone 17 Pro' -only-testing:epacTests/MembersViewModelTests/activeFilterCountIncludesEveryNonDefaultFilter -only-testing:epacTests/MembersViewModelTests/activeFilterCountIsZeroByDefault -only-testing:epacTests/CalendarScrollsToTopDisablerTests/disablesOnlyCalendarScrollViewScrollsToTop -only-testing:epacTests/CalendarScrollsToTopDisablerTests/reportsNoCalendarScrollViewWhenHierarchyDoesNotContainOne(pass)swiftlint --strict(pass, 0 violations)python3 scripts/localization/check_localizations.py --github-warnings(pass exit 0; existing warnings foronboarding.welcome.title)cd ios && make build(pass; existing SwiftData macroRecordedVote/Sendablewarning)cd ios && make simulator(pass; installed and launched on simulator81896E8A-6231-4D20-956D-01CD486CACD9)cd ios && make test(fails outside this change: broad snapshot mismatches,FollowPreferenceAdapterTests.savedMemberNameReflectsPostalCodeStorepersisted-default expectation,AppPreviewRecordingTests.testAppPreviewRecordingSequence, andepacUITests.testAccountabilityTab_BillsListVisible; changed regression tests passed inside this run)xcrun simctl io ... screenshot; artifacts are committed indocs/review-artifacts/EPAC-1940/.EPAC-1940 iPad 26.0(ED697A38-9F0B-445F-A561-97223F9DEAEF) and fresh iPad simulators (0A486EA1-320F-4C83-98CC-998C8BC808E7,CF8374B4-3668-45D1-9ACF-498D76DA32E6), but localxcrun simctl launch ... net.dinglebox.cabinetdoorrepeatedly hung and never foregrounded the app. I did not include a SpringBoard-only frame as app evidence.Screenshots (if UI)
Reviewer-Boundary: review-only
Resolves EPAC-1940
Release-Note: Added a Today button to the Parliament calendar and stopped tab re-tap from scrolling to 2001