githubnext/chopinPublic

Let's compose rich plans together

[use-effect] Fix react-hooks/exhaustive-deps suppression in collaboration bind Effect #222

Closedgithub-actions[bot]botclosed 1 commit intomainfromrefs/pull/222/head
  1. ## Effect and location

    `Collaboration` in `packages/editor/src/collaboration.tsx` — the primary bind Effect (`useEffect(() => { ... }, [editor])`) that creates the Yjs doc, binds Lexical to it via `createYjsBinding`, and wires up cursors, awareness, and connection lifecycle.

    ## Classification

    `external-sync`. This Effect synchronizes with a genuine external system (Yjs document, awareness protocol, browser `beforeunload`/`pagehide` events) and should only rerun when the editor identity changes — everything else inside is read at bind/unbind time.

    ## Why it was incorrect

    The effect read `options.onProvider`, `options.onBinding`, and `options.user.{name,color}` at bind time and called `options.onProvider?.(undefined)` / `options.onBinding?.(undefined)` at teardown, but declared only `[editor]` as its dependency array. Instead of reflecting that intent through the standard mechanism (a ref holding the latest value), it suppressed the lint rule outright:

    ```ts // The editor identity is what matters; options are read at bind time. // eslint-disable-next-line react-hooks/exhaustive-deps }, [editor]); ```

    Per the review guidance, a genuine external-sync Effect must have no dependency suppression — suppressing the rule hides whether the effect actually reads every reactive value it touches, and risks silently going stale if the effect body is extended later without revisiting the suppression.

    ## Fix

    Introduced `optionsRef`, updated on every render (`optionsRef.current = options`), and read `optionsRef.current` inside the effect at both bind time and in the cleanup, instead of closing over the `options` parameter directly. This preserves the exact existing behavior (options read once at bind, and again fresh at unbind) while making `editor` the effect's only true dependency — no suppression needed.

    ## Verification

    - `bun` is not available in this sandbox (no network access to install it, and no local binary), so the full `bun run ci` / `bun run types` suites could not be run here. - Ran the project's `oxlint` binary directly (`node_modules/.bin/oxlint packages/editor/src/collaboration.tsx`), which includes `react-hooks/exhaustive-deps`-equivalent correctness rules: **0 warnings, 0 errors**. - No existing unit test targets this file (it depends on a live Lexical/Yjs/DOM binding, which per the repo's testing strategy belongs in browser/system integration tests, not `bun test`). - Manually reviewed the diff to confirm behavior is unchanged: `options` is still read once at bind and once at unbind, just via `optionsRef.current` instead of a closed-over parameter.

    Please re-run `bun run types` / `bun run ci` in CI to confirm no environment-specific type errors, since I could not install `bun` in this sandbox.

    > [!WARNING] > <details> > <summary>Firewall blocked 2 domains</summary> > > The following domains were blocked by the firewall during workflow execution: > > - `pypi.org` > - `releaseassets.githubusercontent.com` > > To allow these domains, add them to the `network.allowed` list in your workflow frontmatter: > > ```yaml > network: > allowed: > - defaults > - "pypi.org" > - "releaseassets.githubusercontent.com" > ``` > > See [Network Configuration](https://github.github.com/gh-aw/reference/network/) for more information. > > </details>

    > Generated by [Weekly React Effect review](https://github.com/githubnext/chopin/actions/runs/36691027615) · auto · 158.4 AIC · ⌖ 6.4 AIC · ⊞ 8.5K · [◷](https://github.com/search?q=repo%3Agithubnext%2Fchopin+%22gh-aw-workflow-id%3A+weekly-use-effect-review%22&type=pullrequests)

    <!-- gh-aw-agentic-workflow: Weekly React Effect review, engine: copilot, model: auto, id: 36691027615, workflow_id: weekly-use-effect-review, run: https://github.com/githubnext/chopin/actions/runs/36691027615 -->

    <!-- gh-aw-workflow-id: weekly-use-effect-review --> <!-- gh-aw-workflow-call-id: githubnext/chopin/weekly-use-effect-review -->

  2. MaggieAppletonclosedlast month on GitHub

  3. MaggieAppletonreferenced this in Require evidence before the Effect review bot opens PRs #228last month on GitHub