Limit the number of notifications shown simultaneously#40
Limit the number of notifications shown simultaneously#40snipsnipsnip merged 5 commits intoexteditor:mainfrom
Conversation
* Make filename lowercase * Move readme under `doc/` instead of the top page * List modules in subdirectories
I tested the RTL language (Arabic), and noticed that the port number field, which is LTR, looks odd. For now, we align numbers to the right regardless of text direction and await feedback from users of RTL languages.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe pull request consolidates test result reporting, refactors asynchronous email editor updates with status change detection, introduces a notification slot management system, updates CSS alignment handling, and modifies TypeDoc documentation configuration across the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant EmailEditor
participant Promise.all
participant NotifyModule
participant IconModule
participant Status
Caller->>EmailEditor: update()
activate EmailEditor
EmailEditor->>Status: compare with lastStatus
alt Status Changed
EmailEditor->>Promise.all: concurrent operations
activate Promise.all
Promise.all->>NotifyModule: show notification
Promise.all->>IconModule: update compose window icon
activate NotifyModule
activate IconModule
NotifyModule-->>Promise.all: done
IconModule-->>Promise.all: done
deactivate NotifyModule
deactivate IconModule
deactivate Promise.all
else Status Unchanged
Note over EmailEditor: skip notification
end
EmailEditor->>Status: update lastStatus
EmailEditor-->>Caller: Promise<void>
deactivate EmailEditor
sequenceDiagram
participant Client
participant NotificationTray
participant makeId
participant SlotManager as Slot Manager<br/>(slotCount=5)
Client->>NotificationTray: showNotification(options)
activate NotificationTray
NotificationTray->>makeId: generate ID
activate makeId
makeId->>SlotManager: cycle within 5 slots
SlotManager-->>makeId: nextID
makeId-->>NotificationTray: ID
deactivate makeId
NotificationTray->>NotificationTray: create notification<br/>with ID + options
NotificationTray-->>Client: notification created
deactivate NotificationTray
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes The changes are mostly straightforward: workflow config updates are procedural, CSS changes are minimal, TypeScript additions introduce clear async/await patterns and a straightforward slot-based ID management system with no complex business logic, and the TypeDoc config change is a declarative update. Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #40 +/- ##
==========================================
- Coverage 48.47% 48.43% -0.05%
==========================================
Files 55 55
Lines 2133 2141 +8
Branches 286 286
==========================================
+ Hits 1034 1037 +3
- Misses 1092 1097 +5
Partials 7 7 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/build.yml(1 hunks).github/workflows/check-pr.yml(1 hunks)ext/options.css(1 hunks)ext/options.nolint.css(1 hunks)src/ghosttext-adaptor/email_editor.ts(1 hunks)src/thunderbird/background_util/notification_tray.ts(3 hunks)tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/ghosttext-adaptor/email_editor.ts (1)
src/ghosttext-runner/api.ts (1)
ClientStatus(109-109)
src/thunderbird/background_util/notification_tray.ts (1)
src/ghosttext-adaptor/api.ts (1)
INotificationTray(5-8)
🔇 Additional comments (6)
ext/options.nolint.css (1)
5-5: LGTM!Adding
--align-startprovides proper bidirectional text support and complements the existing--align-endvariable..github/workflows/check-pr.yml (1)
59-59: LGTM!This change is consistent with
.github/workflows/build.ymland consolidates test result reporting to a single file.tsconfig.json (1)
34-45: LGTM!The TypeDoc configuration updates improve documentation generation:
readme: "none"prevents default readme duplicationexcludePrivate: falseensures comprehensive API documentation- The new entry point pattern
./src/*/*/index.tscaptures additional module exports- Slug configuration ensures consistent URL formatting
Also applies to: 50-50
src/ghosttext-adaptor/email_editor.ts (1)
23-33: LGTM!The refactor to async
updatewith status change detection and concurrent updates is well-implemented:
isNewStatuscorrectly detects status changes beforelastStatusis updatedPromise.allefficiently handles concurrent notification and icon updates- Notifications now only appear when status actually changes, aligning with the PR objective to limit notifications
src/thunderbird/background_util/notification_tray.ts (1)
9-11: LGTM!The notification slot management correctly implements the PR objective:
slotCount = 5limits simultaneous notificationsmakeId()cycles notification IDs using modulo arithmetic (0→1→2→3→4→0...)- Reusing IDs replaces older notifications, preventing notification overload
Also applies to: 20-21, 34-37
.github/workflows/build.yml (1)
59-59: Test results path is correct
Vitest’sOUTPUT_DIRis set tobuild/test/and the JUnit reporter writesresult.junit.xmlthere, sobuild/test/result.junit.xmlis valid.
In #24, I implemented notifications, but I’ve noticed that repeatedly hitting the toolbar buttons shows many number of notifications, while Thunderbird drops some of them. Limit the number of notifications so that they display smoothly.