feat: add notification history center#53
Merged
flyingrobots merged 4 commits intomainfrom Mar 19, 2026
Merged
Conversation
📝 WalkthroughWalkthroughAdds a notification history feature (counting, filtering, scrollable modal, command integration), exports new history APIs, updates example/demo to use the history UI, adds tests, and bumps several GitHub Actions to newer major versions across workflows. Changes
Sequence DiagramsequenceDiagram
actor User
participant App as Notification App
participant History as History Module
participant Render as Render Engine
User->>App: Trigger history (Shift+H / command)
App->>App: Set historyOpen = true
App->>History: countNotificationHistory(state, filter)
History-->>App: totalCount
App->>App: compute max visible / clamp scroll
App->>History: renderNotificationHistory(state, {width,height,scroll,filter})
History-->>App: formatted modal content
App->>Render: overlay modal with content
Render-->>User: display modal
loop Navigation
User->>App: Arrow / PgUp / PgDn
App->>App: update historyScroll
App->>History: renderNotificationHistory(...)
History-->>App: updated content
App->>Render: redraw modal
Render-->>User: updated view
end
User->>App: Press Escape / quit
App->>App: close modal (historyOpen = false)
App->>Render: render main app
Render-->>User: main view
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/release-dry-run.yml (1)
126-129:actions/upload-artifactremains at v4 while other actions were bumped to v6.This may be intentional if upload-artifact doesn't have a v6 release, but verify consistency if a newer version is available that addresses the Node 20 deprecation warning mentioned in the changelog.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release-dry-run.yml around lines 126 - 129, The workflow is pinning actions/upload-artifact@v4 while other actions were bumped to v6; verify whether a v6 (or newer) release of actions/upload-artifact exists and, if it does, update the usage string "actions/upload-artifact@v4" to the newer tag (e.g., `@v6`) in the release-dry-run.yml, or add a comment explaining why v4 must remain if no compatible v6 exists, so the workflow is consistent and avoids Node 20 deprecation warnings.packages/bijou-tui/src/notification.ts (1)
147-152: Avoid intermediate allocation incountNotificationHistory.
filter(...).lengthallocates an array each call; a loop keeps it allocation-free.♻️ Proposed change
export function countNotificationHistory<Msg>( state: NotificationState<Msg>, filter: NotificationHistoryFilter = 'ALL', ): number { - return state.history.filter((item) => matchesHistoryFilter(item, filter)).length; + let count = 0; + for (const item of state.history) { + if (matchesHistoryFilter(item, filter)) count++; + } + return count; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bijou-tui/src/notification.ts` around lines 147 - 152, countNotificationHistory currently uses state.history.filter(...).length which allocates an intermediate array; replace that with an allocation-free counting loop that iterates over state.history and increments a counter when matchesHistoryFilter(item, filter) is true, then return the counter. Locate the function countNotificationHistory and the use of matchesHistoryFilter, iterate over the NotificationState<Msg>.history array directly (for/for-of) to avoid creating the filtered array; keep the same signature and behavior for NotificationHistoryFilter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.github/workflows/release-dry-run.yml:
- Around line 126-129: The workflow is pinning actions/upload-artifact@v4 while
other actions were bumped to v6; verify whether a v6 (or newer) release of
actions/upload-artifact exists and, if it does, update the usage string
"actions/upload-artifact@v4" to the newer tag (e.g., `@v6`) in the
release-dry-run.yml, or add a comment explaining why v4 must remain if no
compatible v6 exists, so the workflow is consistent and avoids Node 20
deprecation warnings.
In `@packages/bijou-tui/src/notification.ts`:
- Around line 147-152: countNotificationHistory currently uses
state.history.filter(...).length which allocates an intermediate array; replace
that with an allocation-free counting loop that iterates over state.history and
increments a counter when matchesHistoryFilter(item, filter) is true, then
return the counter. Locate the function countNotificationHistory and the use of
matchesHistoryFilter, iterate over the NotificationState<Msg>.history array
directly (for/for-of) to avoid creating the filtered array; keep the same
signature and behavior for NotificationHistoryFilter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 657cc72d-bb44-4cc2-8964-11779a512277
⛔ Files ignored due to path filters (1)
docs/ROADMAP.mdis excluded by!docs/ROADMAP.md
📒 Files selected for processing (9)
.github/workflows/ci.yml.github/workflows/publish.yml.github/workflows/release-dry-run.ymldocs/CHANGELOG.mdexamples/README.mdexamples/notifications/main.tspackages/bijou-tui/src/index.tspackages/bijou-tui/src/notification.test.tspackages/bijou-tui/src/notification.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 81f9c0c0f0
ℹ️ 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".
Owner
Author
|
Addressed the remaining non-threaded review notes in
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/notifications-demo.test.ts (1)
31-48: Consider resetting context after compact terminal test.The test sets
compactCtxas the default context but doesn't restore the originaltestCtxafterward. WhileafterAllcalls_resetDefaultContextForTesting, this could cause issues if additional tests are added to this suite that expect the 80x24 terminal size.🧹 Suggested improvement
it('clamps the history center to compact terminals', async () => { const compactCtx = createTestContext({ noColor: true, runtime: { columns: 40, rows: 12 }, }); setDefaultContext(compactCtx); const app = createNotificationDemoApp(compactCtx, { autoDemo: false }); const result = await runScript(app, [{ key: 'H' }], { ctx: compactCtx, pulseFps: false, }); const rendered = surfaceToString(result.frames.at(-1)!, compactCtx.style); expect(rendered).toContain('History'); expect(rendered).toContain('PgUp/PgDn'); expect(rendered).toContain('No archived notifications'); + + // Restore original context for subsequent tests + setDefaultContext(testCtx); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/notifications-demo.test.ts` around lines 31 - 48, This test sets a custom default context (compactCtx) via setDefaultContext but never restores the original context; wrap the test body or add a finally/afterEach to call _resetDefaultContextForTesting() (or explicitly restore the prior context) so subsequent tests expecting the default 80x24 terminal aren't affected; locate the compactCtx creation and setDefaultContext call in this test (and references to createNotificationDemoApp/runScript) and ensure the default is reset at the end of the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@scripts/notifications-demo.test.ts`:
- Around line 31-48: This test sets a custom default context (compactCtx) via
setDefaultContext but never restores the original context; wrap the test body or
add a finally/afterEach to call _resetDefaultContextForTesting() (or explicitly
restore the prior context) so subsequent tests expecting the default 80x24
terminal aren't affected; locate the compactCtx creation and setDefaultContext
call in this test (and references to createNotificationDemoApp/runScript) and
ensure the default is reset at the end of the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: deed6748-7b88-4956-af59-43b252aebe66
📒 Files selected for processing (8)
.github/workflows/release-dry-run.ymldocs/CHANGELOG.mdexamples/notifications/main.tspackages/bijou-tui/src/app-frame.test.tspackages/bijou-tui/src/app-frame.tspackages/bijou-tui/src/notification.test.tspackages/bijou-tui/src/notification.tsscripts/notifications-demo.test.ts
✅ Files skipped from review due to trivial changes (2)
- .github/workflows/release-dry-run.yml
- docs/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bijou-tui/src/notification.test.ts
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
Verification
Summary by CodeRabbit
New Features
Documentation
Chores
Tests