[Security Solution][Cases] Fix cases breadcrumbs

[michaelolo24]

Summary

This PR fixes the broken breadcrumbs in the new cases plugin, and makes some minor fixes to the README. Instead of adding a spyRoute component, just generalized it to nonVisibleRenderWithCaseData (naming is hard... 😂). Totally up for suggestions on how this can be improved

[elasticmachine]

Pinging @elastic/security-solution (Team: SecuritySolution)

[elasticmachine]

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

[michaelolo24]

Only changes here were escaping some errant pipes | and removing some errant colons :

[stephmilovic]

nice, i swear i tried escaping but guess I did a bad job ¯_(ツ)_/¯

[michaelolo24]

still have a JSX.Element in the return given SpyRoute

[cnasikas]

Hey! Thanks for the fix. I don't understand why cases should bother with the breadcrumb? Shouldn't be the responsibility of the security solution plugin? I think the cases component should be as pure as possible.

I think we can do it as:

// Inside: x-pack/plugins/security_solution/public/cases/components/case_view/index.tsx

export const CaseView = React.memo(({ caseId, subCaseId, userCanCrud }: Props) => {

// A lot of code doing stuff

return <>
       {casesUi.getCaseView({...})}
       <SpyRoute state={{ caseTitle: caseData.title }} pageName={SecurityPageName.case} />;
   </>

}

and remove the nonVisibleRenderWithCaseData.

[michaelolo24]

Hey! Thanks for the fix. I don't understand why cases should bother with the breadcrumb? Shouldn't be the responsibility of the security solution plugin? I think the cases component should be as pure as possible.

I think we can do it as:

// Inside: x-pack/plugins/security_solution/public/cases/components/case_view/index.tsx

export const CaseView = React.memo(({ caseId, subCaseId, userCanCrud }: Props) => {

// A lot of code doing stuff

return <>
       {casesUi.getCaseView({...})}
       <SpyRoute state={{ caseTitle: caseData.title }} pageName={SecurityPageName.case} />;
   </>

}

and remove the nonVisibleRenderWithCaseData.

I 1000% agree @cnasikas. I really don't like nonVisibleRenderWithCaseData, but it depends on caseData.title which is requested in the caseView component. I could surface that request up a level, but then any consuming app of cases would have to do that request. Alternatively, I could create a request for just the title in caseView, just feels weird requesting data twice in the same view. Hence, the solution of just having a general callback for a consuming app to do whatever they may need with that case data. I might make it a hook instead of a component. 🤔

[cnasikas]

Hey! Thanks for the fix. I don't understand why cases should bother with the breadcrumb? Shouldn't be the responsibility of the security solution plugin? I think the cases component should be as pure as possible.
I think we can do it as:

// Inside: x-pack/plugins/security_solution/public/cases/components/case_view/index.tsx

export const CaseView = React.memo(({ caseId, subCaseId, userCanCrud }: Props) => {

// A lot of code doing stuff

return <>
       {casesUi.getCaseView({...})}
       <SpyRoute state={{ caseTitle: caseData.title }} pageName={SecurityPageName.case} />;
   </>

}

and remove the nonVisibleRenderWithCaseData.

I 1000% agree @cnasikas. I really don't like nonVisibleRenderWithCaseData, but it depends on caseData.title which is requested in the caseView component. I could surface that request up a level, but then any consuming app of cases would have to do that request. Alternatively, I could create a request for just the title in caseView, just feels weird requesting data twice in the same view. Hence, the solution of just having a general callback for a consuming app to do whatever they may need with that case data. I might make it a hook instead of a component. 🤔

I see what you mean. I knew there was a good reason but I missed that. It feels awkward that is a component and not a callback function. That's why it filled strange to me and wanted to pointed out. What about having this prop onCaseFetchSuccess?: (theCase: Case) => void instead of nonVisibleRenderWithCaseData?

Example:

// Inside: x-pack/plugins/security_solution/public/cases/components/case_view/index.tsx

export const CaseView = React.memo(({ caseId, subCaseId, userCanCrud }: Props) => {

// A lot of code doing stuff

[caseTitle, setCaseTitle] = useState(null)

const onCaseFetchSuccess = useCallback((theCase: Case) => {
   setCaseTitle(theCase.title)
}, [...])

return <>
       {casesUi.getCaseView({..., onCaseFetchSuccess})}
       caseTitle && <SpyRoute state={{ caseTitle }} pageName={SecurityPageName.case} />;
   </>

}

[michaelolo24]

Hey! Thanks for the fix. I don't understand why cases should bother with the breadcrumb? Shouldn't be the responsibility of the security solution plugin? I think the cases component should be as pure as possible.
I think we can do it as:

// Inside: x-pack/plugins/security_solution/public/cases/components/case_view/index.tsx

export const CaseView = React.memo(({ caseId, subCaseId, userCanCrud }: Props) => {

// A lot of code doing stuff

return <>
       {casesUi.getCaseView({...})}
       <SpyRoute state={{ caseTitle: caseData.title }} pageName={SecurityPageName.case} />;
   </>

}

and remove the nonVisibleRenderWithCaseData.

I 1000% agree @cnasikas. I really don't like nonVisibleRenderWithCaseData, but it depends on caseData.title which is requested in the caseView component. I could surface that request up a level, but then any consuming app of cases would have to do that request. Alternatively, I could create a request for just the title in caseView, just feels weird requesting data twice in the same view. Hence, the solution of just having a general callback for a consuming app to do whatever they may need with that case data. I might make it a hook instead of a component. 🤔

I see what you mean. I knew there was a good reason but I missed that. It feels awkward that is a component and not a callback function. That's why it filled strange to me and wanted to pointed out. What about having this prop onCaseFetchSuccess?: (theCase: Case) => void instead of nonVisibleRenderWithCaseData?

Example:

// Inside: x-pack/plugins/security_solution/public/cases/components/case_view/index.tsx

export const CaseView = React.memo(({ caseId, subCaseId, userCanCrud }: Props) => {

// A lot of code doing stuff

[caseTitle, setCaseTitle] = useState(null)

const onCaseFetchSuccess = useCallback((theCase: Case) => {
   setCaseTitle(theCase.title)
}, [...])

return <>
       {casesUi.getCaseView({..., onCaseFetchSuccess})}
       caseTitle && <SpyRoute state={{ caseTitle }} pageName={SecurityPageName.case} />;
   </>

}

I like 👍🏾

[stephmilovic]

Nicely done! Looks like @cnasikas beat me to the review but I'll go ahead and take those internet points anyways. Very clean, I like. Thanks to you both 🏁

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: ee1b42c
• Storybooks Preview

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [636524a]

History

• 💚 Build #119446 succeeded 2d567f1
• 💔 Build #119391 failed 1dfb74b

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream