Repository navigation
Trace arrows stay on the traced cell when the selection moves - #2481
h-a-n-a-s-h-i merged 1 commit into
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Capture and store trace context crates/ui-egui/src/grid.rs, crates/ui-egui/src/ribbon.rs |
Trace records the active document, revision, sheet, cell, and command result. Successful ribbon trace commands store this state in GridState.trace. |
Scope and anchor trace arrows crates/ui-egui/src/grid.rs |
Trace arrows use the recorded cell and appear only when the active document, revision, and sheet match. Tests cover selection changes, workbook and sheet scope, and revision changes. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Possibly related PRs
- storytold/gridcraft#2325: Clears trace state after document-switch commands; this change scopes arrows to the active document.
Suggested reviewers: marekspodbiedry, vangel76, echelon
Merge Risk: 
b5bf1
-
The supplied evidence does not establish a merge-blocking issue. The trace-arrow change appears ready for normal merge checks.
Security architecture risk: b5bf1
-
The change confines trace arrows to the workbook, sheet, and revision where tracing occurred. It does not add a new external entrypoint or grant additional access. No material security risk was identified in the changed behavior.
Retained concerns
No architecture-level concerns identified.Security review details
-
Security Blast Radius
- inferred — The changed path affects local formula-trace presentation within the existing application session. Its production producer remains the ribbon handler, and rendering remains crate-local; the new public Rust declarations do not establish additional attacker-controlled reachability or authority.
Trust Boundaries and Controls
- observed — The renderer requires matching workbook identity, revision, and sheet before consuming retained trace items. These are display-consistency controls that prevent arrows from being reused in another workbook context; they are not a new authorization mechanism.
-
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: trace arrows remain anchored to the traced cell when the selection moves. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
-
Finishing Touches
-
🧪 Generate unit tests (beta)
-
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
FredApps
left a comment
FredApps
left a comment
There was a problem hiding this comment.
Read the full non-test diff (ui-egui grid.rs). Trace arrows are now stored with the document uid, workbook revision, sheet and traced cell, and drawn from that cell on its own sheet, so moving the selection no longer drags them; they disappear when the revision changes or another document or sheet is shown, which matches the comment about Excel behaviour. The overlay reads the stored cell instead of the active one and the test helper builds the trace the way the ribbon buttons do; tests cover precedents (three arrows, all heads at F7), dependents (tails at B2) and the other-sheet case. Gate and SonarCloud are green at b5bf14d. No defects found.
|
Merged into main in 4eb2fb5 — thank you @marcoarnulfo for the contribution! |
Part of #2475.
The arrows were drawn from
sel.active, because the Trace Precedents / Trace Dependents buttons stored only the command's result ingrid.trace, never the cell the trace ran from.grid.tracenow holds a smallTracewith that cell, its sheet, the workbook (uid) and itsrevisionnext to the result.paint_overlaysdraws from the traced cell, and only while that workbook and sheet are showing and nothing has changed the workbook since. So you can trace F7 and walk around the sheet with the arrows staying on F7, consistent with the Microsoft docs ("To remove all tracer arrows on the worksheet… click Remove Arrows"). Remove Arrows is unchanged.When they go away: the docs say "All tracer arrows disappear if you change the formula to which the arrows point, insert or delete columns or rows, or delete or move cells." Tying the arrows to the workbook's revision covers that, and is a bit stricter: a new value typed into a precedent also clears them. That was the simplest rule that never leaves an arrow pointing at the wrong cell after rows move. Happy to narrow it if you'd rather match Excel exactly.
What's left of the issue: tracing a second cell still replaces the first set of arrows instead of adding to it, and clicking Trace Precedents again doesn't add the next level yet. I kept that out to keep this small. Happy to follow up with it, or fold it in here if you prefer.
Validation:
grid::trace_arrow_tests(5 new tests, which paint the overlay and read back the arrow segments). Before this change the precedent arrows end at H7 instead of F7, the dependent arrows start at C9 instead of B2, and switching from the traced sheet to the precedent's sheet draws an arrow there. All five pass here. The other two cover inserting a row after tracing (no arrows) and a second open workbook with the same cells and revision (no arrows there).sel.activeback fails the first two tests, and dropping the sheet, revision or workbook check fails its own test.cargo test --workspace --lib -- grid::and-- ribbon,cargo clippy --workspace --all-targets -- -D warnings,cargo fmt --all -- --checkSummary by CodeRabbit