CLUE-614: authored prompt in QUESTION_ANSWERS_CHANGE + history/iframe log fixes - #2950
CLUE-614: authored prompt in QUESTION_ANSWERS_CHANGE + history/iframe log fixes#2950lbondaryk wants to merge 2 commits into
Conversation
… log fixes Ask 1 (deliverable): add the Question tile's authored prompt to QUESTION_ANSWERS_CHANGE as a top-level `prompt` key alongside questionId, so the report's $.prompt lookup shows the prompt as the column header instead of the raw questionId. New getQuestionPrompt() returns the fixed-position "Question Prompt" text tile's plain text. Ask 3: moveToHistoryEntryAfterLoad now treats the "first" sentinel (emitted for a student's first change, before any history entry) as index 0, so first-change playback links land instead of opening the UI at a position it never moved to. Affects both QUESTION_ANSWERS_CHANGE and free-standing TEXT_TOOL_CHANGE links. Ask 4: route iframe-interactive logging through logTileChangeEvent instead of Logger.log, so IFRAME_INTERACTIVE_TOOL_CHANGE carries toolId/documentKey/containerIds/documentHistoryId/ tileTitle and the report can link and separate a learner's iframe tiles. Asks 2 (<TYPE>_TOOL_CHANGE naming) and 5 (flag event renames) are conventions — no code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR improves CLUE’s logging and playback behavior to support richer reporting for Researcher Reports (REPORT-36), specifically by including authored question prompts in QUESTION_ANSWERS_CHANGE, correctly handling the "first" history-id sentinel during playback seeking, and ensuring iframe-interactive tile tool-change logs are enriched consistently with other tiles.
Changes:
- Added
getQuestionPrompt()and threaded a top-levelpromptfield intoQUESTION_ANSWERS_CHANGElogging payloads. - Updated history playback seeking to treat
"first"as history index0. - Routed iframe-interactive
"log"messages throughlogTileChangeEvent()to include standard tile-change enrichment fields.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/plugins/iframe-interactive/iframe-interactive-tile.tsx | Routes iframe interactive log messages through logTileChangeEvent for enriched, linkable tool-change logs. |
| src/plugins/iframe-interactive/iframe-interactive-tile.test.tsx | Adds coverage ensuring iframe "log" events call logTileChangeEvent with the expected shape. |
| src/models/tiles/question/question-utils.ts | Introduces getQuestionPrompt() to extract the fixed-position prompt text for Question tiles. |
| src/models/tiles/question/question-utils.test.ts | Adds unit tests for getQuestionPrompt() behavior. |
| src/models/tiles/log/log-tile-base-event.ts | Includes top-level prompt in QUESTION_ANSWERS_CHANGE event parameters. |
| src/models/history/firestore-history-manager.ts | Treats "first" history id sentinel as entry index 0 in moveToHistoryEntryAfterLoad(). |
| src/models/history/firestore-history-manager.test.ts | Adds tests covering "first" sentinel handling and unresolved-id behavior. |
| src/models/document/document-content-tests/question-tile-operations.test.ts | Verifies prompt key is present on emitted QUESTION_ANSWERS_CHANGE parameters. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
collaborative-learning
|
||||||||||||||||||||||||||||
| Project |
collaborative-learning
|
| Branch Review |
CLUE-614-authored-prompt-to-question-answers-change-log-events
|
| Run status |
|
| Run duration | 10m 36s |
| Commit |
|
| Committer | Leslie Bondaryk |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
0
|
|
|
1
|
|
|
5
|
|
|
0
|
|
|
220
|
| View all changes introduced in this branch ↗︎ | |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2950 +/- ##
==========================================
- Coverage 86.07% 86.06% -0.01%
==========================================
Files 980 980
Lines 55955 55964 +9
Branches 14754 14758 +4
==========================================
+ Hits 48161 48165 +4
- Misses 7774 7779 +5
Partials 20 20
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
dougmartin
left a comment
There was a problem hiding this comment.
The Ask 1 and Ask 3 changes look right. Ask 4 needs another pass: routing the iframe log listener through logTileChangeEvent also makes every interactive breadcrumb emit a full QUESTION_ANSWERS_CHANGE, and the field the report keys on (operation) is read from a key the LARA API never sends.
Changes requested
-
src/plugins/iframe-interactive/iframe-interactive-tile.tsx(lines 354-360):logTileChangeEvent→logTileBaseEventwalkscontainerIdsand callslogAnswerChangefor any container that is a Question tile (src/models/tiles/log/log-tile-base-event.tslines 54-61). The oldLogger.logcall did not. So an interactive nested in a Question tile now emits a fullQUESTION_ANSWERS_CHANGE, carrying every answer tile's JSON plus the newprompt, on everylog()message the interactive posts.log()is an analytics breadcrumb (button clicked, hint viewed), not a state change, so the answer-change stream gets driven by the wrong signal and inflated. Meanwhile the change that actually is the student's answer,handleInteractiveState/debouncedSetState(lines 204-221), still logs nothing, so an interactive that never callslog()produces no answer-change event even after this PR. Either log the state change itself vialogTileChangeEventand leave rawlogbreadcrumbs on the un-enriched path, or confirm in the PR that the amplification is intended and bounded. -
src/plugins/iframe-interactive/iframe-interactive-tile.tsx(line 357) andsrc/plugins/iframe-interactive/iframe-interactive-tile.test.tsx(lines 314-320):logData?.eventis dead code.@concord-consortium/lara-interactive-apideclareslog: (action: string, data?: object) => void(api.d.tsline 87) and postspost("log", { action, data })(index.jsline 5029), so there is noeventkey. The new test feeds{ event: "submit", value: 42 }, i.e. the one shape that never occurs in production, so the real{ action, data }path and theoperationvalue the report keys on have zero coverage. Reduce tooperation: logData?.action ?? "log"and change the test payload tologHandler({ action: "submit", data: { value: 42 } })with the matching expectation. -
src/models/tiles/question/question-utils.ts(line 56):asPlainText()returns""for an empty Text tile, and unlikeundefinedan empty string survivesJSON.stringifyinLogger.sendToLoggingService. The stated contract is that the report ignorespromptwhen absent and falls back to the question id, but a present-but-empty string is not absent, so a question with a blank prompt tile renders a blank column header instead of falling back. Fix:const text = (tile.content as TextContentModelType).asPlainText(); return text.trim() || undefined;
Non-blocking
-
src/models/history/firestore-history-manager.ts(lines 292-296): the comment calls index 0 "the earliest entry", butgoToHistoryEntry(n)takes a position (number of entries applied), not an entry index: it early-returns whenn === numHistoryEventsAppliedand replays[numHistoryEventsApplied, n-1](src/models/history/tree-manager.tslines 527-561). So 0 means "before any entry is applied", i.e. the empty starting state, not "at the first change". Mapping"first"to 0 is defensible because the resolved-id path has the same off-by-one (which is what CLUE-613 is about), but the stated justification is wrong and would mislead whoever fixes CLUE-613. Reword to "position 0, i.e. before any history entry is applied, matching how a resolved id maps to the position before its entry." (Collision risk is nil: real entry ids arenanoid().) -
src/models/document/document-content-tests/question-tile-operations.test.ts(lines 140-142):toHaveProperty("prompt")proves nothing here, and the comment "the key must be present for the report" is false, becauseLogger.sendToLoggingServiceserializes withJSON.stringify(src/lib/logger.tsline 213), which drops keys whose value isundefined. The assertion inspects the pre-serialization object, so it passes on a key that never reaches the report. Drop the comment and either drop the assertion or use a fixture with a real fixed-position prompt tile and assertexpect(logSpy.mock.calls[1][1].prompt).toBe("<authored text>"). -
src/models/tiles/question/question-utils.test.ts(lines 171-187): both new tests use hand-rolled object literals cast throughunknown, withisFixedPositionas a plain boolean andasPlainTextas an arrow function. No realQuestionContentModel,TileModel, orTextContentModelis exercised, so a regression in howdefaultQuestionContentcreates and marks the prompt tile would fail nothing. Worth one case inquestion-tile-operations.test.tsbuilt fromdefaultQuestionContent, setting the prompt text and asserting the loggedpromptmatches. -
src/models/tiles/question/question-utils.ts(lines 42-43): the doc comment says the fixed-position "Question Prompt" Text tile, but the code matches the first fixed-position Text tile in row order and never checks the title. That is consistent withgetQuestionAnswersAsJSON, which skips all fixed-position tiles (line 85), butfixedPositionis an authorable prop that round-trips through export (src/models/tiles/tile-model.tslines 82, 147-148), so authored content with two fixed Text tiles would silently log the wrong string. Either match on the title with the fixed-position check as fallback, or reword the comment to state the actual contract. -
src/plugins/iframe-interactive/iframe-interactive-tile.tsx(lines 355-359): the old payload set a top-leveltileType: "IframeInteractive". NeitherprocessTileChangeEventnorprocessTileBaseEventParamsaddstileType, so it is gone. Probably fine since the report derives tile type from the event name (your Ask 5), but that is exactly the kind of silent shape change Ask 5 warns about, so it is worth a line in the PR description confirming the report owner does not readparameters.tileTypefor this event. -
src/models/history/firestore-history-manager.ts(line 292):await when(() => historyStatus === HISTORY_LOADED)never settles when the status isNO_HISTORY(getter at lines 256-258), so the promise leaks silently and a dead playback link is indistinguishable from a slow one. Pre-existing, but"first"is emitted precisely for documents at the start of their history, so the new path is the most likely to hit it. Consider addingNO_HISTORY/HISTORY_ERRORas terminating conditions and warning instead of hanging. -
src/plugins/iframe-interactive/iframe-interactive-tile.tsx(line 358) withsrc/models/tiles/log/log-tile-change-event.ts(line 23):processTileChangeEventdoes{ toolId: tileId, operation, ...change }. A non-objectlogDatafrom a misbehaving iframe spreads into indexed character keys, and alogData.operationorlogData.toolIdkey silently overrides the values just set. Pre-existing with the old...logData, but carried forward. Normalizing at the call site would close it:change: (logData && typeof logData === "object" && !Array.isArray(logData)) ? logData : { value: logData }. -
src/models/history/firestore-history-manager.test.ts(lines 334-338, 345):expect(findSpy).not.toHaveBeenCalled()asserts how the result is produced rather than the behavior, and fails on a harmless refactor. AlsogoToSpy(334, 345) andfindSpy(335) are never restored; onlywarnSpyis (line 350). Drop thefindSpyassertion and addafterEach(() => jest.restoreAllMocks()). -
src/models/tiles/question/question-utils.test.ts(line 171):describe("getQuestionPrompt")is nested insidedescribe("getQuestionAnswersAsJSON")(opened line 53), so the new tests report under the wrong function. Move it out as a sibling and hoistmakeMockTextTile/makeMockDocumentto the outer scope. -
src/models/tiles/log/log-tile-base-event.ts(line 75),src/models/history/firestore-history-manager.test.ts(line 330),src/models/document/document-content-tests/question-tile-operations.test.ts(line 140),src/plugins/iframe-interactive/iframe-interactive-tile.test.tsx(line 301): ticket ids and the internal "Ask N" structure in comments and test names. Six months out nobody can resolve "Ask 3", and it shows up in test-runner output. Suggestit("treats the 'first' sentinel as position 0", ...),it("routes interactive log messages through logTileChangeEvent", ...), and dropping "(REPORT-36 $.prompt lookup)". -
src/plugins/iframe-interactive/iframe-interactive-tile.tsx(lines 351-353) andsrc/models/tiles/log/log-tile-base-event.ts(lines 75-76): both comments argue for the change ("logging Logger.log directly bypasses that enrichment...", "a different name or nesting silently falls back to the id") rather than describing the code as it stands. That is PR-description material; a one-liner each would do. -
src/plugins/iframe-interactive/iframe-interactive-tile.tsx(line 99): stale comment// Note: onLog and onHintChange removed - will use Logger directly. TheLoggerimport is removed by this PR. -
src/models/tiles/log/log-tile-change-event.ts(lines 21-25) withsrc/models/stores/documents.ts(lines 357-359):findDocumentOfTileonly searchesdocuments/networkDocuments. If an iframe interactive is ever rendered from content not backed by a storeDocumentModel,documentisnull,isTileBaseEventreturns false, and the event falls through to plainLogger.logwith none of the enrichment Ask 4 is adding, silently, so the gap looks fixed. Worth confirming that case cannot occur.
…eadcrumbs
Ask 4 rework (B1/B2): route the interactive's *state change* (debouncedSetState →
setInteractiveState — the student's answer) through logTileChangeEvent, and leave the
raw `log()` analytics breadcrumbs on the un-enriched Logger.log path. This stops every
breadcrumb from emitting a full QUESTION_ANSWERS_CHANGE and ensures the real answer
signal is logged. Drops the dead `logData?.event` mapping (LARA sends {action,data}).
B3: getQuestionPrompt treats a blank prompt as absent (text.trim() || undefined) so the
report falls back to the questionId instead of showing a blank column header.
Non-blocking cleanups: reword the history comment to "position 0" semantics (ties to
CLUE-613); real-model getQuestionPrompt tests via defaultQuestionContent (replacing the
mock-based ones and the meaningless toHaveProperty assertion); drop the implementation-
coupled findSpy assertion and add afterEach restoreAllMocks; drop "Ask N"/ticket ids from
test names; trim argumentative comments.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks — all three blocking items addressed in B1 (Ask 4 signal) — reworked to your recommendation. The interactive's state change ( B2 (dead B3 (blank prompt) — fixed. Non-blocking, done:
Non-blocking, acknowledged (not changed):
Re-requesting review. |
Supports full CLUE reporting for the Researcher Reports Student Answers report (REPORT-36). Implements asks 1, 3, and 4; asks 2 and 5 are conventions with no code.
Ask 1 (the deliverable) — authored prompt in
QUESTION_ANSWERS_CHANGEToday every report column falls back to the raw 6-char
questionIdbecause the authored prompt was never in the event payload (confirmed in production: all 44 columns of the MODS PD Spring 2026 run showed the bare id, zero prompts).getQuestionPrompt(doc, questionContent)returns the fixed-position "Question Prompt" Text tile'sasPlainText()(the same tilegetQuestionAnswersAsJSONskips).logAnswerChangethreads it as a top-levelpromptkey alongsidequestionId. The report already reads$.promptand ignores it when absent, so no report-service change, no redeploy — headers start showing the prompt for new logs the day it ships. The exact key name/placement matters (a different name or nesting silently stays on the id fallback), so it is exactlyprompt, top-level.Ask 3 — resolve the
"first"history-id sentinellogDocumentEventemitsdocumentHistoryId: "first"for a change made before a new document has any history entry (3.5% of production QUESTION_ANSWERS_CHANGE events). Nothing resolved it:findHistoryEntryIndexreturned -1 andmoveToHistoryEntryAfterLoadfell through to aconsole.warnwithout navigating — while the playback UI had already opened, so it read as positioned when it was not. NowmoveToHistoryEntryAfterLoadtreats"first"as index 0 (what the emitter means), so first-change links land. Fixes both QUESTION_ANSWERS_CHANGE and the shipped free-standing TEXT_TOOL_CHANGE links (samelogDocumentEventpath).Ask 4 — route iframe-interactive logging through
logTileChangeEventiframe-interactive-tile.tsxcalledLogger.logdirectly, soIFRAME_INTERACTIVE_TOOL_CHANGEbypassed the enrichment and carried notoolId/documentKey/containerIds/documentHistoryId/tileTitle(absent on 100% of 19,110 production events) — the report can't link them and collapses a learner's iframe tiles into one entry. Now routed throughlogTileChangeEvent(LogEventName.IFRAME_INTERACTIVE_TOOL_CHANGE, { tileId, operation, change })like every other tile; the report's gate is structural, so these appear with no report-service change.Asks 2 & 5 (conventions, no code)
<TYPE>_TOOL_CHANGE(the report discovers types by that pattern). Already followed.GRAPH_TOOL_CHANGE→GEOMETRY_TOOL_CHANGErename). Flag any future rename so the report can carry a mapping.Related
CLUE-613 is an adjacent but distinct defect in the same
moveToHistoryEntryAfterLoadfunction (an id that does resolve still fails to seek). Left as its own ticket — worth looking at alongside this.Verification
tsc+ lint clean; 61 tests across the four suites, including new coverage forgetQuestionPrompt, thepromptkey in the emitted event,moveToHistoryEntryAfterLoad("first") → 0, and the iframe "log" listener routing throughlogTileChangeEvent.🤖 Generated with Claude Code