Feature/#19 list downloads#98
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds Wattpad list/archive/library download support: new ChangesList & Collection Download Flow
Sequence DiagramsequenceDiagram
participant User as Frontend User
participant Frontend as Frontend UI
participant Endpoint as Download Endpoint
participant Wattpad as Wattpad API
participant Generator as Generator
participant Storage as In-Memory ZIP
User->>Frontend: choose mode (list/archive/library) + submit (optional cookies)
Frontend->>Endpoint: POST /download (mode, id?, cookies?)
Endpoint->>Wattpad: fetch_list / fetch_archive / fetch_library / fetch_username
Wattpad-->>Endpoint: list metadata / stories[]
loop per story
Endpoint->>Generator: download_story(story, download_images?, cookies?)
Generator->>Wattpad: fetch cover & parts ZIP
Wattpad-->>Generator: parts + images
Generator->>Generator: build parts tree, generate EPUB/PDF
Generator-->>Endpoint: BytesIO (story file) or error
Endpoint->>Storage: write story file or _FAILURE.txt into ZIP
end
Endpoint->>Frontend: stream ZIP or single file (BytesIO)
Frontend-->>User: file download
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (4)
src/api/src/main.py (2)
97-102: Tightencookiesannotation:Optional[dict], notdict.
cookies: dict = Nonesays "dict" but the default isNone, and both callers passNonein unauthenticated paths. UseOptional[dict] = Noneto match the rest of the module (fetch_story,fetch_story_content_zip,fetch_list).🔧 Proposed fix
-async def download_story( - metadata: Story, - download_images: bool = False, - format: DownloadFormat = DownloadFormat.epub, - cookies: dict = None, -) -> BytesIO: +async def download_story( + metadata: Story, + download_images: bool = False, + format: DownloadFormat = DownloadFormat.epub, + cookies: Optional[dict] = None, +) -> BytesIO: ... -async def download_many_stories( - stories: list[Story], - download_images: bool = False, - format: DownloadFormat = DownloadFormat.epub, - cookies: dict = None, -) -> BytesIO: +async def download_many_stories( + stories: list[Story], + download_images: bool = False, + format: DownloadFormat = DownloadFormat.epub, + cookies: Optional[dict] = None, +) -> BytesIO:Also applies to: 171-176
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/src/main.py` around lines 97 - 102, The parameter annotation for cookies should be Optional[dict] = None rather than dict = None; update the function signature of download_story to use cookies: Optional[dict] = None and make the same change for the other functions around lines 171–176 that currently use cookies: dict = None (e.g., the fetch_* helpers noted in the review). Also ensure Optional is imported from typing (add from typing import Optional if missing) so the annotations are valid and consistent with the rest of the module.
117-119:ZipFileis opened but never closed.
archive = ZipFile(story_zip, "r")keeps the zip file handle alive until GC runs. It's in-memory here so leakage is bounded, but awithblock is cheap and matches the pattern used indownload_many_stories.♻️ Proposed fix
- # Fetch parts archive - story_zip = await fetch_story_content_zip(metadata["id"], cookies) - archive = ZipFile(story_zip, "r") - - # Transform part metadata into an easily-indexable dictionary - part_id_title_dictionary = { - str(part["id"]): part["title"] for part in metadata["parts"] - } - - part_trees: list[BeautifulSoup] = [] - - for id in archive.namelist(): - ... - part_trees.append( - clean_tree( - part_id_title_dictionary[id], - id, - archive.read(id).decode("utf-8"), - ) - ) + # Fetch parts archive + story_zip = await fetch_story_content_zip(metadata["id"], cookies) + part_id_title_dictionary = { + str(part["id"]): part["title"] for part in metadata["parts"] + } + part_trees: list[BeautifulSoup] = [] + with ZipFile(story_zip, "r") as archive: + for id in archive.namelist(): + if id not in part_id_title_dictionary: + continue + part_trees.append( + clean_tree( + part_id_title_dictionary[id], + id, + archive.read(id).decode("utf-8"), + ) + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/src/main.py` around lines 117 - 119, The ZipFile opened in the block that calls fetch_story_content_zip (where you assign archive = ZipFile(story_zip, "r")) is not closed; replace that direct instantiation with a context manager (use with ZipFile(story_zip, "r") as archive:) so the archive is closed automatically after use, mirroring the pattern used in download_many_stories and ensuring any operations that currently reference archive are performed inside the with block.src/api/src/create_book/models.py (1)
45-47: Optional: consider renamingListto avoid shadowingtyping.List.The new
ListTypedDict is syntactically fine here, but naming itListcreates a namespace clash withtyping.List(and with common mental models of the built-in). Increate_book.pythe annotation-> List(Story)was introduced at L151/L172 of the new code — very likely an attempt to "parameterize" this TypedDict likeList[Story], which TypedDicts do not support. A less ambiguous name (e.g.StoryList,ListCollection) would make those downstream annotations harder to get wrong.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/src/create_book/models.py` around lines 45 - 47, The TypedDict class named List shadows typing.List and leads to incorrect uses like the `-> List(Story)` annotation; rename the TypedDict (e.g., StoryList or ListCollection) and update all references/usages (including the annotations in create_book.py that currently use `List(Story)`) to either the new TypedDict name or to the proper generic type `list[Story]` where a parameterized sequence is intended; ensure imports and any type hints that referenced `List` (and the symbols `Story` and the TypedDict class) are updated accordingly.src/api/src/create_book/create_book.py (1)
150-189: Deduplicatefetch_archiveandfetch_library.Both functions are byte-for-byte identical aside from the endpoint path segment (
archivevslibrary). Extracting the common paginator keeps behavior tweaks (caching, error handling, pagination key changes) in a single place.♻️ Suggested shape
async def _fetch_user_collection( username: str, cookies: dict, collection: str ) -> list[Story]: with start_action(action_type=f"api_fetch_{collection}"): async with CachedSession(headers=headers, cookies=cookies, cache=None) as session: stories: list[Story] = [] next_url = ( f"https://www.wattpad.com/api/v3/users/{username}/{collection}" "?fields=stories(tags,id,title,createDate,modifyDate,language(name)," "description,completed,mature,url,isPaywalled," "user(username,avatar,description),parts(id,title),cover,copyright)" "&limit=30" ) while next_url: async with session.get(next_url) as response: response.raise_for_status() body = await response.json() next_url = body.get("nextUrl") stories.extend(body.get("stories") or []) return stories `@backoff.on_exception`(backoff.expo, ClientResponseError, max_time=15) async def fetch_archive(username: str, cookies: dict) -> list[Story]: return await _fetch_user_collection(username, cookies, "archive") `@backoff.on_exception`(backoff.expo, ClientResponseError, max_time=15) async def fetch_library(username: str, cookies: dict) -> list[Story]: return await _fetch_user_collection(username, cookies, "library")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/src/create_book/create_book.py` around lines 150 - 189, Both fetch_archive and fetch_library are identical except for the endpoint segment; extract the shared paginator into a helper like _fetch_user_collection(username, cookies, collection) that uses start_action(action_type=f"api_fetch_{collection}"), opens CachedSession(headers=headers, cookies=cookies), builds the next_url for f"https://www.wattpad.com/api/v3/users/{username}/{collection}" with the same fields and &limit=30, loops while next_url, calls session.get(next_url), response.raise_for_status(), body = await response.json(), sets next_url = body.get("nextUrl") and extends stories with body.get("stories") or [], then returns stories; keep the `@backoff.on_exception` decorator on fetch_archive and fetch_library and have them simply return await _fetch_user_collection(username, cookies, "archive") and "library" respectively.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/src/create_book/create_book.py`:
- Line 151: The return type annotations using the non-generic TypedDict name
"List" (e.g., in the function fetch_archive) are invalid; replace occurrences of
List(Story) with the proper generic builtin list type annotation list[Story]
(and similarly update any other functions around the same area using List(...),
e.g., the one reported at line ~172), then remove the unused List import from
the top of the file if it is no longer referenced; reference the fetch_archive
function name and the other reported annotated function(s) so you can locate and
update their signatures and imports accordingly.
- Around line 192-206: fetch_username is calling a fragile frontend data route
("/list?_data=routes/_currentReads.list") which can change; update
fetch_username to use a stable authenticated API (e.g., call a v3 endpoint like
"/api/v3/users?fields=username" or "/api/v3/internal/whoami" using the same
CachedSession and cookies) and parse the response for the username there, and
add a defensive fallback: use body.get("username") and raise/return a clear,
descriptive error if missing (so callers of fetch_username see an explicit
failure rather than a KeyError). If no stable API is available, implement an
alternative decode path from the token cookie as a fallback and log a clear
error when both methods fail.
- Around line 158-168: The pagination loop in the function that fetches Wattpad
stories uses body["stories"] which can KeyError on missing/empty pages; update
the loop in the async request block to use body.get("stories", []) and replace
the manual append loop with stories.extend(body.get("stories", [])) to simplify
and make it safe; leave the pagination handling (nextUrl = body.get("nextUrl"))
as-is but verify you update the block around the async with session.get(...) and
the variable names (stories, nextUrl) accordingly.
- Around line 150-168: In fetch_archive (and similarly in fetch_library and
fetch_username) update the CachedSession instantiation to disable shared caching
for authenticated requests by passing cache=None if cookies else cache; also fix
the return type annotations from the invalid List(Story) to Python typing
list[Story] (or from typing import List and use List[Story] consistently) so
function signatures are syntactically correct—locate the CachedSession(...)
calls and the type hints on fetch_archive/fetch_library/fetch_username to apply
these two changes.
In `@src/api/src/main.py`:
- Line 306: Fix the typo in the user-facing error strings: replace "Login
credenials required for archive downloads" with "Login credentials required for
archive downloads" (and the analogous string in the library branch) wherever the
literal "Login credenials" appears in main.py (update both occurrences
referenced in the diff/content attributes so the user-facing messages use
"credentials").
- Line 189: The filename assembly for file_name is adding an extra underscore
when download_images is false; update the construction so the "_images" segment
(including its leading underscore) is only appended when download_images is
true. Locate the file_name assignment that uses slugify(story['title']),
story['id'], the conditional {'images' if download_images else ''} and
DownloadFormat; change it to conditionally append f"_images" (or nothing) based
on download_images so you don't end up with a trailing underscore (match the
conditional style used in handle_download).
- Line 348: Replace the long inline f-string that builds the Content-Disposition
filename with a small helper function (e.g., build_download_filename(metadata,
mode, id_download, username, download_id, download_images, format, extension,
slugify)) that explicitly handles each case (DownloadMode.story/part:
"<slug>_<id>[_images].<epub|pdf>", DownloadMode.list:
"<name-slug>_<id>[_images]_<format>.zip", DownloadMode.archive/library:
"<username>_<archive|library>[_images]_<format>.zip"), use == for string
comparisons (not is) when checking extension == "zip", and return the final
filename to be used in the Content-Disposition header; update the call site
where slugify(...), metadata, mode, etc. were used to call this helper.
- Around line 171-194: download_many_stories currently awaits download_story for
each story without error handling so one failure aborts the whole ZIP; wrap the
per-story call inside a try/except around await download_story(story,
download_images, format, cookies) (and subsequent archive.writestr) to catch
exceptions, log the error via start_action/process logger with the story
id/title, append a short error entry to a local errors list, and continue to the
next story; after the loop, if errors is non-empty, write a human-readable
errors.txt into the ZipFile (e.g. using archive.writestr("errors.txt",
"\n".join(errors))) so the consumer sees which stories failed; ensure
output_buffer.seek(0) and return behavior remains the same (optionally decide to
raise if zero files were added).
In `@src/frontend/src/routes/`+page.svelte:
- Around line 238-270: These inputs are radio inputs but styled and handled like
checkboxes; fix by keeping radio semantics: add a fourth radio option with value
"" and label "Just a single story" to represent the default, change the input
classes from "checkbox checkbox-warning" to "radio radio-warning" for the three
existing options and the new default, and remove the "toggle" behavior that
clears specialSelector when clicking the already-selected option—update the
handlers so inputs simply set specialSelector (or use Svelte bind:group to bind
to the specialSelector variable) and remove logic in the toggle() function that
allows toggling off.
- Around line 92-98: The list URL branch currently leaves a trailing slug in the
ID because it only does input.split("?", 1)[0].split("/list/")[1]; update the
branch handling input.includes("/list/") to mirror the other URL branches by
further splitting on "-" and taking the first segment (e.g., use
.split("?",1)[0].split("/list/")[1].split("-",1)[0]) before calling
setInputAsValid; apply the same change for the other occurrence that parses list
URLs so setInputAsValid receives the numeric ID without any slug.
- Line 13: The JSDoc union type comment for the page type is missing a space
before the final empty-string member; update the comment string in the JSDoc
(the /** `@type` {...} */ line that currently reads {"story" | "part" | "list" |
"library" | "archive"| ""}) to include a space before the | for the empty string
so it becomes {"story" | "part" | "list" | "library" | "archive" | ""}.
---
Nitpick comments:
In `@src/api/src/create_book/create_book.py`:
- Around line 150-189: Both fetch_archive and fetch_library are identical except
for the endpoint segment; extract the shared paginator into a helper like
_fetch_user_collection(username, cookies, collection) that uses
start_action(action_type=f"api_fetch_{collection}"), opens
CachedSession(headers=headers, cookies=cookies), builds the next_url for
f"https://www.wattpad.com/api/v3/users/{username}/{collection}" with the same
fields and &limit=30, loops while next_url, calls session.get(next_url),
response.raise_for_status(), body = await response.json(), sets next_url =
body.get("nextUrl") and extends stories with body.get("stories") or [], then
returns stories; keep the `@backoff.on_exception` decorator on fetch_archive and
fetch_library and have them simply return await _fetch_user_collection(username,
cookies, "archive") and "library" respectively.
In `@src/api/src/create_book/models.py`:
- Around line 45-47: The TypedDict class named List shadows typing.List and
leads to incorrect uses like the `-> List(Story)` annotation; rename the
TypedDict (e.g., StoryList or ListCollection) and update all references/usages
(including the annotations in create_book.py that currently use `List(Story)`)
to either the new TypedDict name or to the proper generic type `list[Story]`
where a parameterized sequence is intended; ensure imports and any type hints
that referenced `List` (and the symbols `Story` and the TypedDict class) are
updated accordingly.
In `@src/api/src/main.py`:
- Around line 97-102: The parameter annotation for cookies should be
Optional[dict] = None rather than dict = None; update the function signature of
download_story to use cookies: Optional[dict] = None and make the same change
for the other functions around lines 171–176 that currently use cookies: dict =
None (e.g., the fetch_* helpers noted in the review). Also ensure Optional is
imported from typing (add from typing import Optional if missing) so the
annotations are valid and consistent with the rest of the module.
- Around line 117-119: The ZipFile opened in the block that calls
fetch_story_content_zip (where you assign archive = ZipFile(story_zip, "r")) is
not closed; replace that direct instantiation with a context manager (use with
ZipFile(story_zip, "r") as archive:) so the archive is closed automatically
after use, mirroring the pattern used in download_many_stories and ensuring any
operations that currently reference archive are performed inside the with block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cd523920-6c55-4044-8d2c-9055497f4983
📒 Files selected for processing (5)
src/api/src/create_book/__init__.pysrc/api/src/create_book/create_book.pysrc/api/src/create_book/models.pysrc/api/src/main.pysrc/frontend/src/routes/+page.svelte
| media_type=media_type, | ||
| headers={ | ||
| "Content-Disposition": f'attachment; filename="{slugify(metadata["title"])}_{story_id}{"_images" if download_images else ""}.{format.value}"', # Thanks https://stackoverflow.com/a/72729058 | ||
| "Content-Disposition": f'attachment; filename="{slugify(metadata["name" if mode==DownloadMode.list else "title"]) if id_download else (username+'_'+("archive" if mode is DownloadMode.archive else "library"))}{'_'+str(download_id) if id_download else ""}{"_images" if download_images else ""}{'_'+format.value if extension is "zip" else ""}.{extension}"', # Thanks https://stackoverflow.com/a/72729058 |
There was a problem hiding this comment.
Filename expression: is "zip" is wrong, and the whole line is hard to read/maintain.
Ruff F632 is correctly flagging extension is "zip" — is compares identity, which happens to work here only because CPython interns short literals; this is not guaranteed behavior and should be ==. More broadly, this one-line f"" has three nested conditionals, an inline username + '_' + (...), and mixes escape conventions, which makes the supported filenames ambiguous:
DownloadMode.story/part:<slug>_<id>[_images].<epub|pdf>DownloadMode.list:<name-slug>_<id>[_images]_<format>.zipDownloadMode.archive/library:<username>_<archive|library>[_images]_<format>.zip
Please extract this into a small helper — it would also make the archive-vs-library branch explicit and remove the need for the is comparison.
🧹 Proposed refactor
- file_size = output_buffer.getbuffer().nbytes
+ file_size = output_buffer.getbuffer().nbytes
+
+ if id_download:
+ name_part = slugify(
+ metadata["name" if mode == DownloadMode.list else "title"]
+ )
+ id_part = f"_{download_id}"
+ else:
+ collection = "archive" if mode == DownloadMode.archive else "library"
+ name_part = f"{username}_{collection}"
+ id_part = ""
+
+ images_part = "_images" if download_images else ""
+ format_part = f"_{format.value}" if extension == "zip" else ""
+ filename = f"{name_part}{id_part}{images_part}{format_part}.{extension}"
return StreamingResponse(
iterfile(file_size),
media_type=media_type,
headers={
- "Content-Disposition": f'attachment; filename="{slugify(metadata["name" if mode==DownloadMode.list else "title"]) if id_download else (username+'_'+("archive" if mode is DownloadMode.archive else "library"))}{'_'+str(download_id) if id_download else ""}{"_images" if download_images else ""}{'_'+format.value if extension is "zip" else ""}.{extension}"', # Thanks https://stackoverflow.com/a/72729058
+ "Content-Disposition": f'attachment; filename="{filename}"',
"Content-Length": str(file_size),
},
)🧰 Tools
🪛 Ruff (0.15.11)
[error] 348-348: Use == to compare constant literals
Replace is with ==
(F632)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/src/main.py` at line 348, Replace the long inline f-string that
builds the Content-Disposition filename with a small helper function (e.g.,
build_download_filename(metadata, mode, id_download, username, download_id,
download_images, format, extension, slugify)) that explicitly handles each case
(DownloadMode.story/part: "<slug>_<id>[_images].<epub|pdf>", DownloadMode.list:
"<name-slug>_<id>[_images]_<format>.zip", DownloadMode.archive/library:
"<username>_<archive|library>[_images]_<format>.zip"), use == for string
comparisons (not is) when checking extension == "zip", and return the final
filename to be used in the Content-Disposition header; update the call site
where slugify(...), metadata, mode, etc. were used to call this helper.
| <label class="label cursor-pointer text-gray-800"> | ||
| <span class="label-text">This is a Paid Story, and I've purchased it</span> | ||
| <span class="label-text">This contains paid content, and I've purchased it</span> | ||
| <input | ||
| type="checkbox" | ||
| type="radio" | ||
| name="specialSelector" | ||
| class="checkbox-warning checkbox shadow-md" | ||
| value="paid" | ||
| checked={specialSelector === "paid"} | ||
| onclick={() => toggle("paid")} | ||
| /> | ||
| </label> | ||
| <label class="label cursor-pointer text-gray-800"> | ||
| <span class="label-text">I want to download my entire library</span> | ||
| <input | ||
| type="radio" | ||
| name="specialSelector" | ||
| class="checkbox-warning checkbox shadow-md" | ||
| value="library" | ||
| checked={specialSelector === "library"} | ||
| onclick={() => toggle("library")} | ||
| /> | ||
| </label> | ||
| <label class="label cursor-pointer text-gray-800"> | ||
| <span class="label-text">I want to download my entire archive</span> | ||
| <input | ||
| type="radio" | ||
| name="specialSelector" | ||
| class="checkbox-warning checkbox shadow-md" | ||
| bind:checked={isPaidStory} | ||
| value="archive" | ||
| checked={specialSelector === "archive"} | ||
| onclick={() => toggle("archive")} | ||
| /> | ||
| </label> |
There was a problem hiding this comment.
type="radio" inputs styled as checkboxes and manually toggled — inconsistent UX.
These three inputs are radios (they share name="specialSelector", so the browser will only allow one active at a time), but:
- They're styled with daisyUI's
checkbox-warning checkboxclasses, which render them visually as checkboxes. toggle()manually clearsspecialSelectorwhen the already-selected option is clicked again, which radios otherwise can't do. The user has no keyboard-only way to unselect ("back to default") — tabbing to a radio and pressing Space won't toggle it off.
Pick one of the two semantics:
- If users need an unselected state → switch to
type="checkbox"(and enforce mutual exclusion intoggle()). - If they are mutually exclusive and must be in one state → add a 4th radio for the default (
value="", "Just a single story"), remove the toggle-off behavior, and swap the classes toradio radio-warning.
Also applies to: 116-123
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontend/src/routes/`+page.svelte around lines 238 - 270, These inputs
are radio inputs but styled and handled like checkboxes; fix by keeping radio
semantics: add a fourth radio option with value "" and label "Just a single
story" to represent the default, change the input classes from "checkbox
checkbox-warning" to "radio radio-warning" for the three existing options and
the new default, and remove the "toggle" behavior that clears specialSelector
when clicking the already-selected option—update the handlers so inputs simply
set specialSelector (or use Svelte bind:group to bind to the specialSelector
variable) and remove logic in the toggle() function that allows toggling off.
cfe4f8a to
c144a81
Compare
Closes #19
Summary by CodeRabbit