warp-core: tick_patch hygiene #157
Conversation
|
Warning Rate limit exceeded@flyingrobots has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 14 minutes and 40 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
WalkthroughThis PR performs hygiene improvements to Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Reasoning: Type signature changes across multiple public entities demand careful verification of consistency across all call sites and trait implementations; deduplication logic in Possibly related issues
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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: 4
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
crates/warp-core/src/tick_patch.rsdocs/decision-log.mddocs/execution-plan.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Every public API across crates (warp-core, warp-ffi, warp-wasm, etc.) must carry rustdoc comments that explain intent, invariants, and usage. Treat missing docs as a failing test.
Runcargo clippy --all-targets -- -D missing_docsbefore every PR; CI will expect a zero-warning, fully documented surface.
Every source file must start with exactly:// SPDX-License-Identifier: Apache-2.0and// © James Ross Ω FLYING•ROBOTS <https://github.com/flyingrobots>
Files:
crates/warp-core/src/tick_patch.rs
🧠 Learnings (2)
📚 Learning: 2025-12-28T23:14:28.083Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.083Z
Learning: Start each session by updating *Today's Intent* in `docs/execution-plan.md` and capture milestones, blockers, and decisions in the Decision Log.
Applied to files:
docs/execution-plan.md
📚 Learning: 2025-12-28T23:14:28.083Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-28T23:14:28.083Z
Learning: When a PR touches non-doc code, update `docs/execution-plan.md` and `docs/decision-log.md` in the same PR.
Applied to files:
docs/execution-plan.md
🪛 LanguageTool
docs/execution-plan.md
[style] ~40-~40: To elevate your writing, try using a synonym here.
Context: ...ays deterministic, well-documented, and hard to misuse. - Scope: - diff_store: a...
(HARD_TO)
docs/decision-log.md
[style] ~13-~13: To elevate your writing, try using a synonym here.
Context: ...ode should be clear, deterministic, and hard to misuse. Removing boilerplate and amb...
(HARD_TO)
🔇 Additional comments (13)
docs/execution-plan.md (1)
38-48: Decision-log.md has been properly updated; no action required.The 2025-12-29 entry in
docs/decision-log.mddocuments the tick_patch hygiene work with full context, rationale, and consequences. Bothdocs/execution-plan.mdanddocs/decision-log.mdhave been updated as required by the project learnings.Likely an incorrect or invalid review comment.
crates/warp-core/src/tick_patch.rs (12)
16-16: LGTM: thiserror and ContentHash alias introduced.The addition of
thiserrorfor ergonomic error handling and theContentHashalias to disambiguate fromderive(Hash)onSlotIdare both solid hygiene improvements that align with the stated objectives.Also applies to: 20-20
116-117: LGTM: WarpOpKey fields updated to ContentHash.Consistent application of the
ContentHashalias to internal ordering key fields.
162-162: LGTM: Struct fields consistently use ContentHash.Private fields correctly updated to use the new alias; public accessors will propagate this type change.
Also applies to: 167-167
181-181: LGTM: Parameter type updated to ContentHash.Public API signature correctly reflects the ContentHash alias.
230-230: LGTM: Return type updated to ContentHash.Public getter correctly returns the aliased type.
260-260: LGTM: Return type updated to ContentHash.Public getter correctly returns the aliased type.
295-295: LGTM: TickPatchError correctly uses thiserror.The
thiserrorderive eliminates boilerplate while providing clear error messages. Debug formatting for node/edge IDs is appropriate.Also applies to: 298-298, 301-301
307-307: LGTM: Function signature updated to ContentHash.Private helper function consistently uses the alias.
Also applies to: 312-312
346-351: LGTM: Excellent clarification on tag bytes vs. replay ordering.The documentation clearly disambiguates digest encoding tags from
WarpOp::sort_keyordering, preventing future confusion. Well-written maintainer guidance.
399-456: LGTM: Exceptional documentation expansion.This rustdoc is a masterclass in API documentation:
- Intent: Clearly explains the "diff" constructor role
- Invariants: Explicitly documents required GraphStore guarantees
- Semantics: Precise definitions for each op variant
- Edge cases: Handles identical stores, migration scenarios
- Performance: Honest about O(nodes + edges) + O(edges log edges) complexity and allocation costs
- Rationale: Explains why diff vs. direct recording
Fully satisfies the PR objective to "expand rustdoc covering intent, invariants, semantics, edge cases, and performance."
462-465: LGTM: Idiomatic let-else pattern eliminates redundant lookup.The refactoring from
match after.nodes.get(id)to let-else is more idiomatic (Rust 1.65+) and achieves the stated objective of removing double lookups—previously this likely usedcontains_key+get, now it's a singlegetwith immediate destructuring.
520-520: LGTM: Return type updated to ContentHash.Private helper function consistently uses the alias for the BTreeMap key type.
Closes #156
diff_store: avoid double lookups and expand rustdoc (intent, invariants, semantics, edge cases, perf).TickPatchError: switch tothiserrorderive.encode_ops: clarify digest tag bytes vs replay ordering.WarpTickPatchV1::new: dedupe duplicate ops by canonical sort key (last-wins) to prevent replay errors.crate::ident::HashtoContentHashto avoid confusion withderive(Hash)onSlotId.Validation:
cargo test --workspacecargo clippy --workspace --all-targets -- -D warnings -D missing_docs