-
-
Notifications
You must be signed in to change notification settings - Fork 19
Better API Key Input hints #3086
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
implement toast when users click on example messages if they dont have an api key Better message for disabled input field when users dont have API key Link to Google AI Studio for users to get their API Key in the API key Input
WalkthroughLocalization files were updated to add new API key-related messages in English and Vietnamese. Components handling chat input, empty state, and dialog descriptions were modified to incorporate API key checks and display appropriate user guidance. The EmptyScreen component now requires a Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant EmptyScreen
participant Toast
participant Chat
User->>EmptyScreen: Clicks example message
EmptyScreen-->>EmptyScreen: Check hasApiKey
alt hasApiKey is true
EmptyScreen->>Chat: setInput(message)
else hasApiKey is false
EmptyScreen->>Toast: Show "API key required" error
end
Suggested labels
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
apps/upskii/src/components/chat-panel.tsxOops! Something went wrong! :( ESLint: 9.28.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@typescript-eslint/parser' imported from /eslint.config.mjs apps/upskii/src/components/form-apikey.tsxOops! Something went wrong! :( ESLint: 9.28.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@typescript-eslint/parser' imported from /eslint.config.mjs ✨ Finishing Touches
🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
@coderabbitai full review |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3086 +/- ##
==========================================
- Coverage 0.83% 0.83% -0.01%
==========================================
Files 2494 2494
Lines 307830 307859 +29
Branches 2996 2996
==========================================
Hits 2582 2582
- Misses 303119 303148 +29
Partials 2129 2129 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Nitpick comments (5)
apps/upskii/src/components/prompt-form.tsx (1)
736-739
: Placeholder conflates two different disable reasonsThe current condition shows
ai_chat.api_key_required
when either
disabled
istrue
, or- the API key is missing.
If the textarea is disabled for another reason (e.g. chat summarising, rate limiting), the message is misleading.
Split the logic so the placeholder reflects the actual cause:-placeholder={ - disabled || !apiKey - ? t('ai_chat.api_key_required') - : `${t('ai_chat.send_message')}` -} +placeholder={ + !apiKey + ? t('ai_chat.api_key_required') + : disabled + ? t('ai_chat.input_disabled') /* add a new i18n key */ + : t('ai_chat.send_message') +}This keeps the UX clear and avoids user confusion.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 737-738: apps/upskii/src/components/prompt-form.tsx#L737-L738
Added lines #L737 - L738 were not covered by testsapps/upskii/messages/vi.json (1)
2910-2910
: Typographical consistency in “Xóa tài khoản” message
The entry uses “xoá” (with diacritic on ‘á’) and is lowercase, whereas existing entries use “Xóa” (uppercase ‘X’, accent on ‘á’) consistently. Update the key’s text to match the established casing and accent placement.Example diff:
- "delete-account-message": "Tài khoản của bạn sẽ bị xóa vĩnh viễn và không thể phục hồi. Nhập địa chỉ email của bạn để xác nhận xoá tài khoản.", + "delete-account-message": "Tài khoản của bạn sẽ bị xóa vĩnh viễn và không thể phục hồi. Nhập địa chỉ email của bạn để xác nhận xóa tài khoản.",apps/upskii/src/components/empty-screen.tsx (1)
17-17
: Importingtoast
eagerly can bloat the bundleIf
sonner
is sizeable, considerimport('sonner').then(m => m.toast…)
inside the click handler so the extra KBs are only loaded when needed. Not critical but worth a thought for initial-render performance.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 17-17: apps/upskii/src/components/empty-screen.tsx#L17
Added line #L17 was not covered by testsapps/upskii/src/components/chat-panel.tsx (2)
193-213
: Hard-coded JSX fragments reduce i18n flexibilityThe ‘Get API key’ sentence is now split between a translation string and hard-coded JSX. This makes future localisation (e.g., RTL languages) painful because word order may differ.
Consider exposing the whole sentence to the translator and interpolating the link, e.g.:
const link = ( <Link href="https://aistudio.google.com/app/apikey" target="_blank" rel="noopener noreferrer" className="text-primary underline hover:no-underline" > Google AI Studio </Link> ); {t.rich('api_input_description_full', { link })}Then add
"<0>Google AI Studio</0>"
placeholders in the JSON file.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 193-206: apps/upskii/src/components/chat-panel.tsx#L193-L206
Added lines #L193 - L206 were not covered by tests
[warning] 208-210: apps/upskii/src/components/chat-panel.tsx#L208-L210
Added lines #L208 - L210 were not covered by tests
[warning] 212-212: apps/upskii/src/components/chat-panel.tsx#L212
Added line #L212 was not covered by tests
20-20
: Unused ESLint disable comment?
/* eslint-disable no-unused-vars */
remains at the top, but the file doesn’t define any unused vars after your edits. Removing the pragma prevents masking real issues later.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 20-20: apps/upskii/src/components/chat-panel.tsx#L20
Added line #L20 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/upskii/messages/en.json
(1 hunks)apps/upskii/messages/vi.json
(3 hunks)apps/upskii/src/app/[locale]/(dashboard)/[wsId]/ai-chat/chat.tsx
(1 hunks)apps/upskii/src/components/chat-panel.tsx
(2 hunks)apps/upskii/src/components/empty-screen.tsx
(2 hunks)apps/upskii/src/components/prompt-form.tsx
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
apps/upskii/src/app/[locale]/(dashboard)/[wsId]/ai-chat/chat.tsx (1)
apps/upskii/src/components/empty-screen.tsx (1)
EmptyScreen
(19-186)
apps/upskii/src/components/empty-screen.tsx (3)
packages/ai/src/types.ts (1)
UseChatHelpers
(6-6)packages/types/src/db.ts (1)
AIChat
(3-3)packages/ui/src/components/ui/sonner.tsx (1)
toast
(29-29)
🪛 GitHub Check: codecov/patch
apps/upskii/src/app/[locale]/(dashboard)/[wsId]/ai-chat/chat.tsx
[warning] 334-339: apps/upskii/src/app/[locale]/(dashboard)/[wsId]/ai-chat/chat.tsx#L334-L339
Added lines #L334 - L339 were not covered by tests
apps/upskii/src/components/empty-screen.tsx
[warning] 17-17: apps/upskii/src/components/empty-screen.tsx#L17
Added line #L17 was not covered by tests
[warning] 23-23: apps/upskii/src/components/empty-screen.tsx#L23
Added line #L23 was not covered by tests
[warning] 99-105: apps/upskii/src/components/empty-screen.tsx#L99-L105
Added lines #L99 - L105 were not covered by tests
apps/upskii/src/components/chat-panel.tsx
[warning] 20-20: apps/upskii/src/components/chat-panel.tsx#L20
Added line #L20 was not covered by tests
[warning] 193-206: apps/upskii/src/components/chat-panel.tsx#L193-L206
Added lines #L193 - L206 were not covered by tests
[warning] 208-210: apps/upskii/src/components/chat-panel.tsx#L208-L210
Added lines #L208 - L210 were not covered by tests
[warning] 212-212: apps/upskii/src/components/chat-panel.tsx#L212
Added line #L212 was not covered by tests
apps/upskii/src/components/prompt-form.tsx
[warning] 737-738: apps/upskii/src/components/prompt-form.tsx#L737-L738
Added lines #L737 - L738 were not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Deploy-Preview
- GitHub Check: Deploy-Preview
🔇 Additional comments (1)
apps/upskii/src/components/empty-screen.tsx (1)
23-28
: Adding a requiredhasApiKey
prop is a breaking change – double-check all call-sites
hasApiKey
is now mandatory, but onlychat.tsx
was updated according to the PR summary. Any other<EmptyScreen />
usage that wasn’t touched will now fail type-checking or, worse, silently render withundefined
(always triggering the toast).Two quick mitigations:
-export function EmptyScreen({ - // chats, - setInput, - locale, - hasApiKey, -}: … +export function EmptyScreen({ + // chats, + setInput, + locale, + hasApiKey = false, // sensible default +}: …or keep it required but grep the repo to update every invocation.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 23-23: apps/upskii/src/components/empty-screen.tsx#L23
Added line #L23 was not covered by tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/upskii/src/components/form-apikey.tsx (1)
42-44
: Split-message validation looks good, but embed-once regex is brittle👍 Using a separate
api-key-required-format
message cleanly separates “missing” vs “malformed” states.Minor improvement: the
AIza[0-9A-Za-z\-_]{35}
pattern now lives solely in this component. If the key format ever changes you’ll have to hunt it down in multiple places (client, API route, tests, etc.). Consider extracting it to a shared constant (e.g.constants/api.ts
) or even validating server-side only and re-using the same schema to avoid drift.Also, unit tests that assert the new message key would prevent silent regressions (Codecov warns these lines are uncovered).
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 42-44: apps/upskii/src/components/form-apikey.tsx#L42-L44
Added lines #L42 - L44 were not covered by testsapps/upskii/src/components/chat-panel.tsx (1)
20-20
: Use<a>
instead ofnext/link
for external URLs
next/link
is intended for internal navigation; for absolute URLs (https://…
) it adds no benefit and can unintentionally prefetch or bloat the JS bundle.-import Link from 'next/link'; +// For external links we can rely on a plain anchor tagThen inline:
-<Link href="https://aistudio.google.com/app/apikey" …>Google AI Studio</Link> +<a href="https://aistudio.google.com/app/apikey" target="_blank" + rel="noopener noreferrer" + className="text-primary underline hover:no-underline"> + Google AI Studio +</a>🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 20-20: apps/upskii/src/components/chat-panel.tsx#L20
Added line #L20 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/upskii/messages/en.json
(2 hunks)apps/upskii/messages/vi.json
(4 hunks)apps/upskii/src/components/chat-panel.tsx
(2 hunks)apps/upskii/src/components/form-apikey.tsx
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/upskii/messages/en.json
- apps/upskii/messages/vi.json
🧰 Additional context used
🪛 GitHub Check: codecov/patch
apps/upskii/src/components/form-apikey.tsx
[warning] 42-44: apps/upskii/src/components/form-apikey.tsx#L42-L44
Added lines #L42 - L44 were not covered by tests
apps/upskii/src/components/chat-panel.tsx
[warning] 20-20: apps/upskii/src/components/chat-panel.tsx#L20
Added line #L20 was not covered by tests
[warning] 193-206: apps/upskii/src/components/chat-panel.tsx#L193-L206
Added lines #L193 - L206 were not covered by tests
[warning] 208-210: apps/upskii/src/components/chat-panel.tsx#L208-L210
Added lines #L208 - L210 were not covered by tests
[warning] 212-212: apps/upskii/src/components/chat-panel.tsx#L212
Added line #L212 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Deploy-Preview
- GitHub Check: Deploy-Preview
- GitHub Check: Verify generated types
- GitHub Check: Deploy-Preview
- GitHub Check: Deploy-Preview
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks @VNOsST.
Implement toast when users click on example messages if they dont have an api key
Better message for disabled input field when users dont have API key
Link to Google AI Studio for users to get their API Key in the API key Input
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes