feat: share an album over the local network - #1469
Conversation
A naive gethostbyname picks the VMware adapter on a dev machine, so candidates are ranked by default route and the caller keeps an override.
Deliberately not a table: shares die with the process, so no token is ever written to disk and expiry needs no cleanup job.
An image is resolved to a path only after it is confirmed to belong to the shared album, so a token cannot read the whole images table.
The socket is bound before uvicorn sees it, because uvicorn raises SystemExit on a bind failure and would take the whole backend down.
Create, list and revoke stay on localhost. Schema names are share-specific so the generated OpenAPI does not rename the album models.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis change adds local-network album sharing. It adds management APIs, network discovery, an embedded share server, in-memory tokens, protected media routes, a responsive album viewer, lifecycle cleanup, tests, and OpenAPI documentation. ChangesLocal album sharing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ShareRoutes
participant ShareServer
participant ShareRegistry
participant ShareViewer
Client->>ShareRoutes: Create album share
ShareRoutes->>ShareServer: Start embedded listener
ShareRoutes->>ShareRegistry: Store album token
ShareRoutes-->>Client: Return share metadata
Client->>ShareViewer: Open token URL
ShareViewer->>ShareRegistry: Resolve token
ShareViewer-->>Client: Render album page
Client->>ShareViewer: Request thumbnail or photo
ShareViewer->>ShareRegistry: Validate token
ShareViewer-->>Client: Serve album media
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 12
🧹 Nitpick comments (7)
backend/app/share/registry.py (3)
15-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeprecated
typingaliases in the three new share modules. Ruff reports UP035 at all three sites. The safe fix differs per file, because only two of the three modules havefrom __future__ import annotations.
backend/app/share/registry.py#L15-L15: the future import is present at line 9, so replaceDict/Listwithdict/listin the annotations at lines 39 and 86, and keep onlyOptionalin the import.backend/app/share/server.py#L14-L14: the future import is present at line 9, so replaceList[str]at line 69 withlist[str], and keep onlyOptionalin the import.backend/tests/test_share_routes.py#L5-L5: this module has no future import, andList[str]at line 58 is evaluated at runtime. Addfrom __future__ import annotationsbefore switching tolist[str], otherwise the annotation breaks on the Python 3.9 floor. MoveIteratortocollections.abcin the same change.Based on learnings: this repository targets Python 3.9 as a minimum, so builtin generics in runtime-evaluated annotations require
from __future__ import annotations.🤖 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 `@backend/app/share/registry.py` at line 15, Replace deprecated typing aliases in backend/app/share/registry.py#L15-L15 by using dict/list in the annotations at lines 39 and 86, retaining only Optional in the import; apply the same change in backend/app/share/server.py#L14-L14 for List[str] at line 69, retaining only Optional. In backend/tests/test_share_routes.py#L5-L5, add the future-annotations import before changing List[str] at line 58 to list[str], and move Iterator to collections.abc; all three modules must remain compatible with Python 3.9.Sources: Learnings, Linters/SAST tools
30-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
unlock_tokensplaceholder.
unlock_tokensappears only in theShareEntrydataclass default and is never read or written. If you keep it, annotate the field asset[str]; otherwise remove it to avoid implying password/token unblocking support.🤖 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 `@backend/app/share/registry.py` at line 30, Remove the unused unlock_tokens field from the ShareEntry dataclass, since it is never read or written and should not imply token-unblocking support.
99-102: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winStop the share listener when all shares expire.
get_sharesreturns expired entries from the raw_sharesdict instead of usingshare_registry_list(), soshare_registry_count() == 0is currently only checked on explicit revocation. If a share expires and no request revokes it, the listener remains advertised/reachable with no live share to serve. Include the read/lookup paths in the shutdown check so the last expired share also closes the socket.🤖 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 `@backend/app/share/registry.py` around lines 99 - 102, Update the share registry read/lookup paths, especially get_shares and related accessors, to use the live-share filtering in share_registry_list() and trigger the existing shutdown behavior when no unexpired shares remain. Ensure expiration discovered during reads causes the listener to close, while preserving normal lookup behavior for active shares.backend/tests/test_share_registry.py (1)
15-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBoth new test modules omit return annotations. The guideline for
backend/**/*.pyrequires annotated signatures and return types, and neither new test module follows it.
backend/tests/test_share_registry.py#L15-L20: annotateempty_registryas-> Iterator[None], and add-> Noneto every test method inTestCreate,TestLookup,TestRevoke, andTestListing.backend/tests/test_share_routes.py#L28-L29: annotate thetmp_pathparameter aspathlib.Path, parameterize theIterator[dict]return asIterator[dict[str, object]]or aTypedDict, and add-> Noneto every test method inTestViewer,TestTokenLifecycle,TestMedia,TestViewerChrome, andTestSurface.As per coding guidelines: "In Python, annotate function signatures and return types".
🤖 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 `@backend/tests/test_share_registry.py` around lines 15 - 20, Annotate all test function signatures in backend/tests/test_share_registry.py (lines 15-20 and every test in TestCreate, TestLookup, TestRevoke, and TestListing), including empty_registry with -> Iterator[None] and each test with -> None. In backend/tests/test_share_routes.py (lines 28-29 and every test in TestViewer, TestTokenLifecycle, TestMedia, TestViewerChrome, and TestSurface), annotate tmp_path as pathlib.Path, parameterize the fixture return as Iterator[dict[str, object]] or a TypedDict, and add -> None to every test method.Source: Coding guidelines
backend/app/share/routes.py (1)
22-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe public surface produces no log records.
loggeris created at line 22 and never called. This is the only surface reachable from the local network, so an operator has no way to see who fetched what, or how often a bad token was tried. uvicorn access logging is also effectively off, becauseshare_server_startinbackend/app/share/server.pysetslog_level="warning".Log a rejected token at debug or info level, without the token value itself, so a failing share can be diagnosed without creating a token record in the log file.
💡 Sketch
def _require_share(token: str) -> ShareEntry: entry = share_registry_get(token) if entry is None: + # The token itself stays out of the log: it is the only credential. + logger.info("Rejected a share request with an unknown or expired token") raise _not_found() return entryAlso applies to: 70-85
🤖 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 `@backend/app/share/routes.py` at line 22, Update the rejected-token handling in the share route around the affected lines 70–85 to call the module logger at debug or info level, recording that authentication failed without including the token value. Reuse the existing logger symbol and preserve the current rejection response behavior.backend/tests/test_share_routes.py (1)
135-161: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThree security-relevant or fallback branches have no test.
The suite is thorough on token lifecycle and cross-album refusal. Three paths in the code under review remain uncovered.
First,
view_shareinbackend/app/share/routes.pylines 52-55 returns 404 when the album was deleted while shared. Deletealbum-1and assert 404.Second,
share_media_resolve_pathinbackend/app/share/media.pylines 50-51 falls back to the original whenthumbnailPathis missing. Insert an image with a nullthumbnailPathand assert that the thumbnail route serves the original bytes.Third,
backend/app/share/server.pyhas no test at all._bind_first_freewalks up to five ports on collision, which is logic that can regress without notice.As per path instructions: "Verify that all critical functionality is covered by tests".
💚 Sketch for the first two
def test_deleted_album_makes_the_token_meaningless(self, share_env): conn = sqlite3.connect(share_env["db_path"]) conn.execute("DELETE FROM albums WHERE album_id = ?", ("album-1",)) conn.commit() conn.close() assert share_env["client"].get(f"/s/{share_env['token']}").status_code == 404 def test_thumbnail_falls_back_to_the_original(self, share_env): conn = sqlite3.connect(share_env["db_path"]) conn.execute("UPDATE images SET thumbnailPath = NULL WHERE id = ?", ("img-1",)) conn.commit() conn.close() response = share_env["client"].get(f"/s/{share_env['token']}/thumb/img-1") assert response.status_code == 200 assert response.content == JPEG_BYTESDo you want me to write these tests, plus a lifecycle test for
share_server_startandshare_server_stop?🤖 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 `@backend/tests/test_share_routes.py` around lines 135 - 161, Extend the share-route test coverage with cases for a deleted album returning 404 from the shared album view and a null image thumbnailPath causing the thumbnail route to serve the original bytes. Add server coverage for _bind_first_free, verifying it advances through occupied ports and selects the first available one; cover share_server_start and share_server_stop only as needed to exercise this lifecycle behavior.Source: Path instructions
backend/app/share/app.py (1)
16-27: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider adding baseline response headers on the network-exposed app.
The app serves HTML and image bytes to any browser that can route to the machine. Two cheap headers reduce the exposure.
X-Content-Type-Options: nosniffstops a browser from sniffing a served original into an executable type. AContent-Security-Policythat restrictsdefault-srcto'self'matches the intent stated in the docstring ofbackend/app/share/templates/album.html, that the page loads no external resources. The inline<script>blocks in the template requirescript-src 'self' 'unsafe-inline', or a nonce.The album name is already escaped by Jinja and covered by
test_album_name_is_escaped, so this is defense in depth rather than a live exploit.🛡️ Sketch
`@app.middleware`("http") async def _harden(request: Request, call_next): response = await call_next(request) response.headers["X-Content-Type-Options"] = "nosniff" response.headers["Referrer-Policy"] = "no-referrer" # The viewer is fully self-contained; nothing off-origin should load. response.headers["Content-Security-Policy"] = ( "default-src 'self'; img-src 'self'; " "style-src 'self' 'unsafe-inline'; script-src 'self' 'unsafe-inline'" ) return response🤖 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 `@backend/app/share/app.py` around lines 16 - 27, Add HTTP middleware in create_share_app that applies baseline security headers to every response: X-Content-Type-Options nosniff, Referrer-Policy no-referrer, and a self-contained Content-Security-Policy permitting only same-origin resources plus inline styles and scripts required by the album template. Preserve the existing disabled documentation endpoints and share router registration.
🤖 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.
Inline comments:
In `@backend/app/routes/share.py`:
- Around line 52-72: The share route module currently owns database access, URL
construction, and share lifecycle orchestration. Keep the route handlers and
_to_share limited to HTTP parsing, validation, and response conversion; move
album/share database operations into app/database and move _to_share
URL/business orchestration plus create_share and revoke_share lifecycle logic
into app/utils, then have the routes delegate to those helpers while preserving
existing responses.
- Line 76: Update the response declarations for get_interfaces, get_shares, and
revoke_share to include HTTP 500 responses using the ShareErrorResponseEnvelope
model, while preserving their existing response declarations. Regenerate
docs/backend/backend_python/openapi.json so the OpenAPI specification reflects
these additions.
- Around line 116-118: Update the create_share function signature to annotate
body as CreateShareRequest | None while retaining Body(default=None), so the
optional request-body behavior matches its type annotation.
- Around line 162-165: Update the lifecycle logic around revoke_share and
create_share so the zero-share stop decision and share_server_stop() cannot race
with share creation. Keep the relevant lock held through the stop operation, or
otherwise coordinate pending starts and stops so a concurrent create cannot
leave an active share behind a stopped listener.
In `@backend/app/share/media.py`:
- Around line 40-41: Replace the full album fetch and set membership check in
share_media_resolve_path with a call to a new database-layer helper named
db_album_contains_image. Implement that helper in backend/app/database/albums.py
using a targeted album_images existence query for the album_id and image_id,
returning a boolean and closing the connection reliably.
In `@backend/app/share/server.py`:
- Around line 79-98: Supervise the task created by share_server_start: add the
reset and serve-result helpers, update the existing guard to verify _task is
still running and clear stale state before returning, and register a done
callback on _task that retrieves exceptions, logs failures, and resets _server,
_task, _sock, and _port. Preserve the existing successful startup and
port-return behavior.
- Around line 89-93: Update the embedded share server shutdown flow in
share_server_stop() to bound waiting for the server task with an explicit
timeout instead of unbounded asyncio.gather(). If shutdown relies on Uvicorn’s
timeout_graceful_shutdown, upgrade the pinned uvicorn dependency to a version
supporting that option and configure it in the Config construction.
In `@backend/app/share/templates/album.html`:
- Line 407: Update the lightbox keyboard handling in openLightbox and
closeLightbox so that, while the dialog is active, Tab and Shift+Tab cycle only
through focusable elements inside `#lightbox`, wrapping at the first and last
elements; remove the listener and preserve focus restoration when closing.
- Around line 529-534: Update the prefers-color-scheme listener setup near
applyTheme to feature-detect MediaQueryList.addEventListener and use the legacy
addListener API when unavailable. Preserve the existing change handler and
ensure applyTheme and subsequent viewer initialization still execute on older
Safari and WebViews.
In `@backend/app/utils/network.py`:
- Around line 95-98: Update the address filtering logic around the ip loop to
exclude only loopback addresses, allowing usable 169.254.0.0/16 link-local
candidates through. Add or adjust coverage in the network utility tests so a
link-local candidate is retained and can produce a share URL.
- Around line 52-59: Update the rank method to sort active interfaces before
default-route status, while preserving the existing virtual-interface and
interface-name tie-breakers. Add a test covering a down default-route candidate
versus an up non-default candidate, asserting the active interface is ranked
first.
In `@backend/tests/test_network_utils.py`:
- Around line 25-39: Annotate the fake_interfaces fixture and its returned
install callback with precise typing. Define the address and stats parameters as
mappings from interface-name strings to their respective adapter address and
stats values, and declare route_ip as Optional[str], including the callback
return type and fake_interfaces return type as appropriate.
---
Nitpick comments:
In `@backend/app/share/app.py`:
- Around line 16-27: Add HTTP middleware in create_share_app that applies
baseline security headers to every response: X-Content-Type-Options nosniff,
Referrer-Policy no-referrer, and a self-contained Content-Security-Policy
permitting only same-origin resources plus inline styles and scripts required by
the album template. Preserve the existing disabled documentation endpoints and
share router registration.
In `@backend/app/share/registry.py`:
- Line 15: Replace deprecated typing aliases in
backend/app/share/registry.py#L15-L15 by using dict/list in the annotations at
lines 39 and 86, retaining only Optional in the import; apply the same change in
backend/app/share/server.py#L14-L14 for List[str] at line 69, retaining only
Optional. In backend/tests/test_share_routes.py#L5-L5, add the
future-annotations import before changing List[str] at line 58 to list[str], and
move Iterator to collections.abc; all three modules must remain compatible with
Python 3.9.
- Line 30: Remove the unused unlock_tokens field from the ShareEntry dataclass,
since it is never read or written and should not imply token-unblocking support.
- Around line 99-102: Update the share registry read/lookup paths, especially
get_shares and related accessors, to use the live-share filtering in
share_registry_list() and trigger the existing shutdown behavior when no
unexpired shares remain. Ensure expiration discovered during reads causes the
listener to close, while preserving normal lookup behavior for active shares.
In `@backend/app/share/routes.py`:
- Line 22: Update the rejected-token handling in the share route around the
affected lines 70–85 to call the module logger at debug or info level, recording
that authentication failed without including the token value. Reuse the existing
logger symbol and preserve the current rejection response behavior.
In `@backend/tests/test_share_registry.py`:
- Around line 15-20: Annotate all test function signatures in
backend/tests/test_share_registry.py (lines 15-20 and every test in TestCreate,
TestLookup, TestRevoke, and TestListing), including empty_registry with ->
Iterator[None] and each test with -> None. In backend/tests/test_share_routes.py
(lines 28-29 and every test in TestViewer, TestTokenLifecycle, TestMedia,
TestViewerChrome, and TestSurface), annotate tmp_path as pathlib.Path,
parameterize the fixture return as Iterator[dict[str, object]] or a TypedDict,
and add -> None to every test method.
In `@backend/tests/test_share_routes.py`:
- Around line 135-161: Extend the share-route test coverage with cases for a
deleted album returning 404 from the shared album view and a null image
thumbnailPath causing the thumbnail route to serve the original bytes. Add
server coverage for _bind_first_free, verifying it advances through occupied
ports and selects the first available one; cover share_server_start and
share_server_stop only as needed to exercise this lifecycle behavior.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c1b01dad-f640-4944-8649-10e98dd2bdde
📒 Files selected for processing (16)
backend/app/config/settings.pybackend/app/routes/share.pybackend/app/schemas/share.pybackend/app/share/__init__.pybackend/app/share/app.pybackend/app/share/media.pybackend/app/share/registry.pybackend/app/share/routes.pybackend/app/share/server.pybackend/app/share/templates/album.htmlbackend/app/utils/network.pybackend/main.pybackend/tests/test_network_utils.pybackend/tests/test_share_registry.pybackend/tests/test_share_routes.pydocs/backend/backend_python/openapi.json
| def _to_share(entry: ShareEntry, port: int) -> Share: | ||
| album = db_get_album(entry.album_id) | ||
| return Share( | ||
| token=entry.token, | ||
| album_id=entry.album_id, | ||
| # An album deleted while shared leaves the token pointing at nothing; | ||
| # the viewer 404s, so say so here rather than failing the listing. | ||
| album_name=album["album_name"] if album else "(deleted album)", | ||
| image_count=len(db_get_album_images(entry.album_id)), | ||
| port=port, | ||
| created_at=entry.created_at.isoformat(), | ||
| expires_at=entry.expires_at.isoformat() if entry.expires_at else None, | ||
| urls=[ | ||
| ShareUrl( | ||
| interface=candidate.interface, | ||
| ip=candidate.ip, | ||
| url=f"http://{candidate.ip}:{port}/s/{entry.token}", | ||
| ) | ||
| for candidate in network_util_list_candidates() | ||
| ], | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Move share orchestration out of the route module.
_to_share performs database access and share URL construction. create_share and revoke_share manage registry and server lifecycle state. Keep this module limited to HTTP parsing, validation, and response conversion. Move orchestration to app/utils/ and database operations to app/database/.
As per coding guidelines, “Routes handle HTTP and validation only; business logic belongs in app/utils/ and data access belongs in app/database/.”
Also applies to: 125-165
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 67-67: Do not make http calls without encryption
Context: f"http://{candidate.ip}:{port}/s/{entry.token}"
Note: [CWE-319] Cleartext Transmission of Sensitive Information.
(requests-http)
🤖 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 `@backend/app/routes/share.py` around lines 52 - 72, The share route module
currently owns database access, URL construction, and share lifecycle
orchestration. Keep the route handlers and _to_share limited to HTTP parsing,
validation, and response conversion; move album/share database operations into
app/database and move _to_share URL/business orchestration plus create_share and
revoke_share lifecycle logic into app/utils, then have the routes delegate to
those helpers while preserving existing responses.
Source: Coding guidelines
Reading every image id per media request turned one page view into a quadratic number of row reads, plus a SQLite connection per thumbnail.
A serve task that died left the port set, so a share was handed out for a socket nothing was accepting on. Start and stop now hold one lock.
aria-modal claimed the page behind was inert while Tab still reached it. Older Safari lacks addEventListener on MediaQueryList, which killed the script before any thumbnail loaded.
Windows keeps a lease and a default route on a disconnected adapter, so ranking that first advertised an address nothing could reach. Link-local is now kept but ranked last rather than dropped.
Routes now handle HTTP only, with lifecycle and URL building in app/utils. Every share endpoint declares its 500 response and the optional request body is annotated as optional.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/tests/test_network_utils.py (2)
102-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the optional candidate before member access.
network_util_best_candidate()returnsOptional[InterfaceCandidate]. Store its result and assert that it is notNonebefore accessing.ipor.is_default_route.As per path instructions, ensure proper use of type hints.
Also applies to: 118-120, 172-172
🤖 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 `@backend/tests/test_network_utils.py` around lines 102 - 104, Update each affected test using network_util_best_candidate() to assert the returned candidate is not None before accessing its members, preserving the existing IP and default-route assertions. Add or retain appropriate type narrowing so static type checking recognizes the non-optional InterfaceCandidate after the assertion.Source: Path instructions
3-3: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse PEP 585 builtin generic syntax.
Replace
Dict[...],List[...], andIteratorfromtypingwithdict[...],list[...], andIteratorfromcollections.abc.Proposed fix
-from typing import Dict, Iterator, List, Optional, Protocol +from collections.abc import Iterator +from typing import Optional, ProtocolRun
pre-commit run --config .pre-commit-config.yaml --all-filesfrom the repository root to validate the change.🤖 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 `@backend/tests/test_network_utils.py` at line 3, Update the imports in test_network_utils.py to use dict and list builtin generic syntax instead of typing.Dict and typing.List, and import Iterator from collections.abc while retaining Optional and Protocol from typing. Run the repository pre-commit command to validate the change.Sources: Coding guidelines, Learnings, Linters/SAST tools
backend/app/utils/network.py (1)
52-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSpecify the
ranktuple element types.
-> tupledoes not describe the comparison contract. Use the concrete tuple type so static checks can detect incompatible rank values.Proposed fix
- def rank(self) -> tuple: + def rank(self) -> tuple[int, int, int, int, str]:As per coding guidelines, annotate function signatures and return types accurately. As per path instructions, ensure proper use of type hints.
🤖 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 `@backend/app/utils/network.py` around lines 52 - 62, Update the NetworkAddress rank property’s return annotation from generic tuple to a concrete tuple type matching its returned elements: three integer ranking values followed by the string interface name. Preserve the existing ordering logic and values.Sources: Coding guidelines, Path instructions
🤖 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.
Inline comments:
In `@backend/tests/test_albums_db.py`:
- Around line 266-292: Add the appropriate str type annotation to the test_db
parameter and explicit -> None return annotations on each
TestAlbumImageMembership test method shown, including the contains-image and
image-count tests; preserve their existing behavior.
In `@backend/tests/test_share_server.py`:
- Around line 67-70: Update the return type annotations of both test scenario
functions, including scenario near the shown
share_server_start/share_server_stop flow and the scenario around lines 91-94,
from Tuple[object, bool] to Tuple[Optional[int], bool]. Ensure Optional is
imported if needed, while preserving the existing scenario behavior.
---
Nitpick comments:
In `@backend/app/utils/network.py`:
- Around line 52-62: Update the NetworkAddress rank property’s return annotation
from generic tuple to a concrete tuple type matching its returned elements:
three integer ranking values followed by the string interface name. Preserve the
existing ordering logic and values.
In `@backend/tests/test_network_utils.py`:
- Around line 102-104: Update each affected test using
network_util_best_candidate() to assert the returned candidate is not None
before accessing its members, preserving the existing IP and default-route
assertions. Add or retain appropriate type narrowing so static type checking
recognizes the non-optional InterfaceCandidate after the assertion.
- Line 3: Update the imports in test_network_utils.py to use dict and list
builtin generic syntax instead of typing.Dict and typing.List, and import
Iterator from collections.abc while retaining Optional and Protocol from typing.
Run the repository pre-commit command to validate the change.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a63f68f5-6159-4c25-b66c-f1a769496bb0
📒 Files selected for processing (12)
backend/app/database/albums.pybackend/app/routes/share.pybackend/app/schemas/share.pybackend/app/share/media.pybackend/app/share/server.pybackend/app/share/templates/album.htmlbackend/app/utils/network.pybackend/app/utils/share.pybackend/tests/test_albums_db.pybackend/tests/test_network_utils.pybackend/tests/test_share_server.pydocs/backend/backend_python/openapi.json
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/app/schemas/share.py
- backend/app/routes/share.py
- backend/app/share/templates/album.html
- docs/backend/backend_python/openapi.json
- backend/app/share/media.py
Concrete rank tuple, a TypedDict for the share description, and typing.List to match the rest of the backend. Drops an unused ShareEntry field that nothing read.
Return types and fixture parameter types throughout, a TypedDict for the share_env fixture, and narrowing before member access on optional returns.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/app/share/media.py (1)
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace deprecated
typingcollection aliases.Ruff reports UP035 for these new imports. Use built-in generics such as
list[str]andtuple[...]. ImportIteratorfromcollections.abc.
backend/app/share/media.py#L12-L12: replaceListwithlistinshare_media_image_ids.backend/app/utils/network.py#L13-L13: replaceListandTuplewith built-in generics.backend/tests/test_share_registry.py#L2-L2: importIteratorfromcollections.abc.backend/tests/test_share_routes.py#L5-L6: importIteratorfromcollections.abcand replaceList[str]withlist[str].backend/tests/test_share_server.py#L2-L3: importIteratorfromcollections.abcand replaceListandTupleannotations.Verify the update with the repository pre-commit command. Do not run
ruff format.As per coding guidelines, Ruff may be used as a linter and
ruff formatmust not be used.🤖 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 `@backend/app/share/media.py` at line 12, Replace deprecated typing collection aliases across backend/app/share/media.py lines 12-12, backend/app/utils/network.py lines 13-13, backend/tests/test_share_registry.py lines 2-2, backend/tests/test_share_routes.py lines 5-6, and backend/tests/test_share_server.py lines 2-3: use built-in list[...] and tuple[...] annotations, import Iterator from collections.abc, and update share_media_image_ids and the affected test annotations accordingly. Verify with the repository pre-commit command; do not run ruff format.Sources: Coding guidelines, Linters/SAST tools
backend/app/utils/share.py (1)
12-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
typing.Listwith the built-in generic.
Listis deprecated in Ruff for Python 3.9+. Uselist[ShareUrlRecord]inShareDescription.urlsand removeListfrom the import.🤖 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 `@backend/app/utils/share.py` at line 12, Replace the deprecated typing.List usage in ShareDescription.urls with the built-in list[ShareUrlRecord] annotation, and remove List from the typing import while preserving the existing type semantics.Sources: Learnings, Linters/SAST tools
🤖 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.
Inline comments:
In `@backend/app/utils/share.py`:
- Around line 25-45: Move ShareUrlRecord and ShareDescription from
utils/share.py into schemas/share.py and update imports to use the shared schema
definitions. Relocate share_util_describe and its database/network orchestration
from utils/share.py into routes/share.py or the approved service layer, keeping
utils/share.py limited to pure helper logic and preserving existing behavior.
In `@backend/tests/test_share_server.py`:
- Around line 95-98: Replace the fixed asyncio.sleep in scenario with bounded
polling of the observable listener state, yielding across event-loop turns until
share_server_is_running() reports cleanup or the retry limit is reached. Keep
the retry bounded, then return share_server_port() and share_server_is_running()
for the existing final assertions.
---
Nitpick comments:
In `@backend/app/share/media.py`:
- Line 12: Replace deprecated typing collection aliases across
backend/app/share/media.py lines 12-12, backend/app/utils/network.py lines
13-13, backend/tests/test_share_registry.py lines 2-2,
backend/tests/test_share_routes.py lines 5-6, and
backend/tests/test_share_server.py lines 2-3: use built-in list[...] and
tuple[...] annotations, import Iterator from collections.abc, and update
share_media_image_ids and the affected test annotations accordingly. Verify with
the repository pre-commit command; do not run ruff format.
In `@backend/app/utils/share.py`:
- Line 12: Replace the deprecated typing.List usage in ShareDescription.urls
with the built-in list[ShareUrlRecord] annotation, and remove List from the
typing import while preserving the existing type semantics.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 91e643a2-4eae-46c7-b1ce-22df991d42b4
📒 Files selected for processing (9)
backend/app/share/media.pybackend/app/share/registry.pybackend/app/utils/network.pybackend/app/utils/share.pybackend/tests/test_albums_db.pybackend/tests/test_network_utils.pybackend/tests/test_share_registry.pybackend/tests/test_share_routes.pybackend/tests/test_share_server.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/tests/test_network_utils.py
- backend/tests/test_albums_db.py
| class ShareUrlRecord(TypedDict): | ||
| """One reachable address for a share, as the desktop UI receives it.""" | ||
|
|
||
| interface: str | ||
| ip: str | ||
| url: str | ||
|
|
||
|
|
||
| class ShareDescription(TypedDict): | ||
| """Everything the desktop UI needs to present one share.""" | ||
|
|
||
| token: str | ||
| album_id: str | ||
| album_name: str | ||
| image_count: int | ||
| port: int | ||
| created_at: str | ||
| expires_at: Optional[str] | ||
| urls: List[ShareUrlRecord] | ||
|
|
||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Preserve the backend layer boundaries.
ShareUrlRecord and ShareDescription are shared payload contracts. Define them in backend/app/schemas/share.py, not in backend/app/utils/share.py.
share_util_describe performs database reads and network discovery. Move this orchestration to backend/app/routes/share.py or the project's approved service layer. Keep backend/app/utils/share.py limited to pure helpers.
As per coding guidelines, routes belong in app/routes/, shared schemas belong in app/schemas/, database access belongs in app/database/, and pure helpers belong in app/utils/; do not collapse layers.
Also applies to: 75-101
🤖 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 `@backend/app/utils/share.py` around lines 25 - 45, Move ShareUrlRecord and
ShareDescription from utils/share.py into schemas/share.py and update imports to
use the shared schema definitions. Relocate share_util_describe and its
database/network orchestration from utils/share.py into routes/share.py or the
approved service layer, keeping utils/share.py limited to pure helper logic and
preserving existing behavior.
Source: Coding guidelines
A fixed 50ms delay would read a live listener under CI load and fail a callback that works. Polls the observable state to a bounded deadline.
|
@coderabbitai review the changes, and also, mark those reviews as "completed" those which have been addressed. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="AOSSIE-Org/PictoPy"
pr="1469"
echo "== Pull request metadata =="
gh pr view "$pr" --repo "$repo" --json number,title,headRefName,baseRefName,reviewDecision,comments,reviews
echo
echo "== Changed files =="
gh pr diff "$pr" --repo "$repo" --name-only
echo
echo "== CodeRabbit review threads and replies =="
gh api graphql \
-F owner="AOSSIE-Org" \
-F name="PictoPy" \
-F number="$pr" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
isOutdated
path
line
originalLine
comments(first: 50) {
nodes {
author { login }
body
createdAt
url
}
}
}
}
}
}
}' \
--jq '.data.repository.pullRequest.reviewThreads.nodes[] |
select(any(.comments.nodes[]?; .author.login == "coderabbitai")) |
{id, isResolved, isOutdated, path, line, originalLine, comments: [.comments.nodes[] | {author: .author.login, body, url}]}'
echo
echo "== Changed implementation structure =="
ast-grep outline backend/app/share --items all --type function,class
ast-grep outline backend/app/utils/share.py --items all --type function,class
ast-grep outline backend/app/routes/share.py --items all --type function,classLength of output: 121415 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Applicable repository instructions =="
fd -a '^AGENTS\.md$|^\.coderabbit\.yaml$|^pyproject\.toml$|^requirements.*\.txt$' . \
--exclude .git \
--exec sh -c 'echo "--- $1"; sed -n "1,240p" "$1"' sh {}
echo
echo "== Current pull request diff =="
git diff --find-renames origin/main...HEAD -- \
backend/app/config/settings.py \
backend/app/database/albums.py \
backend/app/routes/share.py \
backend/app/schemas/share.py \
backend/app/share \
backend/app/utils/network.py \
backend/app/utils/share.py \
backend/main.py \
backend/tests/test_albums_db.py \
backend/tests/test_network_utils.py \
backend/tests/test_share_registry.py \
backend/tests/test_share_routes.py \
backend/tests/test_share_server.py \
| sed -n '1,2600p'
echo
echo "== Targeted current implementation =="
for file in \
backend/app/routes/share.py \
backend/app/utils/share.py \
backend/app/share/server.py \
backend/app/share/registry.py \
backend/app/share/routes.py \
backend/app/share/media.py \
backend/app/share/app.py \
backend/app/database/albums.py \
backend/app/utils/network.py \
backend/app/schemas/share.py \
backend/main.py
do
echo "--- $file"
cat -n "$file"
done
echo
echo "== Share-related tests =="
for file in \
backend/tests/test_share_server.py \
backend/tests/test_share_registry.py \
backend/tests/test_share_routes.py \
backend/tests/test_network_utils.py \
backend/tests/test_albums_db.py
do
echo "--- $file"
cat -n "$file"
doneLength of output: 221020 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="AOSSIE-Org/PictoPy"
thread_id="PRRT_kwDOJpiIC86XFCYT"
echo "== Resolve verified addressed CodeRabbit thread =="
gh api graphql \
-f query='
mutation($threadId: ID!) {
resolveReviewThread(input: {threadId: $threadId}) {
thread {
id
isResolved
path
line
}
}
}' \
-F threadId="$thread_id"
echo
echo "== Verify the two previously unresolved threads =="
gh api graphql \
-F owner="AOSSIE-Org" \
-F name="PictoPy" \
-F number="1469" \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
path
line
comments(first: 1) { nodes { url } }
}
}
}
}
}' \
--jq '.data.repository.pullRequest.reviewThreads.nodes[] |
select(.id == "PRRT_kwDOJpiIC86XFCYT" or .id == "PRRT_kwDOJpiIC86XGCHP")'Length of output: 1774
I marked the addressed optional request-body thread as completed. I left the I found one remaining issue:
The requested automated CodeRabbit review is temporarily rate-limited. You are interacting with an AI system. |
|
@coderabbitai try again |
|
✅ Action performedReview finished.
|
1 participant
Closes #1468
Adds the ability to serve one album over the local network, so that a recipient on the same Wi-Fi can browse it in a browser while PictoPy is running. Nothing is uploaded, and the recipient needs no account and no app.
Approach
The album is served by a second FastAPI application bound to
0.0.0.0, started on demand when the first share is created and stopped when the last one is revoked. The main backend stays on localhost because it exposes shutdown, delete and metadata routes that must not be reachable from other devices, so binding it more widely was never an option.The share server runs as an asyncio task on the backend's existing event loop rather than as a separate process, which means no packaging or Tauri changes are needed. Its socket is bound before uvicorn receives it, because uvicorn raises
SystemExiton a bind failure and would otherwise terminate the whole backend when a port is busy.Active shares are held in memory rather than in the database. Shares are meant to end when PictoPy closes, and keeping them in memory makes that structural instead of something a cleanup path has to enforce. There is no migration, and no token is ever written to disk.
Security
Four invariants, each covered by a test. An image is resolved to a file only after it has been confirmed to belong to the shared album, so a token cannot be used to read other images. Filesystem paths are never sent to the client, which works entirely in IDs. A revoked, expired or unknown token returns the same 404, so the response cannot be used to probe for valid tokens. The share application carries no CORS middleware and no interactive documentation, and exposes nothing outside
/s/{token}.An album's local lock is deliberately not consulted. Locking protects an album inside PictoPy, while a share carries its own authorization.
Receiver page
The recipient gets a single server-rendered Jinja template with inline CSS and JavaScript, so there is no second frontend build and no asset pipeline. It has a responsive grid with lazily loaded thumbnails, a lightbox with keyboard, swipe and filmstrip navigation, and light and dark themes. It references no external resources, because the page is often served on a network with no route to the internet.
Not included
Password protection, the share dialog in the desktop UI, and diagnostics for networks that block device-to-device traffic are follow-up work. Sharing over the internet is out of scope.
Testing
42 new tests covering the registry, the interface ranking and the route surface, with the full suite at 1044 passing. Verified end to end on three networks, with an album loaded on a phone over Wi-Fi.
Reviewing commit by commit is likely easiest, as each one is self-contained and they build in dependency order.
Summary by CodeRabbit