CLUE-615: add tile-change logging to Image, Expression, AI, and Numberline - #2948
Open
lbondaryk wants to merge 4 commits into
Open
CLUE-615: add tile-change logging to Image, Expression, AI, and Numberline#2948lbondaryk wants to merge 4 commits into
lbondaryk wants to merge 4 commits into
Conversation
…rline These tile types emitted no tile-change event, so free-standing student work in them was invisible to the Researcher Reports Student Answers report. Emitting a <TYPE>_TOOL_CHANGE routes through logTileBaseEvent, which fires the QUESTION_ANSWERS_CHANGE side-effect the report reads when the tile is inside a Question — so each type widens the report automatically, no report-service change. - New LogEventName entries: IMAGE_, EXPRESSION_, AI_, NUMBERLINE_TOOL_CHANGE. - Image: log on setUrl (the answer-capture moment) — the largest gap (222 tiles / 51 docs). - Expression: log on setLatexStr. - AI: log each content setter (setPrompt/setText/setDescription/setHidePrompt/requestRefresh). - Numberline: log point create/delete and min/max changes. - Guard logTileChangeEvent to no-op when logging is disabled (mirrors Logger.log's own guard): skips the document lookup and avoids dereferencing the uninitialized Logger.stores in component tests that trigger content changes. Simulator and Graph are deferred (their content mutates through shared models / a large model that needs a different approach). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds missing tile-change logging for Image, Expression, AI, and Numberline tiles so their content edits can be captured by the existing logTileChangeEvent(...) → logTileBaseEvent(...) pipeline (including Question-tile answer-change side-effects), without requiring report-service changes.
Changes:
- Added new
LogEventNameentries for the four tile types and wired relevant setters/actions to calllogTileChangeEvent(...). - Added/updated unit tests for the four tiles to assert that logging fires on content mutation.
- Added an early
Logger.isLoggingEnabledguard inlogTileChangeEventto avoid dereferencing uninitializedLogger.storesin environments where logging is off (e.g., certain component tests).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/plugins/numberline/models/numberline-content.ts | Emits NUMBERLINE_TOOL_CHANGE events for min/max and point create/delete actions. |
| src/plugins/numberline/models/numberline-content.test.ts | Adds Jest coverage for numberline change-logging behavior. |
| src/plugins/expression/expression-content.ts | Emits EXPRESSION_TOOL_CHANGE when LaTeX content changes. |
| src/plugins/expression/expression-content.test.ts | Adds Jest assertion that expression edits trigger logging. |
| src/plugins/ai/ai-content.ts | Emits AI_TOOL_CHANGE across AI content setters and refresh requests. |
| src/plugins/ai/ai-content.test.ts | Adds Jest assertion(s) for AI logging on content mutation. |
| src/models/tiles/log/log-tile-change-event.ts | Adds a logging-disabled short-circuit to prevent crashes when Logger isn’t initialized. |
| src/models/tiles/image/image-content.ts | Emits IMAGE_TOOL_CHANGE when the image URL/filename is set. |
| src/models/tiles/image/image-content.test.ts | Adds Jest assertion that setting the image URL triggers logging. |
| src/lib/logger-types.ts | Introduces the four new log event name enum entries. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+68
to
+79
| it("logs an AI_TOOL_CHANGE on each content change", () => { | ||
| const content = AIContentModel.create(); | ||
| (logTileChangeEvent as jest.Mock).mockClear(); | ||
| content.setPrompt("ask something"); | ||
| expect(logTileChangeEvent).toHaveBeenCalledWith(LogEventName.AI_TOOL_CHANGE, { | ||
| tileId: "", operation: "setPrompt", change: { prompt: "ask something" } | ||
| }); | ||
| content.requestRefresh(); | ||
| expect(logTileChangeEvent).toHaveBeenCalledWith(LogEventName.AI_TOOL_CHANGE, { | ||
| tileId: "", operation: "requestRefresh", change: { refreshCount: 1 } | ||
| }); | ||
| }); |
Contributor
Author
There was a problem hiding this comment.
Expanded in bedbca4 — the test now asserts AI_TOOL_CHANGE for all five setters (setPrompt/setText/setDescription/setHidePrompt/requestRefresh).
Comment on lines
+21
to
31
| it("logs a NUMBERLINE_TOOL_CHANGE when min/max change", () => { | ||
| const content = NumberlineContentModel.create(); | ||
| (logTileChangeEvent as jest.Mock).mockClear(); | ||
| content.setMin(-5); | ||
| content.setMax(5); | ||
| expect(logTileChangeEvent).toHaveBeenCalledWith(LogEventName.NUMBERLINE_TOOL_CHANGE, | ||
| { tileId: "", operation: "setMin", change: { min: -5 } }); | ||
| expect(logTileChangeEvent).toHaveBeenCalledWith(LogEventName.NUMBERLINE_TOOL_CHANGE, | ||
| { tileId: "", operation: "setMax", change: { max: 5 } }); | ||
| }); | ||
| }); |
Contributor
Author
There was a problem hiding this comment.
Added in bedbca4 — the test now asserts deleteSelectedPoints (with the deleted ids) and deleteAllPoints.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2948 +/- ##
==========================================
- Coverage 86.07% 85.97% -0.11%
==========================================
Files 980 980
Lines 55955 56003 +48
Branches 14754 14760 +6
==========================================
- Hits 48161 48146 -15
- Misses 7774 7836 +62
- Partials 20 21 +1
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:
|
collaborative-learning
 |
