fix: cap about:blank visit count to 1 in history
- commit
- c7a34e5e2e5bff3f6c8f5c174677c5847edd36b4
- author
- brice · · 2026-01-26 04:55 UTC
- parent
- 01028b0
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
5 files · +114 -1
M internal/infrastructure/persistence/sqlite/history_repo.go +25 -1
| @@ -25,15 +25,35 @@ func NewHistoryRepository(db *sql.DB) repository.HistoryRepository { | |||
| 25 | 25 | return &historyRepo{queries: sqlc.New(db)} | |
| 26 | 26 | } | |
| 27 | 27 | ||
| 28 | + | // aboutBlankURL is the special URL for blank pages that should not accumulate visit counts. | |
| 29 | + | const aboutBlankURL = "about:blank" | |
| 30 | + | ||
| 28 | 31 | func (r *historyRepo) Save(ctx context.Context, entry *entity.HistoryEntry) error { | |
| 29 | 32 | log := logging.FromContext(ctx) | |
| 30 | 33 | log.Debug().Str("url", logging.TruncateURL(entry.URL, logURLMaxLen)).Msg("saving history entry") | |
| 31 | 34 | ||
| 32 | - | return r.queries.UpsertHistory(ctx, sqlc.UpsertHistoryParams{ | |
| 35 | + | err := r.queries.UpsertHistory(ctx, sqlc.UpsertHistoryParams{ | |
| 33 | 36 | Url: entry.URL, | |
| 34 | 37 | Title: sql.NullString{String: entry.Title, Valid: entry.Title != ""}, | |
| 35 | 38 | FaviconUrl: sql.NullString{String: entry.FaviconURL, Valid: entry.FaviconURL != ""}, | |
| 36 | 39 | }) | |
| 40 | + | if err != nil { | |
| 41 | + | return err | |
| 42 | + | } | |
| 43 | + | ||
| 44 | + | // Cap about:blank visit count to 1 so it exists but never dominates suggestions | |
| 45 | + | if entry.URL == aboutBlankURL { | |
| 46 | + | capErr := r.queries.CapVisitCount(ctx, sqlc.CapVisitCountParams{ | |
| 47 | + | VisitCount: sql.NullInt64{Int64: 1, Valid: true}, | |
| 48 | + | Url: aboutBlankURL, | |
| 49 | + | VisitCount_2: sql.NullInt64{Int64: 1, Valid: true}, | |
| 50 | + | }) | |
| 51 | + | if capErr != nil { | |
| 52 | + | log.Debug().Err(capErr).Msg("failed to cap about:blank visit count") | |
| 53 | + | } | |
| 54 | + | } | |
| 55 | + | ||
| 56 | + | return nil | |
| 37 | 57 | } | |
| 38 | 58 | ||
| 39 | 59 | func (r *historyRepo) FindByURL(ctx context.Context, url string) (*entity.HistoryEntry, error) { | |
| @@ -239,6 +259,10 @@ func (r *historyRepo) GetAllMostVisited(ctx context.Context) ([]*entity.HistoryE | |||
| 239 | 259 | } | |
| 240 | 260 | ||
| 241 | 261 | func (r *historyRepo) IncrementVisitCount(ctx context.Context, url string) error { | |
| 262 | + | // Skip incrementing about:blank - it should always have visit_count = 1 | |
| 263 | + | if url == aboutBlankURL { | |
| 264 | + | return nil | |
| 265 | + | } | |
| 242 | 266 | return r.queries.IncrementVisitCount(ctx, url) | |
| 243 | 267 | } | |
| 244 | 268 | ||
| @@ -25,15 +25,35 @@ func NewHistoryRepository(db *sql.DB) repository.HistoryRepository { | |||||
| 25 | return &historyRepo{queries: sqlc.New(db)} | 25 | return &historyRepo{queries: sqlc.New(db)} | ||
| 26 | } | 26 | } | ||
| 27 | 27 | ||||
| 28 | + | // aboutBlankURL is the special URL for blank pages that should not accumulate visit counts. | |||
| 29 | + | const aboutBlankURL = "about:blank" | |||
| 30 | + | ||||
| 28 | func (r *historyRepo) Save(ctx context.Context, entry *entity.HistoryEntry) error { | 31 | func (r *historyRepo) Save(ctx context.Context, entry *entity.HistoryEntry) error { | ||
| 29 | log := logging.FromContext(ctx) | 32 | log := logging.FromContext(ctx) | ||
| 30 | log.Debug().Str("url", logging.TruncateURL(entry.URL, logURLMaxLen)).Msg("saving history entry") | 33 | log.Debug().Str("url", logging.TruncateURL(entry.URL, logURLMaxLen)).Msg("saving history entry") | ||
| 31 | 34 | ||||
| 32 | - | return r.queries.UpsertHistory(ctx, sqlc.UpsertHistoryParams{ | 35 | + | err := r.queries.UpsertHistory(ctx, sqlc.UpsertHistoryParams{ |
| 33 | Url: entry.URL, | 36 | Url: entry.URL, | ||
| 34 | Title: sql.NullString{String: entry.Title, Valid: entry.Title != ""}, | 37 | Title: sql.NullString{String: entry.Title, Valid: entry.Title != ""}, | ||
| 35 | FaviconUrl: sql.NullString{String: entry.FaviconURL, Valid: entry.FaviconURL != ""}, | 38 | FaviconUrl: sql.NullString{String: entry.FaviconURL, Valid: entry.FaviconURL != ""}, | ||
| 36 | }) | 39 | }) | ||
| 40 | + | if err != nil { | |||
| 41 | + | return err | |||
| 42 | + | } | |||
| 43 | + | ||||
| 44 | + | // Cap about:blank visit count to 1 so it exists but never dominates suggestions | |||
| 45 | + | if entry.URL == aboutBlankURL { | |||
| 46 | + | capErr := r.queries.CapVisitCount(ctx, sqlc.CapVisitCountParams{ | |||
| 47 | + | VisitCount: sql.NullInt64{Int64: 1, Valid: true}, | |||
| 48 | + | Url: aboutBlankURL, | |||
| 49 | + | VisitCount_2: sql.NullInt64{Int64: 1, Valid: true}, | |||
| 50 | + | }) | |||
| 51 | + | if capErr != nil { | |||
| 52 | + | log.Debug().Err(capErr).Msg("failed to cap about:blank visit count") | |||
| 53 | + | } | |||
| 54 | + | } | |||
| 55 | + | ||||
| 56 | + | return nil | |||
| 37 | } | 57 | } | ||
| 38 | 58 | ||||
| 39 | func (r *historyRepo) FindByURL(ctx context.Context, url string) (*entity.HistoryEntry, error) { | 59 | func (r *historyRepo) FindByURL(ctx context.Context, url string) (*entity.HistoryEntry, error) { | ||
| @@ -239,6 +259,10 @@ func (r *historyRepo) GetAllMostVisited(ctx context.Context) ([]*entity.HistoryE | |||||
| 239 | } | 259 | } | ||
| 240 | 260 | ||||
| 241 | func (r *historyRepo) IncrementVisitCount(ctx context.Context, url string) error { | 261 | func (r *historyRepo) IncrementVisitCount(ctx context.Context, url string) error { | ||
| 262 | + | // Skip incrementing about:blank - it should always have visit_count = 1 | |||
| 263 | + | if url == aboutBlankURL { | |||
| 264 | + | return nil | |||
| 265 | + | } | |||
| 242 | return r.queries.IncrementVisitCount(ctx, url) | 266 | return r.queries.IncrementVisitCount(ctx, url) | ||
| 243 | } | 267 | } | ||
| 244 | 268 | ||||
M internal/infrastructure/persistence/sqlite/history_repo_test.go +70 -0
| @@ -576,3 +576,73 @@ func TestHistoryRepository_Search_SlashSeparatedQuery(t *testing.T) { | |||
| 576 | 576 | require.NoError(t, err) | |
| 577 | 577 | require.NotEmpty(t, results, "slash-separated query should match history") | |
| 578 | 578 | } | |
| 579 | + | ||
| 580 | + | func TestHistoryRepository_AboutBlank_CappedVisitCount(t *testing.T) { | |
| 581 | + | ctx := historyTestCtx() | |
| 582 | + | dbPath := filepath.Join(t.TempDir(), "dumber.db") | |
| 583 | + | ||
| 584 | + | db, err := sqlite.NewConnection(ctx, dbPath) | |
| 585 | + | require.NoError(t, err) | |
| 586 | + | t.Cleanup(func() { _ = db.Close() }) | |
| 587 | + | ||
| 588 | + | repo := sqlite.NewHistoryRepository(db) | |
| 589 | + | ||
| 590 | + | // Save about:blank multiple times - visit count should stay at 1 | |
| 591 | + | for i := 0; i < 5; i++ { | |
| 592 | + | require.NoError(t, repo.Save(ctx, &entity.HistoryEntry{ | |
| 593 | + | URL: "about:blank", | |
| 594 | + | Title: "New Tab", | |
| 595 | + | })) | |
| 596 | + | } | |
| 597 | + | ||
| 598 | + | // Verify about:blank has visit_count = 1 | |
| 599 | + | found, err := repo.FindByURL(ctx, "about:blank") | |
| 600 | + | require.NoError(t, err) | |
| 601 | + | require.NotNil(t, found) | |
| 602 | + | assert.Equal(t, int64(1), found.VisitCount, "about:blank should always have visit_count = 1") | |
| 603 | + | ||
| 604 | + | // Try incrementing visit count directly - should be ignored | |
| 605 | + | require.NoError(t, repo.IncrementVisitCount(ctx, "about:blank")) | |
| 606 | + | ||
| 607 | + | found2, err := repo.FindByURL(ctx, "about:blank") | |
| 608 | + | require.NoError(t, err) | |
| 609 | + | assert.Equal(t, int64(1), found2.VisitCount, "about:blank increment should be ignored") | |
| 610 | + | } | |
| 611 | + | ||
| 612 | + | func TestHistoryRepository_AboutBlank_NotDominant(t *testing.T) { | |
| 613 | + | ctx := historyTestCtx() | |
| 614 | + | dbPath := filepath.Join(t.TempDir(), "dumber.db") | |
| 615 | + | ||
| 616 | + | db, err := sqlite.NewConnection(ctx, dbPath) | |
| 617 | + | require.NoError(t, err) | |
| 618 | + | t.Cleanup(func() { _ = db.Close() }) | |
| 619 | + | ||
| 620 | + | repo := sqlite.NewHistoryRepository(db) | |
| 621 | + | ||
| 622 | + | // Save about:blank many times | |
| 623 | + | for i := 0; i < 10; i++ { | |
| 624 | + | require.NoError(t, repo.Save(ctx, &entity.HistoryEntry{ | |
| 625 | + | URL: "about:blank", | |
| 626 | + | Title: "New Tab", | |
| 627 | + | })) | |
| 628 | + | } | |
| 629 | + | ||
| 630 | + | // Save a regular entry with multiple visits | |
| 631 | + | require.NoError(t, repo.Save(ctx, &entity.HistoryEntry{ | |
| 632 | + | URL: "https://example.com", | |
| 633 | + | Title: "Example", | |
| 634 | + | })) | |
| 635 | + | require.NoError(t, repo.IncrementVisitCount(ctx, "https://example.com")) | |
| 636 | + | require.NoError(t, repo.IncrementVisitCount(ctx, "https://example.com")) | |
| 637 | + | ||
| 638 | + | // Get most visited - example.com should be first (3 visits vs 1) | |
| 639 | + | results, err := repo.GetAllMostVisited(ctx) | |
| 640 | + | require.NoError(t, err) | |
| 641 | + | require.Len(t, results, 2) | |
| 642 | + | ||
| 643 | + | assert.Equal(t, "https://example.com", results[0].URL, "example.com should be first with higher visit count") | |
| 644 | + | assert.Equal(t, int64(3), results[0].VisitCount) | |
| 645 | + | ||
| 646 | + | assert.Equal(t, "about:blank", results[1].URL, "about:blank should be second with capped visit count") | |
| 647 | + | assert.Equal(t, int64(1), results[1].VisitCount) | |
| 648 | + | } | |
| @@ -576,3 +576,73 @@ func TestHistoryRepository_Search_SlashSeparatedQuery(t *testing.T) { | |||||
| 576 | require.NoError(t, err) | 576 | require.NoError(t, err) | ||
| 577 | require.NotEmpty(t, results, "slash-separated query should match history") | 577 | require.NotEmpty(t, results, "slash-separated query should match history") | ||
| 578 | } | 578 | } | ||
| 579 | + | ||||
| 580 | + | func TestHistoryRepository_AboutBlank_CappedVisitCount(t *testing.T) { | |||
| 581 | + | ctx := historyTestCtx() | |||
| 582 | + | dbPath := filepath.Join(t.TempDir(), "dumber.db") | |||
| 583 | + | ||||
| 584 | + | db, err := sqlite.NewConnection(ctx, dbPath) | |||
| 585 | + | require.NoError(t, err) | |||
| 586 | + | t.Cleanup(func() { _ = db.Close() }) | |||
| 587 | + | ||||
| 588 | + | repo := sqlite.NewHistoryRepository(db) | |||
| 589 | + | ||||
| 590 | + | // Save about:blank multiple times - visit count should stay at 1 | |||
| 591 | + | for i := 0; i < 5; i++ { | |||
| 592 | + | require.NoError(t, repo.Save(ctx, &entity.HistoryEntry{ | |||
| 593 | + | URL: "about:blank", | |||
| 594 | + | Title: "New Tab", | |||
| 595 | + | })) | |||
| 596 | + | } | |||
| 597 | + | ||||
| 598 | + | // Verify about:blank has visit_count = 1 | |||
| 599 | + | found, err := repo.FindByURL(ctx, "about:blank") | |||
| 600 | + | require.NoError(t, err) | |||
| 601 | + | require.NotNil(t, found) | |||
| 602 | + | assert.Equal(t, int64(1), found.VisitCount, "about:blank should always have visit_count = 1") | |||
| 603 | + | ||||
| 604 | + | // Try incrementing visit count directly - should be ignored | |||
| 605 | + | require.NoError(t, repo.IncrementVisitCount(ctx, "about:blank")) | |||
| 606 | + | ||||
| 607 | + | found2, err := repo.FindByURL(ctx, "about:blank") | |||
| 608 | + | require.NoError(t, err) | |||
| 609 | + | assert.Equal(t, int64(1), found2.VisitCount, "about:blank increment should be ignored") | |||
| 610 | + | } | |||
| 611 | + | ||||
| 612 | + | func TestHistoryRepository_AboutBlank_NotDominant(t *testing.T) { | |||
| 613 | + | ctx := historyTestCtx() | |||
| 614 | + | dbPath := filepath.Join(t.TempDir(), "dumber.db") | |||
| 615 | + | ||||
| 616 | + | db, err := sqlite.NewConnection(ctx, dbPath) | |||
| 617 | + | require.NoError(t, err) | |||
| 618 | + | t.Cleanup(func() { _ = db.Close() }) | |||
| 619 | + | ||||
| 620 | + | repo := sqlite.NewHistoryRepository(db) | |||
| 621 | + | ||||
| 622 | + | // Save about:blank many times | |||
| 623 | + | for i := 0; i < 10; i++ { | |||
| 624 | + | require.NoError(t, repo.Save(ctx, &entity.HistoryEntry{ | |||
| 625 | + | URL: "about:blank", | |||
| 626 | + | Title: "New Tab", | |||
| 627 | + | })) | |||
| 628 | + | } | |||
| 629 | + | ||||
| 630 | + | // Save a regular entry with multiple visits | |||
| 631 | + | require.NoError(t, repo.Save(ctx, &entity.HistoryEntry{ | |||
| 632 | + | URL: "https://example.com", | |||
| 633 | + | Title: "Example", | |||
| 634 | + | })) | |||
| 635 | + | require.NoError(t, repo.IncrementVisitCount(ctx, "https://example.com")) | |||
| 636 | + | require.NoError(t, repo.IncrementVisitCount(ctx, "https://example.com")) | |||
| 637 | + | ||||
| 638 | + | // Get most visited - example.com should be first (3 visits vs 1) | |||
| 639 | + | results, err := repo.GetAllMostVisited(ctx) | |||
| 640 | + | require.NoError(t, err) | |||
| 641 | + | require.Len(t, results, 2) | |||
| 642 | + | ||||
| 643 | + | assert.Equal(t, "https://example.com", results[0].URL, "example.com should be first with higher visit count") | |||
| 644 | + | assert.Equal(t, int64(3), results[0].VisitCount) | |||
| 645 | + | ||||
| 646 | + | assert.Equal(t, "about:blank", results[1].URL, "about:blank should be second with capped visit count") | |||
| 647 | + | assert.Equal(t, int64(1), results[1].VisitCount) | |||
| 648 | + | } | |||
M internal/infrastructure/persistence/sqlite/queries/history.sql +3 -0
| @@ -97,3 +97,6 @@ ORDER BY last_visited DESC; | |||
| 97 | 97 | -- name: GetAllMostVisited :many | |
| 98 | 98 | SELECT * FROM history | |
| 99 | 99 | ORDER BY visit_count DESC, last_visited DESC; | |
| 100 | + | ||
| 101 | + | -- name: CapVisitCount :exec | |
| 102 | + | UPDATE history SET visit_count = ? WHERE url = ? AND visit_count > ?; | |
| @@ -97,3 +97,6 @@ ORDER BY last_visited DESC; | |||||
| 97 | -- name: GetAllMostVisited :many | 97 | -- name: GetAllMostVisited :many | ||
| 98 | SELECT * FROM history | 98 | SELECT * FROM history | ||
| 99 | ORDER BY visit_count DESC, last_visited DESC; | 99 | ORDER BY visit_count DESC, last_visited DESC; | ||
| 100 | + | ||||
| 101 | + | -- name: CapVisitCount :exec | |||
| 102 | + | UPDATE history SET visit_count = ? WHERE url = ? AND visit_count > ?; | |||
M internal/infrastructure/persistence/sqlite/sqlc/history.sql.go +15 -0
| @@ -10,6 +10,21 @@ import ( | |||
| 10 | 10 | "database/sql" | |
| 11 | 11 | ) | |
| 12 | 12 | ||
| 13 | + | const CapVisitCount = `-- name: CapVisitCount :exec | |
| 14 | + | UPDATE history SET visit_count = ? WHERE url = ? AND visit_count > ? | |
| 15 | + | ` | |
| 16 | + | ||
| 17 | + | type CapVisitCountParams struct { | |
| 18 | + | VisitCount sql.NullInt64 `json:"visit_count"` | |
| 19 | + | Url string `json:"url"` | |
| 20 | + | VisitCount_2 sql.NullInt64 `json:"visit_count_2"` | |
| 21 | + | } | |
| 22 | + | ||
| 23 | + | func (q *Queries) CapVisitCount(ctx context.Context, arg CapVisitCountParams) error { | |
| 24 | + | _, err := q.db.ExecContext(ctx, CapVisitCount, arg.VisitCount, arg.Url, arg.VisitCount_2) | |
| 25 | + | return err | |
| 26 | + | } | |
| 27 | + | ||
| 13 | 28 | const DeleteAllHistory = `-- name: DeleteAllHistory :exec | |
| 14 | 29 | DELETE FROM history | |
| 15 | 30 | ` | |
| @@ -10,6 +10,21 @@ import ( | |||||
| 10 | "database/sql" | 10 | "database/sql" | ||
| 11 | ) | 11 | ) | ||
| 12 | 12 | ||||
| 13 | + | const CapVisitCount = `-- name: CapVisitCount :exec | |||
| 14 | + | UPDATE history SET visit_count = ? WHERE url = ? AND visit_count > ? | |||
| 15 | + | ` | |||
| 16 | + | ||||
| 17 | + | type CapVisitCountParams struct { | |||
| 18 | + | VisitCount sql.NullInt64 `json:"visit_count"` | |||
| 19 | + | Url string `json:"url"` | |||
| 20 | + | VisitCount_2 sql.NullInt64 `json:"visit_count_2"` | |||
| 21 | + | } | |||
| 22 | + | ||||
| 23 | + | func (q *Queries) CapVisitCount(ctx context.Context, arg CapVisitCountParams) error { | |||
| 24 | + | _, err := q.db.ExecContext(ctx, CapVisitCount, arg.VisitCount, arg.Url, arg.VisitCount_2) | |||
| 25 | + | return err | |||
| 26 | + | } | |||
| 27 | + | ||||
| 13 | const DeleteAllHistory = `-- name: DeleteAllHistory :exec | 28 | const DeleteAllHistory = `-- name: DeleteAllHistory :exec | ||
| 14 | DELETE FROM history | 29 | DELETE FROM history | ||
| 15 | ` | 30 | ` | ||
M internal/infrastructure/persistence/sqlite/sqlc/querier.go +1 -0
| @@ -12,6 +12,7 @@ import ( | |||
| 12 | 12 | type Querier interface { | |
| 13 | 13 | AddToWhitelist(ctx context.Context, domain string) error | |
| 14 | 14 | AssignTagToFavorite(ctx context.Context, arg AssignTagToFavoriteParams) error | |
| 15 | + | CapVisitCount(ctx context.Context, arg CapVisitCountParams) error | |
| 15 | 16 | CreateFavorite(ctx context.Context, arg CreateFavoriteParams) (Favorite, error) | |
| 16 | 17 | CreateFolder(ctx context.Context, arg CreateFolderParams) (FavoriteFolder, error) | |
| 17 | 18 | CreateTag(ctx context.Context, arg CreateTagParams) (FavoriteTag, error) | |
| @@ -12,6 +12,7 @@ import ( | |||||
| 12 | type Querier interface { | 12 | type Querier interface { | ||
| 13 | AddToWhitelist(ctx context.Context, domain string) error | 13 | AddToWhitelist(ctx context.Context, domain string) error | ||
| 14 | AssignTagToFavorite(ctx context.Context, arg AssignTagToFavoriteParams) error | 14 | AssignTagToFavorite(ctx context.Context, arg AssignTagToFavoriteParams) error | ||
| 15 | + | CapVisitCount(ctx context.Context, arg CapVisitCountParams) error | |||
| 15 | CreateFavorite(ctx context.Context, arg CreateFavoriteParams) (Favorite, error) | 16 | CreateFavorite(ctx context.Context, arg CreateFavoriteParams) (Favorite, error) | ||
| 16 | CreateFolder(ctx context.Context, arg CreateFolderParams) (FavoriteFolder, error) | 17 | CreateFolder(ctx context.Context, arg CreateFolderParams) (FavoriteFolder, error) | ||
| 17 | CreateTag(ctx context.Context, arg CreateTagParams) (FavoriteTag, error) | 18 | CreateTag(ctx context.Context, arg CreateTagParams) (FavoriteTag, error) | ||