Fix race condition when applying EPUB decorations#765
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses a decoration rendering race in the EPUB navigator when apply(decorations:in:) is called rapidly for the same decoration group, aiming to prevent simultaneous/duplicate highlights.
Changes:
- Introduces per-decoration-group task cancellation to supersede in-flight decoration applications.
- Refactors decoration group identifiers to use a shared
DecorationGrouptypealias across the Navigator and SpreadView delegate APIs. - Updates the changelog to document the race condition fix.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Sources/Navigator/EPUB/EPUBSpreadView.swift | Updates decoration-activation delegate signature to use DecorationGroup. |
| Sources/Navigator/EPUB/EPUBNavigatorViewController.swift | Adds per-group task tracking/cancellation and migrates decoration storage/callbacks to DecorationGroup. |
| Sources/Navigator/Decorator/DecorableNavigator.swift | Updates DecorableNavigator API to use DecorationGroup and defines the typealias. |
| CHANGELOG.md | Documents the race-condition fix. |
Comments suppressed due to low confidence (1)
Sources/Navigator/EPUB/EPUBNavigatorViewController.swift:848
self.decorations[group]is updated totargetbefore any JavaScript updates are applied. If this task is cancelled after the assignment (e.g. by a subsequentapplyfor the same group), the in-memory source of truth will advance while the web views may not have been updated, and the next diff will be computed from the wrongsource, potentially skipping needed updates/clears. Consider only committingself.decorations[group] = targetafter the task group completes successfully (and wasn’t cancelled), or reverting tosourcewhen cancellation happens.
let source = self.decorations[group] ?? []
let target = decorations.map {
var d = $0
d.locator = publication.normalizeLocator(d.locator)
return DiffableDecoration(decoration: d)
}
self.decorations[group] = target
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
Sources/Navigator/EPUB/EPUBNavigatorViewController.swift:855
apply(decorations:in:)updatesself.decorations[group] = targetbefore any JavaScript is actually executed. If this task gets cancelled after that assignment (e.g. a newerapplycall cancels it), the in-memorydecorationsstate can diverge from what’s rendered in the web views. The nextapplywill compute a diff from the updated in-memory state (which may never have been applied) and can end up issuing.update(...)operations for decorations that don’t exist in the DOM, leaving decorations missing or stale. Consider only committingself.decorations[group] = targetonce the task group finished applying scripts and the task wasn’t cancelled, or track “last applied” vs “latest requested” decorations separately so diffs are computed from the actually-rendered state.
let source = self.decorations[group] ?? []
let target = decorations.map {
var d = $0
d.locator = self.publication.normalizeLocator(d.locator)
return DiffableDecoration(decoration: d)
}
self.decorations[group] = target
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* upstream/develop: Fix decoration positions with paragraph indent (readium#767) Replace SwiftSoup with regex-based attribute lookup in Readium CSS injection (readium#742) Add support for SVG covers (readium#751) Fix race condition when applying EPUB decorations (readium#765) Add slide animation for adjacent EPUB resource transitions (readium#763) Remove unavailable APIs (readium#759) Remove Carthage support (readium#760) Improve `DirectionalNavigationAdapter` (readium#757) Update JSON usage from `Any` to `JSONValue` (readium#740) Fix LCP-enabled TestApp (readium#754) Report continuous `totalProgression` in the EPUB locator (readium#753) Document device name behavior for LCP registration (readium#752) Fix screen flashes during unanimated page turns in the EPUB navigator (readium#750) Fix parsing of URI templates (readium#749) Fix EPUB contributor role parsing (readium#748) Introduce the Readium Swift Playground (readium#746) # Conflicts: # Sources/Navigator/EPUB/Assets/Static/scripts/readium-fixed.js # Sources/Navigator/EPUB/Assets/Static/scripts/readium-reflowable.js
Fixed
Navigator
EPUBNavigatorViewControllerwhere rapidly callingapply(decorations:in:)for the same group could cause multiple highlights to appear simultaneously.