feat(inspector): filter the trace by what a reader actually asks - #2115
Conversation
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Approved — I don’t see a blocking correctness issue in the current head. A few non-blocking follow-ups would make the change tighter:
-
kindsandminCostUsdcurrently have no production caller: the panel can only producequery,failedOnly, or an empty filter. Since the PR presents four user-facing predicates, I’d either wire those two into the UI or, preferably for this PR, remove them and their tests until the controls are designed. -
The free-text reach is slightly wider than the rendered surface.
step.statusis not rendered as text, and the turn-levelfailureMessageis added to every step’s search text, so one failure-message match can retain every step in the turn. It would be clearer to align the searchable fields with what each rendered row actually explains. -
The filter result messages update as the user types but are not exposed through a live region. A persistent
role="status"container around the mutually exclusive result summary would make those updates available to screen-reader users. -
The “keeps a turn whose id matches” test does not exercise the special zero-step path because
turnIdis already included in every step’s search text. A zero-step turn fixture would cover the intended branch directly.
One caution: I would not fix the blank-query case by simply requiring query !== '' in turnMatches. With failedOnly active, retaining a failed zero-step turn is the sensible result. If the currently unreachable step-level predicates remain, the no-query/zero-step semantics should be made explicit rather than changed with that one-line guard.
Minor cleanup: the filter label can reuse the existing .maka-visually-hidden utility, and the inner failedOnly check in matchesStep is redundant with the outer turn guard.
|
Review by maka-审美专家 — request changes（逻辑层通过，UI 层要按设计规范改） 通过的部分：过滤器设计为纯谓词而非查询 API 的取舍正确；三条原则（未定价≠便宜、筛选为空≠会话为空、totals 不随筛选重算）都有测试钉死；自由文本触达范围=面板渲染字段并明确声明边界；turn id 命中保位的不对称是对的；replay step-through 主动不做并说明原因——这些判断质量都很高。 必须改（Astryx-first 是仓库现行设计规范）：
裸原生控件拿不到主题的字号/边框/焦点环，会在面板里成为一小块「不属于这个应用」的区域。改完 CI 绿我合。 |
…nothing calls Two reviews on apache#2115. **Design review — the filter bar was native controls.** A raw `<input>`, `<input type="checkbox">` and `<button>` sit in a themed panel without its type scale, borders or focus ring, which is how a small region stops looking like the app it lives in. Now `TextInput` (with `hasClear`, the searchable-input idiom), `Switch` and a ghost `Button`. `isLabelHidden` replaces the hand-rolled sr-only span, so this is no longer a third copy of that recipe, and the CSS it needed goes with it. One deviation from the review, stated rather than silently dropped: `TextInput` does not accept `type="search"` — its type union is `'text' | 'password' | 'email'`. `hasClear` is the part that makes it read as a search field, and it is there. **`kinds` and `minCostUsd` had no caller.** The panel can only produce a query and a failed-only toggle, so two of the four advertised predicates were surface with tests guarding nothing. They are gone until the controls that drive them are designed — a predicate nothing emits is exactly the kind of claim this track keeps having to walk back. **The search reach was wider than the rendered surface.** `step.status` is never rendered as text, and the turn's failure message was folded into every step's haystack, so one match on it retained steps that explain nothing about it. Each row is now searched by what it renders; the turn's id is matched against the turn, where it belongs. **Zero-step turns are explicit rather than incidental.** The old "turn id matches" test had steps, so the zero-step branch never ran — and with `failedOnly` and no text query, a turn that failed before recording a step would have been dropped for being empty, which is precisely what that filter is asked to surface. Both paths now have their own test. Also: the three result messages share one persistent `role="status"` region, so they are announced as the reader types rather than mounting silently; and the redundant turn-level check inside the step predicate is gone. 33 desktop inspector tests; a11y, dead-css, copy and format checks clean.
|
Both reviews addressed in @Astro-Han1. 2. Search reach was wider than the rendered surface. Also right, and the failure-message case was the worse half: folding turn text into every step's haystack meant one match retained steps that explain nothing about it. Each row is now searched by what it renders (kind, label, detail, recovery disposition); 3. Live region added. One persistent 4. The zero-step branch is now actually exercised. You were right that the old test could not reach it. And your caution was the useful part: with Minor cleanup taken: the redundant turn-level check inside the step predicate is gone. The sr-only span went away entirely rather than moving to @jackwenerAgreed on all five, and the reasoning lands: native controls in a themed panel miss the type scale, borders and focus ring, and read as a patch of some other app.
One deviation, flagged rather than silently dropped: Verification33 desktop inspector tests, |
Third layer of apache#1625. The RFC framed this as trace search; the shape settled on the issue was narrower and better: structured predicates are `Array.filter` over `TraceStep[]`, so there is no query API on the contract and nothing new on the wire. Four predicates, because they are the questions people actually bring to a trace: free text, step kind, failed-only, and a cost floor. They compose. Three properties the tests pin, all of them the same shape as rules this track has already had to learn: - **An unpriced step is not a cheap step.** A cost floor excludes it rather than treating absent as zero, exactly as the canonical record does. - **A filter that matches nothing is not an empty session.** "Nothing matches" and "this session did nothing" are different claims, and the panel now makes three silences distinguishable: a failed read, an empty filter result, and a genuinely idle session. - **Filtering never restates the bill.** Session totals are left alone; a total recomputed per filter would be a number that is true of nothing. Free text reaches exactly as far as the panel renders — tool names, model ids, turn ids, error messages, recovery dispositions. It deliberately does not reach into event bodies: that is `searchRuntimeEventHistory`'s job, and shipping every tool result into the renderer to imitate it would multiply a payload this panel is already asked to bound, with silent truncation as the only way out. A filter that quietly stops matching is worse than one with a stated reach, so the reach is stated. A turn whose own id matches keeps its place even when no step does, because answering "where is turn-7" with silence is the wrong answer. Replay step-through, the RFC's other PR 3 bullet, is not here. It needs a decision about what "reconstructed state at a boundary" means against the current ledger, and I would rather ask than guess — the last time this issue guessed at a substrate, the answer was that it did not exist. 8 filter tests; 33 desktop inspector tests overall. a11y, dead-css, copy and format checks clean.
…nothing calls Two reviews on apache#2115. **Design review — the filter bar was native controls.** A raw `<input>`, `<input type="checkbox">` and `<button>` sit in a themed panel without its type scale, borders or focus ring, which is how a small region stops looking like the app it lives in. Now `TextInput` (with `hasClear`, the searchable-input idiom), `Switch` and a ghost `Button`. `isLabelHidden` replaces the hand-rolled sr-only span, so this is no longer a third copy of that recipe, and the CSS it needed goes with it. One deviation from the review, stated rather than silently dropped: `TextInput` does not accept `type="search"` — its type union is `'text' | 'password' | 'email'`. `hasClear` is the part that makes it read as a search field, and it is there. **`kinds` and `minCostUsd` had no caller.** The panel can only produce a query and a failed-only toggle, so two of the four advertised predicates were surface with tests guarding nothing. They are gone until the controls that drive them are designed — a predicate nothing emits is exactly the kind of claim this track keeps having to walk back. **The search reach was wider than the rendered surface.** `step.status` is never rendered as text, and the turn's failure message was folded into every step's haystack, so one match on it retained steps that explain nothing about it. Each row is now searched by what it renders; the turn's id is matched against the turn, where it belongs. **Zero-step turns are explicit rather than incidental.** The old "turn id matches" test had steps, so the zero-step branch never ran — and with `failedOnly` and no text query, a turn that failed before recording a step would have been dropped for being empty, which is precisely what that filter is asked to surface. Both paths now have their own test. Also: the three result messages share one persistent `role="status"` region, so they are announced as the reader types rather than mounting silently; and the redundant turn-level check inside the step predicate is gone. 33 desktop inspector tests; a11y, dead-css, copy and format checks clean.
76c374e to
c5bc230
Compare
|
CI triage on the previous run: the two Both are in Checked rather than assumed: I added a throwaway worktree at pristine Rebased onto |
3 participants
Summary
Third layer of the Session Inspector: filtering the timeline. Refs #1625.
The RFC framed this as trace search. The shape settled on the issue was narrower and better — structured predicates are
Array.filteroverTraceStep[]— so there is no query API on the contract, nothing new on the wire, and no main-process work at all. Two predicates, because they are the two the panel can produce: free text and failed-only. Kind and cost filters were written and then removed in review — a predicate no control emits is dead surface with tests that guard nothing, and they come back when the controls do.Two properties worth pinning
A filter that matches nothing is not an empty session. The panel now distinguishes three silences that used to be one: a read that failed, a filter that matches nothing, and a session that genuinely did nothing. Only the last says "nothing to trace".
Filtering never restates the bill. Session totals are left alone. A total recomputed per filter would be a number that is true of nothing — and this surface has already been careful not to become a second cost authority.
Where free text reaches, and why it stops there
Exactly as far as each row renders: the step kind, its label, its detail, its recovery disposition — plus the turn's own id, matched against the turn rather than folded into every step.
It deliberately does not reach into event bodies. That is
searchRuntimeEventHistory's job, and imitating it here would mean shipping every tool result into the renderer — multiplying a payload this panel is already asked to bound, with silent truncation as the only way out. A filter that quietly stops matching is worse than one with a stated reach, so the reach is stated rather than approximated.One asymmetry on purpose: a turn whose own id matches keeps its place even when no step does, because answering "where is turn-7" with silence is the wrong answer.
Not here: replay step-through
The RFC's other PR 3 bullet is missing on purpose. "Scrub the timeline to inspect reconstructed state at each boundary" needs a decision about what reconstructed state means against the ledger as it stands now, and what a read-only surface may claim about it. I would rather ask than guess — the last time this issue guessed at a substrate, the finding was that the substrate did not exist, and the answer was the accounting refactor in #1679. Happy to take it as its own change once the shape is settled.
Verification
8 filter tests (pure, no DOM), 33 desktop inspector tests overall. The filter bar uses Astryx
TextInput/Switch/Buttonper the design review.check-a11y,check-dead-css,check-copyandformat:checkclean; renderer and main typechecks clean.🤖 Generated with Claude Code