-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: add 'create channel' option in channels list and refetch alert channels on opening the channels dropdown #6416
feat: add 'create channel' option in channels list and refetch alert channels on opening the channels dropdown #6416
Conversation
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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.
👍 Looks good to me! Reviewed everything up to 0b7da41 in 1 minute and 3 seconds
More details
- Looked at
164
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/src/hooks/useFetch.ts:33
- Draft comment:
Consider resetting the loading state to true at the beginning of the fetchData function to ensure it accurately reflects the current state of the fetch operation. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests a change that might not be necessary because the loading state is already managed correctly. The fetchData function is only called when state.loading is true, so resetting it to true at the start of fetchData seems redundant. The current logic appears to handle the loading state appropriately, as it starts with loading set to true and only changes to false after a fetch attempt.
I might be missing a scenario where fetchData is called without the loading state being true, but the current useEffect setup seems to ensure that fetchData is only called when loading is true.
Given the current setup, it seems unlikely that fetchData would be called with loading set to false, as the useEffect hook checks for state.loading before calling fetchData.
The comment is unnecessary because the loading state is already managed correctly. The fetchData function is only called when loading is true, making the suggested change redundant.
2. frontend/src/container/FormAlertRules/ChannelSelect/index.tsx:74
- Draft comment:
Avoid using inline styles in React components. Instead, use external stylesheets, CSS classes, or styled components. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_zNZEHcu22GXtPJX9
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
0b7da41
to
4b9a832
Compare
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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.
❌ Changes requested. Incremental review on 4b9a832 in 1 minute and 11 seconds
More details
- Looked at
217
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
5
drafted comments based on config settings.
1. frontend/src/hooks/useFetch.ts:15
- Draft comment:
Consider initializingloading
tofalse
instead ofnull
to avoid confusion, asloading
should represent a binary state:true
(loading) orfalse
(not loading). - Reason this comment was not posted:
Confidence changes required:50%
TheuseFetch
hook is designed to fetch data and manage its state. However, theloading
state is set totrue
initially and is never set back tonull
. This can lead to confusion asloading
should ideally represent a binary state:true
(loading) orfalse
(not loading).
2. frontend/src/hooks/useFetch.ts:23
- Draft comment:
Consider checking if the state is already loading before setting it to loading again to prevent unnecessary state updates. - Reason this comment was not posted:
Confidence changes required:50%
TheuseFetch
hook now includes arefetch
function, which is a good addition. However, thefetchData
function should ensure that it doesn't set the state to loading if it's already loading to prevent unnecessary state updates.
3. frontend/src/hooks/useFetch.ts:57
- Draft comment:
Ensure thatfetchData
's dependencies are correct to avoid unnecessary re-renders. Currently, it depends onfunctions
andparam
, which seems appropriate. - Reason this comment was not posted:
Confidence changes required:33%
TheuseFetch
hook'sfetchData
function is called within auseEffect
hook for the initial fetch. This is a good practice to ensure data is fetched when the component mounts. However, theuseEffect
dependency array includesfetchData
, which is a function created withuseCallback
. This is correct as it ensures the effect runs whenfetchData
changes, but it's important to ensurefetchData
's dependencies are correct to avoid unnecessary re-renders.
4. frontend/src/container/FormAlertRules/ChannelSelect/index.tsx:99
- Draft comment:
Ensure thatrefetchChannels
is not called unnecessarily to avoid performance issues. Currently, it is called only when the dropdown is opened, which is appropriate. - Reason this comment was not posted:
Confidence changes required:33%
TheChannelSelect
component uses theonDropdownVisibleChange
prop to trigger a refetch of channels when the dropdown is opened. This is a good approach to ensure the list is up-to-date. However, it's important to ensure that therefetchChannels
function is not called unnecessarily, which could lead to performance issues.
5. frontend/src/container/FormAlertRules/ChannelSelect/index.tsx:56
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values to maintain consistency in design and theming. This applies to other instances in this file as well. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The code is already using a design token for the color value, which aligns with the comment's suggestion. Therefore, the comment is not applicable to the current code and should be removed.
I might be missing some context about other instances in the file, but based on the provided code, the use ofColor.BG_ROBIN_500
is correct.
The comment specifically mentions hardcoding color values, which is not the case here. The use of a design token is evident, so the comment is not applicable.
The comment is not applicable as the code already uses a design token for the color value. The comment should be removed.
Workflow ID: wflow_JfV5VpaR8MoTyTBR
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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.
👍 Looks good to me! Incremental review on 4ccf527 in 22 seconds
More details
- Looked at
55
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. frontend/src/container/FormAlertRules/ChannelSelect/index.tsx:53
- Draft comment:
Good use of a styled component for consistent styling of the 'Create a new channel' option. - Reason this comment was not posted:
Confidence changes required:0%
The PR introduces a styled componentStyledCreateChannelOption
for styling the 'Create a new channel' option. This is a good practice for maintaining consistent styling and separation of concerns.
2. frontend/src/container/FormAlertRules/ChannelSelect/index.tsx:4
- Draft comment:
Avoid using inline styles. Move thewidth: '100%'
style to theStyledSelect
styled component. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_3LMYJecg0ohskHiF
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
@ahmadshaheer What happens when there's no alert channel exist, please post screen record of the same |
Sure, @makeavish. 2024-11-11.17-02-26.mov |
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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.
👍 Looks good to me! Incremental review on 51c0eaa in 34 seconds
More details
- Looked at
38
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. frontend/src/container/FormAlertRules/ChannelSelect/index.tsx:62
- Draft comment:
Usingwindow.open
can be blocked by pop-up blockers. Consider usinghistory.push
for navigation within the app instead. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_vSqqjdInNnU2cqQz
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
@ahmadshaheer |
Sure, @makeavish, will fix it. Also displaying the Create a notification channel button, in case there are no channels, seems redundant. I think we should remove it. IMO, we can just have an option inside the dropdown to create a new channel. CC: @YounixM |
When alert notification channel doesn't exist then we just disable Create Rule button and don't show it to user that need to create notification channel before creating alert. So we should show that as text, can keep the button. @GeekBoySupreme might have some input on this |
Build Error! No Linked Issue found. Please link an issue or mention it in the body using #<issue_id> |
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.
❌ Changes requested. Incremental review on 2b47da1 in 47 seconds
More details
- Looked at
79
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. frontend/src/container/FormAlertRules/BasicInfo.tsx:97
- Draft comment:
TheuseEffect
dependency array should be empty to ensure the event is logged only once when the page is first visited. Includingchannels.loading
might cause unnecessary re-renders. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is suggesting a change to the dependency array of auseEffect
hook. The current code includeschannels.loading
in the dependency array, which means the effect will run wheneverchannels.loading
changes. The comment suggests this might cause unnecessary re-renders, but the code is designed to log an event only once when the page is first visited, usinghasLoggedEvent
to track this. Removingchannels.loading
might prevent the effect from running when needed.
The comment might be overlooking the purpose of includingchannels.loading
in the dependency array, which is to ensure the effect runs when the loading state changes. Removing it could prevent the event from being logged correctly.
The use ofhasLoggedEvent
should prevent unnecessary re-renders, as the event will only be logged once. The inclusion ofchannels.loading
ensures the effect runs when the loading state changes, which might be necessary for the logic.
The comment is not valid because it overlooks the purpose of includingchannels.loading
in the dependency array. The current implementation ensures the event is logged correctly without unnecessary re-renders.
Workflow ID: wflow_hWTsWZjs8j5XM1vN
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Done 👍 |
@ahmadshaheer : Based on the screen recordings, why does the whole app re-render when we click on create a new channel ?? |
@YounixM, it doesn’t get re-rendered, it opens the channel creation page in a new tab. It seems that isn’t clear in the recording 😅. |
Yes @ahmadshaheer. It's looks like the whole app is reloading. |
@YounixM, it wasn't clear in the recording, I re-recorded it. when we click on new channel, it opens the new channel page in a new tab 🙂. 2024-11-18.09-50-15.mov |
Docs update not needed for this change |
…el-option-in-channels-list
Summary
Related Issues / PR's
close https://github.com/SigNoz/engineering-pod/issues/1979
Screenshots
2024-11-11.11-47-39.mov
Affected Areas and Manually Tested Areas
Note
This PR just addresses the above tasks. the alerts components might need revamp, we can pick that as a separate task
Important
Adds 'Create Channel' option in dropdown and auto-refreshes channel list using refetch in
useFetch
.ChannelSelect
dropdown to redirect to channel creation page.BasicInfo.tsx
usinguseFetch
with refetch capability.useFetch
inuseFetch.ts
to support refetching withfetchData
function.onDropdownOpen
prop toChannelSelect
to trigger refetching channels.This description was created by for 2b47da1. It will automatically update as commits are pushed.