feat: dispatch custom DOM events for AI generation success/failure#180
feat: dispatch custom DOM events for AI generation success/failure#180
Conversation
Emit dxpr:ai:generation:success and dxpr:ai:generation:failure CustomEvents on document after AI generation completes. Events include model, promptLength, generationDurationMs, and error details. This enables external analytics integration without hard dependencies. Relates to dxpr/dxpr_builder#4414
|
Follow-up update pushed: 7601d7b What changed:
Warning: please include this latest commit in any release/update paired with dxpr/dxpr_builder#4427; otherwise CKEditor success analytics will under-report output metrics (notably |
|
Review finding (P2): Canceled CKEditor generations can be emitted as success. File:
Suggested fix: add an |
|
Addressed the cancellation/success finding in What changed:
Commit: This prevents canceled runs from emitting Also confirmed current PR checks are green:
|
jjroelofs
left a comment
There was a problem hiding this comment.
Code Review
Looks clean overall. A few notes:
generatedContent redundant intermediate assignment
In handleStreamingResponse, generatedContent = contentBuffer is assigned inside updateContent() (~line 407) on every content flush, but it's also assigned in the finally block (~line 447). The finally assignment is the correct/authoritative one since contentBuffer may have new data after the last flush. The intermediate assignment inside updateContent is now redundant — could be removed for clarity.
getOutputMetrics creates a temporary DOM element
The getOutputMetrics method (aiagentservice.ts:53-56) creates a <div>, sets innerHTML, and extracts textContent for word counting. This is fine since it's only called once at the end of generation (not per-chunk). Just noting the approach — it's simple and correct.
Good: double processCompleted removed
The duplicate this.processContentHelper.processCompleted(blockID) that used to run after the streaming finally block is correctly removed. The one inside finally is sufficient.
Good: cancel guard
The if (this.abortGeneration) return; check before dispatching the success event (aiagentservice.ts:349-351) correctly prevents a success event from firing when the user canceled mid-stream. The this.abortGeneration = false reset at the top of run() ensures clean state for each generation.
| Severity | Item |
|---|---|
| Low | Redundant generatedContent = contentBuffer inside updateContent() |
| ✅ | Cancel guard, double processCompleted fix, abort reset |
|
Follow-up update pushed in eb314a1. What changed:
Earlier fixes remain in place:
Warning: include these latest plugin commits in any release/update paired with dxpr/dxpr_builder#4427, otherwise analytics parity between builder and CKEditor paths will drift. |
jjroelofs
left a comment
There was a problem hiding this comment.
Follow-up review (commit eb314a1)
chore: remove redundant streaming metrics assignment — Addressed
The redundant generatedContent = contentBuffer inside updateContent() was removed as suggested. The finally block assignment is now the sole place where generatedContent is set. Good.
Minor: accidental indentation change
The updateContent closure picked up an extra level of indentation (3 tabs → function body at 4 tabs, was 2 → 3). Cosmetic only but inconsistent with the surrounding code:
- const updateContent = async () => {
- if ( contentBuffer ) {
+ const updateContent = async () => {
+ if ( contentBuffer ) {Worth a quick fix.
Overall
LGTM — all review items addressed.
|
Addressed the follow-up cosmetic note in 2f4b9a3.
At this point all follow-up review items are addressed. |
jjroelofs
left a comment
There was a problem hiding this comment.
Follow-up review (commit 2f4b9a3)
style: normalize streaming updateContent indentation
Fixes the indentation I flagged — but only partially. updateInterval was re-indented to match updateContent, but now both are at 3 tabs instead of the expected 2:
let generatedContent = ''; // 2 tabs ✓
const updateInterval = ...; // 3 tabs ✗
const updateContent = ...; // 3 tabs ✗These local variables are inside handleStreamingResponse which is indented at 2 tabs. They should be at 2 tabs as well.
|
You were right — it was still over-indented. I pushed 14da4cf to fully normalize tab depth in
Verified with visible-tab check and |
Summary
dxpr:ai:generation:successCustomEvent on document after successful AI generationdxpr:ai:generation:failureCustomEvent on document after failed AI generationmodel,promptLength,generationDurationMs,outputLength(success) /errorType,errorCode(failure)Relates to dxpr/dxpr_builder#4414
Test plan
dxpr:ai:generation:successevent dispatched on documentdxpr:ai:generation:failureevent dispatched