Skip to content

fix: filter structurally-breaking commands and show syntax hints - #11655

Open
Ryosei-Fukushima wants to merge 4 commits into
feat/185620-editor-slash-command-8.0.xfrom
fix/185622-188174-context-aware-slash-menu
Open

fix: filter structurally-breaking commands and show syntax hints#11655
Ryosei-Fukushima wants to merge 4 commits into
feat/185620-editor-slash-command-8.0.xfrom
fix/185622-188174-context-aware-slash-menu

Conversation

@Ryosei-Fukushima

@Ryosei-Fukushima Ryosei-Fukushima commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

https://redmine.weseek.co.jp/issues/188305

スクリーンショット 2026-08-06 165016

Summary

editor-slash-command の試用フィードバックへの対応。(#11458)

FB ごとの対応

1. ドロップダウンにアイコンを入れたい

emojiと同じくドロップダウンの項目の左にそれぞれに対応するアイコンを入れられるとよい(CCはできると言っているがコスト次第では無視・別タスク化してよい)

→ 未対応。 FB自身が許容している通り見送り、別タスク化。

2. 説明欄が説明しすぎ

説明欄が説明しすぎな感じ?全部 notion に寄せてもいい気がする

→ 対応。 単一行マーカーで完結するコマンド(見出し/リスト系/引用)は説明文をやめ、実際の Markdown 記法(#, -, 1., > 等)をヒントとして表示。記法自体が説明になるので翻訳も不要。codeBlock/table は単一マーカーで表せないため対象外(説明欄は空のまま、将来書く想定)。

3. リスト/テーブル内で Heading・テーブルは選べなくていい

リスト中やテーブル内での挙動: - hoge - foo - / の3行目で / を入れたとき、リストであれば「太字」「リンク」はやりたいが、Heading やテーブル入力は選べなくていいのではないか(メニューから消えていい)

→ 対応。 リスト項目の行では見出し・テーブル・コードブロックを候補から除外。テーブルセル内では全コマンドを除外(改行・ブロック要素を一切含められないため)。「太字」「リンク」は新規コマンドが必要なため未対応(下記6)。

4. リスト中での変換(bullet ⇔ ordered ⇔ task)

リスト中での変換: 今の行の bullet list を ordered list / task list / quote に変換したり、その逆も便利そう([convert] ordered list を選ぶと - baz1. baz になるなど)

→ 対応(形は変更)。 [convert] ... という新規コマンドは追加せず(9コマンド契約を壊すため)、既存のリスト系コマンドがリスト文脈では既存マーカーをその場で置換する動作に変更。引用は改行せず同一行に付加(- /- > )。

5. リンク挿入・テーブルビルダーが欲しい

将来的には: リンク挿入・画像アップロード(既存 Edit Link Modal 起動)、Edit Table Modal の起動も欲しい

→ spec のみ対応(実装は別PR)。 editor-slash-extended-elements(drawio 等を扱う別スペック)に、既存 Edit Link Modal / Handsontable Modal を起動するコマンドとして追加。画像アップロードは既存の添付ボタンが React props 経由の起動が前提のため、今回の仕組みでは対応できず見送り。

6. 太字・リンクの新規コマンド、インラインコード

→ 未対応。 新規コマンド追加は9コマンド契約(toHaveLength(9))を壊すため本PRでは実装しない。空マーカー挿入という仕様は確定済み(tasks.md Implementation Notes 参照、別PRで実装予定)。

Test plan

  • pnpm vitest — 全テスト green(新規テスト含む、ミューテーションチェック済み)
  • tsc --noEmit — エラーなし
  • biome check — クリーン
  • 実アプリでのスモーク確認(リスト/テーブル内での絞り込み、リスト変換、ヒント表示)

@mergify

mergify Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

  • Queue this pull request

@Ryosei-Fukushima Ryosei-Fukushima changed the title Fix/185622 188174 context aware slash menu fix(editor-slash-command): filter structurally-breaking commands and show syntax hints, per usability feedback Aug 6, 2026
@Ryosei-Fukushima Ryosei-Fukushima changed the title fix(editor-slash-command): filter structurally-breaking commands and show syntax hints, per usability feedback fix: filter structurally-breaking commands and show syntax hints, per usability feedback Aug 6, 2026
@Ryosei-Fukushima Ryosei-Fukushima changed the title fix: filter structurally-breaking commands and show syntax hints, per usability feedback fix: filter structurally-breaking commands and show syntax hints Aug 6, 2026
@miya

miya commented Aug 7, 2026

Copy link
Copy Markdown
Member

コードレビュー: PR #11655

タイトル: fix: filter structurally-breaking commands and show syntax hints
ブランチ: fix/185622-188174-context-aware-slash-menu
ベース: feat/185620-editor-slash-command-8.0.x
対象 diff: 18 ファイル、+877 / -102(3 コミット: Req 8 文脈フィルタ / Req 9 リスト内変換 / syntaxHint + 拡張スペック追記)
検証: 全 79 テスト green、tsc / biome ともにエラーなし。文脈判定は実データで挙動確認、Req 9 のテストは mutation 検証を実施。

matchesAncestorNode への一般化で isInCodeContext との重複を解消した設計は良い。以下、重要度順に 8 件。


指摘事項(重要度順)

1. [正当性] パイプなしテーブルのセル内で文脈判定が効かない

場所: slash-command-source.ts:83

TABLE_ROW_LINE_REGEX が行頭 | を必須にしているため、GFM で許容されるパイプなしテーブルa | b / --- | --- / c | /)のセル内が table 文脈と判定されず、9 コマンド全部が候補に残る。

実測: 行頭パイプありは候補 []、なしは全 9 件。ここで「テーブル」や「見出し」を選ぶと表が壊れる(Req 8.2 違反)。


2. [正当性] リスト項目の継続行が list 文脈にならない

場所: slash-command-source.ts:80

LIST_ITEM_LINE_REGEX がカーソル行にマーカーを要求するため、リスト項目の継続行- foo\n /、構文木上は ListItem 配下)が list 文脈と判定されず、heading / table / codeBlock が残る。実測で全 9 件。

