Conversation
SummaryThe following content is AI-generated and provides a summary of the pull request: 📝 Improve and Restructure README DocumentationDocumentation♻️ Refactor: Significantly rewrote and reorganized the Changes
PR Bot InformationVersion:
💌 Have ideas or want to contribute? Create an issue and share your thoughts with us! Made with ❤️ by Hyperspace. |
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR is a significant and well-structured improvement to the README — clearer hierarchy, a proper ToC, and richer code examples — but there are a few correctness issues: broken ToC anchor links for the "Running the Sample" section, a silently dropped build-time validation rule (businesskey input requirement), and an inconsistent field name casing (businesskey vs businessKey) in code examples that could mislead users into runtime errors.
PR Bot Information
Version: 1.19.3 | 📖 Documentation | 🚨 Create Incident | 💬 Feedback
- Correlation ID:
d0a7ad40-2923-11f1-8b0c-7e88a18ccf07 - LLM:
anthropic--claude-4.6-sonnet - Event Trigger:
pull_request.opened
| - [Running the Sample](#running-the-sample) | ||
| - [Running the Bookshop Example](#running-the-bookshop-example) | ||
| - [Troubleshooting](#troubleshooting) |
There was a problem hiding this comment.
Logic Error: Table of Contents links to sections that don't exist in the document
The ToC entries [Running the Sample](#running-the-sample), [Running the Bookshop Example](#running-the-bookshop-example), and [Troubleshooting](#troubleshooting) reference headings that are not present anywhere in the README. Readers who click these links will navigate to a broken anchor.
Consider either adding the corresponding sections or removing these entries from the Table of Contents.
| - [Running the Sample](#running-the-sample) | |
| - [Running the Bookshop Example](#running-the-bookshop-example) | |
| - [Troubleshooting](#troubleshooting) | |
| - [Running the Sample](./tests/bookshop/README.md) | |
| - [Running the Bookshop Example](./tests/bookshop/README.md) | |
| - [Troubleshooting](./tests/bookshop/README.md) |
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
| ``` | ||
| cd /tests/bookshop | ||
| cds watch | ||
| cds bind ProcessService -2 <sbpa-service-instance> |
There was a problem hiding this comment.
Logic Error: Binding command uses inconsistent flag syntax
The Setup section uses -2 as the flag for cds bind ProcessService -2 <sbpa-service-instance>, while the old README used -to. The CDS CLI flag for binding to a service instance is -2 (which is indeed the correct current flag), but the Programmatic Approach → Importing a Service section description says cds bind process -2 <instance>. These inconsistencies can confuse users. More importantly, the command shown here binds ProcessService (capital P), but the note at line 410 says to bind with cds bind process -2 <instance> (lowercase). Confirm both the service name casing and the flag are consistent across all examples.
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
| **Errors:** | ||
|
|
||
| - Entity attributes specified in `@bpm.process.start.inputs` (or all direct attributes if `inputs` is omitted) must exist in the process definition inputs | ||
| - Mandatory inputs from the process definition must be present in the entity |
There was a problem hiding this comment.
Bug: Missing @bpm.process.start.id in the Build-Time Validation error list
The old README explicitly listed: "The process definition must have a businesskey input" as a validation error under Process Start. The new text at lines 591–592 silently drops this check. If this validation is still enforced at build time (which is likely, given it's a hard requirement for the plugin to function), omitting it from the documentation is misleading.
Should either restore the bullet point or add a note explaining that this requirement was removed.
| **Errors:** | |
| - Entity attributes specified in `@bpm.process.start.inputs` (or all direct attributes if `inputs` is omitted) must exist in the process definition inputs | |
| - Mandatory inputs from the process definition must be present in the entity | |
| **Errors:** | |
| - The process definition must have a `businesskey` input | |
| - Entity attributes specified in `@bpm.process.start.inputs` (or all direct attributes if `inputs` is omitted) must exist in the process definition inputs | |
| - Mandatory inputs from the process definition must be present in the entity |
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
| - Entity attributes specified in `@bpm.process.start.inputs` (or all direct attributes if `inputs` is omitted) must exist in the process definition inputs | ||
| - Mandatory inputs from the process definition must be present in the entity | ||
| await processService.start({ | ||
| businesskey: 'order-12345', |
There was a problem hiding this comment.
Naming: Inconsistent casing for businesskey / businessKey
In the "Starting a Process" TypeScript code example, the field is spelled businesskey (all lowercase) at line 444. However, in the suspend/resume/cancel examples at lines 458–464 and throughout the Generic ProcessService section, it is spelled businessKey (camelCase). These are passed as named parameters to API calls, so the wrong casing would cause a runtime failure.
Should verify the exact field name expected by the typed start action and use it consistently. If the typed action truly uses businesskey, consider adding a comment explaining this discrepancy with the generic API.
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
|
|
||
| ## For suspending/resuming/cancelling a process | ||
| > **Note:** The generic ProcessService uses `emit` for lifecycle events (start, cancel, suspend, resume) which are processed asynchronously through the CDS outbox, and `send` for query functions (getAttributes, getOutputs, getInstancesByBusinessKey) which return data synchronously. | ||
| > Make sure to check whether the outbox is correctly used. If not, refer to cds.queued to make sure it is used. |
There was a problem hiding this comment.
Clarity: Note about outbox usage is incomplete and refers to an undefined API
The note at line 563 says "refer to cds.queued to make sure it is used" but cds.queued is not a documented or well-known CAP API, and no link or further explanation is provided. Readers are left without actionable guidance.
Consider providing a concrete example or a link to the relevant CAP documentation section on outbox/messaging.
| > Make sure to check whether the outbox is correctly used. If not, refer to cds.queued to make sure it is used. | |
| > **Note:** The generic ProcessService uses `emit` for lifecycle events (start, cancel, suspend, resume) which are processed asynchronously through the CDS outbox, and `send` for query functions (getAttributes, getOutputs, getInstancesByBusinessKey) which return data synchronously. | |
| > Make sure the CDS outbox is correctly configured. See the [CAP documentation on outboxed messaging](https://cap.cloud.sap/docs/node.js/outbox) for details. |
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
3 participants
Have you...