Skip to content

feat: support clicking post boost notification #4691

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

Merged
merged 6 commits into from
Jul 18, 2025
Merged

Conversation

sshanzel
Copy link
Member

@sshanzel sshanzel commented Jul 15, 2025

Changes

  • When a campaign is finished, we send a notification, we should support clicking it and opening the modal.

Events

Did you introduce any new tracking events?

Experiment

Did you introduce any new experiments?

Manual Testing

Caution

Please make sure existing components are not breaking/affected by this PR

Jira ticket

MI-888

Preview domain

https://mi-888-notification.preview.app.daily.dev

Copy link

vercel bot commented Jul 15, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
daily-webapp ✅ Ready (Inspect) Visit Preview Jul 18, 2025 11:38am
1 Skipped Deployment
Name Status Preview Updated (UTC)
storybook ⬜️ Ignored (Inspect) Jul 18, 2025 11:38am

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

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

Bug: Modal Hook Fails with Array Query Parameters

The useCampaignByIdModal hook unsafely casts campaignId to string. Next.js router query parameters (c_id, post_boost) can be string | string[] | undefined. If multiple c_id or post_boost parameters are present in the URL, the corresponding value will be an array (e.g., ['123', '456']). The as string assertion converts this array to a comma-separated string (e.g., '123,456'), which is not the expected single ID format for the FetchBoostedPostView modal, leading to incorrect behavior.

packages/shared/src/hooks/notifications/useCampaignByIdModal.ts#L10-L23

const {
query: { post_boost: postBoost, c_id: campaignId },
replace,
pathname,
} = useRouter();
useEffect(() => {
if (!user || !postBoost || !campaignId) {
return;
}
openModal({
type: LazyModal.FetchBoostedPostView,
props: {
campaignId: campaignId as string,

Fix in CursorFix in Web


Comment bugbot run to trigger another review on this PR
Was this report helpful? Give feedback by reacting with 👍 or 👎

Copy link
Contributor

@rebelchris rebelchris left a comment

Choose a reason for hiding this comment

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

Joh more and more listeners bloating :(

@sshanzel
Copy link
Member Author

Joh more and more listeners bloating :(

Yeah, not really liking where this is going. Thankfully, we have plans to build a dedicated page for campaigns. Soon we will replace this.

@sshanzel sshanzel merged commit 7953705 into main Jul 18, 2025
10 of 11 checks passed
@sshanzel sshanzel deleted the MI-888-notification branch July 18, 2025 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants