Reject overlapping cells when building plot grids#1041
Draft
SimonHeybrock wants to merge 1 commit into
Draft
Conversation
Grid cells must tile the grid without overlap; two cells claiming the same slot is an invalid layout. Only the click-to-place editor enforced this, via its own occupancy tracking. Every other path into the orchestrator (file upload, persisted-config load, templates, edit-save) went through add_cell with no validation, so a hand-edited or uploaded YAML could inject overlapping cells. Such a grid renders in the live view (cells overwrite last-wins) but its edit preview crashes: building the GridSpec preview feeds an overlapping region that contains a not-yet-placed slot into Panel, which dereferences the empty slot and raises. The exception aborts edit mode before the Save/Copy buttons are shown, leaving the grid uneditable. Enforce the no-overlap invariant in the model: add_cell raises on overlap, and the collection-level entry points (upload, config load) validate up front via reject_overlapping_cells. Uploads are rejected with a user-facing error; faulty persisted grids are skipped on load. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
A downloaded/hand-edited grid config (
esslivedata_loki_*.yaml) could not be edited: clicking the pencil showed the title and row/column counts, but the preview stayed blank and only "Add Grid" appeared — no Save/Copy. The grid had two cells claiming the same slot. Building the edit preview feeds each cell into a PanelGridSpec; an overlapping region that still contains a not-yet-placed slot makes Panel dereference the empty slot and raise, and the exception aborts edit mode before the Save/Copy buttons are shown.Root cause
Grid cells must tile without overlap, but only the click-to-place editor enforced that (via its own occupancy tracking). Every other path into the orchestrator — file upload, persisted-config load, templates, edit-save — went through
add_cellwith no validation, so an overlapping layout could enter freely. It renders in the live view (cells overwrite last-wins) but is uneditable.Change
Enforce the no-overlap invariant in the model rather than papering over it in the preview:
add_cellraises on overlap, and the collection-level entry points validate up front. Uploads with overlapping cells are rejected with a user-facing error; faulty persisted grids are skipped on load (logged) so one bad grid does not block the rest.Follow-up
While investigating I mapped the orchestrator's threading model. The overlap fix is race-free (all UI callbacks are serialized on a single IOLoop), but I found two pre-existing, unrelated concurrency hazards in
PlotOrchestratorshared state — tracked in #1040, out of scope here.Test plan
add_cellrejects overlap and allows adjacent cells;reject_overlapping_cellsaccepts disjoint / rejects overlap; overlapping persisted config is dropped on load; overlapping upload is rejected.esslivedata_loki_deterctors.yamlin Manage Plots and confirm it is rejected with an error (no grid created, no crash).