Merged
Conversation
WalkthroughAdds session recovery: new config schema and example, types and settings entry, a PlayerStateManager to read/save/delete states.json with savedAt/attempts, conditional persistence in UpdateHandler, readiness logic to validate/restore snapshots, and a localization key for disabled recovery messaging. Changes
Sequence Diagram(s)sequenceDiagram
participant UpdateHandler as UpdateHandler
participant Settings as Settings
participant FS as FileSystem
participant ReadyHandler as ReadyEvent
participant PlayerManager as PlayerStateManager
rect rgba(200,200,255,0.5)
UpdateHandler->>Settings: read sessionRecovery.enabled
alt enabled
UpdateHandler->>PlayerManager: collect current player states
PlayerManager-->>UpdateHandler: player states
UpdateHandler->>FS: write states.json (include savedAt)
else disabled
UpdateHandler-->>FS: skip writing states.json
end
end
rect rgba(200,255,200,0.5)
ReadyHandler->>Settings: read sessionRecovery.enabled, maxAge, maxAttempts
ReadyHandler->>FS: read states.json
alt states exist and enabled
ReadyHandler->>PlayerManager: shouldRestore(states)
alt shouldRestore == true
ReadyHandler->>PlayerManager: restorePlayer(snapshot)
PlayerManager-->>ReadyHandler: restored (success/failure)
ReadyHandler->>FS: delete states.json
else shouldRestore == false
ReadyHandler->>FS: delete states.json
end
else no states or disabled
ReadyHandler-->>PlayerManager: skip restore
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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 |
github-actions
bot
added
priority:p1
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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)
src/events/music/ready.ts (1)
99-99:⚠️ Potential issue | 🔴 CriticalBug:
returnexits entire function instead of skipping to next guild.Using
returnhere exits theexecute()function entirely when a snapshot lacksvoiceChannelId. This skips restoration for all remaining guilds and bypasses thedeletePlayerStates()cleanup at line 141, leaving stale state on disk.🐛 Proposed fix
- if (!snapshot.voiceChannelId) return; + if (!snapshot.voiceChannelId) continue;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/events/music/ready.ts` at line 99, In execute(), inside the loop that iterates guild snapshots (where the code currently does "if (!snapshot.voiceChannelId) return;"), change the early exit to skip only the current guild by using continue (or otherwise proceed to the next iteration) instead of returning from the whole function; this keeps the loop running for remaining guilds and ensures deletePlayerStates() is still reached and executed for cleanup. Ensure you update the check referencing snapshot.voiceChannelId in the same block and verify no other return statements prematurely exit execute() before deletePlayerStates().
🧹 Nitpick comments (1)
src/events/music/ready.ts (1)
38-40: Type mismatch: parameter type doesn't include metadata fields.The function is called with an object containing
savedAtandattemptsmetadata (line 88), but the parameter type only declaresRecord<string, QuaverPlayerJSON>. Consider aligning the type with the actual usage for clarity and type safety.♻️ Proposed fix
async function savePlayerStates( - states: Record<string, QuaverPlayerJSON>, + states: Record<string, QuaverPlayerJSON> & { + savedAt?: number; + attempts?: number; + }, ): Promise<void> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/events/music/ready.ts` around lines 38 - 40, The parameter type for savePlayerStates is too narrow—calls pass objects that include metadata fields savedAt and attempts—so update the function signature to accept the actual shape (e.g., Record<string, QuaverPlayerJSON & { savedAt: number | string; attempts?: number }>) or create a named interface (e.g., PlayerStateWithMeta) and use Record<string, PlayerStateWithMeta>; then update any call sites and related types to match so TypeScript reflects the presence of savedAt and attempts and preserves type safety for savePlayerStates and its callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CONFIGURATION.md`:
- Around line 90-93: Update the sessionRecovery defaults so they're consistent
across docs and the example config: change sessionRecovery.enabled to false and
ensure sessionRecovery.maxAge and sessionRecovery.maxAttempts match the values
in settings.example.json; update both occurrences in CONFIGURATION.md (the block
with "sessionRecovery": { "enabled": ..., "maxAge": ..., "maxAttempts": ... }
and the later documented defaults) so all three fields (sessionRecovery.enabled,
sessionRecovery.maxAge, sessionRecovery.maxAttempts) are identical to
settings.example.json.
---
Outside diff comments:
In `@src/events/music/ready.ts`:
- Line 99: In execute(), inside the loop that iterates guild snapshots (where
the code currently does "if (!snapshot.voiceChannelId) return;"), change the
early exit to skip only the current guild by using continue (or otherwise
proceed to the next iteration) instead of returning from the whole function;
this keeps the loop running for remaining guilds and ensures
deletePlayerStates() is still reached and executed for cleanup. Ensure you
update the check referencing snapshot.voiceChannelId in the same block and
verify no other return statements prematurely exit execute() before
deletePlayerStates().
---
Nitpick comments:
In `@src/events/music/ready.ts`:
- Around line 38-40: The parameter type for savePlayerStates is too narrow—calls
pass objects that include metadata fields savedAt and attempts—so update the
function signature to accept the actual shape (e.g., Record<string,
QuaverPlayerJSON & { savedAt: number | string; attempts?: number }>) or create a
named interface (e.g., PlayerStateWithMeta) and use Record<string,
PlayerStateWithMeta>; then update any call sites and related types to match so
TypeScript reflects the presence of savedAt and attempts and preserves type
safety for savePlayerStates and its callers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e9f83a6a-5a22-47c9-bb6f-0dde52c54177
📒 Files selected for processing (9)
CONFIGURATION.mdlocales/en/music.jssettings.example.jsonsrc/events/music/queueFinish.tssrc/events/music/ready.tssrc/events/music/trackStart.tssrc/lib/UpdateHandler.tssrc/lib/music/QuaverPlayer.tssrc/lib/util/settings.ts
…ChannelId is missing
github-actions
bot
added
the
affects:documentation
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/events/music/ready.ts (1)
123-141:⚠️ Potential issue | 🟠 MajorDon’t clear saved state after partial restore failures.
Line 141 unconditionally deletes
states.json, even when restore fails in the catch block at Line 123. That makes configured retry attempts largely ineffective for transient failures.💡 Preserve state when any restore fails
+ let hadRestoreFailure = false; for await (const [ guildId, guildData, ] of data.guild.instance.iterator()) { @@ } catch (error) { + hadRestoreFailure = true; logger.error( `[G ${guildId}] Failed to restore player`, error, ); } continue; } @@ - await deletePlayerStates(); + if (!hadRestoreFailure) { + await deletePlayerStates(); + } else { + await savePlayerStates(states); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/events/music/ready.ts` around lines 123 - 141, The code unconditionally calls deletePlayerStates() after attempting per-guild restores, which removes saved state even if any restore caught an error; change this by tracking whether any restore failed (e.g., a boolean like hadRestoreError) during the loop that contains the try/catch around player restore, set it true in the catch that logs the error, and only call deletePlayerStates() if hadRestoreError is false (or alternatively delete only the successfully restored guild entries instead of the whole file); refer to the restore loop/catch around logger.error(`[G ${guildId}] Failed to restore player`, error) and the deletePlayerStates() invocation to make the conditional deletion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/events/music/ready.ts`:
- Around line 41-44: The code currently calls deletePlayerStates() before
writeFile('states.json', ...), which can permanently lose state if the write
fails; instead, stop deleting upfront and perform an atomic replace: serialize
states to a temporary file (e.g., 'states.json.tmp') using writeFile, fsync if
available, then rename the temp into 'states.json' (or call fs.rename) only
after a successful write; remove the deletePlayerStates() call from the hot path
and, if you still need cleanup, run it only after successful replace or in a
safe recovery/rollback path, and ensure errors from write/rename are caught and
logged so the original file remains intact.
- Around line 64-86: The ready handler's early returns inside execute() stop the
rest of initialization (including the stay-channel bootstrap loop) when session
recovery is disabled or invalid; remove those returns and instead treat session
recovery as optional: after checking settings.sessionRecovery?.enabled, expired
states, or max attempts, log and call deletePlayerStates() as needed but do not
return from execute(); simply skip the restoration flow (e.g., by guarding the
restore logic with a boolean like shouldRestore) so the subsequent
stay-channel/bootstrap loop still runs. Ensure you update the code paths around
readPlayerStates, deletePlayerStates, and the restore section in execute() to
use the guard rather than returning.
---
Outside diff comments:
In `@src/events/music/ready.ts`:
- Around line 123-141: The code unconditionally calls deletePlayerStates() after
attempting per-guild restores, which removes saved state even if any restore
caught an error; change this by tracking whether any restore failed (e.g., a
boolean like hadRestoreError) during the loop that contains the try/catch around
player restore, set it true in the catch that logs the error, and only call
deletePlayerStates() if hadRestoreError is false (or alternatively delete only
the successfully restored guild entries instead of the whole file); refer to the
restore loop/catch around logger.error(`[G ${guildId}] Failed to restore
player`, error) and the deletePlayerStates() invocation to make the conditional
deletion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 89fa1334-66cc-4ddd-88fe-b51dde803697
📒 Files selected for processing (1)
src/events/music/ready.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/events/music/ready.ts (2)
13-18:⚠️ Potential issue | 🟠 MajorDo not skip stay bootstrap when restore fails for a guild.
If snapshot restore is invalid or throws, the current
continuestill bypasses the stay path for that guild.💡 Make restore outcome explicit
async function restorePlayer( guild: QuaverGuild<Initialized> & Guild, snapshot: QuaverPlayerJSON, -): Promise<void> { +): Promise<boolean> { try { - if (!snapshot.voiceChannelId) return; + if (!snapshot.voiceChannelId) return false; const player = await guild.client.music.players.createFromJSON( guild, snapshot, ); @@ if (snapshot.position > 0) { await player.seekTo(snapshot.position); } logger.info(`[G ${guild.id}] Player restored from saved state`); + return true; } catch (error) { logger.error(`[G ${guild.id}] Failed to restore player`, error); + return false; } } @@ - if (snapshot) { - await restorePlayer(guild, snapshot); + if (snapshot && (await restorePlayer(guild, snapshot))) { continue; }Also applies to: 36-38, 76-79
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/events/music/ready.ts` around lines 13 - 18, The restorePlayer function currently uses early returns/continues (e.g., when checking snapshot.voiceChannelId or when restore throws) that skip the "stay bootstrap" path for that guild; modify restorePlayer so that only the explicit no-voice case bypasses restoration, but any restoration failure or thrown error does not skip stay bootstrap—catch exceptions from the restore logic and after handling/logging the error explicitly invoke or allow the existing stay-bootstrap code to run for that guild (or set a flag to run it after the try/catch) so that restore failures still fall back to the stay behavior; reference restorePlayer and snapshot.voiceChannelId to locate the checks and the places where the continue/return currently prevents the stay bootstrap from executing.
58-58:⚠️ Potential issue | 🟠 MajorUse strict boolean check for recovery enablement.
Given settings are loaded from raw JSON, a truthy check can accidentally enable recovery for non-boolean values.
💡 Minimal fix
- if (settings.sessionRecovery?.enabled) { + if (settings.sessionRecovery?.enabled === true) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/events/music/ready.ts` at line 58, The current truthy check "if (settings.sessionRecovery?.enabled)" can misinterpret non-boolean JSON values as enabled; update the condition to an explicit boolean check (e.g., settings.sessionRecovery?.enabled === true) wherever session recovery is evaluated (notably the if in ready.ts referencing settings.sessionRecovery?.enabled) so only the literal boolean true enables recovery.src/lib/music/PlayerStateManager.ts (1)
21-25:⚠️ Potential issue | 🟠 MajorAvoid delete-before-write in snapshot persistence.
Line 22deletes the existing state file before writing the next one. If write fails, recovery data is lost.💡 Safer persistence approach
-import { readFile, unlink, writeFile } from 'node:fs/promises'; +import { readFile, rename, unlink, writeFile } from 'node:fs/promises'; async save(states: PlayerStatesRecord): Promise<void> { - await this.delete(); + const tmpPath = `${this.filePath}.tmp`; try { - await writeFile(this.filePath, JSON.stringify(states, null, 4)); + await writeFile(tmpPath, JSON.stringify(states, null, 4)); + await rename(tmpPath, this.filePath); } catch (error) { logger.error(error); + try { + await unlink(tmpPath); + } catch {} } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/music/PlayerStateManager.ts` around lines 21 - 25, The save method in PlayerStateManager currently calls delete() before writeFile, risking data loss if write fails; remove the pre-write delete and implement atomic persistence: write the JSON to a temporary file (e.g., this.filePath + '.tmp') using writeFile, fs.promises.writeFile or equivalent, then atomically rename/move the temp file to this.filePath using fs.promises.rename; ensure errors during write do not remove the existing file and clean up the temp file on failure, and update references to writeFile, delete, save, and filePath accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/music/PlayerStateManager.ts`:
- Around line 42-44: The current guard uses truthy checks that incorrectly treat
0 as falsy; change them to explicit numeric checks: replace the `states.savedAt
&& Date.now() - states.savedAt > (settings.sessionRecovery.maxAge ?? 86400) *
1000` style guard with an explicit numeric check such as `typeof states.savedAt
=== "number" && !Number.isNaN(states.savedAt) && Date.now() - states.savedAt >
(settings.sessionRecovery.maxAge ?? 86400) * 1000`, and similarly replace any
`states.attempts` truthy checks with `typeof states.attempts === "number" &&
!Number.isNaN(states.attempts) && states.attempts > 0` (or the appropriate
threshold) so zero is handled correctly; update the conditions in
PlayerStateManager where `states.savedAt` and `states.attempts` are used (lines
shown in the diff).
---
Duplicate comments:
In `@src/events/music/ready.ts`:
- Around line 13-18: The restorePlayer function currently uses early
returns/continues (e.g., when checking snapshot.voiceChannelId or when restore
throws) that skip the "stay bootstrap" path for that guild; modify restorePlayer
so that only the explicit no-voice case bypasses restoration, but any
restoration failure or thrown error does not skip stay bootstrap—catch
exceptions from the restore logic and after handling/logging the error
explicitly invoke or allow the existing stay-bootstrap code to run for that
guild (or set a flag to run it after the try/catch) so that restore failures
still fall back to the stay behavior; reference restorePlayer and
snapshot.voiceChannelId to locate the checks and the places where the
continue/return currently prevents the stay bootstrap from executing.
- Line 58: The current truthy check "if (settings.sessionRecovery?.enabled)" can
misinterpret non-boolean JSON values as enabled; update the condition to an
explicit boolean check (e.g., settings.sessionRecovery?.enabled === true)
wherever session recovery is evaluated (notably the if in ready.ts referencing
settings.sessionRecovery?.enabled) so only the literal boolean true enables
recovery.
In `@src/lib/music/PlayerStateManager.ts`:
- Around line 21-25: The save method in PlayerStateManager currently calls
delete() before writeFile, risking data loss if write fails; remove the
pre-write delete and implement atomic persistence: write the JSON to a temporary
file (e.g., this.filePath + '.tmp') using writeFile, fs.promises.writeFile or
equivalent, then atomically rename/move the temp file to this.filePath using
fs.promises.rename; ensure errors during write do not remove the existing file
and clean up the temp file on failure, and update references to writeFile,
delete, save, and filePath accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 39d4c534-ccab-408c-9256-aa049c4bc02e
📒 Files selected for processing (5)
src/events/music/queueFinish.tssrc/events/music/ready.tssrc/lib/music/PlayerStateManager.tssrc/lib/music/index.tssrc/lib/music/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/events/music/queueFinish.ts
zptx-bot
approved these changes
Mar 5, 2026
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/events/music/ready.ts (1)
79-92:⚠️ Potential issue | 🟠 MajorDo not delete recovery state when any guild restore fails.
Line 92 deletes all saved snapshots unconditionally. If restoration fails for a guild at Line 79, that snapshot is still erased, so next startup cannot retry recovery (making
maxAttemptsineffective for partial failures).💡 Suggested fix (preserve failed snapshots, delete only on full success)
+ const failedStates: PlayerStatesRecord = {}; + let hasRestoreFailure = false; for await (const [ guildId, guildData, ] of data.guild.instance.iterator()) { @@ const snapshot = states[guildId]; if (snapshot) { const restored = await restorePlayer(guild, snapshot); - if (restored) continue; + if (restored) continue; + failedStates[guildId] = snapshot; + hasRestoreFailure = true; } @@ } - await stateManager.delete(); + if (!settings.sessionRecovery?.enabled) return; + if (hasRestoreFailure) { + failedStates.savedAt = states.savedAt; + failedStates.attempts = states.attempts; + await stateManager.save(failedStates); + } else { + await stateManager.delete(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/events/music/ready.ts` around lines 79 - 92, The code currently calls stateManager.delete() unconditionally which erases all snapshots even if some guild restores failed; update the logic in the ready handler to only remove snapshots that were successfully restored (or call stateManager.delete() only when all restorePlayer(guild, snapshot) calls succeeded). Concretely, use restorePlayer's return value to track per-guild success (e.g., collect failedGuilds or a success flag) and either (a) call stateManager.delete() only when every restorePlayer returned true, or (b) delete individual snapshots for guilds that returned true while preserving those that failed so maxAttempts can retry later; adjust any loop around restorePlayer, snapshot and stateManager.delete accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/events/music/ready.ts`:
- Around line 79-92: The code currently calls stateManager.delete()
unconditionally which erases all snapshots even if some guild restores failed;
update the logic in the ready handler to only remove snapshots that were
successfully restored (or call stateManager.delete() only when all
restorePlayer(guild, snapshot) calls succeeded). Concretely, use restorePlayer's
return value to track per-guild success (e.g., collect failedGuilds or a success
flag) and either (a) call stateManager.delete() only when every restorePlayer
returned true, or (b) delete individual snapshots for guilds that returned true
while preserving those that failed so maxAttempts can retry later; adjust any
loop around restorePlayer, snapshot and stateManager.delete accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cc4b8d59-d3a6-426b-80b4-9b7b3fdbe903
📒 Files selected for processing (1)
src/events/music/ready.ts
zapteryx
added
status:confirmed
zptx-bot
added a commit
that referenced
this pull request
Mar 5, 2026
# [8.0.0-next.34](8.0.0-next.33...8.0.0-next.34) (2026-03-05) ### Features * configurable session recovery ([#1689](#1689)) ([4f796dd](4f796dd))
Member
|
🎉 This PR is included in version 8.0.0-next.34 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thank you for your interest in contributing!
Before proceeding, please review the guidelines for contributing.
nextbranch? (left side)Scope of change
Type of change
Priority
Description
Please describe the changes.
Adds the ability to configure session recovery - disabling, setting max age, and setting the maximum number of attempts
Summary by CodeRabbit
New Features
Documentation