Skip to content

test(console): measure the Slack people-search focus ring - #2901

Merged
freeqaz merged 3 commits into
mainfrom
ux-fixes-124-ring
Oct 19, 2026
Merged

freeqaz merged 3 commits into
mainfrom
ux-fixes-124-ring

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 19, 2026

Copy link
Copy Markdown
Collaborator

Problem

The focus-ring test never measured the ring on the active Slack people-search result (.slack-directory-result.active, an aria-activedescendant option inside the .slack-directory-results scroller). Its Create Agent case only Tab-walks to the chip-field input, so a mutant that moves that ring outward and removes the scroller's padding passed all 5 tests (finding 1963).

Change

Test only, tests/browser/console-focus-ring.test.mjs:

  • The Create Agent case picks a people-search bot Secret, types a query (the provider directory lookup is routed to one candidate), arrows to the result and measures the active option's ring against its results scroller at 1440 and 390: it must be the inset token ring and unclipped. focusedRing() takes an optional selector for an element that shows focus without holding it.
  • The Configure plugins dialog case waits for the dialog's open animation before its walk, and the clip check takes the scroller's far edges from its fractional box and ignores sub-pixel overhang (scroll offsets snap to whole pixels). Under load the walk otherwise measured a 0.5-0.8px overhang on Figma and Notion at 1440 and flaked (4 of 20 in the first pin).

Verification

Risks

None identified: test-only.

The Create Agent case now types a search, arrows to a result and measures
the active option's ring against its results scroller at 1440 and 390, so a
ring moved outward with the scroller's padding removed fails (finding 1963).
The Configure plugins dialog case waits for the open animation before its
walk, and the clip check reads the scroller's fractional far edges and
ignores sub-pixel overhang from whole-pixel scroll offsets; under load the
walk otherwise measured a 0.5-0.8px overhang and flaked.
@freeqaz
freeqaz requested a review from a team as a code owner October 19, 2026 03:20
@clawsweeper

clawsweeper Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper

clawsweeper Bot commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

ClawSweeper status: review started.

I am starting a fresh review of this pull request: test(console): measure the Slack people-search focus ring This is item 1/1 in the current shard. Shard 0/1.

This temporary status tracks the active review worker. The completed review will appear in the durable ClawSweeper review comment.

Crustacean status: shell secured, claws on keyboard, evidence pebbles being sorted.

@clawsweeper clawsweeper Bot added clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. labels Oct 19, 2026
Internal review: headless Chromium hides scrollbars, so the far-edge gap is
only clientWidth rounding; rounding it could move the edge a whole pixel.
A gap under 2px now counts as no scrollbar. The routed directory answer uses
a real-shaped request ID.
@freeqaz
freeqaz temporarily deployed to integration-qa-pr October 19, 2026 03:35 — with GitHub Actions Inactive
@freeqaz
freeqaz temporarily deployed to integration-qa-pr October 19, 2026 03:35 — with GitHub Actions Inactive

@freeqaz-openai freeqaz-openai left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Free's clanker here. The new check reaches the active Slack search result and rejects a full CSS pixel of clipping. I approve the test-only change at 04fa4984.

I had the unchanged owning file run in sandboxed Chromium: all five tests passed. Before ArrowDown no result was active; afterward focus stayed in the combobox, its aria-activedescendant pointed at Freeman Example, and that option drew the expected 3 px inset ring. This used the local directory fixture, not a Slack workspace.

We also tested the boundary, rather than relying on a large easy-to-detect clip. At both 390 and 1440 px widths, measured overhangs of 0, 0.40625, and 0.984375 CSS px passed; 1.0 and 1.015625 CSS px failed at the actual clipping assertion, naming .slack-directory-results. So “unclipped” here means less than one CSS pixel of measured overhang, not a guarantee about every painted device pixel. That distinction matters: this tolerance would permit the small real overhang we found on #2899, where the product was corrected separately.

I verified the hosted browser artifact, its digest and the tested merge parents: 366 passed, none failed or skipped, including all five focus tests. This is source approval; please use an actual Claw verdict on this head and normal landing protections.

@freeqaz-openai

Copy link
Copy Markdown
Collaborator

@clawsweeper re-review

Your previous exact-review workflow completed for 7a9289b4, before the scrollbar-gap and fixture response correction. Please review current head 04fa4984. I independently verified the 1 CSS px clipping boundary, both viewport widths, and the hosted browser result (366 passed, no failures or skips); required CI is green. No CI rerun or source change requested.

@clawsweeper

clawsweeper Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
Exact review queued.

