Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a configurable EPUB-to-JPEG image conversion pipeline and UI controls to the CrossPoint Reader plugin (v0.2.3). Introduces EpubConverter for multi-pass EPUB processing, integrates conversion into the driver upload flow with temporary-file management, updates docs and module metadata, and extends configuration preferences and UI. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Config as Config UI
participant Driver as Device Driver
participant Converter as EpubConverter
participant FS as File System
participant Device as Wireless Device
User->>Config: Set conversion prefs (enable, quality, light-novel, size, overlap)
Config->>FS: Persist PREFS
User->>Driver: Upload EPUB(s)
Driver->>Driver: Read PREFS
alt conversion enabled
Driver->>Converter: _convert_epub(input_path)
Converter->>FS: Read input EPUB (ZIP)
Converter->>Converter: Pass 1: process/convert images (convert/split)
Converter->>Converter: Pass 2: update XHTML refs & SVG cover fixes
Converter->>Converter: Pass 3: update OPF manifest/meta
Converter->>Converter: Pass 4: copy remaining files & update CSS/NCX refs
Converter->>FS: Write converted EPUB (temp)
Converter-->>Driver: Return converted_path
Driver->>FS: Track/cleanup temp files
Driver->>Device: Upload converted_path
else
Driver->>Device: Upload original EPUB
end
Device-->>Driver: Upload result
Driver-->>User: Transfer complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. 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: 2
🧹 Nitpick comments (3)
crosspoint_reader/converter.py (1)
169-210: Closures inside loop capture loop variables by reference (B023).
replace_blockandreplace_simplereferenceparts,orig_name, andnew_namefrom the enclosingforloop. Since they're passed immediately tore.sub, the current behavior is correct at runtime. However, this is a well-known Python pitfall — any future refactoring that defers execution would silently break. The standard fix is to bind via default arguments.Proposed fix for replace_block (same pattern for replace_simple)
- def replace_block(match): + def replace_block(match, parts=parts, orig_name=orig_name, new_name=new_name): result = [] for i, part in enumerate(parts): if i > 0: result.append('\n') new_block = match.group(0).replace(orig_name, part['imgName']) new_block = new_block.replace(new_name, part['imgName']) result.append(new_block) return ''.join(result)- def replace_simple(match): + def replace_simple(match, parts=parts, orig_name=orig_name, new_name=new_name): result = [] for i, part in enumerate(parts): if i > 0: result.append('\n') new_src = match.group(2).replace(orig_name, part['imgName']) new_src = new_src.replace(new_name, part['imgName']) result.append(match.group(1) + new_src + match.group(3)) return ''.join(result)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crosspoint_reader/converter.py` around lines 169 - 210, The nested functions replace_block and replace_simple capture loop variables parts, orig_name, and new_name by reference which is fragile; bind these loop variables as default arguments to the functions so each closure gets its own copy (e.g., def replace_block(match, parts=parts, orig_name=orig_name, new_name=new_name): and similarly for replace_simple) before passing them into block_pattern.sub and simple_pattern.sub; update both closures accordingly to use the bound names instead of relying on outer-scope lookups.crosspoint_reader/driver.py (1)
365-368:_progressclosure captures loop variableiby reference.The
_progresscallback referencesifrom the enclosingforloop. Whilews_client.upload_filelikely calls this callback synchronously (making the current behavior correct), this is the same late-binding closure pitfall flagged by B023. Bindivia a default argument to be safe.Proposed fix
- def _progress(sent, size): + def _progress(sent, size, i=i): if size > 0: self.report_progress((i + sent / float(size)) / float(total), 'Transferring books to device...')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crosspoint_reader/driver.py` around lines 365 - 368, The _progress callback captures the loop variable i by reference which can cause late-binding issues; modify the closure definition in driver.py so _progress binds the current i as a default argument (e.g. def _progress(sent, size, i=i): ...) and keep the existing logic calling self.report_progress((i + sent/float(size)) / float(total), 'Transferring books to device...'); this ensures ws_client.upload_file receives a callback that references the intended loop index even if invoked later.crosspoint_reader/config.py (1)
109-120: Preset buttons remain enabled when conversion is toggled off.When
enable_conversionis unchecked,_update_conversion_enableddisables the slider, label, and other controls — but the quality preset buttons (Low,Medium,High,Max) are not disabled. Users can still click them, which updates the disabled slider. Consider tracking and disabling these buttons as well.Proposed fix
Store the preset widget and disable it in
_update_conversion_enabled:conv_layout.addRow('JPEG Quality', quality_widget) # Quality presets presets_widget = QWidget() presets_layout = QHBoxLayout(presets_widget) presets_layout.setContentsMargins(0, 0, 0, 0) for name, value in [('Low (60%)', 60), ('Medium (75%)', 75), ('High (85%)', 85), ('Max (95%)', 95)]: btn = QPushButton(name) btn.clicked.connect(lambda checked, v=value: self._set_quality(v)) presets_layout.addWidget(btn) + self.presets_widget = presets_widget conv_layout.addRow('Presets', presets_widget)def _update_conversion_enabled(self, enabled): """Enable/disable conversion options based on master checkbox.""" self.jpeg_quality.setEnabled(enabled) self.quality_label.setEnabled(enabled) + self.presets_widget.setEnabled(enabled) self.light_novel_mode.setEnabled(enabled) self.screen_width.setEnabled(enabled) self.screen_height.setEnabled(enabled) self.split_overlap.setEnabled(enabled)Also applies to: 197-204
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crosspoint_reader/config.py` around lines 109 - 120, The preset buttons are not being disabled when conversion is toggled off; modify the creation in the constructor where you build the presets (the block that creates presets_widget, QPushButton instances and connects via lambda to self._set_quality) so that you store either the presets_widget or the list of QPushButton objects as an instance attribute (e.g., self._quality_presets_widget or self._quality_preset_buttons), and then update _update_conversion_enabled to call setEnabled(enabled) on that stored widget/list (or iterate and setEnabled on each QPushButton). Repeat the same change for the similar block around lines 197-204 so both preset groups are tracked and disabled when conversion is turned off.
🤖 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/converter.py`:
- Around line 162-166: The current loop in converter.py that updates image
references (iterating over renamed and using old_name/new_name to do t =
t.replace(...)) wrongly performs basename-only, global string replacements on
the entire XHTML text (variables renamed, old_name, new_name, and t), causing
collisions and false matches; fix it by replacing only the attribute values for
src/href (or use an XML/HTML parser) instead of global text replace: parse the
XHTML or run a targeted regex that matches src="/path/..." and href="/path/..."
and swap the full original path (use the keys from renamed) with the
corresponding new path, so replacements operate on exact attribute values and
use full paths rather than basenames. Ensure you update the same t variable with
the safer replacements.
- Around line 97-291: The output ZIP currently writes images/XHTML/OPF before
writing 'mimetype', violating EPUB rules; modify the block that opens zout (the
with zipfile.ZipFile(output_path, 'w') as zout: section using zin/zout) to write
the 'mimetype' entry first (zout.writestr('mimetype',
b'mimetype-content-or-read-from-zin', compress_type=zipfile.ZIP_STORED) using
the original mimetype bytes read from zin) before any other writes, ensure it is
stored uncompressed (ZIP_STORED) and skip it in the later "Fourth pass" copy
loop (the loop over zin.namelist() that currently decides compress = ZIP_STORED
if name == 'mimetype' else ZIP_DEFLATED); also avoid adding extra fields or
compression flags when writing that first entry.
---
Nitpick comments:
In `@crosspoint_reader/config.py`:
- Around line 109-120: The preset buttons are not being disabled when conversion
is toggled off; modify the creation in the constructor where you build the
presets (the block that creates presets_widget, QPushButton instances and
connects via lambda to self._set_quality) so that you store either the
presets_widget or the list of QPushButton objects as an instance attribute
(e.g., self._quality_presets_widget or self._quality_preset_buttons), and then
update _update_conversion_enabled to call setEnabled(enabled) on that stored
widget/list (or iterate and setEnabled on each QPushButton). Repeat the same
change for the similar block around lines 197-204 so both preset groups are
tracked and disabled when conversion is turned off.
In `@crosspoint_reader/converter.py`:
- Around line 169-210: The nested functions replace_block and replace_simple
capture loop variables parts, orig_name, and new_name by reference which is
fragile; bind these loop variables as default arguments to the functions so each
closure gets its own copy (e.g., def replace_block(match, parts=parts,
orig_name=orig_name, new_name=new_name): and similarly for replace_simple)
before passing them into block_pattern.sub and simple_pattern.sub; update both
closures accordingly to use the bound names instead of relying on outer-scope
lookups.
In `@crosspoint_reader/driver.py`:
- Around line 365-368: The _progress callback captures the loop variable i by
reference which can cause late-binding issues; modify the closure definition in
driver.py so _progress binds the current i as a default argument (e.g. def
_progress(sent, size, i=i): ...) and keep the existing logic calling
self.report_progress((i + sent/float(size)) / float(total), 'Transferring books
to device...'); this ensures ws_client.upload_file receives a callback that
references the intended loop index even if invoked later.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
crosspoint_reader-v0.1.1.zipis excluded by!**/*.zipcrosspoint_reader-v0.2.0.zipis excluded by!**/*.zip
📒 Files selected for processing (5)
crosspoint_reader/README.mdcrosspoint_reader/__init__.pycrosspoint_reader/config.pycrosspoint_reader/converter.pycrosspoint_reader/driver.py
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (3)
crosspoint_reader/__init__.py (1)
crosspoint_reader/driver.py (1)
CrossPointDevice(20-472)
crosspoint_reader/converter.py (2)
crosspoint_reader/driver.py (2)
_log(53-59)open(96-99)crosspoint_reader/config.py (1)
save(206-221)
crosspoint_reader/config.py (1)
crosspoint_reader/log.py (1)
get_log_text(16-17)
🪛 Ruff (0.15.2)
crosspoint_reader/driver.py
[warning] 24-24: Mutable default value for class attribute
(RUF012)
[warning] 307-307: Consider moving this statement to an else block
(TRY300)
[warning] 309-309: Do not catch blind exception: Exception
(BLE001)
[warning] 336-336: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
[warning] 367-367: Function definition does not bind loop variable i
(B023)
[error] 390-391: try-except-pass detected, consider logging the exception
(S110)
[warning] 390-390: Do not catch blind exception: Exception
(BLE001)
crosspoint_reader/converter.py
[warning] 51-51: Unused lambda argument: x
(ARG005)
[warning] 182-182: Function definition does not bind loop variable parts
(B023)
[warning] 185-185: Function definition does not bind loop variable orig_name
(B023)
[warning] 186-186: Function definition does not bind loop variable new_name
(B023)
[warning] 202-202: Function definition does not bind loop variable parts
(B023)
[warning] 205-205: Function definition does not bind loop variable orig_name
(B023)
[warning] 206-206: Function definition does not bind loop variable new_name
(B023)
[warning] 343-343: Do not catch blind exception: Exception
(BLE001)
crosspoint_reader/config.py
[warning] 117-117: Unused lambda argument: checked
(ARG005)
[warning] 142-142: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
🔇 Additional comments (8)
crosspoint_reader/__init__.py (1)
1-22: LGTM!Module docstring and class docstring additions look good. The feature list in the docstring accurately reflects the capabilities introduced in this PR.
crosspoint_reader/README.md (1)
1-105: LGTM!The README is comprehensive and well-structured, covering features, configuration, installation, usage, and troubleshooting. It aligns well with the code changes across the PR.
crosspoint_reader/config.py (1)
42-74: LGTM!The connection and conversion settings are well-organized into grouped sections. The
save()method correctly persists all new preferences with appropriate type casts, and defaults are sensible.Also applies to: 206-221
crosspoint_reader/converter.py (3)
25-61: LGTM!The
EpubConverterconstructor is well-designed with sensible defaults, clamped quality range, and clean statistics tracking. The no-op logger fallback is appropriate.
311-406: LGTM!Image processing logic is solid — the RGBA/LA/P mode handling with white background compositing is correct, the scaling uses proper aspect ratio preservation, and the split-rotate logic for manga/RTL reading order is well-implemented with correct overlap calculations.
408-481: LGTM!The SVG cover fix and cover meta enforcement methods handle multiple EPUB variations robustly with appropriate fallback behavior.
crosspoint_reader/driver.py (2)
279-312: LGTM!The
_convert_epubmethod cleanly integrates with preferences, creates a temporary file for the converted output, and gracefully falls back to the original file on failure.
333-393: Good use of try/finally for temp file cleanup.The upload flow correctly tracks converted temp files and ensures cleanup in the
finallyblock, preventing temporary file leaks even if uploads fail partway through.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crosspoint_reader/driver.py (1)
36-39: 🛠️ Refactor suggestion | 🟠 Major
MUST_READ_METADATAclass attribute is defined twice.Line 36 and line 39 both set
MUST_READ_METADATA = False. The second definition silently shadows the first and is confusing.♻️ Proposed fix
MUST_READ_METADATA = False MANAGES_DEVICE_PRESENCE = True DEVICE_PLUGBOARD_NAME = 'CROSSPOINT_READER' - MUST_READ_METADATA = False SUPPORTS_DEVICE_DB = False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crosspoint_reader/driver.py` around lines 36 - 39, The class-level attribute MUST_READ_METADATA is declared twice (both set to False) which is redundant and confusing; remove the duplicate declaration so there is only a single MUST_READ_METADATA = False alongside the other attributes (MANAGES_DEVICE_PRESENCE and DEVICE_PLUGBOARD_NAME) in the class/driver definition, ensuring no other logic depends on the second assignment.
♻️ Duplicate comments (1)
crosspoint_reader/converter.py (1)
169-173:⚠️ Potential issue | 🟠 MajorBasename-only replacement still present — same issue as previous unresolved comment.
Lines 170–173 still perform
str.replaceon the full XHTML text using only basenames. The same pattern reappears in the OPF update pass (lines 229–232) and the CSS/NCX pass (lines 294–297). All three locations share the same defects: images in different directories with identical basenames will collide, and the replacement can match arbitrary text/attribute values rather than onlysrc/hrefattributes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crosspoint_reader/converter.py` around lines 169 - 173, The current loop in converter.py that does "for old, new in renamed.items()" and calls t = t.replace(old_name, new_name) incorrectly replaces basenames across the entire XHTML/OPF/CSS/NCX text; update these three passes (the XHTML pass where variable t is modified, the OPF update pass, and the CSS/NCX pass) to perform attribute-scoped replacements instead of blind basename replacements: parse the XML/HTML (e.g., xml/HTML parser used in this module) and update only the src/href attributes (or other relevant attributes) on elements (img, source, link, a, etc.) by matching the original full path or basename in the attribute value and replacing it with the new path from renamed[old]; this ensures changes target attribute values only and preserves distinct files with identical basenames in different directories.
🧹 Nitpick comments (5)
crosspoint_reader/converter.py (2)
126-127: JPEG images written withZIP_DEFLATED— wastes CPU with negligible benefit.JPEG data is already entropy-encoded and essentially incompressible. Writing JPEG images with
ZIP_DEFLATEDburns CPU time for near-zero (often negative) size savings. UseZIP_STOREDfor all image output.⚡ Proposed fix
- zout.writestr(new_path, parts[0]['data'], - compress_type=zipfile.ZIP_DEFLATED) + zout.writestr(new_path, parts[0]['data'], + compress_type=zipfile.ZIP_STORED)- zout.writestr(part_path, part['data'], - compress_type=zipfile.ZIP_DEFLATED) + zout.writestr(part_path, part['data'], + compress_type=zipfile.ZIP_STORED)Also applies to: 141-142, 300-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crosspoint_reader/converter.py` around lines 126 - 127, The JPEG outputs are being written with zipfile.ZIP_DEFLATED which wastes CPU; change the compress_type passed to zout.writestr for JPEG image writes to zipfile.ZIP_STORED. Update the three occurrences where zout.writestr(..., parts[0]['data'], compress_type=zipfile.ZIP_DEFLATED) (and similar calls writing image bytes/new_path) are used — replace ZIP_DEFLATED with ZIP_STORED so image data written by converter.py uses stored mode.
419-427:_fix_svg_covermisseshref-only SVG image references (SVG 2 / newer EPUB 3 covers).The guard on line 421 requires
'xlink:href' in content. SVG covers generated by newer toolchains use a plainhrefattribute instead ofxlink:href. Such covers will pass through unfixed.🔧 Proposed fix
- if '<svg' not in content or 'xlink:href' not in content: + if '<svg' not in content or ('xlink:href' not in content and 'href' not in content): return {'content': content, 'fixed': False}And extend the image-source extraction to handle both:
- match = re.search(r'xlink:href=["\']([^"\']+)["\']', content) + match = (re.search(r'xlink:href=["\']([^"\']+)["\']', content) or + re.search(r'(?<!xlink:)href=["\']([^"\']+\.(?:jpg|jpeg|png|gif|webp|bmp))["\']', content))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crosspoint_reader/converter.py` around lines 419 - 427, The _fix_svg_cover function currently only detects SVG image references via 'xlink:href' so newer SVG/EPUB3 covers using plain 'href' are skipped; update the guard in _fix_svg_cover to check for either 'xlink:href' or 'href' (e.g., "'xlink:href' in content or 'href' in content") and adjust the image-source extraction logic used later in _fix_svg_cover to accept and parse both attribute forms when converting the SVG to an HTML <img> (refer to the _fix_svg_cover function name to locate the block and ensure both attribute variants are handled uniformly).crosspoint_reader/config.py (2)
138-160:QSpinBox()instances created withoutselfparent — inconsistent with other widgets.
self.screen_width,self.screen_height, andself.split_overlapare created without passingselfas the parent widget, unlikeself.port,self.chunk_size, etc., which useQSpinBox(self). While Qt re-parents them when added to layouts, it is inconsistent and slightly less safe (e.g., if the widget is never added to a layout).♻️ Proposed fix
- self.screen_width = QSpinBox() + self.screen_width = QSpinBox(self) ... - self.screen_height = QSpinBox() + self.screen_height = QSpinBox(self) ... - self.split_overlap = QSpinBox() + self.split_overlap = QSpinBox(self)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crosspoint_reader/config.py` around lines 138 - 160, The QSpinBox widgets self.screen_width, self.screen_height, and self.split_overlap are created without a parent; change their constructors to pass self as the parent (e.g., QSpinBox(self)) to match other widgets like self.port and self.chunk_size so they are consistently parented even if not immediately added to a layout.
115-120: Ruff ARG005: unusedcheckedparameter in preset button lambda.
QPushButton.clickedpasses aboolargument that is captured ascheckedbut never used. While harmless at runtime, it triggers a linter warning.♻️ Proposed fix
- btn.clicked.connect(lambda checked, v=value: self._set_quality(v)) + btn.clicked.connect(lambda _checked, v=value: self._set_quality(v))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crosspoint_reader/config.py` around lines 115 - 120, The QPushButton.clicked handler currently defines an unused `checked` parameter in the lambda, triggering Ruff ARG005; update the connection in the loop that creates the buttons (where QPushButton is instantiated and btn.clicked.connect(...) is called) to accept only the stored default `v` value instead of `checked` (e.g., use a lambda that captures `v` without a `checked` argument) so the callback calls self._set_quality(v) and the linter warning is resolved; ensure this change is applied where preset buttons are created and appended to self.preset_buttons.crosspoint_reader/driver.py (1)
387-391: Bareexcept: passsilently swallows temp-file cleanup failures — log them.If
os.removefails (e.g., path is a directory, permission error), the failure is completely hidden. At minimum, log it so users can diagnose disk-space issues.♻️ Proposed fix
for temp_path in temp_files: try: os.remove(temp_path) - except Exception: - pass + except Exception as exc: + self._log(f'[CrossPoint] Failed to remove temp file {temp_path}: {exc}')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crosspoint_reader/driver.py` around lines 387 - 391, The cleanup loop currently swallows all errors; change the bare "except: pass" in the temp_files removal loop to catch Exception as e and log the failure including the temp_path and exception details (e.g., use logger.warning or logging.exception) so failures like permission errors or directories are visible; update the block around os.remove(temp_path) to "except Exception as e: logger.warning('failed to remove temp file %s: %s', temp_path, e)" (or use logging if no module logger exists).
🤖 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/config.py`:
- Line 34: The default for automatic conversion is currently set to True and
will silently change behavior for existing users; change
PREFS.defaults['enable_conversion'] from True to False so conversion is opt-in,
update any related tests or docs referencing that default, and ensure any
migration code (e.g., preference-loading logic that reads PREFS.defaults or
initialization paths that assume conversion on) will continue to preserve
existing user preferences rather than relying on the new default.
In `@crosspoint_reader/converter.py`:
- Around line 488-491: The current code uses content.replace('</metadata>', f'
<meta name="cover" content="{cover_id}"/>\n </metadata>') which will replace
every occurrence; update the call in converter.py to limit replacement to the
first occurrence by passing the count=1 (i.e., use replace(..., 1)) so the cover
meta is only inserted once even if malformed OPF contains multiple </metadata>
tags; ensure you reference the same variables (content and cover_id) and keep
the resulting string exactly as before.
- Around line 258-264: The manifest entries currently use p["imgName"]
(basename) which breaks when images live in subdirectories; instead, use the
existing part_path (the full ZIP path for each split part) to compute the href
relative to the OPF file's directory and insert that relative POSIX path into
the manifest_additions; update the loop that builds manifest_additions
(referencing variables manifest_additions, parts, p, part_path, and the
t.replace call) so it derives href = os.path.relpath(part_path, opf_dir) or
equivalent POSIX-relative computation from the OPF directory and uses that href
in the <item ... href="..."/> attribute.
In `@crosspoint_reader/driver.py`:
- Line 12: The import line in crosspoint_reader/driver.py brings in
TemporaryDirectory from calibre.ptempfile but it is unused; remove the unused
symbol by deleting TemporaryDirectory from the import (leave
PersistentTemporaryFile) and run lint/type-check to confirm no remaining
references to TemporaryDirectory in functions like any that use
PersistentTemporaryFile; if you intended to use a temporary directory, replace
usages accordingly, otherwise just remove TemporaryDirectory from the import
list.
- Around line 365-368: The _progress closure defined inside the loop captures
the loop variable i by reference so callbacks later always see the final i; fix
this by binding the current loop index into the closure (e.g., capture i via a
default argument or assign local_index = i and reference that) so _progress uses
the per-iteration value when calling self.report_progress(total, 'Transferring
books to device...'); keep the function name _progress and the call to
self.report_progress unchanged, only change how i is captured.
- Around line 296-312: The temp file created by PersistentTemporaryFile
(temp_path) can leak if converter.convert_epub raises because temp_path is not
tracked or removed; update the conversion block (the method calling
PersistentTemporaryFile and converter.convert_epub) to immediately register
temp_path with your cleanup collection (e.g., append to self.temp_files used in
upload_books) before calling converter.convert_epub, or wrap
converter.convert_epub in its own try/except that deletes/unlinks temp_path (or
closes/removes the PersistentTemporaryFile) on failure and then re-raises or
returns input_path; reference PersistentTemporaryFile, temp_path,
converter.convert_epub and upload_books when making the change.
- Line 336: The loop using for i, (infile, name) in enumerate(zip(files, names))
can silently drop items when files and names differ in length; add an explicit
length check before that loop (compare len(files) and len(names)) and raise a
clear ValueError if they mismatch (e.g., include both lengths in the message)
rather than using zip(..., strict=True) so we remain compatible with Python 3.8;
update the function/method in driver.py that contains the for i, (infile, name)
loop to perform this validation immediately before iterating.
---
Outside diff comments:
In `@crosspoint_reader/driver.py`:
- Around line 36-39: The class-level attribute MUST_READ_METADATA is declared
twice (both set to False) which is redundant and confusing; remove the duplicate
declaration so there is only a single MUST_READ_METADATA = False alongside the
other attributes (MANAGES_DEVICE_PRESENCE and DEVICE_PLUGBOARD_NAME) in the
class/driver definition, ensuring no other logic depends on the second
assignment.
---
Duplicate comments:
In `@crosspoint_reader/converter.py`:
- Around line 169-173: The current loop in converter.py that does "for old, new
in renamed.items()" and calls t = t.replace(old_name, new_name) incorrectly
replaces basenames across the entire XHTML/OPF/CSS/NCX text; update these three
passes (the XHTML pass where variable t is modified, the OPF update pass, and
the CSS/NCX pass) to perform attribute-scoped replacements instead of blind
basename replacements: parse the XML/HTML (e.g., xml/HTML parser used in this
module) and update only the src/href attributes (or other relevant attributes)
on elements (img, source, link, a, etc.) by matching the original full path or
basename in the attribute value and replacing it with the new path from
renamed[old]; this ensures changes target attribute values only and preserves
distinct files with identical basenames in different directories.
---
Nitpick comments:
In `@crosspoint_reader/config.py`:
- Around line 138-160: The QSpinBox widgets self.screen_width,
self.screen_height, and self.split_overlap are created without a parent; change
their constructors to pass self as the parent (e.g., QSpinBox(self)) to match
other widgets like self.port and self.chunk_size so they are consistently
parented even if not immediately added to a layout.
- Around line 115-120: The QPushButton.clicked handler currently defines an
unused `checked` parameter in the lambda, triggering Ruff ARG005; update the
connection in the loop that creates the buttons (where QPushButton is
instantiated and btn.clicked.connect(...) is called) to accept only the stored
default `v` value instead of `checked` (e.g., use a lambda that captures `v`
without a `checked` argument) so the callback calls self._set_quality(v) and the
linter warning is resolved; ensure this change is applied where preset buttons
are created and appended to self.preset_buttons.
In `@crosspoint_reader/converter.py`:
- Around line 126-127: The JPEG outputs are being written with
zipfile.ZIP_DEFLATED which wastes CPU; change the compress_type passed to
zout.writestr for JPEG image writes to zipfile.ZIP_STORED. Update the three
occurrences where zout.writestr(..., parts[0]['data'],
compress_type=zipfile.ZIP_DEFLATED) (and similar calls writing image
bytes/new_path) are used — replace ZIP_DEFLATED with ZIP_STORED so image data
written by converter.py uses stored mode.
- Around line 419-427: The _fix_svg_cover function currently only detects SVG
image references via 'xlink:href' so newer SVG/EPUB3 covers using plain 'href'
are skipped; update the guard in _fix_svg_cover to check for either 'xlink:href'
or 'href' (e.g., "'xlink:href' in content or 'href' in content") and adjust the
image-source extraction logic used later in _fix_svg_cover to accept and parse
both attribute forms when converting the SVG to an HTML <img> (refer to the
_fix_svg_cover function name to locate the block and ensure both attribute
variants are handled uniformly).
In `@crosspoint_reader/driver.py`:
- Around line 387-391: The cleanup loop currently swallows all errors; change
the bare "except: pass" in the temp_files removal loop to catch Exception as e
and log the failure including the temp_path and exception details (e.g., use
logger.warning or logging.exception) so failures like permission errors or
directories are visible; update the block around os.remove(temp_path) to "except
Exception as e: logger.warning('failed to remove temp file %s: %s', temp_path,
e)" (or use logging if no module logger exists).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crosspoint_reader-v0.2.1.zipis excluded by!**/*.zip
📒 Files selected for processing (4)
crosspoint_reader/README.mdcrosspoint_reader/config.pycrosspoint_reader/converter.pycrosspoint_reader/driver.py
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (2)
crosspoint_reader/converter.py (2)
crosspoint_reader/driver.py (2)
_log(53-59)open(96-99)crosspoint_reader/config.py (1)
save(210-225)
crosspoint_reader/config.py (1)
crosspoint_reader/log.py (1)
get_log_text(16-17)
🪛 Ruff (0.15.2)
crosspoint_reader/converter.py
[warning] 51-51: Unused lambda argument: x
(ARG005)
[warning] 352-352: Do not catch blind exception: Exception
(BLE001)
crosspoint_reader/config.py
[warning] 118-118: Unused lambda argument: checked
(ARG005)
[warning] 144-144: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
crosspoint_reader/driver.py
[warning] 24-24: Mutable default value for class attribute
(RUF012)
[warning] 307-307: Consider moving this statement to an else block
(TRY300)
[warning] 309-309: Do not catch blind exception: Exception
(BLE001)
[warning] 336-336: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
[warning] 367-367: Function definition does not bind loop variable i
(B023)
[error] 390-391: try-except-pass detected, consider logging the exception
(S110)
[warning] 390-390: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
crosspoint_reader/README.md (1)
1-110: LGTM — documentation accurately reflects the implementation.The changelog, feature descriptions, and configuration options align with the actual code changes in
converter.py,config.py, anddriver.py.
| ) | ||
|
|
||
| # Create temp file for converted EPUB | ||
| temp_file = PersistentTemporaryFile(suffix='_baseline.epub') | ||
| temp_path = temp_file.name | ||
| temp_file.close() | ||
|
|
||
| # Convert | ||
| self._log(f'[CrossPoint] Converting: {os.path.basename(input_path)}') | ||
| converter.convert_epub(input_path, temp_path) | ||
|
|
||
| return temp_path | ||
|
|
||
| except Exception as exc: | ||
| self._log(f'[CrossPoint] Conversion failed: {exc}') | ||
| # Return original file if conversion fails | ||
| return input_path |
There was a problem hiding this comment.
Temp file leaks if convert_epub raises after PersistentTemporaryFile is created.
PersistentTemporaryFile creates the file on disk at line 299. If converter.convert_epub(...) at line 305 raises an exception, the except block returns input_path — but temp_path was never added to temp_files in upload_books, so it is never cleaned up.
🐛 Proposed fix: track temp_path before conversion, clean up on failure
try:
converter = EpubConverter(...)
temp_file = PersistentTemporaryFile(suffix='_baseline.epub')
temp_path = temp_file.name
temp_file.close()
+ # Track immediately so upload_books cleans it up even on failure
+ # Caller must have passed temp_files list, or handle here:
self._log(f'[CrossPoint] Converting: {os.path.basename(input_path)}')
converter.convert_epub(input_path, temp_path)
-
- return temp_path
+ return temp_path, temp_path # (upload_path, temp_to_clean)
except Exception as exc:
self._log(f'[CrossPoint] Conversion failed: {exc}')
+ try:
+ os.remove(temp_path)
+ except Exception:
+ pass
return input_pathAlternatively, track temp_path in the finally/except path and clean it up before returning the fallback:
def _convert_epub(self, input_path):
if not PREFS['enable_conversion']:
return input_path
+ temp_path = None
try:
converter = EpubConverter(
jpeg_quality=PREFS['jpeg_quality'],
max_width=PREFS['screen_width'],
max_height=PREFS['screen_height'],
enable_split_rotate=PREFS['light_novel_mode'],
overlap=PREFS['split_overlap'] / 100.0,
logger=self._log,
)
temp_file = PersistentTemporaryFile(suffix='_baseline.epub')
temp_path = temp_file.name
temp_file.close()
self._log(f'[CrossPoint] Converting: {os.path.basename(input_path)}')
converter.convert_epub(input_path, temp_path)
return temp_path
except Exception as exc:
self._log(f'[CrossPoint] Conversion failed: {exc}')
+ if temp_path:
+ try:
+ os.remove(temp_path)
+ except Exception:
+ pass
return input_path🧰 Tools
🪛 Ruff (0.15.2)
[warning] 307-307: Consider moving this statement to an else block
(TRY300)
[warning] 309-309: 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 296 - 312, The temp file created by
PersistentTemporaryFile (temp_path) can leak if converter.convert_epub raises
because temp_path is not tracked or removed; update the conversion block (the
method calling PersistentTemporaryFile and converter.convert_epub) to
immediately register temp_path with your cleanup collection (e.g., append to
self.temp_files used in upload_books) before calling converter.convert_epub, or
wrap converter.convert_epub in its own try/except that deletes/unlinks temp_path
(or closes/removes the PersistentTemporaryFile) on failure and then re-raises or
returns input_path; reference PersistentTemporaryFile, temp_path,
converter.convert_epub and upload_books when making the change.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
crosspoint_reader/converter.py (1)
131-133:⚠️ Potential issue | 🟠 MajorBasename-based rewrites can corrupt references when EPUBs contain duplicate filenames in different folders.
Line 131 keys split tracking by basename, and Lines 170-173/229-233/306-309 apply basename-global replacement. That will collide for cases like
Images/cover.pngandassets/cover.png, causing wrong rewrites.Also applies to: 170-173, 229-233, 306-309
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crosspoint_reader/converter.py` around lines 131 - 133, The code keys and rewrites by basename (using orig_name) which breaks when different EPUB paths share the same filename; change split_images to use the full internal EPUB path (use name, or another unique relative path string) as the key and perform replacements based on that full path rather than basename. Update the places that populate and consult split_images (the orig_name assignment and any replacement logic that references orig_name or relies on basename at the locations where split_images is used) so they use the unique path key and match/replace the exact internal-path occurrences to avoid cross-file collisions.
🧹 Nitpick comments (1)
crosspoint_reader/config.py (1)
205-208: Gate “Split Overlap” on Light Novel Mode to reduce config ambiguity.Line 208 keeps overlap editable even when Light Novel Mode is off, which makes the control appear active when it has no effect.
♻️ Suggested refactor
self.enable_conversion.toggled.connect(self._update_conversion_enabled) + self.light_novel_mode.toggled.connect(self._update_conversion_enabled) self._update_conversion_enabled(self.enable_conversion.isChecked()) @@ def _update_conversion_enabled(self, enabled): """Enable/disable conversion options based on master checkbox.""" self.jpeg_quality.setEnabled(enabled) self.quality_label.setEnabled(enabled) for btn in self.preset_buttons: btn.setEnabled(enabled) self.light_novel_mode.setEnabled(enabled) self.screen_width.setEnabled(enabled) self.screen_height.setEnabled(enabled) - self.split_overlap.setEnabled(enabled) + self.split_overlap.setEnabled(enabled and self.light_novel_mode.isChecked())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crosspoint_reader/config.py` around lines 205 - 208, The split overlap control should be disabled whenever Light Novel Mode is off; update the init and signal handling so split_overlap.enabled follows light_novel_mode state instead of the generic enabled flag: use split_overlap.setEnabled(self.light_novel_mode.isChecked()) for initial state and connect self.light_novel_mode.toggled (or stateChanged) to a handler/lambda that calls split_overlap.setEnabled(checked), keeping the other controls (screen_width, screen_height) unchanged; reference widgets: self.light_novel_mode, self.split_overlap and call split_overlap.setEnabled(...) in the toggle handler.
🤖 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/converter.py`:
- Around line 123-127: When writing single-image entries, don't unconditionally
rename to ".jpg" — only change the extension if the conversion actually
succeeded; otherwise preserve the original filename/extension. In the block that
handles len(parts) == 1 (references: parts, renamed, zout.writestr, re.sub),
detect conversion success (e.g. via parts[0].get('converted') or by checking the
returned bytes/format) and use re.sub(..., '.jpg', name) only when conversion
succeeded; otherwise use renamed.get(name, name) (preserving the original
extension). Apply the same guard to the other single-image code path mentioned
around lines handling return of original bytes (the block referenced by the
review at 364-367).
- Line 378: Replace direct usage of Image.Resampling.LANCZOS with a
backward-compatible lookup: define a resample symbol via resample =
getattr(Image, "Resampling", Image).LANCZOS and pass that to img.resize(...)
instead of Image.Resampling.LANCZOS; update both occurrences (the img.resize
calls that currently reference Image.Resampling.LANCZOS) so older Pillow bundled
with Calibre falls back to Image.LANCZOS while newer Pillow uses
Image.Resampling.LANCZOS.
In `@crosspoint_reader/driver.py`:
- Around line 314-317: The temp-file cleanup blocks that call
os.remove(temp_path) currently swallow all exceptions (except Exception: pass);
update both cleanup sites to catch Exception as e and log the failure including
temp_path and the exception (e.g., logger.exception or logger.warning with the
error), so failed deletions are visible for troubleshooting; keep the removal
attempt but replace the silent pass with a clear log message that includes the
unique symbol temp_path and the context (e.g., "failed to remove temp_path in
<function/method name>") and ensure both occurrences (the block around
os.remove(temp_path) at the first location and the similar block at lines
~399-403) are updated.
---
Duplicate comments:
In `@crosspoint_reader/converter.py`:
- Around line 131-133: The code keys and rewrites by basename (using orig_name)
which breaks when different EPUB paths share the same filename; change
split_images to use the full internal EPUB path (use name, or another unique
relative path string) as the key and perform replacements based on that full
path rather than basename. Update the places that populate and consult
split_images (the orig_name assignment and any replacement logic that references
orig_name or relies on basename at the locations where split_images is used) so
they use the unique path key and match/replace the exact internal-path
occurrences to avoid cross-file collisions.
---
Nitpick comments:
In `@crosspoint_reader/config.py`:
- Around line 205-208: The split overlap control should be disabled whenever
Light Novel Mode is off; update the init and signal handling so
split_overlap.enabled follows light_novel_mode state instead of the generic
enabled flag: use split_overlap.setEnabled(self.light_novel_mode.isChecked())
for initial state and connect self.light_novel_mode.toggled (or stateChanged) to
a handler/lambda that calls split_overlap.setEnabled(checked), keeping the other
controls (screen_width, screen_height) unchanged; reference widgets:
self.light_novel_mode, self.split_overlap and call split_overlap.setEnabled(...)
in the toggle handler.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crosspoint_reader-v0.2.2.zipis excluded by!**/*.zip
📒 Files selected for processing (4)
crosspoint_reader/README.mdcrosspoint_reader/config.pycrosspoint_reader/converter.pycrosspoint_reader/driver.py
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (3)
crosspoint_reader/config.py (1)
crosspoint_reader/log.py (1)
get_log_text(16-17)
crosspoint_reader/converter.py (2)
crosspoint_reader/driver.py (2)
_log(53-59)open(96-99)crosspoint_reader/config.py (1)
save(210-225)
crosspoint_reader/driver.py (3)
crosspoint_reader/converter.py (2)
EpubConverter(25-517)convert_epub(62-330)crosspoint_reader/log.py (1)
add_log(8-13)crosspoint_reader/ws_client.py (2)
_log(22-28)_log(171-177)
🪛 Ruff (0.15.2)
crosspoint_reader/config.py
[warning] 118-118: Unused lambda argument: checked
(ARG005)
[warning] 144-144: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
crosspoint_reader/converter.py
[warning] 51-51: Unused lambda argument: x
(ARG005)
[warning] 364-364: Do not catch blind exception: Exception
(BLE001)
crosspoint_reader/driver.py
[warning] 24-24: Mutable default value for class attribute
(RUF012)
[warning] 308-308: Consider moving this statement to an else block
(TRY300)
[warning] 310-310: Do not catch blind exception: Exception
(BLE001)
[error] 316-317: try-except-pass detected, consider logging the exception
(S110)
[warning] 316-316: Do not catch blind exception: Exception
(BLE001)
[warning] 347-347: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
[error] 402-403: try-except-pass detected, consider logging the exception
(S110)
[warning] 402-402: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (3)
crosspoint_reader/README.md (1)
95-103: README/changelog update is clear and consistent with the code changes.The v0.2.2 notes are useful and match the current implementation direction.
crosspoint_reader/config.py (1)
47-75: Settings UI restructuring and persistence wiring look solid.The grouped layout and save-path for new conversion preferences are implemented cleanly.
Also applies to: 76-123, 173-176, 210-225
crosspoint_reader/driver.py (1)
331-334: Upload-flow hardening here is strong.The length validation, progress closure binding, and finally-based temp cleanup are all good reliability improvements.
Also applies to: 344-359, 377-404
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
crosspoint_reader/converter.py (2)
177-181:⚠️ Potential issue | 🟠 MajorBasename-only replacements can still corrupt references across directories.
Using
os.path.basename(...)+ globalstr.replace(...)can rewrite unintended occurrences when different folders contain the same filename. This can still break manifests/content in multi-directory EPUBs.Also applies to: 238-242, 317-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crosspoint_reader/converter.py` around lines 177 - 181, The current loop in converter.py that does for old, new in renamed.items(): old_name = os.path.basename(old); new_name = os.path.basename(new); t = t.replace(old_name, new_name) is unsafe because basename-only global str.replace can alter same filenames in different directories; instead perform path-aware replacements: parse each path-like token in the string (or split on path separators), detect tokens whose basename equals os.path.basename(old) and replace only that final path segment with os.path.basename(new), or rebuild full paths using os.path.split/posixpath.split so you only change the filename portion of actual paths; apply the same fix to the other occurrences that use the same pattern (the blocks that set old_name/new_name and call t.replace).
133-133:⚠️ Potential issue | 🟡 Minor
images_convertedincrements even when conversion fails.In the single-image path, Line 133 increments regardless of
conversion_success, so stats and logs over-report converted images.📊 Small correctness fix
- self.stats['images_converted'] += 1 + if conversion_success: + self.stats['images_converted'] += 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crosspoint_reader/converter.py` at line 133, In the single-image conversion path in converter.py, the code currently increments self.stats['images_converted'] unconditionally; change it so the increment (and any "converted" log) only occurs when conversion_success is truthy. Locate the block that sets conversion_success (and the line with self.stats['images_converted'] += 1) and wrap the increment and success-specific logging behind an if conversion_success: check (leave failure logging/stats unchanged or increment a failure counter if present).
🧹 Nitpick comments (1)
crosspoint_reader/config.py (1)
118-118: Use_checkedfor the Qt signal arg to satisfy lint and intent.
clickedpasses a boolean; renaming the unused parameter avoids the ARG005 warning while keeping the signal signature explicit.♻️ Small cleanup
- btn.clicked.connect(lambda checked, v=value: self._set_quality(v)) + btn.clicked.connect(lambda _checked, v=value: self._set_quality(v))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crosspoint_reader/config.py` at line 118, The lambda connected to the Qt signal should name the unused boolean argument as _checked to satisfy lint and signal intent; update the connection in the code that calls btn.clicked.connect(lambda checked, v=value: self._set_quality(v)) to use lambda _checked, v=value: self._set_quality(v) (referencing the btn.clicked.connect call and the _set_quality method) so the signal signature stays explicit and ARG005 is avoided.
🤖 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/converter.py`:
- Around line 99-103: The current loop pre-populates the renamed mapping for all
image filenames (using renamed, zin.namelist(), new_name and the regex that
replaces extensions), which causes references to be rewritten to .jpg even if
conversion failed; instead only add an entry to renamed when an image conversion
actually succeeds (i.e., move the renamed[name] = new_name assignment into the
success path of the image conversion routine or maintain a
successful_conversions set and use that when rewriting). Update all similar
blocks (the ones around lines 123-131, 177-181, 238-242, 317-320) so they
conditionally populate renamed only on successful conversion outcomes.
In `@crosspoint_reader/README.md`:
- Around line 50-54: Update the README usage step that currently reads "Send
books to device as usual - images will be automatically converted" to clarify
conversion is opt-in: change the line to say images are converted only if image
conversion is enabled in the plugin settings, and add a brief note directing
users to enable it via Preferences → Plugins → CrossPoint Reader → Customize
plugin; reference the exact sentence "Send books to device as usual - images
will be automatically converted" to locate and replace.
---
Duplicate comments:
In `@crosspoint_reader/converter.py`:
- Around line 177-181: The current loop in converter.py that does for old, new
in renamed.items(): old_name = os.path.basename(old); new_name =
os.path.basename(new); t = t.replace(old_name, new_name) is unsafe because
basename-only global str.replace can alter same filenames in different
directories; instead perform path-aware replacements: parse each path-like token
in the string (or split on path separators), detect tokens whose basename equals
os.path.basename(old) and replace only that final path segment with
os.path.basename(new), or rebuild full paths using os.path.split/posixpath.split
so you only change the filename portion of actual paths; apply the same fix to
the other occurrences that use the same pattern (the blocks that set
old_name/new_name and call t.replace).
- Line 133: In the single-image conversion path in converter.py, the code
currently increments self.stats['images_converted'] unconditionally; change it
so the increment (and any "converted" log) only occurs when conversion_success
is truthy. Locate the block that sets conversion_success (and the line with
self.stats['images_converted'] += 1) and wrap the increment and success-specific
logging behind an if conversion_success: check (leave failure logging/stats
unchanged or increment a failure counter if present).
---
Nitpick comments:
In `@crosspoint_reader/config.py`:
- Line 118: The lambda connected to the Qt signal should name the unused boolean
argument as _checked to satisfy lint and signal intent; update the connection in
the code that calls btn.clicked.connect(lambda checked, v=value:
self._set_quality(v)) to use lambda _checked, v=value: self._set_quality(v)
(referencing the btn.clicked.connect call and the _set_quality method) so the
signal signature stays explicit and ARG005 is avoided.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crosspoint_reader-v0.2.3.zipis excluded by!**/*.zip
📒 Files selected for processing (4)
crosspoint_reader/README.mdcrosspoint_reader/config.pycrosspoint_reader/converter.pycrosspoint_reader/driver.py
📜 Review details
🧰 Additional context used
🧬 Code graph analysis (2)
crosspoint_reader/driver.py (3)
crosspoint_reader/config.py (1)
CrossPointConfigWidget(42-242)crosspoint_reader/converter.py (2)
EpubConverter(25-529)convert_epub(62-341)crosspoint_reader/ws_client.py (3)
_log(22-28)_log(171-177)upload_file(254-294)
crosspoint_reader/converter.py (2)
crosspoint_reader/driver.py (2)
_log(53-59)open(96-99)crosspoint_reader/config.py (1)
save(221-236)
🪛 Ruff (0.15.2)
crosspoint_reader/driver.py
[warning] 24-24: Mutable default value for class attribute
(RUF012)
[warning] 308-308: Consider moving this statement to an else block
(TRY300)
[warning] 310-310: Do not catch blind exception: Exception
(BLE001)
[warning] 316-316: Do not catch blind exception: Exception
(BLE001)
[warning] 347-347: zip() without an explicit strict= parameter
Add explicit value for parameter strict=
(B905)
[warning] 402-402: Do not catch blind exception: Exception
(BLE001)
crosspoint_reader/config.py
[warning] 118-118: Unused lambda argument: checked
(ARG005)
[warning] 144-144: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
crosspoint_reader/converter.py
[warning] 51-51: Unused lambda argument: x
(ARG005)
[warning] 183-183: Loop control variable orig_path not used within loop body
Rename unused orig_path to _orig_path
(B007)
[warning] 259-259: Loop control variable orig_path not used within loop body
Rename unused orig_path to _orig_path
(B007)
[warning] 376-376: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (2)
crosspoint_reader/config.py (1)
173-219: Nice state-gating for dependent conversion controls.The enable/disable flow between
enable_conversion,light_novel_mode, andsplit_overlapis clean and prevents invalid UI states.crosspoint_reader/driver.py (1)
331-404: Good hardening in upload flow.Length validation, per-file conversion handling, and
finallytemp-file cleanup materially improve reliability of the upload pipeline.
| for name in zin.namelist(): | ||
| low = name.lower() | ||
| if re.match(r'.*\.(png|gif|webp|bmp|jpeg)$', low): | ||
| new_name = re.sub(r'\.(png|gif|webp|bmp|jpeg)$', '.jpg', name, flags=re.IGNORECASE) | ||
| renamed[name] = new_name |
There was a problem hiding this comment.
Only apply rename mappings for images that actually converted.
Right now renamed is built up front, so if an image conversion fails and you keep the original extension, later XHTML/OPF/CSS passes can still rewrite references to .jpg, producing broken EPUB links.
🐛 Suggested direction
- renamed = {} # old_path -> new_path
+ renamed = {} # old_path -> new_path (planned)
+ applied_renames = {} # old_path -> new_path (conversion actually succeeded)
@@
- if conversion_success:
+ if conversion_success:
# Conversion succeeded - use .jpg extension
new_path = renamed.get(name, re.sub(r'\.[^.]+$', '.jpg', name))
+ if new_path != name:
+ applied_renames[name] = new_path
else:
# Conversion failed - preserve original extension
new_path = name
@@
- for old, new in renamed.items():
+ for old, new in applied_renames.items():
@@
- for old, new in renamed.items():
+ for old, new in applied_renames.items():
@@
- for old, new in renamed.items():
+ for old, new in applied_renames.items():Also applies to: 123-131, 177-181, 238-242, 317-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crosspoint_reader/converter.py` around lines 99 - 103, The current loop
pre-populates the renamed mapping for all image filenames (using renamed,
zin.namelist(), new_name and the regex that replaces extensions), which causes
references to be rewritten to .jpg even if conversion failed; instead only add
an entry to renamed when an image conversion actually succeeds (i.e., move the
renamed[name] = new_name assignment into the success path of the image
conversion routine or maintain a successful_conversions set and use that when
rewriting). Update all similar blocks (the ones around lines 123-131, 177-181,
238-242, 317-320) so they conditionally populate renamed only on successful
conversion outcomes.
Clarified the condition for image conversion in the README.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crosspoint_reader/README.md (1)
3-3:⚠️ Potential issue | 🟡 MinorClarify conversion is opt-in in feature description too.
These lines still read as always-on behavior. Please qualify them with “when enabled” to stay consistent with the default-off behavior documented later.
Also applies to: 16-16
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crosspoint_reader/README.md` at line 3, Update the feature description lines in README.md (both the header line and the duplicate at line 16) to clarify that EPUB image conversion is not always-on by qualifying the phrase with "when enabled" (e.g., change "with built-in EPUB image conversion for optimal e-reader compatibility" to "with built-in EPUB image conversion when enabled for optimal e-reader compatibility") so the summary matches the default-off behavior described elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crosspoint_reader/README.md`:
- Line 3: Update the feature description lines in README.md (both the header
line and the duplicate at line 16) to clarify that EPUB image conversion is not
always-on by qualifying the phrase with "when enabled" (e.g., change "with
built-in EPUB image conversion for optimal e-reader compatibility" to "with
built-in EPUB image conversion when enabled for optimal e-reader compatibility")
so the summary matches the default-off behavior described elsewhere.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crosspoint_reader/README.md
📜 Review details
🔇 Additional comments (1)
crosspoint_reader/README.md (1)
53-53: Nice fix on usage wording.This now correctly communicates that conversion only happens when the setting is enabled.
select.select() doesn't work with sockets on Windows. Added platform detection to use short timeout on Windows while keeping select.select() for Unix/Linux/Mac where it works correctly.
- Fix ALL SVG-wrapped images (not just covers) - Increase WebSocket timeout: 10s -> 60s for large file transfers - Skip drain_messages on Windows to avoid socket timeout interference
Fix/svg images
2 participants
Added the functionality to automatically convert all the images into baseline jpg compatible with the x4.
If I should make any other changes to the branch structure or files I'm sorry. It's my first time using github to contribute something.
