feat(mosaic): Wire up password section - #9930
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: e664a41 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (4)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: This review used your included allowance. 9 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 10 reviews per hour. 📝 WalkthroughWalkthroughAdds password policy and validation to Mosaic, including password-strength feedback and localized error handling. Adds a password section with asynchronous validation and update handling, and supplies it to the security panel through a slot. Adds a live password page and navigation entry. Updates feature tests, test fixtures, and field feedback support. Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Merge Risk: ⚪ Minimal · up to No new merge-blocking issue is established for these changes. Normal checks and confirmation of the previously reported password-validation concern remain appropriate before merging. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
Comment |
fe5b987 to
677cc0d
Compare
677cc0d to
0d3bec2
Compare
114a51b to
805bf8c
Compare
805bf8c to
9af5b5a
Compare
9af5b5a to
1cf177f
Compare
1cf177f to
8bafdd9
Compare
8bafdd9 to
d6de0b0
Compare
There was a problem hiding this comment.
Adds variant to mirror legacy:
Legacy:
Mosiac:
Ephem
left a comment
Ephem
left a comment
There was a problem hiding this comment.
Great work! I focused mostly on the wrapper+model+controller patterns, not so much the rest of it or the parity with the existing flow.
| return useUserProfilePasswordSlot(props)?.content ?? null; | ||
| } | ||
|
|
||
| export function useUserProfilePasswordSlot({ |
There was a problem hiding this comment.
I get this is a hook to get a typed UserProfilePasswordSlot to work around the fact children can't be typed, but why?
I think this is unnecessary complexity, if we truly want this level of type safety, we should rewrite all our components to be hooks instead, which feels like a different framework. 😅
| const model = useUserProfilePasswordModel(); | ||
| const m = useMessages('userProfilePasswordSection'); | ||
| if (model.status === 'loading') { | ||
| // TODO: Add a password section skeleton as the default loading fallback. |
There was a problem hiding this comment.
Note that there is a tradeoff here. If we do add a default skeleton and it turns out that instanceIsPasswordBased === false, the skeleton would disappear to nothing causing a layout shift.
If we don't have a skeleton by default, it would be a layout shift when this section pops in.
| }; | ||
| } | ||
|
|
||
| function PasswordEditor({ model }: { model: Extract<UserProfilePasswordModel, { status: 'ready' }> }) { |
There was a problem hiding this comment.
I like this approach of putting the controller in a separate component so it always only gets the 'ready' version and can skip a ton of complexity. Wont always be possible, but very nice when it is.
|
|
||
| return ( | ||
| <UserProfilePasswordSectionView | ||
| hasPassword={model.mode === 'change'} |
There was a problem hiding this comment.
I'm curious if using model directly here was a conscious decision or agent-led?
The way we've used model+controller so far has been to re-export any parts of the model we need on the controller so the view only relies on the controller. That way we don't have to reason about the interaction between model and controller when looking at the components and we have one very clear interface and source of truth. Makes refactoring things easier in the future too I think.
Plus, that's a clean story when we want to reuse the controller for swingset too. Mock the things you pass into the controller, pass the controller result to the view.
Happy for pushback if we think that is unnecessarily cumbersome on small components though, this is just how I've been thinking about it.
There was a problem hiding this comment.
This mostly overlaps with the .feature. tests and I think we should remove it. Two tests should likely be moved into the feature tests before removing:
- API error becomes a field error (form_password_incorrect on currentPassword)
- Enterprise accounts: blank name becomes undefined
| useEffect(() => { | ||
| // TODO: Discuss keeping the password hint hidden on open or showing it immediately when the field autofocuses. https://github.com/clerk/javascript/pull/9930#discussion_r4150863181 | ||
| if (!isOpen || (password === '' && !passwordLeft) || !validatePassword) { | ||
| setPasswordFeedback(undefined); | ||
| return; | ||
| } | ||
|
|
||
| let active = true; | ||
| const timeout = setTimeout(() => { | ||
| void Promise.resolve() | ||
| .then(() => validatePassword(password)) | ||
| .then( | ||
| feedback => { | ||
| if (active) { | ||
| setPasswordFeedback(feedback); | ||
| } | ||
| }, | ||
| () => { | ||
| if (active) { | ||
| setPasswordFeedback({ type: 'error', message: validationError }); | ||
| } | ||
| }, | ||
| ); | ||
| }, DEBOUNCE_MS); | ||
| return () => { | ||
| active = false; | ||
| clearTimeout(timeout); | ||
| }; | ||
| }, [isOpen, password, passwordLeft, validatePassword, validationError]); |
There was a problem hiding this comment.
@alexcarpenter This looks like a good candidate for something to support in the form abstraction?
There was a problem hiding this comment.
yeah, took a stab at it here #10065
kept it out of the form though: the hint never blocks submit, so it's not validation. It's now a reusable useDebouncedAsync hook instead of the effect. TanStack Form and RHF keep non-blocking hints out of their validators too.
This branch was successfully deployed
3 participants
Description
Wire the Mosaic password section to Clerk so users can set or change their password, with validation, API errors, pending state, and the option to sign out other sessions. Changing an existing password requires the current password. Users with an active enterprise account cannot set or change a password, matching legacy behavior. Add a Swingset live page at
/live/password.Session reverification UI is deferred. Requests that require reverification surface the API error.
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change