feat: tab groups, live CWD tracking, and UX improvements (v1.8.8)#46
feat: tab groups, live CWD tracking, and UX improvements (v1.8.8)#46ruscher wants to merge 1 commit into
Conversation
ruscher
commented
Apr 8, 2026
- Add Chrome-style tab groups with color-coded chips and alphabetical naming (A-Z, 1A-1Z...)
- Add /proc//cwd polling for real-time tab title updates (1s interval)
- Add VTE_VERSION env var for OSC7 shell integration activation
- Add full path tooltip on tab hover
- Fix tab title reset to 'Local' on focus/click (fallback to last polled CWD)
- Fix tab move auto-join for right side of last group tab
- Fix tab resizing during move mode (use opacity instead of visibility)
- Add tab group CSS styles and chip/collapse UI
- Add group move, rename, ungroup, and context menu actions
- Update README with tab groups and live directory tracking features
- Add Chrome-style tab groups with color-coded chips and alphabetical naming (A-Z, 1A-1Z...) - Add /proc/<pid>/cwd polling for real-time tab title updates (1s interval) - Add VTE_VERSION env var for OSC7 shell integration activation - Add full path tooltip on tab hover - Fix tab title reset to 'Local' on focus/click (fallback to last polled CWD) - Fix tab move auto-join for right side of last group tab - Fix tab resizing during move mode (use opacity instead of visibility) - Add tab group CSS styles and chip/collapse UI - Add group move, rename, ungroup, and context menu actions - Update README with tab groups and live directory tracking features
talesam
left a comment
There was a problem hiding this comment.
PR #46 Review — Tab Groups, Live CWD Tracking, UX Improvements
Hey @ruscher, thanks for the PR! The feature set is solid — tab groups, live CWD tracking, and the Chrome-style UX are great additions. I ran a thorough review and found a few items that need fixing before merge.
Must Fix Before Merge
1. Docstring vs Code Mismatch in manager.py
File: src/ashyterm/terminal/manager.py, method _periodic_process_check()
The docstring says:
This runs every 3 seconds and checks for:
But the timer was changed to 1 second (GLib.timeout_add(1000, ...)).
The inline comment at initialization (line ~94-95) is correct ("runs every second"), but the docstring in the method itself is stale. Please update to match.
2. Accessing Private Internals of ProcessTracker
File: src/ashyterm/terminal/manager.py, method _poll_terminal_cwd()
tracker = self.spawner.process_tracker
with tracker._lock:
processes = dict(tracker._processes)_lock and _processes are private attributes of ProcessTracker. Accessing them directly breaks encapsulation and will break if ProcessTracker internals change.
Suggested fix: Add a public method to ProcessTracker, e.g.:
def get_processes_snapshot(self) -> Dict[int, Dict[str, Any]]:
"""Return a thread-safe snapshot of tracked processes."""
with self._lock:
return dict(self._processes)Then in _poll_terminal_cwd():
processes = tracker.get_processes_snapshot()3. Unused Import in test_tab_groups.py
File: tests/test_tab_groups.py, line 3
import pytestpytest is imported but never used — no pytest.raises, pytest.mark, pytest.fixture, etc. Please remove it.
4. Version Mismatch — Critical
The PR introduces version conflicts with the current main branch:
| File | PR #46 | Current main |
|---|---|---|
README.md |
1.8.8 | 1.9.1 |
config.py |
1.8.8 | 1.8.7 |
pyproject.toml |
0.1.0 | 0.1.0 |
Issues:
- The README badge is downgraded from 1.9.1 → 1.8.8
config.pybumps from 1.8.7 → 1.8.8, but the README inmainalready says 1.9.1- Note:
mainitself already has a version inconsistency (README= 1.9.1 vsconfig.py= 1.8.7). This PR makes it worse by setting both to 1.8.8 pyproject.tomlis0.1.0everywhere and has never been updated
Required: Decide on the correct version number for this release (should be ≥ 1.9.2 since main README already shows 1.9.1), and align all three files (README.md, config.py, pyproject.toml).
Should Fix Before Merge
5. Integration Tests for Save/Restore
The unit tests for TabGroupManager are good and cover CRUD well. However, there are no integration tests for:
- Save/restore of groups via
WindowStateManager - Serialization of
group_idon tabs - Rebuild of tab bar after restore
- Basic state roundtrip
At least 1-2 integration tests for the restore flow would increase confidence before merge.
Can Be Follow-Up
6. id(tab_widget) as Tab Identifier
get_tab_id() returns str(id(tab_widget)) — this is the Python object memory address. It:
- Changes between sessions (not stable across restart)
- Can be reused if a widget is destroyed and a new one is allocated at the same address
The restore code works around this by using positional matching (last-created tab), so it's functional. But for long-term robustness, consider switching to UUID or an incremental counter. This can be a follow-up PR.
7. Empty Callback _on_setting_changed in tabs.py
This method is just pass — but it already existed in main before this PR, so it's not your issue. Just flagging it for awareness.
8. Planning Docs in Repo
The PR adds docs/chrome_tab_groups_reference.md and docs/tab_groups_plan.md. These are useful for development context but may not belong in the production repo long-term. Consider moving them to the wiki or issues after the feature stabilizes.
Summary
| Priority | Items |
|---|---|
| Must fix | Docstring mismatch, tracker encapsulation, unused import, version alignment |
| Should fix | 1-2 integration tests for restore flow |
| Follow-up | Replace id(tab_widget), empty callback, planning docs location |
Overall the implementation is clean and well-structured. The TabGroupManager is nicely decoupled from UI, the CSS is proper, and the Chrome-style UX is a good model. Just need these fixes and we're good to merge.
Looking forward to the updated version! 🚀
Tab groups and live CWD tracking had a few defects that made them fragile under real use. None of these are visible to the unit tests the PR shipped, but they hurt production. Performance / resource usage - CssProvider-per-rebuild leak: _create_group_chip and _apply_group_border_color were building a fresh Gtk.CssProvider on every tab-bar rebuild (which happens on every group mutation). Cache providers keyed by color so we reuse them. - Cached os.uname().nodename once in __init__ instead of calling it on every CWD title update. - /proc/<pid>/cwd polling moved off the GTK main thread to AsyncTaskManager.submit_io, with results applied via GLib.idle_add. - Polling interval tuned from 1s to 2s. 1s was too aggressive and offered no perceptible benefit over 2s once readlink is off-thread. Correctness - set_active_tab auto-expanded collapsed groups on switch, but clicking the chip to collapse the group that owns the active tab left focus on an invisible tab (keyboard input went nowhere). Now the clicker is switched to the first visible tab outside the group. - Removed provider is now properly detached when a tab leaves a group, so the old border color doesn't bleed through after ungroup. Encapsulation - ProcessTracker.snapshot() returns a dict copy under the lock. Call sites (the new CWD polling) stop grabbing tracker._lock / tracker._processes directly, so the internal representation can change safely. Portability - Replaced staticmethod.__func__ trick in WindowStateManager with a module-level _migrate_v1_to_v2 function. __func__ on staticmethod has been soft-deprecated since 3.10 and removed from the advertised API; plain functions are stable across every supported version. Tests - test_process_tracker_snapshot.py: 3 tests for the new snapshot() method, including a disjoint-range concurrent writer check. - test_window_state_migration.py: 6 tests for the v1 → v2 migration, including that the registered callable stays a plain function. - test_tab_groups.py: 4 more tests for move_group edge cases and load_from_list id preservation. Suite: 1126 passed in ~2.8s. ruff check src/ is clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>