Skip to content

Conversation

ChiefWoods
Copy link

Notifications that are read remain on the list. Some users may wish to keep their lists clean. The addition of a 'clear notification' button now allows users to wipe their history for the current app.

Notes:

  • useClearHistory is closely based on useReadHistory

Copy link

vercel bot commented Sep 24, 2025

@ChiefWoods is attempting to deploy a commit to the dialect Team on Vercel.

A member of the Team first needs to authorize it.

@ChiefWoods ChiefWoods changed the base branch from master to api-v2 September 24, 2025 10:59
const sdk = useDialectSdk();

const { trigger, isMutating, error } = useSWRMutation(
appId && clientKey ? CACHE_KEY_READ_MUTATION(appId) : null,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please create a separate key for history clear mutation

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed that, added a new fix commit

@fsher
Copy link
Collaborator

fsher commented Sep 26, 2025

@ChiefWoods thanks for the PR! Haven't checked the UI/UX changes yet, but the code looks good

@fsher fsher requested a review from Copilot September 26, 2025 11:23
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds a "clear notification history" feature that allows users to remove all notifications from their current app, keeping their notification list clean. The feature is accessible through a trash icon button in the notifications header.

  • Implements useClearHistory hook to handle API calls for clearing notification history
  • Adds a clear notifications button to the header component with trash icon
  • Integrates clear functionality with history refresh to update the UI after clearing

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
packages/react-sdk/src/hooks/useClearHistory.ts New hook for clearing notification history via API
packages/react-sdk/src/hooks/index.ts Exports the new useClearHistory hook
packages/react-ui/src/ui/core/Header.tsx Adds clear notifications button with trash icon
packages/react-ui/src/ui/notifications/NotificationsFeed/NotificationsFeedHeader.tsx Integrates clear functionality with header
packages/react-ui/src/ui/notifications/NotificationsFeed/NotificationsFeed.tsx Adds clear error handling to feed container

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

throw await response.json();
}

return response.json() as Promise<void>;
Copy link

Copilot AI Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return type casting is incorrect. response.json() returns Promise<any>, but you're casting it as Promise<void>. Since the API likely returns some response data, either remove the type assertion or change it to Promise<any> or the actual expected response type.

Suggested change
return response.json() as Promise<void>;
return response.json();

Copilot uses AI. Check for mistakes.

onSettingsClick={() => setRoute(Route.Settings)}
onCloseClick={() => setOpen?.(false)}
onClearNotificationsClick={() => {
clear().then(() => refresh());
Copy link

Copilot AI Sep 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing error handling for the clear operation. If clear() fails, the promise chain will be rejected but there's no catch handler to prevent unhandled promise rejection.

Suggested change
clear().then(() => refresh());
clear()
.then(() => refresh())
.catch((err) => {
console.error('Failed to clear notifications:', err);
});

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants