refactor: leaf-helper consolidation — writeAuditAction + extractModalField (v3.2.2)#8
Conversation
…sites The dashboard-API handlers repeated `const triggeredBy = optionalString(body, 'triggeredBy'); await writeAuditLog(guildId, action, triggeredBy, details)`. New `writeAuditAction(guildId, body, action, details?)` folds the triggeredBy extraction in. Migrated the 16 single-use sites; left the 7 non-uniform ones (approve/deny `?? requireString`, bait `?? 'dashboard'` / multi-use triggeredBy, announcement `sentBy`/`createdBy`) untouched. Updated the mock.module(auditHelper) test fakes to add writeAuditAction. (Phase 3 API config-CRUD will build on this.) 1284 tests pass; build + biome clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ct footgun systemFlows' local getModalFieldValue (the only modal reader tolerant of both `.value` text inputs and `.values[]` selects) is promoted to utils/interactions/modalHelper.ts as `extractModalField` and exported from the barrel. systemFlows repointed (12 sites). Migrated 8 inline `getField(id)?.value` reads — including 2 announcement role/channel SELECT reads that were silently returning undefined (the actual footgun). Left checkbox-boolean reads (String coercion would break them) and full-array multi-select reads untouched. (Phase 4 bot-setup edit modals will use this.) 1284 tests pass; build + biome clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 39 minutes and 53 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR introduces two new utility helpers to reduce boilerplate in dashboard API handlers and command handlers, then migrates existing code to use them. Version is bumped to 3.2.2 with detailed changelog documentation of the additions and fixes. ChangesModal Field & Audit Helpers for v3.2.2
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/handlers/botSetup/systemFlows.ts`:
- Line 134: extractModalField returns string | undefined, so the checkbox read
for enabled is wrong: change how enabled is derived (the variable where
currently: const enabled = extractModalField(submit.fields,
'setup_staff_enable') ?? true) to coerce the modal field value to a boolean
consistently. Use the same pattern as other handlers: fetch the raw field via
extractModalField or submit.fields.getField('setup_staff_enable') and wrap with
Boolean(...) or compare to the string 'true' to ensure unchecked ("false" or
undefined) yields false; update any downstream logic that relies on enabled
accordingly.
In `@src/utils/api/handlers/auditHelper.ts`:
- Around line 66-74: writeAuditAction currently calls optionalString(body,
'triggeredBy') which can throw ApiError.badRequest for non-string values and
breaks the "best-effort" audit contract; change writeAuditAction to extract
triggeredBy tolerantly (e.g., check if body['triggeredBy'] is a string and use
it, otherwise set triggeredBy = undefined) before calling writeAuditLog so that
malformed triggeredBy values do not throw and the audit path remains
non-throwing; keep the function signature and ensure writeAuditLog is invoked
with undefined for invalid types instead of propagating optionalString's error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a1e40a6-5bcb-47e5-adb5-eb19cfaffbae
📒 Files selected for processing (23)
CHANGELOG.mdCLAUDE.mdpackage.jsonsrc/commands/handlers/announcement/setup.tssrc/commands/handlers/baitChannel/settings.tssrc/commands/handlers/botSetup/index.tssrc/commands/handlers/botSetup/systemFlows.tssrc/commands/handlers/ticket/typeEdit.tssrc/utils/api/handlers/announcementHandlers.tssrc/utils/api/handlers/applicationHandlers.tssrc/utils/api/handlers/auditHelper.tssrc/utils/api/handlers/configHandlers.tssrc/utils/api/handlers/memoryHandlers.tssrc/utils/api/handlers/permissionHandlers.tssrc/utils/api/handlers/reactionRoleHandlers.tssrc/utils/api/handlers/rulesHandlers.tssrc/utils/api/handlers/setupHandlers.tssrc/utils/api/handlers/ticketHandlers.tssrc/utils/interactions/index.tssrc/utils/interactions/modalHelper.tstests/unit/utils/api/applicationHandlers.test.tstests/unit/utils/api/setupHandlers.test.tstests/unit/utils/api/ticketHandlers.test.ts
- extractModalField now returns undefined for boolean (checkbox) values rather than stringifying them — a checkbox's `.value` is a boolean, and `String(false)` is the truthy "false". New `extractModalBoolean(fields, id, default?)` companion reads checkboxes correctly. Fixed the flagged systemFlows `setup_staff_enable` read (unchecking it was silently ignored) + unified the 7 other ad-hoc `getField(id)?.value as boolean` reads (drops the `as any` casts). - writeAuditAction extracts triggeredBy TOLERANTLY (was via optionalString, which throws a 400 on a non-string). Audit logging is best-effort and runs after side effects, so it must never abort an already-completed request. - +10 unit tests for extractModalField / extractModalBoolean (incl. the checkbox-footgun regression). 1294 tests pass; build + biome clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @coderabbitai — both addressed in 90ffcc0:
1294 tests pass; build + biome clean. |
|
Tip For best results, initiate chat on the files or code changes.
(ʘ‿ʘ)ノ🐇 ✏️ Learnings added
🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Summary
Phase 2 of the consolidation roadmap — two reusable helpers extracted, with one latent-bug fix. No behavior change otherwise.
writeAuditAction(api/handlers)Folds the repeated
const triggeredBy = optionalString(body,'triggeredBy'); await writeAuditLog(...)intowriteAuditAction(guildId, body, action, details?). 16 uniform sites migrated; 7 non-uniform sites (approve/deny?? requireString, bait?? 'dashboard'/multi-use, announcementsentBy/createdBy) left as-is. (Prereq for the Phase 3 API config-CRUD bundle.)extractModalField(utils/interactions)The field-type-tolerant modal reader (text
.valueand select.values[]) — promoted from a botSetup-local helper to the shared barrel. systemFlows repointed (12 sites) + 8 inlinegetField(id)?.valuereads migrated.🐛 Fixed
fields.getField(id)?.value, which silently returnsundefinedfor select components. Now read viaextractModalField(.values[]). Checkbox-boolean reads + full-array multi-selects were intentionally left (would break underString()coercion / are array reads).Verification
1284 tests pass;
bun run build+biomeclean. Reviewed: each migration confirmed single-use before folding; correctness over coverage.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores