Repository navigation
refactor: overengineering-audit cleanup (dead code + honest GA apply + telemetry) - #1263
Merged
Merged
Conversation
Consolidated overengineering-audit cleanup: - remove 3 dead frontend hooks (useCountUp, useLogsQueries, useAutoModQueries) (#1261) - remove never-configured Vercel Flags layer from FeatureToggleService (#1256) - delete dead backend GuildAutomationExecutionService (1,333 LOC) + its test (#1258) - guild-automation web apply is now honest: plan-only run record (status pending, no false autoAppliedOperations), UI no longer claims "Changes applied" (#1259) - instrument guild-automation usage (web plan/apply + bot /guildconfig) (#1260) - dedup unknown-field stripping in validate.ts via one stripUnknownFields helper Slice #1257 reduced to the validate dedup only: wrapHandler and developerAccess removal were rejected — both have real consumers (error-response mapping; test-mock seam), so they are not single-use overengineering. Includes ADRs 2026-06-06 (decommission, web-apply-plan-only, freeze-and-instrument). Closes #1256, closes #1258, closes #1259, closes #1260, closes #1261
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughThis PR implements the guild automation migration freeze strategy with a plan-only web apply interim behavior, adds per-guild usage instrumentation, removes the decommissioned backend execution service, strips never-configured Vercel Flags integration, and refactors validation middleware for code consolidation. ChangesGuild Automation Freeze & Plan-Only Implementation
Vercel Flags Removal
Supporting Infrastructure & Refactors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 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 docstrings
🧪 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 |
|
Failed to generate code suggestions for PR |
|
Size Change: -79 B (-0.02%) Total Size: 433 kB 📦 View Changed
ℹ️ View Unchanged
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/frontend/src/pages/GuildAutomation.test.tsx (2)
82-100:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate mock to reflect plan-only behavior.
The
mockApplyResultstill containsapplied: 2, failed: 0with changes marked asstatus: 'success', but according to the ADRs and stack context, the backend now returns pending runs with emptyautoAppliedOperations. The mock should be updated to reflect plan-only behavior withapplied: 0, failed: 0and changes withstatus: 'pending'(or removed changes array if empty).This mock is used in tests at lines 273, 302, and 332.
📝 Suggested update to mock
const mockApplyResult: ApplyResult = { - applied: 2, + applied: 0, failed: 0, - summary: 'Applied 2 changes successfully', - changes: [ - { - type: 'role', - resource: 'moderator', - action: 'create', - status: 'success', - }, - { - type: 'channel', - resource: 'announcements', - action: 'update', - status: 'success', - }, - ], + summary: 'Plan recorded (pending execution)', + changes: [], }🤖 Prompt for 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. In `@packages/frontend/src/pages/GuildAutomation.test.tsx` around lines 82 - 100, The mockApplyResult should represent plan-only behavior: change applied: 2 -> applied: 0 and failed: 0, update summary to reflect a plan (e.g., "Plan generated" or similar), and either set changes to an empty array or mark each change's status as 'pending' (prefer empty changes to mirror empty autoAppliedOperations). Update the ApplyResult mock object named mockApplyResult and ensure the tests that consume it (the ones referencing mockApplyResult in the GuildAutomation.test.tsx tests) expect applied: 0, failed: 0 and no successful statuses.
294-322:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTest assertions don't match plan-only behavior.
This test "displays apply result with statistics" expects to see "2 applied" (line 317) and "Applied 2 changes successfully" (lines 318-320) after clicking the "Record Plan" button. However, with the plan-only changes, the backend now returns pending runs with 0 applied operations. The test assertions should be updated to match the new behavior.
📝 Suggested test assertion updates
await waitFor(() => { expect(screen.getByText('Plan Record')).toBeInTheDocument() - expect(screen.getByText('2 applied')).toBeInTheDocument() + expect(screen.getByText('0 applied')).toBeInTheDocument() expect( - screen.getByText('Applied 2 changes successfully'), + screen.getByText(/Plan recorded/), ).toBeInTheDocument() })🤖 Prompt for 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. In `@packages/frontend/src/pages/GuildAutomation.test.tsx` around lines 294 - 322, The test 'displays apply result with statistics' in GuildAutomation should be updated because backend now returns plan-only (pending) runs with 0 applied operations; update the assertions after clicking the "Record Plan" button (in this test function and any related expectations that use api.automation.apply / mockApplyResult) to reflect zero applied results (e.g., assert the UI shows "0 applied" and the corresponding message for no applied changes such as "Applied 0 changes" or "No changes applied") instead of the previous "2 applied" and "Applied 2 changes successfully".
🧹 Nitpick comments (2)
packages/frontend/src/pages/GuildAutomation.tsx (2)
454-463: ⚖️ Poor tradeoffSemantic mismatch between heading and result view component.
The heading now says "Plan Record" (line 458) reflecting the plan-only behavior, but the component
ApplyResultView(line 462) still displays apply-oriented statistics like "applied" and "failed" counts (lines 160-167). This creates potential UX confusion where users see a "Plan Record" heading with content showing "0 applied" underneath.Consider either:
- Renaming
ApplyResultViewto a more neutral name likeOperationResultVieworRunResultView- Updating the component to use plan-oriented language when used in plan-only context ("planned" instead of "applied")
- Creating a separate
PlanRecordViewcomponent specific to plan-only resultsThe current implementation will work functionally (showing 0 applied), but the mixed terminology may confuse users.
🤖 Prompt for 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. In `@packages/frontend/src/pages/GuildAutomation.tsx` around lines 454 - 463, The "Plan Record" heading is semantically mismatched with the apply-focused ApplyResultView component; update the UI so terminology aligns: either rename ApplyResultView to a neutral name (e.g., OperationResultView/RunResultView) or add a mode prop to ApplyResultView (e.g., mode='apply'|'plan') and branch its labels/stats to show "planned" instead of "applied" when mode === 'plan'; alternatively implement a small PlanRecordView that reuses ApplyResultView logic but renders plan-oriented labels; update the usage next to the Plan Record heading to pass the new prop or use the new component so displayed text matches "Plan Record".
440-440: 💤 Low valueConsider clarifying the comment text.
The comment reads "Plan / Plan Records" which seems redundant. Should this perhaps be "Plan / Apply Results" (if both plan and apply results can appear here) or just "Plan Results" (if only plan records appear)?
🤖 Prompt for 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. In `@packages/frontend/src/pages/GuildAutomation.tsx` at line 440, Update the ambiguous inline comment "// Plan / Plan Records (surface-panel groups)" to a clearer description that reflects the actual content shown by the UI; for example change it to "// Plan / Apply Results (surface-panel groups)" if both plan and apply results can appear, or to "// Plan Results (surface-panel groups)" if only plan records are present, ensuring the comment near the surface-panel groups accurately describes the data.
🤖 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 `@packages/backend/src/routes/guildAutomation.ts`:
- Around line 110-113: Telemetry logs currently include a userId (e.g., the
infoLog call that sends { guildId, userId } with message 'Guild Automation plan
recorded'), which violates non-PII per-guild telemetry; remove userId from the
payload in that infoLog and the other similar infoLog calls in this file (the
blocks that log guild automation actions/messages around the same patterns),
sending only non-PII fields such as { guildId } (or a guild-level anonymized
identifier if required) and ensure no other references to userId are added to
telemetry payloads.
- Line 109: The usage counter guildAutomationUsageTotal currently increments
only after createPlan/createApplyRun succeed; move or add an increment call to
record attempts by calling guildAutomationUsageTotal.inc({ operation: 'plan' })
(and similarly for 'apply') immediately before invoking
createPlan/createApplyRun in the handlers, and update the post-call logic to
increment a distinct outcome label (e.g. outcome: 'success' or outcome:
'failure') or increment a failure-specific metric on error; ensure you touch the
code paths that currently call guildAutomationUsageTotal.inc (the pre-call
placement around createPlan/createApplyRun and their error handlers) so attempts
and final outcomes are both recorded.
In `@packages/bot/src/functions/management/commands/guildconfig.ts`:
- Around line 260-267: The telemetry call inside infoLog is currently including
interaction.user.id (userId) which exposes PII for this per-guild non-PII
stream; update the object passed to infoLog in the Guild Automation handler to
remove the userId field and only include guildId (guild.id), subcommand, and
blockedByProtected (leave the surrounding message and infoLog invocation
intact), making sure any references to interaction.user.id in that log context
are deleted or commented out so only non-PII fields are emitted.
In `@packages/shared/src/services/FeatureToggleService.ts`:
- Around line 59-62: Update the integration test mocks in
packages/backend/tests/integration/routes/toggles.test.ts so they align with the
new FeatureToggleService behavior: change mocked return value of
FeatureToggleService.getGlobalToggleProvider() from 'vercel' to 'database' (or
adapt the assertion to expect 'database'), and update the mock for
FeatureToggleService.getGlobalToggleStatus() to return the new shape/values
(provider: 'database' or 'environment' and writable: true) so assertions match
FeatureToggleService.getGlobalToggleStatus() and getGlobalToggleProvider()
implementations.
---
Outside diff comments:
In `@packages/frontend/src/pages/GuildAutomation.test.tsx`:
- Around line 82-100: The mockApplyResult should represent plan-only behavior:
change applied: 2 -> applied: 0 and failed: 0, update summary to reflect a plan
(e.g., "Plan generated" or similar), and either set changes to an empty array or
mark each change's status as 'pending' (prefer empty changes to mirror empty
autoAppliedOperations). Update the ApplyResult mock object named mockApplyResult
and ensure the tests that consume it (the ones referencing mockApplyResult in
the GuildAutomation.test.tsx tests) expect applied: 0, failed: 0 and no
successful statuses.
- Around line 294-322: The test 'displays apply result with statistics' in
GuildAutomation should be updated because backend now returns plan-only
(pending) runs with 0 applied operations; update the assertions after clicking
the "Record Plan" button (in this test function and any related expectations
that use api.automation.apply / mockApplyResult) to reflect zero applied results
(e.g., assert the UI shows "0 applied" and the corresponding message for no
applied changes such as "Applied 0 changes" or "No changes applied") instead of
the previous "2 applied" and "Applied 2 changes successfully".
---
Nitpick comments:
In `@packages/frontend/src/pages/GuildAutomation.tsx`:
- Around line 454-463: The "Plan Record" heading is semantically mismatched with
the apply-focused ApplyResultView component; update the UI so terminology
aligns: either rename ApplyResultView to a neutral name (e.g.,
OperationResultView/RunResultView) or add a mode prop to ApplyResultView (e.g.,
mode='apply'|'plan') and branch its labels/stats to show "planned" instead of
"applied" when mode === 'plan'; alternatively implement a small PlanRecordView
that reuses ApplyResultView logic but renders plan-oriented labels; update the
usage next to the Plan Record heading to pass the new prop or use the new
component so displayed text matches "Plan Record".
- Line 440: Update the ambiguous inline comment "// Plan / Plan Records
(surface-panel groups)" to a clearer description that reflects the actual
content shown by the UI; for example change it to "// Plan / Apply Results
(surface-panel groups)" if both plan and apply results can appear, or to "//
Plan Results (surface-panel groups)" if only plan records are present, ensuring
the comment near the surface-panel groups accurately describes the data.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 82f5390c-63f0-40ac-82ab-9ac39b9a60a8
📒 Files selected for processing (21)
decisions/2026-06-06-decommission-backend-guild-automation-execution-service.mddecisions/2026-06-06-guild-automation-migration-freeze-and-instrument.mddecisions/2026-06-06-web-guild-automation-apply-plan-only.mdpackages/backend/src/middleware/validate.tspackages/backend/src/routes/guildAutomation.tspackages/backend/src/services/GuildAutomationExecutionService.tspackages/backend/src/utils/prometheus.tspackages/backend/tests/unit/services/GuildAutomationExecutionService.test.tspackages/bot/src/functions/management/commands/guildconfig.tspackages/frontend/src/hooks/index.tspackages/frontend/src/hooks/useAutoModQueries.tspackages/frontend/src/hooks/useCountUp.test.tspackages/frontend/src/hooks/useCountUp.tspackages/frontend/src/hooks/useLogsQueries.tspackages/frontend/src/pages/GuildAutomation.test.tsxpackages/frontend/src/pages/GuildAutomation.tsxpackages/shared/src/config/index.tspackages/shared/src/config/vercelFlags.tspackages/shared/src/services/FeatureToggleService.spec.tspackages/shared/src/services/FeatureToggleService.tspackages/shared/src/services/guildAutomation/GuildAutomationOrchestrator.ts
💤 Files with no reviewable changes (9)
- packages/frontend/src/hooks/useCountUp.ts
- packages/shared/src/config/vercelFlags.ts
- packages/frontend/src/hooks/useLogsQueries.ts
- packages/frontend/src/hooks/useCountUp.test.ts
- packages/frontend/src/hooks/useAutoModQueries.ts
- packages/shared/src/config/index.ts
- packages/frontend/src/hooks/index.ts
- packages/backend/src/services/GuildAutomationExecutionService.ts
- packages/backend/tests/unit/services/GuildAutomationExecutionService.test.ts
📜 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). (9)
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Socket Security: Project Report
- GitHub Check: Test — frontend
- GitHub Check: Checks
- GitHub Check: Test — backend
- GitHub Check: Test — bot
- GitHub Check: quality / SAST (CodeQL) (javascript-typescript)
- GitHub Check: compressed-size
- GitHub Check: Build — backend
🔇 Additional comments (9)
packages/backend/src/middleware/validate.ts (1)
7-7: LGTM!Also applies to: 9-15, 44-50, 65-71
packages/shared/src/services/FeatureToggleService.ts (1)
9-9: LGTM!Also applies to: 59-61
packages/shared/src/services/FeatureToggleService.spec.ts (2)
181-207: LGTM!
162-166: LGTM!decisions/2026-06-06-decommission-backend-guild-automation-execution-service.md (1)
1-84: LGTM!decisions/2026-06-06-guild-automation-migration-freeze-and-instrument.md (1)
1-82: LGTM!decisions/2026-06-06-web-guild-automation-apply-plan-only.md (1)
1-83: LGTM!packages/shared/src/services/guildAutomation/GuildAutomationOrchestrator.ts (1)
178-190: LGTM!packages/backend/src/utils/prometheus.ts (1)
53-62: LGTM!
Address PR review (CodeRabbit + SonarCloud) on the usage instrumentation: - extract recordAutomationUsage() helper, called on entry → counts attempts not just successes (failed attempts no longer dropped from the freeze-gate signal), and dedups the inc+log block (fixes sonarcloud new-code duplication) - log operation + guildId only; drop user-id from web and bot telemetry (restores the per-guild, non-pii scope) - remove the now-unreachable 'vercel' value from GlobalFeatureToggleProvider (FeatureToggleService always returns 'database' after the vercel-flags removal) and update the toggles integration test accordingly
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Closed
5 tasks
3 tasks
Addresses code-review findings on the overengineering-cleanup PR: - bot: increment a matching lucky_guild_automation_usage_total counter from the /guildconfig plan/apply/reconcile command so the migration-freeze demand signal spans both surfaces (web/API + Discord), not just the web — the gate sums the two. The bot counter lives in its own registry (service=lucky-bot). - shared: add GuildAutomationOrchestrator.createApplyRun tests asserting the honest web-apply contract — status 'pending' (not 'completed'), empty autoAppliedOperations, planRecorded; plus the blocked-by-protected path. - frontend: complete the vercel provider removal (closes the frontend half of the dead-code cleanup) — drop 'vercel' from GlobalFeatureToggleProvider, the provider label map, and the three tests that referenced it. - backend: correct the recordAutomationUsage doc comment (runs after auth, so 401s aren't counted) and note the bot-side counterpart.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
This was referenced Jun 12, 2026
This was referenced Jun 12, 2026
LucasSantana-Dev
added a commit
that referenced
this pull request
Jun 12, 2026
Closes #1260. State-check first: the instrumentation this issue asks for already shipped in #1263 (48567f0) — `lucky_guild_automation_usage_total` Prometheus counter (label: operation, bounded cardinality; guildId in structured logs only) incremented via `recordAutomationUsage()` on backend `POST /automation/plan|apply|reconcile` and on bot `/guildconfig plan|apply|reconcile`, surfaced at the existing `/metrics` endpoints (readable in Grafana for the 2026-07-06 freeze-gate review per `decisions/2026-06-06-guild-automation-migration-freeze-and-instrument.md`). This PR adds the missing verification coverage: 6 tests asserting the counter increments on all three operations on both surfaces and that counter failure never breaks the request path. Verification: backend 67 suites / 1015 tests; bot 2471 tests pass. <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Add tests to verify the `lucky_guild_automation_usage_total` counter increments on plan, apply, and reconcile for both backend POST `/automation` routes and the bot `/guildconfig` command. Aligns with Linear #1260 by confirming telemetry is emitted with the correct `operation` label without affecting request handling. <sup>Written for commit f8cef2e. Summary will update on new commits.</sup> <a href="https://cubic.dev/pr/LucasSantana-Dev/Lucky/pull/1349?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img](https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img){kind=link}
![https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a](https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a){kind=link}
This branch was successfully deployed
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
1 participant
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.
Consolidated overengineering-audit cleanup (one PR per the chosen delivery). Net −~3,700 LOC, fully verified locally before push.
What's in here
useCountUp/useLogsQueries/useAutoModQueries(refactor(web): remove 3 dead React-Query/animation hooks #1261); never-configured Vercel Flags layer (refactor(shared): remove never-configured Vercel Flags layer #1256); backendGuildAutomationExecutionService1,333 LOC + its test (refactor(backend): delete dead GuildAutomationExecutionService (1,333 LOC) #1258).createApplyRunno longer records a falsecompletedrun with phantomautoAppliedOperations(now plan-only /pending); the UI no longer claims "Changes applied" and points users to/guildconfig apply. Fixes a misleading no-op + a false audit trail./plan+/applyand the bot/guildconfig apply— the input the freeze decision gates on.stripUnknownFieldshelper (the safe part of refactor(backend): inline single-use util layers (wrapHandler, developerAccess, validate dedup) #1257).#1257 scope reduced — by design
Only the
validate.tsdedup was kept.wrapHandler→asyncHandlerand thedeveloperAccessinline were rejected: verification showed both have real consumers (in-handler error→response mapping thatartists.testdepends on; a test-mock seam used byadmin/togglestests) — i.e. not single-use overengineering. The evidence test, vindicated.Verification (local, pre-push)
main) — now tracked as fix(shared): environment.test.ts not env-isolated + leaks all secrets to logs on failure #1262.Closes #1256, #1258, #1259, #1260, #1261. (#1257 partially — see above; #1262 tracks the pre-existing env-test bug found during verification.)
Summary by cubic
Cleaned up overengineering: removed dead code and the unused backend executor, made Guild Automation web “Apply” record a plan only (status pending), and added attempt-based usage telemetry across web and bot without PII. Result: ~3,700 LOC deleted and honest audit trails.
Refactors
GuildAutomationExecutionServiceand its unit tests.FeatureToggleService; fully dropped'vercel'from provider types, the frontend label map, and tests.useCountUp,useLogsQueries,useAutoModQueries.stripUnknownFieldshelper in backendvalidate.ts.lucky_guild_automation_usage_totaland arecordAutomationUsagehelper; counts plan/apply/reconcile attempts on the web API and the bot/guildconfigcommand, labels by operation only, and logs operation+guild (no user ID).Migration
pendingrun. Use/guildconfig applyin Discord to execute changes (UI now says “Record Plan”).Written for commit ce86fea. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
/guildconfig applyDiscord command.Documentation