Fix NavigationBehavior.PreserveState layout disposal issue (#67) #69
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
Implemented Active/Inactive State Pattern for layout nodes to prevent ObjectDisposedException when navigating with PreserveState behavior. ## Core Changes **Layout Lifecycle:** - Added IActivatableNode interface defining OnActivate/OnDeactivate - Added virtual OnActivate/OnDeactivate methods to LayoutNode base class - Layout nodes now pause/resume instead of dispose/recreate on navigation - Subjects remain alive during deactivation to prevent in-flight event errors **ReactivePage:** - Layout tree built once and preserved across navigations - OnNavigatedTo: Build layout on first call, activate on subsequent calls - OnNavigatingFrom: Deactivate layout instead of disposing - Added IDisposable implementation for proper cleanup **Node Implementations:** - TextInputNode: Pause/resume cursor animation, removed auto-start from constructor - SpinnerNode: Pause/resume animation timer - ReactiveLayoutNode: Pause/resume observable subscriptions - ConditionalNode: Pause/resume subscription and both branches - SelectionListNode: Deactivate embedded TextInputNode - ModalNode: Propagate lifecycle to content - ContainerNode: Propagate lifecycle to all children **Application Cleanup:** - Added TerminaApplication.Dispose() to clean up cached pages - IBindablePage extends IDisposable for proper resource management ## Testing Added comprehensive test suites (30 tests, all passing): - LayoutNodeLifecycleTests: Timer management, subscription pausing, propagation - ReactivePageLifecycleTests: Layout preservation, race condition prevention ## Documentation Updated documentation to reflect lifecycle changes: - concepts/navigation.md: PreserveState behavior and layout lifecycle - advanced/custom-components.md: Component lifecycle implementation guide - concepts/architecture.md: Navigation flow for both behaviors Fixes #67
ModalNode now extends LayoutNode to properly participate in lifecycle propagation. This fixes the bug where text input in modals would not work because OnActivate() was never being called on the TextInputNode inside the modal. Changes: - ModalNode now extends LayoutNode instead of just implementing interfaces - Added override keywords to OnActivate, OnDeactivate, Measure, Render, Dispose - Added new keywords to WidthConstraint and HeightConstraint properties - TextInputNode constructor no longer auto-starts animation (moved to OnFocused) This ensures that when a page with modals is activated/deactivated, the lifecycle properly propagates: Page -> Layout -> ReactiveLayoutNode -> Modal -> TextInputNode
Critical fix: ModalNode now forwards OnFocused() and OnBlurred() to its content if the content implements IFocusable. This is essential for TextInputNode inside modals to receive focus and start cursor animation. Without this, text input in modals doesn't work because: 1. TextInputNode no longer auto-starts in constructor (fixed earlier) 2. OnActivate() only starts cursor if _hasFocus is true 3. OnFocused() sets _hasFocus and calls Start() 4. But if the modal doesn't forward OnFocused(), the TextInputNode never receives it and never starts This completes the modal lifecycle fix.
Critical fix: ReactiveLayoutNode now calls OnActivate() on new children when they are dynamically swapped in via the observable subscription, if the ReactiveLayoutNode itself is currently active. This fixes the modal text input bug where: 1. Page loads with EmptyNode in the modal slot 2. User presses 'A', IsAddingItem becomes true 3. ReactiveLayoutNode switches from EmptyNode to _addModal 4. Without this fix, _addModal never receives OnActivate() 5. Therefore its TextInputNode content never receives OnActivate() 6. Even when OnFocused() is forwarded, TextInputNode.OnActivate() checks _hasFocus before starting, but focus happens AFTER activation 7. Result: cursor never animates, text input appears broken With this fix: - When modal is swapped in, OnActivate propagates to TextInputNode - When Focus.PushFocus is called, OnFocused propagates to TextInputNode - TextInputNode starts cursor animation - Text input works! ✅ Applied to both ReactiveLayoutNode and ReactiveLayoutNode<T>, in both constructor subscriptions and OnActivate re-subscriptions.
Critical fixes: 1. ReactiveLayoutNode now starts with _isActive = false (was incorrectly true) 2. ReactivePage subscribes to layout Invalidated events and calls RequestRedraw() 3. Made ReactiveViewModel.RequestRedraw public for page access Root cause: PR #68 removed manual RequestRedraw() calls from ViewModels but didn't subscribe to layout invalidation events. This broke reactive updates from backend (streaming demo) and modal interactions. Fixes both issues reported by user: - Streaming demo: backend text not appearing until keyboard input - Todo demo: modal text input not working All 616 tests passing.
ContainerNode now implements IInvalidatingNode and subscribes to all children's Invalidated events, re-emitting them upward. This allows invalidation to bubble up through the layout tree from deep children (like StreamingTextNode) to the root where ReactivePage can catch it. Before: Invalidation stopped at the first parent container After: Invalidation bubbles all the way to the root This fixes the streaming demo where backend updates wouldn't trigger redraws until user input occurred. All 616 tests passing.
PanelNode now implements IInvalidatingNode and subscribes to its content's Invalidated events, re-emitting them upward. This fixes streaming demo where StreamingTextNode emits invalidation but it was stopping at PanelNode and never reaching the root. Also added OnActivate/OnDeactivate to properly manage content lifecycle. All 616 tests passing.
Changes: - Reduced thinking delay from 600ms to 200ms per token (3x faster) - Added animated SpinnerNode with Dots style to thinking panel - Improved spacing between user input and assistant response (2 blank lines) - Increased thinking panel height to accommodate spinner The thinking panel now shows: [spinner] Thinking... [thinking text phrases] This makes the demo feel more responsive and vibrant while backend processing happens. All 616 tests passing.
Key fixes: - Removed Buffer.HasContent check from thinking panel condition The panel now shows immediately when IsGenerating=true, not waiting for thinking tokens to arrive. This ensures spinner is visible. - Moved extra blank line to AFTER assistant output finishes (in CleanupGeneration) instead of before it starts. Creates better visual separation between Q&A pairs: User: question [1 blank line] Assistant: response [2 blank lines] User: next question The animated spinner should now be visible during the thinking phase. All tests passing.
Changes: - Remove separate thinking panel in favor of inline spinner - Emit animated spinner frames directly to ChatOutput observable - Use backspace characters to replace spinner frames for smooth animation - Clear spinner with backspace when first text chunk arrives - Remove unused ThinkingOutput and ClearThinking observables - Simplify UI by showing spinner on same line as "Assistant:" prefix The spinner now appears inline in the chat history during generation and disappears seamlessly when the actual response text begins, providing a more integrated and polished user experience.
The backspace-based spinner animation doesn't work with StreamingTextNode because it's an append-only component that treats backspace as a literal character rather than a control operation. Simplified approach: - Skip ThinkingToken emissions entirely - Show text only when it arrives - Status bar already indicates "Generating response..." during thinking - Removed unused spinner-related fields This eliminates the visual artifacts where spinner frames were appearing as text and cloning to the right.
Instead of trying to emit spinner frames to the text stream, use a proper SpinnerNode component that manages its own animation timer. Architecture: - SpinnerNode is a self-animating layout component with built-in timer - ReactiveLayoutNode switches between spinner and empty node based on IsGenerating state - Spinner appears as a separate line in the chat history panel - No text stream manipulation needed - clean separation of concerns The spinner now appears within the chat history panel during generation and disappears when generation completes, without complex backspace operations or text stream editing.
Problem: Reusing the same SpinnerNode instance across ReactiveLayoutNode emissions caused ownership conflicts. The node would be adopted by one parent layout, then the next emission would try to reparent it, causing lifecycle issues. Solution: - Create a new SpinnerNode instance each time IsGenerating becomes true - Remove conflicting Height constraints - Let ReactiveLayoutNode manage the lifecycle of each child Now the spinner should appear properly when generation starts and disappear when it completes.
Architecture:
- Track HasReceivedText in ViewModel to distinguish thinking vs streaming
- Show "🤖 Assistant: {red spinner}" on separate line while waiting
- On first text chunk, emit Assistant prefix to chat history and hide spinner
- Use CombineLatest to reactively show/hide spinner based on dual state
Flow:
1. User submits → IsGenerating=true, HasReceivedText=false
2. Spinner line appears: " 🤖 Assistant: ⠋" (animating)
3. First text arrives → Emit "🤖 Assistant: " to chat, HasReceivedText=true
4. Spinner line disappears, text streams in chat history
5. Generation completes → IsGenerating=false
The spinner appears inline with matching visual style (emoji, color, bold)
and seamlessly transitions to the actual response text without jarring
layout shifts.
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.
Summary
Fixes #67 by implementing the Active/Inactive State Pattern for layout nodes to prevent ObjectDisposedException when navigating with PreserveState behavior.
Core Changes
Layout Lifecycle Architecture:
IActivatableNodeinterface defining OnActivate/OnDeactivateLayoutNodebase classReactivePage:
Node Implementations:
Application Cleanup:
Bug Fix
Fixed critical bug where text input in modals was broken because ModalNode didn't extend LayoutNode, preventing OnActivate from being called on embedded TextInputNodes.
Testing
Documentation
Updated documentation to reflect lifecycle changes: