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): drag chats onto project folders in the sidebar by ibetitsmike · Pull Request #30795 · coder/coder Conversation
|
|
@codex review |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
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. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Here are some automated review suggestions for this pull request.
Reviewed commit: 649a0ad9b7
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
…ed chat in the cache, not the DOM FE1 leaves rendered presence to screenshots. The row menu test now checks the move request and the project page's own list cache, and the new ChatMovedToAnotherProject story shows that the page leaves out a cached chat of another project.
|
@codex review |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Here are some automated review suggestions for this pull request.
Reviewed commit: fd48f30247
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
… its project A chat moved out of its project, or whose project was deleted, kept the memory index the model saw there, while the memory tools were gone. A root chat outside a project now has an empty index, so its next turn start sends an update that removes those memories. Such a chat reads the prompt only at turn start, the only step where that removal can be due.
Object.groupBy needs Chrome 117 and Safari 17.4, and the browserslist supports Chrome 111 and Safari 16.4 with no polyfill, so the Move to project menu threw on those browsers. lodash groupBy keeps the server order of the projects.
syncMemoryIndex and memoryIndexMessage now call the loaded chat messages history, and the doc comment says plainly when a chat outside a project reads its prompt rows.
|
@codex review |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d14cffa6d
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
… drags by name dnd-kit's default announcements read the draggable and droppable IDs, so a screen reader heard "move:<id>" and "project:<id>" during the new keyboard drag. The sidebar's DndContext now announces the chat title and the folder name, and pin drags keep the defaults.
…ed" in the drag announcement User-facing strings in site/src use "canceled". The drag test now reads the dnd-kit live region by its status role instead of searching for visually hidden text.
|
@codex review |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Here are some automated review suggestions for this pull request.
Reviewed commit: 310bf07153
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
…ithout Move to project in stories The archived, shared and subagent menu cases asserted DOM absence in Vitest, which the frontend contract (FE1) leaves to screenshots. Three stories now open those menus with the chat-projects experiment on, and the Vitest absence checks are removed.
…e rule above its three stories The JSDoc on the first story became that story's description in Storybook, where "these menus" had no referent. A plain comment above the group keeps the rule out of a single story's docs.
|
@codex review |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fc564f8f0
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
…ove after the menu closes The refused-move test waited for the menu item to leave the DOM before it checked the toast. The update now rejects only after the select, so the test still shows that the toast outlives the menu without a DOM check.
|
@codex review |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
The sidebar drags used @dnd-kit/core, @dnd-kit/sortable, and @dnd-kit/utilities. They now use @dnd-kit/react 0.5.1, with @dnd-kit/dom and @dnd-kit/abstract pinned to the same version. The Accessibility plugin is off because it gives each drag handle role="button", and the handles are chat links. ChatDragAnnouncer reads drag events through useDragDropMonitor and writes the instructions and the live announcements. A pinned drag now also announces the position. Up and Down still move a keyboard drag to the next folder. After an optimistic pin reorder, focus goes back to the dragged link. The click guard after a drop is removed, because the pointer sensor stops the click after a drag.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
when the projects section is collapsed, dragging a chat onto the collapsed projects section should expand the projects section. Currently, it is not possible to drag a chat to a project when the projects section is collapsed which could be confusing for users attempting to drag to move a chat to a project for the first time.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Two drags regressed in the move to @dnd-kit/react. A drop with the pointer on a muted folder moved the chat into the folder next to it. When no enabled target is under the pointer, the default collision detector falls back to the dragged row's overlap. The folders and Chats now use pointerIntersection, so a drop counts only for the target under the pointer, as before. @dnd-kit/collision 0.5.1 becomes a direct dependency for it. A keyboard drag could not reach Chats when Chats is taller than the window. The arrow key scrolls the next target into view, but dnd-kit measures the targets again only after the drag has moved, and it does not recheck a position that has not changed. The targets now measure themselves right after the scroll.
…lapsed Projects section under a chat drag A chat dragged over the collapsed Projects header now opens the section, so the folders can take the drop. The section stays open, like a click on the header. Until that drag ends, the header stays a target that moves nothing, so a drop there leaves the chat in place and the live region says it did not move. Keyboard drags reach the header with Up and the folders with Down. Pinned rows still never target Projects.
… target under the drag while the list scrolls A wheel turn during a mouse drag scrolled the list under the still pointer, but the drop stayed on the folder that had been under it. dnd-kit 0.5.1 measures the targets again after a scroll, but its collision check skips an unchanged drag position. A new plugin checks the targets again on each scrolled frame. In a scrolled list, a keyboard drag stuck on one folder because dnd-kit's Scroller scrolled the list after each step, away from the drag's still position. The plugin turns Scroller off during a keyboard row drag, which already scrolls each target into view itself.
…yboard drag targets and the drop announcement in step After a scroll, measuring the targets again for a keyboard step made dnd-kit check collisions at the old drag position. That check could change the target, and dnd-kit ignores collisions until React renders after a target change, so the target under the moved drag was lost: Up from the tall Chats read "not over", and the next Down kept the folder above as the target. The keyboard step now turns the collision checks off until the move has set the new position, then checks once. dnd-kit can also report a new target after a drag ends, until React renders the end, which replaced the drop announcement. The announcer ignores those reports.
|
@jaaydenh Done in 44bfb5c. A chat dragged over the collapsed Projects header now opens the section, so the folders can take the drop.
76d4073 and e6bbcea also fix scroll bugs from the library change that UAT found: a wheel turn during a mouse drag left the drop on the folder the pointer had left, and in a scrolled list keyboard steps got stuck or kept the previous folder as the target.
|
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Add the SetProject transition to the transition list and the execution state diagram, note that a root chat's project can change after creation, and describe how a moved chat's memory index is brought up to date at its next turn.
The PATCH /api/v2/chats/{chat} section said the endpoint emits no state
transitions unless the request changes archived. A project_id change now
applies SetProject to the chat family, so list its supported states and
refusals.
Brings in origin/main through mike/move-chats-chatd. Chat creation on main now checks project access with chatd.ChatProjectUsableBy, so validateChatProjectSelection uses that check for both creation and moves.
A move accepts the same projects as chat creation, which now include projects shared with the chat's owner. Extend the owner-check test to share the project and move the chat, and describe the rule on UpdateChatRequest.ProjectID.
chatd can now move a root chat, together with its subagent chats, into a project, to another project, or out of its project. Nothing calls this yet. #30894 adds the API. **Stack** (moving chats between projects, bottom first): 1. **This PR**: the move in chatd 2. #30894: `PATCH /api/v2/chats/{chat}` takes `project_id` 3. #30895: **Move to project** in the chat menu 4. #30795: drag a chat onto a project folder in the sidebar **Why:** today a chat keeps the project it was created with. A move must follow the family rules of archiving, because a step that runs during a move would still have the memory tools of the old project. - `chatstate.Tx.SetProject` runs only from W, E0 and E1, like `SetArchived`. `SetFamilyProject()` locks the root row before its subagents, in the same order as archiving, so a busy family gets `ErrTransitionNotAllowed` and nothing changes. - A move to the project the chat is already in changes nothing, even while the chat runs. The check reads the locked root row, so it waits for a move that is still in flight. - Only the root row stores `project_id`, and subagents use the project of the root. After the commit, chatd publishes a new `project_change` watch event for each chat of the family. The move keeps `updated_at`, so lists keep their order. - A root chat outside a project now has an empty memory index. When a chat leaves its project, or its project is deleted, its next turn start commits one memory index update that removes the memories the model saw there. **What to scrutinize:** the lock order and the same-project check under the lock in `SetFamilyProject()` (`chatstate/family.go`), the turn-start condition in `memoryIndexMessage()` (`memory.go`), and whether the `ARCHITECTURE.md` text matches them. **Generated** (`make gen`): `queries.sql.go`, `querier.go`, `dbmock` and `dbmetrics` come from `queries/chats.sql`. `coderd/apidoc`, `docs/reference/api/schemas.md` and `typesGenerated.ts` come from the new watch event kind, which is also why two `chats.test.ts` fixtures gain `project_change: false`. **Architecture doc:** `coderd/x/chatd/ARCHITECTURE.md` lists `SetProject` among the transitions and in the execution state diagram (W, E0 and E1 back to themselves). It also says that a root chat's project can change after creation, and that a moved chat's next turn starts with a memory index update from the old project to the new one. ## Testing - `make pre-commit` passes. `go test` passes for `coderd/x/chatd/chatstate`, `coderd/database/dbauthz` (`TestMethodTestSuite`) and the chatd memory tests. - `TestSetFamilyProjectLocksRootFirst` fails with 55P03 when a subagent row is locked first. `TestSetFamilyProjectKeepsTheCurrentProject` covers the same-project move during a run. The transition matrix test gains `SetProject` cases for W, E0 and E1. - `TestMovedChatUsesNewProject` runs two real turns around a move: the second turn uses the instructions and memories of the new project, and `OutOfProject` fails without the memory index removal. > Xum opened this PR on behalf of @ibetitsmike.
`PATCH /api/v2/chats/{chat}` now takes `project_id`. A project ID moves
the chat into that project, and `uuid.Nil` removes it from its project.
**Stack** (moving chats between projects, bottom first):
1. #30893: the move in chatd
2. **This PR**: the chat update API takes `project_id`
3. #30895: **Move to project** in the chat menu
4. #30795: drag a chat onto a project folder in the sidebar
**Why:** #30893 can move a chat family, and the UI layers above need an
endpoint to call.
- A move accepts the same projects as chat creation, through
`validateChatProjectSelection()`, which this PR extracts from
`postChats()` without a change in behavior.
- A project that the chat's owner cannot use gives 404, even for an
admin. The owner must own the project or hold a grant on it
(`chatd.ChatProjectUsableBy`, the check that chat creation uses since
#30439). So an admin cannot put a member's chat into a project that the
member has no access to and expose its memory.
- A child chat, an archived chat or a disabled `chat-projects`
experiment gives 400. A family that is not idle gives 409 ("Cannot move
an active chat. Interrupt or wait for the chat to finish first."). A
project deleted between the check and the move gives 404.
- The move is the last write in `patchChat()`, so a refused move keeps
the earlier writes of the same request, as a refused archive does.
**What to scrutinize:** the access check in
`validateChatProjectSelection()` and the order of the refusals in
`patchChat()` (`coderd/exp_chats.go`).
**Architecture doc:** the `PATCH /api/v2/chats/{chat}` section of
`coderd/x/chatd/ARCHITECTURE.md` lists the `SetProject` states of a move
and its refusals, and no longer says that only an `archived` change
emits transitions.
**Generated** (`make gen`): `coderd/apidoc`,
`docs/reference/api/chats.md`, `docs/reference/api/schemas.md` and
`typesGenerated.ts` come from `UpdateChatRequest.ProjectID` in
`codersdk/chats.go`.
## Testing
- `make pre-commit` passes, and so does `go test ./coderd -run
TestPatchChatProject` (14 subtests).
- `TestPatchChatProject/AnotherOwnersProject` gets 404 for another
user's project, then moves the chat after the project is shared with the
chat's owner. It fails without the access check, and also with an
owner-only check. `MoveBackWaitsForAPendingMove` ends in the wrong
project when the handler skips the move before the lock.
`ActiveFamilyReturnsConflict` checks the 409 for a running, interrupting
or action-waiting root and for a running subagent.
`PublishesProjectChange` and `AuditsTheMove` cover the watch event and
the audit entry.
- Untested: the 404 when the project is deleted between the check and
the move.
> Xum opened this PR on behalf of @ibetitsmike.
The chat menu in the sidebar, the top bar and the project page gets a **Move to project** submenu. It lists the projects of the chat's organization, marks the current one with a check mark and `aria-current`, and ends with **Remove from project**. **Stack** (moving chats between projects, bottom first): 1. #30893: the move in chatd 2. #30894: the chat update API takes `project_id` 3. **This PR**: **Move to project** in the chat menu 4. #30795: drag a chat onto a project folder in the sidebar **Why:** this is the first way for a user to move a chat, through the API from #30894. - The submenu shows only for the owner of a root chat that is not archived, with the `chat-projects` experiment. While the chat or a subagent runs, it looks disabled and does not open, as the API would return 409. It stays keyboard focusable, so keyboard and screen reader users reach the Archive agent hint it names. - `moveChatToProject` updates the cached lists and the chat entity at once. Its rollback restores only `project_id`, so it keeps the fields that watch events wrote while the request ran. A refused move shows the server message in a toast, even after the menu closes. - Other tabs follow the `project_change` watch event. A move keeps `updated_at`, so `project_id` comes only from that event, and the open chat refetches so the last event cannot win over the committed project. First list fetches that started before the move restart. - The project page hides a chat that moved away. The composer's project picker no longer says "Can't move to a project later". **What to scrutinize:** the `project_change` branch in `AgentsPageLayout.tsx` and `mergeWatchedChatSummary()` in `api/queries/chats.ts`. | Move to project submenu | Move is off while the chat runs | Project page after the move | | --- | --- | --- | |  |  |  | Pixel: new `ChatTopBar` submenu stories, `ProjectPage` `ChatMovedToAnotherProject`, and the `ChatsSidebar` menus without **Move to project** (archived, shared, subagent) and `MoveToProjectOffWhileSubagentRuns`, which focuses the blocked item. The open `CompactProjectSelector` stories lose the hint. ## Testing - `make pre-commit` passes. Vitest passes on the 5 touched specs (437 tests), and the Storybook tests of the 4 touched story files pass (134). - The open chat in `AgentsPageLayout.test.tsx` ends on the older project when two moves arrive in reverse order and it does not refetch. The `resetUnloadedChatLists` tests fail when the project list family or the unloaded-only check is removed. - A blocked **Move to project** takes keyboard focus, has the hint as its description and does not open. The test fails when the trigger is `disabled` or the open guard is removed. - Untested: `aria-current` on the current project, which `site/AGENTS.md` leaves to the stories. - Codex: the keyboard reach of a blocked **Move to project** is fixed. Seven other findings are declined in their threads: client race and cache sync hardening, and story and doc nits on patterns or text this PR keeps from main. > Xum opened this PR on behalf of @ibetitsmike.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.