Skip to content

internal/driver: make profile comments easier to discover - #1033

Open
jabrailkhalil wants to merge 8 commits into
google:mainfrom
jabrailkhalil:feat/profile-comments-info
Open

jabrailkhalil wants to merge 8 commits into
google:mainfrom
jabrailkhalil:feat/profile-comments-info

Conversation

@jabrailkhalil

@jabrailkhalil jabrailkhalil commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Make profile information easier to discover through the existing details panel. Use a native button with an information icon and expanded-state semantics; profile comments remain in the panel. Space and Enter activate it only when focused.

Current-main integration

Current head: 6927b33b8552ecd3efb68d5e91c99b838d77ed31; integrated main 1facf8f91b50fbab03aa6cb249e0397d91449f9f by a normal, conflict-free merge. All PR implementation/test files are byte-identical to the previous published head. The net PR remains limited to 5 file(s).

Validation

Fresh Linux/amd64, Go 1.26.7 and Chrome 156 validation passes the complete unmodified bash test.sh: both modules, browser tests, race/coverage, vet, formatting, CI-pinned staticcheck commit 452d5bb86b45 and golangci-lint v2.14.0. Separate uncached race-test JSON passes 537 root tests/subtests (4 existing platform skips) and 4 browser tests/subtests, with no browser skips.

The added profile-information keyboard case executed in Chrome. This is fresh Linux evidence; macOS and Windows were not rerun.

These local checks are separate from current-head upstream CI authorization and maintainer review.

Signed-off-by: Jabrail Khalil <jabrailkhalil@gmail.com>
@aalexand

aalexand commented Oct 1, 2026

Copy link
Copy Markdown
Collaborator

No screenshots?

@jabrailkhalil

Copy link
Copy Markdown
Contributor Author

@aalexand Added the screenshots below and to the PR description. These are captures from actual Chrome execution of the baseline and this PR, using the same example profile. The comments remain available through the existing popover; the change makes that control easier to discover.

Before After
Before: program-name link After: information button
Before: details open After: details open

The narrower 800px layout: before, after.

@jabrailkhalil

Copy link
Copy Markdown
Contributor Author

I added the before/after screenshots above and to the PR description, including the narrow viewport. I also updated this branch from main. On current head 16683c0561ee, both the root and browsertests modules pass go test ./... -count=1 on Linux with Go 1.26.7. The current upstream CI/security workflows are waiting for maintainer approval; could you approve those runs when reviewing this revision?

Comment thread browsertests/browser_test.go Outdated
Comment thread browsertests/browser_test.go Outdated
Comment thread browsertests/browser_test.go Outdated
@aalexand

aalexand commented Oct 4, 2026

Copy link
Copy Markdown
Collaborator

@jabrailkhalil The PR seems too verbose for the goal they are achieving. I appreciate the power of AI and welcome pprof contributions, but to make this good use of time, please review the PRs carefully before submitting them.

@jabrailkhalil

Copy link
Copy Markdown
Contributor Author

Agreed, the tests were too much for this change. I've reduced them to one browser test and removed the markup checks.

@jabrailkhalil

Copy link
Copy Markdown
Contributor Author

Integrated current main 7bae8d8c in e7bdf6ce to clear BEHIND without adding feature scope.

The driver package tests and scoped go vet pass. I adapted only the added browser regression to main’s chromedp v0.20.1 typed-selector/Do API: it failed compilation before this adaptation and now compiles. Button focus, Space/Enter, visibility and comment assertions remain unchanged.

Fresh validation uses Windows Go 1.26.0, inherited from current main. The full Windows suite is not green: the three PE/DWARF mapping subcases also fail on clean current main in the same environment. The initial run additionally lacked the external diff executable; it is not treated as a source regression. Browser modules retain their x86-64 Linux guards and skip execution on Windows: compilation is not a new Linux/browser/race validation. No existing assertions, timing thresholds, platform guards or dependency versions were weakened. Current-head upstream CI and maintainer review are still needed.

Could a maintainer approve the new-head workflow and review the integration?

@jabrailkhalil

Copy link
Copy Markdown
Contributor Author

Validated unchanged head e7bdf6ce1f9d997c92a030351a9eb5239597e201 against main 7bae8d8c4c9ebc3886a79f796fbde65b7e1316f8 in an isolated author-fork Linux run. Ubuntu 24.04 / Go 1.26.8 / Chrome 154, with Graphviz and LLVM available. The complete, unmodified test.sh passed, including both modules, race/coverage, staticcheck 2026.2.1, the upstream golangci-lint v2.14.0 commands, vet and formatting. Fresh go test -race -count=1 -json ./... also passed in each module; no browser test was skipped. The four pre-existing root test/subtest platform skips are retained on both head and main.

The profile-details browser regression actually executes and passes, including mouse activation and focused Space/Enter behavior. With only that browser test applied to main, it fails at the original link's Space-toggle wait (the existing 90-second deadline), rather than at compilation or Chrome setup.

Main passed the same Linux commands. Module manifests, checksums and test.sh are byte-identical between the compared trees. The diagnostic workflow exists only on a separate branch in my fork; this PR head, assertions, deadlines and upstream workflow were not changed. This is author validation on one Linux/Go version, not the full upstream OS/Go matrix or a replacement for required Google CI/review; the earlier Windows failures below are not relabeled as passing.

Could a maintainer authorize the upstream workflow and review the current-head integration?

@jabrailkhalil

jabrailkhalil commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor Author

I integrated main 1facf8f9 in 6927b33b without conflicts. Fresh Linux/amd64, Go 1.26.7 and Chrome 156 validation passes the complete unmodified bash test.sh: both modules, browser tests, race/coverage, vet, formatting, CI-pinned staticcheck commit 452d5bb86b45 and golangci-lint v2.14.0. Separate uncached race-test JSON passes 537 root tests/subtests (4 existing platform skips) and 4 browser tests/subtests, with no browser skips. Upstream CI and review remain separate.

Could a maintainer approve the pending workflow runs for this head?

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.

2 participants