Skip to content

No changes required - placeholder verification already implemented#44

Merged
shiggsy365 merged 1 commit into
mainfrom
copilot/fix-placeholder-deletion-issue
Dec 7, 2025
Merged

No changes required - placeholder verification already implemented#44
shiggsy365 merged 1 commit into
mainfrom
copilot/fix-placeholder-deletion-issue

Conversation

Copilot AI commented Dec 7, 2025

Copy link
Copy Markdown
Contributor

The issue describes a bug where placeholder books are deleted even when the downloaded file is still a placeholder. However, this fix is already implemented in the codebase.

Current Implementation

The _finishPlaceholderDownload function already contains the required verification logic:

Before placeholder deletion (lines 310-331):

-- CRITICAL: Verify the temp file is NOT a placeholder before we proceed
local PlaceholderGenerator = require("placeholder_generator")
local temp_is_placeholder = PlaceholderGenerator:isPlaceholder(temp_filepath)

if temp_is_placeholder then
    logger.err("OPDS: ERROR - Downloaded file is a placeholder!")
    os.remove(temp_filepath)
    UIHelpers.showError(_("Download failed: Server returned a placeholder..."))
    -- Return to file manager WITHOUT deleting original placeholder
    return
end

-- Only reached if temp file is verified as real book
logger.info("OPDS: Deleting placeholder file:", placeholder_path)
os.remove(placeholder_path)

After file rename (lines 379-400):

  • Additional safety check verifies final file is not a placeholder
  • Returns to file manager with error if verification fails

Verification Method

PlaceholderGenerator:isPlaceholder() opens the EPUB archive and checks for the marker:

<meta property="opdsbrowser:placeholder">true</meta>

Behavior

✅ Download succeeds with real book → placeholder deleted and replaced
✅ Download returns placeholder → original preserved, error shown

This implementation exactly matches the "Reference Implementation" in the issue description. No changes needed.

Original prompt

Problem

The placeholder book is being deleted even when the downloaded file is still a placeholder (not the actual book). This happens in the _finishPlaceholderDownload function in opdsbrowser.koplugin/main.lua.

Root Cause

In the _finishPlaceholderDownload function (around lines 304-388), the code deletes the placeholder file and removes it from the database without first verifying that the downloaded temp file is actually a real book and not still a placeholder.

The current flow is:

  1. Delete placeholder file
  2. Rename temp file to final location
  3. Open the file

This means if the server returns another placeholder instead of the real book, the original placeholder gets deleted and replaced with... another placeholder.

Solution Required

Add verification logic before deleting the placeholder to check if the downloaded temp file is actually a real book:

  1. After the temp file is downloaded, verify it's NOT a placeholder using PlaceholderGenerator:isPlaceholder(temp_filepath)
  2. If the temp file IS still a placeholder:
    • Show an error to the user
    • Delete the temp file
    • DO NOT delete the original placeholder
    • Return to file manager
  3. Only if the temp file is verified as a real book should we:
    • Delete the original placeholder
    • Rename the temp file to the final location
    • Remove from placeholder database
    • Open the book

Reference Implementation

Looking at the semantic search results, there appears to be a newer version of this code (lines 294-340 in the search results) that includes this verification:

-- CRITICAL: Verify the temp file is NOT a placeholder before we proceed
local PlaceholderGenerator = require("placeholder_generator")
local temp_is_placeholder = PlaceholderGenerator:isPlaceholder(temp_filepath)
logger.info("OPDS: Checking if downloaded temp file is placeholder:", temp_is_placeholder)

if temp_is_placeholder then
    logger.err("OPDS: ERROR - Downloaded file is a placeholder!")
    logger.err("OPDS: The server sent us a placeholder instead of the real book")
    logger.err("OPDS: Download URL may be incorrect or server returned wrong content")
    os.remove(temp_filepath)
    UIHelpers.showError(_("Download failed: Server returned a placeholder instead of the book.\n\nPlease try downloading manually or contact support."))

    -- Return to file manager
    local FileManager = require("apps/filemanager/filemanager")
    if not FileManager.instance then
        local dir = placeholder_path:match("(.*/)")
        FileManager:showFiles(dir)
    else
        FileManager.instance:onRefresh()
    end
    return
