[Remove Vuetify from Studio] 'Activation failed' page#6026
[Remove Vuetify from Studio] 'Activation failed' page#6026LightCreator1007 wants to merge 4 commits into
Conversation
|
👋 Hi @LightCreator1007, thanks for contributing! For the review process to begin, please verify that the following is satisfied:
Also check that issue requirements are satisfied & you ran Pull requests that don't follow the guidelines will be closed. Reviewer assignment can take up to 2 weeks. |
|
📢✨ Before we assign a reviewer, we'll turn on |
rtibblesbot
left a comment
There was a problem hiding this comment.
Faithful Vuetify removal, closely mirroring the merged Create.vue pattern. No out-of-scope files touched. CI passing. Manual QA did not complete in this pipeline run, so the UI itself is unverified — flagging as COMMENT rather than APPROVE on that basis.
Findings:
- suggestion: local
fieldRequiredMessage/emailValidationMessagestrings duplicate existing shared strings (commonStrings.$tr('fieldRequired')) and diverge slightly fromCreate.vue's wording. - suggestion: email regex
\S+@\S+\.\S+is stricter than the originalEmailField's.+@.+\..+(rejects embedded spaces) — matchesCreate.vue's already-merged pattern, so likely intentional. - suggestion: validation now runs live on every keystroke via
generateFormMixininstead of on blur/submit — same asCreate.vue, likely intentional but a UX shift worth confirming. - suggestion: existing spec (
requestNewActivationLink.spec.js) wasn't updated and doesn't exercise the new validation path — its "invalid email" test only checks the static page header text, noterrors.email/emailErrorTextbehavior. - nitpick: new
form { }style rule selects by bare HTML tag; a scoped class would match codebase convention and stay grep-able.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
| submitButton: 'Submit', | ||
| activationRequestFailed: 'Failed to send a new activation link. Please try again.', | ||
| emailLabel: 'Email', | ||
| fieldRequiredMessage: 'This field is required', |
There was a problem hiding this comment.
suggestion: fieldRequiredMessage: 'This field is required' duplicates the existing shared string commonStrings.$tr('fieldRequired') (shared/translator.js), already reused in Create.vue and shared/utils/validation.js. Per the i18n hoisting convention (3+ usages → shared string), consider importing commonStrings instead of declaring a fourth local copy — keeps translations in sync if the wording ever changes.
| activationRequestFailed: 'Failed to send a new activation link. Please try again.', | ||
| emailLabel: 'Email', | ||
| fieldRequiredMessage: 'This field is required', | ||
| emailValidationMessage: 'Please enter a valid email', |
There was a problem hiding this comment.
suggestion: 'Please enter a valid email' differs by one word from Create.vue's near-identical string 'Please enter a valid email address'. Consider aligning the wording so translators/users see identical messaging for the identical validation rule on both account pages.
| const formMixin = generateFormMixin({ | ||
| email: { | ||
| required: true, | ||
| validator: v => Boolean(v && v.trim()) && /\S+@\S+\.\S+/.test(v), |
There was a problem hiding this comment.
suggestion: This regex (/\S+@\S+\.\S+/) is stricter than the original EmailField's /.+@.+\..+/ — the old pattern's . allowed embedded whitespace, this one rejects it. Same tightening already exists in the merged Create.vue, so likely intentional, but worth a quick confirm since the issue's scope says no functional differences beyond the listed UI changes.
| v-model="email" | ||
| autofocus | ||
| :label="$tr('emailLabel')" | ||
| :errorMessages="errors.email ? [emailErrorText] : []" |
There was a problem hiding this comment.
suggestion: With generateFormMixin, the email setter runs the validator on every v-model update, so errorMessages can now appear after the very first keystroke rather than on blur/submit as before. Matches Create.vue's existing behavior, but flagging since it's a UX change not called out in the issue.
|
|
||
| <style lang="scss" scoped> | ||
| form { |
There was a problem hiding this comment.
nitpick: Selecting by bare HTML tag (form { ... }) fails the "grep test" convention used elsewhere in this codebase (e.g. Create.vue's .form-container). Low blast radius here since it's scoped with a single <form>, but a class name would stay grep-able if the template grows.
…-req-activation-page
rtibblesbot
left a comment
There was a problem hiding this comment.
0 of 5 prior findings resolved; all 5 remain open (see inline comments below). No new findings — RequestNewActivationLink.vue is unchanged since the prior review.
contentcuration/contentcuration/frontend/accounts/pages/tests/requestNewActivationLink.spec.js — suggestion: Still not updated to exercise the new validation path — the "invalid email" test only checks static header text, not errors.email/emailErrorText. The issue's unit-test acceptance criterion asks for a meaningfully updated suite.
CI: pending. Manual QA was required but did not run — UI not visually verified, not a basis for approval.
Prior-finding status
UNADDRESSED — RequestNewActivationLink.vue:94 — local string duplicates shared commonStrings.fieldRequired
UNADDRESSED — RequestNewActivationLink.vue:95 — email-invalid wording diverges from Create.vue
UNADDRESSED — RequestNewActivationLink.vue:47 — email regex stricter than original EmailField
UNADDRESSED — RequestNewActivationLink.vue:22 — validation now runs on every keystroke instead of blur/submit
UNADDRESSED — requestNewActivationLink.spec.js — spec not updated for new validation path
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| submitButton: 'Submit', | ||
| activationRequestFailed: 'Failed to send a new activation link. Please try again.', | ||
| emailLabel: 'Email', | ||
| fieldRequiredMessage: 'This field is required', |
There was a problem hiding this comment.
suggestion: fieldRequiredMessage: 'This field is required' duplicates the existing shared string commonStrings.$tr('fieldRequired') (shared/translator.js), already reused in Create.vue and shared/utils/validation.js. Consider importing commonStrings instead of declaring a fourth local copy.
| activationRequestFailed: 'Failed to send a new activation link. Please try again.', | ||
| emailLabel: 'Email', | ||
| fieldRequiredMessage: 'This field is required', | ||
| emailValidationMessage: 'Please enter a valid email', |
There was a problem hiding this comment.
suggestion: 'Please enter a valid email' differs by one word from Create.vue's 'Please enter a valid email address' — same validation rule, different user-facing wording across the two account pages.
| const formMixin = generateFormMixin({ | ||
| email: { | ||
| required: true, | ||
| validator: v => Boolean(v && v.trim()) && /\S+@\S+\.\S+/.test(v), |
There was a problem hiding this comment.
suggestion: This regex (/\S+@\S+\.\S+/) is stricter than the original EmailField's /.+@.+\..+/ — rejects embedded whitespace the old pattern allowed. Matches Create.vue's pattern so likely intentional — can you confirm this is deliberate and not just inherited from that PR?
| v-model="email" | ||
| autofocus | ||
| :label="$tr('emailLabel')" | ||
| :errorMessages="errors.email ? [emailErrorText] : []" |
There was a problem hiding this comment.
suggestion: With generateFormMixin, the email setter runs the validator on every v-model update, so error text can now appear after the first keystroke rather than on blur/submit as before. Matches Create.vue's existing behavior, but it's a UX shift not called out in the issue.
|
|
||
| <style lang="scss" scoped> | ||
| form { |
There was a problem hiding this comment.
nitpick: Selecting by bare HTML tag (form { ... }) diverges from Create.vue's .form-container class convention. Low risk since scoped with a single <form>, but a class name stays grep-able if the template grows.
Summary
References
Fixes #5929
Reviewer guidance
Screencast.From.2026-07-03.01-46-29.mp4
AI usage
Code was reviewed and some adjustments were made with the help of claude. I tried to bring the UI as close as possible to the original layout, and the LLM generated solutions which felt too over the top for the given scope, and the changes in UI were subtle enough to not be too major, so I decided it would be better to leave it alone and look for further clarification on the matter. Changes were overviewed in the best of my abilities.