Conversation
Fixes #211
There was a problem hiding this comment.
Pull request overview
This pull request implements a speaker selection feature for the text-to-speech (TTS) functionality, allowing users to choose from available voices through a paginated UI. The implementation adds a new /tts set voice command that displays speakers in pages of up to 20 options with navigation buttons.
Changes:
- Added
/tts set voicecommand with paginated speaker selection UI - Implemented VoiceVox API client method to fetch available speakers
- Added message component handlers for speaker selection and page navigation
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| src/internal/bot/messageComponent/tts_voice.go | New message component handlers for speaker selection and pagination |
| src/internal/bot/command/general/tts/set/voice.go | New command implementation for voice selection with speaker fetching and UI building |
| src/internal/bot/command/general/tts/set.go | New subcommand group for TTS settings |
| src/internal/bot/command/general/tts.go | Registered the new set subcommand group |
| src/internal/bot/handler/interaction.go | Added special handling to mark tts set voice as ephemeral |
| src/internal/bot/command/general/help/help-commands.go | Updated help documentation to include the new voice command |
| src/internal/api/voicevox/types.go | Added Speaker and SpeakerStyle types for API responses |
| src/internal/api/voicevox/client.go | Added GetSpeakers method to fetch available speakers from VoiceVox API |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| s.InteractionRespond(i.Interaction, &discordgo.InteractionResponse{ | ||
| Type: discordgo.InteractionResponseUpdateMessage, | ||
| Data: &discordgo.InteractionResponseData{ | ||
| Embeds: []*discordgo.MessageEmbed{ | ||
| { | ||
| Title: "話者設定を更新しました", | ||
| Description: "選択した話者: " + label, | ||
| Color: config.Colors.Success, | ||
| Timestamp: time.Now().Format(time.RFC3339), | ||
| }, | ||
| }, | ||
| Components: []discordgo.MessageComponent{}, | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Missing error handling. The return value of s.InteractionRespond is ignored. While this might be acceptable for some scenarios, errors from InteractionRespond should ideally be logged for debugging purposes, especially since the function logs other errors. Consider logging the error if InteractionRespond fails, similar to how other errors in this file are handled.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func FetchSpeakers(ctx *internal.BotContext) ([]voicevox.Speaker, error) { | ||
| cachedSpeakers.mu.RLock() | ||
| if time.Now().Before(cachedSpeakers.expires) && len(cachedSpeakers.speakers) > 0 { | ||
| speakers := cachedSpeakers.speakers | ||
| cachedSpeakers.mu.RUnlock() | ||
| return speakers, nil | ||
| } | ||
| cachedSpeakers.mu.RUnlock() |
There was a problem hiding this comment.
The cache check and early return logic has a potential race condition. After releasing the read lock and before acquiring the write lock, another goroutine might have already fetched and cached the speakers. This could lead to redundant API calls.
Consider using a double-checked locking pattern where you recheck the cache after acquiring the write lock but before making the API call, or use sync.Once for initialization.
| } | ||
| cachedSpeakers.mu.RUnlock() | ||
|
|
||
| requestCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) |
There was a problem hiding this comment.
The context passed to GetSpeakers is created from context.Background(), ignoring the potential cancellation context that may be available from the bot context. If the bot is shutting down or the request should be cancelled, this independent context won't be affected.
Consider accepting a context.Context parameter in FetchSpeakers or using a context from the BotContext if available.
| requestCtx, cancel := context.WithTimeout(context.Background(), 30*time.Second) | |
| // Derive the request context from the bot context if it implements context.Context, | |
| // otherwise fall back to context.Background(). | |
| baseCtx := context.Background() | |
| if ctx != nil { | |
| if parentCtx, ok := any(ctx).(context.Context); ok && parentCtx != nil { | |
| baseCtx = parentCtx | |
| } | |
| } | |
| requestCtx, cancel := context.WithTimeout(baseCtx, 30*time.Second) |
| return | ||
| } | ||
|
|
||
| speakerID := values[0] |
There was a problem hiding this comment.
The speakerID from the select menu is directly used without validation. While Discord's select menu UI prevents arbitrary input during normal operation, it's a security best practice to validate that the selected speaker ID actually exists before storing it in the database.
Consider adding a validation step that checks if the speakerID exists in the speaker list, similar to how ResolveSpeakerLabel already iterates through speakers to find a match.
| speakerID := values[0] | |
| speakerID := values[0] | |
| // Validate that the selected speakerID actually exists in the select menu options. | |
| validSpeaker := false | |
| if i.Message != nil { | |
| for _, row := range i.Message.Components { | |
| actionRow, ok := row.(*discordgo.ActionsRow) | |
| if !ok { | |
| continue | |
| } | |
| for _, comp := range actionRow.Components { | |
| selectMenu, ok := comp.(*discordgo.SelectMenu) | |
| if !ok { | |
| continue | |
| } | |
| if selectMenu.CustomID != ttsutil.VoiceSelectCustomID { | |
| continue | |
| } | |
| for _, opt := range selectMenu.Options { | |
| if opt.Value == speakerID { | |
| validSpeaker = true | |
| break | |
| } | |
| } | |
| if validSpeaker { | |
| break | |
| } | |
| } | |
| if validSpeaker { | |
| break | |
| } | |
| } | |
| } | |
| if !validSpeaker { | |
| log.Println("HandleTTSSetVoice: invalid speakerID selected:", speakerID) | |
| if err := s.InteractionRespond(i.Interaction, &discordgo.InteractionResponse{ | |
| Type: discordgo.InteractionResponseChannelMessageWithSource, | |
| Data: &discordgo.InteractionResponseData{ | |
| Embeds: []*discordgo.MessageEmbed{ | |
| { | |
| Title: "エラー", | |
| Description: "不正な話者IDが選択されました。もう一度お試しください。", | |
| Color: config.Colors.Error, | |
| Timestamp: time.Now().Format(time.RFC3339), | |
| }, | |
| }, | |
| Flags: discordgo.MessageFlagsEphemeral, | |
| }, | |
| }); err != nil { | |
| log.Println("Failed to respond interaction:", err) | |
| } | |
| return | |
| } |
|
|
||
| cachedSpeakers.mu.Lock() | ||
| cachedSpeakers.speakers = speakers | ||
| cachedSpeakers.expires = time.Now().Add(5 * time.Minute) |
There was a problem hiding this comment.
The cache expiration time 5 * time.Minute is hardcoded. For consistency with the codebase (see src/internal/util/dictionary_cache.go which defines DefaultCacheTTL = 5 * time.Minute as a constant), consider extracting this as a named constant like speakerCacheTTL.
1 participant
Overview
/tts set voiceの追加issue
#211
Review request
@yuito-it
etc...