Skip to content

Resolve reopened Remote UI feedback - #70

Open
firewalker06 wants to merge 8 commits into
mainfrom
feedback-119-125-127
Open

Resolve reopened Remote UI feedback#70
firewalker06 wants to merge 8 commits into
mainfrom
feedback-119-125-127

Conversation

@firewalker06

@firewalker06 firewalker06 commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Summary

  • Make PR diff context discoverable in the real agent PR-detail view: accessible line/range selection, composer preview/removal, stale-safe server validation, and race-safe submission.
  • Replace the microphone toggle with a complete speech-to-text lifecycle: icon, idle/listening/processing/error/unsupported states, interim/final caret-aware insertion, clean stop/cancel/restart, and keyboard/mobile parity.
  • Remove the committed generated usage report and query payload while preserving docs/USAGE_METRICS.md; narrowly ignore regenerated usage reports.

Root causes

  • The existing diff selection UI only existed inside a disabled Review Inbox smoke scenario; the live agent PR-detail renderer still emitted inert lines.
  • Speech recognition only handled final results and collapsed failures to idle, with no visible microphone icon or robust lifecycle handling.
  • One generated metrics report and its query sidecar were tracked as documentation.

Validation

  • bundle exec ruby test/remote_server_test.rb
  • bin/test
  • bin/remote-ui-smoke
  • git diff --check
  • gitleaks detect --no-git --source . --redact --no-banner
  • Independent spec and repository-standards reviews: approved with no remaining material findings

CI note

Ruby 3.4 is green. The Ruby 3.2 job fails in the pre-existing remote-agent stop assertion in test/cli_command_test.rb; the exact failure is also present on base commit 9b277126 in main run 31267294233. This PR does not change that lifecycle path. The full suite passes locally.

Fizzy: #119, #127, #125

@firewalker06

Copy link
Copy Markdown
Owner Author

Follow-up UX refinement from operator feedback is in 2de424c.

  • Diff comments now open as a plain-text editor immediately below the selected line/range. “Send comment” submits one section directly; “Add section” queues multiple labeled ranges for one combined conversation comment.
  • Speech now starts with one press and survives same-route polling. The microphone sits below full-screen, the composer pulses while listening, transcript status remains screen-reader-only, and error/unsupported states stay distinct and actionable.
  • Mobile inline commenting hides the fixed conversation dock while editing so it cannot cover the textarea.

Validation: bin/test, bin/remote-ui-smoke, focused remote server tests, JS/Ruby syntax, git diff --check, and gitleaks all pass. Independent spec and standards re-reviews report no remaining material findings.

@firewalker06

Copy link
Copy Markdown
Owner Author

Latest UI polish is in ca64183.

  • Diff lines now use an empty gutter and show only a checkmark when selected; the native checkbox remains accessible but has no visible chrome.
  • Inline comment forms are structurally separated from wide code rows and sized to the visible diff container, so both actions stay on-screen even with multi-thousand-pixel code lines.
  • Speech transcript insertion follows the active textarea caret. End-of-draft dictation follows the bottom; middle-caret dictation scrolls only enough to keep the insertion visible; cancel restores the original scroll.

Validation: full bin/test, full Chromium bin/remote-ui-smoke, wide desktop/mobile geometry and screenshots, long end/middle STT scroll cases, JS syntax, git diff --check, and gitleaks. Independent spec and standards re-reviews found no remaining material issues.

@firewalker06

Copy link
Copy Markdown
Owner Author

Follow-up for Fizzy #119: fixed PR diff/comment persistence and selection latency.

Root cause: selecting a diff line rebuilt and replaced the full PR subtree, so cost scaled with PR size; polling also cloned the unchanged diff during editor reconciliation. Numeric scroll restoration could not guarantee that the same visible line stayed anchored.

Outcome:

  • line/range selection now patches only checkmarks, the inline form, and the guidance bar
  • unchanged snapshot-keyed diff and form nodes stay mounted through polling/rerenders
  • nested scroll offsets and the clicked line viewport position remain stable
  • Cancel and Escape clear the form, selected rows, checkmarks, and native control state in place
  • the hidden guidance placeholder prevents layout shift when the form appears

Chromium measurement on a 1,500-line fixture: 73.4 ms synchronous full render vs 20.5 ms synchronous selection update (~3.6x faster). The smoke test also verifies viewer/form node identity, replacement-container scroll, visible line position, range state, Cancel, Escape, forced refresh, mobile, and desktop.

Validation: bin/test; bin/remote-ui-smoke; bundle exec ruby test/remote_server_test.rb; git diff --check; gitleaks. Independent spec and standards re-reviews: no findings. Commit: f7290ed.

@firewalker06

Copy link
Copy Markdown
Owner Author

