PR: cursor_new_oma_req_review → cursor_new_oma_req - #68
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds debug console logging statements to three backend service files ( ChangesBackend Debug Logging & Route Registration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@backend/src/controllers/printJobController.js`:
- Around line 13-14: Remove the direct console.log call that prints
'triggerDepartmentPrint' along with order_id, department, and type; replace it
with the project's structured logger (e.g., logger.debug or processLogger.debug)
or remove entirely, and if you must log for diagnostics only emit non-sensitive,
masked or aggregated info (no raw order_id) using the same
'triggerDepartmentPrint' context to locate the log site in
printJobController.js.
In `@backend/src/controllers/webhookController.js`:
- Line 11: Remove the ad-hoc console.log call that prints payload-derived data
in the webhook processing path (the console.log('parseDeliveryDate', dateStr)
inside parseDeliveryDate / webhookController), and either delete it or replace
it with a proper debug-level logger call (e.g., logger.debug or conditional on a
DEBUG/LOG_LEVEL env flag) so production logs do not retain webhook payload
values (ensure the symbol dateStr and the parseDeliveryDate function remain
unchanged except for logging).
In `@backend/src/services/timelineService.js`:
- Line 118: Remove the redundant console.log call that prints
"logManualPrintTriggered" with orderId and department; locate the
console.log('logManualPrintTriggered', orderId, department) in
timelineService.js and delete it (or replace with a proper debug-level logger if
you must keep it) so that only the durable timeline event write remains (e.g.,
the existing timeline/write/createTimelineEvent call) and avoid
duplicate/unnecessary service-level debug output.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: d397a29c-0973-4901-83b1-032488a78aa2
📒 Files selected for processing (4)
backend/src/controllers/printJobController.jsbackend/src/controllers/webhookController.jsbackend/src/routes/orderRoutes.jsbackend/src/services/timelineService.js
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
backend/src/**/*.{js,ts}
📄 CodeRabbit inference engine (Custom checks)
backend/src/**/*.{js,ts}: Backend source code must not contain hardcoded credentials, Shopify webhook secrets, or database passwords (must use env variables andconfig/env.jspatterns)
Backend webhook routes must not skip or weaken HMAC or Shopify authentication validation
Backend code must not build SQL queries by concatenating untrusted strings; must use parameterized queries or ORM usage
Backend async routes and services must implement proper error handling withnext(err)or structured error responses instead of swallowing errors
Files:
backend/src/controllers/printJobController.jsbackend/src/controllers/webhookController.jsbackend/src/services/timelineService.jsbackend/src/routes/orderRoutes.js
**/*.{js,mjs,cjs,ts,tsx,jsx,vue}
📄 CodeRabbit inference engine (.cursor/rules/README.md)
**/*.{js,mjs,cjs,ts,tsx,jsx,vue}: Follow JS/TS language rules: modules, async patterns, TypeScript usage, error handling, and platform considerations
Follow JavaScript/TypeScript architectural patterns: structure, async flow, React habits, and anti-pattern avoidance
Files:
backend/src/controllers/printJobController.jsbackend/src/controllers/webhookController.jsbackend/src/services/timelineService.jsbackend/src/routes/orderRoutes.js
backend/src/**/*.js
⚙️ CodeRabbit configuration file
backend/src/**/*.js: This path is the Node.js + Express API, MySQL access, Shopify webhooks, PDF/print services, and Socket.IO server for OMA. Blocking rules for changed lines:
- Configuration must come from
backend/src/config/env.jsand environment variables—do not hardcode DB passwords, Shopify secrets, or deployment-specific hosts in source.- Webhook and HMAC-sensitive routes must remain protected by the existing Shopify auth middleware (
shopifyAuth/ HMAC validation); do not bypass verification for convenience.- Async route handlers and services must propagate errors to Express (
next(err)or throw into async wrappers) or handle them explicitly—no emptycatchblocks that hide failures.- Database access must use parameterized queries / the existing models layer—no string-concatenated SQL with user-controlled input.
- New Socket.IO handlers must handle errors and avoid leaking internal stack traces to clients in production.
- No new dependencies on
eval,child_processwith untrusted input, or disabling security middleware without strong justification.- Prefer
backend/src/services/for business logic and keep controllers thin; avoid duplicating rule/PDF/print orchestration across files.If any of the above appears in the diff for this path, you MUST request changes and explain the fix.
Files:
backend/src/controllers/printJobController.jsbackend/src/controllers/webhookController.jsbackend/src/services/timelineService.jsbackend/src/routes/orderRoutes.js
🔇 Additional comments (1)
backend/src/routes/orderRoutes.js (1)
15-15:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove duplicated route registration.
Lines 14 and 15 both register the identical GET route
/:order_id/departments/:department/download-pdfwith the same handler. Duplicate route registrations cause ambiguous routing behavior and maintenance risk.Proposed change
router.get('/:order_id/departments/:department/download-pdf', printJobController.downloadDepartmentPdf); -router.get('/:order_id/departments/:department/download-pdf', printJobController.downloadDepartmentPdf);> Likely an incorrect or invalid review comment.
| console.log('triggerDepartmentPrint', order_id, department, type); | ||
|
|
There was a problem hiding this comment.
Drop direct console.log of print trigger parameters.
Lines 13-14 log order_id/department/type per request; this is noisy and leaks internal identifiers into raw logs. Use structured logger with level controls if you need diagnostics.
Proposed change
- console.log('triggerDepartmentPrint', order_id, department, type);
- 📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log('triggerDepartmentPrint', order_id, department, type); | |
🤖 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 `@backend/src/controllers/printJobController.js` around lines 13 - 14, Remove
the direct console.log call that prints 'triggerDepartmentPrint' along with
order_id, department, and type; replace it with the project's structured logger
(e.g., logger.debug or processLogger.debug) or remove entirely, and if you must
log for diagnostics only emit non-sensitive, masked or aggregated info (no raw
order_id) using the same 'triggerDepartmentPrint' context to locate the log site
in printJobController.js.
| function parseDeliveryDate(dateStr) { | ||
| if (!dateStr) return null; | ||
|
|
||
| console.log('parseDeliveryDate', dateStr); |
There was a problem hiding this comment.
Remove ad-hoc payload logging from webhook processing path.
Line 11 logs payload-derived data on every webhook call; this adds noisy production logs and unnecessary data retention. Prefer removing this or gating it behind a proper debug logger flag.
Proposed change
- console.log('parseDeliveryDate', dateStr);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log('parseDeliveryDate', dateStr); |
🤖 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 `@backend/src/controllers/webhookController.js` at line 11, Remove the ad-hoc
console.log call that prints payload-derived data in the webhook processing path
(the console.log('parseDeliveryDate', dateStr) inside parseDeliveryDate /
webhookController), and either delete it or replace it with a proper debug-level
logger call (e.g., logger.debug or conditional on a DEBUG/LOG_LEVEL env flag) so
production logs do not retain webhook payload values (ensure the symbol dateStr
and the parseDeliveryDate function remain unchanged except for logging).
| } | ||
|
|
||
| async function logManualPrintTriggered(orderId, department) { | ||
| console.log('logManualPrintTriggered', orderId, department); |
There was a problem hiding this comment.
Remove redundant service-level debug log before timeline write.
Line 118 writes a raw debug log for data already captured by the timeline event; this increases log volume without adding durable value.
Proposed change
- console.log('logManualPrintTriggered', orderId, department);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log('logManualPrintTriggered', orderId, department); |
🤖 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 `@backend/src/services/timelineService.js` at line 118, Remove the redundant
console.log call that prints "logManualPrintTriggered" with orderId and
department; locate the console.log('logManualPrintTriggered', orderId,
department) in timelineService.js and delete it (or replace with a proper
debug-level logger if you must keep it) so that only the durable timeline event
write remains (e.g., the existing timeline/write/createTimelineEvent call) and
avoid duplicate/unnecessary service-level debug output.
2 participants
Auto-generated PR from branch
cursor_new_oma_req_reviewintocursor_new_oma_req.Created by n8n automation.
Summary by CodeRabbit