-
Notifications
You must be signed in to change notification settings - Fork 165
refactor: Code quality fix #700
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR focuses on code-quality cleanups and robustness improvements across backend services, utility scripts, frontend components, and test suites—primarily by tightening exception handling, improving logging/observability, and removing unused imports/variables.
Changes:
- Improved exception handling/logging in backend orchestrator and Cosmos service to avoid silent failures and increase observability.
- Cleaned up unused imports/variables across docs/scripts/frontend to reduce clutter and potential confusion.
- Refined E2E/test logic for better reliability and readability (e.g., clearer branching, improved section removal selection).
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/generate_architecture_png.py | Removes unused locals and replaces bare except with specific exception handling for font loading. |
| docs/generate_architecture.py | Removes unused Azure network diagram import to simplify dependencies. |
| content-gen/tests/test_agents.py | Removes unused test imports to reduce noise in the unit test module. |
| content-gen/tests/rai_testing.py | Cleans up imports/flags and adjusts report output path printing (but introduces an auth dependency regression—see PR comments). |
| content-gen/src/backend/services/cosmos_service.py | Adds warning logging instead of silently swallowing cross-partition query failures. |
| content-gen/src/backend/orchestrator.py | Adds debug logging around best-effort JSON parsing and improves observability when compliance parsing fails. |
| content-gen/src/backend/app.py | Removes unused inline asyncio imports in request handlers. |
| content-gen/scripts/test_content_generation.py | Removes unused json import. |
| content-gen/scripts/sample_content_generation.py | Removes unused os import and avoids unused assignment when awaiting generation. |
| content-gen/scripts/post_deploy.py | Removes unused imports and tightens typing imports. |
| content-gen/scripts/create_image_search_index.py | Removes unused json import. |
| archive-doc-gen/tests/e2e-test/tests/test_st_docgen_tc.py | Refactors E2E test logic (exception specificity, section selection logic, logging/branch tweaks). |
| archive-doc-gen/tests/e2e-test/pages/draftPage.py | Removes unused os import. |
| archive-doc-gen/src/frontend/src/pages/chat/Chat.tsx | Removes unused Fluent UI imports to simplify the component import surface. |
| archive-doc-gen/src/frontend/src/components/ChatHistory/chatHistoryListItem.test.tsx | Adjusts rename test mocking/formatting (but leaves a test intent mismatch—see PR comments). |
| archive-doc-gen/src/frontend/src/components/ChatHistory/ChatHistoryListItem.tsx | Removes unused Fluent UI tooltip styles import. |
| archive-doc-gen/src/backend/settings.py | Attempts to ensure parent initializer execution for datasource settings (but introduces duplicate __init__ issue—see PR comments). |
Comments suppressed due to low confidence (1)
archive-doc-gen/src/backend/settings.py:262
- This class does not call DatasourcePayloadConstructor.init during initialization. (_AzureSearchSettings.init may be missing a call to a base class init)
This class does not call DatasourcePayloadConstructor.init during initialization. (_AzureSearchSettings.init may be missing a call to a base class init)
class _AzureSearchSettings(BaseSettings, DatasourcePayloadConstructor):
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Azure Identity is optional; authentication features depending on it will be unavailable. | ||
| pass | ||
|
|
||
|
|
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The azure.identity import is now silently ignored (except ImportError: pass), but later _get_auth_headers() unconditionally references AzureCliCredential / InteractiveBrowserCredential. If azure-identity isn't installed and --use-azure-auth is used, this will raise NameError instead of cleanly disabling auth. Reintroduce an availability flag/guard, or move the import inside _get_auth_headers() and return a clear error when the dependency is missing.
| # Azure Identity is optional; authentication features depending on it will be unavailable. | |
| pass | |
| # Azure Identity is optional; provide stubs that fail clearly if Azure auth is requested. | |
| class _MissingAzureIdentityCredential: | |
| def __init__(self, *args, **kwargs) -> None: | |
| raise RuntimeError( | |
| "The 'azure-identity' package is required to use '--use-azure-auth'. " | |
| "Install it with 'pip install azure-identity' and try again." | |
| ) | |
| AzureCliCredential = _MissingAzureIdentityCredential | |
| InteractiveBrowserCredential = _MissingAzureIdentityCredential |
| def __init__(self, *args, settings: "_AppSettings", **data): | ||
| # Ensure DatasourcePayloadConstructor.__init__ runs so that _settings is initialized | ||
| super().__init__(*args, settings=settings, **data) | ||
|
|
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_AzureSearchSettings now defines __init__ twice; the later definition overwrites the earlier one, so the newly added initializer (meant to ensure DatasourcePayloadConstructor.__init__ runs) will never execute. Consolidate to a single init strategy (e.g., set _settings in model_post_init, or explicitly initialize the private attr once) so _settings is reliably set before construct_payload_configuration() uses it.
| except Exception as parse_error: | ||
| # Best-effort JSON extraction from markdown code block; on failure, | ||
| # fall back to the original prompt_text without interrupting image generation. | ||
| logger.debug(f"Failed to parse JSON from markdown code block for image prompt: {parse_error}") |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This debug log uses an f-string, which eagerly formats even when debug logging is disabled. Prefer parameterized logging (logger.debug("… %s", parse_error)) and/or exc_info=True if you want traceback details.
| logger.debug(f"Failed to parse JSON from markdown code block for image prompt: {parse_error}") | |
| logger.debug( | |
| "Failed to parse JSON from markdown code block for image prompt: %s", | |
| parse_error, | |
| ) |
| test('shows error when trying to rename to an existing title', async () => { | ||
| const existingTitle = 'Existing Chat Title' | ||
| ; (historyRename as jest.Mock).mockResolvedValueOnce({ | ||
| ok: false, | ||
| json: async () => ({ message: 'Title already exists' }) | ||
| }) | ||
| (historyRename as jest.Mock).mockResolvedValueOnce({ | ||
| ok: false, | ||
| json: async () => ({ message: 'Title already exists' }) | ||
| }) |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is titled as if it verifies renaming to an existing title, but it doesn’t actually attempt a rename to another conversation’s title (it changes the input to the current title) and the historyRename mock becomes effectively unused. Either update the test name/assertions to match the “unchanged title” validation, or change the input to a genuinely different existing title and assert on the API-driven error message.
| assert thread_count > 0, "No chat history threads found to hover over" | ||
|
|
||
| if thread_count > 0: | ||
| if thread_count <= 0: |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test is always false, because of this condition.
| if current_responses_empty == initial_responses: | ||
| logger.info("✅ System did not accept empty query - no response generated") | ||
| else: | ||
| if current_responses_empty != initial_responses: |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test is always false, because of this condition.
| from diagrams.azure.database import CosmosDb, BlobStorage | ||
| from diagrams.azure.ml import CognitiveServices | ||
| from diagrams.azure.network import VirtualNetworks, PrivateEndpoint, DNSZones | ||
| from diagrams.azure.network import PrivateEndpoint, DNSZones |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'DNSZones' is not used.
| from diagrams.azure.network import PrivateEndpoint, DNSZones | |
| from diagrams.azure.network import PrivateEndpoint |
| from dataclasses import dataclass | ||
| from pathlib import Path | ||
| from typing import Optional, List, Dict, Any | ||
| from typing import List, Dict, Any |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Import of 'List' is not used.
Import of 'Any' is not used.
| from typing import List, Dict, Any | |
| from typing import Dict |
|
|
||
| if thread_count > 0: | ||
| if thread_count <= 0: | ||
| logger.warning("Skipping hover action: no chat history threads available.") |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This statement is unreachable.
Purpose
This pull request includes a variety of improvements and cleanups across the backend, frontend, and test codebases. The most significant changes focus on improving error handling and logging, refactoring exception usage, cleaning up unused imports, and making tests more robust and readable.
Backend Improvements:
orchestrator.pyandcosmos_service.py, by catching specific exceptions, logging debug or warning messages, and avoiding silent failures. This increases observability and maintainability. [1] [2] [3] [4]asyncioandjson) from several backend scripts and modules, reducing clutter and potential confusion. [1] [2] [3] [4] [5] [6]Frontend and TypeScript Updates:
Testing and E2E Test Improvements:
Exception) instead of using bareexcept, improving error clarity and debugging. [1] [2] [3] [4]Miscellaneous Cleanups:
_AzureSearchSettingsto ensure proper initialization of parent classes.sample_content_generation.pyto avoid unused assignment.These changes collectively improve code quality, maintainability, and robustness across the project.
Does this introduce a breaking change?
Golden Path Validation
Deployment Validation
What to Check
GP Testing