Store watches and bindings as element-table components - #53
Conversation
Move per-element reactivity out of standalone stores and into the element table, keyed by ElementId, and reorganize it under one module. Storage: - Add TypeTable::insert_by_column and an "arbitrary components" API on ElementTable (insert/remove/get/get_mut/components) backing the at-most-one-per-element data. - Watch and Binding are now per-element components: their ElementId is the column key, so they drop the stored element_id field, the SparseMap stores, the reverse-index maps, and the explicit remove_for_element cleanup (handled by remove_row). - This requires W: 'static on the build-time and update APIs. Reorganize + rename: - Group both flavors under a `reactive` module (reactive::watch, reactive::binding) sharing one ChangedFn. - Rename the subtree-rebuild concept from `reactive` to `watch`: Watch/WatchElement, ctx.watch(), Fynix::update_watches. - Encapsulate each flush step on its type: Watch::build does the whole rebuild and Binding::build does the field write + mark_dirty, leaving update_watches a thin snapshot-and-loop.
The per-frame flush already reconciles more than one kind of change-driven update and will grow to cover more, so name it for what it does rather than one kind: sync the element tree to the world.
|
Warning Review limit reached
More reviews will be available in 44 minutes and 43 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRemoves the old top-level ChangesWatch/Binding ECS Refactor
Sequence Diagram(s)sequenceDiagram
participant App
participant World
participant Fynix
participant Elements
participant WatchComp
participant BindComp
App->>World: update(dt)
App->>Fynix: sync(world)
Fynix->>Elements: iterate watch components
Fynix->>WatchComp: check changed
WatchComp-->>Fynix: changed ids
Fynix->>WatchComp: rebuild subtree for each id
WatchComp->>Elements: replace child and mark dirty
Fynix->>Elements: iterate binding components
Fynix->>BindComp: check changed
BindComp-->>Fynix: changed ids
Fynix->>BindComp: build field update for each id
BindComp->>Elements: write field and mark dirty
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
examples/vello_winit_examples/src/lib.rs (1)
32-34: 💤 Low valueConsider mentioning bindings alongside watches for completeness.
The doc comment only mentions watches, but
Fynix::syncalso flushes bindings. For consistency with the inline comment at lines 110-111 (which correctly mentions both), consider updating to "Advances world state before watches and bindings are updated."🤖 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 `@examples/vello_winit_examples/src/lib.rs` around lines 32 - 34, The doc comment for the update method currently only mentions "watches are updated" but the implementation also flushes bindings as evidenced by the inline comment at lines 110-111. Update the doc comment to state "Advances world state before watches and bindings are updated" to accurately reflect what the update method does and maintain consistency with the inline documentation elsewhere in the code.crates/fynix/src/reactive/binding.rs (1)
80-89: ⚡ Quick winReduce public unsafe surface for erased-pointer internals.
ApplyFnandGetFnPtrlook like internalBindingplumbing. Exposing them publicly (plustyped_unchecked) broadens downstream UB footguns without clear API value.♻️ Proposed visibility tightening.
-pub type ApplyFn<W> = fn( +pub(crate) type ApplyFn<W> = fn( world: &W, elements: &mut Elements, id: &ElementId, get_fn: GetFnPtr, get_mut: MutFnPtr, ); @@ -pub struct GetFnPtr(*const ()); +pub(crate) struct GetFnPtr(*const ()); @@ - pub const unsafe fn typed_unchecked<S, T>(&self) -> GetFn<S, T> { + pub(crate) const unsafe fn typed_unchecked<S, T>(&self) -> GetFn<S, T> { unsafe { core::mem::transmute::<*const (), GetFn<S, T>>(self.0) } }Also applies to: 113-137
🤖 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 `@crates/fynix/src/reactive/binding.rs` around lines 80 - 89, The ApplyFn type alias and other internal erased-pointer types used in Binding's implementation are currently publicly exposed, which unnecessarily broadens the unsafe surface area without providing clear API value. Remove the pub visibility modifier from ApplyFn to make it private, and apply the same visibility tightening to GetFnPtr and the typed_unchecked function mentioned in the range 113-137. These should be internal implementation details only, not part of the public API.
🤖 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 `@crates/fynix/src/element/table.rs`:
- Around line 107-146: The public component API methods insert_component,
remove_component, get_component, get_component_mut, and components allow callers
to access and modify reserved metadata types like ElementNode, Scene, and
StyleId through the same TypeTable namespace, which breaks ElementTable
invariants. Add a trait bound constraint (such as a marker trait that reserved
types do not implement) to the generic type parameter T in all these methods to
prevent reserved types from being used while allowing user-defined component
types. This ensures that only safe, non-reserved types can be managed through
the public component API.
In `@crates/fynix/src/reactive/watch.rs`:
- Around line 115-125: The code stores a child_id in elem.child even when the
node lookup via node_mut(&child_id) fails, leaving a dangling reference. Move
the elem.child = child assignment inside the first if block that checks both
child_id existence and successful node retrieval via
fynix.elements.table.node_mut(&child_id), so that WatchElement.child only stores
valid child references that exist in the node table.
---
Nitpick comments:
In `@crates/fynix/src/reactive/binding.rs`:
- Around line 80-89: The ApplyFn type alias and other internal erased-pointer
types used in Binding's implementation are currently publicly exposed, which
unnecessarily broadens the unsafe surface area without providing clear API
value. Remove the pub visibility modifier from ApplyFn to make it private, and
apply the same visibility tightening to GetFnPtr and the typed_unchecked
function mentioned in the range 113-137. These should be internal implementation
details only, not part of the public API.
In `@examples/vello_winit_examples/src/lib.rs`:
- Around line 32-34: The doc comment for the update method currently only
mentions "watches are updated" but the implementation also flushes bindings as
evidenced by the inline comment at lines 110-111. Update the doc comment to
state "Advances world state before watches and bindings are updated" to
accurately reflect what the update method does and maintain consistency with the
inline documentation elsewhere in the code.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0322b4dd-32c3-4f08-ad16-8032b23b66a1
📒 Files selected for processing (10)
crates/fynix/src/binding.rscrates/fynix/src/ctx.rscrates/fynix/src/element/storage.rscrates/fynix/src/element/table.rscrates/fynix/src/lib.rscrates/fynix/src/reactive.rscrates/fynix/src/reactive/binding.rscrates/fynix/src/reactive/watch.rscrates/typarena/src/type_table.rsexamples/vello_winit_examples/src/lib.rs
💤 Files with no reviewable changes (1)
- crates/fynix/src/binding.rs
| if let Some(child_id) = child | ||
| && let Some(node) = | ||
| fynix.elements.table.node_mut(&child_id) | ||
| { | ||
| node.parent_id = Some(id); | ||
| } | ||
| if let Some(elem) = | ||
| fynix.elements.get_typed_mut::<WatchElement>(&id) | ||
| { | ||
| elem.child = child; | ||
| } |
There was a problem hiding this comment.
Keep WatchElement.child consistent with node storage.
If build_fn returns Some(child_id) but node_mut(&child_id) is None, the code still stores Some(child_id) in elem.child. That leaves a dangling child reference and can break later layout on that holder.
🐛 Suggested guard to drop invalid child ids.
- if let Some(child_id) = child
- && let Some(node) =
- fynix.elements.table.node_mut(&child_id)
- {
- node.parent_id = Some(id);
- }
+ let child = if let Some(child_id) = child {
+ if let Some(node) = fynix.elements.table.node_mut(&child_id)
+ {
+ node.parent_id = Some(id);
+ Some(child_id)
+ } else {
+ None
+ }
+ } else {
+ None
+ };
if let Some(elem) =
fynix.elements.get_typed_mut::<WatchElement>(&id)
{
elem.child = child;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Some(child_id) = child | |
| && let Some(node) = | |
| fynix.elements.table.node_mut(&child_id) | |
| { | |
| node.parent_id = Some(id); | |
| } | |
| if let Some(elem) = | |
| fynix.elements.get_typed_mut::<WatchElement>(&id) | |
| { | |
| elem.child = child; | |
| } | |
| let child = if let Some(child_id) = child { | |
| if let Some(node) = fynix.elements.table.node_mut(&child_id) | |
| { | |
| node.parent_id = Some(id); | |
| Some(child_id) | |
| } else { | |
| None | |
| } | |
| } else { | |
| None | |
| }; | |
| if let Some(elem) = | |
| fynix.elements.get_typed_mut::<WatchElement>(&id) | |
| { | |
| elem.child = child; | |
| } |
🤖 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 `@crates/fynix/src/reactive/watch.rs` around lines 115 - 125, The code stores a
child_id in elem.child even when the node lookup via node_mut(&child_id) fails,
leaving a dangling reference. Move the elem.child = child assignment inside the
first if block that checks both child_id existence and successful node retrieval
via fynix.elements.table.node_mut(&child_id), so that WatchElement.child only
stores valid child references that exist in the node table.
There was a problem hiding this comment.
Leaving this unresolved, a good concern, but not critical. We might need more debug_assert!.
There was a problem hiding this comment.
@nixonyh, that's a reasonable call. A debug_assert! would be a good fit here - it documents the invariant and catches violations in debug builds without changing the release-build control flow. Something like:
if let Some(child_id) = child
&& let Some(node) = fynix.elements.table.node_mut(&child_id)
{
node.parent_id = Some(id);
} else if child.is_some() {
debug_assert!(false, "build_fn returned a child_id with no corresponding node");
}Or more concisely at the else branch:
debug_assert!(
child.is_none() || fynix.elements.table.node_mut(child.as_ref().unwrap()).is_some(),
"build_fn returned a child_id with no corresponding node in the element table"
);Would you like me to open a follow-up GitHub issue to track adding the debug_assert! guard here?
🧠 Learnings used
Learnt from: nixonyh
Repo: voxell-tech/fynix PR: 32
File: crates/fynix/src/lib.rs:33-39
Timestamp: 2026-05-21T09:23:49.477Z
Learning: In this Rust codebase, `use` import granularity should be at the module level. When importing multiple items from the same module path, group them into a single `use` statement using curly braces (e.g., `use crate::element::{Element, ElementBuild, ...};`). Do not recommend splitting grouped imports into separate lines (including one `pub use` per type) when they originate from the same module.
Learnt from: nixonyh
Repo: voxell-tech/fynix PR: 33
File: examples/vello_winit_examples/src/lib.rs:6-6
Timestamp: 2026-05-30T14:15:01.696Z
Learning: When reviewing Rust code in this repo, do not flag grouped `use` statements as style violations if they match the formatting configured in `rustfmt.toml` (imports_granularity = "Module" and group_imports = "StdExternalCrate"). Grouped imports such as `use fynix::{Fynix, rectree};` and `use vello::{AaConfig, RenderParams, Renderer, RendererOptions, Scene, wgpu};` are intentional and will be auto-enforced by the nightly `rustfmt` settings, so they should be treated as compliant formatting.
1 participant
Move per-element reactivity out of standalone stores and into the element table, keyed by ElementId, and reorganize it under one module.
Storage:
TypeTable::insert_by_columnand an "arbitrary components" API onElementTable(insert/remove/get/get_mut/components) backing the at-most-one-per-element data.WatchandBindingare now per-element components inElementTable.Reorganize + rename:
reactivemodule (reactive::watch, reactive::binding) sharing oneChangedFn.reactivetowatch:Watch/WatchElement,ctx.watch().