fix: ActivityManager tweaks#1220
Merged
ngxson merged 1 commit intocrosspoint-reader:masterfrom Feb 27, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR refactors access control for Activity and ActivityManager members by making them private with friend declarations, adds thread-safety synchronization via RenderLock guards in rendering code paths, replaces direct activity stack manipulation with finish() calls across multiple activities, and ensures UI updates in error handling paths. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/activities/reader/EpubReaderActivity.cpp (1)
314-319:⚠️ Potential issue | 🟠 MajorLock coverage is partial; move predicate reads inside the lock.
At Line 314 and Line 416, shared state is read before acquiring
RenderLock. This leaves a check-then-act race with the render task. Lock first, then evaluate/update.Suggested fix
- if (!result.isCancelled && currentSpineIndex != std::get<ChapterResult>(result.data).spineIndex) { - RenderLock lock(*this); - currentSpineIndex = std::get<ChapterResult>(result.data).spineIndex; - nextPageNumber = 0; - section.reset(); - } + if (!result.isCancelled) { + const auto& chapter = std::get<ChapterResult>(result.data); + RenderLock lock(*this); + if (currentSpineIndex != chapter.spineIndex) { + currentSpineIndex = chapter.spineIndex; + nextPageNumber = 0; + section.reset(); + } + }- if (currentSpineIndex != sync.spineIndex || (section && section->currentPage != sync.page)) { - RenderLock lock(*this); - currentSpineIndex = sync.spineIndex; - nextPageNumber = sync.page; - section.reset(); - } + RenderLock lock(*this); + if (currentSpineIndex != sync.spineIndex || (section && section->currentPage != sync.page)) { + currentSpineIndex = sync.spineIndex; + nextPageNumber = sync.page; + section.reset(); + }Also applies to: 416-421
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activities/reader/EpubReaderActivity.cpp` around lines 314 - 319, The predicate reads (e.g., result.isCancelled and currentSpineIndex comparisons) are done before acquiring the RenderLock, causing a check-then-act race with the render task; to fix, move the reads and any dependent state updates inside the RenderLock scope: acquire RenderLock before inspecting result.isCancelled and std::get<ChapterResult>(result.data).spineIndex, then update currentSpineIndex, nextPageNumber and section while holding the lock (same change for the other occurrence around the block that touches currentSpineIndex/nextPageNumber/section at lines ~416-421), ensuring all shared-state reads and writes are protected by RenderLock.
🧹 Nitpick comments (1)
src/activities/ActivityManager.h (1)
53-56:renderingMutexaccess is still broader than the stated intent.Given the comment says it must only be used via
RenderLock, keeping itprotectedstill exposes it to subclasses. Consider moving just this member toprivate.Suggested tweak
class ActivityManager { friend class RenderLock; protected: GfxRenderer& renderer; MappedInputManager& mappedInput; std::vector<std::unique_ptr<Activity>> stackActivities; std::unique_ptr<Activity> currentActivity; @@ - // Mutex to protect rendering operations from race conditions - // Must only be used via RenderLock - SemaphoreHandle_t renderingMutex = nullptr; + private: + // Mutex to protect rendering operations from race conditions. + // Must only be used via RenderLock. + SemaphoreHandle_t renderingMutex = nullptr; + + protected:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/activities/ActivityManager.h` around lines 53 - 56, The member renderingMutex in ActivityManager is currently protected but the comment mandates it be used only via RenderLock; move the renderingMutex declaration into the private section of ActivityManager and update accessors: either declare RenderLock as a friend (friend class RenderLock) or add a private getter (e.g., SemaphoreHandle_t& getRenderingMutex()) used by RenderLock, then update all direct references to ActivityManager::renderingMutex to use the friend or getter so subclasses no longer access it directly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/activities/reader/EpubReaderActivity.cpp`:
- Around line 314-319: The predicate reads (e.g., result.isCancelled and
currentSpineIndex comparisons) are done before acquiring the RenderLock, causing
a check-then-act race with the render task; to fix, move the reads and any
dependent state updates inside the RenderLock scope: acquire RenderLock before
inspecting result.isCancelled and
std::get<ChapterResult>(result.data).spineIndex, then update currentSpineIndex,
nextPageNumber and section while holding the lock (same change for the other
occurrence around the block that touches
currentSpineIndex/nextPageNumber/section at lines ~416-421), ensuring all
shared-state reads and writes are protected by RenderLock.
---
Nitpick comments:
In `@src/activities/ActivityManager.h`:
- Around line 53-56: The member renderingMutex in ActivityManager is currently
protected but the comment mandates it be used only via RenderLock; move the
renderingMutex declaration into the private section of ActivityManager and
update accessors: either declare RenderLock as a friend (friend class
RenderLock) or add a private getter (e.g., SemaphoreHandle_t&
getRenderingMutex()) used by RenderLock, then update all direct references to
ActivityManager::renderingMutex to use the friend or getter so subclasses no
longer access it directly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/activities/Activity.hsrc/activities/ActivityManager.cppsrc/activities/ActivityManager.hsrc/activities/browser/OpdsBookBrowserActivity.cppsrc/activities/network/CalibreConnectActivity.cppsrc/activities/reader/EpubReaderActivity.cppsrc/activities/settings/CalibreSettingsActivity.cppsrc/activities/settings/KOReaderSettingsActivity.cppsrc/activities/settings/OtaUpdateActivity.cpp
📜 Review details
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: Levrk
Repo: crosspoint-reader/crosspoint-reader PR: 909
File: src/activities/reader/EpubReaderActivity.cpp:461-461
Timestamp: 2026-02-16T20:43:19.339Z
Learning: In src/activities/reader/EpubReaderActivity.cpp, when using ConfirmationActivity with enterNewActivity(), it's not necessary to call exitActivity() beforehand. The ConfirmationActivity operates as a modal dialog and cleanup is handled via the pendingSubactivityExit flag in the callback lambda. This differs from other activities like EpubReaderChapterSelectionActivity or KOReaderSyncActivity which do require exitActivity() before enterNewActivity().
📚 Learning: 2026-02-16T20:43:19.339Z
Learnt from: Levrk
Repo: crosspoint-reader/crosspoint-reader PR: 909
File: src/activities/reader/EpubReaderActivity.cpp:461-461
Timestamp: 2026-02-16T20:43:19.339Z
Learning: In src/activities/reader/EpubReaderActivity.cpp, when using ConfirmationActivity with enterNewActivity(), it's not necessary to call exitActivity() beforehand. The ConfirmationActivity operates as a modal dialog and cleanup is handled via the pendingSubactivityExit flag in the callback lambda. This differs from other activities like EpubReaderChapterSelectionActivity or KOReaderSyncActivity which do require exitActivity() before enterNewActivity().
Applied to files:
src/activities/settings/CalibreSettingsActivity.cppsrc/activities/settings/KOReaderSettingsActivity.cppsrc/activities/browser/OpdsBookBrowserActivity.cppsrc/activities/settings/OtaUpdateActivity.cppsrc/activities/network/CalibreConnectActivity.cppsrc/activities/reader/EpubReaderActivity.cpp
📚 Learning: 2026-02-23T06:18:08.408Z
Learnt from: raygan
Repo: crosspoint-reader/crosspoint-reader PR: 1116
File: src/activities/settings/WallabagSettingsActivity.cpp:57-75
Timestamp: 2026-02-23T06:18:08.408Z
Learning: When navigating from a settings activity to KeyboardEntryActivity (e.g., KOReaderSettingsActivity, WallabagSettingsActivity), call exitActivity() on the parent before calling enterNewActivity(new KeyboardEntryActivity(...)). The KeyboardEntryActivity should then call exitActivity() via its callbacks to return to the parent. Note: ConfirmationActivity does not require exitActivity() before entering KeyboardEntryActivity. Apply this pattern to similar settings activities in this module.
Applied to files:
src/activities/settings/CalibreSettingsActivity.cppsrc/activities/settings/KOReaderSettingsActivity.cppsrc/activities/settings/OtaUpdateActivity.cpp
📚 Learning: 2026-02-12T06:57:35.955Z
Learnt from: CaptainFrito
Repo: crosspoint-reader/crosspoint-reader PR: 725
File: src/activities/network/CalibreConnectActivity.cpp:218-264
Timestamp: 2026-02-12T06:57:35.955Z
Learning: In src/activities/network/CalibreConnectActivity.cpp, button hints (« Exit) are intentionally omitted from the SERVER_STARTING and ERROR states—only the SERVER_RUNNING state displays navigation hints. This is a deliberate design decision by the author.
Applied to files:
src/activities/settings/CalibreSettingsActivity.cppsrc/activities/settings/KOReaderSettingsActivity.cppsrc/activities/browser/OpdsBookBrowserActivity.cppsrc/activities/network/CalibreConnectActivity.cpp
📚 Learning: 2026-02-22T19:13:22.166Z
Learnt from: ngxson
Repo: crosspoint-reader/crosspoint-reader PR: 1016
File: src/activities/reader/EpubReaderActivity.cpp:138-146
Timestamp: 2026-02-22T19:13:22.166Z
Learning: In src/activities/reader/EpubReaderActivity.cpp, the EpubReaderMenuActivity result callback intentionally applies orientation changes (via applyOrientation) before checking result.isCancelled. This differs from other callbacks in the file because orientation is treated as an immediate setting that should persist even when the user cancels the menu, rather than a deferred action requiring confirmation.
Applied to files:
src/activities/settings/CalibreSettingsActivity.cppsrc/activities/settings/KOReaderSettingsActivity.cppsrc/activities/reader/EpubReaderActivity.cpp
📚 Learning: 2026-02-21T16:47:45.578Z
Learnt from: Uri-Tauber
Repo: crosspoint-reader/crosspoint-reader PR: 1031
File: src/activities/reader/EpubReaderActivity.cpp:816-853
Timestamp: 2026-02-21T16:47:45.578Z
Learning: In src/activities/reader/EpubReaderActivity.cpp, the navigateToHref() function uses a fixed-size mini-stack (MAX_FOOTNOTE_DEPTH = 3) for saved positions rather than a dynamic stack. This is intentional to minimize RAM usage on ESP32-C3, which has limited memory. The design accepts that failed navigation at max depth may decrement footnoteDepth even when no push occurred, as a tradeoff for memory efficiency. Users exit by pressing Back three times maximum.
Applied to files:
src/activities/settings/CalibreSettingsActivity.cppsrc/activities/settings/KOReaderSettingsActivity.cppsrc/activities/reader/EpubReaderActivity.cpp
📚 Learning: 2026-02-23T06:18:15.143Z
Learnt from: raygan
Repo: crosspoint-reader/crosspoint-reader PR: 1116
File: src/activities/settings/WallabagSettingsActivity.cpp:57-75
Timestamp: 2026-02-23T06:18:15.143Z
Learning: In settings activities (e.g., KOReaderSettingsActivity, WallabagSettingsActivity), the established pattern for launching KeyboardEntryActivity is to call exitActivity() on the parent settings activity before enterNewActivity(new KeyboardEntryActivity(...)). The lambda callbacks then call exitActivity() to exit the KeyboardEntryActivity and return to the parent. This differs from ConfirmationActivity which does not require exitActivity() before enterNewActivity().
Applied to files:
src/activities/network/CalibreConnectActivity.cpp
📚 Learning: 2026-02-19T17:46:26.871Z
Learnt from: Tritlo
Repo: crosspoint-reader/crosspoint-reader PR: 1003
File: src/activities/reader/EpubReaderActivity.cpp:657-674
Timestamp: 2026-02-19T17:46:26.871Z
Learning: In EpubReaderActivity.cpp, renderContents() behavior is intentional: if uncached images exist, Phase 1 should call displayBuffer(forceFullRefresh) to perform a HALF_REFRESH and clear ghosting; Phase 2 should call renderer.displayBuffer() without forceFullRefresh for a faster refresh. Grayscale anti-aliasing is applied separately after renderContents() via displayGrayBuffer(). This guidance applies specifically to this file and situation.
Applied to files:
src/activities/reader/EpubReaderActivity.cpp
📚 Learning: 2026-02-18T15:43:12.258Z
Learnt from: GavinHigham
Repo: crosspoint-reader/crosspoint-reader PR: 966
File: src/activities/ActivityWithSubactivity.h:20-21
Timestamp: 2026-02-18T15:43:12.258Z
Learning: In the crosspoint-reader activity architecture, when using ActivityWithSubactivity, the currentActivity global variable remains at the top-level activity (e.g., ReaderActivity) even when nested subactivities are active. The supportsOrientation() delegation chain starts from the top-level activity and stops when it reaches a subactivity that overrides the method (e.g., EpubReaderActivity returns true). Deeper nested subactivities (like EpubReaderChapterSelectionActivity or EpubReaderPercentSelectionActivity) do not need to override supportsOrientation() because they never become the currentActivity and the delegation chain doesn't reach them.
Applied to files:
src/activities/reader/EpubReaderActivity.cpp
🧬 Code graph analysis (7)
src/activities/settings/CalibreSettingsActivity.cpp (1)
src/activities/Activity.cpp (2)
finish(28-28)finish(28-28)
src/activities/settings/KOReaderSettingsActivity.cpp (1)
src/activities/Activity.cpp (2)
finish(28-28)finish(28-28)
src/activities/browser/OpdsBookBrowserActivity.cpp (2)
src/activities/ActivityManager.cpp (2)
requestUpdate(217-227)requestUpdate(217-217)src/activities/Activity.cpp (2)
requestUpdate(9-9)requestUpdate(9-9)
src/activities/ActivityManager.h (2)
src/activities/ActivityManager.cpp (3)
RenderLock(230-233)RenderLock(235-238)RenderLock(240-245)src/activities/RenderLock.h (1)
RenderLock(6-16)
src/activities/settings/OtaUpdateActivity.cpp (1)
src/activities/Activity.cpp (2)
finish(28-28)finish(28-28)
src/activities/network/CalibreConnectActivity.cpp (1)
src/activities/Activity.cpp (2)
finish(28-28)finish(28-28)
src/activities/ActivityManager.cpp (1)
src/activities/RenderLock.h (1)
RenderLock(6-16)
🔇 Additional comments (8)
src/activities/browser/OpdsBookBrowserActivity.cpp (1)
386-386: Good fix: error-state UI refresh is now explicit.Adding
requestUpdate()here ensures the newERRORstate/message is rendered immediately after Wi‑Fi selection fails.src/activities/settings/KOReaderSettingsActivity.cpp (1)
31-33: Nice lifecycle cleanup usingfinish().This keeps exit behavior consistent with the activity API while preserving behavior.
src/activities/settings/CalibreSettingsActivity.cpp (1)
29-31: Good consistency change tofinish()on Back.This aligns with the unified activity-exit pattern without changing behavior.
src/activities/ActivityManager.cpp (1)
235-235:[[maybe_unused]]annotation looks good here.Clearer intent for the compatibility constructor, no behavior impact.
src/activities/settings/OtaUpdateActivity.cpp (1)
16-17: Consistentfinish()migration across OTA exit paths.All updated exits preserve behavior and improve lifecycle API consistency.
Also applies to: 174-176, 182-184, 189-191
src/activities/network/CalibreConnectActivity.cpp (1)
65-67: Goodfinish()standardization in connect activity exits.Both updated paths keep behavior intact while following the shared lifecycle convention.
Also applies to: 164-166
src/activities/Activity.h (2)
16-17: LGTM - Friend declaration enables ActivityManager access.The friend declaration correctly allows
ActivityManagerto access protected members likeresultHandlerandresult.
23-24: Verify:protectedaccess vs. stated PR goal.The PR objective states that "only Activity and ActivityManager may access
resultHandlerandresult." However, these members areprotected, which technically grants access to all derived Activity classes.Verification shows no derived classes currently access these members directly—they correctly use the public
startActivityForResult()API instead. Thefriend class ActivityManager;declaration (line 16) explicitly grants ActivityManager special access. If the intent is to strictly enforce encapsulation and prevent future direct access from subclasses, these members should beprivate. Otherwise, the current design appears adequate since derived classes follow the public API contract.
osteotek
approved these changes
Feb 27, 2026
ngxson
approved these changes
Feb 27, 2026
This was referenced Mar 1, 2026
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
3 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.
Summary
What is the goal of this PR?
Small tweaks to #1016:
[[maybe_unused]]in RenderLock constructorAI Usage
While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it
helps set the right context for reviewers.
Did you use AI tools to help write this code? NO