Clear the 13 ui-components lint findings, with tests that catch what the obvious fixes break - #98
Merged
Merged
Conversation
Copies the setup ui-react got in #92: vitest, jsdom, and globals on so @testing-library/react can register its own cleanup. Turbo needs no change because its test task already declares dependsOn ["^build"]. test-support.tsx carries the scaffolding the component tests share. It gives you two levers. withProvider drives the real AuthProvider, AuthManager and AuthClient stack over an injected fetch and storage, which is how a test proves a component works against the code that ships. stubAuth hands back an AuthContextValue the test owns, for the cases that need the session to change while the component stays mounted. AuthContext is exported from ui-react for exactly that. routedFetch dispatches on "METHOD /path" and 404s anything unrouted, so a missing route shows up as a failed assertion instead of a hung test. It serves /v1/me by default, since AuthManager.initialize fetches the profile before it will report a session at all.
DeviceList, SessionList and PasskeyList all called a useCallback that opened with setIsLoading(true) from inside an effect, which is what react-hooks/set-state-in-effect reports. The cheap way out is to move the loading flag past the await so nothing is set synchronously. It passes lint, and it is the bug #94 shipped: isLoading starts true, so the mount case still looks right, and every later load has no spinner at all. Change your session token and you sit looking at the previous account's devices with nothing saying the list is being refetched. So loading is derived here rather than stored. The list is loading whenever what is on screen does not belong to the token it is now being asked about. That is true on the first render and true again the instant the token changes, and no effect has to set it. The refetch a handler triggers keeps its own flag, because a handler may set state synchronously. PasskeyList is worse than the other two under the cheap fix, not better. Its no-token branch settles loading to false, so the pre-hydration pass turns the spinner off before the token ever arrives and it never comes back. Deriving loading also lets that branch settle honestly instead of spinning forever when you are signed out. Each test was checked against a deliberately broken version. The mount test passes under the trap in all three, which is the point: only the token-changes test catches it. Removing the useCallback from PasskeyList also clears the preserve-manual-memoization finding on the same file.
useSubPath was duplicated byte for byte in sign-in.tsx and sign-up.tsx, and useCodeFromURL in device-authorization-form.tsx is the same shape over the query string. All three mirrored window.location into state and resynced it from an effect, which is the set-state-in-effect finding on each. The trap here is not the one the list components had. Each effect opens by re-deriving the value the useState initializer already computed, so that line reads as dead code and is the obvious one to delete. Delete it and every mount test still passes. What breaks is the resync when basePath changes, which is what happens when you mount SignIn and SignUp against the same URL. useSyncExternalStore has no such line to lose. A new basePath is a new snapshot function and React re-reads it, and the third argument reports undefined during SSR the way the old window guard did. useSubPath is now one shared module instead of two copies, so it is fixed and tested once. sign-in.test.tsx covers the swap through the component, since the screen you get is picked off the sub-path.
Two places mirrored a prop into state from an effect. DeviceAuthorizationForm copies initialCode, or the code it read from the URL, into the OTP value. SignUpForm seeds the configured defaults for its dynamic fields. Both now use React's documented pattern for adjusting state when a prop changes, comparing against the last value applied, so nothing is set inside an effect. Worth reading before you touch the device form again: doing what exhaustive-deps asks is the wrong move. It wants code and isSubmitting added to the dep list. Add them and every keystroke re-runs the effect, sees the typed value differ from the code in the URL, and overwrites what you just typed. The suppressed dep list was hiding a real reason. SignUpForm's risk runs the other way. Seeding it only on a change compares equal on the mount pass and the defaults never appear at all, so the initial values come from the useState initializer instead. prev still wins the spread, since a default may only fill a field you have not set. Dropping the `as any` on completeDeviceAuthorization goes here too. The shipped AuthClient in ui-core extends the generated one and types that method properly, so the cast was never buying anything.
This one started as the last no-explicit-any finding and turned out to be a
live bug. handleSocialLogin called startOAuth with an object:
client.startOAuth(providerId, { redirect_url: window.location.href })
but startOAuth takes frontend_url and redirect_url positionally and puts them
in the query string. The object landed in frontend_url, the generated client
ran String() over it, and the request went out as
frontend_url=%5Bobject+Object%5D with no redirect_url at all. So the place
you started from was never sent, and after signing in you did not come back
to it.
It went unnoticed because frontend_url is validated against the allowlist in
plugins/social/plugin.go, "[object Object]" fails that check, and the backend
falls back to a trusted Origin. The flow completes. You just land somewhere
else.
The any and the bug are the same problem. No honest type accepts both an
object at the call site and the real (provider, string?, string?) method,
which is why any was there. Typing it properly forces the call to be right.
frontend_url is left unset on purpose so the backend keeps using the origin
it already trusts, rather than a value this function would be asserting.
The waitlist form's cast is narrowed to a named shape instead. There is no
waitlist method on the generated client and no public accessor for its base
URL, so it still reaches for the field, the same way ui-core's own client.ts
does for /v1/client-config. Worth a getter on AuthClient at some point.
The js job installed, built and typechecked, so pnpm lint had been failing in ui-components for long enough to collect 13 findings and nothing said a word. Turbo stopped at ui-react first, which is why #94 never reached the rest. Tests go in for the same reason. A harness CI never runs is only slightly more visible than a lint CI never runs. Both pass across all five packages today. ui-core still reports nine no-explicit-any warnings and eslint exits 0 on warnings, so the step is green without them being fixed.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL flagged extractSubPath as js/polynomial-redos, high. The trailing-slash trim used /\/+$/, and basePath is the caller's `path` prop rather than anything this module controls. The bad input is a run of slashes that does not end the string. The engine consumes the run from every start position, fails the $, and backtracks the whole way before shifting along one character. Measured on the old version: 46ms at 10k slashes, 1.0s at 50k, 4.2s at 100k, 16.8s at 200k. Clean quadratic. The scan that replaces it does 200k in 0.11ms. The leading-slash trim was never the problem, since /^\/+/ is anchored and only ever tries one start position, but it goes the same way for consistency. This regex is older than the harness. It was duplicated byte for byte in sign-in.tsx and sign-up.tsx and CodeQL only saw it once it moved into a file of its own. Both copies are gone now, so fixing it here fixes it everywhere. The new test asserts elapsed time rather than waiting for the runner to time out, so a regression says what it is instead of just hanging.
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
2 participants
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.
Closes #97.
The 13 findings are gone, the package has a test harness and 33 tests, and the
js CI job now runs lint and tests for the ui workspace so this cannot go quiet
again.
The issue's read of the findings was wrong on five of the eight
Worth knowing before you review, because it changes what the tests are aimed
at. The issue describes all eight
set-state-in-effectsites as networkcomponents with their own loading and error state, and flags
sign-in.tsxandsign-up.tsxas the highest risk. They are three different shapes, and onlythree touch the network.
The three list loaders.
device-list.tsx,session-list.tsx,passkey-list.tsx. Each calls auseCallbackfrom an effect, and thatcallback opens with
setIsLoading(true). This is theuseOrganizationsshapefrom #94 exactly, with the same trap.
Three URL readers.
sign-in.tsx:212andsign-up.tsx:128are bothuseSubPath, byte for byte identical.device-authorization-form.tsx:274isuseCodeFromURL. No network, no loading flag, no error state.sign-inandsign-uphave no spinner to break.Two prop mirrors.
device-authorization-form.tsx:133copiesinitialCodeinto the OTP value, and
sign-up-form.tsx:258seeds configured fielddefaults.
Same lint rule, three unrelated hazards. One test strategy across all eight
would have been pointed at the wrong one five times.
Each family fails differently, and each test was checked against the failure
Every test here was run against a deliberately broken version of the code
before the real fix went in. Three things came out of that.
The list components behave as #94 warned. Moving
setIsLoading(true)past theawait is lint clean and passes the mount test, because
isLoadingalreadystarts
true. What it loses is every load after the first. Change your sessiontoken and the previous account's devices sit there with nothing indicating a
refetch.
passkey-listis worse than the other two, not better: its no-tokenbranch settles loading to
false, so the pre-hydration pass kills the spinnerbefore the token arrives and it never returns. So loading is derived here now,
not stored. The list is loading whenever what is on screen does not belong to
the token it is being asked about.
The URL readers have the opposite trap. Each effect opens by recomputing what
the
useStateinitializer already worked out, so the line looks dead and isthe obvious one to cut. Cut it and every mount test still passes. What breaks
is the resync when
basePathchanges.useSyncExternalStorehas no such lineto lose, and
useSubPathis one shared module now instead of two copies.The prop mirrors are where the linter's own advice is the bug.
exhaustive-depswants
codeandisSubmittingon the device form's dep list. Add them andevery keystroke re-runs the effect, sees your typing differ from the code in
the URL, and overwrites it. The suppressed dep list was hiding a real reason.
sign-up-formfails the other way round: seed only on a change and the mountpass compares equal, so the defaults never appear.
One of the
anys was hiding a live bugsocial-login.ts:42looked like a routineno-explicit-any.handleSocialLoginwas calling:
startOAuthtakesfrontend_urlandredirect_urlpositionally and puts themin the query string. The object went into
frontend_url, the generated clientran
String()over it, and the request went out asfrontend_url=%5Bobject+Object%5Dwith noredirect_urlat all. Sign in with asocial provider and you do not get returned to the page you started on.
It stayed invisible because
frontend_urlis checked against the allowlist inplugins/social/plugin.go,"[object Object]"fails that check, and thebackend falls back to a trusted Origin. The flow finishes. You just end up
somewhere else.
The
anyand the bug are the same problem. No honest type accepts both anobject at the call site and the real
(provider, string?, string?)method,which is why the
anywas there. Typing it forces the call to be right. Thisis the one behaviour change in the PR that goes past a lint cleanup, so it is
worth a second opinion.
The other two
anys are ordinary.completeDeviceAuthorizationis properlytyped on the
AuthClientthat ui-core actually ships, so that cast boughtnothing. The waitlist form still reaches for a private
baseURL, the same wayui-core's own
client.tsdoes for/v1/client-config, but through a namedshape rather than
any. A public getter onAuthClientwould be better andbelongs in its own change.
The harness
Copied from ui-react's, added in #92. Turbo needed nothing, its
testtaskalready declares
dependsOn: ["^build"].test-support.tsxgives you two ways in.withProviderdrives the realAuthProvider,AuthManagerandAuthClientover an injected fetch andstorage, so a test exercises the code that ships.
stubAuthhands back anAuthContextValuethe test owns, for the cases that need the session to changewhile the component stays mounted.
AuthContextis exported from ui-react forthat purpose.
Checking it
--forcematters. While building this, turbo served a cachedui-reactbuildfrom a worktree that no longer exists on disk, so a cached pass will report
green for a tree it never touched.
Run against a clean tree with every
dist/removed: build 5/5, typecheck 8/8,lint 5/5, test 7/7. ui-components is 33 tests across 8 files.
ui-core still reports nine
no-explicit-anywarnings. eslint exits 0 onwarnings, so the new CI step is green without them, and they are somebody
else's afternoon.