Skip to content

fix "what's changed" release toast attention level logic#5959

Merged
DrJKL merged 1 commit intomainfrom
fix/release-show-toast-logic
Oct 7, 2025
Merged

fix "what's changed" release toast attention level logic#5959
DrJKL merged 1 commit intomainfrom
fix/release-show-toast-logic

Conversation

@christian-byrne
Copy link
Contributor

@christian-byrne christian-byrne commented Oct 7, 2025

Summary

Currently, the "What's Changed" popup toast in bottom left appears after updating if three conditions are true:

  1. Using Desktop app
  2. Don't have notifications disabled in settings
  3. Have not seen/dismissed the notification before

Then the fourth condition is

  1. At least 1 of the last 2 notifications is medium or high priority

However, we only ever show the most recent notification, so this logic is flawed. In addition, it presents issues:

  • When the changelog is first generated by AI, it is marked as "low" priority until human review. But if the changelog prior to that is "medium" or "high", the AI-generated one might get shown anyway - which frustrates the intended process.

There's also a bug fixed here concidentally where if the server only returns a single entry, it is never shown (due to slice(0, -1) syntax when checking priorities).

Changes

  • What: Updated Pinia release store to read attention from the newest release only and expanded unit coverage for toast visibility

┆Issue is synchronized with this Notion page by Unito

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Oct 7, 2025
@github-actions
Copy link

github-actions bot commented Oct 7, 2025

🎨 Storybook Build Status

Build completed successfully!

⏰ Completed at: 10/07/2025, 06:34:54 PM UTC

🔗 Links


🎉 Your Storybook is ready for review!

@github-actions
Copy link

github-actions bot commented Oct 7, 2025

🎭 Playwright Test Results

⚠️ Tests passed with flaky tests

⏰ Completed at: 10/07/2025, 07:11:48 PM UTC

📈 Summary

  • Total Tests: 488
  • Passed: 455 ✅
  • Failed: 0
  • Flaky: 3 ⚠️
  • Skipped: 30 ⏭️

📊 Test Reports by Browser

  • chromium: View Report • ✅ 446 / ❌ 0 / ⚠️ 3 / ⏭️ 30
  • chromium-2x: View Report • ✅ 2 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • chromium-0.5x: View Report • ✅ 1 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • mobile-chrome: View Report • ✅ 6 / ❌ 0 / ⚠️ 0 / ⏭️ 0

🎉 Click on the links above to view detailed test results for each browser configuration.

@christian-byrne christian-byrne added the claude-review Add to trigger a PR code review from Claude Code label Oct 7, 2025
Copy link

@claude claude bot left a comment

Choose a reason for hiding this comment

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

Comprehensive PR Review

This review is generated by Claude. It may not always be accurate, as with human reviewers. If you believe that any of the comments are invalid or incorrect, please state why for each. For others, please implement the changes in one way or another.

Review Summary

PR: fix what's changed release toast attention level logic (#5959)
Impact: 4 additions, 8 deletions across 2 files

Issue Distribution

  • Critical: 0
  • High: 0
  • Medium: 1
  • Low: 0

Category Breakdown

  • Architecture: 0 issues
  • Security: 0 issues
  • Performance: 0 issues
  • Code Quality: 1 issue

Key Findings

Architecture & Design

This change correctly fixes a logic bug in the release notification system. The previous implementation was checking all releases except the most recent one (slice 0 to -1) when determining if the toast should be shown, which was counterintuitive since users should only see notifications based on the newest release's attention level.

Security Considerations

No security issues identified. The change is purely business logic focused on notification display.

Performance Impact

Minimal performance improvement - the new logic is simpler and performs fewer operations (direct property access vs array iteration with filtering).

Integration Points

The change maintains backward compatibility and doesn't affect the external API. Test coverage has been appropriately updated to reflect the new logic.

Positive Observations

  • Clear Bug Fix: Addresses the exact problem described in the PR description
  • Comprehensive Testing: Tests have been updated to match the new behavior and include proper edge cases
  • Clean Implementation: The new logic is more straightforward and easier to understand
  • Good Documentation: PR description clearly explains the problem and solution

Next Steps

  1. Consider adding a code comment to explain why only the most recent release attention is checked
  2. Verify that manual testing confirms the toast behavior works as expected
  3. This PR is ready for merge after addressing the minor documentation suggestion

This is a comprehensive automated review. For architectural decisions requiring human judgment, please request additional manual review.

@DrJKL DrJKL merged commit 61d0a12 into main Oct 7, 2025
57 of 59 checks passed
@DrJKL DrJKL deleted the fix/release-show-toast-logic branch October 7, 2025 19:47
christian-byrne added a commit that referenced this pull request Oct 8, 2025
## Summary

Currently, the "What's Changed" popup toast in bottom left appears after
updating if three conditions are true:

1. Using Desktop app
2. Don't have notifications disabled in settings
3. Have not seen/dismissed the notification before

Then the fourth condition is

4. At least 1 of the last 2 notifications is medium or high priority

However, we only ever show the most recent notification, so this logic
is flawed. In addition, it presents issues:

