Web App#2
Conversation
- single page with VITE server - conversations navigation and csv upload UI/UX (just front components) - front and back tests - SQLAlchemy + Alembic for ORM and connection to postgres db
- the upload endpoint allow user to upload a list of csv
we store the agent work like generated figure or csv files
persist user, send last 10, stream SSE, persist assistant + artifacts + CsvFile
Replace the stale case-study brief (it described the old CLI layout) with setup instructions for the web app: docker compose up after setting ANTHROPIC_API_KEY in backend/.env, plus a config reference, the no-Docker make workflow, project layout and how a chat turn streams. Also drop the dead ./agent:/app/agent mount from docker-compose — the agent now lives under backend/app/services/agent/, so the root agent/ folder no longer exists and the mount created an empty stray dir in the container. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
WalkthroughThis PR replaces the previous CLI-oriented setup with a Dockerized backend and React frontend, adds database and S3-backed persistence, introduces SSE-based chat and artifact APIs, adds dataset upload and rendering flows, and includes seeding, tooling, and automated tests. ChangesFull-stack web application
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant Agent
participant Storage
User->>Frontend: Send message
Frontend->>Backend: POST /api/conversations/{id}/messages
Backend->>Agent: Run chat with history and datasets
Agent-->>Backend: Stream reasoning, tool, artifact, text events
Backend->>Storage: Persist figure/table payloads
Backend-->>Frontend: SSE event stream
Frontend->>Backend: Reload conversation/messages
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (28)
frontend/src/services/useMessages.ts-34-47 (1)
34-47:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset
loadingin non-fetch branches to prevent a stuck loading state.When switching conversations during an in-flight fetch, the cancelled request skips the old
finallypath, and these early returns do not clearloading, so the UI can stay stuck.🔧 Proposed fix
useEffect(() => { if (conversationId === null) { + setLoading(false); setMessages([]); setTotal(0); setLoadedCount(0); return; } @@ if (conversationId === skipFetchFor) { + setLoading(false); setTotal(0); setLoadedCount(0); return; }Also applies to: 68-74
🤖 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 `@frontend/src/services/useMessages.ts` around lines 34 - 47, The loading state is not being reset in the early return branches of the useMessages hook, causing the UI to remain stuck in a loading state when switching conversations. Specifically, in both the branch where conversationId is null and the branch where conversationId matches skipFetchFor, add a call to setLoading(false) before the return statement to ensure the loading state is properly cleared whenever the fetch flow is skipped.frontend/src/services/api.ts-90-114 (1)
90-114:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle CRLF-delimited SSE frames in the stream parser.
The frame splitter only recognizes
\n\n. With CRLF streams, frames never flush andonEventis never called.🔧 Proposed fix
for (;;) { const { done, value } = await reader.read(); if (done) break; - buffer += decoder.decode(value, { stream: true }); + buffer += decoder.decode(value, { stream: true }).replace(/\r\n/g, "\n"); flushFrames(); } - buffer += decoder.decode(); + buffer += decoder.decode().replace(/\r\n/g, "\n"); flushFrames();🤖 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 `@frontend/src/services/api.ts` around lines 90 - 114, The flushFrames function only checks for Unix-style line ending delimiters (\n\n) when parsing SSE frames, causing frames delimited with CRLF (\r\n\r\n) to never flush and onEvent to never be called. Modify the frame detection logic to handle both CRLF and LF delimiters by either normalizing the buffer to remove carriage returns before checking for \n\n, or updating the indexOf check to detect both \n\n and \r\n\r\n patterns and extract frames accordingly.frontend/src/services/useConversations.ts-17-25 (1)
17-25:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCatch
refreshfailures to avoid unhandled promise rejections.
refresh()can reject, and the current effect only uses.finally(...), so fetch failures leak as unhandled rejections.🔧 Proposed fix
const refresh = useCallback(async () => { - const page = await listConversations({ limit: PAGE_SIZE, offset: 0 }); - setConversations(page.items); - setTotal(page.total); + try { + const page = await listConversations({ limit: PAGE_SIZE, offset: 0 }); + setConversations(page.items); + setTotal(page.total); + } catch { + setConversations([]); + setTotal(0); + } }, []);🤖 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 `@frontend/src/services/useConversations.ts` around lines 17 - 25, The refresh function in the useEffect hook can reject when listConversations fails, but the current implementation only uses .finally() without catching the rejection, leading to unhandled promise rejections. Add a .catch() handler before the .finally() call in the useEffect hook that handles the error appropriately (such as logging it or setting an error state), ensuring all rejection paths are handled before the finally block sets setLoading to false.frontend/src/services/useMessages.ts-80-95 (1)
80-95:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
loadMoreagainst stale responses from a previously selected conversation.If the user switches conversations while
loadMoreis in flight, the old response can still mutatemessages, mixing threads across conversation boundaries.🔧 Proposed fix
-import { useCallback, useEffect, useState } from "react"; +import { useCallback, useEffect, useRef, useState } from "react"; @@ const [loadedCount, setLoadedCount] = useState(0); + const activeConversationRef = useRef<string | null>(conversationId); @@ useEffect(() => { + activeConversationRef.current = conversationId; if (conversationId === null) { @@ const loadMore = useCallback(async () => { if (conversationId === null) return; + const targetConversationId = conversationId; setLoadingMore(true); try { - const page = await listMessages(conversationId, { + const page = await listMessages(targetConversationId, { limit: PAGE_SIZE, offset: loadedCount, }); + if (activeConversationRef.current !== targetConversationId) return; // Older messages go to the top, still chronological within the page. setMessages((prev) => [...[...page.items].reverse(), ...prev]);🤖 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 `@frontend/src/services/useMessages.ts` around lines 80 - 95, The `loadMore` function has a race condition where stale responses from a previously selected conversation can mutate the messages state. To fix this, capture the conversationId value at the start of the async operation (before calling listMessages) and after receiving the response, verify that the captured conversationId still matches the current conversationId before updating state with setMessages, setTotal, and setLoadedCount. This ensures that if the user switches conversations while the request is in flight, the stale response will be discarded instead of mixing messages across different conversations.frontend/src/components/ChatComposer.tsx-33-37 (1)
33-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent Enter-to-send while IME composition is active.
On Line 34, submitting on raw
Enterwithout checking composition state can send unfinished IME text.Proposed fix
- onKeyDown={(e) => { - if (e.key === "Enter" && !e.shiftKey) { + onKeyDown={(e) => { + if (e.key === "Enter" && !e.shiftKey && !e.nativeEvent.isComposing) { e.preventDefault(); submit(); } }}🤖 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 `@frontend/src/components/ChatComposer.tsx` around lines 33 - 37, The onKeyDown handler in the ChatComposer component submits when Enter is pressed without checking if IME composition is currently active, which can send incomplete or unfinished composed text. To fix this, before calling submit() in the Enter key handler, add a check for the composition state by either checking the e.isComposing property on the keyboard event, or by tracking composition state separately using onCompositionStart and onCompositionEnd event handlers to set a flag. Only proceed with the submit() call when composition is not active.frontend/src/components/PlotlyFigure.tsx-20-44 (1)
20-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate parsed figure structure before rendering Plotly.
On Lines 22-44, parse success is treated as structural validity. If
figureJsonis valid JSON but lacksdata/layout, rendering can fail at runtime.Proposed fix
export default function PlotlyFigure({ figureJson }: PlotlyFigureProps) { const fig = useMemo(() => { try { - return JSON.parse(figureJson) as { data: unknown[]; layout: object }; + const parsed: unknown = JSON.parse(figureJson); + if (!parsed || typeof parsed !== "object") return null; + const maybe = parsed as { data?: unknown; layout?: unknown }; + if (!Array.isArray(maybe.data)) return null; + if ( + maybe.layout !== undefined && + (typeof maybe.layout !== "object" || maybe.layout === null) + ) { + return null; + } + return { + data: maybe.data as Plotly.Data[], + layout: (maybe.layout ?? {}) as Partial<Plotly.Layout>, + }; } catch { return null; } }, [figureJson]); @@ <Plot - data={fig.data as Plotly.Data[]} + data={fig.data} layout={{ - ...(fig.layout as Partial<Plotly.Layout>), + ...fig.layout, autosize: true, - margin: { t: 40, r: 16, b: 40, l: 48, ...((fig.layout as { margin?: object })?.margin ?? {}) }, + margin: { t: 40, r: 16, b: 40, l: 48, ...((fig.layout as { margin?: object })?.margin ?? {}) }, }}🤖 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 `@frontend/src/components/PlotlyFigure.tsx` around lines 20 - 44, The useMemo hook parsing figureJson only catches JSON parsing errors but does not validate that the parsed object has the required structure with data and layout properties. Add validation after successful JSON parsing to check that the parsed object contains both a data property (that is an array) and a layout property (that is an object). If either property is missing or has an invalid type, return null from the useMemo hook, similar to how parse errors are handled, to prevent runtime failures in the Plot component.frontend/src/lib/format.ts-7-14 (1)
7-14:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
toLocaleStringinstead oftoLocaleDateStringfor date+time formatting.
toLocaleDateStringis designed for date-only formatting and does not supporthourandminuteoptions. According to the ECMAScript specification, these options are only valid fortoLocaleStringandtoLocaleTimeString. In the current implementation, the time portion may be silently ignored or produce inconsistent results across browsers.🕒 Proposed fix
export function formatDate(iso: string): string { - return new Date(iso).toLocaleDateString(undefined, { + return new Date(iso).toLocaleString(undefined, { month: "short", day: "numeric", hour: "2-digit", minute: "2-digit", }); }🤖 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 `@frontend/src/lib/format.ts` around lines 7 - 14, The formatDate function uses toLocaleDateString with hour and minute options, but toLocaleDateString is designed for date-only formatting and does not support time-related options according to the ECMAScript specification. Replace the toLocaleDateString method call with toLocaleString, which properly supports both date and time formatting options and will ensure consistent behavior across all browsers.frontend/src/App.tsx-16-19 (1)
16-19:⚠️ Potential issue | 🟠 MajorGuard route decoding against malformed URL segments.
decodeURIComponentthrows aURIErrorwhen it encounters invalid%encodings (e.g., percent sign not followed by two hex digits). This will crash component initialization or browser navigation if a user reaches a URL with malformed encoding like/c/%GG.Proposed fix
function conversationIdFromPath(): string | null { const match = window.location.pathname.match(/^\/c\/(.+)$/); - return match ? decodeURIComponent(match[1]) : null; + if (!match) return null; + try { + return decodeURIComponent(match[1]); + } catch { + return null; + } }🤖 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 `@frontend/src/App.tsx` around lines 16 - 19, The conversationIdFromPath function does not handle the URIError that decodeURIComponent throws when encountering invalid percent encodings like %GG, which will crash component initialization. Wrap the decodeURIComponent call in a try-catch block to catch URIError exceptions and return null when the URL segment contains malformed encoding, ensuring graceful degradation instead of application crashes.frontend/src/App.tsx-66-71 (1)
66-71:⚠️ Potential issue | 🟠 MajorReturn the reconcile promise from
onDone—don't discard it withvoid.The
send()function inuseChatStreamawaits theonDonecallback to ensure persisted messages replace the live bubble before it clears. Usingvoid reconcileMessages(id)discards the promise, sosend()won't actually wait for reconciliation to complete, breaking the intended handoff ordering.Proposed fix
const { reply, running, send: streamSend } = useChatStream({ onTitle: (id, title) => renameConversation(id, title), onDone: (id) => { // Replace optimistic messages + the live bubble with the persisted ones // (which carry artifacts). Pass the id explicitly: for a brand-new // conversation the hook's captured id may still be stale. - void reconcileMessages(id); + return reconcileMessages(id); }, });🤖 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 `@frontend/src/App.tsx` around lines 66 - 71, The onDone callback in App.tsx is discarding the promise returned by reconcileMessages(id) using the void operator, which prevents the send() function from awaiting the reconciliation to complete. Remove the void keyword from the reconcileMessages(id) call and return the promise directly so that send() can properly wait for message reconciliation to finish before clearing the live bubble, ensuring the correct handoff ordering.backend/alembic/env.py-22-22 (1)
22-22:⚠️ Potential issue | 🟠 MajorEscape
%in Alembic URL injection to prevent migration startup failures.At Line 22, passing raw
database_urlinto Alembic config fails when credentials contain URL-encoded characters (for example%40), causing ConfigParser interpolation errors before migrations run.Proposed fix
- config.set_main_option("sqlalchemy.url", get_settings().database_url) + db_url = get_settings().database_url + config.set_main_option("sqlalchemy.url", db_url.replace("%", "%%"))Note: An alternative approach is to bypass the Config object entirely by passing the URL directly to
context.configure()in therun_migrations_online()function, which avoids ConfigParser processing altogether and is considered the recommended practice. However, the escaping solution above is a valid immediate fix.🤖 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/alembic/env.py` at line 22, The database_url containing URL-encoded characters like %40 causes ConfigParser interpolation errors when passed to config.set_main_option in the env.py file. Fix this by escaping all percent signs in the database_url before passing it to set_main_option at line 22, converting each % to %% so ConfigParser treats them as literal characters. Alternatively, pass the raw database_url directly to context.configure() inside the run_migrations_online() function instead of using set_main_option, which bypasses ConfigParser processing entirely.backend/app/core/config.py-32-34 (1)
32-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winURL-encode DB credentials in the constructed DSN.
database_urlcurrently interpolates raw username/password. If either contains URI-reserved characters, SQLAlchemy gets an invalid URL and DB connection fails.Suggested patch
+from urllib.parse import quote_plus @@ def database_url(self) -> str: if self.database_url_override: return self.database_url_override + user = quote_plus(self.postgres_user) + password = quote_plus(self.postgres_password) return ( - f"postgresql+asyncpg://{self.postgres_user}:{self.postgres_password}" + f"postgresql+asyncpg://{user}:{password}" f"@{self.postgres_host}:{self.postgres_port}/{self.postgres_db}" )🤖 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/core/config.py` around lines 32 - 34, The database_url property construction directly interpolates self.postgres_user and self.postgres_password into the PostgreSQL connection string without URL-encoding them. If these credentials contain URI-reserved characters (such as @ or :), the resulting URL will be malformed and SQLAlchemy will fail to connect to the database. To fix this, import the appropriate URL encoding utility (such as quote from urllib.parse) and apply it to both self.postgres_user and self.postgres_password before inserting them into the f-string that constructs the database URL.backend/app/api/csv_files.py-101-103 (1)
101-103:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDelete ordering risks dangling DB references on commit failure.
S3 is deleted before the DB transaction commits. If the commit fails, the DB row still exists but points to a missing object.
Safer ordering
- storage.delete_object(csv_file.s3_key) await db.delete(csv_file) await db.commit() + try: + storage.delete_object(csv_file.s3_key) + except Exception: + # log + schedule retry/outbox cleanup + pass🤖 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/api/csv_files.py` around lines 101 - 103, The current deletion order in the csv_file deletion logic creates a risk where the S3 object is deleted before the database transaction is committed. If the commit fails, the S3 object will already be gone while the database row still exists, creating a dangling reference. Reorder the operations so that the database deletion and commit happen first (the await db.delete(csv_file) and await db.commit() calls), and only after the database transaction successfully commits should the S3 object be deleted via storage.delete_object(csv_file.s3_key). This ensures database consistency is guaranteed before any irreversible S3 operations occur.backend/Dockerfile-1-18 (1)
1-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun the backend process as a non-root user.
The container currently runs the app as root, which is an avoidable privilege escalation risk if the service is compromised.
Suggested hardening diff
FROM python:3.12-slim +RUN groupadd --system app && useradd --system --gid app --create-home app # Install uv (fast Python package manager). COPY --from=ghcr.io/astral-sh/uv:latest /uv /uvx /bin/ WORKDIR /app +RUN chown app:app /app # Install dependencies first (cached layer). COPY pyproject.toml ./ RUN uv sync --no-install-project # Copy application code. COPY . . +RUN chown -R app:app /app +USER app EXPOSE 8000🤖 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/Dockerfile` around lines 1 - 18, The Dockerfile currently runs the application as the root user, which presents a security risk. Add a USER directive to create and switch to a non-root user before the CMD instruction. Create a new user with appropriate permissions that has access to the application directory (/app), then use the USER directive to run all subsequent commands and the application under this non-root user account instead of root.Source: Linters/SAST tools
backend/app/api/csv_files.py-67-93 (1)
67-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpload batch is not atomic across S3 and DB.
If any later upload or
db.commit()fails, already-uploaded objects remain in S3 with no DB record. That violates the all-or-nothing expectation and creates orphaned artifacts.Suggested compensation pattern
- prepared: list[CsvFile] = [] - for filename, body in bodies: + prepared: list[CsvFile] = [] + uploaded_keys: list[str] = [] + try: + for filename, body in bodies: # Best-effort row/column count (does not fail the upload if unparsable). row_count: int | None = None column_count: int | None = None try: df = pd.read_csv(io.BytesIO(body)) row_count, column_count = int(df.shape[0]), int(df.shape[1]) except Exception: pass s3_key = f"{uuid4()}/{filename}" storage.upload_csv(s3_key, body) + uploaded_keys.append(s3_key) csv_file = CsvFile( filename=filename, s3_key=s3_key, size_bytes=len(body), row_count=row_count, column_count=column_count, ) db.add(csv_file) prepared.append(csv_file) - - await db.commit() + await db.commit() + except Exception: + await db.rollback() + for key in uploaded_keys: + try: + storage.delete_object(key) + except Exception: + pass + raise🤖 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/api/csv_files.py` around lines 67 - 93, The current implementation uploads files to S3 before committing to the database, which means if the database commit fails, orphaned files remain in S3. Refactor the logic to defer all S3 uploads until after the database commit succeeds. Restructure the loop to first collect all the CSV file metadata and add entries to the database, commit the transaction, and only then iterate through the prepared CSV files to upload their corresponding bodies to S3 using storage.upload_csv. This ensures that if any operation fails during the database phase, no files are uploaded to S3.backend/app/services/storage.py-12-21 (1)
12-21:⚠️ Potential issue | 🟠 MajorSynchronous boto3 calls block the event loop in async request paths.
The storage functions
get_s3_client(),upload_object(),download_object(), anddelete_object()use blocking boto3 methods directly. These are called from async API endpoints (e.g.,upload_csv_files()in csv_files.py,_persist_assistant()and_load_dataset_context()in chat.py) without thread pool wrapping, causing blocking I/O that reduces throughput under load.Wrap the storage calls using
run_in_threadpool:from starlette.concurrency import run_in_threadpool async def upload_object(...): client = get_s3_client() await run_in_threadpool( client.put_object, Bucket=settings.s3_bucket, Key=key, Body=body, ContentType=content_type, )Then update async callers to
await storage.upload_object(...)(and similarly for download/delete/head/create).🤖 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/services/storage.py` around lines 12 - 21, The storage functions `get_s3_client()`, `upload_object()`, `download_object()`, and `delete_object()` perform blocking boto3 I/O operations that are called from async contexts without thread pool wrapping, which blocks the event loop. Import `run_in_threadpool` from `starlette.concurrency` and wrap all boto3 method calls (such as `client.put_object()`, `client.get_object()`, `client.delete_object()`, etc.) with `await run_in_threadpool(method, args...)` to offload blocking operations to a thread pool. Convert the storage functions to async functions where needed and update all async callers in csv_files.py and chat.py to properly await these wrapped storage calls.backend/app/services/storage.py-27-30 (1)
27-30:⚠️ Potential issue | 🟠 MajorNarrow
head_bucketerror handling and handle regional bucket creation correctly.
head_bucketreturns generic HTTP status codes (400/403/404) with no body, making it impossible to distinguish a missing bucket from permission errors. Catching allClientErrorand always callingcreate_bucketwill mask permission or endpoint failures. Additionally,create_bucketwithoutCreateBucketConfiguration.LocationConstraintfails outsideus-east-1—if the region is not us-east-1, the API returns anIllegalLocationConstraintException.Check the error code to confirm the bucket is actually missing before attempting creation, and include the
LocationConstraintwhen the configured region is not us-east-1.Suggested fix
def ensure_bucket() -> None: """Create the upload bucket if it does not exist (idempotent).""" client = get_s3_client() try: client.head_bucket(Bucket=settings.s3_bucket) - except ClientError: - client.create_bucket(Bucket=settings.s3_bucket) + return + except ClientError as exc: + code = str(exc.response.get("Error", {}).get("Code", "")) + if code not in {"404", "NoSuchBucket", "NotFound"}: + raise + + kwargs = {"Bucket": settings.s3_bucket} + if settings.s3_region and settings.s3_region != "us-east-1": + kwargs["CreateBucketConfiguration"] = { + "LocationConstraint": settings.s3_region + } + client.create_bucket(**kwargs)🤖 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/services/storage.py` around lines 27 - 30, The current exception handling in the storage initialization block catches all ClientError exceptions broadly and always attempts to create the bucket, which masks permission or endpoint failures and doesn't distinguish between a missing bucket versus access issues. Modify the exception handler to check the error code property of the caught ClientError and only call create_bucket when the error code is NoSuchBucket. Additionally, when calling create_bucket, include the CreateBucketConfiguration parameter with LocationConstraint set to the configured region value only when the region is not us-east-1 (since us-east-1 is the default and doesn't require the constraint parameter).docker-compose.yml-22-24 (1)
22-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop hardcoding MinIO root credentials in compose.
Line 22 and Line 45 hardcode admin credentials in source-controlled config. This creates insecure defaults and leaks secrets into shared dev environments. Parameterize both MinIO and backend S3 credentials via env vars.
Suggested patch
minio: @@ environment: - MINIO_ROOT_USER: minioadmin - MINIO_ROOT_PASSWORD: minioadmin + MINIO_ROOT_USER: ${MINIO_ROOT_USER:-minioadmin} + MINIO_ROOT_PASSWORD: ${MINIO_ROOT_PASSWORD:-minioadmin} @@ backend: @@ environment: @@ - S3_ACCESS_KEY: minioadmin - S3_SECRET_KEY: minioadmin + S3_ACCESS_KEY: ${MINIO_ROOT_USER:-minioadmin} + S3_SECRET_KEY: ${MINIO_ROOT_PASSWORD:-minioadmin}Also applies to: 45-46
🤖 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 `@docker-compose.yml` around lines 22 - 24, Replace the hardcoded MinIO root credentials (MINIO_ROOT_USER and MINIO_ROOT_PASSWORD set to "minioadmin") with environment variable references using the ${VARIABLE_NAME} syntax. Apply the same parameterization pattern to the backend S3 credentials at the second location in the file (around line 45-46). This ensures sensitive credentials are not stored in source control but instead injected at runtime through environment variables, eliminating the security vulnerability of hardcoded default credentials in the shared compose configuration.backend/app/services/chat.py-85-90 (1)
85-90:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
run_chatdoes full dataset reload on every user message.Line 86–Line 90 reload all CSV references and materialize datasets each turn. This scales linearly with total dataset count and will degrade latency/memory as usage grows.
🤖 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/services/chat.py` around lines 85 - 90, The `run_chat` function reloads all CSV file references and materializes all datasets on every invocation by executing the select query on CsvFile, calling load_datasets with the refs, and calling describe on the datasets. This needs to be cached at the session or conversation level rather than executed per message. Move the dataset loading logic (the query selecting CsvFile.filename and CsvFile.s3_key, the load_datasets call, and the describe call) outside of the per-message processing so that datasets are loaded once and reused across multiple messages in the same conversation session, rather than being reloaded for each new message.frontend/Dockerfile-1-12 (1)
1-12:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRun the frontend container as a non-root user.
This Dockerfile never switches away from root, so the app process runs with unnecessary privileges.
Suggested patch
FROM node:22-slim WORKDIR /app -COPY package.json ./ -RUN npm install +COPY --chown=node:node package*.json ./ +RUN npm ci -COPY . . +COPY --chown=node:node . . +USER node EXPOSE 5173 CMD ["npm", "run", "dev", "--", "--host"]🤖 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 `@frontend/Dockerfile` around lines 1 - 12, The Dockerfile currently runs all processes as root, which poses a security risk. After the WORKDIR /app instruction, create a new non-root user and group using RUN commands, ensure the user owns the application directory with appropriate permissions, and add a USER directive before the final CMD instruction to switch to this non-root user. This ensures the npm run dev process executes with minimal required privileges.Source: Linters/SAST tools
frontend/Dockerfile-5-7 (1)
5-7:⚠️ Potential issue | 🟠 MajorCopy both
package.jsonandpackage-lock.jsonbefore runningnpm install; usenpm cifor deterministic builds.Line 5-6 only copies
package.jsonand runsnpm install, which performs non-deterministic installs. Althoughpackage-lock.jsonexists in the repository, it isn't available during the build—it's only copied later withCOPY . .(line 8), after installation completes. Copypackage-lock.jsonalongsidepackage.jsonon line 5, then usenpm ciinstead ofnpm installto ensure reproducible builds.🤖 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 `@frontend/Dockerfile` around lines 5 - 7, Modify the COPY command on line 5 to include both package.json and package-lock.json (change from copying just package.json to copying both files together). Then replace npm install on line 6 with npm ci to ensure deterministic and reproducible builds that respect the locked dependency versions.backend/app/models/message_artifact.py-20-31 (1)
20-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce artifact
kindat the database layer.Line 31 stores
kindas free-formString(32)with no check/enum constraint. Invalid values can be inserted outside API validation and break cross-layer contracts.Suggested patch
-from sqlalchemy import DateTime, ForeignKey, Integer, String, Text, func +from sqlalchemy import CheckConstraint, DateTime, ForeignKey, Integer, String, Text, func @@ class MessageArtifact(Base): __tablename__ = "message_artifact" + __table_args__ = ( + CheckConstraint( + "kind IN ('thinking','sql','query_result','figure','table')", + name="ck_message_artifact_kind", + ), + )🤖 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/models/message_artifact.py` around lines 20 - 31, The MessageArtifact class stores the kind field as a plain String(32) column without any database-level constraint, allowing invalid values to be inserted outside API validation. Replace the String(32) type with SQLAlchemy's Enum type, constrained to only the valid values defined in the ARTIFACT_KINDS tuple. This will enforce valid artifact kinds at the database layer and prevent invalid data from being persisted.backend/app/services/agent/title.py-27-31 (1)
27-31:⚠️ Potential issue | 🟠 MajorAdd a timeout around title generation.
Line 27 awaits an external model call without a time limit. If the provider hangs, new-conversation flow can stall before streaming starts. Use
asyncio.wait_for()to enforce an 8-second timeout on the agent call.Suggested patch
+import asyncio from pydantic_ai import Agent @@ agent: Agent[None] = Agent(model=get_model(), system_prompt=_TITLE_SYSTEM_PROMPT) try: - result = await agent.run(first_message) + result = await asyncio.wait_for(agent.run(first_message), timeout=8) title = (result.output or "").strip().strip('"').strip() except Exception as exc: # noqa: BLE001 - titling is best effort print(f"[title] generation failed: {exc}") return DEFAULT_TITLE🤖 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/services/agent/title.py` around lines 27 - 31, The agent.run(first_message) call in the title generation function lacks a timeout, which can cause the conversation flow to hang indefinitely if the external model provider becomes unresponsive. Wrap the await agent.run(first_message) call with asyncio.wait_for() and set the timeout to 8 seconds to ensure the operation fails gracefully if the provider hangs, allowing the function to return the DEFAULT_TITLE instead of blocking the new-conversation flow.backend/app/api/artifacts.py-31-40 (1)
31-40:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAvoid full in-memory buffering for artifact downloads.
This endpoint loads the entire object into memory before responding. For large CSV artifacts, that can cause memory pressure and slower responses. Stream the S3 body in chunks instead.
Suggested direction
-from fastapi.responses import Response +from fastapi.responses import StreamingResponse @@ - try: - body = storage.download_object(artifact.s3_key) - except Exception: + try: + stream = storage.download_object_stream(artifact.s3_key) + except Exception: raise HTTPException(status_code=502, detail="Could not fetch artifact content.") @@ - return Response( - content=body, + return StreamingResponse( + stream, media_type="text/csv", headers={"Content-Disposition": f'attachment; filename="{filename}"'}, )And in
backend/app/services/storage.py, add a chunked iterator (e.g., wrappingBody.iter_chunks()).🤖 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/api/artifacts.py` around lines 31 - 40, The artifact download endpoint currently loads the entire S3 object into memory by buffering the full response from storage.download_object() before sending it to the client. To stream large CSV artifacts without full in-memory buffering, modify the storage.download_object() function (or create a new streaming method) in backend/app/services/storage.py to return an iterator that wraps the S3 Body.iter_chunks() method for chunked iteration. Then update the artifact download endpoint to use this streaming iterator as the Response content instead of the buffered body, allowing the file to be sent to the client in chunks rather than loading it entirely into memory first.backend/app/api/conversations.py-142-144 (1)
142-144:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not expose raw backend exceptions in SSE error events.
Current error handling sends
str(exc)directly to clients, which can leak internal details. Return a generic message to the client and log the real exception server-side.Suggested patch
+import logging @@ router = APIRouter(prefix="/conversations", tags=["conversations"]) +logger = logging.getLogger(__name__) @@ except Exception as exc: # surface failures to the client, then end - yield agent_events.to_sse(agent_events.error(str(exc))) + logger.exception("Chat stream failed for conversation_id=%s", conversation_id) + yield agent_events.to_sse( + agent_events.error("Unable to process message right now.") + ) yield agent_events.to_sse(agent_events.done())🤖 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/api/conversations.py` around lines 142 - 144, The exception handling in the except block is directly exposing the raw exception message to clients by passing str(exc) to agent_events.error(), which can leak sensitive internal details. Replace the str(exc) argument with a generic error message like "An error occurred processing your request" that does not expose implementation details, and separately log the actual exception details using the server-side logger before yielding the error event to the client. This keeps internal error information server-side while providing a safe user-facing error message.backend/tests/conftest.py-55-60 (1)
55-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a hard safety check before schema reset.
Lines 59-60 will drop all tables for whatever
DATABASE_URLresolves to. Since Lines 24-27 allow env override, a misconfigured URL can destroy non-test data.Suggested fix
from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker, create_async_engine +from sqlalchemy.engine import make_url @@ TEST_DATABASE_URL = os.environ["DATABASE_URL"] + +def _assert_test_database_url(url: str) -> None: + db_name = (make_url(url).database or "").lower() + if "test" not in db_name: + raise RuntimeError(f"Refusing to run destructive test schema reset on DB '{db_name}'") + +_assert_test_database_url(TEST_DATABASE_URL)🤖 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/conftest.py` around lines 55 - 60, The _reset() function in the _schema fixture is dropping and recreating all tables without verifying it's actually running against a test database. Add a safety check before the Base.metadata.drop_all call in _reset() to validate that the DATABASE_URL is configured for testing purposes (such as checking for 'test' in the database name, localhost patterns, or an explicit test flag). This prevents accidental destruction of production data if the environment is misconfigured and DATABASE_URL points to a production database.backend/scripts/seed.py-194-199 (1)
194-199:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden the DB safety gate before destructive operations.
Line 195 relies on substring matching, which can incorrectly allow non-local targets. This script can delete/write substantial data, so the guard should parse the URL host explicitly.
Suggested fix
from sqlalchemy import delete, func, select +from sqlalchemy.engine import make_url @@ def _guard_dev_database() -> None: """Refuse to seed anything that doesn't look like a local dev/test DB.""" url = get_settings().database_url - if not any(host in url for host in ("localhost", "127.0.0.1", "`@db`:")): + parsed = make_url(url) + host = (parsed.host or "").lower() + if host not in {"localhost", "127.0.0.1", "db"}: raise SystemExit( f"Refusing to seed a non-local database:\n {url}\n" "Point DATABASE_URL at a local dev DB before seeding." )🤖 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/scripts/seed.py` around lines 194 - 199, The database safety check in the seed script currently uses unreliable substring matching to verify the database URL is local, which can incorrectly allow non-local databases if certain strings appear elsewhere in the URL. Replace the substring matching logic with explicit URL parsing to extract and validate the host component properly. Parse the database_url returned by get_settings() using a URL parsing library to get the hostname/netloc, then verify it matches one of the allowed local hosts (localhost, 127.0.0.1, or `@db`:) before allowing the seed operation to proceed.backend/app/services/agent/tools/visualize.py-52-54 (1)
52-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSanitize artifact titles before storing them.
titleis model/user-influenced and later used inContent-Dispositionfilename formatting (backend/app/api/artifacts.py). Unsanitized CR/LF/quote characters can cause header corruption or response-splitting risk.🔒 Suggested hardening
+def _sanitize_title(value: str) -> str: + clean = value.replace("\r", " ").replace("\n", " ").replace('"', "").strip() + return clean or "artifact" ... - ctx.deps.artifacts.append( - Artifact(kind="figure", title=title, payload=pio.to_json(fig)) - ) + safe_title = _sanitize_title(title) + ctx.deps.artifacts.append( + Artifact(kind="figure", title=safe_title, payload=pio.to_json(fig)) + ) ... - ctx.deps.artifacts.append( - Artifact(kind="table", title=title, payload=result.to_csv(index=False)) - ) + safe_title = _sanitize_title(title) + ctx.deps.artifacts.append( + Artifact(kind="table", title=safe_title, payload=result.to_csv(index=False)) + )Also applies to: 66-68
🤖 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/services/agent/tools/visualize.py` around lines 52 - 54, The title parameter being passed to the Artifact constructor at line 52 (and similarly at line 66) is user-influenced and gets used in Content-Disposition header formatting, creating a potential header injection vulnerability. Sanitize the title variable by removing or escaping dangerous characters such as carriage returns (CR), line feeds (LF), and quote characters before passing it to the Artifact constructor. Apply this sanitization to both occurrences where Artifact is instantiated with the title parameter to prevent header corruption and response-splitting attacks.backend/app/services/agent/runner.py-81-86 (1)
81-86:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset thinking-state on flush to avoid dropping final answer text.
If a response ends with an unmatched
<thinking>section,_insideremainsTrueafter flush, so later normal deltas are emitted asreasoninginstead oftext. That can drop assistant content persistence inchat.run_chat.🐛 Proposed fix
def flush(self) -> list[Event]: out: list[Event] = [] if self._buf: out.append(reasoning(self._buf) if self._inside else text(self._buf)) self._buf = "" + # A model-request node boundary is a safe reset point. + self._inside = False return out🤖 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/services/agent/runner.py` around lines 81 - 86, The `_inside` flag tracking whether we are inside a thinking section is not being reset after flush is called, causing it to persist to True. This means subsequent text deltas get emitted as reasoning instead of text. In the flush method, reset the `_inside` flag to False after appending the buffered content to the output list to ensure the thinking state is cleared and does not affect subsequent content processing.
🟡 Minor comments (8)
frontend/src/components/AssistantMessage.tsx-73-75 (1)
73-75:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSet explicit animation delays for the typing dots.
On Line 171,
var(--d)is never defined by callers, so all dots pulse together.Proposed fix
- {streaming && !text && !hasProcess && ( - <span className="inline-flex gap-1"> - <Dot /> <Dot /> <Dot /> + {streaming && !text && !hasProcess && ( + <span className="inline-flex gap-1"> + <Dot delayMs={0} /> <Dot delayMs={150} /> <Dot delayMs={300} /> </span> )} @@ -function Dot() { +function Dot({ delayMs }: { delayMs: number }) { return ( - <span className="h-1.5 w-1.5 animate-pulse rounded-full bg-white/50 [animation-delay:var(--d)]" /> + <span + className="h-1.5 w-1.5 animate-pulse rounded-full bg-white/50" + style={{ animationDelay: `${delayMs}ms` }} + /> ); }Also applies to: 169-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 `@frontend/src/components/AssistantMessage.tsx` around lines 73 - 75, The typing dots animation is not staggered because the CSS variable var(--d) referenced in the Dot component styling is never defined. Fix this by passing an explicit animation delay prop to each of the three Dot components in the span element, with incrementally increasing values (e.g., 0s, 0.2s, 0.4s). Modify the Dot component to accept this delay prop and apply it as an inline style to the animated element, ensuring each dot pulses with a staggered effect rather than in unison.frontend/src/components/CsvManager.tsx-101-108 (1)
101-108:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard against null
column_countin dimensions display.The code checks
row_count != nullbefore showing dimensions, but bothrow_countandcolumn_countcan benullaccording to theCsvFiletype. Ifrow_countis present butcolumn_countisnull, the UI will render "N×null".🛡️ Proposed fix
- {file.row_count != null && ( + {file.row_count != null && file.column_count != null && ( <> <span>·</span> <span> {file.row_count}×{file.column_count} </span> </> )}🤖 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 `@frontend/src/components/CsvManager.tsx` around lines 101 - 108, The dimensions display in the CsvManager component only guards against null row_count but doesn't check if column_count is null, which can result in rendering invalid text like "N×null". Update the conditional guard to check that both file.row_count and file.column_count are not null before rendering the dimensions span with the row_count and column_count values.frontend/src/index.css-27-27 (1)
27-27:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the inline comment spacing lint error.
This line is missing a space before
*/, which triggers the stylelintcomment-whitespace-insiderule.Proposed fix
- /* displacement filter — so a photo backdrop stays smooth, not pixelated.*/ + /* displacement filter — so a photo backdrop stays smooth, not pixelated. */🤖 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 `@frontend/src/index.css` at line 27, The inline comment "displacement filter — so a photo backdrop stays smooth, not pixelated.*/" is missing a space before the closing comment delimiter. Add a space between the period and the closing */ to comply with the stylelint comment-whitespace-inside rule.Source: Linters/SAST tools
frontend/src/App.tsx-234-240 (1)
234-240:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove hidden drawer toggles from keyboard focus order.
When
showis false, the button is visually hidden but still tabbable. Make it non-focusable while hidden.Proposed fix
<button onClick={onClick} aria-label={`Open ${label} panel`} + aria-hidden={!show} + tabIndex={show ? 0 : -1} className={`liquid-glass absolute top-5 z-20 flex h-10 items-center gap-2 rounded-2xl px-3 text-white/70 transition-all duration-300 hover:text-white ${ side === "left" ? "left-5 flex-row" : "right-5 flex-row-reverse" } ${show ? "scale-100 opacity-100" : "pointer-events-none scale-90 opacity-0"}`} >🤖 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 `@frontend/src/App.tsx` around lines 234 - 240, The button element with the onClick handler and aria-label for opening the panel is visually hidden when show is false using CSS classes (pointer-events-none, opacity-0), but it remains in the keyboard tab order. Add a tabIndex attribute to the button that is set to -1 when show is false to remove it from keyboard focus, and either omit it or set it to 0 when show is true to allow normal keyboard navigation.frontend/src/services/useCsvFiles.ts-23-25 (1)
23-25:⚠️ Potential issue | 🟡 MinorCatch
refresh()failures in the initial effect.A rejected
refresh()promise is currently left unhandled in this effect path. ThelistCsvFiles()call can reject when the API responds with a non-OK status (viagetJson()), and without a.catch()handler, the rejection will surface as an unhandled promise rejection during network/API failures. The same pattern exists inuseConversations.tsand is correctly implemented inuseMessages.ts.Proposed fix
useEffect(() => { - refresh().finally(() => setLoading(false)); + let cancelled = false; + void refresh() + .catch(() => { + // Optionally add an error state here. + }) + .finally(() => { + if (!cancelled) setLoading(false); + }); + return () => { + cancelled = true; + }; }, [refresh]);🤖 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 `@frontend/src/services/useCsvFiles.ts` around lines 23 - 25, The useEffect hook in useCsvFiles.ts calls refresh().finally() without a catch handler, leaving promise rejections from listCsvFiles() unhandled when the API fails. Add a .catch() handler to the refresh() promise chain before the .finally() to properly handle and log errors when listCsvFiles() rejects due to API failures, following the same error handling pattern that is correctly implemented in useMessages.ts.README.md-96-96 (1)
96-96:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language tag to the fenced layout block.
The code fence starting on Line 96 has no language identifier (MD040). Use
textto satisfy markdownlint.Suggested patch
-``` +```text backend/ app/ api/ # routers: conversations (+ chat streaming), csv_files, artifacts @@ docker-compose.yml</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@README.mdat line 96, The fenced code block starting on line 96 of README.md
is missing a language identifier, which violates the MD040 markdownlint rule.
Addtextas the language tag to the opening triple backticks fence that
precedes the directory structure listing (the block containing backend/, app/,
api/, etc.) by changingtotext to satisfy the linter requirement.</details> <!-- cr-comment:v1:671290b5005124dcb1f4af53 --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>backend/app/services/agent/model.py-21-23 (1)</summary><blockquote> `21-23`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Validate Anthropic model id suffix before constructing the provider model.** If `MODEL` is set to `anthropic:` (empty suffix), this path builds an invalid model name and fails later at runtime with a less clear error. <details> <summary>Suggested patch</summary> ```diff if model.startswith("anthropic:"): name = model.split(":", 1)[1] + if not name: + raise ValueError("MODEL must include an Anthropic model id, e.g. 'anthropic:claude-...'.") provider = ( AnthropicProvider(api_key=settings.anthropic_api_key) if settings.anthropic_api_key ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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/services/agent/model.py` around lines 21 - 23, The code extracts the model suffix by splitting on the colon when the model parameter starts with "anthropic:", but it does not validate that the suffix is non-empty. If the model is set to just "anthropic:" without a suffix, the split operation returns an empty string for the name variable, which then gets used to construct an invalid provider model name causing unclear runtime errors later. Add validation immediately after the split operation where name is assigned to check that name is not empty, and raise a clear ValueError if it is, indicating that a valid model suffix must be provided after the "anthropic:" prefix. ``` </details> <!-- cr-comment:v1:3ccb06477e35f63719840ed4 --> </blockquote></details> <details> <summary>backend/tests/test_csv_files.py-105-116 (1)</summary><blockquote> `105-116`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Assert storage rollback in the failed batch-upload test.** Line 106 says “no partial state,” but this test only checks DB state (Line 115). Add a storage assertion to catch orphaned uploads on partial failure paths. <details> <summary>Suggested fix</summary> ```diff def test_upload_rejects_batch_with_one_bad_file(client: TestClient, mock_storage): @@ assert resp.status_code == 400 assert client.get("/api/csv").json()["total"] == 0 + assert mock_storage == {} ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` 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_csv_files.py` around lines 105 - 116, The test_upload_rejects_batch_with_one_bad_file function claims no partial state but only checks that the database was rolled back by asserting the total is 0. Add an assertion to verify that the mock_storage parameter was also properly cleaned up and rolled back, ensuring no orphaned files were left behind when the batch upload fails due to the invalid notes.txt file. Check the mock_storage object to confirm storage rollback occurred alongside the database rollback. ``` </details> <!-- cr-comment:v1:9fe1d81ee01916db4a488ba9 --> </blockquote></details> </blockquote></details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
1 participant
Le readme de cette branche décrit ce qui a été fait et comment run le projet localement avec et sans Docker.
Summary by CodeRabbit
New Features
Infrastructure
Testing