Feat: Architecture Refactor - #1
Merged
Merged
Conversation
… removed redundant code blocks and optimized existing functions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @Zingzy, your pull request is larger than the review limit of 150000 diff characters
There was a problem hiding this comment.
Pull request overview
This PR performs a major refactor from a Flask-based QR generator into a layered FastAPI application, introducing service/middleware/shared modules, a refreshed frontend UI, and a new unit/integration test suite.
Changes:
- Replaces the Flask app with a FastAPI app factory (
create_app) plus structured routing under/api/v1. - Introduces
QRService(async, thread-pooled CPU work) and shared helpers (color parsing, formatters, QR sizing, logging). - Adds Docker/uv packaging, CI workflows, and extensive unit + integration tests; updates the web UI assets.
Reviewed changes
Copilot reviewed 50 out of 65 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| utils.py | Removes the previous monolithic utility module (logic moved into shared/ + services/). |
| main.py | Switches entrypoint to FastAPI (create_app), dotenv loading, and structured logging setup. |
| app.py | Adds FastAPI application factory, middleware, static mounting, routers, and /docs behavior. |
| config.py | Adds AppSettings via pydantic-settings (env/.env driven configuration). |
| errors.py | Introduces typed error hierarchy (AppError, ValidationError, QRGenerationError). |
| dependencies/services.py | Adds FastAPI dependency provider for QRService. |
| services/qr_service.py | Implements async QR generation (classic/gradient/batch) with styling, SVG/PNG, and optional logo embedding. |
| shared/color.py | Adds centralized color parsing utility used by services/routes. |
| shared/formatters.py | Adds structured content formatters (vCard/WiFi/etc.) and a registry keyed by DataFormat. |
| shared/qr_utils.py | Adds QR version/box size calculators with a capacity table. |
| shared/logging.py | Adds structlog-based logging configuration + IP hashing helper. |
| shared/ip_utils.py | Adds proxy-aware client IP extraction helper. |
| schemas/enums.py | Defines enums for data formats, module styles, gradient directions, output formats. |
| schemas/dto/requests/qr.py | Adds request DTOs (currently not wired into routes). |
| schemas/dto/responses/common.py | Adds response DTOs for error and health endpoints. |
| routes/api_v1/init.py | Composes /api/v1 router and includes classic/gradient/batch endpoints. |
| routes/api_v1/classic.py | Adds classic QR GET/POST endpoints with query params and optional multipart logo upload. |
| routes/api_v1/gradient.py | Adds gradient QR GET/POST endpoints with direction/style params and optional multipart logo upload. |
| routes/api_v1/batch.py | Adds batch ZIP endpoint with typed request body and per-item options. |
| routes/health_routes.py | Adds /health endpoint returning a typed health response. |
| routes/page_routes.py | Adds / HTML route rendering the web UI template. |
| middleware/error_handler.py | Adds global exception handlers for consistent JSON error responses. |
| middleware/logging.py | Adds request logging middleware with request id, duration, and IP hashing. |
| middleware/openapi.py | Adds OpenAPI schema configuration and shared error response declarations. |
| templates/index.html | Reworks the frontend HTML structure (new controls + preview panel + theme toggle). |
| static/js/index-script.js | Rewrites frontend logic for theme, parameter controls, fetch-based generation, and download handling. |
| static/css/index-style.css | Replaces the previous stylesheet with a new design system + responsive layout + theme support. |
| tests/unit/conftest.py | Prevents tests from loading real .env via patched dotenv provider. |
| tests/unit/test_color.py | Adds unit tests for parse_color. |
| tests/unit/test_formatters.py | Adds unit tests for structured content formatters + registry coverage. |
| tests/unit/test_qr_utils.py | Adds unit tests for QR sizing/version helper functions. |
| tests/unit/test_qr_service.py | Adds async unit tests for QRService (PNG/SVG, styles, validation, batch). |
| tests/unit/test_errors.py | Adds unit tests for typed errors and serialization. |
| tests/unit/test_config.py | Adds unit tests for settings defaults and environment behavior. |
| tests/integration/conftest.py | Adds FastAPI TestClient fixture using create_app(). |
| tests/integration/test_health.py | Adds integration tests for /health. |
| tests/integration/test_pages.py | Adds integration tests for / returning HTML. |
| tests/integration/test_qr_endpoints.py | Adds integration tests for classic/gradient/batch endpoints and key behaviors. |
| tests/conftest.py | Adds root-level note directing fixtures to unit/integration conftests. |
| requirements.txt | Replaces the previous minimal requirements with a pinned dependency set. |
| pyproject.toml | Adds project metadata, uv deps, pytest config, and coverage configuration. |
| dockerfile | Adds a uv-based container build file (note: lowercase filename). |
| docker-compose.yml | Adds dev compose config running uvicorn via uv with reload. |
| .dockerignore | Adds dockerignore rules (avoids baking secrets/tests into images). |
| .env.example | Adds example environment configuration. |
| .github/workflows/tests.yaml | Adds test+coverage CI workflow across Python 3.10–3.13. |
| .github/workflows/github-ci.yaml | Adds Ruff lint/format checking workflow. |
| .github/workflows/format.yaml | Removes old Black-based formatting workflow. |
| .github/dependabot.yml | Adds Dependabot updates for GitHub Actions. |
| .python-version | Pins local Python version for dev tooling. |
| .gitignore | Expands ignore rules for Python/UV tooling, envs, caches, etc. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
+79
to
+83
| ext = "svg" if item.output == OutputFormat.SVG else "png" | ||
| filename = item.filename or f"qrcode_{i + 1}" | ||
| if not filename.endswith(f".{ext}"): | ||
| filename = f"{filename}.{ext}" | ||
| zf.writestr(filename, stream.read()) |
|
|
||
| @router.get("/", response_class=HTMLResponse, include_in_schema=False) | ||
| async def index(request: Request) -> HTMLResponse: | ||
| return templates.TemplateResponse(request, "index.html") |
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughFull migration from a Flask monolith to a FastAPI application with Pydantic settings, structured errors, request logging middleware, modular routers/services for QR generation (classic, gradient, batch), comprehensive tests, Docker/CI configuration, and redesigned frontend assets. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Middleware as RequestLogging<br/>Middleware
participant App as FastAPI<br/>App
participant Handler as Route<br/>Handler
participant Service as QRService
participant Response
Client->>Middleware: HTTP request
Middleware->>Middleware: generate request_id<br/>bind context
Middleware->>App: forward request
App->>Handler: route dispatch & validate
Handler->>Service: call generate_{classic|gradient|batch}
Service->>Service: parse colors / logos<br/>compute QR (CPU-bound)
Service-->>Handler: BytesIO + MIME type
Handler-->>App: StreamingResponse
App-->>Middleware: response
Middleware->>Middleware: log outcome, add X-Request-ID
Middleware-->>Client: HTTP response
Estimated Code Review Effort🎯 5 (Critical) | ⏱️ ~120 minutes
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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: 16
🧹 Nitpick comments (21)
static/js/index-script.js (1)
18-63: Initialize the form UI once after wiring the listeners.The resize, color-label, and classic/gradient sync logic only runs after the first user event. If the page loads with restored or non-default values, the controls can start out of sync until the user changes something. Reuse the same sync routines once during startup.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@static/js/index-script.js` around lines 18 - 63, Call the existing sync routines once at startup so the form reflects any restored/non-default values: after wiring listeners, invoke autoResize() (to size qrInput), loop the colorInputs array and set each corresponding label.textContent = input.value (to sync color labels), and call the same type change handler logic for typeSelect (or dispatch a "change" on typeSelect) to toggle classicColors/gradientColors and enable/disable the svg option and adjust outputSelect.value if needed; reference functions/variables autoResize, qrInput, colorInputs, typeSelect, classicColors, gradientColors, and outputSelect to locate where to add these startup calls..github/workflows/github-ci.yaml (1)
37-43: Redundant dependency installation when using uvx.The
uv pip install --group devstep installs dependencies into a venv, butuvx ruff checkruns ruff in an isolated ephemeral environment, making the venv installation unnecessary for these linting steps.Simplified workflow
- - name: Install Dependencies - run: | - uv venv - uv pip install --group dev - - name: Lint with Ruff run: uvx ruff check - name: Format with Ruff run: uvx ruff format --check🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/github-ci.yaml around lines 37 - 43, Remove the redundant venv setup and dependency install steps that precede the isolated linter run: delete the "Install Dependencies" block that runs "uv venv" and "uv pip install --group dev" since "uvx ruff check" runs Ruff in an ephemeral environment; keep only the "Lint with Ruff" step invoking "uvx ruff check" (or conditionally install deps only when later steps require the venv). Target the CI steps named "Install Dependencies" and the command lines "uv venv" / "uv pip install --group dev" and the "Lint with Ruff" step invoking "uvx ruff check".dockerfile (2)
4-4: Pin the uv version for reproducible builds.Using
:latestcan cause unexpected breakages when the uv tool is updated. Pin to a specific version (currently 0.10.11) for reproducibility.Proposed fix
-COPY --from=ghcr.io/astral-sh/uv:latest /uv /uvx /bin/ +COPY --from=ghcr.io/astral-sh/uv:0.10.11 /uv /uvx /bin/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dockerfile` at line 4, The Dockerfile currently pulls the uv image with the floating tag "latest" in the COPY --from=ghcr.io/astral-sh/uv:latest /uv /uvx /bin/ step which prevents reproducible builds; change the image reference to the fixed release tag (e.g., ghcr.io/astral-sh/uv:0.10.11) so the COPY --from=... line uses the pinned version and repeatable artifact.
13-15: Consider running as a non-root user.The container runs as root by default, which is a security concern. Add a non-root user for the application process.
Proposed fix
WORKDIR /app RUN uv sync --frozen --no-cache +RUN useradd --create-home --shell /bin/bash appuser +USER appuser + EXPOSE 8080 CMD ["uv", "run", "uvicorn", "main:app", "--host", "0.0.0.0", "--port", "8080", "--no-access-log"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dockerfile` around lines 13 - 15, The Dockerfile currently runs the app as root via the CMD instruction; add a non-root user and switch to it before starting the app: create a dedicated user and group (e.g., appuser), set a proper WORKDIR, chown the app files/directories to that user, and add a USER appuser directive prior to the CMD line so the uvicorn process runs unprivileged; ensure any ports/files the app needs have appropriate ownership/permissions for that user.routes/page_routes.py (1)
19-21: Consider usingResponseorTemplateResponseas return type.The function returns
TemplateResponsebut is annotated asHTMLResponse. WhileTemplateResponseinherits fromHTMLResponse, using the actual return type improves clarity.♻️ Optional fix for type precision
from fastapi import APIRouter, Request from fastapi.responses import HTMLResponse -from fastapi.templating import Jinja2Templates +from fastapi.templating import Jinja2Templates, TemplateResponse ... `@router.get`("/", response_class=HTMLResponse, include_in_schema=False) -async def index(request: Request) -> HTMLResponse: +async def index(request: Request) -> TemplateResponse: return templates.TemplateResponse(request, "index.html")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@routes/page_routes.py` around lines 19 - 21, The handler index is annotated with HTMLResponse but returns a TemplateResponse; update the type annotation to TemplateResponse (or the more generic Response) to match the actual return type. Modify the async def index(request: Request) -> HTMLResponse signature to use TemplateResponse (or fastapi.Response) and keep the body returning templates.TemplateResponse(request, "index.html") so the declared return type matches the real return value.shared/color.py (2)
64-65: Consider preserving original exception context.The exception handler discards the original error. Using
frompreserves the chain for debugging.♻️ Optional fix for exception chaining
- except (ValueError, TypeError): - raise ValidationError(f"Invalid color format: {color_str}", field="color") + except (ValueError, TypeError) as exc: + raise ValidationError(f"Invalid color format: {color_str}", field="color") from exc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/color.py` around lines 64 - 65, The except block that catches (ValueError, TypeError) and raises ValidationError currently discards the original exception; update the handler in the function that parses colors so it re-raises the ValidationError with exception chaining (use "raise ValidationError(f\"Invalid color format: {color_str}\", field=\"color\") from e") where e is the caught exception, so the original traceback is preserved for debugging; reference the caught exception variable (e) and the ValidationError class in the change.
29-65: Type annotation mismatch with actual return types.The function returns
tuple(...)from generator expressions which producestuple[int, ...]rather than the annotatedtuple[int, int, int]. This works at runtime but may cause type checker warnings.♻️ Optional fix for type precision
if color_str.startswith("#"): hex_str = color_str.lstrip("#") if len(hex_str) == 6: - return tuple(int(hex_str[i : i + 2], 16) for i in (0, 2, 4)) + r, g, b = (int(hex_str[i : i + 2], 16) for i in (0, 2, 4)) + return (r, g, b) raise ValueError("Invalid hex color format")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/color.py` around lines 29 - 65, The type checker complains because returns like tuple(int(... ) for ...) and tuple(values) are inferred as variable-length tuples; in parse_color replace generator/list-to-tuple returns with explicit 3-element tuples so the signature tuple[int,int,int] is honored: e.g. in the "#..." and bare hex branches return (int(hex_str[0:2],16), int(hex_str[2:4],16), int(hex_str[4:6],16)), and in the "rgb(...)" and "(...)" branches return (values[0], values[1], values[2]) instead of tuple(values); keep the ValidationError handling unchanged.config.py (1)
22-23: Consider restricting CORS origins in production.The default
cors_origins: list[str] = ["*"]is permissive. While acceptable for development, production deployments should explicitly configure allowed origins via environment variables to avoid exposing the API to cross-origin requests from any domain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config.py` around lines 22 - 23, The cors_origins setting currently allows all origins via the variable cors_origins: list[str] = ["*"]; change it to read and parse an environment variable (e.g., CORS_ORIGINS) and only default to ["*"] in development—parse a comma-separated string into list[str] and validate entries before assigning to cors_origins so production must explicitly supply allowed origins; update any config-loading function or module-level initialization where cors_origins is defined to perform this env read/parse/validation.routes/api_v1/batch.py (1)
50-57: Duplicate batch size validation is acceptable but noted.The 20-item limit is validated both here (Pydantic model) and in
QRService.generate_batch(). This defense-in-depth is fine, but consider centralizing the constant to avoid drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@routes/api_v1/batch.py` around lines 50 - 57, The 20-item limit is duplicated in the Pydantic validator validate_items (on class containing BatchItem) and in QRService.generate_batch; extract that magic number into a single shared constant (e.g., BATCH_MAX_SIZE) in a common module or at top of this module, update validate_items to compare against BATCH_MAX_SIZE and update QRService.generate_batch to reference the same BATCH_MAX_SIZE (importing it if moved to a shared module), and keep the existing error messages but use the constant so the limit cannot drift between the two places.routes/api_v1/gradient.py (1)
45-47: Consider restrictingoutputparameter to valid options.The GET endpoint accepts
OutputFormatincluding SVG, butgenerate_gradientraises aValidationErrorfor SVG output. Consider documenting this limitation more clearly in the Query description or restricting the enum to PNG-only for this endpoint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@routes/api_v1/gradient.py` around lines 45 - 47, The route currently accepts the full OutputFormat enum via the output parameter but generate_gradient only supports PNG; update the endpoint to prevent SVG by either narrowing the parameter type to PNG-only (e.g., replace the parameter type with a Literal or a new PNG-only enum) or adding an explicit validation check in generate_gradient that returns a 400 HTTP error for non-PNG values; update the Query description on the output parameter to state "PNG only" and reference the output parameter and generate_gradient function so callers and maintainers see the restriction.routes/api_v1/classic.py (1)
76-91: Consider closing the UploadFile after reading.FastAPI's
UploadFilewraps a SpooledTemporaryFile. While it's typically cleaned up when the request ends, explicitly closing it after reading is good practice to release resources promptly, especially for large files.♻️ Suggested improvement
async def generate_classic_post( ... logo: Optional[UploadFile] = File( None, description="Logo image to embed (optional)" ), qr_service: QRService = Depends(get_qr_service), ) -> StreamingResponse: - logo_bytes = await logo.read() if logo else None + logo_bytes = None + if logo: + logo_bytes = await logo.read() + await logo.close() effective_output = OutputFormat.PNG if logo_bytes else output🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@routes/api_v1/classic.py` around lines 76 - 91, The UploadFile received as logo is read but not closed; after reading logo.read() in the route that calls qr_service.generate_classic (where logo_bytes is created), ensure you explicitly close the UploadFile (await logo.close()) in the same function (the route handling code using the logo: Optional[UploadFile] parameter) to release the underlying temp file/resource promptly, doing so after you obtain logo_bytes and before returning the StreamingResponse.shared/formatters.py (1)
49-53: Broad exception handling silently swallows errors.Catching all exceptions makes debugging difficult. Consider catching
AttributeErrorspecifically (for missing.strftime), or validate thatstart/endare datetime-like before calling.♻️ Suggested improvement
- try: - start_str = start.strftime("%Y%m%dT%H%M%SZ") - end_str = end.strftime("%Y%m%dT%H%M%SZ") - except Exception: - start_str = end_str = "" + start_str = start.strftime("%Y%m%dT%H%M%SZ") if hasattr(start, "strftime") else "" + end_str = end.strftime("%Y%m%dT%H%M%SZ") if hasattr(end, "strftime") else ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shared/formatters.py` around lines 49 - 53, Replace the broad try/except around the strftime calls so it doesn't swallow all errors: validate that start and end are datetime-like (e.g., isinstance(start, datetime.datetime) and isinstance(end, datetime.datetime)) before calling start.strftime/end.strftime, or if you prefer catch only AttributeError/TypeError around those calls; ensure you still set start_str and end_str to "" on invalid inputs but do not catch Exception generically. Update the logic in shared/formatters.py where start_str and end_str are created to use these narrower checks or specific exception types.tests/unit/test_config.py (1)
6-25: LGTM!Good coverage of
AppSettingsdefaults, environment detection, and CORS configuration. The tests are clear and focused.One consideration: These tests may be affected by a
.envfile present in the test environment. If flakiness occurs, consider usingmonkeypatchto clear environment variables before instantiatingAppSettings().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/test_config.py` around lines 6 - 25, Tests for AppSettings (test_defaults, test_is_production, test_is_not_production, test_cors_defaults) can be flaky if a .env or environment variables are present; update each test to isolate environment by clearing relevant env vars before creating AppSettings (use pytest's monkeypatch to unset or set a minimal clean env), e.g., ensure APP_NAME/ENV/PORT/CORS_* (or all os.environ) do not influence construction of AppSettings so instantiation of AppSettings() is deterministic in test_defaults and related tests.tests/integration/conftest.py (1)
13-18: LGTM!The fixture correctly uses the context manager pattern with
TestClient, ensuring proper setup and teardown. Thecreate_appfactory pattern enables isolated testing.Consider adding a return type hint for better IDE support:
`@pytest.fixture` def client() -> Generator[TestClient, None, None]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/conftest.py` around lines 13 - 18, Add a return type hint to the pytest fixture "client" to improve IDE/type-checker support: annotate the function signature of client (which yields a TestClient created via create_app() and TestClient context manager) with the appropriate typing.Generic alias (Generator[TestClient, None, None]) and import the necessary types (Generator and TestClient) at the top of tests/integration/conftest.py so the fixture signature is typed without changing its behavior.schemas/dto/requests/qr.py (1)
28-44: Consider extracting shared validators to reduce duplication.Both
ClassicQRRequestandGradientQRRequestshare identicalvalidate_sizeandvalidate_text_or_formatmethods. This violates DRY and makes maintenance harder.Additionally, the size validation duplicates what
Field(ge=10, le=1000)would provide automatically.♻️ Suggested approach using a base class
class BaseQRRequest(BaseModel): model_config = ConfigDict(populate_by_name=True) text: Optional[str] = None size: Optional[int] = Field(None, ge=10, le=1000) format: Optional[DataFormat] = None formattings: Optional[str] = None `@model_validator`(mode="after") def validate_text_or_format(self) -> "BaseQRRequest": if not self.text and not self.format: raise ValueError("Text parameter is missing") if self.format and not self.formattings: raise ValueError("Formattings parameter is missing") return self class ClassicQRRequest(BaseQRRequest): """Request body/query for the classic (solid-fill) QR code endpoint.""" fill: str = "black" back: str = "white" class GradientQRRequest(BaseQRRequest): """Request body/query for the gradient QR code endpoint.""" gradient1: str = "(106,26,76)" gradient2: str = "(64,53,60)" back: str = "(255, 255, 255)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@schemas/dto/requests/qr.py` around lines 28 - 44, Both ClassicQRRequest and GradientQRRequest duplicate the same size and cross-field validators; extract them into a shared BaseQRRequest to follow DRY and use Pydantic field constraints for size. Create BaseQRRequest (subclassing BaseModel) defining text, size: Optional[int] = Field(None, ge=10, le=1000), format, and formattings, move the model_validator validate_text_or_format there (rename to validate_text_or_format if needed), and remove validate_size and validate_text_or_format from ClassicQRRequest and GradientQRRequest so they inherit the shared validation logic.templates/index.html (1)
28-28: Pin CDN dependency version to avoid unexpected breakages.Using
@latestmeans the icon library could change unexpectedly, potentially breaking styles or introducing visual inconsistencies in production.♻️ Suggested fix
- <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@tabler/icons-webfont@latest/dist/tabler-icons.min.css" /> + <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@tabler/icons-webfont@3.40.0/dist/tabler-icons.min.css" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@templates/index.html` at line 28, The CDN stylesheet link using "@latest" in the <link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@tabler/icons-webfont@latest/dist/tabler-icons.min.css" /> should be pinned to a specific, tested version to avoid unexpected breakages; update that href to use an exact semver (for example replace "@latest" with a fixed version like "@1.XX.X") and commit the chosen version so the icon library remains stable (locate the tag by the stylesheet link / tabler-icons reference in templates/index.html and replace the token).docker-compose.yml (2)
5-5: Consider removing--reloadfor production usage.The
--reloadflag is appropriate for development but should be disabled in production as it adds overhead and can cause unexpected restarts. Consider using a separate compose file or environment variable to control this.♻️ Suggested approach for environment-specific configuration
- command: uv run uvicorn main:app --host 0.0.0.0 --port 8080 --no-access-log --reload + command: uv run uvicorn main:app --host 0.0.0.0 --port 8080 --no-access-logOr use a docker-compose.override.yml for development with
--reload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` at line 5, The docker-compose service command currently includes the development-only flag `--reload` (seen in the `command: uv run uvicorn main:app --host 0.0.0.0 --port 8080 --no-access-log --reload` line); remove `--reload` for production and instead make reload conditional via an environment variable or use a docker-compose.override.yml for development so the production compose file runs `uvicorn` without `--reload`; adjust the service `command` or compose config to inject the flag only when a DEV/RELOAD env var is set or provided by the override file.
1-11: Consider adding a health check for container orchestration.Adding a health check improves container reliability in orchestrated environments.
♻️ Suggested health check addition
services: app: build: . container_name: spoo_qr command: uv run uvicorn main:app --host 0.0.0.0 --port 8080 --no-access-log --reload volumes: - .:/app - /app/.venv ports: - "8080:8080" env_file: .env + healthcheck: + test: ["CMD", "curl", "-f", "http://localhost:8080/"] + interval: 30s + timeout: 10s + retries: 3🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yml` around lines 1 - 11, Add a Docker healthcheck for the app service (container_name spoo_qr) to enable orchestration to detect unhealthy containers; update the docker-compose service block for app to include a healthcheck that probes the running uvicorn process (use the same host/port as the command, e.g. an HTTP GET against http://localhost:8080/health or /docs) and configure sensible options (test, interval, timeout, retries, start_period) so the orchestrator can mark and restart unhealthy containers. Ensure the healthcheck name/endpoint you choose matches your application (or add a simple /health endpoint to the app if missing) and keep settings conservative (e.g., interval 30s, timeout 10s, retries 3, start_period 10s).requirements.txt (1)
1-54: Consider separating runtime and dev dependencies.The requirements.txt mixes runtime dependencies (fastapi, uvicorn, qrcode) with development/testing dependencies (pytest, ruff, coverage). Consider splitting these for cleaner production deployments.
♻️ Suggested structure
Split into:
requirements.txt- runtime dependencies onlyrequirements-dev.txt- development and testing dependencies (can include-r requirements.txt)This allows production deployments to install only necessary packages.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@requirements.txt` around lines 1 - 54, Split the mixed dependencies into runtime and dev files: move testing/linting/build packages (pytest, pytest-*, ruff, coverage, pytest-cov, pytest-mock, pytest-asyncio, pytest-clarity, pytest-randomly, pytest-sugar, pytest-xdist, tox if present, etc.) and developer tools (ruff, coverage, pytest-*, maybe watchfiles if only for dev) into a new requirements-dev.txt that begins with "-r requirements.txt" (or vice versa), and trim requirements.txt to only runtime packages (fastapi, uvicorn, qrcode, pillow, pydantic, pydantic-core, pydantic-settings, httpx, httpcore, starlette, websockets, uvloop, h11, http-tools/httptools if used at runtime, typing-extensions, etc.); update CI/deploy docs to install the correct file for production vs development.pyproject.toml (1)
43-51: Restore a meaningful coverage gate.
fail_under = 0makes coverage report-only, so future regressions won't fail CI even though this PR adds a real test harness.♻️ Suggested change
-fail_under = 0 +fail_under = 80 # or the team's current baseline🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` around lines 43 - 51, The coverage gate was set to disable enforcement (fail_under = 0); restore a meaningful threshold by updating the [tool.coverage.report] setting: replace fail_under = 0 with a sensible percentage (e.g., fail_under = 80 or your project standard) so CI will fail on regressions, and ensure the chosen value reflects current baseline tests and any planned test additions.app.py (1)
31-32: Make the production docs redirect opt-in.Right now any deployment with
is_production=Truealways sends/docsto the hosted docs URL. That works for the main service, but it breaks self-hosted/staging instances that should keep their own local docs surface. This should come fromAppSettingsand only redirect when explicitly configured.♻️ Suggested change
-_DOCS_URL = "https://spoo.me/docs/qr/introduction" +_DEFAULT_DOCS_URL = "https://spoo.me/docs/qr/introduction" ... - if _is_prod: - return RedirectResponse(_DOCS_URL) + external_docs_url = getattr(settings, "external_docs_url", None) + if _is_prod and external_docs_url: + return RedirectResponse(external_docs_url)Also applies to: 61-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app.py` around lines 31 - 32, Current behavior always redirects /docs to _DOCS_URL when is_production is true; change it to be opt-in from AppSettings. Add a boolean setting (e.g., AppSettings.redirect_docs or docs_redirect) and update the code paths that currently check is_production (referencing _DOCS_URL and the /docs redirect logic) to instead check that AppSettings.docs_redirect is true before performing the redirect; leave local docs served when the flag is false. Apply the same change to the other redirect block referenced (lines around 61-64) so both places use the new AppSettings flag rather than is_production.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env.example:
- Line 4: APP_NAME value in the .env example contains spaces and must be quoted
for reliable dotenv parsing; update the APP_NAME entry (symbol: APP_NAME) to
wrap the value in quotes (e.g., "QR Code Generator API") so all dotenv
implementations parse it consistently.
In `@middleware/error_handler.py`:
- Around line 91-96: The current unhandled-exception logging uses log.error(...)
which doesn't emit a traceback; change that call to log.exception(...) (or call
log.error(..., exc_info=True)) so the exception traceback is captured by the
filter_exceptions() processor; update the log invocation where the unhandled 500
is logged (the log.error("unhandled_exception", error=str(exc),
error_type=type(exc).__name__, path=request.url.path)) to use log.exception and
keep contextual fields (error_type and path) while removing or leaving
error=str(exc) since log.exception will include the full exception and
stacktrace.
In `@README.md`:
- Around line 138-140: The README quick-start uses "git clone
https://github.com/spoo-me/qr.git" but then runs "cd qrcode-api", causing a
mismatch; update the two "cd qrcode-api" instructions to "cd qr" (or
alternatively change the clone target to the qrcode-api repo name) so the
checkout and subsequent install steps use the same directory name; ensure you
update both occurrences referenced in the diff.
- Around line 36-54: README API docs are out-of-date: update all endpoint paths
and parameter names to match the refactored server (replace `/api-docs` with
`/docs`, remove separate `/api/v1/classic/logo` and `/api/v1/gradient/logo`
entries and document that logo is uploaded via POST to `/api/v1/classic` and
`/api/v1/gradient`), and rename parameters in examples and tables from `text`,
`fill`, `back`, `module_style`, `output_format`, `gradient1`, `gradient2`,
`gradient_type` to the current `content`, `color`, `background`, `style`,
`output`, `start`, `end`, `direction` (also update curl examples and any sample
payloads referencing `classic`/`gradient` and `batch` endpoints to use these new
names and `/docs` for Swagger UI).
- Around line 1-11: Replace the nonstandard <image> tag with standard Markdown
image syntax for the banner (e.g.,
); fix the top-nav
fragment links that currently point to "#-features", "#-endpoints",
"#-getting-started", "#-contributing" so they match the actual heading IDs
(e.g., "#features", "#endpoints", "#getting-started", "#contributing"); and
update the API Docs anchor target from "https://qr.spoo.me/api-docs" to the
FastAPI-exposed docs path (e.g., "https://qr.spoo.me/docs" or "/docs") so the
link resolves correctly.
In `@routes/api_v1/gradient.py`:
- Line 96: The code naively does logo_bytes = await logo.read() which can OOM;
add a MAX_LOGO_SIZE constant (e.g., MAX_LOGO_SIZE = 2 * 1024 * 1024) and
validate the uploaded file size before reading: if the UploadFile exposes
content_length use that, otherwise check the underlying file object size via
logo.file.seek(0, 2); size = logo.file.tell(); logo.file.seek(0) and if size >
MAX_LOGO_SIZE raise an HTTPException(413) (or return an error); alternatively
read the upload in controlled chunks up to MAX_LOGO_SIZE and error if exceeded;
update the code around the logo handling/ logo_bytes assignment in
routes/api_v1/gradient.py to perform this check and only then read the file into
memory.
In `@services/qr_service.py`:
- Around line 152-169: The batch builder currently constructs ModuleStyle and
OutputFormat directly which can raise ValueError; add a static helper like
_parse_enum(enum_cls, value, field_name) that converts or raises ValidationError
with a clear field message, then use it when building tasks (replace
ModuleStyle(item.get(...)) and OutputFormat(item.get(...)) with calls to
_parse_enum(ModuleStyle, ...) and _parse_enum(OutputFormat, ...)), including the
item index in the field_name (e.g. "items[<i>].style") so invalid enum values
produce a ValidationError; also ensure the comprehension or its surrounding code
catches and re-raises ValidationError so the existing asyncio.gather except
block handles it.
In `@shared/formatters.py`:
- Around line 33-36: The vCard string construction appends the website without a
trailing newline so the subsequent "END:VCARD" runs onto the URL; fix the string
concatenation that builds data when website is truthy (the block that checks
website and appends "URL;TYPE=Homepage:{website}") to ensure a newline is
appended (e.g., add "\n" after the website or prepend "\n" to the "END:VCARD"
append) so the URL line and END:VCARD appear on separate lines.
- Around line 106-110: The format_tel function appends the country code to the
end of the number instead of prepending it; update format_tel so that when
number does not start with '+' it prepends '+1' (not appends) before returning
f"tel:{number}", and ensure you check for and preserve an existing leading '+'
to avoid duplicating the sign.
- Around line 94-98: The format_sms function currently appends "+1" when a plus
is missing, producing invalid numbers; update format_sms so that when "+" not in
the phone string it prepends "+1" (e.g., phone = "+1" + phone) and then returns
the URI using that corrected phone value; keep the function name format_sms and
its return format "sms:{phone}:{message}" unchanged.
In `@shared/logging.py`:
- Around line 44-50: The hash_ip function uses a deterministic unsalted SHA-256
which is vulnerable to brute-force; replace it with an HMAC using a secret
pepper from your config/env (e.g., APP_SECRET or LOG_PEPPER) when IS_PRODUCTION
is true, so hash_ip(ip_address) returns hmac_sha256(pepper, ip_address)[:16]
(and keep returning None for None); ensure the secret is loaded securely,
rotateable, and documented, and add a unit test for hash_ip to verify
deterministic salted output and fallback behavior when not in production or when
the secret is missing.
- Around line 29-38: REDACTED_FIELDS contains "Authorization" and "Cookie" with
uppercase letters but redact_sensitive_fields() lowercases keys before checking,
so header names like "authorization" and "cookie" bypass redaction; fix by
normalizing REDACTED_FIELDS to the same case used in redact_sensitive_fields()
(e.g., store all keys lowercased) or by lowercasing members at lookup time so
membership checks on REDACTED_FIELDS match redact_sensitive_fields() behavior;
update REDACTED_FIELDS (and any other similar sets referenced around the same
file) to use lowercase entries (or apply .lower() in redact_sensitive_fields())
to ensure consistent redaction.
In `@static/css/index-style.css`:
- Around line 360-364: Remove the unnecessary quotes around the font family name
in the .color-label rule: update the font-family declaration in the .color-label
CSS (the font-family property) so that Consolas is unquoted to satisfy
Stylelint's font-family-name-quotes rule while keeping the other font names and
order intact.
In `@static/js/index-script.js`:
- Around line 120-135: The fetch logic that sets preview state and
currentFileName can suffer from race conditions; modify the handler that calls
setPreviewState and issues fetches (the block using useLogo, FormData,
fetch(basePath + "?" + params.toString()), handleResponse, handleError and the
assignment to currentFileName) to ignore stale responses by either (a) tracking
an incrementing requestId (store it in a module-scoped variable) and passing it
into handleResponse/handleError so they only apply results when the id matches
the latest, or (b) create and store an AbortController for the in-flight fetch
and abort it before starting a new fetch; ensure that only the latest successful
response updates preview, revokes or replaces blob URLs, and sets
currentFileName (and apply the same change to the corresponding fetch block
around lines 151-177).
- Around line 101-129: The code currently builds a URLSearchParams named params
and appends content and render options to the query string (see params, content,
sizeInput, and other params usage) and calls fetch(basePath + "?" +
params.toString()), which exposes user data and can break on long payloads;
instead, move the QR payload and render options into the request body: for the
non-logo flow construct a JSON body with content, style, output, size,
color/gradient fields and call fetch(basePath, { method: "POST", headers:
{'Content-Type':'application/json'}, body: JSON.stringify(...) }) and for the
logo flow append those same fields to the existing FormData
(formData.append(...)) before fetch(basePath, { method: "POST", body: formData
}); keep uses of useLogo, formData, handleResponse and handleError unchanged.
- Around line 158-170: The code currently injects fetched SVG text into the DOM
via output.innerHTML inside the contentType.includes("svg") branch, creating an
XSS sink; instead, create a safe preview by making an object URL from the blob
(URL.createObjectURL(blob)) and set that URL as the src of an <img> (or
<object>) element you append to output, set its sizing (maxWidth/width/height)
and call URL.revokeObjectURL when appropriate, then call
setPreviewState("result"); alternatively, if you must inline the SVG, sanitize
svgText with a trusted sanitizer (e.g., DOMPurify) before assigning to
output.innerHTML — update the logic around blob, output, and setPreviewState
accordingly.
---
Nitpick comments:
In @.github/workflows/github-ci.yaml:
- Around line 37-43: Remove the redundant venv setup and dependency install
steps that precede the isolated linter run: delete the "Install Dependencies"
block that runs "uv venv" and "uv pip install --group dev" since "uvx ruff
check" runs Ruff in an ephemeral environment; keep only the "Lint with Ruff"
step invoking "uvx ruff check" (or conditionally install deps only when later
steps require the venv). Target the CI steps named "Install Dependencies" and
the command lines "uv venv" / "uv pip install --group dev" and the "Lint with
Ruff" step invoking "uvx ruff check".
In `@app.py`:
- Around line 31-32: Current behavior always redirects /docs to _DOCS_URL when
is_production is true; change it to be opt-in from AppSettings. Add a boolean
setting (e.g., AppSettings.redirect_docs or docs_redirect) and update the code
paths that currently check is_production (referencing _DOCS_URL and the /docs
redirect logic) to instead check that AppSettings.docs_redirect is true before
performing the redirect; leave local docs served when the flag is false. Apply
the same change to the other redirect block referenced (lines around 61-64) so
both places use the new AppSettings flag rather than is_production.
In `@config.py`:
- Around line 22-23: The cors_origins setting currently allows all origins via
the variable cors_origins: list[str] = ["*"]; change it to read and parse an
environment variable (e.g., CORS_ORIGINS) and only default to ["*"] in
development—parse a comma-separated string into list[str] and validate entries
before assigning to cors_origins so production must explicitly supply allowed
origins; update any config-loading function or module-level initialization where
cors_origins is defined to perform this env read/parse/validation.
In `@docker-compose.yml`:
- Line 5: The docker-compose service command currently includes the
development-only flag `--reload` (seen in the `command: uv run uvicorn main:app
--host 0.0.0.0 --port 8080 --no-access-log --reload` line); remove `--reload`
for production and instead make reload conditional via an environment variable
or use a docker-compose.override.yml for development so the production compose
file runs `uvicorn` without `--reload`; adjust the service `command` or compose
config to inject the flag only when a DEV/RELOAD env var is set or provided by
the override file.
- Around line 1-11: Add a Docker healthcheck for the app service (container_name
spoo_qr) to enable orchestration to detect unhealthy containers; update the
docker-compose service block for app to include a healthcheck that probes the
running uvicorn process (use the same host/port as the command, e.g. an HTTP GET
against http://localhost:8080/health or /docs) and configure sensible options
(test, interval, timeout, retries, start_period) so the orchestrator can mark
and restart unhealthy containers. Ensure the healthcheck name/endpoint you
choose matches your application (or add a simple /health endpoint to the app if
missing) and keep settings conservative (e.g., interval 30s, timeout 10s,
retries 3, start_period 10s).
In `@dockerfile`:
- Line 4: The Dockerfile currently pulls the uv image with the floating tag
"latest" in the COPY --from=ghcr.io/astral-sh/uv:latest /uv /uvx /bin/ step
which prevents reproducible builds; change the image reference to the fixed
release tag (e.g., ghcr.io/astral-sh/uv:0.10.11) so the COPY --from=... line
uses the pinned version and repeatable artifact.
- Around line 13-15: The Dockerfile currently runs the app as root via the CMD
instruction; add a non-root user and switch to it before starting the app:
create a dedicated user and group (e.g., appuser), set a proper WORKDIR, chown
the app files/directories to that user, and add a USER appuser directive prior
to the CMD line so the uvicorn process runs unprivileged; ensure any ports/files
the app needs have appropriate ownership/permissions for that user.
In `@pyproject.toml`:
- Around line 43-51: The coverage gate was set to disable enforcement
(fail_under = 0); restore a meaningful threshold by updating the
[tool.coverage.report] setting: replace fail_under = 0 with a sensible
percentage (e.g., fail_under = 80 or your project standard) so CI will fail on
regressions, and ensure the chosen value reflects current baseline tests and any
planned test additions.
In `@requirements.txt`:
- Around line 1-54: Split the mixed dependencies into runtime and dev files:
move testing/linting/build packages (pytest, pytest-*, ruff, coverage,
pytest-cov, pytest-mock, pytest-asyncio, pytest-clarity, pytest-randomly,
pytest-sugar, pytest-xdist, tox if present, etc.) and developer tools (ruff,
coverage, pytest-*, maybe watchfiles if only for dev) into a new
requirements-dev.txt that begins with "-r requirements.txt" (or vice versa), and
trim requirements.txt to only runtime packages (fastapi, uvicorn, qrcode,
pillow, pydantic, pydantic-core, pydantic-settings, httpx, httpcore, starlette,
websockets, uvloop, h11, http-tools/httptools if used at runtime,
typing-extensions, etc.); update CI/deploy docs to install the correct file for
production vs development.
In `@routes/api_v1/batch.py`:
- Around line 50-57: The 20-item limit is duplicated in the Pydantic validator
validate_items (on class containing BatchItem) and in QRService.generate_batch;
extract that magic number into a single shared constant (e.g., BATCH_MAX_SIZE)
in a common module or at top of this module, update validate_items to compare
against BATCH_MAX_SIZE and update QRService.generate_batch to reference the same
BATCH_MAX_SIZE (importing it if moved to a shared module), and keep the existing
error messages but use the constant so the limit cannot drift between the two
places.
In `@routes/api_v1/classic.py`:
- Around line 76-91: The UploadFile received as logo is read but not closed;
after reading logo.read() in the route that calls qr_service.generate_classic
(where logo_bytes is created), ensure you explicitly close the UploadFile (await
logo.close()) in the same function (the route handling code using the logo:
Optional[UploadFile] parameter) to release the underlying temp file/resource
promptly, doing so after you obtain logo_bytes and before returning the
StreamingResponse.
In `@routes/api_v1/gradient.py`:
- Around line 45-47: The route currently accepts the full OutputFormat enum via
the output parameter but generate_gradient only supports PNG; update the
endpoint to prevent SVG by either narrowing the parameter type to PNG-only
(e.g., replace the parameter type with a Literal or a new PNG-only enum) or
adding an explicit validation check in generate_gradient that returns a 400 HTTP
error for non-PNG values; update the Query description on the output parameter
to state "PNG only" and reference the output parameter and generate_gradient
function so callers and maintainers see the restriction.
In `@routes/page_routes.py`:
- Around line 19-21: The handler index is annotated with HTMLResponse but
returns a TemplateResponse; update the type annotation to TemplateResponse (or
the more generic Response) to match the actual return type. Modify the async def
index(request: Request) -> HTMLResponse signature to use TemplateResponse (or
fastapi.Response) and keep the body returning
templates.TemplateResponse(request, "index.html") so the declared return type
matches the real return value.
In `@schemas/dto/requests/qr.py`:
- Around line 28-44: Both ClassicQRRequest and GradientQRRequest duplicate the
same size and cross-field validators; extract them into a shared BaseQRRequest
to follow DRY and use Pydantic field constraints for size. Create BaseQRRequest
(subclassing BaseModel) defining text, size: Optional[int] = Field(None, ge=10,
le=1000), format, and formattings, move the model_validator
validate_text_or_format there (rename to validate_text_or_format if needed), and
remove validate_size and validate_text_or_format from ClassicQRRequest and
GradientQRRequest so they inherit the shared validation logic.
In `@shared/color.py`:
- Around line 64-65: The except block that catches (ValueError, TypeError) and
raises ValidationError currently discards the original exception; update the
handler in the function that parses colors so it re-raises the ValidationError
with exception chaining (use "raise ValidationError(f\"Invalid color format:
{color_str}\", field=\"color\") from e") where e is the caught exception, so the
original traceback is preserved for debugging; reference the caught exception
variable (e) and the ValidationError class in the change.
- Around line 29-65: The type checker complains because returns like
tuple(int(... ) for ...) and tuple(values) are inferred as variable-length
tuples; in parse_color replace generator/list-to-tuple returns with explicit
3-element tuples so the signature tuple[int,int,int] is honored: e.g. in the
"#..." and bare hex branches return (int(hex_str[0:2],16), int(hex_str[2:4],16),
int(hex_str[4:6],16)), and in the "rgb(...)" and "(...)" branches return
(values[0], values[1], values[2]) instead of tuple(values); keep the
ValidationError handling unchanged.
In `@shared/formatters.py`:
- Around line 49-53: Replace the broad try/except around the strftime calls so
it doesn't swallow all errors: validate that start and end are datetime-like
(e.g., isinstance(start, datetime.datetime) and isinstance(end,
datetime.datetime)) before calling start.strftime/end.strftime, or if you prefer
catch only AttributeError/TypeError around those calls; ensure you still set
start_str and end_str to "" on invalid inputs but do not catch Exception
generically. Update the logic in shared/formatters.py where start_str and
end_str are created to use these narrower checks or specific exception types.
In `@static/js/index-script.js`:
- Around line 18-63: Call the existing sync routines once at startup so the form
reflects any restored/non-default values: after wiring listeners, invoke
autoResize() (to size qrInput), loop the colorInputs array and set each
corresponding label.textContent = input.value (to sync color labels), and call
the same type change handler logic for typeSelect (or dispatch a "change" on
typeSelect) to toggle classicColors/gradientColors and enable/disable the svg
option and adjust outputSelect.value if needed; reference functions/variables
autoResize, qrInput, colorInputs, typeSelect, classicColors, gradientColors, and
outputSelect to locate where to add these startup calls.
In `@templates/index.html`:
- Line 28: The CDN stylesheet link using "@latest" in the <link rel="stylesheet"
href="https://cdn.jsdelivr.net/npm/@tabler/icons-webfont@latest/dist/tabler-icons.min.css"
/> should be pinned to a specific, tested version to avoid unexpected breakages;
update that href to use an exact semver (for example replace "@latest" with a
fixed version like "@1.XX.X") and commit the chosen version so the icon library
remains stable (locate the tag by the stylesheet link / tabler-icons reference
in templates/index.html and replace the token).
In `@tests/integration/conftest.py`:
- Around line 13-18: Add a return type hint to the pytest fixture "client" to
improve IDE/type-checker support: annotate the function signature of client
(which yields a TestClient created via create_app() and TestClient context
manager) with the appropriate typing.Generic alias (Generator[TestClient, None,
None]) and import the necessary types (Generator and TestClient) at the top of
tests/integration/conftest.py so the fixture signature is typed without changing
its behavior.
In `@tests/unit/test_config.py`:
- Around line 6-25: Tests for AppSettings (test_defaults, test_is_production,
test_is_not_production, test_cors_defaults) can be flaky if a .env or
environment variables are present; update each test to isolate environment by
clearing relevant env vars before creating AppSettings (use pytest's monkeypatch
to unset or set a minimal clean env), e.g., ensure APP_NAME/ENV/PORT/CORS_* (or
all os.environ) do not influence construction of AppSettings so instantiation of
AppSettings() is deterministic in test_defaults and related tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 46185755-fc59-43a5-a25e-1a6a34272982
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (63)
.dockerignore.env.example.github/dependabot.yml.github/workflows/format.yaml.github/workflows/github-ci.yaml.github/workflows/tests.yaml.gitignoreREADME.mdapp.pyconfig.pydependencies/__init__.pydependencies/services.pydocker-compose.ymldockerfileerrors.pymain.pymiddleware/__init__.pymiddleware/error_handler.pymiddleware/logging.pymiddleware/openapi.pypyproject.tomlrequirements.txtroutes/__init__.pyroutes/api_v1/__init__.pyroutes/api_v1/batch.pyroutes/api_v1/classic.pyroutes/api_v1/gradient.pyroutes/health_routes.pyroutes/page_routes.pyschemas/__init__.pyschemas/dto/__init__.pyschemas/dto/requests/__init__.pyschemas/dto/requests/qr.pyschemas/dto/responses/__init__.pyschemas/dto/responses/common.pyschemas/enums.pyservices/__init__.pyservices/qr_service.pyshared/__init__.pyshared/color.pyshared/formatters.pyshared/ip_utils.pyshared/logging.pyshared/qr_utils.pystatic/css/index-style.cssstatic/js/index-script.jstemplates/index.htmltests/__init__.pytests/conftest.pytests/integration/__init__.pytests/integration/conftest.pytests/integration/test_health.pytests/integration/test_pages.pytests/integration/test_qr_endpoints.pytests/unit/__init__.pytests/unit/conftest.pytests/unit/test_color.pytests/unit/test_config.pytests/unit/test_errors.pytests/unit/test_formatters.pytests/unit/test_qr_service.pytests/unit/test_qr_utils.pyutils.py
💤 Files with no reviewable changes (2)
- .github/workflows/format.yaml
- utils.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
middleware/openapi.py (1)
23-35: Avoid hardcoding the Base URL inAPI_DESCRIPTION.The description can drift from
app_urlin non-production environments. Keep base URL dynamic (or rely only onservers) so docs stay accurate across deployments.Proposed refactor
API_DESCRIPTION = ( "Open-source QR code generator API with support for classic solid-fill, " "gradient, and custom-styled QR codes.\n\n" "**Features:**\n" "- Classic QR codes with customizable fill and background colors\n" "- Gradient QR codes with vertical gradient coloring\n" "- Multiple module drawer styles (rounded, circle, bars, gapped)\n" "- Multiple gradient types (vertical, horizontal, radial, square)\n" "- SVG and PNG output formats\n" "- Logo/image embedding in QR codes\n" - "- Batch QR code generation\n" - "**Base URL:** `https://qr.spoo.me`" + "- Batch QR code generation" ) @@ openapi_schema = get_openapi( title=app.title, version=app.version, - description=app.description, + description=f"{app.description}\n\n**Base URL:** `{app_url}`", routes=app.routes, tags=app.openapi_tags, contact=app.contact, license_info=app.license_info, )Also applies to: 60-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@middleware/openapi.py` around lines 23 - 35, API_DESCRIPTION currently hardcodes the Base URL string (in the API_DESCRIPTION constant) which can drift from runtime app_url; remove the hardcoded "`https://qr.spoo.me`" fragment from API_DESCRIPTION and instead construct the base URL dynamically or rely on the OpenAPI servers configuration: update the API_DESCRIPTION constant to omit the Base URL note and, where needed, inject app_url (or reference the servers list) at runtime—search for API_DESCRIPTION in middleware/openapi.py and any use sites around the app_url and servers variables (also check the nearby block around lines 60-70) and replace the static text with a dynamic insertion or remove it entirely so docs reflect the deployed app_url.schemas/dto/requests/qr.py (2)
24-32: Deduplicatesizevalidation to avoid rule drift.Line 24-Line 32 and Line 46-Line 54 implement identical validation logic. Extract a shared validator/helper (or a shared base model) so size rules stay consistent in one place.
♻️ Proposed refactor
+def _validate_qr_size(v: Optional[int]) -> Optional[int]: + if v is not None: + if v > 1000: + raise ValueError("Size is too large") + if v < 10: + raise ValueError("Size is too small") + return v + class ClassicQRRequest(BaseModel): @@ `@field_validator`("size") `@classmethod` def validate_size(cls, v: Optional[int]) -> Optional[int]: - if v is not None: - if v > 1000: - raise ValueError("Size is too large") - if v < 10: - raise ValueError("Size is too small") - return v + return _validate_qr_size(v) @@ class GradientQRRequest(BaseModel): @@ `@field_validator`("size") `@classmethod` def validate_size(cls, v: Optional[int]) -> Optional[int]: - if v is not None: - if v > 1000: - raise ValueError("Size is too large") - if v < 10: - raise ValueError("Size is too small") - return v + return _validate_qr_size(v)Also applies to: 46-54
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@schemas/dto/requests/qr.py` around lines 24 - 32, The size validation logic is duplicated in the two `@field_validator` implementations (validate_size); extract the rule into a single shared helper or base-model validator and have both validators delegate to it. Create a function like validate_size_value(value: Optional[int]) -> Optional[int] (or a SizeValidatedBaseModel with a single `@field_validator` for "size") and replace the inline checks in validate_size (and the other identical validator) with a call to that shared helper so the min/max (10/1000) rule is defined in one place.
17-17: Remove redundantpopulate_by_name=Trueconfiguration from both models.Lines 17 and 38 set
populate_by_name=TrueinConfigDict, but neitherClassicQRRequestnorGradientQRRequestdefines field aliases (viaalias,validation_alias, orserialization_alias). This configuration has no effect and adds unnecessary complexity. Remove it until aliases are actually introduced.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@schemas/dto/requests/qr.py` at line 17, Both ClassicQRRequest and GradientQRRequest set model_config = ConfigDict(populate_by_name=True) even though neither model defines any field aliases; remove the redundant populate_by_name=True setting from the model_config in both classes (i.e., edit the model_config assignments in ClassicQRRequest and GradientQRRequest to omit populate_by_name) so the ConfigDict is only configured with relevant options until aliases are introduced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@middleware/openapi.py`:
- Line 4: Update the inaccurate docstring in middleware/openapi.py to refer to
the current docs UI/path (Scalar at /docs) instead of Swagger at /api-docs; edit
the top-level module or the configure_openapi (or equivalent initializer)
docstring to say "Called once during app creation to configure the Scalar docs
UI at /docs" so it matches the app wiring and avoids confusion.
---
Nitpick comments:
In `@middleware/openapi.py`:
- Around line 23-35: API_DESCRIPTION currently hardcodes the Base URL string (in
the API_DESCRIPTION constant) which can drift from runtime app_url; remove the
hardcoded "`https://qr.spoo.me`" fragment from API_DESCRIPTION and instead
construct the base URL dynamically or rely on the OpenAPI servers configuration:
update the API_DESCRIPTION constant to omit the Base URL note and, where needed,
inject app_url (or reference the servers list) at runtime—search for
API_DESCRIPTION in middleware/openapi.py and any use sites around the app_url
and servers variables (also check the nearby block around lines 60-70) and
replace the static text with a dynamic insertion or remove it entirely so docs
reflect the deployed app_url.
In `@schemas/dto/requests/qr.py`:
- Around line 24-32: The size validation logic is duplicated in the two
`@field_validator` implementations (validate_size); extract the rule into a single
shared helper or base-model validator and have both validators delegate to it.
Create a function like validate_size_value(value: Optional[int]) ->
Optional[int] (or a SizeValidatedBaseModel with a single `@field_validator` for
"size") and replace the inline checks in validate_size (and the other identical
validator) with a call to that shared helper so the min/max (10/1000) rule is
defined in one place.
- Line 17: Both ClassicQRRequest and GradientQRRequest set model_config =
ConfigDict(populate_by_name=True) even though neither model defines any field
aliases; remove the redundant populate_by_name=True setting from the
model_config in both classes (i.e., edit the model_config assignments in
ClassicQRRequest and GradientQRRequest to omit populate_by_name) so the
ConfigDict is only configured with relevant options until aliases are
introduced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 694f28be-6ae8-4d99-bd8c-ec6f42913693
📒 Files selected for processing (4)
middleware/openapi.pyschemas/dto/requests/qr.pyschemas/enums.pytests/integration/test_qr_endpoints.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integration/test_qr_endpoints.py
- schemas/enums.py
…ng, and enhance color validation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Infrastructure
Tests
Chores