Feat/frontend/play bgm#245
Conversation
arkwnet
added
Type: Enhancement
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| }); | ||
|
|
||
| return <RankingTabs />; |
There was a problem hiding this comment.
🔴 Missing [] dependency array causes play() to re-fire on every render, restarting BGM
In Ranking.tsx, useEffect(() => { play("/sounds/bgm0.mp3"); }) has no dependency array, so it runs after every render, not just on mount. Each call to play (in WebAudioPlayer.tsx:55) first calls stop() to halt the current audio, then restarts the track from the beginning. Since play and stop in WebAudioPlayer.tsx:80 are not memoized with useCallback, every render of WebAudioPlayer produces a new context value, triggering consumer re-renders — which in turn re-fire this effect. Concretely, toggling the sound button causes the ranking BGM to restart from the beginning, and any parent re-render will do the same.
| }); | |
| return <RankingTabs />; | |
| useEffect(() => { | |
| play("/sounds/bgm0.mp3"); | |
| }, []); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| }); | ||
|
|
||
| return ( |
There was a problem hiding this comment.
🟡 Missing [] dependency array causes stop() to fire on every render
In Home.tsx, useEffect(() => { stop(); }) has no dependency array, so it runs after every render rather than only on mount. While stop() is idempotent when nothing is playing (it checks sourceRef.current), this still means that any re-render caused by context value changes or parent re-renders will unnecessarily invoke stop(). This is inconsistent with the analogous effects in GameResult.tsx:21-23 and GameTyping.tsx:197-211 which correctly use [].
| }); | |
| return ( | |
| useEffect(() => { | |
| stop(); | |
| }, []); |
Was this helpful? React with 👍 or 👎 to provide feedback.
| }; | ||
| }; | ||
|
|
||
| return <PlayerContext.Provider value={{ play, stop }}>{children}</PlayerContext.Provider>; |
There was a problem hiding this comment.
📝 Info: Context value not memoized — causes unnecessary consumer re-renders
In WebAudioPlayer.tsx:80, the value={{ play, stop }} creates a new object on every render since neither play nor stop are wrapped in useCallback. This means every re-render of WebAudioPlayer (e.g., when isPlay changes) creates a new context value, forcing all useContext(PlayerContext) consumers to re-render. This amplifies the impact of the missing dependency arrays in Ranking.tsx and Home.tsx (reported as bugs), and is generally wasteful. Wrapping play and stop in useCallback (and using an isPlayRef for the isPlay check inside play) would stabilize the context value and prevent unnecessary consumer re-renders.
Was this helpful? React with 👍 or 👎 to provide feedback.
| <> | ||
| <Footer isPlay={isPlay} setIsPlay={setIsPlay} /> | ||
| <div className="children"> | ||
| <WebAudioPlayer isPlay={isPlay}>{children}</WebAudioPlayer> | ||
| </div> |
There was a problem hiding this comment.
📝 Info: Footer rendered before children — visual ordering relies on CSS fixed positioning
In client.tsx:11-15, the Footer is rendered before the children div in the DOM. The previous layout in layout.tsx also had Footer before children, so this matches the existing pattern. The footer uses position: fixed (Footer.module.scss:6) so DOM order doesn't affect visual layout. Not a bug, just noting the ordering is intentional and CSS-dependent.
Was this helpful? React with 👍 or 👎 to provide feedback.
| useEffect(() => { | ||
| play("/sounds/bgm1.mp3"); |
There was a problem hiding this comment.
🚩 Stale play closure in game components won't respond to mid-page sound toggle
In GamePre.tsx:11, GameTyping.tsx:199-203, and GameResult.tsx:22, the play function is called inside useEffect hooks with [] or [nextPage] dependency arrays, but play itself is not included as a dependency. Since play captures isPlay via closure (WebAudioPlayer.tsx:53), the version captured at mount time won't reflect later toggles. This means if a user toggles sound ON while already on one of these pages, the BGM won't start (the captured play still has isPlay=false). This is a minor UX limitation rather than a crash-level bug — sound works correctly when navigating to a new page after toggling.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (!buffer) { | ||
| const response = await fetch(url); | ||
| const arrayBuffer = await response.arrayBuffer(); | ||
| buffer = await ctx.decodeAudioData(arrayBuffer); | ||
| bufferCacheRef.current.set(url, buffer); |
There was a problem hiding this comment.
📝 Info: No error handling in play for failed fetch or decode
In WebAudioPlayer.tsx:61-63, fetch(url) and ctx.decodeAudioData(arrayBuffer) are both async operations that can throw (e.g., 404 response, invalid audio data), but neither is wrapped in a try/catch. An unhandled promise rejection would bubble up silently. Since play is called from useEffect callbacks which don't handle the returned promise, any rejection becomes an unhandled promise rejection warning. Consider wrapping the async body in a try/catch to prevent this.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (isProcessingRef.current) { | ||
| return; | ||
| } | ||
| stop(); |
There was a problem hiding this comment.
📝 Info: sendResultData uses stop without it in the dependency array
In GameTyping.tsx:57, stop() is called inside sendResultData which is wrapped in useCallback with deps [stats, nextPage, router, setScore] at line 108. stop from useWebAudio() is not included. I verified this is safe because stop (WebAudioPlayer.tsx:33-41) only accesses sourceRef.current, which is a ref and always points to the latest value regardless of closure staleness. So even a stale stop reference correctly stops the current audio source.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
(提案)<PlayerContext.Provider>という書き方は古いので<PlayerContext>に変更せよ。 現状だとSOUNDの画像以外の何もないところでも画面半分より右にカーソルがあると isPlayというネーミングはやめた方がいいと思います。 isPlayがclient.tsxとWebAudioPlayer()の両方で、別々のStateとして管理されています。 // isMuted推し
type PlayerContextType = {
play: (url: string) => Promise<void>; // ここ実際のシグネチャに合わせてPromiseにしている
stop: () => void;
isMuted: boolean; // 内部的にはRefとして管理(Stateにしちゃうと、値の変更に伴ってこのContextの子コンポーネント全体が再レンダリングされてしまうので)
toggleIsMuted: () => void;
};こうするとFooterの上にclient.tsxを置いてそこからStateを渡す必要がなくなる(Footerが自分でuseWebAudioすればいいので)ので、client.tsxを削除できます。 (弱提案)さらにそこまで行くと、SOUND ON/OFFのボタンだけ別のクライアントコンポーネントとして別モジュール(=別ファイル)に切り出すことで、Footerから 以下は、弱い提案。
|
|
あ、書き忘れてたけどSOUND OFFの状態でゲームのページ(/game)に遷移して、GamePre画面中にSOUND ONを押すとGameTyping画面に行くまで音が流れないっていうのはやや直感に反するので直せるなら直したいですね。 例えば、SOUNDのボタンを押したら再生、もう一度押したら停止という動作にするなら(←仮定) |
|
別のPRでやるとのことでしたが、bgmはこのPRでチェックインしてもよいと思います。 その前提だと普通にチェックインした場合とLFSを使った場合でgit clone時のダウンロードサイズに差がほぼ出ないので(とSonnet 4.6が言っています)。 メモ: もしbgmを追加するなら、できれば他のテキストファイルに合わせて*.mp3のファイルパーミッションも644に変更してからチェックインしてほしい。 |
KinjiKawaguchi
left a comment
KinjiKawaguchi
left a comment
There was a problem hiding this comment.
連絡いただいてたのに気づけておらず申し訳ないです。
takeyeahさんの構造面の指摘も、Devinの個別バグ指摘もいずれも妥当だと思うので、それらに加えて自分が気になった点は2点です。
| </div> | ||
| </div> | ||
| <div className={styles.right}> | ||
| <div className={styles.sound} onClick={toggleSound}> |
There was a problem hiding this comment.
サウンド切り替えを
There was a problem hiding this comment.
div->buttonに変更依頼。
なぜかレンダーしてくれない
There was a problem hiding this comment.
buttonに変えるの個人的には是非ってほどじゃないけどいいですね。レンダーされないのはbuttonタグがインライン要素だからかなと思いました。対策としては一長一短ですが、ブロック要素で囲むか、クレジット表記(Shizuoka…)の部分の幅を(float外した上で)縮めるか、buttonをposition: absoluteにしてツリーから外すか…あたりかと(試してはない)。
(追記)cursor: pointerの件を解決できるのでよいとは思いますが、是非やろうとまでは私は考えてません(再掲)。理由はこのtyping-app、全体的にアクセシビリティ考えられてないので今更……と思うのと、buttonにするとブラウザのデフォルトスタイルを剥がすためのCSSが増えてめんどくさそう(小並感)と思うからです。
| import WebAudioPlayer from "@/utils/WebAudioPlayer"; | ||
| import Footer from "../components/organism/Footer"; | ||
|
|
||
| export default function ClientLayout({ children }) { |
There was a problem hiding this comment.
このファイルを残す場合は import { ReactNode } from "react" して { children: ReactNode } を付けてください。
3 participants
チケットへのリンク
close #69
やったこと
やらないこと
容量が大きいBGMデータ自体のアップロード
できるようになること（ユーザ目線）
画面右下のボタンでBGMのON/OFFを切り替えられるようになります。暴発を防ぐため初期値はOFF、かつ再読み込みでリセットされます。
できなくなること（ユーザ目線）
無し
動作確認
全機能を目視で確認していますが、レビューされる方はお手元の環境でダブルチェックをお願いします (特に生協指定PCをお持ちの方)
その他
無し