Improve file browser performance and SSH workflows - #43
matchaboar wants to merge 2 commits into
Conversation
nonatofabio
left a comment
There was a problem hiding this comment.
First off — thank you for this. I did a deep read of all 41 files and a lot of it is genuinely good engineering: the process-runner rewrite fixes a real deadlock class (draining pipes concurrently instead of after termination), the hostname validation in SSHTarget is a security improvement with proper injection tests, the generation-counted cancellation in FileBrowserState is exactly how off-main tree building should look, and the test coverage is substantial and honest. I want most of this in Mindle.
That said, I can't take it as-is. Blocking items first:
1. encodeImagePath is out of scope — breaks rendering for any doc with a local image. In Resources/web/reader.js, encodeImagePath is declared inside decodeImagePath (after its returns), but called from resolveImageSrc. Any file://, absolute, or relative image src throws ReferenceError, and since rewriteImages() runs inside applyAll() with no try/catch, the render pipeline dies mid-flight — annotations, search marks, and mermaid never get installed for that document. Move the function up a scope and please give the branch a manual pass with an image-bearing markdown file.
2. Remote asset fetching needs tighter scoping. Auto-fetching referenced images is a nice touch, but as written every  in a remote doc gets scp'd with ../ traversal allowed and no extension filter — so a hostile remote document can make Mindle copy anything the SSH user can read (e.g. ../../../.ssh/id_rsa) into the local cache. Nothing leaves the machine, but it stretches Mindle's "SSH only when you ask" promise further than I'm comfortable with. I'd take this with: an image-extension allowlist, resolved paths confined to the profile root, and a cap on fetch count per document.
3. CI: keep test execution out of the signing job. The new run-tests.sh && run-screenshot-tests.sh step runs inside the job that carries SIGN_IDENTITY and contents: write. Fork-PR protections cover the realistic path, but I'd rather the test step live in a separate job with permissions: contents: read (or checkout with persist-credentials: false).
Smaller things I'd like resolved or discussed:
- LazyVStack in FileBrowserView — the non-lazy VStack on main was a deliberate fix for #36 (scroll nudge when row state flips). Your flattened equatable rows may well avoid the original problem, but I need to re-verify #36 manually before this ships. If you've tested that scenario, say so in the thread.
- Missing remote root falls back to
find "$HOME"— a silent whole-home scan is surprising; I'd rather fail loudly with a clear error. - Default profile ships with
hostname: test— anyone with a realHost testalias in their SSH config gets a connection they didn't configure on first click. Ship the default commented out. decodeImagePathtruncating at?/#changes behavior for local filenames containing those characters (goes with item 1).- The README edit drops the file browser's "Never escapes upward" line — the code still doesn't escape upward, so let's keep the promise in the docs.
- The AGENTS.md edit is benign but unrelated to this PR — happy to take it, just flagging the churn.
If you'd rather split it: the file-browser performance core (FileTree, FileBrowserState, FileBrowserView, GitFileMetadata, PerformanceTrace + their tests) is clean and could land on its own quickly, with the SSH workflow changes as a follow-up PR once the scoping questions above are settled. Either way works for me.
|
Rebased the work onto current
All three branches were created from and rebased onto |
Performance
DocumentStore.refreshFileTree()recursively calledcontentsOfDirectoryand read resource values synchronously on@MainActor, freezing window interaction for the full scan.VStackeagerly created every expanded directory and file row, even when thousands were outside the viewport.LazyVStack.git diffandgit logindependently.os_signpostpoints for tree build, flatten, publication, metadata, and local file reads.Features
.md,.markdown,.mdown,.mkd,.txt, and.pdffiles.~/Library/Application Support/Mindle/ssh-profiles.yaml, including a favorite profile opened from the toolbar.$HOMEwhen the configured SSH directory does not exist. Existing empty directories remain empty, and unreadable/failed searches remain errors.UI
AppleActionOnDoubleClickpreference for zoom, minimize, or no action.Bug fixes
PDFDocumentafter refresh instead of keeping stale bytes behind an unchanged cache URL..fetchpath.#and?characters in local and remote image filenames.no-storeso refreshed remote assets are not hidden by WebKit caching.$HOME.Validation
./run-tests.sh— all checks pass, with 89.73% affected pure-logic line coverage../run-screenshot-tests.sh— 4 screenshot baselines pass, with 89.77%FileBrowserViewline coverage../build.sh— buildsMindle.appsuccessfully.