Skip to content

refactor(view): ハイライトと矢印キーの純粋部分を持ち帰る(#31 の上に積んでいます) - #32

Merged
isamu merged 3 commits into
mainfrom
backport/view-helpers
Sep 24, 2026
Merged

isamu merged 3 commits into
mainfrom
backport/view-helpers

Conversation

@isamu

@isamu isamu commented Sep 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Spreadsheet の第2段です(棚卸し: receptron/mulmoclaude#3287)。#31 の上に積んでいますので、base は backport/engine-from-mulmoclaude です。#31 が入れば、この PR の base は自動で main に切り替わります。

View.vue を丸ごと持ち帰るのは勧めません。MulmoClaude 側の View は全文が i18n で、こちらに i18n の仕組みがないため、訳を剥がす作業になってしまいます。代わりに、あちらで切り出された純粋な部分だけを戻します。

  • src/vue/keyboardNav.ts: getArrowKeyOffset と isWithinSheetBounds。
  • src/vue/cellHighlights.ts: clearCellHighlights / applyCellHighlights / highlightCell。DOM の最小の面だけを型で受けるので、テストに jsdom が要りません。
  • View.vue がこの2つを呼びます。ハイライトの watch の中身と、矢印キーの switch + 範囲チェックが消えます。
  • テストは tests/vue/ に入り、test:engine の glob を tests/**/test_*.ts に広げました(走らないテストはテストではないので)。

Items to Confirm / Review

  • 挙動は変えていません。そしてそれを測って確かめています。 View.vue にあった switch と範囲チェックをそのまま写した対照を使い捨ての試験台に置き、切り出した関数と並べて走らせました。生成した入力は、矢印キー4種+それ以外の5種、座標 -3 / -1 / 0 / 1 / 2 / 7、シート8種(null / undefined / {} / 空 / 空行 / 通常 / 行長の違う配列 / 行が欠けた疎配列)で、矢印キー 324 通り・境界 288 通りのすべてで同一でした。試験台は消しました。残るのは、その生成条件を持ち込んだテストが直接押さえていることです(0 での止まり、非矢印キー、空文字、負の行・列、長さ超え、シート欠落、疎な行、長さ0の行)。
  • ハイライトの watch は早期 return を1つ増やしています。 元は if (miniEditorOpen.value && tableContainer.value) で囲んでいた部分を、clearCellHighlights を先に呼んでから if (!miniEditorOpen.value) return; にしました。消す処理は元も先に無条件で走っていたので、順序は同じです。
  • target という名前が衝突したので nextCell にしました。 ハンドラの先頭に const target = event.target as HTMLElement があるためです。
  • i18n は持ち込んでいません。 上の理由どおり、View.vue の残りの差分(訳と、それに伴う構造)は対象外です。

確認したこと

typecheck / lint(エラー0)/ build と、test / test:engine(980件)/ test:fixtures / test:calculator / test:evaluator / test:functions が通ります。

User Prompt

🤖 Generated with Claude Code

isamu and others added 2 commits September 24, 2026 19:47
…aude

Both were lifted out of this View in MulmoClaude and given tests; the logic
here was still inline. The pure parts move out, with their tests.

- src/vue/keyboardNav.ts: getArrowKeyOffset and isWithinSheetBounds.
- src/vue/cellHighlights.ts: clearCellHighlights, applyCellHighlights,
  highlightCell, over a minimal DOM surface so a test needs no jsdom.
- View.vue calls them; the watch body and the arrow-key switch go away.

Behaviour-preserving, and shown to be: the old inline switch and bounds check
were copied verbatim into a throwaway harness and run beside the helpers over
generated keys, coordinates and sheet shapes (including negative indices, a
sparse row and a zero-length row). Identical on every case. The harness is
deleted; what survives it is the generated ground it covered, which the ported
tests already assert directly.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The view helpers arrived in tests/vue, which the engine-only glob did not
reach — a test that is not run is not a test.

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

isamu commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

CODEX VERDICT: LGTM

  • No behavior-preservation regressions found in the extracted keyboard/bounds helpers.
  • No highlight-pass, DOM typing, test coverage, stale local, placement, or export-map findings found.
  • Verification run: yarn test, yarn run test:engine, yarn typecheck, yarn lint, yarn build, and yarn run test:all all exited 0. lint reported warnings only, not errors.
Axis Result
Behaviour-preservation claim Severity: N. Sites: src/vue/View.vue, src/vue/keyboardNav.ts. I copied the old switch/bounds logic from bf753c6b699c1a055037aa07302f8f6ff413b54d^2:src/vue/View.vue and compared it with getArrowKeyOffset / isWithinSheetBounds. Inputs: keys ArrowUp, ArrowDown, ArrowLeft, ArrowRight, Enter, ""; arrow rows [-2,-1,0,1,5,99,100,101,102]; arrow cols [-2,-1,0,1,5,99] for 324 arrow cases. Bounds inputs: sheets undefined, null, {}, {data: []}, rectangular 2x3, ragged [[1],[1,2,3],[]], sparse [[1,2], , [3,4]], zero-length row [[]]; rows/cols [-2,-1,0,1,2,3] for 288 bounds cases. All results were identical. Smallest change: none. What would change my mind: any mismatching generated case or an old inline branch not represented in the harness.
Highlight pass Severity: N. Sites: src/vue/View.vue, src/vue/cellHighlights.ts. The old code always cleared one .cell-editing via querySelector and all .cell-referenced via querySelectorAll before checking miniEditorOpen; the new watcher calls clearCellHighlights unconditionally, then returns if closed, then applies inside #spreadsheet-table. I do not see anything reachable in old code that the new code skips, or the reverse. Smallest change: none. What would change my mind: a DOM case where the old clear/apply ordering differed, or where #spreadsheet-table lookup moved before clearing.
DOM surface typing Severity: N. Sites: src/vue/cellHighlights.ts, src/vue/View.vue. The minimal interfaces are compatible with the live tableContainer.value call; vue-tsc --noEmit and the production declaration build both pass. I did not find a hidden real type error. Smallest change: none. What would change my mind: a TypeScript failure from the live HTMLElement call site or a runtime DOM method used by helpers but omitted from the interface.
Tests Severity: N. Sites: tests/vue/test_cellHighlights.ts, tests/vue/test_keyboardNav.ts, package.json. The tests are actually run: yarn run test:engine uses tsx --test "tests/**/test_*.ts" and reported the new highlightCell / clearCellHighlights / applyCellHighlights / getArrowKeyOffset / isWithinSheetBounds suites; test:all also reaches them. Mutations checked in a temp copy: changing ArrowRight from col + 1 to col + 2 makes test_keyboardNav fail; changing highlightCell to add wrong-class makes test_cellHighlights fail. Smallest change: none. What would change my mind: CI not invoking test:all/test:engine, or a representative behavior break that these suites miss.
Renamed local Severity: N. Sites: src/vue/View.vue. The old handler's const target = event.target remains only for the event target; the moved destination cell is now nextCell, and newRow/newCol references are gone. No shadowing remains. Smallest change: none. What would change my mind: another target/newRow/newCol reference in the handler or template introduced by a later merge.
Extraction leftovers Severity: N. Sites: src/vue/View.vue. I found no dead state, unused imports, or stale comments describing the moved switch/highlight body. The remaining comments still describe the watcher and keyboard handler at the right level. Smallest change: none. What would change my mind: lint/typecheck surfacing an unused symbol or a moved-code comment still claiming inline implementation details.
Consistency Severity: N. Sites: src/vue/cellHighlights.ts, src/vue/keyboardNav.ts, package.json, src/vue/index.ts. Keeping the helpers beside View.vue matches their private Vue-component role. They are not public package API, so the package export map does not need new entries. Smallest change: none. What would change my mind: external consumers needing these helpers as supported imports, or reuse outside the Vue package boundary.

FINDINGS COMPLETE: I read every hunk of the diff and this is every finding I have.

@isamu

isamu commented Sep 24, 2026

Copy link
Copy Markdown
Contributor Author

Codex cross-review — round 1 (tier C): clean

Tier C for one reason: this PR claims behaviour preservation, and that is proved by running old and new side by side, never by reading.

Codex: CODEX VERDICT: LGTM, no findings, every axis answered, FINDINGS COMPLETE present. It did not take my word for the differential — it rebuilt it with its own inputs:

  • arrow keys ArrowUp/Down/Left/Right, Enter, "" over rows [-2,-1,0,1,5,99,100,101,102] and cols [-2,-1,0,1,5,99] — 324 cases, all identical to the old inline switch taken from bf753c6^2.
  • bounds over sheets undefined, null, {}, {data: []}, rectangular, ragged, sparse and zero-length-row — 288 cases, all identical.
  • mutations in a temp copy: ArrowRight → col + 2 turns test_keyboardNav red; highlightCell adding a wrong class turns test_cellHighlights red. So the ported tests bite.
  • confirmed the tests actually RUN (test:engine now globs tests/**/test_*.ts, and CI reaches it through test:all), that the minimal DOM interfaces hide no real type error (vue-tsc and the declaration build both pass), and that the clear-then-apply ordering matches the old body.

My own evaluation of the same head has no MUST-FIX, and nothing has been pushed since it was reviewed.

Prompt sent (verbatim)
Review PR #32 at https://github.com/receptron/GUIChatPluginSpreadsheet/pull/32 (branch backport/view-helpers, base main, head 0d78d771ac9602b69e62c18cb639dfcec80b28d4, 0 behind main). #31 has merged, so this PR's diff is now just the view-helper change plus a merge commit.

Tier C, for one reason: **this PR claims behaviour preservation.** It replaces the inline highlight pass and the inline arrow-key handling in src/vue/View.vue with two pure modules taken from the other repository (src/vue/cellHighlights.ts, src/vue/keyboardNav.ts). A refactor that claims "same behaviour" is proved by running old and new side by side, never by reading.

── THIS IS THE ONLY ROUND YOU ARE GUARANTEED ────────────────────────────────
A finding held back costs a full extra round. Read the WHOLE diff before posting.

Post inline comments via
  gh api repos/receptron/GUIChatPluginSpreadsheet/pulls/32/comments
or top-level via
  gh pr comment 32

You can run `yarn test`, `yarn run test:engine`, `yarn typecheck`, `yarn lint`, `yarn build`. The OLD inline code is at `git show bf753c6b699c1a055037aa07302f8f6ff413b54d^2:src/vue/View.vue` (the pre-merge tip of #31) — the watch body around line 751 and the handler around line 800.

── AXES — answer EVERY line, even when the answer is 'no findings' ──────────
  the behaviour-preservation claim: extract the OLD switch and bounds check verbatim and run them beside getArrowKeyOffset / isWithinSheetBounds over generated keys, coordinates (including negatives and out-of-range) and sheet shapes (missing, empty, ragged, sparse row, zero-length row). I did this and got identical results on 324 arrow-key cases and 288 bounds cases; reproduce it or refute it, and say which inputs you used.
  the highlight pass: clearCellHighlights + applyCellHighlights replace a body that cleared `.cell-editing` via querySelector (single) and `.cell-referenced` via querySelectorAll, then highlighted inside `#spreadsheet-table`. Check the ORDER and the early return: the new code clears unconditionally and returns when the mini editor is closed. Is anything reachable in the old code that the new code skips, or the reverse?
  the DOM surface the helpers type against: they declare a minimal interface rather than using HTMLElement so a test needs no jsdom. Does that hide a real type error, and does the live call from View.vue still pass a compatible object?
  tests: tests/vue/test_cellHighlights.ts and test_keyboardNav.ts came with the modules. Do they go red when the behaviour breaks — name a mutation for each, and check they are actually RUN (the `test:engine` glob became tests/**/test_*.ts, and CI reaches it via test:all).
  the renamed local: the handler already had `const target = event.target`; the new code calls its cell `nextCell`. Confirm nothing else referenced the old name and that the shadowing is gone.
  anything the extraction left behind in View.vue: dead state, an unused import, a comment describing code that moved.
  consistency: the helpers live in src/vue/ beside the component that uses them; is that the right home for this package's layout, and does the package's export map need anything.

An axis with no findings is a RESULT and must be stated.

── EACH FINDING CARRIES FOUR THINGS ─────────────────────────────────────────
1. SEVERITY — P1 / P2 / P3 / N. 2. EVERY SITE. 3. THE SMALLEST CHANGE. 4. WHAT WOULD CHANGE YOUR MIND.

── ALREADY SETTLED ─────────────────────────────────────────────────────────
| # | iter | finding | disposition | why |
|---|---|---|---|---|
| 0 | 0 | sandbox pre-flight | OK | test / test:engine / test:fixtures / typecheck exit 0; socket bind 0; api.github.com 200; deletions permitted |
| 1 | pre | the engine adoption (#31) | MERGED | this PR was stacked on it; its diff is now the view helpers only |

── END WITH ONE TOP-LEVEL COMMENT, IN THIS ORDER ────────────────────────────
  Line 1: 'CODEX VERDICT: LGTM' or 'CODEX VERDICT: CHANGES REQUESTED' + bulleted summary.
  Then the axis table. Then: 'FINDINGS COMPLETE: I read every hunk of the diff and this is every finding I have.'
Do not apply any fixes yourself.
Codex stdout (verbatim)
Posted the PR review comment: https://github.com/receptron/GUIChatPluginSpreadsheet/pull/32#issuecomment-5822952087

Verdict: `CODEX VERDICT: LGTM`. No findings. All requested axes were covered, including the 324 arrow-key / 288 bounds side-by-side harness and temp-copy mutation checks for both helper test files.
codex exit=0

@isamu
isamu merged commit 7cb6f92 into main Sep 24, 2026
11 checks passed
@isamu
isamu deleted the backport/view-helpers branch September 24, 2026 22:17
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