korbit-ai[bot] commented on code in PR #32529:
URL: https://github.com/apache/superset/pull/32529#discussion_r1983558808
##########
superset/utils/slack.py:
##########
@@ -54,60 +56,82 @@ def get_slack_client() -> WebClient:
return client
+@cache_util.memoized_func(
+ key="slack_conversations_list",
+ cache=cache_manager.cache,
+)
+def get_channels(limit: int, extra_params: dict[str, Any]) ->
list[SlackChannelSchema]:
Review Comment:
### Missing Cache Timeout in Slack Channels Cache <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The @cache_util.memoized_func decorator is applied without a cache_timeout
parameter, which could lead to stale data being served indefinitely.
###### Why this matters
Without a cache timeout, the Slack channels list will remain cached forever
unless manually refreshed, potentially serving outdated channel information to
users.
###### Suggested change ∙ *Feature Preview*
Add cache_timeout parameter to the decorator to ensure cache is periodically
refreshed:
```python
@cache_util.memoized_func(
key="slack_conversations_list",
cache=cache_manager.cache,
cache_timeout=86400, # 24 hours in seconds
)
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a1f382c1-763c-4d97-bc49-9008a9db0e5b?suggestedFixEnabled=true)
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:2a66ce08-db70-49c9-b4eb-767220d3c8b6 -->
##########
superset-frontend/src/features/alerts/components/NotificationMethod.tsx:
##########
@@ -248,61 +254,84 @@ export const NotificationMethod:
FunctionComponent<NotificationMethodProps> = ({
searchString = '',
types = [],
exactMatch = false,
+ force = false,
}: {
searchString?: string | undefined;
types?: string[];
exactMatch?: boolean | undefined;
+ force?: boolean | undefined;
} = {}): Promise<JsonResponse> => {
- const queryString = rison.encode({ searchString, types, exactMatch });
+ const queryString = rison.encode({
+ searchString,
+ types,
+ exactMatch,
+ force,
+ });
const endpoint = `/api/v1/report/slack_channels/?q=${queryString}`;
return SupersetClient.get({ endpoint });
};
+ const updateSlackOptions = async ({
+ force,
+ }: {
+ force?: boolean | undefined;
+ } = {}) => {
+ if (force) {
+ addInfoToast(
+ t('Fetching Slack channels. This operation may take a while.'),
+ );
+ }
+ fetchSlackChannels({ types: ['public_channel', 'private_channel'], force })
+ .then(({ json }) => {
+ const { result } = json;
+ const options: SlackOptionsType = mapChannelsToOptions(result);
+
+ setSlackOptions(options);
+
+ if (isFeatureEnabled(FeatureFlag.AlertReportSlackV2)) {
+ // for edit mode, map existing ids to names for display if slack v2
+ // or names to ids if slack v1
+ const [publicOptions, privateOptions] = options;
+ if (
+ method &&
+ [
+ NotificationMethodOption.SlackV2,
+ NotificationMethodOption.Slack,
+ ].includes(method)
+ ) {
+ setSlackRecipients(
+ mapSlackValues({
+ method,
+ recipientValue,
+ slackOptions: [
+ ...publicOptions.options,
+ ...privateOptions.options,
+ ],
+ }),
+ );
+ }
+ }
+ })
Review Comment:
### Silent error handling in Slack API call <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The catch block silently swallows the error details when the Slack API call
fails, only setting useSlackV1 to true.
###### Why this matters
Without logging or surfacing the actual error details, debugging Slack API
integration issues becomes difficult and time-consuming.
###### Suggested change ∙ *Feature Preview*
Add error logging and surface the error details to users:
```typescript
.catch(error => {
// Fallback to slack v1 if slack v2 is not compatible
setUseSlackV1(true);
addInfoToast(t('Failed to fetch Slack channels. Falling back to legacy
Slack integration.'));
console.error('Slack channels fetch error:', error);
})
```
</details>
<sub>
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/fbfacb37-51b7-48ea-a7d3-4d02b8b57d9e?suggestedFixEnabled=true)
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:90054be6-621b-4b1b-8945-1b5b94da7b91 -->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
