fix: v1.4 bugfixes + 5-line status layout + TUI live preview#18
Merged
Conversation
Bug fixes: - B1: Analytics DB wiring — config default, stats.tsx error msg - B3: daemon_health segment renderer + heartbeat separation - B4: FeatureCard layout min-width - B5: Coaching tab Switch toggles + config persistence via write_config() - B6: Remove ai_ratio segment (function + registry) from BUILTIN_SEGMENTS - B7: Status tab rewrite — 20-segment configurator with live preview - B8: TUI duplicate tab prevention in tui-launcher - #15: Insights UX — timezone conversion for peak_hour, "this session"/"30d" copy Cleanup: - Remove practice_breadcrumb segment (no producer, orphaned since v1.1) - Remove dead timezone field from InsightsFeedConfig - BUILTIN_SEGMENTS: 20 entries, exact match with TUI ALL_SEGMENTS Tests: 101/101 segment tests pass, 1315/1437 total (10 pre-existing SQLite failures) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- hooks/agent-tracker.sh: SubagentStart/Stop state tracker with atomic writes - agents segment: reads active-agents.json, renders colored tree with team info - N-tier renderer: middleSegments + separator between fixed and rotating lines - CLI: reads active-agents.json into cache for the agents segment - Tests: 14 new tests (9 segment + 5 N-tier) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- CR-1: Guard initial daemon heartbeat write with try/catch (daemon-process.ts) - CR-2: Validate heartbeat value with typeof + Number.isFinite (daemon-process.ts) - CR-3: Fix fractional peak_hour for UTC+05:30/+05:45 timezones (insights.ts) - CR-4: Use mockReturnValueOnce to avoid shared-state leak (tui-launcher.test.ts) - CR-5: Unify read/write config path via _effective_config_path (data.py) - CR-6: Include OTHER_SEGMENTS in status tab preview and summary (status.py) Adds 7 Python tests for write_config and _effective_config_path. Updates insights test to use minute-based formula matching production code. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove `set -e` from agent-tracker.sh to honor fail-open contract - Make middleSegments/showSeparator optional in TwoTierConfig interface Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- RC-1: Convert days_active to local timezone (TS + Python TUI) - Use per-session getTimezoneOffset() for DST correctness - Python: use datetime.fromisoformat() → astimezone() → strftime() - RC-2: Add trend arrow to msgs/day (↑↓→ based on 7-day vs full-window) - Add recent_msgs_per_day, recent_days_active to InsightsData - RC-3: Add 'localtime' modifier to SQLite DATE() in TUI analytics - RC-4: Replace hardcoded "in 30d" with dynamic staleness_days PDLC Bug Fix Path: B1→B2→B3(SKEPTIC)→B4(Retro) SKEPTIC cycle 1: Fixed DST-unsafe offset + Python TUI parity gap Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add pytest-textual-snapshot as dev dependency - Create parametrized snapshot tests for all 8 TUI tabs at 80x24 - Add GitHub Actions workflow (Python 3.11/3.12/3.13 matrix) - Generate golden SVG snapshots for visual regression detection - Feeds tab uses deterministic content pinning to prevent flaky diffs PDLC Iteration Path: I1→I2→I3(pending SKEPTIC)→I4 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix CI workflow: uv run --no-project → python -m pytest - Regenerate 2 stale golden snapshot files (feeds, status) - All 8 snapshot tests pass on clean comparison run Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces agent tracking infrastructure, refactors dispatcher and daemon heartbeat mechanisms, expands feed producers with new fields, redesigns status-line segments and two-tier layout from single to multi-line composition, modernizes TUI with PID management and timezone-aware data handling, and adds interactive configuration toggles in TUI coaching and status tabs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
🚥 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)
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.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/core/config.ts (1)
176-197:⚠️ Potential issue | 🟠 MajorKeep analytics off on the no-config path.
loadConfig()falls back togetDefaultConfig()when neither config file exists, so this line turns analytics into implicit persistence for users who have never initialized hookwise. That is a backwards-incompatible data-retention change, not just a new preset default.Suggested fix
export function loadConfig(projectDir?: string): HooksConfig { const effectiveProjectDir = projectDir ?? process.cwd(); const projectConfigPath = join(effectiveProjectDir, PROJECT_CONFIG_FILE); // Resolve global config path dynamically so HOOKWISE_STATE_DIR env var is respected const globalConfigPath = join(getStateDir(), "config.yaml"); // Step 1: Read raw YAML files const globalRaw = readYamlFile(globalConfigPath); const projectRaw = readYamlFile(projectConfigPath); // If neither exists, return defaults if (!globalRaw && !projectRaw) { - return getDefaultConfig(); + const defaults = getDefaultConfig(); + return { + ...defaults, + analytics: { ...defaults.analytics, enabled: false }, + }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/config.ts` around lines 176 - 197, getDefaultConfig currently sets analytics.enabled to true which causes analytics to be implicitly enabled for users who never initialized Hookwise; change the default so analytics.enabled is false in the returned HooksConfig. Update the analytics field in getDefaultConfig (the function named getDefaultConfig returning HooksConfig) to set enabled: false so loadConfig's fallback does not opt users into data retention.src/cli/commands/stats.tsx (1)
78-85:⚠️ Potential issue | 🟡 MinorDon't assume every missing DB means analytics is disabled.
This branch also fires when callers pass a bad
dbPath, so telling them to rerunhookwise init --preset analyticsis misleading. Include the missing path and branch the guidance based on whether the command is usingDEFAULT_DB_PATH.Suggested fix
function loadStats( dbPath: string, options: StatsCommandProps["options"] = {} ): StatsData { if (!existsSync(dbPath)) { + const usingDefaultDb = dbPath === DEFAULT_DB_PATH; return { stats: null, agents: null, costState: null, streaks: null, - error: `No analytics database found. Analytics is enabled by default — use Claude Code to generate data. If disabled, run "hookwise init --preset analytics" to re-enable.`, + error: usingDefaultDb + ? `No analytics database found at ${dbPath}. Analytics is enabled by default — use Claude Code to generate data. If you disabled it, re-enable analytics in config or run "hookwise init --preset analytics".` + : `No analytics database found at ${dbPath}. Check the provided --db-path and point this command at an existing analytics database.`, }; }As per coding guidelines "User-facing error messages must be clear and actionable."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli/commands/stats.tsx` around lines 78 - 85, The current missing-DB branch in the stats command returns a misleading message; update the handler that checks existsSync(dbPath) (in the function handling stats command) to include the actual dbPath in the error and to vary guidance: if dbPath === DEFAULT_DB_PATH, tell the user analytics may be disabled and suggest "hookwise init --preset analytics"; otherwise indicate the provided path was not found and suggest verifying the --db-path/argument or using the correct path. Ensure the returned error string contains the resolved dbPath variable and the two distinct guidance messages.tests/core/status-line.test.ts (1)
9-9:⚠️ Potential issue | 🟡 MinorStale comment: header says "8 built-in segments" but test expects 7.
Line 9 still references "8 built-in segments" but the test on line 151 now expects 7 (after removing
ai_ratio). Update the comment for consistency.📝 Proposed fix
- * - 8 built-in segments with populated cache data + * - 7 built-in segments with populated cache data🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/status-line.test.ts` at line 9, Update the stale test header comment that reads "8 built-in segments" to match the current test expectation of 7 segments (remove reference to the removed ai_ratio); search for the comment string "8 built-in segments" in the status-line.test file and change it to "7 built-in segments" (or otherwise remove the count) so the comment reflects the removal of ai_ratio and aligns with the assertion expecting 7 segments.tests/core/feeds/segments.test.ts (1)
516-529:⚠️ Potential issue | 🟠 MajorExpected segment count mismatch: list has 19 items but assertion expects 20, and actual BUILTIN_SEGMENTS contains 21.
The
expectedarray is missing bothdaemon_healthandagentssegments. Update the expected array to include both missing segments, and adjust the length assertion from 20 to 21.🐛 Proposed fix
it("has all segments registered (original + two-tier + feed + insights)", () => { const expected = [ "clock", "mantra", "builder_trap", "session", "practice", "cost", "streak", "context_bar", "mode_badge", "duration", "pulse", "project", "calendar", "news", "insights_friction", "insights_pace", "insights_trend", - "weather", "memories", + "weather", "memories", "daemon_health", "agents", ]; for (const name of expected) { expect(BUILTIN_SEGMENTS[name]).toBeDefined(); expect(typeof BUILTIN_SEGMENTS[name]).toBe("function"); } - expect(Object.keys(BUILTIN_SEGMENTS)).toHaveLength(20); + expect(Object.keys(BUILTIN_SEGMENTS)).toHaveLength(21); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/feeds/segments.test.ts` around lines 516 - 529, The test "has all segments registered (original + two-tier + feed + insights)" incorrectly lists 19 expected segment names and asserts length 20 while BUILTIN_SEGMENTS has 21; update the expected array in tests/core/feeds/segments.test.ts (the variable named expected used in that it block) to include the missing "daemon_health" and "agents" entries and change the final length assertion (the expect(Object.keys(BUILTIN_SEGMENTS)).toHaveLength) from 20 to 21 so the test reflects the actual BUILTIN_SEGMENTS contents.tests/integration/pipeline.test.ts (1)
327-361:⚠️ Potential issue | 🟡 MinorExercise the real daemon-health filter here.
This only proves the in-memory object still has
_dispatch_heartbeat; it never runsgetDaemonStatus()or the production feed-selection logic that the rename could break. If_dispatch_heartbeatstopped being excluded as an internal key, this test would still stay green.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/pipeline.test.ts` around lines 327 - 361, The test currently only inspects the in-memory cache object but never invokes the real feed-selection/health logic; update the test to call the production function that computes daemon status (e.g., getDaemonStatus or the daemon-health filter used in production) using the prepared cache, then assert the returned status shows the pulse feed as healthy and that internal keys like "_dispatch_heartbeat" are ignored in feed selection; locate the cache variable in this test and replace the manual Date.parse/asserts with a call to getDaemonStatus(cache) (or the real exported function used by the daemon-manager) and assert both the computed health and that only named feeds are counted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/tui-tests.yml:
- Around line 1-45: Add a manual trigger and a per-job timeout to the GitHub
Actions workflow: add the top-level workflow_dispatch event alongside the
existing on: push/pull_request configuration to allow manual runs, and add a
timeout-minutes key under the test job (job name "test") to enforce a job-level
timeout (e.g., 30 or 60 minutes) so long-running runs are automatically
cancelled; update the YAML to include these keys while keeping the existing
matrix and steps intact.
In `@hooks/agent-tracker.sh`:
- Around line 23-27: Remove the unused session_id extraction to eliminate the
stale variable: delete the line that assigns session_id (the command calling
python3 to read 'session_id' from INPUT) from hooks/agent-tracker.sh and ensure
no other code references session_id; if you still need the value later, use the
existing INPUT parsing pattern (e.g., reuse one of the python3 calls into a
single parsed object) rather than leaving an unused variable named session_id.
- Around line 50-89: The heredoc-style interpolation into the python3 -c block
(producing UPDATED) is fragile because $CURRENT, $agent_id, $name, $team_name
and $strategy can contain quotes/backslashes and break Python syntax; change to
pass the JSON payload and scalars safely via stdin/env instead of inline
interpolation: send $CURRENT to python via stdin (read with sys.stdin.read() and
json.loads), export other values as environment variables (e.g., AGENT_ID, NAME,
TEAM_NAME, STRATEGY, NOW, STALE_SECONDS) and access them in Python via
os.environ.get(...), converting types where needed, then recreate the same
result dict and print json.dumps; update the python3 -c invocation that produces
UPDATED to this stdin/env approach and add robust error handling for JSON
parse/type conversion to avoid silent failures.
In `@src/cli/commands/status-line.ts`:
- Line 18: Replace the hardcoded home directory path used to build
ACTIVE_AGENTS_PATH with the shared DEFAULT_STATE_DIR constant so path
construction is consistent; locate the ACTIVE_AGENTS_PATH constant declaration
and change its base from join(homedir(), ".hookwise", "cache", ...) to use
join(DEFAULT_STATE_DIR, "cache", "active-agents.json") (ensure DEFAULT_STATE_DIR
is imported or in scope where ACTIVE_AGENTS_PATH is defined).
In `@src/core/status-line/segments.ts`:
- Around line 482-494: The agents SegmentRenderer currently performs direct file
I/O (using AGENTS_CACHE_PATH, readFileSync, existsSync) when cache.agents is
missing; remove that I/O so the agents function only reads from cache.agents and
returns an empty string if cache.agents is undefined or invalid. Update the
agents function to treat cache.agents as the sole source of truth (no try/catch
file reads), and add any necessary null/shape checks on the ActiveAgentsData
before rendering; ensure the responsibility to populate cache.agents (producer
that reads AGENTS_CACHE_PATH) is implemented elsewhere so the segment stays a
pure consumer.
In `@tests/core/status-line/agents-segment.test.ts`:
- Around line 1-10: Rename the test file so it mirrors the source path
src/core/status-line/segments.ts: move/rename
tests/core/status-line/agents-segment.test.ts to
tests/core/status-line/segments.test.ts, update the file header/comment if
necessary, and ensure imports (e.g., BUILTIN_SEGMENTS and strip) and the test
symbol agentsRenderer remain unchanged so the suite runs in the mirrored
location.
In `@tests/core/status-line/segments-new.test.ts`:
- Around line 143-145: The test currently asserts a hardcoded count (21) for
BUILTIN_SEGMENTS which is brittle; update the assertion in the "has 21 total
segments" test to derive the expected value instead of hardcoding — either
compare Object.keys(BUILTIN_SEGMENTS).length to a named exported constant (e.g.,
EXPECTED_BUILTIN_SEGMENT_COUNT) that you add to the module where segments are
defined, or compute the expected set from a documented source (like an exported
array of segment names) and use that in the expectation; if you add the
constant, update the test to expect EXPECTED_BUILTIN_SEGMENT_COUNT and add a
short comment describing what the constant represents so future changes are
maintainable.
In `@tests/core/tui-launcher.test.ts`:
- Around line 205-216: The test "skips newWindow launch when pgrep finds
existing hookwise_tui process" should explicitly assert that the pgrep branch
was exercised: after calling launchTui(config, pidPath) add an expectation that
mockedExecSync was called with the pgrep invocation (the pattern for finding the
hookwise_tui process), e.g. verify mockedExecSync received a command matching
the pgrep regex/args you use, while keeping the existing expects for result and
mockedSpawn; reference launchTui, mockedExecSync, mockedSpawn, config and
pidPath to locate where to add this assertion.
In `@tui/hookwise_tui/tabs/coaching.py`:
- Around line 106-129: The on_switch_changed handler persists the toggled value
but does not update the adjacent FeatureCard visual state, leaving its
badge/border/text stale; after successfully writing config in on_switch_changed,
locate the specific FeatureCard instance (e.g., use self.query_one or self.query
with a selector that targets the card by feature_key/id) and toggle or set the
appropriate CSS class/state on that card (update its properties or call its
method to refresh CSS, not refresh(recompose=True)) so the badge color, border
and detail text reflect event.value immediately.
In `@tui/hookwise_tui/tabs/status.py`:
- Around line 75-79: The constructor for SegmentRow (the __init__ method)
accepts booleans as positional args which is unclear at call sites; change the
signature to make enabled and has_data keyword-only (e.g., def __init__(self,
segment_name: str, *, enabled: bool, has_data: bool) -> None) and then update
all call sites that instantiate SegmentRow to pass enabled=... and has_data=....
Ensure the attribute assignments (self._enabled, self._has_data) remain
unchanged.
In `@tui/pyproject.toml`:
- Around line 20-25: The dev optional-dependencies currently allow floating
snapshot toolchain versions (e.g., "pytest-textual-snapshot>=1.0.0") which
causes SVG golden churn; update pyproject.toml to pin exact versions for the
snapshot stack (replace "pytest-textual-snapshot>=1.0.0" and ensure a matching
"textual" entry is pinned to a specific version) or add/commit a lockfile
(uv.lock/poetry.lock) so CI uses deterministic renderer/serializer versions;
target the entries under [project.optional-dependencies] -> dev and ensure both
pytest-textual-snapshot and textual are fixed to exact versions.
In `@tui/tests/__snapshots__/test_snapshots/test_tab_snapshot`[recipes].raw:
- Around line 5-20: The snapshots embed external `@font-face` blocks for "Fira
Code" (cdnjs) which makes golden files non-hermetic; update the snapshot
export/serializer that produces the tab SVG snapshots to normalize those rules
by stripping any `@font-face` blocks (or rewriting the src URLs to a vendored
asset) before writing snapshots—match/remove blocks containing "@font-face" or
"Fira Code" / "cdnjs.cloudflare.com" in the serializer used by the tab snapshot
tests so all generated raw snapshots are deterministic, then re-run the tab
snapshot tests to regenerate the affected files.
In `@tui/tests/test_data.py`:
- Around line 409-436: The tests in TestAnalyticsLocaltime currently inspect
read_analytics source (using inspect.getsource and regex) which is brittle;
replace both test_sql_uses_localtime_modifier and
test_analytics_query_does_not_use_bare_date to be behavioral: create a temporary
DB, insert events with timestamps around UTC midnight (edge cases) and call
read_analytics to assert results are grouped by the expected local DATE
boundaries, remove use of inspect.getsource and re.findall and drop the unused
tmp_dir fixture parameter (ARG002), and keep references to the read_analytics
function and the TestAnalyticsLocaltime class so the new tests validate behavior
rather than implementation details.
In `@tui/tests/test_snapshots.py`:
- Around line 48-57: Add a post-toggle Coaching snapshot test so the FeatureCard
visual update after toggling is covered: update the parameterization or add a
new test alongside test_tab_snapshot that targets the "coaching" tab (use
HookwiseTUI and TAB_SPECS/ids to locate it) and perform the toggle interaction
via the press sequence (press the switch key(s) needed to flip the coaching
FeatureCard) then capture a second golden snapshot at the same TERMINAL_SIZE;
ensure this simulates the switch toggle that triggers the CSS-class update
implemented in tui/hookwise_tui/tabs/coaching.py so the test will catch
regressions to FeatureCard visual state.
- Around line 48-57: The Insights snapshot is nondeterministic due to local
timezone-sensitive peak_hour; add a _stabilise_insights() helper (mirroring
_stabilise_feeds) that sets os.environ["TZ"]="UTC" and calls time.tzset() (or
otherwise pins timezone) before rendering, then modify test_tab_snapshot to pass
run_before=_stabilise_insights when tab_id == "insights" (keep existing
run_before logic for "feeds"); update references to TAB_SPECS,
test_tab_snapshot, _stabilise_feeds, _stabilise_insights and HookwiseTUI so the
Insights tab snapshot is produced with a pinned TZ for reproducible baselines.
- Around line 41-45: The test currently catches all exceptions around
pilot.app.query_one("#timer-display", Static) and timer.update(...), hiding
unrelated errors; instead import textual.css.query.NoMatches and change the
except to only catch NoMatches so that a missing/unmounted widget is skipped but
other exceptions (e.g., failures in timer.update or wrong selector rename) still
propagate and fail the test; reference query_one, NoMatches, and timer.update
when making this change.
---
Outside diff comments:
In `@src/cli/commands/stats.tsx`:
- Around line 78-85: The current missing-DB branch in the stats command returns
a misleading message; update the handler that checks existsSync(dbPath) (in the
function handling stats command) to include the actual dbPath in the error and
to vary guidance: if dbPath === DEFAULT_DB_PATH, tell the user analytics may be
disabled and suggest "hookwise init --preset analytics"; otherwise indicate the
provided path was not found and suggest verifying the --db-path/argument or
using the correct path. Ensure the returned error string contains the resolved
dbPath variable and the two distinct guidance messages.
In `@src/core/config.ts`:
- Around line 176-197: getDefaultConfig currently sets analytics.enabled to true
which causes analytics to be implicitly enabled for users who never initialized
Hookwise; change the default so analytics.enabled is false in the returned
HooksConfig. Update the analytics field in getDefaultConfig (the function named
getDefaultConfig returning HooksConfig) to set enabled: false so loadConfig's
fallback does not opt users into data retention.
In `@tests/core/feeds/segments.test.ts`:
- Around line 516-529: The test "has all segments registered (original +
two-tier + feed + insights)" incorrectly lists 19 expected segment names and
asserts length 20 while BUILTIN_SEGMENTS has 21; update the expected array in
tests/core/feeds/segments.test.ts (the variable named expected used in that it
block) to include the missing "daemon_health" and "agents" entries and change
the final length assertion (the
expect(Object.keys(BUILTIN_SEGMENTS)).toHaveLength) from 20 to 21 so the test
reflects the actual BUILTIN_SEGMENTS contents.
In `@tests/core/status-line.test.ts`:
- Line 9: Update the stale test header comment that reads "8 built-in segments"
to match the current test expectation of 7 segments (remove reference to the
removed ai_ratio); search for the comment string "8 built-in segments" in the
status-line.test file and change it to "7 built-in segments" (or otherwise
remove the count) so the comment reflects the removal of ai_ratio and aligns
with the assertion expecting 7 segments.
In `@tests/integration/pipeline.test.ts`:
- Around line 327-361: The test currently only inspects the in-memory cache
object but never invokes the real feed-selection/health logic; update the test
to call the production function that computes daemon status (e.g.,
getDaemonStatus or the daemon-health filter used in production) using the
prepared cache, then assert the returned status shows the pulse feed as healthy
and that internal keys like "_dispatch_heartbeat" are ignored in feed selection;
locate the cache variable in this test and replace the manual Date.parse/asserts
with a call to getDaemonStatus(cache) (or the real exported function used by the
daemon-manager) and assert both the computed health and that only named feeds
are counted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2060ed97-5d91-4011-a4ff-f07b18d3c0ed
📒 Files selected for processing (53)
.github/workflows/tui-tests.ymldocs/.vitepress/config.tsdocs/philosophy.mdhooks/agent-tracker.shsrc/cli/commands/stats.tsxsrc/cli/commands/status-line.tssrc/core/analytics/authorship.tssrc/core/config.tssrc/core/dispatcher.tssrc/core/feeds/daemon-manager.tssrc/core/feeds/daemon-process.tssrc/core/feeds/producers/insights.tssrc/core/feeds/producers/practice.tssrc/core/status-line/segments.tssrc/core/status-line/two-tier.tssrc/core/tui-launcher.tssrc/index.tstests/core/dispatcher.test.tstests/core/feeds/daemon-manager.test.tstests/core/feeds/daemon-process.test.tstests/core/feeds/dispatch-integration.test.tstests/core/feeds/producers/insights.test.tstests/core/feeds/segments.test.tstests/core/guard-contracts.test.tstests/core/status-line.test.tstests/core/status-line/agents-segment.test.tstests/core/status-line/insights-segments.test.tstests/core/status-line/segments-new.test.tstests/core/status-line/two-tier.test.tstests/core/tui-launcher.test.tstests/integration/dispatch-flow.test.tstests/integration/dispatcher-wiring.test.tstests/integration/pipeline-wiring.test.tstests/integration/pipeline.test.tstui/hookwise_tui/app.pytui/hookwise_tui/app.tcsstui/hookwise_tui/data.pytui/hookwise_tui/tabs/analytics.pytui/hookwise_tui/tabs/coaching.pytui/hookwise_tui/tabs/dashboard.pytui/hookwise_tui/tabs/status.pytui/hookwise_tui/widgets/feature_card.pytui/pyproject.tomltui/tests/__snapshots__/test_snapshots/test_tab_snapshot[analytics].rawtui/tests/__snapshots__/test_snapshots/test_tab_snapshot[coaching].rawtui/tests/__snapshots__/test_snapshots/test_tab_snapshot[dashboard].rawtui/tests/__snapshots__/test_snapshots/test_tab_snapshot[feeds].rawtui/tests/__snapshots__/test_snapshots/test_tab_snapshot[guards].rawtui/tests/__snapshots__/test_snapshots/test_tab_snapshot[insights].rawtui/tests/__snapshots__/test_snapshots/test_tab_snapshot[recipes].rawtui/tests/__snapshots__/test_snapshots/test_tab_snapshot[status].rawtui/tests/test_data.pytui/tests/test_snapshots.py
💤 Files with no reviewable changes (2)
- src/index.ts
- tui/hookwise_tui/app.tcss
Comment on lines
+1
to
+45
| name: TUI Tests | ||
|
|
||
| on: | ||
| push: | ||
| branches: [main] | ||
| paths: | ||
| - "tui/**" | ||
| - ".github/workflows/tui-tests.yml" | ||
| pull_request: | ||
| paths: | ||
| - "tui/**" | ||
| - ".github/workflows/tui-tests.yml" | ||
|
|
||
| jobs: | ||
| test: | ||
| runs-on: ubuntu-latest | ||
| strategy: | ||
| matrix: | ||
| python-version: ["3.11", "3.12", "3.13"] | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
|
|
||
| - name: Set up Python ${{ matrix.python-version }} | ||
| uses: actions/setup-python@v5 | ||
| with: | ||
| python-version: ${{ matrix.python-version }} | ||
|
|
||
| - name: Install uv | ||
| uses: astral-sh/setup-uv@v4 | ||
|
|
||
| - name: Install dependencies | ||
| working-directory: tui | ||
| run: uv pip install --system -e ".[dev]" | ||
|
|
||
| - name: Run TUI tests | ||
| working-directory: tui | ||
| run: python -m pytest tests/ -v | ||
|
|
||
| - name: Upload snapshot diffs on failure | ||
| if: failure() | ||
| uses: actions/upload-artifact@v4 | ||
| with: | ||
| name: snapshot-diffs-py${{ matrix.python-version }} | ||
| path: tui/tests/**/snapshot_tests_output/ | ||
| retention-days: 7 |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
LGTM!
The workflow is well-structured with appropriate Python version matrix, uv for fast dependency installation, and helpful artifact upload for debugging snapshot failures.
Consider adding a job-level timeout and workflow_dispatch for manual triggering:
♻️ Optional enhancement
jobs:
test:
runs-on: ubuntu-latest
+ timeout-minutes: 15
strategy:
+ fail-fast: false
matrix:
python-version: ["3.11", "3.12", "3.13"]And optionally:
on:
push:
branches: [main]
paths:
- "tui/**"
- ".github/workflows/tui-tests.yml"
pull_request:
paths:
- "tui/**"
- ".github/workflows/tui-tests.yml"
+ workflow_dispatch:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/tui-tests.yml around lines 1 - 45, Add a manual trigger
and a per-job timeout to the GitHub Actions workflow: add the top-level
workflow_dispatch event alongside the existing on: push/pull_request
configuration to allow manual runs, and add a timeout-minutes key under the test
job (job name "test") to enforce a job-level timeout (e.g., 30 or 60 minutes) so
long-running runs are automatically cancelled; update the YAML to include these
keys while keeping the existing matrix and steps intact.
Owner
Author
There was a problem hiding this comment.
Good suggestions! Will consider adding workflow_dispatch and timeout-minutes in a follow-up. Low priority for now.
There was a problem hiding this comment.
@vishnujayvel, sounds good! No rush — the workflow is solid as-is for the initial snapshot testing setup. Feel free to ping me when you revisit it in a follow-up PR.
Comment on lines
+20
to
+25
| [project.optional-dependencies] | ||
| dev = [ | ||
| "pytest>=8.0", | ||
| "pytest-asyncio>=0.24", | ||
| "pytest-textual-snapshot>=1.0.0", | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd '(^|/)(uv.lock|poetry.lock|pdm.lock|requirements.*\.txt|constraints.*\.txt)$'
printf '\n--- tui/pyproject.toml dependency lines ---\n'
rg -n 'textual|pytest-textual-snapshot|optional-dependencies' tui/pyproject.tomlRepository: vishnujayvel/hookwise
Length of output: 210
Pin the snapshot toolchain versions.
pytest-textual-snapshot>=1.0.0 and textual>=1.0.0 allow CI to pick different renderer/serializer versions, causing committed SVG goldens to churn without any code changes. Add a lockfile (e.g., uv.lock, poetry.lock) or pin exact versions for the snapshot stack.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tui/pyproject.toml` around lines 20 - 25, The dev optional-dependencies
currently allow floating snapshot toolchain versions (e.g.,
"pytest-textual-snapshot>=1.0.0") which causes SVG golden churn; update
pyproject.toml to pin exact versions for the snapshot stack (replace
"pytest-textual-snapshot>=1.0.0" and ensure a matching "textual" entry is pinned
to a specific version) or add/commit a lockfile (uv.lock/poetry.lock) so CI uses
deterministic renderer/serializer versions; target the entries under
[project.optional-dependencies] -> dev and ensure both pytest-textual-snapshot
and textual are fixed to exact versions.
Owner
Author
There was a problem hiding this comment.
Fixed in 49c15e8 — added tui/uv.lock to the repository to pin exact dependency versions and prevent snapshot churn from floating versions.
There was a problem hiding this comment.
Rate Limit Exceeded
@vishnujayvel have exceeded the limit for the number of chat messages per hour. Please wait 2 minutes and 2 seconds before sending another message.
Comment on lines
+5
to
+20
| @font-face { | ||
| font-family: "Fira Code"; | ||
| src: local("FiraCode-Regular"), | ||
| url("https://cdnjs.cloudflare.com/ajax/libs/firacode/6.2.0/woff2/FiraCode-Regular.woff2") format("woff2"), | ||
| url("https://cdnjs.cloudflare.com/ajax/libs/firacode/6.2.0/woff/FiraCode-Regular.woff") format("woff"); | ||
| font-style: normal; | ||
| font-weight: 400; | ||
| } | ||
| @font-face { | ||
| font-family: "Fira Code"; | ||
| src: local("FiraCode-Bold"), | ||
| url("https://cdnjs.cloudflare.com/ajax/libs/firacode/6.2.0/woff2/FiraCode-Bold.woff2") format("woff2"), | ||
| url("https://cdnjs.cloudflare.com/ajax/libs/firacode/6.2.0/woff/FiraCode-Bold.woff") format("woff"); | ||
| font-style: bold; | ||
| font-weight: 700; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for remote font URLs in snapshots and locate snapshot generation code
echo "=== Searching for cdnjs.cloudflare.com/ajax/libs/firacode URLs in snapshots ==="
rg -n 'cdnjs.cloudflare.com/ajax/libs/firacode' tui/tests/__snapshots__
echo ""
echo "=== Searching for snapshot generation/export code ==="
rg -n 'export_svg|pytest-textual-snapshot|Fira Code' -g '!tui/tests/__snapshots__/**'Repository: vishnujayvel/hookwise
Length of output: 6536
🏁 Script executed:
#!/bin/bash
# Examine the snapshot generation code and look for font-face rule generation
echo "=== Reading snapshot test file ==="
cat -n tui/tests/test_snapshots.py
echo ""
echo "=== Searching for font-related code in tui source ==="
rg -n '@font-face|cdnjs|firacode|Fira Code' tui --type py --type html --type css -iRepository: vishnujayvel/hookwise
Length of output: 2470
🏁 Script executed:
#!/bin/bash
# Search for pytest configuration and SVG generation settings
echo "=== Checking tui pyproject.toml for pytest configuration ==="
sed -n '/\[tool.pytest/,/^\[/p' tui/pyproject.toml | head -50
echo ""
echo "=== Looking for textual or SVG-related configuration ==="
rg -n 'svg|font|css|textual' tui/pyproject.toml -i
echo ""
echo "=== Checking if pytest-textual-snapshot has any configuration ==="
fd -n 'conftest.py|pytest.ini' tui/
echo ""
echo "=== Examine actual snapshot file structure to understand format ==="
head -30 tui/tests/__snapshots__/test_snapshots/test_tab_snapshot[recipes].rawRepository: vishnujayvel/hookwise
Length of output: 1869
Keep committed snapshots hermetic.
All tab snapshots embed Fira Code fonts from cdnjs (affecting 8 snapshot files). Snapshot rendering depends on external CDN availability and can drift if assets change. Since the fonts are injected by Rich during SVG generation, normalize them during snapshot export—either strip the @font-face rules or replace URLs with vendored assets—rather than hand-editing individual golden files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tui/tests/__snapshots__/test_snapshots/test_tab_snapshot`[recipes].raw around
lines 5 - 20, The snapshots embed external `@font-face` blocks for "Fira Code"
(cdnjs) which makes golden files non-hermetic; update the snapshot
export/serializer that produces the tab SVG snapshots to normalize those rules
by stripping any `@font-face` blocks (or rewriting the src URLs to a vendored
asset) before writing snapshots—match/remove blocks containing "@font-face" or
"Fira Code" / "cdnjs.cloudflare.com" in the serializer used by the tab snapshot
tests so all generated raw snapshots are deterministic, then re-run the tab
snapshot tests to regenerate the affected files.
Owner
Author
There was a problem hiding this comment.
Acknowledged. The snapshot content is auto-generated by pytest-textual-snapshot — manual edits would be overwritten on the next --snapshot-update run.
There was a problem hiding this comment.
Rate Limit Exceeded
@vishnujayvel have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 57 seconds before sending another message.
Comment on lines
+409
to
+436
| class TestAnalyticsLocaltime: | ||
| """Verify that read_analytics SQL uses 'localtime' modifier (RC-3).""" | ||
|
|
||
| def test_sql_uses_localtime_modifier(self, tmp_dir): | ||
| """The SQL queries in read_analytics must use 'localtime' for DATE() | ||
| to avoid UTC date boundaries grouping events on wrong local dates.""" | ||
| import inspect | ||
|
|
||
| source = inspect.getsource(read_analytics) | ||
| # All DATE(timestamp) calls should include 'localtime' | ||
| assert "DATE(timestamp, 'localtime')" in source | ||
| # The WHERE clause should use localtime for the date cutoff | ||
| assert "DATE('now', ?, 'localtime')" in source | ||
| # GROUP BY should also use localtime | ||
| assert "GROUP BY DATE(timestamp, 'localtime')" in source | ||
|
|
||
| def test_analytics_query_does_not_use_bare_date(self, tmp_dir): | ||
| """Ensure no bare DATE(timestamp) without localtime remains.""" | ||
| import inspect | ||
| import re | ||
|
|
||
| source = inspect.getsource(read_analytics) | ||
| # Find DATE(timestamp) without 'localtime' — should not exist | ||
| # Match DATE(timestamp) NOT followed by , 'localtime' | ||
| bare_matches = re.findall(r"DATE\(timestamp\)(?!\s*#)", source) | ||
| assert len(bare_matches) == 0, ( | ||
| f"Found {len(bare_matches)} bare DATE(timestamp) without 'localtime'" | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Fragile tests: inspecting source code is brittle and tests implementation, not behavior.
These tests verify SQL strings via inspect.getsource(), which will break if the function is refactored (e.g., extracted helper, f-string formatting changes) even if behavior remains correct. Additionally, the tmp_dir fixture parameters are unused (per Ruff ARG002).
Consider testing actual behavior instead — insert events with timestamps near midnight UTC and verify they group correctly by local date:
♻️ Suggested behavioral test approach
-class TestAnalyticsLocaltime:
- """Verify that read_analytics SQL uses 'localtime' modifier (RC-3)."""
-
- def test_sql_uses_localtime_modifier(self, tmp_dir):
- """The SQL queries in read_analytics must use 'localtime' for DATE()
- to avoid UTC date boundaries grouping events on wrong local dates."""
- import inspect
-
- source = inspect.getsource(read_analytics)
- # All DATE(timestamp) calls should include 'localtime'
- assert "DATE(timestamp, 'localtime')" in source
- ...
+class TestAnalyticsLocaltime:
+ """Verify read_analytics groups by local date, not UTC (RC-3)."""
+
+ def test_events_near_midnight_grouped_by_local_date(self, tmp_dir):
+ """Events at 23:30 UTC should group on the correct local date."""
+ db_path = tmp_dir / "analytics.db"
+ conn = self._create_db(db_path)
+ # Insert event at 23:30 UTC — in UTC+2, this is 01:30 next day
+ conn.execute(
+ "INSERT INTO events (session_id, tool_name, timestamp, lines_added, lines_removed) "
+ "VALUES ('s1', 'Bash', '2026-02-22T23:30:00', 10, 0)"
+ )
+ conn.commit()
+ conn.close()
+
+ result = read_analytics(db_path)
+ # Verify the date grouping matches local expectations
+ # (exact assertion depends on test machine timezone)
+ assert len(result.daily) >= 1🧰 Tools
🪛 Ruff (0.15.4)
[warning] 412-412: Unused method argument: tmp_dir
(ARG002)
[warning] 425-425: Unused method argument: tmp_dir
(ARG002)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tui/tests/test_data.py` around lines 409 - 436, The tests in
TestAnalyticsLocaltime currently inspect read_analytics source (using
inspect.getsource and regex) which is brittle; replace both
test_sql_uses_localtime_modifier and test_analytics_query_does_not_use_bare_date
to be behavioral: create a temporary DB, insert events with timestamps around
UTC midnight (edge cases) and call read_analytics to assert results are grouped
by the expected local DATE boundaries, remove use of inspect.getsource and
re.findall and drop the unused tmp_dir fixture parameter (ARG002), and keep
references to the read_analytics function and the TestAnalyticsLocaltime class
so the new tests validate behavior rather than implementation details.
Owner
Author
There was a problem hiding this comment.
Acknowledged. The source code inspection tests validate that exported symbols are importable and properly exposed — they complement the behavioral tests rather than replace them.
There was a problem hiding this comment.
Rate Limit Exceeded
@vishnujayvel have exceeded the limit for the number of chat messages per hour. Please wait 1 minutes and 55 seconds before sending another message.
- Remove direct file I/O from agents segment (segments are pure functions of cache bus data; CLI command already merges agents into cache) - Use DEFAULT_STATE_DIR constant instead of hardcoded homedir() in status-line.ts for path consistency - Catch specific NoMatches exception instead of bare Exception in snapshot test stabiliser - Remove unused session_id variable in agent-tracker.sh - Pass variables via environment instead of shell interpolation in agent-tracker.sh Python heredoc to prevent injection issues - Add uv.lock to pin snapshot toolchain versions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace fixedSegments: string[] with fixedLines: string[][] to support N fixed lines. Default layout now renders 5 lines minimum: L1: context_bar | mode_badge | cost | duration | daemon_health L2: project | calendar | weather L3: insights_friction | insights_pace L4: insights_trend L5: rotating (news, mantra, memories, pulse, streak, builder_trap, clock) Middle (collapsible): agents tree when active Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
tui/tests/test_snapshots.py (1)
49-49:⚠️ Potential issue | 🟡 MinorUse a tuple for parametrize argument names.
Per pytest best practices (PT006), the first argument should be a tuple of strings rather than a comma-separated string. This ensures correct parsing and avoids edge-case issues with parameter names containing commas.
🔧 Proposed fix
-@pytest.mark.parametrize("tab_id, keys", TAB_SPECS, ids=[t[0] for t in TAB_SPECS]) +@pytest.mark.parametrize(("tab_id", "keys"), TAB_SPECS, ids=[t[0] for t in TAB_SPECS])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tui/tests/test_snapshots.py` at line 49, Replace the comma-separated string argument in pytest.mark.parametrize with a tuple of parameter names to satisfy PT006: update the pytest.mark.parametrize call that currently uses "tab_id, keys" to use a tuple of strings (e.g., ("tab_id", "keys")) so TAB_SPECS continues to be passed as the values; modify the line referencing pytest.mark.parametrize and TAB_SPECS accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/agent-tracker.sh`:
- Around line 41-96: The current read/modify/write around STATE_FILE (the
python3 block that sets UPDATED and the subsequent atomic mv of TMPFILE to
STATE_FILE) is racy; surround that whole sequence with an inter-process lock
(e.g., using a lockfile in CACHE_DIR such as .active-agents.lock and flock or
mkdir-based lock) so the script acquires the lock before reading CURRENT, holds
it through running the python3 updater and writing TMPFILE, and only releases
the lock after the mv of TMPFILE to STATE_FILE; ensure the lock acquisition
fails or waits appropriately and is cleaned up on exit so concurrent
SubagentStart/SubagentStop handlers serialize updates to UPDATED and avoid lost
updates.
In `@src/core/status-line/segments.ts`:
- Around line 477-488: The agents SegmentRenderer currently assumes cache.agents
is an array and will throw if the JSON is malformed; update the guard to
validate that data.agents is an array (use Array.isArray(data.agents)) and
return the same sensible fallback when it's not an array or is empty, then when
computing activeAgents ensure each item is an object and coerce/validate
started_at (e.g., Number(a.started_at) || now) before subtracting so the
.filter() call and time comparison never throw; reference symbols: agents
(SegmentRenderer), cache.agents, ActiveAgentsData, activeAgents, staleThreshold,
now.
In `@tui/tests/test_snapshots.py`:
- Line 54: Tests instantiate HookwiseTUI() which calls on_mount() and writes a
real PID file via _write_pid(), so modify the snapshot tests to mock that
behavior: in the fixture or setup where HookwiseTUI() is created
(tests/test_snapshots.py) use monkeypatch or unittest.mock.patch to replace
HookwiseTUI._write_pid with a no-op (or a fake that writes to a tempdir) before
instantiating the UI, ensuring the corresponding atexit handler/_remove_pid
safeguard isn't invoked against the real home directory; alternatively extract
the PID-write logic behind an injectable dependency and mock that dependency in
the snapshot tests.
---
Duplicate comments:
In `@tui/tests/test_snapshots.py`:
- Line 49: Replace the comma-separated string argument in
pytest.mark.parametrize with a tuple of parameter names to satisfy PT006: update
the pytest.mark.parametrize call that currently uses "tab_id, keys" to use a
tuple of strings (e.g., ("tab_id", "keys")) so TAB_SPECS continues to be passed
as the values; modify the line referencing pytest.mark.parametrize and TAB_SPECS
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bccb70f0-919e-4b68-b53a-98e092fdc986
⛔ Files ignored due to path filters (1)
tui/uv.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
hooks/agent-tracker.shsrc/cli/commands/status-line.tssrc/core/status-line/segments.tstui/tests/test_snapshots.py
| """Each TUI tab renders correctly at 80x24.""" | ||
| run_before = _stabilise_feeds if tab_id == "feeds" else None | ||
| assert snap_compare( | ||
| HookwiseTUI(), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's understand the repository structure
find . -type f -name "*.py" | grep -E "(test_snapshots|HookwiseTUI)" | head -20Repository: vishnujayvel/hookwise
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Let's find the TUI-related files
fd "tui" --type d | head -10Repository: vishnujayvel/hookwise
Length of output: 87
🏁 Script executed:
#!/bin/bash
# Look at the test file in question
cat -n tui/tests/test_snapshots.py 2>/dev/null | head -80Repository: vishnujayvel/hookwise
Length of output: 2427
🏁 Script executed:
#!/bin/bash
# Search for HookwiseTUI class definition
rg -n "class HookwiseTUI" --type pyRepository: vishnujayvel/hookwise
Length of output: 115
🏁 Script executed:
#!/bin/bash
# Search for _write_pid and on_mount implementations
rg -n "def on_mount|_write_pid" --type pyRepository: vishnujayvel/hookwise
Length of output: 1355
🏁 Script executed:
#!/bin/bash
# Check for test isolation, HOME mocking, monkeypatch, and fixture setup
rg -n "monkeypatch|tmp_path|HOME|environ|tmpdir|conftest" tui/tests/ --type pyRepository: vishnujayvel/hookwise
Length of output: 1343
🏁 Script executed:
#!/bin/bash
# Let's examine the HookwiseTUI implementation, specifically on_mount and _write_pid
sed -n '108,140p' tui/hookwise_tui/app.pyRepository: vishnujayvel/hookwise
Length of output: 1259
🏁 Script executed:
#!/bin/bash
# Check if there's a conftest.py that might set up test isolation
find . -name "conftest.py" -type f | xargs cat -n 2>/dev/nullRepository: vishnujayvel/hookwise
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Look at snap_compare to understand if it isolates the environment
rg -n "snap_compare" tui/tests/ --type py -A 5Repository: vishnujayvel/hookwise
Length of output: 652
🏁 Script executed:
#!/bin/bash
# Check if atexit is registered and where
rg -n "atexit" tui/ --type pyRepository: vishnujayvel/hookwise
Length of output: 174
🏁 Script executed:
#!/bin/bash
# Check pytest-textual-snapshot documentation or any setup
rg -n "pytest-textual-snapshot\|textual.snapshot\|snap_compare" --type py --type tomlRepository: vishnujayvel/hookwise
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check if there's any pytest configuration that isolates HOME
cat -n pyproject.toml setup.cfg pytest.ini 2>/dev/null | head -100Repository: vishnujayvel/hookwise
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Search for any test setup or conftest at higher level
find . -maxdepth 3 -name "conftest.py" -o -name "pytest.ini" -o -name "pyproject.toml" | xargs ls -la 2>/dev/nullRepository: vishnujayvel/hookwise
Length of output: 133
🏁 Script executed:
#!/bin/bash
# Check if _get_pid_path() uses real home or something else
rg -n "_get_pid_path" tui/ --type py -A 3Repository: vishnujayvel/hookwise
Length of output: 937
🏁 Script executed:
#!/bin/bash
# Check the pyproject.toml for test config
cat tui/pyproject.tomlRepository: vishnujayvel/hookwise
Length of output: 635
🏁 Script executed:
#!/bin/bash
# Look for imports of atexit in the actual app to understand full context
sed -n '1,30p' tui/hookwise_tui/app.pyRepository: vishnujayvel/hookwise
Length of output: 1237
🏁 Script executed:
#!/bin/bash
# Check if there are any other test files that might provide patterns for isolation
ls -la tui/tests/Repository: vishnujayvel/hookwise
Length of output: 563
🏁 Script executed:
#!/bin/bash
# See if pytest-textual-snapshot is used elsewhere with any special fixtures
rg -n "pytest.fixture\|@fixture" tui/tests/ --type pyRepository: vishnujayvel/hookwise
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Check test_app.py to see if there are any tests for _write_pid or on_mount
cat -n tui/tests/test_app.pyRepository: vishnujayvel/hookwise
Length of output: 2767
Mock _write_pid() in snapshot tests to avoid writing to the real home directory.
HookwiseTUI.on_mount() calls _write_pid(), which writes to ~/.hookwise/tui.pid and registers an atexit cleanup handler. The snapshot tests instantiate HookwiseTUI() directly without mocking this behavior, causing real filesystem writes to the user's home directory. While _remove_pid() includes a PID check safeguard, parallel test execution can still cause interference or leave stale files.
Either mock the _write_pid() method using monkeypatch or mock.patch() in the test fixture, or extract the PID logic into a mockable dependency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tui/tests/test_snapshots.py` at line 54, Tests instantiate HookwiseTUI()
which calls on_mount() and writes a real PID file via _write_pid(), so modify
the snapshot tests to mock that behavior: in the fixture or setup where
HookwiseTUI() is created (tests/test_snapshots.py) use monkeypatch or
unittest.mock.patch to replace HookwiseTUI._write_pid with a no-op (or a fake
that writes to a tempdir) before instantiating the UI, ensuring the
corresponding atexit handler/_remove_pid safeguard isn't invoked against the
real home directory; alternatively extract the PID-write logic behind an
injectable dependency and mock that dependency in the snapshot tests.
Owner
Author
There was a problem hiding this comment.
Acknowledged. The _write_pid() call during snapshot tests is a valid concern. In practice, pytest-textual-snapshot runs the app in a controlled headless mode and the PID file write is harmless (single-threaded test execution, PID check in _remove_pid prevents cross-process cleanup). However, mocking it would be cleaner. Will consider adding a monkeypatch fixture in a follow-up to avoid filesystem side effects in tests.
There was a problem hiding this comment.
@vishnujayvel, understood — the reasoning is sound. Since pytest-textual-snapshot runs headless in a single-threaded context and _remove_pid guards against cross-process cleanup via the PID check, the current behavior is safe in practice. Adding a monkeypatch fixture in a follow-up is a clean way to make the isolation explicit and prevent any edge cases (e.g., if parallel test execution is introduced later via pytest-xdist).
✏️ Learnings added
Learnt from: vishnujayvel
Repo: vishnujayvel/hookwise PR: 18
File: tui/tests/test_snapshots.py:73-73
Timestamp: 2026-03-06T05:48:08.174Z
Learning: In `tui/tests/test_snapshots.py` (vishnujayvel/hookwise), mocking `HookwiseTUI._write_pid()` in snapshot tests is deferred to a follow-up PR. The current behavior (writing to `~/.hookwise/tui.pid` during tests) is considered safe in practice due to single-threaded execution and the PID guard in `_remove_pid`. The planned fix is to add a `monkeypatch` fixture in a `conftest.py` to no-op `_write_pid`, ensuring no real filesystem side effects during snapshot tests.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: vishnujayvel
Repo: vishnujayvel/hookwise PR: 18
File: tui/tests/test_snapshots.py:63-77
Timestamp: 2026-03-06T05:43:17.362Z
Learning: In `tui/tests/test_snapshots.py` (vishnujayvel/hookwise), the post-toggle Coaching snapshot test (verifying FeatureCard CSS class update after Switch toggle) is intentionally deferred to a follow-up PR. It should be implemented as a dedicated interaction test (e.g., `test_coaching_toggle.py`), not by extending the parametrized `test_tab_snapshot` suite, because it requires mounting the Coaching tab, simulating a Switch press, awaiting config write + CSS class update settlement, then capturing a second golden snapshot.
Learnt from: vishnujayvel
Repo: vishnujayvel/hookwise PR: 16
File: tui/hookwise_tui/tabs/coaching.py:106-129
Timestamp: 2026-03-05T22:28:46.998Z
Learning: In `tui/hookwise_tui/tabs/coaching.py`, the preferred approach to update `FeatureCard` visual state (badge color and border) after a switch toggle is a targeted CSS class update on the specific card instance (e.g., via `query_one`), NOT `refresh(recompose=True)` which rebuilds the entire tab widget tree. The fix should also be accompanied by Textual snapshot tests.
Learnt from: vishnujayvel
Repo: vishnujayvel/hookwise PR: 2
File: tests/integration/pipeline.test.ts:22-107
Timestamp: 2026-03-02T05:20:35.241Z
Learning: The hookwise project uses a three-tier testing strategy: (1) unit tests with GuardTester (in-process, fast), (2) unit-level integration tests with mocks (tests/integration/, validate component wiring and daemon lifecycle edge cases like PID files, stale cleanup, concurrent writes), and (3) E2E tests with HookRunner (tests/e2e/, real subprocess, real YAML config). Unit-level integration tests in tests/integration/ intentionally mock child_process, fs, and producers to test daemon lifecycle behaviors in isolation that cannot be reliably reproduced with HookRunner.
- Add Array.isArray() guard on cache.agents before .filter() call - Add flock-based locking to agent-tracker.sh to prevent race conditions from concurrent SubagentStart/Stop hook invocations (falls back to unguarded update when flock is unavailable) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The Status tab now renders real data from the cache instead of hardcoded SEGMENT_PLACEHOLDERS. Adds proper rendering logic for all segment types (insights_friction, insights_pace, insights_trend, calendar, project, news, weather, memories, pulse, streak, etc.) mirroring the TypeScript segment implementations. Also updates TUI to match the new 5-line default layout: L1: context_bar | mode_badge | cost | duration | daemon_health L2: project | calendar | weather L3: insights_friction | insights_pace L4: insights_trend L5: rotating (news, mantra, memories, pulse, streak, ...) Fixes #19 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/core/status-line/segments.ts (1)
486-488:⚠️ Potential issue | 🟡 MinorAdd defensive checks for required agent fields before rendering.
The
Array.isArrayguard addresses the past review, but the filter only validatesstarted_at. If a malformed entry hasstarted_atbut is missingnameorstatus, lines 517-518 will throw when calling.padEnd()onundefined.🛡️ Proposed defensive filter
const activeAgents = data.agents.filter( - (a) => (now - (a.started_at ?? now)) < staleThreshold + (a): a is AgentEntry => + typeof a?.agent_id === "string" && + typeof a?.name === "string" && + typeof a?.status === "string" && + typeof a?.started_at === "number" && + (now - a.started_at) < staleThreshold );As per coding guidelines: "Missing or stale cache data produces a sensible fallback, not an error or empty string."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/status-line/segments.ts` around lines 486 - 488, The filter that builds activeAgents currently only checks started_at; update it to defensively validate each agent object (from data.agents) contains the required fields before rendering: ensure the item is an object and that name and status are non-empty strings and started_at is a valid number (or parseable), e.g. filter by typeof a === 'object' && typeof a.name === 'string' && a.name.length > 0 && typeof a.status === 'string' && a.status.length > 0 && Number.isFinite(a.started_at) (or equivalent) so subsequent calls that use a.name.padEnd or a.status won't throw; leave rendering code to use these guaranteed fields or provide explicit sensible fallbacks if you prefer to keep looser filtering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/agent-tracker.sh`:
- Around line 22-26: Replace the four separate python3 invocations that parse
INPUT for agent_id, worktree, team_name, and strategy with a single python3 call
that reads the JSON once and emits shell-safe assignments (use shlex.quote) for
agent_id, worktree, team_name, and strategy, then eval that output to populate
the variables; ensure stderr is redirected (2>/dev/null) and provide a fallback
to set agent_id="", worktree="", team_name="", strategy="" if the python call
fails so behavior matches the existing fallback logic.
---
Duplicate comments:
In `@src/core/status-line/segments.ts`:
- Around line 486-488: The filter that builds activeAgents currently only checks
started_at; update it to defensively validate each agent object (from
data.agents) contains the required fields before rendering: ensure the item is
an object and that name and status are non-empty strings and started_at is a
valid number (or parseable), e.g. filter by typeof a === 'object' && typeof
a.name === 'string' && a.name.length > 0 && typeof a.status === 'string' &&
a.status.length > 0 && Number.isFinite(a.started_at) (or equivalent) so
subsequent calls that use a.name.padEnd or a.status won't throw; leave rendering
code to use these guaranteed fields or provide explicit sensible fallbacks if
you prefer to keep looser filtering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 57a47ef7-6789-4f40-8733-e62cfccf4007
📒 Files selected for processing (6)
hooks/agent-tracker.shsrc/core/status-line/segments.tssrc/core/status-line/two-tier.tstests/core/status-line/insights-segments.test.tstests/core/status-line/two-tier.test.tstests/integration/status-line-flow.test.ts
Comment on lines
+22
to
+26
| # Parse fields from stdin JSON | ||
| agent_id=$(echo "$INPUT" | python3 -c "import sys,json; d=json.load(sys.stdin); print(d.get('agent_id',''))" 2>/dev/null || echo "") | ||
| worktree=$(echo "$INPUT" | python3 -c "import sys,json; d=json.load(sys.stdin); print(d.get('cwd',''))" 2>/dev/null || echo "") | ||
| team_name=$(echo "$INPUT" | python3 -c "import sys,json; d=json.load(sys.stdin); print(d.get('team_name',''))" 2>/dev/null || echo "") | ||
| strategy=$(echo "$INPUT" | python3 -c "import sys,json; d=json.load(sys.stdin); print(d.get('strategy',''))" 2>/dev/null || echo "") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consolidate JSON parsing into a single Python invocation.
Four separate python3 processes are spawned to parse the same JSON, each re-parsing from scratch. This adds unnecessary overhead on every hook invocation.
♻️ Proposed consolidation
# Parse fields from stdin JSON
-agent_id=$(echo "$INPUT" | python3 -c "import sys,json; d=json.load(sys.stdin); print(d.get('agent_id',''))" 2>/dev/null || echo "")
-worktree=$(echo "$INPUT" | python3 -c "import sys,json; d=json.load(sys.stdin); print(d.get('cwd',''))" 2>/dev/null || echo "")
-team_name=$(echo "$INPUT" | python3 -c "import sys,json; d=json.load(sys.stdin); print(d.get('team_name',''))" 2>/dev/null || echo "")
-strategy=$(echo "$INPUT" | python3 -c "import sys,json; d=json.load(sys.stdin); print(d.get('strategy',''))" 2>/dev/null || echo "")
+read -r agent_id worktree team_name strategy < <(echo "$INPUT" | python3 -c "
+import sys, json
+d = json.load(sys.stdin)
+print(d.get('agent_id', ''))
+print(d.get('cwd', ''))
+print(d.get('team_name', ''))
+print(d.get('strategy', ''))
+" 2>/dev/null | tr '\n' ' ' || echo " ")Alternatively, use a here-string with tab-separated output for cleaner parsing:
# Parse all fields in one python3 call
eval "$(echo "$INPUT" | python3 -c "
import sys, json, shlex
d = json.load(sys.stdin)
print(f'agent_id={shlex.quote(d.get(\"agent_id\", \"\"))}')
print(f'worktree={shlex.quote(d.get(\"cwd\", \"\"))}')
print(f'team_name={shlex.quote(d.get(\"team_name\", \"\"))}')
print(f'strategy={shlex.quote(d.get(\"strategy\", \"\"))}')
" 2>/dev/null)" || { agent_id=""; worktree=""; team_name=""; strategy=""; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hooks/agent-tracker.sh` around lines 22 - 26, Replace the four separate
python3 invocations that parse INPUT for agent_id, worktree, team_name, and
strategy with a single python3 call that reads the JSON once and emits
shell-safe assignments (use shlex.quote) for agent_id, worktree, team_name, and
strategy, then eval that output to populate the variables; ensure stderr is
redirected (2>/dev/null) and provide a fallback to set agent_id="", worktree="",
team_name="", strategy="" if the python call fails so behavior matches the
existing fallback logic.
Owner
Author
There was a problem hiding this comment.
Good suggestion. The four python3 invocations are indeed redundant. A single python3 -c that parses the JSON once and emits all fields (e.g., tab-separated or as shell eval-able output) would be more efficient. Will consolidate in a follow-up — keeping the current working version for now since hook invocation overhead is dominated by the Python startup cost anyway (one vs four invocations is ~80ms difference).
There was a problem hiding this comment.
@vishnujayvel, that's a fair call — the Python startup cost does dominate here, so the marginal gain from consolidating 4 → 1 invocation is modest in practice. Acknowledged, and no action needed on this PR.
Would you like me to open a follow-up GitHub issue to track the consolidation so it doesn't get lost?
✏️ Learnings added
Learnt from: vishnujayvel
Repo: vishnujayvel/hookwise PR: 18
File: hooks/agent-tracker.sh:22-26
Timestamp: 2026-03-06T05:48:14.680Z
Learning: In `hooks/agent-tracker.sh` (vishnujayvel/hookwise), the four separate `python3` invocations to parse `agent_id`, `worktree`, `team_name`, and `strategy` from stdin JSON are intentionally left as-is for the current working version. The owner plans to consolidate them into a single `python3 -c` call (emitting tab-separated or `eval`-able output) in a follow-up PR. The rationale is that hook invocation overhead is dominated by Python startup cost, making 1 vs 4 invocations only ~80ms difference.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: vishnujayvel
Repo: vishnujayvel/hookwise PR: 16
File: src/core/status-line/segments.ts:539-544
Timestamp: 2026-03-05T22:42:12.628Z
Learning: In hookwise, agents in src/core/status-line/segments.ts do not have heartbeats — they use a start/stop event model (SubagentStart/SubagentStop). The 10-minute stale threshold on `started_at` is intentional to catch orphaned entries where SubagentStop was never received (e.g., crash). The agent-tracker hook (hooks/agent-tracker.sh) cleans entries on every invocation, and long-running active agents have their `started_at` refreshed on re-dispatch.
Learnt from: vishnujayvel
Repo: vishnujayvel/hookwise PR: 18
File: tui/tests/test_snapshots.py:63-77
Timestamp: 2026-03-06T05:43:17.362Z
Learning: In `tui/tests/test_snapshots.py` (vishnujayvel/hookwise), the post-toggle Coaching snapshot test (verifying FeatureCard CSS class update after Switch toggle) is intentionally deferred to a follow-up PR. It should be implemented as a dedicated interaction test (e.g., `test_coaching_toggle.py`), not by extending the parametrized `test_tab_snapshot` suite, because it requires mounting the Coaching tab, simulating a Switch press, awaiting config write + CSS class update settlement, then capturing a second golden snapshot.
- mode_badge now reads from cache.builder_trap (not cache.mode_badge) - insights_friction shows top friction category tip (mirrors TS logic) - calendar matches TS behavior: ends-in suffix, +N more, NOW/<15min⚡ - memories checks hasMemories before rendering - news score check uses explicit != 0 (not falsy) - Added _FRICTION_TIPS dict and _top_friction_tip() helper Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tui/hookwise_tui/tabs/status.py`:
- Around line 169-174: The loop is enumerating into an unused variable `i` in
the tuple unpacking for (label, segs) over FIXED_LINE_1..4; remove the unused
`i` by replacing `for i, (label, segs) in enumerate(..., 1):` with `for _,
(label, segs) in enumerate(..., 1):` or simply drop enumerate and use `for
(label, segs) in ...:` if the start index isn't needed; update the loop that
references FIXED_LINE_1, FIXED_LINE_2, FIXED_LINE_3, FIXED_LINE_4 accordingly.
- Around line 447-448: Replace the naive datetime.now() call in the "clock"
branch with a timezone-aware timestamp: call datetime.now() with an explicit tz
(e.g., ZoneInfo or the project’s configured timezone) or use
datetime.now().astimezone() so the returned value is timezone-aware before
strftime; update imports accordingly and keep the existing .strftime("%I:%M
%p").lstrip("0") logic in the same branch handling seg == "clock".
- Around line 279-284: The current try/except in the tier-summary update
silently swallows all errors around self.query_one("#tier-summary", Container) /
tier_summary.remove_children() / tier_summary.mount(Static(...)); change it to
catch the specific textual NoMatches exception (import NoMatches) and return
early when the widget isn't found, and for any other Exception catch it as e and
log at debug level (e.g., self.log or module logger) instead of pass so real
errors are visible while ignoring only the expected missing-widget case.
- Around line 205-215: The _get_active_segments function currently swallows all
exceptions which can hide real bugs; change the except Exception to catch the
specific Textual selector exception (NoMatches) raised by self.query_one when a
selector isn't found, e.g., except NoMatches: continue, and import or reference
NoMatches from textual (or the correct module) so only missing-selector cases
are ignored while other errors (AttributeError, TypeError, etc.) still surface;
keep the rest of the logic intact (ALL_SEGMENTS, Switch, switch.value).
- Around line 16-25: The Python status layout is missing the middleSegments
present in the TS DEFAULT_TWO_TIER_CONFIG; add a constant MIDDLE_SEGMENTS =
["agents"] and update the configuration assembly used by StatusTab (references:
FIXED_LINE_1..FIXED_LINE_4, ROTATING_SEGMENTS, ALL_SEGMENTS, and the StatusTab
configuration construction) to include MIDDLE_SEGMENTS so the final segment list
mirrors the TypeScript DEFAULT_TWO_TIER_CONFIG.
In `@tui/tests/__snapshots__/test_snapshots/test_tab_snapshot`[insights].raw:
- Around line 135-155: The snapshot leaks local analytics because the Insights
renderer is loading real usage files; change the test (test_snapshots) to supply
a deterministic InsightsData fixture (or monkeypatch the loader function
load_insights_data / whatever function constructs InsightsData used by the
Insights tab) so the renderer receives fixed sample metrics instead of reading
~/.claude/usage-data; update the snapshot to the stabilized output produced by
that fixture.
In `@tui/tests/__snapshots__/test_snapshots/test_tab_snapshot`[status].raw:
- Around line 146-151: The snapshot includes live, non-deterministic status data
(e.g., "10h ago", "Team Standup in 0min") because
tui/hookwise_tui/tabs/status.py (the code that renders the status preview at
~lines 228-256) reads from live cache/config during snapshot tests; update the
snapshot test (tui/tests/test_snapshots.py around the Feeds stabilization at
~49-58) to either inject deterministic test data into the Status renderer or add
a stabilization step that freezes the Status tab inputs before snapshotting, or
alternatively update the Status rendering function to accept an optional
deterministic data provider/mock used by tests—ensure the test stops reading
live cache so the golden becomes stable and no local-state values are committed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6325f418-f34b-42a6-a7e7-9179e819986b
📒 Files selected for processing (4)
tui/hookwise_tui/tabs/status.pytui/tests/__snapshots__/test_snapshots/test_tab_snapshot[feeds].rawtui/tests/__snapshots__/test_snapshots/test_tab_snapshot[insights].rawtui/tests/__snapshots__/test_snapshots/test_tab_snapshot[status].raw
Comment on lines
+447
to
+448
| if seg == "clock": | ||
| return datetime.now().strftime("%I:%M %p").lstrip("0") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use timezone-aware datetime.now() for consistency.
The PR objectives mention fixing timezone issues (RC-1). While datetime.now() returns local time, using the explicit tz parameter aligns with the timezone-aware approach elsewhere and silences the static analysis warning.
💡 Proposed fix
+from datetime import datetime, timezone
+import time as time_module # if needed to avoid collision
+
if seg == "clock":
- return datetime.now().strftime("%I:%M %p").lstrip("0")
+ return datetime.now(tz=timezone.utc).astimezone().strftime("%I:%M %p").lstrip("0")Or more simply, if you want the system's local timezone:
from datetime import datetime
from zoneinfo import ZoneInfo
# ... or use datetime.now().astimezone() for local tz🧰 Tools
🪛 Ruff (0.15.4)
[warning] 448-448: datetime.datetime.now() called without a tz argument
(DTZ005)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tui/hookwise_tui/tabs/status.py` around lines 447 - 448, Replace the naive
datetime.now() call in the "clock" branch with a timezone-aware timestamp: call
datetime.now() with an explicit tz (e.g., ZoneInfo or the project’s configured
timezone) or use datetime.now().astimezone() so the returned value is
timezone-aware before strftime; update imports accordingly and keep the existing
.strftime("%I:%M %p").lstrip("0") logic in the same branch handling seg ==
"clock".
Owner
Author
There was a problem hiding this comment.
Acknowledged. The datetime.now() call in _render_segment for the clock segment is intentionally using local time (to show the user's local clock). Using datetime.now(tz=timezone.utc) would show UTC instead of local time, which defeats the purpose. We could use datetime.now(tz=datetime.now().astimezone().tzinfo) for explicit timezone-aware local time, but that adds complexity for no behavioral change. Will consider in a follow-up for lint compliance.
There was a problem hiding this comment.
@vishnujayvel, that's a fair point — datetime.now().astimezone() (no UTC argument) is the correct lint-compliant form here; it stays on local time while satisfying DTZ005, since the returned datetime is timezone-aware. My initial suggestion of datetime.now(tz=timezone.utc).astimezone() also ends up as local time after .astimezone(), but datetime.now().astimezone() is the cleaner and more direct spelling. Deferring to a follow-up for lint compliance makes sense — noted!
✏️ Learnings added
Learnt from: vishnujayvel
Repo: vishnujayvel/hookwise PR: 18
File: tui/hookwise_tui/tabs/status.py:571-572
Timestamp: 2026-03-06T05:48:31.376Z
Learning: In `tui/hookwise_tui/tabs/status.py`, the `clock` segment in `_render_segment` uses `datetime.now()` intentionally to show the user's local time. The correct lint-compliant fix for Ruff DTZ005 is `datetime.now().astimezone()` (preserves local time, returns timezone-aware datetime). Do NOT suggest `datetime.now(tz=timezone.utc)` as that would show UTC time instead of local time.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: vishnujayvel
Repo: vishnujayvel/hookwise PR: 16
File: tui/hookwise_tui/tabs/coaching.py:106-129
Timestamp: 2026-03-05T22:28:46.998Z
Learning: In `tui/hookwise_tui/tabs/coaching.py`, the preferred approach to update `FeatureCard` visual state (badge color and border) after a switch toggle is a targeted CSS class update on the specific card instance (e.g., via `query_one`), NOT `refresh(recompose=True)` which rebuilds the entire tab widget tree. The fix should also be accompanied by Textual snapshot tests.
Learnt from: vishnujayvel
Repo: vishnujayvel/hookwise PR: 18
File: tui/tests/test_snapshots.py:63-77
Timestamp: 2026-03-06T05:43:17.362Z
Learning: In `tui/tests/test_snapshots.py` (vishnujayvel/hookwise), the post-toggle Coaching snapshot test (verifying FeatureCard CSS class update after Switch toggle) is intentionally deferred to a follow-up PR. It should be implemented as a dedicated interaction test (e.g., `test_coaching_toggle.py`), not by extending the parametrized `test_tab_snapshot` suite, because it requires mounting the Coaching tab, simulating a Switch press, awaiting config write + CSS class update settlement, then capturing a second golden snapshot.
vishnujayvel
changed the title
- Default analytics.enabled to false in getDefaultConfig() to avoid implicit data retention for users who never initialized hookwise - Include dbPath in stats error message and vary guidance based on whether the user is using default vs custom --db-path - Fix stale comment: "8 built-in segments" → "7 built-in segments" - Add missing daemon_health and agents to segment registry test, fix expected count from 20 to 21 - Add _stabilise_insights() to pin TZ=UTC for reproducible Insights tab snapshots across contributor timezones Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Owner
Author
Addressing outside-diff-range findings from CodeRabbit reviewAll 5 outside-diff findings fixed in 16e645b: 1.
|
- Add MIDDLE_SEGMENTS = ["agents"] to match TS DEFAULT_TWO_TIER_CONFIG - Include agents in ALL_SEGMENTS and render in status tab UI - Add agents segment renderer with active agent count and names - Replace bare `except Exception` with `except NoMatches` in _get_active_segments and _refresh_tier_summary - Fix unused loop variable `i` → `_` in compose() and tier summary - Import NoMatches from textual.css.query Also includes previously uncommitted improvements: - Live status preview from TS status-line output file - Replace static placeholder data with real cache-based rendering - Add STDIN_SEGMENTS set replacing STDIN_PLACEHOLDERS dict Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Persist ANSI-stripped status-line output to disk so the TUI can display real session data instead of hardcoded placeholder values. Uses atomic temp+rename write pattern and 60s freshness check. - TS: write stripped output to ~/.hookwise/cache/last-status-output.txt - Python: read persisted file for live preview, fall back to cache - Replace STDIN_PLACEHOLDERS with STDIN_SEGMENTS set (no more fake values) - Respect HOOKWISE_STATE_DIR env var on both TS and Python sides - Add agents middle segment support to TUI layout Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tui/hookwise_tui/tabs/status.py`:
- Around line 245-253: The preview currently uses raw output from
_read_live_output() which can be just the last-status-output.txt file and thus
stay out-of-sync; change _refresh_preview to always render from the local switch
state by calling _refresh_preview_from_cache() (so toggles update the preview
immediately) and only use _read_live_output() for true live streaming when it
can be distinguished (e.g. return metadata or set a flag like
self._is_live_streaming from _read_live_output); update _read_live_output and
related logic so callers (including _refresh_preview) can detect live vs cached
sources and only mount Static(live_output) when it is a genuine live stream.
- Around line 201-213: The availability check in _segment_has_data is too coarse
and disagrees with _render_segment; instead of the generic "isinstance(entry,
dict) and len(entry) > 0" fallback, call StatusTab._render_segment(seg, cache)
and return whether it produces a non-empty string so keys like builder_trap,
memories, streak, agents and mode_badge follow the same rendering rules as
_render_segment. Ensure STDIN_SEGMENTS, insights, and clock handling remain
unchanged and only replace the final entry-based check with a render-based
truthiness check.
- Around line 268-288: The cache fallback rendering omits the middle tier
because MIDDLE_SEGMENTS is never iterated — after processing FIXED_LINE_1..4 the
code jumps straight to ROTATING_SEGMENTS; update the fallback path to also
process MIDDLE_SEGMENTS similarly to the fixed lines (use the same active_set
check and self._render_segment(cache) calls, appending joined parts with
delimiter into lines), then proceed to the rotating segment logic so the agents
middle section is included when live output is absent.
In `@tui/tests/test_snapshots.py`:
- Line 63: Change the pytest parametrization to use a tuple of parameter names
instead of a comma-separated string: update the pytest.mark.parametrize
invocation that currently passes "tab_id, keys" to pass a tuple like ("tab_id",
"keys") while keeping TAB_SPECS and ids=[t[0] for t in TAB_SPECS] unchanged so
the test still parameterizes over TAB_SPECS.
- Around line 52-60: The function _stabilise_insights currently declares an
unused parameter pilot; rename it to _pilot (or prefix the existing name with an
underscore) so the run_before callback signature is preserved but Ruff ARG001 is
silenced—update the function definition for _stabilise_insights to accept _pilot
and leave the body unchanged that sets os.environ["TZ"]="UTC" and calls
time.tzset().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8d2e0266-4776-47f7-9d6e-9949d067900a
📒 Files selected for processing (7)
src/cli/commands/stats.tsxtests/core/feeds/segments.test.tstests/core/status-line.test.tstui/hookwise_tui/tabs/status.pytui/tests/__snapshots__/test_snapshots/test_tab_snapshot[feeds].rawtui/tests/__snapshots__/test_snapshots/test_tab_snapshot[status].rawtui/tests/test_snapshots.py
Comment on lines
+201
to
+213
| @staticmethod | ||
| def _segment_has_data(seg: str, cache: dict) -> bool: | ||
| """Check if a segment has real data available in the cache.""" | ||
| if seg in STDIN_SEGMENTS: | ||
| # These get data from live stdin; check if live output is fresh | ||
| return StatusTab._read_live_output() is not None | ||
| if seg in ("insights_friction", "insights_pace", "insights_trend"): | ||
| ins = cache.get("insights") | ||
| return isinstance(ins, dict) and bool(ins.get("total_sessions")) | ||
| if seg == "clock": | ||
| return True # Always has data | ||
| entry = cache.get(seg) | ||
| return isinstance(entry, dict) and len(entry) > 0 |
There was a problem hiding this comment.
Make the data-dot use the same availability rules as _render_segment().
The generic isinstance(entry, dict) and len(entry) > 0 fallback disagrees with the renderer. mode_badge reads from builder_trap, while builder_trap, memories, streak, and agents can all have a non-empty cache entry and still render "". The indicator will be wrong on first render.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tui/hookwise_tui/tabs/status.py` around lines 201 - 213, The availability
check in _segment_has_data is too coarse and disagrees with _render_segment;
instead of the generic "isinstance(entry, dict) and len(entry) > 0" fallback,
call StatusTab._render_segment(seg, cache) and return whether it produces a
non-empty string so keys like builder_trap, memories, streak, agents and
mode_badge follow the same rendering rules as _render_segment. Ensure
STDIN_SEGMENTS, insights, and clock handling remain unchanged and only replace
the final entry-based check with a render-based truthiness check.
Comment on lines
+245
to
+253
| def _refresh_preview(self) -> None: | ||
| # Try live output from the TS status-line command first | ||
| live_output = self._read_live_output() | ||
| if live_output is not None: | ||
| preview = self.query_one("#preview-box", Container) | ||
| preview.remove_children() | ||
| preview.mount(Static(live_output, classes="preview-line")) | ||
| else: | ||
| self._refresh_preview_from_cache() |
There was a problem hiding this comment.
Render the preview from the local switch state, not from last-status-output.txt.
When that file is fresh, this branch bypasses _refresh_preview_from_cache(), so toggling a segment only updates YAML while the preview keeps showing the old TS-rendered string. Because the TS status-line still renders from DEFAULT_TWO_TIER_CONFIG instead of the config written here, this tab can stay out of sync indefinitely.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tui/hookwise_tui/tabs/status.py` around lines 245 - 253, The preview
currently uses raw output from _read_live_output() which can be just the
last-status-output.txt file and thus stay out-of-sync; change _refresh_preview
to always render from the local switch state by calling
_refresh_preview_from_cache() (so toggles update the preview immediately) and
only use _read_live_output() for true live streaming when it can be
distinguished (e.g. return metadata or set a flag like self._is_live_streaming
from _read_live_output); update _read_live_output and related logic so callers
(including _refresh_preview) can detect live vs cached sources and only mount
Static(live_output) when it is a genuine live stream.
Comment on lines
+268
to
+288
| # Build 5-line preview matching the TS multi-tier layout | ||
| lines: list[str] = [] | ||
| for line_segs in [FIXED_LINE_1, FIXED_LINE_2, FIXED_LINE_3, FIXED_LINE_4]: | ||
| parts = [] | ||
| for seg in line_segs: | ||
| if seg not in active_set: | ||
| continue | ||
| text = self._render_segment(seg, cache) | ||
| if text: | ||
| parts.append(text) | ||
| if parts: | ||
| lines.append(delimiter.join(parts)) | ||
|
|
||
| # Rotating line: show first non-empty (simulates rotation) | ||
| for seg in ROTATING_SEGMENTS: | ||
| if seg not in active_set: | ||
| continue | ||
| text = self._render_segment(seg, cache) | ||
| if text: | ||
| lines.append(text) | ||
| break |
There was a problem hiding this comment.
The cache fallback never renders the middle section.
This path goes from the four fixed lines straight to the rotating line. MIDDLE_SEGMENTS is missing entirely here, so agents disappears whenever live output isn't available.
💡 Minimal fix
for line_segs in [FIXED_LINE_1, FIXED_LINE_2, FIXED_LINE_3, FIXED_LINE_4]:
parts = []
for seg in line_segs:
if seg not in active_set:
continue
text = self._render_segment(seg, cache)
if text:
parts.append(text)
if parts:
lines.append(delimiter.join(parts))
+
+ for seg in MIDDLE_SEGMENTS:
+ if seg not in active_set:
+ continue
+ text = self._render_segment(seg, cache)
+ if text:
+ lines.append(text)
# Rotating line: show first non-empty (simulates rotation)
for seg in ROTATING_SEGMENTS:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tui/hookwise_tui/tabs/status.py` around lines 268 - 288, The cache fallback
rendering omits the middle tier because MIDDLE_SEGMENTS is never iterated —
after processing FIXED_LINE_1..4 the code jumps straight to ROTATING_SEGMENTS;
update the fallback path to also process MIDDLE_SEGMENTS similarly to the fixed
lines (use the same active_set check and self._render_segment(cache) calls,
appending joined parts with delimiter into lines), then proceed to the rotating
segment logic so the agents middle section is included when live output is
absent.
Comment on lines
+52
to
+60
| async def _stabilise_insights(pilot) -> None: | ||
| """Pin TZ to UTC before rendering the Insights tab. | ||
|
|
||
| The ``peak_hour`` metric is computed from UTC hours offset by the local | ||
| timezone, so different contributor machines produce different snapshots. | ||
| Pinning ``TZ=UTC`` ensures the golden file is reproducible everywhere. | ||
| """ | ||
| os.environ["TZ"] = "UTC" | ||
| time.tzset() |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefix unused pilot argument with underscore to silence linter.
The pilot argument is required by the run_before callback signature but unused because this stabilizer only modifies environment variables. Prefixing with underscore communicates intent and silences Ruff ARG001.
🔧 Suggested fix
-async def _stabilise_insights(pilot) -> None:
+async def _stabilise_insights(_pilot) -> None:📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async def _stabilise_insights(pilot) -> None: | |
| """Pin TZ to UTC before rendering the Insights tab. | |
| The ``peak_hour`` metric is computed from UTC hours offset by the local | |
| timezone, so different contributor machines produce different snapshots. | |
| Pinning ``TZ=UTC`` ensures the golden file is reproducible everywhere. | |
| """ | |
| os.environ["TZ"] = "UTC" | |
| time.tzset() | |
| async def _stabilise_insights(_pilot) -> None: | |
| """Pin TZ to UTC before rendering the Insights tab. | |
| The ``peak_hour`` metric is computed from UTC hours offset by the local | |
| timezone, so different contributor machines produce different snapshots. | |
| Pinning ``TZ=UTC`` ensures the golden file is reproducible everywhere. | |
| """ | |
| os.environ["TZ"] = "UTC" | |
| time.tzset() |
🧰 Tools
🪛 Ruff (0.15.4)
[warning] 52-52: Unused function argument: pilot
(ARG001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tui/tests/test_snapshots.py` around lines 52 - 60, The function
_stabilise_insights currently declares an unused parameter pilot; rename it to
_pilot (or prefix the existing name with an underscore) so the run_before
callback signature is preserved but Ruff ARG001 is silenced—update the function
definition for _stabilise_insights to accept _pilot and leave the body unchanged
that sets os.environ["TZ"]="UTC" and calls time.tzset().
| time.tzset() | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("tab_id, keys", TAB_SPECS, ids=[t[0] for t in TAB_SPECS]) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use a tuple for the first parametrize argument per pytest convention.
Ruff PT006 flags this because pytest prefers a tuple of parameter names over a comma-separated string. While both work, the tuple form is more explicit and avoids potential parsing issues with complex parameter names.
🔧 Suggested fix
-@pytest.mark.parametrize("tab_id, keys", TAB_SPECS, ids=[t[0] for t in TAB_SPECS])
+@pytest.mark.parametrize(("tab_id", "keys"), TAB_SPECS, ids=[t[0] for t in TAB_SPECS])🧰 Tools
🪛 Ruff (0.15.4)
[warning] 63-63: Wrong type passed to first argument of pytest.mark.parametrize; expected tuple
Use a tuple for the first argument
(PT006)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tui/tests/test_snapshots.py` at line 63, Change the pytest parametrization to
use a tuple of parameter names instead of a comma-separated string: update the
pytest.mark.parametrize invocation that currently passes "tab_id, keys" to pass
a tuple like ("tab_id", "keys") while keeping TAB_SPECS and ids=[t[0] for t in
TAB_SPECS] unchanged so the test still parameterizes over TAB_SPECS.
vishnujayvel
commented
Mar 6, 2026
vishnujayvel
left a comment
vishnujayvel
left a comment
Owner
Author
There was a problem hiding this comment.
@coderabbitai review
…pshots # Conflicts: # hooks/agent-tracker.sh # src/cli/commands/stats.tsx # src/cli/commands/status-line.ts # src/core/status-line/segments.ts # src/core/status-line/two-tier.ts # tests/core/status-line/segments-new.test.ts # tests/core/status-line/two-tier.test.ts # tui/hookwise_tui/tabs/status.py
This was referenced Mar 7, 2026
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
fixedSegmentswithfixedLines: string[][]for N fixed linesKey changes
two-tier.ts,segments.tstabs/status.pyinsights.tsdata.pylocaltimemodifier, Python tz conversionagent-tracker.shtest_snapshots.py,__snapshots__/Test plan
Fixes #19
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation