Pre-push review: T-1822 Share-with-Notes filename sanitisation

diff --git a/CHANGELOG.md b/CHANGELOG.mdindex 9938073..f6e1186 100644--- a/CHANGELOG.md+++ b/CHANGELOG.md@@ -19,6 +19,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0  ### Fixed +- Share with Notes on a remote document opened from a host-only or trailing-slash root URL (e.g. `https://example.com`) no longer always fails (T-1822). The display name used for that document falls back to its full absolute URL so a root URL never shows a blank title (T-1177) — but Share with Notes reused that same string, unsanitised, as the temporary export file's name. The URL's `/` and `:` characters were read as path separators, so the write always targeted a nonexistent nested directory and "Export Failed" appeared every time. The export filename is now derived from a sanitised version of the display name, with path separators and control characters stripped rather than passed through.+ - Arrow keys, Page Up/Down, Space, and the View menu's **Page Down**/**Page Up**/**Scroll to Top**/**Scroll to Bottom** no longer scroll the document behind an open note, footnote, add-note, reply, or document-note modal (T-1099, reopened). This was fixed once before; the WebKit rendering cutover restructured the views that carried the fix and the binding was never rebuilt, so scroll commands kept reaching the document underneath a modal that should have blocked them. The gate is restored on both the iPhone and iPad/Mac layouts, taking effect immediately when a modal is already open and staying live across every presentation and dismissal. - Replying to a document-level note is no longer silently discarded on a document that has only imported notes (T-1865). NotesPanel and SidebarNotesView create replies through a convenience method that used the document's saved user notes as its source of context; on a document where no user note had ever been created — only imported ones — that context was `nil`, so the guard returned early before the reply was ever built, leaving the tap with no visible effect and nothing written to disk. The method now falls back to the document's cached identifier, the same fallback its sibling document-note-creation method already used, so a reply always creates the note container it needs. - The safeguard that stops a broken document from reloading forever now holds when the crashes keep landing mid-load (T-2107). When a document's rendering process stops, the app reloads it, and if the reloads repeatedly fail to bring the document back it gives up after a few attempts and shows a banner offering a manual reload rather than retrying endlessly (T-1943 below). But a reload was counted as having succeeded the moment the page reported in — before it had finished laying out — so a renderer that reliably crashed in that window looked like a fresh failure each time instead of the same one continuing: the count started over on every attempt, and the document reloaded forever, which is exactly the loop the safeguard exists to prevent. A recovery now only counts as successful once the reloaded document has actually settled on screen, so crashes landing in that window accumulate toward the limit and reach the banner. Recoveries that do bring the document back still reset the count, and the banner's reload still restores everything as before.

