-
Notifications
You must be signed in to change notification settings - Fork 5
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
fix(kno-5254): hide inaccessible channels #69
Conversation
|
886fc84
to
67b4a48
Compare
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.
one note
packages/react/src/modules/slack/components/SlackChannelCombobox/SlackChannelCombobox.tsx
Outdated
Show resolved
Hide resolved
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
7c07115
to
e9665e0
Compare
useEffect(() => { | ||
setCurrentConnectedChannels(connectedChannels); | ||
}, [connectedChannels]); | ||
// Used to make sure we're only showing currently available channels to select from. | ||
// There are cases where a channel is "connected" in Knock, but it wouldn't be | ||
// posting to it if the channel is private and the Slackbot doesn't belong to it, | ||
// so the channel won't show up here and it won't be posted to. | ||
const slackChannelsMap = new Map( | ||
slackChannels.map((channel) => [channel.id, channel]), | ||
); | ||
|
||
const channels = connectedChannels?.filter((connectedChannel) => { | ||
return slackChannelsMap.has(connectedChannel.channel_id); | ||
}) || []; | ||
|
||
setCurrentConnectedChannels(channels); | ||
}, [connectedChannels, slackChannels]); |
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 think this is definitely fine for now since we're pushing to get this out, but semantically might make more sense for this to run in a useCallback? Can worry about it later!
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.
ok good call, i will come back to this ty!
This handles the case where a user has a slack channel connected in Knock, but it isn't found in the list of Slack channels from the API, potentially because the bot doesn't have access to it (like a private channel it doesn't belong to). In this case it also wouldn't be able to post to the channel so it is accurate to show to the user that it is not connected.
It also updates some minor styling issues.