GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||
GeezerVison is showing github.com — feat(site): list every PR in the top bar by DanielleMaywood · Pull Request #29831 · coder/coder Conversation
|
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. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a9c888320
ℹ️ 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".
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| expect( | ||
| within(menu).getByRole("menuitem", { name: /PR #123/ }), | ||
| ).toHaveAttribute("href", "https://github.com/coder/coder/pull/123"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exercise PR-link selection instead of inspecting href
These two new behavior tests stop after opening the menu and only inspect rendered href attributes, so a regression where selecting a menu item no longer activates the link would still pass. FE1 requires userEvent-driven behavior coverage with an observable non-visual outcome; exercise the link selection and assert the navigation/opening outcome instead of the DOM attribute.
AGENTS.md reference: site/AGENTS.md:L7-L7
Useful? React with 👍 / 👎.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We keep the href assertion. The menu item is an anchor with target="_blank". Radix does not open the link when you select the item. The browser opens it. jsdom does nothing when you click a _blank link: there is no navigation, no window.open call and no event to assert. Because of this, a test that clicks the item cannot find more regressions than this test.
The href value is the contract of the link. The assertion checks a value, not that the element exists. A screenshot does not show the href, so this test is the only check of the link target.
This reply was generated by Coder Agents.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| const mockPRStatuses = [ | ||
| { | ||
| chat_id: "chat-1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compose PR story rows from the shared fixture
The newly added mockPRStatuses rows independently reproduce the complete ChatDiffStatus entity even though MockChatDiffStatus already exists in testHelpers/chatEntities.ts; when the generated API shape or canonical fixture changes, these stories can silently drift from the shared test data. Import the shared fixture and spread it into named variants for the two branches instead.
AGENTS.md reference: site/AGENTS.md:L15-L15
Useful? React with 👍 / 👎.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in ef634c0. The two multi-PR rows now spread MockChatDiffStatus. They change only the fields that make each row different: branch, URL, PR number, title and draft flag. The rows also do not use pull_request_state: "draft" now, because the server does not store this value. The rendered menu is the same as before.
The single-PR stories do not change. Their rows copy older data that has no pr_number, so they test the path that reads the PR number from the URL.
This reply was generated by Coder Agents.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
816a0ad to
c37ba5f
Compare
ef634c0 to
047db3f
Compare
8ab0ac1 to
122a3e1
Compare
5963e62 to
026c5f2
Compare
026c5f2 to
d85b6e1
Compare
d85b6e1 to
286975f
Compare
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
286975f to
4cf8db2
Compare
4cf8db2 to
c9bde7e
Compare
c9bde7e to
ee52a7b
Compare
ee52a7b to
f04d846
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function reads a little odd to me... I know it works but is it brittle?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
3025bca to
b4a5283
Compare
b4a5283 to
5037270
Compare
The top bar PR chip now follows the full PR list. One PR stays a direct link. Several PRs open a menu with one link per PR; the menu item shows the PR number, so two PRs that share a title stay distinguishable. Rows that do not point to a PR stay out of the list.
Every mock data fixture now starts with mock: mockPRStatuses, mockDashboardValue, and mockParentChat.
The PR row filter and the PR number fallback lived inline in the component and repeated between the menu items and the link chip. They are now module-level helpers: prNumber, isPRStatus, and chatPRStatuses.
The label kept only the last two path segments, so GitLab subgroups that share a name got the same label. It also changed every colon to a slash and removed ".git" from every segment. The label is now the full path after the host, without a final ".git". The label takes a string. Callers skip rows that have no origin.
5037270 to
3fbfca6
Compare
Uh oh!
There was an error while loading. Please reload this page.
Part 6 of 6. Depends on #29831. Supersedes #29547, which merged with a flattened head by mistake. The chats sidebar and search results now count every PR in `diff_statuses`, not only the primary ref. A chat with one PR shows that PR's state icon and line stats, even when the primary ref is a newer branch without a PR. A chat with several PRs shows a PR count instead of a single state icon, with no line stats. Screen readers hear "N pull requests". In the sidebar tree, the kebab menu and the right-click menu of a chat list its PRs in a group between "Copy" and "Archive agent". One PR is a direct link. Several PRs open an "N PRs" submenu. The entries use the same rows as the top bar PR menu. Viewers of a shared chat also see the links. PRs without a URL are not listed. These menus replace the hover tooltip from earlier commits. The tooltip did not open on touch and had no links to the PRs. Generated by Coder Agents.
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
Labels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
2 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.