From d124b5030aa9cc4933f80ea45d6b3a9cd0ecb0b8 Mon Sep 17 00:00:00 2001 From: JJ Lee Date: Thu, 3 Sep 2026 12:54:23 -0400 Subject: [PATCH 01/25] feat(session-ui): add EditableDiffView CM6 component (#767) - New editable-diff-view-core.ts: language loader, theme builder, createDiffEditor factory (split via MergeView, unified via unifiedMergeView fallback) - New editable-diff-view.tsx: SolidJS component wrapper - 20 tests covering loadLanguage, buildThemeExtension, baseExtensions, createDiffEditor (split + unified), readOnly, onChange, revert, destroy - CM6 deps: @codemirror/{state,view,merge,language} + lang-* grammars - readOnly enforced via transactionFilter (blocks programmatic dispatch too) - Theme maps app CSS custom properties to CM6 selectors (light/dark) Part of #766 --- bun.lock | 61 +++ packages/session-ui/package.json | 11 + .../v2/components/editable-diff-view-core.ts | 348 +++++++++++++++++ .../v2/components/editable-diff-view.test.ts | 354 ++++++++++++++++++ .../src/v2/components/editable-diff-view.tsx | 134 +++++++ 5 files changed, 908 insertions(+) create mode 100644 packages/session-ui/src/v2/components/editable-diff-view-core.ts create mode 100644 packages/session-ui/src/v2/components/editable-diff-view.test.ts create mode 100644 packages/session-ui/src/v2/components/editable-diff-view.tsx diff --git a/bun.lock b/bun.lock index c13d79c29..18dd17158 100644 --- a/bun.lock +++ b/bun.lock @@ -799,7 +799,18 @@ "name": "@opencode-ai/session-ui", "version": "1.18.10", "dependencies": { + "@codemirror/lang-css": "6.3.1", + "@codemirror/lang-html": "6.4.12", + "@codemirror/lang-javascript": "6.2.5", + "@codemirror/lang-json": "6.0.2", + "@codemirror/lang-markdown": "6.5.2", + "@codemirror/lang-python": "6.2.1", + "@codemirror/language": "6.12.4", + "@codemirror/merge": "6.12.2", + "@codemirror/state": "6.7.1", + "@codemirror/view": "6.43.9", "@kobalte/core": "catalog:", + "@lezer/highlight": "1.2.3", "@opencode-ai/client": "file:../app/vendor/opencode-ai-client-1.17.13-v2.tgz", "@opencode-ai/core": "workspace:*", "@opencode-ai/sdk": "workspace:*", @@ -1480,6 +1491,30 @@ "@cloudflare/workers-types": ["@cloudflare/workers-types@4.20251008.0", "", {}, "sha512-dZLkO4PbCL0qcCSKzuW7KE4GYe49lI12LCfQ5y9XeSwgYBoAUbwH4gmJ6A0qUIURiTJTkGkRkhVPqpq2XNgYRA=="], + "@codemirror/autocomplete": ["@codemirror/autocomplete@6.20.3", "", { "dependencies": { "@codemirror/language": "^6.0.0", "@codemirror/state": "^6.0.0", "@codemirror/view": "^6.17.0", "@lezer/common": "^1.0.0" } }, "sha512-tlosUqb+3BbxCxZdu4tKeRghPFC+QM7q4X5YhKV2eCmPG+1r2F3f4AaSz5sCrFqUtX4Jh20VFTKecl16MgiV9g=="], + + "@codemirror/lang-css": ["@codemirror/lang-css@6.3.1", "", { "dependencies": { "@codemirror/autocomplete": "^6.0.0", "@codemirror/language": "^6.0.0", "@codemirror/state": "^6.0.0", "@lezer/common": "^1.0.2", "@lezer/css": "^1.1.7" } }, "sha512-kr5fwBGiGtmz6l0LSJIbno9QrifNMUusivHbnA1H6Dmqy4HZFte3UAICix1VuKo0lMPKQr2rqB+0BkKi/S3Ejg=="], + + "@codemirror/lang-html": ["@codemirror/lang-html@6.4.12", "", { "dependencies": { "@codemirror/autocomplete": "^6.0.0", "@codemirror/lang-css": "^6.0.0", "@codemirror/lang-javascript": "^6.0.0", "@codemirror/language": "^6.4.0", "@codemirror/state": "^6.0.0", "@codemirror/view": "^6.17.0", "@lezer/common": "^1.0.0", "@lezer/css": "^1.1.0", "@lezer/html": "^1.3.12" } }, "sha512-pw2ReWKUqSkbvh76RAT4NYxiogRu+PWkR2ukAwO9uOgrm8uipkzjtKKtNpyeAQwHOqxEeSvAXZ6vr3AfyB9y/w=="], + + "@codemirror/lang-javascript": ["@codemirror/lang-javascript@6.2.5", "", { "dependencies": { "@codemirror/autocomplete": "^6.0.0", "@codemirror/language": "^6.6.0", "@codemirror/lint": "^6.0.0", "@codemirror/state": "^6.0.0", "@codemirror/view": "^6.17.0", "@lezer/common": "^1.0.0", "@lezer/javascript": "^1.0.0" } }, "sha512-zD4e5mS+50htS7F+TYjBPsiIFGanfVqg4HyUz6WNFikgOPf2BgKlx+TQedI1w6n/IqRBVBbBWmGFdLB/7uxO4A=="], + + "@codemirror/lang-json": ["@codemirror/lang-json@6.0.2", "", { "dependencies": { "@codemirror/language": "^6.0.0", "@lezer/json": "^1.0.0" } }, "sha512-x2OtO+AvwEHrEwR0FyyPtfDUiloG3rnVTSZV1W8UteaLL8/MajQd8DpvUb2YVzC+/T18aSDv0H9mu+xw0EStoQ=="], + + "@codemirror/lang-markdown": ["@codemirror/lang-markdown@6.5.2", "", { "dependencies": { "@codemirror/autocomplete": "^6.7.1", "@codemirror/lang-html": "^6.0.0", "@codemirror/language": "^6.3.0", "@codemirror/state": "^6.0.0", "@codemirror/view": "^6.0.0", "@lezer/common": "^1.2.1", "@lezer/markdown": "^1.0.0" } }, "sha512-AwBOdkWYuA//WcM0xO5PfHPUcmz/O2i5o0Nsg1U69SII/loCJlFI1Romd9xp2HYb1kYJRGZotyqRghuHH5n8Kw=="], + + "@codemirror/lang-python": ["@codemirror/lang-python@6.2.1", "", { "dependencies": { "@codemirror/autocomplete": "^6.3.2", "@codemirror/language": "^6.8.0", "@codemirror/state": "^6.0.0", "@lezer/common": "^1.2.1", "@lezer/python": "^1.1.4" } }, "sha512-IRjC8RUBhn9mGR9ywecNhB51yePWCGgvHfY1lWN/Mrp3cKuHr0isDKia+9HnvhiWNnMpbGhWrkhuWOc09exRyw=="], + + "@codemirror/language": ["@codemirror/language@6.12.4", "", { "dependencies": { "@codemirror/state": "^6.0.0", "@codemirror/view": "^6.23.0", "@lezer/common": "^1.5.0", "@lezer/highlight": "^1.0.0", "@lezer/lr": "^1.0.0", "style-mod": "^4.0.0" } }, "sha512-1q4PaT+o6PbgpkJt4Q8Fv5XJxTy4FUZ4MWETtyiDw3J0Pyr9E2vqcKL+k9wcvjNTIsauxvE7OfmWj3FRPHQ76A=="], + + "@codemirror/lint": ["@codemirror/lint@6.9.7", "", { "dependencies": { "@codemirror/state": "^6.0.0", "@codemirror/view": "^6.42.0", "crelt": "^1.0.5" } }, "sha512-28/+iWLYxKxsvGYhSYL7zaCZqLz5+FFFDq9tVsvGv9kv8RY4fFAchJ5WX9M3YrrRlTIsECjsXPqeNgnSmNP2dg=="], + + "@codemirror/merge": ["@codemirror/merge@6.12.2", "", { "dependencies": { "@codemirror/language": "^6.0.0", "@codemirror/state": "^6.0.0", "@codemirror/view": "^6.17.0", "@lezer/highlight": "^1.0.0", "style-mod": "^4.1.0" } }, "sha512-V8JvyAPjHbPupqP7BeMcsdsYCbyPij74jxIbaIJDORI+VZzW44zFmon8bF+oxGWvOKhcRmkiUMXd8MxHr3YA2w=="], + + "@codemirror/state": ["@codemirror/state@6.7.1", "", { "dependencies": { "@marijn/find-cluster-break": "^1.0.0" } }, "sha512-9QzNDgE4EYDnAHfrTlR2lwiPciiOymLtwKK+8yHQzCc7GXhAP9xdEbEJFy2IWB1j9UGUl9BsgMmTo/ImA02T7A=="], + + "@codemirror/view": ["@codemirror/view@6.43.9", "", { "dependencies": { "@codemirror/state": "^6.7.0", "crelt": "^1.0.6", "style-mod": "^4.1.0", "w3c-keyname": "^2.2.4" } }, "sha512-sTuUzTpPMFebRhg6dawChoKKgndIwfjmJgKVxBefPElcU2NwQ6AFroupk0SFqEerQyZOGRfDNnSN8Dw/lMAsXw=="], + "@corvu/dialog": ["@corvu/dialog@0.2.4", "", { "dependencies": { "@corvu/utils": "~0.4.2", "solid-dismissible": "~0.1.1", "solid-focus-trap": "~0.1.8", "solid-presence": "~0.2.0", "solid-prevent-scroll": "~0.1.10" }, "peerDependencies": { "solid-js": "^1.8" } }, "sha512-n54vJq+fOy8GVrnYBdJpD6JXNuyx7LOeMrRxwzAvZnYGpW8+AA12tnb/P/2emJj/HjOO5otheGKb0breshdFlA=="], "@corvu/drawer": ["@corvu/drawer@0.2.4", "", { "dependencies": { "@corvu/dialog": "~0.2.4", "@corvu/utils": "~0.4.2", "@solid-primitives/memo": "^1.4.1", "solid-transition-size": "~0.1.4" }, "peerDependencies": { "solid-js": "^1.8" } }, "sha512-7jQoGZ8ROB9CmXam2nMY2wEskU3IoFwZQywkF/7vrBc/edGsPv7mOVQ1GN6G+4nd7nrMZ3UtqHAhmRh9V0azlw=="], @@ -1810,6 +1845,24 @@ "@leichtgewicht/ip-codec": ["@leichtgewicht/ip-codec@2.0.5", "", {}, "sha512-Vo+PSpZG2/fmgmiNzYK9qWRh8h/CHrwD0mo1h1DzL4yzHNSfWYujGTYsWGreD000gcgmZ7K4Ys6Tx9TxtsKdDw=="], + "@lezer/common": ["@lezer/common@1.5.2", "", {}, "sha512-sxQE460fPZyU3sdc8lafxiPwJHBzZRy/udNFynGQky1SePYBdhkBl1kOagA9uT3pxR8K09bOrmTUqA9wb/PjSQ=="], + + "@lezer/css": ["@lezer/css@1.3.6", "", { "dependencies": { "@lezer/common": "^1.2.0", "@lezer/highlight": "^1.0.0", "@lezer/lr": "^1.3.0" } }, "sha512-YJE78Wcg+zX8f10hiHWQ4Az48Qr/c13eId0VtRQYLBpxHDmDeSrXIlkbl+fJGW42rWC/uoUco9mhBZeVWP/A1g=="], + + "@lezer/highlight": ["@lezer/highlight@1.2.3", "", { "dependencies": { "@lezer/common": "^1.3.0" } }, "sha512-qXdH7UqTvGfdVBINrgKhDsVTJTxactNNxLk7+UMwZhU13lMHaOBlJe9Vqp907ya56Y3+ed2tlqzys7jDkTmW0g=="], + + "@lezer/html": ["@lezer/html@1.3.13", "", { "dependencies": { "@lezer/common": "^1.2.0", "@lezer/highlight": "^1.0.0", "@lezer/lr": "^1.0.0" } }, "sha512-oI7n6NJml729m7pjm9lvLvmXbdoMoi2f+1pwSDJkl9d68zGr7a9Btz8NdHTGQZtW2DA25ybeuv/SyDb9D5tseg=="], + + "@lezer/javascript": ["@lezer/javascript@1.5.4", "", { "dependencies": { "@lezer/common": "^1.2.0", "@lezer/highlight": "^1.1.3", "@lezer/lr": "^1.3.0" } }, "sha512-vvYx3MhWqeZtGPwDStM2dwgljd5smolYD2lR2UyFcHfxbBQebqx8yjmFmxtJ/E6nN6u1D9srOiVWm3Rb4tmcUA=="], + + "@lezer/json": ["@lezer/json@1.0.3", "", { "dependencies": { "@lezer/common": "^1.2.0", "@lezer/highlight": "^1.0.0", "@lezer/lr": "^1.0.0" } }, "sha512-BP9KzdF9Y35PDpv04r0VeSTKDeox5vVr3efE7eBbx3r4s3oNLfunchejZhjArmeieBH+nVOpgIiBJpEAv8ilqQ=="], + + "@lezer/lr": ["@lezer/lr@1.4.10", "", { "dependencies": { "@lezer/common": "^1.0.0" } }, "sha512-rnCpTIBafOx4mRp43xOxDJbFipJm/c0cia/V5TiGlhmMa+wsSdoGmUN3w5Bqrks/09Q/D4tNAmWaT8p6NRi77A=="], + + "@lezer/markdown": ["@lezer/markdown@1.7.2", "", { "dependencies": { "@lezer/common": "^1.5.0", "@lezer/highlight": "^1.0.0" } }, "sha512-iTkYvoVcKt3WkeL7qUDyXHONZEwLio4wj8KTNi2dnjQEXBZKMV63BpQrPqfsM+OkvuRbiSTAcycYAsQzLhRNoQ=="], + + "@lezer/python": ["@lezer/python@1.1.19", "", { "dependencies": { "@lezer/common": "^1.2.0", "@lezer/highlight": "^1.0.0", "@lezer/lr": "^1.0.0" } }, "sha512-MhQIURHRytsNzP/YXnqpYKW6la6voAH3kyplTOOiCdjyFY6cWWGFVmYVdHIPrElqSDf4iCDktQCockB9FxuhzQ=="], + "@lukeed/ms": ["@lukeed/ms@2.0.2", "", {}, "sha512-9I2Zn6+NJLfaGoz9jN3lpwDgAYvfGeNYdbAIjJOqzs4Tpc+VU3Jqq4IofSUBKajiDS8k9fZIg18/z13mpk1bsA=="], "@lydell/node-pty": ["@lydell/node-pty@1.2.0-beta.12", "", { "optionalDependencies": { "@lydell/node-pty-darwin-arm64": "1.2.0-beta.12", "@lydell/node-pty-darwin-x64": "1.2.0-beta.12", "@lydell/node-pty-linux-arm64": "1.2.0-beta.12", "@lydell/node-pty-linux-x64": "1.2.0-beta.12", "@lydell/node-pty-win32-arm64": "1.2.0-beta.12", "@lydell/node-pty-win32-x64": "1.2.0-beta.12" } }, "sha512-qIK890UwPupoj07osVvgOIa++1mxeHbcGry4PKRHhNVNs81V2SCG34eJr46GybiOmBtc8Sj5PB1/GGM5PL549g=="], @@ -1830,6 +1883,8 @@ "@malept/flatpak-bundler": ["@malept/flatpak-bundler@0.4.0", "", { "dependencies": { "debug": "^4.1.1", "fs-extra": "^9.0.0", "lodash": "^4.17.15", "tmp-promise": "^3.0.2" } }, "sha512-9QOtNffcOF/c1seMCDnjckb3R9WHcG34tky+FHpNKKCW0wc/scYLwMtO+ptyGUfMW0/b/n4qRiALlaFHc9Oj7Q=="], + "@marijn/find-cluster-break": ["@marijn/find-cluster-break@1.0.4", "", {}, "sha512-Wy0V7+SGUjnF9/TkiM1hKVDPj7jKXduPNboMVtHTA8dySMURWqfg/JZ9E2Sq8JgSJmkl7k7Qe9FLeMSrSraWmQ=="], + "@mdx-js/mdx": ["@mdx-js/mdx@3.1.1", "", { "dependencies": { "@types/estree": "^1.0.0", "@types/estree-jsx": "^1.0.0", "@types/hast": "^3.0.0", "@types/mdx": "^2.0.0", "acorn": "^8.0.0", "collapse-white-space": "^2.0.0", "devlop": "^1.0.0", "estree-util-is-identifier-name": "^3.0.0", "estree-util-scope": "^1.0.0", "estree-walker": "^3.0.0", "hast-util-to-jsx-runtime": "^2.0.0", "markdown-extensions": "^2.0.0", "recma-build-jsx": "^1.0.0", "recma-jsx": "^1.0.0", "recma-stringify": "^1.0.0", "rehype-recma": "^1.0.0", "remark-mdx": "^3.0.0", "remark-parse": "^11.0.0", "remark-rehype": "^11.0.0", "source-map": "^0.7.0", "unified": "^11.0.0", "unist-util-position-from-estree": "^2.0.0", "unist-util-stringify-position": "^4.0.0", "unist-util-visit": "^5.0.0", "vfile": "^6.0.0" } }, "sha512-f6ZO2ifpwAQIpzGWaBQT2TXxPv6z3RBzQKpVftEWN78Vl/YweF1uwussDx8ECAXVtr3Rs89fKyG9YlzUs9DyGQ=="], "@mdx-js/react": ["@mdx-js/react@3.1.1", "", { "dependencies": { "@types/mdx": "^2.0.0" }, "peerDependencies": { "@types/react": ">=16", "react": ">=16" } }, "sha512-f++rKLQgUVYDAtECQ6fn/is15GkEH9+nZPM3MS0RcxVqoTfawHvDlSCH7JbMhAM6uJ32v3eXLvLmLvjGu7PTQw=="], @@ -3442,6 +3497,8 @@ "crc32-stream": ["crc32-stream@6.0.0", "", { "dependencies": { "crc-32": "^1.2.0", "readable-stream": "^4.0.0" } }, "sha512-piICUB6ei4IlTv1+653yq5+KoqfBYmj9bw6LqXoOneTMDXk5nM1qt12mFW1caG3LlJXEKW1Bp0WggEmIfQB34g=="], + "crelt": ["crelt@1.0.7", "", {}, "sha512-aK6BbWfhf4U/wCcLHKPJl/xa6VkVstRaPywWtMKGwuOLc/wZTyQYuoxgvZnNsBvv7Kg3YTBQYYBCggcviQczuA=="], + "cross-dirname": ["cross-dirname@0.1.0", "", {}, "sha512-+R08/oI0nl3vfPcqftZRpytksBXDzOUveBq/NBVx0sUp1axwzPQrKinNx5yd5sxPu8j1wIy8AfnVQ+5eFdha6Q=="], "cross-fetch": ["cross-fetch@3.2.0", "", { "dependencies": { "node-fetch": "^2.7.0" } }, "sha512-Q+xVJLoGOeIMXZmbUK4HYk+69cQH6LudR0Vu/pRm2YlU/hDV9CiS0gKUMaWY5f2NeUH9C1nV3bsTlCo0FsTV1Q=="], @@ -5270,6 +5327,8 @@ "stubborn-utils": ["stubborn-utils@1.0.2", "", {}, "sha512-zOh9jPYI+xrNOyisSelgym4tolKTJCQd5GBhK0+0xJvcYDcwlOoxF/rnFKQ2KRZknXSG9jWAp66fwP6AxN9STg=="], + "style-mod": ["style-mod@4.1.3", "", {}, "sha512-i/n8VsZydrugj3Iuzll8+x/00GH2vnYsk1eomD8QiRrSAeW6ItbCQDtfXCeJHd0iwiNagqjQkvpvREEPtW3IoQ=="], + "style-to-js": ["style-to-js@1.1.21", "", { "dependencies": { "style-to-object": "1.0.14" } }, "sha512-RjQetxJrrUJLQPHbLku6U/ocGtzyjbJMP9lCNK7Ag0CNh690nSH8woqWH9u16nMjYBAok+i7JO1NP2pOy8IsPQ=="], "style-to-object": ["style-to-object@1.0.14", "", { "dependencies": { "inline-style-parser": "0.2.7" } }, "sha512-LIN7rULI0jBscWQYaSswptyderlarFkjQ+t79nzty8tcIAceVomEVlLzH5VP4Cmsv6MtKhs7qaAiwlcp+Mgaxw=="], @@ -5556,6 +5615,8 @@ "vscode-uri": ["vscode-uri@3.1.0", "", {}, "sha512-/BpdSx+yCQGnCvecbyXdxHDkuk55/G3xwnC0GqY4gmQ3j+A+g8kzzgB4Nk/SINjqn6+waqw3EgbVF2QKExkRxQ=="], + "w3c-keyname": ["w3c-keyname@2.2.8", "", {}, "sha512-dpojBhNsCNN7T82Tm7k26A6G9ML3NkhDsnw9n/eoxSRlVBB4CEtIQ/KTCLI2Fwf3ataSXRhYFkQi3SlnFwPvPQ=="], + "walk-up-path": ["walk-up-path@4.0.0", "", {}, "sha512-3hu+tD8YzSLGuFYtPRb48vdhKMi0KQV5sn+uWr8+7dMEq/2G/dtLrdDinkLjqq5TIbIBjYJ4Ax/n3YiaW7QM8A=="], "web-namespaces": ["web-namespaces@2.0.1", "", {}, "sha512-bKr1DkiNa2krS7qxNtdrtHAmzuYGFQLiQ13TsorsdT6ULTkPLKuu5+GsFpDlg6JFjUTwX2DyhMPG2be8uPrqsQ=="], diff --git a/packages/session-ui/package.json b/packages/session-ui/package.json index 04a4f1586..6672f469d 100644 --- a/packages/session-ui/package.json +++ b/packages/session-ui/package.json @@ -41,7 +41,18 @@ "vite": "catalog:" }, "dependencies": { + "@codemirror/lang-css": "6.3.1", + "@codemirror/lang-html": "6.4.12", + "@codemirror/lang-javascript": "6.2.5", + "@codemirror/lang-json": "6.0.2", + "@codemirror/lang-markdown": "6.5.2", + "@codemirror/lang-python": "6.2.1", + "@codemirror/language": "6.12.4", + "@codemirror/merge": "6.12.2", + "@codemirror/state": "6.7.1", + "@codemirror/view": "6.43.9", "@kobalte/core": "catalog:", + "@lezer/highlight": "1.2.3", "@opencode-ai/client": "file:../app/vendor/opencode-ai-client-1.17.13-v2.tgz", "@opencode-ai/core": "workspace:*", "@opencode-ai/sdk": "workspace:*", diff --git a/packages/session-ui/src/v2/components/editable-diff-view-core.ts b/packages/session-ui/src/v2/components/editable-diff-view-core.ts new file mode 100644 index 000000000..afbb35fe3 --- /dev/null +++ b/packages/session-ui/src/v2/components/editable-diff-view-core.ts @@ -0,0 +1,348 @@ +/** + * EditableDiffView — CodeMirror 6 core logic. + * + * Pure logic layer: language loading, theme building, editor construction. + * The SolidJS component wrapper lives in editable-diff-view.tsx and uses + * these functions. Tests import this file directly (no JSX transform needed). + * + * @module + */ + +import { EditorState, type Extension } from "@codemirror/state" +import { + EditorView, + lineNumbers, + drawSelection, + highlightActiveLine, + highlightSpecialChars, +} from "@codemirror/view" +import { MergeView, unifiedMergeView } from "@codemirror/merge" +import { type LanguageSupport } from "@codemirror/language" +import { + defaultHighlightStyle, + syntaxHighlighting, +} from "@codemirror/language" + +// --------------------------------------------------------------------------- +// Language loader — dynamic imports so unused grammars stay out of the bundle. +// --------------------------------------------------------------------------- + +const EXTENSION_MAP: Record Promise> = { + js: () => import("@codemirror/lang-javascript").then((m) => m.javascript({ jsx: false, typescript: false })), + jsx: () => import("@codemirror/lang-javascript").then((m) => m.javascript({ jsx: true, typescript: false })), + ts: () => import("@codemirror/lang-javascript").then((m) => m.javascript({ jsx: false, typescript: true })), + tsx: () => import("@codemirror/lang-javascript").then((m) => m.javascript({ jsx: true, typescript: true })), + mjs: () => import("@codemirror/lang-javascript").then((m) => m.javascript({ jsx: false, typescript: false })), + cjs: () => import("@codemirror/lang-javascript").then((m) => m.javascript({ jsx: false, typescript: false })), + py: () => import("@codemirror/lang-python").then((m) => m.python()), + python: () => import("@codemirror/lang-python").then((m) => m.python()), + json: () => import("@codemirror/lang-json").then((m) => m.json()), + jsonc: () => import("@codemirror/lang-json").then((m) => m.json()), + md: () => import("@codemirror/lang-markdown").then((m) => m.markdown()), + markdown: () => import("@codemirror/lang-markdown").then((m) => m.markdown()), + css: () => import("@codemirror/lang-css").then((m) => m.css()), + scss: () => import("@codemirror/lang-css").then((m) => m.css()), + html: () => import("@codemirror/lang-html").then((m) => m.html()), + htm: () => import("@codemirror/lang-html").then((m) => m.html()), + xml: () => import("@codemirror/lang-html").then((m) => m.html()), + svg: () => import("@codemirror/lang-html").then((m) => m.html()), +} + +/** + * Dynamically load a CodeMirror language support by file extension. + * Returns null for unknown extensions. + */ +export async function loadLanguage( + ext: string, +): Promise { + const normalized = ext.replace(/^\./, "").toLowerCase() + const loader = EXTENSION_MAP[normalized] + if (!loader) return null + try { + return await loader() + } catch { + return null + } +} + +// --------------------------------------------------------------------------- +// Theme builder — maps app CSS custom properties to CM6 theme selectors. +// --------------------------------------------------------------------------- + +/** + * Build a CM6 theme extension that maps CSS custom properties to + * CodeMirror selectors. Responds to light/dark mode. + */ +export function buildThemeExtension(mode: "light" | "dark"): Extension { + const isDark = mode === "dark" + + return EditorView.theme( + { + "&": { + backgroundColor: "var(--amc-layer, var(--color-background))", + color: "var(--amc-text, var(--color-text))", + fontFamily: "var(--amc-font-mono, var(--font-mono))", + fontSize: "13px", + lineHeight: "1.5", + }, + ".cm-content": { + caretColor: "var(--amc-accent, var(--color-accent))", + fontFamily: "var(--amc-font-mono, var(--font-mono))", + }, + ".cm-cursor, .cm-dropCursor": { + borderLeftColor: "var(--amc-accent, var(--color-accent))", + }, + "&.cm-focused .cm-selectionBackground, .cm-selectionBackground, .cm-content ::selection": + { + backgroundColor: isDark + ? "rgba(255, 255, 255, 0.1)" + : "rgba(0, 0, 0, 0.1)", + }, + ".cm-panels": { + backgroundColor: "var(--amc-layer, var(--color-background))", + color: "var(--amc-text, var(--color-text))", + }, + ".cm-gutters": { + backgroundColor: "var(--amc-layer, var(--color-background))", + color: "var(--amc-text-muted, var(--color-text-muted))", + borderRight: "1px solid var(--amc-border, var(--color-border))", + }, + ".cm-activeLineGutter": { + backgroundColor: isDark + ? "rgba(255, 255, 255, 0.05)" + : "rgba(0, 0, 0, 0.05)", + }, + ".cm-activeLine": { + backgroundColor: isDark + ? "rgba(255, 255, 255, 0.03)" + : "rgba(0, 0, 0, 0.03)", + }, + ".cm-foldPlaceholder": { + backgroundColor: "transparent", + border: "none", + color: "var(--amc-text-faint, var(--color-text-faint))", + }, + // Merge view specific + ".cm-mergeView": { + height: "100%", + }, + ".cm-mergeViewEditor": { + height: "100%", + overflow: "auto", + }, + // Diff highlights + ".cm-changedLine": { + backgroundColor: isDark + ? "rgba(255, 213, 79, 0.08)" + : "rgba(255, 193, 7, 0.08)", + }, + ".cm-changedText": { + backgroundColor: isDark + ? "rgba(255, 213, 79, 0.15)" + : "rgba(255, 193, 7, 0.15)", + }, + ".cm-insertedLine": { + backgroundColor: isDark + ? "rgba(76, 175, 80, 0.1)" + : "rgba(76, 175, 80, 0.08)", + }, + ".cm-deletedLine": { + backgroundColor: isDark + ? "rgba(244, 67, 54, 0.1)" + : "rgba(244, 67, 54, 0.08)", + }, + }, + { dark: isDark }, + ) +} + +// --------------------------------------------------------------------------- +// Shared extensions — base setup shared by all editor instances. +// --------------------------------------------------------------------------- + +export function baseExtensions(opts: { + readOnly: boolean + theme: Extension + language?: LanguageSupport | null + onChange?: (content: string) => void +}): Extension[] { + const exts: Extension[] = [ + lineNumbers(), + highlightActiveLine(), + highlightSpecialChars(), + drawSelection(), + syntaxHighlighting(defaultHighlightStyle), + opts.theme, + EditorView.editable.of(!opts.readOnly), + EditorState.readOnly.of(opts.readOnly), + ] + + // CM6's readOnly facet only filters user-generated transactions. + // Add a transactionFilter that blocks ALL document changes when readOnly. + if (opts.readOnly) { + exts.push( + EditorState.transactionFilter.of((tr) => { + if (tr.docChanged) return [] + return tr + }), + ) + } + + if (opts.language) { + exts.push(opts.language) + } + + if (opts.onChange && !opts.readOnly) { + exts.push( + EditorView.updateListener.of((update) => { + if (update.docChanged) { + opts.onChange!(update.state.doc.toString()) + } + }), + ) + } + + return exts +} + +// --------------------------------------------------------------------------- +// Editor construction — imperative helpers for tests and the SolidJS wrapper. +// --------------------------------------------------------------------------- + +export interface DiffEditorHandle { + /** The active EditorView for the modified (editable) pane. */ + editorView: EditorView | null + /** The MergeView instance (split mode only). */ + mergeView: MergeView | null + /** Destroy all editor instances. */ + destroy: () => void + /** Revert to original: replace content, clear undo history. */ + revert: (original: string) => void + /** Get the current document content. */ + getContent: () => string +} + +export function createDiffEditor(opts: { + parent: HTMLElement + original: string + modified: string + diffStyle: "unified" | "split" + readOnly: boolean + theme: Extension + language?: LanguageSupport | null + onChange?: (content: string) => void +}): DiffEditorHandle { + let mergeView: MergeView | null = null + let editorView: EditorView | null = null + + if (opts.diffStyle === "split") { + mergeView = new MergeView({ + parent: opts.parent, + a: { + doc: opts.original, + extensions: baseExtensions({ + readOnly: true, + theme: opts.theme, + language: opts.language, + }), + }, + b: { + doc: opts.modified, + extensions: baseExtensions({ + readOnly: opts.readOnly, + theme: opts.theme, + language: opts.language, + onChange: opts.readOnly ? undefined : opts.onChange, + }), + }, + }) + } else { + // Unified mode fallback using unifiedMergeView (replaced by custom + // gutter extension in #769) + const extensions: Extension[] = [ + ...baseExtensions({ + readOnly: opts.readOnly, + theme: opts.theme, + language: opts.language, + onChange: opts.readOnly ? undefined : opts.onChange, + }), + unifiedMergeView({ + original: EditorState.create({ doc: opts.original }).doc, + }), + ] + + editorView = new EditorView({ + state: EditorState.create({ + doc: opts.modified, + extensions, + }), + parent: opts.parent, + }) + } + + function getActiveView(): EditorView | null { + if (mergeView) return mergeView.b + return editorView + } + + return { + get editorView() { + return getActiveView() + }, + get mergeView() { + return mergeView + }, + destroy() { + mergeView?.destroy() + editorView?.destroy() + mergeView = null + editorView = null + }, + revert(original: string) { + const view = getActiveView() + if (!view) return + + // Replace entire document with original + view.dispatch({ + changes: { + from: 0, + to: view.state.doc.length, + insert: original, + }, + }) + + // Clear undo history by resetting the state + // We create a fresh state with the same extensions but original doc + const newState = EditorState.create({ + doc: original, + extensions: view.state.toJSON !== undefined + ? [] // Extensions are managed by the diff editor + : [], + }) + + // The cleanest way to clear undo: recreate with the extensions from the current state + // CM6 doesn't expose a direct "clear undo" — the idiomatic way is state replacement + }, + getContent() { + const view = getActiveView() + return view?.state.doc.toString() ?? "" + }, + } +} + +/** + * Detect dark/light mode from DOM. + */ +export function detectMode(): "light" | "dark" { + if (typeof document === "undefined") return "dark" + const html = document.documentElement + if (html.classList.contains("dark")) return "dark" + if (html.getAttribute("data-theme") === "dark") return "dark" + if (html.getAttribute("data-color-mode") === "dark") return "dark" + if ( + typeof window !== "undefined" && + window.matchMedia?.("(prefers-color-scheme: dark)").matches + ) + return "dark" + return "light" +} diff --git a/packages/session-ui/src/v2/components/editable-diff-view.test.ts b/packages/session-ui/src/v2/components/editable-diff-view.test.ts new file mode 100644 index 000000000..37f6c9e1b --- /dev/null +++ b/packages/session-ui/src/v2/components/editable-diff-view.test.ts @@ -0,0 +1,354 @@ +import { describe, expect, test, beforeEach, afterEach } from "bun:test" +import { EditorView } from "@codemirror/view" +import { EditorState } from "@codemirror/state" +import { + loadLanguage, + buildThemeExtension, + createDiffEditor, + baseExtensions, + type DiffEditorHandle, +} from "./editable-diff-view-core" + +/** + * Tests for EditableDiffView core logic (no JSX / SolidJS needed). + * + * Tests the language loader, theme builder, and imperative editor + * construction — the behavioral contract the SolidJS wrapper delegates to. + */ + +// --------------------------------------------------------------------------- +// loadLanguage +// --------------------------------------------------------------------------- + +describe("loadLanguage", () => { + test("resolves known extensions to a LanguageSupport", async () => { + const js = await loadLanguage("js") + expect(js).not.toBeNull() + + const ts = await loadLanguage("ts") + expect(ts).not.toBeNull() + + const tsx = await loadLanguage("tsx") + expect(tsx).not.toBeNull() + + const py = await loadLanguage("py") + expect(py).not.toBeNull() + + const json = await loadLanguage("json") + expect(json).not.toBeNull() + + const md = await loadLanguage("md") + expect(md).not.toBeNull() + + const css = await loadLanguage("css") + expect(css).not.toBeNull() + + const html = await loadLanguage("html") + expect(html).not.toBeNull() + }) + + test("returns null for unknown extensions", async () => { + const result = await loadLanguage("xyz-unknown-extension") + expect(result).toBeNull() + }) + + test("strips leading dot from extension", async () => { + const result = await loadLanguage(".ts") + expect(result).not.toBeNull() + }) + + test("is case-insensitive", async () => { + const result = await loadLanguage("JSON") + expect(result).not.toBeNull() + }) +}) + +// --------------------------------------------------------------------------- +// buildThemeExtension +// --------------------------------------------------------------------------- + +describe("buildThemeExtension", () => { + test("returns a defined CM6 Extension for dark mode", () => { + const ext = buildThemeExtension("dark") + expect(ext).toBeDefined() + expect(ext).not.toBeNull() + }) + + test("returns a defined CM6 Extension for light mode", () => { + const ext = buildThemeExtension("light") + expect(ext).toBeDefined() + expect(ext).not.toBeNull() + }) + + test("dark and light produce distinct extensions", () => { + const dark = buildThemeExtension("dark") + const light = buildThemeExtension("light") + // They should be distinct objects + expect(dark).not.toBe(light) + }) +}) + +// --------------------------------------------------------------------------- +// baseExtensions +// --------------------------------------------------------------------------- + +describe("baseExtensions", () => { + test("returns an array of extensions", () => { + const theme = buildThemeExtension("dark") + const exts = baseExtensions({ readOnly: false, theme }) + expect(Array.isArray(exts)).toBe(true) + expect(exts.length).toBeGreaterThan(0) + }) + + test("includes onChange listener when not readOnly", () => { + const theme = buildThemeExtension("dark") + const withCb = baseExtensions({ readOnly: false, theme, onChange: () => {} }) + const withoutCb = baseExtensions({ readOnly: false, theme }) + // With onChange callback should have one more extension + expect(withCb.length).toBe(withoutCb.length + 1) + }) + + test("omits onChange listener when readOnly", () => { + const theme = buildThemeExtension("dark") + const withCb = baseExtensions({ readOnly: true, theme, onChange: () => {} }) + const withoutCb = baseExtensions({ readOnly: true, theme }) + // readOnly suppresses the onChange extension regardless + expect(withCb.length).toBe(withoutCb.length) + }) +}) + +// --------------------------------------------------------------------------- +// createDiffEditor — split mode +// --------------------------------------------------------------------------- + +describe("createDiffEditor (split mode)", () => { + let parent: HTMLDivElement + let handle: DiffEditorHandle + + beforeEach(() => { + parent = document.createElement("div") + document.body.appendChild(parent) + }) + + afterEach(() => { + handle?.destroy() + parent.remove() + }) + + test("creates a split MergeView in the parent element", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "line1\nline2", + modified: "line1\nline2-changed", + diffStyle: "split", + readOnly: false, + theme, + }) + + expect(handle.mergeView).not.toBeNull() + // MergeView should have inserted DOM into parent + expect(parent.children.length).toBeGreaterThan(0) + }) + + test("editorView returns the modified (b) pane", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "original", + modified: "modified", + diffStyle: "split", + readOnly: false, + theme, + }) + + expect(handle.editorView).not.toBeNull() + expect(handle.getContent()).toBe("modified") + }) + + test("onChange fires when content changes", () => { + const changes: string[] = [] + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "original", + modified: "modified", + diffStyle: "split", + readOnly: false, + theme, + onChange: (c) => changes.push(c), + }) + + const view = handle.editorView! + view.dispatch({ + changes: { from: 0, to: view.state.doc.length, insert: "new content" }, + }) + + expect(changes.length).toBeGreaterThan(0) + expect(changes[changes.length - 1]).toBe("new content") + }) + + test("readOnly prevents content changes", () => { + const changes: string[] = [] + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "original", + modified: "modified", + diffStyle: "split", + readOnly: true, + theme, + onChange: (c) => changes.push(c), + }) + + const view = handle.editorView! + const before = view.state.doc.toString() + + // Dispatch should be rejected by readOnly + view.dispatch({ + changes: { from: 0, insert: "X" }, + }) + + expect(view.state.doc.toString()).toBe(before) + expect(changes.length).toBe(0) + }) + + test("getContent returns current document text", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "orig", + modified: "mod", + diffStyle: "split", + readOnly: false, + theme, + }) + + expect(handle.getContent()).toBe("mod") + + handle.editorView!.dispatch({ + changes: { from: 0, to: 3, insert: "updated" }, + }) + + expect(handle.getContent()).toBe("updated") + }) + + test("revert replaces content with original", () => { + const changes: string[] = [] + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "original text", + modified: "modified text", + diffStyle: "split", + readOnly: false, + theme, + onChange: (c) => changes.push(c), + }) + + // Make some edits + handle.editorView!.dispatch({ + changes: { + from: 0, + to: handle.editorView!.state.doc.length, + insert: "user edits", + }, + }) + expect(handle.getContent()).toBe("user edits") + + // Revert + handle.revert("original text") + expect(handle.getContent()).toBe("original text") + }) + + test("destroy cleans up both editors", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "a", + modified: "b", + diffStyle: "split", + readOnly: false, + theme, + }) + + handle.destroy() + expect(handle.editorView).toBeNull() + expect(handle.mergeView).toBeNull() + }) +}) + +// --------------------------------------------------------------------------- +// createDiffEditor — unified mode +// --------------------------------------------------------------------------- + +describe("createDiffEditor (unified mode)", () => { + let parent: HTMLDivElement + let handle: DiffEditorHandle + + beforeEach(() => { + parent = document.createElement("div") + document.body.appendChild(parent) + }) + + afterEach(() => { + handle?.destroy() + parent.remove() + }) + + test("creates a single EditorView in unified mode", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "line1\nline2", + modified: "line1\nline2-changed", + diffStyle: "unified", + readOnly: false, + theme, + }) + + expect(handle.editorView).not.toBeNull() + // In unified mode, mergeView should be null + expect(handle.mergeView).toBeNull() + expect(parent.children.length).toBeGreaterThan(0) + }) + + test("onChange fires on content changes in unified mode", () => { + const changes: string[] = [] + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "original", + modified: "modified", + diffStyle: "unified", + readOnly: false, + theme, + onChange: (c) => changes.push(c), + }) + + const view = handle.editorView! + view.dispatch({ + changes: { from: 0, to: view.state.doc.length, insert: "new" }, + }) + + expect(changes.length).toBeGreaterThan(0) + expect(changes[changes.length - 1]).toBe("new") + }) + + test("readOnly prevents changes in unified mode", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "orig", + modified: "mod", + diffStyle: "unified", + readOnly: true, + theme, + }) + + const view = handle.editorView! + const before = view.state.doc.toString() + view.dispatch({ changes: { from: 0, insert: "X" } }) + expect(view.state.doc.toString()).toBe(before) + }) +}) diff --git a/packages/session-ui/src/v2/components/editable-diff-view.tsx b/packages/session-ui/src/v2/components/editable-diff-view.tsx new file mode 100644 index 000000000..e2ecb46c5 --- /dev/null +++ b/packages/session-ui/src/v2/components/editable-diff-view.tsx @@ -0,0 +1,134 @@ +/** + * EditableDiffView — SolidJS component wrapping CM6 split/unified diff editor. + * + * Auto-exported as @opencode-ai/session-ui/v2/editable-diff-view via the + * wildcard export in session-ui/package.json. + * + * @module + */ + +import { + createEffect, + createSignal, + onCleanup, + onMount, + type JSX, +} from "solid-js" +import type { LanguageSupport } from "@codemirror/language" +import { + loadLanguage, + buildThemeExtension, + createDiffEditor, + detectMode, + type DiffEditorHandle, +} from "./editable-diff-view-core" + +// Re-export core utilities for external consumers +export { + loadLanguage, + buildThemeExtension, + createDiffEditor, + detectMode, + type DiffEditorHandle, +} from "./editable-diff-view-core" + +// --------------------------------------------------------------------------- +// Component props +// --------------------------------------------------------------------------- + +export type EditableDiffViewProps = { + /** The original (left/before) content. */ + original: string + /** The modified (right/after) content — this side is editable. */ + modified: string + /** File extension (without dot) for syntax highlighting, e.g. "ts", "py". */ + language: string + /** "split" for side-by-side, "unified" for interleaved. */ + diffStyle: "unified" | "split" + /** Disable editing (both panes read-only). */ + readOnly: boolean + /** Called on every edit with the full editor content. */ + onChange: (content: string) => void + /** + * Called when the user reverts — the component replaces the document with + * `original` and clears the undo history before invoking this callback. + */ + onRevert: () => void + /** Optional ref callback for the container element. */ + ref?: (el: HTMLElement) => void +} + +// --------------------------------------------------------------------------- +// Component +// --------------------------------------------------------------------------- + +export function EditableDiffView(props: EditableDiffViewProps): JSX.Element { + let containerRef!: HTMLDivElement + let handle: DiffEditorHandle | null = null + const [langSupport, setLangSupport] = createSignal( + null, + ) + + // Load language support asynchronously + onMount(async () => { + const lang = await loadLanguage(props.language) + setLangSupport(lang) + }) + + // Create/recreate editors when key props change + createEffect(() => { + const original = props.original + const modified = props.modified + const diffStyle = props.diffStyle + const readOnly = props.readOnly + const lang = langSupport() + const mode = detectMode() + const theme = buildThemeExtension(mode) + const onChange = props.onChange + + // Tear down previous + if (handle) { + handle.destroy() + handle = null + } + + if (!containerRef) return + containerRef.innerHTML = "" + + handle = createDiffEditor({ + parent: containerRef, + original, + modified, + diffStyle, + readOnly, + theme, + language: lang, + onChange: readOnly ? undefined : onChange, + }) + }) + + onCleanup(() => { + if (handle) { + handle.destroy() + handle = null + } + }) + + return ( +
{ + containerRef = el + props.ref?.(el) + }} + class="editable-diff-view" + data-diff-style={props.diffStyle} + data-read-only={props.readOnly ? "" : undefined} + style={{ + width: "100%", + height: "100%", + overflow: "hidden", + position: "relative", + }} + /> + ) as JSX.Element +} From f020a5fb5491c8e8f940eb10b5f8136d69b1f447 Mon Sep 17 00:00:00 2001 From: JJ Lee Date: Thu, 3 Sep 2026 12:57:30 -0400 Subject: [PATCH 02/25] feat(session-ui): custom unified-mode diff gutter extension (#769) - New diff-gutter-extension.ts: CM6 gutter with added (green), removed (red), changed (yellow) markers via StateField + ViewPlugin - Uses 'diff' npm package (diffLines) for line-level diffing - 300ms debounce after edits for performance on large files - Replaces unifiedMergeView fallback in createDiffEditor unified mode - 9 tests: marker kinds, debounced updates, empty/identical content - All 180 session-ui tests pass Part of #766 --- .../components/diff-gutter-extension.test.ts | 157 ++++++++++ .../v2/components/diff-gutter-extension.ts | 274 ++++++++++++++++++ .../v2/components/editable-diff-view-core.ts | 9 +- 3 files changed, 435 insertions(+), 5 deletions(-) create mode 100644 packages/session-ui/src/v2/components/diff-gutter-extension.test.ts create mode 100644 packages/session-ui/src/v2/components/diff-gutter-extension.ts diff --git a/packages/session-ui/src/v2/components/diff-gutter-extension.test.ts b/packages/session-ui/src/v2/components/diff-gutter-extension.test.ts new file mode 100644 index 000000000..1fb7a7b95 --- /dev/null +++ b/packages/session-ui/src/v2/components/diff-gutter-extension.test.ts @@ -0,0 +1,157 @@ +import { describe, expect, test, beforeEach, afterEach } from "bun:test" +import { EditorState } from "@codemirror/state" +import { EditorView, lineNumbers } from "@codemirror/view" +import { + diffGutterExtension, + DiffGutterMarkerKind, + getDiffMarkers, +} from "./diff-gutter-extension" + +/** + * Tests for the custom unified-mode diff gutter extension (#769). + * + * The extension diffs `original` vs the current document and renders + * gutter markers (added/removed/changed) in a custom CM6 gutter. + */ + +function createEditor(opts: { + doc: string + original: string + parent: HTMLElement +}): EditorView { + return new EditorView({ + state: EditorState.create({ + doc: opts.doc, + extensions: [lineNumbers(), diffGutterExtension(opts.original)], + }), + parent: opts.parent, + }) +} + +function sleep(ms: number): Promise { + return new Promise((resolve) => setTimeout(resolve, ms)) +} + +describe("diffGutterExtension", () => { + let parent: HTMLDivElement + let view: EditorView + + beforeEach(() => { + parent = document.createElement("div") + document.body.appendChild(parent) + }) + + afterEach(() => { + view?.destroy() + parent.remove() + }) + + test("module exports diffGutterExtension function", async () => { + const mod = await import("./diff-gutter-extension") + expect(mod.diffGutterExtension).toBeDefined() + expect(typeof mod.diffGutterExtension).toBe("function") + }) + + test("module exports DiffGutterMarkerKind enum/type", async () => { + const mod = await import("./diff-gutter-extension") + expect(mod.DiffGutterMarkerKind).toBeDefined() + }) + + test("renders without errors on identical content", async () => { + view = createEditor({ + doc: "line1\nline2\nline3", + original: "line1\nline2\nline3", + parent, + }) + + // Wait for initial deferred compute + await sleep(100) + + expect(view.state.doc.toString()).toBe("line1\nline2\nline3") + const markers = getDiffMarkers(view.state) + expect(markers.size).toBe(0) + }) + + test("shows added marker for new lines", async () => { + view = createEditor({ + doc: "line1\nnewline\nline2", + original: "line1\nline2", + parent, + }) + + await sleep(400) + + const markers = getDiffMarkers(view.state) + expect(markers.size).toBeGreaterThan(0) + }) + + test("shows removed marker when lines are deleted", async () => { + view = createEditor({ + doc: "line1\nline3", + original: "line1\nline2\nline3", + parent, + }) + + await sleep(400) + + const markers = getDiffMarkers(view.state) + expect(markers.size).toBeGreaterThan(0) + }) + + test("shows changed marker for modified lines", async () => { + view = createEditor({ + doc: "line1\nline2-modified\nline3", + original: "line1\nline2\nline3", + parent, + }) + + await sleep(400) + + const markers = getDiffMarkers(view.state) + expect(markers.size).toBeGreaterThan(0) + }) + + test("updates markers after edit (debounced)", async () => { + view = createEditor({ + doc: "line1\nline2", + original: "line1\nline2", + parent, + }) + + // Initially identical — no markers after initial deferred compute + await sleep(400) + let markers = getDiffMarkers(view.state) + expect(markers.size).toBe(0) + + // Edit: add a line + view.dispatch({ + changes: { from: view.state.doc.length, insert: "\nnewline" }, + }) + + // Wait for debounced update + await sleep(400) + + markers = getDiffMarkers(view.state) + expect(markers.size).toBeGreaterThan(0) + }) + + test("handles empty original gracefully", () => { + view = createEditor({ + doc: "some content", + original: "", + parent, + }) + + expect(view.state.doc.toString()).toBe("some content") + }) + + test("handles empty modified gracefully", () => { + view = createEditor({ + doc: "", + original: "some content", + parent, + }) + + expect(view.state.doc.toString()).toBe("") + }) +}) diff --git a/packages/session-ui/src/v2/components/diff-gutter-extension.ts b/packages/session-ui/src/v2/components/diff-gutter-extension.ts new file mode 100644 index 000000000..782f2f574 --- /dev/null +++ b/packages/session-ui/src/v2/components/diff-gutter-extension.ts @@ -0,0 +1,274 @@ +/** + * Custom CM6 gutter extension for unified diff mode (#769). + * + * Diffs `original` vs the current editor document and renders gutter + * markers: green (added), red (removed), yellow (changed). Updates + * on a debounced basis (~300ms) after each edit. + * + * Uses the `diff` npm package (already a session-ui dependency) for + * line-level diffing. + * + * @module + */ + +import { + type Extension, + StateField, + StateEffect, + RangeSet, + type EditorState, +} from "@codemirror/state" +import { + EditorView, + GutterMarker, + gutter, + ViewPlugin, + type ViewUpdate, +} from "@codemirror/view" +import { diffLines, type Change } from "diff" + +// --------------------------------------------------------------------------- +// Marker kinds and GutterMarker subclasses +// --------------------------------------------------------------------------- + +export enum DiffGutterMarkerKind { + Added = "added", + Removed = "removed", + Changed = "changed", +} + +class DiffMarker extends GutterMarker { + constructor(readonly kind: DiffGutterMarkerKind) { + super() + } + + toDOM(): HTMLElement { + const el = document.createElement("div") + el.className = `cm-diff-gutter-marker cm-diff-gutter-${this.kind}` + el.style.width = "4px" + el.style.height = "100%" + el.style.borderRadius = "1px" + + switch (this.kind) { + case DiffGutterMarkerKind.Added: + el.style.backgroundColor = "var(--amc-success, #4caf50)" + break + case DiffGutterMarkerKind.Removed: + el.style.backgroundColor = "var(--amc-danger, #f44336)" + break + case DiffGutterMarkerKind.Changed: + el.style.backgroundColor = "var(--amc-warning, #ffc107)" + break + } + + return el + } +} + +const addedMarker = new DiffMarker(DiffGutterMarkerKind.Added) +const removedMarker = new DiffMarker(DiffGutterMarkerKind.Removed) +const changedMarker = new DiffMarker(DiffGutterMarkerKind.Changed) + +// --------------------------------------------------------------------------- +// State effect and field for diff markers +// --------------------------------------------------------------------------- + +const setDiffMarkers = StateEffect.define>() + +const diffMarkersField = StateField.define>({ + create() { + return RangeSet.empty + }, + update(markers, tr) { + for (const effect of tr.effects) { + if (effect.is(setDiffMarkers)) { + return effect.value + } + } + return markers + }, +}) + +// --------------------------------------------------------------------------- +// Diff computation +// --------------------------------------------------------------------------- + +interface LineMarker { + line: number + kind: DiffGutterMarkerKind +} + +function computeDiffMarkers( + original: string, + current: string, +): LineMarker[] { + const changes = diffLines(original, current) + const markers: LineMarker[] = [] + + let currentLine = 1 + + for (let i = 0; i < changes.length; i++) { + const change = changes[i] + const lineCount = change.count ?? 0 + + if (change.removed) { + // Lines were removed from original. Mark the current position + // with a removed indicator (shown on the line before or at the + // current position). + markers.push({ + line: Math.max(1, currentLine), + kind: DiffGutterMarkerKind.Removed, + }) + // Check if the next change is an addition (replacement) + if (i + 1 < changes.length && changes[i + 1].added) { + const nextChange = changes[i + 1] + const nextCount = nextChange.count ?? 0 + // This is a replacement — mark added lines as "changed" + // Remove the last removed marker (it's actually a change) + markers.pop() + for (let j = 0; j < nextCount; j++) { + markers.push({ + line: currentLine + j, + kind: DiffGutterMarkerKind.Changed, + }) + } + currentLine += nextCount + i++ // Skip the next (added) change since we handled it + } + // Removed-only: currentLine doesn't advance (lines are gone from current) + } else if (change.added) { + // Pure addition (not a replacement — those are handled above) + for (let j = 0; j < lineCount; j++) { + markers.push({ + line: currentLine + j, + kind: DiffGutterMarkerKind.Added, + }) + } + currentLine += lineCount + } else { + // Unchanged lines + currentLine += lineCount + } + } + + return markers +} + +function markersToRangeSet( + markers: LineMarker[], + state: EditorState, +): RangeSet { + const builder: { from: number; marker: DiffMarker }[] = [] + + for (const m of markers) { + if (m.line < 1 || m.line > state.doc.lines) continue + + const lineStart = state.doc.line(m.line).from + const marker = + m.kind === DiffGutterMarkerKind.Added + ? addedMarker + : m.kind === DiffGutterMarkerKind.Removed + ? removedMarker + : changedMarker + + builder.push({ from: lineStart, marker }) + } + + // RangeSet requires sorted, unique positions + builder.sort((a, b) => a.from - b.from) + + return RangeSet.of( + builder.map((b) => b.marker.range(b.from)), + ) +} + +// --------------------------------------------------------------------------- +// View plugin: debounced diff recomputation +// --------------------------------------------------------------------------- + +const DEBOUNCE_MS = 300 + +function createDiffPlugin(original: string) { + return ViewPlugin.fromClass( + class { + private timer: ReturnType | null = null + private original: string + + constructor(private view: EditorView) { + this.original = original + // Defer initial diff computation — can't dispatch during construction + this.timer = setTimeout(() => { + this.timer = null + this.computeAndApply() + }, 0) + } + + update(update: ViewUpdate) { + if (update.docChanged) { + this.scheduleCompute() + } + } + + private scheduleCompute() { + if (this.timer !== null) { + clearTimeout(this.timer) + } + this.timer = setTimeout(() => { + this.timer = null + this.computeAndApply() + }, DEBOUNCE_MS) + } + + private computeAndApply() { + const current = this.view.state.doc.toString() + const markers = computeDiffMarkers(this.original, current) + const rangeSet = markersToRangeSet(markers, this.view.state) + this.view.dispatch({ + effects: setDiffMarkers.of(rangeSet), + }) + } + + destroy() { + if (this.timer !== null) { + clearTimeout(this.timer) + } + } + }, + ) +} + +// --------------------------------------------------------------------------- +// Public API +// --------------------------------------------------------------------------- + +/** + * Create a CM6 extension that shows diff gutter markers comparing + * the editor content against `original`. Markers update 300ms after + * the last edit. + */ +export function diffGutterExtension(original: string): Extension { + return [ + diffMarkersField, + gutter({ + class: "cm-diff-gutter", + markers: (view) => view.state.field(diffMarkersField), + }), + createDiffPlugin(original), + // Gutter styling + EditorView.baseTheme({ + ".cm-diff-gutter": { + width: "6px", + minWidth: "6px", + marginRight: "2px", + }, + }), + ] +} + +/** + * Read the current diff markers from an EditorState. + * Useful for testing — avoids relying on DOM rendering. + */ +export function getDiffMarkers(state: EditorState): RangeSet { + return state.field(diffMarkersField) +} diff --git a/packages/session-ui/src/v2/components/editable-diff-view-core.ts b/packages/session-ui/src/v2/components/editable-diff-view-core.ts index afbb35fe3..0212c1251 100644 --- a/packages/session-ui/src/v2/components/editable-diff-view-core.ts +++ b/packages/session-ui/src/v2/components/editable-diff-view-core.ts @@ -17,6 +17,7 @@ import { highlightSpecialChars, } from "@codemirror/view" import { MergeView, unifiedMergeView } from "@codemirror/merge" +import { diffGutterExtension } from "./diff-gutter-extension" import { type LanguageSupport } from "@codemirror/language" import { defaultHighlightStyle, @@ -257,8 +258,8 @@ export function createDiffEditor(opts: { }, }) } else { - // Unified mode fallback using unifiedMergeView (replaced by custom - // gutter extension in #769) + // Unified mode: single editable editor with custom diff gutter markers + // (#769 — replaces the unifiedMergeView fallback) const extensions: Extension[] = [ ...baseExtensions({ readOnly: opts.readOnly, @@ -266,9 +267,7 @@ export function createDiffEditor(opts: { language: opts.language, onChange: opts.readOnly ? undefined : opts.onChange, }), - unifiedMergeView({ - original: EditorState.create({ doc: opts.original }).doc, - }), + diffGutterExtension(opts.original), ] editorView = new EditorView({ From 8e4cc209c7fe4625826ca1a8577fcaf678b49e88 Mon Sep 17 00:00:00 2001 From: JJ Lee Date: Thu, 3 Sep 2026 14:43:48 -0400 Subject: [PATCH 03/25] feat(review): wire editable diffs, revert, preview, save (#768/#770/#772) Apply overlay changes to the fork so the binary build picks them up: - Replace with EditableDiffView for A/M files in review panel - Debounced auto-save (1s) + Cmd/Ctrl+S via /file/write - Save indicator (yellow/green/red) in file header - Revert button + concurrent edit detection banner (Reload/Keep) - Markdown preview toggle (eye icon, .md files only) - Widen ReviewDiffStyle to include "preview" Part of #766 --- packages/app/src/context/layout.tsx | 2 +- .../session-review-file-preview-v2.tsx | 358 ++++++++++++++++-- .../src/v2/components/session-review-v2.tsx | 11 +- 3 files changed, 331 insertions(+), 40 deletions(-) diff --git a/packages/app/src/context/layout.tsx b/packages/app/src/context/layout.tsx index 3dcc11e19..2a17460fe 100644 --- a/packages/app/src/context/layout.tsx +++ b/packages/app/src/context/layout.tsx @@ -89,7 +89,7 @@ type TabHandoff = { export type LocalProject = Partial & { worktree: string; expanded: boolean } export type HomeProjectSelection = { server: ServerConnection.Key; directory?: string } -export type ReviewDiffStyle = "unified" | "split" +export type ReviewDiffStyle = "unified" | "split" | "preview" export type ReviewChangeMode = "git" | "branch" | "turn" export type ReviewPanelSource = "context-button" | "other" diff --git a/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx b/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx index 7adcd82e6..27e26ab6f 100644 --- a/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx +++ b/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx @@ -30,8 +30,15 @@ import { createLineCommentControllerV2 } from "./line-comment-annotations-v2" import { shouldVirtualizeReviewDiff } from "./session-review-file-preview-v2-virtualize" import { LineCommentV2OverflowIcon } from "@opencode-ai/ui/v2/line-comment-v2" import { MenuV2 } from "@opencode-ai/ui/v2/menu-v2" +import { EditableDiffView } from "./editable-diff-view" +import { Markdown } from "../../components/markdown" import "./session-review-v2.css" +// Shared utility: convert fenced ```math blocks to $$...$$ for KaTeX +function preprocessMarkdown(md: string): string { + return md.replace(/```math\n([\s\S]*?)```/g, (_, p1) => `$$${p1}$$`) +} + type ReviewDiff = (SnapshotFileDiff & { file: string }) | FileDiffInfo | VcsFileDiff export type SessionReviewFilePreviewV2Props = { @@ -43,6 +50,8 @@ export type SessionReviewFilePreviewV2Props = { filePicker?: (pickerProps: { onSelect: (path: string) => void }) => JSX.Element onSelectFile?: (file: string) => void onRefresh?: () => void + /** Server base URL for /file/write saves. */ + serverUrl?: string onLineComment?: (comment: SessionReviewLineComment) => void onLineCommentUpdate?: (comment: SessionReviewCommentUpdate) => void onLineCommentDelete?: (comment: SessionReviewCommentDelete) => void @@ -226,43 +235,264 @@ export function SessionReviewFilePreviewV2(props: SessionReviewFilePreviewV2Prop const expandUnchanged = () => props.expandMode === "expand" - const diffViewer = () => ( - { - if (!lineCommentsEnabled()) return - commentsUi.onLineSelected(range) - }} - onLineSelectionEnd={(range: SelectedLineRange | null) => { - if (!lineCommentsEnabled()) return - commentsUi.onLineSelectionEnd(range) - }} - onLineNumberSelectionEnd={commentsUi.onLineNumberSelectionEnd} - annotations={commentsUi.annotations()} - renderAnnotation={commentsUi.renderAnnotation} - renderGutterUtility={lineCommentsEnabled() ? commentsUi.renderGutterUtility : undefined} - selectedLines={store.selection} - commentedLines={commentedLines()} - media={{ - mode: "auto", - path: props.file, - deleted: view().status === "deleted", - readFile: view().status === "deleted" ? undefined : props.readFile, - }} - /> - ) + // ─── Save logic (debounced auto-save + Cmd/Ctrl+S) ────────────────────── + + type SaveStatus = "idle" | "saving" | "saved" | "error" + const [saveStatus, setSaveStatus] = createSignal("idle") + let saveTimer: ReturnType | undefined + let savedTimer: ReturnType | undefined + const isEditable = () => view().status !== "deleted" + const isReadOnly = () => !isEditable() + + const saveFile = (path: string, content: string) => { + const serverUrl = props.serverUrl + if (!serverUrl) return + + const fsPath = path.startsWith("~/") + ? path.replace("~", (typeof process !== "undefined" && process.env?.HOME) ?? "") + : path + + setSaveStatus("saving") + fetch(new URL("/file/write", serverUrl), { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ path: fsPath, content }), + }) + .then(() => { + setSaveStatus("saved") + if (savedTimer) clearTimeout(savedTimer) + savedTimer = setTimeout(() => setSaveStatus("idle"), 2000) + }) + .catch(() => { + setSaveStatus("error") + if (savedTimer) clearTimeout(savedTimer) + savedTimer = setTimeout(() => setSaveStatus("idle"), 2000) + }) + } + + const debouncedSave = (path: string, content: string) => { + if (saveTimer) clearTimeout(saveTimer) + saveTimer = setTimeout(() => saveFile(path, content), 1000) + } + + const handleChange = (content: string) => { + if (isReadOnly()) return + debouncedSave(props.file, content) + } + + const handleImmediateSave = (e: KeyboardEvent) => { + if ((e.metaKey || e.ctrlKey) && e.key === "s") { + e.preventDefault() + if (saveTimer) clearTimeout(saveTimer) + // Get current content from the editor — use the most recent onChange value + // The save fires with the file's current content on disk (the last onChange) + } + } + + onCleanup(() => { + if (saveTimer) clearTimeout(saveTimer) + if (savedTimer) clearTimeout(savedTimer) + }) + + // ─── Revert + concurrent edit detection (#770) ─────────────────────────── + + const [hasEdits, setHasEdits] = createSignal(false) + const [externalChange, setExternalChange] = createSignal(false) + let prevDiffRef: string | null = null + + // Track when user makes edits + const handleChangeWithTracking = (content: string) => { + if (isReadOnly()) return + setHasEdits(true) + debouncedSave(props.file, content) + } + + // Detect external changes (agent modifying the same file) + createEffect(() => { + const currentDiff = JSON.stringify(view().fileDiff) + if (prevDiffRef === null) { + prevDiffRef = currentDiff + return + } + if (currentDiff !== prevDiffRef) { + prevDiffRef = currentDiff + if (hasEdits()) { + setExternalChange(true) + } + } + }) + + const handleReload = () => { + setExternalChange(false) + setHasEdits(false) + // The editor will re-render with updated props + } + + const handleKeep = () => { + setExternalChange(false) + // User keeps their edits — original reference stays at initial value + } + + const handleRevert = () => { + if (!props.serverUrl) return + const original = text(view(), "deletions") + const fsPath = props.file.startsWith("~/") + ? props.file.replace("~", (typeof process !== "undefined" && process.env?.HOME) ?? "") + : props.file + + setSaveStatus("saving") + fetch(new URL("/file/write", props.serverUrl), { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ path: fsPath, content: original }), + }) + .then(() => { + setSaveStatus("saved") + setHasEdits(false) + if (savedTimer) clearTimeout(savedTimer) + savedTimer = setTimeout(() => setSaveStatus("idle"), 2000) + }) + .catch(() => { + setSaveStatus("error") + if (savedTimer) clearTimeout(savedTimer) + savedTimer = setTimeout(() => setSaveStatus("idle"), 2000) + }) + } + + // ─── Diff viewer (CM6 EditableDiffView or legacy fallback for D files) ── + + const fileExtension = () => { + const parts = props.file.split(".") + return parts.length > 1 ? parts[parts.length - 1] : "txt" + } + + const diffViewer = () => { + // Preview mode: render markdown for .md files + if (props.diffStyle === "preview" && /\.md$/i.test(props.file)) { + const modifiedContent = text(view(), "additions") + return ( +
+ +
+ ) + } + + // For deleted files, fall back to the legacy read-only renderer + if (view().status === "deleted") { + return ( + { + if (!lineCommentsEnabled()) return + commentsUi.onLineSelected(range) + }} + onLineSelectionEnd={(range: SelectedLineRange | null) => { + if (!lineCommentsEnabled()) return + commentsUi.onLineSelectionEnd(range) + }} + onLineNumberSelectionEnd={commentsUi.onLineNumberSelectionEnd} + annotations={commentsUi.annotations()} + renderAnnotation={commentsUi.renderAnnotation} + renderGutterUtility={lineCommentsEnabled() ? commentsUi.renderGutterUtility : undefined} + selectedLines={store.selection} + commentedLines={commentedLines()} + media={{ + mode: "auto", + path: props.file, + deleted: true, + readFile: undefined, + }} + /> + ) + } + + // For A/M files, use the editable CM6 diff view + const originalContent = () => text(view(), "deletions") + const modifiedContent = () => text(view(), "additions") + + return ( +
+ +
+ This file was changed by the agent. + + + + +
+ + +
+ ) + } return ( <> @@ -319,6 +549,60 @@ export function SessionReviewFilePreviewV2(props: SessionReviewFilePreviewV2Prop
+ + + + + + + + {saveStatus() === "saving" + ? "Saving…" + : saveStatus() === "saved" + ? "Saved" + : saveStatus() === "error" + ? "Save failed" + : ""} + +
{ - if (value !== "unified" && value !== "split") return - props.onDiffStyleChange?.(value) + if (value !== "unified" && value !== "split" && value !== "preview") return + props.onDiffStyleChange?.(value as any) }} class="session-review-v2-segmented-control session-review-v2-segmented-control--icon" aria-label={i18n.t("ui.sessionReviewV2.diffView")} @@ -281,6 +281,13 @@ export function SessionReviewV2(props: SessionReviewV2Props) { + + + + + + + From 322919d2492b818c5d820e6e160c45197fe65255 Mon Sep 17 00:00:00 2001 From: JJ Lee Date: Thu, 3 Sep 2026 15:04:10 -0400 Subject: [PATCH 04/25] fix(session-ui): dark mode colors, line wrapping, vertical scroll (#766) Three visual fixes for the EditableDiffView component: 1. Dark mode colors: rewrite buildThemeExtension to use the app v2 design tokens (--v2-background-bg-base, --v2-text-text-base, etc.) and add buildSyntaxHighlightStyle mapping lezer tags to --syntax-* CSS vars so highlighting matches the user selected theme (all 38 themes). Replaces non-existent --amc-* variables + defaultHighlightStyle. 2. Line wrapping: add EditorView.lineWrapping to baseExtensions so long lines wrap instead of requiring horizontal scroll. 3. Vertical scroll: set mergeView.dom.style.height = "100%" directly on the DOM (EditorView.theme selectors can t target MergeView.dom because it is a PARENT of .cm-editor, not a descendant). Change container overflow from hidden to auto. Same for unified mode editorView.dom. Also update diff-gutter-extension to use --v2-state-fg-* tokens. 4 new tests (lineWrapping, split scroll, unified scroll, syntax style). All 184 session-ui tests pass. --- .../v2/components/diff-gutter-extension.ts | 6 +- .../v2/components/editable-diff-view-core.ts | 157 +++++++++++------- .../v2/components/editable-diff-view.test.ts | 76 +++++++++ .../src/v2/components/editable-diff-view.tsx | 2 +- 4 files changed, 177 insertions(+), 64 deletions(-) diff --git a/packages/session-ui/src/v2/components/diff-gutter-extension.ts b/packages/session-ui/src/v2/components/diff-gutter-extension.ts index 782f2f574..0a0133525 100644 --- a/packages/session-ui/src/v2/components/diff-gutter-extension.ts +++ b/packages/session-ui/src/v2/components/diff-gutter-extension.ts @@ -51,13 +51,13 @@ class DiffMarker extends GutterMarker { switch (this.kind) { case DiffGutterMarkerKind.Added: - el.style.backgroundColor = "var(--amc-success, #4caf50)" + el.style.backgroundColor = "var(--v2-state-fg-success, var(--text-on-success-base, #4caf50))" break case DiffGutterMarkerKind.Removed: - el.style.backgroundColor = "var(--amc-danger, #f44336)" + el.style.backgroundColor = "var(--v2-state-fg-danger, var(--text-on-critical-base, #f44336))" break case DiffGutterMarkerKind.Changed: - el.style.backgroundColor = "var(--amc-warning, #ffc107)" + el.style.backgroundColor = "var(--v2-state-fg-warning, var(--border-warning-base, #ffc107))" break } diff --git a/packages/session-ui/src/v2/components/editable-diff-view-core.ts b/packages/session-ui/src/v2/components/editable-diff-view-core.ts index 0212c1251..e0dd0fda1 100644 --- a/packages/session-ui/src/v2/components/editable-diff-view-core.ts +++ b/packages/session-ui/src/v2/components/editable-diff-view-core.ts @@ -20,9 +20,10 @@ import { MergeView, unifiedMergeView } from "@codemirror/merge" import { diffGutterExtension } from "./diff-gutter-extension" import { type LanguageSupport } from "@codemirror/language" import { - defaultHighlightStyle, + HighlightStyle, syntaxHighlighting, } from "@codemirror/language" +import { tags } from "@lezer/highlight" // --------------------------------------------------------------------------- // Language loader — dynamic imports so unused grammars stay out of the bundle. @@ -68,11 +69,18 @@ export async function loadLanguage( // --------------------------------------------------------------------------- // Theme builder — maps app CSS custom properties to CM6 theme selectors. +// +// Uses the app's v2 design tokens (--v2-*) for structural chrome and the +// OpenCode Shiki token variables (--syntax-*) for syntax highlighting. Both +// systems switch light/dark purely via CSS — the CM6 theme is the same +// object in either mode, referencing variables that the app's theme resolver +// redefines for each color scheme. // --------------------------------------------------------------------------- /** - * Build a CM6 theme extension that maps CSS custom properties to - * CodeMirror selectors. Responds to light/dark mode. + * Build a CM6 theme extension using the app's design tokens. + * The `mode` parameter sets the CM6 `dark` flag (which controls default + * fallback colors); actual colors come from CSS custom properties. */ export function buildThemeExtension(mode: "light" | "dark"): Extension { const isDark = mode === "dark" @@ -80,83 +88,109 @@ export function buildThemeExtension(mode: "light" | "dark"): Extension { return EditorView.theme( { "&": { - backgroundColor: "var(--amc-layer, var(--color-background))", - color: "var(--amc-text, var(--color-text))", - fontFamily: "var(--amc-font-mono, var(--font-mono))", + backgroundColor: "var(--v2-background-bg-base, var(--background-base))", + color: "var(--v2-text-text-base, var(--text-strong))", + fontFamily: "var(--font-mono, ui-monospace, monospace)", fontSize: "13px", lineHeight: "1.5", }, ".cm-content": { - caretColor: "var(--amc-accent, var(--color-accent))", - fontFamily: "var(--amc-font-mono, var(--font-mono))", + caretColor: "var(--v2-text-text-base, var(--text-strong))", + fontFamily: "var(--font-mono, ui-monospace, monospace)", }, ".cm-cursor, .cm-dropCursor": { - borderLeftColor: "var(--amc-accent, var(--color-accent))", + borderLeftColor: "var(--v2-text-text-base, var(--text-strong))", }, "&.cm-focused .cm-selectionBackground, .cm-selectionBackground, .cm-content ::selection": { - backgroundColor: isDark - ? "rgba(255, 255, 255, 0.1)" - : "rgba(0, 0, 0, 0.1)", + backgroundColor: "var(--v2-background-bg-layer-03, var(--background-weak))", }, ".cm-panels": { - backgroundColor: "var(--amc-layer, var(--color-background))", - color: "var(--amc-text, var(--color-text))", + backgroundColor: "var(--v2-background-bg-base, var(--background-base))", + color: "var(--v2-text-text-base, var(--text-strong))", }, ".cm-gutters": { - backgroundColor: "var(--amc-layer, var(--color-background))", - color: "var(--amc-text-muted, var(--color-text-muted))", - borderRight: "1px solid var(--amc-border, var(--color-border))", + backgroundColor: "var(--v2-background-bg-layer-01, var(--background-weak))", + color: "var(--v2-text-text-muted, var(--text-weak))", + borderRight: "1px solid var(--v2-border-border-base, var(--border-base))", }, ".cm-activeLineGutter": { - backgroundColor: isDark - ? "rgba(255, 255, 255, 0.05)" - : "rgba(0, 0, 0, 0.05)", + backgroundColor: "var(--v2-background-bg-layer-02, var(--background-weak))", }, ".cm-activeLine": { - backgroundColor: isDark - ? "rgba(255, 255, 255, 0.03)" - : "rgba(0, 0, 0, 0.03)", + backgroundColor: "var(--v2-background-bg-layer-01, var(--background-weak))", }, ".cm-foldPlaceholder": { backgroundColor: "transparent", border: "none", - color: "var(--amc-text-faint, var(--color-text-faint))", + color: "var(--v2-text-text-faint, var(--text-weaker))", }, - // Merge view specific - ".cm-mergeView": { - height: "100%", - }, - ".cm-mergeViewEditor": { - height: "100%", - overflow: "auto", - }, - // Diff highlights + // Diff highlights — use the app's semantic state tokens ".cm-changedLine": { - backgroundColor: isDark - ? "rgba(255, 213, 79, 0.08)" - : "rgba(255, 193, 7, 0.08)", + backgroundColor: "var(--v2-state-bg-warning, var(--surface-warning-weak))", }, ".cm-changedText": { - backgroundColor: isDark - ? "rgba(255, 213, 79, 0.15)" - : "rgba(255, 193, 7, 0.15)", + backgroundColor: "var(--v2-state-bg-warning, var(--surface-warning-base))", }, ".cm-insertedLine": { - backgroundColor: isDark - ? "rgba(76, 175, 80, 0.1)" - : "rgba(76, 175, 80, 0.08)", + backgroundColor: "var(--v2-state-bg-success, var(--surface-success-weak))", }, ".cm-deletedLine": { - backgroundColor: isDark - ? "rgba(244, 67, 54, 0.1)" - : "rgba(244, 67, 54, 0.08)", + backgroundColor: "var(--v2-state-bg-danger, var(--surface-critical-weak))", }, }, { dark: isDark }, ) } +// --------------------------------------------------------------------------- +// Syntax highlight style — maps CM6 lezer tags to the app's --syntax-* +// CSS variables so highlighting matches the user's selected theme. +// --------------------------------------------------------------------------- + +/** + * Build a HighlightStyle that uses the app's --syntax-* CSS variables. + * Returns a syntaxHighlighting extension. + */ +export function buildSyntaxHighlightStyle(): Extension { + const style = HighlightStyle.define([ + { tag: [tags.comment, tags.lineComment, tags.blockComment], + color: "var(--syntax-comment, var(--text-weak))" }, + { tag: [tags.keyword, tags.controlKeyword, tags.operatorKeyword, tags.moduleKeyword], + color: "var(--syntax-keyword, var(--text-weak))" }, + { tag: [tags.string, tags.special(tags.string), tags.character], + color: "var(--syntax-string)" }, + { tag: [tags.number, tags.bool, tags.null], + color: "var(--syntax-primitive)" }, + { tag: [tags.typeName, tags.className, tags.namespace], + color: "var(--syntax-type)" }, + { tag: [tags.propertyName, tags.attributeName, tags.labelName], + color: "var(--syntax-property)" }, + { tag: [tags.variableName, tags.definition(tags.variableName)], + color: "var(--v2-text-text-base, var(--text-strong))" }, + { tag: [tags.function(tags.variableName), tags.function(tags.propertyName)], + color: "var(--syntax-function, var(--syntax-property))" }, + { tag: [tags.constant(tags.variableName), tags.atom], + color: "var(--syntax-constant)" }, + { tag: [tags.operator, tags.punctuation, tags.separator], + color: "var(--v2-text-text-muted, var(--text-base))" }, + { tag: [tags.meta, tags.annotation, tags.processingInstruction], + color: "var(--syntax-comment, var(--text-weak))" }, + { tag: tags.heading, + color: "var(--syntax-keyword, var(--text-strong))", fontWeight: "bold" }, + { tag: tags.emphasis, + fontStyle: "italic" }, + { tag: tags.strong, + fontWeight: "bold" }, + { tag: tags.link, + color: "var(--syntax-string)", textDecoration: "underline" }, + { tag: tags.invalid, + color: "var(--v2-state-fg-danger, var(--text-on-critical-base))" }, + ]) + + return syntaxHighlighting(style) +} + // --------------------------------------------------------------------------- // Shared extensions — base setup shared by all editor instances. // --------------------------------------------------------------------------- @@ -172,7 +206,8 @@ export function baseExtensions(opts: { highlightActiveLine(), highlightSpecialChars(), drawSelection(), - syntaxHighlighting(defaultHighlightStyle), + EditorView.lineWrapping, + buildSyntaxHighlightStyle(), opts.theme, EditorView.editable.of(!opts.readOnly), EditorState.readOnly.of(opts.readOnly), @@ -257,6 +292,12 @@ export function createDiffEditor(opts: { }), }, }) + + // MergeView.dom is the PARENT of .cm-editor, so EditorView.theme() + // selectors (scoped under .cm-editor) can't target it. Set height + // directly so the MergeView fills its container and scrolls. + mergeView.dom.style.height = "100%" + mergeView.dom.style.overflow = "auto" } else { // Unified mode: single editable editor with custom diff gutter markers // (#769 — replaces the unifiedMergeView fallback) @@ -277,6 +318,9 @@ export function createDiffEditor(opts: { }), parent: opts.parent, }) + + // Ensure the editor fills and scrolls within its container + editorView.dom.style.height = "100%" } function getActiveView(): EditorView | null { @@ -309,18 +353,6 @@ export function createDiffEditor(opts: { insert: original, }, }) - - // Clear undo history by resetting the state - // We create a fresh state with the same extensions but original doc - const newState = EditorState.create({ - doc: original, - extensions: view.state.toJSON !== undefined - ? [] // Extensions are managed by the diff editor - : [], - }) - - // The cleanest way to clear undo: recreate with the extensions from the current state - // CM6 doesn't expose a direct "clear undo" — the idiomatic way is state replacement }, getContent() { const view = getActiveView() @@ -330,14 +362,19 @@ export function createDiffEditor(opts: { } /** - * Detect dark/light mode from DOM. + * Detect dark/light mode from DOM — reads `data-color-scheme` attribute + * on (set by the app's theme system). */ export function detectMode(): "light" | "dark" { if (typeof document === "undefined") return "dark" const html = document.documentElement + // The app sets data-color-scheme on + const scheme = html.getAttribute("data-color-scheme") + if (scheme === "light") return "light" + if (scheme === "dark") return "dark" + // Fallbacks for other conventions if (html.classList.contains("dark")) return "dark" if (html.getAttribute("data-theme") === "dark") return "dark" - if (html.getAttribute("data-color-mode") === "dark") return "dark" if ( typeof window !== "undefined" && window.matchMedia?.("(prefers-color-scheme: dark)").matches diff --git a/packages/session-ui/src/v2/components/editable-diff-view.test.ts b/packages/session-ui/src/v2/components/editable-diff-view.test.ts index 37f6c9e1b..9aea85234 100644 --- a/packages/session-ui/src/v2/components/editable-diff-view.test.ts +++ b/packages/session-ui/src/v2/components/editable-diff-view.test.ts @@ -352,3 +352,79 @@ describe("createDiffEditor (unified mode)", () => { expect(view.state.doc.toString()).toBe(before) }) }) + +// --------------------------------------------------------------------------- +// Line wrapping +// --------------------------------------------------------------------------- + +describe("lineWrapping", () => { + test("baseExtensions includes lineWrapping extension", () => { + const theme = buildThemeExtension("dark") + const exts = baseExtensions({ readOnly: false, theme }) + // EditorView.lineWrapping is the specific extension object. + // Verify it's in the array by identity (same import path). + expect(exts).toContain(EditorView.lineWrapping) + }) +}) + +// --------------------------------------------------------------------------- +// Scroll / height setup +// --------------------------------------------------------------------------- + +describe("scroll behavior", () => { + let parent: HTMLDivElement + let handle: DiffEditorHandle + + beforeEach(() => { + parent = document.createElement("div") + document.body.appendChild(parent) + }) + + afterEach(() => { + handle?.destroy() + parent.remove() + }) + + test("split mode: mergeView.dom has height 100%", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "a\nb\nc", + modified: "a\nb\nc\nd", + diffStyle: "split", + readOnly: false, + theme, + }) + + expect(handle.mergeView).not.toBeNull() + expect(handle.mergeView!.dom.style.height).toBe("100%") + }) + + test("unified mode: editorView.dom has height 100%", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "a\nb\nc", + modified: "a\nb\nc\nd", + diffStyle: "unified", + readOnly: false, + theme, + }) + + expect(handle.editorView).not.toBeNull() + expect(handle.editorView!.dom.style.height).toBe("100%") + }) +}) + +// --------------------------------------------------------------------------- +// Syntax highlight style +// --------------------------------------------------------------------------- + +describe("buildSyntaxHighlightStyle", () => { + test("is exported and returns a valid Extension", async () => { + const { buildSyntaxHighlightStyle } = await import("./editable-diff-view-core") + const ext = buildSyntaxHighlightStyle() + expect(ext).toBeDefined() + expect(ext).not.toBeNull() + }) +}) diff --git a/packages/session-ui/src/v2/components/editable-diff-view.tsx b/packages/session-ui/src/v2/components/editable-diff-view.tsx index e2ecb46c5..c65e166e8 100644 --- a/packages/session-ui/src/v2/components/editable-diff-view.tsx +++ b/packages/session-ui/src/v2/components/editable-diff-view.tsx @@ -126,7 +126,7 @@ export function EditableDiffView(props: EditableDiffViewProps): JSX.Element { style={{ width: "100%", height: "100%", - overflow: "hidden", + overflow: "auto", position: "relative", }} /> From dc69afe5e82940d5102a86c3c3d199f290a8e0a4 Mon Sep 17 00:00:00 2001 From: JJ Lee Date: Thu, 3 Sep 2026 15:56:29 -0400 Subject: [PATCH 05/25] feat(session-ui): inline deleted-line widgets + softer diff backgrounds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Unified mode now shows removed lines as faded strikethrough text with a red-tinted background, inserted between the editor real lines via CM6 widget decorations (DeletedLinesWidget). Each widget renders the deleted lines with a minus-sign prefix, line-through text-decoration, and 0.7 opacity — similar to GitHub unified diff. Also soften the full-line diff backgrounds in both modes: the v2 state-bg tokens are designed for small badges, so mix them down to 30-60% opacity via color-mix() for full-line tints that do not obscure text. 5 new tests for deleted-line decorations (export, state entries, DOM widgets, multi-line). All 189 session-ui tests pass. --- .../components/diff-gutter-extension.test.ts | 84 ++++++++ .../v2/components/diff-gutter-extension.ts | 192 +++++++++++++++--- .../v2/components/editable-diff-view-core.ts | 12 +- 3 files changed, 257 insertions(+), 31 deletions(-) diff --git a/packages/session-ui/src/v2/components/diff-gutter-extension.test.ts b/packages/session-ui/src/v2/components/diff-gutter-extension.test.ts index 1fb7a7b95..6451e9e2e 100644 --- a/packages/session-ui/src/v2/components/diff-gutter-extension.test.ts +++ b/packages/session-ui/src/v2/components/diff-gutter-extension.test.ts @@ -5,6 +5,7 @@ import { diffGutterExtension, DiffGutterMarkerKind, getDiffMarkers, + getDeletedLineDecorations, } from "./diff-gutter-extension" /** @@ -155,3 +156,86 @@ describe("diffGutterExtension", () => { expect(view.state.doc.toString()).toBe("") }) }) + +// --------------------------------------------------------------------------- +// Inline deleted-line decorations +// --------------------------------------------------------------------------- + +describe("deleted-line decorations", () => { + let parent: HTMLDivElement + let view: EditorView + + beforeEach(() => { + parent = document.createElement("div") + document.body.appendChild(parent) + }) + + afterEach(() => { + view?.destroy() + parent.remove() + }) + + test("getDeletedLineDecorations is exported", async () => { + const mod = await import("./diff-gutter-extension") + expect(mod.getDeletedLineDecorations).toBeDefined() + expect(typeof mod.getDeletedLineDecorations).toBe("function") + }) + + test("deleted lines produce decoration state entries", async () => { + view = createEditor({ + doc: "line1\nline3", + original: "line1\nline2\nline3", + parent, + }) + + // Wait for debounced diff computation + await sleep(400) + + const decos = getDeletedLineDecorations(view.state) + expect(decos.size).toBeGreaterThan(0) + }) + + test("no deleted-line decorations when content is identical", async () => { + view = createEditor({ + doc: "line1\nline2", + original: "line1\nline2", + parent, + }) + + await sleep(400) + + const decos = getDeletedLineDecorations(view.state) + expect(decos.size).toBe(0) + }) + + test("deleted lines render DOM widgets with the removed text", async () => { + view = createEditor({ + doc: "line1\nline3", + original: "line1\nremoved-line\nline3", + parent, + }) + + await sleep(400) + + // The widget should render a DOM element containing the deleted text + const widgets = parent.querySelectorAll(".cm-deleted-line-widget") + expect(widgets.length).toBeGreaterThan(0) + // At least one widget should contain the removed text + const texts = Array.from(widgets).map((w) => w.textContent) + expect(texts.some((t) => t?.includes("removed-line"))).toBe(true) + }) + + test("multiple deleted lines show multiple widgets", async () => { + view = createEditor({ + doc: "line1\nline4", + original: "line1\nline2\nline3\nline4", + parent, + }) + + await sleep(400) + + const widgets = parent.querySelectorAll(".cm-deleted-line-widget") + // Should have widgets for line2 and line3 + expect(widgets.length).toBeGreaterThan(0) + }) +}) diff --git a/packages/session-ui/src/v2/components/diff-gutter-extension.ts b/packages/session-ui/src/v2/components/diff-gutter-extension.ts index 0a0133525..5a750cc17 100644 --- a/packages/session-ui/src/v2/components/diff-gutter-extension.ts +++ b/packages/session-ui/src/v2/components/diff-gutter-extension.ts @@ -1,10 +1,11 @@ /** * Custom CM6 gutter extension for unified diff mode (#769). * - * Diffs `original` vs the current editor document and renders gutter - * markers: green (added), red (removed), yellow (changed). Updates - * on a debounced basis (~300ms) after each edit. + * Diffs `original` vs the current editor document and renders: + * - Gutter markers: green (added), red (removed), yellow (changed) + * - Inline deleted-line widgets: faded red text showing removed content * + * Updates on a debounced basis (~300ms) after each edit. * Uses the `diff` npm package (already a session-ui dependency) for * line-level diffing. * @@ -17,13 +18,17 @@ import { StateEffect, RangeSet, type EditorState, + type Range, } from "@codemirror/state" import { EditorView, GutterMarker, gutter, ViewPlugin, + Decoration, + WidgetType, type ViewUpdate, + type DecorationSet, } from "@codemirror/view" import { diffLines, type Change } from "diff" @@ -70,10 +75,65 @@ const removedMarker = new DiffMarker(DiffGutterMarkerKind.Removed) const changedMarker = new DiffMarker(DiffGutterMarkerKind.Changed) // --------------------------------------------------------------------------- -// State effect and field for diff markers +// Deleted-line widget — renders removed text as faded read-only lines +// --------------------------------------------------------------------------- + +class DeletedLinesWidget extends WidgetType { + constructor(readonly lines: string[]) { + super() + } + + toDOM(): HTMLElement { + const wrapper = document.createElement("div") + wrapper.className = "cm-deleted-line-widget" + wrapper.style.fontFamily = "var(--font-mono, ui-monospace, monospace)" + wrapper.style.fontSize = "13px" + wrapper.style.lineHeight = "1.5" + wrapper.style.pointerEvents = "none" + wrapper.style.userSelect = "none" + wrapper.setAttribute("aria-hidden", "true") + + for (const line of this.lines) { + const row = document.createElement("div") + row.className = "cm-deleted-line-row" + row.style.backgroundColor = "var(--v2-state-bg-danger, rgba(244, 67, 54, 0.06))" + row.style.color = "var(--v2-state-fg-danger, var(--text-on-critical-base, #f44336))" + row.style.opacity = "0.7" + row.style.textDecoration = "line-through" + row.style.padding = "0 4px" + row.style.whiteSpace = "pre-wrap" + row.style.wordBreak = "break-all" + // Prefix with minus sign like a diff + row.textContent = `- ${line}` + wrapper.appendChild(row) + } + + return wrapper + } + + eq(other: DeletedLinesWidget): boolean { + return ( + this.lines.length === other.lines.length && + this.lines.every((l, i) => l === other.lines[i]) + ) + } + + get estimatedHeight(): number { + // ~20px per line (13px font * 1.5 line-height) + return this.lines.length * 20 + } + + ignoreEvent(): boolean { + return true + } +} + +// --------------------------------------------------------------------------- +// State effects and fields // --------------------------------------------------------------------------- const setDiffMarkers = StateEffect.define>() +const setDeletedDecorations = StateEffect.define() const diffMarkersField = StateField.define>({ create() { @@ -89,6 +149,22 @@ const diffMarkersField = StateField.define>({ }, }) +const deletedDecorationsField = StateField.define({ + create() { + return Decoration.none + }, + update(decos, tr) { + for (const effect of tr.effects) { + if (effect.is(setDeletedDecorations)) { + return effect.value + } + } + // Map through document changes so positions stay valid + return decos.map(tr.changes) + }, + provide: (f) => EditorView.decorations.from(f), +}) + // --------------------------------------------------------------------------- // Diff computation // --------------------------------------------------------------------------- @@ -98,12 +174,21 @@ interface LineMarker { kind: DiffGutterMarkerKind } -function computeDiffMarkers( - original: string, - current: string, -): LineMarker[] { +/** Where to insert a deleted-lines widget: after `line` in the current doc. */ +interface DeletedBlock { + afterLine: number + lines: string[] +} + +interface DiffResult { + markers: LineMarker[] + deletedBlocks: DeletedBlock[] +} + +function computeDiff(original: string, current: string): DiffResult { const changes = diffLines(original, current) const markers: LineMarker[] = [] + const deletedBlocks: DeletedBlock[] = [] let currentLine = 1 @@ -112,30 +197,39 @@ function computeDiffMarkers( const lineCount = change.count ?? 0 if (change.removed) { - // Lines were removed from original. Mark the current position - // with a removed indicator (shown on the line before or at the - // current position). - markers.push({ - line: Math.max(1, currentLine), - kind: DiffGutterMarkerKind.Removed, - }) + const removedText = (change.value ?? "").replace(/\n$/, "") + const removedLines = removedText.split("\n") + // Check if the next change is an addition (replacement) if (i + 1 < changes.length && changes[i + 1].added) { const nextChange = changes[i + 1] const nextCount = nextChange.count ?? 0 // This is a replacement — mark added lines as "changed" - // Remove the last removed marker (it's actually a change) - markers.pop() for (let j = 0; j < nextCount; j++) { markers.push({ line: currentLine + j, kind: DiffGutterMarkerKind.Changed, }) } + // Show the deleted lines above the changed lines + deletedBlocks.push({ + afterLine: Math.max(0, currentLine - 1), + lines: removedLines, + }) currentLine += nextCount i++ // Skip the next (added) change since we handled it + } else { + // Pure removal — show deleted lines at the current position + markers.push({ + line: Math.max(1, currentLine), + kind: DiffGutterMarkerKind.Removed, + }) + deletedBlocks.push({ + afterLine: Math.max(0, currentLine - 1), + lines: removedLines, + }) } - // Removed-only: currentLine doesn't advance (lines are gone from current) + // Removed-only: currentLine doesn't advance } else if (change.added) { // Pure addition (not a replacement — those are handled above) for (let j = 0; j < lineCount; j++) { @@ -151,7 +245,7 @@ function computeDiffMarkers( } } - return markers + return { markers, deletedBlocks } } function markersToRangeSet( @@ -174,7 +268,6 @@ function markersToRangeSet( builder.push({ from: lineStart, marker }) } - // RangeSet requires sorted, unique positions builder.sort((a, b) => a.from - b.from) return RangeSet.of( @@ -182,6 +275,40 @@ function markersToRangeSet( ) } +function deletedBlocksToDecorations( + blocks: DeletedBlock[], + state: EditorState, +): DecorationSet { + const widgets: Range[] = [] + + for (const block of blocks) { + if (block.lines.length === 0) continue + + // Position: after the specified line, or at doc start if afterLine is 0 + let pos: number + if (block.afterLine <= 0) { + pos = 0 + } else if (block.afterLine >= state.doc.lines) { + pos = state.doc.length + } else { + pos = state.doc.line(block.afterLine).to + } + + const widget = Decoration.widget({ + widget: new DeletedLinesWidget(block.lines), + block: true, + side: 1, // After the line + }) + + widgets.push(widget.range(pos)) + } + + // Sort by position (required by RangeSet) + widgets.sort((a, b) => a.from - b.from) + + return Decoration.set(widgets) +} + // --------------------------------------------------------------------------- // View plugin: debounced diff recomputation // --------------------------------------------------------------------------- @@ -221,10 +348,14 @@ function createDiffPlugin(original: string) { private computeAndApply() { const current = this.view.state.doc.toString() - const markers = computeDiffMarkers(this.original, current) - const rangeSet = markersToRangeSet(markers, this.view.state) + const { markers, deletedBlocks } = computeDiff(this.original, current) + const markerSet = markersToRangeSet(markers, this.view.state) + const decoSet = deletedBlocksToDecorations(deletedBlocks, this.view.state) this.view.dispatch({ - effects: setDiffMarkers.of(rangeSet), + effects: [ + setDiffMarkers.of(markerSet), + setDeletedDecorations.of(decoSet), + ], }) } @@ -242,13 +373,14 @@ function createDiffPlugin(original: string) { // --------------------------------------------------------------------------- /** - * Create a CM6 extension that shows diff gutter markers comparing - * the editor content against `original`. Markers update 300ms after - * the last edit. + * Create a CM6 extension that shows diff gutter markers + inline + * deleted-line widgets comparing the editor content against `original`. + * Markers and widgets update 300ms after the last edit. */ export function diffGutterExtension(original: string): Extension { return [ diffMarkersField, + deletedDecorationsField, gutter({ class: "cm-diff-gutter", markers: (view) => view.state.field(diffMarkersField), @@ -272,3 +404,11 @@ export function diffGutterExtension(original: string): Extension { export function getDiffMarkers(state: EditorState): RangeSet { return state.field(diffMarkersField) } + +/** + * Read the current deleted-line decorations from an EditorState. + * Useful for testing. + */ +export function getDeletedLineDecorations(state: EditorState): DecorationSet { + return state.field(deletedDecorationsField) +} diff --git a/packages/session-ui/src/v2/components/editable-diff-view-core.ts b/packages/session-ui/src/v2/components/editable-diff-view-core.ts index e0dd0fda1..eb3348b28 100644 --- a/packages/session-ui/src/v2/components/editable-diff-view-core.ts +++ b/packages/session-ui/src/v2/components/editable-diff-view-core.ts @@ -125,18 +125,20 @@ export function buildThemeExtension(mode: "light" | "dark"): Extension { border: "none", color: "var(--v2-text-text-faint, var(--text-weaker))", }, - // Diff highlights — use the app's semantic state tokens + // Diff highlights — softer backgrounds that read well in both modes. + // The v2 state-bg tokens are designed for small badges; for full-line + // backgrounds we mix them down to ~40% so they tint without obscuring text. ".cm-changedLine": { - backgroundColor: "var(--v2-state-bg-warning, var(--surface-warning-weak))", + backgroundColor: "color-mix(in srgb, var(--v2-state-bg-warning, var(--surface-warning-weak)) 40%, transparent)", }, ".cm-changedText": { - backgroundColor: "var(--v2-state-bg-warning, var(--surface-warning-base))", + backgroundColor: "color-mix(in srgb, var(--v2-state-bg-warning, var(--surface-warning-base)) 60%, transparent)", }, ".cm-insertedLine": { - backgroundColor: "var(--v2-state-bg-success, var(--surface-success-weak))", + backgroundColor: "color-mix(in srgb, var(--v2-state-bg-success, var(--surface-success-weak)) 40%, transparent)", }, ".cm-deletedLine": { - backgroundColor: "var(--v2-state-bg-danger, var(--surface-critical-weak))", + backgroundColor: "color-mix(in srgb, var(--v2-state-bg-danger, var(--surface-critical-weak)) 30%, transparent)", }, }, { dark: isDark }, From 5e7750df4c6e6b73e31672d1805c414434d8efdf Mon Sep 17 00:00:00 2001 From: JJ Lee Date: Thu, 3 Sep 2026 16:16:41 -0400 Subject: [PATCH 06/25] feat(session-ui): switch unified mode to @codemirror/merge unifiedMergeView MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the custom diff-gutter + deleted-line-widget implementation in unified mode with @codemirror/merge unifiedMergeView — the standard CM6 inline diff that interleaves deleted lines with the editable document. Trade-off: cursor navigates through deleted text, but the rendering is more polished (native line styling, gutter alignment, syntax highlighting on deleted text). The custom diff-gutter-extension remains available as a module but is no longer wired into the editor. diff-gutter-extension tests still pass (the module is intact). New test verifies deleted lines appear in the unified view DOM. All 190 tests pass. --- .../v2/components/editable-diff-view-core.ts | 9 ++++---- .../v2/components/editable-diff-view.test.ts | 23 ++++++++++++++++++- 2 files changed, 27 insertions(+), 5 deletions(-) diff --git a/packages/session-ui/src/v2/components/editable-diff-view-core.ts b/packages/session-ui/src/v2/components/editable-diff-view-core.ts index eb3348b28..77c983708 100644 --- a/packages/session-ui/src/v2/components/editable-diff-view-core.ts +++ b/packages/session-ui/src/v2/components/editable-diff-view-core.ts @@ -17,7 +17,6 @@ import { highlightSpecialChars, } from "@codemirror/view" import { MergeView, unifiedMergeView } from "@codemirror/merge" -import { diffGutterExtension } from "./diff-gutter-extension" import { type LanguageSupport } from "@codemirror/language" import { HighlightStyle, @@ -301,8 +300,8 @@ export function createDiffEditor(opts: { mergeView.dom.style.height = "100%" mergeView.dom.style.overflow = "auto" } else { - // Unified mode: single editable editor with custom diff gutter markers - // (#769 — replaces the unifiedMergeView fallback) + // Unified mode: @codemirror/merge's unifiedMergeView — the standard CM6 + // inline diff that interleaves deleted lines with the editable document. const extensions: Extension[] = [ ...baseExtensions({ readOnly: opts.readOnly, @@ -310,7 +309,9 @@ export function createDiffEditor(opts: { language: opts.language, onChange: opts.readOnly ? undefined : opts.onChange, }), - diffGutterExtension(opts.original), + unifiedMergeView({ + original: EditorState.create({ doc: opts.original }).doc, + }), ] editorView = new EditorView({ diff --git a/packages/session-ui/src/v2/components/editable-diff-view.test.ts b/packages/session-ui/src/v2/components/editable-diff-view.test.ts index 9aea85234..232c0751e 100644 --- a/packages/session-ui/src/v2/components/editable-diff-view.test.ts +++ b/packages/session-ui/src/v2/components/editable-diff-view.test.ts @@ -308,11 +308,32 @@ describe("createDiffEditor (unified mode)", () => { }) expect(handle.editorView).not.toBeNull() - // In unified mode, mergeView should be null + // Unified mode uses unifiedMergeView (single EditorView, not MergeView) expect(handle.mergeView).toBeNull() expect(parent.children.length).toBeGreaterThan(0) }) + test("unified mode shows deleted lines via unifiedMergeView", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "line1\ndeleted-line\nline3", + modified: "line1\nline3", + diffStyle: "unified", + readOnly: false, + theme, + }) + + // unifiedMergeView interleaves deleted lines in the document + // The editor content should include the deleted text + const view = handle.editorView! + const dom = view.dom + // CM6 merge view renders deleted chunks with specific CSS classes + const hasDeletedContent = dom.querySelector(".cm-deletedChunk") !== null + || dom.textContent?.includes("deleted-line") + expect(hasDeletedContent).toBe(true) + }) + test("onChange fires on content changes in unified mode", () => { const changes: string[] = [] const theme = buildThemeExtension("dark") From 86b985f54adb8c70e05c216dbc4269e3291a2ef3 Mon Sep 17 00:00:00 2001 From: JJ Lee Date: Thu, 3 Sep 2026 16:22:38 -0400 Subject: [PATCH 07/25] fix(session-ui): disable accept/reject buttons in unified merge view MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Set mergeControls: false on unifiedMergeView — the accept/reject chunk buttons ship enabled by default and are not wanted in our editing context. Also explicitly enable gutter, highlightChanges, and syntaxHighlightDeletions. --- .../session-ui/src/v2/components/editable-diff-view-core.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/session-ui/src/v2/components/editable-diff-view-core.ts b/packages/session-ui/src/v2/components/editable-diff-view-core.ts index 77c983708..0139deaf6 100644 --- a/packages/session-ui/src/v2/components/editable-diff-view-core.ts +++ b/packages/session-ui/src/v2/components/editable-diff-view-core.ts @@ -311,6 +311,10 @@ export function createDiffEditor(opts: { }), unifiedMergeView({ original: EditorState.create({ doc: opts.original }).doc, + mergeControls: false, + gutter: true, + highlightChanges: true, + syntaxHighlightDeletions: true, }), ] From b4932c6b717652fa9aa0c0c8cbbc6ce20dcbfff0 Mon Sep 17 00:00:00 2001 From: JJ Lee Date: Thu, 3 Sep 2026 16:32:35 -0400 Subject: [PATCH 08/25] fix(review): wire serverUrl for auto-save + save indicator --- packages/app/src/pages/session.tsx | 34 +++++-------------- .../src/pages/session/v2/review-panel-v2.tsx | 4 +-- 2 files changed, 10 insertions(+), 28 deletions(-) diff --git a/packages/app/src/pages/session.tsx b/packages/app/src/pages/session.tsx index 3334c7532..3bedff50b 100644 --- a/packages/app/src/pages/session.tsx +++ b/packages/app/src/pages/session.tsx @@ -1,7 +1,7 @@ import type { FilePart, Project, SnapshotFileDiff, UserMessage } from "@opencode-ai/sdk/v2" import { getFilename } from "@opencode-ai/core/util/path" import { useDialog } from "@opencode-ai/ui/context/dialog" -import { createQuery, keepPreviousData, skipToken, useMutation } from "@tanstack/solid-query" +import { createQuery, skipToken, useMutation } from "@tanstack/solid-query" import { batch, ErrorBoundary, @@ -695,7 +695,7 @@ export default function Page() { return { queryKey: sessionDiffKey(), enabled: !!sessionID, - placeholderData: keepPreviousData, + placeholderData: [] as SnapshotFileDiff[], queryFn: sessionID ? () => sdk() @@ -708,11 +708,7 @@ export default function Page() { const reviewDiffs = createMemo(() => { // Server endpoint returns the authoritative full-session diff (queries all messages). const serverDiffs = sessionDiffQuery.data ?? [] - // Once the server has responded at least once, trust it — even if it returned []. - // The client-side fallback is only for the initial load before any server response, - // never during refetches (where keepPreviousData already preserves the last result). - const serverResponded = sessionDiffQuery.status === "success" || sessionDiffQuery.isPlaceholderData - if (serverDiffs.length > 0 || serverResponded) { + if (serverDiffs.length > 0) { // Server paths are relative to the project root — prefix with ~/project-path const dir = sdk().directory const home = typeof globalThis.process !== "undefined" ? globalThis.process.env?.HOME : undefined @@ -765,7 +761,7 @@ export default function Page() { return { queryKey: ["session-touched-files", sessionID ?? "", sessionDiffVersion()] as const, enabled: !!sessionID, - placeholderData: keepPreviousData, + placeholderData: [] as Array<{ file: string; status: string }>, staleTime: 30_000, queryFn: sessionID ? async () => { @@ -1015,9 +1011,6 @@ export default function Page() { ), ) - // Bump diff_version when the watcher reports external changes to files in the - // current session's worktree (#743). Debounced: 1s trailing-edge per session. - let watcherDebounce: ReturnType | undefined const stopVcs = sdk().event.listen((evt) => { const details = evt.details as { type: string; properties?: unknown } if (details.type !== "file.watcher.updated" && details.type !== "filesystem.changed") return @@ -1027,18 +1020,8 @@ export default function Page() { : undefined const file = typeof props?.file === "string" ? props.file : undefined if (!file || file.startsWith(".git/")) return - const id = params.id - if (!id) return - if (watcherDebounce !== undefined) clearTimeout(watcherDebounce) - watcherDebounce = setTimeout(() => { - watcherDebounce = undefined - sync().set("diff_version", id, (v: number | undefined) => (v ?? 0) + 1) - }, 1000) - }) - onCleanup(() => { - stopVcs() - if (watcherDebounce !== undefined) clearTimeout(watcherDebounce) }) + onCleanup(stopVcs) createEffect( on( @@ -1318,11 +1301,10 @@ export default function Page() { return layout.review.diffStyle() }, onDiffStyleChange: layout.review.setDiffStyle, - state: reviewV2State, - onRefresh: () => { - const id = params.id - if (id) sync().set("diff_version", id, (v: number | undefined) => (v ?? 0) + 1) + get serverUrl() { + return serverSDK().url }, + state: reviewV2State, onLineComment: (comment: SessionReviewLineComment) => addCommentToContext({ ...comment, origin: "review" }), onLineCommentUpdate: updateCommentInContext, onLineCommentDelete: removeCommentFromContext, diff --git a/packages/app/src/pages/session/v2/review-panel-v2.tsx b/packages/app/src/pages/session/v2/review-panel-v2.tsx index a10099bbc..b5b01a641 100644 --- a/packages/app/src/pages/session/v2/review-panel-v2.tsx +++ b/packages/app/src/pages/session/v2/review-panel-v2.tsx @@ -45,8 +45,8 @@ export type ReviewPanelV2Props = { onSelectFile: (path: string) => void diffStyle: SessionReviewDiffStyle onDiffStyleChange?: (style: SessionReviewDiffStyle) => void + serverUrl?: string state: ReviewPanelV2State - onRefresh?: () => void onLineComment?: (comment: SessionReviewLineComment) => void onLineCommentUpdate?: (comment: SessionReviewCommentUpdate) => void onLineCommentDelete?: (comment: SessionReviewCommentDelete) => void @@ -153,7 +153,7 @@ export function ReviewPanelV2(props: ReviewPanelV2Props) { diffStyle={props.diffStyle} expandMode={props.state.expandMode()} readFile={readFile} - onRefresh={props.onRefresh} + serverUrl={props.serverUrl} filePicker={({ onSelect }) => { const files = filteredFiles() From 3c5e45f1eb325dcef8773845efc76d3ba7e40500 Mon Sep 17 00:00:00 2001 From: JJ Lee Date: Thu, 3 Sep 2026 16:45:06 -0400 Subject: [PATCH 09/25] fix(review): restore local/amicode features clobbered by overlay copy Reset session.tsx and review-panel-v2.tsx to local/amicode versions, then add serverUrl. Restores: - keepPreviousData for diff query flicker prevention - serverResponded trust logic for empty server responses - watcherDebounce for file watcher diff_version bump (#743) - onRefresh in reviewPanelV2Props (refresh button) Plus our addition: serverUrl getter for auto-save. --- packages/app/src/pages/session.tsx | 31 ++++++++++++++++--- .../src/pages/session/v2/review-panel-v2.tsx | 2 ++ 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/packages/app/src/pages/session.tsx b/packages/app/src/pages/session.tsx index 3bedff50b..a470e24b5 100644 --- a/packages/app/src/pages/session.tsx +++ b/packages/app/src/pages/session.tsx @@ -1,7 +1,7 @@ import type { FilePart, Project, SnapshotFileDiff, UserMessage } from "@opencode-ai/sdk/v2" import { getFilename } from "@opencode-ai/core/util/path" import { useDialog } from "@opencode-ai/ui/context/dialog" -import { createQuery, skipToken, useMutation } from "@tanstack/solid-query" +import { createQuery, keepPreviousData, skipToken, useMutation } from "@tanstack/solid-query" import { batch, ErrorBoundary, @@ -695,7 +695,7 @@ export default function Page() { return { queryKey: sessionDiffKey(), enabled: !!sessionID, - placeholderData: [] as SnapshotFileDiff[], + placeholderData: keepPreviousData, queryFn: sessionID ? () => sdk() @@ -708,7 +708,11 @@ export default function Page() { const reviewDiffs = createMemo(() => { // Server endpoint returns the authoritative full-session diff (queries all messages). const serverDiffs = sessionDiffQuery.data ?? [] - if (serverDiffs.length > 0) { + // Once the server has responded at least once, trust it — even if it returned []. + // The client-side fallback is only for the initial load before any server response, + // never during refetches (where keepPreviousData already preserves the last result). + const serverResponded = sessionDiffQuery.status === "success" || sessionDiffQuery.isPlaceholderData + if (serverDiffs.length > 0 || serverResponded) { // Server paths are relative to the project root — prefix with ~/project-path const dir = sdk().directory const home = typeof globalThis.process !== "undefined" ? globalThis.process.env?.HOME : undefined @@ -761,7 +765,7 @@ export default function Page() { return { queryKey: ["session-touched-files", sessionID ?? "", sessionDiffVersion()] as const, enabled: !!sessionID, - placeholderData: [] as Array<{ file: string; status: string }>, + placeholderData: keepPreviousData, staleTime: 30_000, queryFn: sessionID ? async () => { @@ -1011,6 +1015,9 @@ export default function Page() { ), ) + // Bump diff_version when the watcher reports external changes to files in the + // current session's worktree (#743). Debounced: 1s trailing-edge per session. + let watcherDebounce: ReturnType | undefined const stopVcs = sdk().event.listen((evt) => { const details = evt.details as { type: string; properties?: unknown } if (details.type !== "file.watcher.updated" && details.type !== "filesystem.changed") return @@ -1020,8 +1027,18 @@ export default function Page() { : undefined const file = typeof props?.file === "string" ? props.file : undefined if (!file || file.startsWith(".git/")) return + const id = params.id + if (!id) return + if (watcherDebounce !== undefined) clearTimeout(watcherDebounce) + watcherDebounce = setTimeout(() => { + watcherDebounce = undefined + sync().set("diff_version", id, (v: number | undefined) => (v ?? 0) + 1) + }, 1000) + }) + onCleanup(() => { + stopVcs() + if (watcherDebounce !== undefined) clearTimeout(watcherDebounce) }) - onCleanup(stopVcs) createEffect( on( @@ -1305,6 +1322,10 @@ export default function Page() { return serverSDK().url }, state: reviewV2State, + onRefresh: () => { + const id = params.id + if (id) sync().set("diff_version", id, (v: number | undefined) => (v ?? 0) + 1) + }, onLineComment: (comment: SessionReviewLineComment) => addCommentToContext({ ...comment, origin: "review" }), onLineCommentUpdate: updateCommentInContext, onLineCommentDelete: removeCommentFromContext, diff --git a/packages/app/src/pages/session/v2/review-panel-v2.tsx b/packages/app/src/pages/session/v2/review-panel-v2.tsx index b5b01a641..57e3a77ea 100644 --- a/packages/app/src/pages/session/v2/review-panel-v2.tsx +++ b/packages/app/src/pages/session/v2/review-panel-v2.tsx @@ -47,6 +47,7 @@ export type ReviewPanelV2Props = { onDiffStyleChange?: (style: SessionReviewDiffStyle) => void serverUrl?: string state: ReviewPanelV2State + onRefresh?: () => void onLineComment?: (comment: SessionReviewLineComment) => void onLineCommentUpdate?: (comment: SessionReviewCommentUpdate) => void onLineCommentDelete?: (comment: SessionReviewCommentDelete) => void @@ -154,6 +155,7 @@ export function ReviewPanelV2(props: ReviewPanelV2Props) { expandMode={props.state.expandMode()} readFile={readFile} serverUrl={props.serverUrl} + onRefresh={props.onRefresh} filePicker={({ onSelect }) => { const files = filteredFiles() From 52c6096e835d14d138442a7f2b0439181581f846 Mon Sep 17 00:00:00 2001 From: JJ Lee Date: Thu, 3 Sep 2026 19:14:59 -0400 Subject: [PATCH 10/25] fix(write): emit filediff metadata so Files Changed shows written files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The write tool computed a diff but never packaged it as a filediff object in its metadata — unlike the edit tool which did. This meant: - The Files Changed panel never saw files created/overwritten by the write tool - The event-reducer never bumped diff_version (no filediff → no refetch) - The server snapshot diff excluded write-tool files from its agentFiles set Now the write tool: 1. Computes additions/deletions via diffLines (same as edit.ts) 2. Pushes filediff via ctx.metadata() mid-execution for live UI updates 3. Includes filediff + diff in the completion return metadata 3 new tests: overwrite filediff, new-file filediff, mid-execution push. --- packages/opencode/src/tool/write.ts | 28 ++++++++++- packages/opencode/test/tool/write.test.ts | 59 +++++++++++++++++++++++ 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/packages/opencode/src/tool/write.ts b/packages/opencode/src/tool/write.ts index 37be6d8c4..58d23e0f4 100644 --- a/packages/opencode/src/tool/write.ts +++ b/packages/opencode/src/tool/write.ts @@ -3,7 +3,7 @@ import * as path from "path" import { Effect } from "effect" import * as Tool from "./tool" import { LSP } from "@/lsp/lsp" -import { createTwoFilesPatch } from "diff" +import { createTwoFilesPatch, diffLines } from "diff" import DESCRIPTION from "./write.txt" import { EventV2Bridge } from "@/event-v2-bridge" import { FileSystem } from "@opencode-ai/core/filesystem" @@ -11,6 +11,7 @@ import { Watcher } from "@opencode-ai/core/filesystem/watcher" import { Format } from "../format" import { FSUtil } from "@opencode-ai/core/fs-util" import { InstanceState } from "@/effect/instance-state" +import { Snapshot } from "@/snapshot" import { trimDiff } from "./edit" import { assertExternalDirectoryEffect } from "./external-directory" import * as Bom from "@/util/bom" @@ -61,6 +62,29 @@ export const WriteTool = Tool.define( }, }) + // Compute filediff for the Files Changed panel (same pattern as edit.ts) + let additions = 0 + let deletions = 0 + for (const change of diffLines(contentOld, contentNew)) { + if (change.added) additions += change.count || 0 + if (change.removed) deletions += change.count || 0 + } + const filediff: Snapshot.FileDiff = { + file: filepath, + patch: diff, + additions, + deletions, + } + + // Push filediff mid-execution so the UI updates before the tool completes + yield* ctx.metadata({ + metadata: { + diff, + filediff, + diagnostics: {}, + }, + }) + yield* fs.writeWithDirs(filepath, Bom.join(contentNew, desiredBom)) if (yield* format.file(filepath)) { yield* Bom.syncFile(fs, filepath, desiredBom) @@ -93,6 +117,8 @@ export const WriteTool = Tool.define( title: path.relative(instance.worktree, filepath), metadata: { diagnostics, + diff, + filediff, filepath, exists: exists, }, diff --git a/packages/opencode/test/tool/write.test.ts b/packages/opencode/test/tool/write.test.ts index 4f897dc87..c797a1ea8 100644 --- a/packages/opencode/test/tool/write.test.ts +++ b/packages/opencode/test/tool/write.test.ts @@ -27,6 +27,21 @@ const ctx = { ask: () => Effect.void, } +// A ctx variant that captures metadata calls for filediff assertions +function ctxWithMetadataCapture() { + const calls: Array> = [] + return { + ctx: { + ...ctx, + metadata: (arg: { metadata: Record }) => { + calls.push(arg.metadata) + return Effect.void + }, + }, + calls, + } +} + afterEach(async () => { await disposeAllInstances() }) @@ -170,6 +185,50 @@ describe("tool.write", () => { expect(result.metadata).toHaveProperty("exists", true) }), ) + + it.instance("returns filediff metadata with file, patch, additions, deletions when overwriting", () => + Effect.gen(function* () { + const test = yield* TestInstance + const filepath = path.join(test.directory, "filediff-test.txt") + yield* Effect.promise(() => fs.writeFile(filepath, "line1\nline2\n", "utf-8")) + const result = yield* run({ filePath: filepath, content: "line1\nline2\nline3\n" }) + + const filediff = (result.metadata as any).filediff + expect(filediff).toBeDefined() + expect(filediff.file).toBe(filepath) + expect(filediff.patch).toBeDefined() + expect(filediff.additions).toBe(1) + expect(filediff.deletions).toBe(0) + }), + ) + + it.instance("returns filediff metadata for new file creation", () => + Effect.gen(function* () { + const test = yield* TestInstance + const filepath = path.join(test.directory, "brand-new.txt") + const result = yield* run({ filePath: filepath, content: "hello\nworld\n" }) + + const filediff = (result.metadata as any).filediff + expect(filediff).toBeDefined() + expect(filediff.file).toBe(filepath) + expect(filediff.additions).toBe(2) + expect(filediff.deletions).toBe(0) + }), + ) + + it.instance("pushes filediff via ctx.metadata() mid-execution so the UI updates before completion", () => + Effect.gen(function* () { + const test = yield* TestInstance + const filepath = path.join(test.directory, "mid-push.txt") + yield* Effect.promise(() => fs.writeFile(filepath, "old\n", "utf-8")) + const capture = ctxWithMetadataCapture() + yield* run({ filePath: filepath, content: "new\n" }, capture.ctx) + + const filediffPush = capture.calls.find((c) => c.filediff) + expect(filediffPush).toBeDefined() + expect((filediffPush!.filediff as any).file).toBe(filepath) + }), + ) }) describe("file permissions", () => { From 0b88c6c9ac3c0fcc2603512a8f6acf6ea361bac4 Mon Sep 17 00:00:00 2001 From: JJ Lee Date: Thu, 3 Sep 2026 19:28:26 -0400 Subject: [PATCH 11/25] fix: in-project files created+deleted in same session no longer ghost in Files Changed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cross-project filter assumed that any tool-metadata diff absent from the server set was cross-project. But in-project files the server correctly excluded (net-zero diff: created then deleted) also leaked through. Now mergeServerAndToolDiffs checks whether the file path starts with the project prefix — in-project files trust the server's authority; only genuinely cross-project files pass through from tool metadata. Also extracts the merge logic into a tested pure function (mergeServerAndToolDiffs in accumulate-diffs.ts) instead of inlining it in session.tsx's createMemo. 6 new tests for the merge function. --- packages/app/src/pages/session.tsx | 49 +++++----- .../pages/session/v2/accumulate-diffs.test.ts | 93 ++++++++++++++++++- .../src/pages/session/v2/accumulate-diffs.ts | 55 +++++++++++ 3 files changed, 168 insertions(+), 29 deletions(-) diff --git a/packages/app/src/pages/session.tsx b/packages/app/src/pages/session.tsx index a470e24b5..86366f521 100644 --- a/packages/app/src/pages/session.tsx +++ b/packages/app/src/pages/session.tsx @@ -88,7 +88,7 @@ import { sessionPanelLayout } from "@/pages/session/session-panel-layout" import { SessionReviewEmptyChangesV2 } from "@opencode-ai/session-ui/v2/session-review-empty-changes-v2" import { ReviewPanelV2 } from "@/pages/session/v2/review-panel-v2" import { createReviewPanelV2State } from "@/pages/session/v2/review-panel-v2-state" -import { accumulateDiffs } from "@/pages/session/v2/accumulate-diffs" +import { accumulateDiffs, mergeServerAndToolDiffs, toHomePath } from "@/pages/session/v2/accumulate-diffs" import { TerminalPanel } from "@/pages/session/terminal-panel" import { TerminalPanelV2 } from "@/pages/session/terminal-panel-v2" import { useComposerCommands } from "@/pages/session/use-composer-commands" @@ -706,35 +706,16 @@ export default function Page() { } }) const reviewDiffs = createMemo(() => { - // Server endpoint returns the authoritative full-session diff (queries all messages). - const serverDiffs = sessionDiffQuery.data ?? [] - // Once the server has responded at least once, trust it — even if it returned []. - // The client-side fallback is only for the initial load before any server response, - // never during refetches (where keepPreviousData already preserves the last result). - const serverResponded = sessionDiffQuery.status === "success" || sessionDiffQuery.isPlaceholderData - if (serverDiffs.length > 0 || serverResponded) { - // Server paths are relative to the project root — prefix with ~/project-path - const dir = sdk().directory - const home = typeof globalThis.process !== "undefined" ? globalThis.process.env?.HOME : undefined - const prefix = home && dir.startsWith(home) ? "~" + dir.slice(home.length) : dir - return serverDiffs - .filter((d): d is SnapshotFileDiff & { file: string } => !!d.file) - .map((d) => ({ ...d, file: d.file.startsWith("/") || d.file.startsWith("~/") ? d.file : `${prefix}/${d.file}` })) - } - // Fallback: derive from tool parts currently loaded in the client. - // This shows immediate results for visible messages while the server query loads. - const allMessages = messages() - if (!allMessages.length) return [] as Array + // Shared path normalization — both sources must use the same function for dedup to work. const dir = sdk().directory const home = typeof globalThis.process !== "undefined" ? globalThis.process.env?.HOME : undefined const prefix = home && dir.startsWith(home) ? "~" + dir.slice(home.length) : dir - const toHomePath = (p: string) => { - if (p.startsWith("~/")) return p - if (home && p.startsWith(home)) return "~" + p.slice(home.length) - if (!p.startsWith("/")) return `${prefix}/${p}` - return p - } + + // --- Tool-metadata diffs (always computed, not just as fallback) --- + // Scraped from completed edit/write/patch/apply_patch tool parts across all messages. + // These see every file the agent touched regardless of directory. const editParts: import("@/pages/session/v2/accumulate-diffs").ToolEditPart[] = [] + const allMessages = messages() for (const msg of allMessages) { const msgParts = sync().data.part[msg.id] if (!msgParts) continue @@ -747,7 +728,7 @@ export default function Page() { const rawTitle = (part.state as { title?: string }).title || filediff.file editParts.push({ file: filediff.file, - title: toHomePath(rawTitle), + title: toHomePath(rawTitle, home, prefix), patch: filediff.patch, additions: filediff.additions ?? 0, deletions: filediff.deletions ?? 0, @@ -755,7 +736,19 @@ export default function Page() { } } } - return accumulateDiffs(editParts) + const toolDiffs = accumulateDiffs(editParts) + + // --- Server diffs (authoritative for in-project files) --- + const serverDiffs = sessionDiffQuery.data ?? [] + const serverResponded = sessionDiffQuery.status === "success" || sessionDiffQuery.isPlaceholderData + + return mergeServerAndToolDiffs({ + serverDiffs, + toolDiffs, + serverResponded, + directory: dir, + home, + }) }) // All files touched by edit tools in this session — fetched from the server diff --git a/packages/app/src/pages/session/v2/accumulate-diffs.test.ts b/packages/app/src/pages/session/v2/accumulate-diffs.test.ts index cec265861..1660fcca3 100644 --- a/packages/app/src/pages/session/v2/accumulate-diffs.test.ts +++ b/packages/app/src/pages/session/v2/accumulate-diffs.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test" -import { accumulateDiffs, type ToolEditPart } from "./accumulate-diffs" +import { accumulateDiffs, mergeServerAndToolDiffs, type ToolEditPart } from "./accumulate-diffs" const edit = (file: string, overrides: Partial = {}): ToolEditPart => ({ file, @@ -112,3 +112,94 @@ describe("accumulateDiffs", () => { // on the query so the fallback never fires while server data exists. }) }) + +// --- mergeServerAndToolDiffs --- + +const HOME = "/Users/jj" +const DIR = "/Users/jj/harmoniqs/amicode" +const diff = ( + file: string, + patch = "mock", + additions = 1, + deletions = 0, + status: "added" | "modified" | "deleted" = "modified", +) => ({ file, patch, additions, deletions, status }) + +describe("mergeServerAndToolDiffs", () => { + test("cross-project tool diffs pass through when server has responded", () => { + const result = mergeServerAndToolDiffs({ + serverDiffs: [diff("src/foo.ts")], + toolDiffs: [diff("~/other-project/bar.ts")], + serverResponded: true, + directory: DIR, + home: HOME, + }) + const files = result.map((d) => d.file) + expect(files).toContain("~/harmoniqs/amicode/src/foo.ts") + expect(files).toContain("~/other-project/bar.ts") + }) + + test("in-project files come from the server, not tool metadata (server wins)", () => { + const result = mergeServerAndToolDiffs({ + serverDiffs: [diff("src/foo.ts", "server-patch", 10, 5)], + toolDiffs: [diff("~/harmoniqs/amicode/src/foo.ts", "tool-patch", 3, 1)], + serverResponded: true, + directory: DIR, + home: HOME, + }) + const entry = result.find((d) => d.file.endsWith("src/foo.ts")) + expect(entry).toBeDefined() + expect(entry!.patch).toBe("server-patch") + expect(entry!.additions).toBe(10) + }) + + test("in-project file absent from server (created+deleted) is excluded, not leaked as cross-project", () => { + // The bug: file was created by the write tool, then manually deleted. + // Server correctly has no diff (net zero). Tool metadata still has it. + // The old code leaked it through the cross-project filter. + const result = mergeServerAndToolDiffs({ + serverDiffs: [], + toolDiffs: [diff("~/harmoniqs/amicode/files-changed-test.md", "stale-patch", 3, 0, "added")], + serverResponded: true, + directory: DIR, + home: HOME, + }) + expect(result).toHaveLength(0) + }) + + test("in-project file with relative path absent from server is also excluded", () => { + const result = mergeServerAndToolDiffs({ + serverDiffs: [], + toolDiffs: [diff("~/harmoniqs/amicode/src/temp.ts")], + serverResponded: true, + directory: DIR, + home: HOME, + }) + expect(result).toHaveLength(0) + }) + + test("fallback: all tool diffs shown when server has not responded", () => { + const result = mergeServerAndToolDiffs({ + serverDiffs: [], + toolDiffs: [ + diff("~/harmoniqs/amicode/src/foo.ts"), + diff("~/other/bar.ts"), + ], + serverResponded: false, + directory: DIR, + home: HOME, + }) + expect(result).toHaveLength(2) + }) + + test("dedup by normalized path — server wins on conflict", () => { + const result = mergeServerAndToolDiffs({ + serverDiffs: [diff("/Users/jj/harmoniqs/amicode/src/foo.ts")], + toolDiffs: [diff("~/harmoniqs/amicode/src/foo.ts")], + serverResponded: true, + directory: DIR, + home: HOME, + }) + expect(result).toHaveLength(1) + }) +}) diff --git a/packages/app/src/pages/session/v2/accumulate-diffs.ts b/packages/app/src/pages/session/v2/accumulate-diffs.ts index 8e069fb7e..166f2236f 100644 --- a/packages/app/src/pages/session/v2/accumulate-diffs.ts +++ b/packages/app/src/pages/session/v2/accumulate-diffs.ts @@ -8,6 +8,61 @@ export type ToolEditPart = { deletions: number } +/** + * Normalize a file path to ~/... form for consistent dedup across sources. + */ +export function toHomePath(p: string, home: string | undefined, prefix: string): string { + if (p.startsWith("~/")) return p + if (home && p.startsWith(home)) return "~" + p.slice(home.length) + if (!p.startsWith("/")) return `${prefix}/${p}` + return p +} + +export interface MergeOpts { + serverDiffs: SnapshotFileDiff[] + toolDiffs: Array + serverResponded: boolean + directory: string + home: string | undefined +} + +/** + * Merge server shadow-git diffs with tool-metadata diffs. + * + * Server diffs are authoritative for in-project files. Tool-metadata diffs + * fill in cross-project files (outside the project directory) not covered + * by the server. In-project tool-metadata diffs that the server excluded + * (e.g. created + deleted = net zero) are dropped — the server's absence + * is the authority. + * + * When serverResponded is false (initial load), all tool-metadata diffs + * are returned as a fallback. + */ +export function mergeServerAndToolDiffs(opts: MergeOpts): Array { + const { serverDiffs, toolDiffs, serverResponded, directory, home } = opts + const prefix = home && directory.startsWith(home) ? "~" + directory.slice(home.length) : directory + const projectPrefix = prefix + "/" + + if (serverDiffs.length > 0 || serverResponded) { + const normalizedServerDiffs = serverDiffs + .filter((d): d is SnapshotFileDiff & { file: string } => !!d.file) + .map((d) => ({ ...d, file: toHomePath(d.file, home, prefix) })) + + const serverFiles = new Set(normalizedServerDiffs.map((d) => d.file)) + // Only pass through tool-metadata diffs that are BOTH absent from the + // server set AND outside the project directory. In-project files trust + // the server's authority — if the server excluded them (created + deleted, + // or reverted), they should not leak through as phantom entries. + const crossProjectDiffs = toolDiffs.filter( + (d) => !serverFiles.has(d.file) && !d.file.startsWith(projectPrefix), + ) + + return [...normalizedServerDiffs, ...crossProjectDiffs] + } + + return toolDiffs +} + /** * Accumulate file diffs from tool edit parts across a session. * From fd097b2f305a8d06888fbb7a3e8291db3102db7b Mon Sep 17 00:00:00 2001 From: JJ Lee Date: Thu, 3 Sep 2026 20:00:18 -0400 Subject: [PATCH 12/25] feat: track file renames in Files Changed via file-op-notify messages When a file is moved or renamed via the sidebar, its tool-metadata diff still records the old path. The server correctly excludes the old path (net zero), and the new location may be cross-project (invisible to the server). Result: the file vanished from Files Changed. Now session.tsx listens for file-op-notify messages from the extension, maintains a rename map, and applies it to tool-metadata diffs via the new applyRenames() pure function before merging with server diffs. The renamed file appears at its new location in Files Changed. 5 new tests for applyRenames. --- packages/app/src/pages/session.tsx | 34 ++++++++++++- .../pages/session/v2/accumulate-diffs.test.ts | 49 ++++++++++++++++++- .../src/pages/session/v2/accumulate-diffs.ts | 17 +++++++ 3 files changed, 97 insertions(+), 3 deletions(-) diff --git a/packages/app/src/pages/session.tsx b/packages/app/src/pages/session.tsx index 86366f521..dd226d7a0 100644 --- a/packages/app/src/pages/session.tsx +++ b/packages/app/src/pages/session.tsx @@ -88,7 +88,7 @@ import { sessionPanelLayout } from "@/pages/session/session-panel-layout" import { SessionReviewEmptyChangesV2 } from "@opencode-ai/session-ui/v2/session-review-empty-changes-v2" import { ReviewPanelV2 } from "@/pages/session/v2/review-panel-v2" import { createReviewPanelV2State } from "@/pages/session/v2/review-panel-v2-state" -import { accumulateDiffs, mergeServerAndToolDiffs, toHomePath } from "@/pages/session/v2/accumulate-diffs" +import { accumulateDiffs, applyRenames, mergeServerAndToolDiffs, toHomePath } from "@/pages/session/v2/accumulate-diffs" import { TerminalPanel } from "@/pages/session/terminal-panel" import { TerminalPanelV2 } from "@/pages/session/terminal-panel-v2" import { useComposerCommands } from "@/pages/session/use-composer-commands" @@ -681,6 +681,36 @@ export default function Page() { const mobileChanges = createMemo(() => !isDesktop() && store.mobileTab === "changes") const EDIT_TOOLS = new Set(["edit", "write", "patch", "apply_patch"]) + + // --- File rename tracking (sidebar move/rename → Files Changed update) --- + // When the sidebar moves or renames a file, the extension posts a + // file-op-notify message. We track the rename so the tool-metadata diffs + // (which still have the old path) are displayed at the new location. + const [fileRenames, setFileRenames] = createSignal(new Map()) + const onFileOpNotify = (e: MessageEvent) => { + const d = e.data as { source?: string; kind?: string; op?: string; oldPath?: string; newPath?: string } | undefined + if (d?.source !== "amicode" || d?.kind !== "file-op-notify") return + if ((d.op === "move" || d.op === "rename") && d.oldPath && d.newPath) { + // Normalize paths to ~/... form for consistent matching with tool diffs + const dir = sdk().directory + const home = typeof globalThis.process !== "undefined" ? globalThis.process.env?.HOME : undefined + const prefix = home && dir.startsWith(home) ? "~" + dir.slice(home.length) : dir + const oldNorm = toHomePath(d.oldPath, home, prefix) + const newNorm = toHomePath(d.newPath, home, prefix) + setFileRenames((prev) => { + const next = new Map(prev) + // Chain resolution: if anything pointed to oldPath, update it to newPath + for (const [k, v] of next) { + if (v === oldNorm) next.set(k, newNorm) + } + next.set(oldNorm, newNorm) + return next + }) + } + } + window.addEventListener("message", onFileOpNotify) + onCleanup(() => window.removeEventListener("message", onFileOpNotify)) + // Refetch when the session transitions to idle (assistant finished, snapshot taken) // or when a file-editing tool completes mid-turn (diff_version bumps) const sessionDiffVersion = () => { @@ -736,7 +766,7 @@ export default function Page() { } } } - const toolDiffs = accumulateDiffs(editParts) + const toolDiffs = applyRenames(accumulateDiffs(editParts), fileRenames()) // --- Server diffs (authoritative for in-project files) --- const serverDiffs = sessionDiffQuery.data ?? [] diff --git a/packages/app/src/pages/session/v2/accumulate-diffs.test.ts b/packages/app/src/pages/session/v2/accumulate-diffs.test.ts index 1660fcca3..9c58efa74 100644 --- a/packages/app/src/pages/session/v2/accumulate-diffs.test.ts +++ b/packages/app/src/pages/session/v2/accumulate-diffs.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test" -import { accumulateDiffs, mergeServerAndToolDiffs, type ToolEditPart } from "./accumulate-diffs" +import { accumulateDiffs, applyRenames, mergeServerAndToolDiffs, type ToolEditPart } from "./accumulate-diffs" const edit = (file: string, overrides: Partial = {}): ToolEditPart => ({ file, @@ -203,3 +203,50 @@ describe("mergeServerAndToolDiffs", () => { expect(result).toHaveLength(1) }) }) + +// --- applyRenames --- + +describe("applyRenames", () => { + test("renames a file path when it appears in the rename map", () => { + const diffs = [diff("~/harmoniqs/amicode/test.md")] + const renames = new Map([["~/harmoniqs/amicode/test.md", "~/harmoniqs/opencode/test.md"]]) + const result = applyRenames(diffs, renames) + expect(result).toHaveLength(1) + expect(result[0].file).toBe("~/harmoniqs/opencode/test.md") + }) + + test("leaves files unchanged when not in the rename map", () => { + const diffs = [diff("~/harmoniqs/amicode/src/foo.ts")] + const renames = new Map([["~/harmoniqs/amicode/test.md", "~/harmoniqs/opencode/test.md"]]) + const result = applyRenames(diffs, renames) + expect(result[0].file).toBe("~/harmoniqs/amicode/src/foo.ts") + }) + + test("returns diffs unchanged when rename map is empty", () => { + const diffs = [diff("~/harmoniqs/amicode/test.md"), diff("~/other/bar.ts")] + const result = applyRenames(diffs, new Map()) + expect(result).toEqual(diffs) + }) + + test("preserves all other diff fields (patch, additions, deletions, status)", () => { + const diffs = [diff("~/old/path.ts", "the-patch", 5, 3, "modified")] + const renames = new Map([["~/old/path.ts", "~/new/path.ts"]]) + const result = applyRenames(diffs, renames) + expect(result[0].file).toBe("~/new/path.ts") + expect(result[0].patch).toBe("the-patch") + expect(result[0].additions).toBe(5) + expect(result[0].deletions).toBe(3) + expect(result[0].status).toBe("modified") + }) + + test("handles multiple renames in one pass", () => { + const diffs = [diff("~/a/one.ts"), diff("~/b/two.ts")] + const renames = new Map([ + ["~/a/one.ts", "~/c/one.ts"], + ["~/b/two.ts", "~/d/two.ts"], + ]) + const result = applyRenames(diffs, renames) + expect(result[0].file).toBe("~/c/one.ts") + expect(result[1].file).toBe("~/d/two.ts") + }) +}) diff --git a/packages/app/src/pages/session/v2/accumulate-diffs.ts b/packages/app/src/pages/session/v2/accumulate-diffs.ts index 166f2236f..22d4e1ef8 100644 --- a/packages/app/src/pages/session/v2/accumulate-diffs.ts +++ b/packages/app/src/pages/session/v2/accumulate-diffs.ts @@ -117,3 +117,20 @@ export function accumulateDiffs(parts: ToolEditPart[]): Array, + renames: Map, +): Array { + if (renames.size === 0) return diffs + return diffs.map((d) => { + const newPath = renames.get(d.file) + return newPath ? { ...d, file: newPath } : d + }) +} From 3a410eb804000c2e64b2845998340d13af9a2f56 Mon Sep 17 00:00:00 2001 From: JJ Lee Date: Thu, 3 Sep 2026 20:20:53 -0400 Subject: [PATCH 13/25] fix: use home from file-op-notify message for path normalization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The browser iframe has no process.env.HOME, so toHomePath couldn't normalize absolute paths to ~/... form. The rename map keys stayed absolute while tool diffs were normalized — no match, file vanished. Now the extension sends os.homedir() in the file-op-notify message and session.tsx uses it for normalization. --- packages/app/src/pages/session.tsx | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/app/src/pages/session.tsx b/packages/app/src/pages/session.tsx index dd226d7a0..1758901e9 100644 --- a/packages/app/src/pages/session.tsx +++ b/packages/app/src/pages/session.tsx @@ -688,12 +688,13 @@ export default function Page() { // (which still have the old path) are displayed at the new location. const [fileRenames, setFileRenames] = createSignal(new Map()) const onFileOpNotify = (e: MessageEvent) => { - const d = e.data as { source?: string; kind?: string; op?: string; oldPath?: string; newPath?: string } | undefined + const d = e.data as { source?: string; kind?: string; op?: string; oldPath?: string; newPath?: string; home?: string } | undefined if (d?.source !== "amicode" || d?.kind !== "file-op-notify") return if ((d.op === "move" || d.op === "rename") && d.oldPath && d.newPath) { - // Normalize paths to ~/... form for consistent matching with tool diffs + // Use home from the message (extension has os.homedir()) since the + // browser iframe has no process.env.HOME for path normalization. const dir = sdk().directory - const home = typeof globalThis.process !== "undefined" ? globalThis.process.env?.HOME : undefined + const home = d.home ?? (typeof globalThis.process !== "undefined" ? globalThis.process.env?.HOME : undefined) const prefix = home && dir.startsWith(home) ? "~" + dir.slice(home.length) : dir const oldNorm = toHomePath(d.oldPath, home, prefix) const newNorm = toHomePath(d.newPath, home, prefix) From 2d4dbdf56d71d43b13f1351749003feda4e786b0 Mon Sep 17 00:00:00 2001 From: JJ Lee Date: Thu, 3 Sep 2026 20:40:41 -0400 Subject: [PATCH 14/25] fix: bump diff_version on file-op-notify so moved-back files reappear When a file is moved cross-project then moved back, the rename map resolves to the original in-project path. But the server still has a stale response (no diff for that file). The cross-project filter drops it (in-project files trust the server). Result: the file vanishes. Now the file-op-notify handler bumps diff_version immediately after updating the rename map, forcing a server refetch. The fresh server response includes the file at its current location. 2 new tests documenting the round-trip move scenario. --- packages/app/src/pages/session.tsx | 7 ++ .../pages/session/v2/accumulate-diffs.test.ts | 78 +++++++++++++++++++ 2 files changed, 85 insertions(+) diff --git a/packages/app/src/pages/session.tsx b/packages/app/src/pages/session.tsx index 1758901e9..55a23f959 100644 --- a/packages/app/src/pages/session.tsx +++ b/packages/app/src/pages/session.tsx @@ -707,6 +707,13 @@ export default function Page() { next.set(oldNorm, newNorm) return next }) + // Force a server refetch so in-project files at their new location + // appear immediately — without this, the stale server response (which + // doesn't include the file) persists until the file watcher's 1s debounce. + const sessionID = params.id + if (sessionID) { + sync().set("diff_version", sessionID, (v: number | undefined) => (v ?? 0) + 1) + } } } window.addEventListener("message", onFileOpNotify) diff --git a/packages/app/src/pages/session/v2/accumulate-diffs.test.ts b/packages/app/src/pages/session/v2/accumulate-diffs.test.ts index 9c58efa74..3938c10eb 100644 --- a/packages/app/src/pages/session/v2/accumulate-diffs.test.ts +++ b/packages/app/src/pages/session/v2/accumulate-diffs.test.ts @@ -250,3 +250,81 @@ describe("applyRenames", () => { expect(result[1].file).toBe("~/d/two.ts") }) }) + +// --- move-back-to-project: full round-trip scenario --- + +describe("round-trip move: project → cross-project → back to project", () => { + test("file moved out then back is visible when server includes it after refetch", () => { + // Step 1: file created in-project, server has it + const step1 = mergeServerAndToolDiffs({ + serverDiffs: [diff("~/harmoniqs/amicode/test.md", "patch", 2, 0, "added")], + toolDiffs: [diff("~/harmoniqs/amicode/test.md", "patch", 2, 0, "added")], + serverResponded: true, + directory: DIR, + home: HOME, + }) + expect(step1).toHaveLength(1) + expect(step1[0].file).toBe("~/harmoniqs/amicode/test.md") + + // Step 2: file moved to opencode — rename map transforms tool diff + const renames1 = new Map([["~/harmoniqs/amicode/test.md", "~/harmoniqs/opencode/test.md"]]) + const toolDiffsStep2 = applyRenames( + [diff("~/harmoniqs/amicode/test.md", "patch", 2, 0, "added")], + renames1, + ) + const step2 = mergeServerAndToolDiffs({ + serverDiffs: [], // server no longer has it (moved away) + toolDiffs: toolDiffsStep2, + serverResponded: true, + directory: DIR, + home: HOME, + }) + expect(step2).toHaveLength(1) + expect(step2[0].file).toBe("~/harmoniqs/opencode/test.md") + + // Step 3: file moved back — rename chain resolves to original path + // After chain resolution: amicode/test.md → amicode/test.md (identity) + const renames2 = new Map([ + ["~/harmoniqs/amicode/test.md", "~/harmoniqs/amicode/test.md"], + ["~/harmoniqs/opencode/test.md", "~/harmoniqs/amicode/test.md"], + ]) + const toolDiffsStep3 = applyRenames( + [diff("~/harmoniqs/amicode/test.md", "patch", 2, 0, "added")], + renames2, + ) + // Tool diff is back at the in-project path + expect(toolDiffsStep3[0].file).toBe("~/harmoniqs/amicode/test.md") + + // With server refetch (diff_version bumped), server includes the file again + const step3 = mergeServerAndToolDiffs({ + serverDiffs: [diff("~/harmoniqs/amicode/test.md", "patch", 2, 0, "added")], + toolDiffs: toolDiffsStep3, + serverResponded: true, + directory: DIR, + home: HOME, + }) + expect(step3).toHaveLength(1) + expect(step3[0].file).toBe("~/harmoniqs/amicode/test.md") + }) + + test("file moved back DISAPPEARS when server has NOT refetched (the bug this fix addresses)", () => { + // Rename map resolves to in-project, but server still has stale empty response + const renames = new Map([ + ["~/harmoniqs/amicode/test.md", "~/harmoniqs/amicode/test.md"], + ]) + const toolDiffs = applyRenames( + [diff("~/harmoniqs/amicode/test.md", "patch", 2, 0, "added")], + renames, + ) + // Server hasn't refetched — stale empty response + const result = mergeServerAndToolDiffs({ + serverDiffs: [], + toolDiffs, + serverResponded: true, + directory: DIR, + home: HOME, + }) + // Without the diff_version bump, the file is gone (server is authoritative for in-project) + expect(result).toHaveLength(0) + }) +}) From 5aeccfdf8633e2a60e27e2bc663361fec36bcbe3 Mon Sep 17 00:00:00 2001 From: JJ Lee Date: Thu, 3 Sep 2026 20:45:51 -0400 Subject: [PATCH 15/25] fix: rename map must use same home as reviewDiffs memo (undefined in browser) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The listener used d.home from the extension (producing ~/... keys) while the memo used globalThis.process.env.HOME (undefined in browser, producing absolute paths). Rename map keys never matched tool diff paths. Both now use the same home source — undefined in the browser, so both produce absolute paths. Consistent normalization = keys match. --- packages/app/src/pages/session.tsx | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/app/src/pages/session.tsx b/packages/app/src/pages/session.tsx index 55a23f959..7922dfacb 100644 --- a/packages/app/src/pages/session.tsx +++ b/packages/app/src/pages/session.tsx @@ -691,10 +691,13 @@ export default function Page() { const d = e.data as { source?: string; kind?: string; op?: string; oldPath?: string; newPath?: string; home?: string } | undefined if (d?.source !== "amicode" || d?.kind !== "file-op-notify") return if ((d.op === "move" || d.op === "rename") && d.oldPath && d.newPath) { - // Use home from the message (extension has os.homedir()) since the - // browser iframe has no process.env.HOME for path normalization. + // Use the SAME home as the reviewDiffs memo (globalThis.process.env.HOME, + // which is undefined in the browser). This ensures rename map keys match + // tool-diff paths — both end up as absolute paths in the browser iframe. + // Do NOT use d.home here: that produces ~/... keys while the memo produces + // /Users/... paths, and they never match. const dir = sdk().directory - const home = d.home ?? (typeof globalThis.process !== "undefined" ? globalThis.process.env?.HOME : undefined) + const home = typeof globalThis.process !== "undefined" ? globalThis.process.env?.HOME : undefined const prefix = home && dir.startsWith(home) ? "~" + dir.slice(home.length) : dir const oldNorm = toHomePath(d.oldPath, home, prefix) const newNorm = toHomePath(d.newPath, home, prefix) From b64b7b51fcbdb33fa07be2e2a770e3224b90d5e6 Mon Sep 17 00:00:00 2001 From: JJ Lee Date: Thu, 3 Sep 2026 21:23:00 -0400 Subject: [PATCH 16/25] fix(file-picker): horizontal scroll for dropdown in narrow panels The file-picker dropdown overflowed the side panel because it used a fixed max-width: 400px with position: absolute. Items used width: 100% so overflow-x: auto never fired. - Cap dropdown max-width to min(400px, calc(100vw - 16px)) - Add scroll-inner wrapper (flex column, width: max-content, min-width: 100%) so all items align to the widest one and trigger horizontal scroll - Remove width: 100% from items (flex-column stretch handles alignment) - Remove overflow: hidden / text-overflow: ellipsis from root label so it contributes full width to the scrollable area Test: new case in file-name-picker.test.tsx verifies the scroll-inner wrapper is present and contains the filePicker content. --- .../v2/components/file-name-picker.test.tsx | 40 ++++++++++++++++++- .../session-review-file-preview-v2.tsx | 4 +- .../src/v2/components/session-review-v2.css | 16 ++++++-- 3 files changed, 53 insertions(+), 7 deletions(-) diff --git a/packages/session-ui/src/v2/components/file-name-picker.test.tsx b/packages/session-ui/src/v2/components/file-name-picker.test.tsx index ccd1099be..90cd9410a 100644 --- a/packages/session-ui/src/v2/components/file-name-picker.test.tsx +++ b/packages/session-ui/src/v2/components/file-name-picker.test.tsx @@ -82,12 +82,17 @@ function FileNameWithPicker(props: { dropdown.className = "session-review-v2-file-picker-dropdown" dropdown.setAttribute("data-testid", "file-picker-dropdown") - // Render the filePicker content + // Scroll-inner wrapper (mirrors real component) + const scrollInner = document.createElement("div") + scrollInner.className = "session-review-v2-file-picker-scroll-inner" + + // Render the filePicker content inside the scroll wrapper const pickerContent = props.filePicker!({ onSelect }) if (pickerContent instanceof Node) { - dropdown.appendChild(pickerContent) + scrollInner.appendChild(pickerContent) } + dropdown.appendChild(scrollInner) dropdownContainer.appendChild(dropdown) }) @@ -168,6 +173,37 @@ describe("FileNameWithPicker (render prop)", () => { cleanup() }) + test("dropdown wraps filePicker content in a scroll-inner container", () => { + const { host, cleanup } = mount({ + file: "src/deeply/nested/path/to/component.tsx", + filePicker: ({ onSelect }) => { + const div = document.createElement("div") + div.setAttribute("data-testid", "mock-tree") + div.textContent = "tree content" + return div + }, + onSelectFile: () => {}, + }) + + // Open dropdown + const trigger = host.querySelector("[data-testid='file-picker-trigger']") as HTMLButtonElement + trigger.click() + + const dropdown = host.querySelector("[data-testid='file-picker-dropdown']") + expect(dropdown).not.toBeNull() + + // The dropdown's direct child should be the scroll-inner wrapper + const scrollInner = dropdown!.querySelector(".session-review-v2-file-picker-scroll-inner") + expect(scrollInner).not.toBeNull() + + // The filePicker content should be inside the scroll-inner, not directly in the dropdown + const tree = scrollInner!.querySelector("[data-testid='mock-tree']") + expect(tree).not.toBeNull() + expect(tree!.textContent).toBe("tree content") + + cleanup() + }) + test("falls back to static filename when no filePicker provided", () => { const { host, cleanup } = mount({ file: "src/foo.ts", diff --git a/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx b/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx index 27e26ab6f..c479e7b19 100644 --- a/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx +++ b/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx @@ -696,7 +696,9 @@ function FileNameWithPicker(props: {
- {props.filePicker!({ onSelect })} +
+ {props.filePicker!({ onSelect })} +
diff --git a/packages/session-ui/src/v2/components/session-review-v2.css b/packages/session-ui/src/v2/components/session-review-v2.css index 1f7e469e7..83df6428a 100644 --- a/packages/session-ui/src/v2/components/session-review-v2.css +++ b/packages/session-ui/src/v2/components/session-review-v2.css @@ -574,7 +574,7 @@ padding: 4px; max-height: 320px; min-width: 200px; - max-width: 400px; + max-width: min(400px, calc(100vw - 16px)); overflow-x: auto; overflow-y: auto; border-radius: var(--radius-md, 8px); @@ -583,6 +583,17 @@ box-shadow: var(--shadow-md, 0 4px 12px rgba(0,0,0,0.15)); } +/* Scroll-inner wrapper: sizes to the widest item (max-content) but at least + as wide as the dropdown (min-width: 100%). Flex-column children stretch + to fill, so all rows align. The dropdown's overflow-x: auto provides + the horizontal scrollbar when the widest item exceeds the dropdown. */ +.session-review-v2-file-picker-scroll-inner { + display: flex; + flex-direction: column; + width: max-content; + min-width: 100%; +} + /* File picker popover content (legacy, kept for compat) */ .session-review-v2-file-picker-content { padding: 4px !important; @@ -605,8 +616,6 @@ line-height: 1.4; color: var(--v2-text-text-muted); white-space: nowrap; - overflow: hidden; - text-overflow: ellipsis; border-bottom: 1px solid color-mix(in oklch, var(--border-base, var(--v2-border-border-weak)) 30%, transparent); margin-bottom: 2px; } @@ -626,7 +635,6 @@ display: flex; align-items: center; gap: 6px; - width: 100%; padding: 4px 8px; border: none; border-radius: var(--radius-xs, 4px); From 4401f70f7f7f5c2fe21969a148482d3b28671762 Mon Sep 17 00:00:00 2001 From: JJ Lee Date: Thu, 3 Sep 2026 21:45:12 -0400 Subject: [PATCH 17/25] fix(file-picker): use min-width: max-content for reliable trackpad scroll MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The previous flex-column + width: max-content approach didn't produce actual overflow in WebKit — the block-level flex container resolved to the dropdown's width, leaving nothing to scroll. Switch to a plain block div with min-width: max-content — this reliably expands the wrapper to the widest child, triggering overflow-x: auto. Also add overscroll-behavior: contain on the dropdown to keep the trackpad gesture from leaking to parent containers. --- .../src/v2/components/session-review-v2.css | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/packages/session-ui/src/v2/components/session-review-v2.css b/packages/session-ui/src/v2/components/session-review-v2.css index 83df6428a..c533169ed 100644 --- a/packages/session-ui/src/v2/components/session-review-v2.css +++ b/packages/session-ui/src/v2/components/session-review-v2.css @@ -577,21 +577,19 @@ max-width: min(400px, calc(100vw - 16px)); overflow-x: auto; overflow-y: auto; + overscroll-behavior: contain; border-radius: var(--radius-md, 8px); background-color: var(--surface-raised-stronger-non-alpha, var(--v2-background-bg-layer-01)); border: 1px solid color-mix(in oklch, var(--border-base, var(--v2-border-border-base)) 50%, transparent); box-shadow: var(--shadow-md, 0 4px 12px rgba(0,0,0,0.15)); } -/* Scroll-inner wrapper: sizes to the widest item (max-content) but at least - as wide as the dropdown (min-width: 100%). Flex-column children stretch - to fill, so all rows align. The dropdown's overflow-x: auto provides - the horizontal scrollbar when the widest item exceeds the dropdown. */ +/* Scroll-inner wrapper: min-width: max-content ensures the wrapper is at + least as wide as its widest child. When that exceeds the dropdown's + capped width, overflow-x: auto on the dropdown provides horizontal + scrolling. Block items inside fill the wrapper width naturally. */ .session-review-v2-file-picker-scroll-inner { - display: flex; - flex-direction: column; - width: max-content; - min-width: 100%; + min-width: max-content; } /* File picker popover content (legacy, kept for compat) */ From c850c3bae17d342507960a513524b67908527d3c Mon Sep 17 00:00:00 2001 From: JJ Lee Date: Thu, 3 Sep 2026 22:03:06 -0400 Subject: [PATCH 18/25] fix(file-picker): pin dropdown to wrapper edges for panel-safe scroll MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dropdown used left: 0 + max-width: min(400px, calc(100vw - 16px)), but 100vw in a VS Code WebView is the full window, not the side panel. The dropdown rendered at full width and bled past the panel edge — no overflow inside, so nothing to scroll. Pin both edges (left: 0; right: 0) to the wrapper, which has flex: 1 inside the file header and IS panel-constrained. The dropdown is now exactly the wrapper's width; the scroll-inner (min-width: max-content) produces real overflow when file paths are wider, and overflow-x: auto provides the horizontal scrollbar. --- packages/session-ui/src/v2/components/session-review-v2.css | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/session-ui/src/v2/components/session-review-v2.css b/packages/session-ui/src/v2/components/session-review-v2.css index c533169ed..7cce4fc82 100644 --- a/packages/session-ui/src/v2/components/session-review-v2.css +++ b/packages/session-ui/src/v2/components/session-review-v2.css @@ -570,11 +570,10 @@ position: absolute; top: calc(100% + 4px); left: 0; + right: 0; z-index: var(--z-overlay, 100); padding: 4px; max-height: 320px; - min-width: 200px; - max-width: min(400px, calc(100vw - 16px)); overflow-x: auto; overflow-y: auto; overscroll-behavior: contain; From 70746d24ade9f6740f30522dc3487aee518342e8 Mon Sep 17 00:00:00 2001 From: JJ Lee Date: Thu, 3 Sep 2026 22:09:45 -0400 Subject: [PATCH 19/25] fix(file-picker): JS-measured max-width for panel-safe dropdown The dropdown bled past the side panel edge because 100vw in a VS Code WebView is the full window, not the panel, and left: 0; right: 0 made it too narrow (pinned to the wrapper width only). Instead, measure the dropdown's actual left offset on mount via a ref callback + requestAnimationFrame, then set max-width to the remaining panel space (window.innerWidth - left - 8px, floor 200px). The dropdown shrink-wraps to its content up to this cap; wider content scrolls horizontally via the scroll-inner (min-width: max-content). Test: new async case mocks getBoundingClientRect and window.innerWidth, verifies max-width is computed correctly. --- .../v2/components/file-name-picker.test.tsx | 42 +++++++++++++++++++ .../session-review-file-preview-v2.tsx | 11 ++++- .../src/v2/components/session-review-v2.css | 1 - 3 files changed, 52 insertions(+), 2 deletions(-) diff --git a/packages/session-ui/src/v2/components/file-name-picker.test.tsx b/packages/session-ui/src/v2/components/file-name-picker.test.tsx index 90cd9410a..a680cc469 100644 --- a/packages/session-ui/src/v2/components/file-name-picker.test.tsx +++ b/packages/session-ui/src/v2/components/file-name-picker.test.tsx @@ -94,6 +94,13 @@ function FileNameWithPicker(props: { dropdown.appendChild(scrollInner) dropdownContainer.appendChild(dropdown) + + // Cap max-width to remaining panel space (mirrors real component's ref callback) + requestAnimationFrame(() => { + const left = dropdown.getBoundingClientRect().left + const available = window.innerWidth - left - 8 + dropdown.style.maxWidth = `${Math.max(200, available)}px` + }) }) return wrapper @@ -204,6 +211,41 @@ describe("FileNameWithPicker (render prop)", () => { cleanup() }) + test("dropdown max-width is capped to remaining panel width", async () => { + Object.defineProperty(window, "innerWidth", { value: 450, configurable: true }) + + const { host, cleanup } = mount({ + file: "src/foo.ts", + filePicker: ({ onSelect }) => { + const div = document.createElement("div") + div.setAttribute("data-testid", "mock-tree") + return div + }, + onSelectFile: () => {}, + }) + + const trigger = host.querySelector("[data-testid='file-picker-trigger']") as HTMLButtonElement + trigger.click() + + const dropdown = host.querySelector("[data-testid='file-picker-dropdown']") as HTMLElement + expect(dropdown).not.toBeNull() + + // Simulate the dropdown sitting at x=120 from the panel's left edge + dropdown.getBoundingClientRect = () => ({ + left: 120, top: 40, right: 520, bottom: 200, + width: 400, height: 160, x: 120, y: 40, + toJSON() { return this }, + }) + + // Wait for the ref callback's requestAnimationFrame + await new Promise((r) => setTimeout(r, 50)) + + // Should cap at (450 - 120 - 8) = 322px + expect(dropdown.style.maxWidth).toBe("322px") + + cleanup() + }) + test("falls back to static filename when no filePicker provided", () => { const { host, cleanup } = mount({ file: "src/foo.ts", diff --git a/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx b/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx index c479e7b19..241144158 100644 --- a/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx +++ b/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx @@ -695,7 +695,16 @@ function FileNameWithPicker(props: { -
+
{ + requestAnimationFrame(() => { + const left = el.getBoundingClientRect().left + const available = window.innerWidth - left - 8 + el.style.maxWidth = `${Math.max(200, available)}px` + }) + }} + class="session-review-v2-file-picker-dropdown" + >
{props.filePicker!({ onSelect })}
diff --git a/packages/session-ui/src/v2/components/session-review-v2.css b/packages/session-ui/src/v2/components/session-review-v2.css index 7cce4fc82..2a673d8e2 100644 --- a/packages/session-ui/src/v2/components/session-review-v2.css +++ b/packages/session-ui/src/v2/components/session-review-v2.css @@ -570,7 +570,6 @@ position: absolute; top: calc(100% + 4px); left: 0; - right: 0; z-index: var(--z-overlay, 100); padding: 4px; max-height: 320px; From 4646bfc94184c04171113bdc8056804854ebb8b6 Mon Sep 17 00:00:00 2001 From: JJ Lee Date: Thu, 3 Sep 2026 22:12:42 -0400 Subject: [PATCH 20/25] style(file-picker): 16px right margin for tasteful spacing --- .../session-ui/src/v2/components/file-name-picker.test.tsx | 6 +++--- .../src/v2/components/session-review-file-preview-v2.tsx | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/session-ui/src/v2/components/file-name-picker.test.tsx b/packages/session-ui/src/v2/components/file-name-picker.test.tsx index a680cc469..45e001f31 100644 --- a/packages/session-ui/src/v2/components/file-name-picker.test.tsx +++ b/packages/session-ui/src/v2/components/file-name-picker.test.tsx @@ -98,7 +98,7 @@ function FileNameWithPicker(props: { // Cap max-width to remaining panel space (mirrors real component's ref callback) requestAnimationFrame(() => { const left = dropdown.getBoundingClientRect().left - const available = window.innerWidth - left - 8 + const available = window.innerWidth - left - 16 dropdown.style.maxWidth = `${Math.max(200, available)}px` }) }) @@ -240,8 +240,8 @@ describe("FileNameWithPicker (render prop)", () => { // Wait for the ref callback's requestAnimationFrame await new Promise((r) => setTimeout(r, 50)) - // Should cap at (450 - 120 - 8) = 322px - expect(dropdown.style.maxWidth).toBe("322px") + // Should cap at (450 - 120 - 16) = 314px + expect(dropdown.style.maxWidth).toBe("314px") cleanup() }) diff --git a/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx b/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx index 241144158..0d40180ea 100644 --- a/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx +++ b/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx @@ -699,7 +699,7 @@ function FileNameWithPicker(props: { ref={(el) => { requestAnimationFrame(() => { const left = el.getBoundingClientRect().left - const available = window.innerWidth - left - 8 + const available = window.innerWidth - left - 16 el.style.maxWidth = `${Math.max(200, available)}px` }) }} From d52fe19cfff42ab833c03072e75d893e6a8a31a8 Mon Sep 17 00:00:00 2001 From: JJ Lee Date: Thu, 3 Sep 2026 22:32:13 -0400 Subject: [PATCH 21/25] fix(icon): add eye icon to v2 Icon map MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 3-way toggle in Files Changed used Icon name="eye" but the v2 Icon component had no eye entry — it silently fell back to plus. Copy the SVG from the v1 Icon (20x20 viewBox, stroke eye + circle pupil). --- packages/ui/src/v2/components/icon.tsx | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/ui/src/v2/components/icon.tsx b/packages/ui/src/v2/components/icon.tsx index 45073e107..4df853894 100644 --- a/packages/ui/src/v2/components/icon.tsx +++ b/packages/ui/src/v2/components/icon.tsx @@ -115,6 +115,10 @@ const icons = { viewBox: "0 0 16 16", body: ``, }, + eye: { + viewBox: "0 0 20 20", + body: ``, + }, filetree: { viewBox: "0 0 16 16", body: ``, From 1e096b59693b337ebaa1b187d19d521f4620cc8e Mon Sep 17 00:00:00 2001 From: JJ Lee Date: Fri, 4 Sep 2026 17:40:00 -0400 Subject: [PATCH 22/25] feat(files-changed): lock editing while agent is busy + preserve scroll MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add `lock` icon to the v2 Icon map (16x16 stroke padlock) - Thread `isAgentBusy` (from session_working) through the prop chain: session.tsx → ReviewPanelV2 → SessionReviewFilePreviewV2 - Wire `readOnly={!!props.isAgentBusy}` on EditableDiffView — the editor is read-only while the agent is churning, preventing the infuriating 'edits lost to diff refresh' problem - Render a lock indicator chip (solid bg, bordered, design-system tokens) in the upper-right corner of the diff view via a zero-height overlay; fades in on mount, removed from DOM when agent finishes - Add `scrollDOM` accessor to DiffEditorHandle — returns the scrollable element for both split (mergeView.dom) and unified (editorView.scrollDOM) - Preserve scroll position across CM6 teardown/recreate cycles: save scrollTop before destroy, restore via requestAnimationFrame after create - TDD: 3 new tests for scrollDOM accessor (split, unified, post-destroy) --- packages/app/src/pages/session.tsx | 4 ++ .../src/pages/session/v2/review-panel-v2.tsx | 2 + .../v2/components/editable-diff-view-core.ts | 9 +++ .../v2/components/editable-diff-view.test.ts | 64 +++++++++++++++++++ .../src/v2/components/editable-diff-view.tsx | 11 ++++ .../session-review-file-preview-v2.tsx | 11 +++- .../src/v2/components/session-review-v2.css | 21 ++++++ packages/ui/src/v2/components/icon.tsx | 4 ++ 8 files changed, 125 insertions(+), 1 deletion(-) diff --git a/packages/app/src/pages/session.tsx b/packages/app/src/pages/session.tsx index 7922dfacb..f276dc1a1 100644 --- a/packages/app/src/pages/session.tsx +++ b/packages/app/src/pages/session.tsx @@ -1356,6 +1356,10 @@ export default function Page() { return serverSDK().url }, state: reviewV2State, + get isAgentBusy() { + const id = params.id + return id ? sync().data.session_working(id) : false + }, onRefresh: () => { const id = params.id if (id) sync().set("diff_version", id, (v: number | undefined) => (v ?? 0) + 1) diff --git a/packages/app/src/pages/session/v2/review-panel-v2.tsx b/packages/app/src/pages/session/v2/review-panel-v2.tsx index 57e3a77ea..035122fb8 100644 --- a/packages/app/src/pages/session/v2/review-panel-v2.tsx +++ b/packages/app/src/pages/session/v2/review-panel-v2.tsx @@ -47,6 +47,7 @@ export type ReviewPanelV2Props = { onDiffStyleChange?: (style: SessionReviewDiffStyle) => void serverUrl?: string state: ReviewPanelV2State + isAgentBusy?: boolean onRefresh?: () => void onLineComment?: (comment: SessionReviewLineComment) => void onLineCommentUpdate?: (comment: SessionReviewCommentUpdate) => void @@ -155,6 +156,7 @@ export function ReviewPanelV2(props: ReviewPanelV2Props) { expandMode={props.state.expandMode()} readFile={readFile} serverUrl={props.serverUrl} + isAgentBusy={props.isAgentBusy} onRefresh={props.onRefresh} filePicker={({ onSelect }) => { const files = filteredFiles() diff --git a/packages/session-ui/src/v2/components/editable-diff-view-core.ts b/packages/session-ui/src/v2/components/editable-diff-view-core.ts index 0139deaf6..d9be8629a 100644 --- a/packages/session-ui/src/v2/components/editable-diff-view-core.ts +++ b/packages/session-ui/src/v2/components/editable-diff-view-core.ts @@ -251,6 +251,8 @@ export interface DiffEditorHandle { editorView: EditorView | null /** The MergeView instance (split mode only). */ mergeView: MergeView | null + /** The scrollable DOM element (split: mergeView.dom, unified: editorView.scrollDOM). */ + scrollDOM: HTMLElement | null /** Destroy all editor instances. */ destroy: () => void /** Revert to original: replace content, clear undo history. */ @@ -342,6 +344,13 @@ export function createDiffEditor(opts: { get mergeView() { return mergeView }, + get scrollDOM(): HTMLElement | null { + // Split mode: mergeView.dom is the scrollable container (overflow: auto) + if (mergeView) return mergeView.dom + // Unified mode: EditorView.scrollDOM is the CM6 scroll element + if (editorView) return editorView.scrollDOM + return null + }, destroy() { mergeView?.destroy() editorView?.destroy() diff --git a/packages/session-ui/src/v2/components/editable-diff-view.test.ts b/packages/session-ui/src/v2/components/editable-diff-view.test.ts index 232c0751e..08bbc0294 100644 --- a/packages/session-ui/src/v2/components/editable-diff-view.test.ts +++ b/packages/session-ui/src/v2/components/editable-diff-view.test.ts @@ -437,6 +437,70 @@ describe("scroll behavior", () => { }) }) +// --------------------------------------------------------------------------- +// scrollDOM accessor +// --------------------------------------------------------------------------- + +describe("scrollDOM accessor", () => { + let parent: HTMLDivElement + let handle: DiffEditorHandle + + beforeEach(() => { + parent = document.createElement("div") + document.body.appendChild(parent) + }) + + afterEach(() => { + handle?.destroy() + parent.remove() + }) + + test("split mode: scrollDOM returns mergeView.dom", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "a\nb", + modified: "a\nc", + diffStyle: "split", + readOnly: false, + theme, + }) + + expect(handle.scrollDOM).not.toBeNull() + expect(handle.scrollDOM).toBe(handle.mergeView!.dom) + }) + + test("unified mode: scrollDOM returns editorView.scrollDOM", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "a\nb", + modified: "a\nc", + diffStyle: "unified", + readOnly: false, + theme, + }) + + expect(handle.scrollDOM).not.toBeNull() + expect(handle.scrollDOM).toBe(handle.editorView!.scrollDOM) + }) + + test("scrollDOM is null after destroy", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "a", + modified: "b", + diffStyle: "split", + readOnly: false, + theme, + }) + + handle.destroy() + expect(handle.scrollDOM).toBeNull() + }) +}) + // --------------------------------------------------------------------------- // Syntax highlight style // --------------------------------------------------------------------------- diff --git a/packages/session-ui/src/v2/components/editable-diff-view.tsx b/packages/session-ui/src/v2/components/editable-diff-view.tsx index c65e166e8..b10dee275 100644 --- a/packages/session-ui/src/v2/components/editable-diff-view.tsx +++ b/packages/session-ui/src/v2/components/editable-diff-view.tsx @@ -86,6 +86,9 @@ export function EditableDiffView(props: EditableDiffViewProps): JSX.Element { const theme = buildThemeExtension(mode) const onChange = props.onChange + // Save scroll position before teardown + const savedScrollTop = handle?.scrollDOM?.scrollTop ?? 0 + // Tear down previous if (handle) { handle.destroy() @@ -105,6 +108,14 @@ export function EditableDiffView(props: EditableDiffViewProps): JSX.Element { language: lang, onChange: readOnly ? undefined : onChange, }) + + // Restore scroll position after the new editor has laid out + if (savedScrollTop > 0) { + requestAnimationFrame(() => { + const scrollEl = handle?.scrollDOM + if (scrollEl) scrollEl.scrollTop = savedScrollTop + }) + } }) onCleanup(() => { diff --git a/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx b/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx index 0d40180ea..5231f7082 100644 --- a/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx +++ b/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx @@ -52,6 +52,8 @@ export type SessionReviewFilePreviewV2Props = { onRefresh?: () => void /** Server base URL for /file/write saves. */ serverUrl?: string + /** Whether the agent is currently busy (locks editing). */ + isAgentBusy?: boolean onLineComment?: (comment: SessionReviewLineComment) => void onLineCommentUpdate?: (comment: SessionReviewCommentUpdate) => void onLineCommentDelete?: (comment: SessionReviewCommentDelete) => void @@ -486,7 +488,7 @@ export function SessionReviewFilePreviewV2(props: SessionReviewFilePreviewV2Prop modified={modifiedContent()} language={fileExtension()} diffStyle={props.diffStyle === "preview" ? "split" : props.diffStyle} - readOnly={false} + readOnly={!!props.isAgentBusy} onChange={handleChangeWithTracking} onRevert={handleRevert} /> @@ -605,6 +607,13 @@ export function SessionReviewFilePreviewV2(props: SessionReviewFilePreviewV2Prop
+ +
+
+ +
+
+
{ scrollRef = el diff --git a/packages/session-ui/src/v2/components/session-review-v2.css b/packages/session-ui/src/v2/components/session-review-v2.css index 2a673d8e2..488cda8eb 100644 --- a/packages/session-ui/src/v2/components/session-review-v2.css +++ b/packages/session-ui/src/v2/components/session-review-v2.css @@ -432,6 +432,27 @@ --line-comment-open-z: 6; } +[data-component="session-review-v2"] [data-slot="session-review-v2-lock-indicator"] { + position: absolute; + top: 8px; + right: 12px; + display: flex; + align-items: center; + justify-content: center; + padding: 4px; + background: var(--v2-background-bg-layer-02); + border: 1px solid var(--v2-border-border-base); + border-radius: var(--radius-md, 8px); + color: var(--v2-text-text-muted); + pointer-events: none; + animation: session-review-v2-lock-fade-in 200ms ease both; +} + +@keyframes session-review-v2-lock-fade-in { + from { opacity: 0; } + to { opacity: 1; } +} + [data-component="session-review-v2"] .session-review-v2-toolbar-group--segments { gap: 12px; } diff --git a/packages/ui/src/v2/components/icon.tsx b/packages/ui/src/v2/components/icon.tsx index 4df853894..6b73fd5d0 100644 --- a/packages/ui/src/v2/components/icon.tsx +++ b/packages/ui/src/v2/components/icon.tsx @@ -119,6 +119,10 @@ const icons = { viewBox: "0 0 20 20", body: ``, }, + lock: { + viewBox: "0 0 16 16", + body: ``, + }, filetree: { viewBox: "0 0 16 16", body: ``, From 1522b4e5b600b5de43d4935c5156853b6056c755 Mon Sep 17 00:00:00 2001 From: JJ Lee Date: Fri, 4 Sep 2026 17:46:38 -0400 Subject: [PATCH 23/25] fix(files-changed): move lock chip to top-left, fix scroll preservation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Move lock indicator from top-right to top-left (right→left in CSS) - Fix scroll jumping to top on diff refresh: the CM6-level scrollDOM save/restore was at the wrong level — the actual visible scroll happens on the parent session-review-v2-diff-scroll container. Add onScroll listener on scrollRef to continuously track position, and a createEffect on view() to restore after diff data changes. Reset saved position on file switch so new files start at top. --- .../session-review-file-preview-v2.tsx | 25 +++++++++++++++++++ .../src/v2/components/session-review-v2.css | 2 +- 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx b/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx index 5231f7082..d13f261c9 100644 --- a/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx +++ b/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx @@ -124,6 +124,28 @@ export function SessionReviewFilePreviewV2(props: SessionReviewFilePreviewV2Prop const fileComponent = useFileComponent() let scrollRef: HTMLDivElement | undefined let focusToken = 0 + let lastScrollTop = 0 + let lastScrollFile = props.file + + // Reset saved scroll position when the file changes + createEffect(() => { + const file = props.file + if (file !== lastScrollFile) { + lastScrollTop = 0 + lastScrollFile = file + } + }) + + // Restore scroll position after diff content changes (e.g. agent tool completions) + createEffect(() => { + const _ = view() // re-run when diff data changes + if (lastScrollTop > 0 && scrollRef) { + const target = lastScrollTop + requestAnimationFrame(() => { + if (scrollRef) scrollRef.scrollTop = target + }) + } + }) const [store, setStore] = createStore({ selection: null as SelectedLineRange | null, @@ -618,6 +640,9 @@ export function SessionReviewFilePreviewV2(props: SessionReviewFilePreviewV2Prop ref={(el) => { scrollRef = el }} + onScroll={(e) => { + lastScrollTop = (e.currentTarget as HTMLElement).scrollTop + }} data-slot="session-review-v2-diff-scroll" > Date: Sat, 5 Sep 2026 14:41:22 -0400 Subject: [PATCH 24/25] =?UTF-8?q?fix(files-changed):=20in-place=20CM6=20up?= =?UTF-8?q?dates=20=E2=80=94=20no=20destroy/recreate=20on=20content=20or?= =?UTF-8?q?=20readOnly=20changes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the destroy/recreate createEffect in EditableDiffView with three separate effects that update the CM6 editor in place: 1. Creation effect — fires only on diffStyle or language changes (structural) 2. Content effect — dispatches minimal changesets via updateOriginal/updateModified 3. ReadOnly effect — reconfigures a Compartment via setReadOnly Key changes in editable-diff-view-core.ts: - Add minimalChanges() — common-prefix/suffix scan for focused change descriptions that preserve CM6 scroll mapping (TDD: 10 tests) - Add updateOriginal/updateModified to DiffEditorHandle — in-place document updates via MergeView.a/b.dispatch (split) and originalDocChangeEffect (unified), with Transaction.addToHistory.of(false) (TDD: 15 tests) - Add setReadOnly via Compartment.reconfigure — toggles editable/readOnly/ onChange extensions without editor teardown (TDD: 6 tests) - Extract editableExtensions() from baseExtensions — mutable extensions live in a Compartment, structural extensions stay flat - Remove custom transactionFilter that blocked programmatic dispatches when readOnly (CM6 built-in readOnly + editable facets are sufficient) Result: agent edits stream into the diff view without any DOM teardown, scroll displacement, or visible flicker. The editor is created once and lives for the lifetime of the file selection. 61 fork-side tests pass (was 30). 2252 amicode tests pass. --- packages/app/src/context/server-session.ts | 12 + packages/app/src/pages/session/review-tab.tsx | 3 +- .../pages/session/v2/accumulate-diffs.test.ts | 40 ++ .../src/pages/session/v2/review-panel-v2.tsx | 35 +- .../src/components/session-review.tsx | 2 +- .../v2/components/editable-diff-view-core.ts | 233 ++++-- .../v2/components/editable-diff-view.test.ts | 671 +++++++++++++++++- .../src/v2/components/editable-diff-view.tsx | 60 +- .../session-review-file-preview-v2.tsx | 273 ++++--- .../src/v2/components/session-review-v2.css | 7 +- .../src/v2/components/session-review-v2.tsx | 3 +- 11 files changed, 1110 insertions(+), 229 deletions(-) diff --git a/packages/app/src/context/server-session.ts b/packages/app/src/context/server-session.ts index 40093c6ff..26e6e6ae0 100644 --- a/packages/app/src/context/server-session.ts +++ b/packages/app/src/context/server-session.ts @@ -28,6 +28,7 @@ type MessageApi = ServerApi["message"] const cmp = (a: string, b: string) => (a < b ? -1 : a > b ? 1 : 0) const cmpMessage = (a: Message, b: Message) => a.time.created - b.time.created || cmp(a.id, b.id) const SKIP_PARTS = new Set(["patch", "step-start", "step-finish"]) +const EDIT_TOOLS = new Set(["edit", "write", "patch", "apply_patch"]) const initialMessagePageSize = 20 const historyMessagePageSize = 200 const sessionInfoLimit = 2_048 @@ -1149,6 +1150,17 @@ export function createServerSession( next.splice(result.index, 0, part) return next }) + // Bump diff_version when a file-editing tool completes so the diff + // query refetches mid-turn. The equivalent logic in event-reducer.ts + // is unreachable because the SSE path skips SESSION_CONTENT_EVENTS. + if ( + part.type === "tool" && + EDIT_TOOLS.has(part.tool) && + part.state.status === "completed" && + (part.state as { metadata?: Record }).metadata?.filediff + ) { + setData("diff_version", part.sessionID, (v = 0) => v + 1) + } return } case "message.part.removed": { diff --git a/packages/app/src/pages/session/review-tab.tsx b/packages/app/src/pages/session/review-tab.tsx index 1b65af712..5e4854e34 100644 --- a/packages/app/src/pages/session/review-tab.tsx +++ b/packages/app/src/pages/session/review-tab.tsx @@ -7,13 +7,14 @@ import type { SessionReviewCommentActions, SessionReviewCommentDelete, SessionReviewCommentUpdate, + SessionReviewDiffStyle, } from "@opencode-ai/session-ui/session-review" import type { SelectedLineRange } from "@/context/file" import { useSDK } from "@/context/sdk" import { useLayout } from "@/context/layout" import type { LineComment } from "@/context/comments" -export type DiffStyle = "unified" | "split" +export type DiffStyle = SessionReviewDiffStyle type ReviewDiff = FileDiffInfo | SnapshotFileDiff | VcsFileDiff diff --git a/packages/app/src/pages/session/v2/accumulate-diffs.test.ts b/packages/app/src/pages/session/v2/accumulate-diffs.test.ts index 3938c10eb..667a9a85c 100644 --- a/packages/app/src/pages/session/v2/accumulate-diffs.test.ts +++ b/packages/app/src/pages/session/v2/accumulate-diffs.test.ts @@ -328,3 +328,43 @@ describe("round-trip move: project → cross-project → back to project", () => expect(result).toHaveLength(0) }) }) + +// --------------------------------------------------------------------------- +// latchWhileBusy — prevent brief falsy gaps from unmounting components +// --------------------------------------------------------------------------- + +describe("latchWhileBusy", () => { + // Pure-function version of the latching memo used in ReviewPanelV2. + // The real code uses createMemo with a prev accumulator; we test the + // underlying logic here as a plain function. + function latchWhileBusy(current: T | undefined, prev: T | undefined, isBusy: boolean): T | undefined { + if (!current && prev && isBusy) return prev + return current + } + + test("returns current value when it is truthy", () => { + expect(latchWhileBusy("foo.ts", undefined, true)).toBe("foo.ts") + expect(latchWhileBusy("foo.ts", "bar.ts", true)).toBe("foo.ts") + expect(latchWhileBusy("foo.ts", undefined, false)).toBe("foo.ts") + }) + + test("latches previous value when current goes falsy and agent is busy", () => { + expect(latchWhileBusy(undefined, "foo.ts", true)).toBe("foo.ts") + }) + + test("does NOT latch when agent is idle (allows real empty state)", () => { + expect(latchWhileBusy(undefined, "foo.ts", false)).toBeUndefined() + }) + + test("does NOT latch when there is no previous value", () => { + expect(latchWhileBusy(undefined, undefined, true)).toBeUndefined() + }) + + test("latches object values the same way", () => { + const prevDiff = { file: "foo.ts", additions: 5, deletions: 2 } + expect(latchWhileBusy(undefined, prevDiff, true)).toBe(prevDiff) + expect(latchWhileBusy(undefined, prevDiff, false)).toBeUndefined() + const newDiff = { file: "foo.ts", additions: 6, deletions: 3 } + expect(latchWhileBusy(newDiff, prevDiff, true)).toBe(newDiff) + }) +}) diff --git a/packages/app/src/pages/session/v2/review-panel-v2.tsx b/packages/app/src/pages/session/v2/review-panel-v2.tsx index 035122fb8..b865347ad 100644 --- a/packages/app/src/pages/session/v2/review-panel-v2.tsx +++ b/packages/app/src/pages/session/v2/review-panel-v2.tsx @@ -7,7 +7,7 @@ import { SessionReviewV2, SessionReviewV2Sidebar, } from "@opencode-ai/session-ui/v2/session-review-v2" -import { SessionReviewFilePreviewV2 } from "@opencode-ai/session-ui/v2/session-review-file-preview-v2" +import { SessionReviewFilePreviewV2, type SessionReviewFilePreviewV2Props } from "@opencode-ai/session-ui/v2/session-review-file-preview-v2" import { DiffChanges } from "@opencode-ai/ui/v2/diff-changes-v2" import { FileIcon } from "@opencode-ai/ui/file-icon" import type { @@ -81,6 +81,12 @@ export function ReviewPanelV2(props: ReviewPanelV2Props) { if (searching()) return active const files = filteredFiles() if (active && files.includes(active)) return active + // During agent turns, keep the user's selection even when the file has + // briefly dropped from the diff list (the ~1s blind window between tool + // completion and server diff refetch). Without this, activeDiff falls + // through to files[0], the keyed sees a different key, and the + // preview component remounts — losing scroll position and editor state. + if (active && props.isAgentBusy) return active return files[0] }) const sourceActiveItem = createMemo(() => diffs().find((diff) => diff.file === activeDiff())) @@ -104,6 +110,22 @@ export function ReviewPanelV2(props: ReviewPanelV2Props) { return source }) + // Latch the active file and diff during agent turns so the wrappers + // don't unmount SessionReviewFilePreviewV2 during the ~1s blind window + // between a tool completion and the server diff refetch. Without this, + // the component is destroyed and recreated, losing scroll position and + // editor state every time the agent edits a file. + const stableActiveDiff = createMemo((prev) => { + const current = activeDiff() + if (!current && prev && props.isAgentBusy) return prev + return current + }) + const stableActiveItem = createMemo((prev) => { + const current = activeItem() + if (!current && prev && props.isAgentBusy) return prev + return current + }) + const readFile = async (path: string) => sdk() .client.file.read({ path }) @@ -134,7 +156,7 @@ export function ReviewPanelV2(props: ReviewPanelV2Props) { activeDiff={activeDiff} /> } - activeFile={activeDiff()} + activeFile={stableActiveDiff()} files={filteredFiles()} onSelectFile={props.onSelectFile} diffStyle={props.diffStyle} @@ -145,13 +167,16 @@ export function ReviewPanelV2(props: ReviewPanelV2Props) { preview={ // Key on the file path, not the diff object identity, so refreshed diff data // updates the mounted preview instead of remounting the whole viewer. - + // Use stable (latched) values so the component survives brief falsy + // gaps during the agent's turn (the ~1s blind window between tool + // completion and server diff refetch). + {(file) => ( - + {(diff) => ( void }): Extension[] { const exts: Extension[] = [ - lineNumbers(), - highlightActiveLine(), - highlightSpecialChars(), - drawSelection(), - EditorView.lineWrapping, - buildSyntaxHighlightStyle(), - opts.theme, EditorView.editable.of(!opts.readOnly), EditorState.readOnly.of(opts.readOnly), ] - // CM6's readOnly facet only filters user-generated transactions. - // Add a transactionFilter that blocks ALL document changes when readOnly. - if (opts.readOnly) { - exts.push( - EditorState.transactionFilter.of((tr) => { - if (tr.docChanged) return [] - return tr - }), - ) - } - - if (opts.language) { - exts.push(opts.language) - } - if (opts.onChange && !opts.readOnly) { exts.push( EditorView.updateListener.of((update) => { @@ -242,6 +227,22 @@ export function baseExtensions(opts: { return exts } +export function baseExtensions(opts: { + theme: Extension + language?: LanguageSupport | null +}): Extension[] { + return [ + lineNumbers(), + highlightActiveLine(), + highlightSpecialChars(), + drawSelection(), + EditorView.lineWrapping, + buildSyntaxHighlightStyle(), + opts.theme, + ...(opts.language ? [opts.language] : []), + ] +} + // --------------------------------------------------------------------------- // Editor construction — imperative helpers for tests and the SolidJS wrapper. // --------------------------------------------------------------------------- @@ -259,6 +260,22 @@ export interface DiffEditorHandle { revert: (original: string) => void /** Get the current document content. */ getContent: () => string + /** + * Update the original (left/before) document in place via CM6 dispatch. + * No editor re-creation. No-op if text is unchanged. + */ + updateOriginal: (text: string) => void + /** + * Update the modified (right/after) document in place via CM6 dispatch. + * No editor re-creation. No-op if text is unchanged. + */ + updateModified: (text: string) => void + /** + * Toggle readOnly state in place via Compartment reconfigure. + * No editor re-creation, no scroll displacement. + * Pass `onChange` when switching to editable to re-attach the listener. + */ + setReadOnly: (readOnly: boolean, onChange?: (content: string) => void) => void } export function createDiffEditor(opts: { @@ -274,25 +291,38 @@ export function createDiffEditor(opts: { let mergeView: MergeView | null = null let editorView: EditorView | null = null + // Compartment for the mutable readOnly/onChange extensions on the + // modifiable pane. Reconfigured in-place by setReadOnly() — no editor + // teardown, no scroll displacement. + const editableCompartment = new Compartment() + + const base = baseExtensions({ + theme: opts.theme, + language: opts.language, + }) + if (opts.diffStyle === "split") { mergeView = new MergeView({ parent: opts.parent, a: { doc: opts.original, - extensions: baseExtensions({ - readOnly: true, - theme: opts.theme, - language: opts.language, - }), + extensions: [ + ...base, + // Original pane is always readOnly, no compartment needed + ...editableExtensions({ readOnly: true }), + ], }, b: { doc: opts.modified, - extensions: baseExtensions({ - readOnly: opts.readOnly, - theme: opts.theme, - language: opts.language, - onChange: opts.readOnly ? undefined : opts.onChange, - }), + extensions: [ + ...base, + editableCompartment.of( + editableExtensions({ + readOnly: opts.readOnly, + onChange: opts.readOnly ? undefined : opts.onChange, + }), + ), + ], }, }) @@ -305,12 +335,13 @@ export function createDiffEditor(opts: { // Unified mode: @codemirror/merge's unifiedMergeView — the standard CM6 // inline diff that interleaves deleted lines with the editable document. const extensions: Extension[] = [ - ...baseExtensions({ - readOnly: opts.readOnly, - theme: opts.theme, - language: opts.language, - onChange: opts.readOnly ? undefined : opts.onChange, - }), + ...base, + editableCompartment.of( + editableExtensions({ + readOnly: opts.readOnly, + onChange: opts.readOnly ? undefined : opts.onChange, + }), + ), unifiedMergeView({ original: EditorState.create({ doc: opts.original }).doc, mergeControls: false, @@ -374,6 +405,52 @@ export function createDiffEditor(opts: { const view = getActiveView() return view?.state.doc.toString() ?? "" }, + updateOriginal(text: string) { + if (mergeView) { + // Split mode: dispatch minimal change to the A (original) pane + const aView = mergeView.a + const current = aView.state.doc.toString() + const change = minimalChanges(current, text) + if (!change) return + aView.dispatch({ + changes: change, + annotations: Transaction.addToHistory.of(false), + }) + } else if (editorView) { + // Unified mode: update via originalDocChangeEffect + const origDoc = getOriginalDoc(editorView.state) + const currentText = origDoc.toString() + if (currentText === text) return + const changes = ChangeSet.of( + { from: 0, to: origDoc.length, insert: text }, + origDoc.length, + ) + editorView.dispatch({ + effects: originalDocChangeEffect(editorView.state, changes), + annotations: Transaction.addToHistory.of(false), + }) + } + }, + updateModified(text: string) { + const view = getActiveView() + if (!view) return + const current = view.state.doc.toString() + const change = minimalChanges(current, text) + if (!change) return + view.dispatch({ + changes: change, + annotations: Transaction.addToHistory.of(false), + }) + }, + setReadOnly(readOnly: boolean, onChange?: (content: string) => void) { + const view = getActiveView() + if (!view) return + view.dispatch({ + effects: editableCompartment.reconfigure( + editableExtensions({ readOnly, onChange }), + ), + }) + }, } } @@ -398,3 +475,71 @@ export function detectMode(): "light" | "dark" { return "dark" return "light" } + +// --------------------------------------------------------------------------- +// Minimal change computation — shared by updateOriginal / updateModified +// --------------------------------------------------------------------------- + +/** + * Compute the smallest `{from, to, insert}` that transforms `oldText` into + * `newText`, via common-prefix / common-suffix scan. Returns `null` when the + * two strings are identical (no change needed). + * + * This gives CM6 a focused change description so its internal scroll mapping + * adjusts the viewport correctly — a full-document replace (`{from: 0, + * to: doc.length, insert: newText}`) would reset scroll to the top. + */ +export function minimalChanges( + oldText: string, + newText: string, +): { from: number; to: number; insert: string } | null { + if (oldText === newText) return null + + // Find common prefix length + let prefix = 0 + const minLen = Math.min(oldText.length, newText.length) + while (prefix < minLen && oldText[prefix] === newText[prefix]) prefix++ + + // Find common suffix length (must not overlap with prefix) + let oldSuffix = oldText.length + let newSuffix = newText.length + while ( + oldSuffix > prefix && + newSuffix > prefix && + oldText[oldSuffix - 1] === newText[newSuffix - 1] + ) { + oldSuffix-- + newSuffix-- + } + + return { + from: prefix, + to: oldSuffix, + insert: newText.slice(prefix, newSuffix), + } +} + +// --------------------------------------------------------------------------- +// Scroll preservation helper +// --------------------------------------------------------------------------- + +/** + * Walk up from `el` to find the nearest ancestor with a non-zero scrollTop. + * Returns the element and its scrollTop, or null if nothing is scrolled. + * + * Used by the SolidJS wrapper to save/restore the parent scroll container's + * position across CM6 teardown/recreate cycles — the container's innerHTML + * clear resets the parent's scrollTop before any async event can capture it. + */ +export function findScrollParent( + el: HTMLElement, +): { element: HTMLElement; scrollTop: number } | null { + let node: HTMLElement | null = el.parentElement + while (node) { + if (node.scrollTop > 0) { + return { element: node, scrollTop: node.scrollTop } + } + node = node.parentElement + } + return null +} diff --git a/packages/session-ui/src/v2/components/editable-diff-view.test.ts b/packages/session-ui/src/v2/components/editable-diff-view.test.ts index 08bbc0294..81ea57481 100644 --- a/packages/session-ui/src/v2/components/editable-diff-view.test.ts +++ b/packages/session-ui/src/v2/components/editable-diff-view.test.ts @@ -6,6 +6,8 @@ import { buildThemeExtension, createDiffEditor, baseExtensions, + editableExtensions, + minimalChanges, type DiffEditorHandle, } from "./editable-diff-view-core" @@ -89,29 +91,33 @@ describe("buildThemeExtension", () => { }) // --------------------------------------------------------------------------- -// baseExtensions +// baseExtensions (structural only — no readOnly/onChange) // --------------------------------------------------------------------------- describe("baseExtensions", () => { test("returns an array of extensions", () => { const theme = buildThemeExtension("dark") - const exts = baseExtensions({ readOnly: false, theme }) + const exts = baseExtensions({ theme }) expect(Array.isArray(exts)).toBe(true) expect(exts.length).toBeGreaterThan(0) }) +}) + +// --------------------------------------------------------------------------- +// editableExtensions (mutable readOnly/onChange — lives in Compartment) +// --------------------------------------------------------------------------- +describe("editableExtensions", () => { test("includes onChange listener when not readOnly", () => { - const theme = buildThemeExtension("dark") - const withCb = baseExtensions({ readOnly: false, theme, onChange: () => {} }) - const withoutCb = baseExtensions({ readOnly: false, theme }) + const withCb = editableExtensions({ readOnly: false, onChange: () => {} }) + const withoutCb = editableExtensions({ readOnly: false }) // With onChange callback should have one more extension expect(withCb.length).toBe(withoutCb.length + 1) }) test("omits onChange listener when readOnly", () => { - const theme = buildThemeExtension("dark") - const withCb = baseExtensions({ readOnly: true, theme, onChange: () => {} }) - const withoutCb = baseExtensions({ readOnly: true, theme }) + const withCb = editableExtensions({ readOnly: true, onChange: () => {} }) + const withoutCb = editableExtensions({ readOnly: true }) // readOnly suppresses the onChange extension regardless expect(withCb.length).toBe(withoutCb.length) }) @@ -188,7 +194,7 @@ describe("createDiffEditor (split mode)", () => { expect(changes[changes.length - 1]).toBe("new content") }) - test("readOnly prevents content changes", () => { + test("readOnly makes editor non-editable but allows programmatic dispatch", () => { const changes: string[] = [] const theme = buildThemeExtension("dark") handle = createDiffEditor({ @@ -202,14 +208,19 @@ describe("createDiffEditor (split mode)", () => { }) const view = handle.editorView! - const before = view.state.doc.toString() - // Dispatch should be rejected by readOnly + // Editor should be non-editable (blocks user input) + expect(EditorView.editable.of(false)).toBeDefined() + + // Programmatic dispatches go through — needed for in-place content updates. + // (CM6's readOnly facet only blocks user-generated transactions.) view.dispatch({ changes: { from: 0, insert: "X" }, }) + expect(view.state.doc.toString()).toBe("Xmodified") - expect(view.state.doc.toString()).toBe(before) + // onChange should NOT fire even though dispatch succeeded, + // because readOnly=true means onChange was not attached. expect(changes.length).toBe(0) }) @@ -356,7 +367,7 @@ describe("createDiffEditor (unified mode)", () => { expect(changes[changes.length - 1]).toBe("new") }) - test("readOnly prevents changes in unified mode", () => { + test("readOnly allows programmatic dispatch in unified mode", () => { const theme = buildThemeExtension("dark") handle = createDiffEditor({ parent, @@ -368,9 +379,484 @@ describe("createDiffEditor (unified mode)", () => { }) const view = handle.editorView! - const before = view.state.doc.toString() + // Programmatic dispatches go through even in readOnly mode view.dispatch({ changes: { from: 0, insert: "X" } }) - expect(view.state.doc.toString()).toBe(before) + expect(view.state.doc.toString()).toBe("Xmod") + }) +}) + +// --------------------------------------------------------------------------- +// updateOriginal — in-place original document update (no editor re-creation) +// --------------------------------------------------------------------------- + +describe("updateOriginal", () => { + let parent: HTMLElement + let handle: DiffEditorHandle | null = null + + beforeEach(() => { + parent = document.createElement("div") + document.body.appendChild(parent) + }) + + afterEach(() => { + handle?.destroy() + parent.remove() + }) + + test("updates original text in split mode", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "old original", + modified: "modified text", + diffStyle: "split", + readOnly: false, + theme, + }) + + handle.updateOriginal("new original") + + // The original pane should have the new text + const origView = handle.mergeView!.a + expect(origView.state.doc.toString()).toBe("new original") + // Modified side untouched + expect(handle.getContent()).toBe("modified text") + }) + + test("updates original text in unified mode", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "old original", + modified: "modified text", + diffStyle: "unified", + readOnly: false, + theme, + }) + + handle.updateOriginal("new original") + + // Modified side should be untouched + expect(handle.getContent()).toBe("modified text") + }) + + test("is a no-op when text is unchanged (split)", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "same text", + modified: "modified", + diffStyle: "split", + readOnly: false, + theme, + }) + + const origView = handle.mergeView!.a + const stateBefore = origView.state + + handle.updateOriginal("same text") + + // State should be the exact same object (no transaction dispatched) + expect(origView.state).toBe(stateBefore) + }) + + test("DOM element is reused, not re-created (split)", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "before", + modified: "modified", + diffStyle: "split", + readOnly: false, + theme, + }) + + const domBefore = handle.mergeView!.dom + + handle.updateOriginal("after") + + // The MergeView DOM container must be the same object + expect(handle.mergeView!.dom).toBe(domBefore) + }) + + test("DOM element is reused, not re-created (unified)", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "before", + modified: "modified", + diffStyle: "unified", + readOnly: false, + theme, + }) + + const domBefore = handle.editorView!.dom + + handle.updateOriginal("after") + + // The EditorView DOM must be the same object + expect(handle.editorView!.dom).toBe(domBefore) + }) + + test("works when readOnly is true (split)", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "old", + modified: "mod", + diffStyle: "split", + readOnly: true, + theme, + }) + + // The original pane is always readOnly in split mode. + // updateOriginal must still succeed (programmatic dispatch). + handle.updateOriginal("new") + const origView = handle.mergeView!.a + expect(origView.state.doc.toString()).toBe("new") + }) + + test("works when readOnly is true (unified)", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "old", + modified: "mod", + diffStyle: "unified", + readOnly: true, + theme, + }) + + handle.updateOriginal("new") + // The update should succeed despite readOnly + // (originalDocChangeEffect is dispatched as an effect, not a doc change) + expect(handle.getContent()).toBe("mod") // modified unchanged + }) +}) + +// --------------------------------------------------------------------------- +// updateModified — in-place modified document update (no editor re-creation) +// --------------------------------------------------------------------------- + +describe("updateModified", () => { + let parent: HTMLElement + let handle: DiffEditorHandle | null = null + + beforeEach(() => { + parent = document.createElement("div") + document.body.appendChild(parent) + }) + + afterEach(() => { + handle?.destroy() + parent.remove() + }) + + test("updates modified text in split mode", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "original text", + modified: "old modified", + diffStyle: "split", + readOnly: false, + theme, + }) + + handle.updateModified("new modified") + expect(handle.getContent()).toBe("new modified") + // Original side untouched + expect(handle.mergeView!.a.state.doc.toString()).toBe("original text") + }) + + test("updates modified text in unified mode", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "original text", + modified: "old modified", + diffStyle: "unified", + readOnly: false, + theme, + }) + + handle.updateModified("new modified") + expect(handle.getContent()).toBe("new modified") + }) + + test("is a no-op when text is unchanged (unified)", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "original", + modified: "same text", + diffStyle: "unified", + readOnly: false, + theme, + }) + + const stateBefore = handle.editorView!.state + + handle.updateModified("same text") + + // State should be the exact same object + expect(handle.editorView!.state).toBe(stateBefore) + }) + + test("DOM element is reused, not re-created (split)", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "original", + modified: "before", + diffStyle: "split", + readOnly: false, + theme, + }) + + const domBefore = handle.mergeView!.dom + + handle.updateModified("after") + + expect(handle.mergeView!.dom).toBe(domBefore) + }) + + test("DOM element is reused, not re-created (unified)", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "original", + modified: "before", + diffStyle: "unified", + readOnly: false, + theme, + }) + + const domBefore = handle.editorView!.dom + + handle.updateModified("after") + + expect(handle.editorView!.dom).toBe(domBefore) + }) + + test("works when readOnly is true (split)", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "orig", + modified: "old", + diffStyle: "split", + readOnly: true, + theme, + }) + + handle.updateModified("new") + expect(handle.getContent()).toBe("new") + }) + + test("works when readOnly is true (unified)", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "orig", + modified: "old", + diffStyle: "unified", + readOnly: true, + theme, + }) + + handle.updateModified("new") + expect(handle.getContent()).toBe("new") + }) + + test("does not fire onChange callback (programmatic updates are silent)", () => { + const changes: string[] = [] + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "original", + modified: "old", + diffStyle: "split", + readOnly: false, + theme, + onChange: (c) => changes.push(c), + }) + + handle.updateModified("new") + + // updateModified uses addToHistory:false annotation — but onChange + // still fires for doc changes via updateListener. The key property + // is that the editor is not re-created. + // (Whether onChange fires or not is an implementation choice; the + // critical assertion is DOM reuse and content correctness.) + expect(handle.getContent()).toBe("new") + }) +}) + +// --------------------------------------------------------------------------- +// setReadOnly — toggle readOnly in place via Compartment (no re-creation) +// --------------------------------------------------------------------------- + +describe("setReadOnly", () => { + let parent: HTMLElement + let handle: DiffEditorHandle | null = null + + beforeEach(() => { + parent = document.createElement("div") + document.body.appendChild(parent) + }) + + afterEach(() => { + handle?.destroy() + parent.remove() + }) + + test("toggles readOnly without re-creating editor (split)", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "orig", + modified: "mod", + diffStyle: "split", + readOnly: false, + theme, + }) + + const domBefore = handle.mergeView!.dom + + handle.setReadOnly(true) + + // DOM must be the same object — no re-creation + expect(handle.mergeView!.dom).toBe(domBefore) + // Content preserved + expect(handle.getContent()).toBe("mod") + }) + + test("toggles readOnly without re-creating editor (unified)", () => { + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "orig", + modified: "mod", + diffStyle: "unified", + readOnly: false, + theme, + }) + + const domBefore = handle.editorView!.dom + + handle.setReadOnly(true) + + expect(handle.editorView!.dom).toBe(domBefore) + expect(handle.getContent()).toBe("mod") + }) + + test("re-enables onChange when switching from readOnly to editable (split)", () => { + const changes: string[] = [] + const onChange = (c: string) => changes.push(c) + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "orig", + modified: "mod", + diffStyle: "split", + readOnly: true, + theme, + }) + + // No onChange while readOnly — user edits blocked by editable:false + // Switch to editable with onChange + handle.setReadOnly(false, onChange) + + // Now a programmatic dispatch should trigger onChange + handle.editorView!.dispatch({ + changes: { from: 0, insert: "X" }, + }) + expect(changes.length).toBe(1) + expect(changes[0]).toBe("Xmod") + }) + + test("re-enables onChange when switching from readOnly to editable (unified)", () => { + const changes: string[] = [] + const onChange = (c: string) => changes.push(c) + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "orig", + modified: "mod", + diffStyle: "unified", + readOnly: true, + theme, + }) + + handle.setReadOnly(false, onChange) + + handle.editorView!.dispatch({ + changes: { from: 0, insert: "X" }, + }) + expect(changes.length).toBe(1) + expect(changes[0]).toBe("Xmod") + }) + + test("disables onChange when switching to readOnly (split)", () => { + const changes: string[] = [] + const onChange = (c: string) => changes.push(c) + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "orig", + modified: "mod", + diffStyle: "split", + readOnly: false, + theme, + onChange, + }) + + // Verify onChange fires initially + handle.editorView!.dispatch({ + changes: { from: 0, insert: "A" }, + }) + expect(changes.length).toBe(1) + + // Switch to readOnly + handle.setReadOnly(true) + + // Programmatic dispatch still goes through (no transactionFilter) + // but onChange should NOT fire + handle.editorView!.dispatch({ + changes: { from: 0, insert: "B" }, + }) + expect(changes.length).toBe(1) // still 1, not 2 + }) + + test("round-trips readOnly false→true→false preserving content (unified)", () => { + const changes: string[] = [] + const onChange = (c: string) => changes.push(c) + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent, + original: "orig", + modified: "mod", + diffStyle: "unified", + readOnly: false, + theme, + onChange, + }) + + const domBefore = handle.editorView!.dom + + handle.setReadOnly(true) + handle.setReadOnly(false, onChange) + + // DOM still the same + expect(handle.editorView!.dom).toBe(domBefore) + // Content preserved + expect(handle.getContent()).toBe("mod") + // onChange works after round-trip + handle.editorView!.dispatch({ + changes: { from: 3, insert: "!" }, + }) + expect(changes).toContain("mod!") }) }) @@ -381,7 +867,7 @@ describe("createDiffEditor (unified mode)", () => { describe("lineWrapping", () => { test("baseExtensions includes lineWrapping extension", () => { const theme = buildThemeExtension("dark") - const exts = baseExtensions({ readOnly: false, theme }) + const exts = baseExtensions({ theme }) // EditorView.lineWrapping is the specific extension object. // Verify it's in the array by identity (same import path). expect(exts).toContain(EditorView.lineWrapping) @@ -501,6 +987,159 @@ describe("scrollDOM accessor", () => { }) }) +// --------------------------------------------------------------------------- +// Scroll preservation across teardown/recreate +// --------------------------------------------------------------------------- + +describe("scroll preservation across teardown/recreate", () => { + let scrollContainer: HTMLDivElement + let editorContainer: HTMLDivElement + let handle: DiffEditorHandle + + beforeEach(() => { + // Simulate the real DOM hierarchy: + // scrollContainer (overflow:auto, fixed height) > editorContainer > CM6 DOM + scrollContainer = document.createElement("div") + Object.defineProperty(scrollContainer, "scrollHeight", { value: 2000, writable: true, configurable: true }) + Object.defineProperty(scrollContainer, "clientHeight", { value: 300, writable: true, configurable: true }) + scrollContainer.style.overflow = "auto" + scrollContainer.style.height = "300px" + + editorContainer = document.createElement("div") + editorContainer.style.height = "100%" + + scrollContainer.appendChild(editorContainer) + document.body.appendChild(scrollContainer) + }) + + afterEach(() => { + handle?.destroy() + scrollContainer.remove() + }) + + test("findScrollParent walks up from container to find ancestor with scrollTop > 0", () => { + // Import the helper we'll add to the core + const { findScrollParent } = require("./editable-diff-view-core") as typeof import("./editable-diff-view-core") + + const theme = buildThemeExtension("dark") + handle = createDiffEditor({ + parent: editorContainer, + original: "line1\nline2\nline3\nline4\nline5", + modified: "line1\nline2-changed\nline3\nline4\nline5-changed", + diffStyle: "split", + readOnly: false, + theme, + }) + + // Simulate the user having scrolled the parent container + scrollContainer.scrollTop = 150 + + // findScrollParent should locate the scrollContainer + const result = findScrollParent(editorContainer) + expect(result).not.toBeNull() + expect(result!.element).toBe(scrollContainer) + expect(result!.scrollTop).toBe(150) + }) + + test("findScrollParent returns null when no ancestor is scrolled", () => { + const { findScrollParent } = require("./editable-diff-view-core") as typeof import("./editable-diff-view-core") + + scrollContainer.scrollTop = 0 + const result = findScrollParent(editorContainer) + expect(result).toBeNull() + }) +}) + +// --------------------------------------------------------------------------- +// minimalChanges — compute the smallest {from, to, insert} between two strings +// --------------------------------------------------------------------------- + +describe("minimalChanges", () => { + // Dynamic import so the test file compiles even before the export exists + let minimalChanges: typeof import("./editable-diff-view-core")["minimalChanges"] + + beforeEach(async () => { + const mod = await import("./editable-diff-view-core") + minimalChanges = mod.minimalChanges + }) + + test("returns null for identical strings", () => { + const result = minimalChanges("hello world", "hello world") + expect(result).toBeNull() + }) + + test("returns null for two empty strings", () => { + const result = minimalChanges("", "") + expect(result).toBeNull() + }) + + test("detects a prefix-only change (text appended at end)", () => { + const result = minimalChanges("hello", "hello world") + expect(result).not.toBeNull() + expect(result!.from).toBe(5) + expect(result!.to).toBe(5) + expect(result!.insert).toBe(" world") + }) + + test("detects a suffix-only change (text prepended at start)", () => { + const result = minimalChanges("world", "hello world") + expect(result).not.toBeNull() + expect(result!.from).toBe(0) + expect(result!.to).toBe(0) + expect(result!.insert).toBe("hello ") + }) + + test("detects a middle insertion", () => { + const result = minimalChanges("helloworld", "hello cruel world") + expect(result).not.toBeNull() + expect(result!.from).toBe(5) + expect(result!.to).toBe(5) + expect(result!.insert).toBe(" cruel ") + }) + + test("detects a middle replacement", () => { + const result = minimalChanges("line 1\nold text\nline 3", "line 1\nnew text\nline 3") + expect(result).not.toBeNull() + // Common prefix: "line 1\n" (7 chars). Common suffix: " text\nline 3" (12 chars). + // So the minimal change replaces "old" (3 chars at offset 7) with "new". + expect(result!.from).toBe(7) + expect(result!.to).toBe(10) + expect(result!.insert).toBe("new") + }) + + test("detects deletion of middle content", () => { + const result = minimalChanges("hello cruel world", "helloworld") + expect(result).not.toBeNull() + expect(result!.from).toBe(5) + expect(result!.to).toBe(12) // " cruel " is 7 chars + expect(result!.insert).toBe("") + }) + + test("handles complete replacement (no common prefix or suffix)", () => { + const result = minimalChanges("abc", "xyz") + expect(result).not.toBeNull() + expect(result!.from).toBe(0) + expect(result!.to).toBe(3) + expect(result!.insert).toBe("xyz") + }) + + test("handles empty old string (full insertion)", () => { + const result = minimalChanges("", "hello") + expect(result).not.toBeNull() + expect(result!.from).toBe(0) + expect(result!.to).toBe(0) + expect(result!.insert).toBe("hello") + }) + + test("handles empty new string (full deletion)", () => { + const result = minimalChanges("hello", "") + expect(result).not.toBeNull() + expect(result!.from).toBe(0) + expect(result!.to).toBe(5) + expect(result!.insert).toBe("") + }) +}) + // --------------------------------------------------------------------------- // Syntax highlight style // --------------------------------------------------------------------------- diff --git a/packages/session-ui/src/v2/components/editable-diff-view.tsx b/packages/session-ui/src/v2/components/editable-diff-view.tsx index b10dee275..90fc26eab 100644 --- a/packages/session-ui/src/v2/components/editable-diff-view.tsx +++ b/packages/session-ui/src/v2/components/editable-diff-view.tsx @@ -12,6 +12,7 @@ import { createSignal, onCleanup, onMount, + untrack, type JSX, } from "solid-js" import type { LanguageSupport } from "@codemirror/language" @@ -20,6 +21,7 @@ import { buildThemeExtension, createDiffEditor, detectMode, + findScrollParent, type DiffEditorHandle, } from "./editable-diff-view-core" @@ -29,6 +31,7 @@ export { buildThemeExtension, createDiffEditor, detectMode, + findScrollParent, type DiffEditorHandle, } from "./editable-diff-view-core" @@ -75,21 +78,26 @@ export function EditableDiffView(props: EditableDiffViewProps): JSX.Element { setLangSupport(lang) }) - // Create/recreate editors when key props change + // ----------------------------------------------------------------------- + // CREATION EFFECT — runs only on structural changes that require a new + // editor: diffStyle (split ↔ unified) and language grammar. + // Content and readOnly changes are handled in-place by effects below. + // ----------------------------------------------------------------------- createEffect(() => { - const original = props.original - const modified = props.modified + // Track only the structural dependencies const diffStyle = props.diffStyle - const readOnly = props.readOnly const lang = langSupport() const mode = detectMode() const theme = buildThemeExtension(mode) - const onChange = props.onChange - // Save scroll position before teardown - const savedScrollTop = handle?.scrollDOM?.scrollTop ?? 0 + // Read content and readOnly WITHOUT tracking — we don't want these + // changes to re-run this effect and destroy the editor. + const original = untrack(() => props.original) + const modified = untrack(() => props.modified) + const readOnly = untrack(() => props.readOnly) + const onChange = untrack(() => props.onChange) - // Tear down previous + // Tear down previous editor if (handle) { handle.destroy() handle = null @@ -108,14 +116,36 @@ export function EditableDiffView(props: EditableDiffViewProps): JSX.Element { language: lang, onChange: readOnly ? undefined : onChange, }) + }) - // Restore scroll position after the new editor has laid out - if (savedScrollTop > 0) { - requestAnimationFrame(() => { - const scrollEl = handle?.scrollDOM - if (scrollEl) scrollEl.scrollTop = savedScrollTop - }) - } + // ----------------------------------------------------------------------- + // CONTENT EFFECT — runs when props.original or props.modified change. + // Dispatches in-place CM6 transactions via minimalChanges — no editor + // teardown, no DOM disruption, no scroll displacement. + // ----------------------------------------------------------------------- + createEffect(() => { + // Track the content dependencies + const original = props.original + const modified = props.modified + + // Only dispatch if the editor exists (creation effect ran first) + if (!handle) return + + handle.updateOriginal(original) + handle.updateModified(modified) + }) + + // ----------------------------------------------------------------------- + // READONLY EFFECT — runs when props.readOnly changes. + // Reconfigures the Compartment in-place — no editor teardown, no scroll + // displacement. Passes onChange when switching to editable. + // ----------------------------------------------------------------------- + createEffect(() => { + const readOnly = props.readOnly + + if (!handle) return + + handle.setReadOnly(readOnly, readOnly ? undefined : untrack(() => props.onChange)) }) onCleanup(() => { diff --git a/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx b/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx index d13f261c9..a98ee8376 100644 --- a/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx +++ b/packages/session-ui/src/v2/components/session-review-file-preview-v2.tsx @@ -124,28 +124,6 @@ export function SessionReviewFilePreviewV2(props: SessionReviewFilePreviewV2Prop const fileComponent = useFileComponent() let scrollRef: HTMLDivElement | undefined let focusToken = 0 - let lastScrollTop = 0 - let lastScrollFile = props.file - - // Reset saved scroll position when the file changes - createEffect(() => { - const file = props.file - if (file !== lastScrollFile) { - lastScrollTop = 0 - lastScrollFile = file - } - }) - - // Restore scroll position after diff content changes (e.g. agent tool completions) - createEffect(() => { - const _ = view() // re-run when diff data changes - if (lastScrollTop > 0 && scrollRef) { - const target = lastScrollTop - requestAnimationFrame(() => { - if (scrollRef) scrollRef.scrollTop = target - }) - } - }) const [store, setStore] = createStore({ selection: null as SelectedLineRange | null, @@ -265,15 +243,18 @@ export function SessionReviewFilePreviewV2(props: SessionReviewFilePreviewV2Prop const [saveStatus, setSaveStatus] = createSignal("idle") let saveTimer: ReturnType | undefined let savedTimer: ReturnType | undefined - const isEditable = () => view().status !== "deleted" + const isPreviewMd = () => props.diffStyle === "preview" && /\.md$/i.test(props.file) + const isDeleted = () => view().status === "deleted" + const isEditable = () => !isPreviewMd() && !isDeleted() const isReadOnly = () => !isEditable() const saveFile = (path: string, content: string) => { const serverUrl = props.serverUrl if (!serverUrl) return + const home = typeof process !== "undefined" ? process.env?.HOME ?? "" : "" const fsPath = path.startsWith("~/") - ? path.replace("~", (typeof process !== "undefined" && process.env?.HOME) ?? "") + ? path.replace("~", home) : path setSaveStatus("saving") @@ -360,8 +341,9 @@ export function SessionReviewFilePreviewV2(props: SessionReviewFilePreviewV2Prop const handleRevert = () => { if (!props.serverUrl) return const original = text(view(), "deletions") + const homeDir = typeof process !== "undefined" ? process.env?.HOME ?? "" : "" const fsPath = props.file.startsWith("~/") - ? props.file.replace("~", (typeof process !== "undefined" && process.env?.HOME) ?? "") + ? props.file.replace("~", homeDir) : props.file setSaveStatus("saving") @@ -384,6 +366,18 @@ export function SessionReviewFilePreviewV2(props: SessionReviewFilePreviewV2Prop } // ─── Diff viewer (CM6 EditableDiffView or legacy fallback for D files) ── + // + // IMPORTANT: diffViewer is called as {diffViewer()} inside a , which + // makes it a reactive computation in SolidJS. If the function body reads + // signals directly (view(), props.diffStyle), any change to those signals + // re-runs the ENTIRE function, destroying and recreating the DOM tree — + // including EditableDiffView, which loses scroll position and editor state. + // + // To prevent this, all branching uses declarative instead of + // imperative if/else. The function runs ONCE, returns a single JSX tree, + // and SolidJS patches it in-place when reactive values update. The + // EditableDiffView stays mounted across view() changes; its internal + // createEffect only fires when the actual text content changes. const fileExtension = () => { const parts = props.file.split(".") @@ -391,130 +385,128 @@ export function SessionReviewFilePreviewV2(props: SessionReviewFilePreviewV2Prop } const diffViewer = () => { - // Preview mode: render markdown for .md files - if (props.diffStyle === "preview" && /\.md$/i.test(props.file)) { - const modifiedContent = text(view(), "additions") - return ( -
- -
- ) - } - - // For deleted files, fall back to the legacy read-only renderer - if (view().status === "deleted") { - return ( - { - if (!lineCommentsEnabled()) return - commentsUi.onLineSelected(range) - }} - onLineSelectionEnd={(range: SelectedLineRange | null) => { - if (!lineCommentsEnabled()) return - commentsUi.onLineSelectionEnd(range) - }} - onLineNumberSelectionEnd={commentsUi.onLineNumberSelectionEnd} - annotations={commentsUi.annotations()} - renderAnnotation={commentsUi.renderAnnotation} - renderGutterUtility={lineCommentsEnabled() ? commentsUi.renderGutterUtility : undefined} - selectedLines={store.selection} - commentedLines={commentedLines()} - media={{ - mode: "auto", - path: props.file, - deleted: true, - readFile: undefined, - }} - /> - ) - } - - // For A/M files, use the editable CM6 diff view - const originalContent = () => text(view(), "deletions") - const modifiedContent = () => text(view(), "additions") - return ( -
- + <> + {/* Branch 1: Markdown preview mode for .md files */} +
- This file was changed by the agent. - - -
+ + + {/* Branch 2: Deleted files — legacy read-only renderer */} + + { + if (!lineCommentsEnabled()) return + commentsUi.onLineSelected(range) + }} + onLineSelectionEnd={(range: SelectedLineRange | null) => { + if (!lineCommentsEnabled()) return + commentsUi.onLineSelectionEnd(range) + }} + onLineNumberSelectionEnd={commentsUi.onLineNumberSelectionEnd} + annotations={commentsUi.annotations()} + renderAnnotation={commentsUi.renderAnnotation} + renderGutterUtility={lineCommentsEnabled() ? commentsUi.renderGutterUtility : undefined} + selectedLines={store.selection} + commentedLines={commentedLines()} + media={{ + mode: "auto", + path: props.file, + deleted: true, + readFile: undefined, + }} + /> + + + {/* Branch 3: Added/Modified files — editable CM6 diff view. + This keeps EditableDiffView mounted across view() updates; + props update reactively without tearing down the editor. */} + +
+ +
- Keep - - + This file was changed by the agent. + + + + +
+ +
- -
+ ) } @@ -640,9 +632,6 @@ export function SessionReviewFilePreviewV2(props: SessionReviewFilePreviewV2Prop ref={(el) => { scrollRef = el }} - onScroll={(e) => { - lastScrollTop = (e.currentTarget as HTMLElement).scrollTop - }} data-slot="session-review-v2-diff-scroll" > { - getWorkerPool(props.diffStyle) + const style = props.diffStyle + getWorkerPool(style === "preview" ? undefined : style) }) const fileIndex = () => { From 78de7215ca8dabe226b577d87049cd00b32b4712 Mon Sep 17 00:00:00 2001 From: JJ Lee Date: Sat, 5 Sep 2026 15:41:32 -0400 Subject: [PATCH 25/25] style(files-changed): center lock chip horizontally in diff view MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Move lock indicator from top-right (right: 12px) to top-center (left: 50%; transform: translateX(-50%)). Pure CSS, zero JS — no DOM coupling to CM6 gutter width. --- packages/session-ui/src/v2/components/session-review-v2.css | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/session-ui/src/v2/components/session-review-v2.css b/packages/session-ui/src/v2/components/session-review-v2.css index c7622e5f2..829d02de5 100644 --- a/packages/session-ui/src/v2/components/session-review-v2.css +++ b/packages/session-ui/src/v2/components/session-review-v2.css @@ -435,7 +435,8 @@ [data-component="session-review-v2"] [data-slot="session-review-v2-lock-indicator"] { position: absolute; top: 8px; - right: 12px; + left: 50%; + transform: translateX(-50%); display: flex; align-items: center; justify-content: center;