-
-
Notifications
You must be signed in to change notification settings - Fork 1k
Set focus and fix i18n #4648
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
Set focus and fix i18n #4648
Conversation
WalkthroughUpdated CLI dialog behavior in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant G as GUI
participant D as CLI_Dialog
participant I18N as i18n
participant IN as CLI_Input
participant R as CLI_Response
U->>G: Open CLI dialog
G->>D: open()
G->>I18N: get("close")
I18N-->>G: localized "close"
G->>D: set buttonCloseText
note over G,D: After dialog open
G->>G: setTimeout(...)
G->>IN: focus() if visible
U->>G: Reset CLI
G->>R: clear()
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)src/js/gui.js (1)
🔇 Additional comments (3)
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 |
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: 0
🧹 Nitpick comments (1)
src/js/gui.js (1)
462-469: Good UX improvement with proper safeguards.The delayed focus implementation correctly handles the timing issue with dialog rendering. The visibility and existence checks prevent potential errors, and the 100ms timeout is a reasonable delay.
Consider whether the dialog system exposes an
openorshownevent that could be used instead ofsetTimeout. This would be more robust than a fixed delay:dialog[0].addEventListener('open', () => { const cliInput = $("#cli-command"); if (cliInput.length > 0 && cliInput.is(":visible")) { cliInput.focus(); } });However, if the dialog doesn't expose such events, the current timeout approach is acceptable.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/gui.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/gui.js (1)
src/js/localization.js (1)
i18n(7-7)
🔇 Additional comments (2)
src/js/gui.js (2)
454-454: LGTM! Proper i18n implementation.The change correctly makes the close button text translatable by using the
i18n.getMessage("close")key instead of a hardcoded string, which aligns with the i18n pattern used elsewhere in the codebase.
457-458: Clearing response area prevents stale data from previous sessions.The change to clear the CLI response area instead of the command input makes sense, as it prevents old responses from appearing when reopening the CLI dialog. The command input is already cleared after each command execution (line 436).
ff421cf to
6b844a7
Compare
|



Summary by CodeRabbit
New Features
Bug Fixes