CI follow-up for f7290ed: both Ruby 3.4 jobs pass. Both Ruby 3.2 jobs fail at the existing unrelated remote-agent lifecycle assertion in test/cli_command_test.rb:568 (expected remote agent stop to stop the target), matching the baseline failure already seen on this PR. The changed Remote UI tests and complete local bin/test pass.

@firewalker06

Copy link
Copy Markdown
Owner Author

Implemented the PR diff render optimization in b440df9.\n\nRoot cause: the mounted diff DOM was preserved, but every unchanged poll still mapped every file, hunk, and line into a redundant HTML string before reconciliation. The viewer now uses snapshot + expand state as its version and emits an empty keyed shell only when the matching mounted viewer can be transplanted.\n\nBrowser validation with 1,500 lines:\n- full render: 80.7 ms synchronous\n- first transition poll: 71.0 ms\n- steady unchanged poll: 6.9 ms\n- both unchanged polls called renderDiffLine zero times\n- viewer node, inline comment node/value, focus, page scroll, and visible line position stayed stable\n- surrounding PR metadata still updated with poll preservation disabled\n- snapshot change and collapse each regenerated all 1,500 lines and replaced the viewer\n- collapse now keeps every file closed after a rebuild\n\nValidation:\n- bin/test\n- bin/remote-ui-smoke\n- git diff --check\n- gitleaks detect --no-banner --redact --source .\n- independent spec review: clean\n- independent standards review: clean\n\nNo merge performed; PR remains ready for orchestrator review.

@firewalker06

Copy link
Copy Markdown
Owner Author

CI follow-up: both Ruby 3.4 jobs and one Ruby 3.2 job passed on b440df9. The duplicate Ruby 3.2 run failed at test/cli_command_test.rb:568 (remote agent stop timing), the same unrelated flaky lifecycle assertion seen before; the other Ruby 3.2 run on the identical commit passed. Local bin/test also passed.

@firewalker06

Copy link
Copy Markdown
Owner Author

Added persistent, network-free agent PR listing in 8048a39.\n\nRoot cause: the browser cache was transient, but the minutes-long delay came from the server issuing one serial GitHub metadata request per PR on every GET /agents/:key/pull-requests. The first implementation review also caught a second scaling trap: reading the full diff snapshot JSON once per listed PR.\n\nOutcome:\n- newly discovered canonical PR references persist in ~/.tycho/logs/agents/pull_request_catalog.json\n- compact GitHub metadata survives server/browser restarts\n- ordinary listing makes zero GitHub requests\n- ordinary listing reads the catalog once and parses the shared diff snapshot store once\n- Refresh metadata is now an explicit operation and remains separate from patch fetching\n- fetching a diff also updates the same cached metadata\n- existing attachment records remain the authoritative agent occurrence source\n\nRegression coverage uses three PRs and proves zero metadata calls, one snapshot-store parse, explicit refresh behavior, persisted catalog contents, and restart recovery.\n\nValidation passed: bin/test, bin/remote-ui-smoke, git diff --check, gitleaks, independent spec review, and independent standards review.

@firewalker06

Copy link
Copy Markdown
Owner Author

CI follow-up for 8048a39: both Ruby 3.4 jobs and one Ruby 3.2 job passed. The duplicate Ruby 3.2 run failed at the existing test/cli_command_test.rb:568 remote-agent-stop timing assertion; the other Ruby 3.2 run on the identical commit passed. This is the same unrelated flake recorded earlier.

@firewalker06
firewalker06 force-pushed the feedback-119-125-127 branch from 8048a39 to eef6d00 Compare August 9, 2026 14:30
@firewalker06

Copy link
Copy Markdown
Owner Author

Refined the persistent catalog in eef6d00 so ownership is per agent.\n\n- each agent writes .pull_request_catalog.json beside its memory and attachment sidecars\n- opening one agent reads only that agent’s catalog; no shared cross-agent catalog is constructed\n- Catalog requires an explicit path, so callers cannot silently fall back to a shared file\n- metadata refresh and diff fetch update the owning agent’s sidecar\n- concurrent refreshes of the same PR across two agents still share one GitHub metadata/patch fetch, then each caller writes its own catalog\n- archive moves the catalog, backup, and lock sidecars and leaves no active copies\n\nRegression coverage now creates distinct agents with isolated catalogs and also exercises the same-PR coalescing follower path. Full bin/test, final bin/remote-ui-smoke, git diff --check, gitleaks, and both independent reviews pass.

@firewalker06

Copy link
Copy Markdown
Owner Author

CI follow-up for eef6d00: both Ruby 3.4 jobs passed. Both initial Ruby 3.2 jobs hit the known test/cli_command_test.rb:568 agent-stop timing flake; rerunning the failed jobs produced one Ruby 3.2 pass and one repeat failure on the identical commit. Local bin/test passes.

