refactor(reader): model scroll intent as a single ScrollAction BL-1901 - #190
Merged
solomonstorts merged 2 commits intoJul 10, 2026
Merged
Conversation
Replace the scrollToTop flag + scrollTargetReference pair with a single ScrollAction enum (.none/.top/.toVerse) as the source of truth, removing the same-run-loop coalescing hazard where scrollToTop=true was overwritten before SwiftUI observed it. Add finishChapterChange() as the single navigation exit and route the coordinator's clearScrollState() and goToReference's error path through it. Collapse the two scroll onChange handlers into one. No public API change.
andrewse02
force-pushed
the
ae/scroll-action
branch
from
July 10, 2026 14:29
bb9daff to
c99ecdd
Compare
Contributor
✅ Commit Lint: passedAll commit messages in this PR conform to Conventional Commits. 📦 Release preview:
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
andrewse02
force-pushed
the
ae/scroll-action
branch
from
July 10, 2026 18:12
8d5abc6 to
0a39396
Compare
Contributor
Code Coverage ReportCoverage after merging ae/scroll-action into ss/drive-reader-from-outside-BL-1901 will be
Coverage Report
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
solomonstorts
approved these changes
Jul 10, 2026
solomonstorts
merged commit Jul 10, 2026
6397fca
into
ss/drive-reader-from-outside-BL-1901
7 checks passed
solomonstorts
pushed a commit
that referenced
this pull request
Jul 13, 2026
#190) * refactor(reader): model scroll intent as a single ScrollAction BL-1901 Replace the scrollToTop flag + scrollTargetReference pair with a single ScrollAction enum (.none/.top/.toVerse) as the source of truth, removing the same-run-loop coalescing hazard where scrollToTop=true was overwritten before SwiftUI observed it. Add finishChapterChange() as the single navigation exit and route the coordinator's clearScrollState() and goToReference's error path through it. Collapse the two scroll onChange handlers into one. No public API change. * refactor(reader): route scroll-to-top settle through finishChapterChange BL-1901
solomonstorts
pushed a commit
that referenced
this pull request
Jul 13, 2026
#190) * refactor(reader): model scroll intent as a single ScrollAction BL-1901 Replace the scrollToTop flag + scrollTargetReference pair with a single ScrollAction enum (.none/.top/.toVerse) as the source of truth, removing the same-run-loop coalescing hazard where scrollToTop=true was overwritten before SwiftUI observed it. Add finishChapterChange() as the single navigation exit and route the coordinator's clearScrollState() and goToReference's error path through it. Collapse the two scroll onChange handlers into one. No public API change. * refactor(reader): route scroll-to-top settle through finishChapterChange BL-1901
solomonstorts
pushed a commit
that referenced
this pull request
Jul 16, 2026
#190) * refactor(reader): model scroll intent as a single ScrollAction BL-1901 Replace the scrollToTop flag + scrollTargetReference pair with a single ScrollAction enum (.none/.top/.toVerse) as the source of truth, removing the same-run-loop coalescing hazard where scrollToTop=true was overwritten before SwiftUI observed it. Add finishChapterChange() as the single navigation exit and route the coordinator's clearScrollState() and goToReference's error path through it. Collapse the two scroll onChange handlers into one. No public API change. * refactor(reader): route scroll-to-top settle through finishChapterChange BL-1901
solomonstorts
pushed a commit
that referenced
this pull request
Jul 17, 2026
#190) * refactor(reader): model scroll intent as a single ScrollAction BL-1901 Replace the scrollToTop flag + scrollTargetReference pair with a single ScrollAction enum (.none/.top/.toVerse) as the source of truth, removing the same-run-loop coalescing hazard where scrollToTop=true was overwritten before SwiftUI observed it. Add finishChapterChange() as the single navigation exit and route the coordinator's clearScrollState() and goToReference's error path through it. Collapse the two scroll onChange handlers into one. No public API change. * refactor(reader): route scroll-to-top settle through finishChapterChange BL-1901
solomonstorts
added a commit
that referenced
this pull request
Jul 17, 2026
…#174) * feat(reader): drive the reader to a passage via BibleReaderNavigation Add BibleReaderNavigation, a shared observable a host app uses to move an on-screen reader to a new passage from elsewhere — e.g. a "Read" button in another tab. Callers `request(reference, showsFullChapter:)`; the reader picks up the pending reference, loads that chapter in place via goToReference, and scrolls to the verse (building on the verse-scroll support from the prior commit). Adds the navigation init parameter to BibleReaderView, a Navigate tab to the sample app demonstrating it, and README docs. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(reader): persist showsFullChapter across cold launch BL-1901 A reader restored from storage (no explicit reference) dropped its showsFullChapter intent, so a passage last viewed as a full chapter came back as just its verse range after a cold launch. - Persist showsFullChapter to UserDefaults alongside reference - Restore it on the cold-launch branch of init; no verse scroll is armed on restore, so a user who scrolled within the chapter is not pulled back to the saved verse - Add persistence tests and clear the new key in test teardown Co-authored-by: Claude (AI Assistant) * docs(reader): fix navigation param label in README BL-1901 The BibleReaderView example used the old `navigation:` label; the parameter is `readerNavigation:`, so the snippet would not compile. Co-authored-by: Claude (AI Assistant) * fix(reader): release isChangingChapter after a verse scroll BL-1901 A full-chapter verse navigation (goToReference) armed a verse scroll, which suppresses the chapter-top scroll by setting scrollToTop back to false in the same run-loop turn. SwiftUI never observed the transient scrollToTop = true, so the onChange handler that clears isChangingChapter never fired and the flag stayed true — wedging handleScroll and leaving the header unresponsive to scrolling. - Clear isChangingChapter from VerseScrollCoordinator when the pending scroll resolves (landed, abandoned, or timed out), the moment that actually ends the chapter change for a verse scroll - Consolidate the scroll teardown into a single clearScrollState() - Assert isChangingChapter stays engaged until the scroll settles Co-authored-by: Claude (AI Assistant) * fix(reader): handle failed chapter load in goToReference BL-1901 goToReference committed showsFullChapter and armed the verse scroll around an await that can fail. On a cross-version navigation whose version fetch fails, onHeaderSelectionChange swallows the error and leaves the old reference in place, which caused three issues: the verse scroll armed against a stale reference, the new showsFullChapter value was committed (and persisted) despite the failed load, and isChangingChapter stayed true — permanently gating handleScroll and freezing the header chrome. - Reorder so onHeaderSelectionChange runs first; commit showsFullChapter and arm the scroll only once the requested chapter actually loaded - Reset isChangingChapter in the failure branch so the header stays responsive - Assert showsFullChapter and isChangingChapter in the failure test Co-authored-by: Claude (AI Assistant) * refactor(reader): model scroll intent as a single ScrollAction BL-1901 (#190) * refactor(reader): model scroll intent as a single ScrollAction BL-1901 Replace the scrollToTop flag + scrollTargetReference pair with a single ScrollAction enum (.none/.top/.toVerse) as the source of truth, removing the same-run-loop coalescing hazard where scrollToTop=true was overwritten before SwiftUI observed it. Add finishChapterChange() as the single navigation exit and route the coordinator's clearScrollState() and goToReference's error path through it. Collapse the two scroll onChange handlers into one. No public API change. * refactor(reader): route scroll-to-top settle through finishChapterChange BL-1901 * refactor(reader): apply review feedback on ScrollAction BL-1901 - Reword the ScrollAction doc comment to describe the type rather than the old scrollToTop/scrollTargetReference pattern it replaced - Rename the verse case from .toVerse to .reference - Say "reference" instead of "verse" in the scrollTargetReference doc Co-authored-by: Claude (AI Assistant) * fix: address review comments * refactor(reader): split reader init into explicit and restore paths BL-1901 BibleReaderView previously took an optional reference plus a showsFullChapter flag, and silently ignored the flag in favor of the persisted value whenever reference was nil. Replace that shape with two coherent entry points: - init(reference:...) now requires a non-optional reference, so a display mode can no longer be passed without the reference it describes - BibleReaderView.restoringLastPassage(...) is a self-describing factory that restores the last-viewed passage and its display mode (or falls back to John 1), with no showsFullChapter parameter to conflict with the restored state BibleReaderViewModel mirrors the split with two convenience inits delegating to a private designated init that keeps the original resolution logic. No behavior change; the sample app adopts the factory for its reader tab. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(reader): keep BibleReaderView(reference:) source-compatible BL-1901 Bryan's init split made BibleReaderView(reference:) non-optional, which would have broken existing callers who relied on the nil default (e.g. BibleReaderView(), BibleReaderView(onVerseTap:)). Restore source compatibility instead of shipping a breaking change: - Re-add the original init(reference: BibleReference? = nil, ...) as a deprecated overload that forwards to the designated init. Callers keep compiling; the deprecation message steers them to a non-optional reference or BibleReaderView.restoringLastPassage(). - Update README examples to use restoringLastPassage() for the restore case. Verified with swift-api-digester: no breaking changes against the shipped baseline, so this remains a minor release. The baseline file is refreshed separately in the post-release chore, per repo convention (e.g. commit 051a318 "chore: update api-baseline files after 5.3.0 release"). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(reader): rename clear-request test to a verb phrase BL-1901 Rename clearsPendingRequest() to clearPendingRequestSetsItToNil() so the test name reads as an action and outcome, matching the sibling tests (requestSetsPendingRequest, requestDefaultsToVerseRange). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Andrew Elliott <26266444+andrewse02@users.noreply.github.com> Co-authored-by: Bryan Montz <bryan.montz@youversion.com>
jhampton
pushed a commit
that referenced
this pull request
Jul 27, 2026
![github-actions[bot][release]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
## [5.4.0](5.3.0...5.4.0) (2026-07-27) ### Features * add backoff to refresh token process ([fbed37a](fbed37a)) * adjust highlight colors when in dark mode ([1f0e6b6](1f0e6b6)) * **reader:** drive the reader to a passage via BibleReaderNavigation ([#174](#174)) ([8b6382a](8b6382a)), closes [#190](#190) * **reader:** focus a verse in the Bible reader BL-1901 ([#200](#200)) ([21caa5a](21caa5a)) ### Bug Fixes * avoid data race while refreshing tokens ([b771cdc](b771cdc)) * language names list prefers the iOS name, but also sends Accept-Language ([193f47f](193f47f)) * split wall-of-text chapters into 10-verse chunks when necessary ([57053ff](57053ff))
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
2 participants
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.
What changes
scrollToTop: Bool+scrollTargetReference: BibleReference?into oneScrollActionenum (.none/.top/.toVerse). The two were mutually exclusive but independently mutable, sosetScrollTarget()had to hand-maintain the invariant. As one value they can't get out of sync — and it closes the same-run-loop coalescing gap wherescrollToTop = true(inonHeaderSelectionChange) was overwritten byscrollToTop = false(insetScrollTarget) before SwiftUI observed it.scrollTargetReferencestays as a read-only derived accessor, so the coordinator and tests read unchanged.finishChapterChange()(clears the armed scroll + resetsisChangingChapter) as the one place a navigation returns to rest. The coordinator'sclearScrollState()andgoToReference's error branch both route through it instead of pokingisChangingChapter/clearScrollTarget()independently.onChangehandler. The view's two handlers collapse into a single exhaustiveswitchonscrollAction.Intentionally left alone
The
Task.sleep(0.5s)scroll-to-top settle — it exists to ignore scroll-offset changes during the programmatic scroll animation (SwiftUI gives no scroll-completion callback), and fully event-driving it wants device testing. Flagged as a possible follow-up.No public API change
All changes are
internal/privateonBibleReaderViewModel(afinalinternal class);ScrollActionis internal. API-stability gate unaffected.Verification
VerseScroll,Navigation,NavigationState,Interaction); SwiftLint--strictclean.isChangingChapteris released after the verse-scroll path, which is exactly the single-exit guarantee.Totally your call whether to take it, adapt it, or toss it. Relates to BL-1901; follows up the review thread on #174.
Greptile Summary
This PR refactors the reader's scroll intent into a single
ScrollActionenum (.none/.top/.toVerse), replacing the two independently-mutable fieldsscrollToTop: BoolandscrollTargetReference: BibleReference?. The key win is eliminating a same-run-loop coalescing bug where a.topwrite could be overwritten by a.toVersearm before SwiftUI observed it, and introducingfinishChapterChange()as the single exit point that returns the reader to rest.ScrollActionenum eliminates the "can't arm both or neither" invariant thatsetScrollTarget()previously maintained by hand;scrollTargetReferenceis retained as a read-only computed accessor so existing callsites (coordinator, tests) are unaffected.finishChapterChange(clearingScroll:)unifies what were three independent mutation sites into one function; theclearingScroll: falseoverload correctly protects a re-armed scroll during the 0.5 s animation settle window.onChange(of: viewModel.scrollAction, initial: true)replaces two separate handlers and addsinitial: truecoverage for the cold-launch state, closing a minor gap in the previous implementation.Confidence Score: 5/5
Clean refactor with no regressions; all scroll paths reviewed and the single-exit invariant holds throughout.
All scroll intent mutations now flow through
ScrollActionandfinishChapterChange(). TheclearingScroll: falsepath correctly guards against clobbering a re-armed scroll during the async settle Task. Theinitial: trueaddition on the mergedonChangeis a net improvement. Unit suites cover the affected navigation paths.No files require special attention.
Important Files Changed
ScrollActionenum and replaces two independent fields with a single source of truth; addsclearScrollAction()andfinishChapterChange(clearingScroll:)as the unified resting-state exit point.scrollToTop = truesites replaced withscrollAction = .top; error-branchisChangingChapter = falseingoToReferencereplaced withfinishChapterChange(). Consistent and correct.onChangehandlers collapsed into one exhaustiveswitchoverscrollActionwithinitial: true;finishChapterChange(clearingScroll: false)correctly used in the 0.5 s settle Task.clearScrollState()simplified to a singlefinishChapterChange()call, removing the independent two-field mutation. No logic changes.scrollToTopassertions replaced withscrollAction == .top/scrollAction == .noneexpectations. Coverage unchanged.scrollAction == .topafteronHeaderSelectionChange; aligns correctly with the new model.Sequence Diagram
%%{init: {'theme': 'neutral'}}%% sequenceDiagram participant Nav as Navigation participant VM as BibleReaderViewModel participant View as BibleReaderView participant VSC as VerseScrollCoordinator Note over Nav,VSC: Chapter navigation (scroll to top) Nav->>VM: "scrollAction = .top" VM-->>View: onChange fires (.top) View->>View: scrollProxy.scrollTo(topOfContent) View->>VM: clearScrollAction() View->>View: Task.sleep(0.5s) View->>VM: finishChapterChange(clearingScroll: false) Note over Nav,VSC: Verse navigation Nav->>VM: "onHeaderSelectionChange -> scrollAction = .top" Nav->>VM: "setScrollTarget() -> scrollAction = .toVerse(ref)" VM-->>View: onChange fires (.toVerse) View->>VSC: handleScrollTarget(proxy) VSC->>VM: finishChapterChange() Note over Nav,VSC: Guard branch (reference mismatch) Nav->>VM: goToReference - guard fails VM->>VM: finishChapterChange()%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% sequenceDiagram participant Nav as Navigation participant VM as BibleReaderViewModel participant View as BibleReaderView participant VSC as VerseScrollCoordinator Note over Nav,VSC: Chapter navigation (scroll to top) Nav->>VM: "scrollAction = .top" VM-->>View: onChange fires (.top) View->>View: scrollProxy.scrollTo(topOfContent) View->>VM: clearScrollAction() View->>View: Task.sleep(0.5s) View->>VM: finishChapterChange(clearingScroll: false) Note over Nav,VSC: Verse navigation Nav->>VM: "onHeaderSelectionChange -> scrollAction = .top" Nav->>VM: "setScrollTarget() -> scrollAction = .toVerse(ref)" VM-->>View: onChange fires (.toVerse) View->>VSC: handleScrollTarget(proxy) VSC->>VM: finishChapterChange() Note over Nav,VSC: Guard branch (reference mismatch) Nav->>VM: goToReference - guard fails VM->>VM: finishChapterChange()Reviews (2): Last reviewed commit: "refactor(reader): route scroll-to-top se..." | Re-trigger Greptile