diff --git a/prism/Services/InlineNotesShareHelper.swift b/prism/Services/InlineNotesShareHelper.swiftindex 2f82203..559d49c 100644--- a/prism/Services/InlineNotesShareHelper.swift+++ b/prism/Services/InlineNotesShareHelper.swift@@ -62,10 +62,7 @@ enum InlineNotesShareHelper {             showHTMLComments: settings.showHTMLComments         )         let displayName = notesManager.documentNotes?.displayName ?? "document.md"-        let baseName = displayName.hasSuffix(".md")-            ? String(displayName.dropLast(3)) : displayName-        let filename = "\(baseName)-\(UUID().uuidString.prefix(8)).md"-        let url = FileManager.default.temporaryDirectory.appendingPathComponent(filename)+        let url = Self.temporaryExportURL(for: displayName)         do {             try content.write(to: url, atomically: true, encoding: .utf8)         } catch {@@ -105,6 +102,86 @@ enum InlineNotesShareHelper {         return true         #endif     }++    // MARK: - Filename Sanitisation++    /// Base filename (no extension, no unique suffix) derived from a+    /// document display name, safe to use as a single filesystem path+    /// component.+    ///+    /// `displayName` comes from `NotesManager.documentNotes?.displayName`,+    /// which for a remote document is `DocumentSource.urlDisplayTitle` —+    /// falling back to the full absolute URL string (containing `/` and `:`)+    /// for host-only or slash-terminated URLs so those never produce a blank+    /// display name (T-1177). Interpolating that fallback directly into+    /// `appendingPathComponent` let `/` act as a path separator, so the+    /// export write targeted nonexistent nested directories and always+    /// failed (T-1822). Strips path separators and control characters —+    /// collapsing runs of them into a single `-` — drops any leading run of+    /// `.` or `-` (a leading dot would make the exported temp file hidden),+    /// caps the result at+    /// `maxBaseNameBytes` UTF-8 bytes (APFS limits a path component to+    /// 255 bytes; the same absolute-URL fallback can exceed that once the+    /// `-{uuid}.md` suffix is appended), and falls back to a generic name+    /// if nothing safe remains.+    static func sanitizedBaseName(from displayName: String) -> String {+        var name = displayName+        let lowered = name.lowercased()+        for suffix in [".markdown", ".md"] where lowered.hasSuffix(suffix) {+            name.removeLast(suffix.count)+            break+        }++        var result = ""+        var lastWasReplaced = false+        let controlCharacters = CharacterSet.controlCharacters+        for scalar in name.unicodeScalars {+            let isUnsafe = scalar == "/" || scalar == "\\" || scalar == ":"+                || controlCharacters.contains(scalar)+            if isUnsafe {+                if !lastWasReplaced {+                    result.append("-")+                    lastWasReplaced = true+                }+            } else {+                result.unicodeScalars.append(scalar)+                lastWasReplaced = false+            }+        }++        // Drop the whole leading run of dots AND dashes in one pass: trimming+        // them separately lets an alternation like ".-.foo" sneak a dot back+        // to the front. Truncation only removes from the end, so it can never+        // reintroduce a leading dot — only a trailing "-", trimmed last.+        var trimmed = String(result.drop { $0 == "." || $0 == "-" })+        trimmed = truncated(trimmed, toUTF8ByteLimit: maxBaseNameBytes)+        while trimmed.hasSuffix("-") { trimmed.removeLast() }+        return trimmed.isEmpty ? "document" : trimmed+    }++    /// Maximum UTF-8 byte length for the sanitised base name. APFS caps a+    /// path component at 255 bytes (`NAME_MAX`); 180 leaves ample headroom+    /// for the `-{8-char uuid}.md` suffix appended by `temporaryExportURL`.+    static let maxBaseNameBytes = 180++    /// Truncates `name` to at most `limit` UTF-8 bytes without splitting a+    /// grapheme cluster (a `Character` is never cut mid-way).+    private static func truncated(_ name: String, toUTF8ByteLimit limit: Int) -> String {+        var result = name+        while result.utf8.count > limit {+            result.removeLast()+        }+        return result+    }++    /// A unique, single-path-component temporary file URL for exporting+    /// `displayName`'s notes, guaranteed to sit directly under+    /// `FileManager.default.temporaryDirectory` (T-1822).+    static func temporaryExportURL(for displayName: String) -> URL {+        let baseName = sanitizedBaseName(from: displayName)+        let filename = "\(baseName)-\(UUID().uuidString.prefix(8)).md"+        return FileManager.default.temporaryDirectory.appendingPathComponent(filename)+    } }  // MARK: - Export Button View

diff --git a/prismTests/InlineNotesShareHelperTests.swift b/prismTests/InlineNotesShareHelperTests.swiftnew file mode 100644index 0000000..631822c--- /dev/null+++ b/prismTests/InlineNotesShareHelperTests.swift@@ -0,0 +1,150 @@+//+//  InlineNotesShareHelperTests.swift+//  prismTests+//+//  T-1822 regression: `InlineNotesShareHelper.share` built its temp filename+//  directly from `NotesManager.documentNotes?.displayName`. For a remote+//  document at a host-only or slash-terminated URL, `DocumentSource+//  .urlDisplayTitle` falls back to the full absolute URL string (T-1177),+//  which contains "/" and ":". Interpolating that into+//  `appendingPathComponent` makes those characters act as path separators,+//  so the write targets nonexistent nested directories and always fails.+//+//  These tests exercise the pure, non-MainActor sanitisation helpers+//  directly (no UI presentation involved).+//++import Foundation+import Testing+@testable import prism++@Suite("InlineNotesShareHelper filename sanitisation")+struct InlineNotesShareHelperTests {++    // MARK: - sanitizedBaseName++    @Test("host-only URL display name has separators stripped")+    func hostOnlyURLIsSanitized() {+        let base = InlineNotesShareHelper.sanitizedBaseName(from: "https://example.com")+        #expect(!base.contains("/"))+        #expect(!base.contains(":"))+        #expect(base == "https-example.com")+    }++    @Test("slash-terminated URL display name has separators stripped")+    func slashTerminatedURLIsSanitized() {+        let base = InlineNotesShareHelper.sanitizedBaseName(from: "https://example.com/")+        #expect(!base.contains("/"))+        #expect(base == "https-example.com")+    }++    @Test(".md and .markdown suffixes are stripped case-insensitively")+    func extensionSuffixStripped() {+        #expect(InlineNotesShareHelper.sanitizedBaseName(from: "notes.md") == "notes")+        #expect(InlineNotesShareHelper.sanitizedBaseName(from: "notes.MD") == "notes")+        #expect(InlineNotesShareHelper.sanitizedBaseName(from: "notes.Markdown") == "notes")+    }++    @Test("URL-encoded separators are left untouched")+    func encodedSeparatorsUntouched() {+        let base = InlineNotesShareHelper.sanitizedBaseName(from: "path%2Fsegment.md")+        #expect(base == "path%2Fsegment")+    }++    @Test("control characters are stripped")+    func controlCharactersStripped() {+        let base = InlineNotesShareHelper.sanitizedBaseName(from: "line\nbreak\ttab.md")+        #expect(!base.contains("\n"))+        #expect(!base.contains("\t"))+    }++    @Test("backslashes and colons are stripped")+    func backslashesAndColonsStripped() {+        let base = InlineNotesShareHelper.sanitizedBaseName(from: "C:\\Users\\name.md")+        #expect(!base.contains("\\"))+        #expect(!base.contains(":"))+    }++    @Test("a display name with nothing safe remaining falls back to a generic name")+    func fallsBackWhenNothingSafeRemains() {+        #expect(InlineNotesShareHelper.sanitizedBaseName(from: "://") == "document")+    }++    @Test("leading dot is stripped so the temp file is not hidden")+    func leadingDotStripped() {+        #expect(InlineNotesShareHelper.sanitizedBaseName(from: ".gitignore") == "gitignore")+        // Suffix strip runs first: ".gitignore.md" -> ".gitignore" -> "gitignore".+        #expect(InlineNotesShareHelper.sanitizedBaseName(from: ".gitignore.md") == "gitignore")+        // Separators sanitise to a leading "-", trim, then dot-strip.+        #expect(InlineNotesShareHelper.sanitizedBaseName(from: "/.hidden") == "hidden")+        // Alternating dots and separators must not sneak a dot back to the+        // front: ".:.foo" sanitises to ".-.foo", and dropping dots and+        // dashes as one leading run yields "foo" (not the hidden ".foo").+        #expect(InlineNotesShareHelper.sanitizedBaseName(from: ".:.foo") == "foo")+    }++    @Test("a display name of only dots falls back to a generic name")+    func allDotsFallsBack() {+        #expect(InlineNotesShareHelper.sanitizedBaseName(from: "...") == "document")+    }++    @Test("long display names are capped in UTF-8 bytes")+    func longNameIsByteCapped() {+        let longQuery = "https://example.com/?" + String(repeating: "a", count: 400)+        let base = InlineNotesShareHelper.sanitizedBaseName(from: longQuery)+        #expect(base.utf8.count <= InlineNotesShareHelper.maxBaseNameBytes)+        // The capped base plus the "-{8-char uuid}.md" suffix stays under+        // APFS's 255-byte NAME_MAX.+        let url = InlineNotesShareHelper.temporaryExportURL(for: longQuery)+        #expect(url.lastPathComponent.utf8.count <= 255)+    }++    @Test("byte-cap truncation never splits a grapheme cluster")+    func truncationPreservesGraphemes() {+        // 8 bytes per flag (two regional indicators, no format scalars the+        // sanitiser would strip); 60 of them is 480 bytes, well past the+        // cap, and 180 is not a multiple of 8, so a naive byte cut would+        // land mid-cluster.+        let flag = "🇦🇺"+        let name = String(repeating: flag, count: 60)+        let base = InlineNotesShareHelper.sanitizedBaseName(from: name)+        #expect(base.utf8.count <= InlineNotesShareHelper.maxBaseNameBytes)+        #expect(base.allSatisfy { $0 == Character(flag) })+        #expect(name.hasPrefix(base))+        // Truncation must be maximal, not merely safe: every whole cluster+        // that fits under the cap is kept (180 / 8 = 22.5 -> 22 flags).+        #expect(base.count == 22)+    }++    @Test("short names are not truncated")+    func shortNameUnchangedByCap() {+        #expect(InlineNotesShareHelper.sanitizedBaseName(from: "notes.md") == "notes")+    }++    @Test("writing to the temporary export URL for a very long display name succeeds")+    func writingLongNameExportSucceeds() throws {+        let longName = "https://example.com/?" + String(repeating: "x", count: 500)+        let url = InlineNotesShareHelper.temporaryExportURL(for: longName)+        defer { try? FileManager.default.removeItem(at: url) }+        try "content".write(to: url, atomically: true, encoding: .utf8)+        #expect(FileManager.default.fileExists(atPath: url.path))+    }++    // MARK: - temporaryExportURL++    @Test("temporary export URL for a root URL display name is a direct child of the temp directory")+    func temporaryExportURLHasNoNestedPathComponents() {+        let tempDir = FileManager.default.temporaryDirectory+        let url = InlineNotesShareHelper.temporaryExportURL(for: "https://example.com")+        #expect(url.deletingLastPathComponent().path == tempDir.path)+        #expect(url.pathExtension == "md")+    }++    @Test("writing to the temporary export URL for a root URL display name succeeds")+    func writingToTemporaryExportURLSucceeds() throws {+        let url = InlineNotesShareHelper.temporaryExportURL(for: "https://example.com")+        defer { try? FileManager.default.removeItem(at: url) }+        try "content".write(to: url, atomically: true, encoding: .utf8)+        #expect(FileManager.default.fileExists(atPath: url.path))+    }+}