Skip to content

Conversation

@ammar-agent
Copy link
Collaborator

@ammar-agent ammar-agent commented Jan 31, 2026

Summary

This PR reduces renderer allocations when viewing large code-review diffs by removing O(lines) tooltip/icon subtrees from SelectableDiffRenderer, replacing them with a single hover-positioned “add review comment” affordance and event delegation. It also adds an opt-in make dev-fast mode to profile dev builds closer to production.

Background

A heap profile (captured Jan 18, 2026) showed DiffRenderer.tsx, Radix Tooltip, and lucide-react as major allocation sources (amplified in dev via jsxDEV + StrictMode).

Implementation

  • DiffRenderer
    • Remove per-line Radix tooltips and per-line MessageSquare icons.
    • Render one overlay button positioned over the hovered +/- indicator cell; use delegated handlers (no per-line closures).
    • Reliability: show the overlay on mouseover (not mousemove) so it appears immediately on entry.
  • DiffContainer
    • Add contentProps/contentRef hooks and make the inner grid relative so overlays can be anchored reliably.
  • Dev profiling
    • Add make dev-fast (Vite --mode fast) + VITE_MUX_FAST_DEV=1 to disable StrictMode, avoid jsxDEV, and disable React Refresh.

Validation

  • make static-check

Risks

  • Hover affordance relies on delegated DOM attributes; regressions are possible around selection/drag behavior.

Generated with mux • Model: openai:gpt-5.2 • Thinking: xhigh • Cost: $10.49

@github-actions github-actions bot added refactor enhancement New feature or functionality labels Jan 31, 2026
@ammar-agent
Copy link
Collaborator Author

I reproduced two issues you found and pushed a fix:

  • Hover affordance never showing: mousemove events can target a Text node for the +/- character, so .closest() never matched [data-diff-indicator].
  • Sticky empty vertical scroll area: a hidden-but-transformed absolutely-positioned overlay button can still affect scroll extents. The hide path now fully removes it from layout.

Patch: make hover detection work for Text node targets, and toggle the overlay button via style.display ("none"/"flex") while clearing transform/size on hide.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d33f2e6bbc

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

@ammar-agent
Copy link
Collaborator Author

Pushed another iteration to address the two regressions:

  • Hover affordance still not showing: stop relying on Tailwind data-[visible=true] variants for visibility. The overlay now toggles display/opacity/pointerEvents via inline styles, so it should show even if the data-variant CSS isn't generated.
  • Escape not cancelling: add a capture-phase window keydown listener while a selection is active to ensure Esc always cancels the in-progress review note (even if focus is not on the textarea or a global handler intercepts).

Also hardened the delegated hover target lookup to avoid instanceof Element (can be flaky across Electron realms).

@ammar-agent
Copy link
Collaborator Author

@codex review

Addressed the Makefile Windows dev-fast indentation issue and updated the DiffRenderer hover affordance logic; would appreciate a re-review.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 775091b720

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

npm x concurrently -k --raw \
"bun x nodemon --watch src --watch tsconfig.main.json --watch tsconfig.json --ext ts,tsx,json --ignore dist --ignore node_modules --exec node scripts/build-main-watch.js" \
'npx esbuild src/cli/api.ts $(ESBUILD_CLI_FLAGS) --watch' \
"VITE_MUX_FAST_DEV=1 vite --mode fast"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use Windows-safe env syntax for dev-fast

On Windows, concurrently runs commands via cmd.exe, which doesn’t understand Unix-style VAR=... prefixes. The dev-fast Windows target uses VITE_MUX_FAST_DEV=1 vite --mode fast, so the command fails (or the env var is never set), breaking dev-fast on Windows. Use set VITE_MUX_FAST_DEV=1&& vite --mode fast (or cross-env) or set the env var on the outer concurrently invocation.

Useful? React with 👍 / 👎.

Stop positioning the shared review-comment button via rect math.
Instead portal a single button into the hovered indicator cell, and keep it
in sync while scrolling via elementFromPoint().

---

_Generated with `mux` • Model: `openai:gpt-5.2` • Thinking: `xhigh` • Cost: `$15.86`_

<!-- mux-attribution: model=openai:gpt-5.2 thinking=xhigh costs=15.86 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or functionality refactor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant