fix(login): trim usernames and tolerate missing login payloads - #2876
fix(login): trim usernames and tolerate missing login payloads#2876yyqdbngt wants to merge 1 commit into
Conversation
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
This PR adds defensive input handling to the login flow: trimming whitespace from usernames, rejecting whitespace-only input at the form validation level, and safely handling null/malformed API responses. The changes are well-tested with 3 new test cases covering the key scenarios.
Findings
- [Info] web/src/pages/login/index.tsx:53 — Good use of optional chaining and null checks to prevent crashes on unexpected API responses.
- [Info] web/src/pages/login/index.tsx:56 — Minor style note on translating error messages at throw time (non-blocking).
Suggestions
The overall approach is solid. The combination of values.username.trim() + whitespace: true form rule + data?.user optional chaining provides three layers of defense against bad input/data. Test coverage is thorough.
No correctness, performance, or compatibility concerns.
Automated review by github-manager
| try { | ||
| const data = await loginApi(values.username, values.password); | ||
| authLogin(data.user.username, data.user.userId, data.user.admin); | ||
| const data = await loginApi(values.username.trim(), values.password); |
There was a problem hiding this comment.
[Info] Good defensive improvement — using optional chaining on data?.user and then validating user?.username before accessing properties prevents crashes on malformed API responses. Combined with the whitespace: true form rule, this makes the login flow significantly more robust.
| const data = await loginApi(values.username.trim(), values.password); | ||
| const user = data?.user; | ||
| if (!user?.username) { | ||
| throw new Error(t('login.failed')); |
There was a problem hiding this comment.
[Info] Minor note: throw new Error(t('login.failed')) translates the message at throw time. This works correctly since the catch block displays err.message directly, but if the error handling ever changes (e.g., logging the raw error or re-translating), the already-localized string could cause issues. Consider using a stable error key and mapping it in the catch block — though this is a minor style preference, not a blocker.
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Defensive fix that improves input validation and error handling. Code looks clean and follows existing patterns.
LGTM
Automated review by "github-manager-bot"
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
LGTM — defensive fix improving input validation and error handling.
Automated review by "github-manager-bot"
3 participants
Summary
whitespace: true)userobject in the login payload: instead of a rawTypeErrorfromdata.user.username, the user sees the standard "login failed" messageWhy
Users commonly copy usernames from docs or spreadsheets, which drags along leading/trailing spaces; those were sent verbatim and failed with a cryptic backend 401 even though the account exists. Separately,
login()returnsres.data.data, which isnullwhenever the backend wraps the response in an unexpected envelope —data.user.usernamethen threwCannot read properties of null, and that raw TypeError leaked into the error toast.Testing
./node_modules/.bin/vitest run src/pages/login/index.test.tsx→ 6 passed (3 new)./node_modules/.bin/tsc --noEmit→ clean./node_modules/.bin/eslint src/pages/login/index.tsx src/pages/login/index.test.tsx→ 0 errors