fix(notifications): clean up control flow and fix PR/Issue action confirmations #744
+1,688
−143
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sideshowbarker
changed the title
…firmations - Refactor NotificationsView key handling for clearer control flow: - Check for PR/Issue actions first, then handle navigation - Always return after updating prView/issueSidebar (fixes tab navigation) - Remove redundant MarkAsRead handler (section handles it) - Fix PR/Issue action confirmations in Notifications view: - Add promptConfirmationForNotificationPR/Issue methods - Use footer-based confirmation instead of section confirmation - Add executeNotificationAction to run tasks after confirmation - Add closeIssue/reopenIssue helper methods
sideshowbarker
force-pushed
the
dlvhdr/notifications-fixes
branch
from
9ff588b to
d8ccb5c
Compare
dlvhdr
reviewed
Jan 21, 2026
This change fixes a bug that caused us to not be showing up-to-date status/ state in Reviewers. The fix causes the cached GraphQL client to be cleared when a refresh is requested — ensuring the next enrichment invalidates the cache, and then a request for fresh data is made over the network to GitHub. Otherwise, without this change, when pressing “r” or “R” to refresh, the enriched PR data (including reviewers) was being served from a 5-minute-TTL GraphQL cache. That meant that if someone approved the PR, and then we refreshed in dash, the reviewer status wouldn't update.
…onview Move the pendingNotificationAction state from ui.Model to notificationview.Model, consolidating notification-related state in one place. Add comprehensive tests for the pending action methods: - SetPendingPRAction/SetPendingIssueAction with all action types - Prompt text verification including "ready" → "mark as ready" case - HasPendingAction, GetPendingAction, ClearPendingAction - Nil subject handling for both PR and Issue - Confirmation acceptance (y, Y, Enter) and cancellation
The TestRefresh_ClearsEnrichmentCache and TestRefreshAll_ClearsEnrichmentCache tests were missing sidebar initialization, causing a nil pointer dereference when syncSidebar() was called during the refresh key handling.
GetSidebarContentWidth was checking m.ctx.Config == nil but m.ctx itself could be nil (since NewModel() sets ctx: nil), causing a panic in tests.
The tests were calling m.Update() which triggers refresh logic that calls ctx.StartTask(). Without initializing StartTask to a no-op function, this caused a nil pointer dereference.
The tests were triggering syncProgramContext which calls UpdateProgramContext on all model components (tabs, footer, prView, issueSidebar, branchSidebar, notificationView). Without initializing these components, nil pointer dereferences occurred.
…te method Move the confirmation key handling from ui.go into notificationview's new Update() method, matching the pattern used by prview. The component now owns its confirmation behavior via a callback set by ui.go. Changes: - Add Update() method to notificationview that handles y/Y/Enter to confirm and any other key to cancel - Add onConfirmAction callback field and SetOnConfirmAction() setter - Modify executeNotificationAction() to accept action as parameter - Set callback in initComponents() during initialization This removes special-case handling from ui.go and makes the component self-contained.
Remove early returns when handling PR/Issue keybindings in notification view so that key messages reach the sidebar's Update method. This fixes Ctrl+D and other scroll keys not working in the sidebar.
Same fix as 6619183 — remove early return so key messages reach the sidebar's Update method.
Use m.currSectionId directly instead of calling getCurrSection() and GetId().
Fetch PR and Issue subjects once at the start of the function instead of redundantly in each switch case.
Move CloseIssue and ReopenIssue functions to tasks/issue.go, following the same pattern as PR operations in tasks/pr.go. This eliminates duplicate implementations that existed in ui.go and issuessection. Also moves UpdateIssueMsg to the tasks package for consistency with UpdatePRMsg.
sideshowbarker
force-pushed
the
dlvhdr/notifications-fixes
branch
from
1cced65 to
c0c2605
Compare
When issueview.Model is not properly initialized (e.g., in tests that create partial models), the ac pointer is nil. This caused a panic when syncProgramContext called UpdateProgramContext on all components.
sideshowbarker
force-pushed
the
dlvhdr/notifications-fixes
branch
from
0d0bc72 to
ae0e6c1
Compare
- Document the `s` key for switching to PRs view - Update confirmation prompt section to reflect current architecture where pendingAction is managed by notificationView.Model with callback pattern
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Refactor
NotificationsViewkey handling for clearer control flow:prView/issueSidebar(fixes tab navigation)MarkAsReadhandler (section handles it)Fix PR/Issue action confirmations in Notifications view:
promptConfirmationForNotificationPR/IssuemethodsexecuteNotificationActionto run tasks after confirmationcloseIssue/reopenIssuehelper methodsHow did you test this change?
Tests included!