doc: adr broadcast offline - #662
Conversation
📝 WalkthroughWalkthroughAdds a new Architecture Decision Record at ChangesOffline Broadcast ADR
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~5 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🧹 Nitpick comments (1)
doc/library-development/ADRs/broadcast-offline.md (1)
41-55: ⚡ Quick winNormalize the outline and fenced example.
Use h3s under the h2 sections here, and give the fenced example a language like
textor convert it to a list. That will make the ADR easier to scan and keep markdownlint happy; the same heading jump repeats again in Impact Radius.🤖 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 `@doc/library-development/ADRs/broadcast-offline.md` around lines 41 - 55, The ADR document has inconsistent heading hierarchy and a fenced code block without a language identifier. Under the h2 section "Refactor the Broadcast use case into several concerns," add h3 headings to organize the bullet points (such as "Cache with state information," "Cache of deleted events," "Broadcast monitor," and "Broadcast function"). For the example showing the cache structure with PriorityBroadcast and event details, either add a language identifier like `text` to the fenced code block or convert it to a properly formatted markdown list. Apply the same heading hierarchy improvements to the "Impact Radius" section mentioned in the comment to ensure consistent structure throughout the document and satisfy markdownlint requirements.Source: Linters/SAST tools
🤖 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 `@doc/library-development/ADRs/broadcast-offline.md`:
- Around line 5-17: The ADR metadata in the status section contains conflicting
information: the status field shows "In progress" while the "Final decision made
by" field indicates the decision has been finalized, and the "Updated on" date
appears outdated. To resolve this inconsistency, either change the status from
"In progress" to "Accepted" and update the date to the current date (15-06-2025
or later as appropriate), or remove the "Final decision made by" field entirely
and keep the status as "In progress" to maintain the document as a draft. Choose
the approach based on whether this ADR's decision has actually been finalized or
is still under discussion.
- Around line 27-35: The Requirements section in the broadcast-offline ADR uses
incomplete placeholder language (like `todo`, `%`, `different modes`) instead of
concrete, implementable policy rules. Replace these vague entries with specific,
actionable requirements based on the actual behavior in relay_manager.dart
(particularly the reject handling logic in lines L469-L640). Specifically, add
`auth-required` to the list of retryable reject classes (since the relay_manager
already treats it as retryable rather than terminal), define explicit thresholds
and eviction policy rules instead of vague guidance, and enumerate the concrete
reject classes (such as retry, dismiss, paymentRequired, POW, blocked,
auth-required) with their respective handling behaviors to match the current
broadcast flow implementation.
---
Nitpick comments:
In `@doc/library-development/ADRs/broadcast-offline.md`:
- Around line 41-55: The ADR document has inconsistent heading hierarchy and a
fenced code block without a language identifier. Under the h2 section "Refactor
the Broadcast use case into several concerns," add h3 headings to organize the
bullet points (such as "Cache with state information," "Cache of deleted
events," "Broadcast monitor," and "Broadcast function"). For the example showing
the cache structure with PriorityBroadcast and event details, either add a
language identifier like `text` to the fenced code block or convert it to a
properly formatted markdown list. Apply the same heading hierarchy improvements
to the "Impact Radius" section mentioned in the comment to ensure consistent
structure throughout the document and satisfy markdownlint requirements.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d117ce5-70e6-4719-8197-ac336a928566
📒 Files selected for processing (1)
doc/library-development/ADRs/broadcast-offline.md
| ## status | ||
|
|
||
| In progress | ||
|
|
||
| Updated on 15-06-2025 | ||
|
|
||
| ## contributors | ||
|
|
||
| - Main contributor(s): fmar | ||
|
|
||
| - Reviewer(s): fmar, leo, nogringo | ||
|
|
||
| - Final decision made by: fmar, leo, nogringo |
There was a problem hiding this comment.
Make the ADR metadata consistent.
In progress conflicts with Final decision made by, and the Updated on 15-06-2025 stamp looks stale for this PR. Please either keep this as a draft and remove the final-decision field, or mark it accepted with the correct date.
🤖 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 `@doc/library-development/ADRs/broadcast-offline.md` around lines 5 - 17, The
ADR metadata in the status section contains conflicting information: the status
field shows "In progress" while the "Final decision made by" field indicates the
decision has been finalized, and the "Updated on" date appears outdated. To
resolve this inconsistency, either change the status from "In progress" to
"Accepted" and update the date to the current date (15-06-2025 or later as
appropriate), or remove the "Final decision made by" field entirely and keep the
status as "In progress" to maintain the document as a draft. Choose the approach
based on whether this ADR's decision has actually been finalized or is still
under discussion.
| #### Requirements: | ||
| - relay specific | ||
| - gossip | ||
| - Consider done % | ||
| - different modes, | ||
| specific: relay broadcast must succeed | ||
| - eviction policy (keep in cache, delete old ones) | ||
| - Streamlined reject handling (retry, dismiss, paymentRequired, POW, blocked, etc.) | ||
|
|
There was a problem hiding this comment.
Tighten the acceptance criteria and preserve auth-required retry behavior.
This section still reads like notes (todo, %, different modes) instead of implementable policy rules, and it omits auth-required, which packages/ndk/lib/domain_layer/usecases/relay_manager.dart#L469-L640 already treats as retryable rather than terminal. Please spell out the concrete thresholds and reject classes so the refactor matches the current broadcast flow.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 27-27: Heading levels should only increment by one level at a time
Expected: h3; Actual: h4
(MD001, heading-increment)
🤖 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 `@doc/library-development/ADRs/broadcast-offline.md` around lines 27 - 35, The
Requirements section in the broadcast-offline ADR uses incomplete placeholder
language (like `todo`, `%`, `different modes`) instead of concrete,
implementable policy rules. Replace these vague entries with specific,
actionable requirements based on the actual behavior in relay_manager.dart
(particularly the reject handling logic in lines L469-L640). Specifically, add
`auth-required` to the list of retryable reject classes (since the relay_manager
already treats it as retryable rather than terminal), define explicit thresholds
and eviction policy rules instead of vague guidance, and enumerate the concrete
reject classes (such as retry, dismiss, paymentRequired, POW, blocked,
auth-required) with their respective handling behaviors to match the current
broadcast flow implementation.
|
This will pretty quickly be out of date, and I don't want to still need to update such docs along the way, so I would propose to not keep such doc that will be anyhow out-of-date once everything is in place, unless you want to come back to this and update it to the final implementation details @1-leo |
2 participants
ADR for offline broadcast refactor
Summary by CodeRabbit