Refactor: Extract root component architecture and add comprehensive test coverage#2296
Refactor: Extract root component architecture and add comprehensive test coverage#2296BChoudhury-ms wants to merge 1 commit intomasterfrom
Conversation
98b19b9 to
9487879
Compare
|
@copilot Review this PR. |
|
@BChoudhury-ms I've opened a new pull request, #2332, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
This PR refactors the root application architecture by extracting monolithic code from Main.tsx into separate, focused components: Root.tsx for theme/provider integration, App.tsx for application lifecycle management, ExplorerContainer.tsx for the main explorer interface, and LoadingExplorer.tsx for loading state display. The refactoring improves code organization, separation of concerns, and adds comprehensive test coverage for all new components.
Changes:
- Extracted root component architecture into separate files in a new
RootComponentsdirectory - Added
SidePanelandDialogcomponents toContainerCopyPanelto consolidate shared UI elements - Exported
MetricScenarioContextValueinterface fromMetricScenarioProviderfor test usage
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/RootComponents/Root.tsx | New root component managing FluentProvider theme integration and error boundary |
| src/RootComponents/Root.test.tsx | Comprehensive test coverage for Root component including theme subscription and component hierarchy |
| src/RootComponents/App.tsx | Main application component handling explorer lifecycle, metrics tracking, and conditional rendering |
| src/RootComponents/App.test.tsx | Comprehensive test coverage for App component including metric scenarios, platform-specific theming, and conditional rendering |
| src/RootComponents/ExplorerContainer.tsx | Container component consolidating all explorer child components and their layout |
| src/RootComponents/ExplorerContainer.test.tsx | Comprehensive test coverage for ExplorerContainer including component rendering and state management |
| src/RootComponents/LoadingExplorer.tsx | Loading state component with accessibility features |
| src/RootComponents/LoadingExplorer.test.tsx | Comprehensive test coverage for LoadingExplorer component |
| src/Main.tsx | Simplified to delegate to new component architecture |
| src/Metrics/MetricScenarioProvider.tsx | Exported interface for test usage |
| src/Explorer/ContainerCopy/ContainerCopyPanel.tsx | Added SidePanel and Dialog components |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@BChoudhury-ms I've opened a new pull request, #2333, to work on those changes. Once the pull request is ready, I'll request review from you. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/Explorer/ContainerCopy/ContainerCopyPanel.tsx:16
- The useEffect dependency array includes
monitorCopyJobsRef.current, which is a mutable ref value. React refs are intentionally mutable and don't trigger re-renders when changed, so this dependency will not work as intended. The effect will only run on initial mount, not when the ref changes. Either remove this dependency (making it run only on mount which seems to be the intent based on the setRef pattern), or if you need to track ref changes, use a state variable instead.
}, [monitorCopyJobsRef.current]);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| features: { enableContainerCopy: false }, | ||
| apiType: "SQL", | ||
| }; | ||
|
|
There was a problem hiding this comment.
The import statement for updateStyles from ../Common/StyleConstants doesn't match the actual named export. The mock uses updateStyles but the code imports * as StyleConstants and calls StyleConstants.updateStyles(). This inconsistency in the test could lead to false positives.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
c47a9fe to
8dcfb8d
Compare
|
@BChoudhury-ms I've opened a new pull request, #2340, to work on those changes. Once the pull request is ready, I'll request review from you. |
f316d6e to
f894c6c
Compare
Preview this branch
This PR introduces a comprehensive refactoring of the root application architecture to improve maintainability, testing, and separation of concerns.
Changes:
Test Coverage: