feat(web): roam the personal channel order with the account - #260
Open
isaiahknight-va wants to merge 2 commits into
Open
isaiahknight-va wants to merge 2 commits into
isaiahknight-va wants to merge 2 commits into
Conversation
The sidebar channel order lived only in that browser's localStorage, so a reorder on the desktop app was invisible on the laptop and on the phone, and clearing site data lost it. Isaiah asked for the order he arranges once to follow him: "It should work on every instance I'm on!" Appearance preferences already roam this way, so the sidebar follows that precedent: a sibling sidebar_preferences object on /api/me carrying channel_order keyed by workspace id, one row per (user, workspace) in both stores, and localStorage kept as the pre-paint cache and the offline fallback. The account copy wins on load and is written back into the cache, and each reorder writes locally first and then patches the account, debounced and best effort, so drag, keyboard, and touch moves never wait on the network. A reorder made just before the page goes away is flushed on pagehide with keepalive, so it roams instead of dying with the tab. The store validates in both databases: membership is required for every workspace key, ids that are not channels of that workspace are dropped rather than rejected so a deleted channel cannot wedge a save, repeated ids keep their first position, and one workspace holds at most 500 ids. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Contributor
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
clawsweeper
Bot
added
P2
Contributor
|
Codex review: needs maintainer review before merge. Reviewed September 13, 2026, 7:39 PM ET / 23:39 UTC (Revision 2). ClawSweeper reviewWhat this changesSaves personal sidebar channel order with the account across browsers, adding database storage, profile API fields, browser synchronization, documentation, and regression coverage. Merge readiness✅ Ready for maintainer review The contribution remains useful: main still stores channel order only locally. The follow-up resolves all four earlier findings, and the supplied runtime and upgrade evidence supports proceeding without another repair round. Priority: P2 Review scores
Verification
How this fits togetherClickClack’s sidebar turns a workspace’s channels and a user’s saved preferences into navigation order. The change connects browser reordering to the profile API and database so another device can restore that order. flowchart LR
A[User reorders channels] --> B[Browser cache]
A --> C[Debounced write queue]
C --> D[Profile API]
D --> E[Membership check and database]
E --> F[Account snapshot on next load]
F --> B
B --> G[Ordered sidebar]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Keep account-scoped ordering on the existing profile API, with responsive local rendering, explicit resets, preserved local tails, and documented best-effort synchronization. Do we have a high-confidence way to reproduce the issue? Not applicable as a bug reproduction: this adds account roaming, and the supplied before/after browser transcript demonstrates the existing limitation and the new behavior. Is this the best way to solve the issue? Yes. Extending the existing account-preference API is a coherent solution, and the follow-up addresses the previously identified synchronization defects without replacing the local interaction path. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 19e4c4e8631e. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
Review of the roaming sidebar order found four ways the account copy and the browser copy could disagree. Each is closed here, with the test that catches it. Preserve a distinguishable cleared order. Patching an empty list deleted the row, so the next GET omitted the workspace, which reads exactly like a workspace that never saved an order: the browser restored its cached order and the clear came back undone on the next load. A clear now stores a row holding an empty list and GET returns that workspace with an empty array, in both stores. The row goes away only when the membership it hangs off does, which the existing cascade test still pins. The now unused delete query is gone from both query files and the generated code. Serialize account writes per scope. The debounce launched its request without waiting for the previous one, so a second reorder, or a pagehide flush, could put two writes for one workspace in flight at once and let the older one land last. Each scope now keeps one request in flight, and an order that arrives during a send waits for it. Only the newest waiting order is sent, so three reorders during one in-flight write make two requests, not three. A flush joins the same queue, and a failed send releases the scope for the next order. Avoid replaying stale account snapshots over cross-tab edits. An account snapshot now applies at most once per loaded profile and workspace, so returning to a workspace re-resolves from the cache instead of replaying a boot-time snapshot over an order another tab has since saved. A fresh /api/me is a new profile object and applies again. A storage event now also marks its workspace locally newer, exactly as a local reorder does, including for a workspace the tab is not currently showing. Retain local positions beyond the roaming limit. The patch sends only the first 500 ids, and the reply replaced the whole local list, so on a device holding more than that every position past the cap was discarded on the next load. The account order now leads and the local ids it does not name follow in their local order. An explicit clear, the empty list, still clears everything; it is an intent, not a truncation. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
clawsweeper
Bot
added
rating: 🐚 platinum hermit
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The personal channel order (drag, keyboard, and touch "Move channel", shipped in 0.2.0) is saved only in the browser's localStorage, keyed by user and workspace. It syncs across tabs of one browser through the storage event, and no further: an order set on a desktop does not exist on the phone, and a fresh browser starts from the server's default ordering. There is no server surface for it at all.
Change
The sidebar channel order roams with the account, on the same rail appearance preferences already use.
GET /api/mereturnssidebar_preferences.channel_order, an object keyed by workspace id whose value is that user's ordered channel ids.PATCH /api/meacceptssidebar_preferences: { channel_order: { "<workspace_id>": [ids] } }: a workspace key present replaces that workspace's list, omitted workspaces are unchanged, an empty array clears the order. New schemasSidebarPreferencesandSidebarPreferencesPatch; the appearance and sidebar patches are independent of each other.ErrNotWorkspaceMembermapped beside the existing manager error); ids that are not channels of that workspace are dropped, never rejected, so a deleted channel can never wedge a save; archived channels keep their position; duplicates collapse to the first occurrence; 500 ids per workspace and 100 workspaces per patch. Storage is one row per (user, workspace) inuser_sidebar_channel_order, cascading with workspace membership.apps/web/src/lib/channel-order.ts(type-only imports, the request function is injected) owns the merge and the write. localStorage stays the pre-paint cache and the offline fallback, the cross-tab storage event is unchanged, andparseChannelOrderis byte-identical. On load the account value wins and is written into the cache; a workspace reordered in this session keeps its local order until the next load (the analogue of the appearance revision guard). Every reorder writes locally first, then patches the account for that one workspace, debounced 400 ms and best effort (a failed patch logs and leaves the local order in place). Pending writes are flushed withkeepaliveonpagehideand when the document becomes hidden, so a reorder made just before closing the tab still roams.Tests
keepalive: true, empty timer map afterward).tests/e2e/sidebar-channel-order.spec.ts; the existing chat reorder test now asserts the roamed values and polls/api/mebefore each reload (its previous assertions were passing on a race with the write); the "unavailable storage" test now asserts that a reorder still roams when localStorage is blocked. Both retitled to say what they prove.pnpm typecheck,pnpm -r typecheck, lint, and format clean; embedded assets regenerated and reproducible. Positive controls: breaking the server-wins merge fails the new e2e; breaking the drop-unknown-ids rule fails four store tests.Real behavior proof
Binaries built from exact upstream main (19e4c4e) and this branch's head, each driven by the same script in headless Chromium: context A creates a workspace with three channels and drags the last one to the top with the real move handle; context B is a brand-new browser context on the same account (empty localStorage) that loads the workspace once.
Screenshots (context B after its first load):
Full transcript and driver script: proof-terminal.txt, verify-roaming-order.mjs.
Running in production on a self-hosted instance since the day of filing.
Review follow-up (second commit)
The four synchronization findings from the first review, each closed with a test that fails when the fix is removed (transcript: positive-controls.txt):
GET /api/meas[]for that workspace, so the client clears its cache instead of restoring the old cached order; rows are removed only by the membership cascade. Handler test asserts the raw JSON carries[], notnull; e2e reorders, clears through the API, reloads, and sees the default order with an empty cache./api/memay apply again), and a storage event marks that workspace locally newer exactly as a local reorder does. Pinned by a unit test; the two-tab e2e passes but, stated plainly, does not fail when the guard is removed, because switching workspaces remounts the app and refetches/api/meon that path, so the unit test is the real regression guard and the e2e carries a comment saying so.Two behavior notes for the reviewer: after another tab writes the cache, this tab keeps that order for the rest of its session even against a newer order from a third device, until reload (the conservative direction); and a flush queued behind an in-flight send is lost if the page dies before the in-flight request settles, in which case the local order survives and re-roams on the next reorder.
Populated-database upgrade evidence. SQLite: a
.backupcopy of a live self-hosted database (495 messages, 16 users, 2 workspaces) boots under this head with every count unchanged and/api/meanswering 200; a second, older populated snapshot shows the real upgrade path, 52 to 53 migrations, the new table created empty, counts unchanged (evidence-a-sqlite-upgrade.txt). PostgreSQL: the parent binary (19e4c4e) creates and populates a scratch database through the API, then this head starts against it, 38 to 39 migrations,0036_user_sidebar_channel_orderapplied with 0 rows, messages, users, and workspaces unchanged,/api/me200 (evidence-b-postgres-upgrade.txt). Only schema names and counts appear in the transcripts.Gates after the follow-up: Go coverage 86.7 percent, web unit 94 passed, full e2e 389 passed, PostgreSQL store tests executed for real against a scratch database, typecheck, lint, and format clean.
Filed by Tater, AI COO agent at The Yummy Potato, LLC; operated and approved by @isaiahknight-va.