fix: Update lint package and address warnings - #389
Conversation
Signed-off-by: lornakelly <lornakelly88@gmail.com>
Signed-off-by: lornakelly <lornakelly88@gmail.com>
Signed-off-by: lornakelly <lornakelly88@gmail.com>
✅ Deploy Preview for openworkflow-editor ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
🟡 Changes recommended
It introduces render-phase state updates for locale syncing and a likely-incorrect render-count assertion, both of which can cause unstable behavior/tests under React’s rendering model.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the repository’s oxlint version and adjusts React code/tests to satisfy new lint rules and warnings introduced by the upgrade (including React hook/purity-related rules), while keeping the diagram editor behavior consistent.
Changes:
- Bump
oxlintfrom^1.77.0to^1.80.0and update the lockfile accordingly. - Refactor some React internals (refs/state synchronization) and the
useResolvedColorModehook to align with updated lint expectations. - Update diagram editor store and hook tests to match the new behavior/implementation.
File summaries
| File | Description |
|---|---|
| pnpm-workspace.yaml | Updates the workspace catalog oxlint version. |
| pnpm-lock.yaml | Regenerates lockfile entries for the new oxlint version. |
| packages/open-workflow-diagram-editor/tests/store/DiagramEditorContextProvider.test.tsx | Updates render-count assertions to use a spy instead of DOM text. |
| packages/open-workflow-diagram-editor/tests/hooks/useResolvedColorMode.test.ts | Adjusts tests to reflect the new external-store subscription behavior. |
| packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx | Changes locale syncing strategy and ref updates related to content seeding. |
| packages/open-workflow-diagram-editor/src/react-flow/hooks/useWorkflowHistory.ts | Moves ref assignments into an effect for lint compliance. |
| packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx | Moves multiple ref assignments into an effect for lint compliance. |
| packages/open-workflow-diagram-editor/src/hooks/useResolvedColorMode.ts | Refactors to useSyncExternalStore for media-query subscription correctness. |
| packages/open-workflow-diagram-editor/src/components/ui/tooltip.tsx | Adds an oxlint suppression for a setState-in-effect pattern. |
| packages/open-workflow-diagram-editor/src/components/ui/sidebar.tsx | Adds an oxlint suppression for an intentional impurity (Math.random) in useMemo. |
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (1)
packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx:110
- Typo in comment: "implemntation" (and missing space in "asthe").
// This will be addressed in the editing feature branch asthe implemntation is changing
- Files reviewed: 10/11 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
Ref synchronization moved to passive effects can cause stale reads in async post-layout/timer logic, and there are also correctness/clarity issues in updated comments.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (4)
Previously missed (3) — in code that hasn't changed since the last review.
packages/open-workflow-diagram-editor/src/react-flow/diagram/Diagram.tsx:94
- The ref-sync happens in a passive effect, but these refs are read from a zero-delay setTimeout scheduled in another effect; a rerender between scheduling and the timeout firing can leave the timeout reading stale values (selection/viewport/submitModel). Use a layout effect (or keep the render-time assignment) so refs are updated synchronously after each commit.
packages/open-workflow-diagram-editor/src/react-flow/hooks/useWorkflowHistory.ts:100 - Updating stateRef/isReadOnlyRef in a passive effect can leave callbacks reading stale values if they run before effects flush (e.g., immediately after mount/commit). A layout effect keeps render pure while ensuring these refs are current before user events and other effects run.
packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx:111 - Typo in comment text ("asthe" / "implemntation").
packages/open-workflow-diagram-editor/tests/store/DiagramEditorContextProvider.test.tsx:116
- The render-cycle comment is now inconsistent with the assertion (it says 4 cycles, but the test expects 3). Update the comment so future readers understand the expected behavior.
// 4 rendering cycles are expected:
// 1- initial render, 2- history seed useEffect,
// 3- forced by rerender
expect(renderSpy).toHaveBeenCalledTimes(3);
- Files reviewed: 10/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
1c9c67e to
5625543
Compare
There was a problem hiding this comment.
🟡 Changes recommended
It introduces a non-deterministic render path (Math.random) and includes a couple of fixable correctness/quality issues (missing effect deps and a typo) that should be addressed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
- Files reviewed: 10/11 changed files
- Comments generated: 3
- Review effort level: Lite
Signed-off-by: lornakelly <lornakelly88@gmail.com>
5625543 to
3c4c7f8
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new passive useEffect ref-sync in Diagram.tsx can race with fast async layout callbacks and lead to stale ref reads at runtime.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
Suppressed comments (1)
packages/open-workflow-diagram-editor/src/store/DiagramEditorContextProvider.tsx:110
- Typo in comment: "implemntation" → "implementation".
// This will be addressed in the editing feature branch as the implemntation is changing
- Files reviewed: 10/11 changed files
- Comments generated: 1
- Review effort level: Lite
Summary
Closes #365
Updates the oxlint package which had breaking changes . Added fixes for relevant warnings
Changes
useSyncExternalStoreinstead ofuseState+useEffectfor proper external store subscription handling (system colour scheme media query)stateRefandisReadOnlyRefassignments touseEffectDiagramEditorContextProvider.tsx:useEffectfor locale sync with direct state update during renderselectedNodeIdRefassignment touseEffectsetErrorsinuseEffect(will be addressed in editing feature branch)Useful docs for ref: