Merged
Conversation
…ayer The orders API route handlers contained all business logic inline — direct ORM operations, query building, and order merging logic — bypassing the service layer. This moves all domain logic into order_service.py, making routes thin HTTP wrappers that only handle request parsing and error translation. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Xitee1
changed the title
Replace 43 duplicated unsafe error-handling blocks (using TypeScript `as` casts) across 19 files with type-safe getApiErrorMessage/getApiErrorStatus utilities that use axios.isAxiosError(). Extract 5 duplicated formatDate, 3 formatAmount, and formatTimeAgo/formatTimeUntil functions into shared utils/format.ts module. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…dlers The IMAP test_connection and list_folders endpoints used synchronous imaplib, which blocked the entire asyncio event loop during network I/O. Extract shared async IMAP utilities (connect, test, list folders, parse) into _shared/email/imap_utils.py using aioimaplib, eliminating both the event loop blocking bug and the duplicated IMAP logic between email-user and email-global routers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Eliminate ~350 lines of duplicated IMAP watching logic between email_user and email_global services by extracting generic watch/idle/poll/fetch functions that accept provider-specific behavior via a callbacks dataclass. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move EmailAnalysis/EmailItem to shared schemas so the core service layer no longer depends on a specific plugin module. Analysis is now routed through the module registry via a new `analyze` hook on ModuleInfo, consistent with how `notify` already works for notifiers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…fallback - Rename EmailAnalysis→AnalysisResult, EmailItem→ExtractedItem, email_type→document_type across all files - Rename email_analysis.py→analysis.py (shared schema) - Change get_active_analyser() to get_active_analysers() returning all enabled analysers in priority order (ordered by ModuleConfig.priority) - Queue worker now tries each analyser in priority order, falling back to the next on failure - Update SYSTEM_PROMPT to use generic "data" language instead of "email" - Clean up email_item_names→extracted_item_names variable naming Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Register centralized exception handlers for domain exceptions (OrderNotFoundError, InvalidSortError), database IntegrityError, and unhandled exceptions. This eliminates repetitive try/except boilerplate in order route handlers, prevents raw exception messages from leaking to API clients (smtp, llm, webhook test endpoints), and fixes test endpoints that incorrectly returned HTTP 200 for failures. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…connections The is_configured module hook was the only hook without an AsyncSession parameter, forcing 3 modules to create their own DB sessions even though callers already had one. Align its signature with status/analyze/notify hooks, and keep the session open in get_active_analysers() so it can be shared. Also fix == True comparisons to use .is_(True) across the board. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…howing Replace window.location.href with router.push() in the 401 interceptor so failed login attempts preserve component state and display the error message. Convert all string path navigations to named routes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use i18n translation for failed login instead of raw backend error detail. Add language select dropdown to the login/setup page so users can switch language before authenticating. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adds `python -m app.cli` with list-users and reset-password commands, usable inside Docker containers or locally. Documents the CLI and adds Docker development setup section to the README. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to improve overall architecture by centralizing common logic (error/message extraction, formatting), refactoring backend service boundaries, and making the email/analysis pipeline more modular and reusable.
Changes:
- Centralize frontend API error handling and date/amount formatting utilities, updating multiple views/components to use them.
- Refactor backend order operations into a service layer (with domain exceptions) and add global exception handlers for consistent HTTP responses.
- Generalize IMAP watching logic into shared watch-loop/utilities and update queue processing to support multiple analysers with fallback by priority.
Reviewed changes
Copilot reviewed 58 out of 58 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/views/admin/UsersView.vue | Use centralized API error-message extraction helper. |
| frontend/src/views/admin/StatusView.vue | Use shared API error helper; move time formatting to shared util. |
| frontend/src/views/admin/QueueSettingsView.vue | Use centralized API error-message extraction helper. |
| frontend/src/views/admin/AdminSmtpSettingsView.vue | Use centralized API error helpers for message/status extraction. |
| frontend/src/views/VerifyEmailView.vue | Use centralized API error-message extraction helper. |
| frontend/src/views/ProfileView.vue | Use centralized API error helper; move date formatting to shared util. |
| frontend/src/views/OrdersView.vue | Switch to named-route navigation; use shared formatting util. |
| frontend/src/views/OrderDetailView.vue | Switch to named-route navigation; use shared API error + formatting utils. |
| frontend/src/views/LoginView.vue | Add locale switcher; use named-route navigation; use API error helper for setup. |
| frontend/src/views/HistoryView.vue | Switch to named-route navigation; use shared date-time formatter. |
| frontend/src/views/DashboardView.vue | Switch to named-route navigation; use shared formatting util. |
| frontend/src/utils/format.ts | Introduce shared formatting helpers (date/date-time/amount/relative time). |
| frontend/src/utils/api-error.ts | Introduce axios-aware API error message/status extraction helpers. |
| frontend/src/modules/providers/email-user/UserImapAccountsView.vue | Use centralized API error helpers (message/status). |
| frontend/src/modules/providers/email-user/AdminImapSettingsView.vue | Use centralized API error-message extraction helper. |
| frontend/src/modules/providers/email-global/UserForwardingView.vue | Use centralized API error-message extraction helper. |
| frontend/src/modules/providers/email-global/AdminGlobalMailView.vue | Use centralized API error-message extraction helper. |
| frontend/src/modules/notifiers/notify-webhook/UserNotifyWebhookView.vue | Use centralized API error helpers (message/status). |
| frontend/src/modules/notifiers/notify-email/UserNotifyEmailView.vue | Use centralized API error helpers (message/status). |
| frontend/src/modules/analysers/llm/AdminLLMConfigView.vue | Use centralized API error-message extraction helper. |
| frontend/src/i18n/locales/en.json | Add security warning copy for webhook notifier description. |
| frontend/src/i18n/locales/de.json | Add security warning copy for webhook notifier description (DE). |
| frontend/src/components/OrderFormModal.vue | Use centralized API error-message extraction helper. |
| frontend/src/components/AppLayout.vue | Switch logout redirect to named-route navigation. |
| frontend/src/api/client.ts | Add 401 interceptor that logs out and navigates to login. |
| backend/tests/test_queue_worker.py | Update tests for new analysis schema + analyser discovery/fallback behavior. |
| backend/tests/test_order_matcher.py | Update tests to use new analysis schema types. |
| backend/tests/test_llm_service.py | Update tests to use new analysis schema field names/types. |
| backend/tests/test_imap_worker.py | Use shared fallback message-id generator instead of duplicating logic in test. |
| backend/tests/test_global_mail_folders.py | Mock new IMAP folder listing helper instead of patching imaplib directly. |
| backend/app/services/queue/queue_worker.py | Support multiple analysers via module registry with fallback execution. |
| backend/app/services/orders/order_service.py | Move order CRUD/listing/counting/linking into a service layer + domain exceptions. |
| backend/app/services/orders/order_matcher.py | Update matcher to use new analysis schema and naming. |
| backend/app/services/notification_service.py | Use SQLAlchemy boolean .is_(True) comparisons. |
| backend/app/schemas/analysis.py | Introduce shared AnalysisResult/ExtractedItem schema for analysers + services. |
| backend/app/modules/providers/email_user/user_router.py | Use shared IMAP utilities for test connection / folder listing. |
| backend/app/modules/providers/email_user/service.py | Refactor watcher logic to use shared IMAP watch loop with callbacks. |
| backend/app/modules/providers/email_global/service.py | Refactor global watcher logic to use shared IMAP watch loop with callbacks. |
| backend/app/modules/providers/email_global/router.py | Use shared IMAP utilities for folder listing instead of imaplib. |
| backend/app/modules/providers/email_global/init.py | Update check_configured to accept an AsyncSession. |
| backend/app/modules/notifiers/notify_webhook/user_router.py | Replace raw exception echo with logging + standardized 502 error. |
| backend/app/modules/notifiers/notify_webhook/init.py | Update module description to include SSRF risk warning. |
| backend/app/modules/notifiers/notify_email/init.py | Update check_configured to accept an AsyncSession. |
| backend/app/modules/analysers/llm/service.py | Switch analyser output to shared AnalysisResult; check_configured(db); schema rename email_type -> document_type. |
| backend/app/modules/analysers/llm/router.py | Replace raw exception echo with logging + standardized 502 error. |
| backend/app/modules/analysers/llm/init.py | Register analyser analyze hook on ModuleInfo and update description. |
| backend/app/modules/_shared/email/imap_watch_loop.py | Add shared IMAP watch-loop (IDLE/poll/backoff) and email fetch/enqueue logic. |
| backend/app/modules/_shared/email/imap_utils.py | Add shared IMAP utilities for connection test + folder listing using aioimaplib. |
| backend/app/main.py | Register global exception handlers during app init. |
| backend/app/core/module_registry.py | Replace analyser availability boolean with prioritized get_active_analysers() list. |
| backend/app/core/module_base.py | Update module interface: is_configured(db) and new analyze hook. |
| backend/app/core/exceptions.py | Add centralized exception-to-HTTP response mapping for domain/DB errors. |
| backend/app/cli.py | Add admin CLI for listing users and resetting passwords. |
| backend/app/api/system.py | Update module configured checks to call is_configured(db). |
| backend/app/api/smtp.py | Standardize SMTP test error response and log failure details. |
| backend/app/api/orders.py | Refactor routes to delegate to order service layer functions. |
| backend/app/api/modules.py | Update module configured checks to call is_configured(db). |
| README.md | Document system workflow, order statuses, and new admin CLI usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+102
to
+145
| ## How It Works | ||
|
|
||
| 1. **Email provider** — a background worker per watched folder uses IMAP IDLE (push notifications) with a polling fallback to detect new emails | ||
| 2. **Processing queue** — new emails are added to a queue and processed asynchronously by a scheduled worker (every 5 seconds) | ||
| 3. **LLM analysis** — the configured LLM extracts structured data (order number, tracking number, carrier, vendor, items, status, etc.) from the email | ||
| 4. **Order matching** — the system matches the analysis to existing orders by order number, tracking number, or vendor + item similarity | ||
| 5. **Order updates** — if a matching order is found it updates the status; otherwise it creates a new order. Every status change is recorded as a state entry for auditability. | ||
| 6. **Notifications** — configured notifiers (email, webhook) are triggered for relevant events | ||
|
|
||
| ## Order Statuses | ||
|
|
||
| Orders progress through these statuses as emails are processed: | ||
|
|
||
| `ordered` → `shipment_preparing` → `shipped` → `in_transit` → `out_for_delivery` → `delivered` | ||
|
|
||
| ## Admin CLI | ||
|
|
||
| A built-in `pt-admin` command is available for user management tasks like listing users and resetting passwords. | ||
|
|
||
| ### Docker (production) | ||
|
|
||
| ```bash | ||
| docker compose -f docker-compose.prod.yaml exec package-tracker python -m app.cli list-users | ||
| docker compose -f docker-compose.prod.yaml exec package-tracker python -m app.cli reset-password <username> | ||
| ``` | ||
|
|
||
| ### Docker (development) | ||
|
|
||
| ```bash | ||
| docker compose exec backend python -m app.cli list-users | ||
| docker compose exec backend python -m app.cli reset-password <username> | ||
| ``` | ||
|
|
||
| ### Without Docker | ||
|
|
||
| ```bash | ||
| cd backend | ||
| python -m app.cli list-users | ||
| python -m app.cli reset-password <username> | ||
| ``` | ||
|
|
||
| The `reset-password` command prompts for the new password interactively. Pass `--password <pw>` to skip the prompt. | ||
|
|
||
| ## Development Setup |
There was a problem hiding this comment.
The PR description says to "only analyze the backend" and to "exclude tests", but this PR includes substantial frontend changes (frontend/src/...) and multiple backend test updates (backend/tests/...). Please align the PR scope/description with the actual changes (or split frontend/test changes into separate PRs) so reviewers can apply the intended review criteria.
Comment on lines
1
to
22
| @@ -14,8 +16,9 @@ api.interceptors.response.use( | |||
| (response) => response, | |||
| (error) => { | |||
| if (error.response?.status === 401) { | |||
| localStorage.removeItem('token') | |||
| window.location.href = '/login' | |||
| const auth = useAuthStore() | |||
| auth.logout() | |||
| router.push({ name: 'login' }) | |||
| } | |||
There was a problem hiding this comment.
api/client.ts now imports both @/router and useAuthStore(), but the auth store itself imports this API client (stores/auth.ts imports @/api/client). That creates a circular dependency (router -> auth store -> api client -> router/auth store) that can lead to undefined imports or interceptors not being registered reliably at runtime. Consider breaking the cycle by removing router/store imports from the API client (e.g., handle 401 via a lightweight token-clear + window.location, emit an event, or install the interceptor from an app/plugin layer after Pinia+router are created).
Comment on lines
+12
to
+20
| export function formatDateTime(dateStr: string): string { | ||
| const date = new Date(dateStr) | ||
| return date.toLocaleDateString(undefined, { | ||
| month: 'short', | ||
| day: 'numeric', | ||
| year: 'numeric', | ||
| hour: 'numeric', | ||
| minute: '2-digit', | ||
| }) |
There was a problem hiding this comment.
formatDateTime() uses Date.toLocaleDateString() while also passing time options (hour, minute). In many browsers toLocaleDateString() ignores time components, which would regress displays that expect a date+time output (the removed view-local implementations explicitly appended toLocaleTimeString()). Use toLocaleString() for date+time formatting, or build it from toLocaleDateString() + toLocaleTimeString() to ensure the time is included consistently.
Comment on lines
+54
to
+66
| async def connect(db: AsyncSession) -> ConnectResult | None: | ||
| from aioimaplib import IMAP4_SSL | ||
| from app.core.encryption import decrypt_value | ||
|
|
||
| if msg == STOP_WAIT_SERVER_PUSH: | ||
| imap.idle_done() | ||
| await asyncio.wait_for(idle_task, timeout=5) | ||
| continue | ||
| account = await db.get(EmailAccount, account_id) | ||
| folder = await db.get(WatchedFolder, folder_id) | ||
| if not account or not folder or not account.is_active: | ||
| return None | ||
|
|
||
| password = decrypt_value(account.imap_password_encrypted) | ||
| imap = IMAP4_SSL(host=account.imap_host, port=account.imap_port) | ||
| await imap.wait_hello_from_server() | ||
| await imap.login(account.imap_user, password) |
There was a problem hiding this comment.
The watcher connect callback always uses aioimaplib.IMAP4_SSL(...) even when the account is configured with use_ssl = false. This can break connections for non-SSL IMAP servers and diverges from the router-level IMAP utilities which support both IMAP4 and IMAP4_SSL. Please select the correct client based on account.use_ssl (and keep the rest of the login logic the same).
Comment on lines
+33
to
+55
| async def connect(db: AsyncSession) -> ConnectResult | None: | ||
| from aioimaplib import IMAP4_SSL | ||
| from app.core.encryption import decrypt_value | ||
|
|
||
| if state: | ||
| if uids: | ||
| state.mode = WorkerMode.PROCESSING | ||
| state.queue_total = len(uids) | ||
| state.last_activity_at = datetime.now(timezone.utc) | ||
|
|
||
| for i, uid_bytes in enumerate(uids): | ||
| uid = int(uid_bytes) | ||
| if uid <= config.last_seen_uid: | ||
| continue | ||
|
|
||
| _, msg_data = await imap.uid("fetch", str(uid), "(RFC822)") | ||
| if not msg_data or not msg_data[0]: | ||
| continue | ||
|
|
||
| raw_email = None | ||
| for part in msg_data: | ||
| if isinstance(part, bytearray): | ||
| raw_email = bytes(part) | ||
| break | ||
| if raw_email is None: | ||
| continue | ||
| msg = email.message_from_bytes(raw_email) | ||
|
|
||
| sender = decode_header_value(msg.get("From", "")) | ||
| sender_email = extract_email_from_header(sender) | ||
|
|
||
| # Sender gate: look up in UserSenderAddress | ||
| result = await db.execute( | ||
| select(UserSenderAddress).where(UserSenderAddress.email_address == sender_email) | ||
| mod_result = await db.execute( | ||
| select(ModuleConfig).where(ModuleConfig.module_key == "email-global") | ||
| ) | ||
| sender_addr = result.scalar_one_or_none() | ||
| if not sender_addr: | ||
| logger.info(f"Global mail: discarding email from unregistered sender: {sender_email}") | ||
| config.last_seen_uid = uid | ||
| module = mod_result.scalar_one_or_none() | ||
| if not module or not module.enabled: | ||
| logger.info("Stopping global mail watcher: module disabled") | ||
| return None | ||
|
|
||
| result = await db.execute(select(GlobalMailConfig)) | ||
| config = result.scalar_one_or_none() | ||
| if not config: | ||
| logger.info("Stopping global mail watcher: inactive or removed") | ||
| return None | ||
|
|
||
| password = decrypt_value(config.imap_password_encrypted) | ||
| imap = IMAP4_SSL(host=config.imap_host, port=config.imap_port) | ||
| await imap.wait_hello_from_server() | ||
| await imap.login(config.imap_user, password) | ||
|
|
There was a problem hiding this comment.
The global mail watcher connect callback always uses aioimaplib.IMAP4_SSL(...) and ignores config.use_ssl. This will fail for installations using plain IMAP and is inconsistent with the list_imap_folders() admin endpoint which supports both SSL and non-SSL. Please choose IMAP4 vs IMAP4_SSL based on config.use_ssl before logging in.
Comment on lines
+18
to
+27
| async def _run_analysis(raw_data: dict, db, analysers: list[tuple[str, callable]]): | ||
| """Try each analyser in priority order, falling back to the next on failure.""" | ||
| last_error = None | ||
| for module_key, analyze in analysers: | ||
| try: | ||
| return await analyze(raw_data, db) | ||
| except Exception as e: | ||
| logger.warning(f"Analyser {module_key} failed: {e}, trying next") | ||
| last_error = e | ||
| raise last_error or RuntimeError("No analysers available") |
There was a problem hiding this comment.
Logging in _run_analysis() currently uses f-strings without exc_info, which drops stack traces and makes analyser failures hard to debug in production. Consider switching to structured logging (logger.warning("Analyser %s failed, trying next", module_key, exc_info=True)) and similarly use exc_info=True for the Failed to process queue item error log so you retain the traceback.
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
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.
Prompt:
This project has been vibe coded with AI and also has been heavily refactored and restructured in the meantime. Analyze the code of this project and find areas that can be improved. I mainly want to address the overall architecture and for things to follow best practices and good conventions. Also make use of official documentations by fetching them with context7. You MUST do that if using new libraries or change functions of those. Only analyze the backend. Exclude tests from analyzing. I want you to find the most severe issue and then only work on that, nothing else. Also feel free to suggest adding new libraries if appropriate. The prompt will be looped multiple times until the code is in good shape. This is run X.
To replace:
backend,Exclude tests from analyzing.X