PR: cursor_new_oma_req_review → cursor_new_oma_req - #69
Conversation
📝 WalkthroughWalkthroughThis PR adds debug logging statements across multiple backend controllers and services (printJobController, webhookController, timelineService) and registers a duplicate PDF download route. However, the retryDepartmentPrint logging references variables before they are declared, creating a runtime error. ChangesDebug Logging & Route Registration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 59-60: The console.log in printJobController.js is reading
order_id and department before they are declared, causing a TDZ ReferenceError;
move the console.log statement to after the destructuring assignment "const {
order_id, department } = req.params" (or destructure into locals earlier in the
function handling retries, e.g., inside retryDepartmentPrint handler) so the log
uses the declared variables, and ensure any other uses of order_id/department in
retryDepartmentPrint reference the same destructured variables.
In `@backend/src/controllers/webhookController.js`:
- Line 30: The console.log call that prints 'extractDeliveryInfo' along with
noteAttributes and lineItems exposes customer PII (addresses, emails, phones);
remove that raw debug log and replace it with a non-PII-safe log (e.g., log only
counts or presence flags) or mask/redact sensitive fields before logging;
specifically update the console.log('extractDeliveryInfo', noteAttributes,
lineItems) invocation (or within the extractDeliveryInfo code path) to emit only
metadata such as noteAttributes length, lineItems length, or a boolean
hasDeliveryInfo, or call a central maskPII helper to redact emails/phones before
any logging.
In `@backend/src/routes/orderRoutes.js`:
- Around line 14-16: Duplicate GET route registrations for
'/:order_id/departments/:department/download-pdf' were added using router.get
and printJobController.downloadDepartmentPdf; remove the extra two registrations
and keep a single router.get('/:order_id/departments/:department/download-pdf',
printJobController.downloadDepartmentPdf) to avoid ambiguous routing (ensure
only one occurrence remains and that the router export is unchanged).
🪄 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: 297004e1-ad60-45f6-bb43-45843330438b
📒 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/services/timelineService.jsbackend/src/controllers/webhookController.jsbackend/src/routes/orderRoutes.jsbackend/src/controllers/printJobController.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/services/timelineService.jsbackend/src/controllers/webhookController.jsbackend/src/routes/orderRoutes.jsbackend/src/controllers/printJobController.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/services/timelineService.jsbackend/src/controllers/webhookController.jsbackend/src/routes/orderRoutes.jsbackend/src/controllers/printJobController.js
🔇 Additional comments (2)
backend/src/services/timelineService.js (1)
107-110: Debug log placement is safe.The added trace at Line 108 preserves behavior and keeps useful context before the timeline write.
backend/src/controllers/printJobController.js (1)
38-39: Manual-print timeline hook placement looks correct.The added logging + timeline call sequence is coherent and does not change endpoint semantics.
| console.log('retryDepartmentPrint', order_id, department); | ||
| const { order_id, department } = req.params; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify declaration order inside retryDepartmentPrint in a read-only way.
# Expected: destructuring declaration appears before the console.log usage.
file="backend/src/controllers/printJobController.js"
rg -n -C3 "async function retryDepartmentPrint|console\\.log\\('retryDepartmentPrint', order_id, department\\)|const \\{ order_id, department \\} = req\\.params;" "$file"Repository: IM-Agents/Pravin-AI-2026
Length of output: 871
Move the console.log after the variable declaration to avoid TDZ crash.
Line 59 reads order_id and department before they are declared on line 60. This causes a ReferenceError at runtime and returns a 500 error for all retry requests.
Proposed fix
async function retryDepartmentPrint(req, res) {
try {
- console.log('retryDepartmentPrint', order_id, department);
const { order_id, department } = req.params;
+ console.log('retryDepartmentPrint', order_id, 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/controllers/printJobController.js` around lines 59 - 60, The
console.log in printJobController.js is reading order_id and department before
they are declared, causing a TDZ ReferenceError; move the console.log statement
to after the destructuring assignment "const { order_id, department } =
req.params" (or destructure into locals earlier in the function handling
retries, e.g., inside retryDepartmentPrint handler) so the log uses the declared
variables, and ensure any other uses of order_id/department in
retryDepartmentPrint reference the same destructured variables.
| let specificDeliveryTime = null; | ||
| let deliveryDay = null; | ||
| let deliveryLink = null; | ||
| console.log('extractDeliveryInfo', noteAttributes, lineItems); |
There was a problem hiding this comment.
Remove or redact PII from debug logging.
This log statement outputs noteAttributes and lineItems, both of which contain customer PII such as delivery addresses, contact information, emails, phone numbers, and product purchase details. Logging this data creates compliance/privacy risks under GDPR/CCPA and violates data minimization principles.
🔒 Proposed fix: Remove the PII-sensitive log statement
- console.log('extractDeliveryInfo', noteAttributes, lineItems);If debugging is required, log only non-sensitive metadata (e.g., array lengths):
- console.log('extractDeliveryInfo', noteAttributes, lineItems);
+ console.log('extractDeliveryInfo: processing', noteAttributes.length, 'note attributes and', lineItems.length, 'line items');As per coding guidelines, backend source code must avoid logging sensitive data like emails and other user identifiers.
📝 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('extractDeliveryInfo', noteAttributes, lineItems); | |
| // (preceding code remains unchanged) |
| console.log('extractDeliveryInfo', noteAttributes, lineItems); | |
| console.log('extractDeliveryInfo: processing', noteAttributes.length, 'note attributes and', lineItems.length, 'line items'); |
🤖 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 30, The console.log
call that prints 'extractDeliveryInfo' along with noteAttributes and lineItems
exposes customer PII (addresses, emails, phones); remove that raw debug log and
replace it with a non-PII-safe log (e.g., log only counts or presence flags) or
mask/redact sensitive fields before logging; specifically update the
console.log('extractDeliveryInfo', noteAttributes, lineItems) invocation (or
within the extractDeliveryInfo code path) to emit only metadata such as
noteAttributes length, lineItems length, or a boolean hasDeliveryInfo, or call a
central maskPII helper to redact emails/phones before any logging.
| router.get('/:order_id/departments/:department/download-pdf', printJobController.downloadDepartmentPdf); | ||
| router.get('/:order_id/departments/:department/download-pdf', printJobController.downloadDepartmentPdf); | ||
| router.get('/:order_id/departments/:department/download-pdf', printJobController.downloadDepartmentPdf); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify duplicate route declarations for download-pdf.
# Expected after fix: exactly one match.
file="backend/src/routes/orderRoutes.js"
rg -n "router\\.get\\('/:order_id/departments/:department/download-pdf'" "$file"Repository: IM-Agents/Pravin-AI-2026
Length of output: 392
Remove duplicate route registration for download-pdf.
Lines 14–16 declare the same GET route three times. Keep a single registration to avoid ambiguous routing behavior.
Proposed fix
router.post('/:order_id/departments/:department/print', printJobController.triggerDepartmentPrint);
router.post('/:order_id/departments/:department/retry', printJobController.retryDepartmentPrint);
router.get('/:order_id/departments/:department/download-pdf', printJobController.downloadDepartmentPdf);
-router.get('/:order_id/departments/:department/download-pdf', printJobController.downloadDepartmentPdf);
-router.get('/:order_id/departments/:department/download-pdf', printJobController.downloadDepartmentPdf);📝 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.
| router.get('/:order_id/departments/:department/download-pdf', printJobController.downloadDepartmentPdf); | |
| router.get('/:order_id/departments/:department/download-pdf', printJobController.downloadDepartmentPdf); | |
| router.get('/:order_id/departments/:department/download-pdf', printJobController.downloadDepartmentPdf); | |
| router.post('/:order_id/departments/:department/print', printJobController.triggerDepartmentPrint); | |
| router.post('/:order_id/departments/:department/retry', printJobController.retryDepartmentPrint); | |
| router.get('/:order_id/departments/:department/download-pdf', printJobController.downloadDepartmentPdf); |
🤖 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/routes/orderRoutes.js` around lines 14 - 16, Duplicate GET route
registrations for '/:order_id/departments/:department/download-pdf' were added
using router.get and printJobController.downloadDepartmentPdf; remove the extra
two registrations and keep a single
router.get('/:order_id/departments/:department/download-pdf',
printJobController.downloadDepartmentPdf) to avoid ambiguous routing (ensure
only one occurrence remains and that the router export is unchanged).
2 participants
Auto-generated PR from branch
cursor_new_oma_req_reviewintocursor_new_oma_req.Created by n8n automation.
Summary by CodeRabbit
Release Notes