Pinned sessions and groups - #118
Conversation
A session or a whole group can be pinned. Pinned things sit above a rule at the top of the sidebar, and the idle window stops ageing them out. The pin is a hover-only control beside the archive x, and becomes an unpin once set. The exemption is narrower than it sounds. There are two roads into the archived view: the idle window, which is derived and says "nothing has happened here for a month", and archiving, which is stored and says "I am finished with this one". Pinning suppresses the first and leaves the second completely alone — a pinned session can still be archived on purpose, and doing so clears the pin, because a record asserting both "keep this in front of me" and "I am done with this" has stopped meaning anything. Restoring it later brings it back unpinned rather than silently re-pinning something the operator last saw as finished. The four decisions, and why: 1. A pinned session inside an UNPINNED group is lifted to the top block, tagged with the group it came from. Leaving it in place makes pinning useless in the one case you most want it — a single agent inside a long group, far down the list — and drawing it in both places puts two live rows on screen for one session, each with its own state and controls. Lifting is a view: membership is untouched and unpinning drops it home. A session inside a PINNED group is not lifted; the group is already at the top carrying it. 2. Pinning a group exempts every member from the idle window, not just the group's position. Position alone eats itself: a pinned group whose agents age out empties one row at a time and then disappears, since the sidebar drops a group once all its agents are archived. Members get no pinnedAt of their own — membership carries them — so unpinning the group hands them all straight back, and a member pinned in its own right stays pinned. 3. Pinning adds no second ordering. It partitions the one manual order and keeps each half in it, so the two cannot disagree about what comes first. Dragging ACROSS the rule is the case that would otherwise let them fight, and the answer is that a dropped row takes the pin state of whatever it lands beside, the same way a row dropped into a group takes that group's membership: a pinned row dropped below an unpinned one unpins, and the reverse pins. The rule is a boundary you can drag across rather than a line that snaps your drop back where it came from. 4. Hover-only does have an answer on touch, and the codebase had already given it: below 720px .row-actions stops being hover-only and .age gives up its space, with a comment saying why it had to — hover-only controls "a touch device can never satisfy: on a phone they were simply unreachable". The pin joins that strip and inherits all of it. Measured at 390px: every row's controls visible, nothing overflowing. The tap target is 25px, the same as the archive x beside it; that is below the usual guidance and this change does not make it worse, but enlarging the strip is a separate decision. The empty state renders no block at all rather than an empty one, so there is no stray rule and no gap when nothing is pinned. Server-side the state is stored beside the archive state: pinnedAt on the session and on the group record, with a route per direction rather than a toggle so a double click cannot flip it to the opposite of what was on screen. A group's pin is set through its own function rather than through update(), which is the shape the tree editor sends and would otherwise clear a pin it never knew about. The rules live in web/src/lib/pinned.ts rather than spread across three components, because every one of them is a judgement with a cheaper wrong answer that would pass a casual read. Both suites are mutation-tested: the web one fails when a pinned group stops carrying its members, when lifting duplicates a row, when nested pins stop rising, and when the manual order is not preserved; the server one fails when archiving leaves the pin set, when pinning blocks archiving, when a rename wipes a group's pin, and when a group's pin is not persisted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Operator, after seeing the alternative built: a session inside a group should not be pinnable at all, because the UX of lifting one out of its group is odd. They are right, and this is a deletion rather than a new branch of logic. Gone: liftedSessions(), the append-lifted-rows loop in partitionByPin, the `from` argument on SessionRow, the `.from-group` tag that told a lifted row which group it came from, the CSS for it, and the tests that covered all of it. In its place is one predicate — canPin(ref, groups) — true for a group or a session in no group. The rules module's code went from 51 lines to 44 and lost an exported mechanism in exchange for a predicate. The three edges: The pin control on a grouped row is ABSENT, not disabled. Pinning a group is a group-level action, so it lives on the group's header alongside rename, hide-from-overview and delete-group — none of which appear on a member either. A disabled pin on every member would repeat what the header's single pin already says, once per member, and argue back when clicked. On touch, where every row's controls are visible at once, the layout teaches the rule by itself: pins on group headers and ungrouped rows, never on a member. Dragging into a group clears the pin, by the same rule that already governed the boundary above — a dropped row takes the pin state of where it lands, and it has landed somewhere a pin cannot exist. pinAfterDrop answers both boundaries now instead of one. Without this the pin would sit there invisibly and come back the moment the row was dragged out again. A stray pinnedAt on a grouped session is ignored rather than honoured, wherever the pin is read: it buys no exemption from the idle window, and a grouped session reads as unpinned for drag purposes so a leftover value cannot resurface on the way out. It cannot reach the pinned block at all — `order` holds top-level refs only, so a grouped session is not in it and there is no code left that could draw one there. Checked against a real leftover record from earlier testing: the row stayed with its group and had no pin control. Enforcing this by clearing the pin server-side wherever a session joins a group would mean touching six call sites for a value that is already inert at every point it is read. The reader-side rule is the smaller expression of the same invariant, so that is where it lives. Decisions 2, 3 and 4 stand unchanged, as does clearing the pin on a manual archive. The test suite gained the new rule and lost the old one; it still fails under every mutation, now including a grouped session becoming pinnable, a stray grouped pin being honoured, and a drag into a group keeping its pin. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lvwerra
left a comment
There was a problem hiding this comment.
Verdict: changes requested.
-
[Medium — correctness] A hidden member pin becomes active again when the member leaves its group.
web/src/components/Sidebar.tsx:463-492makes a grouped session read as unpinned, butcarryPin()only changes the dragged ref. This misses the ordinary asymmetric pairing path: pin session A, then drag unpinned session B onto A to create a group.carryPin({ landsInGroup: true })examines B, so A keeps its storedpinnedAtwhile becoming a member. Delete the group, or pull A out beside an unpinned row, and A is loose again;partitionByPin()now reads that old value and lifts A into the pinned block. I reproduced the full state transition against the real server and then saw Chromium render the released A under.pinned-block. The direct session-pin endpoint can create the same stray state while a session is grouped.This is the exact impossible-state case the settled design and the comment at
Sidebar.tsx:463-466say must never reappear. The assertion atweb/test/pinned.test.mjs:190-196does not catch it; in fact, its expectednullfor a grouped session dropped beside an unpinned row is whycarryPin()emits no unpin and the stored value survives. Please cover the resulting persisted state/final partition, not only the helper's view of the row while it is still grouped. -
[Low — test coverage] The feature suite does not guard two load-bearing integration points claimed in the PR body. In a disposable checkout I disabled the
pinnedIdsskip inApp.tsx(so pinned sessions/groups age out again) and independently removedcanPin(...)from the actual session-button render condition (so grouped members get a pin control).node test/pinned.test.mjsand the typechecked production build both still passed. The pure helpers are tested well, but there is no test showing that the archive verdict consumes them or thatSidebaruses the pinnability result. Those regressions need tests that fail when the wiring—not merely the helper—is reverted.
Queued-merge interactions:
- #145 merges mechanically, but its API facade contract is silently bypassed by all four new routes at
server/src/index.js:2483-2510, which register withapp.postrather than #145'sapi.post. On the combined tree I sent a nonemptytext/plainbody to a pin route: it returned 200 and mutated state, while an existing bodyless command routed through the facade returned the #145 policy's 415. The combined #145 boundary/API/pin suites all remained green, so its route inventory does not catch newly added raw registrations. When these branches are integrated, the pin routes need to join that facade and its inventory. - #139 currently has a content conflict with this branch in
App.tsx, around the archive verdict. The resolution must retain both its unread work and this branch'spinnedSessionIdscomputation/skip; taking #139's side would silently remove the ageing exemption. The effective merge with currentorigin/main(ef08e84) is otherwise clean and builds.
Verification: branch exercised with a real local server and headless Chromium at 1200px and 390px; touch actions were visible, page overflow was zero, and no-pin state rendered no .pinned-block. web: 22/22 discovered suites, render suite, and production build pass. server: pin suite 20/20 plus all suites after the known cron failure pass; the aggregate stopped only at the acknowledged crons.test.mjs #4 failure. The synthesized current-main merge also builds and passes both focused pin suites.
Review found that making a grouped session merely READ as unpinned was not enough. The suppression is conditional on the session being grouped, so a stray `pinnedAt` comes back to life the moment it leaves — and it can leave several ways: dragged out, the group deleted, an anchor move, a tree edit that drops it. Guarding the read side covered none of those exits. The value now never exists. Membership is written in exactly two places — groups.attach() and groups.update() — so both clear any pin on the sessions they take in, which covers every route that groups a session (pair, into, adjacent-to-a-nested-anchor, spawn-into-group, the tree editor). The other way a member could acquire one was the API: POST /api/sessions/:id/pin now refuses with 409 for a grouped session. The sidebar never asks, since it leaves the control off a grouped row; this answers agents and direct callers. Records written before this carry strays already, so groups.init() clears them on load rather than leaving a pin that springs back on the way out. The reviewer's own repro is covered: pin A, drop unpinned B onto A to pair them (the sidebar's carry-pin examines B, never A), then delete the group — A comes back loose and unpinned. Each entry and each exit is asserted separately, plus the load-time reconcile from hand-written state. None of the read-side guards changed: they are now belt-and-braces rather than load-bearing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Second review finding: lib/pinned.ts is unit-tested, but `canPin(...)` could be deleted from the row's render condition and every suite plus the typechecked build stayed green — while a grouped session grew a pin control, the impossible state the design exists to prevent. Renders the real Sidebar over a tree with an ungrouped session, a group, and a member carrying a stray pin, then reads the DOM: the loose row offers a pin, the member offers none, the group's header does, and the member's stray value opens no pinned block. Reverting canPin() out of the condition fails the second check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both findings addressed in 1. Hidden member pin reactivating on the way out — fixed at the sourceYou were right and my earlier reasoning was wrong. I argued a stray So rather than patch the exit you found, the value no longer exists. Membership is written in exactly two places, You also noted the direct endpoint can create the same state. That's the other half, so Third: records written before this already carry strays, so Going through the exits you listed, none needs its own guard now — dragged out, group deleted, group dissolved, anchor move, tree edit all start from a session that has no pin to resurrect. Tests (
I reverted each of the four parts in turn: dropping the I did not touch any of the read-side guards or 2. Untested wiringHalf done, and I want to flag the half I left. Sidebar → Archive verdict → Queued-merge notesUntouched, as they're integration-time work: the four pin routes still use Status
|
An ungrouped session or a whole group can be pinned. Pinned things sit above a rule at the top of the sidebar, and the idle window stops ageing them out. The pin is a hover-only control beside the archive
×, and becomes an unpin once set.Screens — every state, plus the reasoning: https://lvwerra-agent-artifacts.static.hf.space/pinned-sessions.html
The exemption is narrower than it sounds
archive.after)archivedAt)Archiving clears the pin because the alternative is a record asserting "keep this in front of me" and "I am done with this" at once. The later statement is the true one. Restoring later brings it back unpinned. There is a test that pinning does not block archiving, and it fails if someone makes it.
What can be pinned
An ungrouped session, or a whole group. A session inside a group cannot be pinned — its group is the thing you pin.
The lifting mechanism is gone, not disabled:
liftedSessions(), the append-lifted-rows loop, thefromargument onSessionRow, the.from-grouptag and its CSS, and the tests that covered them. In their place is one predicate,canPin(ref, groups). The rules module's code went from 51 lines to 44 and traded an exported mechanism for a predicate.The control is absent, not disabled. Pinning a group is a group-level action, so it lives on the group's header alongside rename, hide-from-overview and delete-group — none of which appear on a member either. A disabled pin on every member would repeat what the header's single pin already says, once per member, and argue back when clicked. On touch, where every row's controls are visible at once, the layout teaches the rule by itself: pins on group headers and ungrouped rows, never on a member.
The two edges that come with it
Dragging into a group clears the pin — the same rule that already governed the boundary above. A dropped row takes the pin state of where it lands, and it has landed somewhere a pin cannot exist.
pinAfterDropanswers both boundaries now instead of one. Without this the pin would sit there invisibly and come back the moment the row was dragged out again.Dragging out of a group leaves it unpinned. A grouped session reads as unpinned wherever the pin is consulted, whatever its stored record says, so on the way out it takes the state of wherever it lands — never a stale
true. Dropping it deliberately into the pinned block does pin it, which is the operator asking for that.A stray
pinnedAton a grouped session is ignored, not rendered. It buys no exemption from the idle window, and it cannot reach the pinned block:orderholds top-level refs only, so a grouped session is not in it and no code is left that could draw one there. Verified against a real leftover record from my own earlier testing — the row stayed with its group and had no pin control.Enforcing this by clearing the pin server-side wherever a session joins a group would mean touching six call sites for a value that is already inert at every point it is read. The reader-side rule is the smaller expression of the same invariant, so that is where it lives.
The other three decisions (unchanged)
Pinning a group exempts its members from the idle window, not just the group's position. Position alone eats itself: a pinned group whose agents each age out empties one row at a time and then disappears entirely, because the sidebar drops a group once all its agents are archived. Members get no
pinnedAtof their own — membership carries them — so unpinning the group hands every member straight back.Pinning adds no second ordering. It partitions the one manual order and keeps each half in it. Dragging across the rule is covered above.
Hover-only has an answer on touch, and the codebase had already given it. Below 720px
.row-actionsstops being hover-only and.agegives up its space — with a comment saying hover-only controls are something "a touch device can never satisfy: on a phone they were simply unreachable."One caveat I will not paper over: the pin's tap target is 25px, the same as the archive
×beside it. Below the usual 44px guidance. It is the size these controls already are and this change does not make it worse, but enlarging the strip is a separate decision.Empty state
No block is rendered when nothing is pinned — not an empty one — so no stray rule and no gap. Verified: with everything unpinned the
.pinned-blockelement does not exist.Where the state lives
Stored beside the archive state:
pinnedAton the session and group records, not derived, surviving a restart.POST /api/sessions/:id/pin|unpinandPOST /api/groups/:id/pin|unpin— one per direction rather than a toggle, so a double click or a stale row cannot flip it to the opposite of what was on screen. A group's pin is set through its own function rather thanupdate(), the shape the tree editor sends, which would otherwise clear a pin it never knew about; there is a test for exactly that.Tests
Both new suites are mutation-tested.
web/test/pinned.test.mjsfails when a grouped session becomes pinnable, when a stray grouped pin is honoured, when a drag into a group keeps its pin, and when a pinned group stops carrying its members.server/test/pin.test.mjsfails when archiving leaves the pin set, when pinning blocks archiving, when a group rename wipes the pin, and when a group's pin is not persisted.Green: typecheck, web 22/22, every server suite except
server/test/crons.test.mjs, which is red on cleanmainand is not mine.Not merged, not deployed.