Repository navigation
Conversation
The screen asked for four deliberate actions per sentence — play, check, grade, next — when three of them are inferable. A learner has finished an answer when it is as long as the sentence; an answer they heard correctly needs nothing said about it; and arriving at a sentence means they want to hear it. What is left is typing, which is the exercise. Nothing is removed. The transport, Check and the grades are all still there for the times the inference is not what was wanted; they are no longer the way through. **Marking fires on completeness**, counted in characters that will be scored rather than characters typed — an input method inserting its own punctuation must not bring the check forward, and a half-typed syllable sitting in the composition buffer is not in the field yet, so it cannot either. **A correct answer moves on** after a beat long enough to read the result; a wrong one stays, because the marking is the whole feedback the exercise gives and taking it away unasked is worse than a button. **Arriving plays**, except in the boundary editor, where arriving somewhere is a drag rather than a decision to practise. **Resuming rewinds.** Pausing a sentence and starting it again restarted it, so pausing to think near the end of a long one meant never reaching the end. It picks up two seconds back now, which is what a podcast player does and for the same reason. **Sentences no longer begin mid-word.** Phrase playback seeked with a tenth of a second of tolerance. That is right for dragging a scrubber through a narration and wrong when the target *is* the first syllable: AVPlayer resolves a tolerance by taking whichever sync sample is cheapest, in practice an earlier one. Phrase playback seeks exactly; scrubbing keeps its tolerance. **The field asks for a Chinese input source** while it has focus, so practising does not mean switching twice a sentence and finding out you forgot after the answer is marked. It is a request: the system honours it when a matching input source is installed and ignores it otherwise, which is the right shape for something that could otherwise strand somebody. **Mistakes carry their pinyin**, beside what was typed. The reading is the thing worth taking away from a wrong character, and seeing it next to the wrong one is what turns "wrong" into "those two sound alike". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
PR Summary by QodoMake dictation practice flow from typing and fix phrase playback
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
Code Review by Qodo
1. Finished phrases replay only their tail
|
Comment on lines
+61
to
+65
| let from = max(segment.lowerBound, playhead - Self.resumeRewind) | ||
| // Close enough to the start that a rewind would land there anyway, | ||
| // or past the end after a finished pass: begin the phrase again. | ||
| let isNearlyOver = playhead >= segment.upperBound - 0.05 | ||
| play(segment: segment, from: isNearlyOver ? nil : from) |
There was a problem hiding this comment.
1. Finished phrases replay only their tail 🐞 Bug ≡ Correctness
resumeSegment() treats a phrase as finished only when the last reported playhead is within 50 ms of its endpoint. Phrase completion does not update that playhead, which is reported at 250 ms intervals, so pressing Play after completion or during a drill gap commonly starts about two seconds before the end instead of at the beginning.
Agent Prompt
## Issue description
A completed phrase is mistaken for a paused phrase because the finish callback leaves a periodically reported playhead short of its end.
## Fix Focus Areas
- Sources/HanReaderPlayback/PlaybackModel+Phrase.swift[22-34]
- Sources/HanReaderPlayback/PlaybackModel+Phrase.swift[59-65]
- Sources/HanReaderPlayback/PlaybackModel+Phrase.swift[85-96]
## Recommended Fix
Record completion when a phrase pass finishes and restart from its beginning on a subsequent Play; reserve rewind for an actual mid-phrase pause. Test replay with a playhead more than 50 ms short of the endpoint.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+50
to
+54
| let phrase = current | ||
| await check() | ||
| guard current == phrase, let result else { return } | ||
| if result.listeningAccuracy >= 1 { | ||
| await advanceAfterPass(from: phrase) |
There was a problem hiding this comment.
3. Returning to a phrase can show stale marks 🐞 Bug ≡ Correctness
answerChanged() accepts a completed check() when current again equals its original phrase index, without knowing whether the reader left and returned while the lookup was pending. On that navigation sequence, the old answer can replace the returned phrase’s result, record an attempt, and schedule another automatic advance.
Agent Prompt
## Issue description
An automatic check started before navigation can finish after the reader leaves and returns to the same phrase index.
## Fix Focus Areas
- Sources/HanReaderUI/Models/PracticeModel+Flow.swift[45-55]
- Sources/HanReaderUI/Models/PracticeModel.swift[221-233]
- Sources/HanReaderUI/Models/PracticeModel.swift[267-292]
## Recommended Fix
Associate each check with a selection generation or cancellable task, invalidate it on selection, and reject its result before marking, recording, or advancing if that generation has changed.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+157
to
+159
| let character = mark.expected ?? mark.typed | ||
| guard let character, let reading = readings[character]?.first else { return " " } | ||
| return reading.display |
There was a problem hiding this comment.
4. Some mistakes show the wrong reading 🐞 Bug ≡ Correctness
MarkView.pinyin displays the first dictionary reading for a wrong character without using its context in the sentence. For a character whose sentence reading differs from its globally ranked first reading, the new annotation teaches a pronunciation that does not match the audio.
Agent Prompt
## Issue description
Wrong-character pinyin uses the first standalone dictionary reading even when the character has a different reading in the sentence.
## Fix Focus Areas
- Sources/HanReaderUI/Screens/DictationMarkedView.swift[153-160]
- Sources/HanReaderUI/Models/PracticeModel.swift[279-291]
## Recommended Fix
Resolve expected-character readings in sentence context before passing them to the marked view. Keep standalone lookup only as a fallback where no contextual reading is available, and cover a polyphonic phrase in a test.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+144
to
+147
| Text(verbatim: pinyin) | ||
| .font(ReaderFont.ruby(style)) | ||
| .foregroundStyle(.secondary) | ||
| .frame(minHeight: style.rubyReservation) |
There was a problem hiding this comment.
5. Screen readers cannot hear mistake pinyin 🐞 Bug ≡ Correctness
MarkView adds the pinyin as a child Text but keeps .accessibilityElement(children: .ignore) and a label that omits that reading. VoiceOver users therefore receive the mistake classification and characters but not the pronunciation supplied by this feature.
Agent Prompt
## Issue description
The new pinyin text is hidden from accessibility because the mark ignores its children and provides a label without the reading.
## Fix Focus Areas
- Sources/HanReaderUI/Screens/DictationMarkedView.swift[141-160]
- Sources/HanReaderUI/Screens/DictationMarkedView.swift[178-191]
## Recommended Fix
Add the resolved reading to the accessible label for wrong marks, retaining the existing character and mistake information.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+91
to
+93
| func playOnArrival() { | ||
| guard phase == .ready, !isEditing, tapping == nil, result == nil else { return } | ||
| play() |
There was a problem hiding this comment.
6. The first manually split phrase stays silent 🐞 Bug ≡ Correctness
playOnArrival() suppresses playback while isEditing is true, but manual splitting enters the ready phase and opens the boundary editor before the writing view appears. Closing that editor does not retry playback, so the first phrase requires an explicit Play or a move to another phrase.
Agent Prompt
## Issue description
The initial arrival after manual splitting occurs while the boundary editor is open, and its suppressed playback is never retried.
## Fix Focus Areas
- Sources/HanReaderUI/Models/PracticeModel+Flow.swift[84-94]
- Sources/HanReaderUI/Models/PracticeModel+Boundaries.swift[47-61]
- Sources/HanReaderUI/Models/PracticeModel+Boundaries.swift[116-140]
## Recommended Fix
Keep playback suppressed during editing, but trigger it when the initial editor closes if the reader is still on the unplayed phrase. Cover the manual-split path in a flow test.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+137
to
+139
| .onChange(of: model.typed) { _, _ in | ||
| Task { await model.answerChanged() } | ||
| } |
There was a problem hiding this comment.
2. Going back to a correct sentence bounces you forward 🐞 Bug ≡ Correctness
select() restores a saved answer into typed, but PracticeScreen treats that restoration as a learner edit and calls answerChanged(). When the saved answer is complete and the result has been hidden, selecting its phrase in the progress strip or pressing ⌘← triggers check() without a keystroke, records another attempt, and moves the learner away after 1.2 seconds if the answer passes.
Agent Prompt
## Issue description
Selecting a phrase restores its saved answer into `typed`, which triggers automatic checking as though the learner had edited the field. A complete saved answer can therefore record another attempt and, if correct, advance away from the phrase.
## Fix Focus Areas
- Sources/HanReaderUI/Models/PracticeModel.swift[221-239]
- Sources/HanReaderUI/Models/PracticeModel+Flow.swift[45-56]
- Sources/HanReaderUI/Screens/PracticeScreen.swift[137-139]
## Recommended Fix
Distinguish restored answers from user-entered changes: record the restored value in `select()` (for example, as `restoredTyped`) and return early from `answerChanged()` while `typed == restoredTyped`. Add a navigation test that selects a phrase with a saved complete, correct answer and asserts that selection neither records another attempt nor advances the model.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+49
to
+51
| guard phase == .ready, !isRevealed, isAnswerComplete else { return } | ||
| let phrase = current | ||
| await check() |
There was a problem hiding this comment.
7. Fast typing can record one answer twice 🐞 Bug ☼ Reliability
Each change to typed starts a separate task, but answerChanged() uses !isRevealed as its only guard and check() does not set isRevealed until its dictionary lookup returns. If another edit arrives after the answer is complete but before that lookup finishes, both checks can record the same phrase, schedule an advance, and race to save the answer.
Agent Prompt
## Issue description
Rapid edits to a complete answer can start overlapping auto-checks before either reveals the result, recording the same phrase twice and racing to save the answer.
## Fix Focus Areas
- Sources/HanReaderUI/Models/PracticeModel+Flow.swift[45-56]
- Sources/HanReaderUI/Models/PracticeModel.swift[267-304]
## Recommended Fix
Add an `isChecking` flag to the model. Set it synchronously before `await check()`, include `!isChecking` in the auto-check guard, clear the flag with `defer`, and reset it in `select`. Test two rapid complete-answer changes while the dictionary lookup is suspended to verify that only one check records the phrase.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+23
to
+31
| let resuming = segment == range && !isPlaying | ||
| cancelDrill() | ||
| if let drill { | ||
| self.drill = drill | ||
| } | ||
| segment = range | ||
| repeatsPlayed = 0 | ||
| playSegmentOnce() | ||
| if resuming { | ||
| resumeSegment() | ||
| } else { |
There was a problem hiding this comment.
8. Resuming a phrase restarts the drill's repeat count 🐞 Bug ≡ Correctness
play(segment:drill:) calls cancelDrill(), which sets repeatsPlayed = 0, before the new resuming branch calls resumeSegment(). Unlike the restart branch, the resume branch was meant to keep the count. A learner who pauses part-way through repeat 3 of 3 and resumes is given a full set of repeats again, so the drill no longer ends where it should.
Agent Prompt
## Issue description
Resuming a paused phrase resets the drill's repeat counter, because `cancelDrill()` zeroes it before `resumeSegment()` runs.
## Fix Focus Areas
- Sources/HanReaderPlayback/PlaybackModel+Phrase.swift[21-35]
## Recommended Fix
Capture `repeatsPlayed` before `cancelDrill()` and restore it in the resuming branch, or cancel only `drillTask` when resuming. Add a test that pauses during the second repeat, resumes, and asserts the total number of passes.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| engine.setStopTime(nil) | ||
| } | ||
| engine.seek(to: target) | ||
| engine.seek(to: target, exactly: segment != nil) |
There was a problem hiding this comment.
9. Scrubbing inside a phrase seeks exactly 🐞 Bug ➹ Performance
seek(to:) passes exactly: segment != nil, so every scrub or skip that stays inside the current phrase makes a frame-accurate seek. The PR description and the AudioEngine comment both say scrubbing keeps the tolerance. Dragging the scrubber within a phrase therefore pays the expensive exact seek on every move, which the old code avoided on purpose.
Agent Prompt
## Issue description
Scrubs and skips inside the active phrase seek exactly, which contradicts the stated design and is expensive while dragging.
## Fix Focus Areas
- Sources/HanReaderPlayback/PlaybackModel.swift[325-325]
## Recommended Fix
Pass `exactly: false` here and keep exact seeks only for the start of phrase playback in `play(segment:from:)`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| // MARK: - One phrase at a time | ||
|
|
||
| /// Plays a single phrase, stopping exactly at its end. | ||
| /// Plays a single phrase, stopping exactly at its end. |
There was a problem hiding this comment.
10. A doc comment line appears twice 🐞 Bug ⚙ Maintainability
play(segment:drill:) now begins its doc comment with "Plays a single phrase, stopping exactly at its end." on two consecutive lines. The original line was kept when the new doc block was added, so the rendered documentation repeats its summary.
Agent Prompt
## Issue description
The doc comment summary line is duplicated.
## Fix Focus Areas
- Sources/HanReaderPlayback/PlaybackModel+Phrase.swift[14-15]
## Recommended Fix
Remove one of the two identical lines.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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.
Answers your UX feedback on 听写. Your overarching point — "too many buttons I
need to click to work with this UX instead just typing to see my mistakes" —
drove the shape of it.
zh-Hansinput source while it has focusOn #6, which you asked me to double-check
I measured the stored boundaries against actual audio energy (10 ms frames,
speech = 6% of peak) on 听力64 and its narration:
So the data is not systematically early there. The playback was:
That comment ("a tenth of a second is inaudible") is true for scrubbing a
narration and false when the target is the first syllable — and AVPlayer
resolves a tolerance by taking whichever sync sample is cheapest, in practice an
earlier one. Phrase playback seeks exactly now; scrubbing keeps the tolerance,
which is what it was written for.
I also suspected the editor drew the waveform against a different clock than the
handles. That was wrong —
WaveformReaderand the transport both readduration from the same
AVURLAsset. Discarded rather than reported as a finding.Judgement calls worth disagreeing with
the whole feedback the exercise gives. Say if you'd rather it always moved on.
IME's punctuation doesn't fire the check a character early.
allowedInputSourceLocalesishonoured when a Simplified Chinese source is installed and ignored otherwise.
An app that could force one could also strand somebody.
Verification, and what is not verified
13 new flow tests; every one of 10 planted mutants dies. Mutation testing
also found two redundant guards, which were removed rather than papered over.
Full suite green, lint clean, both app targets built.
Not verified on screen. Synthetic input stopped reaching the app partway
through this work — the cursor moves but clicks do not land, including on
controls unrelated to this change — so I could not drive the practice screen the
way I did for the earlier PRs. The behaviour is covered at the model layer, but
the feel of it, the input-source switch, and the pinyin layout are unconfirmed
by eye. Worth a look before merging.
🤖 Generated with Claude Code