Refactor CRM, API, and dashboard monoliths#372
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
📝 WalkthroughWalkthroughThis PR extracts admin dashboard UI code into dedicated modules, centralizes FastAPI route registration and request schemas, refactors Discord CRM selection flows onto a shared requester-scoped base, adjusts cog discovery order, converts the worker skills extractor into a shim over a shared implementation, switches the default model-profiles path to a packaged location, and consolidates the CI test step. ChangesAdmin Dashboard Frontend Refactor
Backend API Route and Schema Centralization
Discord CRM Selection Refactor
Worker/Model Catalog/CI Misc
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3d8fd7c5e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| _core.__package__ = __name__ | ||
| _core.__path__ = [str(Path(__file__).parent)] # type: ignore[attr-defined] | ||
| sys.modules[__name__] = _core |
There was a problem hiding this comment.
When Bot508.load_extensions() discovers this new package cog, discord.py 2.6 loads five08.discord_bot.cogs.crm via importlib.util.module_from_spec() and then looks for setup on the package module object it created. Replacing sys.modules[__name__] here does not add setup to that object, so loading the CRM extension raises NoEntryPointError and the bot starts without any CRM commands. Re-export _core.setup (and any needed symbols) from this __init__ instead of swapping sys.modules.
Useful? React with 👍 / 👎.
![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
🤖 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 `@apps/api/src/five08/backend/routes.py`:
- Around line 5-12: `register_routes` currently takes `api: Any`, which bypasses
static checking for the route contract and hides missing or renamed handlers.
Replace the `Any` annotation with a precise `Protocol` (or a
`TYPE_CHECKING`-only import of the expected API type) that lists the
route-facing attributes used inside `register_routes`, including
`_OptionalDirectoryStaticFiles`, so Pyrefly can validate the `api` surface while
preserving the circular-import workaround.
In `@apps/discord_bot/src/five08/discord_bot/cogs/crm/__init__.py`:
- Around line 1-16: The crm package shim is replacing the module object via
sys.modules, which prevents discord.py from seeing the expected extension entry
point on the originally created module. Update the __init__ compatibility module
to populate its own namespace from five08.discord_bot.cogs.crm.core instead of
reassigning sys.modules, so bot.load_extension can still discover setup; keep
the existing compatibility behavior while exposing the core module’s public
symbols through globals().
In `@apps/discord_bot/src/five08/discord_bot/cogs/crm/core.py`:
- Around line 493-494: The selection buttons for the CRM flows are missing the
same anti-double-click protection used by MemberAgreementSelectionButton. Update
CreateSSOUserSelectionButton, CreateUserAccountsSelectionButton,
OutlineInviteSelectionButton, MarkIdVerifiedSelectionButton, and
ReprocessResumeSelectionButton to set use_selection_lock = True and
disable_before_handle = True so the view is locked and disabled before handling
the action.
In `@packages/shared/src/five08/model_catalog.py`:
- Around line 36-43: load_model_profiles() silently falls back to
_EMPTY_MODEL_PROFILES when _load_packaged_model_profiles() returns no data, so
add a warning log on that failure path before returning the empty profiles.
Mirror the existing handling in llm.py and use the
load_model_profiles/_load_packaged_model_profiles symbols so the missing or
unreadable packaged resource is visible during startup.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5937a355-2f51-4c8e-a2d1-505d254abab4
📒 Files selected for processing (20)
.github/workflows/test.ymlapps/admin_dashboard/src/components/empty.tsxapps/admin_dashboard/src/components/sortable-table-head.tsxapps/admin_dashboard/src/configuration-view.test.tsxapps/admin_dashboard/src/main.tsxapps/admin_dashboard/src/views/configuration-view.tsxapps/admin_dashboard/src/views/newsletter-models.tsapps/admin_dashboard/src/views/newsletter-view.tsxapps/api/src/five08/backend/api.pyapps/api/src/five08/backend/routes.pyapps/api/src/five08/backend/schemas.pyapps/discord_bot/src/five08/discord_bot/bot.pyapps/discord_bot/src/five08/discord_bot/cogs/crm/__init__.pyapps/discord_bot/src/five08/discord_bot/cogs/crm/core.pyapps/worker/src/five08/worker/crm/skills_extractor.pypackages/shared/src/five08/model_catalog.pytests/evals/model-profiles.jsontests/evals/resume-extraction/README.mdtests/unit/test_bot.pytests/unit/test_model_catalog.py
💤 Files with no reviewable changes (1)
- tests/evals/model-profiles.json
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit bd31206. Configure here.
| for item in self.view.children: | ||
| if isinstance(item, discord.ui.Button): | ||
| item.disabled = True | ||
| await self.handle_selection(interaction, view) |
There was a problem hiding this comment.
Selection buttons disable before failure
Medium Severity
The refactoring of contact selection buttons introduced a regression. If an asynchronous CRM operation fails, the UI can be left in an unusable state. Buttons are either disabled or the view is locked before the operation's success is confirmed, preventing users from retrying or selecting another contact from the same prompt.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit bd31206. Configure here.
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 (2)
apps/discord_bot/src/five08/discord_bot/cogs/crm/core.py (2)
528-553: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winAdd shared best-effort audit records for base-level denials and callback failures.
The shared callback now handles requester mismatch, duplicate processing, and unexpected callback errors before the concrete CRM helpers run; only the member-agreement flow is described as auditing these outcomes. This leaves the other migrated CRM actions without worker-audit records for those human-triggered attempts. Centralize the audit call in the base hooks, or require every subclass to override them.
As per coding guidelines, "Human-triggered CRM actions from Discord should write to the worker audit ingest endpoint, audit writes must be best effort, and reusable audit-write logic should live outside individual cogs. Discord CRM audit writes should use
AUDIT_API_BASE_URLand the sharedAPI_SHARED_SECRET."Also applies to: 555-581
🤖 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 `@apps/discord_bot/src/five08/discord_bot/cogs/crm/core.py` around lines 528 - 553, Add shared best-effort audit logging in the base CRM callback hooks for all human-triggered denials and failures, not just the member-agreement flow. Centralize the worker-audit ingest write in the shared callback path around handle_requester_mismatch, handle_selection_already_processing, and the exception path in handle_callback_error, using AUDIT_API_BASE_URL and API_SHARED_SECRET with failure ignored so the main interaction still proceeds. Keep the reusable audit-write logic outside individual cogs so subclasses like the CRM helpers inherit the same base-level audit coverage.Source: Coding guidelines
551-581: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the fallback reply on
interaction.response.is_done().
handle_callback_error()always usesfollowup.send, but this callback can fail before the interaction is acknowledged. Useresponse.send_message(...)wheninteraction.response.is_done()is false, andfollowup.send(...)only after it’s already been acknowledged.🤖 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 `@apps/discord_bot/src/five08/discord_bot/cogs/crm/core.py` around lines 551 - 581, The fallback reply in handle_callback_error currently always uses interaction.followup.send, which will fail if the interaction has not been acknowledged yet. Update handle_callback_error in the CRM cog to check interaction.response.is_done() and send the callback_error_message with interaction.response.send_message(...) when it is false, otherwise keep using interaction.followup.send(...). Keep the existing error-handling flow around callback_error_log and handle_callback_error unchanged.
🧹 Nitpick comments (2)
apps/discord_bot/src/five08/discord_bot/cogs/crm/__init__.py (2)
14-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
dir(_core)-based export can leak core's own imports/private helpers.
dir(_core)includes names_coreitself imported at module scope (e.g. third-party modules,logger,settings) as long as they don't start with_; those get folded into__all__and copied into this package's namespace as part of its public surface. Separately, the copy loop (line 16-19) only skips full dunders, so single-underscore "private" names oncore(excluded from__all__) still get copied intoglobals(), which is inconsistent with the__all__filter above.🤖 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 `@apps/discord_bot/src/five08/discord_bot/cogs/crm/__init__.py` around lines 14 - 19, The export logic in the crm package init is too broad and inconsistent with its own public API filter. Update the module-level re-export in __init__ to build __all__ only from the intended public names on _core, and make the globals() copy loop use the same predicate so single-underscore helpers and imported internals from _core are not leaked into this package namespace.
10-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDynamic
ModuleTypeproxy is now redundant for existing symbols.Since all of
_core's public attributes are already copied intoglobals()at import time (lines 16-19),__getattr__/__setattr__/__delattr__on_CRMPackageModuleonly matter for attributes added to_coreafter this module is imported — an edge case unlikely to occur for a cog module. Maintaining both a static copy and a class-swap proxy adds complexity without a corresponding backward-compat requirement; per PEP 562, module-level lookups bypass__getattr__when the name is already a global, so the proxy is largely dead weight for the current attribute set. Consider dropping theModuleTypesubclass and relying solely on the static copy, unless the setattr/delattr forwarding to_coreis a deliberate requirement (e.g., tests monkeypatching via the package path expected to affectcore).♻️ Suggested simplification
-class _CRMPackageModule(ModuleType): - def __getattr__(self, name: str) -> object: - return getattr(_core, name) - - def __setattr__(self, name: str, value: object) -> None: - if not (name.startswith("__") and name.endswith("__")) and hasattr(_core, name): - setattr(_core, name, value) - super().__setattr__(name, value) - - def __delattr__(self, name: str) -> None: - if not (name.startswith("__") and name.endswith("__")) and hasattr(_core, name): - delattr(_core, name) - super().__delattr__(name) - - -sys.modules[__name__].__class__ = _CRMPackageModule🤖 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 `@apps/discord_bot/src/five08/discord_bot/cogs/crm/__init__.py` around lines 10 - 37, The _CRMPackageModule proxy is redundant because crm/__init__.py already copies _core’s public symbols into globals() and module-level lookup will use those first. Simplify the package by removing the ModuleType subclass and the sys.modules class-swap, and keep the direct re-export logic around __all__ and the globals() population unless you intentionally need setattr/delattr forwarding for monkeypatching or future dynamic symbols in _core.
🤖 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.
Outside diff comments:
In `@apps/discord_bot/src/five08/discord_bot/cogs/crm/core.py`:
- Around line 528-553: Add shared best-effort audit logging in the base CRM
callback hooks for all human-triggered denials and failures, not just the
member-agreement flow. Centralize the worker-audit ingest write in the shared
callback path around handle_requester_mismatch,
handle_selection_already_processing, and the exception path in
handle_callback_error, using AUDIT_API_BASE_URL and API_SHARED_SECRET with
failure ignored so the main interaction still proceeds. Keep the reusable
audit-write logic outside individual cogs so subclasses like the CRM helpers
inherit the same base-level audit coverage.
- Around line 551-581: The fallback reply in handle_callback_error currently
always uses interaction.followup.send, which will fail if the interaction has
not been acknowledged yet. Update handle_callback_error in the CRM cog to check
interaction.response.is_done() and send the callback_error_message with
interaction.response.send_message(...) when it is false, otherwise keep using
interaction.followup.send(...). Keep the existing error-handling flow around
callback_error_log and handle_callback_error unchanged.
---
Nitpick comments:
In `@apps/discord_bot/src/five08/discord_bot/cogs/crm/__init__.py`:
- Around line 14-19: The export logic in the crm package init is too broad and
inconsistent with its own public API filter. Update the module-level re-export
in __init__ to build __all__ only from the intended public names on _core, and
make the globals() copy loop use the same predicate so single-underscore helpers
and imported internals from _core are not leaked into this package namespace.
- Around line 10-37: The _CRMPackageModule proxy is redundant because
crm/__init__.py already copies _core’s public symbols into globals() and
module-level lookup will use those first. Simplify the package by removing the
ModuleType subclass and the sys.modules class-swap, and keep the direct
re-export logic around __all__ and the globals() population unless you
intentionally need setattr/delattr forwarding for monkeypatching or future
dynamic symbols in _core.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b62d7cd8-5f89-457b-990f-f97fcdb2cd7b
📒 Files selected for processing (5)
apps/api/src/five08/backend/api.pyapps/api/src/five08/backend/routes.pyapps/discord_bot/src/five08/discord_bot/cogs/crm/__init__.pyapps/discord_bot/src/five08/discord_bot/cogs/crm/core.pypackages/shared/src/five08/model_catalog.py
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/shared/src/five08/model_catalog.py
- apps/api/src/five08/backend/api.py
- apps/api/src/five08/backend/routes.py
1 participant
Summary
Verification
Summary by CodeRabbit
Note
Medium Risk
Broad structural moves across API route wiring, dashboard entrypoints, and Discord cog loading—intended behavior-preserving but high touch area if imports or handler registration drift.
Overview
This PR shrinks several large modules by moving code into dedicated files without changing the outward product behavior described in verification.
The admin dashboard pulls Configuration and Newsletter UI (plus related types/helpers) out of
main.tsxintoviews/configuration-view,views/newsletter-view, andviews/newsletter-models, and reuses new sharedEmptyandSortableTableHeadcomponents. Tests now import configuration types from the view module;main.tsxre-exports configuration symbols for compatibility.The backend API moves Pydantic request bodies to
schemas.pyand the longadd_api_routeblock toroutes.register_routes, which binds handlers from the existingapimodule via aBackendRouteSurfaceprotocol.The Discord bot loads package cogs (not only top-level
.pyfiles), keeps thecrmimport path via a compatibility package pointing atcrm.core, and deduplicates requester-scoped contact selection views/buttons behind shared base classes (member-agreement audit hooks preserved).The worker
skills_extractorbecomes a thin wrapper around sharedresume_skills_extractorinstead of a forked implementation.CI runs pytest once under coverage (with verbose/traceback flags) instead of a separate non-coverage pytest step. Model profiles are loaded only from
packages/shared/.../data/model-profiles.json; the duplicate undertests/evalsis removed and docs/tests point at the packaged catalog.Reviewed by Cursor Bugbot for commit bd31206. Bugbot is set up for automated code reviews on this repo. Configure here.