diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a3f2634..f5d7f27 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -52,14 +52,27 @@ jobs: - name: Stub the build inputs run: mkdir -p dist && touch dist/index.html - # Every target except `no_write_on_open`, which is skipped rather than run and failing. - # That suite checks the promise the whole project rests on, that the editor writes nothing - # the user did not edit, and it checks it against a real git repository rather than a - # TempDir. The repository is hand built at a hardcoded path under /tmp, so there is nothing - # here to create it and macOS reaps it after three days without access. It is not skipped - # because it is unimportant. It is skipped because it currently cannot run anywhere, and a - # CI that is permanently red says less than one that is green about a named subset. - # Making the fixture reproducible is tracked in the issue. + # Every target except `no_write_on_open`, which gets the step below to itself. - name: Test working-directory: src-tauri run: cargo test --lib --bins --test fs --test fts5 --test watch --test watch_payload + + # The suite that checks the promise the whole project rests on, that the editor writes + # nothing the user did not edit. It runs against a real git repository rather than against a + # TempDir, because the evidence it wants is a folder somebody could plausibly have opened + # coming back from `git status` with no lines in it, and a bag of files in a TempDir cannot + # say that. The repository used to be built by hand at a fixed path under /tmp, which meant + # the suite ran on one machine until macOS reaped the folder and then ran nowhere, and that + # is why it spent a while excluded from this file. It is now built from scratch by + # `tests/support/notes_repo.rs` on the first test that asks for it: the documents are copied + # out of the markdown corpus, the vendored node_modules is generated, and a git init and one + # commit turn the result into the oracle. Nothing has to exist on the runner beforehand + # except git itself, and the suite says so plainly if git is missing rather than passing on a + # folder nobody checked. + # + # It gets its own step because the tests share that one folder and several of them mutate it, + # so they have to run one at a time. Passing `--test-threads=1` to the command above would + # slow the other suites to the same pace for no reason. + - name: Test no_write_on_open + working-directory: src-tauri + run: cargo test --test no_write_on_open -- --test-threads=1 diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 1d28646..e1115bb 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -81,41 +81,16 @@ jobs: build: needs: prepare - strategy: - fail-fast: false - matrix: - include: - - os: macos-26 - args: "--target universal-apple-darwin --config src-tauri/tauri.release.conf.json" - rust-targets: "aarch64-apple-darwin,x86_64-apple-darwin" - # Ubuntu 22.04 is the glibc baseline: the bundle will not run on anything older than the - # glibc it was linked against, so build on the oldest supported. - - os: ubuntu-22.04 - args: "--config src-tauri/tauri.release.conf.json" - rust-targets: "" - runs-on: ${{ matrix.os }} + # One runner, no matrix. The app is macOS only, and a universal build links the aarch64 and + # x86_64 slices into a single bundle, so there is exactly one thing to build and nothing for a + # matrix to vary. A matrix of one is where the Linux row survived long after the app stopped + # shipping on Linux; if a second target ever arrives, putting the matrix back is a small change. + runs-on: macos-26 steps: - uses: actions/checkout@v7 with: ref: ${{ needs.prepare.outputs.tag }} - - name: Install Linux dependencies - if: startsWith(matrix.os, 'ubuntu') - run: | - sudo apt-get update - sudo apt-get install -y \ - libwebkit2gtk-4.1-dev \ - libgtk-3-dev \ - libayatana-appindicator3-dev \ - librsvg2-dev \ - patchelf \ - libxdo-dev \ - libssl-dev \ - build-essential \ - curl \ - wget \ - file - - uses: actions/setup-node@v6 with: node-version: 26 @@ -127,7 +102,7 @@ jobs: - name: Install Rust uses: dtolnay/rust-toolchain@stable with: - targets: ${{ matrix.rust-targets }} + targets: aarch64-apple-darwin,x86_64-apple-darwin - uses: swatinem/rust-cache@v2 with: @@ -144,7 +119,7 @@ jobs: TAURI_SIGNING_PRIVATE_KEY_PASSWORD: ${{ secrets.TAURI_SIGNING_PRIVATE_KEY_PASSWORD }} with: releaseId: ${{ needs.prepare.outputs.release_id }} - args: ${{ matrix.args }} + args: "--target universal-apple-darwin --config src-tauri/tauri.release.conf.json" publish: needs: [prepare, build] @@ -159,7 +134,14 @@ jobs: gh release download "$TAG" --repo "$REPO" --pattern latest.json --output latest.json --clobber echo "Platforms in latest.json:" jq '.platforms | keys' latest.json - for key in darwin-aarch64 darwin-x86_64 linux-x86_64; do + # A universal build emits one .app.tar.gz, but tauri-action writes it into latest.json + # under both darwin-aarch64 and darwin-x86_64, pointing them at the same file and the same + # signature. It has to: an installed copy asks the manifest for the architecture it is + # running on and never for the universal key, so a manifest that only carried + # darwin-universal would offer nobody an update. Those two keys are the whole manifest for + # this app, and a release that is missing either one is a release half the users cannot + # take. + for key in darwin-aarch64 darwin-x86_64; do if ! jq -e ".platforms[\"$key\"].url" latest.json > /dev/null; then echo "::error::latest.json is missing platform '$key': refusing to publish a partial update manifest. Re-run the release." exit 1 diff --git a/docs/architecture.md b/docs/architecture.md index f07885e..7d6237e 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -44,6 +44,16 @@ character typed into a hand wrapped file put a backslash at the end of every lin `prosemirror-transform` asks the same field before it joins two blocks, so Backspace between two hand wrapped paragraphs rewrote the wraps in both. +The heading needs all three for the same reason and was declared with none of them, which is the +same bug wearing the one hat markdown lets an author wrap by hand. A setext heading is two lines of +text over an underline, and mdast writes a break inside one as a backslash, so a character typed +into a two line heading put a backslash in the user's words exactly as it used to in a paragraph. +The first save cannot rescue it either, because a heading that has been wrapped has no ATX spelling +to be normalised into, so it stays setext and every later keystroke re-arms the bug. The field goes +on at every level rather than only the two that have an underline: the deeper four cannot be wrapped +in the file, but they can hold a line ending written ` `, and at the default a keystroke turned +that into a break the writer then swallowed as a space. + The paragraph's parse rule says the opposite on purpose, and a rule's own answer outranks the node's. Whitespace is significant in a paragraph this editor rendered, and meaningless in a `