||||||||||||||||||||||||||||
| Project |
collaborative-learning
|
| Branch Review |
CLUE-615-tile-change-logging-for-missing-tile-types
|
| Run status |
|
| Run duration | 03m 24s |
| Commit |
|
| Committer | Leslie Bondaryk |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
 |
0
|
 |
0
|
 |
0
|
 |
0
|
 |
4
|
| View all changes introduced in this branch ↗︎ | |
The graph content model has ~43 actions, most UI-state/selection/styling (some, like setInteractionInProgress, fire per drag frame). Rather than an ignore-list of the noisy majority, override the base no-op onTileAction and log only an allow-list of answer-relevant actions (attribute/axis/plot/label/layer/adornment changes) as GRAPH_TOOL_CHANGE. Logging only reads state, so it adds no history entry (CLUE-496 invariant preserved). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
….log The isLoggingEnabled guard I added short-circuited logTileChangeEvent before Logger.log. The cypress specs stub Logger.log and assert tile-change events reach it even though logging isn't "enabled" in the qa app mode, so the guard broke every log-assertion spec (drawing, dataflow, datacard, ...). Remove the guard; instead tolerate an undefined Logger.stores (the only thing that actually crashed in un-initialized component tests) by null-guarding the document lookup in processTileChangeEvent. Cypress keeps working (Logger.log is still called); component tests no longer crash. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- ai-content.test: assert AI_TOOL_CHANGE for all five setters (setText/setDescription/ setHidePrompt too), matching the "on each content change" test name. - numberline-content.test: add deleteSelectedPoints/deleteAllPoints assertions. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Four registered tile types emitted no tile-change event, so free-standing student work in them was invisible to the Researcher Reports Student Answers report. This wires them into the existing
logTileChangeEvent(LogEventName.<TYPE>_TOOL_CHANGE, …)convention.Why this widens the report with no report-service change:
logTileChangeEvent→logTileBaseEvent, which — when the tile sits inside a Question tile — fires theQUESTION_ANSWERS_CHANGEside-effect the report reads. So each type we wire up shows up in the report automatically.Changes
New
LogEventNameentries:IMAGE_TOOL_CHANGE,EXPRESSION_TOOL_CHANGE,AI_TOOL_CHANGE,NUMBERLINE_TOOL_CHANGE.setUrl(the answer-capture moment)setLatexStrsetPrompt/setText/setDescription/setHidePrompt/requestRefresh)setMin/setMaxonTileActionallow-list (attribute/axis/plot/label/layer/adornment)Each tile has a unit test that mocks the log module and asserts the setter fires the event.
Infra guard (please eyeball)
Wiring these up surfaced that component tests which render these tiles and trigger a setter crashed: the real
logTileChangeEventdereferencesLogger.storesbefore any logging-enabled check. Added an earlyif (!Logger.isLoggingEnabled) return;guard, mirroringLogger.log's own guard (logger.ts:203). It's a no-op in production (everything already bottoms out at the gatedLogger.log) but avoids the crash and skips the document-lookup work when logging is off. Alternative was mocking the log module per component test (the existing convention); the guard is the more robust fix but touches shared infra, so flagging it.Scope
Deferred: Simulator (26) — it has no student-authored content setter; its interactive state lives in the shared-variables model (slider →
variable.setValue), so logging it needs a shared-variable/UI-control hook rather than a content-setter — a separate follow-up. Graph is now included via anonTileActionallow-list (only the answer-relevant actions; the ~43-action model is mostly UI-state/styling, and logging only reads state so it adds no history entry).Verification
tscclean, lint clean; the four new tests pass, and 115 tests across the existing logging tiles + exemplar controller still pass with the guard.🤖 Generated with Claude Code