refactor: centralize magic numbers and time constants#234
refactor: centralize magic numbers and time constants#234BillChirico wants to merge 0 commit intomainfrom
Conversation
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR introduces centralized time and rate-limit constants, refactors middleware defaults to use them, establishes code quality documentation, splits monolithic route files, enhances permission validation, and adds comprehensive test coverage. Changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
|
| Filename | Overview |
|---|---|
| src/constants/time.js | New centralized time constants module - all calculations are mathematically correct |
| src/utils/duration.js | Successfully replaced 6 magic number calculations with named constants - semantically equivalent |
| src/utils/cache.js | Replaced 60_000 with MS_PER_MINUTE in cleanup interval - minor constant reorganization |
| src/api/middleware/rateLimit.js | Replaced 15 * 60 * 1000 with RATE_LIMIT_WINDOW.SHORT - improves maintainability |
| src/utils/permissions.js | Added guild-scoped admin roles support to isGuildAdmin() - this is a behavioral change not documented in PR description |
Last reviewed commit: 1f8fced
There was a problem hiding this comment.
Pull request overview
This PR primarily centralizes time-related “magic numbers” into a shared constants module and updates several utilities/middleware to use those constants. It also introduces a functional change to permission evaluation by adding support for guild-scoped admin roles (permissions.adminRoles) in isGuildAdmin, along with new unit tests and task/documentation additions.
Changes:
- Add
src/constants/time.js(+src/constants/index.js) to centralize time durations and rate-limit windows. - Replace inline time calculations across duration parsing, cache cleanup, guild spend windows, and rate limiting.
- Extend
isGuildAdminto supportconfig.permissions.adminRolesand add targeted tests for the new behavior.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/utils/permissions.test.js | Adds coverage for isGuildAdmin behavior with adminRoles, fallback logic, and precedence. |
| src/utils/permissions.js | Implements guild-scoped admin roles support in isGuildAdmin and updates docs. |
| src/constants/time.js | Introduces centralized millisecond constants + rate-limit window presets. |
| src/constants/index.js | Re-exports constants to standardize imports. |
| src/utils/duration.js | Replaces inline unit multipliers and max duration with constants. |
| src/utils/guildSpend.js | Uses MS_PER_DAY as the default rolling spend window. |
| src/utils/cache.js | Uses MS_PER_MINUTE for cleanup interval and clarifies cache size constant placement. |
| src/api/middleware/rateLimit.js | Uses RATE_LIMIT_WINDOW.SHORT as default window. |
| src/api/middleware/redisRateLimit.js | Uses RATE_LIMIT_WINDOW.SHORT as default window and reformats options destructuring. |
| TASK.md | Adds a task write-up for the constants refactor (currently contains naming mismatches vs implementation). |
| CODE_IMPROVEMENTS.md | Adds a codebase improvement backlog (currently includes an outdated TODO reference). |
| tasks/task-001.md | Adds a task plan for splitting guilds.js routes. |
| tasks/task-002.md | Adds a task plan for adding missing tests in several modules/utils. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
CODE_IMPROVEMENTS.md
Outdated
| // src/utils/permissions.js:132 | ||
| // TODO(#71): check guild-scoped admin roles once per-guild config is implemented | ||
|
|
There was a problem hiding this comment.
This TODO entry references src/utils/permissions.js having a pending TODO(#71) for guild-scoped admin roles, but this PR implements that behavior (and the TODO/line numbers no longer match). Please update/remove this entry so the document stays accurate.
| // src/utils/permissions.js:132 | |
| // TODO(#71): check guild-scoped admin roles once per-guild config is implemented |
src/utils/permissions.js
Outdated
| /** | ||
| * Check if a member is a guild admin (has ADMINISTRATOR permission or bot admin role). | ||
| * Check if a member is a guild admin (has ADMINISTRATOR permission, bot admin role, or guild-scoped admin roles). | ||
| * | ||
| * Currently delegates to {@link isAdmin}. This is an intentional alias to establish | ||
| * a separate semantic entry-point for per-guild admin checks. When per-guild config | ||
| * lands (Issue #71), this function will diverge to check guild-scoped admin roles | ||
| * instead of the global bot admin role. | ||
| * Checks guild-scoped admin roles from per-guild config (permissions.adminRoles array), | ||
| * then falls back to global admin checks via {@link isAdmin}. |
There was a problem hiding this comment.
The PR description states this is a pure refactor of time/magic-number constants, but this change modifies permission behavior by adding support for config.permissions.adminRoles in isGuildAdmin. Please either update the PR description/scope (and note the behavioral change) or move the adminRoles feature + tests to a separate PR so reviewers can evaluate it independently.
src/utils/permissions.js
Outdated
| * Check if a member is a guild admin (has ADMINISTRATOR permission, bot admin role, or guild-scoped admin roles). | ||
| * | ||
| * Currently delegates to {@link isAdmin}. This is an intentional alias to establish | ||
| * a separate semantic entry-point for per-guild admin checks. When per-guild config | ||
| * lands (Issue #71), this function will diverge to check guild-scoped admin roles | ||
| * instead of the global bot admin role. | ||
| * Checks guild-scoped admin roles from per-guild config (permissions.adminRoles array), | ||
| * then falls back to global admin checks via {@link isAdmin}. | ||
| * |
There was a problem hiding this comment.
The JSDoc claims isGuildAdmin “checks guild-scoped admin roles ... then falls back to isAdmin”, but the implementation checks bot owner + Discord Administrator permission before checking adminRoles. Please adjust the wording so the documented precedence matches the actual behavior.
TASK.md
Outdated
| // Rate limit windows | ||
| export const RATE_LIMIT = { | ||
| SHORT: 15 * MS_PER_MINUTE, // 15 minutes | ||
| MEDIUM: MS_PER_HOUR, | ||
| LONG: MS_PER_DAY, | ||
| }; |
There was a problem hiding this comment.
The task snippet defines RATE_LIMIT and later suggests replacing usages with RATE_LIMIT.SHORT, but the implementation exports RATE_LIMIT_WINDOW (used in rateLimit.js/redisRateLimit.js). Please align the task documentation with the actual constant name to prevent copy/paste errors.
TASK.md
Outdated
| Replace inline calculations with imports: | ||
| - `24 * 60 * 60 * 1000` → `MS_PER_DAY` or `DURATION.DAY` | ||
| - `15 * 60 * 1000` → `RATE_LIMIT.SHORT` | ||
| - etc. |
There was a problem hiding this comment.
The replacement guidance refers to RATE_LIMIT.SHORT, but the code uses RATE_LIMIT_WINDOW.SHORT. Please update this example to match the exported constant name.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/utils/permissions.js`:
- Around line 120-149: You introduced a new authorization path in isGuildAdmin
by checking config.permissions.adminRoles which changes access semantics; either
revert that change from isGuildAdmin and open a separate feature/security PR or
make it opt-in (e.g., add and check a boolean flag like
config.permissions.enableGuildAdminRoles before using adminRoles), update
docs/rollout notes, and add tests; ensure isGuildAdmin still falls back to
isAdmin when the opt-in flag is false and reference the symbols isGuildAdmin,
config.permissions.adminRoles, config.permissions.enableGuildAdminRoles (new),
and isAdmin in the change.
In `@TASK.md`:
- Line 3: Fix markdownlint warnings MD022 and MD009 by ensuring each heading has
a blank line above and below it (e.g., the "Context" heading) and removing
trailing spaces from all checklist lines and list items; update TASK.md to
insert the required blank lines surrounding every Markdown heading and trim
whitespace at ends of lines (including checklist entries and any list items
mentioned in the comment) so no lines end with extra spaces.
- Around line 39-43: The doc defines a constant named RATE_LIMIT but the
codebase uses RATE_LIMIT_WINDOW; update the TASK.md occurrences of RATE_LIMIT
(and its members SHORT/MEDIUM/LONG) to RATE_LIMIT_WINDOW so the documentation
matches the implemented API (or conversely rename the implementation to
RATE_LIMIT if you prefer code change) — ensure all references in the file
(including the other occurrence noted) are updated to the same symbol name
RATE_LIMIT_WINDOW.
In `@tasks/task-001.md`:
- Line 7: Add a blank line immediately after the markdown headings to satisfy
MD022: insert an empty line after the "## Context" heading and likewise after
the other section headings present in this document (the second-level headings
around the other noted locations) so each heading is followed by a single blank
line before the next paragraph or content; update the markdown file where
headings are declared (e.g., the "## Context" heading and the other
top/second-level headings referenced in the review) to include that blank line.
In `@tasks/task-002.md`:
- Line 3: The MD022 heading-spacing violations are caused by missing blank lines
after headings (e.g., the "## Parent" heading and the other listed heading
instances); for each heading like "## Parent" and the other similar headings in
the document, insert a single blank line immediately after the heading so there
is an empty line before the following paragraph or list, ensuring all headings
comply with markdownlint MD022.
- Around line 69-70: Remove the trailing whitespace on the markdown lines shown
in the diff (the lines containing "**Commits:**" and "**Issues:**") to satisfy
markdownlint MD009; open the tasks/task-002.md content and trim any spaces at
the end of those lines (and any other lines flagged by MD009) so there are no
trailing spaces.
In `@tests/utils/permissions.test.js`:
- Around line 424-442: The test for isGuildAdmin should explicitly assert that
adminRoleId is not consulted when adminRoles matches: after the existing
positive assertions, add a negative assertion verifying member.roles.cache.has
was not called with the adminRoleId value ('global-admin-role') (or that any
function used to check adminRoleId was not called), so the test enforces that
adminRoles takes precedence over adminRoleId in isGuildAdmin.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (13)
CODE_IMPROVEMENTS.mdTASK.mdsrc/api/middleware/rateLimit.jssrc/api/middleware/redisRateLimit.jssrc/constants/index.jssrc/constants/time.jssrc/utils/cache.jssrc/utils/duration.jssrc/utils/guildSpend.jssrc/utils/permissions.jstasks/task-001.mdtasks/task-002.mdtests/utils/permissions.test.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Agent
- GitHub Check: Greptile Review
- GitHub Check: Docker Build Validation
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
src/**/*.{js,ts,jsx,tsx}: Use ESM only — Useimport/export, no CommonJS
Use single quotes for strings — No double quotes except in JSON
Always require semicolons at end of statements
Use 2-space indent, enforced by Biome
Always use async/await for asynchronous operations and promise handling
Files:
src/constants/index.jssrc/api/middleware/rateLimit.jssrc/utils/permissions.jssrc/api/middleware/redisRateLimit.jssrc/utils/duration.jssrc/constants/time.jssrc/utils/guildSpend.jssrc/utils/cache.js
{src,web}/**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use Winston logger from
src/logger.js, NEVER useconsole.*
Files:
src/constants/index.jssrc/api/middleware/rateLimit.jssrc/utils/permissions.jssrc/api/middleware/redisRateLimit.jssrc/utils/duration.jssrc/constants/time.jssrc/utils/guildSpend.jssrc/utils/cache.js
src/**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Use parameterized SQL queries — Never use string interpolation in database queries
Files:
src/constants/index.jssrc/api/middleware/rateLimit.jssrc/utils/permissions.jssrc/api/middleware/redisRateLimit.jssrc/utils/duration.jssrc/constants/time.jssrc/utils/guildSpend.jssrc/utils/cache.js
tests/**/*.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Maintain 80% test coverage threshold — Never lower the coverage requirement
Files:
tests/utils/permissions.test.js
🧠 Learnings (4)
📚 Learning: 2026-03-02T21:23:59.491Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-02T21:23:59.491Z
Learning: Applies to tests/**/*.{js,ts} : Maintain 80% test coverage threshold — Never lower the coverage requirement
Applied to files:
tasks/task-002.md
📚 Learning: 2026-03-02T21:23:59.491Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-02T21:23:59.491Z
Learning: Applies to src/modules/**/*.{js,ts} : Use Discord cache utilities — `src/utils/discordCache.js` for channels/roles/members, `src/utils/reputationCache.js` for leaderboard/rank data
Applied to files:
src/utils/cache.js
📚 Learning: 2026-03-02T21:23:59.491Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-02T21:23:59.491Z
Learning: Applies to src/modules/**/*.{js,ts} : Redis caching should use `src/utils/cache.js` for generic caching with Redis + in-memory fallback
Applied to files:
src/utils/cache.js
📚 Learning: 2026-03-02T21:23:59.491Z
Learnt from: CR
Repo: VolvoxLLC/volvox-bot PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-03-02T21:23:59.491Z
Learning: Create modules in `src/modules/` for new features, add config section to `config.json`, update `SAFE_CONFIG_KEYS`, create slash command if needed, add database migration if needed, write tests, and update dashboard UI if configurable
Applied to files:
tasks/task-001.md
🧬 Code graph analysis (2)
src/utils/permissions.js (4)
src/api/routes/config.js (1)
config(109-109)src/index.js (1)
config(91-91)src/utils/modAction.js (1)
config(64-64)src/api/routes/guilds.js (1)
config(658-658)
tests/utils/permissions.test.js (1)
src/utils/permissions.js (1)
isGuildAdmin(129-150)
🪛 LanguageTool
tasks/task-001.md
[style] ~14-~14: To elevate your writing, try using a synonym here.
Context: ...onsibility principle and makes the file hard to maintain. ## Files to Work On - `sr...
(HARD_TO)
🪛 markdownlint-cli2 (0.21.0)
TASK.md
[warning] 3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 7-7: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 16-16: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 26-26: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 34-34: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 40-40: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 45-45: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 51-51: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 60-60: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 64-64: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 65-65: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
tasks/task-002.md
[warning] 3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 7-7: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 15-15: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 24-24: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 31-31: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 37-37: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 44-44: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 51-51: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 58-58: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 65-65: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 69-69: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 70-70: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
tasks/task-001.md
[warning] 7-7: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 25-25: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 34-34: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 35-35: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 46-46: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 48-48: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
[warning] 65-65: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🔇 Additional comments (9)
CODE_IMPROVEMENTS.md (1)
1-176: Clear and actionable quality backlog document.The prioritization and file-level recommendations are well organized and useful for follow-up work.
src/utils/duration.js (1)
8-15: Nice refactor: constants centralization is clean and behavior-preserving.Using shared time constants here removes duplication and keeps duration math consistent across modules.
Also applies to: 18-23, 28-28
src/utils/cache.js (1)
12-13: Good constants refactor with no functional regression.The interval replacement to
MS_PER_MINUTEand retainedunref()keep behavior intact while improving maintainability.Also applies to: 32-34, 54-55
tests/utils/permissions.test.js (1)
349-422: Strong coverage expansion for guild-scoped admin role behavior.These cases materially improve confidence around
adminRoleshandling and fallback behavior.src/constants/index.js (1)
6-15: Clean constants barrel export.This keeps imports concise across the codebase and matches the centralization goal.
src/utils/guildSpend.js (1)
9-9: Good constant centralization with no behavior drift.Using
MS_PER_DAYfor default window parameters is clean and keeps existing behavior while removing magic numbers.Also applies to: 23-23, 59-59
src/api/middleware/rateLimit.js (1)
6-7: Default rate-limit window now correctly references shared constants.This keeps the middleware behavior intact while removing hardcoded durations.
Also applies to: 21-21
src/constants/time.js (1)
7-13: Constants module is clear and reusable.The base units and grouped presets are well-structured and support the intended magic-number cleanup.
Also applies to: 17-24, 29-33
src/api/middleware/redisRateLimit.js (1)
9-9: Redis middleware default window is now consistently centralized.Nice alignment with the in-memory rate limiter defaults through
RATE_LIMIT_WINDOW.SHORT.Also applies to: 23-27
src/utils/permissions.js
Outdated
| * Check if a member is a guild admin (has ADMINISTRATOR permission, bot admin role, or guild-scoped admin roles). | ||
| * | ||
| * Currently delegates to {@link isAdmin}. This is an intentional alias to establish | ||
| * a separate semantic entry-point for per-guild admin checks. When per-guild config | ||
| * lands (Issue #71), this function will diverge to check guild-scoped admin roles | ||
| * instead of the global bot admin role. | ||
| * Checks guild-scoped admin roles from per-guild config (permissions.adminRoles array), | ||
| * then falls back to global admin checks via {@link isAdmin}. | ||
| * | ||
| * @param {GuildMember} member - Discord guild member | ||
| * @param {Object} config - Bot configuration | ||
| * @param {Object} config - Bot configuration (should be guild-specific via getConfig(guildId)) | ||
| * @returns {boolean} True if member is a guild admin | ||
| */ | ||
| export function isGuildAdmin(member, config) { | ||
| // TODO(#71): check guild-scoped admin roles once per-guild config is implemented | ||
| if (!member) return false; | ||
|
|
||
| // Bot owner always bypasses permission checks | ||
| if (isBotOwner(member, config)) return true; | ||
|
|
||
| // Check if member has Discord Administrator permission | ||
| if (member.permissions?.has(PermissionFlagsBits.Administrator)) { | ||
| return true; | ||
| } | ||
|
|
||
| // Check guild-scoped admin roles (per-guild config) | ||
| const adminRoles = config?.permissions?.adminRoles; | ||
| if (Array.isArray(adminRoles) && adminRoles.length > 0) { | ||
| if (adminRoles.some((roleId) => member.roles?.cache?.has(roleId))) { | ||
| return true; | ||
| } | ||
| } | ||
|
|
||
| // Fall back to global admin checks (single adminRoleId, etc.) | ||
| return isAdmin(member, config); |
There was a problem hiding this comment.
Authorization semantics changed, so this is not a pure refactor.
This segment adds a new admin authorization path via config.permissions.adminRoles, which materially changes access control behavior. Please split this into a feature/security PR or update the PR scope and rollout notes accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/utils/permissions.js` around lines 120 - 149, You introduced a new
authorization path in isGuildAdmin by checking config.permissions.adminRoles
which changes access semantics; either revert that change from isGuildAdmin and
open a separate feature/security PR or make it opt-in (e.g., add and check a
boolean flag like config.permissions.enableGuildAdminRoles before using
adminRoles), update docs/rollout notes, and add tests; ensure isGuildAdmin still
falls back to isAdmin when the opt-in flag is false and reference the symbols
isGuildAdmin, config.permissions.adminRoles,
config.permissions.enableGuildAdminRoles (new), and isAdmin in the change.
TASK.md
Outdated
| @@ -0,0 +1,74 @@ | |||
| # Task: Centralize Magic Numbers and Time Constants | |||
|
|
|||
| ## Context | |||
There was a problem hiding this comment.
Fix markdownlint warnings before merge (MD022, MD009).
Headings are missing required surrounding blank lines, and there are trailing spaces on checklist lines.
Suggested cleanup
## Context
+
The codebase has time constants scattered throughout (24 * 60 * 60 * 1000 appearing 8+ times). This makes maintenance hard and configuration impossible.
## Files to Work On
+
- Create: `src/constants/time.js` - Time duration constants
...
-- [ ] src/constants/time.js created with all time constants
-- [ ] src/constants/index.js created for clean imports
+- [ ] src/constants/time.js created with all time constants
+- [ ] src/constants/index.js created for clean importsAlso applies to: 6-6, 17-17, 19-19, 46-46, 52-52, 57-57, 62-62, 70-70, 64-65
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TASK.md` at line 3, Fix markdownlint warnings MD022 and MD009 by ensuring
each heading has a blank line above and below it (e.g., the "Context" heading)
and removing trailing spaces from all checklist lines and list items; update
TASK.md to insert the required blank lines surrounding every Markdown heading
and trim whitespace at ends of lines (including checklist entries and any list
items mentioned in the comment) so no lines end with extra spaces.
TASK.md
Outdated
| export const RATE_LIMIT = { | ||
| SHORT: 15 * MS_PER_MINUTE, // 15 minutes | ||
| MEDIUM: MS_PER_HOUR, | ||
| LONG: MS_PER_DAY, | ||
| }; |
There was a problem hiding this comment.
Align task doc constant name with implemented API.
The task snippet uses RATE_LIMIT, but code changes use RATE_LIMIT_WINDOW. This inconsistency can mislead follow-up work.
Suggested doc fix
-export const RATE_LIMIT = {
+export const RATE_LIMIT_WINDOW = {
SHORT: 15 * MS_PER_MINUTE, // 15 minutes
MEDIUM: MS_PER_HOUR,
LONG: MS_PER_DAY,
};-- `15 * 60 * 1000` → `RATE_LIMIT.SHORT`
+- `15 * 60 * 1000` → `RATE_LIMIT_WINDOW.SHORT`Also applies to: 49-50
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 40-40: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@TASK.md` around lines 39 - 43, The doc defines a constant named RATE_LIMIT
but the codebase uses RATE_LIMIT_WINDOW; update the TASK.md occurrences of
RATE_LIMIT (and its members SHORT/MEDIUM/LONG) to RATE_LIMIT_WINDOW so the
documentation matches the implemented API (or conversely rename the
implementation to RATE_LIMIT if you prefer code change) — ensure all references
in the file (including the other occurrence noted) are updated to the same
symbol name RATE_LIMIT_WINDOW.
tasks/task-001.md
Outdated
| - **Master Task:** Code improvements from CODE_IMPROVEMENTS.md | ||
| - **Branch:** refactor/guilds-routes | ||
|
|
||
| ## Context |
There was a problem hiding this comment.
Fix markdownlint heading-spacing violations (MD022).
Add a blank line after these headings to satisfy lint and keep formatting consistent at Line 7, Line 25, Line 34, and Line 46.
Also applies to: 25-25, 34-34, 46-46
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 7-7: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tasks/task-001.md` at line 7, Add a blank line immediately after the markdown
headings to satisfy MD022: insert an empty line after the "## Context" heading
and likewise after the other section headings present in this document (the
second-level headings around the other noted locations) so each heading is
followed by a single blank line before the next paragraph or content; update the
markdown file where headings are declared (e.g., the "## Context" heading and
the other top/second-level headings referenced in the review) to include that
blank line.
tasks/task-002.md
Outdated
| @@ -0,0 +1,70 @@ | |||
| # Task 002: Add Missing Tests | |||
|
|
|||
| ## Parent | |||
There was a problem hiding this comment.
Fix markdownlint heading-spacing violations (MD022).
Add blank lines after the listed headings to satisfy markdownlint and keep the task template consistent.
Also applies to: 7-7, 15-15, 24-24, 31-31, 37-37, 44-44, 51-51, 58-58, 65-65
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 3-3: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tasks/task-002.md` at line 3, The MD022 heading-spacing violations are caused
by missing blank lines after headings (e.g., the "## Parent" heading and the
other listed heading instances); for each heading like "## Parent" and the other
similar headings in the document, insert a single blank line immediately after
the heading so there is an empty line before the following paragraph or list,
ensuring all headings comply with markdownlint MD022.
tasks/task-002.md
Outdated
| **Commits:** | ||
| **Issues:** |
There was a problem hiding this comment.
Remove trailing whitespace flagged by markdownlint (MD009).
Trim trailing spaces on these lines to keep lint clean.
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 69-69: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
[warning] 70-70: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tasks/task-002.md` around lines 69 - 70, Remove the trailing whitespace on
the markdown lines shown in the diff (the lines containing "**Commits:**" and
"**Issues:**") to satisfy markdownlint MD009; open the tasks/task-002.md content
and trim any spaces at the end of those lines (and any other lines flagged by
MD009) so there are no trailing spaces.
tests/utils/permissions.test.js
Outdated
| it('should prioritize adminRoles over adminRoleId when both are set', () => { | ||
| const member = { | ||
| permissions: { has: vi.fn().mockReturnValue(false) }, | ||
| roles: { | ||
| cache: { | ||
| has: vi.fn().mockImplementation((roleId) => roleId === 'guild-admin-role-1'), | ||
| }, | ||
| }, | ||
| }; | ||
| const config = { | ||
| permissions: { | ||
| adminRoles: ['guild-admin-role-1'], | ||
| adminRoleId: 'global-admin-role', | ||
| }, | ||
| }; | ||
| expect(isGuildAdmin(member, config)).toBe(true); | ||
| // Should match on adminRoles without checking adminRoleId | ||
| expect(member.roles.cache.has).toHaveBeenCalledWith('guild-admin-role-1'); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tighten the precedence test to enforce intent explicitly.
If this test is meant to guarantee adminRoles precedence, add a negative assertion that adminRoleId is not consulted in this success path.
Suggested assertion hardening
expect(isGuildAdmin(member, config)).toBe(true);
// Should match on adminRoles without checking adminRoleId
expect(member.roles.cache.has).toHaveBeenCalledWith('guild-admin-role-1');
+ expect(member.roles.cache.has).not.toHaveBeenCalledWith('global-admin-role');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/utils/permissions.test.js` around lines 424 - 442, The test for
isGuildAdmin should explicitly assert that adminRoleId is not consulted when
adminRoles matches: after the existing positive assertions, add a negative
assertion verifying member.roles.cache.has was not called with the adminRoleId
value ('global-admin-role') (or that any function used to check adminRoleId was
not called), so the test enforces that adminRoles takes precedence over
adminRoleId in isGuildAdmin.
BillChirico
force-pushed
the
feat/centralize-constants
branch
from
1f8fced to
4c49a09
Compare
|
No reviewable files after applying ignore patterns. |
Summary
Centralizes scattered magic number calculations into a dedicated constants module for better maintainability and configuration.
Changes
New Files
src/constants/time.js- Time duration constants (MS_PER_SECOND, MS_PER_MINUTE, etc.)src/constants/index.js- Clean re-exports for easy importsUpdated Files
src/utils/duration.js- Replaced 6 inline calculationssrc/utils/guildSpend.js- Replaced day calculation with MS_PER_DAYsrc/utils/cache.js- Replaced minute calculation with MS_PER_MINUTEsrc/api/middleware/rateLimit.js- Replaced window with RATE_LIMIT_WINDOW.SHORTsrc/api/middleware/redisRateLimit.js- Same replacementVerification
Commits
71de590refactor: create centralized time constants module55b2f64refactor: replace magic numbers in duration.js8e04806refactor: replace magic numbers in guildSpend.js5f735a9refactor: replace magic number in cache.js3420408refactor: replace magic number in rateLimit.jsa1ad8d0refactor: replace magic number in redisRateLimit.jsb403d2estyle: fix import sorting after constants refactorCloses code quality improvements from CODE_IMPROVEMENTS.md