` off somebody else's web page, where the line endings are that page's source indentation and keeping @@ -144,6 +154,16 @@ every construct markdown has except a list, which carries on over one and swallo so a list written next to preserved source is proved apart and respelled, with a wider item indent or the other bullet, until it is. +A rule inside a list item is the same question asked from the other side, and it is the one place +the house style has to give way rather than the seam. `---` is the house spelling and it is a setext +underline wherever a paragraph is still open above it, which inside a tight item is the only place +the line under the item's text can be. Reading that as "these two blocks cannot be written a line +apart" is correct about `---` and wrong about the rule, so the writer spread the whole list to make +room, and a tight list came back loose with its items wrapped in paragraphs they did not have. `***` +is the same rule in the one spelling that does interrupt a paragraph, the writer already reaches for +it when a rule lands on the first byte of a file, and a tight item now gets it. A loose item keeps +`---`, because the blank line in front of it has already closed the paragraph. + Those respellings cover every raw block a file can hand over, because a file's own raw block starts in the first three columns and column four is indented code. An edited one can be anything, and for some of them no spelling exists at all: four spaces typed into a raw block below a list makes bytes @@ -154,6 +174,15 @@ knowingly drops something the user typed, and it is the right way round: the alt the same file swallowed the list into the raw block and then moved bytes around inside somebody's html on the save after. A raw block with no list beside it is written exactly as it always was. +Dropping it is the decision; dropping it in silence was the bug. The writer had no way to say it had +happened, so the save path marked the buffer clean over bytes the edit was not in: the file stopped +changing, the unsaved dot went out, and the only trace was the edit still sitting on screen. The +writer now reports the refusal on the way past, the dirty flag has that as a second input beside the +tree comparison, and the toast says which block and why the file won. The flag stands for exactly as +long as those bytes are the file's, so a later lap of the debounce over the same unsavable block +says nothing more, and it is deliberately kept out of the question the debounce asks, since a +refused edit is not a difference another write could fix and re-arming for it would be a loop. + Where the body starts is the other: `---` on the first line of a file is not a rule, it is the opening delimiter of frontmatter, and everything down to the next one stops being markdown. That check is asked only of a body whose first three characters could open a @@ -308,7 +337,21 @@ carries no HTML for ProseMirror to parse and so arrives as the empty slice, empt clipboard extension now declares a priority above every other extension in the tree, which puts it at the head of the list whatever order the extension file lists things in, and being first is asserted rather than believed: `src/editor/fits.test.ts` reads the built plugin list, computes the -index of every plugin claiming a paste or a drop, and fails if anything is in front. Being first +index of every plugin claiming a paste or a drop, and fails if anything is in front. A guard is also worth nothing when the gesture it is watching for produces no event to answer, and +that is the sibling of the ordering fault rather than a different kind of mistake. Typing over a +rectangle of cells is refused through `handleTextInput`, which prosemirror-view offers only for a +character the browser was about to insert. An IME produces none: the browser writes the composition +into the DOM itself and prosemirror-view reads the result back, and on the way there its own +`compositionstart` replaces the rectangle with whatever a DOM range spanning cells makes, because +prosemirror-tables hands back a cell selection only while a mouse drag is still down. What follows +is either that range collapsed on to the whole content of one cell and replaced, or a text selection +left spanning cell boundaries and replaced across them, which joins the cells and rows away and +hands the writer a table whose rows disagree in width. Neither reaches the guard, and the second +never reaches it by construction, since a change crossing a textblock is dispatched without the +offer being made. Claiming the event does not help, because a composition cannot be cancelled and +returning true only stops ProseMirror's own bookkeeping. What is decidable is where the composition +lands, so the rectangle is collapsed to a caret at the end of its anchor cell before one can start, +and false is returned so the library's own handler still runs against that caret. Being first also means standing aside deliberately, for the one paste the library does better: cells copied out of a table and pasted into one, recognised with prosemirror-tables' own predicate rather than a reimplementation of it. Emptying cells is a real thing to want and it is Backspace. @@ -416,6 +459,38 @@ answers three things a plain file tree cannot: quick open by filename and path o search across every open root on `Cmd+Shift+F`, and the backlinks section appended to a document, a reverse lookup of every relative markdown link elsewhere that resolves to the file currently open. +Derived state that only rebuilds at launch has to survive the session, and the worker behind it was +written as though it could not fail. It ran a bare loop over its queue, so one panic ended indexing +for as long as the app stayed open: the receiver dropped, every later send was discarded by a `.ok()` +that never looked, and search, quick open and backlinks went on answering from a frozen snapshot +with nothing on screen to say so. There was a reachable way to reach it, too, since a document is +free to contain the control character the search snippets are marked with, and a line holding one +and nothing else made the snippet slice start after it ended. The panic fired inside the search +command, which holds the connection across its whole row loop, so it poisoned the mutex and took the +worker with it. The loop now catches a panic, clears the poison, says so in the index status and +takes the next job, and it drains its queue before acting on it rather than one message at a time, +because the watcher's report that the kernel dropped events is a full walk of the root and hundreds +of those can pile up behind a long build. The drain keeps the last word about each path and never +reorders, which is the rule the debouncer already applies inside one batch. + +The watcher had the same shape of gap about its own root. Detecting that the open folder had gone +rode on an event arriving for something inside it, because `notify` does not ask FSEvents to watch +the path to the root and discards anything above it, so renaming or deleting a parent directory was +never noticed at all and the watch was left holding a stream on a path that no longer existed. A +watchdog beside each watch stats the root on the debounce tick instead, which is one call and does +not depend on any event being delivered by any backend. + +The link rewrite sweep is the one consumer that deliberately does not share the walk. The tree and +the index honour `.gitignore` because a sidebar full of build output and a search box full of +vendored READMEs are both worse than those files being hidden, and the invariant that a file the +tree hides is a file no search result can open is worth keeping for both. The sweep is not display, +it writes to the user's files, and a `.gitignore` is a statement about version control rather than +about whether something is a document: a relative link inside an ignored draft is one the user still +follows, and leaving it pointing at a path this app is the one that moved is a break nobody finds +until they follow it. So it gets its own walk with the git sources off and everything else on, the +four always skipped folders included, and it is bounded by a document count it reports rather than +by a depth it would have to hide. + ## Order of work The filesystem layer and the markdown bridge are the two things everything else depends on, and diff --git a/docs/release.md b/docs/release.md index e3e4d89..e7d61c1 100644 --- a/docs/release.md +++ b/docs/release.md @@ -22,14 +22,17 @@ bump the patch number, or give one to set it. The workflow bumps `tauri.conf.jso and `src-tauri/Cargo.toml` together, commits that to main, tags it, and builds the tag rather than whatever main happens to be by then. -It builds a universal macOS bundle and an x86_64 Linux one, and publishes nothing until both have -landed. The last job downloads `latest.json` and refuses to take the release out of draft unless -`darwin-aarch64`, `darwin-x86_64` and `linux-x86_64` are all present in it. A half-populated -manifest is worse than no release at all: the updater would offer an update to the platforms that -made it and error on the ones that did not. +It builds one universal macOS bundle and publishes nothing until it has landed. The last job +downloads `latest.json` and refuses to take the release out of draft unless `darwin-aarch64` and +`darwin-x86_64` are both present in it. One universal build writes both of those keys, pointing +them at the same archive and the same signature, because an installed copy asks the manifest for +the architecture it is running on and never for a universal one. A half-populated manifest is worse +than no release at all: the updater would offer an update to the platforms that made it and error +on the ones that did not. -Linux builds on Ubuntu 22.04 on purpose. The bundle will not run on anything older than the glibc -it was linked against, and 22.04 is the oldest baseline worth supporting. +Linux is not built. It was until recently, because this pipeline was copied from margin-calendar, +which ships on Linux. This app does not, so the row and the manifest key it required are gone and +the release runs on one macOS runner. Windows is not built. Nothing in `tauri.conf.json` targets it and the app has never claimed it. diff --git a/package.json b/package.json index 557c119..4ccbce2 100644 --- a/package.json +++ b/package.json @@ -45,6 +45,7 @@ "@playwright/test": "^1.62.1", "@tauri-apps/cli": "^2", "@types/katex": "^0.16.8", + "@types/node": "^22.20.1", "@types/react": "^19.1.8", "@types/react-dom": "^19.1.6", "@vitejs/plugin-react": "^4.6.0", diff --git a/playwright.config.ts b/playwright.config.ts index da99aca..d56c13d 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -8,6 +8,10 @@ const BASE_URL = `http://localhost:${PORT}`; export default defineConfig({ testDir: "./tests", + // Run before anything else, because `reuseExistingServer` below means the server on the port may + // belong to another checkout, and a suite that tests somebody else's code is worse than no suite. + // What it checks and why it can check it by comparing bytes is in tests/identity.ts. + globalSetup: "./tests/identity.ts", fullyParallel: true, // No retries on purpose. A test that only passes on the second go is a test that is lying about // something. diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 42f8ef1..ff468f7 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -96,6 +96,9 @@ importers: '@types/katex': specifier: ^0.16.8 version: 0.16.8 + '@types/node': + specifier: ^22.20.1 + version: 22.20.1 '@types/react': specifier: ^19.1.8 version: 19.2.18 @@ -104,16 +107,16 @@ importers: version: 19.2.4(@types/react@19.2.18) '@vitejs/plugin-react': specifier: ^4.6.0 - version: 4.7.0(vite@7.3.6) + version: 4.7.0(vite@7.3.6(@types/node@22.20.1)) typescript: specifier: ~5.8.3 version: 5.8.3 vite: specifier: ^7.0.4 - version: 7.3.6 + version: 7.3.6(@types/node@22.20.1) vitest: specifier: ^3.2.4 - version: 3.2.7(@types/debug@4.1.13) + version: 3.2.7(@types/debug@4.1.13)(@types/node@22.20.1) packages: @@ -911,6 +914,9 @@ packages: '@types/ms@2.1.0': resolution: {integrity: sha512-GsCCIZDE/p3i96vtEqx+7dBUGXrc7zeSK3wwPHIaRThS+9OhWIXRqzs4d6k1SVU8g91DrNRWxWUGhp5KXQb2VA==} + '@types/node@22.20.1': + resolution: {integrity: sha512-EANqOCF9QFyra+4pfxUcX9STKJpCLjMbObVzljIJomAWSnuSIEAvyzEU53GaajbXJEgdh0iEcPL+DGvpUd4k1Q==} + '@types/react-dom@19.2.4': resolution: {integrity: sha512-Bsc+QHgp+P/F02XDzNCY9jnZNCUuLki36KT7VKrTXXLdHf+vHMNZnW1rVu5DNW/rCK+fya3DATySbLM4yhtKUw==} peerDependencies: @@ -1713,6 +1719,9 @@ packages: engines: {node: '>=14.17'} hasBin: true + undici-types@6.21.0: + resolution: {integrity: sha512-iwDZqg0QAGrg9Rav5H4n0M64c3mkR59cJ6wQp+7C4nI0gsmExaedaYLNO44eT4AtBBwjbTiGPMlt2Md0T9H9JQ==} + unified@11.0.5: resolution: {integrity: sha512-xKvGhPWw3k84Qjh8bI3ZeJjqnyadK+GEFtazSfZv/rKeTkTjOJho6mFqh2SM96iIcZokxiOpg78GazTSg8+KHA==} @@ -2594,6 +2603,10 @@ snapshots: '@types/ms@2.1.0': {} + '@types/node@22.20.1': + dependencies: + undici-types: 6.21.0 + '@types/react-dom@19.2.4(@types/react@19.2.18)': dependencies: '@types/react': 19.2.18 @@ -2614,7 +2627,7 @@ snapshots: d3-selection: 3.0.0 d3-transition: 3.0.1(d3-selection@3.0.0) - '@vitejs/plugin-react@4.7.0(vite@7.3.6)': + '@vitejs/plugin-react@4.7.0(vite@7.3.6(@types/node@22.20.1))': dependencies: '@babel/core': 7.29.7 '@babel/plugin-transform-react-jsx-self': 7.29.7(@babel/core@7.29.7) @@ -2622,7 +2635,7 @@ snapshots: '@rolldown/pluginutils': 1.0.0-beta.27 '@types/babel__core': 7.20.5 react-refresh: 0.17.0 - vite: 7.3.6 + vite: 7.3.6(@types/node@22.20.1) transitivePeerDependencies: - supports-color @@ -2634,13 +2647,13 @@ snapshots: chai: 5.3.3 tinyrainbow: 2.0.0 - '@vitest/mocker@3.2.7(vite@7.3.6)': + '@vitest/mocker@3.2.7(vite@7.3.6(@types/node@22.20.1))': dependencies: '@vitest/spy': 3.2.7 estree-walker: 3.0.3 magic-string: 0.30.21 optionalDependencies: - vite: 7.3.6 + vite: 7.3.6(@types/node@22.20.1) '@vitest/pretty-format@3.2.7': dependencies: @@ -3679,6 +3692,8 @@ snapshots: typescript@5.8.3: {} + undici-types@6.21.0: {} + unified@11.0.5: dependencies: '@types/unist': 3.0.3 @@ -3735,13 +3750,13 @@ snapshots: '@types/unist': 3.0.3 vfile-message: 4.0.3 - vite-node@3.2.4: + vite-node@3.2.4(@types/node@22.20.1): dependencies: cac: 6.7.14 debug: 4.4.3 es-module-lexer: 1.7.0 pathe: 2.0.3 - vite: 7.3.6 + vite: 7.3.6(@types/node@22.20.1) transitivePeerDependencies: - '@types/node' - jiti @@ -3756,7 +3771,7 @@ snapshots: - tsx - yaml - vite@7.3.6: + vite@7.3.6(@types/node@22.20.1): dependencies: esbuild: 0.28.2 fdir: 6.5.0(picomatch@4.0.5) @@ -3765,13 +3780,14 @@ snapshots: rollup: 4.62.5 tinyglobby: 0.2.17 optionalDependencies: + '@types/node': 22.20.1 fsevents: 2.3.3 - vitest@3.2.7(@types/debug@4.1.13): + vitest@3.2.7(@types/debug@4.1.13)(@types/node@22.20.1): dependencies: '@types/chai': 5.2.3 '@vitest/expect': 3.2.7 - '@vitest/mocker': 3.2.7(vite@7.3.6) + '@vitest/mocker': 3.2.7(vite@7.3.6(@types/node@22.20.1)) '@vitest/pretty-format': 3.2.7 '@vitest/runner': 3.2.7 '@vitest/snapshot': 3.2.7 @@ -3789,11 +3805,12 @@ snapshots: tinyglobby: 0.2.17 tinypool: 1.1.1 tinyrainbow: 2.0.0 - vite: 7.3.6 - vite-node: 3.2.4 + vite: 7.3.6(@types/node@22.20.1) + vite-node: 3.2.4(@types/node@22.20.1) why-is-node-running: 2.3.0 optionalDependencies: '@types/debug': 4.1.13 + '@types/node': 22.20.1 transitivePeerDependencies: - jiti - less diff --git a/src-tauri/src/fs.rs b/src-tauri/src/fs.rs index 9d99787..91f2af6 100644 --- a/src-tauri/src/fs.rs +++ b/src-tauri/src/fs.rs @@ -20,7 +20,7 @@ use std::sync::atomic::{AtomicU64, Ordering as Memory}; use std::sync::{Arc, LazyLock, Mutex}; use std::time::{SystemTime, UNIX_EPOCH}; -use ignore::WalkBuilder; +use ignore::{DirEntry, WalkBuilder}; use tauri::{AppHandle, State}; use tauri_plugin_opener::OpenerExt; @@ -387,6 +387,27 @@ fn assemble( Some(node) } +/// True for everything a walk should keep, which is everything except one of the four always +/// skipped names turning up as a folder somewhere below the walk root. Returning false for a +/// directory prunes it, so nothing inside it is walked either. +/// +/// The root itself is kept whatever it is called, because a user who opens a folder named `dist` +/// opened it deliberately and hiding its entire contents from them would be absurd. Files are kept +/// whatever they are called too: the four names describe folders, and a document called `target.md` +/// is a document. +/// +/// Shared by every walk in the app rather than written out once per walk, so the sidebar, the index +/// and the link sweep cannot drift apart about which folders are never worth descending into. +fn not_always_skipped(entry: &DirEntry) -> bool { + if entry.depth() == 0 { + return true; + } + if !entry.file_type().map(|t| t.is_dir()).unwrap_or(false) { + return true; + } + !ALWAYS_SKIPPED.contains(&entry.file_name().to_string_lossy().as_ref()) +} + /// One pass over a folder, returning the root node with everything under it already attached. /// /// `show_ignored` turns off gitignore, the hidden file rule and the four always skipped folders in @@ -407,15 +428,7 @@ pub fn scan_tree(root: &Path, show_ignored: bool) -> Result { .require_git(false) .standard_filters(!show_ignored); if !show_ignored { - builder.filter_entry(|entry| { - if entry.depth() == 0 { - return true; - } - if !entry.file_type().map(|t| t.is_dir()).unwrap_or(false) { - return true; - } - !ALWAYS_SKIPPED.contains(&entry.file_name().to_string_lossy().as_ref()) - }); + builder.filter_entry(not_always_skipped); } let mut nodes: HashMap = HashMap::new(); @@ -445,6 +458,73 @@ pub fn scan_tree(root: &Path, show_ignored: bool) -> Result { assemble(root, &mut nodes, &children).ok_or_else(|| format!("cannot read {}", root.display())) } +/// Every markdown document under `root`, as flat paths, walked by the link sweep's rules rather +/// than the sidebar's. +/// +/// This walk exists because a .gitignore is a statement about version control and not about whether +/// a file is a document. The tree and the index honour it, and they are right to: a sidebar full of +/// build output and a search box full of vendored READMEs are both worse than those files staying +/// out of sight, and neither of them changes anything by leaving a file alone. A link is the +/// opposite case. A relative link inside an ignored draft is still a link the user follows, and +/// leaving it pointing at a path that this app is the thing that moved is a break nobody finds +/// until the day they click it. Hiding that file costs the user a broken document rather than a +/// tidy sidebar, so the sweep walks by its own rules and the two are allowed to disagree. +/// +/// So the three git sources come off and the four hardcoded folders stay on, which is what keeps a +/// checkout's node_modules out of the sweep whether or not git was ever asked about it. Hidden +/// files stay out for the same reason, since a dotted folder is where other languages keep their +/// tooling and none of `.venv`, `.next`, `.cache`, `.tox` or `.gradle` holds a link anybody wrote. +/// A `.ignore` or `.rgignore` is still honoured, because that file is written for tools that walk +/// rather than for git, which makes it the honest way to tell this walk to stay out of a folder. +/// +/// `limit` bounds the work, because the sweep reads and rewrites every file this returns and an +/// unbounded one over a folder the size of somebody's home directory is not what they asked for +/// when they renamed a file. There is deliberately no depth cap to go with it: a document one level +/// past a depth cap is silently not swept and nothing anywhere says so, which is the same class of +/// bug this function exists to fix. A count is honest instead, because the caller can see it was +/// hit. Which is why one path past the limit comes back rather than exactly `limit` of them: a +/// folder holding exactly the budget and a folder holding ten thousand more look identical at +/// `limit` paths, and the caller has to be able to tell them apart to say that its coverage was +/// partial rather than reporting a complete sweep of a subset. +pub fn documents_for_sweep(root: &Path, limit: usize) -> Vec { + let mut builder = WalkBuilder::new(root); + builder + // A symlinked folder pointing back at one of its own ancestors would otherwise walk for + // ever, exactly as it would for the tree. + .follow_links(false) + .hidden(true) + // No climbing above the walk root looking for ignore files. The sweep is about this one + // folder, and what some parent of it happens to say about it is not this folder's business. + .parents(false) + .ignore(true) + .git_ignore(false) + .git_global(false) + .git_exclude(false); + builder.filter_entry(not_always_skipped); + + let mut out = Vec::new(); + for entry in builder.build() { + // One unreadable entry is one document the sweep does not visit and not a failed sweep. The + // caller reports what it covered either way, so losing a row here is a smaller and more + // honest failure than refusing to rewrite anything at all. + let Ok(entry) = entry else { continue }; + if entry.file_type().map(|t| t.is_dir()).unwrap_or(true) { + continue; + } + // Markdown only. Plain text is indexed and searchable, but nothing in a .txt is a markdown + // link this app knows how to rewrite, and opening every one of them to find that out would + // be work spent to change nothing. + if kind_for(entry.path(), false) != "markdown" { + continue; + } + out.push(path_string(entry.path())); + if out.len() > limit { + break; + } + } + out +} + pub fn read_document(path: &Path) -> Result { // The mtime is taken before the read rather than after. Read the other way round and a change // landing between the two would be stamped onto older text, and the next save would overwrite @@ -756,6 +836,31 @@ pub fn tree_read(roots: State<'_, Roots>, root_id: String) -> Result

e.preventDefault()} > + {/* The two paragraphs and the rule are not items, so a menu announces the things that can be + chosen and nothing else. Neither line is thrown away with them: both are named on the + menu's own aria-describedby above, which is where a sentence about a menu belongs and is + what gets them read once, on the way in, rather than as a row somebody has to arrow past. */} {target.suggestions.length === 0 ? ( -

No suggestions

+ ) : ( target.suggestions.map((suggestion) => ( -

Learning a word teaches this Mac, not only Margin Docs.

+
, document.body, ); diff --git a/src/dev/mockIpc.ts b/src/dev/mockIpc.ts index 34be760..16fcf4d 100644 --- a/src/dev/mockIpc.ts +++ b/src/dev/mockIpc.ts @@ -246,6 +246,21 @@ export async function mockCall(command: string, args?: Record r.id === (a.rootId as unknown as string)); + if (!root) throw new Error(`no such root: ${a.rootId as unknown as string}`); + const limit = a.limit as unknown as number; + // One path past the limit, the way `documents_for_sweep` does it, because that is how the + // caller tells a folder that exactly fills its budget from one that goes over it. The + // fixture has no ignore files in it, so what makes this different from `tree_read` on real + // disk does not show here; what a browser and Playwright need is that the command answers at + // all, since a sweep that throws reports every move as partial. + return subtree(root.path) + .filter((e) => !e.dir && kindOf(e) === "markdown") + .map((e) => e.path) + .slice(0, limit + 1) as unknown as T; + } + case "reveal_in_finder": case "open_external": // Nothing to hand a file to in a browser tab, so say so rather than looking broken. diff --git a/src/document.ts b/src/document.ts index da684b9..73c6bf1 100644 --- a/src/document.ts +++ b/src/document.ts @@ -2,10 +2,14 @@ // next door holds what is on screen and the setters a keystroke can settle on its own; this module // reads the file, hands it to the markdown bridge, and writes it back 500ms after the last edit. // -// Opening writes nothing. There is exactly one call to `fileWrite` in this file, it sits inside -// `performSave`, and `performSave` returns before reaching it unless the buffer is dirty, which -// only `setContent` can make it. That is the product's first promise and -// src/store/useDocument.test.ts asserts it rather than trusting this paragraph. +// Opening writes nothing. There are exactly two calls to `fileWrite` in this file and they are +// worth naming. The first is inside `performSave`, which returns before reaching it unless the +// buffer is dirty, which only `setContent` can make it. That is the product's first promise and +// src/store/useDocument.test.ts asserts it rather than trusting this paragraph. The second is +// inside `rewriteOpenDocument` at the bottom, which src/linkRewrite.ts calls when a move has made +// the open document's own links wrong. It is reached only for a buffer nobody has typed into, and +// only with bytes the caller worked out from the file's own text rather than from the serializer, +// so nothing on that path can put a house style rewrite on a document the user has not edited. // // `setContent` marks the buffer dirty through `differsFromDisk` below, which is the second half of // that promise. That question is asked of the document the file was read from and never of the @@ -15,6 +19,14 @@ // the same answer for the same reason, since the file would not show it either. Dragging a table // column is the whole of that today. // +// One edit is refused rather than written, and the serializer's own comments are where that is +// argued out: a raw block beside a list is put back to the bytes the file gave it, because the +// edited bytes are read back as part of the list. That decision is not this module's, but saying +// so is. The writer marks it on the way past, this module puts it in a toast and holds the buffer +// dirty until the edit is taken back or the block is moved, and `refusedOnDisk` below is where it +// is kept. It is the one difference between the buffer and the file that comparing two trees +// cannot find, since the tree those bytes were written from is the buffer itself. +// // `performSave` also never runs twice at once for the open document: `saveNow` keeps at most one // call to it on the wire, folding anything that arrives while one is running into a single next // lap rather than starting a second write alongside the first. @@ -31,6 +43,7 @@ import type { ReadResult, WriteResult } from "./ipc"; import { parseMarkdown, parsePlainText, + type Refused, serializeMarkdown, serializePlainText, } from "./markdown"; @@ -41,6 +54,19 @@ import { notify } from "./store/useToast"; /** Long enough that a sentence is one save, short enough that Cmd+Tab away is already on disk. */ export const SAVE_DEBOUNCE_MS = 500; +/** + * Said when the writer hands back bytes an edit is not in, which is the one edit this editor + * knowingly does not save. + * + * It says which block and it says why, because the alternative the user is left with otherwise is a + * change that is on screen, is not in the file, and has nothing anywhere to say so. The wording is + * about the file rather than about the writer: what has happened to them is that the block on disk + * is the one that was there before, and the reason is that a list is the one construct a blank line + * does not close. + */ +const REFUSED_MESSAGE = + "The edit to that block was not saved: beside a list those bytes read back as part of the list, so the file keeps the block it had."; + let saveTimer: ReturnType | null = null; let unsubscribe: (() => void) | null = null; @@ -64,11 +90,25 @@ let unsubscribe: (() => void) | null = null; let diskText: string | null = null; let diskDoc: ProseMirrorNode | null = null; -/** The only way either of those moves, so that they cannot drift apart into two answers about the - * same file, one of which sends a write and the other of which holds it back. */ -function rememberDisk(text: string | null, doc: ProseMirrorNode | null): void { +/** + * Whether the bytes in `diskText` are missing an edit the buffer still holds. + * + * `diskDoc` cannot answer this and it is not a bug in it. The tree the writer refused part of is + * the same tree the bytes were written from, and serializing it again gives the same bytes back, so + * the comparison below says the buffer and the file agree at the one moment the thing on screen is + * not in the file at all. The writer says so on the way past and this is where that is kept, for + * exactly as long as those bytes are the file's. + */ +let refusedOnDisk = false; + +/** The only way any of those move, so that they cannot drift apart into two answers about the + * same file, one of which sends a write and the other of which holds it back. The refusal defaults + * to none, because every caller but the serializer's own is saying where the file is rather than + * what went into it, and none of those can leave an edit behind. */ +function rememberDisk(text: string | null, doc: ProseMirrorNode | null, refused = false): void { diskText = text; diskDoc = doc; + refusedOnDisk = refused; } /** Markdown and plain text are two different round trips and picking the wrong one mangles a .txt. */ @@ -192,8 +232,22 @@ function sameToTheSerializer(a: ProseMirrorNode, b: ProseMirrorNode): boolean { * one. A change wrongly called insignificant is a keystroke that never reaches the disk, which is * the worst thing in this module; a change wrongly called significant costs one write that * `performSave` then finds nothing to do. + * + * The comparison is not the whole answer, because there is one difference no comparison of trees + * can find: a refused edit is in the buffer and is not in the file, and the tree the bytes were + * written from is the buffer, so the two agree. That is what `refusedOnDisk` is holding and it is + * why it comes first. */ export function differsFromDisk(next: ProseMirrorNode): boolean { + return refusedOnDisk || wouldWrite(next); +} + +/** + * The same question with the refusal left out: whether another write would put different bytes on + * the disk. A refused edit is a difference the user has to keep seeing and not one a second write + * could fix, so it makes the buffer dirty and it does not arm the debounce. + */ +function wouldWrite(next: ProseMirrorNode): boolean { return diskDoc === null || !sameToTheSerializer(diskDoc, next); } @@ -203,6 +257,12 @@ function bufferDiffersFromDisk(): boolean { return now !== null && differsFromDisk(now); } +/** And the same again for the debounce, which only ever wants to know about the bytes. */ +function bufferWouldWrite(): boolean { + const now = useDocument.getState().content; + return now !== null && wouldWrite(now); +} + function apply(read: ReadResult, document: MarkdownDocument): void { rememberDisk(read.text, document.doc); useDocument.setState({ @@ -231,6 +291,21 @@ export function cancelPendingSave(): void { saveTimer = null; } +/** + * The one edit this editor knowingly does not save, said out loud. + * + * Once per occurrence rather than once per save. The flag stays up for as long as those bytes are + * the file's, so five more laps of the debounce over the same unsavable block say nothing more, and + * taking the edit back and making it again is a new occurrence and is worth saying again. + * + * Read before `rememberDisk`, never after: this is the only place that can tell the refusal that + * has just happened from the one that was already standing, and the call that records the new one + * is what wipes that difference out. + */ +function noteRefusal(refused: boolean): void { + if (refused && !refusedOnDisk) notify(REFUSED_MESSAGE); +} + /** * Starts the debounce. Idempotent, and returning the teardown rather than keeping it private is * what lets a test run the lifecycle without leaving a timer behind for the next one. @@ -325,9 +400,10 @@ async function performSave(): Promise<"wrote" | "conflict" | "skipped"> { const { serialize } = bridgeFor(path); useDocument.setState({ savePhase: "saving", saveError: null }); + const refused: Refused = { rawBlock: false }; let text: string; try { - text = serialize(document, content); + text = serialize(document, content, refused); } catch (e) { useDocument.setState({ savePhase: "error", saveError: String(e) }); throw e; @@ -344,7 +420,11 @@ async function performSave(): Promise<"wrote" | "conflict" | "skipped"> { if (text === diskText) { // Those bytes are on disk and this is a tree that produces them, which is all `diskDoc` has // ever claimed to be. Nothing was written, so nothing needs to be. - rememberDisk(text, content); + // + // This is the branch the refused edit arrives on, and it arrives on it precisely because the + // writer put the file's own bytes back, so saying nothing here is saying nothing at all. + noteRefusal(refused.rawBlock); + rememberDisk(text, content, refused.rawBlock); useDocument.setState({ dirty: bufferDiffersFromDisk(), savePhase: "idle", @@ -378,19 +458,23 @@ async function performSave(): Promise<"wrote" | "conflict" | "skipped"> { return "conflict"; } - rememberDisk(text, content); - const stillDirty = bufferDiffersFromDisk(); + noteRefusal(refused.rawBlock); + rememberDisk(text, content, refused.rawBlock); + const unwritten = bufferWouldWrite(); useDocument.setState({ modifiedMs: result.modifiedMs, - dirty: stillDirty, + dirty: bufferDiffersFromDisk(), savePhase: "idle", saveError: null, externalChange: "synced", }); // Typed into, or undone, while that write was on the wire. An undo is the case that needs this: // it went past the subscription at a moment when the buffer and the file did agree, so nothing - // armed the debounce for it, and this write is what has just made it a difference again. - if (stillDirty) scheduleSave(); + // armed the debounce for it, and this write is what has just made it a difference again. A + // refusal is not one of these. Those bytes are already everything the file can hold, so the + // buffer stays dirty and the debounce stays down, which is the difference between the two + // questions asked above: the flag is about what the user can see, the timer is about the bytes. + if (unwritten) scheduleSave(); else cancelPendingSave(); return "wrote"; } @@ -442,6 +526,34 @@ export function abandonDocument(): void { useDocument.getState().close(); } +/** + * This app has just renamed or moved the open document's own file, and its buffer holds an edit + * nothing else has. + * + * The buffer follows the file instead of being thrown away and read back from it, because that + * edit is the only copy of itself and a reopen is a read. Nothing else here moves. The bytes and + * the document this module last saw are still the ones the file held when it was moved, the + * timestamp is still that file's, and whatever armed the debounce is still armed. So the save that + * follows goes to the new path carrying the old file's timestamp: it lands when nothing touched + * the file on the way, and it comes back a conflict when the link rewrite did, which is a question + * put to the user rather than either copy being lost. + * + * What this replaces is worse than it sounds. Left pointed at the path the file has just moved off, + * that same save writes a path that no longer exists, and `write_document` falls through its + * timestamp check when the file it is checking has gone, because a document deleted under the user + * has to have somewhere to land. So the file came back at the old path, and the user was left with + * two copies of one document and a sidebar showing both. + */ +export function documentMovedTo(path: string): void { + if (useDocument.getState().path === null) return; + // The history entry goes with it. Where the user is standing in the back and forward list is the + // document they are looking at, and this app is the thing that made the old spelling of it dead. + useDocument.setState((s) => ({ + path, + history: s.history.map((entry, at) => (at === s.historyIndex ? path : entry)), + })); +} + /** * Something outside the app touched the open document. Clean buffers take the new bytes silently, * dirty ones are left exactly as they are and the UI is told there is a choice to make. @@ -458,8 +570,11 @@ export async function documentChangedOnDisk(path: string): Promise { // document stays, because it is still the last one this module knew the file to hold and it is // what keeps a buffer nobody has typed into from turning dirty and putting the file back. // Resurrecting a file the user deleted is `keepBuffer`, and it is the user's word, not a - // side effect of clicking into the editor afterwards. - rememberDisk(null, diskDoc); + // side effect of clicking into the editor afterwards. The refusal is carried over for the same + // reason the document is: a save that left an edit behind still left it behind, and forgetting + // that here would let the next keystroke work the buffer out as clean over bytes that never + // held it. + rememberDisk(null, diskDoc, refusedOnDisk); useDocument.setState({ externalChange: "changed-on-disk" }); return; } @@ -485,3 +600,127 @@ export async function documentChangedOnDisk(path: string): Promise { const { parse } = bridgeFor(path); apply(read, parse(read.text, read.path)); } + +/** + * What one pass of the link rewrite did to the open document. + * + * `held` is not a failure and not a no-op. It is this module saying the file was not the sweep's to + * write at that moment, which the caller has to report rather than count as a file with nothing in + * it to change. + */ +export type OpenRewrite = + | { kind: "held" } + | { kind: "unchanged" } + | { kind: "rewritten" } + | { kind: "failed"; reason: string }; + +/** + * The open document's own file, rewritten by src/linkRewrite.ts after a move made the links in it + * wrong, without going through the serializer and without going around the single writer above. + * + * `rewrite` is handed the bytes this module last saw and returns the bytes to put back, or null + * when there was nothing in them to change. It is pure and it is synchronous, and that is the whole + * design rather than a convenience. Every question about the buffer, the answer, and the write go + * out in one run of the event loop, so there is no window between reading `dirty` and writing for a + * keystroke to arrive in. A callback that could await would put the window straight back. + * + * The buffer takes the rewrite before the bytes leave rather than after they land. A keystroke + * arriving while the write is on the wire then lands on top of the new links, and the save it arms + * writes the user's own edit over a file that already agrees with what they are looking at. Doing + * it the other way round is what put a conflict banner in front of somebody for a change this app + * made itself: the write went out carrying the timestamp the buffer had from before they typed, and + * the file they were looking at came back looking like somebody else's copy. + * + * It costs nothing that the success path was not already paying, because that path swaps the + * editor's tree either way. + */ +export async function rewriteOpenDocument( + path: string, + rewrite: (text: string) => string | null, +): Promise { + const state = useDocument.getState(); + // A buffer with an edit in it is the only copy of that edit, and a write already on the wire owns + // the file until it lands. Neither is this function's to write over. + if (state.path !== path || state.document === null || state.dirty) return { kind: "held" }; + if (writeInFlight !== null || diskText === null || state.modifiedMs === null) { + return { kind: "held" }; + } + + const was = { document: state.document, text: diskText, modifiedMs: state.modifiedMs }; + let prepared: { text: string; document: MarkdownDocument }; + try { + const next = rewrite(was.text); + if (next === null) return { kind: "unchanged" }; + prepared = { text: next, document: bridgeFor(path).parse(next, path) }; + } catch (e) { + return { kind: "failed", reason: String(e) }; + } + const { text, document } = prepared; + + apply({ path, text, modifiedMs: was.modifiedMs }, document); + + const run = async (): Promise => { + let result: WriteResult; + try { + result = await fileWrite(path, text, was.modifiedMs); + } catch (e) { + revert(path, was); + return { kind: "failed", reason: String(e) }; + } + // Switched away from while the write was in flight. The bytes did land, so this is not a + // failure, and the buffer it would have been reconciled against is not on screen any more. + if (useDocument.getState().path !== path) return { kind: "rewritten" }; + if (result.conflict) { + revert(path, was); + return { kind: "failed", reason: "it changed on disk part way through" }; + } + useDocument.setState({ modifiedMs: result.modifiedMs }); + // Typed into while that write was on the wire, on top of the new links. The same belt and + // braces `performSave` ends with, since an undo can pass the subscription while it is clean. + if (useDocument.getState().dirty) scheduleSave(); + return { kind: "rewritten" }; + }; + + const inFlight = run(); + writeInFlight = inFlight.then(() => undefined); + const held = writeInFlight; + try { + return await inFlight; + } finally { + if (writeInFlight === held) writeInFlight = null; + } +} + +/** + * Puts the buffer back to the document the file still holds, for a rewrite that was installed and + * then did not land. + * + * The same object goes back into the store rather than a fresh parse of the same bytes, which is + * what lets src/editor/Editor.tsx see the document it already had instead of a different file: it + * stashes the caret by path on the way past and puts it back, so the user is left where they were + * typing rather than at the top of a document that flickered. + */ +function revert( + path: string, + was: { document: MarkdownDocument; text: string; modifiedMs: number }, +): void { + const now = useDocument.getState(); + if (now.path !== path) return; + if (now.dirty) { + // Typed into while the write was on the wire, so the buffer holds an edit on top of the new + // links and nothing here gets to throw it away. What is on disk is somebody else's copy this + // module has not read, which is exactly what `performSave` says in the same situation. No + // refusal is carried over: a null document already makes every question about the buffer come + // back dirty, which is the whole of what the flag was holding up. + rememberDisk(null, null); + useDocument.setState({ externalChange: "changed-on-disk" }); + return; + } + // Whatever the watcher found while the write was out lives through this. `apply` says "synced" + // because every other caller of it has just read the file, and this one has not: it is putting + // back the document the file had before, which is the claim this module was already making when + // the rewrite started, and it is not an answer to a change somebody else made in the meantime. + const flagged = now.externalChange; + apply({ path, text: was.text, modifiedMs: was.modifiedMs }, was.document); + if (flagged !== "synced") useDocument.setState({ externalChange: flagged }); +} diff --git a/src/editor/blocks/tables.ts b/src/editor/blocks/tables.ts index 0e92fc9..0c9dcc0 100644 --- a/src/editor/blocks/tables.ts +++ b/src/editor/blocks/tables.ts @@ -33,6 +33,7 @@ import { Plugin, PluginKey, TextSelection } from "@tiptap/pm/state"; import type { Command, Transaction } from "@tiptap/pm/state"; import type { Node as ProseMirrorNode } from "@tiptap/pm/model"; import { + CellSelection, TableMap, addColumnAfter, addColumnBefore, @@ -48,6 +49,7 @@ import { tableEditing, } from "@tiptap/pm/tables"; import type { TableRect } from "@tiptap/pm/tables"; +import type { EditorView } from "@tiptap/pm/view"; import type { ColumnAlign } from "../../model/doc"; import { overCells } from "../fits"; import type { TableOp } from "../index"; @@ -301,9 +303,71 @@ const typing = new Plugin({ key: typingKey, props: { handleTextInput: (view) => overCells(view.state), + handleDOMEvents: { + compositionstart: collapseRectangle, + // The same DOM level insert arriving without a composition behind it. macOS's Replace menu + // and its autocorrect send `insertReplacementText`, dictation sends `insertText`, and neither + // of them goes anywhere near `handleTextInput`. A printable character over a rectangle never + // reaches here, because prosemirror-view's keypress handler calls preventDefault for any + // selection that is not one text range inside one textblock, and nor does Backspace, which + // the keymap at the end of this file claims first. + beforeinput: (view, event) => + event.inputType.startsWith("insert") ? collapseRectangle(view) : false, + }, }, }); +/** + * The same rule for a composition, which is the one gesture a handler cannot refuse. + * + * `handleTextInput` above is offered a character the browser was about to insert. An IME never + * produces one: the browser fires compositionstart, writes the composition into the DOM itself, and + * prosemirror-view reads the result back out afterwards. So the guard above is not bypassed by a + * mistake in it, it is simply not on the route, and neither is anything else that answers true or + * false. + * + * Claiming the event does not help and it makes things worse. compositionstart is not cancelable, + * so preventDefault does nothing and the IME composes whatever a handler returns. What returning + * true DOES do is stop prosemirror-view's own compositionstart from running, which leaves + * `view.composing` false while a real composition is in progress, and every DOM write the IME makes + * then reads back as an ordinary edit. + * + * What is decidable is where the composition lands, and that is decided before it starts. Left + * alone, prosemirror-view's compositionstart replaces the rectangle with whatever + * `selectionFromDOM` makes of a DOM range spanning cells, which is a text selection, because + * prosemirror-tables only hands back a cell selection while a mouse drag is still down. + * prosemirror-tables then either collapses that on to the whole content of one cell, and the + * composition replaces it, or leaves it spanning cell boundaries, and prosemirror-view replaces + * across them and joins the cells and the rows away. Both are the rectangle destroyed by a key + * pressed to type one word, and the second one never reaches `handleTextInput` at all: a change + * that crosses a textblock is dispatched without the offer ever being made. + * + * So the rectangle is collapsed first, to a caret at the end of the cell the drag started in, and + * the composition lands there. Nothing is replaced and no boundary is crossed, which is the promise + * the guard above makes for an ordinary character: the rows and the columns survive it. False is + * returned rather than true, so prosemirror-view's own compositionstart still runs, against the + * caret this left it. The dispatch is synchronous for the same reason: the DOM selection has to be + * the caret before the browser applies the first composed character, and a microtask is already + * too late. + * + * `instanceof` rather than `overCells`, which asks this exact question in src/editor/fits.ts and is + * what the guard above uses. What is needed here is not the boolean but the narrowing that comes + * with it, since `$anchorCell` only exists on the cell selection. + */ +function collapseRectangle(view: EditorView): false { + const { selection } = view.state; + if (!(selection instanceof CellSelection)) return false; + const cell = selection.$anchorCell.nodeAfter; + if (!cell) return false; + // The end of the anchor cell's content, so the composition appends rather than replacing. A cell + // holds inline content in src/model/schema.ts and no blocks at all, which is what makes that + // position a caret already: `near` takes it as it stands rather than searching forward out of the + // cell for one, which is where it would have to go if a paragraph closed in front of it. + const at = selection.$anchorCell.pos + 1 + cell.content.size; + view.dispatch(view.state.tr.setSelection(TextSelection.near(view.state.doc.resolve(at)))); + return false; +} + export const Tables = Extension.create({ name: "tables", diff --git a/src/editor/fits.ts b/src/editor/fits.ts index 2e933ae..20489c2 100644 --- a/src/editor/fits.ts +++ b/src/editor/fits.ts @@ -68,6 +68,12 @@ const OPAQUE = ["callout", "toggle"] as const; /** A line with nothing but spaces on it, which is what ends an html block in markdown. */ const BLANK_LINE = /\n[ \t]*\n/; +/** + * Every line ending in a piece of text, which since the heading became `whitespace: "pre"` is the + * other way a second line gets into a block that has nowhere to put one on its own. + */ +const LINE_ENDING = /\r\n?|\n/g; + /** * The rectangle of whole cells a drag across a table makes, named once so that the four questions * below and src/editor/blocks/tables.ts ask it in the same words. @@ -172,7 +178,7 @@ export function place( export type Change = "convert" | "wrap" | "unwrap"; /** Whether this position is inside the one block whose bytes are the file's own. */ -function inRaw($pos: ResolvedPos): boolean { +export function inRaw($pos: ResolvedPos): boolean { for (let depth = $pos.depth; depth > 0; depth -= 1) { if ($pos.node(depth).type.name === RAW) return true; } @@ -180,7 +186,7 @@ function inRaw($pos: ResolvedPos): boolean { } /** Whether the selection is inside, or reaches across, a raw block. */ -function touchesRaw(state: EditorState): boolean { +export function touchesRaw(state: EditorState): boolean { return state.selection.ranges.some(({ $from, $to }) => { if (inRaw($from) || inRaw($to)) return true; let found = false; @@ -225,12 +231,17 @@ export function changeable(state: EditorState): boolean { * * A GFM cell is one line, and src/markdown/serialize.ts flattens a break inside one into the space * a cell can hold. A heading is one line too, except at the two levels that have a setext - * spelling: `#### a` has nowhere to put the second line, so mdast writes the break out as a space, + * spelling: `#### a` has nowhere to put a second line, so mdast writes the break out as a space, * while an underlined heading keeps it. * * Either way nothing is lost that the user typed, and either way the editor is drawing a line the * next open of the file will not have. An editor showing a construct the file silently swallows is * an editor lying about what was saved. + * + * `strandedBreaks` asks this of a newline in the text as well, which since the heading became + * `whitespace: "pre"` is the other way a second line arrives. That one the writer can spell, as + * ` `, and it comes back, so the no there is not about losing it: a heading below the two + * underlined levels is one line of the file, and this editor does not put a second one in it. */ function holdsBreak(parent: ProseMirrorNode): boolean { const role = parent.type.spec.tableRole; @@ -259,7 +270,8 @@ function holdsTrailingBreak(parent: ProseMirrorNode): boolean { } /** - * The breaks in this document that the next save will swallow or spell wrong. + * The breaks in this document that the next save will swallow or spell wrong, and the newlines in a + * block that is not allowed a second line at all. * * A break with nothing after it in its block is usually not one of them: that is the half typed * line somebody is in the middle of, it has no spelling either, and the next character they type @@ -273,6 +285,14 @@ function strandedBreaks(doc: ProseMirrorNode): number { const trailing = holdsTrailingBreak(parent); if (held && trailing) return; parent.forEach((child, _offset, index) => { + // A newline in the text is the same second line arriving by the other door. It used to be + // impossible to build one here, because a conversion into a heading rewrote every newline as + // a break on the way in; now that a heading is `whitespace: "pre"` the newline survives, and + // a block that cannot hold a break does not get to hold this either. + if (child.isText) { + if (!held) total += (child.text?.match(LINE_ENDING) ?? []).length; + return; + } if (child.type.name !== "hardBreak") return; const last = index === parent.childCount - 1; if (last ? !trailing : !held) total += 1; @@ -329,11 +349,12 @@ export function change( if (kind !== "unwrap" && OPAQUE.some((name) => countOf(tr.doc, name) < countOf(before, name))) { return false; } - // And the block the content lands in has to be able to write down what is in it. A fence of two - // lines turned into a heading is two lines in a heading, which only the two levels with an - // underlined spelling can hold: the deeper four write the break out as a space, so the second - // line would be on screen and gone from the file, which is `breakable` refusing Shift+Enter in - // the same block, arrived at from the other direction. + // And the block the content lands in has to be able to hold what is in it. A fence of two lines + // turned into a heading is two lines in a heading, which only the two levels with an underlined + // spelling have: the deeper four are one line of the file, whether the second line arrives as a + // break, which they write out as a space and lose, or as a newline in the text, which they can + // spell but which this editor still does not put there. That is `breakable` refusing Shift+Enter + // in the same block, arrived at from the other direction. if (strandedBreaks(tr.doc) > strandedBreaks(before)) return false; editor.view.dispatch(tr); @@ -369,15 +390,28 @@ export function markable(state: EditorState, type: MarkType): boolean { * `` put those bytes through the middle of the tag, and the * file reopened as a heading, two paragraphs and a list with no raw block anywhere in it. * - * Text with no blank line in it is fine everywhere, which is what the first line says: a fence and - * a raw block are `whitespace: "pre"` so an ordinary newline stays a newline, and the serializer - * writes a newline inside a table cell as the space a GFM cell can hold. + * Text with no blank line in it is fine everywhere else, and everywhere else is what the first line + * answers: a fence and a raw block are `whitespace: "pre"` so an ordinary newline stays a newline, + * and the serializer writes a newline inside a table cell as the space a GFM cell can hold. + * + * Inside a raw block the question is asked of the RESULT, because the blank line does not have to + * be in the paste. Text copied as whole lines carries the line ending they were copied with, and + * one of those pasted at the end of a line puts its newline against the newline already there. + * Neither string has a blank line in it and the block has one afterwards, which is the same lost + * construct arriving by arithmetic instead of by content: `
` with "hello\n" pasted + * at the end of that first line reopened as a raw block, a paragraph and a second raw block, with + * no toast and nothing refused. */ export function holdsText(state: EditorState, text: string): boolean { - if (!BLANK_LINE.test(text.replace(/\r\n?/g, "\n"))) return true; - return state.selection.ranges.every( - ({ $from, $to }) => $from.parent.type.name !== RAW && $to.parent.type.name !== RAW, - ); + const value = text.replace(/\r\n?/g, "\n"); + return state.selection.ranges.every(({ $from, $to }) => { + if ($from.parent.type.name !== RAW && $to.parent.type.name !== RAW) return true; + if (!$from.sameParent($to)) return !BLANK_LINE.test(value); + const parent = $from.parent; + const head = parent.textBetween(0, $from.parentOffset); + const tail = parent.textBetween($to.parentOffset, parent.content.size); + return !BLANK_LINE.test(head + value + tail); + }); } /** diff --git a/src/editor/paste.ts b/src/editor/paste.ts index e333e0c..ddf1dd4 100644 --- a/src/editor/paste.ts +++ b/src/editor/paste.ts @@ -38,6 +38,13 @@ // line is what ends an html block, so a paste of two paragraphs into one wrote them through the // middle of a preserved `` and the file came back as a heading, two paragraphs and a // list with no raw block in it. `holdsText` in src/editor/fits.ts is that question. +// +// And the sixth is that question asked of the wrong string. It read the clipboard and not the block +// the clipboard was going into, so a paste of whole lines, which carries the line ending they were +// copied with, put its newline against the newline already at the end of the line it landed on: no +// blank line in the payload, a blank line in the raw block, nothing refused and no toast. A drop +// was worse and asked nothing at all, and it is the one route that cannot be talked round, since a +// drop is the dragged content itself rather than something reparsed against where it lands. import { Extension } from "@tiptap/core"; import type { Editor, JSONContent } from "@tiptap/core"; @@ -47,7 +54,7 @@ import type { EditorState } from "@tiptap/pm/state"; import type { EditorView } from "@tiptap/pm/view"; import { __pastedCells as pastedCells, isInTable } from "@tiptap/pm/tables"; import { assetWrite } from "../api/files"; -import { carriesBlocks, fits, holdsText, place, placeable } from "./fits"; +import { carriesBlocks, fits, holdsText, inRaw, place, placeable, touchesRaw } from "./fits"; /** * This app's own paste handler, named so that a test can find it in the plugin list and say where @@ -270,7 +277,18 @@ export function createPaste(context: PasteContext): Extension { // a raw block arrives as one text node with the newlines still in it and no block // boundary anywhere: `carriesBlocks` says no, the insert is ProseMirror's, and the // bytes it writes end the html block halfway through the user's tag. - if (!carriesBlocks(slice) && takesText(view.state)) { + // + // A raw block never stands aside, whatever the slice looks like. "Better" above + // means marks, lists and tables kept, and a raw block holds text and nothing else, + // so what is better there is a hard break put into a node whose content is `text*` + // and a mark applied where `marks` is "", neither of which ProseMirror refuses: an + // inline slice carrying one break turned somebody's `
` into two raw blocks with + // a stray backslash written into the file. What this branch measures is + // `sliceText`, which is only what ProseMirror inserts when the slice is a bare text + // node, so standing aside on the strength of it is answering for a different insert + // from the one that happens. The text path below is the only one that leaves the + // block whole, and a raw target falls through to it. + if (!carriesBlocks(slice) && takesText(view.state) && !touchesRaw(view.state)) { if (holdsText(view.state, sliceText(slice))) return false; event.preventDefault(); context.onError(BLANK_LINE); @@ -300,10 +318,22 @@ export function createPaste(context: PasteContext): Extension { * meant by pointing at the middle of a fence. */ handleDrop(view: EditorView, event: DragEvent, slice: Slice) { - if (!carriesBlocks(slice)) return false; const at = view.posAtCoords({ left: event.clientX, top: event.clientY }); if (!at) return false; - if (fits(view.state.doc.resolve(at.pos), view.state.schema.nodes.paragraph)) return false; + const $at = view.state.doc.resolve(at.pos); + // A drop into a raw block is refused whatever it carries, and refused before the + // slice is looked at. A paste is reparsed against the block the caret is in, so one + // that lands in a raw block arrives as text; a drop is not, it is the dragged + // content itself, so an inline one carries hard breaks and marks into a node whose + // content is text and nothing else, and a plain text one carries whatever blank + // lines were in it. The block is the file's own bytes and a drop is a pointer + // gesture with nothing to ask, so nothing goes in. + if (inRaw($at)) { + event.preventDefault(); + return true; + } + if (!carriesBlocks(slice)) return false; + if (fits($at, view.state.schema.nodes.paragraph)) return false; event.preventDefault(); return true; }, diff --git a/src/editor/proofing.ts b/src/editor/proofing.ts index ea53bbf..092702b 100644 --- a/src/editor/proofing.ts +++ b/src/editor/proofing.ts @@ -296,17 +296,16 @@ async function pass(view: EditorView): Promise { prune(blocks); } -/** The misspelling under a position, if the menu should open over one. */ -function menuAt(view: EditorView, pos: number): boolean { - // A document held open while a conflict is resolved is one nothing may edit, and a menu whose - // every item is an edit has nothing to offer there. The underlines stay; the menu does not open. - if (!view.editable) return false; - const decorations = proofingKey.getState(view.state); - if (!decorations) return false; - const found = decorations.find(pos, pos); - if (found.length === 0) return false; - // A position at the seam between two words touches both, so a hit that actually contains it wins. - const hit = found.find((deco) => deco.from < pos && pos < deco.to) ?? found[0]; +/** + * Puts the menu over one underlined word, whichever of the three ways in found it. + * + * `fromKeyboard` is the only thing that separates a chord from a pointer once the word is known, + * and the menu reads it to decide whether to take focus. A click has already put the caret where + * the user wanted it, so a menu that took focus from that click would have broken the commoner half + * of what clicking a word means; a chord has no other way of reaching the items it has just + * offered. + */ +function openFor(view: EditorView, hit: Decoration, fromKeyboard: boolean): boolean { const spec = hit.spec as { word?: unknown; suggestions?: unknown }; if (typeof spec.word !== "string") return false; @@ -324,10 +323,61 @@ function menuAt(view: EditorView, pos: number): boolean { top: start.top, // A word that wraps across two lines ends on the lower one, which is where the menu belongs. bottom: Math.max(start.bottom, end.bottom), + fromKeyboard, }); return true; } +/** The misspelling under a position, if the menu should open over one. */ +function menuAt(view: EditorView, pos: number): boolean { + // A document held open while a conflict is resolved is one nothing may edit, and a menu whose + // every item is an edit has nothing to offer there. The underlines stay; the menu does not open. + if (!view.editable) return false; + const decorations = proofingKey.getState(view.state); + if (!decorations) return false; + const found = decorations.find(pos, pos); + if (found.length === 0) return false; + // A position at the seam between two words touches both, so a hit that actually contains it wins. + const hit = found.find((deco) => deco.from < pos && pos < deco.to) ?? found[0]; + return openFor(view, hit, false); +} + +/** + * The same menu, opened by a chord instead of by a pointer, over the misspelling at the caret or + * the nearest one to it. + * + * Nearest within the caret's own paragraph and no further. The menu is placed at the viewport + * coordinates of the word it is about, so a search that ran to the end of the document would open a + * menu somewhere off screen about a mistake the user cannot see, and correcting a word you are not + * looking at is not what the key was pressed for. Inside a paragraph the distance is almost always + * zero or a character or two: this is the word just typed, with the caret still sitting against its + * end. + * + * False when there is nothing to offer, which is what the caller turns into a line of explanation. + */ +export function openSpellingMenu(): boolean { + const view = activeView; + if (!view || view.isDestroyed || !view.editable) return false; + const decorations = proofingKey.getState(view.state); + if (!decorations) return false; + + const $head = view.state.selection.$head; + if (!$head.parent.isTextblock) return false; + + let best: Decoration | null = null; + let nearest = Infinity; + for (const deco of decorations.find($head.start(), $head.end())) { + const gap = $head.pos < deco.from ? deco.from - $head.pos : Math.max($head.pos - deco.to, 0); + // Strictly nearer, so a caret sitting exactly between two of them takes the earlier word, + // which is the one it was most likely just finished typing. + if (gap >= nearest) continue; + best = deco; + nearest = gap; + } + if (!best) return false; + return openFor(view, best, true); +} + /** * The debounce, the store subscription and the one view a menu can act on, for as long as there is * an editor to act on. diff --git a/src/keys/bindings.ts b/src/keys/bindings.ts index 418897c..32e52fd 100644 --- a/src/keys/bindings.ts +++ b/src/keys/bindings.ts @@ -20,7 +20,7 @@ import type { CommandId } from "./commands"; */ export type KeyContext = "global" | "document" | "overlay"; -export type BindingGroup = "File" | "Navigation" | "Search" | "View" | "App"; +export type BindingGroup = "File" | "Navigation" | "Search" | "Editing" | "View" | "App"; interface BindingBase { /** Every combo that runs it. The sheet shows them all; the dispatcher accepts any. */ @@ -81,6 +81,19 @@ export const BINDINGS: readonly Binding[] = [ allowInInput: true, }, + // The Mac's own key for this. AppKit gives every NSTextView Cmd+; for "Check Spelling", so it is + // the one chord a user is liable to try before reading anything, and it is free: nothing else in + // this table binds it and neither does any extension in src/editor, whose chords are all Mod with + // a letter, a digit or an editing key (src/editor/fits.test.ts enumerates them). Marked for input + // because the caret is in the contenteditable document every time this is pressed. + { + keys: ["cmd+;"], + command: "correct-spelling", + context: "document", + group: "Editing", + allowInInput: true, + }, + { keys: ["cmd+\\"], command: "toggle-sidebar", @@ -104,7 +117,14 @@ export const BINDINGS: readonly Binding[] = [ }, ]; -export const GROUPS: readonly BindingGroup[] = ["File", "Navigation", "Search", "View", "App"]; +export const GROUPS: readonly BindingGroup[] = [ + "File", + "Navigation", + "Search", + "Editing", + "View", + "App", +]; const isMac = typeof navigator !== "undefined" && /mac|iphone|ipad/i.test(navigator.userAgent ?? ""); diff --git a/src/keys/commands.ts b/src/keys/commands.ts index 0fd2ecf..947c767 100644 --- a/src/keys/commands.ts +++ b/src/keys/commands.ts @@ -44,6 +44,7 @@ export type CommandId = | "editor-width-normal" | "editor-width-wide" | "toggle-spelling" + | "correct-spelling" | "shortcuts" | "settings" | "check-updates" @@ -319,6 +320,18 @@ const TABLE: Record> = { run: () => useProofing.getState().toggle(), }, + // Dispatched, unlike the one above it, because its whole result is the correction menu on screen + // and the component that draws that menu is the only thing here that knows where the caret is. + // + // Not in the palette either. What it acts on is the misspelling beside the caret, which is a + // target the palette cannot show a row for, and by the time a command name has been typed at a + // field the caret is no longer the one the user meant. + "correct-spelling": { + label: "Correct Spelling", + palette: false, + run: () => dispatch("correct-spelling"), + }, + shortcuts: { label: "Keyboard Shortcuts", palette: true, run: () => dispatch("shortcuts") }, settings: { label: "Settings…", palette: true, run: () => dispatch("settings") }, "check-updates": { diff --git a/src/linkRewrite.ts b/src/linkRewrite.ts index 249c416..13c9576 100644 --- a/src/linkRewrite.ts +++ b/src/linkRewrite.ts @@ -33,23 +33,25 @@ // And every write is proved before it goes out. The spliced text is parsed again and compared // against what the splice was meant to do: the same destinations in the same order, each holding // the href it was given, each sitting at the offset the replacements before it shift it to. A file -// that does not answer exactly that is not written at all. The proof sits between the splice and -// the only call to `fileWrite` in this module, with no condition in front of it, so there is no -// path to disk that goes around it. +// that does not answer exactly that is not written at all. The proof sits inside `rewriteLinksIn`, +// between the splice and the bytes coming back out of it, with no condition in front of it, so +// neither the write below nor the one src/document.ts makes for the open document has a path to +// disk that goes around it. // // Which files get looked at is decided by walking the open roots rather than by asking the index. // `backlinksFor` is the cheap route and it is deliberately unused: the index is derived state with // no freshness this module can check, and a stale answer is a file quietly left broken, which is // the one outcome this project ranks below doing nothing. The sweep reads every markdown document -// in every open root, skipping the parse for text that cannot name the thing that moved, and it -// gives up and says so rather than reading more documents than `MAX_SWEEP_DOCUMENTS`. A file -// outside every open root is never seen by anything here and never will be. +// in every open root, a gitignored one included, since the tree's filter is about what a sidebar +// should show and not about whether a link is worth keeping. It skips the parse for text that +// cannot name the thing that moved, and it gives up and says so rather than reading more documents +// than `MAX_SWEEP_DOCUMENTS`. A file outside every open root is never seen by anything here and +// never will be. import type { Root } from "mdast"; import { fileRead, fileWrite } from "./api/files"; -import { treeRead } from "./api/roots"; -import { documentChangedOnDisk } from "./document"; -import type { FileNode } from "./ipc"; +import { sweepDocuments } from "./api/roots"; +import { rewriteOpenDocument } from "./document"; import { relativeFrom, resolveRelative } from "./links"; import { BOM } from "./markdown/frontmatter"; import { parseToMdast } from "./markdown/handlers"; @@ -79,7 +81,11 @@ export interface LinkRewriteReport { refused: number; /** Files that could not be read, could not be written, or that another writer reached first. */ failed: readonly FailedFile[]; - /** The open document, when it was left alone because its buffer holds an edit nothing else has. */ + /** + * The open document, when writing it would have gone under what the user is looking at: an + * unsaved edit, a save already on the wire, or bytes the editor cannot vouch for because the + * file moved on under it. + */ heldBack: string | null; /** * Whether every document that could hold a link into the move was actually read. `partial` means @@ -603,30 +609,79 @@ async function rewriteFile(path: string, move: Move, marker: string | null): Pro return { kind: "rewritten", path: newPath, refused: outcome.refused }; } -function documentsUnder(node: FileNode, into: string[]): void { - if (node.kind === "dir") { - for (const child of node.children) documentsUnder(child, into); - return; +/** + * The same work for the file the user is looking at, handed to src/document.ts rather than done + * here. + * + * That module owns the one write the open document is allowed, and it is the only place that can + * ask whether the buffer is clean and write in the same run of the event loop. Doing it from out + * here meant reading `dirty`, then two parses of the file, then a write, which on a document of any + * size is long enough for a keystroke to land in the middle and come back at the user as a conflict + * over a change this app made itself. + * + * `rewriteLinksIn` stays here, because what a file's new bytes are is this module's question and + * nothing else's. It is handed over as a callback, and everything it works out other than the bytes + * comes back out of the closure. + * + * Only for the document that did not move. One that did is at a path the store does not have open + * yet, and its caller reopens it there the moment this returns. + */ +async function rewriteOpenFile( + path: string, + move: Move, + marker: string | null, +): Promise { + const outcomes: TextOutcome[] = []; + const result = await rewriteOpenDocument(path, (text) => { + if (marker !== null && !text.includes(marker) && !text.includes("%")) return null; + const outcome = rewriteLinksIn(text, path, move); + outcomes.push(outcome); + return outcome.text; + }); + + const outcome = outcomes.length === 0 ? null : outcomes[0]; + // Asked before the write's own answer, and it has to be. A file whose links could not be matched + // to its text is one the callback returned null for, and null is also how it says there was + // nothing to do, so the outcome is the only thing that can tell those two apart. + if (outcome !== null && outcome.unprovable) { + return { kind: "failed", path, reason: "its links could not be matched to its text" }; } - // Markdown only. A .txt holding something that looks like a link is not markdown, and parsing one - // as markdown to edit it would be this module deciding what a file is against its own extension. - if (documentKindForPath(node.path) === "markdown") into.push(node.path); + if (result.kind === "held") return "held"; + if (result.kind === "failed") return { kind: "failed", path, reason: result.reason }; + const refused = outcome === null ? 0 : outcome.refused; + return result.kind === "rewritten" + ? { kind: "rewritten", path, refused } + : { kind: "unchanged", refused }; } -/** Every markdown document in every open root, read fresh so the move is already in it. */ -async function sweepCandidates(): Promise { +/** + * Every markdown document in every open root, read fresh so the move is already in it, and whether + * that is all of them. + * + * `sweep_documents` and not `tree_read`: the tree hides what the folder's gitignore hides, which is + * the right answer for a sidebar and the wrong one for a writer. `complete` is false when a root + * came back holding more documents than the sweep will read, which the backend says by handing back + * one path past the cap. + */ +async function sweepCandidates(): Promise<{ paths: string[]; complete: boolean } | null> { const roots = useWorkspace.getState().roots; - const found: string[] = []; + const paths: string[] = []; + let complete = true; for (const root of roots) { + let batch: string[]; try { - documentsUnder(await treeRead(root.id), found); + // One past the cap, so a root that goes over costs the cap rather than the folder and is + // still visible as having gone over. + batch = await sweepDocuments(root.id, MAX_SWEEP_DOCUMENTS + 1); } catch { // A root that will not answer is a root whose documents were not looked at, and the report // has to say so rather than count the ones that did answer as the whole workspace. return null; } + if (batch.length > MAX_SWEEP_DOCUMENTS) complete = false; + paths.push(...batch); } - return found; + return { paths, complete }; } async function inParallel(items: readonly T[], run: (item: T) => Promise): Promise { @@ -650,7 +705,10 @@ function describe(report: LinkRewriteReport): string | null { trouble.push(`${count(report.failed.length, "file")} could not be updated`); } if (report.heldBack !== null) { - trouble.push(`${baseName(report.heldBack)} has unsaved changes and was left alone`); + // Not "has unsaved changes" any more. That is the usual reason and it is no longer the only + // one: a save still on the wire and a file the editor has lost track of hold it back too, and + // naming a cause the user can check and find is not true is worse than naming none. + trouble.push(`${baseName(report.heldBack)} is open and was left as it is`); } if (report.refused > 0) { trouble.push(`${count(report.refused, "link")} could not be matched exactly`); @@ -687,8 +745,12 @@ function describe(report: LinkRewriteReport): string | null { * a failure part way through leaves the files before it correctly rewritten, the file that failed * with every byte it had, and the ones after it untouched; that is what `failed` is for, and there * is no rollback because a rollback is another round of writes that can fail in the same way. And - * the open document is skipped outright when its buffer is dirty, because the buffer is the only - * copy of that edit and writing under it would put the two on a collision the user has to resolve. + * the open document is skipped when its buffer is dirty, and equally when a save of it is already + * on the wire, because in both cases the bytes in front of the user are the ones that count and + * writing under them would put the two on a collision the user has to resolve. When it is written + * it goes through src/document.ts rather than around it, and the buffer is handed the new links + * before the bytes leave, so the file and the document on screen cannot come out of this as two + * copies of the same move. */ export async function rewriteLinksForMove(move: Move): Promise { const rewritten: string[] = []; @@ -704,14 +766,18 @@ export async function rewriteLinksForMove(move: Move): Promise path === move.to || isUnder(path, move.to))); - // A document dragged into a folder the tree does not list, an ignored one, is not in the sweep at - // all, and its own links are the half of this that needs no sweep to be answered. + // An ignored document is in the sweep now, so what is left for this line is the case that still + // is not: a document moved to somewhere outside every open root is in no list this module can + // ask for, and its own links are the half of this that needs no sweep to be answered. if (documentKindForPath(move.to) === "markdown") inside.add(move.to); let outside = documents.filter((path) => !inside.has(path)); - if (candidates === null || outside.length > MAX_SWEEP_DOCUMENTS) { + // `complete` is not something `outside.length` can stand in for. A list cut off at the cap whose + // every entry landed inside the move leaves nothing outside at all, which reads exactly like a + // workspace that had nothing to update while documents were quietly dropped. + if (candidates === null || !candidates.complete || outside.length > MAX_SWEEP_DOCUMENTS) { coverage = "partial"; outside = []; } @@ -743,17 +809,20 @@ export async function rewriteLinksForMove(move: Move): Promise {}); - } + take(await rewriteFile(open.path, move, open.marker)); } } diff --git a/src/markdown/bridge.test.ts b/src/markdown/bridge.test.ts index afbd015..37c3db0 100644 --- a/src/markdown/bridge.test.ts +++ b/src/markdown/bridge.test.ts @@ -51,7 +51,9 @@ describe("the modelled inventory", () => { expect([...marks].sort()).toEqual(["code", "em", "link", "strikethrough", "strong"]); const link = paragraph.child(paragraph.childCount - 2); - expect(link.marks[0].attrs).toEqual({ href: "./a.md", title: "T" }); + // `run` is 0 because the link has no link beside it. It is only ever the other value on a link + // whose immediate neighbour goes to the same place, which is what keeps the pair two marks. + expect(link.marks[0].attrs).toEqual({ href: "./a.md", title: "T", run: 0 }); }); it("maps images, hard breaks and inline math", () => { diff --git a/src/markdown/index.ts b/src/markdown/index.ts index 5303148..f17ed7d 100644 --- a/src/markdown/index.ts +++ b/src/markdown/index.ts @@ -33,9 +33,9 @@ import { rawNode } from "../model/doc"; import { BOM, normaliseSource, splitFrontmatter, withFrontmatter } from "./frontmatter"; import { parseToMdast } from "./handlers"; import { buildDoc } from "./parse"; -import { serializeBody } from "./serialize"; +import { type Refused, serializeBody } from "./serialize"; -export type { MarkdownDocument }; +export type { MarkdownDocument, Refused }; /** * Parses one markdown file into the document the editor edits. @@ -75,9 +75,21 @@ export function parseMarkdown(source: string, path: string): MarkdownDocument { * first byte, and that is the only thing it is told. A block whose bytes open `---` or `+++` is * read back as frontmatter when it sits at the top of a file and is perfectly ordinary two lines * down, so the defence against it has to be skipped for a body that already has a prefix coming. + * + * `refused` is the traffic in the other direction and the only thing that goes back up: a caller + * that hands one in is told, by the time this returns, whether the bytes it is getting are missing + * an edit the document still holds. Nothing else about it is optional, so a caller with no use for + * the answer leaves it out. */ -export function serializeMarkdown(document: MarkdownDocument, doc: ProseMirrorNode): string { - return withFrontmatter(document.frontmatter, serializeBody(doc, document.frontmatter === null)); +export function serializeMarkdown( + document: MarkdownDocument, + doc: ProseMirrorNode, + refused?: Refused, +): string { + return withFrontmatter( + document.frontmatter, + serializeBody(doc, document.frontmatter === null, refused), + ); } /** @@ -122,14 +134,23 @@ export function parsePlainText(source: string, path: string): MarkdownDocument { } /** - * The counterpart of `parsePlainText`, taking the same pair as `serializeMarkdown` so a caller can - * pick the two functions by `documentKindForPath` and then stop caring which it got. + * The counterpart of `parsePlainText`, taking the same arguments as `serializeMarkdown` so a caller + * can pick the two functions by `documentKindForPath` and then stop caring which it got. * * No escaping, no reflowing, no normalising: the characters in the document are the bytes of the * file. */ -export function serializePlainText(document: MarkdownDocument, doc: ProseMirrorNode): string { +export function serializePlainText( + document: MarkdownDocument, + doc: ProseMirrorNode, + refused?: Refused, +): string { void document; + // Taken and ignored so the two serializers stay one type. `bridgeFor` in src/document.ts hands + // its caller whichever of the pair the path calls for, and a call the other half has no parameter + // for does not typecheck against the union. There is nothing here to refuse either way: plain + // text has no raw blocks and no lists, only lines. + void refused; const lines: string[] = []; doc.forEach((block) => lines.push(block.textContent)); return lines.join("\n"); diff --git a/src/markdown/parse.ts b/src/markdown/parse.ts index bb988b7..8d8ea70 100644 --- a/src/markdown/parse.ts +++ b/src/markdown/parse.ts @@ -13,7 +13,7 @@ // is reopened. import type { Mark, Node as ProseMirrorNode } from "@tiptap/pm/model"; -import type { BlockContent, DefinitionContent, List, ListItem, PhrasingContent, RootContent, Table } from "mdast"; +import type { BlockContent, DefinitionContent, Link, List, ListItem, PhrasingContent, RootContent, Table } from "mdast"; import { calloutKindFromLabel, rawNode } from "../model/doc"; import { isFrontmatterNode } from "./frontmatter"; import type { ColumnAlign, HeadingLevel } from "../model/doc"; @@ -424,9 +424,13 @@ function toggleFrom(children: RootContent[], opening: number, closing: number, t * Soft line breaks stay inside the text as the newlines mdast gives them, so a paragraph that was * hard wrapped at some column on disk is written back wrapped at the same places. Rewrapping is a * whole file diff on a document nobody meaningfully edited. + * + * The leaves go into one accumulator shared by every level of the mark nesting, rather than each + * level building its own and the caller spreading it in. A link has to know what leaf is + * immediately to its left to answer `runAfter`, and the emphasis or strikethrough that leaf came + * out of is no part of that question, so the walk cannot be the thing that hides it. */ -function inlineFrom(nodes: PhrasingContent[], marks: readonly Mark[]): ProseMirrorNode[] | null { - const out: ProseMirrorNode[] = []; +function inlineFrom(nodes: PhrasingContent[], marks: readonly Mark[], out: ProseMirrorNode[] = []): ProseMirrorNode[] | null { for (const node of nodes) { switch (node.type) { case "text": { @@ -440,9 +444,7 @@ function inlineFrom(nodes: PhrasingContent[], marks: readonly Mark[]): ProseMirr // addToSet, not a spread: Mark.setFrom sorts but does not deduplicate, and GFM really does // nest a mark inside itself for "~~a ~~b~~ c~~". Two identical marks on one text node // serialize as four tildes, which reopen as literal text and then grow on every save. - const inner = inlineFrom(node.children, mark.create().addToSet(marks)); - if (!inner) return null; - out.push(...inner); + if (!inlineFrom(node.children, mark.create().addToSet(marks), out)) return null; break; } case "inlineCode": { @@ -458,12 +460,11 @@ function inlineFrom(nodes: PhrasingContent[], marks: readonly Mark[]): ProseMirr // The block goes back as raw source instead: keeping the bytes is the honest answer, and // dropping a destination quietly is the one thing this bridge is here not to do. if (marks.some((mark) => mark.type === m.link)) return null; - const mark = m.link.create({ href: node.url, title: node.title ?? null }); - const inner = inlineFrom(node.children, mark.addToSet(marks)); + const mark = m.link.create({ href: node.url, title: node.title ?? null, run: runAfter(out, node) }); + const at = out.length; // A mark needs something to sit on, so a link with no text at all, `[](./x.md)`, has // nowhere to live in the document and would be dropped along with its destination. - if (!inner || inner.length === 0) return null; - out.push(...inner); + if (!inlineFrom(node.children, mark.addToSet(marks), out) || out.length === at) return null; break; } case "image": @@ -481,3 +482,19 @@ function inlineFrom(nodes: PhrasingContent[], marks: readonly Mark[]): ProseMirr } return out; } + +/** + * Whether this link has to be told apart from the one before it, and how. + * + * ProseMirror has no way to hold two adjacent leaves whose marks compare equal: `Fragment.fromArray` + * welds the text nodes together, and the serializer's own fold groups by the same equality, so + * `[a](/)[b](/)` was one link by the time anything could see it was two. `run` is what makes the + * two marks unequal, and it alternates rather than counting so that it is 0 on every link in every + * file that does not have a neighbour to be told apart from. + */ +function runAfter(out: ProseMirrorNode[], node: Link): number { + const before = out[out.length - 1]?.marks.find((mark) => mark.type === m.link); + if (!before) return 0; + if (before.attrs.href !== node.url || before.attrs.title !== (node.title ?? null)) return 0; + return before.attrs.run === 0 ? 1 : 0; +} diff --git a/src/markdown/serialize.ts b/src/markdown/serialize.ts index 39c6227..cfe4bbd 100644 --- a/src/markdown/serialize.ts +++ b/src/markdown/serialize.ts @@ -89,6 +89,19 @@ const EMAIL_SHAPED = /^[^\s<>@]+@[^\s<>@]+$/; */ const ANGLE_SHAPED = /^[A-Za-z][A-Za-z0-9+.-]+:[^\s<>]*$/; +/** + * What the writer could not put in the file, filled in on the way past. + * + * There is one of these and the comment above `separate` is the whole of why: a raw block beside a + * list whose edit no spelling can hold is written from the bytes the file gave it. That decision + * stands. What this adds is that it is not made in silence, because a caller that cannot see it + * marks the buffer clean over bytes the user's edit is not in. A caller that does not care passes + * nothing. + */ +export interface Refused { + rawBlock: boolean; +} + /** * The document as the body of a file, which is one question more than the document as blocks. * @@ -112,8 +125,8 @@ const ANGLE_SHAPED = /^[A-Za-z][A-Za-z0-9+.-]+:[^\s<>]*$/; * first save and then gained a byte every save for the rest of its life. `serializeMarkdown` knows * whether it is about to put a prefix in front and is the only thing that can answer, so it does. */ -export function serializeBody(doc: ProseMirrorNode, atStartOfFile = true): string { - const tree = docToMdast(doc); +export function serializeBody(doc: ProseMirrorNode, atStartOfFile = true, refused?: Refused): string { + const tree = docToMdast(doc, refused); const body = stringifyMdast(tree); if (!atStartOfFile || !swallowed(body)) return body; @@ -133,14 +146,14 @@ function swallowed(body: string): boolean { return isFrontmatterNode(parseToMdast(body).children[0]); } -export function docToMdast(doc: ProseMirrorNode): Root { +export function docToMdast(doc: ProseMirrorNode, refused?: Refused): Root { const blocks: RootContent[][] = []; const nodes: ProseMirrorNode[] = []; doc.forEach((child) => { nodes.push(child); blocks.push(verifiedBlock(child)); }); - separate(blocks, nodes); + separate(blocks, nodes, refused); return { type: "root", children: blocks.flat() }; } @@ -231,19 +244,38 @@ function listToMdast(node: ProseMirrorNode, ordered: boolean, start: number): Ro // cannot be written a single line apart whatever the attribute says, and one of those makes the // whole list loose here rather than on the next save. const loose = node.attrs.tight !== true || contents.some(runsTogether); + if (!loose) contents.forEach(respellRules); const children: ListItem[] = contents.map((content, index) => ({ type: "listItem", spread: loose, checked: checks[index], children: content })); return ordered ? { type: "list", ordered: true, start, spread: loose, children } : { type: "list", ordered: false, start: null, spread: loose, children }; } +/** + * The rules an item has to spell the other way, asked only of an item about to be written tight. + * + * `---` is the house rule and is a setext underline wherever a paragraph is still open above it, + * which inside a tight item is the only place the line under a paragraph can be. `***` is the same + * rule in the one spelling that interrupts a paragraph, and it is the same swap `serializeBody` + * makes for a rule that lands on the first byte of a file. A loose item keeps the house spelling, + * because the blank line in front of it has already closed the paragraph. + */ +function respellRules(blocks: Array): void { + for (let index = 1; index < blocks.length; index += 1) { + const right = blocks[index]; + if (right.type === "thematicBreak" && leftOpen(blocks[index - 1]) === "paragraph") withOtherRule(right, true); + } +} + /** * Whether an item's blocks would run into one another if they were written a single line apart. * * Two blocks written flush are two blocks only when the second one opens a block of its own, which * is less often than it looks. A paragraph is still open at the end of its last line, so the line - * under it joins it: a second paragraph runs on into the first, and `---` under a paragraph is a - * setext heading rather than a rule. A table and an html block are worse than open: every line up - * to the next blank one is another row or more html, whatever it says. And two quotes written - * flush are one quote, because the second one's markers are read as more of the first. + * under it joins it, and a second paragraph runs on into the first. A rule is the one construct + * that is only half caught by that: `---` under a paragraph is a setext heading rather than a rule, + * but `***` is the same rule in a spelling that interrupts, and `respellRules` has already put the + * item's rules into it by the time this is asked. A table and an html block are worse than open: + * every line up to the next blank one is another row or more html, whatever it says. And two quotes + * written flush are one quote, because the second one's markers are read as more of the first. * * Only neighbours are asked about, because the blocks are written in order and a pair of them is * the whole of what stands between one block and the next. @@ -312,6 +344,10 @@ function opensBlock(block: BlockContent | DefinitionContent): boolean { case "blockquote": case "table": case "html": + // A rule interrupts a paragraph in every spelling markdown has for it except `---`, which is a + // setext underline there, and `respellRules` has already put the item's rules into the other + // one by the time this is asked. + case "thematicBreak": return true; case "list": { // A list interrupts a paragraph only when it counts from one and its first item says @@ -765,20 +801,24 @@ function verifiedBlock(node: ProseMirrorNode): RootContent[] { * block, in every document that has no list beside it, is written exactly as it always was. * src/editor/ owns whether the edit should have been possible at all. */ -function separate(blocks: RootContent[][], nodes: ProseMirrorNode[]): void { +function separate(blocks: RootContent[][], nodes: ProseMirrorNode[], refused?: Refused): void { // Twice at the most: the second pass is for the lists settled against bytes the first pass then // put back, and a block already holding its source is not put back again, so it cannot go round. - if (spellEveryList(blocks, nodes)) spellEveryList(blocks, nodes); + if (spellEveryList(blocks, nodes, refused)) spellEveryList(blocks, nodes, refused); } /** One pass over every list, and whether any raw block beside one gave its edit up to it. */ -function spellEveryList(blocks: RootContent[][], nodes: ProseMirrorNode[]): boolean { +function spellEveryList(blocks: RootContent[][], nodes: ProseMirrorNode[], refused?: Refused): boolean { let gaveUp = false; for (let index = 0; index < blocks.length; index += 1) { if (listIn(blocks[index]) === null || spelledApart(blocks, index)) continue; if (keepSource(blocks, nodes, index - 1) || keepSource(blocks, nodes, index + 1)) { gaveUp = true; + // A flag rather than a count, and the second pass above is why: what the caller has to be + // told is that this save is not carrying an edit somebody made, and raising the same flag + // twice over the same block says exactly that once. + if (refused) refused.rawBlock = true; spelledApart(blocks, index); } } @@ -981,7 +1021,24 @@ function sameAttrs(a: Record, b: Record): bool } function sameMarks(a: readonly Mark[], b: readonly Mark[]): boolean { - return a.length === b.length && a.every((mark, at) => mark.type.name === b[at].type.name && sameAttrs(mark.attrs, b[at].attrs)); + return a.length === b.length && a.every((mark, at) => mark.type.name === b[at].type.name && sameAttrs(spelled(mark), spelled(b[at]))); +} + +/** + * A mark's attributes minus the ones no spelling of it carries, which is `run` and nothing else. + * + * `run` is not in the file. It exists so that two adjacent links to one destination are two marks + * rather than one, and the parser numbers it from the block it has just read, so a link whose + * neighbour has since been deleted carries a number the re-parse of it will not produce. Comparing + * it here fails a block that came back perfectly, and the ladder answers that by writing the block + * at a plainer fidelity: a code span holding a line ending loses it, on a paragraph nobody edited. + * The difference `run` exists to make is two links written as one, and that is a difference in the + * text and in the child count, which this walk already sees. + */ +function spelled(mark: Mark): Record { + if (mark.type.name !== "link") return mark.attrs; + const { run: _run, ...rest } = mark.attrs; + return rest; } /** diff --git a/src/markdown/writer.test.ts b/src/markdown/writer.test.ts index 9aef2ce..436a497 100644 --- a/src/markdown/writer.test.ts +++ b/src/markdown/writer.test.ts @@ -192,6 +192,7 @@ describe("a list written tight", () => { ["a table", [paragraph(text("a")), TABLE]], ["an equation", [paragraph(text("a")), n.mathBlock.createChecked({ latex: "y" })]], ["a quote", [paragraph(text("a")), n.blockquote.createChecked(null, paragraph(text("q")))]], + ["a rule, which is written as the spelling that interrupts a paragraph", [paragraph(text("a")), n.horizontalRule.createChecked()]], ]; for (const [what, content] of tight) { @@ -206,7 +207,6 @@ describe("a list written tight", () => { it("writes an item loose when its blocks would run into each other", () => { const loose: Array<[string, ProseMirrorNode[]]> = [ ["two paragraphs run together into one", [paragraph(text("a")), paragraph(text("b"))]], - ["a rule under a paragraph is a setext heading", [paragraph(text("a")), n.horizontalRule.createChecked()]], ["a paragraph under a nested list is a lazy continuation of it", [paragraph(text("a")), NESTED, paragraph(text("e"))]], ["a paragraph under a quote is a lazy continuation of it", [paragraph(text("a")), n.blockquote.createChecked(null, paragraph(text("q"))), paragraph(text("e"))]], ["a paragraph under a table is another row of it", [paragraph(text("a")), TABLE, paragraph(text("e"))]], diff --git a/src/model/schema.test.ts b/src/model/schema.test.ts index b5aeee6..7cf2e53 100644 --- a/src/model/schema.test.ts +++ b/src/model/schema.test.ts @@ -235,9 +235,12 @@ describe("callout and toggle", () => { describe("inline", () => { it("keeps image and link attributes", () => { expect(n.image.create().attrs).toEqual({ src: "", alt: null, title: null }); - expect(m.link.create().attrs).toEqual({ href: null, title: null }); + // `run` is the third one, and it is 0 here because it is 0 on every link that does not sit + // next to another link to the same place. It is what makes two of those two marks instead of + // one, and src/markdown/parse.ts is where it gets a value other than this. + expect(m.link.create().attrs).toEqual({ href: null, title: null, run: 0 }); const link = m.link.create({ href: "./a.md", title: "A" }); - expect(link.toJSON()).toEqual({ type: "link", attrs: { href: "./a.md", title: "A" } }); + expect(link.toJSON()).toEqual({ type: "link", attrs: { href: "./a.md", title: "A", run: 0 } }); }); it("lets a code span sit inside a link and inside emphasis", () => { diff --git a/src/model/schema.ts b/src/model/schema.ts index 0f31905..2feadd3 100644 --- a/src/model/schema.ts +++ b/src/model/schema.ts @@ -102,12 +102,31 @@ export const nodes: { [name in NodeName]: NodeSpec } = { toDOM: () => ["p", 0], }, + // `whitespace: "pre"` is the same three part sentence as the paragraph's, for the same reason: a + // setext heading is the one heading markdown lets an author wrap by hand, and the wrap is theirs. + // src/markdown/parse.ts builds a heading through the same call a paragraph goes through, so the + // soft break stays a newline in the text, and prose.css draws the whole surface pre-wrap, so the + // newline is on screen where the author put it. Left at the default, prosemirror-view read every + // one of those wraps back out of the editor's own DOM as a `hardBreak` on the next keystroke, and + // mdast writes a break inside a setext heading as a backslash and a line ending, so one character + // typed into a two line heading put a backslash into the user's own words. + // + // It holds at every level and not only at the two underlined ones. The deeper four have no setext + // spelling and so no hand wrap to keep, but they can still hold a line ending, spelled ` ` on + // disk, and it round trips; left at the default a keystroke turned that into a break, which those + // levels write out as a space. On screen until the next save, and gone after it. + // + // The parse rules say the opposite on purpose, and a rule's own answer outranks the node's, which + // is the paragraph's reasoning verbatim: whitespace is significant in a heading THIS editor + // rendered, and meaningless in an `

` off somebody else's page, where the newlines and the + // runs of spaces are the html source's own indentation. heading: { content: "inline*", group: "block", defining: true, + whitespace: "pre", attrs: { level: { default: 1, validate: "number" } }, - parseDOM: HEADING_LEVELS.map((level) => ({ tag: `h${level}`, attrs: { level } })), + parseDOM: HEADING_LEVELS.map((level) => ({ tag: `h${level}`, attrs: { level }, preserveWhitespace: false })), toDOM: (node) => [`h${node.attrs.level}`, 0], }, @@ -400,19 +419,31 @@ export const nodes: { [name in NodeName]: NodeSpec } = { }; export const marks: { [name in MarkName]: MarkSpec } = { + // `run` is not a property of the link, it is what tells one link from the one beside it. Marks + // compare by type and attributes, and a document has no way to hold two adjacent leaves whose + // marks compare equal: `Fragment.fromArray` welds the text nodes into one and the serializer's + // own fold groups by the same equality, so `[a](/)[b](/)` was a single link before anything in + // the app could see it was two. It alternates between 0 and 1 rather than counting, so it leaves + // the default on every link that has no neighbour to be told apart from, which is every link in + // very nearly every file, and so a block re-parsed on its own gives the same number back. + // + // The DOM half is not decoration. prosemirror-view re-parses the editor's own DOM after some + // input, and a pair that came out of that parse without `data-run` is a pair merged back into + // one between the keystroke and the save. link: { inclusive: false, attrs: { href: { default: null, validate: "string|null" }, title: { default: null, validate: "string|null" }, + run: { default: 0, validate: "number" }, }, parseDOM: [ { tag: "a[href]", - getAttrs: (dom) => ({ href: dom.getAttribute("href"), title: dom.getAttribute("title") }), + getAttrs: (dom) => ({ href: dom.getAttribute("href"), title: dom.getAttribute("title"), run: dom.getAttribute("data-run") === "1" ? 1 : 0 }), }, ], - toDOM: (mark) => ["a", { href: mark.attrs.href, title: mark.attrs.title }, 0], + toDOM: (mark) => ["a", { href: mark.attrs.href, title: mark.attrs.title, "data-run": mark.attrs.run ? "1" : null }, 0], }, strong: { diff --git a/src/store/useProofing.ts b/src/store/useProofing.ts index 88083c3..074f864 100644 --- a/src/store/useProofing.ts +++ b/src/store/useProofing.ts @@ -43,6 +43,9 @@ export interface ProofTarget { left: number; top: number; bottom: number; + /** True when a chord opened the menu rather than a pointer, which is the whole of what decides + * whether the menu takes focus. src/components/ProofPopover.tsx says why a click must not. */ + fromKeyboard: boolean; } /** "missing" is both a checker that said no and a command that was not there to ask. */ diff --git a/src/store/useSearch.test.ts b/src/store/useSearch.test.ts index 306fa5c..f3f104a 100644 --- a/src/store/useSearch.test.ts +++ b/src/store/useSearch.test.ts @@ -19,6 +19,7 @@ vi.mock("../api/roots", () => ({ rootOpen: vi.fn(), rootClose: vi.fn(), treeRead: vi.fn(), + sweepDocuments: vi.fn(), revealInFinder: vi.fn(), openExternal: vi.fn(), })); diff --git a/src/store/useWorkspace.test.ts b/src/store/useWorkspace.test.ts index fc68cc2..31a9009 100644 --- a/src/store/useWorkspace.test.ts +++ b/src/store/useWorkspace.test.ts @@ -9,6 +9,7 @@ const roots = vi.hoisted(() => ({ rootOpen: vi.fn(), rootClose: vi.fn(), treeRead: vi.fn(), + sweepDocuments: vi.fn(), revealInFinder: vi.fn(), openExternal: vi.fn(), })); @@ -110,6 +111,10 @@ beforeEach(() => { file(`${path}/logo.png`, "other"), ]); }); + roots.sweepDocuments.mockImplementation(async (rootId: string) => { + const path = rootId.slice("id-".length); + return [`${path}/README.md`, `${path}/guides/writing.md`]; + }); roots.rootClose.mockResolvedValue(undefined); roots.revealInFinder.mockResolvedValue(undefined); watch.watchStart.mockResolvedValue(undefined); diff --git a/src/styles/proofing.css b/src/styles/proofing.css index fbed981..7e5abbe 100644 --- a/src/styles/proofing.css +++ b/src/styles/proofing.css @@ -57,11 +57,24 @@ } .proof-suggestion:hover, -.proof-action:hover { +.proof-action:hover, +.proof-suggestion:focus-visible, +.proof-action:focus-visible { background: var(--accent-wash); color: var(--ink); } +/* The keyboard's item is drawn as the pointer's is, plus the app's own focus ring from app.css, + because a menu that marked the focused row differently from the hovered one would be claiming + there are two selections in it. Only the offset is changed: at the shared 2px the ring sits + outside the five pixels of padding the popover has and is clipped by its own border, so it is + turned inward to the same distance instead. The colour stays --accent, which is the token, and + is what makes this legible on both papers. */ +.proof-suggestion:focus-visible, +.proof-action:focus-visible { + outline-offset: -2px; +} + .proof-none { margin: 0; padding: 7px 10px; diff --git a/src/workspace.ts b/src/workspace.ts index 3cdc1df..4e148a2 100644 --- a/src/workspace.ts +++ b/src/workspace.ts @@ -21,7 +21,12 @@ import { } from "./api/files"; import { revealInFinder, rootClose, rootOpen, rootsList, treeRead } from "./api/roots"; import { watchStart, watchStop } from "./api/watch"; -import { abandonDocument, documentChangedOnDisk, flushPendingSave } from "./document"; +import { + abandonDocument, + documentChangedOnDisk, + documentMovedTo, + flushPendingSave, +} from "./document"; import { rewriteLinksForMove } from "./linkRewrite"; import { INDEX_PROGRESS_EVENT, @@ -243,11 +248,31 @@ export async function renamePath(path: string, name: string): Promise { if (useWorkspace.getState().selectedPath === path) { useWorkspace.getState().select(node.path); } - // Still dirty means the flush conflicted, so the buffer is the only copy of that edit and - // reopening at the new name would throw it away. It stays where it is and stays flagged. - if (affected && open !== null && !useDocument.getState().dirty) { - await useDocument.getState().open(node.path + open.slice(path.length)); - } + await followTheFile(open, affected, path, node.path); +} + +/** + * Points the open document at where its file went, either way round. + * + * A clean buffer is reopened, which is what puts the rewritten links in front of the user. A dirty + * one is not, because the flush at the top of the caller conflicted or the user typed while the + * sweep was running, and either way the buffer is the only copy of that edit and a reopen is a + * read that would throw it away. What it is not allowed to be is left where it was: a sweep can + * read and write five thousand documents, which is a long time to be holding a buffer over a path + * this app has already moved the file off, and the save on the other end of that would have put a + * second copy of the document back at the old path. `documentMovedTo` is the whole of the fix and + * it says why there. + */ +async function followTheFile( + open: string | null, + affected: boolean, + from: string, + to: string, +): Promise { + if (!affected || open === null) return; + const moved = to + open.slice(from.length); + if (useDocument.getState().dirty) documentMovedTo(moved); + else await useDocument.getState().open(moved); } /** @@ -269,9 +294,7 @@ export async function movePath(path: string, destDir: string): Promise { if (useWorkspace.getState().selectedPath === path) { useWorkspace.getState().select(node.path); } - if (affected && open !== null && !useDocument.getState().dirty) { - await useDocument.getState().open(node.path + open.slice(path.length)); - } + await followTheFile(open, affected, path, node.path); return node.path; } diff --git a/tests/identity.ts b/tests/identity.ts new file mode 100644 index 0000000..d12a2b3 --- /dev/null +++ b/tests/identity.ts @@ -0,0 +1,106 @@ +// Proof that the dev server on the port is serving this checkout, run once before any spec. +// +// playwright.config.ts sets `reuseExistingServer`, which is what makes the suite quick to run +// against a server that is already up, and it is also what lets the suite talk to a server another +// checkout left behind. A stale server serves that checkout, so every assertion afterwards is about +// code this working tree does not contain: a pass proves nothing and a failure sends you looking +// for a bug in a file you never changed. `bytes.spec.ts` has carried a check of its own for that +// since it caught one, but it was the only suite that had it, and the four beside it were happy to +// pass against anybody's copy. +// +// Vite serves any module with `?raw` as its own source, so the check is a byte comparison rather +// than a guess: read the file here, ask the server for the same file, and require the two to be +// the same string. The files are the load bearing ones, the schema the editor is built on, the save +// path, the writer, the fixture the specs read and the seam they mock, so a server old enough to +// matter differs in at least one of them. + +import { readFileSync } from "node:fs"; +import { fileURLToPath } from "node:url"; +import { join } from "node:path"; +import type { FullConfig } from "@playwright/test"; + +/** The repository root, resolved off this file rather than off the working directory. */ +const ROOT = fileURLToPath(new URL("..", import.meta.url)); + +const WITNESSES = [ + "src/model/schema.ts", + "src/document.ts", + "src/markdown/serialize.ts", + "src/dev/fixture.ts", + "src/ipc.ts", +]; + +/** + * Reads back the string in `export default '...'`, which is the whole body of a `?raw` module. + * Written out rather than evaluated, because the point of this file is that the server is not + * trusted yet, and running what it sends to find out whether to trust it has the order wrong. + */ +function decode(module: string): string { + const body = module + // Vite appends an inline source map to the module it serves, which is a comment about the + // module rather than part of it. + .replace(/\n\/\/# sourceMappingURL=[\s\S]*$/, "") + .trim() + .replace(/^export default\s*/, "") + .replace(/;$/, ""); + const quote = body[0]; + if ((quote !== "'" && quote !== '"') || body[body.length - 1] !== quote) { + throw new Error(`the server answered ?raw with something that is not a string literal: ${body.slice(0, 80)}`); + } + + let out = ""; + for (let i = 1; i < body.length - 1; i++) { + if (body[i] !== "\\") { + out += body[i]; + continue; + } + const escape = body[++i]; + if (escape === "n") out += "\n"; + else if (escape === "r") out += "\r"; + else if (escape === "t") out += "\t"; + else if (escape === "b") out += "\b"; + else if (escape === "f") out += "\f"; + else if (escape === "v") out += "\v"; + else if (escape === "0") out += "\0"; + else if (escape === "u" && body[i + 1] === "{") { + const end = body.indexOf("}", i); + out += String.fromCodePoint(parseInt(body.slice(i + 2, end), 16)); + i = end; + } else if (escape === "u") { + out += String.fromCharCode(parseInt(body.slice(i + 1, i + 5), 16)); + i += 4; + } else if (escape === "x") { + out += String.fromCharCode(parseInt(body.slice(i + 1, i + 3), 16)); + i += 2; + } else if (escape === "\n") { + // A line continuation stands for nothing at all. + } else out += escape; + } + return out; +} + +export default async function identity(config: FullConfig): Promise { + const baseURL = config.projects[0]?.use?.baseURL; + if (!baseURL) throw new Error("no baseURL to check: playwright.config.ts stopped setting one."); + + for (const witness of WITNESSES) { + const url = `${baseURL}/${witness}?raw`; + const response = await fetch(url, { cache: "no-store" }); + if (!response.ok) { + throw new Error( + `${url} answered ${response.status}. Whatever is on this port is not the Vite dev server ` + + `for this checkout. Stop it and let \`pnpm test:ui\` start its own.`, + ); + } + + const served = decode(await response.text()); + const here = readFileSync(join(ROOT, witness), "utf8"); + if (served !== here) { + throw new Error( + `The dev server on this port is serving a different ${witness} from the one in this ` + + `working tree, so it is serving somebody else's checkout and this suite would be ` + + `testing their code. Stop that server and run \`pnpm test:ui\` again.`, + ); + } + } +} diff --git a/vite.config.ts b/vite.config.ts index 273efef..7898a1f 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -37,5 +37,35 @@ export default defineConfig({ test: { include: ["src/**/*.test.ts"], environment: "node", + + // The markdown suites are thousands of generated documents pushed through the writer inside one + // synchronous loop, and the worst of them holds a worker's event loop for the better part of + // half a minute even on a fast machine. Everything below exists because of that. + // + // A worker tells the main process about each test it starts, over an RPC call it then waits on, + // and that call is given sixty seconds before it gives up with `Timeout calling "onTaskUpdate"`. + // A test that blocks the loop cannot take delivery of the answer while it is running, so what + // the sixty seconds really bounds is the length of the slowest single test, not the round trip. + // Vitest does not expose that timeout, so the only lever left is keeping the tests from getting + // slower, and what makes them slower is contention. The default is one worker per core bar one, + // which on a two or three core hosted runner has the heaviest suites fighting each other and the + // main process for the same cores. Half the cores gives each worker a core to itself and still + // leaves one for the main process to answer on. This is a percentage rather than a number + // behind a CI check because it lands on the right answer for a two core runner, a three core + // one and a developer's laptop without anyone having to know which one the job got, and it + // costs nothing locally: the run is bounded by its slowest single file either way. + maxWorkers: "50%", + + // Five seconds is the default and nothing in the markdown suites honours it. The slow tests + // pass their own timeout to `it` as a third argument. The merely slow-ish ones do not, and they + // are the ones that go red on a loaded runner having done nothing wrong. Thirty seconds is what + // most of the annotated tests already ask for, and it stays under the sixty the RPC allows, so + // this cannot itself cause the failure described above. + testTimeout: 30_000, + hookTimeout: 30_000, + + // How long a worker is given to shut down before it is killed. Ten seconds is plenty when the + // machine is idle and is not when several forks are all winding up at once. + teardownTimeout: 30_000, }, });