Conversation
Contributor
Author
|
@oliverfoster Will you review this PR? What are your thoughts on implementing this fix in Spoor vs. Core (assuming you agree that it's a bug that should be fixed)? |
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.
Fixes #319
Fix
A trickle-locked course using
_completionCriteria._requireContentCompleted: truecould fail to report acompletedstatus to the LMS when the course completed for the first time during the restore on reopen.Root cause: core
tracking.jssubscribes toAdapt.course change:_isCompleteonapp:dataReadyand firestracking:complete. Spoor only subscribed totracking:completeinsidesetupEventListeners, which runs onadapt:start(afterapp:dataReady). Trickle buttons are created onadapt:startand are not tracked, so they do not exist during the restore. On reopen, the restored content completes the blocks in theapp:dataReadywindow, the course completes, and core firestracking:completebefore spoor is listening. The event is missed and the LMS status is never written.This change subscribes
tracking:completeinbeginSessionas well, so the listener exists before the restore completion cascade. It mirrors howapp:dataReadyandadapt:startare already registered in bothbeginSessionandsetupEventListeners;setupEventListenersstill re-binds it for the live session after itsstopListeningreset, so there is exactly one active listener at any time.Testing
_completionCriteria._requireContentCompleted: trueand spoor_onTrackingCriteriaMet: "completed".incompleteand suspend data is committed.incomplete. After this fix: LMS status becomescompleted.Also verified, with the early listener in place:
passedand the score correctly._isLockedOnRevisitre-completion fires once per genuine completion, with no spurious write on the intervening reset.Alternatives considered / anticipated questions
Is this trickle behaving as intended? Should the course stay incomplete until the final button is clicked?
The course completing on restore is correct: all content components are genuinely complete, and trickle buttons are deliberately non-tracked UI for pacing, not learning criteria. Requiring a learner to re-click "Finish" on resume to re-confirm already-complete content would be poor UX and is not how resume is expected to work. The defect is not that the course completes; it is that the
tracking:completeevent is dropped, so the LMS is never told. That event loss is unambiguously a bug.Why fix in spoor rather than core?
The race is generic (core fires
tracking:completeduring restore before consumers subscribe), so a core-side change is a legitimate option, e.g. deferring the completion check until afteradapt:start. That was rejected for now because it has a much larger blast radius across everytracking:completeconsumer, whereas the spoor-side listener is an 8-line, self-contained change that restores the intended contract (spoor listens for completion) without altering core timing for anyone else. Happy to move it to core if maintainers prefer the systemic fix.Why not just disable trickle on the final block?
It does avoid the bug (step locking means the only "all content complete but a gate pending" state is the final button), but it is not an acceptable resolution. The vulnerable configuration is trickle's own documented default:
example.jsonships_showEndOfPage: truewith a final "Finish" button. So the workaround means "do not use trickle the way the docs show," it is silent (no warning links the missing LMS status to a Finish button), and it removes a deliberate end-of-page affordance to dodge a tracking-layer timing issue. It masks one trigger, not the underlying event-ordering gap.Does this affect xAPI /
adapt-contrib-xapi?Possibly. xapi subscribes to
tracking:completeand may share the same restore-window timing gap (candidate cause of adapt-contrib-xapi#108). Out of scope for this PR, but flagging it: if maintainers prefer the core-side fix, it would likely cover xapi too.Posted via collaboration with Claude Code