From f99e05598e09af8a4a36b8f221338991e2bba312 Mon Sep 17 00:00:00 2001 From: Aaron Martell Date: Thu, 20 Aug 2026 15:34:49 -0500 Subject: [PATCH 1/6] feat: surface HTTP 403 title-fetch failures instead of empty title --- cmd/book/mark.go | 15 +++++-- internal/web/web.go | 22 ++++++---- internal/web/web_test.go | 90 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 114 insertions(+), 13 deletions(-) create mode 100644 internal/web/web_test.go diff --git a/cmd/book/mark.go b/cmd/book/mark.go index 138451c..f4cb714 100644 --- a/cmd/book/mark.go +++ b/cmd/book/mark.go @@ -1,6 +1,7 @@ package cmd import ( + "errors" "fmt" "net/url" "strings" @@ -63,10 +64,16 @@ func addMark(bs *book.BookShelves, URL string, tags string, shelfName string, co } else { fetchedTitle, err := web.LoadWebsite(mark.URL) if err != nil { - return err - } - if fetchedTitle == "" { - mark.Name = "couldn't fetch page title" + if errors.Is(err, web.ErrTitleUnavailable) { + // Non-interactive path can't prompt for a title. + if shelfName != "" && collectionName != "" { + return fmt.Errorf("couldn't fetch title for %s; provide --title", mark.URL) + } + // Interactive path: leave the title empty so the user is + // forced to enter it manually in the edit form. + } else { + return err + } } else { mark.Name = fetchedTitle } diff --git a/internal/web/web.go b/internal/web/web.go index 8700f04..3a807cc 100644 --- a/internal/web/web.go +++ b/internal/web/web.go @@ -4,6 +4,7 @@ package web import ( "context" + "errors" "fmt" "net/http" "os/exec" @@ -15,6 +16,11 @@ import ( "github.com/PuerkitoBio/goquery" ) +// ErrTitleUnavailable is returned when a page title cannot be fetched +// automatically (e.g. HTTP 403 or an empty tag). Callers should +// prompt the user to enter a title manually. +var ErrTitleUnavailable = errors.New("couldn't fetch title") + // OpenURL opens the given URL in the default browser. func OpenURL(url string) error { switch runtime.GOOS { @@ -41,9 +47,13 @@ func WebsiteTitle(url string) (string, error) { if err != nil { return "", err } - return strings.Join(strings.Fields(strings.TrimSpace(doc.Find("title").Text())), " "), nil + title := strings.Join(strings.Fields(strings.TrimSpace(doc.Find("title").Text())), " ") + if title == "" { + return "", ErrTitleUnavailable + } + return title, nil case http.StatusForbidden: - return "", nil + return "", ErrTitleUnavailable case http.StatusNotFound: return "", fmt.Errorf("betta check yerself - that's a 4oh4!\n%s", url) default: @@ -63,13 +73,7 @@ func LoadWebsite(url string) (string, error) { Context(ctx). ActionWithErr(func(context.Context) error { title, err = WebsiteTitle(url) - if err != nil { - return err - } - if title != "" { - return nil - } - return nil + return err }). Title("Loading mark title ..."). Type(spinner.Line). diff --git a/internal/web/web_test.go b/internal/web/web_test.go new file mode 100644 index 0000000..b5ad4c6 --- /dev/null +++ b/internal/web/web_test.go @@ -0,0 +1,90 @@ +package web + +import ( + "errors" + "fmt" + "net/http" + "net/http/httptest" + "testing" +) + +func TestWebsiteTitle(t *testing.T) { + tests := []struct { + name string + status int + body string + wantTitle string + wantErr bool + wantErrTitleUnavailable bool + }{ + { + name: "OK with title", + status: http.StatusOK, + body: `<html><head><title> Hello World `, + wantTitle: "Hello World", + wantErr: false, + }, + { + name: "OK with empty title", + status: http.StatusOK, + body: ` `, + wantTitle: "", + wantErr: true, + wantErrTitleUnavailable: true, + }, + { + name: "OK with missing title tag", + status: http.StatusOK, + body: `hi`, + wantTitle: "", + wantErr: true, + wantErrTitleUnavailable: true, + }, + { + name: "Forbidden", + status: http.StatusForbidden, + body: "", + wantTitle: "", + wantErr: true, + wantErrTitleUnavailable: true, + }, + { + name: "NotFound", + status: http.StatusNotFound, + body: "", + wantTitle: "", + wantErr: true, + }, + { + name: "ServerError", + status: http.StatusInternalServerError, + body: "", + wantTitle: "", + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(tt.status) + fmt.Fprint(w, tt.body) + })) + defer server.Close() + + gotTitle, gotErr := WebsiteTitle(server.URL) + if gotTitle != tt.wantTitle { + t.Errorf("title mismatch: got %q, want %q", gotTitle, tt.wantTitle) + } + if tt.wantErr && gotErr == nil { + t.Errorf("expected error, got nil") + } + if !tt.wantErr && gotErr != nil { + t.Errorf("unexpected error: %v", gotErr) + } + if tt.wantErrTitleUnavailable && !errors.Is(gotErr, ErrTitleUnavailable) { + t.Errorf("expected ErrTitleUnavailable, got %v", gotErr) + } + }) + } +} From ccc9667c215ccc5a9bf678329e339a1f70e893f2 Mon Sep 17 00:00:00 2001 From: Aaron Martell Date: Thu, 20 Aug 2026 16:06:25 -0500 Subject: [PATCH 2/6] feat: add windows url open support --- internal/web/web.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/web/web.go b/internal/web/web.go index 3a807cc..c0f31fe 100644 --- a/internal/web/web.go +++ b/internal/web/web.go @@ -25,7 +25,7 @@ var ErrTitleUnavailable = errors.New("couldn't fetch title") func OpenURL(url string) error { switch runtime.GOOS { case "windows": - return fmt.Errorf("yeah - this ain't gonna work on windows") + return exec.Command("cmd", "/c", "start", url).Start() case "darwin": return exec.Command("open", url).Start() default: From 60d212ed3888f7da0963ad111cc5d430172b4fa8 Mon Sep 17 00:00:00 2001 From: Aaron Martell Date: Thu, 20 Aug 2026 16:10:20 -0500 Subject: [PATCH 3/6] bug: fsync file + parent dir in CreateTOML around rename --- internal/catalog/toml.go | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/internal/catalog/toml.go b/internal/catalog/toml.go index e7d8750..f33b407 100644 --- a/internal/catalog/toml.go +++ b/internal/catalog/toml.go @@ -58,7 +58,23 @@ func CreateTOML(t book.TOMLFile) (err error) { return err } - return os.Rename(tmpPath, writePath) + if err = f.Sync(); err != nil { + return err + } + + if err = os.Rename(tmpPath, writePath); err != nil { + return err + } + + dir, err := os.Open(filepath.Dir(writePath)) + if err != nil { + return err + } + if err = dir.Sync(); err != nil { + _ = dir.Close() + return err + } + return dir.Close() } // UpdateShelfFile persists the given shelf to its on-disk TOML file. From 57daf5d13500ec2ff986e6ed069fc4c99f659c37 Mon Sep 17 00:00:00 2001 From: Aaron Martell Date: Thu, 20 Aug 2026 16:17:52 -0500 Subject: [PATCH 4/6] chore: assert toml map ordering --- internal/catalog/toml_test.go | 44 +++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/internal/catalog/toml_test.go b/internal/catalog/toml_test.go index 69c57e6..3ed663c 100644 --- a/internal/catalog/toml_test.go +++ b/internal/catalog/toml_test.go @@ -4,6 +4,7 @@ import ( "bytes" "os" "path/filepath" + "strings" "testing" "github.com/BurntSushi/toml" @@ -101,6 +102,49 @@ func normalizeTOMLWhitespace(b []byte) []byte { return bytes.Join(out, []byte("\n")) } +// TestCollectionMapOrdering asserts that BurntSushi/toml encodes the +// Collections map in a stable, deterministic order (alphabetical by key). +// This pins down the current behavior so a library upgrade cannot silently +// reshuffle the on-disk layout. +func TestCollectionMapOrdering(t *testing.T) { + shelf := book.Shelf{ + Name: "ordering", + Description: "collection ordering test", + Collections: map[string]*book.Collection{ + "zebra": { + Name: "zebra", + Description: "last alphabetically", + }, + "alpha": { + Name: "alpha", + Description: "first alphabetically", + }, + "mike": { + Name: "mike", + Description: "middle alphabetically", + }, + }, + } + + var buf bytes.Buffer + if err := toml.NewEncoder(&buf).Encode(shelf); err != nil { + t.Fatalf("encode shelf: %v", err) + } + + output := buf.String() + alphaIdx := strings.Index(output, "[Collections.alpha]") + mikeIdx := strings.Index(output, "[Collections.mike]") + zebraIdx := strings.Index(output, "[Collections.zebra]") + + if alphaIdx == -1 || mikeIdx == -1 || zebraIdx == -1 { + t.Fatalf("expected all collection sections in output:\n%s", output) + } + + if alphaIdx >= mikeIdx || mikeIdx >= zebraIdx { + t.Errorf("collections not in alphabetical order; expected alpha < mike < zebra, got alpha=%d mike=%d zebra=%d\n%s", alphaIdx, mikeIdx, zebraIdx, output) + } +} + // TestCreateTOMLAtomic verifies that CreateTOML writes a temp file and // renames it into place, leaving no .tmp debris on success. func TestCreateTOMLAtomic(t *testing.T) { From 382f186ce1ad66e8086f2fd4e8de21fd4462688a Mon Sep 17 00:00:00 2001 From: Aaron Martell Date: Thu, 20 Aug 2026 16:23:01 -0500 Subject: [PATCH 5/6] chore: added unit tests for internal helpers --- internal/book/types_test.go | 290 ++++++++++++++++++++++++++++++++++++ 1 file changed, 290 insertions(+) create mode 100644 internal/book/types_test.go diff --git a/internal/book/types_test.go b/internal/book/types_test.go new file mode 100644 index 0000000..fff7ee7 --- /dev/null +++ b/internal/book/types_test.go @@ -0,0 +1,290 @@ +package book + +import ( + "slices" + "testing" +) + +func TestDedupUnique(t *testing.T) { + tests := []struct { + name string + in [][]string + want []string + }{ + { + name: "preserves first-seen order", + in: [][]string{{"a", "b", "c"}, {"b", "a", "d"}}, + want: []string{"a", "b", "c", "d"}, + }, + { + name: "merges multiple slices", + in: [][]string{{"x"}, {"y"}, {"z"}, {"x"}}, + want: []string{"x", "y", "z"}, + }, + { + name: "empty input", + in: [][]string{}, + want: []string{}, + }, + { + name: "empty slices are ignored", + in: [][]string{{}, {"a"}, {}, {"a", "b"}}, + want: []string{"a", "b"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := DedupUnique(tt.in...) + if !slices.Equal(got, tt.want) { + t.Errorf("DedupUnique() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestStructIsEmpty(t *testing.T) { + tests := []struct { + name string + ptr *Mark + want bool + }{ + { + name: "nil pointer", + ptr: nil, + want: true, + }, + { + name: "zero struct", + ptr: &Mark{}, + want: true, + }, + { + name: "non-zero struct", + ptr: &Mark{Name: "example"}, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := StructIsEmpty(tt.ptr); got != tt.want { + t.Errorf("StructIsEmpty() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestVerifyUniqueURL(t *testing.T) { + bs := BookShelves{ + { + Name: "shelf-a", + Collections: map[string]*Collection{ + "col-1": { + Name: "col-1", + Marks: []*Mark{ + {ID: "abc12345", Name: "first", URL: "https://example.com/first"}, + }, + }, + }, + }, + { + Name: "shelf-b", + Collections: map[string]*Collection{ + "col-2": { + Name: "col-2", + Marks: []*Mark{ + {ID: "def67890", Name: "second", URL: "https://example.com/second"}, + }, + }, + }, + }, + } + bs.LoadParents() + + tests := []struct { + name string + id string + wantErr bool + }{ + { + name: "unique id passes", + id: "00000000", + wantErr: false, + }, + { + name: "duplicate in first shelf", + id: "abc12345", + wantErr: true, + }, + { + name: "duplicate in second shelf", + id: "def67890", + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := bs.VerifyUniqueURL(tt.id) + if tt.wantErr && err == nil { + t.Errorf("VerifyUniqueURL(%q) expected error, got nil", tt.id) + } + if !tt.wantErr && err != nil { + t.Errorf("VerifyUniqueURL(%q) unexpected error: %v", tt.id, err) + } + }) + } +} + +func TestAllTags(t *testing.T) { + tests := []struct { + name string + col *Collection + want []string + }{ + { + name: "sorts and merges across marks", + col: &Collection{ + Marks: []*Mark{ + {Tags: []string{"z", "a"}}, + {Tags: []string{"b", "a"}}, + }, + }, + want: []string{"a", "a", "b", "z"}, + }, + { + name: "empty collection", + col: &Collection{}, + want: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := tt.col.AllTags() + if !slices.Equal(got, tt.want) { + t.Errorf("AllTags() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestDeleteMark(t *testing.T) { + markA := &Mark{Name: "a"} + markB := &Mark{Name: "b"} + markC := &Mark{Name: "c"} + + tests := []struct { + name string + start []*Mark + remove *Mark + wantNames []string + wantLength int + }{ + { + name: "removes by pointer identity", + start: []*Mark{markA, markB, markC}, + remove: markB, + wantNames: []string{"a", "c"}, + wantLength: 2, + }, + { + name: "removing absent mark is no-op", + start: []*Mark{markA, markC}, + remove: markB, + wantNames: []string{"a", "c"}, + wantLength: 2, + }, + { + name: "removes only exact pointer match", + start: []*Mark{markA, &Mark{Name: "a"}}, + remove: markA, + wantNames: []string{"a"}, + wantLength: 1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + col := &Collection{Marks: tt.start} + col.DeleteMark(tt.remove) + + if len(col.Marks) != tt.wantLength { + t.Errorf("len(Marks) = %d, want %d", len(col.Marks), tt.wantLength) + } + + gotNames := make([]string, len(col.Marks)) + for i, m := range col.Marks { + gotNames[i] = m.Name + } + if !slices.Equal(gotNames, tt.wantNames) { + t.Errorf("remaining marks = %v, want %v", gotNames, tt.wantNames) + } + }) + } +} + +func TestGenerateID(t *testing.T) { + tests := []struct { + name string + url string + want string + }{ + { + name: "known URL golden value", + url: "https://example.com", + want: "100680ad", + }, + { + name: "different URL different id", + url: "https://example.org", + want: "50d7a905", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := GenerateID(tt.url) + if len(got) != 8 { + t.Errorf("GenerateID() length = %d, want 8", len(got)) + } + if got != tt.want { + t.Errorf("GenerateID() = %q, want %q", got, tt.want) + } + }) + } +} + +func TestMergeTags(t *testing.T) { + tests := []struct { + name string + in [][]string + want []string + }{ + { + name: "dedups and removes empty strings", + in: [][]string{{"a", "", "b"}, {"", "b", "c"}}, + want: []string{"a", "b", "c"}, + }, + { + name: "earlier arguments have priority", + in: [][]string{{"z", "a"}, {"a", "b"}}, + want: []string{"z", "a", "b"}, + }, + { + name: "empty input", + in: [][]string{}, + want: []string{}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := MergeTags(tt.in...) + if !slices.Equal(got, tt.want) { + t.Errorf("MergeTags() = %v, want %v", got, tt.want) + } + }) + } +} From 8184e688bf7885edfd7d42be2c2d62d4f3d7ceef Mon Sep 17 00:00:00 2001 From: Aaron Martell Date: Thu, 20 Aug 2026 16:30:30 -0500 Subject: [PATCH 6/6] chore: version bump --- VERSION | 2 +- internal/book/types_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/VERSION b/VERSION index d61de99..188bef5 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -v0.9.2 \ No newline at end of file +v0.9.3 diff --git a/internal/book/types_test.go b/internal/book/types_test.go index fff7ee7..152fce8 100644 --- a/internal/book/types_test.go +++ b/internal/book/types_test.go @@ -198,7 +198,7 @@ func TestDeleteMark(t *testing.T) { }, { name: "removes only exact pointer match", - start: []*Mark{markA, &Mark{Name: "a"}}, + start: []*Mark{markA, {Name: "a"}}, remove: markA, wantNames: []string{"a"}, wantLength: 1,