refactor(app): remove unnecessary reactive effects#35386
Open
Hona wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors Solid/React-style “derived state” and event wiring to reduce unnecessary reactive effects, shifting toward render-time derivation and explicit lifecycle helpers while tightening async ownership/cancellation across app, UI, and session viewer surfaces.
Changes:
- Replace multiple effect-driven sync/reset patterns with small deterministic domain helpers (effective selections, change-mode fallback, file-tree open state, etc.).
- Introduce reusable lifecycle primitives for bounded retries and animation-frame scheduling/cancellation; apply them to session history loading, review focus, and viewer readiness/selection replay.
- Expand and harden browser/unit test coverage (Happy DOM preload for
packages/ui, plus new tests across UI/session-ui/app domains).
Reviewed changes
Copilot reviewed 59 out of 60 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui/src/v2/components/tooltip-v2.tsx | Refactors tooltip trigger observation away from a reactive effect into a ref-based observer lifecycle. |
| packages/ui/src/hooks/use-filtered-list.tsx | Adds controlled filter support + active reconciliation + move notifications for list navigation. |
| packages/ui/src/hooks/use-filtered-list.test.tsx | Tests async item resolution preserving a valid current selection. |
| packages/ui/src/context/dialog.tsx | Ensures detached dialog roots are disposed when the provider cleans up. |
| packages/ui/src/context/dialog.test.tsx | Tests provider disposal cleans up detached dialog root content. |
| packages/ui/src/components/tooltip.test.tsx | Tests tooltip reactivation and popup-expanded blocking behavior. |
| packages/ui/src/components/text-reveal.tsx | Improves rAF scheduling/cancellation and disposal-guarding around readiness + swaps. |
| packages/ui/src/components/text-reveal.test.tsx | Tests readiness rAF cancellation and post-dispose font readiness behavior. |
| packages/ui/src/components/list.tsx | Removes internal filter sync effects; routes filter + clear through unified path. |
| packages/ui/src/components/list.test.tsx | Tests controlled filter clear path + onMove semantics. |
| packages/ui/package.json | Updates UI test script for browser-conditions + Happy DOM preload; adds dev deps. |
| packages/ui/happydom.ts | Registers Happy DOM globals and Solid TSX plugin for Bun test runs. |
| packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx | Tightens focus token ownership for review comment focus clearing/scrolling. |
| packages/session-ui/src/pierre/selection-bridge.ts | Adds createSelectionReplay to buffer/replay selection before viewer readiness. |
| packages/session-ui/src/pierre/selection-bridge.test.ts | Tests selection replay behavior with a “viewer not ready yet” gate. |
| packages/session-ui/src/pierre/media.ts | Adds svgMediaError classification helper for unusable SVG values. |
| packages/session-ui/src/pierre/media.test.ts | Tests SVG error classification vs accepted SVG sources. |
| packages/session-ui/src/pierre/file-runtime.ts | Adds frame tracking + cancellation/guarding to viewer “ready” watcher. |
| packages/session-ui/src/pierre/file-runtime.test.ts | Tests watcher clear cancels and guards pending readiness callbacks. |
| packages/session-ui/src/pierre/animation-frame.ts | Introduces an animation-frame scope to cancel/guard stale scheduled callbacks. |
| packages/session-ui/src/pierre/animation-frame.test.ts | Tests animation-frame scope cancellation + stale callback guarding. |
| packages/session-ui/src/components/session-review.tsx | Uses animation-frame scope to prevent stale focus-scroll work from accumulating. |
| packages/session-ui/src/components/file.tsx | Uses animation-frame scopes + selection replay to guard async viewer selection/application. |
| packages/session-ui/src/components/file-media.tsx | Routes SVG invalid handling through svgMediaError and propagates structured errors. |
| packages/app/src/pages/session/use-session-hash-scroll.ts | Adds request latch + cursor-based keying to bound history load retries and log failures. |
| packages/app/src/pages/session/timeline/message-timeline.tsx | Removes effect-based wiring; adds error handling for sync calls and exposes callbacks directly. |
| packages/app/src/pages/session/terminal-panel-v2.tsx | Removes focus-blur effect on close; retains inert/aria-hidden behavior. |
| packages/app/src/pages/session/session-model-helpers.ts | Adds ChangeMode + effectiveChangeMode fallback helper. |
| packages/app/src/pages/session/session-model-helpers.test.ts | Tests change-mode fallback behavior. |
| packages/app/src/pages/session/file-tabs.tsx | Uses scheduled task to clear focus safely and avoids stale focus clearing. |
| packages/app/src/pages/session/effect-lifecycle.ts | Adds createInputRequestLatch + createScheduledTask primitives for effect lifecycles. |
| packages/app/src/pages/session/effect-lifecycle.test.ts | Tests bounded retries, ownership, and scheduled-task cancellation semantics. |
| packages/app/src/pages/session/composer/session-revert-dock.tsx | Removes effect-driven auto-collapse reset on item changes. |
| packages/app/src/pages/session.tsx | Applies change-mode fallback, request latches, scheduled review diff scrolling, followup edit command slot, and improved error logging. |
| packages/app/src/pages/home.tsx | Switches to “effective selection” derivation and memoized active-key derivation for search results. |
| packages/app/src/pages/home-domain.ts | Adds effectiveHomeSelection and homeSearchActiveKey domain helpers. |
| packages/app/src/pages/home-domain.test.ts | Tests home selection fallback + active search key derivation. |
| packages/app/src/context/settings.tsx | Adds persisted migration (migrateSettings) and removes effect-based migration. |
| packages/app/src/context/settings.test.ts | Tests settings migration behavior. |
| packages/app/src/context/permission.tsx | Removes effect-based auto-accept persistence; derives allow-all behavior via options. |
| packages/app/src/context/permission-auto-respond.ts | Adds allow-all fallback logic gated by persisted readiness without mutating persisted state. |
| packages/app/src/context/permission-auto-respond.test.ts | Tests allow-all fallback behavior and readiness gating. |
| packages/app/src/context/global.tsx | Adds effectiveSettingsServer derivation and removes effect-driven persisted key rewriting. |
| packages/app/src/context/global.test.ts | Tests effective settings server selection fallback behavior. |
| packages/app/src/components/titlebar.tsx | Simplifies effect to only “remember” current tab when available. |
| packages/app/src/components/status-popover-body.tsx | Removes a no-op effect and marks unused prop as intentionally unused. |
| packages/app/src/components/settings-v2/dialog-server-v2.tsx | Aligns server dialog lifecycle with controller callbacks instead of effect-driven close. |
| packages/app/src/components/prompt-input/edit.ts | Introduces edit “slot” + command for applying edits and scheduling focus. |
| packages/app/src/components/prompt-input/edit.test.ts | Tests edit slot mount-load and command focus scheduling cancellation. |
| packages/app/src/components/prompt-input.tsx | Replaces edit effect with edit command ref lifecycle and explicit cleanup. |
| packages/app/src/components/file-tree-v2.tsx | Introduces effective open-state derivation and preserves explicit collapse during filtering. |
| packages/app/src/components/file-tree-v2.test.tsx | Tests effective open-state derivation behavior. |
| packages/app/src/components/file-tree-v2-domain.ts | Adds effectiveFileTreeOpen helper. |
| packages/app/src/components/directory-picker-domain.ts | Adds navigation lifecycle helper to invalidate async navigation on disposal. |
| packages/app/src/components/directory-picker-domain.test.ts | Tests navigation lifecycle disposal invalidation. |
| packages/app/src/components/dialog-select-server.tsx | Adds form event callbacks and improves invalid URL handling; centralizes reset/callback logic. |
| packages/app/src/components/dialog-select-server.test.ts | Tests form event application/reset behavior. |
| packages/app/src/components/dialog-select-server-domain.ts | Adds applyServerFormEvent helper and callback type. |
| packages/app/src/components/dialog-select-directory-v2.tsx | Uses navigation lifecycle object and disposes it on cleanup to prevent stale async updates. |
| bun.lock | Records new dev dependencies for UI testing/tooling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
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
Testing
Reference: https://react.dev/learn/you-might-not-need-an-effect