fix(channels): keep the backend task channel in sync on rename and delete - #3228
fix(channels): keep the backend task channel in sync on rename and delete#3228adamleithp wants to merge 2 commits into
Conversation
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
|
Reviews (1): Last reviewed commit: "fix(channels): keep backend task channel..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Renaming a channel folder left the backend task channel behind (it is resolved by name), so the feed re-provisioned an empty channel and existing tasks/messages were orphaned. Rename now moves the backend channel by id in lockstep with the folder, with rollback if the folder rename fails. Deleting a channel now soft-deletes the backend channel's tasks and the channel itself (both soft deletes server-side, so data is recoverable) before removing the folder, instead of leaving them orphaned on an unreachable channel. Generated-By: PostHog Code Task-Id: 3ac7dc89-890b-4013-bf1f-5dc40476f093
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
9d724ca to
a7483f1
Compare
| // backend name back so the two sides stay in sync. | ||
| if (backendChannel) { | ||
| await client | ||
| .renameTaskChannel(backendChannel.id, from) |
There was a problem hiding this comment.
Once the folder rename fails and this compensating revert is attempted, its own failure is discarded here (.catch(() => undefined)). If the revert also fails (e.g. a concurrent rename already took from, or a transient error), the backend channel is left permanently stuck on to while the folder still shows from — with no log or user-visible signal beyond the generic "Couldn't rename channel" toast. Worth at least logging via ROOT_LOGGER so the desync is diagnosable.
| await client.deleteTaskChannel(backendChannel.id); | ||
| } | ||
| } | ||
| return client.deleteDesktopFileSystem(id); |
There was a problem hiding this comment.
This delete path soft-deletes the backend channel and its tasks unconditionally before an unguarded deleteDesktopFileSystem(id) call — unlike renameMutation below, there's no rollback if this final call fails. If it fails after the backend soft-delete already succeeded, the folder stays in place but its backend channel is now soft-deleted and absent from getTaskChannels(). Reopening that folder later will make useBackendChannel silently provision a brand-new empty channel, permanently orphaning the original tasks with no error surfaced about that specific consequence.
| const deleteMutation = useMutation({ | ||
| mutationFn: async (id: string) => { | ||
| mutationFn: async ({ id, name }: { id: string; name: string }) => { | ||
| if (!client) throw new Error("Not authenticated"); |
There was a problem hiding this comment.
Both deleteMutation and renameMutation implement multi-step, cross-backend orchestration inline in a useMutation — list backend channels, find a match by normalized name, conditionally mutate, and (for rename) manually compensate on failure. Per this repo's CLAUDE.md, that's business orchestration ("orchestration, retries, dedupe, sagas... Inversify services only" is packages/core's job), and Rule 5 says hooks should wrap exactly one mutation, with multi-source orchestration belonging in a service method. The repo already has a Saga abstraction (packages/shared/src/saga.ts) used by other packages/core sagas, and a sibling ChannelTasksService in packages/core/src/canvas/ — this logic seems like a natural fit there instead, and would be reusable/testable outside a React hook.
| // than 500 tasks soft-deletes only the first page here. The channel | ||
| // itself is still soft-deleted below, so any stragglers stay attached | ||
| // to it and remain recoverable — nothing is lost, just left behind. | ||
| const tasks = await client.getTasks({ channel: backendChannel.id }); |
There was a problem hiding this comment.
This fires one concurrent HTTP DELETE per task with no batching — for a channel near the 500-task getTasks cap, that's up to 500 simultaneous requests. Promise.allSettled handles individual failures fine, but there's no cap on concurrency, which could trigger backend rate-limiting during the burst on a large channel. (Not a new pattern in this codebase, but worth a concurrency limit here given the potential scale.)
| // local has changed. | ||
| const normalized = normalizeChannelName(name); | ||
| if (normalized !== PERSONAL_CHANNEL_NAME) { | ||
| const backendChannels = await client.getTaskChannels(); |
There was a problem hiding this comment.
This "find the public backend channel by normalized name" lookup is duplicated here and in renameMutation below, and a very similar lookup already exists in useBackendChannel (useTaskChannels.ts). Worth extracting to one shared helper (e.g. findPublicBackendChannel(channels, normalizedName)) so the matching rule only needs to change in one place.
2 participants
Problem
Channels exist in two places: the desktop file-system folder (the sidebar entry) and a backend task channel that owns the task feed and thread messages — and the UI maps one onto the other by name. That made two channel actions lossy:
Changes
renameTaskChannel/deleteTaskChannelto the api-client (the backend endpoints already existed; the client never called them).How did you test this?
useChannels.test.tsxcovering the rename lockstep/rollback paths and the delete ordering/failure paths; full@posthog/uisuite passes.turbo typecheckfor@posthog/api-client+@posthog/ui, Biome clean.Automatic notifications
Created with PostHog Code