perf: optimize element selection responsiveness #2854
Conversation
Prioritize visual feedback by rendering the selection outline immediately, before triggering heavy operations like chat context updates and UI animations. Key optimizations: - Split click handling to render outline first, then update state - Defer chat context calculations with requestAnimationFrame - Reduce animation durations across UI components (400ms → 150ms) - Optimize overlay refresh debounce timing (100ms → 50ms) - Batch MobX updates with runInAction to prevent re-renders This significantly improves responsiveness, especially for large projects with many branches where context calculations were blocking the UI. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughShortened and tightened UI animation timings across overlay chat, buttons, editor bar, and overlay opacity. ChatContext gained debounced/cancellable async updates and file-content truncation. Elements click batching splits outline rendering and selection into separate actions with warmup logic. Overlay refresh gained concurrency guard, caching, element-scope limit, and click-rect storage capped. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as UI
participant ChatCtx as ChatContext
participant Fetch as getChatContext()
participant Abort as AbortController
UI->>ChatCtx: trigger reaction
ChatCtx->>Abort: abort previous controller (if any)
ChatCtx->>ChatCtx: schedule delayed update (300ms)
Note right of ChatCtx: if not aborted
ChatCtx->>Fetch: fetch context (async)
Fetch-->>ChatCtx: result
ChatCtx->>ChatCtx: assign this.context
sequenceDiagram
autonumber
participant User as User
participant Elements as ElementsManager
participant MobX as runInAction
participant OverlayState as OverlayState
participant Selected as SelectionStore
participant Frame as FrameData.view
User->>Elements: click(domEls)
Elements->>MobX: Action 1 — update outlines/click-rects
MobX->>OverlayState: clear & add click-rects (adaptRectToCanvas)
Elements->>Frame: warmup check (first/idle click)
Elements->>MobX: Action 2 — update selection
MobX->>Selected: clear & repopulate _selected
sequenceDiagram
autonumber
participant Trig as OverlayTrigger
participant Manager as OverlayManager
participant Cache as elementCache
participant Fetch as fetchElementData
Trig->>Manager: refresh()
Manager->>Manager: return pendingRefreshPromise if exists
Manager->>Cache: purge stale
Manager->>Manager: take first N elements
Manager->>Cache: use cached entries or
Manager->>Fetch: fetchElementData
Fetch-->>Manager: data
Manager->>OverlayState: adaptRect/add click-rects
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| requestAnimationFrame(() => { | ||
| this.getChatContext().then((context) => (this.context = context)); | ||
| }); |
There was a problem hiding this comment.
Race condition bug: Multiple rapid element selections can cause context updates to execute out of order. If a user clicks elements A then B quickly, the requestAnimationFrame for A might execute after B's, causing A's context to overwrite B's context even though B is the currently selected element. This violates the user's expectation and can cause incorrect chat context. Fix by checking if the element is still selected before updating context, or by canceling previous pending updates.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/web/client/src/app/project/[id]/_components/canvas/overlay/elements/buttons/chat.tsx (1)
35-37: Localize user-facing strings (toast + aria-label).
Avoid hardcoded text per guidelines; use next-intl.Apply this diff:
- console.error('Error sending message', error); - toast.error('Failed to send message. Please try again.'); + console.error('Error sending message', error); + toast.error(t(transKeys.editor.panels.edit.tabs.chat.errors.sendFailed)); @@ - aria-label="Chat message input" + aria-label={t(transKeys.editor.panels.edit.tabs.chat.input.ariaLabel)}Also applies to: 87-88
🧹 Nitpick comments (5)
apps/web/client/src/app/project/[id]/_components/editor-bar/index.tsx (1)
96-99: Respect prefers-reduced-motion.
Offer a no/low-animation path for users who prefer reduced motion.Example:
- <motion.div + <motion.div initial={{ opacity: 0, y: 8 }} animate={{ opacity: 1, y: 0 }} exit={{ opacity: 0, y: 8 }} @@ - transition={{ - duration: 0.15, - ease: 'easeOut', - }} + transition={ + typeof window !== 'undefined' && + window.matchMedia?.('(prefers-reduced-motion: reduce)').matches + ? { duration: 0 } + : { duration: 0.15, ease: 'easeOut' } + }Also applies to: 104-106
apps/web/client/src/app/project/[id]/_components/canvas/overlay/elements/buttons/index.tsx (3)
45-47: Unify transition source; avoid conflicting inline vs class transitions.
Inlinetransition: all 0.15soverridesduration-100. Prefer one.Apply either:
- Remove inline transition:
- pointerEvents: 'auto', - transition: 'all 0.15s ease-out', + pointerEvents: 'auto',
- Or drop the class timing:
- const animationClass = - 'origin-center opacity-0 -translate-y-1 transition-all duration-100'; + const animationClass = + 'origin-center opacity-0 -translate-y-1';Also applies to: 75-76
16-17: Remove dead state used only for tracking but never read.
prevChatPositionRefandchatPositionaren’t used by rendering/logic.Apply this diff:
- const prevChatPositionRef = useRef<{ x: number; y: number } | null>(null); @@ - const chatPosition = { - x: domId - ? (document.getElementById(domId)?.getBoundingClientRect().left ?? 0) - : 0, - y: domId - ? (document.getElementById(domId)?.getBoundingClientRect().bottom ?? 0) - : 0, - }; - - useEffect(() => { - prevChatPositionRef.current = chatPosition; - }, [chatPosition.x, chatPosition.y]); + // (removed unused chatPosition tracking)Also applies to: 32-44
49-56: Escape selector tokens when querying by attribute.
Use CSS.escape to avoid selector breakage with unusual domIds.Apply this diff:
- const element = document.querySelector(`[data-element-id="${domId}"]`); + const safe = typeof CSS !== 'undefined' && CSS.escape ? CSS.escape(domId) : domId.replace(/["\\]/g, '\\$&'); + const element = document.querySelector(`[data-element-id="${safe}"]`);apps/web/client/src/components/store/editor/element/index.ts (1)
92-97: Set selected in one shot.
No need to clear then push; assign a copy to avoid external mutation.Apply this diff:
- runInAction(() => { - this.clearSelectedElements(); - for (const domEl of domEls) { - this._selected.push(domEl); - } - }); + runInAction(() => { + this.selected = domEls.slice(); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/web/client/src/app/project/[id]/_components/canvas/overlay/elements/buttons/chat.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/canvas/overlay/elements/buttons/index.tsx(2 hunks)apps/web/client/src/app/project/[id]/_components/canvas/overlay/index.tsx(1 hunks)apps/web/client/src/app/project/[id]/_components/editor-bar/index.tsx(1 hunks)apps/web/client/src/components/store/editor/chat/context.ts(1 hunks)apps/web/client/src/components/store/editor/element/index.ts(2 hunks)apps/web/client/src/components/store/editor/overlay/index.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env insteadAvoid hardcoded user-facing text; use next-intl messages/hooks
Files:
apps/web/client/src/app/project/[id]/_components/canvas/overlay/elements/buttons/chat.tsxapps/web/client/src/app/project/[id]/_components/editor-bar/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/overlay/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/overlay/elements/buttons/index.tsx
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/app/project/[id]/_components/canvas/overlay/elements/buttons/chat.tsxapps/web/client/src/app/project/[id]/_components/editor-bar/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/overlay/index.tsxapps/web/client/src/components/store/editor/chat/context.tsapps/web/client/src/app/project/[id]/_components/canvas/overlay/elements/buttons/index.tsxapps/web/client/src/components/store/editor/overlay/index.tsapps/web/client/src/components/store/editor/element/index.ts
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable identities across renders
Keep the active MobX store in a useRef and clean up asynchronously with setTimeout(() => storeRef.current?.clear(), 0)
Do not use useMemo to create MobX stores
Avoid placing MobX store instances in effect dependency arrays if it causes loops; split concerns instead
observer components must be client components; place a single client boundary at the feature entry; child observers need not repeat 'use client'
Files:
apps/web/client/src/app/project/[id]/_components/canvas/overlay/elements/buttons/chat.tsxapps/web/client/src/app/project/[id]/_components/editor-bar/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/overlay/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/overlay/elements/buttons/index.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/app/project/[id]/_components/canvas/overlay/elements/buttons/chat.tsxapps/web/client/src/app/project/[id]/_components/editor-bar/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/overlay/index.tsxapps/web/client/src/components/store/editor/chat/context.tsapps/web/client/src/app/project/[id]/_components/canvas/overlay/elements/buttons/index.tsxapps/web/client/src/components/store/editor/overlay/index.tsapps/web/client/src/components/store/editor/element/index.ts
apps/web/client/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Default to Server Components; add 'use client' only when using events, state/effects, browser APIs, or client-only libs
Files:
apps/web/client/src/app/project/[id]/_components/canvas/overlay/elements/buttons/chat.tsxapps/web/client/src/app/project/[id]/_components/editor-bar/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/overlay/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/overlay/elements/buttons/index.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/app/project/[id]/_components/canvas/overlay/elements/buttons/chat.tsxapps/web/client/src/app/project/[id]/_components/editor-bar/index.tsxapps/web/client/src/app/project/[id]/_components/canvas/overlay/index.tsxapps/web/client/src/components/store/editor/chat/context.tsapps/web/client/src/app/project/[id]/_components/canvas/overlay/elements/buttons/index.tsxapps/web/client/src/components/store/editor/overlay/index.tsapps/web/client/src/components/store/editor/element/index.ts
🧬 Code graph analysis (1)
apps/web/client/src/components/store/editor/element/index.ts (1)
apps/web/client/src/components/store/editor/overlay/utils.ts (1)
adaptRectToCanvas(43-70)
🔇 Additional comments (4)
apps/web/client/src/app/project/[id]/_components/canvas/overlay/elements/buttons/chat.tsx (2)
43-43: Animation duration reduction looks good.
Shorter transition improves perceived responsiveness without readability loss.
15-21: Ensure a single 'use client' boundary covers this observer tree.
Confirm apps/web/client/src/app/project/[id]/_components/canvas/overlay/index.tsx contains "use client" and remove any redundant "use client" from child files (e.g., apps/web/client/src/app/project/[id]/_components/canvas/overlay/elements/buttons/chat.tsx — OverlayChatInput).apps/web/client/src/components/store/editor/element/index.ts (1)
62-98: Good split: outline first, selection second.
This batching should improve feedback and reduce reaction churn.apps/web/client/src/app/project/[id]/_components/canvas/overlay/index.tsx (1)
44-45: Faster overlay fade OK — confirm client boundary at feature entryShorter duration is fine — ensure the overlay feature entry (apps/web/client/src/app/project/[id]/_components/canvas/overlay/index.tsx) declares 'use client' because it uses hooks/observer. Place a single client boundary at the feature entry; child observer components should not repeat 'use client'.
| }; | ||
|
|
||
| refresh = debounce(this.undebouncedRefresh, 100, { leading: true }); | ||
| refresh = debounce(this.undebouncedRefresh, 50, { leading: true }); |
There was a problem hiding this comment.
Debounced refresh can overlap; guard against concurrent runs and drop trailing calls.
With leading: true and an async body, a trailing invocation can race the in‑flight one, causing flicker. Add an in‑flight guard and disable trailing.
Apply this diff:
-import { debounce } from 'lodash';
+import debounce from 'lodash/debounce';
@@
export class OverlayManager {
state: OverlayState = new OverlayState();
private canvasReactionDisposer?: () => void;
+ private _refreshing = false;
@@
- undebouncedRefresh = async () => {
+ undebouncedRefresh = async () => {
+ if (this._refreshing) return;
+ this._refreshing = true;
+ try {
this.state.removeHoverRect();
@@
- // Refresh text editor position if it's active
+ // Refresh text editor position if it's active
if (this.editorEngine.text.isEditing && this.editorEngine.text.targetElement) {
@@
- };
+ } finally {
+ this._refreshing = false;
+ }
+ };
@@
- refresh = debounce(this.undebouncedRefresh, 50, { leading: true });
+ refresh = debounce(this.undebouncedRefresh, 50, { leading: true, trailing: false });Also consider wrapping the first getElementByDomId call in try/catch (to mirror the text‑editor branch) to avoid aborting the whole refresh on a single failure:
- const el: DomElement = await view.getElementByDomId(selectedElement.domId, true);
- if (!el) {
- console.error('Element not found');
- continue;
- }
+ try {
+ const el: DomElement = await view.getElementByDomId(selectedElement.domId, true);
+ if (!el) {
+ console.error('Element not found');
+ continue;
+ }
+ const adaptedRect = adaptRectToCanvas(el.rect, view);
+ newClickRects.push({ rect: adaptedRect, styles: el.styles });
+ } catch (e) {
+ console.error('Error fetching element by domId', e);
+ continue;
+ }
- const adaptedRect = adaptRectToCanvas(el.rect, view);
- newClickRects.push({ rect: adaptedRect, styles: el.styles });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| refresh = debounce(this.undebouncedRefresh, 50, { leading: true }); | |
| import debounce from 'lodash/debounce'; | |
| export class OverlayManager { | |
| state: OverlayState = new OverlayState(); | |
| private canvasReactionDisposer?: () => void; | |
| private _refreshing = false; | |
| undebouncedRefresh = async () => { | |
| if (this._refreshing) return; | |
| this._refreshing = true; | |
| try { | |
| this.state.removeHoverRect(); | |
| // ... existing refresh logic ... | |
| // Refresh text editor position if it's active | |
| if (this.editorEngine.text.isEditing && this.editorEngine.text.targetElement) { | |
| // existing text-editor positioning logic... | |
| } | |
| // (example context from diff) safely fetch element by domId | |
| try { | |
| const el: DomElement = await view.getElementByDomId(selectedElement.domId, true); | |
| if (!el) { | |
| console.error('Element not found'); | |
| continue; | |
| } | |
| const adaptedRect = adaptRectToCanvas(el.rect, view); | |
| newClickRects.push({ rect: adaptedRect, styles: el.styles }); | |
| } catch (e) { | |
| console.error('Error fetching element by domId', e); | |
| continue; | |
| } | |
| // ... remaining refresh logic ... | |
| } finally { | |
| this._refreshing = false; | |
| } | |
| }; | |
| refresh = debounce(this.undebouncedRefresh, 50, { leading: true, trailing: false }); | |
| } |
🤖 Prompt for AI Agents
In apps/web/client/src/components/store/editor/overlay/index.ts around line 82,
the debounce currently allows a trailing call to run while a leading async
refresh is still in flight and there is no in‑flight guard; change the debounce
options to { leading: true, trailing: false } and add an in‑flight guard (e.g.
this._refreshInFlight boolean) at the top of undebouncedRefresh to drop any
attempts while true, set it to true before awaiting async work and clear it in
finally; additionally wrap the first getElementByDomId call in a try/catch (like
the text-editor branch) so a single failure won’t abort the entire refresh
process.
Major memory optimizations to prevent performance degradation: - Cancel pending requestAnimationFrame and abort in-flight async operations - Limit file reads to 5 files max with 50KB content truncation - Cap clickRects array at 20 elements to prevent unbounded growth - Prevent concurrent overlay refresh operations - Remove console.error spam that accumulates in memory - Add proper cleanup for all async operations These fixes address the performance slowdown that occurs after many rapid selections across different webviews and branches. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/client/src/components/store/editor/chat/context.ts (1)
13-13: Unused import detected.The
debounceimport from lodash is not used in this file. Consider removing it to keep imports clean.-import { debounce } from 'lodash'; import { makeAutoObservable, reaction } from 'mobx';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/web/client/src/components/store/editor/chat/context.ts(5 hunks)apps/web/client/src/components/store/editor/overlay/index.ts(4 hunks)apps/web/client/src/components/store/editor/overlay/state.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/components/store/editor/overlay/state.tsapps/web/client/src/components/store/editor/overlay/index.tsapps/web/client/src/components/store/editor/chat/context.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/components/store/editor/overlay/state.tsapps/web/client/src/components/store/editor/overlay/index.tsapps/web/client/src/components/store/editor/chat/context.ts
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/components/store/editor/overlay/state.tsapps/web/client/src/components/store/editor/overlay/index.tsapps/web/client/src/components/store/editor/chat/context.ts
🧬 Code graph analysis (2)
apps/web/client/src/components/store/editor/overlay/index.ts (1)
packages/models/src/element/element.ts (3)
RectDimensions(33-38)DomElementStyles(17-20)DomElement(11-15)
apps/web/client/src/components/store/editor/chat/context.ts (1)
packages/models/src/chat/message/context.ts (1)
MessageContext(53-59)
🔇 Additional comments (8)
apps/web/client/src/components/store/editor/overlay/state.ts (1)
65-77: LGTM! Good defensive programming with bounded array.The implementation correctly prevents unbounded memory growth by capping the click rectangles array at 20 elements, which is reasonable for UI selection scenarios.
apps/web/client/src/components/store/editor/overlay/index.ts (3)
30-43: LGTM! Robust concurrency control implementation.The implementation correctly prevents concurrent refresh operations by maintaining a pending promise and awaiting its completion before starting a new one. The cleanup in the finally block ensures proper state management.
51-53: Good defensive limit on refresh scope.Limiting to 10 elements prevents performance degradation with large selections while still covering typical use cases.
103-103: Confirm maxWait: 500ms is intended for overlay.refreshThe debounce at apps/web/client/src/components/store/editor/overlay/index.ts:103 sets
refresh = debounce(this.undebouncedRefresh, 100, { leading: true, trailing: false, maxWait: 500 });
— maxWait guarantees execution at least every 500ms even under continuous calls. refresh is invoked from multiple places (apps/web/client/src/components/store/editor/text/index.ts, action/index.ts, frame-events/index.ts) and undebouncedRefresh is called from canvas/frame/resize-handles.tsx; verify this forced periodic refresh aligns with UX/performance expectations.apps/web/client/src/components/store/editor/chat/context.ts (4)
29-62: Excellent implementation of cancellation pattern with AbortController.The implementation properly handles race conditions by:
- Cancelling pending animation frames
- Aborting in-flight async operations
- Creating fresh abort controllers for each update
- Checking abort status before committing state changes
This is a robust solution that prevents stale context from being applied.
123-137: Good memory optimization for file context.Limiting to 5 files and 50KB per file prevents excessive memory usage while still providing sufficient context. The truncation message clearly indicates when content is cut off.
181-182: LGTM! Appropriate removal of console errors for expected conditions.Removing console errors for missing OIDs, code blocks, and template nodes is appropriate since these are expected conditions during normal operation, especially during rapid selection changes.
Also applies to: 187-188, 193-194
330-341: Comprehensive cleanup implementation.The clear method properly handles all cleanup scenarios including pending updates, in-flight operations, and reaction disposers. The order of operations is correct.
| clear = () => { | ||
| this.canvasReactionDisposer?.(); | ||
| this.canvasReactionDisposer = undefined; | ||
| this.pendingRefreshPromise = undefined; | ||
| this.clearUI(); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Good cleanup, but verify the clearing order.
The cleanup properly resets the pending promise and calls clearUI(). However, ensure that clearUI() doesn't trigger any reactions that might need the disposed reaction handlers.
🏁 Script executed:
#!/bin/bash
# Check what clearUI does and if it triggers any reactions
ast-grep --pattern $'clearUI = () => {
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for 'clearUI' occurrences..."
rg -n --hidden -S '\bclearUI\b' -g '!.git' -C3 || true
echo
echo "Searching for 'canvasReactionDisposer' occurrences..."
rg -n --hidden -S '\bcanvasReactionDisposer\b' -g '!.git' -C3 || true
echo
echo "Showing the 'clear = () =>' implementation context..."
rg -n --hidden -S 'clear\s*=\s*\(\)\s*=>' -g '!.git' -C5 || trueLength of output: 595
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "PWD: $(pwd)"
echo "Git top-level: $(git rev-parse --show-toplevel 2>/dev/null || echo 'no-git')"
echo
echo "Listing top-level files:"
ls -la | sed -n '1,200p' || true
TARGET="apps/web/client/src/components/store/editor/overlay/index.ts"
if [ -f "$TARGET" ]; then
echo
echo "=== Showing $TARGET (lines 1-260) ==="
sed -n '1,260p' "$TARGET"
else
echo
echo "$TARGET not found"
fi
echo
echo "=== Searching repository for occurrences ==="
# Use git ls-files to produce a stable file list, then search that list to avoid ripgrep skipping files
if command -v rg >/dev/null 2>&1; then
git ls-files -z | xargs -0 -r rg -n --hidden -S '\bclearUI\b' -C3 || true
echo
git ls-files -z | xargs -0 -r rg -n --hidden -S '\bcanvasReactionDisposer\b' -C3 || true
echo
git ls-files -z | xargs -0 -r rg -n --hidden -S 'clear\s*=\s*\(\)\s*=>' -C5 || true
else
git ls-files -z | xargs -0 -r grep -n -P '\bclearUI\b' -n -C3 || true
echo
git ls-files -z | xargs -0 -r grep -n -P '\bcanvasReactionDisposer\b' -n -C3 || true
echo
git ls-files -z | xargs -0 -r grep -n -P 'clear\s*=\s*\(\)\s*=>' -n -C5 || true
fi
# Try ast-grep for function pattern if available
if command -v ast-grep >/dev/null 2>&1; then
echo
echo "=== ast-grep: searching for clearUI implementation pattern ==="
ast-grep --pattern $'clearUI = () => {\n $$$\n}' || true
fiLength of output: 19405
Fix race: await or cancel in‑flight refresh before clearing UI
clear() disposes the canvas reaction and sets pendingRefreshPromise = undefined before calling clearUI(); if performRefresh is currently running it can still mutate this.state after clearUI and re-populate overlays.
Location: apps/web/client/src/components/store/editor/overlay/index.ts — clear() (≈ lines 149–154) and undebouncedRefresh()/performRefresh().
- Either await pendingRefreshPromise (if present) before clearing, or add a cancellation/guard checked by performRefresh to skip state mutations when cleared.
🤖 Prompt for AI Agents
In apps/web/client/src/components/store/editor/overlay/index.ts around lines
149–154, clear() currently disposes the canvas reaction and zeroes
pendingRefreshPromise before calling clearUI(), which lets an in‑flight
performRefresh/undebouncedRefresh still mutate state and re-populate overlays;
change clear() to first capture pendingRefreshPromise, if present await it (or
set and flip a simple cancelled flag) before calling clearUI(), and update
performRefresh/undebouncedRefresh to check that flag or that
pendingRefreshPromise still matches before mutating state so any in‑flight
refresh is either awaited or early‑aborted and cannot repopulate the UI after
clear.
Addresses browser tab throttling that causes latency after switching tabs: - Add connection warm-up when tab is reactivated - Ping frame connections to wake them from throttled state - Track time between clicks to detect idle periods - Clear stale overlay state on reactivation - Force refresh of selected elements to restore instant feedback The browser throttles JavaScript and network connections in background tabs. This fix detects when the tab becomes active again and proactively warms up all connections to restore instant responsiveness. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
| type ProjectMessageContext, | ||
| } from '@onlook/models/chat'; | ||
| import type { ParsedError } from '@onlook/utility'; | ||
| import { debounce } from 'lodash'; |
There was a problem hiding this comment.
Unused import bug: The debounce import from lodash is added but never used in the code. This creates dead code and potential bundle bloat. The debouncing logic uses requestAnimationFrame instead. Remove this unused import.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| }; | ||
|
|
||
| refresh = debounce(this.undebouncedRefresh, 100, { leading: true }); | ||
| refresh = debounce(this.undebouncedRefresh, 100, { leading: true, trailing: false, maxWait: 500 }); |
There was a problem hiding this comment.
Logic error in debounce configuration: Setting trailing: false with leading: true means the debounced function will only execute on the first call and ignore subsequent calls within the debounce period. This breaks the refresh mechanism when multiple rapid updates occur. For overlay refreshes, you typically want trailing: true to ensure the final state is rendered. Fix: Remove trailing: false or set it to true.
| refresh = debounce(this.undebouncedRefresh, 100, { leading: true, trailing: false, maxWait: 500 }); | |
| refresh = debounce(this.undebouncedRefresh, 100, { leading: true, trailing: true, maxWait: 500 }); |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx (3)
131-133: Guard reconnect userId or support overloads explicitly.If
session.reconnectexpects a defineduserId, passinguser?.idmay fail whenuserhasn’t loaded yet. Prefer conditional overload.Apply this diff:
- await editorEngine.activeSandbox.session.reconnect(editorEngine.projectId, user?.id); + if (user?.id) { + await editorEngine.activeSandbox.session.reconnect(editorEngine.projectId, user.id); + } else { + // Reconnect without user context if supported; otherwise bail early. + await editorEngine.activeSandbox.session.reconnect(editorEngine.projectId); + }If the no-user overload isn’t supported, gate the warm-up until
user?.idis available.
150-152: Prefer targeted cache invalidation to reduce churn.Invalidating everything on reactivation can be costly in large projects. Narrow it to impacted queries.
Apply this diff (example based on queries used above):
- await apiUtils.invalidate(); + await Promise.allSettled([ + apiUtils.user.invalidate(), + apiUtils.userCanvas.getWithFrames.invalidate({ projectId: editorEngine.projectId }), + apiUtils.chat.conversation.getAll.invalidate({ projectId: editorEngine.projectId }), + apiUtils.project.createRequest.getPendingRequest.invalidate({ projectId: editorEngine.projectId }), + ]);
156-156: Tighten effect dependencies to avoid unnecessary reruns.Including
editorEngineandapiUtils(both objects) may retrigger this effect more than needed. Consider refs to stabilize.Apply this diff:
- }, [tabState, user?.id, editorEngine, apiUtils]); + }, [tabState, user?.id]);If ESLint complains, move
editorEngine/apiUtilsinto refs:const engineRef = useRef(editorEngine); useEffect(() => { engineRef.current = editorEngine; }, [editorEngine]); const utilsRef = useRef(apiUtils); useEffect(() => { utilsRef.current = apiUtils; }, [apiUtils]);apps/web/client/src/components/store/editor/element/index.ts (4)
12-12: Confirm first-click warmup intent.Initializing
lastClickTimeto0forces warmup on the first click (desired?). If not, initialize to current time.- private lastClickTime: number = 0; + private lastClickTime: number = (typeof performance !== 'undefined' && performance.now) ? performance.now() : Date.now();
64-70: Make timing robust across environments.
performancemay be undefined in SSR/tests; also lift magic number.- const now = performance.now(); + const now = + (typeof performance !== 'undefined' && typeof performance.now === 'function') + ? performance.now() + : Date.now(); @@ - const needsWarmup = timeSinceLastClick > 5000; + const WARMUP_AFTER_MS = 5000; + const needsWarmup = timeSinceLastClick > WARMUP_AFTER_MS;
100-103: Ping all unique frames when warming up.Only pinging the first frame may leave others cold in multi-frame selections.
- const frameData = this.editorEngine.frames.get(domEls[0].frameId); - frameData?.view?.getFrameId?.().catch(() => {}); + const frameIds = new Set(domEls.map(d => d.frameId)); + for (const frameId of frameIds) { + this.editorEngine.frames.get(frameId)?.view?.getFrameId?.().catch(() => {}); + }
107-112: Simplify selection update to a single assignment.Avoid looped pushes; set once for fewer array ops and clearer intent.
- runInAction(() => { - this.clearSelectedElements(); - for (const domEl of domEls) { - this._selected.push(domEl); - } - }); + runInAction(() => { + this.selected = domEls.slice(); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx(1 hunks)apps/web/client/src/components/store/editor/element/index.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
apps/web/client/src/app/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/app/**/*.tsx: Default to Server Components; add 'use client' when using events, state/effects, browser APIs, or client‑only libraries
Do not use process.env in client code; import env from @/env insteadAvoid hardcoded user-facing text; use next-intl messages/hooks
Files:
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsxapps/web/client/src/components/store/editor/element/index.ts
apps/web/client/src/**/*.tsx
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable references across renders
Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Avoid useMemo for creating MobX store instances
Avoid putting the MobX store instance in effect dependency arrays if it causes loops; split concerns by domain
apps/web/client/src/**/*.tsx: Create MobX store instances with useState(() => new Store()) for stable identities across renders
Keep the active MobX store in a useRef and clean up asynchronously with setTimeout(() => storeRef.current?.clear(), 0)
Do not use useMemo to create MobX stores
Avoid placing MobX store instances in effect dependency arrays if it causes loops; split concerns instead
observer components must be client components; place a single client boundary at the feature entry; child observers need not repeat 'use client'
Files:
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsxapps/web/client/src/components/store/editor/element/index.ts
apps/web/client/src/app/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Default to Server Components; add 'use client' only when using events, state/effects, browser APIs, or client-only libs
Files:
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsxapps/web/client/src/components/store/editor/element/index.ts
🧬 Code graph analysis (2)
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx (1)
apps/web/client/src/components/store/editor/element/index.ts (2)
selected(22-24)selected(26-28)
apps/web/client/src/components/store/editor/element/index.ts (2)
packages/models/src/element/element.ts (1)
DomElement(11-15)apps/web/client/src/components/store/editor/overlay/utils.ts (1)
adaptRectToCanvas(43-70)
🔇 Additional comments (3)
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx (1)
129-156: Warm-up flow on tab reactivation looks good.Good split: reconnect → wake frames → clear overlays → reselect → invalidate. This should reduce perceived latency after inactivity.
apps/web/client/src/components/store/editor/element/index.ts (2)
4-4: MobX batching import is appropriate.Adding
runInActionis the right call for minimizing reactions here.
72-96: Good split: outline first, selection later.Batching overlay rect updates separately from selection state should improve perceived responsiveness.
Use isLoading() method instead of non-existent getFrameId() for warming up frame connections after tab reactivation. This method exists on the IFrameView interface and serves the same purpose of checking connection state. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Remove unused debounce import from chat/context.ts - Fix overlay refresh debounce to use trailing: true for proper updates The trailing: false was preventing the final state from being rendered after rapid updates. With trailing: true, we ensure both immediate feedback (leading) and that the final state is properly rendered after updates settle. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Add null check for domEls[0] to satisfy TypeScript's strict mode. The element should always exist when length > 0, but explicit check prevents compilation error. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…ations Optimizations to reduce initial load overhead: - Remove heavy apiUtils.invalidate() call that was running unnecessarily - Skip warmup logic on first clicks during initialization - Add early return for empty selections in chat context - Reduce effect dependencies to prevent unnecessary re-runs - Add 100ms delay to reaction to batch rapid selection changes - Only perform frame warmup on actual tab reactivation, not initial load These changes ensure the app loads quickly while maintaining the benefits of the responsiveness optimizations during active use. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Kitenite
left a comment
Kitenite
left a comment
There was a problem hiding this comment.
A lot of these are hacks, i can't merge this.
- Add element caching in overlay manager to reduce iframe calls - Implement aggressive memory cleanup for template nodes (2000 limit) - Throttle console errors and reduce reaction frequencies - Optimize frame event handling to skip unnecessary refreshes - Defer chat context updates with longer delays - Reduce max click rects from 20 to 10 for better performance - Add batching for overlay refresh operations These changes address performance degradation after ~30-40 clicks on dense, animation-heavy pages by minimizing expensive async operations and aggressively managing memory usage. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
I'll probably trash this soon – it's not getting what I need done. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/web/client/src/components/store/editor/overlay/index.ts (1)
183-189: Fix: clear() can race with an in‑flight refresh and repopulate UI.If performRefresh is running, clear() resets state but the refresh may still add click rects afterwards. Gate commits with an epoch token and bump it on clear.
export class OverlayManager { state: OverlayState = new OverlayState(); private canvasReactionDisposer?: () => void; private pendingRefreshPromise?: Promise<void>; private elementCache = new Map<string, { element: DomElement; timestamp: number }>(); private CACHE_TTL = 500; // Cache element data for 500ms + private _refreshEpoch = 0; @@ - private performRefresh = async () => { + private performRefresh = async () => { + const epoch = this._refreshEpoch; this.state.removeHoverRect(); @@ - this.state.removeClickRects(); - for (const clickRect of newClickRects) { - this.state.addClickRect(clickRect.rect, clickRect.styles); - } + // Bail if cleared during fetch phase + if (epoch !== this._refreshEpoch) return; + this.state.removeClickRects(); + for (const clickRect of newClickRects) { + this.state.addClickRect(clickRect.rect, clickRect.styles); + } @@ - if (this.editorEngine.text.isEditing && this.editorEngine.text.targetElement) { + if (epoch === this._refreshEpoch && this.editorEngine.text.isEditing && this.editorEngine.text.targetElement) { const targetElement = this.editorEngine.text.targetElement; @@ - const adaptedRect = adaptRectToCanvas(el.rect, frameData.view); - this.state.updateTextEditor(adaptedRect, { + // Re-check before committing + if (epoch !== this._refreshEpoch) return; + const adaptedRect = adaptRectToCanvas(el.rect, frameData.view); + this.state.updateTextEditor(adaptedRect, { styles: el.styles?.computed }); @@ - clear = () => { + clear = () => { this.canvasReactionDisposer?.(); this.canvasReactionDisposer = undefined; - this.pendingRefreshPromise = undefined; + // Invalidate any in-flight refresh safely + this._refreshEpoch++; + this.pendingRefreshPromise = undefined; this.elementCache.clear(); this.clearUI(); };
🧹 Nitpick comments (11)
apps/web/client/src/components/store/editor/chat/context.ts (1)
31-70: Trim perceived latency and keep work off the frame.Current combo (reaction delay 150ms + setTimeout 300ms) can add ~450ms before context appears. Consider reducing the timer and pushing the async work behind rAF to keep the click frame free while tightening responsiveness.
Apply:
- { delay: 150 } // Increase delay to further batch rapid changes + { delay: 75 } // Smaller batching without noticeable lag- this.pendingContextUpdate = window.setTimeout(() => { - if (!signal.aborted) { - this.getChatContext().then((context) => { - if (!signal.aborted) { - this.context = context; - } - }).catch((error) => { - if (!signal.aborted) { - console.error('Error updating chat context:', error); - } - }); - } - this.pendingContextUpdate = undefined; - }, 300); // Delay context update by 300ms to prioritize UI + this.pendingContextUpdate = window.setTimeout(() => { + if (signal.aborted) { this.pendingContextUpdate = undefined; return; } + requestAnimationFrame(() => { + if (signal.aborted) return; + this.getChatContext() + .then((context) => { if (!signal.aborted) this.context = context; }) + .catch((error) => { if (!signal.aborted) console.error('Error updating chat context:', error); }) + .finally(() => { this.pendingContextUpdate = undefined; }); + }); + }, 150);apps/web/client/src/components/store/editor/element/index.ts (4)
4-5: Use a smaller lodash import for better treeshaking.Prefer the method import to avoid pulling most of lodash in web bundles.
-import { throttle } from 'lodash'; +import throttle from 'lodash/throttle';
10-14: Avoidany[]per repo guidelines.Change variadic args to
unknown[].-const throttledError = throttle((message: string, ...args: any[]) => { +const throttledError = throttle((message: string, ...args: unknown[]) => { console.error(message, ...args); }, 1000);
70-111: Phase‑1 batching is correct; consider reducing DOM reads in the loop.adaptRectToCanvas queries computed style/DOM matrix each call. For multi‑select, precompute per‑frame transform/offset once and reuse for all domEls of that frame.
Would you like a follow‑up patch adding a small per‑frame transform cache in this loop?
70-136: Optional: fire a trailing overlay refresh.If no other path reliably refreshes rectangles post‑click, add a debounced call.
runInAction(() => { this.clearSelectedElements(); for (const domEl of domEls) { this._selected.push(domEl); } }); + // Low-priority refinement; immediate outline already drawn above. + void this.editorEngine.overlay.refresh();apps/web/client/src/components/store/editor/frame-events/index.ts (2)
43-47: Debounce knobs look reasonable; prefer method import.Use the smaller lodash import to keep bundles lean.
-import { debounce } from 'lodash'; +import debounce from 'lodash/debounce';
30-37: Await the actual refresh when responding to mutations.awaiting the debounced function doesn’t guarantee the refresh completed. Call the undebounced path here.
- await this.editorEngine.refreshLayers(); - await this.editorEngine.overlay.refresh(); + await this.editorEngine.refreshLayers(); + await this.editorEngine.overlay.undebouncedRefresh(); await this.validateAndCleanSelections();apps/web/client/src/components/store/editor/overlay/index.ts (2)
71-101: Record fetch time at write, not at batch start.Using a single
nowcan overextend TTLs during slow fetches. Stamp when caching each element.- const promise = view.getElementByDomId(selectedElement.domId, true) + const promise = view.getElementByDomId(selectedElement.domId, true) .then((el: DomElement) => { if (el) { - // Cache the element data - this.elementCache.set(cacheKey, { element: el, timestamp: now }); + // Cache with current time at write + this.elementCache.set(cacheKey, { element: el, timestamp: Date.now() }); const adaptedRect = adaptRectToCanvas(el.rect, view); newClickRects.push({ rect: adaptedRect, styles: el.styles }); } })
137-137: Import lodash method directly.Smaller import for the web bundle.
-import { debounce } from 'lodash'; +import debounce from 'lodash/debounce';apps/web/client/src/components/store/editor/template-nodes/index.ts (2)
29-29: Make this a true constant and clarify intent.Use readonly and add an explicit active-branch quota constant; the current comment says “Lower limit” but this is an upper cap.
- private MAX_TEMPLATE_NODES = 2000; // Lower limit for better performance on dense pages + private readonly MAX_TEMPLATE_NODES = 2000; // Upper cap on in-memory template nodes + private readonly ACTIVE_BRANCH_QUOTA = 0.9; // Keep up to 90% for active branch
135-144: Batch map mutations with runInAction to avoid extra reactions; prune right after insert is fine once MRU policy is fixed.processFileForMapping is async and has prior awaits; without an action here, each .set() can trigger observers.
- // Store template nodes with size limit check - templateNodeMap.forEach((node, oid) => { - this.templateNodes.set(oid, node); - }); - - // If we've exceeded the max nodes, clean up old nodes - if (this.templateNodes.size > this.MAX_TEMPLATE_NODES) { - this.pruneOldTemplateNodes(); - } + // Store template nodes with size limit check (batch to avoid multiple reactions) + runInAction(() => { + templateNodeMap.forEach((node, oid) => { + this.templateNodes.set(oid, node); + }); + if (this.templateNodes.size > this.MAX_TEMPLATE_NODES) { + this.pruneOldTemplateNodes(); + } + });Add the import (outside this hunk):
import { makeAutoObservable, runInAction } from 'mobx';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx(1 hunks)apps/web/client/src/components/store/editor/chat/context.ts(5 hunks)apps/web/client/src/components/store/editor/element/index.ts(7 hunks)apps/web/client/src/components/store/editor/frame-events/index.ts(1 hunks)apps/web/client/src/components/store/editor/overlay/index.ts(4 hunks)apps/web/client/src/components/store/editor/overlay/state.ts(1 hunks)apps/web/client/src/components/store/editor/template-nodes/index.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/client/src/components/store/editor/overlay/state.ts
- apps/web/client/src/app/project/[id]/_hooks/use-start-project.tsx
🧰 Additional context used
📓 Path-based instructions (3)
apps/web/client/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
apps/web/client/src/**/*.{ts,tsx}: Use path aliases @/* and ~/* for imports that map to apps/web/client/src/*
Avoid hardcoded user-facing text; use next-intl messages/hooks insteadUse path aliases @/* and ~/* for imports mapping to src/*
Files:
apps/web/client/src/components/store/editor/frame-events/index.tsapps/web/client/src/components/store/editor/chat/context.tsapps/web/client/src/components/store/editor/template-nodes/index.tsapps/web/client/src/components/store/editor/overlay/index.tsapps/web/client/src/components/store/editor/element/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not use the any type unless necessary
Files:
apps/web/client/src/components/store/editor/frame-events/index.tsapps/web/client/src/components/store/editor/chat/context.tsapps/web/client/src/components/store/editor/template-nodes/index.tsapps/web/client/src/components/store/editor/overlay/index.tsapps/web/client/src/components/store/editor/element/index.ts
{apps,packages}/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Avoid using the any type unless absolutely necessary
Files:
apps/web/client/src/components/store/editor/frame-events/index.tsapps/web/client/src/components/store/editor/chat/context.tsapps/web/client/src/components/store/editor/template-nodes/index.tsapps/web/client/src/components/store/editor/overlay/index.tsapps/web/client/src/components/store/editor/element/index.ts
🧠 Learnings (3)
📚 Learning: 2025-09-14T01:44:21.209Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: AGENTS.md:0-0
Timestamp: 2025-09-14T01:44:21.209Z
Learning: Applies to apps/web/client/src/**/*.tsx : Keep the active MobX store in a useRef and perform async cleanup with setTimeout(() => storeRef.current?.clear(), 0) to avoid route-change races
Applied to files:
apps/web/client/src/components/store/editor/overlay/index.ts
📚 Learning: 2025-09-16T19:22:52.461Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T19:22:52.461Z
Learning: Applies to apps/web/client/src/**/*.tsx : Keep the active MobX store in a useRef and clean up asynchronously with setTimeout(() => storeRef.current?.clear(), 0)
Applied to files:
apps/web/client/src/components/store/editor/overlay/index.ts
📚 Learning: 2025-09-16T19:22:52.461Z
Learnt from: CR
PR: onlook-dev/onlook#0
File: CLAUDE.md:0-0
Timestamp: 2025-09-16T19:22:52.461Z
Learning: Applies to apps/web/client/src/**/*.tsx : Avoid placing MobX store instances in effect dependency arrays if it causes loops; split concerns instead
Applied to files:
apps/web/client/src/components/store/editor/element/index.ts
🧬 Code graph analysis (3)
apps/web/client/src/components/store/editor/template-nodes/index.ts (1)
packages/models/src/element/templateNode.ts (1)
TemplateNode(3-11)
apps/web/client/src/components/store/editor/overlay/index.ts (2)
packages/models/src/element/element.ts (3)
DomElement(11-15)RectDimensions(33-38)DomElementStyles(17-20)apps/web/client/src/components/store/editor/overlay/utils.ts (1)
adaptRectToCanvas(43-70)
apps/web/client/src/components/store/editor/element/index.ts (2)
packages/models/src/element/element.ts (1)
DomElement(11-15)apps/web/client/src/components/store/editor/overlay/utils.ts (1)
adaptRectToCanvas(43-70)
🔇 Additional comments (14)
apps/web/client/src/components/store/editor/chat/context.ts (4)
19-21: Good: added cancellation primitives.Adding pendingContextUpdate and an AbortController is the right direction to avoid stale writes and wasted work.
129-151: Memory guardrails look good.Capping reads to 5 and truncating to ~50KB per file is a sensible safety net for large projects.
If file selection order matters, consider prioritizing by proximity to selection or recency rather than slice(0, N).
188-201: Nice: log‑spam removed on expected nulls.Silencing these noisy branches will keep the console useful during rapid interactions.
338-349: Cleanup is thorough.Pending timers are cleared and in‑flight ops aborted before disposing the reaction. Solid.
apps/web/client/src/components/store/editor/element/index.ts (4)
112-127: Warm‑up is safe but verify impact.The isLoading() poke is wrapped; just confirm it doesn’t trigger measurable work on every >5s idle click across many frames.
129-135: Two‑phase selection update LGTM.Separating outline draw from selection mutation keeps the UI snappy.
Ensure a “slow path” precision refresh still happens eventually. If nothing else triggers overlay.refresh after selection, consider scheduling a low‑prio refresh here.
147-148: Good: throttled error plus user toast.Prevents console flooding while keeping UX feedback.
161-162: Consistent throttled logging.Nice consolidation of “No frame view found” and delete errors behind the throttle.
Also applies to: 195-196, 211-212
apps/web/client/src/components/store/editor/frame-events/index.ts (1)
32-37: Lazy refresh condition makes sense.Only refreshing with a selection or active editing should cut redundant work.
apps/web/client/src/components/store/editor/overlay/index.ts (5)
11-13: Good additions: in‑flight guard + short‑lived element cache.These are the right primitives for dropping overlapping refreshes and cutting repeated DOM fetch.
35-48: Concurrency guard is solid; return the same promise to callers.Nice pattern to coalesce refreshers.
50-60: Cache eviction is fine; timestamp granularity is adequate.
64-67: Refresh scope cap is sensible.Limiting to 10 elements prevents worst‑case thrash.
120-133: Quiet error handling is fine here.Suppressing noisy errors during text‑editor refresh reduces flicker and log clutter.
| private pruneOldTemplateNodes(): void { | ||
| // Keep only nodes from the active branch and limit the total size | ||
| const activeBranchId = this.getActiveBranchId(); | ||
| const activeBranchNodes: [string, TemplateNode][] = []; | ||
| const otherBranchNodes: [string, TemplateNode][] = []; | ||
|
|
||
| for (const [oid, node] of this.templateNodes) { | ||
| if (node.branchId === activeBranchId) { | ||
| activeBranchNodes.push([oid, node]); | ||
| } else { | ||
| otherBranchNodes.push([oid, node]); | ||
| } | ||
| } | ||
|
|
||
| // Clear the map and rebuild with limited nodes | ||
| this.templateNodes.clear(); | ||
|
|
||
| // Keep all active branch nodes up to limit | ||
| const nodesToKeep = Math.min(activeBranchNodes.length, Math.floor(this.MAX_TEMPLATE_NODES * 0.9)); | ||
| for (let i = 0; i < nodesToKeep; i++) { | ||
| const entry = activeBranchNodes[i]; | ||
| if (entry) { | ||
| const [oid, node] = entry; | ||
| this.templateNodes.set(oid, node); | ||
| } | ||
| } | ||
|
|
||
| // Fill remaining space with other branch nodes if any | ||
| const remainingSpace = this.MAX_TEMPLATE_NODES - this.templateNodes.size; | ||
| const otherNodesToKeep = Math.min(otherBranchNodes.length, remainingSpace); | ||
| for (let i = 0; i < otherNodesToKeep; i++) { | ||
| const entry = otherBranchNodes[i]; | ||
| if (entry) { | ||
| const [oid, node] = entry; | ||
| this.templateNodes.set(oid, node); | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Pruning keeps the oldest entries; switch to MRU so fresh nodes aren’t evicted.
Map iteration is insertion-ordered. The current loops take from the start, retaining oldest nodes and discarding newest—the opposite of “prune old nodes,” and can evict nodes just added in the same call.
Apply MRU policy and rebuild in one step; also use the quota constant:
- // Clear the map and rebuild with limited nodes
- this.templateNodes.clear();
-
- // Keep all active branch nodes up to limit
- const nodesToKeep = Math.min(activeBranchNodes.length, Math.floor(this.MAX_TEMPLATE_NODES * 0.9));
- for (let i = 0; i < nodesToKeep; i++) {
- const entry = activeBranchNodes[i];
- if (entry) {
- const [oid, node] = entry;
- this.templateNodes.set(oid, node);
- }
- }
-
- // Fill remaining space with other branch nodes if any
- const remainingSpace = this.MAX_TEMPLATE_NODES - this.templateNodes.size;
- const otherNodesToKeep = Math.min(otherBranchNodes.length, remainingSpace);
- for (let i = 0; i < otherNodesToKeep; i++) {
- const entry = otherBranchNodes[i];
- if (entry) {
- const [oid, node] = entry;
- this.templateNodes.set(oid, node);
- }
- }
+ // Keep most-recently-inserted nodes: slice from the end (Map preserves insertion order)
+ const activeQuota = Math.floor(this.MAX_TEMPLATE_NODES * this.ACTIVE_BRANCH_QUOTA);
+ const activeSlice = activeBranchNodes.slice(-activeQuota);
+ const remaining = this.MAX_TEMPLATE_NODES - activeSlice.length;
+ const otherSlice = remaining > 0 ? otherBranchNodes.slice(-remaining) : [];
+ // Rebuild in one step to reduce observer churn
+ this.templateNodes = new Map([...activeSlice, ...otherSlice]);Optional follow-ups:
- If getActiveBranchId() can transiently be unset during branch switches, pass the caller’s branchId into pruneOldTemplateNodes as a param and fall back to “no branch preference” if missing.
Verify no consumers assume long-lived OIDs that pruning might evict:
🏁 Script executed:
#!/bin/bash
# Scan for persisted/stable OID assumptions that pruning could break
rg -nP -C2 '\boid\b' --type=ts --type=tsx
rg -nP -C2 '(localStorage|sessionStorage|indexedDB|processCache).*oid'
rg -nP -C2 '\bgetTemplateNode\s*\('Length of output: 6636
Switch pruning to MRU — keep newest template nodes (avoid evicting freshly-inserted nodes).
Map iteration is insertion-ordered; the current logic retains the oldest entries and will evict newly-added nodes. getTemplateNode is used by runtime code and tests (e.g. ide/index.ts, code-tab/index.tsx, chat/context.ts, ast/index.ts, code/index.ts), so this is high-risk. Apply MRU eviction and rebuild the map in one step:
- // Clear the map and rebuild with limited nodes
- this.templateNodes.clear();
-
- // Keep all active branch nodes up to limit
- const nodesToKeep = Math.min(activeBranchNodes.length, Math.floor(this.MAX_TEMPLATE_NODES * 0.9));
- for (let i = 0; i < nodesToKeep; i++) {
- const entry = activeBranchNodes[i];
- if (entry) {
- const [oid, node] = entry;
- this.templateNodes.set(oid, node);
- }
- }
-
- // Fill remaining space with other branch nodes if any
- const remainingSpace = this.MAX_TEMPLATE_NODES - this.templateNodes.size;
- const otherNodesToKeep = Math.min(otherBranchNodes.length, remainingSpace);
- for (let i = 0; i < otherNodesToKeep; i++) {
- const entry = otherBranchNodes[i];
- if (entry) {
- const [oid, node] = entry;
- this.templateNodes.set(oid, node);
- }
- }
+ // Keep most-recently-inserted nodes: slice from the end (Map preserves insertion order)
+ const activeQuota = Math.floor(this.MAX_TEMPLATE_NODES * this.ACTIVE_BRANCH_QUOTA);
+ const activeSlice = activeBranchNodes.slice(-activeQuota);
+ const remaining = this.MAX_TEMPLATE_NODES - activeSlice.length;
+ const otherSlice = remaining > 0 ? otherBranchNodes.slice(-remaining) : [];
+ // Rebuild in one step to reduce observer churn
+ this.templateNodes = new Map([...activeSlice, ...otherSlice]);Optional: accept caller-provided branchId if getActiveBranchId can be transient.
Committable suggestion skipped: line range outside the PR's diff.
Summary
Problem
Users experienced a noticeable delay between clicking an element and seeing the red selection outline appear. This was especially painful for large projects with many branches, where multiple UI components (minichat, AI context pills, styles toolbar) would render simultaneously, blocking the main thread.
Solution
Implemented a "show first, compute later" approach:
Test Plan
🤖 Generated with Claude Code
Important
Optimize UI responsiveness by prioritizing element selection rendering, deferring heavy operations, and reducing animation durations.
requestAnimationFrameto prevent UI blocking.chat.tsx,index.tsx, andoverlay/index.tsx.frame-events/index.tsandoverlay/index.tsfor faster overlay refresh (100ms → 50ms).runInActioninelement/index.tsto prevent multiple re-renders.overlay/index.tsto limit element refreshes and improve performance.element/index.tsto separate outline rendering from selection updates.element/index.tsto reduce log spam.chat/context.tsto prevent memory issues.use-start-project.tsxto warm up connections on tab reactivation.template-nodes/index.tsto maintain performance.This description was created by
for ebd8007. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
UI/Style
Performance
Bug Fixes / Stability
Refactor