fix(sync-220): address Copilot review findings (code + zh-TW + spelling) by yeelam-gordon · Pull Request #226 · microsoft/intelligent-terminal

yeelam-gordon · 2026-06-04T09:47:16Z

Follow-up to #220 addressing the 8 Copilot review findings. Targeting upstream-sync/2026-06-04 so it lands together with PR #220 (or can be rebased onto main if #220 merges first).

Code fixes (commit `29a9e38`)

ControlCore::AdjustFontSize — accumulator drift past clamp floor. Clamp requestedSize first, then derive _accumulatedFontSizeDelta = requestedSize - baseSize so repeated zoom-out at the floor no longer accumulates phantom negative delta.
SixelParser exception path — _maybeFlushImageBuffer(true) (which restores cursor visibility) was only called on normal end-of-sequence. Now also runs in the catch (...) block.
SixelParser typo — \� a scroll\ → \� scroll\.

Localization fix (commit `e8d9359`)

zh-TW RestartConnectionText / RestartConnectionKey — 工作模式 (work mode) → 工作階段 (session), matching the en-US session source.

Spelling cleanup (commit `8dbeab6`)

Move NODEFAULT / SND (Win32 API constants) to allow/apis.txt.
Move slnf (Microsoft solution-filter ext) to allow/microsoft.txt.
Remove all three from expect/expect.txt per repo policy (allow = stable identifiers, expect = transient).

Build note

PR #220 itself built clean (5-min incremental). For this follow-up, bz no_clean from a fresh worktree on the same toolchain hit a pre-existing vcpkg-vs-VS18-preview detection issue (Unable to find a valid Visual Studio instance) that is unrelated to these changes — it reproduces on a clean checkout of the PR #220 head as well. The changes here are surgical (clamp logic refactor with same symbols, one extra call to an in-scope function inside an existing catch, text-only resw + spelling-list edits) and should not introduce build regressions.

Copilot threads addressed on #220

ControlCore.cpp:1187 ✅
SixelParser.cpp:101 ✅
SixelParser.cpp:248 ✅
TerminalApp/zh-TW/Resources.resw:919 ✅
TerminalSettingsModel/zh-TW/Resources.resw:706 ✅
expect.txt:1080 NODEFAULT ✅
expect.txt:1558 slnf ✅
expect.txt:1566 SND ✅

- ControlCore: AdjustFontSize now clamps requestedSize first, then derives _accumulatedFontSizeDelta from the clamp so the accumulator never drifts past the 1.0f floor across repeated decrement operations. - SixelParser: ensure _maybeFlushImageBuffer(true) (which restores cursor visibility) runs on the exception path as well, not just on normal end-of-sequence. - SixelParser: typo 'a a scroll' -> 'a scroll'. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Both RestartConnectionText and RestartConnectionKey in zh-TW used '工作模式' (work mode) where '工作階段' (session) is correct, matching the meaning of the en-US 'session' source string. Flagged by Copilot on PR 220. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Per repo policy (.github/actions/spelling/allow/README.md): stable real identifiers belong in allow/*.txt, not expect/*.txt. NODEFAULT and SND are Win32 API identifiers (allow/apis.txt); slnf is the Microsoft solution-filter file extension (allow/microsoft.txt). All three were added to expect.txt by the upstream cherry-picks on PR 220. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

This follow-up PR addresses prior Copilot review findings from the upstream sync work by tightening correctness in sixel parsing error handling, fixing a zh-TW terminology mismatch for “session”, and aligning the repo’s spell-check allow/expect lists with the established policy.

Changes:

Fix ControlCore::AdjustFontSize accumulator drift by clamping the requested size before updating _accumulatedFontSizeDelta.
Ensure SixelParser performs end-of-sequence cleanup (cursor visibility restore) even when parsing throws, and fix a minor comment typo.
Update zh-TW resource strings to use 工作階段 (“session”) and move stable identifiers (NODEFAULT, SND, slnf) from expect to the appropriate allow lists.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/terminal/adapter/SixelParser.cpp	Adds exception-path cleanup for sixel parsing and fixes a small comment typo.
src/cascadia/TerminalControl/ControlCore.cpp	Clamps requested font size before recording the accumulated delta to prevent drift at the minimum size.
src/cascadia/TerminalApp/Resources/zh-TW/Resources.resw	Updates “Restart connection” text to use 工作階段 (“session”).
src/cascadia/TerminalSettingsModel/Resources/zh-TW/Resources.resw	Updates the corresponding key string to use 工作階段 (“session”).
.github/actions/spelling/expect/expect.txt	Removes stable identifiers that should not live in `expect`.
.github/actions/spelling/allow/microsoft.txt	Allows `slnf` as a stable Microsoft-related identifier.
.github/actions/spelling/allow/apis.txt	Allows `NODEFAULT` and `SND` as stable API identifiers.

Codifies the fix-in-PR vs. follow-up-PR policy learned on PR #220 / #226: build-blocking failures get one extra commit on the sync branch, but all substantive review feedback (Copilot or human) goes into a separate follow-up PR based on the sync branch's HEAD. Preserves the cherry-pick PR's 'faithful to upstream' audit property. - SKILL.md: new 'After-PR review handling' subsection + Gotcha bullet. - references/follow-up-pr.md: new doc with full rubric, worktree mechanics, reply+resolve flow, and rebase-onto-main fallback. - references/workflow.md: new step 10 cross-linking the policy. - scripts/06-finalize-pr.ps1: PR banner now spells the policy out so the first reviewer doesn't push back on deferred fixes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

yeelam-gordon and others added 3 commits June 4, 2026 17:46

yeelam-gordon requested a review from Copilot June 4, 2026 23:12

Copilot started reviewing on behalf of yeelam-gordon June 4, 2026 23:12 View session

Copilot AI reviewed Jun 4, 2026

View reviewed changes

Comment thread src/terminal/adapter/SixelParser.cpp

yeelam-gordon requested a review from Copilot June 4, 2026 23:43

Copilot started reviewing on behalf of yeelam-gordon June 4, 2026 23:43 View session

Copilot AI reviewed Jun 4, 2026

View reviewed changes

vanzue approved these changes Jun 4, 2026

View reviewed changes

DDKinger approved these changes Jun 5, 2026

View reviewed changes

yeelam-gordon merged commit de1179b into upstream-sync/2026-06-04 Jun 5, 2026
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sync-220): address Copilot review findings (code + zh-TW + spelling)#226

fix(sync-220): address Copilot review findings (code + zh-TW + spelling)#226
yeelam-gordon merged 3 commits into
upstream-sync/2026-06-04from
dev/yeelam/sync-220-review-fixes

yeelam-gordon commented Jun 4, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

yeelam-gordon commented Jun 4, 2026

Code fixes (commit 29a9e38)

Localization fix (commit e8d9359)

Spelling cleanup (commit 8dbeab6)

Build note

Copilot threads addressed on #220

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Code fixes (commit `29a9e38`)

Localization fix (commit `e8d9359`)

Spelling cleanup (commit `8dbeab6`)