feat: show actually assigned shortcuts in the tips modal#45
Open
boyonglin wants to merge 1 commit into
Open
Conversation
The tips modal displayed hardcoded suggested combos (Alt+S, Ctrl+Shift+S, ...), but suggested keys are silently dropped when the OS or another extension already owns them — the user reads a shortcut that does nothing when pressed. - tag each shortcut row in popup.html with data-command matching the manifest command names - modal.js queries chrome.commands.getAll() and replaces the hardcoded text with the shortcut actually assigned on this machine (also platform-correct, e.g. Mac symbols) - unassigned commands show a localized 'Not set — click to configure' label in warning color; the existing row click handler already opens chrome://extensions/shortcuts - re-checked every time the modal opens, so reassignments show up without reloading the popup - new locale key shortcutUnsetLabel (en/ja/zh_TW); style added to popup.scss (popup.css rebuilt) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01HHTr9jh2Y9e5pkRAQwuBhx
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 13 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull request overview
This PR updates the Tips modal to display the actually assigned keyboard shortcuts (via chrome.commands.getAll()), rather than hardcoded “suggested” key combos that may be silently overridden/unassigned by the OS or other extensions.
Changes:
- Add
data-commandmarkers to the Tips modal shortcut rows and dynamically replace their displayed text based onchrome.commands.getAll(). - Introduce an “unset shortcut” label + warning style (
.shortcut-unset) and add i18n strings for EN/JA/ZH-TW. - Add/adjust unit tests and rebuild the compiled popup CSS artifacts.
Reviewed changes
Copilot reviewed 8 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/setup.js | Extends the Chrome API mock to include chrome.commands.getAll(). |
| tests/popupDOMFixture.js | Updates the Tips modal DOM fixture to include shortcut rows with data-command. |
| tests/modal.test.js | Adds coverage for “assigned vs unset shortcuts” rendering and refresh-on-reopen behavior. |
| scss/popup.scss | Adds styling for unset shortcut rows (.shortcut-unset). |
| Package/popup.html | Adds data-command attributes to shortcut rows in the Tips modal. |
| Package/dist/components/modal.js | Implements shortcut display refresh using chrome.commands.getAll() when the modal opens. |
| Package/css/popup.css | Rebuilt compiled CSS including .shortcut-unset rules. |
| Package/css/popup.css.map | Updated sourcemap from CSS rebuild. |
| Package/_locales/zh_TW/messages.json | Adds localized shortcutUnsetLabel. |
| Package/_locales/ja/messages.json | Adds localized shortcutUnsetLabel. |
| Package/_locales/en/messages.json | Adds localized shortcutUnsetLabel. |
Files not reviewed (1)
- Package/css/popup.css: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div class="d-flex justify-content-between mt-3"> | ||
| <h2 class="fs-6 fw-normal" data-locale="quickSearchKeyLabel"></h2> | ||
| <p type="button" class="text-muted" title="Keyboard shortcuts" data-locale-title="shortcutsLabel"> | ||
| <p type="button" class="text-muted" title="Keyboard shortcuts" data-locale-title="shortcutsLabel" data-command="run-search"> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
問題
Tips modal 顯示的是寫死的建議快捷鍵(
Alt+S、Ctrl+Shift+S…)。但 manifest 的suggested_key被作業系統或其他擴充功能占用時會靜默失效——使用者照著畫面按,什麼都不會發生,也不知道為什麼。四個快捷鍵是這個產品的核心動線,這是可發現性的大洞。實作
popup.html四列快捷鍵加上data-command(對應 manifest 的 command 名稱)modal.js新增_updateShortcutsDisplay():用chrome.commands.getAll()查本機實際指派的按鍵,取代寫死文字(順帶得到平台正確的顯示,例如 Mac 的 ⌘⇧ 符號).shortcut-unset警示色 + 虛線底線;該列既有的點擊行為本來就會開chrome://extensions/shortcuts,正好接上shortcutUnsetLabel×3 語系;樣式進popup.scss(popup.css重建)測試
全部 21 套件 / 1258 個測試通過(新增 4 個):實際指派覆蓋顯示、未指派標示、重開 modal 後旗標清除、無對應列的 command 安全忽略。
🤖 Generated with Claude Code
https://claude.ai/code/session_01HHTr9jh2Y9e5pkRAQwuBhx
Generated by Claude Code