Skip to content

refactor: fold the detail and diff views' flags into a mode and a phase - #60

Merged
kukv merged 16 commits into
mainfrom
worktree-glistening-floating-perlis
Sep 7, 2026
Merged

refactor: fold the detail and diff views' flags into a mode and a phase#60
kukv merged 16 commits into
mainfrom
worktree-glistening-floating-perlis

Conversation

@kukv

@kukv kukv commented Sep 7, 2026

Copy link
Copy Markdown
Owner

Phase 2 立て直しの Part 2(設計書 §6 の作業順 9 / 6 / 7 / 8)。

何をしたか

# 内容
9 internal/tui/app/scenario_test.go に、キー入力だけで画面をまたぐシナリオテスト 3 本
golden を 54 本追録(detail 30 / diff 24)。以降の等価性検証のため
6 detail の bool 10 個 + エラー文字列 4 本、diff の bool 5 個 + エラー文字列 3 本を mode / phase の enum + errText 1 本に畳んだ
7 detail / diff / appUpdate の各 case を名前つき method に分割(141→41 / 123→35 / 82→45 行)
8 非テストコードから実装計画・設計書への参照 27 箇所を削除または事実に書き換え
.claude/rules の暫定注記(TRANSIENT)を削除、設計書 §6 に実際の作業順を追記

リファクタリングは表示を変えていない。 追録した 54 本を含む golden 80 本が、作業順 6 と 7 を通して 1 バイトも変わっていない。

設計書からの逸脱(実装計画の Decisions に記載)

  • D1 / D2: 作業順を 9 → golden 追録 → 6 → 7 → 8 に並べ替えた。9 のシナリオテストは 6 が書き換える遷移をそのまま通るので、現行コードの上で先に書けば回帰ネットになる。golden も同じ理由で先に録った(「golden 不変で等価性を確認する」は録れているセルにしか効かない)
  • D3: diffloading は畳まない。c / v / X はレビューコンテキストの到着でゲートされており、差分の取得中でもオーバーレイは開ける
  • D4: 送信中かどうかは review.Modelsending に任せ、phase に写さない(真実の出所を 2 つにしない)
  • D5: picker が持っていた 4 本目のエラー文字列も 1 本に畳んだ

意図的な挙動変更 2 件(どちらもテストで固定)

  1. 詳細画面のホイールが、ピッカー候補の取得中とレビューコンテキストの取得中に本文を動かさなくなった。 それらの状態では本文が描かれておらず、見えていないものをスクロールしていた
  2. 詳細画面の c(コメント入力)が、close 失敗で出ていたエラーも消すようになった。 エラー文字列 1 本化(設計書 §4.4 の要求)の帰結。消さずに入力欄を開くと、close の失敗が入力欄の中に描かれてより悪い

完了条件(設計書 §8)

# 条件 状態
1 実機で全機能を 1 つずつ確認 未実施(下記)
2 make check 通る(794 tests / lint 0 issues)
3 interface 1 宣言あたり 6 メソッド以下 itemSource 6 / candidateSource 2 / reviewOpener 1
4 internal/tui に bool の mode フラグが無い grep 0
5 非テストコードに実装計画・spec への参照が 0 grep 0
6 internal/gh/cli のパーステストが実物の testdata を使う Part 1 で完了
7 キー入力だけのシナリオテストが 3 本以上 3 本(Work 始まり 1 / Repos 始まり 2)
8 rules 10 項目と depguard Part 1 で完了、TRANSIENT は本 PR で削除

条件 1 は残っている。 gh 認証と tty が要るため、マージ前に手元で次を通す:

go run ./cmd/octoscope
go run ./cmd/octoscope --lang ja

差分から見て特に見るべき箇所:

  1. 詳細で v / l / a を押した直後の 1 フレーム(View の分岐順を変えた唯一の箇所)
  2. diff で提出を失敗させ → escc、同じく X(エラーが持ち越されないこと)
  3. 詳細でピッカー取得中にホイール(本文が動かないこと=意図的変更 1)
  4. 詳細で close 失敗のエラーが出た状態から c(エラーが消えること=意図的変更 2)
  5. --lang ja でピッカー / 確認 / 提出ポップアップ / フッターの折返し(全角の桁ずれ)

