1.18 ports10 #7285
1.18 ports10 #7285
Conversation
* NU-1901 fix deploy status activity for periodic scenarios
📝 WalkthroughWalkthroughThe changes in this pull request primarily focus on the The Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (2)
designer/client/src/components/toolbars/activities/helpers/activityItemColors.ts (1)
Line range hint
7-10: Consider updating variable names for consistencyThe variable names still use "running" terminology while the logic has moved to "deployment active" concept. Consider renaming for better consistency:
runningActiveFoundHeaderBackground→activeDeploymentFoundHeaderBackgroundrunningHeaderBackground→activeDeploymentHeaderBackgrounddesigner/client/src/components/toolbars/activities/ActivityPanelRowItem/ActivityItemHeader.tsx (1)
Line range hint
169-174: Add JSDoc documentation for the Props interface.Consider adding JSDoc documentation to explain the purpose and expected values of the
isDeploymentActiveprop, especially since it represents a significant change in how deployment status is determined.Example:
interface Props { activity: ItemActivity; /** Indicates whether the activity is actively deployed based on firstDeployedIndex and scenarioState status */ isDeploymentActive: boolean; isActiveFound: boolean; isFound: boolean; searchQuery: string; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
designer/client/src/components/toolbars/activities/ActivitiesPanelRow.tsx(3 hunks)designer/client/src/components/toolbars/activities/ActivityPanelRowItem/ActivityItem.tsx(2 hunks)designer/client/src/components/toolbars/activities/ActivityPanelRowItem/ActivityItemHeader.tsx(4 hunks)designer/client/src/components/toolbars/activities/helpers/activityItemColors.ts(2 hunks)docs/Changelog.md(1 hunks)
🔇 Additional comments (6)
docs/Changelog.md (1)
55-55: LGTM: Changelog entry is clear and follows the standard format.
The entry properly documents a bug fix for deployment status indicators in periodic scenarios, with appropriate PR reference.
designer/client/src/components/toolbars/activities/helpers/activityItemColors.ts (1)
15-16: LGTM! Parameter rename is consistent with the new deployment status logic.
The parameter rename from isRunning to isDeploymentActive accurately reflects the expanded deployment status conditions.
designer/client/src/components/toolbars/activities/ActivityPanelRowItem/ActivityItem.tsx (1)
31-31: LGTM! Prop updates are consistent and maintain type safety.
The prop rename from isRunning to isDeploymentActive is properly implemented in both the type definition and component usage.
Also applies to: 58-58
designer/client/src/components/toolbars/activities/ActivitiesPanelRow.tsx (2)
73-73: LGTM! Dependencies are properly updated.
The useMemo dependencies array correctly includes the renamed isDeploymentActive prop.
30-31: Document the implications of including "SCHEDULED" status
Please document why "SCHEDULED" status is now considered an active deployment and its implications on the UI/UX.
Let's verify the usage of these statuses:
designer/client/src/components/toolbars/activities/ActivityPanelRowItem/ActivityItemHeader.tsx (1)
40-47: LGTM! Improved prop naming for better clarity.
The rename from isRunning to isDeploymentActive better reflects the broader deployment status conditions. The type safety is maintained in the styled component definition.
| const scenarioStatusesToActiveDeploy = ["RUNNING", "SCHEDULED"]; | ||
| const isDeploymentActive = firstDeployedIndex === index && scenarioStatusesToActiveDeploy.includes(scenarioState.status.name); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract status constants to a shared location
Consider moving the status array to a constants file to avoid magic strings and enable reuse:
- const scenarioStatusesToActiveDeploy = ["RUNNING", "SCHEDULED"];
+ import { ACTIVE_DEPLOYMENT_STATUSES } from "../../../constants/scenarioStatuses";
+ // In constants file:
+ export const ACTIVE_DEPLOYMENT_STATUSES = ["RUNNING", "SCHEDULED"] as const;Committable suggestion skipped: line range outside the PR's diff.
| @@ -230,7 +230,7 @@ const WithOpenVersion = ({ | |||
| ); | |||
| }; | |||
|
|
|||
| const ActivityItemHeader = ({ activity, isRunning, isFound, isActiveFound, searchQuery }: Props) => { | |||
| const ActivityItemHeader = ({ activity, isDeploymentActive, isFound, isActiveFound, searchQuery }: Props) => { | |||
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can see that there are no existing test files for the ActivityItemHeader component and its deployment status logic. The code shows that isDeploymentActive is a critical prop that affects the header's styling and behavior through getHeaderColors. Given this finding, I'll generate the final response:
Add tests for ActivityItemHeader's deployment status logic
Please create a new test file ActivityItemHeader.test.tsx to cover:
- Visual verification of header styling based on
isDeploymentActiveprop:- When deployment is active and found (
isDeploymentActive={true}, isActiveFound={true}) - When deployment is active but not found (
isDeploymentActive={true}, isActiveFound={false}) - When deployment is inactive (
isDeploymentActive={false})
- When deployment is active and found (
- Test header color changes using the
getHeaderColorshelper - Verify header content rendering with different activity types during deployment
🔗 Analysis chain
Add tests for the new deployment status logic.
Since this change modifies how deployment status is determined, please ensure there are tests covering:
- Different combinations of
firstDeployedIndexandscenarioState.status.name - Edge cases where deployment status changes
- Visual verification of the header styling based on
isDeploymentActive
This aligns with the PR objective requiring automated test coverage for changes.
Let's check for existing test coverage:
Also applies to: 282-282
Let me search for existing test files specifically related to ActivityItemHeader and deployment status.
Let me search for any existing test files in the activities directory to see if there's test coverage for related components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files related to ActivityItemHeader
fd -e test.tsx -e spec.tsx ActivityItemHeader
# Search for existing test cases related to deployment status
rg -i "test|describe|it.*deployment.*active|running" --type ts
Length of output: 67564
Script:
#!/bin/bash
# Search for test files related to ActivityItemHeader and deployment status
fd -e test.tsx -e spec.tsx ActivityItemHeader
# Search for test cases related to deployment status in ActivityItemHeader
rg -A 5 "isDeploymentActive|deployment.*status" --type ts designer/client/src/components/toolbars/activities
Length of output: 9826
Script:
#!/bin/bash
# Search for test files in the activities directory
fd -e test.tsx -e spec.tsx . designer/client/src/components/toolbars/activities/
# Check for any test coverage of deployment status in existing test files
rg -A 5 "test.*deployment|describe.*deployment" designer/client/test
Length of output: 153
Describe your changes
Checklist before merge
Summary by CodeRabbit
New Features
Bug Fixes
Documentation