Add Mermaid diagrams illustrating event workflow and supervisor internals - #1
Conversation
…nals Adds a component overview, the descriptor Debounce/Heartbeat state machine, a trigger()/tick() sequence diagram (debounced flap + heartbeat resend), and a startup/lifecycle flow diagram to README.md, placed alongside the existing prose they illustrate.
WalkthroughREADME.md adds architecture diagrams for event routing, descriptor debounce and heartbeat behavior, and supervisor startup lifecycle constraints. ChangesEvent system documentation
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 181-188: Update the sequence diagram to add an EventSupervisor
participant and route the external trigger through
EventSupervisor::trigger(EventId, EventValue) before it invokes
EventDescriptor::trigger(). Replace the direct Ext-to-Desc trigger interaction
while preserving the existing descriptor activation and debounce state notes.
- Around line 92-110: Update the README state-machine diagram so OneShot
transitions from Debounce to an idle/disarmed state rather than terminal [*],
allowing a later trigger() to begin a new cycle. Add the missing Debounce
transition for Interval mode with delay == 0, showing an immediate changed-value
send followed by entry into Heartbeat.
- Around line 253-255: Update the README lifecycle description for
registerDescriptor() and emitInitialSnapshot() to call assert() a debug-time
precondition rather than production enforcement, or add an always-on runtime
guard in the corresponding implementation paths. Ensure post-start calls cannot
mutate the descriptor collection while the supervisor iterates it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| Debounce --> Debounce : trigger()\n(delay > 0: re-arm timer) | ||
| Debounce --> Debounce : tick() before ArmedAt elapses\n(no-op) | ||
| Debounce --> Heartbeat : tick() at ArmedAt, mode=Interval\n(fireIfChangedLocked, then arm +interval) | ||
| Debounce --> [*] : tick() at ArmedAt, mode=OneShot\n(fireIfChangedLocked, then disarm) | ||
|
|
||
| Heartbeat --> Heartbeat : tick() at ArmedAt\n(unconditional resend, re-arm +interval) | ||
| Heartbeat --> Debounce : trigger()\n(delay > 0: re-arm timer, even from Heartbeat) | ||
| Heartbeat --> Heartbeat : trigger()\n(delay == 0: send if changed, stay in Heartbeat) | ||
|
|
||
| note right of Debounce | ||
| fireIfChangedLocked(): | ||
| pending_ != image_ -> send, Raised++ | ||
| pending_ == image_ -> Suppressed++ | ||
| end note | ||
| ``` | ||
|
|
||
| Two shapes fall out of this one machine: | ||
| - **OneShot**: `Debounce -> [*]` — fires at most once per trigger cycle, then | ||
| goes idle (no heartbeat loop). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Align the state machine with the actual descriptor lifecycle.
Debounce -> [*] incorrectly makes OneShot terminal; the implementation only disarms the timer, so a later trigger() can begin another cycle. The diagram also omits the Interval, delay == 0 transition from Debounce to immediate send/Heartbeat.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 92 - 110, Update the README state-machine diagram so
OneShot transitions from Debounce to an idle/disarmed state rather than terminal
[*], allowing a later trigger() to begin a new cycle. Add the missing Debounce
transition for Interval mode with delay == 0, showing an immediate changed-value
send followed by entry into Heartbeat.
| participant Ext as External Thread\n(netlink callback) | ||
| participant Desc as EventDescriptor | ||
| participant Worker as Worker Thread | ||
| participant Sender as IEventSender | ||
|
|
||
| Ext->>Desc: trigger(true) | ||
| activate Desc | ||
| Note over Desc: pending_=true, Phase=Debounce\narm delay timer |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Route external triggers through EventSupervisor.
The actual API is EventSupervisor::trigger(EventId, EventValue), which finds the descriptor before invoking EventDescriptor::trigger(). Showing Ext->>Desc bypasses that routing and contradicts the component diagram; add a supervisor participant to the sequence.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 181 - 188, Update the sequence diagram to add an
EventSupervisor participant and route the external trigger through
EventSupervisor::trigger(EventId, EventValue) before it invokes
EventDescriptor::trigger(). Replace the direct Ext-to-Desc trigger interaction
while preserving the existing descriptor activation and debounce state notes.
| `registerDescriptor()` and `emitInitialSnapshot()` are only valid before | ||
| `start()` (enforced by `assert`) — this is what lets the supervisor iterate | ||
| its descriptor collection without a lock once running. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Do not describe the lifecycle guard as production enforcement.
assert() is disabled in release builds. A post-start() registration can therefore mutate the descriptor vector while the worker iterates it, so this is a precondition/debug check rather than an enforced runtime guarantee. Clarify the wording or add a release-mode guard in the implementation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 253 - 255, Update the README lifecycle description
for registerDescriptor() and emitInitialSnapshot() to call assert() a debug-time
precondition rather than production enforcement, or add an always-on runtime
guard in the corresponding implementation paths. Ensure post-start calls cannot
mutate the descriptor collection while the supervisor iterates it.
There was a problem hiding this comment.
Pull request overview
This PR updates README.md to add Mermaid diagrams that explain the event system’s architecture and runtime behavior, helping readers understand how EventSupervisor, EventDescriptor, and IEventSender interact (including debounce + heartbeat semantics and the startup lifecycle).
Changes:
- Added a component/architecture Mermaid graph under Core Concepts.
- Added a Mermaid
stateDiagram-v2describing the descriptorDebounce/Heartbeatphase machine under Timing Behavior. - Added Mermaid sequence + lifecycle flow diagrams to illustrate thread-safety interactions and startup order.
Comments suppressed due to low confidence (2)
README.md:189
- To match the real call flow, the external thread should invoke
EventSupervisor::trigger(EventId, EventValue)and the supervisor should forward to the descriptor’strigger(value)(the descriptor is not typically reachable by external threads).
Ext->>Desc: trigger(true)
activate Desc
Note over Desc: pending_=true, Phase=Debounce\narm delay timer
deactivate Desc
README.md:194
- Same as above: show
EventSupervisor::trigger(id, value)dispatching to the descriptor, rather than the external thread calling the descriptor directly.
Ext->>Desc: trigger(false) (cable flap, within delay)
activate Desc
Note over Desc: pending_=false, Phase=Debounce\nre-arm delay timer
deactivate Desc
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| Netlink -- "trigger(EventId, EventValue)" --> Supervisor | ||
| NtpClient -- "trigger(EventId, EventValue)" --> Supervisor | ||
| Supervisor -- "owns 1..32" --> Descriptor |
| participant Ext as External Thread\n(netlink callback) | ||
| participant Desc as EventDescriptor | ||
| participant Worker as Worker Thread | ||
| participant Sender as IEventSender |
Summary
EventSupervisor,EventDescriptor,IEventSender, and the remote controller relate.stateDiagram-v2for the descriptor's internalDebounce/Heartbeatphase machine under "Timing Behavior," matching the actual transitions inEventDescriptor::trigger()/tick().registerDescriptor → emitInitialSnapshot → start → running → stop) before the Example section.No code changes — documentation only,
README.md.Test plan
EventDescriptor.hppandEventSupervisor.hppin full to confirm diagram transitions/labels match actual code behavior.@mermaid-js/mermaid-cli(mmdc) to confirm valid syntax (fixed one escaped-quote issue found this way).README.mdto confirm diagrams are placed logically next to the prose they illustrate and don't contradict it.Generated by Claude Code
Summary by CodeRabbit