-
Notifications
You must be signed in to change notification settings - Fork 5
chore: add sound effect to alerts #1919
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
WalkthroughAdds sound feedback to the extension: introduces use-sound dependency, exports WAV assets, integrates playback in alert notifications and DApp removal flows, and updates webpack to emit audio files as resources. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant UI as UI Component
participant Hook as useAlerts
participant Audio as useSound(play)
User->>UI: Trigger alert-worthy action
UI->>Hook: notify(alert)
rect rgba(220,245,255,0.5)
note right of Hook: Add alert to state/queue
Hook->>Audio: play()
Audio-->>Hook: sound played
end
sequenceDiagram
autonumber
actor User
participant Access as WebsitesAccess
participant API as removeAuthorization
participant Audio as useSound(play)
User->>Access: Delete authorized DApp
Access->>API: removeAuthorization(dApp)
alt success or error
API-->>Access: Promise settled
rect rgba(235,255,235,0.6)
note right of Access: finally {}
Access->>Audio: play()
Audio-->>Access: sound played
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🧪 Early access (Sonnet 4.5): enabledWe are currently testing the Sonnet 4.5 model, which is expected to improve code review quality. However, this model may lead to increased noise levels in the review comments. Please disable the early access features if the noise level causes any inconvenience. Note:
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/extension-polkagate/src/partials/WebsitesAccess.tsx (1)
62-72: Don’t play success sound or show success snackbar on failure.Currently both happen in finally, even if removeAuthorization fails.
Apply this to only play and show success on success, while always clearing busy:
- const onDeleteDapp = useCallback((url: string) => { - setIsBusy(true); - removeAuthorization(url) - .catch(console.error) - .finally(() => { - play(); - setIsBusy(false); - setShowSnackbar(true); - }); - }, [play]); + const onDeleteDapp = useCallback((url: string) => { + setIsBusy(true); + removeAuthorization(url) + .then(() => { + play(); + setShowSnackbar(true); + }) + .catch(console.error) + .finally(() => { + setIsBusy(false); + }); + }, [play]);
🧹 Nitpick comments (4)
packages/extension/webpack.shared.cjs (1)
75-78: Set a stable filename pattern for emitted audio assets.Without a generator, Webpack’s default asset name may differ from your
static/[name].[ext]convention used elsewhere.Apply this diff to keep audio files under static/:
{ test: /\.(wav|mp3|ogg)$/, - type: 'asset/resource' + type: 'asset/resource', + generator: { + filename: 'static/[name][ext]' + } }packages/extension-polkagate/src/hooks/useAlerts.ts (1)
16-16: Avoid overlapping sounds by interrupting previous playback.Prevents stacking when multiple alerts fire in quick succession.
- const [play] = useSound(soft, { volume: 0.4 }); + const [play] = useSound(soft, { volume: 0.4, interrupt: true });packages/extension-polkagate/src/partials/WebsitesAccess.tsx (1)
53-53: Interrupt previous playback to avoid overlapping sounds.- const [play] = useSound(heavy, { volume: 0.2 }); + const [play] = useSound(heavy, { volume: 0.2, interrupt: true });packages/extension-polkagate/src/assets/sounds/index.ts (1)
4-4: Avoid // @ts-nocheck; add asset module types instead.Provide module declarations for audio files to keep type safety.
- Remove the ts-nocheck line from this file.
- Add a types file (example path):
// packages/extension-polkagate/src/types/assets.d.ts declare module '*.wav' { const src: string; export default src; } declare module '*.mp3' { const src: string; export default src; } declare module '*.ogg' { const src: string; export default src; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (12)
packages/extension-polkagate/src/assets/sounds/bonk-click-wdeny-feel.wavis excluded by!**/*.wavpackages/extension-polkagate/src/assets/sounds/button-casual-event.wavis excluded by!**/*.wavpackages/extension-polkagate/src/assets/sounds/click-pop-two-part.wavis excluded by!**/*.wavpackages/extension-polkagate/src/assets/sounds/heavy_00.wavis excluded by!**/*.wavpackages/extension-polkagate/src/assets/sounds/littlerobotsoundfactory_06.wavis excluded by!**/*.wavpackages/extension-polkagate/src/assets/sounds/mouth_06.wavis excluded by!**/*.wavpackages/extension-polkagate/src/assets/sounds/ping-bing.wavis excluded by!**/*.wavpackages/extension-polkagate/src/assets/sounds/quick-ui-or-event-deep.wavis excluded by!**/*.wavpackages/extension-polkagate/src/assets/sounds/rollover-low.wavis excluded by!**/*.wavpackages/extension-polkagate/src/assets/sounds/soft_00.wavis excluded by!**/*.wavpackages/extension-polkagate/src/assets/sounds/standard_00.wavis excluded by!**/*.wavyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (5)
package.json(1 hunks)packages/extension-polkagate/src/assets/sounds/index.ts(1 hunks)packages/extension-polkagate/src/hooks/useAlerts.ts(2 hunks)packages/extension-polkagate/src/partials/WebsitesAccess.tsx(3 hunks)packages/extension/webpack.shared.cjs(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
packages/extension-polkagate/src/hooks/useAlerts.ts (1)
packages/extension-polkagate/src/util/constants.ts (1)
TIME_TO_REMOVE_ALERT(307-307)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: build
| "umi-request": "^1.4.0", | ||
| "use-sound": "^5.0.0" | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
Ensure the workspace that imports use-sound declares it as a dependency.
use-sound is used in packages/extension-polkagate; relying on root hoisting can break under stricter linking (e.g., PnP).
Run this to verify the dependency is declared in the package that uses it:
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Imports of use-sound:"
rg -n -C1 -t ts -t tsx -P $'from\\s+[\'"]use-sound[\'"]'
PKG="packages/extension-polkagate/package.json"
if [[ -f "$PKG" ]]; then
echo -n "packages/extension-polkagate depends on use-sound: "
jq -r '.dependencies["use-sound"] // "MISSING"' "$PKG"
else
echo "Missing $PKG"
fiLength of output: 49
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Searching for any occurrences of the literal 'use-sound' (imports/usages):"
rg -n -S --hidden --glob '!node_modules' "use-sound" || true
echo
echo "Searching for common import/require patterns:"
rg -n -S --hidden --glob '!node_modules' "from 'use-sound'" || true
rg -n -S --hidden --glob '!node_modules' 'from "use-sound"' || true
rg -n -S --hidden --glob '!node_modules' "require('use-sound')" || true
rg -n -S --hidden --glob '!node_modules' 'require("use-sound")' || true
echo
PKG="packages/extension-polkagate/package.json"
if [[ -f "$PKG" ]]; then
echo "$PKG found — checking for use-sound in dependencies/devDependencies/peerDependencies:"
jq -r '.dependencies["use-sound"] // .devDependencies["use-sound"] // .peerDependencies["use-sound"] // "MISSING"' "$PKG"
else
echo "$PKG not found. Listing packages/ and searching package.json files for extension-polkagate:"
ls -la packages || true
rg -n --hidden --glob '!node_modules' --glob 'packages/**/package.json' '"name"\s*:\s*"?extension-polkagate"?' || true
fiLength of output: 998
Declare use-sound in packages/extension-polkagate package.json dependencies
packages/extension-polkagate imports use-sound but its package.json does not list it (jq -> "MISSING"); add "use-sound" to packages/extension-polkagate/package.json -> dependencies to avoid breakage under PnP/strict linking.
🤖 Prompt for AI Agents
In package.json around lines 99 to 101, packages/extension-polkagate imports
use-sound but does not declare it in its dependencies; add "use-sound": "^5.0.0"
to the dependencies object in packages/extension-polkagate/package.json
(matching the workspace root version if present), then run the package manager
install to update the lockfile so PnP/strict linking will resolve the module.
Summary by CodeRabbit