Add dedicated Flask test coverage for search, workspaces, and export APIs (#101)#104
Open
bradjin8 wants to merge 3 commits into
Open
Add dedicated Flask test coverage for search, workspaces, and export APIs (#101)#104bradjin8 wants to merge 3 commits into
bradjin8 wants to merge 3 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughSplits monolithic endpoint tests into three focused modules, adds a reusable exclusion-rule test client helper, hardens export_state JSON parsing to require a dict, and updates CI to run the new test modules with updated mypy guidance. ChangesEndpoint Test Coverage Expansion (
Sequence DiagramsequenceDiagram
participant Test as Test Client
participant Flask as Flask Route Handler
participant DB as SQLite / Fixtures
participant Export as api.export_api
Test->>Flask: GET /api/search?q=term
Flask->>DB: query conversations/search index
DB-->>Flask: SearchResult rows
Flask-->>Test: 200 + SearchResult list
Test->>Flask: GET /api/workspaces
Flask->>DB: list workspace projects
DB-->>Flask: projects array
Flask-->>Test: 200 + projects
Test->>Flask: POST /api/export
Flask->>DB: fetch conversations
DB-->>Flask: conversation records
Flask->>Export: build ZIP and update export_state.json
Export-->>Flask: ZIP bytes / state persisted
Flask-->>Test: 200 + ZIP (application/zip)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
bradjin8
force-pushed
the
feat/three-api-tests
branch
from
ca22532 to
1f36f81
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/tests.yml (1)
152-152: 💤 Low valueConsider clarifying why the Flask types note appears here.
The comment about Flask 3.1+ inline types is accurate and useful, but it appears before installation commands that don't attempt to install
types-Flask. Consider expanding it to explain the connection:- # Flask 3.1+ ships inline types — do not install types-Flask (conflicts). + # Flask 3.1+ ships inline types and conflicts with types-Flask; we rely on + # the inline types from Flask in requirements-lock.txt (no stub package needed).This makes it clearer that the comment is preventative documentation for future maintainers.
🤖 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 @.github/workflows/tests.yml at line 152, Update the inline comment about Flask 3.1+ to explicitly state that Flask now ships type hints so maintainers should not add or install the separate types-Flask package (it will conflict), and note this is preventative documentation for the following pip install steps that install various typing packages; change the comment to something like “Flask 3.1+ includes inline type hints — do not add or install types-Flask (it will conflict with our pip installs), keeping this as preventative guidance for future maintainers.”services/workspace_tabs.py (1)
910-916: ⚡ Quick winReuse the shared composer-row SQL constant in this path.
Line [910]-[916] rebuilds the composer filter query inline, while other paths already use
COMPOSER_ROWS_WITH_HEADERS_SQL. Reusing the constant here reduces drift risk and keeps filtering semantics consistent across tabs endpoints.Proposed diff
- composer_rows = safe_fetchall( - global_db, - "SELECT key, value FROM cursorDiskKV WHERE key LIKE 'composerData:%'" - " AND value IS NOT NULL" - " AND value LIKE '%fullConversationHeadersOnly%'" - " AND value NOT LIKE '%fullConversationHeadersOnly\":[]%'", - ) + composer_rows = safe_fetchall(global_db, COMPOSER_ROWS_WITH_HEADERS_SQL)🤖 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 `@services/workspace_tabs.py` around lines 910 - 916, Replace the inline SQL used to populate composer_rows with the shared constant COMPOSER_ROWS_WITH_HEADERS_SQL to avoid duplication and drift: locate the call to safe_fetchall that currently passes the hard-coded "SELECT key, value FROM cursorDiskKV ..." string and change it to use COMPOSER_ROWS_WITH_HEADERS_SQL (keeping the same safe_fetchall(global_db, COMPOSER_ROWS_WITH_HEADERS_SQL) signature and existing variable name composer_rows), ensuring any surrounding logic that expects the same filter semantics remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.gitignore:
- Line 12: The .gitignore contains an unexplained entry `.mypy-ci-test/` that
isn't referenced elsewhere; either remove that ignore or document/produce the
directory during CI. Fix by either deleting the `.mypy-ci-test/` line from
`.gitignore` or adding documentation and CI changes that create/populate
`.mypy-ci-test/` (or set mypy's cache_dir) so the entry is justified; check
`pyproject.toml`'s `[tool.mypy]` and existing `.mypy_cache/` usage to ensure
consistency with the chosen approach.
In `@api/export_api.py`:
- Around line 42-49: The _get_export_state function currently casts json.load(f)
to dict[str, Any] without runtime validation; change it to read the parsed value
into a local (e.g., parsed = json.load(f)), check isinstance(parsed, dict) and
return parsed if true, otherwise return an empty dict (and optionally log/debug
that the file contained non-object JSON) so export_chats()'s state.get(...)
won't raise.
---
Nitpick comments:
In @.github/workflows/tests.yml:
- Line 152: Update the inline comment about Flask 3.1+ to explicitly state that
Flask now ships type hints so maintainers should not add or install the separate
types-Flask package (it will conflict), and note this is preventative
documentation for the following pip install steps that install various typing
packages; change the comment to something like “Flask 3.1+ includes inline type
hints — do not add or install types-Flask (it will conflict with our pip
installs), keeping this as preventative guidance for future maintainers.”
In `@services/workspace_tabs.py`:
- Around line 910-916: Replace the inline SQL used to populate composer_rows
with the shared constant COMPOSER_ROWS_WITH_HEADERS_SQL to avoid duplication and
drift: locate the call to safe_fetchall that currently passes the hard-coded
"SELECT key, value FROM cursorDiskKV ..." string and change it to use
COMPOSER_ROWS_WITH_HEADERS_SQL (keeping the same safe_fetchall(global_db,
COMPOSER_ROWS_WITH_HEADERS_SQL) signature and existing variable name
composer_rows), ensuring any surrounding logic that expects the same filter
semantics remains unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c41a648b-85c0-4e13-92b0-ed8b5d386c57
📒 Files selected for processing (41)
.github/workflows/tests.yml.gitignoreCHANGELOG.mdapi/composers.pyapi/config_api.pyapi/export_api.pyapi/flask_config.pyapi/logs.pyapi/pdf.pyapi/search.pyapi/workspaces.pyapp.pymodels/__init__.pymodels/conversation.pymodels/parse_warnings.pymodels/search.pypyproject.tomlservices/cli_tabs.pyservices/search.pyservices/summary_cache.pyservices/workspace_context.pyservices/workspace_db.pyservices/workspace_listing.pyservices/workspace_resolver.pyservices/workspace_tabs.pytests/conftest.pytests/test_api_endpoints.pytests/test_api_export.pytests/test_api_search.pytests/test_api_workspaces.pytests/test_models.pytests/test_normalize_file_path.pytests/test_search_helpers.pyutils/cli_chat_reader.pyutils/cursor_md_exporter.pyutils/debug_flag.pyutils/exclusion_rules.pyutils/path_helpers.pyutils/text_extract.pyutils/tool_parser.pyutils/workspace_descriptor.py
💤 Files with no reviewable changes (1)
- tests/test_api_endpoints.py
bradjin8
force-pushed
the
feat/three-api-tests
branch
from
51188ca to
a30c6b4
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_api_export.py (1)
42-59: ⚡ Quick winOptional: Add test for malformed export_state.json to verify warning behavior.
The validation added in
api/export_api._get_export_state(lines 49-55) logs a warning and returns{}when the state file contains non-dict JSON (e.g., an array or string). Consider adding a test that writes such a file and verifies the endpoint still returns a valid response.📋 Example test case
def test_get_state_handles_malformed_file(self, client, export_state_dir): """Non-dict JSON in state file should not crash the endpoint.""" state_path = export_state_dir / "export_state.json" state_path.write_text('["not", "a", "dict"]', encoding="utf-8") response = client.get("/api/export/state") assert response.status_code == 200 body = response.get_json() assert isinstance(body, dict) assert body == {} # Falls back to empty dict🤖 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 `@tests/test_api_export.py` around lines 42 - 59, Add a test to exercise api/export_api._get_export_state's handling of malformed JSON: in TestExportState add a method (e.g., test_get_state_handles_malformed_file) that writes non-dict JSON (like '["not","a","dict"]') to export_state.json in the export_state_dir, calls client.get("/api/export/state"), asserts a 200 response, asserts the returned body is a dict and equals {} to verify the function logs the warning and falls back to an empty dict; reference TestExportState and the endpoint "/api/export/state" to locate where to add the new test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_api_export.py`:
- Around line 42-59: Add a test to exercise api/export_api._get_export_state's
handling of malformed JSON: in TestExportState add a method (e.g.,
test_get_state_handles_malformed_file) that writes non-dict JSON (like
'["not","a","dict"]') to export_state.json in the export_state_dir, calls
client.get("/api/export/state"), asserts a 200 response, asserts the returned
body is a dict and equals {} to verify the function logs the warning and falls
back to an empty dict; reference TestExportState and the endpoint
"/api/export/state" to locate where to add the new test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 70c3e4b6-13dd-4a17-86cb-8278dd754906
📒 Files selected for processing (7)
.github/workflows/tests.ymlapi/export_api.pytests/conftest.pytests/test_api_endpoints.pytests/test_api_export.pytests/test_api_search.pytests/test_api_workspaces.py
💤 Files with no reviewable changes (1)
- tests/test_api_endpoints.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/test_api_workspaces.py
- tests/conftest.py
- tests/test_api_search.py
timon0305
approved these changes
Jun 12, 2026
| assert body["name"] == "Other chats" | ||
|
|
||
|
|
||
| class TestGetWorkspaceTabs: |
Collaborator
There was a problem hiding this comment.
The split was supposed to keep all the old tests. This one for "global workspace + summary mode" is gone. Please add it back to TestGetWorkspaceTabs. Everything else carried over fine, so this looks accidental.
| return app.test_client() | ||
|
|
||
|
|
||
| def client_with_rules(rule_lines: list[str]) -> FlaskClient: |
Collaborator
There was a problem hiding this comment.
Importing helpers from conftest.py is discouraged (pytest treats it specially). Works, but move it to a normal file like tests/_helpers.py — same pattern you already use with tests/_fixture_ids.py. Minor.
Comment on lines
+134
to
+143
| def test_since_last_after_export_returns_404_when_nothing_new( | ||
| self, client, export_state_dir | ||
| ): | ||
| first = _post_export(client, {"since": "all"}) | ||
| assert first.status_code == 200 | ||
|
|
||
| second = _post_export(client, {"since": "last"}) | ||
| assert second.status_code == 404 | ||
| body = second.get_json() | ||
| assert body.get("error") == "No conversations to export since last export" |
Collaborator
There was a problem hiding this comment.
This only passes because the seeded chat's timestamp is in the past. Fine now, but add a one-line comment saying so — otherwise changing fixture dates later would break this test in a confusing way.
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
2 participants
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
tests/test_api_endpoints.pyinto three dedicated route test modules:test_api_search.py,test_api_workspaces.py, andtest_api_export.pyPOST /api/exportandGET /api/export/state, including zip response headers, export state persistence, andsince=lastbehaviorclient_with_rules()helper inconftest.pyand update CI to run the new test filesCloses #101.
Test plan
python -m pytest tests/test_api_search.py tests/test_api_workspaces.py tests/test_api_export.py -v(33 passed)Summary by CodeRabbit