[use-effect] Fix reset-or-adjust-state Effect in callout heading picker - #152
Draft
github-actions[bot] wants to merge 1 commit into
Draft
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
Effect location and classification
Headinginpackages/editor/src/widgets/callout.tsx(callout type picker for a callout node's heading chrome), around line 231 (pre-fix):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
disabledbecomes true, forcechoosingto false" — a pure state adjustment derivable during render. Using an Effect for this means:choosing→ effect fires → re-render), causing the picker to visibly stay open for one frame after becoming disabled.Selected refactor
Replaced the Effect with the React-documented "adjust state during render" pattern: track the previous
disabledvalue in a ref, and if it changed on this render, synchronously callsetChoosing(false)(only when actuallychoosing) before the render commits. This eliminates the extra effect round trip and the render, and doesn't touch theuseEffectimport (which is still used by two genuinely external-sync effects for editor listener registration lower in the same file).Verification
bunwas unavailable in this sandbox (network-restricted install), sobun run types/bun testcould not be run directly. As a substitute, I ran the bundledtsgotype checker directly againstpackages/editor/tsconfig.json:Result: two pre-existing, unrelated module-resolution errors (missing
type-festtransitively frommermaid, and missingvite/clienttypes) caused by the repo's dependencies not being installed in this sandbox — neither referencescallout.tsxor the changed code path. No new errors were introduced by this change; the edited function's types are unchanged (same props/state shapes, sameuseRef/useStateusage 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.comTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.