Adding notification for planning questionnaire#3028
Conversation
|
@BugBot run |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the user experience by adding a new native notification for planning questionnaires, ensuring users are promptly alerted when their input is needed, even when the application is in the background. It also refines existing notification-related text across the application to be more comprehensive and accurate, reflecting the broader range of events for which notifications are provided. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Greptile SummaryThis PR extends the existing chat-completion notification system to also fire a native OS notification when a planning questionnaire needs user input. It renames the Key changes:
Issues found:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant PlanEventClient
participant usePlanEvents
participant QueryClient
participant Notification API
User->>PlanEventClient: Triggers plan questionnaire
PlanEventClient->>usePlanEvents: onQuestionnaire(payload)
usePlanEvents->>usePlanEvents: setPendingQuestionnaire(payload)
usePlanEvents->>usePlanEvents: Check enableChatEventNotifications & Notification.permission & !document.hasFocus()
alt Notifications enabled & window not focused
usePlanEvents->>QueryClient: getQueryData(apps.detail(selectedAppId))
QueryClient-->>usePlanEvents: app (or null)
usePlanEvents->>Notification API: new Notification(app.name ?? "Dyad", { body: "A questionnaire needs your input" })
Notification API-->>User: Native OS notification
end
Note over usePlanEvents: Chat completion path (useStreamChat.ts)
PlanEventClient->>usePlanEvents: onEnd(response)
usePlanEvents->>usePlanEvents: Check enableChatEventNotifications & Notification.permission & !document.hasFocus()
alt Notifications enabled & window not focused & not cancelled
usePlanEvents->>QueryClient: getQueryData(apps.detail(selectedAppId))
QueryClient-->>usePlanEvents: app, chat summary
usePlanEvents->>Notification API: new Notification(appName, { body: chatSummary })
Notification API-->>User: Native OS notification
end
Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/hooks/usePlanEvents.ts
Line: 189-191
Comment:
**Non-null assertion on potentially-null `selectedAppIdRef.current`**
`selectedAppIdRef.current!` suppresses the null type but doesn't guard against a null value at runtime. If no app is selected, `queryKeys.apps.detail({ appId: null })` is invoked and `getQueryData` returns `undefined`, which is correctly handled by the `app?.name ?? "Dyad"` fallback — so there's no crash. However, the pattern is inconsistent with how the equivalent block in `useStreamChat.ts` (line 238) is written (it passes `selectedAppId` without a non-null assertion), and it silently assumes an app is always selected when a questionnaire fires.
Consider aligning with `useStreamChat.ts` and dropping the assertion, or adding an explicit early-return guard:
```suggestion
const app = queryClient.getQueryData<App | null>(
queryKeys.apps.detail({ appId: selectedAppIdRef.current }),
);
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "Merge branch 'main' ..." |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues SummaryNo HIGH or MEDIUM issues found. 🟢 Low Priority Notes (2 items)
🚫 Dropped False Positives (1 item)
Generated by Dyadbot multi-agent code review |
github-actions
bot
added
the
needs-human:review-issue
|
@BugBot run |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO - Do NOT merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (2 items)
🚫 Dropped False Positives (3 items)
Generated by Dyadbot multi-agent code review |
![dyad-assistant[bot]](https://avatars.githubusercontent.com/in/3027874?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
- Add migration for renamed enableChatCompletionNotifications -> enableChatEventNotifications setting - Improve banner message to be user-friendly: "Get notified when responses finish or input is needed." Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
@BugBot run |
🤖 Claude Code Review SummaryPR Confidence: 4/5All review issues addressed; migration added and banner copy improved. Confidence is 4 instead of 5 because additional E2E testing of the migration path would be ideal. Unresolved ThreadsNo unresolved threads Resolved Threads
Product Principle SuggestionsNo suggestions — principles were clear enough for all decisions in this review. 🤖 Generated by Claude Code |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
🔍 Dyadbot Code Review SummaryVerdict: ⛔ NO - Do NOT merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (3 items)
🚫 Dropped False Positives (1 item)
Generated by Dyadbot multi-agent code review |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@BugBot run |
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (3 items)
🚫 Dropped False Positives (2 items)
Generated by Dyadbot multi-agent code review |
| <SkippableBanner | ||
| icon={Bell} | ||
| message="Get notified when chat responses finish." | ||
| message="Get notified about chat events." |
There was a problem hiding this comment.
🟡 MEDIUM | microcopy
Banner message uses developer jargon
"Get notified about chat events" uses internal terminology ("chat events") that won't resonate with users. The previous message "Get notified when chat responses finish" was more concrete and user-friendly.
💡 Suggestion: Use a message that describes the actual benefit, e.g.:
| message="Get notified about chat events." | |
| message="Get notified when responses finish or input is needed." |
wwwillchen
left a comment
wwwillchen
left a comment
There was a problem hiding this comment.
LGTM. Left one comment to improve UX.
having trouble getting notifications to show up on mac locally (but i think it's unrelated to your changes)
src/hooks/usePlanEvents.ts
Outdated
| Notification.permission === "granted" && | ||
| !document.hasFocus() | ||
| ) { | ||
| new Notification("Dyad", { |
There was a problem hiding this comment.
let's use a more helpful notification title,
dyad/src/hooks/useStreamChat.ts
Line 252 in 4fb6701
|
@BugBot run |
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues SummaryNo new HIGH or MEDIUM issues found. The previously flagged microcopy issue ("chat events" banner text) remains open from a prior review. 🟢 Low Priority Notes (3 items)
🚫 Dropped False Positives (3 items)
Generated by Dyadbot multi-agent code review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2867f49e05
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| enableChatEventNotifications: | ||
| stored.enableChatEventNotifications ?? | ||
| stored.enableChatCompletionNotifications, |
There was a problem hiding this comment.
This migration only fixes the upgrade path. readSettings()/writeSettings() still preserve deprecated fields for backward compatibility (src/main/settings.ts), but the renderer now updates only enableChatEventNotifications, so after a user toggles notifications in this build the JSON can contain a stale enableChatCompletionNotifications. Reopening the same profile in an older Dyad release will then restore the wrong notification preference instead of the value the user last chose.
Useful? React with 👍 / 👎.
|
@BugBot run |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f47000726a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const app = queryClient.getQueryData<App | null>( | ||
| queryKeys.apps.detail({ appId: selectedAppIdRef.current! }), |
There was a problem hiding this comment.
If the user starts a plan in one app, switches to another app while the model is thinking, and the questionnaire arrives with Dyad unfocused, this lookup will title the native notification with whichever app is currently selected instead of the app that actually needs input. I checked src/ipc/types/plan.ts and src/pro/main/ipc/handlers/local_agent/tools/planning_questionnaire.ts: the plan:questionnaire event only carries chatId, so selectedAppIdRef.current is not a reliable way to identify the questionnaire's app. Please include appId in the event payload or resolve it from payload.chatId before showing the notification.
Useful? React with 👍 / 👎.
🔍 Dyadbot Code Review SummaryVerdict: ✅ YES - Ready to merge Reviewed by 3 independent agents: Correctness Expert, Code Health Expert, UX Wizard. Issues Summary
🟢 Low Priority Notes (4 items)
🚫 Dropped False Positives (4 items)
Generated by Dyadbot multi-agent code review |
| <SkippableBanner | ||
| icon={Bell} | ||
| message="Get notified when chat responses finish." | ||
| message="Get notified about chat events." |
There was a problem hiding this comment.
🟡 MEDIUM | microcopy
"Chat events" is vague, user-unfriendly microcopy
"Get notified about chat events." uses developer jargon — users don't think in terms of "chat events." The previous copy ("Get notified when chat responses finish.") was more specific and action-oriented. Since the feature now covers two cases, the banner should communicate both clearly.
💡 Suggestion:
| message="Get notified about chat events." | |
| message="Get notified when responses finish or input is needed." |
🎭 Playwright Test Results❌ Some tests failed
Summary: 797 passed, 5 failed, 5 flaky, 248 skipped Failed Tests🍎 macOS
🪟 Windows
📋 Re-run Failing Tests (macOS)Copy and paste to re-run all failing spec files locally: npm run e2e \
e2e-tests/setup_flow.spec.ts
|
2 participants
closes #2808