Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc617deeaf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| @override | ||
| Widget build(BuildContext context) { | ||
| WidgetsBinding.instance.addPostFrameCallback((_) => _reportActivity()); |
There was a problem hiding this comment.
When an onActivity consumer updates parent or store state, that update rebuilds PapyrusReader, this unconditional post-frame callback emits a fresh event, and the consumer schedules another rebuild; the cycle can continue every frame even though reader state is unchanged. Deduplicate activity state or schedule reporting only from actual controller, panel, and route transitions rather than from every build.
Useful? React with 👍 / 👎.
| _panel == _ReaderPanel.none && | ||
| _compactPanelRoute == null && | ||
| (ModalRoute.of(context)?.isCurrent ?? true), | ||
| cause: _controller.navigationCause, |
There was a problem hiding this comment.
When the reader moves through a direct EPUB swipe/scroll or a pdfrx position update, the engine reaches _syncFromEngine without invoking any controller navigation method, so navigationCause remains restore or the previous command and this event reports a stale cause. Hosts will consequently misclassify ordinary reading activity, while ReaderNavigationCause.viewport is never emitted; engine-originated position changes need to select that cause unless they belong to a currently scoped command.
Useful? React with 👍 / 👎.
| final class ReaderActivityEvent { | ||
| const ReaderActivityEvent({ |
There was a problem hiding this comment.
The newly exported ReaderActivityEvent and its nested ReaderContentCoverage provide no toJson/fromJson representation, so hosts cannot serialize the new host-facing observations through the package's domain API and must invent incompatible enum and field encodings. Add a stable versioned serialization format for both types.
AGENTS.md reference: AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
| final intersection = rect.intersect(visible); | ||
| if (intersection.width * intersection.height >= | ||
| rect.width * rect.height * .25) { | ||
| exposed.add(i); |
There was a problem hiding this comment.
When a PDF page is zoomed enough that the viewport covers less than 25% of the page, this filter returns no exposed pages even though the user is actively reading a visible portion. PdfReaderEngine then derives contentReady: false, which also forces activity visible to false and stops host time tracking throughout zoomed reading; readiness should remain true after the viewer is ready even if the stable-coverage threshold excludes the current fragment.
Useful? React with 👍 / 👎.
| List<ReaderTocEntry> toc = const [], | ||
| this.locationLabel, | ||
| this.coverage = const [], | ||
| this.contentReady = true, | ||
| this.atEnd = false, | ||
| }) : toc = List.unmodifiable(toc); |
There was a problem hiding this comment.
When a host modifies snapshot.coverage or the same list received through ReaderActivityEvent, this constructor has retained the caller's mutable list rather than copying it as it does for toc. For EPUB this can also mutate the engine's _coverage list directly, silently changing the published snapshot without notification and corrupting subsequent activity observations; wrap coverage with List.unmodifiable in the initializer.
Useful? React with 👍 / 👎.
Summary
Expose optional, Goals-independent activity observations from the reader: content readiness, visibility, stable EPUB coverage, exposed PDF pages/spreads, navigation cause, and end-of-document state. Existing locator callbacks, version-1 locators, custom engines, and worker parsing remain compatible. Coverage reporting preserves exact EPUB content offsets during restoration/reflow.
Testing
Compatibility and migration
Additive callback and snapshot fields; no Goals dependency or package version change. Target
development. Merge before the client pins this revision.Notes
The host owns clocks, lifecycle, persistence, calibration, aggregation, and explicit completion. EPUB coverage is chapter-local normalized content, not screen pagination.
Coordinated server #12 and workspace integration pin these changes without a version bump.
Workspace integration #13 pins all three component revisions and documents release order.