config: demote magic-constant keys to module-local constants (closes #442)#467
Merged
Conversation
…442) Three config keys existed because the original code reached for getNumber() instead of declaring a constant — not because operators ever tuned them. Each surfaced in the Settings UI, demanded label/ description/type metadata, and added noise to the operator surface for zero practical benefit. Demoted (config key → module-local const): quotes.cleanup_interval → quote-channel-manager.ts CLEANUP_INTERVAL_MINUTES = 5 notices.cleanup_interval → notices-channel-manager.ts CLEANUP_INTERVAL_MINUTES = 5 voicechannels.ownership.grace_period_seconds → voice-channel-manager.ts OWNERSHIP_TRANSFER_GRACE_SECONDS = 30 Each module-local const carries a short comment explaining why the value is here and not in the schema, citing #442 so the demotion is self-documenting. Kept: quotes.max_length. Operators genuinely have preferences about quote length — some communities want short pithy quotes, some allow longer. Not a magic constant. Schema: removed the three keys from ConfigSchema, defaultConfig, and settingsMetadata. ConfigService.cleanupUnknownSettings() already runs on startup and deletes orphan DB rows for any key not in defaultConfig, so existing deployments shed the rows on next start with no extra migration code. All 744 tests pass.
Contributor
There was a problem hiding this comment.
Pull request overview
Demotes three never-tuned config keys to module-local constants to shrink the operator-facing settings surface area, leaving runtime behavior unchanged. Orphan DB rows for the removed keys are cleaned up automatically by the existing ConfigService.cleanupUnknownSettings() path on next startup.
Changes:
- Remove
voicechannels.ownership.grace_period_seconds,quotes.cleanup_interval, andnotices.cleanup_intervalfromConfigSchema,defaultConfig, andsettingsMetadata. - Replace the corresponding
configService.getNumber(...)lookups with module-localconsts carrying a comment that cites #442 as the demotion rationale. - Update log messages and cron-expression assembly to read from the new constants.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/services/config-schema.ts | Drops the three demoted keys from the schema interface, defaults map, and metadata table. |
| src/services/voice-channel-manager.ts | Adds OWNERSHIP_TRANSFER_GRACE_SECONDS = 30 constant and replaces the previous config lookup in the ownership-handoff path. |
| src/services/quote-channel-manager.ts | Adds CLEANUP_INTERVAL_MINUTES = 5 and uses it directly when building the cleanup cron expression and log message. |
| src/services/notices-channel-manager.ts | Same demotion pattern as the quote manager for the notices cleanup loop. |
| @@ -37,7 +36,6 @@ export interface ConfigSchema { | |||
| "quotes.delete_roles": string; // Comma-separated role IDs | |||
| "quotes.max_length": number; // Maximum quote length | |||
| "quotes.cooldown": number; // Cooldown in seconds between quote additions | |||
Copilot review caught that SETTINGS.md still documented the three keys this PR demoted. Anyone consulting the doc would be told to configure keys that no longer exist in defaultConfig — cleanupUnknownSettings() silently deletes them on startup, and the YAML import path now rejects them as "unknown key". Removed from SETTINGS.md: - The quotes settings table row and follow-up note for quotes.cleanup_interval. - The notices settings table row for notices.cleanup_interval. - The voicechannels.ownership.grace_period_seconds table row plus the entire "Ownership grace period" subsection with its "Recommended values" guidance. - The four Quick-reference entries for the same three keys. The constants now live in the relevant service modules; tuning them is a code change, not a config change.
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
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.
Closes #442.
Demoted
Three config keys existed because the original code reached for
getNumber()instead of declaring a constant — not because operators ever tuned them. Each surfaced in the Settings UI, demanded label/description/type metadata, and added noise to the operator surface for zero practical benefit.quotes.cleanup_intervalquote-channel-manager.ts—CLEANUP_INTERVAL_MINUTES = 5notices.cleanup_intervalnotices-channel-manager.ts—CLEANUP_INTERVAL_MINUTES = 5voicechannels.ownership.grace_period_secondsvoice-channel-manager.ts—OWNERSHIP_TRANSFER_GRACE_SECONDS = 30Each const carries a short comment explaining why the value lives in the module and not the schema, citing #442 so the demotion is self-documenting.
Kept
quotes.max_length— operators genuinely have preferences about quote length (some communities want short pithy quotes, some allow longer). Not a magic constant.How removal works
ConfigService.cleanupUnknownSettings()already runs on startup and deletes orphan DB rows for any key not indefaultConfig. Removing the three keys from the schema means existing deployments shed the rows on next start. No extra migration code.Verification
npm test— 744 passed, 1 skipped, 0 failed.npx tsc --noEmit— clean.npm run lint— 0 errors.npm run format:check— clean.grep -rn '<key>' srcreturns only the demotion-history comments for each removed key.🧹 Found 3 unknown/old settings in database, removing them...listing the three demoted keys.Test plan
Generated by Claude Code