fix: use correct file extension when generating name off book ID

[imnotjames]

there was a mismatch in the typing for getDownloadPath so the book filename property was getting accessed but that never existed. instead, switch to primary.fileName so that we get a correct file extension for generated book names

Summary by CodeRabbit

• Bug Fixes

  • Refined file extension detection for downloaded books, ensuring proper format identification and improved consistency in how book files are stored and managed within the application.

• Documentation

  • Added documentation comments to enhance code clarity for book download and synchronization functions.

[coderabbitai]

📝 Walkthrough

Walkthrough

The getBookDownloadPath function in the synchronize module is updated to derive downloaded-file extensions from the book's primary file instead of the book metadata itself. A LuaDoc annotation documents the book parameter, and the fallback extension logic now applies "bin" when no suffix is detected.

Changes

Book download path extension fix

Layer / File(s)	Summary
Book download path extension derivation grimmory.koplugin/grimmory/synchronize.lua	Added ---@param book Book LuaDoc annotation. Modified getBookDownloadPath to extract file extension from book.primary_file.filename (instead of book.filename) with a "bin" fallback, and use the computed extension when building the fallback downloaded-<id>.<ext> path.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Suggested labels

enhancement

Poem

🐰 In Grimm's great library, files are found,
Primary names now guide the way,
Extensions bloom where they abound,
With bin as anchor, come what may!
✨ Documentation whispers true,
The path is clearer, tried and new. 🎩

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title follows conventional commit format with 'fix:' prefix and clearly describes the main change: correcting file extension usage when generating downloaded filenames from book IDs.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

✨ Simplify code

• Create PR with simplified code

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

grimmory.koplugin/grimmory/synchronize.lua (1)

326-332: ⚡ Quick win

Consider adding nil safety check for book.primary_file.

The function accesses book.primary_file.filename at lines 333 and 349 without verifying that book.primary_file exists. While the current code path is safe (only books passing isTargetBook reach here), adding a defensive check would prevent potential runtime errors if this function is called directly elsewhere.

🛡️ Suggested defensive validation

 function GrimmorySynchronize:getBookDownloadPath(book)
     local download_directory = self.settings:getSyncDownloadDirectory()
 
     if not download_directory or download_directory == "" then
         return nil
     end
+
+    if not book.primary_file or not book.primary_file.filename then
+        logger:err("Book missing primary file information:", book.id)
+        return nil
+    end
 
     local download_path = download_directory .. "/" .. util.getSafeFilename(book.primary_file.filename)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@grimmory.koplugin/grimmory/synchronize.lua` around lines 326 - 332, In
GrimmorySynchronize:getBookDownloadPath, add a defensive nil check for
book.primary_file before accessing book.primary_file.filename (and any other
primary_file fields); if book.primary_file is nil, return nil (or handle
accordingly) so calls to getBookDownloadPath outside the isTargetBook path won't
error; update the checks around lines referencing book.primary_file.filename to
first verify book.primary_file exists.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@grimmory.koplugin/grimmory/synchronize.lua`:
- Around line 326-332: In GrimmorySynchronize:getBookDownloadPath, add a
defensive nil check for book.primary_file before accessing
book.primary_file.filename (and any other primary_file fields); if
book.primary_file is nil, return nil (or handle accordingly) so calls to
getBookDownloadPath outside the isTargetBook path won't error; update the checks
around lines referencing book.primary_file.filename to first verify
book.primary_file exists.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: ea16e061-8d85-46c7-b8ec-f6ee1c3d7484

📥 Commits

Reviewing files that changed from the base of the PR and between e9d8cb1 and 7283fc1.

📒 Files selected for processing (1)

• grimmory.koplugin/grimmory/synchronize.lua

📜 Review details

🔇 Additional comments (3)

grimmory.koplugin/grimmory/synchronize.lua (3)

325-325: LGTM!

---

349-353: LGTM!

---

355-355: LGTM!