@firewalker06
firewalker06 force-pushed the feedback-119-125-127 branch from eef6d00 to a5c8849 Compare August 9, 2026 14:53
@firewalker06

Copy link
Copy Markdown
Owner Author

Updated the agent-owned PR catalog so each listed PR now uses the origin PR title and exposes its current state in both the list and detail header.

  • Persists title, state, draft, and merged per agent after explicit metadata or diff refreshes.
  • Backfills title/status from an existing saved diff snapshot without a network request.
  • Tracks metadata provenance so snapshot-backed display data does not falsely mark a diff fresh; only live GitHub metadata can establish freshness.
  • Uses status precedence Merged → Closed → Draft → Open.
  • Adds Chromium coverage that renders and verifies the origin title plus Open, Draft, Closed, and Merged in both navigation and detail UI.

Validation: bin/test, bin/remote-ui-smoke, git diff --check, and gitleaks pass locally. Independent spec and standards reviews found no remaining material issues.

@firewalker06

Copy link
Copy Markdown
Owner Author

CI is green on a5c8849: both Ruby 3.2 and Ruby 3.4 runs pass. The one intermittent agent-stop timing failure passed on rerun.

@firewalker06

Copy link
Copy Markdown
Owner Author

Fixed slow PR-to-PR navigation in commit 9e1f48f.

Root cause: selecting a PR started a forced agent conversation refresh and waited for conversation, skills, and project data before requesting the already-saved diff. Large agent histories could therefore delay the diff by minutes even though the snapshot was local.

The per-agent pull_request_catalog.json stores listing metadata. Diff bodies currently remain in the shared persistent snapshot store; the real 24.8 MB store parses in about 46 ms locally, so it was not the reported bottleneck.

PR route hydration now starts the catalog/saved-diff path concurrently with unrelated agent-shell data. The existing diff queue renders independently when the snapshot arrives, while diff DOM, selection form, and scroll preservation remain unchanged.

The Chromium regression holds conversation loading behind an explicit gate and requires the selected diff request to occur before releasing it. This proves causal independence without a machine-speed threshold.

Validation: bin/test, bin/remote-ui-smoke, git diff --check, and gitleaks pass. The 1,500-line smoke fixture renders in 84.0 ms synchronously; steady polling takes 6.1 ms. Independent spec and standards reviews found no remaining material issues.

@firewalker06

Copy link
Copy Markdown
Owner Author

CI is green on 9e1f48f across both Ruby 3.2 and Ruby 3.4 runs. The initial Ruby 3.2 attempts hit unrelated timing flakes in the agent-stop and peer-resource tests; each passed on rerun, and a duplicate full Ruby 3.2 run passed independently on the same commit.

@firewalker06

Copy link
Copy Markdown
Owner Author

Follow-up performance fix is now in e956dfd.

I reproduced this against the real Cookpad Pull Request Reviews agent with polling disabled. The agent had 27 catalog entries and 6 saved snapshots. The list API returned in about 230 ms and a diff API request in about 170 ms, but a cold browser switch took 3.5–3.6 s. A previously opened switch still took 1.62 s with no network request, which isolated the main bottleneck to browser DOM construction/layout rather than the poll or PR size alone.

The Remote UI now:

  • keeps the per-agent catalog and up to 12 loaded snapshot payloads in bounded memory;
  • preloads up to 6 saved snapshots, while reserving a request slot and prioritizing foreground navigation;
  • cancels queued preloads for agents/routes that are no longer open and discards their in-flight results;
  • retains up to 6 exact rendered diff viewers and their per-PR desktop/mobile scroll positions, so switching does not rebuild thousands of line controls;
  • renders large diffs in 100-line containment chunks;
  • keeps bulk refresh bounded and preserves the selected PR;
  • reuses an immutable parsed snapshot document on the server until the backing file changes.

Validation:

  • bin/test
  • bin/remote-ui-smoke — 5,000-line diff; foreground-before-background, refresh race, stale preload cancellation, cache bounds/LRU, DOM/form/selection/focus, desktop/mobile scroll, and polling persistence
  • bundle exec ruby test/pull_request_diff_test.rb
  • git diff --check
  • gitleaks detect --source . --no-banner --redact
  • independent spec review: no material gaps
  • independent standards/design review: no material gaps

PR: #70

@firewalker06

Copy link
Copy Markdown
Owner Author

CI follow-up: the complete pull-request-triggered workflow is green on Ruby 3.2 and 3.4. The duplicate push-triggered Ruby 3.2 job still fails at the pre-existing flaky assertion test/cli_command_test.rb:568 (expected remote agent stop to stop the target); Ruby 3.4 is green. I inspected the log and reran that failed job twice after the paired workflow completed, with the same unrelated stop-timing failure. Local bin/test passes. No merge was attempted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant