Skip to content

[use-effect] Fix reset-or-adjust-state Effect in callout heading picker - #152

Draft
github-actions[bot] wants to merge 1 commit into
mainfrom
automation/use-effect/callout-disabled-picker-8cf9e49ba5c09471
Draft

[use-effect] Fix reset-or-adjust-state Effect in callout heading picker#152
github-actions[bot] wants to merge 1 commit into
mainfrom
automation/use-effect/callout-disabled-picker-8cf9e49ba5c09471

Conversation

@github-actions

@github-actions github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Effect location and classification

Heading in packages/editor/src/widgets/callout.tsx (callout type picker for a callout node's heading chrome), around line 231 (pre-fix):

useEffect(() => {
	if (disabled) setChoosing(false);
}, [disabled]);

Classification: reset-or-adjust-state. This Effect does not synchronize with any external system (DOM, network, subscription, storage, widget). It only adjusts one piece of local component state (choosing, whether the type-picker popover is open) in response to a change in a prop (disabled).

Why the old Effect was incorrect

Per the synchronization-boundary standard, an Effect should exist only for external synchronization. Here the goal is purely "when disabled becomes true, force choosing to false" — a pure state adjustment derivable during render. Using an Effect for this means:

  • An extra render happens after the state update (mount → render with stale choosing → effect fires → re-render), causing the picker to visibly stay open for one frame after becoming disabled.
  • It doesn't tolerate Strict Mode economically and adds unnecessary effect scheduling for something that's just a render-time invariant.

Selected refactor

Replaced the Effect with the React-documented "adjust state during render" pattern: track the previous disabled value in a ref, and if it changed on this render, synchronously call setChoosing(false) (only when actually choosing) before the render commits. This eliminates the extra effect round trip and the render, and doesn't touch the useEffect import (which is still used by two genuinely external-sync effects for editor listener registration lower in the same file).

// Becoming disabled must close the picker immediately, not on the next
// render pass — derive it during render instead of adjusting state in an
// Effect after the fact.
let disabledForRender = useRef(disabled);
if (disabledForRender.current !== disabled) {
	disabledForRender.current = disabled;
	if (disabled && choosing) setChoosing(false);
}

Verification

bun was unavailable in this sandbox (network-restricted install), so bun run types / bun test could not be run directly. As a substitute, I ran the bundled tsgo type checker directly against packages/editor/tsconfig.json:

node node_modules/`@typescript/native-preview`/bin/tsgo.js --noEmit -p packages/editor/tsconfig.json

Result: two pre-existing, unrelated module-resolution errors (missing type-fest transitively from mermaid, and missing vite/client types) caused by the repo's dependencies not being installed in this sandbox — neither references callout.tsx or the changed code path. No new errors were introduced by this change; the edited function's types are unchanged (same props/state shapes, same useRef/useState usage patterns already used elsewhere in the file).

Scope

Single, independent fix. No dependency manifests, lockfiles, workflow files, or agent instructions were touched.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • releaseassets.githubusercontent.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "releaseassets.githubusercontent.com"

See Network Configuration for more information.

Generated by Weekly React Effect review · auto · 61 AIC · ⌖ 3.04 AIC · ⊞ 8.8K ·

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants