docs: ActivityManager migration guide#1222
Conversation
📝 WalkthroughWalkthroughDocuments a refactor from per-activity FreeRTOS render tasks to a centralized ActivityManager with a single render task, global render mutex, RenderLock RAII, an ActivityStack-based lifecycle, updated navigation/subactivity APIs, and a migration checklist with examples and pitfalls. (49 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant ActivityManager
participant ActivityStack
participant RenderTask
participant Activity
Caller->>ActivityManager: pushActivity(newActivity)
ActivityManager->>ActivityStack: enqueue PendingPush(newActivity)
ActivityManager->>RenderTask: notify pending action
RenderTask->>ActivityManager: acquire globalRenderMutex (RenderLock)
ActivityManager->>ActivityStack: apply PendingPush -> set active Activity
ActivityManager->>Activity: call onEnter()
RenderTask->>Activity: call render() (while holding RenderLock)
Activity-->>RenderTask: render complete
ActivityManager->>Activity: return to caller (push complete)
RenderTask->>ActivityManager: release globalRenderMutex (RenderLock destruct)
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 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 |
|
You can view the rendered document here |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/activity-manager.md (1)
23-23: Add languages to fenced blocks to satisfy markdownlint (MD040).These blocks are plaintext diagrams/flows; tag them explicitly (for example,
text) to clear lint warnings.Suggested doc-only diff
-``` +```text ┌─────────────────────────────────────────────────────────┐ ... -``` +``` -``` +```text ┌──────────────────────────────────────────────────────────┐ ... -``` +``` -``` +```text ┌──────────────────────┐ ┌──────────────────────────┐ ... -``` +``` -``` +```text Main Task Render Task ... -``` +``` -``` +```text requestUpdate(false) requestUpdate(true) ... -``` +``` -``` +```text Calling Task Render Task ... -``` +``` -``` +```text activityManager.replaceActivity(make_unique<MyActivity>(...)) ... -``` +``` -``` +```text Parent calls: startActivityForResult(make_unique<Child>(...), handler) ... -``` +```Also applies to: 55-55, 251-251, 313-313, 329-329, 352-352, 377-377, 406-406
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/activity-manager.md` at line 23, The markdown fenced diagram blocks in activity-manager.md are missing language tags and trigger markdownlint MD040; update each triple-backtick fence that contains the ASCII diagrams (the several unnamed fenced blocks around the diagrams) to use a language tag such as "text" (i.e., change ``` to ```text) so the diagrams are explicitly marked and the lint warning is cleared; ensure every diagram block referenced (including the ones around the Main Task/Render Task, Calling Task/Render Task, activityManager.replaceActivity, and startActivityForResult examples) uses the same ```text tag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/activity-manager.md`:
- Around line 123-132: Add a brief clarifying note to the section describing the
old enterNewActivity()/exitActivity() pattern that explains modal
ConfirmationActivity flows were an exception and did not require a paired
exitActivity() call; mention that in legacy code (e.g.,
EpubReaderActivity::launch or other uses of ConfirmationActivity via
enterNewActivity()) cleanup was handled in the callback using
pendingSubactivityExit, so readers should not change already-correct
ConfirmationActivity usages.
---
Nitpick comments:
In `@docs/activity-manager.md`:
- Line 23: The markdown fenced diagram blocks in activity-manager.md are missing
language tags and trigger markdownlint MD040; update each triple-backtick fence
that contains the ASCII diagrams (the several unnamed fenced blocks around the
diagrams) to use a language tag such as "text" (i.e., change ``` to ```text) so
the diagrams are explicitly marked and the lint warning is cleared; ensure every
diagram block referenced (including the ones around the Main Task/Render Task,
Calling Task/Render Task, activityManager.replaceActivity, and
startActivityForResult examples) uses the same ```text tag.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/activity-manager.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: cppcheck
🧰 Additional context used
🧠 Learnings (2)
📚 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:
docs/activity-manager.md
📚 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:
docs/activity-manager.md
🪛 LanguageTool
docs/activity-manager.md
[style] ~213-~213: ‘Almost never’ might be wordy. Consider a shorter alternative.
Context: ... away ``` When to use immediate: Almost never. Deferred updates are batched — if `loo...
(EN_WORDINESS_PREMIUM_ALMOST_NEVER)
🪛 markdownlint-cli2 (0.21.0)
docs/activity-manager.md
[warning] 23-23: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 55-55: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 251-251: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 313-313: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 329-329: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 352-352: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 377-377: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 406-406: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (1)
docs/activity-manager.md (1)
1-472: Strong migration guide overall.Clear structure, practical before/after snippets, and good pitfall coverage. This should materially reduce migration errors.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/activity-manager.md (1)
269-274: Clarify that background-task writes also requireRenderLock.This section currently frames locking mostly as
loop()vsrender(). Please explicitly include background task mutations too, since the guide already allows those tasks to trigger updates.Suggested doc tweak
-A single FreeRTOS mutex (`renderingMutex`) protects shared state between `loop()` and `render()`. Since these run on different tasks, any state read by `render()` and written by `loop()` must be guarded. +A single FreeRTOS mutex (`renderingMutex`) protects shared state between `loop()`, background tasks, and `render()`. Any state read by `render()` and written by another task (`loop()` or background worker) must be guarded.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/activity-manager.md` around lines 269 - 274, Clarify that any background task which mutates shared UI state must also acquire the same RenderLock (the FreeRTOS mutex referenced as renderingMutex) before writing to state that render() reads; update the text around loop(), render(), and the render mutex to explicitly state that background tasks that call requestUpdate() or requestUpdateAndWait() must lock via RenderLock (or the renderingMutex) when performing mutations, and provide one short example sentence noting that failing to use RenderLock in background tasks can race with render().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/activity-manager.md`:
- Line 241: Update the sentence that currently reads "This removes
`std::function` overhead (~2-4KB per unique signature) and eliminates lifetime
risks from captured `this` pointers." to remove the hardcoded numeric claim and
the "per unique signature" phrasing; instead state a qualitative benefit such as
"reduces callback storage and indirection overhead and eliminates lifetime risks
from captured `this` pointers" (referencing `std::function` in the text), so the
doc no longer asserts platform-specific byte sizes or incorrect attribution to
signature uniqueness.
---
Nitpick comments:
In `@docs/activity-manager.md`:
- Around line 269-274: Clarify that any background task which mutates shared UI
state must also acquire the same RenderLock (the FreeRTOS mutex referenced as
renderingMutex) before writing to state that render() reads; update the text
around loop(), render(), and the render mutex to explicitly state that
background tasks that call requestUpdate() or requestUpdateAndWait() must lock
via RenderLock (or the renderingMutex) when performing mutations, and provide
one short example sentence noting that failing to use RenderLock in background
tasks can race with render().
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/activity-manager.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: cppcheck
🧰 Additional context used
🧠 Learnings (5)
📚 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:
docs/activity-manager.md
📚 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:
docs/activity-manager.md
📚 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:
docs/activity-manager.md
📚 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:
docs/activity-manager.md
📚 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:
docs/activity-manager.md
🪛 LanguageTool
docs/activity-manager.md
[style] ~213-~213: ‘Almost never’ might be wordy. Consider a shorter alternative.
Context: ... away ``` When to use immediate: Almost never. Deferred updates are batched — if `loo...
(EN_WORDINESS_PREMIUM_ALMOST_NEVER)
docs/activity-manager.md
Outdated
| RenderLock lock; | ||
| state = LOADING; | ||
| } | ||
| requestUpdate(); // render "Loading..." while we work |
There was a problem hiding this comment.
| requestUpdate(); // render "Loading..." while we work | |
| requestUpdate(true); // render "Loading..." while we work |
Here we will also need immediate rendering, otherwise the render will only be called after loop finishes (aggregate multiple calls to requestUpdate())
| ```cpp | ||
| // BEFORE | ||
| class MyActivity final : public ActivityWithSubactivity { | ||
| MyActivity(GfxRenderer& r, MappedInputManager& m, std::function<void()> goBack) |
There was a problem hiding this comment.
note that GfxRenderer and MappedInputManager are pretty much singletons at this point, I think we can remove them from activity argument in near future
docs/activity-manager.md
Outdated
|
|
||
| Both tasks run at priority 1. Since the ESP32-C3 is single-core, they alternate execution: the main task runs `loop()`, then at the end of the loop iteration, notifies the render task if an update was requested. The render task wakes, acquires the mutex, calls `render()`, releases the mutex, and blocks again. | ||
|
|
||
| Activities may also create their own background tasks (e.g., for network operations). These tasks can call `requestUpdate()` or `requestUpdateAndWait()` to trigger renders, but must never call `render()` directly. |
There was a problem hiding this comment.
Actually there are only 2 places in the code that creates a new task, but I would say it's very error-prone and indeed, they can be converted to simple loop() code.
I think we should consider arbitrary xTaskCreate as an anti-pattern at this point, until someone really need it (at such point, I think we should better have a Worker wrapper that is tied to the life cycle of the activity)
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/activity-manager.md (1)
121-146: Consider adding a concreteKeyboardEntryActivitymigration example.This section is good, but a short old→new snippet for settings-style keyboard flows would prevent mis-migrations in a very common pattern (parent
exitActivity()before launching keyboard, then child callback exit).Based on learnings, in settings activities the established legacy flow for
KeyboardEntryActivityrelied on parentexitActivity()beforeenterNewActivity(...), with callback-driven exit from the child.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/activity-manager.md` around lines 121 - 146, Add a concrete migration example for the KeyboardEntryActivity showing the old pattern (parent calls exitActivity() then enterNewActivity(new KeyboardEntryActivity(..., callback)) where the child triggers the callback which causes the parent to continue) and the new pattern using startActivityForResult(...) with a typed ActivityResult (e.g., KeyboardResult) and child using setResult(KeyboardResult{...}) followed by finish(); reference the symbols enterNewActivity, exitActivity, KeyboardEntryActivity, startActivityForResult, ActivityResult, setResult, and finish so readers can map the old callback-driven flow to the new typed result stack in settings-style keyboard flows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/activity-manager.md`:
- Around line 121-146: Add a concrete migration example for the
KeyboardEntryActivity showing the old pattern (parent calls exitActivity() then
enterNewActivity(new KeyboardEntryActivity(..., callback)) where the child
triggers the callback which causes the parent to continue) and the new pattern
using startActivityForResult(...) with a typed ActivityResult (e.g.,
KeyboardResult) and child using setResult(KeyboardResult{...}) followed by
finish(); reference the symbols enterNewActivity, exitActivity,
KeyboardEntryActivity, startActivityForResult, ActivityResult, setResult, and
finish so readers can map the old callback-driven flow to the new typed result
stack in settings-style keyboard flows.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/activity-manager.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: cppcheck
🧰 Additional context used
🧠 Learnings (5)
📚 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:
docs/activity-manager.md
📚 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:
docs/activity-manager.md
📚 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:
docs/activity-manager.md
📚 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:
docs/activity-manager.md
📚 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:
docs/activity-manager.md
🪛 LanguageTool
docs/activity-manager.md
[style] ~213-~213: ‘Almost never’ might be wordy. Consider a shorter alternative.
Context: ... away ``` When to use immediate: Almost never. Deferred updates are batched — if `loo...
(EN_WORDINESS_PREMIUM_ALMOST_NEVER)
🔇 Additional comments (1)
docs/activity-manager.md (1)
51-80: Strong migration guide — architecture and concurrency guidance are clear and practical.The old/new model comparisons,
requestUpdatesemantics, and deadlock warning aroundRenderLock+requestUpdateAndWait()are especially well documented and should reduce migration regressions.Also applies to: 199-216, 271-373
docs/activity-manager.md
Outdated
|
|
||
| Both tasks run at priority 1. Since the ESP32-C3 is single-core, they alternate execution: the main task runs `loop()`, then at the end of the loop iteration, notifies the render task if an update was requested. The render task wakes, acquires the mutex, calls `render()`, releases the mutex, and blocks again. | ||
|
|
||
| Avoid creating additional FreeRTOS tasks inside activities. Most work that seems to need a background task can be done directly in `loop()` using non-blocking patterns or `requestUpdateAndWait()`. There are currently only two activities that create their own tasks (for network operations), and these are considered technical debt — arbitrary `xTaskCreate` is error-prone because the task must be carefully tied to the activity's lifecycle and cleaned up in `onExit()`. If a background task is genuinely unavoidable, it should be wrapped in a lifecycle-aware `Worker` helper that ties creation and deletion to `onEnter()` / `onExit()`, rather than using raw `xTaskCreate` / `vTaskDelete`. Such a wrapper does not exist yet — if you find yourself needing one, that's the right time to build it. Background tasks must never call `render()` directly; use `requestUpdate()` or `requestUpdateAndWait()` instead. |
There was a problem hiding this comment.
I think just simple mention of this style should be good: about (1) do not use xTaskCreate and (2) open a discussion for Worker when someone need it.
The Worker does not exist yet, but it would be helpful if someone find a legitimate use case for it and make a proposal first.
The 2 places currently using xTaskCreate will be removed soon so I think there is no need to mention in the docs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/activity-manager.md (1)
464-464: Consider clarifying the comment for technical precision.The comment says "render 'Loading...' immediately, before we block", but
requestUpdate(true)only starts the render (notifies the render task) without waiting for completion (per lines 336-338).In practice on the single-core ESP32-C3 with equal-priority tasks, the render will likely complete before the blocking network call proceeds, making the current approach pragmatically correct. However, for strict guarantees that the screen reflects the new state before proceeding, lines 215-216 explicitly recommend
requestUpdateAndWait()for this use case.Optional: Clarify the comment
- requestUpdate(true); // render "Loading..." immediately, before we block + requestUpdate(true); // start rendering "Loading..." before we blockOr, if strict ordering is required, consider using
requestUpdateAndWait()to guarantee the loading message is visible before the network call begins.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/activity-manager.md` at line 464, Update the inline comment next to the call to requestUpdate(true) to state that requestUpdate(true) only schedules/starts a render (it notifies the render task but does not wait for completion), and note that if you need a strict guarantee the screen shows "Loading..." before blocking network work you should call requestUpdateAndWait() instead; keep the comment concise and mention both requestUpdate(true) and requestUpdateAndWait() by name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/activity-manager.md`:
- Line 464: Update the inline comment next to the call to requestUpdate(true) to
state that requestUpdate(true) only schedules/starts a render (it notifies the
render task but does not wait for completion), and note that if you need a
strict guarantee the screen shows "Loading..." before blocking network work you
should call requestUpdateAndWait() instead; keep the comment concise and mention
both requestUpdate(true) and requestUpdateAndWait() by name.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
docs/activity-manager.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cppcheck
- GitHub Check: build
🧰 Additional context used
🧠 Learnings (4)
📚 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:
docs/activity-manager.md
📚 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:
docs/activity-manager.md
📚 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:
docs/activity-manager.md
📚 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:
docs/activity-manager.md
🪛 LanguageTool
docs/activity-manager.md
[style] ~213-~213: ‘Almost never’ might be wordy. Consider a shorter alternative.
Context: ... away ``` When to use immediate: Almost never. Deferred updates are batched — if `loo...
(EN_WORDINESS_PREMIUM_ALMOST_NEVER)
🔇 Additional comments (1)
docs/activity-manager.md (1)
1-472: Excellent comprehensive migration guide!This is a thorough, well-structured document that clearly explains the architectural shift from per-activity render tasks to the centralized ActivityManager. The comparison tables, ASCII diagrams, migration checklist, and technical details about the FreeRTOS task model and render mutex are all valuable. The common pitfalls section with concrete examples is particularly helpful.
|
I get benefits of RAII wrapper for locks, but its implicit behavior always seemed less obvious to me. But I'm just old-fashioned I guess |
## Summary **What is the goal of this PR?** Added overview and migration guide for ActivityManager changes in crosspoint-reader#1016. Thanks for the suggestion, @drbourbon! --- ### AI 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? _**YES, fully written by Claude 4.6**_
## Summary Ref discussion: #1222 (comment) Important note that this is a bug-for-bug fix. In reality, this branch `WiFi.status() == WL_CONNECTED` is pretty much a dead code because the entry point of these 2 activities don't use wifi. It is better to refactor the management of network though, but it's better to be a dedicated PR. --- ### AI 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**
3 participants
Summary
What is the goal of this PR?
Added overview and migration guide for ActivityManager changes in #1016. Thanks for the suggestion, @drbourbon!
AI 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? YES, fully written by Claude 4.6