Skip to content

Comments

feat: react router v6#3059

Open
arunjaindev wants to merge 14 commits intofeat/react-18from
feat/react-router-v6
Open

feat: react router v6#3059
arunjaindev wants to merge 14 commits intofeat/react-18from
feat/react-router-v6

Conversation

@arunjaindev
Copy link
Contributor

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue)

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Test A
  • Test B

Checklist:

  • The title of the PR states what changed and the related issues number (used for the release note).
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

) {
return `${baseGoBackURL}${appendEnvOverridePath ? envOverridePath : ''}`
}
return `${baseGoBackURL}${appendEnvOverridePath ? envOverridePath : ''}/${currentResourceType}${currentResourceName ? `/${currentResourceName}` : ''}`
Copy link
Member

Choose a reason for hiding this comment

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

${baseGoBackURL}${appendEnvOverridePath ? envOverridePath : ''} could be a variable

const getNavItemHref = (resourceType: EnvResourceType, resourceName: string) =>
`${generatePath(routePath, { ...params, resourceType, resourceName })}${location.search}`

const calculatedGoBackURL = useMemo(() => {
Copy link
Member

Choose a reason for hiding this comment

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

is memo required here?

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

{
appId,
envId,
...params,
Copy link
Member

Choose a reason for hiding this comment

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

appId and envId should be after params right?

{
appId,
envId,
...params,
Copy link
Member

Choose a reason for hiding this comment

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

Same here

setGlobalAPITimeout(window._env_.GLOBAL_API_TIMEOUT)
}, [])

useEffect(() => {
Copy link
Member

Choose a reason for hiding this comment

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

Lets keep it for now

import { FILTER_NAME_REGEX } from '../ApplicationGroup/Constants'
import { NO_WORKFLOW_NAME, INVALID_WORKFLOW_NAME, MIN_3CHARS, MAX_30CHARS, SUCCESS_CREATION } from './constants'

export default function EmptyWorkflow(props: EmptyWorkflowProps) {
const params = useParams<{ appId: string }>()
Copy link
Member

Choose a reason for hiding this comment

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

Could destructure here

}
}
const AddWorkflow = ({ onClose, getWorkflows, isTemplateView }: AddWorkflowProps) => {
const params = useParams<{ appId: string; workflowId: string }>()
Copy link
Member

Choose a reason for hiding this comment

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

Here as well

Comment on lines +612 to +615
to={`${generatePath(ROUTER_URLS.INFRASTRUCTURE_MANAGEMENT_APP_DETAIL.EXTERNAL_HELM_APP, {
appId: `1|devtroncd|${appName}`,
appName
})}/${URLS.APP_DETAILS}/${URLS.APP_DETAILS_K8}/pod/${logPodName}/logs`}
Copy link
Member

Choose a reason for hiding this comment

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

${generatePath(ROUTER_URLS.INFRASTRUCTURE_MANAGEMENT_APP_DETAIL.EXTERNAL_HELM_APP, { appId: 1|devtroncd|${appName}, appName })} This much part can be variable and reduced

].map((pathPattern) => (
<Route
key={pathPattern}
path={pathPattern}
Copy link
Member

Choose a reason for hiding this comment

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

Does route not provide support for multiple paths?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not from v6

@@ -250,47 +219,7 @@ const EnvironmentSelectorComponent = ({

<div className="fw-6 fs-14 cb-5">
<div style={{ minWidth: '200px' }}>
{environments && environments.length > 0 && (
Copy link
Member

Choose a reason for hiding this comment

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

Is this and useEffects not required?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope

const location = useLocation()
const { path } = useRouteMatch()
const path = location.pathname
Copy link
Member

Choose a reason for hiding this comment

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

This might be wrong since location would give live location

@@ -105,10 +104,10 @@ const NodeDeleteComponent = ({

function describeNodeWrapper(tab) {
queryParams.set('kind', params.podName)
const updatedPath = `${path.substring(0, path.indexOf('/k8s-resources/'))}/${
const updatedPath = `${location.pathname.substring(0, location.pathname.indexOf('/k8s-resources/'))}/${
Copy link
Member

Choose a reason for hiding this comment

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

Path and location are not interchangable, pls check once

@@ -153,7 +152,7 @@ const NodeComponent = ({
setTableHeader(tableHeaders)

// splitting with /group as group is present in application-group url as well
let [, _selectedResource] = url.split('/group/')
let [, _selectedResource] = location.pathname.split('/group/')
Copy link
Member

Choose a reason for hiding this comment

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

Pls check since it was coming from useRouteMatch

@@ -180,7 +179,7 @@ const NodeComponent = ({

setSelectedHealthyNodeCount(_healthyNodeCount)
}
}, [params.nodeType, podType, url, filteredNodes, podLevelExternalLinks])
}, [params.nodeType, podType, location.pathname, filteredNodes, podLevelExternalLinks])
Copy link
Member

Choose a reason for hiding this comment

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

same here

@@ -221,7 +220,7 @@ const NodeComponent = ({
}

const handleActionTabClick = (node: iNode, _tabName: string, containerName?: string) => {
let [_url] = url.split('/group/')
let [_url] = location.pathname.split('/group/')
Copy link
Member

Choose a reason for hiding this comment

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

Here as well

@@ -125,13 +125,13 @@ const TerminalComponent = ({
}

useEffect(() => {
selectedTab(NodeDetailTab.TERMINAL, url)
selectedTab(NodeDetailTab.TERMINAL, pathname)
Copy link
Member

Choose a reason for hiding this comment

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

Are these same?

handleAbort()
}, [params.podName, params.name, params.namespace])

useEffect(() => {
if (showTerminal) {
selectedTab(NodeDetailTab.TERMINAL, `${url}/terminal`)
selectedTab(NodeDetailTab.TERMINAL, `${pathname}/terminal`)
Copy link
Member

Choose a reason for hiding this comment

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

Same here

@@ -257,7 +256,7 @@ const ManifestComponent = ({
}

useEffect(() => {
selectedTab(NodeDetailTab.MANIFEST, url)
selectedTab(NodeDetailTab.MANIFEST, location.pathname)
Copy link
Member

Choose a reason for hiding this comment

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

pls check this

@@ -431,10 +435,16 @@ const LogsComponent = ({

useEffect(() => {
if (selectedTab) {
selectedTab(NodeDetailTab.LOGS, url)
selectedTab(NodeDetailTab.LOGS, location.pathname)
Copy link
Member

Choose a reason for hiding this comment

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

pls check

const [events, setEvents] = useState([])
const [loading, setLoading] = useState(true)
const appDetails = IndexStore.getAppDetails()

useEffect(() => {
selectedTab(NodeDetailTab.EVENTS, url)
selectedTab(NodeDetailTab.EVENTS, location.pathname)
Copy link
Member

Choose a reason for hiding this comment

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

here as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this will work fine

@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

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