fix(koplugin): stop sync from wiping cloud book fields + Library polish (#4166)
* 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Huang Xin
@@ -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
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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
|
||||
|
||||
|
||||
@@ -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")
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user