ここでコードブロックを選ぶと開始フェンスがインデント位置・終了フェンスが桁 0 となり未終端フェンスになる。Req 8.1 が防ごうとしている破壊そのもの


3. [テスト] Req 9 のテストが本番経路を通っていない

場所: insertion-builders.spec.ts:143

applyAtTrailingSlash ヘルパーが apply の dispatch を再実装しており、本番の applyCommandreplaceFromOffset 処理を一切通っていない。

実測 mutation: slash-command-source.ts:196const replaceFrom = from; に変異させても 79 テスト全部 green のまま。Req 9 機能を丸ごと殺す変異が検出できない

essential-test-design の「本番経路で観測可能な契約を検証する」に反する。ヘルパーではなく applyCommand を呼ぶ形に寄せるべき。


4. [spec 一致] syntaxHint が要件・task・design のいずれにも無い

場所: requirements.md:119

syntaxHint(FB #2)は要件も task も design 記載もなく実装のみ追加されている。

さらに全ロケールの description を空にしたことで codeBlock / table は detail が undefined になり、Req 1.3(各コマンドに説明テキストを表示)が満たせていない。Req 8 / 9 は同 PR できちんと spec に追記されているだけに、ここだけ抜けている。


5. [spec 一致] design.md の型定義ブロックが stale

場所: design.md:240

型定義ブロックに SlashInsertion.replaceFromOffset / SlashCommand.disallowedIn / syntaxHint のいずれも未反映(散文側の 319・357 行にしか記載がない)。

子スペック editor-slash-extended-elements の Allowed Dependencies も SlashInsertion({ insert, cursorOffset }) のまま。


6. [重複] リストマーカー正規表現の二重定義

場所: insertion-builders.ts:37

BARE_LIST_MARKER_REGEXslash-command-source.ts:80LIST_ITEM_LINE_REGEX が同じ文法を二重定義している。

design は「両者の認識がずれないように」と明記しているが強制する仕組みがなく、既に [ \t]+ vs \s 等で微妙に差がある。片方だけ更新すると、フィルタは通すのにビルダーが変換しない- /- \n- )という無音のデグレになる。


7. [一貫性] 本文のあるリスト行で quote がリストを抜ける

場所: insertion-builders.ts:133

- foo /- foo \n> となり、codeBlock を除外する根拠にした「項目の内側で働くコマンドだけ残す」ルールと矛盾する。Req 9.5 で黙認されてはいるが、除外マトリクスとして一貫していない。


8. [dead data] 45 件の空 description が残存

場所: translation.json:1097

5 ロケール × 9 コマンド = 45 件の空 description が残り、descriptionKeyt()detail の経路が本番データに対して死んでいる。

現状は i18next の returnEmptyString: true(既定)で無害だが、これを false にすると popup にキー文字列がそのまま出る。


問題なしと確認した点

セキュリティ

新規の入力経路・DOM 挿入なし。detail は CodeMirror が textContent として描画し、syntaxHint は静的定数。両新規正規表現とも (?:>\s*)* の反復が必ず 1 文字消費するため ReDoS 懸念なし(かつ対象は 1 行のみ)。

パフォーマンス

matchesAncestorNode の走査が 1 クエリあたり最大 3 回に増えるが、syntaxTree(state) はキャッシュ済み・resolveInner は O(深さ)。.filter().filter() の 2 パスも 9 要素なので無視できる。

dead code

明白な dead code はなし。ListItemBehaviorlineMarkerInsertion の引数型として利用されており、barrel 非公開も妥当。上記 8 番の空 description が唯一の「死んだデータ」。

重複の解消

matchesAncestorNode への一般化で isInCodeContext との重複を除いたのは良い設計。


補足(diff 外)

tasks.md の Implementation Note 6(絵文字レンダラで null を返す修正)と Note 7(/ 直前の空白吸収)は、いずれも現行コードに反映されていない(emojiAutocompletionSettings.ts:36 は空 span を返す、detectSlashTrigger は空白を吸収しない)。今回の変更の責任範囲ではないが、ノートが実態と食い違っている。

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.

2 participants