fix: resolve audio overlap and switch default language to English#134
Conversation
- Queue PCM audio chunks sequentially using Web Audio API scheduling to prevent overlapping playback that caused garbled sound - Change all hardcoded Korean text to English in system prompts, greeting triggers, and UI messages for international hackathon - Default language is now English; non-English personas use their detected language code Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello @ComBba, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily addresses two key areas: improving the audio experience and enhancing internationalization. It ensures that audio playback is seamless by preventing sound overlaps and standardizes the application's default language to English, making it more accessible to a broader audience. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Walkthrough웹소켓 온보딩 문구와 세션 관련 사용자/시스템 메시지를 한국어에서 영어로 변경하고, 세션 재회(리유니온) 시스템 지침의 기본 언어를 영어로 설정하며, 웹 오디오 재생을 순차적으로 스케줄링하도록 플레이백 로직을 수정했습니다. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 분 시
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the audio overlap issue by implementing a sequential playback queue in useAudio.ts. The switch to English as the default language is also thoroughly implemented across the backend. My review includes a couple of suggestions for improving maintainability by replacing hardcoded values (a magic number in the audio hook and language strings in the session manager) with named constants. These changes will make the code easier to read and update in the future.
| langNote := "Speak naturally in English." | ||
| if lang != "" && lang != "en" { | ||
| langNote = fmt.Sprintf("Speak naturally in the language code '%s'.", lang) | ||
| } |
There was a problem hiding this comment.
The default language instruction and language code ("en") are hardcoded. To improve maintainability and make future localization easier, consider defining these as package-level constants. For example:
const (
defaultLangCode = "en"
defaultLangInstruction = "Speak naturally in English."
)You can then use these constants here, making the code cleaner and easier to update.
|
|
||
| source.onended = () => { | ||
| // Mark not playing only when no more queued audio remains. | ||
| if (ctx.currentTime >= nextStartTimeRef.current - 0.01) { |
There was a problem hiding this comment.
The magic number 0.01 is used here as a tolerance, likely to account for precision issues with AudioContext.currentTime. To improve code clarity and maintainability, it's best to extract this value into a named constant with a comment explaining its purpose. For example:
// Small tolerance for AudioContext.currentTime precision when checking if playback has ended.
const PLAYBACK_END_TOLERANCE_SECONDS = 0.01;Using this constant would make the logic here self-documenting.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/session/manager_test.go (1)
410-419: “기본값” 검증은 빈 언어 코드 경로도 직접 확인해 주세요.현재 테스트는
lang="en"명시 케이스를 검증합니다. 기본값(미지정) 동작 보장을 위해lang=""케이스를 이 테스트(또는 별도 테스트)로 추가하는 편이 더 정확합니다.테스트 의도에 맞춘 최소 수정 예시
-func TestBuildReunionConfig_DefaultEnglish(t *testing.T) { +func TestBuildReunionConfig_DefaultEnglishWhenLangEmpty(t *testing.T) { mgr := NewManager("test-en-lang") - mgr.SetPersona("Friend", "Kore", "en", "Cheerful", "Casual") + mgr.SetPersona("Friend", "Kore", "", "Cheerful", "Casual") cfg := mgr.BuildReunionConfig() sysText := cfg.SystemInstruction.Parts[0].Text if !strings.Contains(sysText, "Speak naturally in English") { - t.Fatalf("expected default English language note for lang='en', got: %s", sysText) + t.Fatalf("expected default English language note for empty lang, got: %s", sysText) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/session/manager_test.go` around lines 410 - 419, Update the test to also verify the "default language" behavior when no language is provided: in TestBuildReunionConfig_DefaultEnglish (or a new test) create a Manager with NewManager("test-en-lang"), call mgr.SetPersona("Friend", "Kore", "" /* empty lang */, "Cheerful", "Casual"), then call mgr.BuildReunionConfig() and assert that cfg.SystemInstruction.Parts[0].Text contains "Speak naturally in English" (same assertion as the existing en case); this ensures BuildReunionConfig/Manager.SetPersona handles the empty lang path the same as "en".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/session/manager_test.go`:
- Around line 410-419: Update the test to also verify the "default language"
behavior when no language is provided: in TestBuildReunionConfig_DefaultEnglish
(or a new test) create a Manager with NewManager("test-en-lang"), call
mgr.SetPersona("Friend", "Kore", "" /* empty lang */, "Cheerful", "Casual"),
then call mgr.BuildReunionConfig() and assert that
cfg.SystemInstruction.Parts[0].Text contains "Speak naturally in English" (same
assertion as the existing en case); this ensures
BuildReunionConfig/Manager.SetPersona handles the empty lang path the same as
"en".
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/handler/websocket.gointernal/session/manager.gointernal/session/manager_test.goweb/hooks/useAudio.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba25efc9e8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (ctx.currentTime >= nextStartTimeRef.current - 0.01) { | ||
| setIsPlaying(false); |
There was a problem hiding this comment.
This onended condition now relies on ctx.currentTime and nextStartTimeRef, but the existing audio-hook test harness does not provide currentTime on MockAudioContext or duration on the mocked buffer (web/__tests__/hooks/useAudio.test.ts), so the comparison becomes undefined >= NaN and setIsPlaying(false) never executes. Since frontend CI runs unit tests (.github/workflows/ci.yml, Unit tests step), this change causes a deterministic test regression for the source-end path unless the reset logic (or the mock contract) is updated in the same change.
Useful? React with 👍 / 👎.
- Fix frontend test regression: add currentTime and duration to MockAudioContext so onended callback correctly triggers setIsPlaying - Extract PLAYBACK_END_TOLERANCE_S constant (Gemini) - Extract defaultLangCode/defaultLangInstruction constants (Gemini) - Add TestBuildReunionConfig_DefaultEnglishWhenLangEmpty for empty language code path coverage (CodeRabbit) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/session/manager.go (1)
371-379:lang값 정규화로 기본 영어 분기를 더 견고하게 만드세요.Line 378 조건은
"EN"또는" en "같은 입력에서 비영어 분기로 처리될 수 있습니다. 비교 전에TrimSpace + ToLower정규화를 권장합니다.제안 diff
func buildReunionSystemInstruction(name, personality, speechStyle, lang string) string { langNote := defaultLangInstruction - if lang != "" && lang != defaultLangCode { - langNote = fmt.Sprintf("Speak naturally in the language code '%s'.", lang) + normalizedLang := strings.ToLower(strings.TrimSpace(lang)) + if normalizedLang != "" && normalizedLang != defaultLangCode { + langNote = fmt.Sprintf("Speak naturally in the language code '%s'.", normalizedLang) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/session/manager.go` around lines 371 - 379, The lang comparison in buildReunionSystemInstruction is brittle for inputs like "EN" or " en "; normalize lang by calling strings.TrimSpace and strings.ToLower (e.g., normalizedLang := strings.ToLower(strings.TrimSpace(lang))) before checking against defaultLangCode and emptiness, then use the normalized value when deciding whether to set langNote (defaultLangInstruction vs formatted "Speak naturally in the language code '%s'.") so the English branch is robust to casing/whitespace.internal/session/manager_test.go (1)
44-46: 온보딩 인사 검증을 더 구체적으로 고정하세요.현재
"welcome"만 확인하면 의도한 문구가 바뀌어도 테스트가 통과할 수 있습니다. 핵심 문장 전체 검증이 더 안전합니다.제안 diff
- if !strings.Contains(sysText, "welcome") { - t.Fatalf("expected English greeting in system instruction") + if !strings.Contains(sysText, "Hi there, welcome to missless") { + t.Fatalf("expected exact English greeting in system instruction") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/session/manager_test.go` around lines 44 - 46, Replace the loose substring check that only looks for "welcome" with an assertion that verifies the full expected onboarding sentence present in sysText (replace the strings.Contains(sysText, "welcome") check). Locate the test's sysText variable and update the assertion to check for the exact core greeting phrase used in the system instruction (e.g., the full English onboarding sentence) so changes to unrelated words won't let the test pass; keep the failure message descriptive and reference sysText in the assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/hooks/useAudio.ts`:
- Around line 55-59: The cleanup function in useAudio currently closes the
AudioContext but doesn't reset playback state; update the cleanup callback to
explicitly set isPlaying to false (call setIsPlaying(false)) and, if you
maintain a ref like isPlayingRef, set isPlayingRef.current = false as well so
the UI and any ref-based logic reflect that playback has stopped when
audioCtxRef.current is closed.
---
Nitpick comments:
In `@internal/session/manager_test.go`:
- Around line 44-46: Replace the loose substring check that only looks for
"welcome" with an assertion that verifies the full expected onboarding sentence
present in sysText (replace the strings.Contains(sysText, "welcome") check).
Locate the test's sysText variable and update the assertion to check for the
exact core greeting phrase used in the system instruction (e.g., the full
English onboarding sentence) so changes to unrelated words won't let the test
pass; keep the failure message descriptive and reference sysText in the
assertion.
In `@internal/session/manager.go`:
- Around line 371-379: The lang comparison in buildReunionSystemInstruction is
brittle for inputs like "EN" or " en "; normalize lang by calling
strings.TrimSpace and strings.ToLower (e.g., normalizedLang :=
strings.ToLower(strings.TrimSpace(lang))) before checking against
defaultLangCode and emptiness, then use the normalized value when deciding
whether to set langNote (defaultLangInstruction vs formatted "Speak naturally in
the language code '%s'.") so the English branch is robust to casing/whitespace.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/session/manager.gointernal/session/manager_test.goweb/__tests__/hooks/useAudio.test.tsweb/hooks/useAudio.ts
| const cleanup = useCallback(() => { | ||
| audioCtxRef.current?.close(); | ||
| audioCtxRef.current = null; | ||
| nextStartTimeRef.current = 0; | ||
| }, []); |
There was a problem hiding this comment.
cleanup에서 isPlaying을 명시적으로 false로 내려주세요.
Line 55-59에서 오디오 컨텍스트를 닫아도, 재생 중 cleanup이 호출되면 UI 상태가 재생 중으로 남을 수 있습니다. 정리 루틴에서 상태를 함께 초기화하는 것이 안전합니다.
제안 diff
const cleanup = useCallback(() => {
audioCtxRef.current?.close();
audioCtxRef.current = null;
nextStartTimeRef.current = 0;
+ setIsPlaying(false);
}, []);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@web/hooks/useAudio.ts` around lines 55 - 59, The cleanup function in useAudio
currently closes the AudioContext but doesn't reset playback state; update the
cleanup callback to explicitly set isPlaying to false (call setIsPlaying(false))
and, if you maintain a ref like isPlayingRef, set isPlayingRef.current = false
as well so the UI and any ref-based logic reflect that playback has stopped when
audioCtxRef.current is closed.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 participant
Summary
AudioContext.currentTimequeueChanges
Audio overlap fix (
web/hooks/useAudio.ts)nextStartTimeto schedule each PCM buffer after the previous one finishesAudioBufferSourcenodes from playing simultaneouslyEnglish default (
internal/session/manager.go,internal/handler/websocket.go)Speak naturally in the language code 'ko'.Local CI
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores
Tests