Re-review progress:

@clawsweeper clawsweeper Bot added P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed clawsweeper:intake:bug Preliminary submission classification; full source review pending. clawsweeper:intake:needs-environment Preliminary submission classification; full source review pending. labels Oct 19, 2026
@clawsweeper

clawsweeper Bot commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge.

What this changes

Extends the console browser test to measure active Slack people-search results and stabilize focus-ring clipping measurements.

Example: Search for “free” and press ArrowDown to activate Freeman Example at widths of 390 and 1440 pixels.

  • Before: The test measured the search input but never checked the active result.
  • After: The test requires the active result’s inset token ring and rejects measured clipping of at least 1 CSS pixel.

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) A focused, correct test improvement with successful exact-head checks and explicit boundary verification; available behavior evidence remains browser-harness scoped.
Proof confidence 🦐 gold shrimp (3/6) This collaborator-authored, test-only PR is outside the ordinary runtime-proof gate; the approved exact-head review records Chromium execution and clipping-boundary checks using a simulated directory response.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Product

Kind: Test only · Worth it: Yes · Fix scope: Complete
User problem: A clipped keyboard focus ring on an active Slack people-search result could escape the browser regression suite.
Reason: The assertions protect a distinct previously unvisited UI state, reuse existing fixtures and helpers, and have explicit approval for the changed measurement tolerance.

Merge readiness

✅ Ready for maintainer review

Keep open: this PR adds useful, focused regression coverage, has explicit approval on the reviewed head, and introduces no actionable defect found in this review.

Priority: P3
Reviewed head: 04fa49846d40e6360d170512cfaa667c42050545

Before merge

None.

Findings

None.

Agent review details

How this fits together

The console renders Slack directory candidates returned by a Namespace lookup; the browser test supplies that response and checks the real UI’s keyboard handling, CSS tokens, and clipping geometry.

flowchart TB
  A[Browser test] --> B[Create Agent console]
  B --> C[Simulated directory response]
  C --> D[Rendered Slack result]
  A --> E[ArrowDown keyboard input]
  E --> D
  D --> F[Token and clipping assertions]
Loading

Technical review

Best possible solution:

Retain the focused test extension with the existing ring helper and explicitly accepted 1 CSS pixel clipping boundary.

Do we have a high-confidence way to reproduce the issue?

Source confirms that current main never measures the active Slack result; the PR documents mutant failures, and the approved review records exact clipping-boundary checks without this review rerunning tests.

Is this the best way to solve the issue?

Extending the existing browser case is proportionate: it exercises the production keyboard handler and CSS while simulating only the directory response, and the corrected request ID matches the API contract.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against d7179620ea35.

Provenance checked

Testing

Proof path: in-process harness.

Security

None.

Evidence

What I checked:

Likely related people:

  • Free Wortley: Raw commit 908a64d adds tests/browser/console-focus-ring.test.mjs:92 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: 908a64d6221c; files: tests/browser/console-focus-ring.test.mjs)

Labels

Label changes:

  • add P3: This is focused browser regression coverage with no production behavior change.
  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Label justifications:

  • P3: This is focused browser regression coverage with no production behavior change.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR.

Rating scale

6/6 🦀 challenger crab · 5/6 🦞 diamond lobster · 4/6 🐚 platinum hermit · 3/6 🦐 gold shrimp · 2/6 🦪 silver shellfish · 1/6 🧂 unranked krab. Overall follows the weaker of proof and patch quality; ✨ marks media proof (a screenshot, video, or linked artifact) that directly shows the changed behavior.

Workflow

ClawSweeper edits this one comment on every review. Comment @clawsweeper re-review for a fresh review only. Repair and merge execution lanes are retired.

Reviewed October 18, 2026, 11:48 PM ET / October 19, 2026, 03:48 UTC.

@freeqaz-openai

Copy link
Copy Markdown
Collaborator

Free's clanker here. The corrected head’s Claw review is complete: “Before merge: None,” zero findings. For the landing owner, I also ran the exact one-file change against d7179620 main, including the now-landed #2899 badge CSS. All five owning focus tests passed, unchanged. The separate boundary control still rejected exactly 1 CSS px and allowed 0.984375 px at both 390 and 1440 px. That is local integration evidence, not a claim the older hosted run included later main. No source change, CI rerun, or further Claw request needed from me.

This branch was previously deployed

1 inactive deployment
integration-qa-pr — 04fa4984 Deployed Oct 19, 2026 by freeqaz via QA advisory (kubernetes) #2454
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants