From e19daa582a20d49d8ac11040483f6c7b0bdb620d Mon Sep 17 00:00:00 2001 From: Mark Pitman Date: Tue, 13 Oct 2026 21:29:31 -0700 Subject: [PATCH] Improve error handling and the output file lifecycle MDVIEW_DIR and the Snap temp directory are created with MkdirAll, so nested paths work. check() now writes to stderr, names the failing path, and shows the Snap hint only for permission and not-found errors, not for every failure. The output file is written by writeOutput, which closes the file explicitly and returns Sync and Close errors. Before, log.Fatal skipped the deferred Close and those errors were dropped. Render errors now go through check(). Generated temp files older than 24 hours are removed from the temp directory before a new one is written. Only names that match mdview<32 hex>.html are touched. Mermaid detection now looks for a mermaid block in the AST, so raw HTML that mentions class="mermaid" no longer adds the 2.6 MB script. Closes #68 --- CLAUDE.md | 6 +-- main.go | 144 +++++++++++++++++++++++++++++++++++---------------- main_test.go | 114 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 215 insertions(+), 49 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 6a15a87..3242962 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -39,13 +39,13 @@ Everything happens in `main()` in `main.go`, in a straight-line pipeline: - `go.abhg.dev/goldmark/mermaid` with `NoScript: true` (diagram rendering is handled ourselves) - `html.WithUnsafe()` (raw HTML passthrough is required for the image-tag handling above) 4. **Title extraction** (`getTitleFromAST`/`extractText`) — walks the parsed AST for the first H1 to use as the ``. -5. **Mermaid script embedding** (`embedMermaidScript`) — only injects the embedded `mermaid.min.js` + an init script (with light/dark theme detection via `prefers-color-scheme`) if the rendered HTML actually contains a mermaid block, keeping non-diagram output lean. -6. **Templating & output** — `template.html` and `github-markdown.css` are embedded via `//go:embed` and combined with the rendered content via `fmt.Fprintf`. `-bare` skips the CSS. Output goes to `-o <path>` if given, otherwise a random temp filename (see below). +5. **Mermaid script embedding** (`embedMermaidScript`) — only injects the embedded `mermaid.min.js` + an init script (with light/dark theme detection via `prefers-color-scheme`) if the parsed AST contains a mermaid block (`hasMermaidBlock`; raw HTML that mentions `class="mermaid"` does not count), keeping non-diagram output lean. +6. **Templating & output** — `template.html` and `github-markdown.css` are embedded via `//go:embed` and combined with the rendered content via `fmt.Fprintf`. `-bare` skips the CSS. `writeOutput` writes the file and reports write and close errors. Output goes to `-o <path>` if given, otherwise a random temp filename (see below). 7. **Launch** — opens the resulting HTML file via `github.com/pkg/browser`. ### Temp file / Snap sandboxing (`getTempDir`, `isSnap`) -Output location resolution order: `MDVIEW_DIR` env var → (if running as a Snap, per `SNAP_USER_COMMON` env var) `~/mdview-temp` → OS default temp dir. This exists because Snap-sandboxed browsers (e.g. Snap Firefox) can't read from `/tmp`, only from the user's home directory — `check()` also prints a Snap-specific hint on file errors for this reason. +Output location resolution order: `MDVIEW_DIR` env var → (if running as a Snap, per `SNAP_USER_COMMON` env var) `~/mdview-temp` → OS default temp dir. `MDVIEW_DIR` and `~/mdview-temp` are created with `MkdirAll`. Before writing a temp file, `removeStaleTempFiles` deletes generated `mdview<32 hex>.html` files older than 24 hours from that directory (the browser reads the new file after mdview exits, so mdview cannot delete it right away). This exists because Snap-sandboxed browsers (e.g. Snap Firefox) can't read from `/tmp`, only from the user's home directory — `check()` writes errors to stderr, names the failing path, and prints a Snap-specific hint for permission and not-found errors for this reason. ### Packaging diff --git a/main.go b/main.go index 3b48418..4d359ab 100644 --- a/main.go +++ b/main.go @@ -18,6 +18,7 @@ import ( "path/filepath" "regexp" "strings" + "time" "github.com/pkg/browser" "github.com/yuin/goldmark" @@ -123,32 +124,27 @@ func main() { // Render to HTML var buf bytes.Buffer - if err := md.Renderer().Render(&buf, dat, doc); err != nil { - log.Fatal(err) - } + check(md.Renderer().Render(&buf, dat, doc)) htmlContent := buf.String() - + // Add embedded Mermaid.js and initialization script when diagrams are present - htmlContent = embedMermaidScript(htmlContent) + if hasMermaidBlock(doc) { + htmlContent = embedMermaidScript(htmlContent) + } outfilePath := *outfilePtr if outfilePath == "" { - outfilePath = tempFileName("mdview", ".html") + tempDir := getTempDir() + removeStaleTempFiles(tempDir, staleTempFileAge) + outfilePath = tempFileName(tempDir, "mdview", ".html") } - f, err := os.Create(outfilePath) - check(err) - defer f.Close() - var actualStyle string + actualStyle := style if *barePtr { actualStyle = "" - } else { - actualStyle = style } - _, err = fmt.Fprintf(f, template, actualStyle, stdhtml.EscapeString(title), htmlContent) - check(err) - f.Sync() + check(writeOutput(outfilePath, actualStyle, stdhtml.EscapeString(title), htmlContent)) browser.Stderr = nil browser.Stdout = nil err = browser.OpenFile(outfilePath) @@ -189,54 +185,102 @@ func stdinIsTerminal() bool { return err == nil && info.Mode()&os.ModeCharDevice != 0 } -func tempFileName(prefix, suffix string) string { +// writeOutput writes the HTML document to path and reports any write or close error +func writeOutput(path string, style string, title string, content string) error { + f, err := os.Create(path) + if err != nil { + return err + } + if _, err := fmt.Fprintf(f, template, style, title, content); err != nil { + f.Close() + return err + } + if err := f.Sync(); err != nil { + f.Close() + return err + } + return f.Close() +} + +func tempFileName(dir, prefix, suffix string) string { randBytes := make([]byte, 16) rand.Read(randBytes) - return filepath.Join(getTempDir(), prefix+hex.EncodeToString(randBytes)+suffix) + return filepath.Join(dir, prefix+hex.EncodeToString(randBytes)+suffix) +} + +// staleTempFileAge is how old a generated temp file must be before mdview removes it +const staleTempFileAge = 24 * time.Hour + +// tempFileRegex matches the names produced by tempFileName, so cleanup never touches other files +var tempFileRegex = regexp.MustCompile(`^mdview[0-9a-f]{32}\.html$`) + +// removeStaleTempFiles deletes generated temp files in dir that are older than maxAge. +// The browser reads the current file after mdview exits, so only older files are removed. +func removeStaleTempFiles(dir string, maxAge time.Duration) { + entries, err := os.ReadDir(dir) + if err != nil { + return + } + for _, entry := range entries { + if !entry.Type().IsRegular() || !tempFileRegex.MatchString(entry.Name()) { + continue + } + if info, err := entry.Info(); err == nil && time.Since(info.ModTime()) > maxAge { + os.Remove(filepath.Join(dir, entry.Name())) + } + } } func getTempDir() string { if os.Getenv("MDVIEW_DIR") != "" { var tempDir = os.Getenv(("MDVIEW_DIR")) - if _, err := os.Stat(tempDir); os.IsNotExist(err) { - err = os.Mkdir(tempDir, 0700) - check(err) - } - + check(os.MkdirAll(tempDir, 0700)) return tempDir } if isSnap() { var tmpdir = os.Getenv("HOME") + "/mdview-temp" - if _, err := os.Stat(tmpdir); os.IsNotExist(err) { - err = os.Mkdir(tmpdir, 0700) - check(err) - } + check(os.MkdirAll(tmpdir, 0700)) return tmpdir } return os.TempDir() } +// check prints a failure to standard error and exits with status 1 func check(e error) { - if e != nil { - if errors.Is(e, fs.ErrPermission) { - fmt.Println("There was a permission error accessing the file.") - } + if e == nil { + return + } - if errors.Is(e, fs.ErrNotExist) { - fmt.Println("mdview was unable to find the file.") - } + permission := errors.Is(e, fs.ErrPermission) + notExist := errors.Is(e, fs.ErrNotExist) - if isSnap() { - fmt.Println("Since mdview was installed as a Snap, it can only access files in your HOME directory.") - fmt.Println("If you need to use it with files outside of your HOME directory, choose a different installation method.") - fmt.Println("https://github.com/mapitman/mdview?tab=readme-ov-file#installation") - fmt.Println() - } + // Name the path, because the failing file can be the input, the output or the output directory + target := "the file" + var pathErr *fs.PathError + if errors.As(e, &pathErr) { + target = pathErr.Path + } - log.Fatal(e) + if permission { + fmt.Fprintf(os.Stderr, "There was a permission error accessing %s.\n", target) } + + if notExist { + fmt.Fprintf(os.Stderr, "mdview was unable to find %s.\n", target) + } + + // The Snap sandbox only blocks file access, so show its hint for those errors only + if isSnap() && (permission || notExist) { + fmt.Fprintln(os.Stderr, "Since mdview was installed as a Snap, it can only access files in your HOME directory.") + fmt.Fprintln(os.Stderr, "If you need to use it with files outside of your HOME directory, choose a different installation method.") + fmt.Fprintln(os.Stderr, "https://github.com/mapitman/mdview?tab=readme-ov-file#installation") + fmt.Fprintln(os.Stderr) + } + + fmt.Fprintln(os.Stderr, "mdview:", e) + os.Exit(1) } func getTitleFromAST(doc ast.Node, source []byte) string { @@ -533,15 +577,23 @@ func getMimeType(path string) string { return "" } +// hasMermaidBlock reports whether the document contains a mermaid code block. +// Checking the AST ignores raw HTML that merely mentions class="mermaid". +func hasMermaidBlock(doc ast.Node) bool { + found := false + _ = ast.Walk(doc, func(n ast.Node, entering bool) (ast.WalkStatus, error) { + if entering && n.Kind() == mermaid.Kind { + found = true + return ast.WalkStop, nil + } + return ast.WalkContinue, nil + }) + return found +} + // embedMermaidScript adds the embedded Mermaid.js library to the HTML content // Since we set NoScript: true on the Mermaid extender, we need to manually add the script func embedMermaidScript(htmlContent string) string { - // Check if there are any mermaid diagrams in the content - // The Goldmark mermaid extension uses class="mermaid" for diagram blocks - if !strings.Contains(htmlContent, `class="mermaid"`) { - return htmlContent // No mermaid diagrams, don't add the script - } - // Escape any </script> tags (with closing >) inside the mermaid.js code // to prevent premature script closure. The standard way is to escape the forward slash. escapedMermaidJS := strings.ReplaceAll(mermaidJS, "</script>", "<\\/script>") diff --git a/main_test.go b/main_test.go index 4d4c620..2b5b8c8 100644 --- a/main_test.go +++ b/main_test.go @@ -10,6 +10,7 @@ import ( "reflect" "strings" "testing" + "time" "github.com/yuin/goldmark" "github.com/yuin/goldmark/text" @@ -274,3 +275,116 @@ func TestParseFlags(t *testing.T) { }) } } + +func TestHasMermaidBlock(t *testing.T) { + tests := []struct { + name string + markdown string + want bool + }{ + {"mermaid fence", "```mermaid\ngraph TD; A-->B\n```\n", true}, + {"other fence", "```go\nfmt.Println()\n```\n", false}, + {"no code", "# Title\n\ntext\n", false}, + {"raw html mentioning mermaid", "<pre class=\"mermaid\">graph TD</pre>\n", false}, + {"code span mentioning mermaid", "Use `class=\"mermaid\"` in HTML.\n", false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + source := []byte(tt.markdown) + doc := newMarkdown(".").Parser().Parse(text.NewReader(source)) + if got := hasMermaidBlock(doc); got != tt.want { + t.Fatalf("hasMermaidBlock = %v, want %v", got, tt.want) + } + }) + } +} + +func TestEmbedMermaidScript(t *testing.T) { + out := embedMermaidScript("<p>x</p>") + if !strings.HasPrefix(out, "<p>x</p><script>") || !strings.Contains(out, "mermaid.initialize") { + t.Fatalf("mermaid script was not appended: %.80s", out) + } + if strings.Count(out, "</script>") != 2 { + t.Fatalf("expected exactly two closing script tags, got %d", strings.Count(out, "</script>")) + } +} + +func TestGetTempDir(t *testing.T) { + t.Run("MDVIEW_DIR is created with parents", func(t *testing.T) { + dir := filepath.Join(t.TempDir(), "a", "b", "c") + t.Setenv("MDVIEW_DIR", dir) + if got := getTempDir(); got != dir { + t.Fatalf("getTempDir = %q, want %q", got, dir) + } + if info, err := os.Stat(dir); err != nil || !info.IsDir() { + t.Fatalf("directory was not created: %v", err) + } + }) + + t.Run("snap uses mdview-temp in HOME", func(t *testing.T) { + home := t.TempDir() + t.Setenv("MDVIEW_DIR", "") + t.Setenv("SNAP_USER_COMMON", "/snap/common") + t.Setenv("HOME", home) + want := filepath.Join(home, "mdview-temp") + if got := getTempDir(); got != want { + t.Fatalf("getTempDir = %q, want %q", got, want) + } + }) + + t.Run("default is the OS temp dir", func(t *testing.T) { + t.Setenv("MDVIEW_DIR", "") + t.Setenv("SNAP_USER_COMMON", "") + if got := getTempDir(); got != os.TempDir() { + t.Fatalf("getTempDir = %q, want %q", got, os.TempDir()) + } + }) +} + +func TestWriteOutput(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "out.html") + if err := writeOutput(path, "body{}", "T", "<p>hi</p>"); err != nil { + t.Fatal(err) + } + got, err := os.ReadFile(path) + if err != nil || !strings.Contains(string(got), "<title>T") || !strings.Contains(string(got), "

