Skip to content

Minh small bugs#6

Open
minhNgnn wants to merge 5 commits into
mainfrom
minh-small-bugs
Open

Minh small bugs#6
minhNgnn wants to merge 5 commits into
mainfrom
minh-small-bugs

Conversation

@minhNgnn

Copy link
Copy Markdown
Collaborator

see commits. small bugs so should be safe to merge

@minhNgnn minhNgnn requested review from Feguna-1103 and Copilot June 22, 2025 07:59

Copilot AI left a comment

Copy link
Copy Markdown

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 removes the conflict style selection step and refines the industry icon logic in the scenario selector, while updating the navigation flow in the configuration panel.

  • Replaced the generic industry icon with a mapping of industry names to specific icons in ScenarioSelector.
  • Deleted the unused ConflictStyleSelector component and removed its step from the configuration flow.
  • Adjusted ConfigurationPanel to a two-step setup and updated button rendering/handlers accordingly.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
frontend/src/components/configuration/ScenarioSelector.tsx Added industryIcons map and dynamic icon rendering for each industry
frontend/src/components/configuration/ConflictStyleSelector.tsx Removed the entire unused component
frontend/src/components/configuration/ConfigurationPanel.tsx Dropped conflict styles step and revamped navigation buttons
Comments suppressed due to low confidence (2)

frontend/src/components/configuration/ScenarioSelector.tsx:92

  • The dynamic icon selection and fallback logic would benefit from unit tests. Add tests to verify that each known industry name renders the correct icon and that unknown names fall back to the default.
            {industries.map((industry) => {

frontend/src/components/configuration/ScenarioSelector.tsx:9

  • [nitpick] The config prop is typed as any, which can hide shape mismatches. Consider defining and using a specific interface (e.g. ScenarioConfig) to improve type safety and maintainability.
  config: any;

{currentStep === 1 && isConfigurationComplete() && (
<>
<Button
onClick={() => onComplete(config)}

Copilot AI Jun 22, 2025

Copy link

Choose a reason for hiding this comment

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

The "Run AI vs AI Demo" button is calling onComplete but should likely invoke the quick practice callback (onQuickPractice), while the "Start Practice Session" button triggers onQuickPractice via handleNext. Please swap these handlers so each button calls its intended prop.

Copilot uses AI. Check for mistakes.
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.

2 participants