refactor(filter): encapsulate FilterableTable behind an item-oriented API - #692
refactor(filter): encapsulate FilterableTable behind an item-oriented API#692jvanbuel wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThe PR refactors ChangesFilterable table migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 review |
✅ Action performedReview finished.
|
![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
🤖 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 `@src/app/model/filterable_table/mod.rs`:
- Around line 55-58: Update FilterableTable’s apply_filter flow to capture the
currently selected item’s identity before rebuilding view.items, then reselect
that item’s new index afterward. When the selected item no longer exists, clamp
the selection to a valid index (or the empty-view state), preserving selection
across set_items, update_all, and sort_by.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1aa292e2-e0b0-4928-83d7-0e53759382de
📒 Files selected for processing (14)
src/app/model/config/mod.rssrc/app/model/dagruns/mod.rssrc/app/model/dagruns/render.rssrc/app/model/dags/render.rssrc/app/model/filter/matching.rssrc/app/model/filterable_table/mod.rssrc/app/model/filterable_table/render.rssrc/app/model/taskinstances/mod.rssrc/app/model/taskinstances/render.rssrc/app/state/breadcrumb.rssrc/app/state/mod.rssrc/app/state/sync.rssrc/app/worker/config.rssrc/ui.rs
| pub fn set_items(&mut self, items: Vec<T>) { | ||
| self.all = items; | ||
| self.apply_filter(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how often sort_dag_runs / sort_task_instances / set_items run relative to polling/user input
rg -n "sort_dag_runs|sort_task_instances|\.set_items\(" src/app/state/sync.rs src/app/worker -A3 -B3Repository: jvanbuel/flowrs
Length of output: 2893
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the filterable table implementation and the refresh callers.
sed -n '1,220p' src/app/model/filterable_table/mod.rs
printf '\n--- sync.rs ---\n'
sed -n '1,120p' src/app/state/sync.rs
printf '\n--- tasks.rs ---\n'
sed -n '1,80p' src/app/worker/tasks.rs
# Find any code that reselects/clamps after filtering.
printf '\n--- selection-related search ---\n'
rg -n "selected\(|select\(|state\.select|clamp|reselect|selected_position|current\(\)" src/app/model/filterable_table src/app/state src/app/worker -A2 -B2Repository: jvanbuel/flowrs
Length of output: 21638
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where the current/selected item is used for actions.
rg -n "selected_ids\(|current\(\)|current_mut\(|selected_position\(" src/app -A3 -B3
printf '\n--- dagruns selection actions ---\n'
sed -n '1,260p' src/app/model/dagruns/mod.rs
printf '\n--- taskinstances selection actions ---\n'
sed -n '1,280p' src/app/model/taskinstances/mod.rsRepository: jvanbuel/flowrs
Length of output: 33736
Re-anchor selection when the filtered view changes In src/app/model/filterable_table/mod.rs:55-58, apply_filter() only rebuilds view.items; it leaves view.state.selected() untouched. After set_items, update_all, or sort_by, the highlighted row can silently move to a different item, so current()/selected_ids() will drive m, c, Enter, and o against the wrong row. Track the selected item’s identity and reselect it after filtering, falling back to clamping if it no longer exists.
🤖 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 `@src/app/model/filterable_table/mod.rs` around lines 55 - 58, Update
FilterableTable’s apply_filter flow to capture the currently selected item’s
identity before rebuilding view.items, then reselect that item’s new index
afterward. When the selected item no longer exists, clamp the selection to a
valid index (or the empty-view state), preserving selection across set_items,
update_all, and sort_by.
1 participant
Summary
FilterableTable<T>exposedpub all: Vec<T>andpub filtered: StatefulTable<T>, so callers reached past the abstraction — andfilteredheld cloned copies of every matched item, re-cloned on every keystroke and every 2s refresh (on top ofsync_panelcloning the wholeVec<T>out ofenvironment_state).This encapsulates the table so the representation is a private detail, and eliminates the per-keystroke/per-refresh clone of the matched items.
Design
all(canonical items),view(filtered indices + selection state),filter,visual_anchorare all private. Callers go through an item-oriented API and never see the index representation:items(),current()/current_mut(),item_at(),len()/is_empty(),selected_ids(),sort_by()/update_all(),set_items()/clear(),filter()/filter_mut(),state_mut(),clear_visual_mode().filter_itemsnow returnsVec<usize>(indices intoall); the matched items are referenced, not copied.items()resolves indices to&T.sort_by/update_all, which mutate the canonicalalland re-derive the view — so the view is always inall's order, andmark_*writes to the source of truth.items()+state_mut(); the row builders now own their display strings (small, per-frame, and inherent to rendering) so theTableno longer borrows the table and the selection state can be updated in the same pass.Because the index representation is now hidden, the follow-up win (sharing
allasArc<[T]>to also drop theenvironment_state → panelclone) becomes a private one-line change touching zero call sites.Verification
cargo clippy --workspace --all-targets --all-features -- -D warnings— clean.cargo test --workspace --lib --bins— 223 pass (the existingFilterableTablenavigation / visual-mode /selected_idsand DAG-run sort/filter tests are the safety net; adapted to the public API).cargo fmt --all --check— clean.Context: this supersedes the leaky index-patch approach; the goal was a real abstraction rather than "we made
filteredan index list."🤖 Generated with Claude Code
Summary by CodeRabbit