Refactor Discord package for long-term maintainability - #1
Conversation
Split commands, pagination, and manifest/client concerns into focused modules, centralize error handling with Sentry-aware logging, and clean up startup/sync flow to reduce coupling and improve readability. Made-with: Cursor
Cover clan-id resolution, Discord relative time formatting fallbacks, and pagination token/custom-id parsing to lock in behavior during refactors. Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
Made-with: Cursor
There was a problem hiding this comment.
Pull request overview
Refactors the Discord ingress/bot codebase into smaller domain-focused modules (manifest, pagination, command helpers) while centralizing deferred-error reporting and introducing optional Sentry initialization during app startup.
Changes:
- Split large monolithic handlers into
src/commands/*,src/pagination/*, andsrc/manifest/*modules with focused helpers and shared utilities. - Introduced
create_app()insrc/app_factory.pyand simplifiedsrc/main.pyto only expose the FastAPI app. - Added optional Sentry initialization + logger capture support, and added a set of
unittest-based tests for key helpers.
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_subscription_helpers.py | Adds unit tests for subscription helper functions and route constants. |
| tests/test_subscribe_resolution.py | Adds unit tests for clan group id parsing. |
| tests/test_shared_time_format.py | Adds unit tests for ISO-to-Discord relative timestamp formatting. |
| tests/test_raidhub_client_envelope.py | Adds unit tests for envelope normalization behavior. |
| tests/test_pagination_tokens_and_ids.py | Adds unit tests for pagination custom IDs and nav token parsing. |
| tests/test_manifest_commands.py | Adds unit tests to enforce stable command manifest shape/names. |
| src/sync_commands.py | Updates command manifest import to new src/manifest package. |
| src/subscribe_handlers.py | Deletes legacy subscribe/unsubscribe handler module (moved into src/commands). |
| src/structured_logger.py | Adds Sentry exception capture to error() logging. |
| src/sentry_init.py | New helper to initialize Sentry from settings. |
| src/raidhub_client_types.py | Extracts RaidHubEnvelopeCode into its own module. |
| src/raidhub_client_envelope.py | New module for normalizing RaidHub envelope responses. |
| src/raidhub_client.py | Refactors headers creation and delegates envelope normalization to shared helper. |
| src/pagination/tokens.py | Extracts token parsing/clamping helpers. |
| src/pagination/session_pager.py | Deletes legacy monolithic pagination implementation (split into new modules). |
| src/pagination/runtime.py | New runtime/session store + pager dispatch logic. |
| src/pagination/ids.py | New helpers for parsing/building pager custom_ids. |
| src/pagination/components.py | New helpers for building pager action rows (Prev/Next/Start). |
| src/pagination/init.py | Re-exports the refactored pagination API surface. |
| src/manifest/schema.py | New DTO schema for command/option JSON generation. |
| src/manifest/builders.py | Refactors command building into build_commands() and shared option builders. |
| src/manifest/init.py | New wrapper for building the JSON command manifest from DTOs. |
| src/main.py | Simplifies app entrypoint to app = create_app(). |
| src/interaction_handlers.py | Deletes legacy monolithic interaction handlers module (moved into app_factory + src/commands). |
| src/config.py | Adds Sentry-related settings and env parsing. |
| src/commands/unsubscribe.py | Routes + error reporting refactor; uses shared subscription helpers and centralized exception reporting. |
| src/commands/subscription_routes.py | New constants for subscription route IDs. |
| src/commands/subscription_helpers.py | New module for subscription payload building + status formatting + error messaging. |
| src/commands/subscription.py | Refactors subscription command to use extracted helpers and centralized exception reporting. |
| src/commands/subscribe_resolution.py | Extracts clan/player resolution logic from subscribe handler. |
| src/commands/subscribe.py | Refactors subscribe command to use extracted resolution + centralized exception reporting. |
| src/commands/shared.py | Adds iso_to_discord_relative() + report_deferred_exception() utilities. |
| src/commands/player_search_helpers.py | Extracts player search pager rendering + formatting helpers. |
| src/commands/player_search.py | Refactors player-search command to use extracted helpers and centralized exception reporting. |
| src/commands/instance.py | Refactors instance command to use centralized exception reporting. |
| src/cli_sync_commands.py | Changes CLI entrypoint wrapper to call into src.sync_commands.cli(). |
| src/app_factory.py | New create_app() composing settings, clients, routing, signature validation, and metrics. |
| pyproject.toml | Adds build system metadata and Sentry dependency; updates console script mapping. |
| README.md | Updates documentation to match new structure and adds observability section. |
| .github/workflows/start.yml | Adds package build step and updates Python version. |
| .github/workflows/pr-ci.yml | Updates Python version and switches to compileall for syntax check. |
| .github/workflows/deploy.yml | Adds package build step and updates Python version. |
| .env.example | Adds Sentry env vars (currently with a concrete DSN value). |
Comments suppressed due to low confidence (1)
pyproject.toml:24
sync-discord-commandsis configured to loadcli_sync_commands:cli, but there is no top-levelcli_sync_commands.pymodule in the repo (the file lives atsrc/cli_sync_commands.py). As-is, the console script will raiseModuleNotFoundErrorwhen installed/built. Update the entry point tosrc.cli_sync_commands:cli(or add proper setuptoolspackage-dirconfig / move the module so the import path matches).
[project.scripts]
sync-discord-commands = "cli_sync_commands:cli"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SENTRY_DSN="https://11a145d0ec878f0306afca7a63062d6d@o4510296341217280.ingest.us.sentry.io/4511267202072576" | ||
| SENTRY_ENVIRONMENT=development | ||
| SENTRY_RELEASE= | ||
| SENTRY_SEND_DEFAULT_PII=true | ||
| SENTRY_TRACES_SAMPLE_RATE=0.1 |
There was a problem hiding this comment.
.env.example includes a concrete Sentry DSN value. This can unintentionally send local/dev error data to a real Sentry project (and effectively publishes the DSN). Replace it with an empty value or an obvious placeholder DSN, and document that users should supply their own DSN via SENTRY_DSN.
| def cli() -> int: | ||
| from src.sync_commands import cli as sync_cli | ||
|
|
||
| return sync_cli() |
There was a problem hiding this comment.
src/cli_sync_commands.py mutates sys.path to make imports resolve. This is brittle in an installed package and likely indicates the packaging/module layout should be fixed instead (e.g., ensure the console entry point imports src.cli_sync_commands, and then import src.sync_commands via normal package imports without path surgery).
Made-with: Cursor
Switch subscription command surface to focused status/delete plus unsubscribe-player and unsubscribe-clan, update option names to player/clan, and preserve existing rules when subscribing new targets. Enrich subscribe confirmations and subscription status with resolved player/clan names while keeping status embeds text-only. Made-with: Cursor
Made-with: Cursor
Summary
manifest,pagination, command helpers)create_app()and keepingmain.pythinTest plan
python3 - <<'PY' ... py_compile ... PYacrosssrc/sync-discord-commandswith dry-run env)/subscription status,/subscribe,/player-searchin a Discord test guildMade with Cursor