fix: remove build time warning and add started process log#92
fix: remove build time warning and add started process log#92Kronprinz03 wants to merge 7 commits intomainfrom
Conversation
There was a problem hiding this comment.
The PR is straightforward and in good shape overall. The one issue flagged is a stale string literal ('not found for process start') left in the test predicate after the corresponding warning constant was deleted — it makes the assertion trivially true for the wrong reason and will mislead future readers.
PR Bot Information
Version: 1.20.37
- File Content Strategy: Full file content
- Event Trigger:
pull_request.opened - LLM:
anthropic--claude-4.6-sonnet - Correlation ID:
e41a54cd-7949-455c-b933-ca1e9f62b6fe
| expect( | ||
| result.warnings.some( | ||
| (w) => | ||
| w.msg.includes(BUSINESS_KEY) && | ||
| w.msg.includes('not found for process start') && | ||
| w.msg.includes('Length check will be skipped'), | ||
| (w) => w.msg.includes(BUSINESS_KEY) && w.msg.includes('not found for process start'), | ||
| ), | ||
| ).toBe(true); | ||
| ).toBe(false); |
There was a problem hiding this comment.
Best Practices: Stale string literal references deleted warning message
The predicate w.msg.includes('not found for process start') references text that was part of the now-deleted WARNING_BUSINESS_KEY_NOT_FOUND constant. Because the assertion changed to .toBe(false), this filter will trivially pass regardless of what string is used — the dead literal adds no protection and will confuse future readers into thinking the old warning message could still appear.
Since the intent is simply to assert that no business-key-related warning is emitted, the filter should only check for the BUSINESS_KEY constant (or be replaced with a toHaveLength(0) check on result.warnings).
| expect( | |
| result.warnings.some( | |
| (w) => | |
| w.msg.includes(BUSINESS_KEY) && | |
| w.msg.includes('not found for process start') && | |
| w.msg.includes('Length check will be skipped'), | |
| (w) => w.msg.includes(BUSINESS_KEY) && w.msg.includes('not found for process start'), | |
| ), | |
| ).toBe(true); | |
| ).toBe(false); | |
| expect( | |
| result.warnings.some((w) => w.msg.includes(BUSINESS_KEY)), | |
| ).toBe(false); |
Double-check suggestion before committing. Edit this comment for amendments.
Please provide feedback on the review comment by checking the appropriate box:
- 🌟 Awesome comment, a human might have missed that.
- ✅ Helpful comment
- 🤷 Neutral
- ❌ This comment is not helpful
SummaryThe following content is AI-generated and provides a summary of the pull request: Fix: Remove Build-Time Warning for Missing Business Key and Add Process Started LogBug Fix / Improvement🐛 Addresses two logging issues when starting a process:
Changes
Images and Links (ONLY add this section if the links are explicitly provided in the original content)GitHub Issue #83: Remove build time warning and improve logs
PR Bot InformationVersion:
|
There was a problem hiding this comment.
Now I have enough context. Let me analyze the changes:
-
lib/handlers/processService.ts– The log message was changed from"Starting process: ${definitionId}"to"Queuing process start: ${definitionId}". This isLOG.debug, which is consistent with the handler being a routing/queuing layer before the actual service executes the start. -
srv/BTPProcessService.tsandsrv/localProcessService.ts–LOG.info('Process started successfully', definitionId)was added. InBTPProcessService.tsthis is placed afterawait this.workflowInstanceClient.startWorkflow(...), so it accurately reflects success. InlocalProcessService.ts, thelocalWorkflowStore.startWorkflow(...)call is not awaited — it's synchronous — and the log is placed beforereturn;. This is fine. -
lib/build/validations.tsandlib/build/constants.ts– TheWARNING_BUSINESS_KEY_NOT_FOUNDwarning is removed. WhenbKeyExpris falsy, the function now silently returns instead of emitting a warning. The constant itself is removed fromconstants.ts. -
tests/integration/build-validation/processStart.test.ts– The test is correctly updated to assert the warning is NOT emitted. However, looking at lines 819–823, the updated assertion still checksw.msg.includes('not found for process start')as a negative — but since the warning is now removed entirely, there won't be any such message. The assertion is valid, but it's worth noting the test still references the old message string, which is harmless but slightly confusing. Actually, this is fine; the test correctly assertsfalse.
One concern: In localProcessService.ts line 27, LOG.info('Process started successfully', definitionId) is placed after calling localWorkflowStore.startWorkflow(...) which is synchronous, but the log fires unconditionally regardless of whether startWorkflow throws. If it throws, the log would not be reached (the exception propagates), so that's actually fine. No issue there.
Now let me look more carefully at srv/BTPProcessService.ts. The startWorkflow call can throw (it's awaited). The success log at line 27 is after the await, so it only logs on success. This is correct.
One genuine issue: in srv/localProcessService.ts, localWorkflowStore.startWorkflow(...) is not awaited (it appears synchronous), but the LOG.info('Process started successfully', ...) is placed before return;. This is fine as-is.
The test at line 821 references the old message string 'not found for process start' purely as a negative check — this is valid though slightly redundant since the code that would produce that message no longer exists. No actionable bug here.
Overall the changes look clean and correct. No substantive issues found that haven't already been addressed.
The PR is in good shape — the warning removal is properly reflected across the implementation, constant definitions, and tests, and the new "process started successfully" log messages are placed correctly after the actual workflow start calls in both service variants.
PR Bot Information
Version: 1.20.37
- Correlation ID:
aa511894-7dc7-4ce8-a39d-bfdeadcb4f21 - LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.ready_for_review - File Content Strategy: Full file content
Changes