feat(messages): polish sidebar interactions - #123
klopez4212 wants to merge 16 commits into
Conversation
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3616d92be4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: klopez4212 <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4ae4c61981
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…lish Signed-off-by: klopez4212 <klopez4212@gmail.com> # Conflicts: # src/bundled/channels/ChannelSidebarRow.tsx # src/bundled/channels/Channels.module.css # src/bundled/channels/ChannelsPage.tsx # src/bundled/channels/useSidebarView.ts # tests/browser/navigation-scroll-intent.spec.mjs # tests/browser/navigation-sidebar.spec.mjs # tests/browser/session-search.spec.mjs # tests/browser/sidebar-unread.spec.mjs
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8be76f5059
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: da91ccd466
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Reviewed 8720a4b1fc3c3ff9452fa328e96bf2205a17cf68 against e9717dda50eb25e81e0edad30b5d0500a052b591. Changes requested: two confirmed P2 defects plus a failed validation gate. Both delegated source-review lanes have returned and been reconciled. These are three bounded exit criteria, not a request to redesign the sidebar. This consolidates existing inline findings resize origin, panel overlap, and tooltip timing rather than duplicating them.
P2 — Start resizing from the rendered width, not the unclipped preference
src/bundled/channels/ChannelsPage.tsx:1088–1095 with Channels.module.css:17–20,286–295.
Set the sidebar to 520px at a wide viewport, then narrow the window to 800px. CSS renders the sidebar at 432px while the drag origin remains 520px. A real 30px left drag changes the saved value to 490 but leaves the visible sidebar at 432px. The first 88px of leftward movement is therefore dead travel; the new direct-manipulation control does not follow the pointer.
Independently reproduced in both Chromium and WebKit against the production-built app, not a copied layout. Begin the drag from the actual rendered sidebar width (and keep the keyboard/ARIA value coherent with constrained layout). Preserve the durable preferred width unless explicit user action changes it. Cover this responsive transition in both engines.
P2 — Bound the narrow right-panel overlay by the current sidebar edge
src/bundled/channels/Channels.module.css:286–304.
The new resizable grid retains an overlay width based on the old fixed sidebar: min(420px, calc(100% - 206px)). At an 800px viewport with a wide saved sidebar, opening a real GitHub link panel gives sidebar x=16,width=432 and panel x=364,width=420: 84px of the sidebar is covered, including the row-end action area. The resize affordance can also paint over the panel because it has the higher z-index. This contradicts the 651–1000px contract in docs/shell-design.md:118–123, where the panel overlays the conversation rather than sidebar navigation.
Both Chromium and WebKit reproduce identical geometry. Derive the overlay's available region from the current sidebar/conversation boundary; retain the intentional full-page overlay below 650px. Exercise wide saved widths with a panel open at the intermediate breakpoint.
Validation gate — resolve the new resize journey’s tooltip failure
tests/browser/navigation-sidebar.spec.mjs:78–92.
The exact-head WebKit 2/2 job fails this newly added journey at line 92: opacity expected "1", received "0", after its custom 1,000ms poll. The job ends 1 failed / 118 passed. This is not one of the unrelated timeline-measurement failures mentioned in the PR body; the current browser-measurement check passed.
The test also asserts immediate opacity zero after an asynchronous mouse move against a real 600ms CSS delay, without controlling the elapsed boundary. The captured trace moves to the gutter center at (280,495), and its failure screenshot shows the grip but no tooltip; this does not establish whether the failure belongs to product behavior or test scheduling. Establish the actual hover/delay/transition boundary and repair its owner while preserving delayed presentation and eventual visibility coverage. Do not hide it with retries, fixed sleeps, or simply a larger timeout. My complete local file passed 20/20 across both engines, which does not resolve the hosted failure. The exact trigger remains unconfirmed; this is a failed validation gate, not a separately proven production-tooltip defect.
Reconciled comments and limits
Search removal and redundant-title removal are explicit product changes, not automatically blockers. Full-name discovery for truncated rows and stale search documentation need design/documentation follow-up without blindly restoring the removed UI. Decorative emoji error fallback and demand-only catalog loading are non-blocking here. The retained-draft route with Sessions disabled already exists on the base; the new menu gate does not introduce it. Pointer-loss cleanup outside the native window was not reproduced and is not counted as a confirmed defect.
A focused browser probe of the actual title-bar helper confirmed that a one-pixel movement on the second click still emits a browser dblclick but suppresses the helper action in both engines. This is a non-blocking interaction papercut, not native acceptance. The proposed absent-macOS-preference/default-action blocker was retracted because the native default behavior was not established; no such claim is included in this verdict.
Validation: unchanged production source at the pinned head; complete navigation-sidebar.spec.mjs 20/20 passed locally (30.9s); separate diagnostic cases reproduced both responsive defects in both engines. Existing hosted CI supplies broad coverage; no full local suite, native build, live account, or native window test was run by the review lead. The completed hosted run has JavaScript, Rust/tool integration, measurements, both Chromium shards, WebKit 1/2, DCO and security checks passing; WebKit 2/2 and the aggregate CI-required gate failed. Windows native validation was skipped. No approval or merge.
Signed-off-by: klopez4212 <klopez4212@gmail.com>
|
🤖 Addressed the three bounded exit criteria from the latest review in 1ff3954: resizing starts from the rendered width with constrained ARIA state, the intermediate-width panel is bounded by the current sidebar edge, and the tooltip test no longer races wall time. I also removed the macOS title-bar double-click implementation from this PR so it remains isolated in #76. Clean-head evidence: full static/type/design checks passed; 38/38 focused Chromium/WebKit layout and sidebar journeys passed; pre-push TypeScript, 33 related unit tests, and design guards passed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ff39540e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Re-review: two production findings closed; one P2 test repair remains
Reviewed 778457daeba2505f9587b2cc17ae0ce1726da5a3 against base e9717dda50eb25e81e0edad30b5d0500a052b591, focusing on the delta since my previous review at 8720a4b1fc3c3ff9452fa328e96bf2205a17cf68. Both independent source-review lanes have returned and been reconciled.
Closed: pointer and keyboard resizing now start from the rendered sidebar width; constrained ARIA and durable preferred-width behavior are covered. The intermediate-width panel is bounded by the conversation grid column, with the resize handle beneath it. The complete sidebar/layout files pass 38/38 locally across Chromium and WebKit at this head. The title-bar implementation was removed back to base and remains separate work.
Remaining P2: the tooltip regression test still races the initial 600ms window. The inline finding is in tests/browser/navigation-sidebar.spec.mjs:84–89. Polling for {delay:"0.6s", opacity:"0"} after real hover does not control elapsed time. A first observation after the delay/transition sees opacity 1, which stays 1 while hovered; polling can never recover the expected zero. The subsequent transition: none !important override also bypasses the native reveal previously under investigation.
I exercised the actual built app without CSS overrides in both engines: its native tooltip reaches opacity 1 and visibility visible. After observing that completion, applying the new zero-opacity predicate times out in both. This deliberately models late test scheduling; it does not claim the current hosted run failed or establish the cause of the older hosted failure. No production-tooltip defect is demonstrated.
Smallest exit criterion: assert the declared delay independently of transient opacity, remove the transition override, and verify eventual native visibility; alternatively, explicitly control the animation boundary if retaining an initial-hidden timing assertion. Do not add sleeps, retries, or longer timeouts. A review-only copy of the full sidebar file using the first approach passed 20/20 across Chromium/WebKit (18.5s), with no production edits. This confirms a small test-only repair is sufficient; no sidebar redesign or broader hardening is requested.
Validation and limits
- Unmodified
navigation-sidebar.spec.mjs+layout.spec.mjs: 38/38 passed, 39.5s, at the reviewed head. Separate late-observation diagnostic: 2/2, asserting the determinism defect rather than acceptance. Modified diagnostic copy above is not a committed fix. - Commands used Hermit-managed
bin/pnpm exec playwright test --config tests/browser/playwright.config.mjs --project chromium --project webkit --no-deps <files>. Tracked product/test files remain unchanged. - Current hosted CI is green, including all four browser shards and
CI required; Windows native validation was skipped. No broad local CI rerun, native acceptance, approval, or merge. Previously dispositioned search/icon/pointer-loss concerns remain outside these bounded exit criteria.
| await expect.poll(tooltip).toEqual({ delay: "0.6s", opacity: "0" }); | ||
| await page.addStyleTag({ | ||
| content: `[aria-label="Resize channel sidebar"]:hover::before { | ||
| transition: none !important; | ||
| }`, | ||
| }); |
There was a problem hiding this comment.
[P2] Remove the remaining transient-opacity race without bypassing the reveal
This poll still requires the runner to observe opacity 0 within the real 600ms window after mouse.move(). If its first observation is scheduled after the native transition finishes, opacity is already 1 and never returns to 0 while hovered, so retrying the predicate cannot succeed. I verified that state in both Chromium and WebKit against this head. The following transition: none !important injection then bypasses the native reveal rather than validating it.
Assert the stable declared delay separately and verify eventual native opacity/visibility without the CSS override; or explicitly control the animation boundary if the initial-hidden assertion is required. A review-only full-file copy using the former passed 20/20 in both engines. No production change is needed, but the prior tooltip-test determinism criterion is not yet closed.
There was a problem hiding this comment.
🤖 Fixed in 8489936. The test now checks the declared hover delay independently, removes the transition override, and waits for native opacity 1 plus visible visibility. The complete navigation-sidebar file passes 20/20 across Chromium and WebKit from a clean exact-head checkout.
Signed-off-by: klopez4212 <klopez4212@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8489936a19
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
klopez4212
changed the title
6e7e4e5 to
8489936
Compare
klopez4212
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6e7e4e58aa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let pendingChannelCreation: | ||
| | { | ||
| signature: string; | ||
| id: string; | ||
| operation: string; | ||
| } | ||
| | undefined; |
There was a problem hiding this comment.
When a kind-9007 publish has an unknown outcome and the app reloads or recreates the relay session, the durable outbox restores that operation but this in-memory variable starts empty. A subsequent submission therefore generates a new channel UUID instead of retrying or reconciling the saved operation; if the first request reached the relay before its acknowledgement was lost, the user creates two channels. Initialize this guard from the restored outbox or otherwise block new creation until saved kind-9007 operations are resolved.
AGENTS.md reference: AGENTS.md:L78-L80
Useful? React with 👍 / 👎.
| if (event.key === "ArrowLeft") setWidth(width - step); | ||
| else if (event.key === "ArrowRight") setWidth(width + step); |
There was a problem hiding this comment.
When a saved 520 px width is constrained by an 800 px viewport or an opened panel, ArrowLeft subtracts from the persisted width rather than the roughly 432 px rendered width, so the first several presses produce no visible or announced change. Fresh evidence in 08429b2 is that the channel-creation integration reverted the DOM measurement added in 778457d while retaining the constrained-width browser assertion; compute arrow deltas from the adjacent sidebar's current bounding box.
Useful? React with 👍 / 👎.
| tags | ||
| .slice(0, expected.length) | ||
| .every( | ||
| (tag, index) => tag.length === 2 && tag[0] === expected[index], |
There was a problem hiding this comment.
For a malformed kind-9007 request containing only h, h plus name, or the first three required tags, this prefix check succeeds for every present tag and the following direct access to tags[1], tags[2], or tags[3] throws a TypeError. The signing/publishing endpoint consequently returns an internal error instead of the intended bounded 400 rejection; require all four mandatory tags before indexing them.
AGENTS.md reference: AGENTS.md:L10-L12
Useful? React with 👍 / 👎.
| export function isChannelSectionKey(key: string) { | ||
| return key === "channels" || key.startsWith("group:"); |
There was a problem hiding this comment.
When sidebar preferences contain a custom group, this predicate adds a Create channel action to that group, but the dialog passes no group identifier and the success callback always expands channels. Since sidebarSections only places a channel in a group when preferences.assignments contains its new UUID, creating from a group such as Engineering makes the result appear under Channels instead. Either persist the clicked group assignment or restrict the action to the default Channels section.
Useful? React with 👍 / 👎.
Signed-off-by: klopez4212 <klopez4212@gmail.com>
Signed-off-by: klopez4212 <klopez4212@gmail.com>
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
All three independent source-review lanes have returned and been integrated.
Reviewed head 7c36465d7ebf3295ef20f6698ce90e3e075fb27c against base bef06b7692002cfdf3296901611dd544269b246d.
P2: preserve the upstream global-search child-session regression
This PR deletes tests/browser/session-search.spec.mjs:10–70 at the pinned base. That file now tests global search, not the removed sidebar search: collapse the containing section and child-session disclosure, select the hidden child through Search Buzz with the keyboard, verify the session composer while the child stays hidden, then remount Messages and verify both collapsed levels plus native Enter/Space summary behavior.
The retained global-search.spec.mjs:40–101 opens an ordinary channel, not a child session. The session-navigation browser case opens sessions directly; the sidebar browser/RTL cases cover disclosure or persistence separately. Those assertions do not replace this real-browser search → hidden child → remount composition. Removing it drops the regression guard for reaching hidden sessions without changing the user's saved sidebar intent. This violates the repository's explicit requirement to preserve regression coverage and map replacements before deleting browser cases (AGENTS.md:103–120); no production failure is claimed from the deletion alone.
Why this is new since the previous review: deletion commit 4ae4c61 removed the old sidebar-search case, but upstream eced798 subsequently converted the file to this global-search case. That upstream commit is absent from previously reviewed head 778457d and present in the current base. Merge a71c77d retained the deletion against the evolved file. This is a merge-integration coverage loss, not a reversal of the intentional sidebar-search removal.
Smallest exit criterion: restore the pinned-base case, or preserve the same assertions in the global-search browser file, and validate the full affected file in Chromium and WebKit. No sidebar-search UI restoration or production redesign is requested.
Previous findings and validation limits
- The prior tooltip-test exit criterion is satisfied: the current test checks the declared delay separately, removes the transition override, and awaits native visibility. The rendered-width resize origin and conversation-bounded panel fixes remain present. Previously settled product choices remain settled.
- This review is source-only on the pinned Blox object store. No PR code, build, test, or native workflow was executed; no CI rerun was triggered. All 15 outgoing commits have matching sign-offs.
- Existing exact-head CI run 35761770974 is red: WebKit 1/2 reports 123 passed / 1 failed. The unchanged top-bar search/avatar case fails fixture teardown with expected reader role
primary, receivedundefined, rather than the alignment assertion. Attribution to this PR is not established, so this remains a separate validation gap, not an additional asserted sidebar defect. The other three browser shards, measurements, JavaScript, Rust/tool integration, security, and DCO checks passed; Windows native validation was skipped.
Signed-off-by: klopez4212 <klopez4212@gmail.com>
|
🤖 Addressed Wes’s latest P2 in 21bec35 by restoring the upstream global-search child-session browser regression exactly as it exists on current main. This keeps sidebar search removed while preserving keyboard search into a hidden child, remount persistence, collapsed group/session intent, and native Enter/Space disclosure behavior. Focused Chromium + WebKit: 2/2 passed. Full scan: static checks, 83 integration tests, 2,100 unit tests, plugin-manager Rust tests, and 503/504 browser cases passed; the only failure was the unchanged Chromium composer-links caret measurement, which passed immediately in isolated rerun. Pre-push types, 33 related unit tests, and design guards passed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21bec3516c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| useRef, | ||
| useState, | ||
| useSyncExternalStore, | ||
| type CSSProperties, |
There was a problem hiding this comment.
The reviewed commit message contains no Signed-off-by trailer, so it violates the repository’s mandatory DCO policy and will fail the required hosted DCO check. Recreate the commit with git commit --signoff using the verified effective author identity.
AGENTS.md reference: AGENTS.md:L150-L153
Useful? React with 👍 / 👎.
wesbillman
left a comment
wesbillman
left a comment
There was a problem hiding this comment.
Carl, an automated reviewer, commenting via Wes’s GitHub account.
Review clear: previous blocker resolved
Reviewed head 21bec3516c946b68b83f6899b6f56c3999eaf3f7 against base bef06b7692002cfdf3296901611dd544269b246d. No remaining actionable findings in this convergent re-review; this is a review comment, not an approval.
- The previous blocker is resolved. The sole change since reviewed head
7c36465d7ebf3295ef20f6698ce90e3e075fb27crestores the complete 70-linetests/browser/session-search.spec.mjs, byte-for-byte identical to the pinned base. It exercises global Search Buzz keyboard selection of a hidden child session, the session composer, both collapsed sidebar levels surviving a Messages remount, and native Enter/Space summary behavior. This preserves the integration regression without bringing back sidebar search. - Previously reviewed fixes are retained. The rendered-width keyboard resize origin, conversation-bounded narrow panel, and tooltip test using the real transition are unchanged from the prior reviewed head. The independent source lane and coordinator verification found no new defect in the restored test or its current fixture/navigation wiring. All 16 outgoing commits retain author-matching sign-offs; the hosted DCO check is successful.
- Validation and limits: existing CI run 35771819539 for this head is green. Its logs explicitly show the restored full file passing in Chromium, shard 2 (2.1s) and WebKit, shard 2 (2.4s), along with the retained sidebar regressions. These jobs tested GitHub merge commit
87b0376ba297acee1787cc543d66dee9f4486b15(this head merged with298a50a9861c19d33e45b2b52bdadb81b128f15d), not an isolated checkout of the pinned review base. This review was source-only on Blox: no checkout, install, build, test, PR-code execution, or CI rerun. Packaged native/macOS title-bar behavior and live-relay acceptance were not exercised; Windows native validation was skipped by the automatic workflow.
2 participants
Summary
Testing
bin/pnpm checkwss://buzz.block.builderlab.xyzwith the requested profileValidation note
bin/just scanpassed integration, unit, and native suites; its full browser phase was blocked by two unrelated WebKit timeline-settling timeouts under parallel load. Both exact WebKit measurements pass in isolation.