feat(history): add native GTK Ctrl+H sidebar#304
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 2 hours, 20 minutes, and 43 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds a native GTK history sidebar: new history port, config (sidebar width), main-window sidebar pane and APIs, complete GTK sidebar component (widgets, keyboard, loading/search, rendering), dispatcher wiring for Ctrl+H, CSS, docs update, and extensive unit/integration tests. ChangesNative GTK History Sidebar
Estimated code review effort 🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/ui/component/history_sidebar_loading_search.go`:
- Around line 37-52: The code uses glib.IdleAdd directly inside the callback,
bypassing the component's injected idle scheduler; replace those direct
glib.IdleAdd calls with the component's scheduling wrapper hs.scheduleIdle(...)
so GTK-thread callbacks are dispatched via the same seam used elsewhere.
Concretely, where you create cb := glib.SourceFunc(...) and then call
glib.IdleAdd(&cb, 0), change that final call to hs.scheduleIdle(&cb) (and do the
same for the other occurrence that mirrors this pattern) so the scheduling goes
through hs.scheduleIdle and preserves existing behavior and testability.
In `@internal/ui/component/history_sidebar_widgets.go`:
- Around line 76-80: The comment is inaccurate because
hs.listBox.SetActivateOnSingleClick(true) enables single-click activation;
update the comment above rowActivatedCb (or the surrounding line) to accurately
describe activation behavior (e.g., "Connect row activation (single-click and
keyboard activation)" or "Connect row activation (single-click, Enter or
double-click as configured)") so it reflects that SetActivateOnSingleClick(true)
enables click-based activation; alternatively, if the intent was to require
Enter/double-click, change hs.listBox.SetActivateOnSingleClick(true) to false
and keep the original comment—ensure the comment and the
SetActivateOnSingleClick setting are consistent.
In `@internal/ui/component/history_sidebar.go`:
- Around line 235-273: The Show method holds hs.mu while calling hs.scheduleIdle
which can re-enter and deadlock; fix HistorySidebar.Show by collecting the
minimal data needed for each idle callback (e.g., capture hs.destroyed and
hs.searchEntry into locals or capture hs pointer but avoid holding hs.mu), then
release hs.mu before calling hs.scheduleIdle for both reloadCb and the focus cb.
Concretely: inside HistorySidebar.Show, lock and read required fields into local
variables, unlock (defer removed), then build the glib.SourceFunc closures (they
may still check hs.destroyed inside) and call hs.scheduleIdle; ensure the
visible flag and outerBox.SetVisible(true) remain set while holding the lock but
move the scheduleIdle invocations to after unlocking to avoid scheduling while
hs.mu is held.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 657c121d-e5bb-4bdb-8576-751b37566cda
📒 Files selected for processing (31)
.mockery.yamldocs/reference/keybindings.mdinternal/application/port/history_sidebar.gointernal/application/usecase/search_history.gointernal/infrastructure/config/defaults.gointernal/infrastructure/config/loader.gointernal/infrastructure/config/migrate.gointernal/infrastructure/config/schema.gointernal/infrastructure/config/schema_provider.gointernal/infrastructure/config/validation.gointernal/ui/app.gointernal/ui/browser_window.gointernal/ui/browser_window_history_sidebar.gointernal/ui/browser_window_history_sidebar_test.gointernal/ui/browser_window_test.gointernal/ui/component/history_model.gointernal/ui/component/history_model_test.gointernal/ui/component/history_sidebar.gointernal/ui/component/history_sidebar_keyboard.gointernal/ui/component/history_sidebar_loading_search.gointernal/ui/component/history_sidebar_rendering.gointernal/ui/component/history_sidebar_search_test.gointernal/ui/component/history_sidebar_widgets.gointernal/ui/component/history_test_helpers_test.gointernal/ui/dispatcher/keyboard.gointernal/ui/dispatcher/keyboard_test.gointernal/ui/theme/css.gointernal/ui/theme/history_sidebar_css.gointernal/ui/theme/history_sidebar_css_test.gointernal/ui/window/main_window.gointernal/ui/window/main_window_sidebar.go
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/ui/browser_window_history_sidebar.go (1)
30-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick winForce the sidebar hidden during initialization.
Line 31 only updates the mirror flag. This path never calls
HistorySidebar.Hide()orMainWindow.SetSidebarVisible(false), so the “hidden by default” behavior depends on external widget defaults instead of being enforced here.Proposed fix
bw.historySidebar = sidebar - bw.sidebarVisible = false // Mount into the main window's sidebar box bw.mainWindow.SetSidebarWidget(sidebar.Widget()) + bw.historySidebar.Hide() + bw.mainWindow.SetSidebarVisible(false) + bw.sidebarVisible = false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/ui/browser_window_history_sidebar.go` around lines 30 - 38, The code only sets bw.sidebarVisible = false but doesn't actually hide the widget; update the initialization in the constructor that sets bw.historySidebar and bw.mainWindow to explicitly hide the sidebar by calling bw.historySidebar.Hide() and bw.mainWindow.SetSidebarVisible(false) (call these after bw.mainWindow.SetSidebarWidget(sidebar.Widget()) and before bw.applySidebarWidthConfig(a)) so the "hidden by default" state is enforced rather than relying on widget defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/ui/component/history_sidebar_rendering.go`:
- Line 81: The empty-state label uses fmt.Sprintf with the %q verb which
produces Go-escaped literals (e.g. showing backslashes); update the
label.SetText calls that use fmt.Sprintf("No results for %q", query) to use a
plain-string format instead, e.g. fmt.Sprintf("No results for %s", query) (and
make the same change for the other occurrence in the file) so the query is shown
as normal text rather than a Go-escaped literal.
---
Outside diff comments:
In `@internal/ui/browser_window_history_sidebar.go`:
- Around line 30-38: The code only sets bw.sidebarVisible = false but doesn't
actually hide the widget; update the initialization in the constructor that sets
bw.historySidebar and bw.mainWindow to explicitly hide the sidebar by calling
bw.historySidebar.Hide() and bw.mainWindow.SetSidebarVisible(false) (call these
after bw.mainWindow.SetSidebarWidget(sidebar.Widget()) and before
bw.applySidebarWidthConfig(a)) so the "hidden by default" state is enforced
rather than relying on widget defaults.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0a7c05b4-12c6-49a1-b387-2607e06f7df8
📒 Files selected for processing (11)
internal/ui/browser_window_history_sidebar.gointernal/ui/browser_window_history_sidebar_test.gointernal/ui/component/history_model.gointernal/ui/component/history_model_test.gointernal/ui/component/history_sidebar.gointernal/ui/component/history_sidebar_keyboard.gointernal/ui/component/history_sidebar_loading_search.gointernal/ui/component/history_sidebar_rendering.gointernal/ui/component/history_sidebar_search_test.gointernal/ui/component/history_sidebar_widgets.gointernal/ui/window/main_window.go
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
Ctrl+Hhistory with a native GTK history sidebarBehavior
Ctrl+Htoggles the native history sidebarCtrl+Hreturns a clean error instead of falling back todumb://historyShift+Enteropens the selected history item in a new splitValidation
go test ./internal/...go vet ./internal/...Summary by CodeRabbit
New Features
Documentation
Tests