繰り越した指摘

  • diffsubmitCancelled だけが errText を残し、開く側(c / v / X)が消す流儀になっている。他の退出経路は出るときに消す
  • pickErrorMsgref が無いための微差(前の detail インスタンスからの遅延メッセージが要る作為的な経路。既存由来)
  • mode / phaseString() が無く、テストの失敗メッセージが数値で出る
  • diff/golden_test.gosubmittingModel のコメントが pendingCount() の実挙動と食い違う(既存由来)
  • TestClosingFromTheReposTabShowsTheNewState の前提ガード(#12)が弱い。Repos 一覧も番号を描くため。本体のアサーションは有効

🤖 Generated with Claude Code

kukv and others added 15 commits September 7, 2026 10:37
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…r line

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…rst frame

Task 5 will fold diff's parallel bools into mode/phase enums; these golden
cells give it 24 more byte-for-byte checks (compose/submit/discard/loading
x en/ja x 160/120/80) alongside the existing 8, all reached via Update.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ten parallel bools and three error strings made 1024 nominal states out of
the eight this view has, and Update, handleKey and View each assumed a
different subset. They are now one mode (which overlay is on screen), one
phase (that mode's round trip) and one errText.

The wheel no longer scrolls the body while a mode's own fetch is in flight:
the view draws a spinner then, not the body.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Opening the composer over a failed close clears the body's error rather
than draw it inside the composer, and it does not come back on esc. Say
where that error goes on the field itself, and let the two messages that
open an overlay name the mode they open instead of leaning on the key
handler having set it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five parallel bools and three error strings named far more states than the
four overlays this view has, and Update, handleKey, View and the mouse each
assumed a different subset of them. loading stays a bool of its own: the
diff and the review context arrive separately, and c, v and X are gated on
the context, so an overlay can be open while the files are still on their
way.

One error string costs less here than in the detail view -- modeView draws
no errText at all, because what the diff itself has to say goes to reviewErr
and declined -- so the three keys that open an overlay clearing it is the
whole of what keeps a failed submission out of the next composer. Say that
where the popup is cancelled, and pin it on all three keys.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The three scenarios all started with 2 -- the Repos tab -- so nothing walked
the board the root actually opens on, and the one path a user takes first
(pick a card, press enter) was only ever covered by injecting
work.OpenDetailMsg directly.

The board's own d opens the diff too, so reaching the diff is no evidence
enter worked: without the guard the scenario passes with the board's enter
returning nil. The number is no evidence either -- the card carries it. The
state line is what only the detail view draws.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mouse.go listed m.gutter among the geometry it shares with render.go, but it
never calls it -- the gutter is the drawing's alone, which bd40d96 already
said on the other side.

detail's mode said "the eight this view actually has" without anything to
count. There are eleven: modeView idle or loading, modeCompose and
modeConfirm idle or working, modePick loading, idle or working, modeSubmit
loading or idle. The bools it replaced were ten, so 2^10, not 2^n.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

This comment has been minimized.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown

Code Metrics Report

Coverage Test Execution Time
88.3% 36s

Code coverage of files in pull request scope (91.1%, patch 86.9%)

Files Coverage Patch Coverage
internal/gh/review.go 0.0% -
internal/tui/app/app.go 95.5% 100.0%
internal/tui/app/render.go 98.0% -
internal/tui/detail/detail.go 91.3% 93.2%
internal/tui/detail/picker.go 96.2% 100.0%
internal/tui/detail/render.go 95.4% 100.0%
internal/tui/diff/comment.go 94.5% 100.0%
internal/tui/diff/diff.go 80.1% 66.3%
internal/tui/diff/mouse.go 82.6% 100.0%
internal/tui/diff/render.go 97.0% 92.8%
internal/tui/diff/review.go 97.7% 100.0%
internal/tui/icon/icon.go 97.2% -
internal/tui/repo/render.go 96.0% -
internal/tui/theme/theme.go 63.0% -
internal/tui/work/drawer.go 97.5% -
internal/tui/work/mouse.go 93.1% -
internal/tui/work/render.go 98.4% -

Reported by octocov

@kukv
kukv merged commit e42ec4e into main Sep 7, 2026
3 checks passed
@kukv
kukv deleted the worktree-glistening-floating-perlis branch September 7, 2026 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant