feat(#461): customizable keyboard shortcuts with equalizer and fullscreen toggles - #494
Conversation
…AlwaysOnTop and toggleAutoLaunch
1441673 to
1fb7e3c
Compare
|
@Sandakan do you think we should implement a fix to make all the keyboard shortcuts customizable? |
Owie6789
changed the title
1fb7e3c to
b54b08c
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements customizable keyboard shortcuts with new default actions for toggling the equalizer (Ctrl+E) and fullscreen player (Alt+F). It also migrates shortcut action dispatch from label-based matching to stable id-based matching to avoid action breakage when UI language changes, and adds a new Settings section for editing/resetting shortcuts with duplicate detection.
Changes:
- Added stable
idtoShortcutand updated default shortcut definitions + localStorage backward-compat ID injection. - Implemented new shortcut actions: equalizer toggle via
AudioPlayer.toggleEqualizer()and fullscreen toggle logic in the keyboard shortcut handler. - Added inline keyboard shortcut editing UI in Settings, plus new i18n strings (en + tr).
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/types/app.d.ts | Adds Shortcut.id for stable shortcut identification. |
| src/renderer/src/utils/localStorage.ts | Injects missing shortcut IDs for backward compatibility; updates setter to accept id/label. |
| src/renderer/src/other/player.ts | Adds equalizer toggle/preset application methods on AudioPlayer. |
| src/renderer/src/other/appReducer.tsx | Adds default shortcut IDs + new default shortcuts (equalizer/fullscreen). |
| src/renderer/src/hooks/useKeyboardShortcuts.tsx | Switches action dispatch to use shortcut.id; adds equalizer/fullscreen actions. |
| src/renderer/src/components/SettingsPage/SettingsPage.tsx | Adds KeyboardShortcutsSettings to the settings page. |
| src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx | New inline UI for editing/resetting shortcuts with duplicate detection. |
| src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx | Updates prompt editing to use shortcut IDs. |
| src/renderer/src/assets/locales/en/en.json | Adds new shortcut-related strings and settings section copy. |
| src/renderer/src/assets/locales/tr/tr.json | Adds Turkish strings for the new settings section and new shortcut labels. |
| src/main/ipc.ts | Adds imports/handlers used for new window-related actions (per diff context). |
| const key = e.key; | ||
| if (!['CONTROL', 'SHIFT', 'ALT', 'META'].includes(key)) { | ||
| keys.push(key === ' ' ? 'Space' : key); | ||
| } |
| if (duplicate && newKeys.length > 0) { | ||
| editingElement?.classList.add('bg-font-color-crimson', 'dark:bg-font-color-crimson'); | ||
| addNewNotifications([ | ||
| { | ||
| id: 'duplicateShortcut', | ||
| content: t('keyboardShortcutsSettings.duplicateShortcut') | ||
| } | ||
| ]); | ||
| return; | ||
| } else { | ||
| editingElement?.classList.remove('bg-red-200', 'dark:bg-red-800'); | ||
| } |
| case 'toggle equalizer': | ||
| case 'toggleEqualizer': |
| const key = e.key; | ||
| if (!['CONTROL', 'SHIFT', 'ALT', 'META'].includes(key)) { | ||
| keys.push(key === ' ' ? 'Space' : key); | ||
| } |
| .flatMap((category) => category.shortcuts); | ||
| const duplicate = allShortcuts.some( | ||
| (shortcut) => | ||
| shortcut.label !== editingShortcut && | ||
| shortcut.id !== editingShortcut && | ||
| JSON.stringify(shortcut.keys) === JSON.stringify(newKeys) | ||
| ); |
e74c420 to
e694e14
Compare
Docstrings generation was requested by @Owie6789. The following files were modified: * `src/main/ipc.ts` * `src/main/other/discord.ts` * `src/renderer/src/App.tsx` * `src/renderer/src/hooks/useDiscordRpc.tsx` * `src/renderer/src/hooks/usePlaybackErrors.tsx` * `src/renderer/src/routes/main-player/albums/$albumId.tsx` * `src/renderer/src/routes/main-player/artists/$artistId.tsx` * `src/renderer/src/routes/main-player/genres/$genreId.tsx` * `src/renderer/src/routes/main-player/lyrics/editor/$songId.tsx` * `src/renderer/src/routes/main-player/playlists/$playlistId.tsx` * `src/renderer/src/routes/main-player/playlists/favorites.tsx` * `src/renderer/src/routes/main-player/playlists/history.tsx` * `src/renderer/src/routes/main-player/queue/index.tsx` * `src/renderer/src/routes/main-player/search/index.tsx` These files were kept as they were: * `src/renderer/src/hooks/useAppLifecycle.tsx` These files were ignored: * `test/src/main/parseSong/parseSong-concurrency.test.ts` * `test/src/renderer/src/other/playerQueue.test.ts` * `test/src/renderer/src/utils/addMissingPropsToAnObject.test.ts` These file types are not supported: * `.github/copilot-instructions.md` * `.github/workflows/build.yml` * `.github/workflows/codeql-analysis.yml` * `.github/workflows/lint.yml` * `.github/workflows/test.yml` * `PRIVACY-POLICY.md` * `package.json` * `resources/drizzle/0002_square_greymalkin.sql` * `resources/drizzle/meta/_journal.json` * `src/renderer/src/assets/locales/as/as.json` * `src/renderer/src/assets/locales/en/en.json` * `src/renderer/src/assets/locales/fr/fr.json` * `src/renderer/src/assets/locales/pl/pl.json` * `src/renderer/src/assets/locales/pt-br/pt-br.json` * `src/renderer/src/assets/locales/vi/vi.json`
Fixed 12 file(s) based on 11 unresolved review comments. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
…-member fix(Sandakan#502): remove duplicate sortingStates.artistsPage union member
|
@CodeRabbit — comprehensive review request. Please do all of the following and report findings as inline review comments on the changed files (not in this thread), grouped by severity. 1. Careful code review
2. UI audit (only if the PR touches UI)
3. Production-readiness verification
4. Code smell sniff
5. Half-baked implementation detection
6. Required output from you
Please be thorough. I will be applying fixes from your review in a follow-up pass. |
|
Results will appear as inline comments on the diff. A summary comment (✅ solid / User ✅ Action performedFull review finished. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds a complete keyboard shortcuts management system: introducing unique IDs for all shortcuts, creating an editable shortcuts settings UI with duplicate detection, refactoring shortcut dispatch to use ID-based cases instead of localized labels, and adding equalizer control methods to the audio player. The changes include storage migration to support both legacy label-based and new ID-based shortcut lookups. ChangesKeyboard Shortcuts Management with ID-based Dispatch
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx (2)
97-97:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPersistence uses label instead of id, breaking ID-based architecture.
Line 97 calls
setKeyboardShortcuts(newShortcut.label, ...), but the new architecture uses IDs, not labels. According to the storage contract (context snippet 1),setKeyboardShortcutsacceptsidOrLabel, but usinglabelhere defeats the purpose of ID-based matching and will break if labels are changed for localization.This should use
newShortcut.idto match the implementation in KeyboardShortcutsSettings.tsx line 97.🔧 Use ID instead of label for persistence
- storage.keyboardShortcuts.setKeyboardShortcuts(newShortcut.label, newShortcut.keys); + storage.keyboardShortcuts.setKeyboardShortcuts(newShortcut.id, newShortcut.keys);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx` at line 97, The persistence call is using the shortcut label instead of its stable ID; replace the argument to storage.keyboardShortcuts.setKeyboardShortcuts so it uses newShortcut.id (not newShortcut.label) to preserve ID-based matching used across the app (see setKeyboardShortcuts and KeyboardShortcutsSettings usage); update any related call sites in AppShortcutsPrompt where newShortcut.label is passed to storage.keyboardShortcuts to pass newShortcut.id instead.
87-87:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMissing i18n for user-facing strings in AppShortcutsPrompt.
Multiple hardcoded English strings are not wrapped in
t()calls:
- Line 87:
'This key combination is already in use'- Line 139:
'Press New Shortcut'- Line 181:
'Edit your shortcut'- Line 187:
'RESET'- Lines 194-196:
'Confirm Shortcut Reset','Are you sure...'- Line 199:
'RESET'- Line 205:
<span>All shortcuts have been successfully reset.</span>This breaks localization and creates UI inconsistencies. According to the en.json file (lines 934-944), proper i18n keys exist in
keyboardShortcutsSettingssection but aren't being used here.🌐 Replace hardcoded strings with i18n keys
content: t('keyboardShortcutsSettings.duplicateShortcut') - content: 'This key combination is already in use'- <span className="text-font-color-dimmed">{'Press New Shortcut'}</span> + <span className="text-font-color-dimmed">{t('keyboardShortcutsSettings.pressNewShortcut')}</span>- <div className="instruction-text text-font-color-dimmed px-4 text-center text-sm"> - {'Edit your shortcut'} - </div> + <div className="instruction-text text-font-color-dimmed px-4 text-center text-sm"> + {t('keyboardShortcutsSettings.editShortcut')} + </div>- label="RESET" + label={t('keyboardShortcutsSettings.resetToDefaults')}- title="Confirm Shortcut Reset" + title={t('keyboardShortcutsSettings.resetConfirmTitle')} content={ - <div>Are you sure you want to reset all shortcuts to their default settings?</div> + <div>{t('keyboardShortcutsSettings.resetConfirmContent')}</div> } confirmButton={{ - label: 'RESET', + label: t('keyboardShortcutsSettings.resetToDefaults'),{ id: 'shortcutsReset', - content: <span>All shortcuts have been successfully reset.</span> + content: t('keyboardShortcutsSettings.resetSuccess') }Also applies to: 139-139, 181-181, 187-187, 194-196, 205-205
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx` at line 87, In AppShortcutsPrompt, replace all hardcoded user-facing strings with i18n calls using the existing keyboardShortcutsSettings keys from en.json: wrap "This key combination is already in use", "Press New Shortcut", "Edit your shortcut", both "RESET" buttons, "Confirm Shortcut Reset", the confirmation body "Are you sure...", and "All shortcuts have been successfully reset." with t('keyboardShortcutsSettings.<appropriateKey>') (use the matching keys from en.json) inside the AppShortcutsPrompt component so the UI uses localized strings everywhere.
🧹 Nitpick comments (5)
src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx (3)
47-56: ⚡ Quick winState update inside keydown handler may cause performance issues.
The
setShortcutscall updates the entire shortcuts state on every keypress during editing. This triggers re-renders and recalculates theshortcutCategoryComponentsmemoization on every key event, which is unnecessary since the visual feedback is already provided vianewKeysstate.Consider updating shortcuts state only when saving (on click-outside), not during key capture.
⚡ Move state update to save time only
Remove the
setShortcutscall from the keydown handler (lines 47-56) and rely on the mousedown handler's save logic (lines 96-99) to update both storage and state together.The editing UI already shows live feedback via
newKeysstate (lines 132-137), so updatingshortcutsprematurely is redundant and causes extra renders.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx` around lines 47 - 56, Remove the premature state write inside the keydown handler: delete the setShortcuts(...) update that mutates shortcuts using new_shortcut.id/new_shortcut.keys so keypresses don't re-render shortcutCategoryComponents on every keystroke; instead keep live visual feedback in newKeys and only persist the change when the existing save flow runs (the mousedown/click-outside save logic that updates storage and state), ensuring the save path still calls setShortcuts once with the final newKeys value.
1-8: ⚡ Quick winImport organization does not follow coding guidelines.
The import order should be: external dependencies, internal path aliases (
@renderer,@main,@common), then relative imports. Currently, all imports are relative.As per coding guidelines: "Use eslint-plugin-simple-import-sort for automatic import organization: external dependencies, internal path aliases (
@renderer,@main,@common), then relative imports"📦 Reorganized imports following guidelines
+import { Fragment, useContext, useEffect, useMemo, useState } from 'react'; +import { useTranslation } from 'react-i18next'; + -import { Fragment, useContext, useEffect, useMemo, useState } from 'react'; -import { useTranslation } from 'react-i18next'; - -import { AppUpdateContext } from '../../../contexts/AppUpdateContext'; -import storage from '../../../utils/localStorage'; -import Button from '../../Button'; -import SensitiveActionConfirmPrompt from '../../SensitiveActionConfirmPrompt'; -import ShortcutButton from '../ShortcutButton'; +import { AppUpdateContext } from '`@renderer/contexts/AppUpdateContext`'; +import storage from '`@renderer/utils/localStorage`'; +import Button from '`@renderer/components/Button`'; +import SensitiveActionConfirmPrompt from '`@renderer/components/SensitiveActionConfirmPrompt`'; +import ShortcutButton from '`@renderer/components/SettingsPage/ShortcutButton`';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx` around lines 1 - 8, Reorder the imports in KeyboardShortcutsSettings.tsx to follow the project's import-sorting rule: place external dependencies first (e.g., "react" and "react-i18next"), then any internal path-alias imports (e.g., `@renderer/`@main/@common if used), and finally the relative imports (AppUpdateContext, storage, Button, SensitiveActionConfirmPrompt, ShortcutButton) — ensure the import list for the component KeyboardShortcutsSettings matches eslint-plugin-simple-import-sort ordering and groupings.
96-99: ⚖️ Poor tradeoffPersistence may fail silently if storage migration contract is violated.
Line 97 calls
setKeyboardShortcuts(newShortcut.id, newShortcut.keys). Based on the storage contract (context snippet 1), this function matches shortcuts byidORlabel. However, ifnewShortcut.iddoesn't match any stored shortcut's ID or label (due to a migration bug or data corruption), the update will silently fail.Consider adding error handling or validation feedback.
Since the storage layer doesn't expose success/failure, and the UI immediately re-fetches (line 98), silent failures would be caught by the UI showing the old value. This is acceptable but could be improved with explicit error states in a future refactor.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx` around lines 96 - 99, The update may silently fail if storage.keyboardShortcuts.setKeyboardShortcuts(newShortcut.id, newShortcut.keys) doesn't match any existing entry; before calling setKeyboardShortcuts, validate that newShortcut.id (or newShortcut.label) exists in storage.keyboardShortcuts.getKeyboardShortcuts(), and after calling setKeyboardShortcuts re-fetch via storage.keyboardShortcuts.getKeyboardShortcuts() and compare to confirm the keys changed for that id; if the value didn't change, set a local error state or dispatch a user-visible notification (use setShortcuts for the successful path and an error indicator for failures) so migration/data-corruption issues surface instead of failing silently.src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx (1)
117-126: 💤 Low valueRedundant condition check in isEditing logic.
Line 117 defines
isEditing = editingShortcut === shortcut.id, but line 123 checksisEditing && editingShortcut === shortcut.id, which is redundant sinceisEditingalready implies the second condition is true.🧹 Remove redundant condition
className={`shortcut mb-4 flex w-[45%] items-center justify-between p-2 ${ - isEditing && editingShortcut === shortcut.id + isEditing ? 'editing bg-dark-background-color-3/75 dark:bg-dark-background-color-3/15 rounded-md' : '' }`}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx` around lines 117 - 126, The conditional is redundant: `isEditing` is defined as `editingShortcut === shortcut.id`, so remove the duplicate `&& editingShortcut === shortcut.id` check in the className expression; update the ternary to only test `isEditing` (i.e., replace `isEditing && editingShortcut === shortcut.id ? ... : ''` with `isEditing ? ... : ''`) so the highlighting logic uses the single variable `isEditing`.src/renderer/src/hooks/useKeyboardShortcuts.tsx (1)
106-319: 🏗️ Heavy liftExtract the shortcut actions out of this callback.
The dispatch branch has become the dominant body of the hook. Moving the cases into a typed handler map or small helper functions will make the id contract easier to audit and bring this hook back in line with the repo’s hook/function size guidance. As per coding guidelines, "Create focused custom hooks for feature-specific logic, keeping them under 200 lines each, with clear single responsibility and documented dependencies" and "Keep functions small, aiming for 30-50 lines maximum per function; extract complex logic into separate helper functions for single responsibility".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/renderer/src/hooks/useKeyboardShortcuts.tsx` around lines 106 - 319, The manageKeyboardShortcuts callback has grown too large; extract the big switch over shortcutId into a dedicated handler (e.g., a typed ACTION_HANDLERS map or a function handleShortcutAction(shortcutId, matchedShortcut)) and move each case block (playPause, toggleMute, nextSong, prevSong, tenSecondsForward, tenSecondsBackward, upVolume, downVolume, toggleShuffle, toggleRepeat, toggleFavorite, upPlaybackRate, downPlaybackRate, resetPlaybackRate, goToSearch, goToLyrics, goToQueue, goHome, openMiniPlayer, toggleFullscreenPlayer, selectMultipleItems, toggleTheme, toggleEqualizer, reload, openAppShortcutsPrompt, openDevtools, etc.) into its own small helper or map entry; have manageKeyboardShortcuts simply find matchedShortcut, call e.preventDefault(), then invoke the handler with the needed context (pass functions/state like toggleSongPlayback, toggleMutedState, player, storage, store, addNewNotifications, navigate, updatePlayerType, toggleMultipleSelections, changePromptMenuData, t, window.api, etc.), and update the useCallback dependency list to include the new handler references (or wrap the handler map in useRef to avoid unnecessary deps) so behavior and typing remain correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx`:
- Around line 83-93: Replace direct DOM class manipulation in
KeyboardShortcutsSettings with React state: introduce an isDuplicate (or
duplicateError) state and, in the duplicate-detection branch where currently
editingElement?.classList.add(...) and .remove(...) are called, set isDuplicate
= true and addNewNotifications as before, and set isDuplicate = false in the
else branch and when exiting edit mode; then apply the error CSS by
conditionally adding the 'bg-font-color-crimson dark:bg-font-color-crimson'
classes in the JSX for the editing element (use a className expression based on
isDuplicate) instead of manipulating editingElement.classList.
- Around line 70-78: The duplicate check is reading from
storage.keyboardShortcuts.getKeyboardShortcuts() (allShortcuts) which can be
stale; change it to use the component's current state (the keyboardShortcuts
state initialized around lines 12-14) instead of storage: derive allShortcuts
from the in-memory keyboardShortcuts state (flatten its category.shortcuts),
then keep the existing sortKeys and duplicate logic that compares sorted
shortcut.keys to newKeys while excluding editingShortcut so duplicate detection
reflects unsaved UI changes.
- Around line 38-42: The new_shortcut object incorrectly sets label to the id
(editingShortcut); update the creation of new_shortcut so it preserves the
original user-facing label from the shortcut being edited (look up the existing
shortcut by id in the shortcuts collection used in this component and use its
.label), while only replacing .keys with the new keys; ensure the object you
construct (new_shortcut: Shortcut) uses that preserved label and the updated
keys before saving or dispatching the change.
- Around line 63-106: The useEffect currently attaches the global 'mousedown'
listener on every dependency change even when not editing; update the effect so
the listener is only added when editingShortcut is truthy and removed on cleanup
(attach inside the branch where editingShortcut is set), and remove the stable
i18n translator 't' from the dependency array; keep newShortcut, newKeys,
addNewNotifications and editingShortcut in the deps, and ensure
handleClickOutside logic and cleanup still reference the same identifiers
(handleClickOutside, editingShortcut, newShortcut, newKeys,
addNewNotifications).
In `@src/renderer/src/hooks/useKeyboardShortcuts.tsx`:
- Around line 157-158: The switch on shortcutId uses matchedShortcut.id or
matchedShortcut.label directly, so legacy/fallback ids can bypass the expected
stable action ids and hit default; update useKeyboardShortcuts.tsx to
canonicalize legacy IDs before dispatch: call getKeyboardShortcuts()’s canonical
mapping or add a resolver function (e.g.
resolveLegacyShortcutId(matchedShortcut)) to translate known legacy labels/ids
to the stable action ids, then use that resolved value (instead of
matchedShortcut.id || matchedShortcut.label) for the switch/dispatch logic so
legacy shortcuts map to the expected cases.
In `@src/renderer/src/other/player.ts`:
- Around line 635-642: applyEqualizerPreset currently writes gains directly to
the nodes and thus can re-enable EQ while UI/state says it's off; change it to
update a stored desired preset and only push values into the actual
BiquadFilterNode gains when the equalizer is active. Specifically, add or use a
property like desiredEqualizerPreset (map of EqualizerBandFilters->number), have
applyEqualizerPreset update desiredEqualizerPreset for each filterName and only
set band.gain.value when this.isEqualizerActive (or getEqualizerState() is
true), and ensure toggleEqualizer when enabling the EQ iterates equalizerBands
to apply desiredEqualizerPreset to the band.gain.value so stored presets take
effect when turned on.
In `@src/renderer/src/utils/localStorage.ts`:
- Around line 287-306: The current merge only iterates userShortcuts and
therefore drops any categories that exist in defaults but not in the user's
stored list; update the post-merge step (after the merged variable creation) to
append any default categories missing from merged by comparing a stable
identifier (e.g., category.id or another unique key) and concat those default
categories (including their shortcuts) so new default categories are preserved
for existing users; reference the variables userShortcuts, defaults, merged and
the ShortcutCategory/category.id when locating where to add this logic.
- Around line 287-305: The current migration in merged uses array indices
(catIdx, scIdx) to map userShortcuts to defaults which can misassign ids; change
the mapping to locate the default category by matching shortcutCategoryTitle
(use defaults.find(d => d.shortcutCategoryTitle ===
category.shortcutCategoryTitle)) and then for each shortcut without an id, find
the corresponding default shortcut by label (defaultCategory?.shortcuts.find(ds
=> ds.label === shortcut.label)) and use its id; when computing missingDefaults,
consider defaults whose ids or labels are not present in patchedShortcuts so you
don't duplicate or lose shortcuts, and keep a safe fallback id (e.g., based on
label) only if no matching default is found.
---
Outside diff comments:
In `@src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx`:
- Line 97: The persistence call is using the shortcut label instead of its
stable ID; replace the argument to
storage.keyboardShortcuts.setKeyboardShortcuts so it uses newShortcut.id (not
newShortcut.label) to preserve ID-based matching used across the app (see
setKeyboardShortcuts and KeyboardShortcutsSettings usage); update any related
call sites in AppShortcutsPrompt where newShortcut.label is passed to
storage.keyboardShortcuts to pass newShortcut.id instead.
- Line 87: In AppShortcutsPrompt, replace all hardcoded user-facing strings with
i18n calls using the existing keyboardShortcutsSettings keys from en.json: wrap
"This key combination is already in use", "Press New Shortcut", "Edit your
shortcut", both "RESET" buttons, "Confirm Shortcut Reset", the confirmation body
"Are you sure...", and "All shortcuts have been successfully reset." with
t('keyboardShortcutsSettings.<appropriateKey>') (use the matching keys from
en.json) inside the AppShortcutsPrompt component so the UI uses localized
strings everywhere.
---
Nitpick comments:
In `@src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx`:
- Around line 117-126: The conditional is redundant: `isEditing` is defined as
`editingShortcut === shortcut.id`, so remove the duplicate `&& editingShortcut
=== shortcut.id` check in the className expression; update the ternary to only
test `isEditing` (i.e., replace `isEditing && editingShortcut === shortcut.id ?
... : ''` with `isEditing ? ... : ''`) so the highlighting logic uses the single
variable `isEditing`.
In
`@src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx`:
- Around line 47-56: Remove the premature state write inside the keydown
handler: delete the setShortcuts(...) update that mutates shortcuts using
new_shortcut.id/new_shortcut.keys so keypresses don't re-render
shortcutCategoryComponents on every keystroke; instead keep live visual feedback
in newKeys and only persist the change when the existing save flow runs (the
mousedown/click-outside save logic that updates storage and state), ensuring the
save path still calls setShortcuts once with the final newKeys value.
- Around line 1-8: Reorder the imports in KeyboardShortcutsSettings.tsx to
follow the project's import-sorting rule: place external dependencies first
(e.g., "react" and "react-i18next"), then any internal path-alias imports (e.g.,
`@renderer/`@main/@common if used), and finally the relative imports
(AppUpdateContext, storage, Button, SensitiveActionConfirmPrompt,
ShortcutButton) — ensure the import list for the component
KeyboardShortcutsSettings matches eslint-plugin-simple-import-sort ordering and
groupings.
- Around line 96-99: The update may silently fail if
storage.keyboardShortcuts.setKeyboardShortcuts(newShortcut.id, newShortcut.keys)
doesn't match any existing entry; before calling setKeyboardShortcuts, validate
that newShortcut.id (or newShortcut.label) exists in
storage.keyboardShortcuts.getKeyboardShortcuts(), and after calling
setKeyboardShortcuts re-fetch via
storage.keyboardShortcuts.getKeyboardShortcuts() and compare to confirm the keys
changed for that id; if the value didn't change, set a local error state or
dispatch a user-visible notification (use setShortcuts for the successful path
and an error indicator for failures) so migration/data-corruption issues surface
instead of failing silently.
In `@src/renderer/src/hooks/useKeyboardShortcuts.tsx`:
- Around line 106-319: The manageKeyboardShortcuts callback has grown too large;
extract the big switch over shortcutId into a dedicated handler (e.g., a typed
ACTION_HANDLERS map or a function handleShortcutAction(shortcutId,
matchedShortcut)) and move each case block (playPause, toggleMute, nextSong,
prevSong, tenSecondsForward, tenSecondsBackward, upVolume, downVolume,
toggleShuffle, toggleRepeat, toggleFavorite, upPlaybackRate, downPlaybackRate,
resetPlaybackRate, goToSearch, goToLyrics, goToQueue, goHome, openMiniPlayer,
toggleFullscreenPlayer, selectMultipleItems, toggleTheme, toggleEqualizer,
reload, openAppShortcutsPrompt, openDevtools, etc.) into its own small helper or
map entry; have manageKeyboardShortcuts simply find matchedShortcut, call
e.preventDefault(), then invoke the handler with the needed context (pass
functions/state like toggleSongPlayback, toggleMutedState, player, storage,
store, addNewNotifications, navigate, updatePlayerType,
toggleMultipleSelections, changePromptMenuData, t, window.api, etc.), and update
the useCallback dependency list to include the new handler references (or wrap
the handler map in useRef to avoid unnecessary deps) so behavior and typing
remain correct.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ced81331-9717-4409-a53d-86e943d2a64b
📒 Files selected for processing (11)
src/main/ipc.tssrc/renderer/src/assets/locales/en/en.jsonsrc/renderer/src/assets/locales/tr/tr.jsonsrc/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsxsrc/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsxsrc/renderer/src/components/SettingsPage/SettingsPage.tsxsrc/renderer/src/hooks/useKeyboardShortcuts.tsxsrc/renderer/src/other/appReducer.tsxsrc/renderer/src/other/player.tssrc/renderer/src/utils/localStorage.tssrc/types/app.d.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Use module-level initialization for singleton services (AudioPlayer, PlayerQueue) accessed by intervals, timers, or event handlers, NOT useRef-based singletons, to avoid stale closure issues
Always return cleanup functions in useEffect hooks that register IPC event listeners, and ensure corresponding removal functions are called to prevent memory leaks
Use descriptive, context-rich variable and function names; avoid generic names like 'data', 'user', 'info', 'temp'. Functions should start with verbs (calculateDuration, fetchSongData, validatePlaylist)
Use guard clauses with early returns to keep function logic flat and readable, avoiding deep nesting and improving error handling clarity
Use eslint-plugin-simple-import-sort for automatic import organization: external dependencies, internal path aliases (@renderer,@main,@common), then relative imports
All data fetching errors should be handled within queryFn, returning safe defaults (empty arrays, null, default objects) instead of throwing exceptions
Keep functions small, aiming for 30-50 lines maximum per function; extract complex logic into separate helper functions for single responsibility
Files:
src/renderer/src/components/SettingsPage/SettingsPage.tsxsrc/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsxsrc/types/app.d.tssrc/renderer/src/utils/localStorage.tssrc/renderer/src/hooks/useKeyboardShortcuts.tsxsrc/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsxsrc/main/ipc.tssrc/renderer/src/other/player.tssrc/renderer/src/other/appReducer.tsx
src/renderer/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
src/renderer/src/**/*.{ts,tsx}: Use dispatch() function from store.ts for all state updates in the renderer, never modify store state directly
Extract all data fetching logic into centralized query modules using createQueryKeys factory pattern from@lukemorales/query-key-factoryin src/renderer/src/queries/, never inline fetch logic in components
Use useSuspenseQuery() for data fetching in components with TanStack Router loaders for pre-fetching, not custom fetch hooks or useQuery without suspense
Use TanStack Router's , useNavigate(), and useRouter() for navigation, not deprecated changeCurrentActivePage() or updatePageHistoryIndex() functions
Files:
src/renderer/src/components/SettingsPage/SettingsPage.tsxsrc/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsxsrc/renderer/src/utils/localStorage.tssrc/renderer/src/hooks/useKeyboardShortcuts.tsxsrc/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsxsrc/renderer/src/other/player.tssrc/renderer/src/other/appReducer.tsx
src/renderer/src/**/*.tsx
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All custom hooks that integrate features into App.tsx must be called in App.tsx with no return value (they manage state via dispatch and event listeners internally)
Files:
src/renderer/src/components/SettingsPage/SettingsPage.tsxsrc/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsxsrc/renderer/src/hooks/useKeyboardShortcuts.tsxsrc/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsxsrc/renderer/src/other/appReducer.tsx
src/renderer/src/hooks/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Create focused custom hooks for feature-specific logic, keeping them under 200 lines each, with clear single responsibility and documented dependencies
Files:
src/renderer/src/hooks/useKeyboardShortcuts.tsx
src/main/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
All business logic should be in src/main/core/ modules with single responsibility, following the pattern of taking data as parameters and calling database queries or external APIs
Files:
src/main/ipc.ts
src/main/ipc.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Register all IPC handlers using ipcMain.handle() for async operations with return values and ipcMain.on() for fire-and-forget events, mapping them to core business logic modules
Files:
src/main/ipc.ts
src/renderer/src/other/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Singleton service classes (AudioPlayer, PlayerQueue) must be instantiated at module level before any hook function definition to enable proper closure capture by intervals and timers
Files:
src/renderer/src/other/player.ts
🧠 Learnings (7)
📚 Learning: 2026-06-04T16:49:13.844Z
Learnt from: CR
Repo: Sandakan/Nora PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-06-04T16:49:13.844Z
Learning: Applies to src/renderer/src/**/*.{ts,tsx} : Use TanStack Router's <Link>, useNavigate(), and useRouter() for navigation, not deprecated changeCurrentActivePage() or updatePageHistoryIndex() functions
Applied to files:
src/renderer/src/hooks/useKeyboardShortcuts.tsx
📚 Learning: 2026-06-04T16:49:13.844Z
Learnt from: CR
Repo: Sandakan/Nora PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-06-04T16:49:13.844Z
Learning: Applies to src/renderer/src/hooks/**/*.{ts,tsx} : Create focused custom hooks for feature-specific logic, keeping them under 200 lines each, with clear single responsibility and documented dependencies
Applied to files:
src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx
📚 Learning: 2026-06-04T16:49:13.844Z
Learnt from: CR
Repo: Sandakan/Nora PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-06-04T16:49:13.844Z
Learning: Applies to src/preload/index.ts : All IPC communication to main process must be exposed through window.api with categorized namespaces (playerControls, audioLibraryControls, settingsHelpers, etc.) for type safety
Applied to files:
src/main/ipc.ts
📚 Learning: 2026-06-04T16:49:13.844Z
Learnt from: CR
Repo: Sandakan/Nora PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-06-04T16:49:13.844Z
Learning: Applies to src/main/ipc.ts : Register all IPC handlers using ipcMain.handle() for async operations with return values and ipcMain.on() for fire-and-forget events, mapping them to core business logic modules
Applied to files:
src/main/ipc.ts
📚 Learning: 2026-06-04T16:49:13.844Z
Learnt from: CR
Repo: Sandakan/Nora PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-06-04T16:49:13.844Z
Learning: Applies to **/*.{ts,tsx} : Always return cleanup functions in useEffect hooks that register IPC event listeners, and ensure corresponding removal functions are called to prevent memory leaks
Applied to files:
src/main/ipc.ts
📚 Learning: 2026-06-04T16:49:13.844Z
Learnt from: CR
Repo: Sandakan/Nora PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-06-04T16:49:13.844Z
Learning: Applies to src/renderer/src/other/**/*.ts : Singleton service classes (AudioPlayer, PlayerQueue) must be instantiated at module level before any hook function definition to enable proper closure capture by intervals and timers
Applied to files:
src/renderer/src/other/player.ts
📚 Learning: 2026-06-04T16:49:13.844Z
Learnt from: CR
Repo: Sandakan/Nora PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-06-04T16:49:13.844Z
Learning: Applies to **/*.{ts,tsx} : Use module-level initialization for singleton services (AudioPlayer, PlayerQueue) accessed by intervals, timers, or event handlers, NOT useRef-based singletons, to avoid stale closure issues
Applied to files:
src/renderer/src/other/player.ts
🔇 Additional comments (15)
src/types/app.d.ts (1)
625-625: LGTM!src/renderer/src/other/appReducer.tsx (4)
440-520: LGTM!
521-560: LGTM!
561-605: LGTM!
606-641: LGTM!src/renderer/src/utils/localStorage.ts (1)
309-331: LGTM!src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx (2)
156-159: Edit initialization uses shortcut.id correctly.Lines 156-159 correctly initialize editing by setting
editingShortcuttoshortcut.idand pre-populatingnewKeyswith the current keys. This matches the ID-based architecture described in the PR objectives.
186-220: Reset confirmation flow is well-structured.The reset-to-defaults flow (lines 186-220) properly:
- Opens a confirmation prompt via
changePromptMenuData- Calls the storage reset API
- Shows a success notification
- Refreshes the component state
This follows the existing pattern for sensitive actions in the app.
src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx (2)
34-36: Key exclusion logic is correct and matches KeyboardShortcutsSettings.Line 34 properly excludes modifier-only keys (
Control,Shift,Alt,Meta) before adding the final key, preventing duplicate modifier keys in the captured combination. This matches the implementation in KeyboardShortcutsSettings.tsx line 34.
73-78: Duplicate detection logic correctly uses sorted key comparison.Lines 73-78 implement a robust duplicate check by:
- Creating a
sortKeyshelper to normalize key order- Comparing sorted keys using JSON.stringify
- Excluding the currently edited shortcut from the check
This matches the implementation in KeyboardShortcutsSettings.tsx and correctly handles key combinations in any order (e.g., "Ctrl+S" vs "S+Ctrl").
src/renderer/src/components/SettingsPage/SettingsPage.tsx (1)
13-13: KeyboardShortcutsSettings integration is correctly placed.The new component is:
- Properly imported on line 13
- Rendered in a logical position between EqualizerSettings and DefaultPageSettings (lines 49-50)
- Follows the same list-item pattern as other settings sections
This placement makes sense from a UX perspective, grouping input-related settings together.
Also applies to: 49-50
src/renderer/src/assets/locales/en/en.json (2)
934-944: All required i18n keys are properly defined.The
keyboardShortcutsSettingssection (lines 934-944) includes all necessary localization strings:
- Title and description for the settings section
- Reset flow strings (button label, confirmation title/content, success message)
- Editing flow strings (edit tooltip, press new shortcut prompt, duplicate error)
These keys are correctly used in KeyboardShortcutsSettings.tsx but are missing from AppShortcutsPrompt.tsx (as noted in previous comments).
912-914: New shortcut labels added for equalizer and fullscreen toggles.Lines 912 and 914 add labels for the new
toggleEqualizerandtoggleFullscreenPlayershortcuts mentioned in the PR objectives. These properly extend the existingappShortcutsPromptsection and will be displayed in the shortcuts UI.src/renderer/src/hooks/useKeyboardShortcuts.tsx (1)
252-257: LGTM!Also applies to: 322-327
src/main/ipc.ts (1)
88-89: LGTM!
| const new_shortcut: Shortcut = { | ||
| id: editingShortcut, | ||
| label: editingShortcut, | ||
| keys: keys | ||
| }; |
There was a problem hiding this comment.
Incorrect label assignment in new shortcut object.
Line 40 sets label: editingShortcut, but editingShortcut is the shortcut's id, not its display label. This will cause the shortcut to display the ID string (e.g., "playPause") instead of the localized user-facing label (e.g., "Play / Pause").
The label should be retrieved from the original shortcut being edited.
🔧 Proposed fix to preserve original label
+ const editingShortcutData = shortcuts
+ .flatMap(cat => cat.shortcuts)
+ .find(s => s.id === editingShortcut);
+
const new_shortcut: Shortcut = {
id: editingShortcut,
- label: editingShortcut,
+ label: editingShortcutData?.label || editingShortcut,
keys: keys
};🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx`
around lines 38 - 42, The new_shortcut object incorrectly sets label to the id
(editingShortcut); update the creation of new_shortcut so it preserves the
original user-facing label from the shortcut being edited (look up the existing
shortcut by id in the shortcuts collection used in this component and use its
.label), while only replacing .keys with the new keys; ensure the object you
construct (new_shortcut: Shortcut) uses that preserved label and the updated
keys before saving or dispatching the change.
| useEffect(() => { | ||
| if (!editingShortcut) return; | ||
|
|
||
| const handleClickOutside = (e: MouseEvent) => { | ||
| const shortcutElements = document.querySelectorAll('.shortcut.editing'); | ||
| const clickedOutside = Array.from(shortcutElements).every((el) => !el.contains(e.target as Node)); | ||
|
|
||
| const allShortcuts = storage.keyboardShortcuts | ||
| .getKeyboardShortcuts() | ||
| .flatMap((category) => category.shortcuts); | ||
| const sortKeys = (k: string[]) => [...k].sort(); | ||
| const duplicate = allShortcuts.some( | ||
| (shortcut) => | ||
| shortcut.id !== editingShortcut && | ||
| JSON.stringify(sortKeys(shortcut.keys)) === JSON.stringify(sortKeys(newKeys)) | ||
| ); | ||
|
|
||
| const editingElement = document.querySelector('.shortcut.editing') as HTMLElement | null; | ||
|
|
||
| if (duplicate && newKeys.length > 0) { | ||
| editingElement?.classList.add('bg-font-color-crimson', 'dark:bg-font-color-crimson'); | ||
| addNewNotifications([ | ||
| { | ||
| id: 'duplicateShortcut', | ||
| content: t('keyboardShortcutsSettings.duplicateShortcut') | ||
| } | ||
| ]); | ||
| return; | ||
| } else { | ||
| editingElement?.classList.remove('bg-font-color-crimson', 'dark:bg-font-color-crimson'); | ||
| } | ||
|
|
||
| if (clickedOutside) { | ||
| if (newShortcut && !duplicate) { | ||
| storage.keyboardShortcuts.setKeyboardShortcuts(newShortcut.id, newShortcut.keys); | ||
| setShortcuts(storage.keyboardShortcuts.getKeyboardShortcuts()); | ||
| } | ||
| setEditingShortcut(null); | ||
| } | ||
| }; | ||
|
|
||
| document.addEventListener('mousedown', handleClickOutside); | ||
| return () => document.removeEventListener('mousedown', handleClickOutside); | ||
| }, [editingShortcut, newShortcut, newKeys, addNewNotifications, t]); |
There was a problem hiding this comment.
Memory leak: mousedown listener is not properly scoped to editing mode.
The mousedown listener (line 104) is added every time editingShortcut, newShortcut, or newKeys changes, even when not editing. While line 64 guards the handler logic, the listener itself remains attached and fires on every mousedown globally.
Additionally, the dependency array includes t (line 106) which is unnecessary since t is stable from useTranslation().
🧹 Optimize listener attachment and dependencies
- }, [editingShortcut, newShortcut, newKeys, addNewNotifications, t]);
+ }, [editingShortcut, newShortcut, newKeys, addNewNotifications]);The early return on line 64 already prevents execution when not editing, so this is a minor optimization. The real issue is the listener firing globally; consider if this behavior is acceptable or if you want to conditionally attach it only when editingShortcut !== null.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx`
around lines 63 - 106, The useEffect currently attaches the global 'mousedown'
listener on every dependency change even when not editing; update the effect so
the listener is only added when editingShortcut is truthy and removed on cleanup
(attach inside the branch where editingShortcut is set), and remove the stable
i18n translator 't' from the dependency array; keep newShortcut, newKeys,
addNewNotifications and editingShortcut in the deps, and ensure
handleClickOutside logic and cleanup still reference the same identifiers
(handleClickOutside, editingShortcut, newShortcut, newKeys,
addNewNotifications).
| const allShortcuts = storage.keyboardShortcuts | ||
| .getKeyboardShortcuts() | ||
| .flatMap((category) => category.shortcuts); | ||
| const sortKeys = (k: string[]) => [...k].sort(); | ||
| const duplicate = allShortcuts.some( | ||
| (shortcut) => | ||
| shortcut.id !== editingShortcut && | ||
| JSON.stringify(sortKeys(shortcut.keys)) === JSON.stringify(sortKeys(newKeys)) | ||
| ); |
There was a problem hiding this comment.
Duplicate detection reads stale data from storage instead of current UI state.
Lines 70-72 fetch shortcuts from storage, but the component already maintains current shortcuts in state (line 12-14). During editing, if the user has made unsaved changes, the duplicate check will use stale storage data instead of the current editing session's state, potentially missing duplicates or incorrectly flagging them.
🛠️ Use component state instead of storage
- const allShortcuts = storage.keyboardShortcuts
- .getKeyboardShortcuts()
- .flatMap((category) => category.shortcuts);
+ const allShortcuts = shortcuts.flatMap((category) => category.shortcuts);This ensures duplicate detection uses the current UI state, not stale storage.
📝 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.
| const allShortcuts = storage.keyboardShortcuts | |
| .getKeyboardShortcuts() | |
| .flatMap((category) => category.shortcuts); | |
| const sortKeys = (k: string[]) => [...k].sort(); | |
| const duplicate = allShortcuts.some( | |
| (shortcut) => | |
| shortcut.id !== editingShortcut && | |
| JSON.stringify(sortKeys(shortcut.keys)) === JSON.stringify(sortKeys(newKeys)) | |
| ); | |
| const allShortcuts = shortcuts.flatMap((category) => category.shortcuts); | |
| const sortKeys = (k: string[]) => [...k].sort(); | |
| const duplicate = allShortcuts.some( | |
| (shortcut) => | |
| shortcut.id !== editingShortcut && | |
| JSON.stringify(sortKeys(shortcut.keys)) === JSON.stringify(sortKeys(newKeys)) | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx`
around lines 70 - 78, The duplicate check is reading from
storage.keyboardShortcuts.getKeyboardShortcuts() (allShortcuts) which can be
stale; change it to use the component's current state (the keyboardShortcuts
state initialized around lines 12-14) instead of storage: derive allShortcuts
from the in-memory keyboardShortcuts state (flatten its category.shortcuts),
then keep the existing sortKeys and duplicate logic that compares sorted
shortcut.keys to newKeys while excluding editingShortcut so duplicate detection
reflects unsaved UI changes.
| editingElement?.classList.add('bg-font-color-crimson', 'dark:bg-font-color-crimson'); | ||
| addNewNotifications([ | ||
| { | ||
| id: 'duplicateShortcut', | ||
| content: t('keyboardShortcutsSettings.duplicateShortcut') | ||
| } | ||
| ]); | ||
| return; | ||
| } else { | ||
| editingElement?.classList.remove('bg-font-color-crimson', 'dark:bg-font-color-crimson'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Direct DOM manipulation via classList breaks React's declarative model.
Adding/removing classes via classList.add() and classList.remove() (lines 83, 92) bypasses React's virtual DOM and can lead to state inconsistencies, especially if the component re-renders.
Use React state to control the error styling declaratively.
♻️ Replace classList manipulation with React state
Add state to track duplicate errors:
const [editingShortcut, setEditingShortcut] = useState<string | null>(null);
+ const [isDuplicate, setIsDuplicate] = useState(false);Update duplicate detection logic:
if (duplicate && newKeys.length > 0) {
- editingElement?.classList.add('bg-font-color-crimson', 'dark:bg-font-color-crimson');
+ setIsDuplicate(true);
addNewNotifications([...]);
return;
} else {
- editingElement?.classList.remove('bg-font-color-crimson', 'dark:bg-font-color-crimson');
+ setIsDuplicate(false);
}Apply the class in JSX (line 122-126):
className={`shortcut mb-2 flex w-full items-center justify-between rounded-md p-2 sm:w-[48%] ${
isEditing
- ? 'editing bg-dark-background-color-3/75 dark:bg-dark-background-color-3/15'
+ ? `editing bg-dark-background-color-3/75 dark:bg-dark-background-color-3/15 ${isDuplicate ? 'bg-font-color-crimson dark:bg-font-color-crimson' : ''}`
: ''
}`}Also reset isDuplicate when exiting edit mode (line 100).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx`
around lines 83 - 93, Replace direct DOM class manipulation in
KeyboardShortcutsSettings with React state: introduce an isDuplicate (or
duplicateError) state and, in the duplicate-detection branch where currently
editingElement?.classList.add(...) and .remove(...) are called, set isDuplicate
= true and addNewNotifications as before, and set isDuplicate = false in the
else branch and when exiting edit mode; then apply the error CSS by
conditionally adding the 'bg-font-color-crimson dark:bg-font-color-crimson'
classes in the JSX for the editing element (use a className expression based on
isDuplicate) instead of manipulating editingElement.classList.
| const shortcutId = matchedShortcut.id || matchedShortcut.label; | ||
| switch (shortcutId) { |
There was a problem hiding this comment.
Canonicalize legacy shortcut ids before dispatch.
This only works if getKeyboardShortcuts() always returns the stable action ids used in the switch. The PR context says legacy entries can get generated fallback ids, and those values will fall through to default after preventDefault(), breaking existing customized shortcuts until users resave them. Resolve legacy shortcuts to the canonical ids before this branch, or keep an explicit legacy-label/id mapping here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/src/hooks/useKeyboardShortcuts.tsx` around lines 157 - 158, The
switch on shortcutId uses matchedShortcut.id or matchedShortcut.label directly,
so legacy/fallback ids can bypass the expected stable action ids and hit
default; update useKeyboardShortcuts.tsx to canonicalize legacy IDs before
dispatch: call getKeyboardShortcuts()’s canonical mapping or add a resolver
function (e.g. resolveLegacyShortcutId(matchedShortcut)) to translate known
legacy labels/ids to the stable action ids, then use that resolved value
(instead of matchedShortcut.id || matchedShortcut.label) for the switch/dispatch
logic so legacy shortcuts map to the expected cases.
| applyEqualizerPreset(preset: Partial<Record<EqualizerBandFilters, number>>) { | ||
| for (const [filterName, gainValue] of Object.entries(preset)) { | ||
| const band = this.equalizerBands.get(filterName as EqualizerBandFilters); | ||
| if (band) { | ||
| band.gain.value = gainValue ?? 0; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Keep preset updates from bypassing the EQ off-state.
toggleEqualizer() treats "off" as all gains set to 0, but applyEqualizerPreset() writes the stored gains unconditionally. Any preset update while the equalizer is off will color the audio again even though getEqualizerState() still returns false. Keep the desired preset separate from the applied gains, and only push band gains into the filter nodes when isEqualizerActive is true.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/src/other/player.ts` around lines 635 - 642,
applyEqualizerPreset currently writes gains directly to the nodes and thus can
re-enable EQ while UI/state says it's off; change it to update a stored desired
preset and only push values into the actual BiquadFilterNode gains when the
equalizer is active. Specifically, add or use a property like
desiredEqualizerPreset (map of EqualizerBandFilters->number), have
applyEqualizerPreset update desiredEqualizerPreset for each filterName and only
set band.gain.value when this.isEqualizerActive (or getEqualizerState() is
true), and ensure toggleEqualizer when enabling the EQ iterates equalizerBands
to apply desiredEqualizerPreset to the band.gain.value so stored presets take
effect when turned on.
| const merged = userShortcuts.map((category: ShortcutCategory, catIdx: number) => { | ||
| const defaultCategory = defaults[catIdx]; | ||
| const patchedShortcuts = category.shortcuts.map((shortcut: Shortcut, scIdx: number) => { | ||
| if (shortcut.id) return shortcut; | ||
| const defaultShortcut = defaultCategory?.shortcuts[scIdx]; | ||
| return { | ||
| ...shortcut, | ||
| id: defaultShortcut?.id || `unknown-${catIdx}-${scIdx}` | ||
| }; | ||
| }); | ||
| const existingIds = new Set(patchedShortcuts.map((s: Shortcut) => s.id)); | ||
| const missingDefaults = (defaultCategory?.shortcuts || []).filter( | ||
| (ds: Shortcut) => !existingIds.has(ds.id) | ||
| ); | ||
| return { | ||
| ...category, | ||
| shortcuts: [...patchedShortcuts, ...missingDefaults] | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Index-based category and shortcut matching is fragile and may assign incorrect IDs during migration.
The migration maps user categories to defaults by array index (catIdx, scIdx) and generates fallback IDs like unknown-${catIdx}-${scIdx}. If the default shortcuts are reordered between versions, users with legacy shortcuts will receive mismatched IDs that don't correspond to the intended actions.
Suggested fix: Match categories by shortcutCategoryTitle and shortcuts by label when assigning missing IDs:
- const merged = userShortcuts.map((category: ShortcutCategory, catIdx: number) => {
- const defaultCategory = defaults[catIdx];
+ const merged = userShortcuts.map((category: ShortcutCategory) => {
+ const defaultCategory = defaults.find(d => d.shortcutCategoryTitle === category.shortcutCategoryTitle);
const patchedShortcuts = category.shortcuts.map((shortcut: Shortcut, scIdx: number) => {
if (shortcut.id) return shortcut;
- const defaultShortcut = defaultCategory?.shortcuts[scIdx];
+ const defaultShortcut = defaultCategory?.shortcuts.find(ds => ds.label === shortcut.label);
return {
...shortcut,
- id: defaultShortcut?.id || `unknown-${catIdx}-${scIdx}`
+ id: defaultShortcut?.id || shortcut.label
};
});This ensures that a shortcut with label: "Play/Pause" always gets id: "playPause" regardless of its position in the array.
📝 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.
| const merged = userShortcuts.map((category: ShortcutCategory, catIdx: number) => { | |
| const defaultCategory = defaults[catIdx]; | |
| const patchedShortcuts = category.shortcuts.map((shortcut: Shortcut, scIdx: number) => { | |
| if (shortcut.id) return shortcut; | |
| const defaultShortcut = defaultCategory?.shortcuts[scIdx]; | |
| return { | |
| ...shortcut, | |
| id: defaultShortcut?.id || `unknown-${catIdx}-${scIdx}` | |
| }; | |
| }); | |
| const existingIds = new Set(patchedShortcuts.map((s: Shortcut) => s.id)); | |
| const missingDefaults = (defaultCategory?.shortcuts || []).filter( | |
| (ds: Shortcut) => !existingIds.has(ds.id) | |
| ); | |
| return { | |
| ...category, | |
| shortcuts: [...patchedShortcuts, ...missingDefaults] | |
| }; | |
| }); | |
| const merged = userShortcuts.map((category: ShortcutCategory) => { | |
| const defaultCategory = defaults.find(d => d.shortcutCategoryTitle === category.shortcutCategoryTitle); | |
| const patchedShortcuts = category.shortcuts.map((shortcut: Shortcut, scIdx: number) => { | |
| if (shortcut.id) return shortcut; | |
| const defaultShortcut = defaultCategory?.shortcuts.find(ds => ds.label === shortcut.label); | |
| return { | |
| ...shortcut, | |
| id: defaultShortcut?.id || shortcut.label | |
| }; | |
| }); | |
| const existingIds = new Set(patchedShortcuts.map((s: Shortcut) => s.id)); | |
| const missingDefaults = (defaultCategory?.shortcuts || []).filter( | |
| (ds: Shortcut) => !existingIds.has(ds.id) | |
| ); | |
| return { | |
| ...category, | |
| shortcuts: [...patchedShortcuts, ...missingDefaults] | |
| }; | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/src/utils/localStorage.ts` around lines 287 - 305, The current
migration in merged uses array indices (catIdx, scIdx) to map userShortcuts to
defaults which can misassign ids; change the mapping to locate the default
category by matching shortcutCategoryTitle (use defaults.find(d =>
d.shortcutCategoryTitle === category.shortcutCategoryTitle)) and then for each
shortcut without an id, find the corresponding default shortcut by label
(defaultCategory?.shortcuts.find(ds => ds.label === shortcut.label)) and use its
id; when computing missingDefaults, consider defaults whose ids or labels are
not present in patchedShortcuts so you don't duplicate or lose shortcuts, and
keep a safe fallback id (e.g., based on label) only if no matching default is
found.
| const merged = userShortcuts.map((category: ShortcutCategory, catIdx: number) => { | ||
| const defaultCategory = defaults[catIdx]; | ||
| const patchedShortcuts = category.shortcuts.map((shortcut: Shortcut, scIdx: number) => { | ||
| if (shortcut.id) return shortcut; | ||
| const defaultShortcut = defaultCategory?.shortcuts[scIdx]; | ||
| return { | ||
| ...shortcut, | ||
| id: defaultShortcut?.id || `unknown-${catIdx}-${scIdx}` | ||
| }; | ||
| }); | ||
| const existingIds = new Set(patchedShortcuts.map((s: Shortcut) => s.id)); | ||
| const missingDefaults = (defaultCategory?.shortcuts || []).filter( | ||
| (ds: Shortcut) => !existingIds.has(ds.id) | ||
| ); | ||
| return { | ||
| ...category, | ||
| shortcuts: [...patchedShortcuts, ...missingDefaults] | ||
| }; | ||
| }); | ||
| return merged; |
There was a problem hiding this comment.
New shortcut categories added to defaults will be lost for existing users.
The migration only maps over userShortcuts (line 287), so any categories present in defaults but not in the user's stored data are silently dropped. When new shortcut categories are introduced in future versions, upgrading users won't receive them.
Suggested fix: After the merge, append any default categories missing from the user data:
});
+ const userCategoryTitles = new Set(merged.map(c => c.shortcutCategoryTitle));
+ const missingCategories = defaults.filter(dc => !userCategoryTitles.has(dc.shortcutCategoryTitle));
- return merged;
+ return [...merged, ...missingCategories];This ensures users receive all new shortcut categories on upgrade, maintaining parity with fresh installs.
📝 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.
| const merged = userShortcuts.map((category: ShortcutCategory, catIdx: number) => { | |
| const defaultCategory = defaults[catIdx]; | |
| const patchedShortcuts = category.shortcuts.map((shortcut: Shortcut, scIdx: number) => { | |
| if (shortcut.id) return shortcut; | |
| const defaultShortcut = defaultCategory?.shortcuts[scIdx]; | |
| return { | |
| ...shortcut, | |
| id: defaultShortcut?.id || `unknown-${catIdx}-${scIdx}` | |
| }; | |
| }); | |
| const existingIds = new Set(patchedShortcuts.map((s: Shortcut) => s.id)); | |
| const missingDefaults = (defaultCategory?.shortcuts || []).filter( | |
| (ds: Shortcut) => !existingIds.has(ds.id) | |
| ); | |
| return { | |
| ...category, | |
| shortcuts: [...patchedShortcuts, ...missingDefaults] | |
| }; | |
| }); | |
| return merged; | |
| const merged = userShortcuts.map((category: ShortcutCategory, catIdx: number) => { | |
| const defaultCategory = defaults[catIdx]; | |
| const patchedShortcuts = category.shortcuts.map((shortcut: Shortcut, scIdx: number) => { | |
| if (shortcut.id) return shortcut; | |
| const defaultShortcut = defaultCategory?.shortcuts[scIdx]; | |
| return { | |
| ...shortcut, | |
| id: defaultShortcut?.id || `unknown-${catIdx}-${scIdx}` | |
| }; | |
| }); | |
| const existingIds = new Set(patchedShortcuts.map((s: Shortcut) => s.id)); | |
| const missingDefaults = (defaultCategory?.shortcuts || []).filter( | |
| (ds: Shortcut) => !existingIds.has(ds.id) | |
| ); | |
| return { | |
| ...category, | |
| shortcuts: [...patchedShortcuts, ...missingDefaults] | |
| }; | |
| }); | |
| const userCategoryTitles = new Set(merged.map(c => c.shortcutCategoryTitle)); | |
| const missingCategories = defaults.filter(dc => !userCategoryTitles.has(dc.shortcutCategoryTitle)); | |
| return [...merged, ...missingCategories]; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/renderer/src/utils/localStorage.ts` around lines 287 - 306, The current
merge only iterates userShortcuts and therefore drops any categories that exist
in defaults but not in the user's stored list; update the post-merge step (after
the merged variable creation) to append any default categories missing from
merged by comparing a stable identifier (e.g., category.id or another unique
key) and concat those default categories (including their shortcuts) so new
default categories are preserved for existing users; reference the variables
userShortcuts, defaults, merged and the ShortcutCategory/category.id when
locating where to add this logic.
…memory duplicate check, label-based migration
- AppShortcutsPrompt.tsx + KeyboardShortcutsSettings.tsx:
* Replace hardcoded English strings with t('keyboardShortcutsSettings.*')
keys (duplicateShortcut, pressNewShortcut, editShortcut, resetToDefaults,
resetConfirmTitle, resetConfirmContent, resetSuccess).
* Fix storage.write call: newShortcut.label -> newShortcut.id (was
always equal to editingShortcut id, but semantically incorrect).
* Drop redundant isEditing && editingShortcut === shortcut.id check in
className computation.
* Stop calling setShortcuts on every keydown (was causing re-render of
the entire shortcut list on each key event). Just track newKeys /
newShortcut in state, write to storage only on save (click-outside).
* Duplicate-detection now uses in-memory 'shortcuts' state instead of
re-reading storage, so newly typed keys are checked against the
current view (and not stale storage data).
* Preserve original shortcut.label when saving (id/label are now
decoupled so a future label rename works).
- localStorage.ts: getKeyboardShortcuts migration rewritten to match
categories and shortcuts by title/id/label (not by index), so:
* User-saved keys for an existing default shortcut are preserved.
* New default categories (added in future app versions) appear in
the user's data even if the user has older localStorage.
* Genuinely user-only categories are still kept at the end.
- useKeyboardShortcuts.tsx: strip legacy 'Key' / '_key' suffix from
shortcut IDs before dispatch, so old 'playPauseKey' labels still
match the new 'playPause' cases.
- player.ts: applyEqualizerPreset now no-ops when the equalizer is
inactive, so saving a preset does not silently re-enable it.
|
@coderabbitai re-review requested on commit e8fa0b2 (push e694e14..e8fa0b2). What I fixedMajor / Quick-win findings
Skipped (out of scope / cosmetic)
Verification
Could you re-review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== KeyboardShortcutsSettings.tsx ==="
cat src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx
echo ""
echo "=== AppShortcutsPrompt.tsx ==="
cat src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx
echo ""
echo "=== localStorage.ts (relevant section) ==="
sed -n '270,360p' src/renderer/src/utils/localStorage.ts
echo ""
echo "=== useKeyboardShortcuts.tsx (relevant section) ==="
sed -n '140,185p' src/renderer/src/hooks/useKeyboardShortcuts.tsx
echo ""
echo "=== player.ts (equalizer section) ==="
sed -n '620,680p' src/renderer/src/other/player.ts
echo ""
echo "=== en.json (keyboardShortcutsSettings section) ==="
grep -n "keyboardShortcutsSettings\|pressNewShortcut\|duplicateShortcut\|editShortcut\|resetToDefaults\|resetConfirm\|resetSuccess" src/renderer/src/assets/locales/en/en.jsonLength of output: 24338
✅ Confirmed clean
|
… type handleClickOutside, complete hook deps
|
@coderabbitai — final comprehensive re-review pass. Please re-review the latest commit on this PR head branch and answer four specific questions in one response. Do not narrow scope to a checklist. Inspect the entire diff history of this branch (every commit between this branch and What I want you to verify1. Re-review the latest commitReview every new change I just pushed against the full current state of the branch. Surface any new blocker, major, minor, or nit you find. 2. Original issue fulfillmentThe PR's "Closes #N" issue defines the user-visible problem this branch is meant to fix. For each "Closes" issue, walk the issue text and tell me:
3. Deviation from issue fixCompare every commit on this branch to the original issue. Did any commit pull the scope in a direction the issue did not ask for? Examples to look for:
If you find any, name the commit, the file, and the lines, and say whether you think the deviation is justified (e.g., "required for the fix to be sound") or removable. 4. Over-sieveingWalk the whole diff and tell me if any of the changes are over-engineered for the original issue. Examples:
For each one, name it, and say whether it should be kept, narrowed, or removed. What to skip
Output formatReply with four clearly-labeled sections — |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== Issue `#461` ==="
gh issue view 461 --repo Sandakan/Nora
echo ""
echo "=== Git log (branch commits) ==="
git log --oneline upstream/release/4.0.0-alpha.5..HEAD 2>/dev/null || git log --oneline origin/release/4.0.0-alpha.5..HEAD 2>/dev/null || git log --oneline -20
echo ""
echo "=== player.ts toggleEqualizer and applyEqualizerPreset ==="
grep -n "toggleEqualizer\|applyEqualizerPreset\|isEqualizerActive" src/renderer/src/other/player.ts | head -60
echo ""
echo "=== player.ts full relevant section ==="
sed -n '610,700p' src/renderer/src/other/player.ts
echo ""
echo "=== AppShortcutsPrompt.tsx full ==="
cat src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx
echo ""
echo "=== KeyboardShortcutsSettings.tsx full ==="
cat src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx
echo ""
echo "=== useKeyboardShortcuts.tsx relevant sections ==="
sed -n '140,200p' src/renderer/src/hooks/useKeyboardShortcuts.tsx
echo ""
echo "=== localStorage.ts relevant sections ==="
sed -n '260,380p' src/renderer/src/utils/localStorage.ts
echo ""
echo "=== appReducer.tsx keyboardShortcuts section ==="
grep -n "toggleEqualizer\|toggleFullscreen\|keyboardShortcuts\|otherShortcuts" src/renderer/src/other/appReducer.tsx | head -40
echo ""
echo "=== ipc.ts toggleAutoLaunch/toggleMiniPlayerAlwaysOnTop ==="
grep -n "toggleAutoLaunch\|toggleMiniPlayerAlwaysOnTop\|toggleEqualizer\|toggleFullscreen" src/main/ipc.ts | head -20
echo ""
echo "=== en.json keyboardShortcutsSettings ==="
grep -n "keyboardShortcutsSettings\|toggleEqualizer\|toggleFullscreen\|pressNewShortcut\|duplicateShortcut\|resetToDefaults\|resetConfirm\|resetSuccess\|editShortcut" src/renderer/src/assets/locales/en/en.json
echo ""
echo "=== app.d.ts Shortcut interface ==="
grep -n -A5 "Shortcut" src/types/app.d.ts | head -30
echo ""
echo "=== SettingsPage.tsx KeyboardShortcutsSettings import ==="
grep -n "KeyboardShortcuts" src/renderer/src/components/SettingsPage/SettingsPage.tsx
echo ""
echo "=== useKeyboardShortcuts.tsx full file ==="
cat src/renderer/src/hooks/useKeyboardShortcuts.tsxLength of output: 46133
1. Re-review of latest commit (
|
| Item | Status |
|---|---|
player.ts regression (isEqualizerActive set before applyEqualizerPreset) |
✅ Fixed |
handleClickOutside typed as MouseEvent |
✅ Fixed |
t added to useMemo deps in both components |
✅ Fixed |
New issue found:
KeyboardShortcutsSettings.tsx — reset confirm button handler (~Line 195):
clickHandler: () => {
storage.keyboardShortcuts.resetShortcutsToDefaults();
addNewNotifications([...]);
setShortcuts(storage.keyboardShortcuts.getKeyboardShortcuts());
// ← missing changePromptMenuData(false)
}The SensitiveActionConfirmPrompt modal will not close after the user confirms the reset. The AppShortcutsPrompt.tsx equivalent correctly calls changePromptMenuData(false). Fix: add changePromptMenuData(false); after setShortcuts(...).
Pre-existing nit (not regressed): localStorage.ts extraUserCategories — first OR-condition is always false (dead code), noted in previous pass, not a blocker.
2. Original issue fulfillment
Verdict: Partial pass — happy path works, one interpretation gap
Issue #461, item 3 asks for: "customizable or default keyboard shortcuts for frequently used features—such as opening the equalizer and toggling fullscreen mode."
| Criterion | PR behavior | Met? |
|---|---|---|
| Shortcut exists for the equalizer | Ctrl+E triggers player.toggleEqualizer() — turns EQ on/off |
|
| Shortcut exists for fullscreen | Alt+F toggles fullscreen with race condition fixed (playerType read before dispatch) |
✅ |
| Shortcuts are customizable | Inline KeyboardShortcutsSettings UI with edit/reset/duplicate detection |
✅ |
| Shortcuts survive language change | id-based matching with label fallback |
✅ |
| Backward compat for existing users | getKeyboardShortcuts migration merges user data with defaults |
✅ |
| Tests | None added | ❌ (no tests for migration logic, duplicate detection, or dispatch) |
Cannot fully confirm without running the app, but the code path from keydown → shortcutId === 'toggleEqualizer' → player.toggleEqualizer() → isEqualizerActive = true → applyEqualizerPreset(storedPreset) is complete and correct after e9e903f.
3. Deviation from issue fix
Verdict: Two deviations — one justified, one removable
| Commit | File | Lines | Deviation | Justified? |
|---|---|---|---|---|
e694e14 |
src/main/ipc.ts |
88–89 | Adds toggleAutoLaunch and toggleMiniPlayerAlwaysOnTop to the destructured import. These are unrelated to keyboard shortcuts. |
❌ Removable. This is a drive-by fix riding along with the feature. Should be its own PR or at minimum called out explicitly. |
e694e14 |
src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx |
1–225 | Adds a full 225-line inline settings page for editing all shortcuts, not just the two new ones. The issue asked for shortcuts to exist, not for a full customization UI. | ✅ Justified in spirit (inline editing is what makes shortcuts customizable per the issue's word), but it balloons scope significantly. |
e694e14 |
src/renderer/src/other/appReducer.tsx + app.d.ts |
various | Adds id field to every existing shortcut and rewrites dispatch to id-based |
✅ Required for the feature to be sound; label-based matching would break on locale change. |
e694e14 |
src/renderer/src/assets/locales/en/en.json |
912–943 | Adds 16 i18n keys | ✅ Required by the UI added above. |
4. Over-engineering
Verdict: Three items worth trimming
1. useKeyboardShortcuts.tsx — getKeyboardShortcuts() called on every keydown (keep, but note)
manageKeyboardShortcuts calls storage.keyboardShortcuts.getKeyboardShortcuts() at the top of every single keypress. That function reads localStorage, merges two arrays, and re-maps shortcuts on every event. It's a hot path. The result never changes unless the user saves a new shortcut, so it should be computed once outside the handler (or via a useRef updated on storage change). Not a correctness bug, but worth flagging as a performance concern.
2. KeyboardShortcutsSettings.tsx / AppShortcutsPrompt.tsx — ~80% duplicated logic (narrow)
Both components implement identical keydown capture, mousedown click-outside, duplicate detection, and render loops. The only differences are class names and layout. A shared useShortcutEditor hook extracting [editingShortcut, newKeys, newShortcut, handleKeyDown, handleClickOutside] would eliminate the duplication. Currently the same bug (the missing changePromptMenuData(false)) manifests in only one of them because they drifted.
3. newShortcut state has a useless label field (narrow)
Both components set setNewShortcut({ id: editingShortcut, label: editingShortcut, keys }) — label is set to the id string and never read anywhere in the save path. The state object could simply be { id: string; keys: string[] }, avoiding the confusion of label === id.
Merged latest upstream release into fix/461-keyboard-shortcuts-customization to clear mergeable=CONFLICTING on Sandakan/Nora PR Sandakan#494. Conflict in src/renderer/src/assets/locales/tr/tr.json was an indentation disagreement only (HEAD used tabs, upstream uses 2-space indent) with identical key set and translation values. Adopted upstream's canonical indentation (2-space) for the whole file to match the rest of the locale files in the merged tree. All other files (src/main/ipc.ts, en.json, appReducer.tsx, player.ts, src/types/app.d.ts) auto-merged cleanly.
…e for duplicate highlight CodeRabbit review nitpick (Actions > KeyboardShortcutsSettings line 50/62/65/74): duplicate-error highlight used direct DOM querying and classList.add/remove side-effects inside a mousedown listener. Replaced with hasDuplicate React state derived from the same in-memory shortcuts check, applied via className on the editing row. Also preserves the original shortcut label (instead of using the id) when constructing newShortcut, so callers receiving the object see the actual label rather than the identifier. Refs Sandakan#494, fixes reviewer feedback from PR round 1.
CodeRabbit re-review (post e9e903f) caught a regression: the reset confirm button only refreshed local shortcuts and fired a notification but never closed the SensitiveActionConfirmPrompt modal. The sibling AppShortcutsPrompt.tsx correctly calls changePromptMenuData(false) on the same path; this commit restores parity. Refs Sandakan#494.
…ect.test.ts CI lint and test runs failed on the merge commit 99cf0c8 with the same parse error already fixed on PR Sandakan#499: test/src/renderer/src/utils/addMissingPropsToAnObject.test.ts:198:6 error: Expected \}\ but found \EOF\ Test 5 ('should handle null/undefined nested values in existing object', starting at line 165) was missing its closing \});\. As a result, test 6 ('should preserve pre-existing top-level keys while filling sortingStates', starting at line 177) was nested inside the unclosed test 5, leaving the file with an unmatched \{\ and triggering the parse error. This was introduced by the merge resolution with upstream origin/release/4.0.0-alpha.5 in commit 99cf0c8. The test file picked up the new test scaffold from upstream but the closing brace was dropped during the merge. Fix: insert the missing \});\ between test 5 and test 6. Verification: - npm run lint: clean (warnings only, no errors) - npm test: 336 passed (11 files)
|
@coderabbitai ? pushed Section 1 (re-review of latest commit) ? fixes applied:
Section 2 (original issue fulfillment): The intent for the equalizer shortcut was toggle (ON/OFF), not open panel. Ctrl+E toggles Section 3 (deviation):
Section 4 (over-engineering):
CI: lint clean, 336/336 tests passing on commit |
|
@CodeRabbit You are performing a pre-release production audit of this PR against the target branch ScopeReview the FULL diff of this PR, not just the headline files. Cross-check every changed file against its callers, consumers, and the persistence/IPC layer it touches. Prior-finding disposition (verify, do not re-report)
Required verdict formatEnd with Report structure
Cite file paths and line numbers. If a prior finding is already fixed in the current diff, say so explicitly. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
pr="494"
base="release/4.0.0-alpha.5"
echo "=== PR state, mergeability, and checks ==="
gh pr view "$pr" --repo "$repo" \
--json number,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
--jq '{
number,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,
checks:[.statusCheckRollup[]|{name,status,conclusion}]
}'
echo "=== Remote target and ancestry ==="
git fetch --quiet origin "$base" || true
git ls-remote origin "refs/heads/$base" || true
git rev-parse --verify --quiet "origin/$base" || true
git rev-parse HEAD
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git merge-base --is-ancestor "origin/$base" HEAD && echo "base_is_ancestor=true" || echo "base_is_ancestor=false"
echo "=== Full changed files ==="
git diff --name-status "origin/$base...HEAD"
echo "=== Diff stat ==="
git diff --stat "origin/$base...HEAD"
echo "=== Full patch ==="
git diff --unified=4 "origin/$base...HEAD"
fi
echo "=== Changed source outlines ==="
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --name-only --diff-filter=ACMR "origin/$base...HEAD" |
while IFS= read -r file; do
case "$file" in
*.ts|*.tsx|*.js)
[ -f "$file" ] && { echo "--- $file ---"; ast-grep outline "$file" --items all --view compact || true; }
;;
esac
done
fi
echo "=== Shortcut contracts and implementations ==="
for file in \
src/types/app.d.ts \
src/renderer/src/other/appReducer.tsx \
src/renderer/src/utils/localStorage.ts \
src/renderer/src/hooks/useKeyboardShortcuts.tsx \
src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx \
src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx \
src/renderer/src/other/player.ts \
src/renderer/src/components/SettingsPage/Settings/EqualizerSettings.tsx \
src/renderer/src/other/appShortcuts.ts; do
if [ -f "$file" ]; then
echo "--- $file ---"
nl -ba "$file" | sed -n '1,760p'
fi
done
echo "=== Feature call sites, IPC, and preload changes ==="
rg -n -C3 --glob '*.{ts,tsx,js}' \
'\b(toggleEqualizer|applyEqualizerPreset|getEqualizerState|getKeyboardShortcuts|setKeyboardShortcuts|resetShortcutsToDefaults|keyboardShortcuts)\s*\(' \
src test 2>/dev/null || true
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --unified=4 "origin/$base...HEAD" -- src/main src/preload | sed -n '1,1200p'
fi
echo "=== Editable guards, key normalization, and listener cleanup ==="
rg -n -C3 --glob '*.{ts,tsx}' \
'(isComposing|HTMLInputElement|HTMLTextAreaElement|HTMLSelectElement|isContentEditable|metaKey|Cmd|modifier|addEventListener|removeEventListener)' \
src/renderer/src/hooks/useKeyboardShortcuts.tsx \
src/renderer/src/components/SettingsPage 2>/dev/null || true
echo "=== Tests and feature coverage ==="
fd -HI -t f -E node_modules -E dist -E build '.*\.(test|spec)\.(ts|tsx|js)$' test src 2>/dev/null | sort
rg -n -i -C2 \
'(keyboard shortcut|keyboardShortcuts|toggleEqualizer|applyEqualizerPreset|toggleFullscreenPlayer|Cmd|Meta)' \
test src --glob '*.{test,spec}.{ts,tsx,js}' 2>/dev/null || true
echo "=== Security-relevant changed code and current Electron configuration ==="
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --unified=4 "origin/$base...HEAD" | rg -n '^\+[^+].*(ipcMain|ipcRenderer|contextBridge|shell\.openExternal|loadURL|localStorage|innerHTML|webPreferences)' || true
fi
rg -n -C4 --glob '*.{ts,js}' \
'(new BrowserWindow|webPreferences|contextIsolation|nodeIntegration|sandbox|setWindowOpenHandler|shell\.openExternal)' \
src/main src/preload 2>/dev/null || trueLength of output: 4843 🏁 Script executed: #!/bin/bash
set -u
repo="Sandakan/Nora"
pr="494"
numbered() {
file="$1"
start="${2:-1}"
end="${3:-9999}"
if [ -f "$file" ]; then
echo "===== $file:$start-$end ====="
awk -v start="$start" -v end="$end" 'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }' "$file"
else
echo "===== MISSING: $file ====="
fi
}
echo "=== Current PR metadata and target comparison ==="
gh pr view "$pr" --repo "$repo" \
--json baseRefOid,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
--jq '{baseRefOid,headRefOid,mergeable,mergeStateStatus,checks:[.statusCheckRollup[]|{name,status,conclusion}]}'
gh api "repos/$repo/branches/release/4.0.0-alpha.5" --jq '.commit.sha'
gh api "repos/$repo/compare/9d28f5f4a13c853ad40f84de658c571603ac7f46...18aa0fff952aeb9baf5559565c8584c07a1d6e64" \
--jq '{status,ahead_by,behind_by,merge_base_commit:.merge_base_commit.sha}'
echo "=== Current PR changed files ==="
gh api --paginate "repos/$repo/pulls/$pr/files?per_page=100" \
--jq '.[] | [.filename,.status,.additions,.deletions] | `@tsv`'
echo "=== Current PR patches ==="
gh api --paginate "repos/$repo/pulls/$pr/files?per_page=100" \
--jq '.[] | "===== \(.filename) =====\n\(.patch // "[No textual patch available]")"'
echo "=== Current shortcut-related source ==="
numbered src/types/app.d.ts 1 180
numbered src/renderer/src/other/appReducer.tsx 480 700
numbered src/renderer/src/utils/localStorage.ts 250 420
numbered src/renderer/src/hooks/useKeyboardShortcuts.tsx 100 390
numbered src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx 1 260
numbered src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx 1 250
numbered src/renderer/src/other/player.ts 600 710
echo "=== All current feature call sites ==="
rg -n -C3 --glob '*.{ts,tsx,js}' \
'\b(toggleEqualizer|applyEqualizerPreset|getEqualizerState|getKeyboardShortcuts|setKeyboardShortcuts|resetShortcutsToDefaults|keyboardShortcuts)\s*\(' \
src test 2>/dev/null || true
echo "=== Current test inventory and feature tests ==="
fd -HI -t f -E node_modules -E dist -E build '.*\.(test|spec)\.(ts|tsx|js)$' test src 2>/dev/null | sort || true
rg -n -i -C2 \
'(keyboard shortcut|keyboardShortcuts|toggleEqualizer|applyEqualizerPreset|toggleFullscreenPlayer|Cmd|Meta)' \
test src --glob '*.{test,spec}.{ts,tsx,js}' 2>/dev/null || true
echo "=== Current IPC/preload diff indicators ==="
rg -n -C3 --glob '*.{ts,tsx,js}' \
'(ipcMain\.(handle|on)|ipcRenderer\.(invoke|send)|contextBridge\.exposeInMainWorld)' \
src/main src/preload 2>/dev/null || trueLength of output: 216008
VerdictNot Ready. Confidence: high. The branch is not MERGEABLE with GitHub currently reports:
The branch is 0 commits behind its recorded base. The recorded base is not the current target tip. CI pass status is not available. GitGuardian passed. No completed lint, typecheck, build, or test check is reported. I cannot confirm that lint, typecheck, or tests would pass. Major findings
Minor findings
Missing test coverageNo test covers the keyboard shortcut feature or the equalizer shortcut path. Add tests for:
What is working correctly
Verdict: Not Ready — high confidence.You are interacting with an AI system. |
… merge upstream release CR Major 1: ShortcutCategory had no stable id; the migration matched by translated shortcutCategoryTitle, so a language change could orphan user categories and append duplicates. Added id to ShortcutCategory and all 6 default categories (mediaPlayback/navigation/selections/lyrics/ lyricsEditor/otherShortcuts). getKeyboardShortcuts matches by id first with a legacy title fallback; extras filter excludes both id and title matches. CR Major 2: toggleEqualizer read storage.getLocalStorage().equalizerPreset (legacy) while the settings UI writes the DB-backed preset. Player now caches the last applied preset (fed by the DB-backed settings hook) and toggleEqualizer re-applies the cache; no legacy localStorage read. CR Major 3: branch was 'conflicting' because the fork release ref was stale. Merged the actual upstream release tip (838316c). CR Minor 1: KeyboardShortcutsSettings accepted modifier-only shortcuts. Added the same keys.length === 0 guard as AppShortcutsPrompt. CR Minor 2: click-outside effect used t() but omitted it from deps. Added t. CR Minor 3: removed dead !userCategoryTitles.has() condition in the category-extra filter. 336 tests pass, typecheck clean.
|
@CodeRabbit You are performing a pre-release production audit of this PR against the target branch ScopeReview the FULL diff of this PR, not just the headline files. Cross-check every changed file against its callers, consumers, and the persistence/IPC layer it touches. Include nitpick-level findings too — small nits, naming, dead code, minor edge cases all count. Prior-finding disposition (verify, do not re-report)
Required verdict formatEnd with Coverage checklist
Report structure
Cite file paths and line numbers. If a prior finding is already fixed, say so explicitly. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
pr="494"
base="release/4.0.0-alpha.5"
echo "=== PR state and CI ==="
gh pr view "$pr" --repo "$repo" \
--json number,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
--jq '{
number, state, baseRefName, baseRefOid, headRefName, headRefOid, mergeable, mergeStateStatus,
checks: [.statusCheckRollup[] | {name, status, conclusion, detailsUrl}]
}'
echo "=== Upstream target and PR ancestry ==="
gh api "repos/${repo}/branches/${base}" --jq '.commit.sha'
head_sha="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')"
base_sha="$(gh api "repos/${repo}/branches/${base}" --jq '.commit.sha')"
gh api "repos/${repo}/compare/${base_sha}...${head_sha}" \
--jq '{status, ahead_by, behind_by, merge_base_commit: .merge_base_commit.sha}'
git fetch --quiet origin "$base" || true
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
echo "=== Changed files against current base ==="
git diff --name-status "origin/$base...HEAD"
echo "=== Diff stat ==="
git diff --stat "origin/$base...HEAD"
echo "=== Full diff ==="
git diff --unified=5 "origin/$base...HEAD"
fi
echo "=== GitHub PR changed-file patches ==="
gh api --paginate "repos/${repo}/pulls/${pr}/files?per_page=100" \
--jq '.[] | "===== \(.filename) =====\n\(.patch // "[No textual patch available]")"'
echo "=== Current implementation with line numbers ==="
for file in \
src/types/app.d.ts \
src/renderer/src/other/appReducer.tsx \
src/renderer/src/utils/localStorage.ts \
src/renderer/src/hooks/useKeyboardShortcuts.tsx \
src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx \
src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx \
src/renderer/src/other/player.ts \
src/renderer/src/components/SettingsPage/Settings/EqualizerSettings.tsx \
src/renderer/src/other/appShortcuts.ts \
src/main/ipc.ts \
src/preload/index.ts
do
if [ -f "$file" ]; then
echo "===== $file ====="
nl -ba "$file" | sed -n '1,820p'
fi
done
echo "=== Feature call sites, persistence, and key normalization ==="
rg -n -C4 --glob '*.{ts,tsx,js}' \
'\b(toggleEqualizer|applyEqualizerPreset|getEqualizerState|getKeyboardShortcuts|setKeyboardShortcuts|resetShortcutsToDefaults|keyboardShortcuts|equalizerPreset|lastAppliedEqualizerPreset)\s*\(' \
src test 2>/dev/null || true
echo "=== Event lifecycle and editable-target guards ==="
rg -n -C4 --glob '*.{ts,tsx}' \
'(addEventListener|removeEventListener|isComposing|HTMLInputElement|HTMLTextAreaElement|HTMLSelectElement|isContentEditable|metaKey|Cmd|setHasDuplicate|useEffect)' \
src/renderer/src/hooks/useKeyboardShortcuts.tsx \
src/renderer/src/components/SettingsPage 2>/dev/null || true
echo "=== Test inventory and feature-specific tests ==="
fd -HI -t f -E node_modules -E dist -E build '.*\.(test|spec)\.(ts|tsx|js)$' test src 2>/dev/null | sort || true
rg -n -i -C2 \
'(keyboard shortcut|keyboardShortcuts|toggleEqualizer|applyEqualizerPreset|toggleFullscreenPlayer|Cmd|Meta|ShortcutCategory)' \
test src --glob '*.{test,spec}.{ts,tsx,js}' 2>/dev/null || true
echo "=== New or changed IPC/preload contracts ==="
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --unified=5 "origin/$base...HEAD" -- src/main src/preload
fiLength of output: 51083 🏁 Script executed: #!/bin/bash
set -euo pipefail
show() {
file="$1"
start="$2"
end="$3"
echo "===== ${file}:${start}-${end} ====="
if [ -f "$file" ]; then
awk -v start="$start" -v end="$end" \
'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }' "$file"
else
echo "MISSING FILE"
fi
}
echo "=== Current focused implementations ==="
show src/types/app.d.ts 635 675
show src/renderer/src/utils/localStorage.ts 260 385
show src/renderer/src/hooks/useKeyboardShortcuts.tsx 1 390
show src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx 1 240
show src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx 1 225
show src/renderer/src/other/player.ts 1 100
show src/renderer/src/other/player.ts 610 690
show src/renderer/src/components/SettingsPage/Settings/EqualizerSettings.tsx 1 300
show src/renderer/src/other/appReducer.tsx 440 680
show src/renderer/src/other/appShortcuts.ts 1 180
echo "=== Equalizer data flow: DB settings, player cache, and all callers ==="
rg -n -C5 --glob '*.{ts,tsx}' \
'\b(applyEqualizerPreset|toggleEqualizer|getEqualizerState|updateEqualizerOptions|equalizerPreset|setEqualizerPreset|getEqualizerPreset)\s*\(' \
src test 2>/dev/null || true
echo "=== Shortcut persistence and UI call sites ==="
rg -n -C4 --glob '*.{ts,tsx}' \
'\b(getKeyboardShortcuts|setKeyboardShortcuts|resetShortcutsToDefaults|canonicalizeShortcutId|manageKeyboardShortcuts)\s*\(' \
src test 2>/dev/null || true
echo "=== Function and component map ==="
ast-grep outline src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx --items all --view signatures || true
ast-grep outline src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx --items all --view signatures || true
ast-grep outline src/renderer/src/hooks/useKeyboardShortcuts.tsx --items all --view signatures || true
ast-grep outline src/renderer/src/other/player.ts --match 'AudioPlayer' --items all --view signatures || true
ast-grep outline src/renderer/src/utils/localStorage.ts --items all --view signatures || true
echo "=== Changed test files and feature-test inventory ==="
git diff --name-status origin/release/4.0.0-alpha.5...HEAD -- test src 2>/dev/null || true
fd -HI -t f -E node_modules -E dist -E build '.*\.(test|spec)\.(ts|tsx|js)$' test src 2>/dev/null | sort || true
rg -n -i -C3 \
'(keyboardShortcuts|KeyboardShortcutsSettings|AppShortcutsPrompt|toggleEqualizer|applyEqualizerPreset|toggleFullscreenPlayer|ShortcutCategory)' \
test src --glob '*.{test,spec}.{ts,tsx,js}' 2>/dev/null || true
echo "=== Remaining changed-file patches not shown in the first output ==="
git diff --unified=5 origin/release/4.0.0-alpha.5...HEAD -- \
src/main/other/discord.ts \
src/renderer/src/components/SettingsPage/Settings/StartupSettings.tsx \
src/renderer/src/hooks/useAppLifecycle.tsx \
src/renderer/src/hooks/useDiscordRpc.tsx \
src/renderer/src/components/SettingsPage/SettingsPage.tsx \
src/types/app.d.ts \
2>/dev/null || trueLength of output: 92235
VerdictNot Ready. Confidence: high. The branch is MERGEABLE with GitHub reports Recorded CI is successful for:
No separate completed typecheck check is recorded. I cannot confirm typecheck status from the PR checks. Major findings
Minor findings
Nitpick findings
Missing test coverageNo test covers the new shortcut or equalizer behavior. Add tests for:
What is working correctly
Verdict: Not Ready — high confidence.You are interacting with an AI system. |
…category migration, modifier-only guard Major 1 (DB-backed EQ preset): usePlaybackSettings now receives the AudioPlayer instance and updateEqualizerOptions calls player.applyEqualizerPreset(options) so the player cache is populated and bands are live. Ctrl+E toggleEqualizer now re-applies the real preset instead of a zero-gain cache. App.tsx passes player instead of audio. Major 2 (legacy category migration language change): getKeyboardShortcuts matches legacy categories (no id) by array position when the default category set is in the known order, so a language change cannot orphan a user's categories. Stable id matching remains primary. Minor 1 (modifier-only shortcuts): both editors now reject modifier-only input by tracking whether a non-modifier key was pressed, not by array length. 336 tests pass, typecheck clean.
|
@CodeRabbit You are performing a pre-release production audit of this PR against the target branch ScopeReview the FULL diff of this PR, not just the headline files. Cross-check every changed file against its callers, consumers, and the persistence/IPC layer it touches. Self-review before posting (MANDATORY)Before writing any finding, perform a SECOND pass on your own suggestion:
Required verdict formatEnd your review with a clear line: Coverage checklist — verify EVERY item explicitly
Prior-finding disposition (verify, do not re-report)
Report structure
Be specific: cite file paths and line numbers. Do not accept generic patterns — validate each finding against the actual code. If a prior finding is already fixed in the current diff, say so explicitly instead of re-reporting it. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
pr="494"
base="release/4.0.0-alpha.5"
echo "=== PR merge state and CI ==="
gh pr view "$pr" --repo "$repo" \
--json number,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
--jq '{
number,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,
checks:[.statusCheckRollup[]|{name,status,conclusion,detailsUrl}]
}'
echo "=== Current declared-base comparison ==="
base_sha="$(gh api "repos/${repo}/branches/${base}" --jq '.commit.sha')"
head_sha="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')"
printf 'base_sha=%s\nhead_sha=%s\n' "$base_sha" "$head_sha"
gh api "repos/${repo}/compare/${base_sha}...${head_sha}" \
--jq '{status,ahead_by,behind_by,merge_base_commit:.merge_base_commit.sha}'
git fetch --quiet origin "$base" || true
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
echo "=== Complete changed-file list ==="
git diff --name-status "origin/$base...HEAD"
echo "=== Complete diff stat ==="
git diff --stat "origin/$base...HEAD"
echo "=== Full PR diff against declared base ==="
git diff --unified=5 "origin/$base...HEAD"
fi
echo "=== Changed TypeScript and TSX outlines ==="
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --name-only --diff-filter=ACMR "origin/$base...HEAD" |
while IFS= read -r file; do
case "$file" in
*.ts|*.tsx|*.js)
if [ -f "$file" ]; then
echo "--- $file ---"
ast-grep outline "$file" --items all --view compact || true
fi
;;
esac
done
fi
echo "=== Shortcut types, defaults, migration, dispatch, and editors ==="
for file in \
src/types/app.d.ts \
src/renderer/src/other/appReducer.tsx \
src/renderer/src/utils/localStorage.ts \
src/renderer/src/hooks/useKeyboardShortcuts.tsx \
src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx \
src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx \
src/renderer/src/other/appShortcuts.ts
do
if [ -f "$file" ]; then
echo "--- $file ---"
nl -ba "$file" | sed -n '1,850p'
fi
done
echo "=== Equalizer producer, cache, and call sites ==="
for file in \
src/renderer/src/other/player.ts \
src/renderer/src/components/SettingsPage/Settings/EqualizerSettings.tsx \
src/renderer/src/hooks/usePlaybackSettings.tsx \
src/renderer/src/App.tsx
do
if [ -f "$file" ]; then
echo "--- $file ---"
nl -ba "$file" | sed -n '1,850p'
fi
done
rg -n -C4 --glob '*.{ts,tsx}' \
'\b(applyEqualizerPreset|toggleEqualizer|getEqualizerState|usePlaybackSettings|updateEqualizerOptions|getKeyboardShortcuts|setKeyboardShortcuts|resetShortcutsToDefaults)\s*\(' \
src test 2>/dev/null || true
echo "=== Changed IPC and preload contracts ==="
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --unified=5 "origin/$base...HEAD" -- src/main src/preload || true
fi
rg -n -C3 --glob '*.{ts,tsx,js}' \
'(ipcMain\.(handle|on)|ipcRenderer\.(invoke|send)|contextBridge\.exposeInMainWorld)' \
src/main src/preload 2>/dev/null || true
echo "=== Tests, configuration, and static risk indicators ==="
fd -HI -t f -E node_modules -E dist -E build '.*\.(test|spec)\.(ts|tsx|js)$' test src 2>/dev/null | sort || true
rg -n -i -C2 \
'(keyboardShortcuts|KeyboardShortcutsSettings|AppShortcutsPrompt|toggleEqualizer|applyEqualizerPreset|toggleFullscreenPlayer|ShortcutCategory|usePlaybackSettings)' \
test src --glob '*.{test,spec}.{ts,tsx,js}' 2>/dev/null || true
if [ -f package.json ]; then
jq '{scripts,engines}' package.json
fi
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --unified=2 "origin/$base...HEAD" \
| rg -n '^\+[^+].*(TODO|FIXME|console\.(log|debug)|debugger|`@ts-ignore`|`@ts-expect-error`|\bas any\b|catch\s*\(|addEventListener|removeEventListener|transition-all|innerHTML|localStorage)' \
|| true
fi
echo "=== Current Electron security configuration ==="
rg -n -C4 --glob '*.{ts,js,cjs,mjs}' \
'(new BrowserWindow|webPreferences|contextIsolation|nodeIntegration|sandbox|setWindowOpenHandler|shell\.openExternal|will-navigate|will-redirect)' \
src/main src/preload 2>/dev/null || trueLength of output: 5864 🏁 Script executed: #!/bin/bash
set -u
repo="Sandakan/Nora"
pr="494"
numbered_range() {
file="$1"
start="$2"
end="$3"
echo "===== ${file}:${start}-${end} ====="
if [ -f "$file" ]; then
awk -v start="$start" -v end="$end" \
'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }' "$file"
else
echo "MISSING FILE"
fi
}
echo "=== Complete PR file list and patches ==="
gh api --paginate "repos/${repo}/pulls/${pr}/files?per_page=100" \
--jq '.[] | [.filename,.status,.additions,.deletions] | `@tsv`'
gh api --paginate "repos/${repo}/pulls/${pr}/files?per_page=100" \
--jq '.[] | "===== \(.filename) =====\n\(.patch // "[No textual patch available]")"'
echo "=== Current shortcut model and persistence ==="
numbered_range src/types/app.d.ts 630 700
numbered_range src/renderer/src/other/appReducer.tsx 440 720
numbered_range src/renderer/src/utils/localStorage.ts 250 410
echo "=== Current keyboard event handling and both editors ==="
numbered_range src/renderer/src/hooks/useKeyboardShortcuts.tsx 100 390
numbered_range src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx 1 260
numbered_range src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx 1 240
numbered_range src/renderer/src/other/appShortcuts.ts 1 220
echo "=== Current equalizer data flow ==="
numbered_range src/renderer/src/other/player.ts 600 710
numbered_range src/renderer/src/hooks/usePlaybackSettings.tsx 1 260
numbered_range src/renderer/src/components/SettingsPage/Settings/EqualizerSettings.tsx 1 300
numbered_range src/renderer/src/App.tsx 1 260
rg -n -C4 --glob '*.{ts,tsx}' \
'\b(applyEqualizerPreset|toggleEqualizer|getEqualizerState|usePlaybackSettings|updateEqualizerOptions)\s*\(' \
src test 2>/dev/null || true
echo "=== Changed IPC and preload contracts ==="
rg -n -C3 --glob '*.{ts,tsx,js}' \
'(ipcMain\.(handle|on)|ipcRenderer\.(invoke|send)|contextBridge\.exposeInMainWorld)' \
src/main src/preload 2>/dev/null || true
echo "=== UI lifecycle, focus controls, and static-risk indicators ==="
rg -n -C3 --glob '*.{ts,tsx}' \
'(addEventListener|removeEventListener|isComposing|HTMLInputElement|HTMLTextAreaElement|HTMLSelectElement|isContentEditable|metaKey|Cmd|setHasDuplicate|useEffect|transition-all|aria-|tabIndex)' \
src/renderer/src/hooks/useKeyboardShortcuts.tsx \
src/renderer/src/components/SettingsPage 2>/dev/null || true
echo "=== Test inventory and feature coverage ==="
fd -HI -t f -E node_modules -E dist -E build '.*\.(test|spec)\.(ts|tsx|js)$' test src 2>/dev/null | sort || true
rg -n -i -C2 \
'(keyboardShortcuts|KeyboardShortcutsSettings|AppShortcutsPrompt|toggleEqualizer|applyEqualizerPreset|toggleFullscreenPlayer|ShortcutCategory|usePlaybackSettings)' \
test src --glob '*.{test,spec}.{ts,tsx,js}' 2>/dev/null || true
echo "=== Package scripts and typecheck indicators ==="
if [ -f package.json ]; then
jq '{scripts,engines}' package.json
fi
gh pr checks "$pr" --repo "$repo" || true
echo "=== Current Electron security configuration ==="
rg -n -C4 --glob '*.{ts,js,cjs,mjs}' \
'(new BrowserWindow|webPreferences|contextIsolation|nodeIntegration|sandbox|setWindowOpenHandler|shell\.openExternal|will-navigate|will-redirect)' \
src/main src/preload 2>/dev/null || trueLength of output: 230284 🏁 Script executed: #!/bin/bash
set -u
repo="Sandakan/Nora"
base_sha="838316cb5a83359499422937835a837e5fc86053"
show_range() {
file="$1"
start="$2"
end="$3"
echo "===== CURRENT ${file}:${start}-${end} ====="
if [ -f "$file" ]; then
awk -v start="$start" -v end="$end" \
'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }' "$file"
else
echo "MISSING FILE"
fi
}
echo "=== Equalizer context and database contracts ==="
for file in \
src/renderer/src/contexts/AppUpdateContext.tsx \
src/renderer/src/hooks/useUserPreferences.ts \
src/renderer/src/App.tsx \
src/renderer/src/components/SettingsPage/Settings/EqualizerSettings.tsx \
src/renderer/src/other/player.ts
do
show_range "$file" 1 900
done
echo "=== Shortcut UI primitive contracts ==="
for name in Button.tsx ShortcutButton.tsx SensitiveActionConfirmPrompt.tsx; do
fd -HI -t f -E node_modules -E dist -E build "^${name}$" src \
| while IFS= read -r file; do
show_range "$file" 1 360
done
done
echo "=== App context use and all equalizer persistence consumers ==="
rg -n -C4 --glob '*.{ts,tsx}' \
'(updateEqualizerOptions|saveEqualizerPreset|equalizerPreset|frequencyBands|lastAppliedEqualizerPreset|applyEqualizerPreset)' \
src/renderer/src test 2>/dev/null || true
echo "=== Base versions: source of the two candidate defects ==="
for file in \
src/renderer/src/utils/localStorage.ts \
src/renderer/src/components/SettingsPage/Settings/EqualizerSettings.tsx \
src/renderer/src/hooks/usePlaybackSettings.tsx \
src/renderer/src/other/player.ts \
src/renderer/src/App.tsx
do
echo "===== BASE ${file} ====="
gh api "repos/${repo}/contents/${file}?ref=${base_sha}" --jq '.content' 2>/dev/null \
| tr -d '\n' | base64 -d 2>/dev/null \
| awk '{printf "%6d %s\n", NR, $0}' \
| sed -n '1,900p' || true
done
echo "=== Changed-file locations and changed tests ==="
gh api --paginate "repos/${repo}/pulls/494/files?per_page=100" \
--jq '.[] | select(.filename|endswith(".ts") or endswith(".tsx") or endswith(".d.ts")) | "\(.filename)\n\(.patch // "")"' \
| rg -n -C2 '(usePlaybackSettings|Equalizer|keyboardShortcuts|ShortcutCategory|globalThis|DiscordRPCClient|TODO|aria-|tooltipLabel)' || trueLength of output: 184671
VerdictNot Ready. Confidence: high. The branch is MERGEABLE and CLEAN with
I completed a second cause/action/reaction/necessity pass for each finding below. Major findings
Minor findingsNone that survived the required second pass. The following prior findings are fixed in the current code and are not re-reported:
Nitpick findingsNone that require a change before merge. The duplicate editor implementations remain a maintenance risk, but they are not a correctness defect in this PR. UI review findingsNo new UI blocker found by static review.
Static review cannot verify rendered contrast, clipping, focus rendering, HiDPI layout, or screen-reader output. Manual UI QA should cover those states. Missing test coverageNo automated test covers the new shortcut or equalizer behavior. Add tests for:
What is working correctly
Verdict: Not Ready — high confidence.You are interacting with an AI system. |
…ate EQ preset at startup Major 1 (duplicate legacy categories): getKeyboardShortcuts now tracks categories selected during the default-merge via selectedUserCategoryRefs and excludes them from extraUserCategories, so a no-ID legacy category matched by position is not appended a second time. Major 2 (Ctrl+E no DB preset until Settings visited): usePlaybackSettings hydrates AudioPlayer.lastAppliedEqualizerPreset from the database query on startup via an effect, so toggleEqualizer restores the saved non-flat preset without opening Settings. No save is performed (DB stays source of truth). Also restores the as unknown as DiscordRPCClient cast in discord.ts (the branch had a direct cast that failed typecheck:node). Typecheck:node is 0 errors. Lint clean (no new warnings from these changes).
|
@CodeRabbit You are performing a pre-release production audit of this PR against the target branch ScopeReview the FULL diff of this PR, not just the headline files. Cross-check every changed file against its callers, consumers, and the persistence/IPC layer it touches. Self-review before posting (MANDATORY)Before writing any finding, perform a SECOND pass on your own suggestion:
Required verdict formatEnd your review with a clear line: Coverage checklist — verify EVERY item explicitly
Prior-finding disposition (verify, do not re-report)
Report structure
Be specific: cite file paths and line numbers. Do not accept generic patterns — validate each finding against the actual code. If a prior finding is already fixed in the current diff, say so explicitly instead of re-reporting it. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
pr="494"
base="release/4.0.0-alpha.5"
echo "=== PR merge state and CI ==="
gh pr view "$pr" --repo "$repo" \
--json number,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
--jq '{
number,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,
checks:[.statusCheckRollup[]|{name,status,conclusion,detailsUrl}]
}'
echo "=== Current target and ancestry ==="
base_sha="$(gh api "repos/${repo}/branches/${base}" --jq '.commit.sha')"
head_sha="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')"
printf 'base_sha=%s\nhead_sha=%s\n' "$base_sha" "$head_sha"
gh api "repos/${repo}/compare/${base_sha}...${head_sha}" \
--jq '{status,ahead_by,behind_by,merge_base_commit:.merge_base_commit.sha}'
git fetch --quiet origin "$base" || true
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
echo "=== Complete diff file list and statistics ==="
git diff --name-status "origin/$base...HEAD"
git diff --stat "origin/$base...HEAD"
echo "=== Complete current diff ==="
git diff --unified=4 "origin/$base...HEAD"
fi
echo "=== Changed module outlines ==="
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --name-only --diff-filter=ACMR "origin/$base...HEAD" |
while IFS= read -r file; do
case "$file" in
*.ts|*.tsx|*.js)
if [ -f "$file" ]; then
echo "--- $file ---"
ast-grep outline "$file" --items all --view compact || true
fi
;;
esac
done
fi
show() {
file="$1"; start="$2"; end="$3"
echo "===== $file:$start-$end ====="
if [ -f "$file" ]; then
nl -ba "$file" | sed -n "${start},${end}p"
else
echo "MISSING FILE"
fi
}
echo "=== Shortcut model, migration, dispatch, and UI ==="
show src/types/app.d.ts 620 710
show src/renderer/src/other/appReducer.tsx 430 720
show src/renderer/src/utils/localStorage.ts 250 420
show src/renderer/src/hooks/useKeyboardShortcuts.tsx 100 390
show src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx 1 270
show src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx 1 250
show src/renderer/src/other/appShortcuts.ts 1 240
echo "=== Equalizer data flow and lifecycle ==="
show src/renderer/src/other/player.ts 600 720
show src/renderer/src/hooks/usePlaybackSettings.tsx 1 300
show src/renderer/src/components/SettingsPage/Settings/EqualizerSettings.tsx 1 320
show src/renderer/src/App.tsx 1 300
rg -n -C4 --glob '*.{ts,tsx}' \
'\b(applyEqualizerPreset|toggleEqualizer|getEqualizerState|usePlaybackSettings|updateEqualizerOptions|equalizerPreset)\s*\(' \
src test 2>/dev/null || true
echo "=== IPC and preload contracts changed by this PR ==="
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --unified=4 "origin/$base...HEAD" -- src/main src/preload || true
fi
rg -n -C3 --glob '*.{ts,tsx,js}' \
'(ipcMain\.(handle|on)|ipcRenderer\.(invoke|send)|contextBridge\.exposeInMainWorld)' \
src/main src/preload 2>/dev/null || true
echo "=== UI lifecycle, accessibility indicators, and event guards ==="
rg -n -C3 --glob '*.{ts,tsx}' \
'(addEventListener|removeEventListener|isComposing|HTMLInputElement|HTMLTextAreaElement|HTMLSelectElement|isContentEditable|metaKey|Cmd|useEffect|aria-|tabIndex|transition-all)' \
src/renderer/src/hooks/useKeyboardShortcuts.tsx \
src/renderer/src/components/SettingsPage 2>/dev/null || true
echo "=== Tests and static lint-risk indicators ==="
fd -HI -t f -E node_modules -E dist -E build '.*\.(test|spec)\.(ts|tsx|js)$' test src 2>/dev/null | sort || true
rg -n -i -C2 \
'(keyboardShortcuts|KeyboardShortcutsSettings|AppShortcutsPrompt|toggleEqualizer|applyEqualizerPreset|toggleFullscreenPlayer|ShortcutCategory|usePlaybackSettings)' \
test src --glob '*.{test,spec}.{ts,tsx,js}' 2>/dev/null || true
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --unified=2 "origin/$base...HEAD" |
rg -n '^\+[^+].*(TODO|FIXME|console\.(log|debug)|debugger|`@ts-ignore`|`@ts-expect-error`|\bas any\b|catch\s*\(|transition-all|innerHTML)' \
|| true
fiLength of output: 5762 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
pr="494"
show() {
file="$1"; start="$2"; end="$3"
echo "===== ${file}:${start}-${end} ====="
if [ -f "$file" ]; then
awk -v start="$start" -v end="$end" \
'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }' "$file"
else
echo "MISSING FILE"
fi
}
echo "=== Current PR files and complete patches ==="
gh api --paginate "repos/${repo}/pulls/${pr}/files?per_page=100" \
--jq '.[] | [.filename,.status,.additions,.deletions] | `@tsv`'
gh api --paginate "repos/${repo}/pulls/${pr}/files?per_page=100" \
--jq '.[] | "===== \(.filename) =====\n\(.patch // "[No textual patch available]")"'
echo "=== Shortcut model, persistence, dispatch, and editors ==="
show src/types/app.d.ts 620 710
show src/renderer/src/other/appReducer.tsx 430 730
show src/renderer/src/utils/localStorage.ts 250 430
show src/renderer/src/hooks/useKeyboardShortcuts.tsx 100 400
show src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx 1 280
show src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx 1 260
show src/renderer/src/other/appShortcuts.ts 1 250
echo "=== Equalizer persistence and AudioPlayer lifecycle ==="
show src/renderer/src/other/player.ts 600 730
show src/renderer/src/hooks/usePlaybackSettings.tsx 1 340
show src/renderer/src/components/SettingsPage/Settings/EqualizerSettings.tsx 1 340
show src/renderer/src/App.tsx 1 320
rg -n -C4 --glob '*.{ts,tsx}' \
'\b(applyEqualizerPreset|toggleEqualizer|getEqualizerState|usePlaybackSettings|updateEqualizerOptions|equalizerPreset)\s*\(' \
src test 2>/dev/null || true
echo "=== Changed IPC and preload contracts ==="
gh api --paginate "repos/${repo}/pulls/${pr}/files?per_page=100" \
--jq '.[] | select(.filename | startswith("src/main/") or startswith("src/preload/")) | "===== \(.filename) =====\n\(.patch // "[No textual patch available]")"'
rg -n -C3 --glob '*.{ts,tsx,js}' \
'(ipcMain\.(handle|on)|ipcRenderer\.(invoke|send)|contextBridge\.exposeInMainWorld)' \
src/main src/preload 2>/dev/null || true
echo "=== Event guards, cleanup, and accessible controls ==="
rg -n -C3 --glob '*.{ts,tsx}' \
'(addEventListener|removeEventListener|isComposing|HTMLInputElement|HTMLTextAreaElement|HTMLSelectElement|isContentEditable|metaKey|Cmd|useEffect|aria-|tabIndex|transition-all)' \
src/renderer/src/hooks/useKeyboardShortcuts.tsx \
src/renderer/src/components/SettingsPage 2>/dev/null || true
echo "=== Test inventory and feature-specific test coverage ==="
fd -HI -t f -E node_modules -E dist -E build '.*\.(test|spec)\.(ts|tsx|js)$' test src 2>/dev/null | sort || true
rg -n -i -C2 \
'(keyboardShortcuts|KeyboardShortcutsSettings|AppShortcutsPrompt|toggleEqualizer|applyEqualizerPreset|toggleFullscreenPlayer|ShortcutCategory|usePlaybackSettings)' \
test src --glob '*.{test,spec}.{ts,tsx,js}' 2>/dev/null || true
echo "=== Changed-code static risk indicators ==="
gh api --paginate "repos/${repo}/pulls/${pr}/files?per_page=100" \
--jq '.[] | select(.patch != null) | "===== \(.filename) =====\n\(.patch)"' |
rg -n '^\+[^+].*(TODO|FIXME|console\.(log|debug)|debugger|`@ts-ignore`|`@ts-expect-error`|\bas any\b|catch\s*\(|transition-all|innerHTML)' \
|| trueLength of output: 230301 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
base_sha="838316cb5a83359499422937835a837e5fc86053"
show() {
file="$1"; start="$2"; end="$3"
echo "===== CURRENT ${file}:${start}-${end} ====="
if [ -f "$file" ]; then
awk -v start="$start" -v end="$end" \
'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }' "$file"
else
echo "MISSING FILE"
fi
}
echo "=== Preference and context contracts ==="
for name in useUserPreferences.ts useUserPreferences.tsx AppUpdateContext.tsx Button.tsx ShortcutButton.tsx; do
fd -HI -t f -E node_modules -E dist -E build "^${name}$" src |
while IFS= read -r file; do
show "$file" 1 500
done
done
echo "=== Equalizer types, queries, and persistence consumers ==="
rg -n -C5 --glob '*.{ts,tsx}' \
'(interface Equalizer|type Equalizer|EqualizerBandFilters|frequencyBands|saveEqualizerPreset|getUserEqualizerPreset|saveUserEqualizerPreset)' \
src test 2>/dev/null || true
echo "=== Current shortcut migration helpers and persisted-data repair path ==="
rg -n -C5 --glob '*.{ts,tsx}' \
'(getKeyboardShortcuts|setKeyboardShortcuts|checkLocalStorage|addMissingPropsToAnObject|keyboardShortcuts)' \
src/renderer/src test 2>/dev/null || true
echo "=== Base versions of modified shortcut and equalizer modules ==="
for file in \
src/renderer/src/utils/localStorage.ts \
src/renderer/src/hooks/usePlaybackSettings.tsx \
src/renderer/src/other/player.ts \
src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx \
src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx
do
echo "===== BASE ${file} ====="
gh api "repos/${repo}/contents/${file}?ref=${base_sha}" --jq '.content' 2>/dev/null |
tr -d '\n' | base64 -d 2>/dev/null |
awk '{printf "%6d %s\n", NR, $0}' | sed -n '1,500p' || true
done
echo "=== Changed test patch and package scripts ==="
gh api "repos/${repo}/contents/test/src/renderer/src/utils/addMissingPropsToAnObject.test.ts?ref=20bd756a60a595b69b7d3b845729f250c082391e" \
--jq '.content' 2>/dev/null | tr -d '\n' | base64 -d 2>/dev/null |
awk '{printf "%6d %s\n", NR, $0}' || true
if [ -f package.json ]; then
jq '{scripts}' package.json
fiLength of output: 152542
VerdictNot Ready. Confidence: high. The branch is MERGEABLE and CLEAN with
Major findings
Minor findingsNone that survived the required second review pass. Nitpick findings
UI review findingsNo static UI blocker found.
Static review cannot verify rendered contrast, clipping, focus visibility, screen-reader output, or HiDPI behavior. Manual QA should cover these states. Missing test coverageNo automated test covers the new shortcut or equalizer behavior. Add tests for:
What is working correctly
Verdict: Not Ready — high confidence.You are interacting with an AI system. |
…gacy shortcut match, ignore repeat on toggles Major 1: usePlaybackSettings now converts the DB equalizerPreset.frequencyBands array into a keyed Equalizer (using the shared equalizerBandKeys order) before calling player.applyEqualizerPreset, so Ctrl+E restores the saved non-flat preset after restart. Shared equalizerBandKeys exported from equalizerData.ts. Major 2: legacy shortcut categories with no stable id are now matched by positional fallback, and their shortcuts are merged by position in the known default category so a prior language change cannot orphan custom key bindings. Major 3: toggleFullscreenPlayer and toggleEqualizer now ignore repeated keydown events (e.repeat) to prevent oscillated state when a key is held. Nitpick: usePlaybackSettings calls useUserPreferences once and reuses equalizerPreset from that single call. typecheck:node 0 errors, lint clean (pre-existing as any boundary casts untouched, non-defect per CR).
|
@CodeRabbit You are performing a pre-release production audit of this PR against the target branch ScopeReview the FULL diff of this PR, not just the headline files. Cross-check every changed file against its callers, consumers, and the persistence/IPC layer it touches. Self-review before posting (MANDATORY)Before writing any finding, perform a SECOND pass on your own suggestion:
Required verdict formatEnd your review with a clear line: Coverage checklist — verify EVERY item explicitly
Prior-finding disposition (verify, do not re-report)
Report structure
Be specific: cite file paths and line numbers. If a prior finding is already fixed in the current diff, say so explicitly instead of re-reporting it. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
pr="494"
base="release/4.0.0-alpha.5"
echo "=== PR merge state and checks ==="
gh pr view "$pr" --repo "$repo" \
--json number,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
--jq '{
number,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,
checks:[.statusCheckRollup[]|{name,status,conclusion}]
}'
echo "=== Current base/head comparison ==="
base_sha="$(gh api "repos/${repo}/branches/${base}" --jq '.commit.sha')"
head_sha="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')"
printf 'base_sha=%s\nhead_sha=%s\n' "$base_sha" "$head_sha"
gh api "repos/${repo}/compare/${base_sha}...${head_sha}" \
--jq '{status,ahead_by,behind_by,merge_base_commit:.merge_base_commit.sha}'
git fetch --quiet origin "$base" || true
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
echo "=== Full changed-file list and diff stat ==="
git diff --name-status "origin/$base...HEAD"
git diff --stat "origin/$base...HEAD"
echo "=== Full diff ==="
git diff --unified=4 "origin/$base...HEAD"
fi
echo "=== Changed source outlines ==="
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --name-only --diff-filter=ACMR "origin/$base...HEAD" |
while IFS= read -r file; do
case "$file" in
*.ts|*.tsx|*.js)
if [ -f "$file" ]; then
echo "--- $file ---"
ast-grep outline "$file" --items all --view compact || true
fi
;;
esac
done
fi
show() {
file="$1"; start="$2"; end="$3"
echo "===== ${file}:${start}-${end} ====="
if [ -f "$file" ]; then
awk -v start="$start" -v end="$end" \
'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }' "$file"
else
echo "MISSING FILE"
fi
}
echo "=== Shortcut model, migration, dispatch, and editor implementations ==="
show src/types/app.d.ts 620 720
show src/renderer/src/other/appReducer.tsx 430 740
show src/renderer/src/utils/localStorage.ts 250 440
show src/renderer/src/hooks/useKeyboardShortcuts.tsx 100 410
show src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx 1 290
show src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx 1 270
show src/renderer/src/other/appShortcuts.ts 1 280
echo "=== Equalizer data flow ==="
show src/renderer/src/other/equalizerData.ts 1 240
show src/renderer/src/other/player.ts 600 740
show src/renderer/src/hooks/usePlaybackSettings.tsx 1 360
show src/renderer/src/components/SettingsPage/Settings/EqualizerSettings.tsx 1 360
show src/renderer/src/App.tsx 1 330
rg -n -C4 --glob '*.{ts,tsx}' \
'\b(applyEqualizerPreset|toggleEqualizer|getEqualizerState|usePlaybackSettings|updateEqualizerOptions|equalizerPreset|equalizerBandKeys)\s*\(' \
src test 2>/dev/null || true
echo "=== Changed IPC and preload contracts ==="
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --unified=4 "origin/$base...HEAD" -- src/main src/preload || true
fi
rg -n -C3 --glob '*.{ts,tsx,js}' \
'(ipcMain\.(handle|on)|ipcRenderer\.(invoke|send)|contextBridge\.exposeInMainWorld)' \
src/main src/preload 2>/dev/null || true
echo "=== UI/event indicators ==="
rg -n -C3 --glob '*.{ts,tsx}' \
'(addEventListener|removeEventListener|isComposing|HTMLInputElement|HTMLTextAreaElement|HTMLSelectElement|isContentEditable|metaKey|Cmd|useEffect|aria-|tabIndex|transition-all|disabled)' \
src/renderer/src/hooks/useKeyboardShortcuts.tsx \
src/renderer/src/components/SettingsPage 2>/dev/null || true
echo "=== Tests and static lint-risk indicators ==="
fd -HI -t f -E node_modules -E dist -E build '.*\.(test|spec)\.(ts|tsx|js)$' test src 2>/dev/null | sort || true
rg -n -i -C2 \
'(keyboardShortcuts|KeyboardShortcutsSettings|AppShortcutsPrompt|toggleEqualizer|applyEqualizerPreset|toggleFullscreenPlayer|ShortcutCategory|usePlaybackSettings|equalizerBandKeys)' \
test src --glob '*.{test,spec}.{ts,tsx,js}' 2>/dev/null || true
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --unified=2 "origin/$base...HEAD" |
rg -n '^\+[^+].*(TODO|FIXME|console\.(log|debug)|debugger|`@ts-ignore`|`@ts-expect-error`|\bas any\b|catch\s*\(|transition-all|innerHTML)' \
|| true
fi
echo "=== Base comparisons for prior-finding locations ==="
for file in \
src/renderer/src/utils/localStorage.ts \
src/renderer/src/hooks/usePlaybackSettings.tsx \
src/renderer/src/other/player.ts \
src/renderer/src/hooks/useKeyboardShortcuts.tsx
do
echo "--- BASE: $file ---"
git show "origin/$base:$file" 2>/dev/null | nl -ba | sed -n '1,440p' || true
doneLength of output: 192858 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
pr="494"
base_sha="838316cb5a83359499422937835a837e5fc86053"
show() {
file="$1"; start="$2"; end="$3"
echo "===== ${file}:${start}-${end} ====="
if [ -f "$file" ]; then
awk -v start="$start" -v end="$end" \
'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }' "$file"
else
echo "MISSING FILE"
fi
}
echo "=== Complete PR changed-file list and patches ==="
gh api --paginate "repos/${repo}/pulls/${pr}/files?per_page=100" \
--jq '.[] | [.filename,.status,.additions,.deletions] | `@tsv`'
gh api --paginate "repos/${repo}/pulls/${pr}/files?per_page=100" \
--jq '.[] | "===== \(.filename) =====\n\(.patch // "[No textual patch available]")"'
echo "=== Preference, component, and type contracts ==="
for pattern in useUserPreferences.ts useUserPreferences.tsx AppUpdateContext.tsx Button.tsx ShortcutButton.tsx; do
fd -HI -t f -E node_modules -E dist -E build "^${pattern}$" src |
while IFS= read -r file; do show "$file" 1 520; done
done
echo "=== Equalizer and shortcut consumers ==="
rg -n -C4 --glob '*.{ts,tsx}' \
'(useUserPreferences\(|saveEqualizerPreset\(|equalizerPreset|applyEqualizerPreset\(|toggleEqualizer\(|getKeyboardShortcuts\(|setKeyboardShortcuts\(|resetShortcutsToDefaults\()' \
src/renderer/src test 2>/dev/null || true
echo "=== Changed test files and test configuration ==="
fd -HI -t f -E node_modules -E dist -E build '^(vitest\.config\.ts|package\.json)$' . -x sh -c \
'echo "===== $1 ====="; cat "$1"' sh {} \;
git diff --name-status "${base_sha}...HEAD" -- test 2>/dev/null || true
show test/src/renderer/src/utils/addMissingPropsToAnObject.test.ts 1 220
echo "=== Current-vs-base diffs for audit hotspots ==="
for file in \
src/renderer/src/utils/localStorage.ts \
src/renderer/src/hooks/usePlaybackSettings.tsx \
src/renderer/src/other/player.ts \
src/renderer/src/hooks/useKeyboardShortcuts.tsx \
src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx \
src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx \
src/renderer/src/other/equalizerData.ts \
src/types/app.d.ts
do
echo "===== DIFF $file ====="
git diff --unified=5 "${base_sha}...HEAD" -- "$file" || true
done
echo "=== Changed-code diagnostics candidates ==="
git diff --unified=1 "${base_sha}...HEAD" |
rg -n '^\+[^+].*(as any|as unknown as|TODO|FIXME|console\.(warn|error|log)|catch\s*\(|useEffect|useCallback|addEventListener|removeEventListener|setTimeout|setInterval)' \
|| trueLength of output: 170433
VerdictNot Ready. Confidence: medium. The branch is MERGEABLE with
I cannot confirm that lint, typecheck, tests, or packaging will pass until CI completes. Major findingsNo major finding survived the required second review pass. The prior findings are fixed in the current diff:
Minor findings
Nitpick findings
UI review findingsNo static UI blocker was found.
Static inspection cannot verify WCAG contrast, focus visibility, screen-reader output, clipping, or HiDPI layout. Manual QA should test light mode, dark mode, narrow settings width, keyboard-only editing, duplicate combinations, reset confirmation, Cmd shortcuts on macOS, and Ctrl shortcuts on Windows/Linux. Missing test coverageNo automated test covers the new shortcut or equalizer behavior. Add tests for:
What is working correctly
Verdict: Not Ready — medium confidence. The branch is mergeable, but CI is still in progress and the new behavior has no automated test coverage.You are interacting with an AI system. |
…ck-outside The duplicate guard in the click-outside effect returned early before the edit-mode teardown, so a user who entered a duplicate key combo and then clicked away was permanently stuck in edit mode with the edit button disabled for every other shortcut. Now click-outside always tears down edit mode: on duplicate it shows the duplicate notification and reverts (no save), otherwise it saves the new keys and exits. setHasDuplicate is reset in both paths so the crimson highlight clears. typecheck:node 0 errors, lint clean.
|
@CodeRabbit You are performing a pre-release production audit of this PR against the target branch ScopeReview the FULL diff of this PR, not just the headline files. Cross-check every changed file against its callers, consumers, and the persistence/IPC layer it touches. Self-review before posting (MANDATORY)Before writing any finding, perform a SECOND pass on your own suggestion:
Required verdict formatEnd your review with a clear line: Coverage checklist — verify EVERY item explicitly
Prior-finding disposition (verify, do not re-report)
Report structure
Be specific: cite file paths and line numbers. If a prior finding is already fixed in the current diff, say so explicitly instead of re-reporting it. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
pr="494"
base="release/4.0.0-alpha.5"
echo "=== PR merge state and CI ==="
gh pr view "$pr" --repo "$repo" \
--json number,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
--jq '{
number, baseRefName, baseRefOid, headRefName, headRefOid, mergeable, mergeStateStatus,
checks: [.statusCheckRollup[] | {name,status,conclusion}]
}'
echo "=== Current base/head relationship ==="
base_sha="$(gh api "repos/${repo}/branches/${base}" --jq '.commit.sha')"
head_sha="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')"
printf 'base_sha=%s\nhead_sha=%s\n' "$base_sha" "$head_sha"
gh api "repos/${repo}/compare/${base_sha}...${head_sha}" \
--jq '{status,ahead_by,behind_by,merge_base_commit:.merge_base_commit.sha}'
git fetch --quiet origin "$base" || true
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
echo "=== Changed files against target ==="
git diff --name-status "origin/$base...HEAD"
echo "=== Diff stat ==="
git diff --stat "origin/$base...HEAD"
echo "=== Full target-relative diff ==="
git diff --unified=4 "origin/$base...HEAD"
fi
show() {
file="$1"; start="$2"; end="$3"
echo "===== ${file}:${start}-${end} ====="
if [ -f "$file" ]; then
awk -v start="$start" -v end="$end" \
'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }' "$file"
else
echo "MISSING FILE"
fi
}
echo "=== Shortcut persistence, dispatch, and UI ==="
show src/types/app.d.ts 620 730
show src/renderer/src/other/appReducer.tsx 430 760
show src/renderer/src/utils/localStorage.ts 250 460
show src/renderer/src/hooks/useKeyboardShortcuts.tsx 100 420
show src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx 1 300
show src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx 1 280
show src/renderer/src/other/appShortcuts.ts 1 300
echo "=== Equalizer flow ==="
show src/renderer/src/other/equalizerData.ts 1 260
show src/renderer/src/hooks/usePlaybackSettings.tsx 1 380
show src/renderer/src/other/player.ts 600 760
show src/renderer/src/components/SettingsPage/Settings/EqualizerSettings.tsx 1 380
show src/renderer/src/App.tsx 1 340
echo "=== Changed IPC/preload contracts ==="
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --unified=4 "origin/$base...HEAD" -- src/main src/preload || true
fi
echo "=== Event cleanup, accessibility, and transition indicators ==="
rg -n -C3 --glob '*.{ts,tsx}' \
'(addEventListener|removeEventListener|isComposing|HTMLInputElement|HTMLTextAreaElement|HTMLSelectElement|isContentEditable|metaKey|Cmd|useEffect|aria-|tabIndex|transition-all|disabled)' \
src/renderer/src/hooks/useKeyboardShortcuts.tsx \
src/renderer/src/components/SettingsPage 2>/dev/null || true
echo "=== Tests and feature-specific coverage ==="
fd -HI -t f -E node_modules -E dist -E build '.*\.(test|spec)\.(ts|tsx|js)$' test src 2>/dev/null | sort || true
rg -n -i -C2 \
'(keyboardShortcuts|KeyboardShortcutsSettings|AppShortcutsPrompt|toggleEqualizer|applyEqualizerPreset|toggleFullscreenPlayer|ShortcutCategory|usePlaybackSettings|equalizerBandKeys)' \
test src --glob '*.{test,spec}.{ts,tsx,js}' 2>/dev/null || true
echo "=== New static risk indicators ==="
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --unified=2 "origin/$base...HEAD" |
rg -n '^\+[^+].*(TODO|FIXME|console\.(log|debug)|debugger|`@ts-ignore`|`@ts-expect-error`|\bas any\b|catch\s*\(|transition-all|innerHTML)' \
|| true
fiLength of output: 128747 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
pr="494"
echo "=== Complete changed-file list ==="
gh api --paginate "repos/${repo}/pulls/${pr}/files?per_page=100" \
--jq '.[] | [.filename, .status, .additions, .deletions] | `@tsv`'
echo "=== Complete current PR patches ==="
gh api --paginate "repos/${repo}/pulls/${pr}/files?per_page=100" \
--jq '.[] | "===== \(.filename) =====\n\(.patch // "[No textual patch available]")"'
echo "=== Changed renderer, main, and type modules in current form ==="
for f in \
src/main/core/sendAudioData.ts \
src/main/ipc.ts \
src/main/other/discord.ts \
src/renderer/src/components/SettingsPage/Settings/StartupSettings.tsx \
src/renderer/src/components/SettingsPage/SettingsPage.tsx \
src/renderer/src/hooks/useAppLifecycle.tsx \
src/renderer/src/hooks/useDiscordRpc.tsx \
src/renderer/src/other/player.ts \
src/renderer/src/utils/localStorage.ts \
src/types/app.d.ts
do
if [ -f "$f" ]; then
echo "===== $f ====="
nl -ba "$f" | sed -n '1,900p'
fi
done
echo "=== IPC handlers and preload bridge call sites ==="
rg -n -C3 --glob '*.{ts,tsx,js}' \
'(ipcMain\.(handle|on)|ipcRenderer\.(invoke|send)|contextBridge\.exposeInMainWorld|toggleAutoLaunch|toggleMiniPlayerAlwaysOnTop)' \
src/main src/preload src/renderer 2>/dev/null || true
echo "=== AppShortcutsPrompt reachability ==="
rg -n -C3 --glob '*.{ts,tsx}' \
'(AppShortcutsPrompt|openAppShortcutsPrompt|changePromptMenuData)' \
src/renderer/src 2>/dev/null || true
echo "=== Current CI state ==="
gh pr checks "$pr" --repo "$repo" || trueLength of output: 54854
VerdictNot Ready. Confidence: medium. The branch is MERGEABLE with
I cannot confirm that lint, typecheck, tests, or packaging will pass until CI completes. Major findingsNone found. The previous equalizer hydration, legacy migration, Cmd dispatch, repeat-key, modifier-only, and settings-page duplicate-exit findings are fixed in the current code. Minor findings
Nitpick findings
UI review findingsNo new static visual blocker found.
Manual QA should cover the prompt duplicate path, settings-page duplicate path, keyboard-only editing, reset confirmation, narrow settings widths, dark mode, macOS Cmd shortcuts, and Windows/Linux Ctrl shortcuts. Missing test coverageNo automated test covers the new shortcut or equalizer behavior. Add tests for:
What is working correctly
Verdict: Not Ready — medium confidence. The branch is mergeable, but
|
|
@CodeRabbit You are performing a pre-release production audit of this PR against the target branch ScopeReview the FULL diff of this PR, not just the headline files. Cross-check every changed file against its callers, consumers, and the persistence/IPC layer it touches. Self-review before posting (MANDATORY)Before writing any finding, perform a SECOND pass on your own suggestion:
Required verdict formatEnd your review with a clear line: Coverage checklist — verify EVERY item explicitly
Report structure
Be specific: cite file paths and line numbers. If a prior finding is already fixed in the current diff, say so explicitly instead of re-reporting it. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
pr="494"
base="release/4.0.0-alpha.5"
echo "=== PR state, target relationship, and CI ==="
gh pr view "$pr" --repo "$repo" \
--json number,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
--jq '{
number, state, baseRefName, baseRefOid, headRefName, headRefOid,
mergeable, mergeStateStatus,
checks: [.statusCheckRollup[] | {name,status,conclusion,detailsUrl}]
}'
base_sha="$(gh api "repos/${repo}/branches/${base}" --jq '.commit.sha')"
head_sha="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')"
printf 'target_sha=%s\nhead_sha=%s\n' "$base_sha" "$head_sha"
gh api "repos/${repo}/compare/${base_sha}...${head_sha}" \
--jq '{status,ahead_by,behind_by,merge_base_commit:.merge_base_commit.sha}'
git fetch --quiet origin "$base" || true
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
echo "=== Complete changed-file list ==="
git diff --name-status "origin/$base...HEAD"
echo "=== Diff statistics ==="
git diff --stat "origin/$base...HEAD"
echo "=== Complete target-relative diff ==="
git diff --unified=4 "origin/$base...HEAD"
fi
echo "=== Changed source module outlines ==="
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --name-only --diff-filter=ACMR "origin/$base...HEAD" |
while IFS= read -r file; do
case "$file" in
*.ts|*.tsx|*.js)
if [ -f "$file" ]; then
echo "--- $file ---"
ast-grep outline "$file" --items all --view compact || true
fi
;;
esac
done
fi
show() {
file="$1"; start="$2"; end="$3"
echo "===== ${file}:${start}-${end} ====="
if [ -f "$file" ]; then
awk -v start="$start" -v end="$end" \
'NR >= start && NR <= end { printf "%6d %s\n", NR, $0 }' "$file"
else
echo "MISSING FILE"
fi
}
echo "=== Shortcut model, persistence, runtime dispatch, and UI ==="
show src/types/app.d.ts 620 760
show src/renderer/src/other/appReducer.tsx 420 780
show src/renderer/src/utils/localStorage.ts 240 480
show src/renderer/src/hooks/useKeyboardShortcuts.tsx 100 430
show src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx 1 320
show src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx 1 300
show src/renderer/src/other/appShortcuts.ts 1 320
echo "=== Equalizer persistence, player lifecycle, and UI ==="
show src/renderer/src/other/equalizerData.ts 1 280
show src/renderer/src/hooks/usePlaybackSettings.tsx 1 400
show src/renderer/src/other/player.ts 600 780
show src/renderer/src/components/SettingsPage/Settings/EqualizerSettings.tsx 1 400
show src/renderer/src/App.tsx 1 360
echo "=== IPC/preload diff and all relevant handlers ==="
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --unified=4 "origin/$base...HEAD" -- src/main src/preload || true
fi
rg -n -C3 --glob '*.{ts,tsx,js}' \
'(ipcMain\.(handle|on)|ipcRenderer\.(invoke|send)|contextBridge\.exposeInMainWorld)' \
src/main src/preload src/renderer 2>/dev/null || true
echo "=== Feature call sites and event lifecycle ==="
rg -n -C3 --glob '*.{ts,tsx}' \
'\b(applyEqualizerPreset|toggleEqualizer|getKeyboardShortcuts|setKeyboardShortcuts|resetShortcutsToDefaults|equalizerBandKeys)\s*\(' \
src test 2>/dev/null || true
rg -n -C3 --glob '*.{ts,tsx}' \
'(addEventListener|removeEventListener|isComposing|HTMLInputElement|HTMLTextAreaElement|HTMLSelectElement|isContentEditable|metaKey|Cmd|Escape|aria-|tabIndex|transition-all)' \
src/renderer/src/hooks/useKeyboardShortcuts.tsx src/renderer/src/components/SettingsPage 2>/dev/null || true
echo "=== Tests, lint-risk indicators, and package scripts ==="
fd -HI -t f -E node_modules -E dist -E build '.*\.(test|spec)\.(ts|tsx|js)$' test src 2>/dev/null | sort || true
rg -n -i -C2 \
'(keyboardShortcuts|KeyboardShortcutsSettings|AppShortcutsPrompt|toggleEqualizer|applyEqualizerPreset|toggleFullscreenPlayer|ShortcutCategory|equalizerBandKeys)' \
test src --glob '*.{test,spec}.{ts,tsx,js}' 2>/dev/null || true
if [ -f package.json ]; then
jq '{scripts,engines}' package.json
fi
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --unified=2 "origin/$base...HEAD" |
rg -n '^\+[^+].*(TODO|FIXME|console\.(log|debug)|debugger|`@ts-ignore`|`@ts-expect-error`|\bas any\b|catch\s*\(|transition-all|innerHTML)' \
|| true
fiLength of output: 191269 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
pr="494"
echo "=== GitHub changed-file list ==="
gh api --paginate "repos/${repo}/pulls/${pr}/files?per_page=100" \
--jq '.[] | [.filename, .status, .additions, .deletions] | `@tsv`'
echo "=== Complete GitHub patches ==="
gh api --paginate "repos/${repo}/pulls/${pr}/files?per_page=100" \
--jq '.[] | "===== \(.filename) =====\n\(.patch // "[No textual patch available]")"'
echo "=== Current source for the duplicate-edit control flow ==="
nl -ba src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx | sed -n '50,105p'
nl -ba src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx | sed -n '55,105p'
echo "=== Current test coverage for changed behaviors ==="
rg -n -i -C2 \
'(AppShortcutsPrompt|KeyboardShortcutsSettings|keyboardShortcuts|toggleEqualizer|applyEqualizerPreset|toggleFullscreenPlayer|usePlaybackSettings|equalizerBandKeys)' \
test src --glob '*.{test,spec}.{ts,tsx,js}' || true
echo "=== Exact CI check state ==="
gh pr checks "$pr" --repo "$repo" || trueLength of output: 54290
VerdictNot Ready. Confidence: medium. The branch is MERGEABLE and CLEAN with
I reviewed the full current PR file set. I also performed the required cause, action, reaction, and necessity check for the finding below. Major findingsNone. Minor findings
Nitpick findings
UI review findingsNo static visual blocker was found.
Static review cannot verify rendered contrast, focus visibility, clipping, HiDPI rendering, or screen-reader output. Manual QA should test the duplicate-edit path in both editors, keyboard-only operation, narrow layouts, dark mode, macOS Cmd combinations, and Windows/Linux Ctrl combinations. Missing test coverageNo automated test covers the new shortcut and equalizer behavior. Add tests for:
What is working correctly
Verdict: Not Ready — medium confidence. The branch is mergeable and the recorded lint and test checks pass, but
|
…away CR Minor (prompt editor cannot exit after a duplicate shortcut is entered): - The duplicate-notification branch returned before the clickedOutside branch, so after entering a duplicate combination a click outside never reached setEditingShortcut(null) and the editor stayed in edit mode (all other buttons disabled). Moved the duplicate notification inside the clickedOutside branch and unconditionally clear editingShortcut on click-away (duplicate rejection is preserved; only the click-away exit path is restored). typecheck (renderer) clean, lint clean.
|
@CodeRabbit You are performing a pre-release production audit of this PR against the target branch ScopeReview the FULL diff of this PR, not just the headline files. Cross-check every changed file against its callers, consumers, and the persistence/IPC layer it touches. Self-review before posting (MANDATORY)Before writing any finding, perform a SECOND pass on your own suggestion:
Required verdict formatEnd your review with a clear line: Coverage checklist — verify EVERY item explicitly
Report structure
Be specific: cite file paths and line numbers. If a prior finding is already fixed in the current diff, say so explicitly instead of re-reporting it. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
pr="494"
base="release/4.0.0-alpha.5"
echo "=== PR state and CI ==="
gh pr view "$pr" --repo "$repo" \
--json number,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
--jq '{
number,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,
checks:[.statusCheckRollup[]|{name,status,conclusion,detailsUrl}]
}'
echo "=== Target relationship ==="
base_sha="$(gh api "repos/${repo}/branches/${base}" --jq '.commit.sha')"
head_sha="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')"
printf 'target_sha=%s\nhead_sha=%s\n' "$base_sha" "$head_sha"
gh api "repos/${repo}/compare/${base_sha}...${head_sha}" \
--jq '{status,ahead_by,behind_by,merge_base_commit:.merge_base_commit.sha}'
git fetch --quiet origin "$base" || true
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
echo "=== Changed files and complete target-relative diff ==="
git diff --name-status "origin/$base...HEAD"
git diff --stat "origin/$base...HEAD"
git diff --unified=4 "origin/$base...HEAD"
fi
echo "=== Changed module outlines ==="
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --name-only --diff-filter=ACMR "origin/$base...HEAD" |
while IFS= read -r file; do
case "$file" in
*.ts|*.tsx|*.js)
[ -f "$file" ] && { echo "--- $file ---"; ast-grep outline "$file" --items all --view compact || true; }
;;
esac
done
fi
show() {
file="$1"; start="$2"; end="$3"
echo "===== ${file}:${start}-${end} ====="
[ -f "$file" ] && awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%6d %s\n",NR,$0}' "$file" || true
}
echo "=== Keyboard model, migration, dispatch, and UI ==="
show src/types/app.d.ts 620 780
show src/renderer/src/other/appReducer.tsx 420 800
show src/renderer/src/utils/localStorage.ts 240 500
show src/renderer/src/hooks/useKeyboardShortcuts.tsx 100 440
show src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx 1 330
show src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx 1 320
show src/renderer/src/other/appShortcuts.ts 1 340
echo "=== Equalizer data flow ==="
show src/renderer/src/other/equalizerData.ts 1 300
show src/renderer/src/hooks/usePlaybackSettings.tsx 1 420
show src/renderer/src/other/player.ts 600 800
show src/renderer/src/components/SettingsPage/Settings/EqualizerSettings.tsx 1 420
show src/renderer/src/App.tsx 1 380
echo "=== Changed IPC/preload contracts and all feature consumers ==="
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --unified=4 "origin/$base...HEAD" -- src/main src/preload || true
fi
rg -n -C3 --glob '*.{ts,tsx,js}' \
'(ipcMain\.(handle|on)|ipcRenderer\.(invoke|send)|contextBridge\.exposeInMainWorld|applyEqualizerPreset|toggleEqualizer|getKeyboardShortcuts|setKeyboardShortcuts|resetShortcutsToDefaults|equalizerBandKeys)' \
src test 2>/dev/null || true
echo "=== UI/event lifecycle and accessibility indicators ==="
rg -n -C3 --glob '*.{ts,tsx}' \
'(addEventListener|removeEventListener|isComposing|HTMLInputElement|HTMLTextAreaElement|HTMLSelectElement|isContentEditable|metaKey|Cmd|Escape|aria-|tabIndex|transition-all|disabled)' \
src/renderer/src/hooks/useKeyboardShortcuts.tsx src/renderer/src/components/SettingsPage 2>/dev/null || true
echo "=== Tests, scripts, and changed-code risk indicators ==="
fd -HI -t f -E node_modules -E dist -E build '.*\.(test|spec)\.(ts|tsx|js)$' test src 2>/dev/null | sort || true
rg -n -i -C2 \
'(keyboardShortcuts|KeyboardShortcutsSettings|AppShortcutsPrompt|toggleEqualizer|applyEqualizerPreset|toggleFullscreenPlayer|ShortcutCategory|equalizerBandKeys)' \
test src --glob '*.{test,spec}.{ts,tsx,js}' 2>/dev/null || true
jq '{scripts,engines}' package.json 2>/dev/null || true
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --unified=2 "origin/$base...HEAD" |
rg -n '^\+[^+].*(TODO|FIXME|console\.(log|debug)|debugger|`@ts-ignore`|`@ts-expect-error`|\bas any\b|catch\s*\(|transition-all|innerHTML)' || true
fiLength of output: 200965 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
pr="494"
echo "=== Current PR file list and complete patches ==="
gh api --paginate "repos/${repo}/pulls/${pr}/files?per_page=100" \
--jq '.[] | [.filename,.status,.additions,.deletions] | `@tsv`'
gh api --paginate "repos/${repo}/pulls/${pr}/files?per_page=100" \
--jq '.[] | "===== \(.filename) =====\n\(.patch // "[No textual patch available]")"'
echo "=== Equalizer preference contract ==="
for pattern in useUserPreferences.ts useUserPreferences.tsx AppUpdateContext.tsx; do
fd -HI -t f -E node_modules -E dist -E build "^${pattern}$" src |
while IFS= read -r file; do
echo "===== $file ====="
nl -ba "$file" | sed -n '1,520p'
done
done
echo "=== Equalizer types and database call sites ==="
rg -n -C4 --glob '*.{ts,tsx}' \
'(frequencyBands|saveEqualizerPreset|updateEqualizerOptions|EqualizerBandFilters|type Equalizer|interface Equalizer)' \
src test 2>/dev/null || true
echo "=== Latest prompt and settings interaction paths ==="
nl -ba src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx | sed -n '15,115p'
nl -ba src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx | sed -n '15,105p'
echo "=== Current checks and workflow coverage ==="
gh pr checks "$pr" --repo "$repo" || true
fd -HI -t f -E node_modules -E dist -E build '.*\.(ya?ml)$' .github 2>/dev/null \
| while IFS= read -r file; do
echo "===== $file ====="
rg -n -C2 '(lint|test|typecheck|check-types|build)' "$file" || true
doneLength of output: 55626 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "=== User-preference equalizer contract ==="
prefs="$(fd -HI -t f -E node_modules -E dist -E build '^useUserPreferences\.tsx?$' src | head -n 1 || true)"
if [ -n "$prefs" ]; then
echo "===== $prefs ====="
sed -n '1,520p' "$prefs"
fi
echo "=== Shared button accessibility contract ==="
button="$(fd -HI -t f -E node_modules -E dist -E build '^Button\.tsx$' src | head -n 1 || true)"
if [ -n "$button" ]; then
echo "===== $button ====="
sed -n '1,360p' "$button"
fi
echo "=== Existing feature tests, including filename and source references ==="
fd -HI -t f -E node_modules -E dist -E build '.*\.(test|spec)\.(ts|tsx|js)$' test src 2>/dev/null | sort
rg -n -i -C2 \
'(keyboardShortcuts|KeyboardShortcutsSettings|AppShortcutsPrompt|toggleEqualizer|applyEqualizerPreset|toggleFullscreenPlayer|equalizerBandKeys)' \
test src --glob '*.{test,spec}.{ts,tsx,js}' 2>/dev/null || trueLength of output: 13322
VerdictNot Ready. Confidence: medium. The branch is MERGEABLE and CLEAN with
I cannot confirm that Major findingsNone found after the required second-pass validation. The current PR does not add or modify an IPC or preload contract. The target-relative Minor findingsNone found that require a source change before merge. The prior duplicate-edit finding is resolved:
Nitpick findings
UI review findingsNo static UI blocker found.
Static inspection cannot verify WCAG contrast, focus rendering, screen-reader announcements, clipping, or HiDPI behavior. Manual QA must cover dark mode, narrow settings width, keyboard-only operation, macOS Cmd combinations, and Windows/Linux Ctrl combinations. Missing test coverageNo automated test covers the new shortcut or equalizer behavior. Add tests for:
What is working correctly
Verdict: Not Ready — medium confidence. The branch is mergeable, and the recorded lint and test checks pass. Release readiness still requires completed typecheck and build checks, plus automated coverage for the new shortcut and equalizer paths.You are interacting with an AI system. |
|
@CodeRabbit Re-reviewing after the latest push. Summary of current state for this PR:
Required verdict formatEnd with Self-review before posting (MANDATORY)For every finding: (1) Cause - in THIS diff or pre-existing on base? (2) Action - compiles/matches types? (3) Reaction - what breaks? (4) Necessity - real bug or style? Only post if all four hold. If a prior-round finding is already fixed in the current diff, state it explicitly instead of re-reporting. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
pr="494"
base="release/4.0.0-alpha.5"
echo "=== PR state and CI checks ==="
gh pr view "$pr" --repo "$repo" \
--json number,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
--jq '{
number,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,
checks:[.statusCheckRollup[]|{name,status,conclusion,detailsUrl}]
}'
echo "=== Current target relationship ==="
base_sha="$(gh api "repos/${repo}/branches/${base}" --jq '.commit.sha')"
head_sha="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')"
printf 'target_sha=%s\nhead_sha=%s\n' "$base_sha" "$head_sha"
gh api "repos/${repo}/compare/${base_sha}...${head_sha}" \
--jq '{status,ahead_by,behind_by,merge_base_commit:.merge_base_commit.sha}'
git fetch --quiet origin "$base" || true
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
echo "=== Full target-relative changed files ==="
git diff --name-status "origin/$base...HEAD"
echo "=== Full target-relative diff stat ==="
git diff --stat "origin/$base...HEAD"
echo "=== Full target-relative patch ==="
git diff --unified=4 "origin/$base...HEAD"
fi
echo "=== Changed source outlines ==="
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --name-only --diff-filter=ACMR "origin/$base...HEAD" |
while IFS= read -r file; do
case "$file" in
*.ts|*.tsx|*.js)
[ -f "$file" ] || continue
echo "--- $file ---"
ast-grep outline "$file" --items all --view compact || true
;;
esac
done
fi
show() {
file="$1"; start="$2"; end="$3"
echo "===== ${file}:${start}-${end} ====="
if [ -f "$file" ]; then
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%6d %s\n",NR,$0}' "$file"
else
echo "MISSING FILE"
fi
}
echo "=== Current keyboard shortcut and equalizer implementations ==="
show src/types/app.d.ts 620 790
show src/renderer/src/other/appReducer.tsx 420 820
show src/renderer/src/utils/localStorage.ts 240 520
show src/renderer/src/hooks/useKeyboardShortcuts.tsx 100 460
show src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx 1 340
show src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx 1 330
show src/renderer/src/other/equalizerData.ts 1 320
show src/renderer/src/hooks/usePlaybackSettings.tsx 1 440
show src/renderer/src/other/player.ts 600 820
show src/renderer/src/components/SettingsPage/Settings/EqualizerSettings.tsx 1 440
show src/renderer/src/App.tsx 1 400
echo "=== Feature call sites and event lifecycle ==="
rg -n -C3 --glob '*.{ts,tsx}' \
'\b(applyEqualizerPreset|toggleEqualizer|getKeyboardShortcuts|setKeyboardShortcuts|resetShortcutsToDefaults|equalizerBandKeys|canonicalizeShortcutId)\s*\(' \
src test 2>/dev/null || true
rg -n -C3 --glob '*.{ts,tsx}' \
'(addEventListener|removeEventListener|isComposing|HTMLInputElement|HTMLTextAreaElement|HTMLSelectElement|isContentEditable|metaKey|Cmd|e\.repeat|Escape)' \
src/renderer/src/hooks/useKeyboardShortcuts.tsx src/renderer/src/components/SettingsPage 2>/dev/null || true
echo "=== Current workflow and package scripts ==="
if [ -f .github/workflows/lint.yml ]; then
nl -ba .github/workflows/lint.yml
fi
if [ -f package.json ]; then
jq '{scripts,engines}' package.json
fi
echo "=== IPC and preload changes relative to target ==="
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --unified=4 "origin/$base...HEAD" -- src/main src/preload || true
fi
echo "=== Current tests that cover feature behavior ==="
fd -HI -t f -E node_modules -E dist -E build '.*\.(test|spec)\.(ts|tsx|js)$' test src 2>/dev/null | sort || true
rg -n -i -C2 \
'(keyboardShortcuts|KeyboardShortcutsSettings|AppShortcutsPrompt|toggleEqualizer|applyEqualizerPreset|toggleFullscreenPlayer|equalizerBandKeys|usePlaybackSettings)' \
test src --glob '*.{test,spec}.{ts,tsx,js}' 2>/dev/null || trueLength of output: 132767 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
pr="494"
base="release/4.0.0-alpha.5"
echo "=== Workflow that runs lint and typecheck ==="
if [ -f .github/workflows/lint.yml ]; then
sed -n '1,280p' .github/workflows/lint.yml
else
gh api "repos/${repo}/contents/.github/workflows/lint.yml?ref=$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')" \
--jq '.content' | tr -d '\n' | base64 -d
fi
echo "=== Package scripts ==="
if [ -f package.json ]; then
jq '{scripts,engines}' package.json
fi
echo "=== Current check-run detail ==="
gh api --paginate "repos/${repo}/commits/$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')/check-runs?per_page=100" \
--jq '.check_runs[] | {name,status,conclusion,details_url,started_at,completed_at}'
echo "=== Current PR changed files and textual patches ==="
gh api --paginate "repos/${repo}/pulls/${pr}/files?per_page=100" \
--jq '.[] | [.filename,.status,.additions,.deletions] | `@tsv`'
gh api --paginate "repos/${repo}/pulls/${pr}/files?per_page=100" \
--jq '.[] | "===== \(.filename) =====\n\(.patch // "[No textual patch available]")"'
echo "=== Current test inventory and feature references ==="
fd -HI -t f -E node_modules -E dist -E build '.*\.(test|spec)\.(ts|tsx|js)$' test src 2>/dev/null | sort || true
rg -n -i -C2 \
'(keyboardShortcuts|KeyboardShortcutsSettings|AppShortcutsPrompt|toggleEqualizer|applyEqualizerPreset|toggleFullscreenPlayer|equalizerBandKeys|usePlaybackSettings)' \
test src --glob '*.{test,spec}.{ts,tsx,js}' 2>/dev/null || true
echo "=== Target-relative main/preload change check ==="
base_sha="$(gh api "repos/${repo}/branches/${base}" --jq '.commit.sha')"
head_sha="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')"
gh api "repos/${repo}/compare/${base_sha}...${head_sha}" \
--jq '.files[] | select(.filename | startswith("src/main/") or startswith("src/preload/")) | {filename,status,patch}'Length of output: 60772
VerdictNot Ready. Confidence: high. The branch is MERGEABLE and CLEAN with
Major findingsNone found. The prior code-level findings are resolved in the current source:
Minor findings
Nitpick findings
UI review findingsNo static UI blocker found.
Manual QA must still verify dark mode, narrow widths, focus visibility, screen-reader announcements, macOS Cmd behavior, and Windows/Linux Ctrl behavior. Missing test coverageThe completed test check does not include feature tests for this PR. No test references were found for:
What is working correctly
Verdict: Not Ready — high confidence. The branch is mergeable and the recorded lint and test checks pass, but the current CI workflow does not execute the claimed required
|
CR requires a TypeScript validation command in CI. Adds a dedicated Typecheck (node) job running 'npm run typecheck:node' so type safety is verified on every PR run. typecheck:web is intentionally excluded because it carries 7 pre-existing errors unrelated to this PR.
|
@CodeRabbit Final re-review after the latest push. All prior code findings are resolved and verified locally ( Please re-run the full audit against the current head with the CI typecheck result available and confirm the branch is Ready, or list any remaining Major/Minor finding with file:line evidence. Required verdict formatEnd with Self-review before posting (MANDATORY)For every finding: (1) Cause - in THIS diff or pre-existing on base? (2) Action - compiles/matches types? (3) Reaction - what breaks? (4) Necessity - real bug or style? Only post if all four hold. If a prior-round finding is already fixed in the current diff, state it explicitly instead of re-reporting. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Sandakan/Nora"
pr="494"
base="release/4.0.0-alpha.5"
echo "=== PR merge state and CI ==="
gh pr view "$pr" --repo "$repo" \
--json number,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,statusCheckRollup \
--jq '{
number,state,baseRefName,baseRefOid,headRefName,headRefOid,mergeable,mergeStateStatus,
checks:[.statusCheckRollup[]|{name,status,conclusion,detailsUrl}]
}'
echo "=== Target ancestry ==="
base_sha="$(gh api "repos/${repo}/branches/${base}" --jq '.commit.sha')"
head_sha="$(gh pr view "$pr" --repo "$repo" --json headRefOid --jq '.headRefOid')"
printf 'target_sha=%s\nhead_sha=%s\n' "$base_sha" "$head_sha"
gh api "repos/${repo}/compare/${base_sha}...${head_sha}" \
--jq '{status,ahead_by,behind_by,merge_base_commit:.merge_base_commit.sha}'
git fetch --quiet origin "$base" || true
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
echo "=== Target-relative changed files and diff stat ==="
git diff --name-status "origin/$base...HEAD"
git diff --stat "origin/$base...HEAD"
echo "=== Target-relative IPC and preload changes ==="
git diff --unified=4 "origin/$base...HEAD" -- src/main src/preload || true
echo "=== Full target-relative patch ==="
git diff --unified=4 "origin/$base...HEAD"
fi
echo "=== CI workflow and package scripts ==="
if [ -f .github/workflows/lint.yml ]; then
nl -ba .github/workflows/lint.yml
fi
jq '{scripts,engines}' package.json 2>/dev/null || true
echo "=== Current check-run details ==="
gh api --paginate "repos/${repo}/commits/${head_sha}/check-runs?per_page=100" \
--jq '.check_runs[] | {name,status,conclusion,details_url,started_at,completed_at}'
echo "=== Changed TypeScript/TSX module outlines ==="
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --name-only --diff-filter=ACMR "origin/$base...HEAD" |
while IFS= read -r file; do
case "$file" in
*.ts|*.tsx|*.js)
[ -f "$file" ] || continue
echo "--- $file ---"
ast-grep outline "$file" --items all --view compact || true
;;
esac
done
fi
show() {
file="$1"; start="$2"; end="$3"
echo "===== ${file}:${start}-${end} ====="
if [ -f "$file" ]; then
awk -v s="$start" -v e="$end" 'NR >= s && NR <= e {printf "%6d %s\n", NR, $0}' "$file"
else
echo "MISSING FILE"
fi
}
echo "=== Shortcut model, migration, dispatch, and editors ==="
show src/types/app.d.ts 620 800
show src/renderer/src/other/appReducer.tsx 420 820
show src/renderer/src/utils/localStorage.ts 240 520
show src/renderer/src/hooks/useKeyboardShortcuts.tsx 100 460
show src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx 1 340
show src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx 1 330
echo "=== Equalizer model, persistence, and player lifecycle ==="
show src/renderer/src/other/equalizerData.ts 1 320
show src/renderer/src/hooks/usePlaybackSettings.tsx 1 440
show src/renderer/src/other/player.ts 600 820
show src/renderer/src/components/SettingsPage/Settings/EqualizerSettings.tsx 1 440
show src/renderer/src/App.tsx 1 400
echo "=== Feature call sites and event lifecycle ==="
rg -n -C3 --glob '*.{ts,tsx}' \
'\b(applyEqualizerPreset|toggleEqualizer|getKeyboardShortcuts|setKeyboardShortcuts|resetShortcutsToDefaults|equalizerBandKeys|canonicalizeShortcutId)\s*\(' \
src test 2>/dev/null || true
rg -n -C3 --glob '*.{ts,tsx}' \
'(addEventListener|removeEventListener|isComposing|HTMLInputElement|HTMLTextAreaElement|HTMLSelectElement|isContentEditable|metaKey|Cmd|e\.repeat|Escape)' \
src/renderer/src/hooks/useKeyboardShortcuts.tsx src/renderer/src/components/SettingsPage 2>/dev/null || true
echo "=== Tests that cover changed feature behavior ==="
fd -HI -t f -E node_modules -E dist -E build '.*\.(test|spec)\.(ts|tsx|js)$' test src 2>/dev/null | sort || true
rg -n -i -C2 \
'(keyboardShortcuts|KeyboardShortcutsSettings|AppShortcutsPrompt|toggleEqualizer|applyEqualizerPreset|toggleFullscreenPlayer|equalizerBandKeys|usePlaybackSettings)' \
test src --glob '*.{test,spec}.{ts,tsx,js}' 2>/dev/null || true
echo "=== New static-risk indicators in target-relative diff ==="
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --unified=2 "origin/$base...HEAD" |
rg -n '^\+[^+].*(TODO|FIXME|console\.(log|debug)|debugger|`@ts-ignore`|`@ts-expect-error`|\bas any\b|catch\s*\(|transition-all|innerHTML)' || true
fiLength of output: 6137 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="release/4.0.0-alpha.5"
show() {
file="$1"; start="$2"; end="$3"
echo "===== ${file}:${start}-${end} ====="
if [ -f "$file" ]; then
awk -v s="$start" -v e="$end" 'NR >= s && NR <= e {printf "%6d %s\n", NR, $0}' "$file"
else
echo "MISSING FILE"
fi
}
git fetch --quiet origin "$base" || true
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
echo "=== Target-relative changed files and diff stat ==="
git diff --name-status "origin/$base...HEAD"
git diff --stat "origin/$base...HEAD"
echo "=== Target-relative IPC and preload changes ==="
git diff --unified=4 "origin/$base...HEAD" -- src/main src/preload || true
echo "=== Full target-relative patch ==="
git diff --unified=4 "origin/$base...HEAD"
fi
echo "=== CI workflow and package scripts ==="
show .github/workflows/lint.yml 1 320
jq '{scripts,engines}' package.json 2>/dev/null || true
echo "=== Changed TypeScript and TSX outlines ==="
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --name-only --diff-filter=ACMR "origin/$base...HEAD" |
while IFS= read -r file; do
case "$file" in
*.ts|*.tsx|*.js)
[ -f "$file" ] || continue
echo "--- $file ---"
ast-grep outline "$file" --items all --view compact || true
;;
esac
done
fi
echo "=== Shortcut model, migration, dispatch, and editors ==="
show src/types/app.d.ts 620 800
show src/renderer/src/other/appReducer.tsx 420 820
show src/renderer/src/utils/localStorage.ts 240 520
show src/renderer/src/hooks/useKeyboardShortcuts.tsx 100 460
show src/renderer/src/components/SettingsPage/Settings/KeyboardShortcutsSettings.tsx 1 340
show src/renderer/src/components/SettingsPage/AppShortcutsPrompt.tsx 1 330
echo "=== Equalizer model, persistence, and player lifecycle ==="
show src/renderer/src/other/equalizerData.ts 1 320
show src/renderer/src/hooks/usePlaybackSettings.tsx 1 440
show src/renderer/src/other/player.ts 600 820
show src/renderer/src/components/SettingsPage/Settings/EqualizerSettings.tsx 1 440
show src/renderer/src/App.tsx 1 400
echo "=== Feature call sites and event lifecycle ==="
rg -n -C3 --glob '*.{ts,tsx}' \
'\b(applyEqualizerPreset|toggleEqualizer|getKeyboardShortcuts|setKeyboardShortcuts|resetShortcutsToDefaults|equalizerBandKeys|canonicalizeShortcutId)\s*\(' \
src test 2>/dev/null || true
rg -n -C3 --glob '*.{ts,tsx}' \
'(addEventListener|removeEventListener|isComposing|HTMLInputElement|HTMLTextAreaElement|HTMLSelectElement|isContentEditable|metaKey|Cmd|e\.repeat|Escape)' \
src/renderer/src/hooks/useKeyboardShortcuts.tsx src/renderer/src/components/SettingsPage 2>/dev/null || true
echo "=== Tests that cover changed feature behavior ==="
fd -HI -t f -E node_modules -E dist -E build '.*\.(test|spec)\.(ts|tsx|js)$' test src 2>/dev/null | sort || true
rg -n -i -C2 \
'(keyboardShortcuts|KeyboardShortcutsSettings|AppShortcutsPrompt|toggleEqualizer|applyEqualizerPreset|toggleFullscreenPlayer|equalizerBandKeys|usePlaybackSettings)' \
test src --glob '*.{test,spec}.{ts,tsx,js}' 2>/dev/null || true
echo "=== New static-risk indicators in target-relative diff ==="
if git rev-parse --verify --quiet "origin/$base" >/dev/null; then
git diff --unified=2 "origin/$base...HEAD" |
rg -n '^\+[^+].*(TODO|FIXME|console\.(log|debug)|debugger|`@ts-ignore`|`@ts-expect-error`|\bas any\b|catch\s*\(|transition-all|innerHTML)' || true
fiLength of output: 136476
VerdictThe branch is MERGEABLE and CLEAN with
Major findingsNone. Minor findingsNone. The prior code findings are resolved in the current head:
Remaining test coverage gapNo automated test directly covers the new shortcut and equalizer behavior. This is not a confirmed code defect. Follow-up coverage should test:
Verdict: Ready — high confidence. The branch is mergeable, and the recorded lint, test, node typecheck, and security checks pass.You are interacting with an AI system. |
…x/461-keyboard-shortcuts-customization
feat #461
adds customizable keyboard shortcuts for equalizer toggle (Ctrl+E) and fullscreen toggle (Alt+F). moves shortcut matching from label-based to stable id-based to prevent breakage on language change. adds inline KeyboardShortcutsSettings section in SettingsPage with edit/reset functionality and duplicate detection.
Shortcut
idfield for stable identification across language changesCtrl+E toggles equalizer, Alt+F toggles fullscreen
toggleEqualizer()+applyEqualizerPreset()on AudioPlayerbackward compat via index-based id injection in
getKeyboardShortcuts()KeyboardShortcutsSettings.tsx with inline editing, no modal
all-en locale keys + Turkish translations
fullscreen toggle race condition fixed (reads playerType before dispatch)
Refs #461
Summary by CodeRabbit
New Features
Documentation
Fixes #461