fix(dashboard): restore sort/filter in the state initialiser, not an effect (agent-os-14b) - #29
Merged
Merged
Conversation
…effect Dependabot's npm group PR (#22) fails frontend CI at Lint. The code it trips over is pre-existing and untouched by that PR — what changed is the linter. eslint-plugin-react-hooks moves ^7.0.1 -> ^7.1.1 in that group, and 7.1.x enforces react-hooks/set-state-in-effect: DashboardPage.tsx 77:20 error Calling setState synchronously within an effect can trigger cascading renders react-hooks/set-state-in-effect Left alone this fails every weekly npm group PR, so none of them get merged and Dependabot stops being worth having. The rule is pointing at something real rather than being noise, so this fixes the effect instead of disabling the rule. Reading localStorage in the useState initialiser gives the same result in one render instead of two: the mount effect rendered the defaults, then re-rendered with the stored values. Verified against #22's actual dependency set rather than against main's: git worktree add <tmp> pr22 && pnpm install --frozen-lockfile node ./node_modules/eslint/bin/eslint.js . before this change: 1 error, exit 1 (the error quoted above, and it was the only one in the tree) after this change: 0 errors, exit 0 Also adds the first coverage of the localStorage-restore contract, which had none. Those two tests pin the restored values the page hands to StacksTab; they do NOT distinguish this implementation from the effect-based one, and say so — StacksTab is gated behind the loading state, so it first renders after effects have already flushed and both versions look identical from the DOM. Confirmed by reverting the fix with the tests in place: they still passed. The guard against a relapse is the lint rule itself. Gates: eslint exit 0, tsc -b clean, 823 tests in 80 files (821 baseline plus the two added here), build ok. Refs: agent-os-14b Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
React Doctor found no new issues. 🎉 Reviewed by React Doctor for commit |
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.
Unblocks Dependabot's npm group PR #22, which fails frontend CI at Lint.
What is actually wrong
The code #22 trips over is pre-existing and untouched by that PR. What changed is the linter:
eslint-plugin-react-hooksmoves^7.0.1→^7.1.1in that group, and 7.1.x enforcesreact-hooks/set-state-in-effect.This is not the pnpm
minimumReleaseAgeinteraction — that was verified working on the same run (Install dependencies → success).Left alone, every weekly npm group PR carries the same failure and none get merged, which defeats the point of adding Dependabot.
Fix
The rule points at something real, so this fixes the effect rather than disabling the rule. Reading
localStoragein theuseStateinitialiser gives the same result in one render instead of two — the mount effect rendered the defaults, then re-rendered with the stored values.Verification
Checked against #22's actual dependency set, not main's, since main's eslint does not yet have the rule:
Tests
Adds the first coverage of the localStorage-restore contract, which had none.
Being straight about what they prove: these two tests pin the restored values the page hands to
StacksTab. They do not distinguish this implementation from the effect-based one, and the comment says so.StacksTabis gated behind the loading state, so it first renders after effects have already flushed and both versions look identical from the DOM — confirmed by reverting the fix with the tests in place, where they still passed. The guard against a relapse is the lint rule itself.Gates
tsc -b→ no errorsvitest --run→ 823 passed in 80 files (821 baseline + the 2 added here)pnpm build→ okCloses
agent-os-14b.