Skip to content

fix(workspace): reserve the rail column while the stage loads (#2711) - #2785

Merged
vybe merged 4 commits into
devfrom
fix/2711-rail-column-reserved
Sep 22, 2026
Merged

vybe merged 4 commits into
devfrom
fix/2711-rail-column-reserved

Conversation

@dolho

@dolho dolho commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Stacked on #2784 → #2783 → #2782 → #2781 → #2780 → #2778. Part of epic #1430.

Fixes #2711

The shift, measured

On every cold load the conversation column renders full width and then loses the rail's width when the roster arrives. Since #2676 it is a 300ms animated slide rather than a jump — prettier, still a shift, and the contract says loading and loaded share one footprint.

Before the fix, the Send button during load:

width Send x movement
1440 1327 → 1300 27px
1024 916 → 884 32px

The fix

railVisibleFor answers may the rail render, and false for a non-ready stage is right — its tabs need the roster. But the column's width does not need the roster: it comes from the persisted rail state and is known synchronously at first paint.

So the wrapper renders on railHasColumn || railColumnReserved and gates only PortalRail inside. Space is reserved; content is not faked.

What it deliberately does NOT reserve

railColumnReservedFor is narrower than railVisibleFor's route set, and the exclusions are the interesting part:

So it reserves for exactly the case the bug is about: a 1:1 conversation route, mid-load. If the rail then turns out not to render, the column leaves through the existing width transition — a shrink, not a jump.

Motion is untouched

The <Transition> classes, the #2676 voice-canvas swap and motion-reduce are unchanged, and the reserved column is present from the first frame, so its enter never runs.

Verification

  • Send's x stable within 1px across 24 samples through roster arrival at 1440 / 1024 / 768 / 640.
  • The column keeps the same width when the rail lands (reserved → settled, ≤1px).
  • workspace-model-choice.spec.js — the spec whose measurements the The Workspace rail column steps discretely when a voice call ends (#2640 follow-up) #2676 transition split, and which the issue asks to keep green — passes.
  • Negative control: reverted, the spec fails with exactly the 27px/32px slides above.
  • Unit spec (8 cases) pins the rule, including that reserve and visible are never both true and that a 1:1 column is continuously occupied across the loading → ready transition.
  • Frontend unit suite: 135 files, 2957 tests green.

One test removed rather than left skipping

An "agent page carries no rail column" e2e case asserted a premise the router does not hold: /workspace/a/:name redirects into a conversation, which legitimately has a rail. I found that by chasing the skip instead of accepting it. The rule it meant to check is covered in the unit spec, which does not depend on which URL the router settles on.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Q19uRCksdn4DiRAJ55rfpZ

@dolho

dolho commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Review — /review pass

Two findings, one of them reproduced. This PR should not merge as-is.

The fix is right for the case it targets — Send is stable within 1px through roster arrival at 1440/1024/768/640, and reverting it reproduces the 27px/32px slides. But the reservation predicate is wider than the case, and on the routes it over-reaches it creates the shift it exists to remove.

[C1] On an empty or failed roster, the reserved column slides in and back out (confidence 10/10 — measured)

railColumnReservedFor returns true for any non-room, non-agent-page route while stageState === 'loading'. But stageZone keys on the roster, not on threads:

const { state } = viewState({ hasLoaded, error, count: agents.length })

so a caller with no rostered agents settles on empty, and a failed roster fetch on failed. In both, railHasColumn never becomes true, the reservation drops, and the column leaves through the 300ms width transition.

Reproduced by stubbing the roster response, sampling the column every 120ms:

EMPTY roster   ->  none → reserved(48) → reserved(22) → reserved(3) → none
FAILED roster  ->  none → reserved(48) → reserved(10) → reserved(1) → none

That is a 48px animated shift on first-run (nobody has shared an agent yet) and on every roster error — landing next to an error state, where layout stability matters most. It is the same defect class the PR removes, introduced by the same change, and it is precisely the hazard the docblock reasons about for rooms ("reserving on a capability we have not been told about yet would trade this shift for the opposite one") without applying that reasoning to empty / failed.

Fix options, in order of preference:

  1. Reserve only when the route names a conversation (/workspace/c/:id) — the route the AC actually specifies. A session in the URL means a rail is certain, and the empty/failed roster cannot reach it.
  2. Keep the current predicate but render the reserved column outside the <Transition>, so an over-reservation corrects in one frame instead of animating. Weaker: it is still a shift, just a faster one.

Option 1 changes what the spec measures — see [C2].

[C2] The spec does not measure the route the AC names (confidence 9/10)

AC #1 says "on a cold load of /workspace/c/<session>". Every arm of workspace-rail-reserved.spec.js loads bare /workspace. On an instance with threads that redirects into a conversation, so the measurement is meaningful — but it is meaningful by accident of the fixture, and on an instance where /workspace settles empty the same spec would measure the [C1] flash and still pass, because it only samples the Send button, which does not exist on an empty stage.

Add an arm that resolves a session id and loads /workspace/c/<id> directly, and an arm that stubs an empty roster and asserts the column never appears-then-leaves. The second is the regression test for [C1] and I have already written the probe for it above.

Checked and clean

  • The reserve/visible pair is proven mutually exclusive and jointly continuous across loading → ready for a 1:1 route — good properties to have pinned.
  • Motion is genuinely untouched: <Transition> classes, the The Workspace rail column steps discretely when a voice call ends (#2640 follow-up) #2676 voice-canvas swap and motion-reduce are unchanged, and the reserved column is present from the first frame so its enter never runs.
  • workspace-model-choice.spec.js — the spec the The Workspace rail column steps discretely when a voice call ends (#2640 follow-up) #2676 transition split, and which the issue asks to keep green — passes.
  • Removing the "agent page carries no rail column" case rather than leaving it skipping was right: /workspace/a/:name redirects into a conversation (verified: it lands on /workspace/c/<id>), so the test asserted a premise the router does not hold.
  • portalVoiceLayoutMotion.spec.js slices Portal.vue ±400/200 chars around the ws-rail-column testid; the new :data-reserved binding lands inside that window and it still passes, but that coupling is fragile and worth knowing about when editing this markup again.

@dolho
dolho force-pushed the fix/2202-settings-404-and-key-pagination branch from db37874 to 0524908 Compare September 14, 2026 14:42
@dolho
dolho force-pushed the fix/2711-rail-column-reserved branch from 2d51d0a to fc337ce Compare September 14, 2026 14:42
@dolho

dolho commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Re-review — [C1] and [C2] fixed

1bb2aa4f6.

[C1] — the unfulfilled reservation

I did not narrow the predicate, and the reason is worth stating: reserving only on /workspace/c/:id would protect the AC's route and leave the common path — the nav's bare /workspace, which redirects into a conversation — shifting exactly as before, because the redirect lands after the roster does. That would have traded a measured fix for a narrower one.

Instead the leave is animated only when the column actually held a rail (railEverHeldRail). A reservation that pays off animates as before; one that does not is given back in a single frame. Measured on the same stubbed loads that produced the finding:

roster before after
empty reserved(48) → 22 → 3 → none reserved(48) → none
failed reserved(48) → 10 → 1 → none reserved(48) → none
normal reserved(48) → rail(48) reserved(48) → rail(48)

The enter keeps its transition unconditionally — it only ever runs for the voice-canvas swap (#2676), since a reserved column is present from the first frame and never enters.

[C2] — the spec now measures the route the AC names

Two arms added:

  1. /workspace/c/<session> loaded directly (the id resolved from the sessions route), so the measurement no longer depends on bare /workspace happening to redirect on a fixture with threads. Send stays within 1px.
  2. Empty and failed roster — asserted on the shape of the removal, not its speed: every width ever observed must be the full reserved width or nothing, because an intermediate width is an animation frame. Both stubs pass.

Negative control on the new arm: forcing the old unconditional leave fails it with empty roster: the column animated away through 6,22px. It bites.

One spec updated rather than deleted

portalVoiceLayoutMotion.spec.js pinned the literal leave-to-class="!w-0", which is now a binding — this is the fragile ±400/200-char slicer I flagged in the first review, and it caught a real change. Updated to assert the conditional form, because the #2676 property it protects is intact: the canvas only ever takes the row from a rail that was already on screen, so railEverHeldRail is true wherever that spec cares.

Residual, stated rather than hidden

railEverHeldRail resets on route.fullPath, assigning railHasColumn's value at that instant. An in-session transition from an open conversation to a rosterless state — which needs a roster refresh to start failing mid-session, not a cold load — could carry true across and animate the give-back. The cold-load paths that produced this finding are all covered above; I did not chase that one because reproducing it needs a mid-session roster failure and the payoff is one animated frame in a state the user is already seeing an error for.

Frontend unit suite 135 files / 2957 tests; rail + model-choice e2e 12/12.

@dolho

dolho commented Sep 14, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up — a real bug in my own fix, found by chasing the CI red

810e9f706. The previous commit's reset watcher read route.value.fullPath, but useRoute() returns a reactive object, not a ref. Every other route read in this 2,000-line file is the plain form; that line was the only route.value. in it.

It is not a silent no-op, it is a silent throw. Vue routes a watch-getter error to its error handler rather than aborting setup, so:

  • the page rendered normally,
  • the rail e2e passed 12/12,
  • the unit suite passed 2957,
  • and every Workspace load logged TypeError: Cannot read properties of undefined (reading 'fullPath') while the watcher was dead.

I only found it because #2787's e2e went red after the rebase and the first plausible story — an 11.4-minute run versus the previous 2.5 — was "slow runner, ignore it". Reproducing locally instead, then reading the browser console rather than the test output, is what surfaced it.

What was and wasn't affected. The measured behaviour in the previous comment stands, because railEverHeldRail is only ever SET on a cold load by the other watcher: reserved(48) → none on an empty or failed roster, reserved(48) → rail(48) normally — re-measured on this build. What was broken is the per-route reset, i.e. exactly the in-session case I listed as a residual. That residual is now a working watcher rather than a dead one.

Guarded, not just fixed. portalRailReserve.spec.js now fails on any route.value. in the shell, and I proved the guard bites by reintroducing the bug (it fails) before trusting it. A dead watcher is invisible to every test that does not read the console, so the spelling is worth pinning in the one file that mixed it.

Unit suite 135 files / 2959 tests; the three workspace suites CI flagged — rail-reserved, model-choice, stick-to-bottom — 15/15 locally. #2787 rebased onto this; its own diff is unchanged at 95 files / 752 / 330.

@dolho
dolho force-pushed the fix/2202-settings-404-and-key-pagination branch from 0524908 to 32600a5 Compare September 18, 2026 08:48
@dolho
dolho force-pushed the fix/2711-rail-column-reserved branch from 810e9f7 to e5d44ad Compare September 18, 2026 08:48
@dolho
dolho marked this pull request as ready for review September 18, 2026 08:48
@dolho

dolho commented Sep 18, 2026

Copy link
Copy Markdown
Contributor Author

/review — post-rebase (stacked on fix/2202-settings-404-and-key-pagination, 3 own commits)

Scope: CLEAN. Plan completion vs #2711: 2 done / 1 partial (AC1, see I1) / 1 unverifiable (model-choice spec — suite green on the tip).

Critical

None.

Informational

  • [I1] A persisted-OPEN rail is reserved at the collapsed width, then jumps (8/10) — Portal.vue:973 railOpen: computed(() => railState.value.open && railVisible.value) feeds useColumnResize.js:254 railWidth = railOpen ? effectiveRail : RAIL_COLLAPSED, and railVisibleFor returns false for every non-ready stage. So during loading the reservation is always 48px; on roster arrival --ws-rail becomes the open width and the conversation column shifts by open − 48 — the "dragged width when open" case the issue names. The docblock's "the width is the persisted one, known synchronously" holds only for the collapsed default, which is what the e2e (fresh browser, no persisted state) measures. Fix: railOpen: computed(() => railState.value.open && (railVisible.value || railColumnReserved.value)) + one e2e case that opens the rail, reloads, and asserts spread ≤ 1px. Not blocking — the default path is fixed and the open case is no worse than before.
  • [I2] No test for the railEverHeldRail route reset semantics (7/10) — Portal.vue:1081; the unit spec pins the route.value spelling only.

Clean

Frontend-only; watchers on railHasColumn / route.fullPath converge in either order; RAIL_MOTION is now the single enter/leave definition; stageState values exhaustively iterated in the spec; data-reserved="true" distinguishes "space held" from "rail present". architecture/workspace.md could take a one-line note under the ent#474 rail section.

@dolho

dolho commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

merge-train: pushed the @source-text-pin declaration to this branch — mechanical. portalRailReserve.spec.js's last describe reads Portal.vue by text (the route.value spelling guard) and is a NEW spec, so #2922's ratchet fails on the combined tree. Only the text can prove a spelling is absent, so it declares the pin with that reason. #2787 rebased on top.

dolho added a commit that referenced this pull request Sep 21, 2026
@vybe
vybe force-pushed the fix/2202-settings-404-and-key-pagination branch 2 times, most recently from b2a5fc9 to f7bccc1 Compare September 22, 2026 17:53
dolho and others added 4 commits September 22, 2026 19:14
On every cold load the conversation column rendered full width, then lost the
rail's width the moment the roster arrived — sliding the composer, the thread and
the header left. Since #2676 that is a 300ms animated slide rather than a
one-frame jump, which is prettier and still a shift: the contract's
layout-stability rule is that loading and loaded share ONE footprint and nothing
moves on arrival.

Measured on a local instance before the fix, the Send button at 1440px goes
1327 → 1300 (27px) as the roster lands, and 916 → 884 (32px) at 1024px.

`railVisibleFor` answers "may the rail RENDER", and false for a non-ready stage
is right — its tabs need the roster. But the column's WIDTH does not need the
roster: it comes from the persisted rail state and is known synchronously at
first paint. So the wrapper now renders on `railHasColumn || railColumnReserved`
and gates only `PortalRail` inside. Space is reserved; content is not faked.

`railColumnReservedFor` is deliberately NARROWER than `railVisibleFor`'s route
set, and the exclusions are the interesting part:

  * an agent page never carries a rail, so reserving there would invent a gap;
  * a ROOM route is excluded even though a ready room usually has a rail,
    because that depends on `roomsAvailable`, which arrives ON the roster
    payload (#2128). Reserving against a capability we have not been told about
    yet would trade this shift for the opposite one on every install without
    rooms.

So it reserves for exactly the case the bug is about: a 1:1 conversation route,
mid-load. If the rail then turns out not to render, the column leaves through the
existing width transition — a shrink, not a jump.

Nothing about the motion changes: the `<Transition>` classes, the #2676
voice-canvas swap and `motion-reduce` are untouched, and the reserved column is
present from the first frame so its enter never runs.

Verified: the composer's x is stable within 1px across 24 samples through roster
arrival at 1440/1024/768/640, the column keeps the same width when the rail
lands, and `workspace-model-choice.spec.js` — the spec whose measurements the
#2676 transition split — stays green. Reverted, the same spec fails with the
27px/32px slides above, so it bites. Unit suite 135 files / 2957 tests.

One e2e case was written and then removed rather than left skipping: "an agent
page carries no rail column" asserted a premise the router does not hold —
`/workspace/a/:name` REDIRECTS into a conversation, which legitimately has a
rail. The rule it meant to check is covered in the unit spec, which does not
depend on which URL the router settles on.

Fixes #2711

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q19uRCksdn4DiRAJ55rfpZ
#2711)

Review finding, reproduced: the reservation fires for any non-room, non-agent-
page route while the stage loads, but `stageZone` keys on the ROSTER — so a
caller with no rostered agents settles `empty` and a failed roster fetch settles
`failed`. In both the rail never arrives, the reservation drops, and the column
left through the 300ms width transition:

    EMPTY roster   ->  none -> reserved(48) -> reserved(22) -> reserved(3) -> none
    FAILED roster  ->  none -> reserved(48) -> reserved(10) -> reserved(1) -> none

A 48px animated shift on first run — nobody has shared an agent yet — and on
every roster error, landing beside an error state. That is the same defect this
PR removes, handed back, and it is exactly the hazard the docblock reasons about
for ROOMS without applying it to `empty` / `failed`.

The fix is not to reserve less. Narrowing to `/workspace/c/:id` would protect the
AC's route and leave the common path — the nav's `/workspace`, which redirects
into a conversation — shifting as before, because the redirect happens after the
roster lands. Instead the LEAVE is animated only when the column actually held a
rail (`railEverHeldRail`): a reservation that paid off animates as before, one
that did not is given back in a single frame. Measured after:

    EMPTY roster   ->  none -> reserved(48) -> none
    FAILED roster  ->  none -> reserved(48) -> none

and the normal load is unchanged: `reserved(48) -> rail(48)`, no shift.

The ENTER keeps its transition unconditionally — it only ever runs for the
voice-canvas swap (#2676), since a reserved column is present from the first
frame and never enters. The flag resets per route, because the question is about
THIS stage: navigating from a conversation to an empty roster must not inherit
the conversation's verdict.

Two spec arms added for the second finding — the suite measured bare
`/workspace`, not the `/workspace/c/<session>` the AC names, so it was right
only by accident of the fixture:

  * a direct load of `/workspace/c/<id>` (resolved from the sessions route)
    keeps Send within 1px;
  * an empty and a failed roster give the column back with NO intermediate
    width. The assertion is about the shape of the removal, not its speed:
    every width observed must be the full reserved width or nothing, because an
    intermediate width IS an animation frame. Negative control: forcing the old
    unconditional leave fails it with "animated away through 6,22px".

`portalVoiceLayoutMotion.spec.js` pinned the literal `leave-to-class="!w-0"`,
which is now a binding. Updated rather than deleted: the #2676 property it
protects is intact, since the canvas only ever takes the row from a rail that was
on screen, so `railEverHeldRail` is true wherever that spec cares.

Frontend unit suite 135 files / 2957 tests; the rail and model-choice e2e suites
12/12.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q19uRCksdn4DiRAJ55rfpZ
)

Caught by chasing a red CI run rather than dismissing it. The reset watcher
added with the reservation read `route.value.fullPath`, but `useRoute()` returns
a reactive OBJECT, not a ref — every other route read in this 2,000-line file is
the plain form, and this line was the only `route.value.` in it.

It is not a silent no-op, it is a silent THROW: Vue routes a watch-getter error
to its error handler instead of aborting setup, so the page rendered, the rail
e2e passed 12/12, the unit suite passed 2957, and the watcher was dead the whole
time while every Workspace load logged

    TypeError: Cannot read properties of undefined (reading 'fullPath')

Verified in a browser before and after: the error is on the previous build and
gone from this one. The measured rail behaviour is unchanged (`reserved(48) ->
none` on an empty or failed roster, `reserved(48) -> rail(48)` normally) because
the flag is only ever SET by the other watcher on a cold load; what was broken is
the per-route reset, which is the in-session case.

Guarded rather than just fixed: `portalRailReserve.spec.js` now fails on any
`route.value.` in the shell, and the guard was proven to bite by reintroducing
the bug (it fails) — a dead watcher is invisible to every test that does not read
the console, so the spelling is worth pinning in the one file that mixes it.

Unit suite 135 files / 2959 tests. The three workspace e2e suites CI flagged —
rail-reserved, model-choice, stick-to-bottom — 15/15 locally.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q19uRCksdn4DiRAJ55rfpZ
…urce-text-pin (#2711) — mechanical, per the merge-train note on the PR

The train (#2922's source-text ratchet + this stack) found the joint break:
the `route.value` spelling guard reads Portal.vue by text and this is a NEW
spec with no baseline entry. It is a legitimate pin — only the text proves a
spelling is absent — so it declares itself one.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@vybe
vybe changed the base branch from fix/2202-settings-404-and-key-pagination to dev September 22, 2026 18:14
@vybe
vybe force-pushed the fix/2711-rail-column-reserved branch from 85111e8 to 4347570 Compare September 22, 2026 18:14
@vybe

vybe commented Sep 22, 2026

Copy link
Copy Markdown
Contributor

merge-train: rebased onto dev and retargeted — mechanical. git rebase --onto origin/dev b2a5fc98d (the pre-squash tip of #2784, merged as c45e565); the four #2711 commits replayed cleanly, nothing changed. Validation: READY, lane B. Advisory, not applied: the route.fullPath watcher that resets railEverHeldRail also disables the #2676 leave animation when navigating from a 1:1 conversation to a rail-less route (one-frame collapse instead of the 300ms shrink) — small blast radius, but the motion spec's comment does not hold for that path.

@vybe vybe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge-train: validated (lane B), rebased onto dev, all checks green on the retargeted run.

@vybe
vybe merged commit 1944348 into dev Sep 22, 2026
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ui PR touches the frontend UI — triggers Playwright e2e tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants