fix(console): move tabList into component to avoid out of context#293
fix(console): move tabList into component to avoid out of context#293
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the console Assets page tab configuration to be created within the ListAssets component (addressing “out of context” usage), and aligns generated API client/query param typings with backend support for the tlsHosts filter on asset list endpoints.
Changes:
- Move
tabListdefinition intoListAssetsso tab JSX elements are created in-component. - Add
tlsHosts?: string[]to several Assets query parameter types in generated API clients (worker + console).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
worker/services/core-api/api.ts |
Adds tlsHosts to assets-related request query typings in the generated worker API client. |
console/src/services/apis/gen/queries.ts |
Adds tlsHosts to assets-related query param typings in the generated console API hooks. |
console/src/pages/assets/list-assets.tsx |
Moves tabList into the ListAssets component scope. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const tabList = [ | ||
| { | ||
| value: 'service', | ||
| text: 'All Services', | ||
| tab: <AssetTabContent />, | ||
| }, | ||
| { |
There was a problem hiding this comment.
tabList is now created on every render of ListAssets, which can cause avoidable rerenders/allocations (especially if TriggerList or tab content ever becomes memoized). Consider wrapping the tabList definition in useMemo (or splitting triggers vs content) so the array reference stays stable across renders.
| const tabList = [ | ||
| { | ||
| value: 'service', | ||
| text: 'All Services', | ||
| tab: <AssetTabContent />, | ||
| }, | ||
| { | ||
| value: 'technology', | ||
| text: 'Technologies', | ||
| tab: <TechnologyAssetsTab />, | ||
| }, | ||
| { | ||
| value: 'ip', | ||
| text: 'IP Addresses', | ||
| tab: <IpAssetsTab />, | ||
| }, | ||
| { | ||
| value: 'port', | ||
| text: 'Ports', | ||
| tab: <PortAssetsTab />, | ||
| }, | ||
| { | ||
| value: 'host', | ||
| text: 'Hosts', | ||
| tab: <HostAssetsTab />, | ||
| }, | ||
| { | ||
| value: 'status-code', | ||
| text: 'Status Code', | ||
| tab: <StatusCodeAssetsTab />, | ||
| }, | ||
| { | ||
| value: 'tls', | ||
| text: 'TLS', | ||
| tab: <TlsAssetsTab />, | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
TriggerList’s prop type only includes { value, text, isNew? }, but tabList items also include a tab field. TypeScript allows passing this because it’s not an object literal, but it can hide mistakes and makes the contract unclear. Consider either (1) mapping to the expected shape when passing to TriggerList, or (2) defining a shared TabItem type that both ListAssets and TriggerList use.
| hosts?: string[]; | ||
| techs?: string[]; | ||
| statusCodes?: string[]; | ||
| tlsHosts?: string[]; | ||
| }; |
There was a problem hiding this comment.
The PR title indicates this change is about moving tabList in the console, but this PR also updates generated API query params by adding tlsHosts. Please update the PR title/description (or split into separate PRs) so the change set is discoverable in review/release notes.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly moves the tabList constant inside the ListAssets component to resolve a React context issue. Additionally, it introduces a new tlsHosts filter parameter across various asset-related API endpoints, which is reflected in the generated client code.
My main feedback is a suggestion to wrap the tabList initialization in a useMemo hook. This will prevent it from being recreated on every render, which is a good practice for performance optimization.
Note: Security Review did not run due to the size of the PR.
| const tabList = [ | ||
| { | ||
| value: 'service', | ||
| text: 'All Services', | ||
| tab: <AssetTabContent />, | ||
| }, | ||
| { | ||
| value: 'technology', | ||
| text: 'Technologies', | ||
| tab: <TechnologyAssetsTab />, | ||
| }, | ||
| { | ||
| value: 'ip', | ||
| text: 'IP Addresses', | ||
| tab: <IpAssetsTab />, | ||
| }, | ||
| { | ||
| value: 'port', | ||
| text: 'Ports', | ||
| tab: <PortAssetsTab />, | ||
| }, | ||
| { | ||
| value: 'host', | ||
| text: 'Hosts', | ||
| tab: <HostAssetsTab />, | ||
| }, | ||
| { | ||
| value: 'status-code', | ||
| text: 'Status Code', | ||
| tab: <StatusCodeAssetsTab />, | ||
| }, | ||
| { | ||
| value: 'tls', | ||
| text: 'TLS', | ||
| tab: <TlsAssetsTab />, | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
While moving tabList inside the component is the correct fix for the context issue, it will be recreated on every render of ListAssets. To optimize performance and prevent unnecessary re-renders of child components, you should memoize this constant array using useMemo. You will also need to import useMemo from react.
const tabList = useMemo(
() => [
{
value: 'service',
text: 'All Services',
tab: <AssetTabContent />,
},
{
value: 'technology',
text: 'Technologies',
tab: <TechnologyAssetsTab />,
},
{
value: 'ip',
text: 'IP Addresses',
tab: <IpAssetsTab />,
},
{
value: 'port',
text: 'Ports',
tab: <PortAssetsTab />,
},
{
value: 'host',
text: 'Hosts',
tab: <HostAssetsTab />,
},
{
value: 'status-code',
text: 'Status Code',
tab: <StatusCodeAssetsTab />,
},
{
value: 'tls',
text: 'TLS',
tab: <TlsAssetsTab />,
},
],
[],
);
l1ttps
left a comment
l1ttps
left a comment
There was a problem hiding this comment.
Co-authored-by: Quang Vinh <32523515+l1ttps@users.noreply.github.com>
* feat(console, admin): add list users for administration * feat(console): redesign user detail sheet * fix(console): fix sorting for server datatable * refactor(console): restyle user-detail-sheet * feat(console): add confirm dialog for action ban * feat(console): add user detail section * fix(console): fix small typo in tls tab (#290) * feat(asset): add tls filter for asset * fix(core): fix asset test * fix(asset): fix based on bot reviews * fix(console): fix small typo in tls tab * fix(console): add missing tls for queryParams in asset context * fix(console): fix tls query hook in dashboard --------- Co-authored-by: Quang Vinh <32523515+l1ttps@users.noreply.github.com> * chore(deps): bump multer from 2.0.2 to 2.1.0 (#292) Bumps [multer](https://github.com/expressjs/multer) from 2.0.2 to 2.1.0. - [Release notes](https://github.com/expressjs/multer/releases) - [Changelog](https://github.com/expressjs/multer/blob/main/CHANGELOG.md) - [Commits](expressjs/multer@v2.0.2...v2.1.0) --- updated-dependencies: - dependency-name: multer dependency-version: 2.1.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * feat(targets): create multiple targets (#291) * refactor(targets): combine logic create target in transaction * feat(targets): add bulk target creation endpoint * feat(targets): add bulk creation support * fix(console): move tabList into component to avoid out of context (#293) Co-authored-by: Quang Vinh <32523515+l1ttps@users.noreply.github.com> * fix(assets): select asset relations in query (#297) * refactor(ui): improve flow onboarding with first workspace creation and re-design settings ui (#299) * feat(console): add all workspaces navigation and improve 404 page UI * refactor(layout): extract header into dedicated HeaderBar component * refactor(console): add workspace-aware header layout * refactor(console): convert workspace creation to page and add route protection * refactor(console): update workspaces UI from table to card layout * feat(workspaces): add member and target counts to workspace list * refactor(settings): reorganize settings page with sidebar layout * feat(settings): add API keys management * refactor(settings): improve API key display layout * fix(screenshot-cell): add type assertion for screenshotPath * refactor(workspaces): use workspace selector hook * feat(auth): add session retry with exponential backoff * chore(agent): migrate ai agent * feat(router): add admin users route * feat(console): implement create user * feat(console): add change name, email and reset password in user detail * fix(console): fix duplicate tlsHosts in context * fix(console): use loading state of data table and improve client user type * fix(console): add admin route * feat(console): Implement role-based access control for settings tabs and sidebar menu items based on user roles. * style(console): update 'Add User' button to outline variant * refactor(console): move add user button to table toolbar * fix(console): add autoComplete to user detail input --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: Quang Vinh <32523515+l1ttps@users.noreply.github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: l1ttps <l1ttps443@gmail.com>
No description provided.