Send out web socket event when malware scanner finishes#435
Conversation
SummaryThe following content is AI-generated and provides a summary of the pull request: Emit WebSocket Event When Malware Scanner FinishesNew Feature✨ Automatically emits an
Changes
PR Bot InformationVersion:
|
There was a problem hiding this comment.
The PR introduces a useful WebSocket event feature for malware scan completion, but has several correctness issues: unhandled errors in fire-and-forget cds.spawn blocks, a fragile isActiveEntity derivation that can produce wrong OData paths, potential mismatch when updateStatus is called with a complex CQL where expression rather than plain keys, and a test spy that overwrites spawnCallback on each call — masking assertions on the first spawn invocation.
PR Bot Information
Version: 1.20.33
- Correlation ID:
08a912ba-f14b-4cb8-ba1e-9069d2b01d40 - Event Trigger:
pull_request.opened - File Content Strategy: Full file content
- LLM:
anthropic--claude-4.6-sonnet
| ) | ||
| const result = draft ?? active | ||
| if (result) { | ||
| const isActiveEntity = !draft |
There was a problem hiding this comment.
Logic Error: isActiveEntity is derived from whether a draft row exists, but this is unreliable
const isActiveEntity = !draft means: if a draft record exists, isActiveEntity=false; otherwise true. However, draft is the result of SELECT.one.from(target.drafts).where(where) — if the draft row simply doesn't exist in the DB at query time (e.g., was already activated), this will incorrectly emit IsActiveEntity=true even if the attachment actually belongs to an active entity reached via draft activation. The logic works for the happy path but conflates "no draft row" with "is active entity", which can produce a wrong sideEffectSource path and fail to trigger the UI side-effect on the correct entity.
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 [active, draft] = await Promise.all([ | ||
| SELECT.one.from(target).where(where), | ||
| SELECT.one.from(target.drafts).where(where), | ||
| ]) |
There was a problem hiding this comment.
Bug: updateStatus is called with a complex CQL expression array for where (not a plain key object) when scanning completes, but the spawned SELECT.one.from(target).where(where) and SELECT.one.from(target.drafts).where(where) will receive that complex expression on the initial updateStatus("Scanning") call and then again on the final status call with a hash-qualified expression. The CQL where array including or and xpr nodes may not be supported by all CDS DB adapters in the same way for SELECT, potentially returning no result and silently skipping the event emission.
Consider querying by the original plain keys object (passed as a separate parameter, or stored before the compound expression is built) rather than the raw where argument which can be an arbitrary CQL expression.
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
Note: Does not yet fully work with Fiori elements because somehow a table row cannot be targeted by the side effect event. Event is however received.