[use-effect] Derive effective access during render instead of an Effect - #155
Draft
github-actions[bot] wants to merge 1 commit into
Draft
github-actions[bot] wants to merge 1 commit into
github-actions[bot] wants to merge 1 commit into
Conversation
The effectiveCanEdit/effectiveCanManage Effect in RoomWorkspace only recomputed a pure function of the canEdit/archivedAt/canManage props; it did not synchronize with any external system, so it caused an extra render showing stale authorization before the Effect ran. Replace it with the React 'adjust state during render when a prop changes' pattern, comparing the previous props in a ref and updating state synchronously in the render body. The latestCanEdit/latestCanManage refs (read by later socket handlers) and the state setters keep their prior update semantics. Verified with 'bun run types' (all workspaces pass) and 'bun test apps/web' (304 pass, 0 fail); 'bunx oxlint apps/web/src/room-workspace.tsx' reports 0 warnings/errors. 'bun run ci' could not run locally because the dprint plugin download is blocked in this sandbox. 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 reviewed
RoomWorkspace(apps/web/src/room-workspace.tsx, previously lines 332-338):Classification
derive-render.Why the Effect was incorrect
effectiveCanEdit/effectiveCanManagestart out equal to a pure function of thecanEdit/archivedAt/canManageprops (useState(canEdit && !archivedAt)/useState(canManage)). This Effect only re-ran that same pure computation whenever those props changed — it never talked to any external system (no subscription, no DOM, no network). Because it ran in an Effect instead of during render, a prop change was visible for one extra paint with the old authorization values before the Effect fired and corrected them. That is exactly the case the React docs call out: you don't need an Effect for adjusting state that is a pure function of props.The
roomdependency was already vestigial —RoomWorkspaceis always mounted withkey={channel.id}from its callers (document-workspace-host.tsx), soroomnever changes without remounting the whole component and resetting all state; it did not need to participate in the comparison.Fix
Replaced the Effect with the React "adjust state during render when a prop changes" pattern: track the previous
{ archivedAt, canEdit, canManage }in aref, and if any of them differ from the current render's props, update the ref and call the state setters synchronously in the render body (before any JSX is produced). This keeps the exact same values and the samelatestCanEdit/latestCanManageref side effects that latersession:*socket handlers rely on (those still update the same refs/state directly, unaffected by this change), but removes the extra render pass.Verification
bun run types— all workspaces (@chopin/webincluded) type-check cleanly.bun test apps/web— 304 pass, 0 fail (no regressions).bunx oxlint apps/web/src/room-workspace.tsx— 0 warnings, 0 errors.bun run cicould not complete in this sandbox:dprint checkfails to download its TypeScript plugin binary because outbound network access to GitHub Releases is blocked here.oxlint(run standalone above) and the token checker were not otherwise affected by this change.Only this one Effect/file was touched, per the one-violation-per-PR constraint.
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.