Add plot grid #519
Open
Add plot grid #519
Conversation
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
Implements a new PlotGrid widget based on Panel's GridSpec that allows users to create customizable grid layouts for displaying multiple HoloViews plots. Key features: - Configurable grid dimensions (nrows × ncols) - Two-click region selection (click first corner, then second corner) - Rectangular region selection with automatic normalization - Plot insertion via callback mechanism (callback has no knowledge of grid) - Plot removal with always-visible close button - Overlap detection and prevention - Visual feedback for selection in progress - Error notifications for invalid selections Implementation details: - Based on Panel's GridSpec for flexible layout management - Follows existing widget patterns (exposes .panel property) - Uses button-based cells for reliable click handling - Empty cells show "Click to add plot" placeholder - Uses .layout pattern for HoloViews DynamicMap rendering - State management tracks occupied cells, selection, and highlights Testing: - 20 comprehensive unit tests covering all functionality - Tests include: initialization, selection, occupancy, insertion, removal, region availability, and error handling - All tests pass Demo application: - Standalone Panel app (examples/plot_grid_demo.py) - No dependency on ESSlivedata services/controllers - Four plot types: curves, scatter, heatmaps, bars - All plots are interactive HoloViews DynamicMaps - Run with: panel serve examples/plot_grid_demo.py --show Code quality: - Passes ruff check and ruff format - Full type hints and NumPy-style docstrings - Follows SPDX license headers Files created: - src/ess/livedata/dashboard/widgets/plot_grid.py (251 lines) - tests/dashboard/widgets/test_plot_grid.py (300 lines) - examples/plot_grid_demo.py (205 lines) - examples/README.md - docs/developer/plans/plot-grid-implementation-summary.md - docs/developer/plans/plot-grid-questions.md Original prompt: We need to add a new PlotGrid widget. It should be based on Panel's GridSpec, with configurable nrow and ncol. On creation there should be no plots. Instead we need a way to select a cell (or a rectangular region, e.g., to select the top left 2x2 subgrid of a 3x3 layout). Maybe we can put a button or checkbox widget into each cell? The selection should then trigger some mechanism (outside the new widget, via some callback/controller) to select data, configure a plot and insert it into the grid. What we are designing here is only the widget (select area, trigger something that returns a plot (hv.DynamicMap in practice), and inserts it). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Changes: 1. Close button: smaller (25x25), transparent background, minimal margins 2. After first click: - Selected cell shows "Click again for 1x1 plot" - Invalid cells are disabled with light-red background and no text - Valid cells show "Click for NxM plot" with dimensions 3. Attempted larger font (24px) during selection (not working yet) Original prompt: We need some small changes in plot_grid.py 1. Close button should be top right, smaller (text size ok, but smaller margins?), and use same color as background, to only the text is visible. 2. When clicking once to select a cell: (a) disable all cells that would lead to invalid selection (b) change other cells text to "click for 1x1 plot", "click for 1x2 plot", etc. Follow-up: disabled cells should have empty label and light-red background instead of gray Follow-up: first-clicked cell should say "Click again for 1x1 plot" and font should be larger (24px) during selection 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Use Panel's stylesheets parameter to properly target the button element and apply 24px bold font during selection. The styles parameter only affects the container, but stylesheets can target internal elements. This provides better visual feedback when selecting plot regions. Original prompt: We need some small changes in plot_grid.py Follow-up: font should be larger (24px) during selection Research showed that Panel button font-size must be set via the stylesheets parameter (targeting the button element) rather than the styles parameter (which only affects the widget container). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Use pn.io.hold() to batch multiple grid cell updates together, preventing the distracting one-by-one cascade effect during cell selection/deselection and plot removal. Applies batching to: - _initialize_empty_cells(): Initial grid population - _refresh_all_cells(): Cell updates during selection - _remove_plot(): Restoring empty cells after plot removal This matches the pattern used in dashboard.py:204 for batched updates. Original prompt: "Please look into @src/ess/livedata/dashboard/widgets/plot_grid.py - when cells update (e.g., to change to red disabled cell during selection) this happens one-cell-at-a-time, which is visually distracting since it happens with maybe 100-200 ms delay for each cell. Is there a better way? Either to make it faster, or do all at once (pn.hold comes to mind, iirc)?" 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The close button's transparent background, red color, and bold font were not taking effect because the `styles` dict lacks the CSS specificity needed to override Panel/Bootstrap defaults. Changes: - Move appearance styling from `styles` dict to `stylesheets` parameter - Use `!important` declarations to override default button styling - Keep positioning styles (absolute, top, right, z-index) in `styles` - Add hover effect for better UX (subtle red background on hover) - Consolidate all styles into constructor (removed `.styles.update()`) The close button now displays correctly as a transparent button with bold red × symbol that highlights on hover. --- Original prompt: Consider `close_button` in plot_grid.py - Why is styles split in two? The button looks white (or very light gray) on the light-gray plot background. Is the transparent option not working? Is there a better approach to make the button invisible (expect for the text)? Follow-up: I have unified into a single `styles` and it still works. However I now notice that the `color` and `font-weight` take no effect either. Can you try option 2?
Implements a new "Plot Grid" tab in PlotCreationWidget, providing a 3x3 grid where users can place plots of varying sizes by selecting rectangular regions. The integration follows a two-modal workflow pattern. Changes: 1. Enhanced PlotGrid (plot_grid.py): - Added in-flight state tracking to prevent concurrent workflows - Changed callback to async pattern (no return value expected) - Added insert_plot_deferred() for completing plot insertion after modal workflow - Added cancel_pending_selection() to abort and reset state - Disabled all cells during modal workflow with visual feedback 2. Created JobPlotterSelectionModal (job_plotter_selection_modal.py): - Two-step wizard: job/output selection → plotter type selection - Uses Tabulator for job/output table (same pattern as PlotCreationWidget) - Filters available plotters by compatibility with selected job/output - Proper modal close detection and cleanup 3. Created PlotGridTab (plot_grid_tab.py): - Orchestrates PlotGrid + two-modal workflow - Chains JobPlotterSelectionModal → ConfigurationModal - Handles success/cancellation paths with proper state cleanup - Inserts plots into grid via deferred insertion 4. Extracted PlotConfigurationAdapter (plot_configuration_adapter.py): - Moved from plot_creation_widget.py to avoid circular imports - Makes adapter reusable between PlotCreationWidget and PlotGridTab 5. Integrated PlotGridTab into PlotCreationWidget (plot_creation_widget.py): - Added "Plot Grid" tab between "Create Plot" and "Plots" - Registered with job service updates for data refresh Technical approach: - Fixed 3x3 grid size for initial implementation - In-flight boolean prevents concurrent plot creation workflows - Deferred insertion enables async modal workflow (vs synchronous callback) - Reuses existing ConfigurationModal without changes Original prompt: Please carefully read @docs/developer/plans/plot-grid-integration-plan.md think through it and start with the implementation. Then use subagents to perform steps 1,2, and 3.
When selecting a cell in an empty PlotGrid, Panel's GridSpec was emitting warnings about overlapping regions. This occurred in two places: 1. During cell refresh (_refresh_all_cells): When updating cell appearance during selection, new cell widgets were assigned to grid positions that already contained cells, causing overlap warnings. 2. During plot insertion (_insert_plot): When inserting a plot container, the target cells still contained the disabled placeholder cells from the selection phase. The fix explicitly deletes existing cells before assigning new ones in both locations. This ensures Panel's GridSpec never detects overlapping widgets during assignment. Changes: - _refresh_all_cells(): Delete each cell before creating and assigning new one - _insert_plot(): Delete all cells in target region before inserting plot All existing tests pass, confirming functionality remains intact while eliminating the warnings. --- Initial prompt: When selecting a cell in an (empty) PlotGrid we get tons of warnings
Fixes two race condition bugs that prevented plots from appearing in the
PlotGrid after completing the modal workflow.
Race Condition Fixes:
1. PlotGridTab: Clear modal reference before plot insertion
- Issue: ConfigurationModal close event would trigger cancel_pending_selection()
after plot was inserted, undoing the insertion
- Fix: Set self._current_config_modal = None before calling insert_plot_deferred()
- Location: plot_grid_tab.py:128-133
2. JobPlotterSelectionModal: Prevent cancel on successful completion
- Issue: When user clicked "Configure", modal closed and triggered
_on_modal_closed() which called cancel_callback(), resetting grid cells
- Fix: Track _success_callback_invoked flag and skip cancel if already succeeded
- Location: job_plotter_selection_modal.py:52, 300, 315
Pattern: Both fixes mark success state BEFORE triggering events that might
fire cleanup handlers.
Test Updates:
- Updated all 20 PlotGrid tests to use deferred insertion API
- Changed mock_callback fixture to return None (async pattern)
- Added insert_plot_deferred() calls after callback invocations
- Updated error tests to verify cancel_pending_selection() behavior
- All tests pass
Documentation:
- Updated plot-grid-implementation-summary.md with:
- Test coverage limitations
- Known issues and fixes section
- Testing approach explanation
Testing: Manual UI testing verified both issues are resolved. Integration
layer race conditions are not covered by automated tests due to complexity
of mocking Panel modal lifecycle events.
Original prompt: I am trying this out in the UI: The dialog chain works, but once complete no plot shows up in the PlotGrid!
Follow-up: Still not working! Observation: During the initial dialog (data selection) all cells are disabled, but once I confirm and the plot-config dialog opens I can see in the background that all cells are back the init "Click to create plot" state.
Final verification: Great, it works now! Can you double check whether our tests capture this corrected behavior adequately?
Replace the dropdown selector and separate configure button with direct action buttons for each plotter type. This reduces the number of clicks required from 2 to 1, making the workflow more convenient especially when there are only 1-2 plotter options available. Changes: - Replace Select widget with dynamically generated Button widgets - Remove configure button - plotter buttons directly trigger configuration - Keep cancel button for dismissing the modal - Update help text to reflect new one-click interaction - Refactor event handling to use _on_plotter_selected() method 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> --- Original prompt: Have a look at @src/ess/livedata/dashboard/widgets/job_plotter_selection_modal.py - we need to make the widget from _show_step_2 more convenient (fewer clicks). There are typically just 1 or a couple of plotter types to select from, thus I would like to: - Replace selector with buttons. - Remove configure button and instead trigger configure when any of the selection buttons is clicked. - Keep the cancel button.
The refresh() method was calling a non-existent refresh() method on JobPlotterSelectionModal, which would have caused an AttributeError. Investigation showed the method is unnecessary because: - A new modal instance is created each time the user requests a plot - The modal holds a reference to JobService (not a snapshot) - Modal's show() method reads directly from JobService's live dictionaries - Modals are short-lived wizards, so live updates during selection aren't valuable This simplifies the code following KISS principles while maintaining correct behavior - modals always display current job data when opened. Original prompt: Please read @plot-grid-code-review.md and investigate item 1. Why was refresh added? Is it needed? Follow-up: I think we should go with option 2. But can you please verify that we correctly get the latest state every time a new modal is created? 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The _setup_keyboard_handler() method was called during initialization but only contained a no-op pass statement with misleading comments suggesting functionality that was not implemented. This commit removes the method entirely to clean up the codebase. Changes: - Removed _setup_keyboard_handler() method definition - Removed call to _setup_keyboard_handler() in __init__ - Updated code review document to mark this issue as fixed All tests continue to pass after this change. Original prompt: Can you address item 6 of @plot-grid-code-review.md -> Remove the method. Update doc and commit when done. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
The modal container was competing with the PlotGrid for vertical space in the parent Column. When the modal was added, Panel redistributed the height, causing the grid to shrink. Solution: Use a zero-height Row container (height=0) for the modal. This keeps the modal in the component tree (required for rendering) while ensuring it doesn't take up any layout space. The modal itself still renders as an overlay when opened. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Extract magic numbers and inline calculations into well-documented helpers: 1. Add _CellStyles dataclass with all color, dimension, and typography constants - Eliminates magic values like '#007bff', '100px', etc. - Makes styling changes easier to maintain and understand 2. Extract pure helper functions for region calculations: - _normalize_region(): normalizes corner coordinates - _calculate_region_span(): calculates row/col span from corners - _format_region_label(): formats dimension labels consistently 3. Update all methods to use new constants and helpers: - _create_empty_cell(): uses _CellStyles for all styling - _on_cell_click(): uses _normalize_region() and _calculate_region_span() - _get_cell_for_state(): uses all three helper functions - _insert_plot(): uses _CellStyles for close button Benefits: - Improved maintainability: all styling in one place - Better testability: pure helper functions easier to unit test - Clearer intent: named constants self-document their purpose - Reduced duplication: region calculation logic centralized All existing tests pass without modification. Original prompt: "Please carefully think through @src/ess/livedata/dashboard/widgets/plot_grid.py - can we improve the code quality? Extract helpers?" 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
….show() The _success_callback_invoked flag prevents calling the cancel callback when the modal is programmatically closed after successful completion. Without resetting this flag in show(), reopening the modal after a successful completion would leave the flag set to True, causing incorrect behavior on subsequent uses. --- Original prompt: Please carefully read job_plotter_selection_modal.py and the diff w.r.t. main - is the _success_callback_invoked mechanism (and related) still needed? There was an intermediate refactoring step with a chain of two modal dialogs instead of one which made this necessary (some race condition), but we have now simplified to a single modal. Follow-up: Can you fix it?
Remove 3 unnecessary _refresh_all_cells() calls that were recreating all empty cell widgets during the plot insertion workflow: 1. After setting _plot_creation_in_flight flag in _on_cell_click 2. After plot insertion in insert_plot_deferred() 3. When cancelling pending selection The visual disabled state was redundant since clicks are already blocked by the early return check (lines 239-241). This reduces widget operations from 3 full grid refreshes to 1 per plot insertion, a 67% reduction for a 4x4 grid. The optimization maintains the same UX while eliminating unnecessary DOM manipulation. --- Original prompt: Have a close look at @src/ess/livedata/dashboard/widgets/plot_grid.py - the _on_cell_click mechanism with _plot_creation_in_flight seems inefficient and complicated: Why do we need to create disabled cells instead of simply reverting to the state before the first click and temporarily set a flag so further clicks (before callback mechanism returns a plot) are ignored? Think and investigate! 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Replace the fragile `_success_callback_invoked` flag pattern with an explicit WizardState enum to prevent race conditions in modal lifecycle management. This makes state transitions clearer and more testable. Changes: - Add WizardState enum (ACTIVE, COMPLETED, CANCELLED) - Replace _success_callback_invoked flag with _state tracking - Add logging throughout for better debuggability - Improve error handling: replace bare except with specific logging - Document modal cleanup workaround for Panel's private API Benefits: - Eliminates race condition between modal close and success callbacks - State transitions are now explicit and type-safe - Better error visibility through logging - Easier to test and reason about lifecycle - No behavioral changes, purely architectural improvement Original prompt: Would you say that refactoring JobPlotterSelectionModal into a cleaner architecture would be beneficial before writing any tests? It should be noted that the contents of "step 1" are a legacy widget and will be replaced fully. Decision: Yes, refactor first with git as safety net, using explicit state machine pattern to eliminate race conditions and improve testability. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Extract steps 2 and 3 into independent, testable component classes following the WizardStep protocol. This makes the wizard more modular and prepares for easy replacement of step 1 (legacy). Changes: - Add WizardStep protocol defining step component interface - Extract PlotterSelectionStep class for step 2 * Encapsulates plotter button creation and filtering logic * Handles errors independently with logging * Implements render() and on_enter() lifecycle methods - Extract ConfigurationStep class for step 3 * Encapsulates configuration panel creation * Manages panel lifecycle (create/reset) * Handles errors for missing sources and invalid specs - Update JobPlotterSelectionModal to use step components * Delegates to step components via set_selection() and on_enter() * Simplified _show_step_2() and _show_step_3() methods * Removed duplicate plotter button logic Benefits: - Steps 2 and 3 are now independently testable - Clear separation of concerns and lifecycle management - Easier to understand and maintain - Ready for step 1 replacement (kept inline as legacy) - No behavioral changes, purely structural improvement Original prompt: Can we also perform the Pattern 2 and 3 refactorings you suggested earlier? Note: Pattern 3 (Command Pattern) was already implemented via WizardState enum in the previous commit. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Complete the step component extraction by moving step 1 (job/output selection) into JobOutputSelectionStep class. The modal is now a pure orchestrator with zero knowledge of step internals. Changes: - Add JobOutputSelectionStep class for step 1 * Encapsulates job/output table creation and management * Handles table population from JobService * Manages selection state and validation * Calls back with (job, output, is_valid) on changes * Implements render() and on_enter() lifecycle methods - Simplify JobPlotterSelectionModal * Removed _create_job_output_table() (now in step component) * Removed _update_job_output_table() (now in step component) * Replaced _on_job_output_selection_change() with simpler callback * Updated _show_step_1() to use step1.render() * Updated show() to rely on on_enter() for data refresh * Modal reduced from ~360 to ~280 lines Architecture Benefits: - Modal is now PURE ORCHESTRATION * Only manages: navigation, state transitions, modal lifecycle * Zero knowledge of table structure, plotter buttons, or config panels * All step logic delegated to independent components - All three steps follow same pattern * Consistent WizardStep protocol * Each step independently testable * Easy to swap/replace any step (including step 1) - Plugin architecture achieved * New step 1 just needs to implement WizardStep protocol * Pass different step1 object in __init__, done! * No changes to modal orchestration needed Original prompt: Exactly, extract it please! 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Replace tests that accessed private attributes and methods with tests that interact exclusively through the public API. This makes tests resilient to internal refactoring and clearer in their intent. Changes: - Remove all accesses to private fields (_nrows, _occupied_cells, _first_click, _pending_selection, _plot_creation_in_flight) - Remove all calls to private methods (_on_cell_click(), _is_cell_occupied(), _remove_plot(), _is_region_available()) - Add helper functions to interact through public API: * simulate_click() - triggers Panel button events * is_cell_occupied() - checks observable grid state * get_cell_button() - retrieves buttons from grid cells * find_close_button() - locates plot close buttons - Test observable behavior through grid.panel instead of internal state - Reorganize tests into clearer categories (Initialization, Selection, Insertion, Removal, Overlap Prevention, Cancellation, Error Handling) Benefits: - Tests won't break when refactoring internal implementation - Tests document proper usage of the public API - Tests express what users can observe and do, not how it's implemented - All 16 tests pass with improved maintainability 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> Original prompt: Please carefully look at @tests/dashboard/widgets/plot_grid_test.py - the tests are accessing private fields and methods. We should NEVER test implementation details. Please think through everything, come up with a strategy for writing better tests that only test public behavior, and implement them. Clean test code is paramount!
Replace MagicMock with a simple fake class in PlotGrid tests to comply with project standards that prohibit use of unittest.mock. The FakeCallback class provides the same functionality needed for testing: - Call counting - Call history tracking - Assertion methods (assert_called_once, assert_not_called) - Reset capability - Side effect support for error testing All tests continue to pass with this change. Original prompt: Can you see what violates our standards in @tests/dashboard/widgets/plot_grid_test.py ? Follow-up: Exactly, go! Follow-up: Commit when done and tests pass.
Refactors the multi-step wizard logic into a reusable Wizard class, addressing several design issues in the original implementation: Key improvements: - Context-based state: Replace scattered state variables with PlotterSelectionContext dataclass for type-safe data flow between steps - Unified navigation: All advancement paths (Next button, plotter buttons, config completion) now use the same wizard.advance()/complete() methods - Clean completion: Steps update context and call wizard.complete(), eliminating callback spaghetti (_on_config_complete, _on_panel_success chains) - Separation of concerns: Wizard manages navigation/modal UI/lifecycle, steps handle content/validation/context updates Implementation details: - WizardStep protocol: render(), is_valid(), on_enter() - Steps receive wizard callbacks (advance, complete) via constructor (Option 4 pattern) - Wizard.refresh_ui() allows steps to update button state on selection changes - ~68% code reduction in JobPlotterSelectionModal (280 → 90 lines) All 349 dashboard tests pass. --- Original prompt: "Please think through @src/ess/livedata/dashboard/widgets/job_plotter_selection_modal.py - I wonder if it would make sense to extract a generic `class Wizard` from JobPlotterSelectionModal?" Follow-up decisions: - Use dataclass for context (not dict-like or Pydantic) for simplicity - Pass advance/complete callbacks to steps via constructor to avoid circular dependencies - Plotter buttons call wizard.advance() after updating context (no special "bypass")
Move modal management from Wizard to JobPlotterSelectionModal. The Wizard is now a reusable component that handles navigation logic and returns renderable content, while consumers decide how to present it (modal, inline, etc.). Changes: - Wizard: Remove modal creation, title/width/height parameters, show() method - Wizard: Add reset() and render() methods for generic usage - JobPlotterSelectionModal: Create and manage modal wrapping wizard content - JobPlotterSelectionModal: Handle modal lifecycle (open/close events) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> Original prompt: Please have a look at @src/ess/livedata/dashboard/widgets/wizard.py and its usage - I am not sure there is benefit in making the Wizard itself a Modal. Would it be cleaner to have it is a generic widget, which can then be shown in a modal (in the job plotter)?
Replace complex callback injection pattern with clean observer pattern: - Convert WizardStep from Protocol to ABC base class - Add simple on_ready_changed callback mechanism (single callback, no param dependency) - Steps notify wizard of ready state changes via _notify_ready_changed() - Wizard registers callback and updates Next button state reactively - Steps that need wizard methods store reference via set_wizard() Eliminates: - Placeholder callbacks (lambda: None) - Private attribute reassignment (step._on_selection_changed = wizard.refresh_ui) - param.Parameterized dependency and metaclass complexity - Multiple callback support (YAGNI) - Duplicate callback registration bug Original prompt: "Please have a look at @src/ess/livedata/dashboard/widgets/job_plotter_selection_modal.py - I am not sure why we have callbacks such as on_selection_changed? Think!" Discussion: Initially considered using param.Parameterized reactive system, but decided against it due to metaclass conflicts with ABC and unnecessary complexity. Implemented clean observer pattern instead with single callback. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Fix inconsistent button placement across wizard steps by: 1. Standardizing button order to [Cancel | Back | Spacer | Next] following UI conventions (destructive actions left, progression right) 2. Adding embedded mode to ConfigurationPanel to support usage without its own buttons when embedded in other containers 3. Enabling custom action button labels on the wizard's last step Previously, Step 2 had Cancel sandwiched between Back and Next, which was confusing. Step 3 had two separate button rows (ConfigurationPanel's buttons plus Wizard's buttons), creating visual inconsistency. Now all steps have a single, consistently ordered button row, with the Next button relabeling to "Create Plot" on the final step. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> --- Original prompt: Consider @src/ess/livedata/dashboard/widgets/job_plotter_selection_modal.py - the control buttons are a bit inconsistent: In step 2, cancel is between back and next, which is confusing. In step 3, there is Create Plot" in a row above "Back | Cancel". Not sure the latter is easy to fix since it is part of ConfigurationPanel? But maybe that is a bad approach and the button should move into ConfigurationModal (for other uses of ConfigurationPanel) and we re-label our "Next" button when in the last step? Ultrathink and inspect everything tying into this!
Refactor to properly separate concerns between ConfigurationPanel and ConfigurationModal by moving all button-related logic to the modal: ConfigurationPanel (business logic): - Focuses on validation and action execution - No UI buttons, just the form and error display - Exposes execute_action() for external control ConfigurationModal (presentation): - Wraps ConfigurationPanel and adds its own buttons - Manages button placement and styling - Handles button clicks by calling panel.execute_action() This eliminates the `show_buttons` parameter hack and provides clean separation of concerns. ConfigurationPanel can now be embedded in any container (like Wizard) without button-related configuration. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> --- Original prompt: I am not sure the mechanism for optional buttons in ConfigurationPanel is great. Would it make sense to move the buttons into ConfigurationModal? The current solution feels like a hack.
Remove the hasattr() check for action_button_label() method on steps. Instead, pass the action button label as an optional parameter to the Wizard constructor, making it a wizard-level concern rather than a step-level method. This improves separation of concerns and removes dynamic method checking. Original prompt: The action_button_label hack is ugly. Can we pass the label as a constructor argument to the Wizard instead of getting it from the step? 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Implements 54 tests covering all aspects of the wizard framework: - State management (active, completed, cancelled) - Navigation (advance, back, step validation) - Rendering (buttons, step content, visibility) - Completion and cancellation workflows - Observer pattern for step ready state changes - Integration tests for complete wizard flows Tests follow project conventions using simple fakes instead of mocks and focus on testing public behavior rather than implementation details. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> Original prompt: Please implement tests for the classes in @src/ess/livedata/dashboard/widgets/wizard.py
…attern Move duplicated HTML header generation from individual wizard steps into the WizardStep base class. Steps now define their name and optional description via abstract properties, and the base class automatically generates consistent headers with step numbers. Changes: - Add abstract `name` property to WizardStep - Add optional `description` property to WizardStep - Convert `render()` to template method that generates header and calls `render_content()` - Update all steps to implement `render_content()` instead of `render()` - Remove manual HTML header code from JobOutputSelectionStep, PlotterSelectionStep, and ConfigurationStep - Update FakeWizardStep test implementation to match new pattern Benefits: - DRY: Header generation logic centralized in one place - Consistency: All steps have uniform header formatting - Maintainability: Easy to change header style across all steps - Type safety: Step name enforced via abstract property - Automatic numbering: Wizard injects step number based on position 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> Original prompt: "In @src/ess/livedata/dashboard/widgets/job_plotter_selection_modal.py all steps set an HTML header -- could this be moved into the base class, if steps are required to define their name?" Follow-up: "Yes, `name` and `description` is cleaner!"
…xecution
This commit eliminates several design issues in the wizard and configuration
panel architecture:
1. Remove hasattr hack from Wizard.advance()
- Add execute() method to WizardStep base class with default no-op implementation
- Wizard now calls execute() on all steps instead of checking with hasattr
- Makes the protocol explicit and type-safe
2. Separate validation from execution in ConfigurationPanel
- Split execute_action() into three methods:
- validate(): validates and shows errors inline
- execute_action(): executes the action (assumes validation passed)
- validate_and_execute(): convenience method for modal use case
- ConfigurationStep now uses validate() instead of accessing private members
- No duplicate validation in wizard flow
3. Remove unnecessary callbacks from ConfigurationPanel
- Remove success_callback and error_callback parameters
- Panel focuses on UI state management
- Owners (Modal/Wizard) manage workflow directly based on return values
- Clearer separation of concerns
4. Simplify ConfigurationModal
- Uses validate_and_execute() and manages modal close directly
- No intermediate callback layer needed
Benefits:
- Explicit protocols with no runtime reflection
- Single Responsibility Principle - each class has clear, focused responsibilities
- Less coupling - panel doesn't need callbacks, owner controls workflow
- No duplicate validation - validation happens once, execution happens once
- Clearer control flow - easy to understand by reading the code
- Type-safe and statically analyzable
All 403 dashboard tests pass.
Original prompt: In @src/ess/livedata/dashboard/widgets/wizard.py the check
for the `execute` method feels like a hack. Is there are cleaner way?
Follow-up discussion:
- Does the separation of `is_valid` from `execute` still make sense?
- Is `ConfigurationPanel.execute_action` a design error and the culprit for
this dilemma?
- Looking at callbacks - can't the owner call validate, then call the callback
itself directly?
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
This is a checkpoint commit before further refinement. Current state: - Wizard steps use Generic[TInput, TOutput] for type-safe data flow - Each step's execute() returns data for the next step - Eliminated shared mutable context in favor of data threading - ConfigurationAdapter.start_action() changed to return Any - PlotConfigurationAdapter returns tuple instead of using callback Known issues to be addressed: - ConfigurationStep has brittle isinstance(result, tuple) check - Inconsistent with other ConfigurationAdapter implementations - Will be cleaned up by restoring callback pattern Original prompt: Please carefully think through the uncommitted changes. The refactor was a bit involved, do you think everything is sound and correct?
…ionStep Cleaner implementation that separates concerns: - ConfigurationAdapter.start_action() remains callback-based (returns None) - PlotConfigurationAdapter uses success callback to report plot creation - ConfigurationPanel.execute_action() returns bool (success/failure) - ConfigurationStep stores callback result locally and returns it from execute() Benefits: - No brittle isinstance() checks in ConfigurationStep - Consistent with other ConfigurationAdapter implementations - Clear separation: Panel handles UI/validation, Step handles result threading - ConfigurationPanel remains generic and reusable All wizard tests pass (53/53). Original prompt: Please carefully think through the uncommitted changes. The refactor was a bit involved, do you think everything is sound and correct? Follow-up: Can you think a bit harder if there is a really clean solution that is not brittle?
The term "commit" better captures what wizard steps do - they finalize and commit the user's selections for the pipeline, rather than "execute" which implies side effects. Semantic improvements: - Step 1 & 2: "commit the selection" (not "execute the selection") - Step 3: "commit to create the plot" (actually does execute, but commit works too) - Aligns with transaction/pipeline mental model (like Git, databases) - Reduces confusion about side effects on intermediate steps Changes: - WizardStep.execute() → commit() with improved docstring - Wizard.advance() now calls commit() instead of execute() - All step implementations updated (JobOutputSelectionStep, PlotterSelectionStep, ConfigurationStep) - Test implementations updated All 53 wizard tests pass. Original prompt: How about execute -> commit? Does that name make more sense?
Apply pn.io.hold() context manager when modifying multiple grid cells in _insert_plot() to batch updates and avoid intermediate rendering states. This makes the usage pattern consistent with other methods that modify multiple cells (_initialize_empty_cells, _refresh_all_cells, _remove_plot). --- Original prompt: Have a look at @src/ess/livedata/dashboard/widgets/plot_grid.py - we are using pn.io.hold in some cases but not in all? 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
SimonHeybrock
commented
Oct 27, 2025
Comment on lines
+51
to
+52
| This is mostly copied from the legacy PlotCreationWidget but is considered legacy. | ||
| The contents of this widget will be fully replaced. |
There was a problem hiding this comment.
In other words: Don't bother reviewing this class.
- Add PlotterSpec import and use it instead of 'object' in type hints - Remove broad try/except Exception block in _update_plotter_selection() - Let errors propagate naturally to expose bugs rather than silently catching them The job_output data comes from the previous wizard step which validates the selection, so these values should always be valid. If they're not, that's a bug that should fail fast rather than being caught. Original prompt: The type hints around `available_plots` in @src/ess/livedata/dashboard/widgets/job_plotter_selection_modal.py seem to be improvable? Follow-up: Why do we have some unmotivated(?) try/except in _update_plotter_selection? Decision: Remove the try/except - the data should always be valid.
SimonHeybrock
commented
Oct 27, 2025
| from ess.livedata.dashboard.plotting_controller import PlottingController | ||
|
|
||
|
|
||
| class PlotConfigurationAdapter(ConfigurationAdapter): |
There was a problem hiding this comment.
Extracted from plot_creation_widget.py
SimonHeybrock
commented
Oct 27, 2025
Comment on lines
+58
to
+62
| self._plot_grid_tab = PlotGridTab( | ||
| job_service=job_service, | ||
| job_controller=job_controller, | ||
| plotting_controller=plotting_controller, | ||
| ) |
There was a problem hiding this comment.
The entire PlotCreationWidget is legacy, putting this here is temporary.
SimonHeybrock
commented
Oct 27, 2025
Comment on lines
+42
to
+45
| # Create PlotGrid (3x3 fixed) | ||
| self._plot_grid = PlotGrid( | ||
| nrows=3, ncols=3, plot_request_callback=self._on_plot_requested | ||
| ) |
There was a problem hiding this comment.
Medium term we will probably add a way to add new tabs with a custom row and column count.
Replace the underutilized WizardState enum with a simpler boolean flag and public is_finished() method. Changes: - Remove WizardState enum (ACTIVE, COMPLETED, CANCELLED) - Replace internal _state with _finished boolean flag - Add public is_finished() method for external state checking - Update job_plotter_selection_modal to use is_finished() instead of accessing private _state - Update all tests to use is_finished() - Remove redundant/trivial tests: * SampleContext dataclass (unused) * test_creates_wizard_with_single_step (trivial) * test_creates_wizard_with_multiple_steps (trivial) * test_render_returns_column (weak type check) * test_render_step_includes_step_content (vague assertion) * TestWizardState class (entire test class) Rationale: The three-state enum was only ever tested as "ACTIVE vs not-ACTIVE", which is just a boolean. The wizard never used the state internally for decision making - it only set it. External code (modal) was reaching into private _state to check if wizard was done. The boolean flag with a public method is simpler, clearer, and better encapsulated. Test count: 49 → 45 (all passing) Original prompt: In @src/ess/livedata/dashboard/widgets/wizard.py the state machine WizardState seems pretty unused, or is it accessed from anywhere? Follow-up: Do all the wizard tests still make sense or are there legacy tests (that have been refactored and work but are not really sensible any more)? 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
The goal of this is twofold:
Medium term this should allow us to also save plot layout configs.
For now the, result-data-table is kind of reused in step 1 of the config wizard. This is temporary and will be replaced in a follow-up.