hi

") { + t.Fatalf("unexpected output (err %v): %s", err, got) + } + if err := writeOutput(filepath.Join(dir, "missing", "out.html"), "", "T", ""); err == nil { + t.Fatal("expected an error for a missing directory") + } +} + +func TestRemoveStaleTempFiles(t *testing.T) { + dir := t.TempDir() + name := func(c string) string { return "mdview" + strings.Repeat(c, 32) + ".html" } + old := time.Now().Add(-48 * time.Hour) + + files := map[string]time.Time{ + name("a"): old, // stale generated file: removed + name("b"): time.Now(), // fresh generated file: kept + "notes.html": old, // other file: kept + "mdview-old.html": old, // wrong name pattern: kept + name("c") + "x": old, // wrong suffix: kept + } + for file, mtime := range files { + path := filepath.Join(dir, file) + if err := os.WriteFile(path, []byte("x"), 0644); err != nil { + t.Fatal(err) + } + if err := os.Chtimes(path, mtime, mtime); err != nil { + t.Fatal(err) + } + } + + removeStaleTempFiles(dir, staleTempFileAge) + + for file := range files { + _, err := os.Stat(filepath.Join(dir, file)) + wantRemoved := file == name("a") + if wantRemoved != os.IsNotExist(err) { + t.Errorf("%s: removed = %v, want %v", file, os.IsNotExist(err), wantRemoved) + } + } +}