-
Notifications
You must be signed in to change notification settings - Fork 84
feat: activity tab on schema explorer [ENG-1647] #7162
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: activity tab on schema explorer [ENG-1647] #7162
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
40f657d to
471cb51
Compare
471cb51 to
047cce0
Compare
047cce0 to
9d243cb
Compare
9d243cb to
dc31bbc
Compare
2899a1b to
1764fd9
Compare
fix: title for tests wip: more refactoring refactor: page structure fix: minor style issues refactor: page based wip: still standardizing wip: further refactors refactor: filter related changes linting and other fixes fix: type issue fix: broken imports chore: update changelog fix: more imports
1ef20e5 to
f507531
Compare
Greptile SummaryRefactored action center to support tab-based navigation and added monitor-scoped activity views. Key changes:
Issues:
Confidence Score: 4/5
Important Files Changed
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
19 files reviewed, 3 comments
|
|
||
| return ( | ||
| <Flex className="h-[calc(100%-48px)] overflow-hidden" gap="middle" vertical> | ||
| <Flex className="justify-between "> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: update onChange to use updateSearch from useSearch hook
If switching to useSearch hook, update this to use updateSearch instead of setSearchQuery
| <Flex className="justify-between "> | |
| <DebouncedSearchInput value={searchQuery} onChange={updateSearch} /> |
Context Used: Rule from dashboard - Use the existing useSearch hook for search functionality to maintain URL state synchronization ins... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another miss
| hidden={!!filters?.monitorId} | ||
| value={filters?.monitorId ?? searchQuery ?? ""} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: search input hidden but still shows monitorId as value when filtered
When filters?.monitorId is provided, the search input is hidden but still displays the monitorId as its value. Consider passing undefined or empty string when hidden for consistency
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intentional for the moment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not sure why we need to show a hidden input with a value?
| flags: { webMonitor: webMonitorEnabled, heliosV2: heliosV2Enabled }, | ||
| } = useFeatures(); | ||
| const { paginationProps, pageIndex, pageSize, resetPagination } = | ||
| useAntPagination(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: use useSearch hook instead of local state for search
This component uses local useState for search, but the existing useSearch hook should be used to maintain URL state synchronization (as done in other components like InProgressMonitorTasksList)
| useAntPagination(); | |
| const { searchQuery, updateSearch } = useSearch(); |
Context Used: Rule from dashboard - Use the existing useSearch hook for search functionality to maintain URL state synchronization ins... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrong line. Also was existing code. Trying to reduce the amount of changes in a single pr
lucanovera
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new menu / page routing code looks good. I see the Activity tab but it's got no results on it. Can you include instructions for how to see results in there or a link to the preview where it has results?
| defaultKey?: Exclude<keyof R, symbol | number>; | ||
| } | ||
|
|
||
| const useMenuNavigation = < |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for implementing this as a generic we can reuse. That will be useful when refactoring old tabs with hashes for pages and menus.
| hidden={!!filters?.monitorId} | ||
| value={filters?.monitorId ?? searchQuery ?? ""} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm also not sure why we need to show a hidden input with a value?
Ticket ENG-1647
Description Of Changes
Adds the
Activitytab to monitor screens so that users can view activity specific to a monitor they are working on.Code Changes
Action-Centerto contain a shared layout.Steps to Confirm
Activitytab is visibleActivitytab only displays values specific to the monitor being viewedActivitytabPre-Merge Checklist
CHANGELOG.mdupdatedmaindowngrade()migration is correct and works