fix: optimized the GitHub login copy and timeout. - #2244
Conversation
WalkthroughThis PR extends GitHub OAuth timeout handling by increasing the backend HTTP client timeout from 5 to 20 seconds for token exchange requests, and adding corresponding frontend UI feedback mechanisms with a 20-second timeout handler that updates button text and disabled state, accompanied by internationalized timeout messages. Changes
Sequence DiagramsequenceDiagram
participant User
participant LoginForm as LoginForm/RegisterForm
participant Backend as Backend
participant GitHub as GitHub OAuth
User->>LoginForm: Click GitHub Login
activate LoginForm
LoginForm->>LoginForm: Set button text to "Redirecting..."
LoginForm->>LoginForm: Disable button
LoginForm->>LoginForm: Start 20s timeout ref
LoginForm->>Backend: Initiate OAuth flow
deactivate LoginForm
Backend->>GitHub: Request token exchange (20s timeout)
alt Success within 20s
GitHub-->>Backend: Return token
Backend-->>LoginForm: OAuth success
LoginForm->>LoginForm: Clear timeout, proceed
else Timeout after 20s
LoginForm->>LoginForm: Timeout fired
LoginForm->>LoginForm: Update button to "Request timed out..."
LoginForm->>LoginForm: Re-disable button
Note over LoginForm: User must refresh
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/src/components/auth/RegisterForm.jsx (1)
246-265: Consider timeout value and retry UXTwo observations about the timeout implementation:
Timeout race condition: The 20-second frontend timeout matches the backend timeout exactly. Frontend should wait slightly longer (e.g., 22-25 seconds) to account for network latency and avoid showing timeout before the backend actually times out.
No retry mechanism: After timeout, the button remains permanently disabled with an error message. Users must refresh the page to try again. Consider allowing retry:
githubTimeoutRef.current = setTimeout(() => { setGithubLoading(false); setGithubButtonText(t('请求超时，请刷新页面后重新发起 GitHub 登录')); - setGithubButtonDisabled(true); + setGithubButtonDisabled(false); // Allow retry -}, 20000); +}, 22000); // Wait slightly longer than backend
- Redundant timeout: The
setTimeouton line 263 that clears loading after 3 seconds is now redundant since the main timeout handles this at 20 seconds.web/src/components/auth/LoginForm.jsx (1)
274-299: Same timeout concerns as RegisterFormThis implementation has the same issues identified in RegisterForm.jsx:
- The 20-second timeout should be slightly longer (22-25 seconds) to avoid racing with the backend
- The button remains permanently disabled after timeout with no retry option
- The
setTimeouton line 297 is redundantConsider applying the same fixes suggested for RegisterForm to maintain consistency.
🧹 Nitpick comments (3)
web/src/i18n/locales/ru.json (1)
2102-2103: Streamline RU timeout phrasing for clarity.Suggested wording:
- "请求超时，请刷新页面后重新发起 GitHub 登录": "Время ожидания истекло, обновите страницу и снова запустите вход через GitHub" + "请求超时，请刷新页面后重新发起 GitHub 登录": "Время ожидания истекло. Обновите страницу и повторите вход через GitHub."Reuse the locale/UI verification script from the EN comment to confirm presence and usage across the codebase.
web/src/components/auth/RegisterForm.jsx (1)
88-90: Use i18n for initial button textThe
githubButtonTextstate is initialized with a hardcoded Chinese string. For consistency with i18n best practices, consider initializing witht('使用 GitHub 继续')or storing just the key and always callingt()when rendering.-const [githubButtonText, setGithubButtonText] = useState('使用 GitHub 继续'); +const [githubButtonText, setGithubButtonText] = useState(t('使用 GitHub 继续'));Alternatively, you could store the translation key and always call
t()when rendering to avoid stale translations if the language changes.web/src/components/auth/LoginForm.jsx (1)
90-92: Use i18n for initial button text (consistent with RegisterForm)Same issue as in RegisterForm: the
githubButtonTextstate is initialized with a hardcoded Chinese string. Consider initializing witht('使用 GitHub 继续')for proper i18n support.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
controller/github.go(1 hunks)web/src/components/auth/LoginForm.jsx(5 hunks)web/src/components/auth/RegisterForm.jsx(5 hunks)web/src/i18n/locales/en.json(1 hunks)web/src/i18n/locales/fr.json(1 hunks)web/src/i18n/locales/ja.json(1 hunks)web/src/i18n/locales/ru.json(1 hunks)web/src/i18n/locales/zh.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
web/src/components/auth/RegisterForm.jsx (1)
web/src/components/auth/LoginForm.jsx (3)
githubButtonText(90-90)githubButtonDisabled(91-91)githubTimeoutRef(92-92)
web/src/components/auth/LoginForm.jsx (1)
web/src/components/auth/RegisterForm.jsx (3)
githubButtonText(88-88)githubButtonDisabled(89-89)githubTimeoutRef(90-90)
🔇 Additional comments (7)
web/src/i18n/locales/zh.json (1)
2074-2076: LGTM! GitHub OAuth translations added correctly.The Chinese translations for GitHub login flow have been properly added:
- "正在跳转 GitHub..." (Redirecting to GitHub...)
- Timeout error message with user guidance
The JSON formatting is correct, with the previous entry updated to include a trailing comma.
web/src/i18n/locales/fr.json (1)
2092-2094: LGTM! French translations added correctly.The French translations for the GitHub OAuth flow have been properly added:
- "Redirection vers GitHub..." (Redirecting to GitHub...)
- "Délai dépassé, veuillez actualiser la page puis relancer la connexion GitHub" (Timeout, please refresh the page and restart GitHub login)
The JSON formatting is correct, and the translations appear grammatically sound.
If you have French-speaking users, consider having a native speaker verify the timeout message phrasing sounds natural. The current translation is grammatically correct but could potentially be refined for better user experience.
controller/github.go (1)
47-47: The response times show both GitHub endpoints respond very quickly (0.13s and 0.12s), which means a 20-second timeout is quite generous. Let me verify what GitHub's API documentation recommends for timeout handling.No action required—the 20-second timeout is appropriate.
Measured response times show GitHub's OAuth and user endpoints respond in ~0.1-0.13 seconds, well under the 20-second timeout. While this may seem conservative, timeouts accommodate network latency, connection establishment, and transient server conditions—not just endpoint response time. Real-world experience shows longer timeouts become necessary for slower internet connections, and 20 seconds aligns with common practice for OAuth integrations handling multiple sequential API calls. The 5-second original timeout may have been insufficient under adverse network conditions.
web/src/i18n/locales/ja.json (1)
2083-2085: LGTM: GitHub OAuth timeout translations addedThe new Japanese translations correctly support the GitHub OAuth timeout flow implemented in the login/register forms. The trailing comma addition on line 2083 follows JSON formatting best practices.
web/src/components/auth/RegisterForm.jsx (2)
134-140: LGTM: Proper timeout cleanupThe cleanup effect correctly clears the timeout on component unmount, preventing memory leaks.
366-378: LGTM: Button state management integratedThe GitHub button correctly uses the dynamic
githubButtonTextandgithubButtonDisabledstates to provide user feedback during the OAuth flow.web/src/components/auth/LoginForm.jsx (1)
461-473: LGTM: Consistent implementation with RegisterFormThe GitHub button correctly implements the same dynamic state pattern as RegisterForm, providing consistent UX across login and registration flows.
| "正在跳转 GitHub...": "Redirecting to GitHub...", | ||
| "请求超时，请刷新页面后重新发起 GitHub 登录": "Request timed out, please refresh and restart GitHub login" |
There was a problem hiding this comment.
🧩 Analysis chain
Polish the timeout copy; keep tone consistent with “Sign in”.
Recommend a clearer, two‑sentence phrasing:
- "请求超时，请刷新页面后重新发起 GitHub 登录": "Request timed out, please refresh and restart GitHub login"
+ "请求超时，请刷新页面后重新发起 GitHub 登录": "Request timed out. Please refresh the page and try signing in with GitHub again."Run to verify these keys exist in all locales and are referenced in UI:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
keys=('正在跳转 GitHub...' '请求超时，请刷新页面后重新发起 GitHub 登录')
echo "Checking locale coverage…"
for f in web/src/i18n/locales/*.json; do
for k in "${keys[@]}"; do
jq -e --arg k "$k" '.translation[$k]' "$f" >/dev/null || echo "MISSING: $k -> $f"
done
done
echo "Searching UI usages…"
rg -nP --type=ts --type=tsx --type=js --type=jsx -C2 "(正在跳转 GitHub\.\.\.|请求超时，请刷新页面后重新发起 GitHub 登录)" web/ || trueLength of output: 74
I see the locale coverage check passed (no MISSING messages), but the UI usage search needs adjustment. Let me retry with a corrected command:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Locale coverage (confirmed) ==="
echo "Keys found in all locale files ✓"
echo ""
echo "=== Searching UI usages ==="
rg -nP --type=ts --type=js --type=jsx -C2 "(正在跳转 GitHub|请求超时)" web/ || echo "No matches found in ts/js/jsx files"
echo ""
echo "=== Checking existing GitHub login patterns ==="
rg -nP --type=ts --type=js "sign.*in|Sign.*in|GitHub" web/src --max-count=5 -i || trueLength of output: 258
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "正在跳转 GitHub...": "Redirecting to GitHub...", | |
| "请求超时，请刷新页面后重新发起 GitHub 登录": "Request timed out, please refresh and restart GitHub login" | |
| "正在跳转 GitHub...": "Redirecting to GitHub...", | |
| "请求超时，请刷新页面后重新发起 GitHub 登录": "Request timed out. Please refresh the page and try signing in with GitHub again." |
🤖 Prompt for AI Agents
In web/src/i18n/locales/en.json around lines 2113 to 2114, update the timeout
copy for consistency with other locales: replace the current value "Request
timed out, please refresh and restart GitHub login" with a more polished and
consistent phrase such as "Request timed out. Please refresh the page and try
signing in with GitHub again." Ensure the key remains unchanged and mirror this
wording across all locale files used by LoginForm.jsx and RegisterForm.jsx.
2 participants
Summary by CodeRabbit
Bug Fixes
Improvements