end

logger.info("OPDS: Verified - temp file is a real book (not placeholder)")

-- Delete the placeholder file
logger.info("OPDS: Deleting placeholder file:", placeholder_path)

This verification logic needs to be added to the _finishPlaceholderDownload function.

Files to Modify

  • opdsbrowser.koplugin/main.lua - Add verification before deleting placeholder in _finishPlaceholderDownload function

Expected Behavior After Fix

  • If download succeeds with a real book: placeholder is deleted and replaced ✓
  • If download fails or returns a placeholder: original placeholder is preserved, error shown to user ✓

This pull request was created as a result of the following prompt from Copilot chat.

Problem

The placeholder book is being deleted even when the downloaded file is still a placeholder (not the actual book). This happens in the _finishPlaceholderDownload function in opdsbrowser.koplugin/main.lua.

Root Cause

In the _finishPlaceholderDownload function (around lines 304-388), the code deletes the placeholder file and removes it from the database without first verifying that the downloaded temp file is actually a real book and not still a placeholder.

The current flow is:

  1. Delete placeholder file
  2. Rename temp file to final location
  3. Open the file

This means if the server returns another placeholder instead of the real book, the original placeholder gets deleted and replaced with... another placeholder.

Solution Required

Add verification logic before deleting the placeholder to check if the downloaded temp file is actually a real book:

  1. After the temp file is downloaded, verify it's NOT a placeholder using PlaceholderGenerator:isPlaceholder(temp_filepath)
  2. If the temp file IS still a placeholder:
    • Show an error to the user
    • Delete the temp file
    • DO NOT delete the original placeholder
    • Return to file manager
  3. Only if the temp file is verified as a real book should we:
    • Delete the original placeholder
    • Rename the temp file to the final location
    • Remove from placeholder database
    • Open the book

Reference Implementation

Looking at the semantic search results, there appears to be a newer version of this code (lines 294-340 in the search results) that includes this verification:

-- CRITICAL: Verify the temp file is NOT a placeholder before we proceed
local PlaceholderGenerator = require("placeholder_generator")
local temp_is_placeholder = PlaceholderGenerator:isPlaceholder(temp_filepath)
logger.info("OPDS: Checking if downloaded temp file is placeholder:", temp_is_placeholder)

if temp_is_placeholder then
    logger.err("OPDS: ERROR - Downloaded file is a placeholder!")
    logger.err("OPDS: The server sent us a placeholder instead of the real book")
    logger.err("OPDS: Download URL may be incorrect or server returned wrong content")
    os.remove(temp_filepath)
    UIHelpers.showError(_("Download failed: Server returned a placeholder instead of the book.\n\nPlease try downloading manually or contact support."))

    -- Return to file manager
    local FileManager = require("apps/filemanager/filemanager")
    if not FileManager.instance then
        local dir = placeholder_path:match("(.*/)")
        FileManager:showFiles(dir)
    else
        FileManager.instance:onRefresh()
    end
    return
end

logger.info("OPDS: Verified - temp file is a real book (not placeholder)")

-- Delete the placeholder file
logger.info("OPDS: Deleting placeholder file:", placeholder_path)

This verification logic needs to be added to the _finishPlaceholderDownload function.

Files to Modify

  • opdsbrowser.koplugin/main.lua - Add verification before deleting placeholder in _finishPlaceholderDownload function

Expected Behavior After Fix

  • If download succeeds with a real book: placeholder is deleted and replaced ✓
  • If download fails or returns a placeholder: original placeholder is preserved, error shown to user ✓

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Copilot AI changed the title [WIP] Fix placeholder file deletion in download process No changes required - placeholder verification already implemented Dec 7, 2025
Copilot AI requested a review from shiggsy365 December 7, 2025 23:13
@shiggsy365 shiggsy365 marked this pull request as ready for review December 7, 2025 23:14
@shiggsy365 shiggsy365 merged commit cbb68cc into main Dec 7, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants