Conversation
github-actions
bot
added
the
waiting for review
There was a problem hiding this comment.
Pull request overview
Refactors the legacy login/register page from imperative DOM handlers + static HTML/SCSS into SolidJS components, centralizing page UI state (loader/disabled inputs) in a small store.
Changes:
- Replace
pages/login.ts+html/pages/login.html+styles/login.scsswith SolidJSLoginPage/Login/Registercomponents mounted via<mount>. - Add
stores/login.tsto manage login page loader + input enabled state; wire into page lifecycle + google sign-up modal. - Simplify
ready.tsserver configuration sync behavior; adjust firebase/auth error messaging flow.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/ts/stores/login.ts | New shared state for login page loader + input enable/disable + reset hook. |
| frontend/src/ts/ready.ts | Removes login-page-specific DOM toggling; keeps config sync. |
| frontend/src/ts/pages/login.ts | Removes legacy imperative login page implementation. |
| frontend/src/ts/modals/google-sign-up.ts | Switches from page module helpers to store-based UI reset. |
| frontend/src/ts/index.ts | Drops legacy login event-handler import. |
| frontend/src/ts/event-handlers/login.ts | Removes legacy “forgot password” click handler. |
| frontend/src/ts/controllers/page-controller.ts | Converts login route to solidPage("login") and resets login state before show. |
| frontend/src/ts/components/ui/ValidatedInput.tsx | Extends input props (type/autocomplete/name/focus/disabled) for new login UI. |
| frontend/src/ts/components/pages/login/LoginPage.tsx | New Solid login page with loader + server-config gating. |
| frontend/src/ts/components/pages/login/Login.tsx | New Solid login form (providers + email/password). |
| frontend/src/ts/components/pages/login/Register.tsx | New Solid registration form with validations + disposable-email check. |
| frontend/src/ts/components/mount.tsx | Registers loginpage mount component. |
| frontend/src/ts/components/common/Separator.tsx | Adds optional text variant for separator (used in login). |
| frontend/src/ts/firebase.ts | Updates translated firebase auth error messages and adds fallback. |
| frontend/src/styles/login.scss | Removes legacy login page SCSS. |
| frontend/src/styles/index.scss | Stops importing removed login.scss. |
| frontend/src/index.html | Replaces <load src="html/pages/login.html" /> with mount-based login page container. |
| frontend/src/html/pages/login.html | Removes legacy static login page HTML. |
| frontend/src/ts/auth.tsx | Removes internal error toast emission from provider sign-in helper. |
| <div | ||
| class={cn( | ||
| "flex place-items-center gap-4", | ||
| props.vertical ? "flex-col" : "flex-row", | ||
| props.class, | ||
| )} | ||
| > | ||
| <div | ||
| class={cn( | ||
| props.vertical ? "h-full w-1" : "h-1 w-full", | ||
| `rounded bg-sub-alt`, | ||
| )} |
There was a problem hiding this comment.
place-items-center only applies to CSS grid, not flex, so it won’t center items here. Also, using w-full for both line segments in a flex row is prone to overflow/shrinking quirks; use flex growth (e.g. flex-1) + fixed thickness instead so the separator reliably fills remaining space around the text.
Co-authored-by: Miodec <jack@monkeytype.com>
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 40 changed files in this pull request and generated 12 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
frontend/src/ts/modals/google-sign-up.ts:75
- When a provider sign-in creates a new user,
firebase.signInWithPopupkeepsignoreAuthCallback=trueand shows this modal, but there’s no longer any coupling to the login UI state. Result: the login page can re-enable inputs while the sign-up modal is still active. Consider toggling the shared login-page “inputs enabled” signal here (disable onshow, re-enable inhide/applyfinally), or otherwise exposing a completion hook so the login page waits for this flow to finish.
async function hide(): Promise<void> {
void modal.hide({
afterAnimation: async () => {
resetIgnoreAuthCallback();
if (signedInUser !== undefined) {
showNoticeNotification("Sign up process cancelled", {
durationMs: 5000,
});
if (getAdditionalUserInfo(signedInUser)?.isNewUser) {
await Ape.users.delete();
await signedInUser?.user.delete().catch(() => {
//user might be deleted already by the server
});
}
AccountController.signOut();
signedInUser = undefined;
}
},
});
}
You can also share your feedback on Copilot code review. Take the survey.
| type="checkbox" | ||
| checked={checked()} | ||
| disabled={props.disabled} | ||
| class="hidden" |
There was a problem hiding this comment.
The underlying checkbox <input> is display:none (class="hidden"), which removes it from tab order and screen readers in many browsers, making the control keyboard-inaccessible. Prefer a visually-hidden pattern (e.g., sr-only / offscreen / opacity-0 + absolute) so the real input remains focusable/accessible.
| class="hidden" | |
| class="sr-only" |
| onSubmit: async ({ value }) => | ||
| doLogin(async () => | ||
| signIn(value.email, value.password, value.rememberMe), | ||
| ), |
There was a problem hiding this comment.
This error message formatting adds an extra space before the colon ("with Google : ..."). Consider formatting as "with Google: ..." to avoid awkward notifications.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 40 out of 41 changed files in this pull request and generated 5 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
You can also share your feedback on Copilot code review. Take the survey.
No description provided.