Skip to content

Conversation

@t-miya
Copy link
Contributor

@t-miya t-miya commented Dec 26, 2025

No description provided.

@t-miya
Copy link
Contributor Author

t-miya commented Dec 26, 2025

!haiku レビューして

@github-actions

This comment was marked as resolved.

@t-miya
Copy link
Contributor Author

t-miya commented Dec 29, 2025

!haiku 指摘箇所を修正したので再度レビューして

@github-actions

This comment was marked as resolved.

@t-miya
Copy link
Contributor Author

t-miya commented Dec 29, 2025

!haiku 修正箇所をレビューして
MediaChannel: エラーハンドリングロジックの重複 については setAudioSoftMute のみ判定ロジックが残っているため中途半端になるので対応しない

@github-actions

This comment was marked as resolved.

@t-miya
Copy link
Contributor Author

t-miya commented Dec 29, 2025

!sonnet haikuのレビュー内容を踏まえてクロスレビューして。また haiku 側でチェックしていない箇所もチェックして

@github-actions

This comment was marked as resolved.

@t-miya
Copy link
Contributor Author

t-miya commented Dec 29, 2025

!sonnet 指摘箇所を修正したので再レビューして

@github-actions

This comment was marked as resolved.

@t-miya t-miya changed the title (WIP)音声ソフトミュート/ハードミュート機能追加 音声ソフトミュート/ハードミュート機能追加 Dec 29, 2025
@t-miya t-miya requested a review from zztkm December 29, 2025 06:59
import WebRTC

/// RTCAudioDeviceModule の録音ポーズ/再開をラップするクラス
public final class AudioDeviceModuleWrapper {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

特に公開する必要がないなら public じゃなくて internal (アクセス修飾子をつけない) で良さそうだと思いました

/// MediaChannel の接続中にマイクをソフトミュート有効化 / 無効化します
/// - Parameter mute: `true` で有効化、`false` で無効化
/// - Returns: 成功した場合は `true`、接続状態などの理由で処理しなかった場合は `false`
public func setAudioSoftMute(_ mute: Bool) -> Bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

成否を表す値を Bool 型で返すかという部分でちょっと議論の余地はありそうだなと今更思いました(ドキュメント時点で気づけばよかった)。

例えば DataChannel でメッセージを送信する sendMessage は Error? を返すように実装されてます。
https://github.com/shiguredo/sora-ios-sdk/blob/develop/Sora/MediaChannel.swift#L498

Error? を返すほうが統一感がありそうだし、Error? 型にするでもありだなと思いました。
Error? のメリットとしては失敗理由を Error に乗せることができるってとこです。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

仮に Error? に直すとしたら、canSwitchAudioMute なども Error? にする必要はありそうですね 📝

@t-miya
Copy link
Contributor Author

t-miya commented Dec 29, 2025

!haiku 変更した内容についてレビューして

@github-actions
Copy link

github-actions bot commented Dec 29, 2025

Claude finished @t-miya's task in 25s —— View job


コードレビュー

以下の内容について指摘します。優先順位が高い順に記載しています。

1. [重大な論理エラー] setAudioHardMute の戻り値判定が逆

MediaChannel.setAudioHardMute() のコード (L538) に論理エラーがあります:

if NativePeerChannelFactory.default.audioDeviceModuleWrapper.setAudioHardMute(mute) {
  return SoraError.mediaChannelError(reason: "AudioDeviceModuleWrapper::setAudioHardMute failed")
}

AudioDeviceModuleWrapper.setAudioHardMute() は成功時に true を返しますが、現在のコードでは成功時にエラーを返しています。逆の判定になっています。

修正:

if !NativePeerChannelFactory.default.audioDeviceModuleWrapper.setAudioHardMute(mute) {
  return SoraError.mediaChannelError(reason: "AudioDeviceModuleWrapper::setAudioHardMute failed")
}

2. アクセス修飾子の設計

AudioDeviceModuleWrapper がアクセス修飾子なしで定義されています。NativePeerChannelFactory から外部にアクセスされる必要がなければ internal を明示すべきです。CLAUDE.md に従い、公開する必要がなければ internal を付与してください。

3. 戻り値型の一貫性

zztkm の指摘通り、戻り値型の一貫性に問題があります:

  • AudioDeviceModuleWrapper.setAudioHardMute()Bool を返す
  • MediaChannel.setAudioHardMute()Error? を返す

既存の sendMessage() (L498) が Error? を返すように実装されている一方で、他のメソッドとの一貫性が完全ではありません。ただし、現在の設計は「ラッパーの内部実装詳細として Bool を使い、公開 API では Error を返す」という方式として理解できます。これでよいか確認してください。

4. CHANGES.md との整合性確認

CHANGES.md の記述内容:

  • ハードミュート:iOS 端末のマイクインジケーターを消灯させる ✓
  • ソフトミュート:シンタックスシュガー ✓
  • AudioDeviceModuleWrapper:RTCAudioDeviceModule の pauseRecording/resumeRecording をラップ ✓
  • hasAudioTrack の追加 ✓

変更点とリリースノートの整合性は取れています。

5. その他確認事項

  • Logger の使用が適切に行われている
  • DispatchQueue で直列化処理を実装し、並行処理の安全性を確保している
  • エラーハンドリングの判定ロジックはほぼ適切(上記の逆判定を除く)
  • isHardMuted の状態管理で多重呼び出しの防止ができている

重要:上記の「論理エラー (item 1)」を修正してください。これはコードの動作に直接影響する重大な問題です。

View PR

@t-miya
Copy link
Contributor Author

t-miya commented Dec 29, 2025

!haiku 修正したので再度レビューして
AudioDeviceModuleWrapper は内部モジュールのためログ出力と Bool を返す、で十分。MediaChannel の公開APIで Error を返すという想定。

@github-actions

This comment was marked as resolved.

@t-miya t-miya merged commit 195ccb9 into develop Dec 29, 2025
3 checks passed
@t-miya t-miya deleted the feature/audio-hard-mute branch December 29, 2025 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants