Conversation
Books in subfolders now show in device list, deleting removes the book from the list immediately, and newly added books show up immediately
📝 WalkthroughWalkthroughModified the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crosspoint_reader/driver.py
🧰 Additional context used
🪛 Ruff (0.15.1)
crosspoint_reader/driver.py
[warning] 157-157: Do not catch blind exception: Exception
(BLE001)
[warning] 174-174: Unused method argument: end_session
(ARG002)
🔇 Additional comments (4)
crosspoint_reader/driver.py (4)
174-196: Looks good: immediate in-memory list population.The recursive listing plus optional metadata hydration is wired cleanly and should satisfy the “show immediately” requirement.
371-382: Removal logic looks solid.Normalizing both
pathandlpathbefore comparison makes the deletion matching robust.
339-352: The code is correct as written. According to Calibre's DevicePlugin API,booklistsis always guaranteed to be a 3-tuple of BookList objects, neverNone. Thelen(booklists)call on line 341 is safe and will not raise a TypeError. Theif booklists:check on line 348 guards against an empty tuple/list, not aNonevalue. No fix is needed.
10-10: Document the minimum supported Calibre version and verifyBookListimport compatibility.The
calibre.devices.usbms.books.BookListclass is available in at least Calibre 5.11.0 and later releases, but the plugin does not declare a minimum version requirement. If the plugin must support older Calibre releases, confirm whetherBookListwas available at that point, and consider adding a compatibility fallback or explicit version constraint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crosspoint_reader/driver.py`:
- Around line 152-172: The current _list_files_recursive method swallows all
exceptions from self._http_get_json and returns an empty list, hiding real
errors; change it to let exceptions propagate (or re-raise after logging)
instead of returning results — either remove the try/except around the
self._http_get_json call or, if you want to keep the log, catch Exception as
exc, call self._log(f'[CrossPoint] listing {path} failed: {exc}'), then re-raise
the exception (raise) so callers and the test harness see the failure; keep the
rest of _list_files_recursive logic (including recursion and checks for
isDirectory/isEpub) unchanged.
| def _list_files_recursive(self, path='/'): | ||
| """Return a flat list of (lpath, size) for all EPUB files on device.""" | ||
| results = [] | ||
| try: | ||
| entries = self._http_get_json('/api/files', params={'path': path}) | ||
| except Exception as exc: | ||
| self._log(f'[CrossPoint] listing {path} failed: {exc}') | ||
| return results | ||
| for entry in entries: | ||
| if entry.get('isDirectory'): | ||
| continue | ||
| if not entry.get('isEpub'): | ||
| continue | ||
| name = entry.get('name', '') | ||
| if not name: | ||
| continue | ||
| size = entry.get('size', 0) | ||
| lpath = '/' + name if not name.startswith('/') else name | ||
| title = os.path.splitext(os.path.basename(name))[0] | ||
| if path == '/': | ||
| entry_path = '/' + name | ||
| else: | ||
| entry_path = path + '/' + name | ||
| if entry.get('isDirectory'): | ||
| results.extend(self._list_files_recursive(entry_path)) | ||
| elif entry.get('isEpub'): | ||
| results.append((entry_path, entry.get('size', 0))) | ||
| return results |
There was a problem hiding this comment.
Don’t swallow listing failures.
Returning an empty list on any exception makes the device appear empty and hides real errors. Let the exception propagate (or re‑raise after logging) so failures surface.
💡 Suggested fix
- try:
- entries = self._http_get_json('/api/files', params={'path': path})
- except Exception as exc:
- self._log(f'[CrossPoint] listing {path} failed: {exc}')
- return results
+ try:
+ entries = self._http_get_json('/api/files', params={'path': path})
+ except ControlError as exc:
+ self._log(f'[CrossPoint] listing {path} failed: {exc}')
+ raise🧰 Tools
🪛 Ruff (0.15.1)
[warning] 157-157: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crosspoint_reader/driver.py` around lines 152 - 172, The current
_list_files_recursive method swallows all exceptions from self._http_get_json
and returns an empty list, hiding real errors; change it to let exceptions
propagate (or re-raise after logging) instead of returning results — either
remove the try/except around the self._http_get_json call or, if you want to
keep the log, catch Exception as exc, call self._log(f'[CrossPoint] listing
{path} failed: {exc}'), then re-raise the exception (raise) so callers and the
test harness see the failure; keep the rest of _list_files_recursive logic
(including recursion and checks for isDirectory/isEpub) unchanged.
1 participant
Books in subfolders now show in device list, deleting removes the book from the list immediately, and newly added books show up immediately