Remove bootstrap frontend and keep SPA routes only#38
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR removes the server-side Django rendering layer for many features: deletes numerous view modules, templates, authentication middleware and handlers, prunes URL routing, and replaces several server redirects with SPA route helpers and SPA-focused URL generation. Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant Server as Django Backend
participant SPA as Frontend SPA
participant API as Backend API
Browser->>Server: GET /sql/detail/<id> (old server-rendered route)
alt route removed
Server-->>Browser: 302 -> SPA path (spa_path_for_workflow -> /workflows/<id>)
Browser->>SPA: navigate /workflows/<id>
SPA->>API: GET /api/workflow/<id> (load data via API)
API-->>SPA: 200 + workflow JSON
SPA-->>Browser: render workflow detail
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
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)
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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sql/urls.py (1)
24-99:⚠️ Potential issue | 🔴 CriticalVerify SPA deep links are still served after this change.
sql/sql_workflow.py:39andsql/archiver.py:875redirect users to/workflows/{id}and/archives/{id}, but these paths don't match any route in the current URLconf. The rootarchery/urls.pyhas no catch-all route or SPA entrypoint handler, so these redirects will result in 404s instead of the SPA shell loading with the deep link.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql/urls.py` around lines 24 - 99, Redirects in sql_workflow.py (around the redirect on line ~39) and archiver.py (around the redirect on line ~875) send users to /workflows/{id} and /archives/{id} which are not defined in the URLconf; add routes to this urls.py to serve the SPA entrypoint for those deep links (e.g., add path("workflows/<int:id>/", <SPA_entry_view>) and path("archives/<int:id>/", <SPA_entry_view>)) so the SPA shell loads for deep links, or alternatively change the redirects in sql_workflow.redirect and archiver.archive_apply/archive_switch (the specific redirect calls) to point to existing valid routes; ensure the chosen SPA entry view is the same handler used by your frontend root so deep links render correctly.
🧹 Nitpick comments (3)
sql/notify.py (1)
41-48: Prefer fail-fast behavior for unsupported workflow types.Returning
base_urlon unknown types can silently produce incorrect links. Raising a clear error (or logging + explicit default path) will make routing drift easier to catch.Proposed refactor
def _spa_workflow_url(base_url, workflow_type, workflow_id): if workflow_type == WorkflowType.SQL_REVIEW: return f"{base_url}/workflows/{workflow_id}" if workflow_type == WorkflowType.QUERY: return f"{base_url}/permission-management?requestId={workflow_id}" if workflow_type == WorkflowType.ARCHIVE: return f"{base_url}/archives/{workflow_id}" - return base_url + raise ValueError(f"Unsupported workflow type for SPA URL: {workflow_type}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql/notify.py` around lines 41 - 48, The helper _spa_workflow_url currently returns base_url for unknown WorkflowType values which hides routing errors; change it to fail-fast by raising a clear exception (e.g., ValueError) when workflow_type is not one of WorkflowType.SQL_REVIEW, WorkflowType.QUERY, or WorkflowType.ARCHIVE, or alternatively log an explicit error and return a well-documented default path; update any callers of _spa_workflow_url to handle the exception if needed, and reference WorkflowType and _spa_workflow_url when making the change so the behavior is consistent across the codebase.common/middleware/check_login_middleware.py (1)
5-14: Consider removing this middleware from the stack now that it is a no-op.With unconditional
return None, this class adds per-request overhead and can imply login enforcement that no longer exists. Prefer removingcommon.middleware.check_login_middleware.CheckLoginMiddlewarefromMIDDLEWARE(or renaming/repurposing it to match current behavior).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@common/middleware/check_login_middleware.py` around lines 5 - 14, The CheckLoginMiddleware class (subclassing MiddlewareMixin) only defines process_request and unconditionally returns None, making it a no-op that adds overhead and misleading behavior; either remove common.middleware.check_login_middleware.CheckLoginMiddleware from the Django MIDDLEWARE setting or update this file: delete the CheckLoginMiddleware class entirely, or rename/repurpose it and implement real logic in process_request (or other middleware hooks) if you need retained functionality, and update any references to the class accordingly so the middleware stack no longer includes a no-op.sql/sql_workflow.py (1)
38-39: Centralize SPA redirect construction.
/workflows/{id}is now hardcoded here, while/archives/{id}is hardcoded separately insql/archiver.py. That makes future SPA route or base-path changes easy to miss. A shared redirect helper/config value would keep these post-action redirects from drifting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql/sql_workflow.py` around lines 38 - 39, Centralize SPA redirect construction by extracting the hardcoded path into a shared helper or config and update callers: create a single helper (e.g., spa_redirect(resource, id) or spa_url(resource, id)) and use it instead of hardcoding "/workflows/{workflow_id}" in _workflow_detail_redirect and the "/archives/{id}" use in sql/archiver.py; update _workflow_detail_redirect to call the new helper (preserving its signature) and change the archiver redirect to the same helper so future base-path or route changes are made in one place.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@sql/urls.py`:
- Around line 24-99: Redirects in sql_workflow.py (around the redirect on line
~39) and archiver.py (around the redirect on line ~875) send users to
/workflows/{id} and /archives/{id} which are not defined in the URLconf; add
routes to this urls.py to serve the SPA entrypoint for those deep links (e.g.,
add path("workflows/<int:id>/", <SPA_entry_view>) and path("archives/<int:id>/",
<SPA_entry_view>)) so the SPA shell loads for deep links, or alternatively
change the redirects in sql_workflow.redirect and
archiver.archive_apply/archive_switch (the specific redirect calls) to point to
existing valid routes; ensure the chosen SPA entry view is the same handler used
by your frontend root so deep links render correctly.
---
Nitpick comments:
In `@common/middleware/check_login_middleware.py`:
- Around line 5-14: The CheckLoginMiddleware class (subclassing MiddlewareMixin)
only defines process_request and unconditionally returns None, making it a no-op
that adds overhead and misleading behavior; either remove
common.middleware.check_login_middleware.CheckLoginMiddleware from the Django
MIDDLEWARE setting or update this file: delete the CheckLoginMiddleware class
entirely, or rename/repurpose it and implement real logic in process_request (or
other middleware hooks) if you need retained functionality, and update any
references to the class accordingly so the middleware stack no longer includes a
no-op.
In `@sql/notify.py`:
- Around line 41-48: The helper _spa_workflow_url currently returns base_url for
unknown WorkflowType values which hides routing errors; change it to fail-fast
by raising a clear exception (e.g., ValueError) when workflow_type is not one of
WorkflowType.SQL_REVIEW, WorkflowType.QUERY, or WorkflowType.ARCHIVE, or
alternatively log an explicit error and return a well-documented default path;
update any callers of _spa_workflow_url to handle the exception if needed, and
reference WorkflowType and _spa_workflow_url when making the change so the
behavior is consistent across the codebase.
In `@sql/sql_workflow.py`:
- Around line 38-39: Centralize SPA redirect construction by extracting the
hardcoded path into a shared helper or config and update callers: create a
single helper (e.g., spa_redirect(resource, id) or spa_url(resource, id)) and
use it instead of hardcoding "/workflows/{workflow_id}" in
_workflow_detail_redirect and the "/archives/{id}" use in sql/archiver.py;
update _workflow_detail_redirect to call the new helper (preserving its
signature) and change the archiver redirect to the same helper so future
base-path or route changes are made in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c75d8e31-99a7-4fd6-bfc2-e55cc245091a
📒 Files selected for processing (42)
common/dashboard.pycommon/middleware/check_login_middleware.pycommon/templates/2fa.htmlcommon/templates/base.htmlcommon/templates/config.htmlcommon/templates/dashboard.htmlcommon/templates/error.htmlcommon/templates/errors/400.htmlcommon/templates/errors/403.htmlcommon/templates/errors/404.htmlcommon/templates/errors/500.htmlcommon/templates/login.htmlcommon/tests.pysql/archiver.pysql/notify.pysql/query_privileges.pysql/sql_workflow.pysql/templates/archive.htmlsql/templates/archivedetail.htmlsql/templates/detail.htmlsql/templates/group.htmlsql/templates/groupmgmt.htmlsql/templates/instance.htmlsql/templates/legacy_login_form.htmlsql/templates/queryapplydetail.htmlsql/templates/queryapplylist.htmlsql/templates/queryuserprivileges.htmlsql/templates/rollback.htmlsql/templates/sqlexportsubmit.htmlsql/templates/sqlexportworkflow.htmlsql/templates/sqlquery.htmlsql/templates/sqlsubmit.htmlsql/templates/sqlworkflow.htmlsql/templates/workflow.htmlsql/templates/workflow_display.htmlsql/test_archiver.pysql/test_query_privileges.pysql/test_workflow.pysql/tests.pysql/urls.pysql/utils/test_workflow_audit.pysql/views.py
💤 Files with no reviewable changes (26)
- sql/utils/test_workflow_audit.py
- sql/test_workflow.py
- sql/templates/queryapplydetail.html
- sql/templates/legacy_login_form.html
- sql/templates/group.html
- sql/templates/instance.html
- sql/templates/groupmgmt.html
- sql/templates/queryapplylist.html
- common/templates/login.html
- sql/templates/sqlexportsubmit.html
- sql/templates/workflow_display.html
- sql/templates/queryuserprivileges.html
- common/templates/base.html
- common/templates/2fa.html
- common/dashboard.py
- sql/templates/sqlworkflow.html
- sql/templates/sqlquery.html
- common/templates/dashboard.html
- sql/templates/detail.html
- sql/templates/archivedetail.html
- sql/templates/sqlsubmit.html
- sql/templates/workflow.html
- sql/templates/sqlexportworkflow.html
- sql/views.py
- sql/templates/rollback.html
- sql/templates/archive.html
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
sql/test_openai.py (1)
8-15: Close client in fixture teardown, not only intest_init.
openai_client.client.close()is called at Line 21 only for one test. Move cleanup into the fixture so all tests close resources consistently.♻️ Proposed refactor
`@pytest.fixture` def openai_client(setup_sys_config): # Use mock config values. setup_sys_config.set("openai_base_url", "https://api.openai.com") setup_sys_config.set("openai_api_key", "sk-xxxx") setup_sys_config.set("default_chat_model", "gpt-3.5-turbo") - yield OpenaiClient() + client = OpenaiClient() + try: + yield client + finally: + client.client.close() @@ def test_init(openai_client): assert openai_client.base_url == "https://api.openai.com" assert openai_client.api_key == "sk-xxxx" assert openai_client.default_chat_model == "gpt-3.5-turbo" - openai_client.client.close()Also applies to: 17-22
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sql/test_openai.py` around lines 8 - 15, The fixture openai_client currently yields an OpenaiClient() but the client is only closed in one test; move the cleanup into the fixture by using yield in openai_client and closing the underlying resource after yield (call openai_client.client.close() or OpenaiClient.close() as appropriate) so every test gets the instance and the fixture always performs teardown; locate the openai_client fixture and add the post-yield cleanup that closes the client's transport/connection using the OpenaiClient instance created from setup_sys_config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sql/test_openai.py`:
- Around line 3-5: Test coverage for generate_sql and check_openai was
removed—add function-level tests that exercise their validation and error
branches: write tests that call check_openai to assert errors when the OpenAI
config is missing or invalid and when an OpenaiClient instance is not provided,
and write tests for generate_sql to assert it raises/returns the expected
validation errors when query_desc or db_type are missing and when OpenaiClient
interaction fails; reference the functions check_openai and generate_sql in
sql/query.py and reuse or mock the OpenaiClient behavior (from
common.utils.openai.OpenaiClient) to simulate success and failure paths.
---
Nitpick comments:
In `@sql/test_openai.py`:
- Around line 8-15: The fixture openai_client currently yields an OpenaiClient()
but the client is only closed in one test; move the cleanup into the fixture by
using yield in openai_client and closing the underlying resource after yield
(call openai_client.client.close() or OpenaiClient.close() as appropriate) so
every test gets the instance and the fixture always performs teardown; locate
the openai_client fixture and add the post-yield cleanup that closes the
client's transport/connection using the OpenaiClient instance created from
setup_sys_config.
🪄 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: fa4d049b-fae4-4dc7-b20a-a56d268b23aa
📒 Files selected for processing (19)
api_core/tests.pyapi_instances/tests.pyarchery/settings.pyarchery/urls.pycommon/auth.pycommon/check.pycommon/middleware/check_login_middleware.pycommon/tests.pycommon/utils/spa.pysql/admin.pysql/archiver.pysql/notify.pysql/query_privileges.pysql/sql_workflow.pysql/test_archiver.pysql/test_openai.pysql/test_query_privileges.pysql/tests.pysql/urls.py
💤 Files with no reviewable changes (7)
- api_core/tests.py
- archery/settings.py
- sql/urls.py
- common/check.py
- common/middleware/check_login_middleware.py
- archery/urls.py
- sql/admin.py
✅ Files skipped from review due to trivial changes (1)
- api_instances/tests.py
🚧 Files skipped from review as they are similar to previous changes (4)
- sql/query_privileges.py
- sql/archiver.py
- common/tests.py
- sql/test_query_privileges.py
Comment on lines
+3
to
+5
| import pytest | ||
|
|
||
| setup_sys_config.set("openai_api_key", "sk-xxxx") | ||
| response = admin_client.get("/check/openai/") | ||
| assert response.status_code == 200 | ||
| assert response.json()["data"] == True | ||
| from common.utils.openai import OpenaiClient |
Contributor
There was a problem hiding this comment.
Re-add coverage for check_openai / generate_sql behavior.
This test file now validates only OpenaiClient, but sql/query.py still contains generate_sql and check_openai (source snippet: sql/query.py:339-414). Removing their tests drops protection for core validation/error branches (missing config, missing query_desc/db_type, missing instance).
🧪 Suggested test additions (route-independent, function-level)
+import json
+from django.test import RequestFactory
+from sql.query import check_openai, generate_sql
+from common.models.instance import Instance
+
+def test_check_openai_missing_config(setup_sys_config):
+ setup_sys_config.set("openai_base_url", "")
+ setup_sys_config.set("openai_api_key", "")
+ setup_sys_config.set("default_chat_model", "")
+ request = RequestFactory().get("/check/openai/")
+ response = check_openai(request)
+ payload = json.loads(response.content)
+ assert payload["status"] == 1
+ assert payload["data"] is False
+
+def test_generate_sql_missing_required_fields():
+ request = RequestFactory().post("/query/generate_sql/", data={})
+ response = generate_sql(request)
+ payload = json.loads(response.content)
+ assert payload["status"] == 1
+ assert payload["msg"] == "query_desc or db_type does not exist"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sql/test_openai.py` around lines 3 - 5, Test coverage for generate_sql and
check_openai was removed—add function-level tests that exercise their validation
and error branches: write tests that call check_openai to assert errors when the
OpenAI config is missing or invalid and when an OpenaiClient instance is not
provided, and write tests for generate_sql to assert it raises/returns the
expected validation errors when query_desc or db_type are missing and when
OpenaiClient interaction fails; reference the functions check_openai and
generate_sql in sql/query.py and reuse or mock the OpenaiClient behavior (from
common.utils.openai.OpenaiClient) to simulate success and failure paths.
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
Testing
common,sql, and auth-related modulesSummary by CodeRabbit
New Features
Changes