Support html files#130
Conversation
…into eventTypes
![hyperspace-insights[bot]](https://avatars.githubusercontent.com/u/2531208?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
The PR introduces a well-structured feature for defining notification types via CDS @notification annotations, HTML email file references, i18n resolution, and automatic type registration at startup. The implementation is thorough with good test coverage.
Two issues were flagged:
-
lib/compile.js–resolveHtmlFilecrash on missing$location: When a CDS event definition lacks a$location(e.g. programmatically constructed models), passingundefinedas thelocationargument and then accessinglocation.filethrows aTypeError. A null guard was suggested. -
lib/content-deployment.js– operator precedence with spread and??: The expression...readFile(filePath) ?? []has ambiguous precedence intent; parentheses should be used to make the fallback apply correctly:...(readFile(filePath) ?? []).
Additionally, there is a minor typo in README.md line 219: "iori notifications icon" should be "Fiori notifications icon" (capital 'F' dropped).
PR Bot Information
Version: 1.21.0
- Correlation ID:
a1fd8159-c276-4391-8e4e-0a856cca8015 - Event Trigger:
pull_request.opened - File Content Strategy: Full file content
- LLM:
anthropic--claude-4.6-sonnet
| function resolveHtmlFile(value, location) { | ||
| if (typeof value !== 'string') return value | ||
| if (!value.startsWith('./') && !value.startsWith('../')) return value | ||
|
|
||
| const cdsFile = resolve(cds.root, location.file) |
There was a problem hiding this comment.
Bug: resolveHtmlFile crashes with TypeError when location is undefined
If def.$location is absent (e.g. in programmatically constructed models or certain CDS compiler outputs), location will be undefined. When the annotation value starts with ./ or ../, the code reaches location.file and throws TypeError: Cannot read properties of undefined (reading 'file').
Should guard against a missing location before dereferencing it.
| function resolveHtmlFile(value, location) { | |
| if (typeof value !== 'string') return value | |
| if (!value.startsWith('./') && !value.startsWith('../')) return value | |
| const cdsFile = resolve(cds.root, location.file) | |
| function resolveHtmlFile(value, location) { | |
| if (typeof value !== 'string') return value | |
| if (!value.startsWith('./') && !value.startsWith('../')) return value | |
| if (!location?.file) { | |
| LOG._warn && LOG.warn(`Cannot resolve relative HTML path '${value}': no source location available`) | |
| return value | |
| } | |
| const cdsFile = resolve(cds.root, location.file) | |
| const htmlPath = resolve(dirname(cdsFile), value) | |
| if (!existsSync(htmlPath)) { | |
| LOG._warn && LOG.warn(`HTML file not found: ${htmlPath}`) | |
| return value | |
| } | |
| return readFileSync(htmlPath, 'utf8') | |
| } |
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
|
|
||
| const notificationTypes = [ | ||
| ...notificationTypesFromModel(model), | ||
| ...readFile(filePath) ?? [] |
There was a problem hiding this comment.
Bug: Operator precedence causes spread of null/undefined instead of empty array
The expression ...readFile(filePath) ?? [] is parsed as ...(readFile(filePath) ?? []) only if readFile never returns null/undefined — but if filePath is an empty string '', readFile logs a warning and returns [], so this is safe in practice. However, the ?? has lower precedence than the spread operator, so the actual parse is (...readFile(filePath)) ?? [], which means the nullish coalescing never applies as intended and a null/undefined return from readFile would cause a runtime error spreading null.
Should wrap the expression in parentheses to ensure correct evaluation order.
| ...readFile(filePath) ?? [] | |
| ...(readFile(filePath) ?? []) |
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: Feat: Define Notification Types via CDS Annotations and Add Email SupportNew FeaturesThis pull request introduces a powerful new way to define notification types directly within your CDS models using ✨ Key Enhancements:
ChangesThe documentation has been completely rewritten to reflect these new features, using a
PR Bot InformationVersion:
|
1 participant
Should support using HTML files for email body for clarity