From ab2def32dd3f319e56b2c1f278424ed4dddab4c4 Mon Sep 17 00:00:00 2001 From: Huang Xin Date: Fri, 15 May 2026 14:05:49 +0800 Subject: [PATCH] fix(koplugin): stop sync from wiping cloud book fields + Library polish (#4166) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(koplugin): pull before push so sync doesn't wipe cloud book fields, closes #4138 The Library sync pushed a touched book row before pulling, so a row still missing the cloud's uploaded_at / metadata / group_id (e.g. one created by lightScan, not yet merged from a cloud pull) was sent with those fields nil. The server's transformBookToDB explicit-nulls uploaded_at and metadata for any field absent from the wire payload, wiping the cloud copy — after which every device that pulled lost the book's upload state. syncBooks("both") now pulls first, then pushes, and takes a before_push callback. syncBooksLibrary passes touchOpenBook through it so the open book's updated_at bump lands after the pull has refreshed the local row, letting the push carry the preserved cloud fields. Co-Authored-By: Claude Opus 4.7 (1M context) * fix(koplugin): hide Library books with neither an uploaded nor a local file The Library showed any row with cloud_present = 1, but a bare cloud *record* whose file was never uploaded (uploaded_at NULL) has no cover and can't be opened — showing it is meaningless. Tighten the visibility predicate to (uploaded_at IS NOT NULL OR local_present = 1) across listBooks, getGroups, listBookshelfGroups and listBooksInGroup. This mirrors Readest, which only adds a synced book to the library when uploadedAt is set and keeps locally-imported books that carry a downloadedAt. Co-Authored-By: Claude Opus 4.7 (1M context) * fix(koplugin): close the Library widget when opening a book Opening a book from the Library called ReaderUI:showReader without closing the Library Menu, so it stayed in the UIManager widget stack with M._menu still set. A later background M.refresh() — a cloud-sync or cover-download completion — then repainted that ghost Library over the reader, making it flash on screen for a few seconds. Add M.close(); route the title-bar X, M.reopen() and both handleTap book-open paths through it. A wrapped onCloseWidget clears M._menu on every close path, so M.refresh() no-ops once the Menu is gone. Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- .../readest.koplugin/library/librarystore.lua | 28 ++++-- .../library/librarywidget.lua | 45 ++++++++-- apps/readest.koplugin/library/syncbooks.lua | 35 +++++--- apps/readest.koplugin/main.lua | 17 ++-- .../spec/library/librarystore_spec.lua | 59 ++++++++++++- .../spec/library/syncbooks_spec.lua | 85 +++++++++++++++++++ 6 files changed, 234 insertions(+), 35 deletions(-) diff --git a/apps/readest.koplugin/library/librarystore.lua b/apps/readest.koplugin/library/librarystore.lua index 95528585..219714ac 100644 --- a/apps/readest.koplugin/library/librarystore.lua +++ b/apps/readest.koplugin/library/librarystore.lua @@ -108,6 +108,18 @@ local GROUP_WHITELIST = { group_name = true, } +-- A book is shown in the Library only when its file is actually reachable: +-- either uploaded to Readest cloud (uploaded_at set, so the file + its cover +-- can be downloaded) or present on this device (local_present = 1). +-- +-- A bare cloud *record* with no uploaded file (cloud_present = 1 but +-- uploaded_at NULL) has no cover and cannot be opened, so showing it is +-- meaningless. This mirrors Readest, which only adds a synced book to the +-- library when uploadedAt is set, and keeps locally-imported books that +-- carry a downloadedAt — see useBooksSync.updateLibrary at +-- apps/readest-app/src/app/library/hooks/useBooksSync.ts:136-139. +local VISIBLE_BOOK_SQL = "(uploaded_at IS NOT NULL OR local_present = 1)" + local M = {} M.__index = M @@ -319,7 +331,7 @@ function M:listBooks(filters) local where = { "user_id = ?", "deleted_at IS NULL", - "(cloud_present = 1 OR local_present = 1)", + VISIBLE_BOOK_SQL, } local args = { self.user_id } @@ -400,11 +412,11 @@ function M:getGroups(group_by) MAX(created_at) AS latest_created FROM books WHERE user_id = ? AND deleted_at IS NULL - AND (cloud_present = 1 OR local_present = 1) + AND %s AND %s IS NOT NULL AND %s != '' GROUP BY %s ORDER BY name ASC - ]], group_by, group_by, group_by, group_by) + ]], group_by, VISIBLE_BOOK_SQL, group_by, group_by, group_by) local stmt = self.db:prepare(sql) stmt:reset():bind1(1, self.user_id) @@ -468,7 +480,7 @@ function M:listBookshelfGroups(group_by, parent_path) -- (one row per unique path), so a Lua-side bucket is cheap. -- Per-sort aggregates mirror getGroups so the merged-shelf sort can -- use a folder's "most recent child" timestamp under any sort_by. - local stmt = self.db:prepare([[ + local stmt = self.db:prepare(string.format([[ SELECT group_name, COUNT(*) AS cnt, MAX(updated_at) AS latest_updated, @@ -476,10 +488,10 @@ function M:listBookshelfGroups(group_by, parent_path) MAX(created_at) AS latest_created FROM books WHERE user_id = ? AND deleted_at IS NULL - AND (cloud_present = 1 OR local_present = 1) + AND %s AND group_name IS NOT NULL AND group_name != '' GROUP BY group_name - ]]) + ]], VISIBLE_BOOK_SQL)) stmt:reset():bind1(1, self.user_id) local prefix = parent_path and (parent_path .. "/") or nil @@ -585,11 +597,11 @@ function M:listBooksInGroup(group_by, group_value, limit, opts) local sql = string.format([[ SELECT %s FROM books WHERE user_id = ? AND deleted_at IS NULL - AND (cloud_present = 1 OR local_present = 1) + AND %s AND %s ORDER BY %s %s, hash ASC LIMIT ? - ]], table.concat(BOOK_COLS, ", "), where_extra, sort_expr, sort_dir) + ]], table.concat(BOOK_COLS, ", "), VISIBLE_BOOK_SQL, where_extra, sort_expr, sort_dir) local stmt = self.db:prepare(sql) stmt:reset() for i, v in ipairs(args) do stmt:bind1(i, v) end diff --git a/apps/readest.koplugin/library/librarywidget.lua b/apps/readest.koplugin/library/librarywidget.lua index 46062bfc..73cac56c 100644 --- a/apps/readest.koplugin/library/librarywidget.lua +++ b/apps/readest.koplugin/library/librarywidget.lua @@ -427,6 +427,21 @@ function M.refresh() M._menu:switchItemTable(title_for(M._search), items, jump_to) end +-- --------------------------------------------------------------------------- +-- close() — tear down the Library Menu if it's open. +-- --------------------------------------------------------------------------- +-- UIManager:close fires the Menu's onCloseWidget, which M.open wraps to +-- clear M._menu + the visible-hash filter. That matters because background +-- work (book-sync and cover-download completions) calls M.refresh(): once +-- M._menu is nil, refresh() no-ops instead of repainting a ghost Library +-- on top of whatever replaced it — e.g. the reader, after a book was +-- opened from the Library. Safe to call when nothing is open. +function M.close() + if M._menu then + UIManager:close(M._menu) + end +end + -- Close the current Menu and rebuild it from scratch using the latest -- settings. Used after view-menu changes that affect layout dimensions -- (view mode, column count, cover fit) — those are baked in at Menu @@ -435,13 +450,7 @@ end -- query (sort, group, search) keep using M.refresh() since rebuilding -- the whole Menu would be needless flicker. function M.reopen() - if M._menu then - UIManager:close(M._menu) - M._menu = nil - -- Clear the visible-hash filter so any in-flight BIM lookups - -- after close don't kick off downloads we no longer want. - libraryitem.set_visible_hashes(nil) - end + M.close() -- A view-mode/columns/cover-fit change is layout-only — keep the -- user's current drill-in. _keep_state tells M.open to skip its -- per-session resets (group_path, search reset). @@ -691,6 +700,21 @@ function M.open(opts, internal) } menu.onTapTitle = function() view_menu_callback() return true end + -- Drop the module reference whenever this Menu leaves the screen — + -- the title-bar X, the hardware Back key, M.close() on book-open, or + -- any other path all funnel through UIManager:close → onCloseWidget. + -- Without this, M._menu stays set after the Menu is gone and a later + -- M.refresh() (book-sync or cover-download completion) repaints a + -- ghost Library over whatever replaced it — e.g. the open reader. + local prev_on_close_widget = menu.onCloseWidget + menu.onCloseWidget = function(self, ...) + if M._menu == self then + M._menu = nil + libraryitem.set_visible_hashes(nil) + end + if prev_on_close_widget then return prev_on_close_widget(self, ...) end + end + M._menu = menu UIManager:show(menu) @@ -740,7 +764,10 @@ function M.handleTap(item, opts) }) return end + -- Close the Library before handing off to the reader so it isn't + -- left in the widget stack underneath — see M.close(). local ReaderUI = require("apps/reader/readerui") + M.close() ReaderUI:showReader(row.file_path) return end @@ -784,8 +811,10 @@ function M.handleTap(item, opts) hash = row.hash, title = row.title, local_present = 1, file_path = dst_or_err, }) - M.refresh() + -- Hand off to the reader and close the Library — no + -- M.refresh() needed since the Menu is going away. local ReaderUI = require("apps/reader/readerui") + M.close() ReaderUI:showReader(dst_or_err) end) end, diff --git a/apps/readest.koplugin/library/syncbooks.lua b/apps/readest.koplugin/library/syncbooks.lua index b0d144c3..628135bb 100644 --- a/apps/readest.koplugin/library/syncbooks.lua +++ b/apps/readest.koplugin/library/syncbooks.lua @@ -278,28 +278,41 @@ function M.pushChangedBooks(opts, cb) end -- --------------------------------------------------------------------------- --- syncBooks(opts, mode, cb) — convenience wrapper for the bidirectional --- sync the web does on auto-sync (useBooksSync.handleAutoSync). Modes: +-- syncBooks(opts, mode, cb, before_push) — convenience wrapper for the +-- bidirectional sync the web does on auto-sync (useBooksSync.handleAutoSync). +-- Modes: -- "push" — pushChangedBooks only -- "pull" — pullBooks only (existing fetch) --- "both" — push then pull (matches the web's syncBooks(..., 'both') call) +-- "both" — pull then push (closes #4138) -- cb is invoked once after the LAST step completes; intermediate failures --- are logged but do not abort (push failure shouldn't prevent pull). +-- are logged but do not abort (pull failure shouldn't prevent push). +-- +-- before_push (optional): callback invoked AFTER pull and BEFORE push in +-- "both" / "push" modes. Callers use this to bump updated_at on the open +-- book so its touched row gets included in the push delta — but crucially, +-- AFTER pull has refreshed the local row with the cloud's uploaded_at / +-- metadata / group_id. Doing the touch before pull (the original ordering) +-- meant the push could send a row with those fields nil, and the server's +-- transformBookToDB explicit-nulls uploaded_at and metadata for any field +-- absent in the wire payload — wiping the cloud copy on every device. +-- See apps/readest-app/src/utils/transform.ts:99,103. -- --------------------------------------------------------------------------- -function M.syncBooks(opts, mode, cb) +function M.syncBooks(opts, mode, cb, before_push) mode = mode or "both" if mode == "push" then + if before_push then before_push() end M.pushChangedBooks(opts, cb) elseif mode == "pull" then M.pullBooks(opts, cb) else -- "both" - M.pushChangedBooks(opts, function(push_ok, push_msg) - M.pullBooks(opts, function(pull_ok, pull_msg, pull_status) + M.pullBooks(opts, function(pull_ok, pull_msg, pull_status) + if before_push then before_push() end + M.pushChangedBooks(opts, function(push_ok, push_msg) if cb then - cb(push_ok and pull_ok, - string.format("push=%s/%s pull=%s/%s", - tostring(push_ok), tostring(push_msg), - tostring(pull_ok), tostring(pull_msg)), + cb(pull_ok and push_ok, + string.format("pull=%s/%s push=%s/%s", + tostring(pull_ok), tostring(pull_msg), + tostring(push_ok), tostring(push_msg)), pull_status) end end) diff --git a/apps/readest.koplugin/main.lua b/apps/readest.koplugin/main.lua index 75d2def6..54badfaf 100644 --- a/apps/readest.koplugin/main.lua +++ b/apps/readest.koplugin/main.lua @@ -669,8 +669,10 @@ end -- syncBooksLibrary(mode, interactive) — bidirectional book-row sync, -- mirroring useBooksSync.handleAutoSync at apps/readest-app/src/app/ -- library/hooks/useBooksSync.ts:66-78. mode: "push"|"pull"|"both". --- Touches the currently-open book first so its updated_at gets included --- in the push delta. Interactive=true shows toast feedback. +-- The touched-row bump happens via the before_push callback so it lands +-- AFTER pull has refreshed the local row with the cloud's uploaded_at / +-- metadata / group_id — see syncbooks.syncBooks docstring + issue #4138. +-- Interactive=true shows toast feedback. function ReadestSync:syncBooksLibrary(mode, interactive) if not self.settings.access_token or not self.settings.user_id then if interactive then @@ -686,11 +688,6 @@ function ReadestSync:syncBooksLibrary(mode, interactive) return end - if mode ~= "pull" then - -- Touch only matters for push (and 'both' which includes push) - self:touchOpenBook() - end - local syncbooks = require("library.syncbooks") syncbooks.syncBooks({ sync_auth = SyncAuth, @@ -711,6 +708,12 @@ function ReadestSync:syncBooksLibrary(mode, interactive) -- If the Library widget is open, refresh it so newly-pulled rows show local LibraryWidget = require("library.librarywidget") if LibraryWidget._menu then LibraryWidget.refresh() end + end, function() + -- before_push: bump updated_at on the open book so its row is in + -- the push delta. Runs after pull so the cloud's uploaded_at / + -- metadata / group_id have already merged into the local row; + -- touchBook then preserves those fields. + self:touchOpenBook() end) end diff --git a/apps/readest.koplugin/spec/library/librarystore_spec.lua b/apps/readest.koplugin/spec/library/librarystore_spec.lua index 04a5c7b9..ef0adb2c 100644 --- a/apps/readest.koplugin/spec/library/librarystore_spec.lua +++ b/apps/readest.koplugin/spec/library/librarystore_spec.lua @@ -11,7 +11,16 @@ local T_MID = 1750000000000 -- 2025-06-15 local T_RECENT = 1770000000000 -- 2026-02-01 -- Minimal book row factory; tests override fields they care about. +-- +-- The store only shows a book whose file is reachable: uploaded to cloud +-- (uploaded_at set) or present locally (local_present = 1). A real +-- cloud-present book always has its file uploaded — that's what makes it +-- downloadable — so the factory keeps cloud_present and uploaded_at +-- consistent: cloud_present = 1 implies an uploaded_at unless the test +-- pins one. To model a *phantom* cloud record (the book row exists but +-- the file was never uploaded), pass uploaded_at = false. local function book(over) + over = over or {} local row = { hash = "h" .. tostring(math.random(2 ^ 31)), meta_hash = nil, @@ -36,7 +45,12 @@ local function book(over) updated_at = T_OLD, deleted_at = nil, } - if over then for k, v in pairs(over) do row[k] = v end end + for k, v in pairs(over) do row[k] = v end + if row.uploaded_at == false then + row.uploaded_at = nil -- explicit phantom: cloud record, no file + elseif row.cloud_present == 1 and row.uploaded_at == nil then + row.uploaded_at = row.updated_at or T_OLD + end return row end @@ -317,6 +331,35 @@ describe("LibraryStore", function() for _, r in ipairs(rows) do assert.is_not.equal("Ghost", r.title) end end) + it("hides a phantom cloud record — cloud_present=1 but file never uploaded", function() + -- A book row synced from cloud whose file was never uploaded + -- (uploaded_at NULL) has no cover and cannot be opened. It is + -- not local either, so it must not appear in the Library. + store:upsertBook(book({ + hash = "h-phantom", title = "Phantom", + cloud_present = 1, uploaded_at = false, local_present = 0, + })) + local rows = store:listBooks({}) + assert.are.equal(3, #rows) + for _, r in ipairs(rows) do assert.is_not.equal("Phantom", r.title) end + end) + + it("shows a phantom cloud record once its file is present locally", function() + -- Same row as above, but the local scanner found the file. + -- local_present = 1 makes the book openable, so it shows. + store:upsertBook(book({ + hash = "h-phantom", title = "Phantom", + cloud_present = 1, uploaded_at = false, local_present = 1, + })) + local rows = store:listBooks({}) + assert.are.equal(4, #rows) + local found = false + for _, r in ipairs(rows) do + if r.title == "Phantom" then found = true end + end + assert.is_true(found) + end) + it("filters by case-insensitive substring search across title and author", function() local r1 = store:listBooks({ search = "asimov" }) assert.are.equal(1, #r1) @@ -414,6 +457,20 @@ describe("LibraryStore", function() assert.are.equal(2, groups[1].count) end) + it("excludes phantom cloud records from group counts", function() + -- A phantom row (cloud record, no uploaded file, not local) in + -- the Sci-Fi group must not inflate the group's book count. + store:upsertBook(book({ + hash = "g-phantom", title = "GP", author = "Anon", + cloud_present = 1, uploaded_at = false, local_present = 0, + group_name = "Sci-Fi", updated_at = T_RECENT, + })) + local groups = store:getGroups("group_name") + assert.are.equal(1, #groups) + assert.are.equal("Sci-Fi", groups[1].name) + assert.are.equal(2, groups[1].count) + end) + it("memoizes results across calls", function() local g1 = store:getGroups("author") local g2 = store:getGroups("author") diff --git a/apps/readest.koplugin/spec/library/syncbooks_spec.lua b/apps/readest.koplugin/spec/library/syncbooks_spec.lua index 8c48048d..dccf2885 100644 --- a/apps/readest.koplugin/spec/library/syncbooks_spec.lua +++ b/apps/readest.koplugin/spec/library/syncbooks_spec.lua @@ -250,4 +250,89 @@ describe("library.syncbooks", function() assert.are.equal("README (1)", out) end) end) + + -- ===================================================================== + -- syncBooks(opts, mode, cb, before_push) — bidirectional orchestration. + -- + -- The order matters: pull must run BEFORE push in "both" mode so the + -- local row has fresh cloud-side fields (uploaded_at, metadata, etc.) + -- before we touch + push it. Otherwise the push would send a row with + -- those fields nil, and the server's transformBookToDB explicit-nulls + -- them on the cloud — wiping out group/upload state on every device + -- that pulls afterward. Regression guard for issue #4138. + -- ===================================================================== + describe("syncBooks", function() + -- Stub the network-touching halves so we can record call order and + -- assert it without standing up Spore + a fake server. + local function with_stubs(fn) + local original_pull = syncbooks.pullBooks + local original_push = syncbooks.pushChangedBooks + local calls = {} + syncbooks.pullBooks = function(_opts, cb) + table.insert(calls, "pull") + if cb then cb(true, 0) end + end + syncbooks.pushChangedBooks = function(_opts, cb) + table.insert(calls, "push") + if cb then cb(true, 0) end + end + local ok, err = pcall(fn, calls) + syncbooks.pullBooks = original_pull + syncbooks.pushChangedBooks = original_push + if not ok then error(err) end + end + + it("runs pull before push in 'both' mode", function() + with_stubs(function(calls) + local before_push_at + syncbooks.syncBooks({}, "both", + function() end, + function() before_push_at = #calls end) + assert.are.same({ "pull", "push" }, calls) + -- before_push runs AFTER pull and BEFORE push — i.e. with + -- exactly one call ("pull") recorded so far. + assert.are.equal(1, before_push_at) + end) + end) + + it("invokes before_push between pull and push in 'both' mode", function() + with_stubs(function(calls) + local before_push_called = 0 + syncbooks.syncBooks({}, "both", + function() end, + function() before_push_called = before_push_called + 1 end) + assert.are.equal(1, before_push_called) + end) + end) + + it("invokes before_push and then push in 'push' mode", function() + with_stubs(function(calls) + local before_push_at + syncbooks.syncBooks({}, "push", + function() end, + function() before_push_at = #calls end) + assert.are.same({ "push" }, calls) + assert.are.equal(0, before_push_at) + end) + end) + + it("skips before_push in 'pull' mode (no push happens)", function() + with_stubs(function(calls) + local before_push_called = 0 + syncbooks.syncBooks({}, "pull", + function() end, + function() before_push_called = before_push_called + 1 end) + assert.are.same({ "pull" }, calls) + assert.are.equal(0, before_push_called) + end) + end) + + it("tolerates a missing before_push callback", function() + with_stubs(function(calls) + -- No before_push passed; orchestration should still work. + syncbooks.syncBooks({}, "both", function() end) + assert.are.same({ "pull", "push" }, calls) + end) + end) + end) end)