- When the changelog is first generated by AI, it is marked as "low"
priority until human review. But if the changelog _prior_ to that is
"medium" or "high", the AI-generated one might get shown anyway - which
frustrates the intended process.

There's also a bug fixed here concidentally where if the server only
returns a single entry, it is never shown (due to `slice(0, -1)` syntax
when checking priorities).

## Changes

- **What**: Updated Pinia release store to read `attention` from the
newest release only and expanded unit coverage for toast visibility

┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-5959-fix-what-s-changed-release-toast-attention-level-logic-2856d73d36508141b9b2d8d3b11153b2)
by [Unito](https://www.unito.io)
@AustinMroz AustinMroz added needs-backport Fix/change that needs to be cherry-picked to the current feature freeze branch 1.28 labels Oct 8, 2025
AustinMroz pushed a commit that referenced this pull request Oct 8, 2025
## Summary

Currently, the "What's Changed" popup toast in bottom left appears after
updating if three conditions are true:

1. Using Desktop app
2. Don't have notifications disabled in settings
3. Have not seen/dismissed the notification before

Then the fourth condition is

4. At least 1 of the last 2 notifications is medium or high priority

However, we only ever show the most recent notification, so this logic
is flawed. In addition, it presents issues:

- When the changelog is first generated by AI, it is marked as "low"
priority until human review. But if the changelog _prior_ to that is
"medium" or "high", the AI-generated one might get shown anyway - which
frustrates the intended process.

There's also a bug fixed here concidentally where if the server only
returns a single entry, it is never shown (due to `slice(0, -1)` syntax
when checking priorities).

## Changes

- **What**: Updated Pinia release store to read `attention` from the
newest release only and expanded unit coverage for toast visibility

┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-5959-fix-what-s-changed-release-toast-attention-level-logic-2856d73d36508141b9b2d8d3b11153b2)
by [Unito](https://www.unito.io)
AustinMroz added a commit that referenced this pull request Oct 8, 2025
Includes
- Subgraph widget promotion fixes (#5911)
- fix "what's changed" release toast attention level logic (#5959)
- Fix: Missing Node Title Editor bug (#5963)
- OpenAIVideoSora2 Frontend pricing (#5958)
- hotfix: quick test updates for sora2 pricing badge. (#5966)

┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-5986-Initial-1-28-backports-2866d73d365081448ff2c4827bdbb9e5)
by [Unito](https://www.unito.io)

---------

Co-authored-by: Christian Byrne <cbyrne@comfy.org>
Co-authored-by: Alexander Brown <drjkl@comfy.org>
Co-authored-by: Marwan Ahmed <155799754+marawan206@users.noreply.github.com>
arjansingh pushed a commit that referenced this pull request Oct 12, 2025
## Summary

Currently, the "What's Changed" popup toast in bottom left appears after
updating if three conditions are true:

1. Using Desktop app
2. Don't have notifications disabled in settings
3. Have not seen/dismissed the notification before

Then the fourth condition is

4. At least 1 of the last 2 notifications is medium or high priority

However, we only ever show the most recent notification, so this logic
is flawed. In addition, it presents issues:

- When the changelog is first generated by AI, it is marked as "low"
priority until human review. But if the changelog _prior_ to that is
"medium" or "high", the AI-generated one might get shown anyway - which
frustrates the intended process.

There's also a bug fixed here concidentally where if the server only
returns a single entry, it is never shown (due to `slice(0, -1)` syntax
when checking priorities).

## Changes

- **What**: Updated Pinia release store to read `attention` from the
newest release only and expanded unit coverage for toast visibility

┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-5959-fix-what-s-changed-release-toast-attention-level-logic-2856d73d36508141b9b2d8d3b11153b2)
by [Unito](https://www.unito.io)
arjansingh pushed a commit that referenced this pull request Oct 13, 2025
## Summary

Currently, the "What's Changed" popup toast in bottom left appears after
updating if three conditions are true:

1. Using Desktop app
2. Don't have notifications disabled in settings
3. Have not seen/dismissed the notification before

Then the fourth condition is

4. At least 1 of the last 2 notifications is medium or high priority

However, we only ever show the most recent notification, so this logic
is flawed. In addition, it presents issues:

- When the changelog is first generated by AI, it is marked as "low"
priority until human review. But if the changelog _prior_ to that is
"medium" or "high", the AI-generated one might get shown anyway - which
frustrates the intended process.

There's also a bug fixed here concidentally where if the server only
returns a single entry, it is never shown (due to `slice(0, -1)` syntax
when checking priorities).

## Changes

- **What**: Updated Pinia release store to read `attention` from the
newest release only and expanded unit coverage for toast visibility

┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-5959-fix-what-s-changed-release-toast-attention-level-logic-2856d73d36508141b9b2d8d3b11153b2)
by [Unito](https://www.unito.io)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1.28 area:desktop area:system-notification claude-review Add to trigger a PR code review from Claude Code needs-backport Fix/change that needs to be cherry-picked to the current feature freeze branch size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants