Skip to content

Conversation

@klingonaston
Copy link
Collaborator

@klingonaston klingonaston commented Oct 23, 2025

Overview: This PR introduces a new endpoint to fetch the final generated JSON report data.

Changes

  • Added a new endpoint to retrieve report data by report_id.
  • The endpoint returns a 202 (Accepted) if the report is still processing, and a 200 (OK) with the report data if completed.
  • Implemented the core logic in backend/app/services/report_service.py within a new get_report_data function.
  • Leverages the same storage mechanism used for status tracking to retrieve the final report data.

Summary by CodeRabbit

  • New Features

    • Added a report data retrieval endpoint that returns completed report data, indicates when a report is still processing (202), or reports not found (404/409 as appropriate).
  • Tests

    • Added tests covering report data retrieval for processing, completed, and non-existent report scenarios.

@coderabbitai
Copy link

coderabbitai bot commented Oct 23, 2025

Walkthrough

Adds a service function to retrieve report data from the in-memory store and a new GET /reports/{report_id}/data endpoint that returns completed data, processing (202), failed (409), or not-found (404) results; updates import paths in main and adds async tests for the new endpoint.

Changes

Cohort / File(s) Summary
Report data service & endpoint
backend/app/services/report_service.py, backend/app/api/v1/routes.py
Added get_report_data(report_id) to read in-memory reports and return either data (when completed) or status; added GET /reports/{report_id}/data endpoint that returns 200 with data, 202 if processing, 409 if failed, or 404 if not found. Uses JSONResponse for non-200 responses.
Import path updates
backend/main.py
Adjusted import paths to backend.app.* (settings and router) to match package layout.
Tests
backend/tests/test_routes.py
New async tests covering processing (expect 202), completed (expect 200 with agent_results), and not-found (expect 404). Clears in-memory store around tests and uses TestClient with asyncio sleeps to allow background tasks.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant API as GET /api/v1/reports/{id}/data
    participant Service as get_report_data(report_id)
    participant Store as In-Memory Store

    Client->>API: Request report data
    API->>Service: get_report_data(report_id)
    Service->>Store: lookup(report_id)

    alt found & status == "completed"
        Store-->>Service: report with agent_results
        Service-->>API: {report_id, data}
        API-->>Client: 200 OK (body: data)
    else found & status == "processing"
        Store-->>Service: report with status "processing"
        Service-->>API: {report_id, status}
        API-->>Client: 202 Accepted (JSONResponse: still processing)
    else found & status == "failed"
        Store-->>Service: report with status "failed"
        Service-->>API: {report_id, status}
        API-->>Client: 409 Conflict (JSONResponse: failed)
    else not found
        Store-->>Service: None
        Service-->>API: None
        API-->>Client: 404 Not Found (HTTPException)
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • felixjordandev

Poem

🐇 I hopped through memory, sniffed each id,
Found agents busy, then data wide,
I thumped a 202 when work's in play,
A 200 hug when results convey,
Cheers — this rabbit rates the endpoint spry!

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The pull request title "Feat: Add endpoint to retrieve final report data" directly and accurately reflects the main change in the changeset. The primary modification is the addition of a new GET endpoint (/reports/{report_id}/data) that retrieves final report data by report ID, along with supporting logic in the service layer and comprehensive tests. The title clearly communicates the core objective without being vague or misleading, and a teammate reviewing the commit history would immediately understand that this PR adds endpoint functionality for fetching report data.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feat/get-report-data-endpoint

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ad33f82 and 45aeae4.

⛔ Files ignored due to path filters (5)
  • __pycache__/main.cpython-313.pyc is excluded by !**/*.pyc
  • backend/app/api/v1/__pycache__/routes.cpython-313.pyc is excluded by !**/*.pyc
  • backend/app/services/__pycache__/report_service.cpython-313.pyc is excluded by !**/*.pyc
  • backend/tests/__pycache__/test_report_processor.cpython-313-pytest-8.4.2.pyc is excluded by !**/*.pyc
  • backend/tests/__pycache__/test_routes.cpython-313-pytest-8.4.2.pyc is excluded by !**/*.pyc
📒 Files selected for processing (2)
  • backend/app/api/v1/routes.py (2 hunks)
  • backend/app/services/report_service.py (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • backend/app/api/v1/routes.py
🔇 Additional comments (1)
backend/app/services/report_service.py (1)

32-41: Past issue resolved; implementation now correct.

The response structure now properly nests agent_results within the data field, matching test expectations and the API contract. The use of .get("agent_results", {}) appropriately guards against missing keys.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
backend/tests/test_routes.py (2)

22-34: Avoid brittle full-body equality for 202; assert key fields

Reduce coupling to response shape while still validating semantics.

Apply:

-    assert response.status_code == 202
-    assert response.json() == {"report_id": report_id, "message": "Report is still processing.", "detail": "Report is still processing."} # Added detail to match the actual response
+    assert response.status_code == 202
+    body = response.json()
+    assert body["report_id"] == report_id
+    assert body["detail"] == "Report is still processing."
+    assert body.get("message") == "Report is still processing."

45-51: Reduce flakiness: poll for completion instead of fixed sleep

Polling with a timeout is more reliable than sleep(2).

Apply:

-    # Wait for the background task to complete
-    await asyncio.sleep(2)  # Give enough time for dummy agents to complete
-
-    # Request data, should be completed
-    response = await anyio.to_thread.run_sync(client.get, f"/api/v1/reports/{report_id}/data")
+    # Wait until completed or timeout (~4s total)
+    response = None
+    for _ in range(40):
+        response = await anyio.to_thread.run_sync(client.get, f"/api/v1/reports/{report_id}/data")
+        if response.status_code == 200:
+            break
+        await asyncio.sleep(0.1)
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 83e8b51 and ad33f82.

⛔ Files ignored due to path filters (7)
  • backend/__pycache__/main.cpython-313.pyc is excluded by !**/*.pyc
  • backend/app/api/v1/__pycache__/__init__.cpython-313.pyc is excluded by !**/*.pyc
  • backend/app/api/v1/__pycache__/routes.cpython-313.pyc is excluded by !**/*.pyc
  • backend/app/core/__pycache__/config.cpython-313.pyc is excluded by !**/*.pyc
  • backend/app/core/__pycache__/orchestrator.cpython-313.pyc is excluded by !**/*.pyc
  • backend/app/services/__pycache__/report_service.cpython-313.pyc is excluded by !**/*.pyc
  • backend/tests/__pycache__/test_routes.cpython-313-pytest-8.4.2.pyc is excluded by !**/*.pyc
📒 Files selected for processing (4)
  • backend/app/api/v1/routes.py (2 hunks)
  • backend/app/services/report_service.py (1 hunks)
  • backend/main.py (1 hunks)
  • backend/tests/test_routes.py (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/app/api/v1/routes.py (1)
backend/app/services/report_service.py (3)
  • generate_report (11-20)
  • get_report_status_from_memory (29-30)
  • get_report_data (32-39)
🔇 Additional comments (1)
backend/main.py (1)

6-7: Import path update looks correct

Module paths match repo layout; no functional change.

@felixjordandev felixjordandev merged commit 4109a58 into main Oct 23, 2025
1 check passed
@felixjordandev felixjordandev deleted the feat/get-report-data-endpoint branch October 23, 2025 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants