feat(workspace): show memory and skill-sync state in the sidebar - #1279
feat(workspace): show memory and skill-sync state in the sidebar#1279sahrizvi wants to merge 8 commits into
Conversation
The pane names the workspace but says nothing about whether local state has
drifted from it — so there is no moment at which a user learns they have memory
the workspace never received, or skills that have not synced this session.
`/workspace sync` has no discoverability problem to solve if nothing ever
suggests running it.
Adds two lines under the existing name and manage URL:
12 memories · 3 not synced
skills synced 6m ago
Both are status, not affordances — the pane takes no input, and none of the five
sidebar plugins does.
`allowNetwork: false` on the status call is load-bearing, not a micro-optimisation.
The sidebar refreshes every 30s, and the memory-enabled cache is deliberately
positive-only, so a workspace with memory switched OFF is never memoized. Asking
the service on a cache miss would therefore put a request on the wire every 30
seconds for the lifetime of the session, for exactly the workspaces whose answer
is "no". With the network forbidden the counts are shown when the cache already
knows and omitted otherwise — unknown reported as unknown, because treating it as
enabled shows a backlog on a workspace that has memory off, and treating it as
disabled hides a real one.
`skillsSyncedAt` is null for "unknown", not "never": the store is per-process, so
a fresh session has not synced yet even where the on-disk snapshot is current.
The line is hidden rather than rendered as "never synced".
Supporting changes:
- `skill-sync`: `lastSuccessfulSyncAt`. `recentlySynced` answers a boolean against
the poll interval, which cannot say "6 minutes ago" — and a line whose whole job
is to make staleness visible needs the age, not a threshold. Reads the
process-global store, so the plugin realm sees the map the sync writes.
- `memory-sync`: `memoryEnabledCached`, the no-network read of the enablement
cache.
Tests: 8 in the manage suite, 486 across the workspace and plugin suites.
Mutation-checked — ignoring `allowNetwork`, and treating unknown enablement as
enabled, each fail a test.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Both were caught by watching the real TUI, not by the suite. Both are now
covered.
**1. The sidebar's counts never appeared.** `allowNetwork: false` was implemented
as "never touch the network", so the counts could only render once something ELSE
warmed the shared enablement cache — a memory write, or opening `/workspace`. On a
session where neither happened they simply never showed, which defeats the point:
the line exists so drift is noticed without being told to look.
The requirement was a BOUND, not a ban. `memoryEnabledForPoller` resolves over the
network at most once per five minutes when the answer is "no", and not at all once
it is "yes" (the shared cache holds positives). `memoryEnabled` itself is untouched
and stays positive-only, so the WRITE path still picks up a newly enabled
workspace immediately — that is the property that matters for not losing memory,
and a poller being a few minutes behind costs nothing.
The counts now render on first paint. A poller also reports the LOCAL block count
even when memory is off: "how many memories do I have" needs no service, and only
"how many are outstanding" depends on the workspace setting.
**2. The sync toast reported a completely failed sweep as success.** On a project
where the service refused every block, `sync` returned
`{sent: 0, failed: 0, skipped: 0, declined: 19}` — and the message only checked
`sent === 0 && failed === 0`, so it said "Everything is already in the workspace."
while nineteen memories had just been turned away.
`declined` is now counted. It means the service said no — quota, permissions, a
workspace setting — which is a different outcome from having nothing to send, and
reporting it as the latter tells the user their memory arrived when none of it
did. The message moved into `syncMessage` with its own tests.
Simplified while there: a redundant `declined === 0` was removed from the
success guard. The branch above already takes every refused sweep with nothing
sent, so it was a second guard that could never be the one that fires — and a
reader has to prove that before trusting either. Both remaining branches are now
load-bearing; mutating either fails a test, which was not true before.
Tests: 493 across the workspace and plugin suites, 5 new for the toast wording.
Mutation-checked — the poller asking on every tick, the poller never asking at
all (the original hole), a stale negative memo surviving re-enablement, ignoring
a fully refused sweep, and dropping partial refusals each fail a test. Also fixed
an order-dependent assertion: counting ALL requests picked up the fire-and-forget
backfill that `recordApprovedBinding` starts, so it passed alone and failed in a
full run; it now counts only calls to the endpoint the poller uses.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6
…ld bug The call site's comment justified `allowNetwork: false` as "the memory-enabled cache never memoizes a no, so asking on a miss would put a request on the wire every 30 seconds". That was the reasoning behind the bug the previous commit fixed, not behind the code as it now stands: `status` resolves through `memoryEnabledForPoller`, which asks at most once every few minutes on a "no" and never once it is "yes". Left alone, the next reader either trusts the comment and reintroduces the ban, or trusts the code and stops trusting the comments. Says bound, not ban, and points at `Manage.status` where the reasoning lives. Verified end to end against prod while recording the feature: the sidebar now renders "sidebar-demo · 6 memories · 6 not synced" on first paint with a cold cache — the exact case that used to show nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6
…a refusal Third bug from the same recording, spotted by the user in the video: a sweep of six blocks reported "The workspace refused all 5 memories — nothing was sent." Six local, five named. The missing one was a transport failure, and the branch that claims a total refusal fired on `declined > 0 && sent === 0` without looking at `failed` at all. So the sentence was wrong twice. "all" was false — six were attempted, not five — and the failure itself vanished. That is the worst one to drop: a refusal is the service's considered no, while a failure is retryable and can mean the network or the credentials are wrong. The outcome the user could act on was the one the message swallowed. `failed === 0` now guards that branch, so a mixed sweep falls through to the parts list and reports both: "Nothing was sent, 1 failed, 5 refused by the workspace." The list also leads with "Nothing was sent" instead of "Sent 0 memories" — that path is reachable with `sent === 0` now, and a zero reads as a statistic when it is the headline. Same family as the two before it, and the same lesson: the sweep knows exactly what happened, and every one of these bugs was the message throwing part of it away. `partitionPending` is shared by the status line and the sweep precisely so the two can never disagree; that guarantee is worth nothing if the sentence built from the result drops a field. Tests: 496 pass, 3 new for the mixed case. Mutation-checked — reverting the `failed === 0` guard fails two tests, dropping the zero-sent phrasing fails one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6
…oject
Fourth bug from the recordings, and the same shape as the first: the tile read
the binding through `readLocalBinding`, which never touches the network. On a
cold cache — a fresh machine, a new session, anything that has not resolved this
project yet — that returns null, and the tile rendered
Workspace
Not linked — run altimate-code link
about a project that IS linked. It corrected itself only when some unrelated
code path happened to warm the cache, so how long the lie lasted depended on
what else the session did. Caught on screen: the sidebar said "Not linked" at
22s and 38s into a session, then showed the workspace at 63s with nothing having
changed but time.
"Not linked" is the worst state to get wrong, because it is the one that tells
the user to go and run a command. The instruction was the false part.
Two changes. The tile now resolves through `resolveBindingOutcome` rather than
reading the cache, which asks the server on a miss and is already safe to poll:
a confirmed "unbound" is memoized for MISS_TTL_MS and a known binding is trusted
for REVALIDATE_MS, so the worst case is one request per five minutes — the same
bound the counts fix uses. And "unknown" (unreachable, 5xx) now leaves the last
answer standing instead of collapsing to null, so a blip cannot downgrade a
working tile to "Not linked"; `resolveBindingOutcome` exists precisely to keep
those two apart and this caller was throwing the distinction away.
The signal also carries a third state: `undefined` for "the first read has not
returned", `null` for "resolved, genuinely unlinked". Starting at `null` meant
the very first paint asserted a state nothing had checked yet.
Verified from a cold state directory against prod: the tile shows the workspace
and its counts on first paint, and the "Not linked" flash is gone. 496 tests
pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
Code Review SummaryStatus: 10 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (9 files)
Fix these issues in Kilo Cloud Previous Review SummaryCurrent summary above is authoritative. Previous snapshots are kept for context only. Previous reviewThis review did not run. Your provider API key hit its rate limit, so the Reviewed by glm-5.2 · Input: 120.4K · Output: 45.5K · Cached: 2.4M Review guidance: REVIEW.md from base branch |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 8 files
Not reviewed (too large): packages/opencode/src/provider/models-snapshot.ts (~2 lines) - if these are generated or fixture files, add them to ignored paths to exclude them from future reviews.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…king it wait Unlink took up to thirty seconds to show in the tile. The sidebar polls every POLL_MS and nothing published a binding change, so the pane kept naming the workspace long after the server and the local cache had both dropped it. What the user saw was a green "Unlinked from X" toast sitting beside a panel still reporting "X · 3 memories" — the UI contradicting itself, with the stale half looking the more authoritative of the two. The lag is 0-30s depending on where in the cycle the action lands, so it is also inconsistent between attempts, which reads as flakiness rather than latency. `state.ts` now keeps a listener set, notified from the two places that actually change a binding: `forgetBinding` (which every unlink and every server-says- unbound resolve funnels through) and `recordApprovedBinding`, the latter only when `bindingChanged` — a warm cache re-read is not a change, and waking the tile on every resolve would give back what the poll interval buys. A plain listener set rather than an event bus: every writer already funnels through this module, so one hook covers link, unlink and rebind, where a bus would mean threading a dependency through each writer for a single subscriber. The interval stays as the backstop — it is what catches a change made by another process, which no in-process notifier can see. Two details worth their lines. `notifyBindingChanged` is called OUTSIDE `forgetBinding`'s try: a listener is a UI refresh, its failure is not a failed cache drop, and notifying from inside logged a throwing subscriber as "could not drop a binding" — a misleading line about a write that had already succeeded. And the tile's in-flight guard now coalesces instead of dropping: a notification arriving mid-poll used to return early, leaving the tile stale until the next tick, which is precisely the lag this removes. Tests: 501 pass, 5 new. Mutation-checked — never notifying on a drop, notifying on a warm re-record, and rethrowing a listener error each fail a test. One mutation survives knowingly: notifying even when the cache write failed costs a redundant refresh and changes nothing a user can see, so it has no test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Seven findings, all valid. Two of them correct claims I made in earlier commit messages, which is worth saying plainly. **The poller was still hitting the network for enabled workspaces.** I claimed it asks "not at all once it is yes". True for sixty seconds — `memoryEnabled`'s positive TTL — after which an ENABLED workspace went back to the wire on every other tick, a steady drip of `/datamates` for the life of the session. The poller now keeps its own memo of BOTH answers on a five-minute TTL. **A transient network error was rendered as "0 not synced".** `memoryStatus` is deliberately three-way, with a comment saying an unreachable service must not be reported as "this workspace has no memory" — and then the poller path called `memoryEnabled`, which folds error into `false`, memoized that for five minutes, and `memoryCounts` turned it into `unsynced: 0`. A failed request rendered as "your memory is up to date". `unsynced` is now `number | null`; null means "not known", and both call sites print the bare count instead of claiming zero. This is the same defect as the sync toast, one layer down: an error wearing the costume of a clean answer. **The poller memo was keyed by workspace id alone.** Ids are tenant-local, so after an account switch a same-numbered workspace in the new tenant inherited the old tenant's answer for the whole TTL. Keyed by tenant and API URL now, the same scoping the binding cache already uses. **Unlink did not notify when the cache write failed.** I had guarded the notification on a successful drop and called the difference unobservable when a mutation survived. That was wrong: the server-side unlink has already happened, and the resolve path does not depend on this file being rewritten — it drops the revalidation stamp and records a lookup miss, so the next resolve hears "unbound" regardless. Guarding on the write meant the pane kept naming a workspace the project was no longer bound to, in the case where something had already gone wrong. **A rename did not wake the sidebar.** `bindingChanged` comes from `sameBinding`, which compares identity — id, remote, path — because it also gates the memory seed; widening it would re-seed a workspace on every rename. The tile renders the name, so the rename is checked separately. Also: the tile clears counts and the manage URL when the workspace actually changes, so a rebind cannot show the old numbers under the new name (an "unknown" outcome still leaves them standing, rather than blanking a working tile over a blip); and a queued refresh no longer starts after the view is disposed. Tests: 504 pass, 3 new. Mutation-checked — reporting unknown as 0, memoizing an error as "disabled", and ignoring renames each fail a test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6
There was a problem hiding this comment.
2 issues found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/altimate/workspace/manage.ts">
<violation number="1" location="packages/opencode/src/altimate/workspace/manage.ts:216">
P2: When the poller returns `unknown`, this line encodes it only as `unsynced: null`, but both status consumers render that as an ordinary memory count. Expose an explicit unknown state or render `unknown` so an outage is distinguishable from disabled/current, as promised by the status contract.</violation>
</file>
<file name="packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx">
<violation number="1" location="packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx:118">
P2: When credentials switch to a tenant whose workspace reuses the same numeric ID, this comparison treats the binding as unchanged and leaves the old status detail under the new workspace name. Compare the resolved account scope as well as the ID, or clear the detail whenever the binding scope changes.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // and say nothing about sync. | ||
| if (status !== "enabled") { | ||
| const blocks = await MemoryStore.listAll({ directory }) | ||
| return { local: blocks.length, unsynced: status === "disabled" ? 0 : null } |
There was a problem hiding this comment.
P2: When the poller returns unknown, this line encodes it only as unsynced: null, but both status consumers render that as an ordinary memory count. Expose an explicit unknown state or render unknown so an outage is distinguishable from disabled/current, as promised by the status contract.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/altimate/workspace/manage.ts, line 216:
<comment>When the poller returns `unknown`, this line encodes it only as `unsynced: null`, but both status consumers render that as an ordinary memory count. Expose an explicit unknown state or render `unknown` so an outage is distinguishable from disabled/current, as promised by the status contract.</comment>
<file context>
@@ -194,16 +197,24 @@ export async function sync(directory: string): Promise<SyncReport> {
+ // and say nothing about sync.
+ if (status !== "enabled") {
+ const blocks = await MemoryStore.listAll({ directory })
+ return { local: blocks.length, unsynced: status === "disabled" ? 0 : null }
+ }
}
</file context>
| // real change; an "unknown" outcome deliberately leaves everything | ||
| // standing rather than blanking a working tile over a blip. (cubic P2 | ||
| // on #1279.) | ||
| if (binding()?.datamateId !== outcome.binding.datamateId) { |
There was a problem hiding this comment.
P2: When credentials switch to a tenant whose workspace reuses the same numeric ID, this comparison treats the binding as unchanged and leaves the old status detail under the new workspace name. Compare the resolved account scope as well as the ID, or clear the detail whenever the binding scope changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/opencode/src/plugin/tui/altimate/workspace-sidebar.tsx, line 118:
<comment>When credentials switch to a tenant whose workspace reuses the same numeric ID, this comparison treats the binding as unchanged and leaves the old status detail under the new workspace name. Compare the resolved account scope as well as the ID, or clear the detail whenever the binding scope changes.</comment>
<file context>
@@ -107,8 +108,23 @@ function View(props: { api: TuiPluginApi }) {
+ // real change; an "unknown" outcome deliberately leaves everything
+ // standing rather than blanking a working tile over a blip. (cubic P2
+ // on #1279.)
+ if (binding()?.datamateId !== outcome.binding.datamateId) {
+ setDetail(null)
+ setManageUrl(null)
</file context>
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| * is to make staleness visible needs the age, not a threshold. Reads the | ||
| * process-global store, so the TUI plugin realm sees the same map the sync | ||
| * writes (see `STORE_KEY` above). */ | ||
| export function lastSuccessfulSyncAt(directory: string): number | null { |
There was a problem hiding this comment.
WARNING: lastSuccessfulSyncAt reads a per-thread map — the routine skill syncs run in the server worker, so the sidebar "skills synced Xm ago" line never sees them
The TUI (and this sidebar) renders on the main thread (src/cli/cmd/tui.ts:265), while the per-message sync that stamps lastSyncedAt runs inside the server worker (src/session/prompt.ts:393-394, behind the new Worker at tui.ts:171). The file header documents that Symbol.for/globalThis stores do not cross threads ("Threads do NOT share globalThis", lines 114-116; "a bind stamping an in-process map is invisible to the thread that serves the next turn", lines 183-190). So the comment here — "the TUI plugin realm sees the same map the sync writer writes" — only holds for main-thread writers: the bind-time sync (state.ts:651) and the /workspace refresh (manage.ts:125), not the routine POLL_INTERVAL_MS syncs the worker performs per turn.
In a normal TUI session the line therefore either stays hidden forever (no main-thread sync has run) or freezes at link/refresh time and grows without bound while the worker actually re-syncs every 5 minutes — reporting false staleness from the one line whose job is to make staleness visible. Consider a disk-backed signal, e.g. the manifest mtime already read by snapshotFingerprint (lines 193-202), which this file treats as the cross-thread source of truth for exactly this reason.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
| } | ||
| const blocks = await MemoryStore.listAll({ directory }) | ||
| return { local: blocks.length, unsynced: await MemorySync.pendingCount(blocks, binding) } |
There was a problem hiding this comment.
WARNING: The poller still drips a /datamates request every ~60s for an enabled workspace — the pollMemo fix covers the resolver but not this pendingCount call
When memoryEnabledForPoller answers "enabled" from the 5-minute pollMemo, this line falls through to pendingCount, whose gate at memory-sync.ts:736 re-resolves via memoryEnabled → memoryStatus. That path only short-circuits on the 60-second memoryEnabledCache (memory-sync.ts:183-184), with the network fetch at line 186 — and a pollMemo hit does not refresh memoryEnabledCache. For an enabled workspace with at least one local block, the 30s sidebar poll therefore fires one /datamates request per minute for the life of the session (each hit re-arms the 60s cache): the exact "steady drip of /datamates requests" the comment at memory-sync.ts:627-630 says the memoization eliminated, halved in rate but not removed. The manage-suite tests cannot see it because the stub workspace has memory off. Either write the positive back into memoryEnabledCache on a pollMemo "enabled" hit, or pass the resolved status into pendingCount so it skips the re-check.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| * its current payload is the ordinary case, and naming it invites the reader to | ||
| * think something went wrong. */ | ||
| function syncMessage(result: Manage.SyncReport): string { | ||
| if (result.gated) return "Nothing to sync — workspace memory is off for this project." |
There was a problem hiding this comment.
WARNING: gated has several producers — this message tells users their workspace memory is off when the real cause may be a failed local read
sync() returns gated: true not only when the workspace memory toggle is off, but also when the pilot flag is off (manage.ts:173), when there is no binding (manage.ts:175), and — reachable from this menu — when reading the local memory store fails (manage.ts:177-181). In the read-failure case the toast asserts "workspace memory is off for this project" with the success variant (failed/declined are both 0), which is misinformation about the user configuration rather than a report of what happened — the thing this PR rewrote the toast to guarantee. Consider a neutral "Nothing to sync" for the non-toggle causes, or threading a reason through gated.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| export async function memoryEnabledForPoller( | ||
| binding: CachedBinding, | ||
| ): Promise<"enabled" | "disabled" | "unknown"> { | ||
| const cached = memoryEnabledCache.get(binding.datamateId) |
There was a problem hiding this comment.
SUGGESTION: The tenant-unscoped memoryEnabledCache is consulted before the tenant-scoped pollMemo, leaving a 60s cross-tenant window
The account-switch fix scoped pollMemo by tenant/API URL, but this first check keys only by datamateId, so a positive written under the previous tenant within the last 60s is served to the newly switched account — the same inheritance the comment at lines 633-636 describes, bounded to 60s. It is also redundant: memoryStatus makes the identical check at lines 183-184 on the memo-miss path, so removing this early return loses nothing except the unscoped shortcut and lets pollMemo be the authoritative — and correctly scoped — source.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
|
|
||
| function forgetBinding(directory: string, key: { tenant: string; apiUrl: string }): void { | ||
| let dropped = false |
There was a problem hiding this comment.
SUGGESTION: dropped is dead — assigned but never read, with void dropped only silencing the lint
Lines 446/452/471 are leftovers from the earlier if (dropped) guard that the comment itself describes as replaced ("an earlier version of this guarded on dropped"). Notification is now unconditional, so the flag, its assignment, and the void dropped consumption can all be deleted.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // (cubic P2 on #1279; an earlier version of this guarded on `dropped` and I | ||
| // wrongly called the difference unobservable.) | ||
| void dropped | ||
| notifyBindingChanged() |
There was a problem hiding this comment.
SUGGESTION: The "notified even when the write failed" chain re-serves the stale row one refresh later
The justification in the comment (the next resolve asks the server, hears unbound, and the tile updates) holds for exactly one resolve when writeCache persistently fails: that resolve re-stamps lastValidatedAt (line 392) and re-enters forgetBinding (line 394 → this notify), which queues another refresh; that follow-up hits the early return at line 383 (stamp still warm) and returns the stale on-disk row as "bound" — so the tile that just cleared re-names the unlinked workspace until the stamp expires, and the cycle repeats every REVALIDATE_MS. The poll backstop would eventually do the same, so this is an exacerbation on an already-broken path (persistent cache-write failure), not a happy-path regression — but the notify makes the wrong state reappear immediately after the correct one. Stamping validation only after the drop actually persisted (or clearing the stamp on a failed drop) would make the notified refresh chain converge on unbound.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| } | ||
| const b = binding() | ||
| if (!b) { | ||
| setManageUrl(null) |
There was a problem hiding this comment.
SUGGESTION: setManageUrl(null) here is unreachable-with-effect
Every path that reaches if (!b) already has manageUrl() null: the unbound branch cleared it at line 125, and the unknown-with-falsy-binding paths can only have a null manageUrl (it is set only under a truthy binding, and the only transition back to falsy goes through line 125). Harmless, but it is a third null-write on the unbound pass; either this line or the line-125 clear suffices.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| function describeAge(at: number): string { | ||
| const seconds = Math.max(0, Math.round((Date.now() - at) / 1000)) | ||
| if (seconds < 60) return "just now" | ||
| const minutes = Math.round(seconds / 60) |
There was a problem hiding this comment.
SUGGESTION: Double rounding narrows each age label window to ~30s — "1m ago" disappears after 89.5s
seconds is already rounded (line 60); rounding seconds / 60 again means elapsed 89.5s → seconds 90 → minutes 2 → "2m ago", so "1m ago" only covers a 30-second window (and 59.5s already shows "1m ago"). Rounding once with Math.floor gives each label a full 60s window and matches the coarse-but-monotone intent of the doc comment.
| const minutes = Math.round(seconds / 60) | |
| const minutes = Math.floor(seconds / 60) |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| /** Test seam: the poller memo is process-global and would otherwise leak between | ||
| * cases in the same file. */ | ||
| export function resetPollMemoForTests(): void { |
There was a problem hiding this comment.
SUGGESTION: resetPollMemoForTests does not clear memoryEnabledCache, which memoryEnabledForPoller consults first — the new poller tests are order-dependent
memoryEnabledForPoller checks memoryEnabledCache (60s TTL, keyed by bare id) before pollMemo, and this seam clears only pollMemo. memory-sync.test.ts uses the same datamateId: 42 with memory_enabled: true (lines 77, 152, 164) and leaves live positives in the shared process-global cache with no afterAll cleanup — so running it before manage.test.ts in one process (explicit path order; bun runs all test files in a single process) makes "a poller resolves the workspace setting once" fail (memoryEnabledForPoller returns "enabled" with zero /datamates requests) and flips the null-vs-0 assertions. Default alphabetical discovery happens to run manage before memory-sync, which is why it passes today. Clearing both maps here (or memoryEnabledCache in the manage suite beforeEach) makes the tests order-independent.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
#1279 sits on top of #1278, and three commits landed on the base while this branch moved — the symlink guard, the three unlink defects, and the status/sweep gating fixes. GitHub had this PR as CONFLICTING. Both conflicts were additive rather than semantic: each side inserted new code at the same point, and git could not tell they were independent. `state.ts` — the base added `forgetBindingUnscoped` (the no-credentials unlink path) exactly where this branch added the binding-change listener registry. Both kept. The conflict split INSIDE `notifyBindingChanged`, so the closing braces after the marker belonged to only one of the two blocks and the naive resolution left the function unterminated; restored. `manage.test.ts` — the two import blocks are a union, not a choice: `onBindingChanged` and `resetPollMemoForTests` from this branch, `resolveProjectIdentifier` and `pendingCount` from the base. 508 tests pass, typecheck clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012Q51zFUmPg1WwtS5CrGJE6
1 participant
Issue for this PR
Part of #1272 — the discoverability half.
Type of change
What does this PR do?
The workspace pane names the workspace but says nothing about whether local state
has drifted from it. So there is no moment at which a user learns they have memory
the workspace never received, or skills that have not synced this session — which
means
/workspace synchas no discoverability problem to solve, because nothingever suggests running it.
Two lines under the existing name and manage URL:
Both are status, not affordances. The pane takes no input and none of the five
sidebar plugins does, so nothing here becomes clickable.
allowNetwork: falseis load-bearing, not a micro-optimisation. The sidebarrefreshes every 30s, and the memory-enabled cache is deliberately positive-only —
a workspace with memory switched off is never memoized, so switching it on is
picked up immediately. Asking the service on a cache miss would therefore put a
request on the wire every 30 seconds for the lifetime of the session, for exactly
the workspaces whose answer is "no". With the network forbidden, counts show when
the cache already knows and are omitted otherwise.
Unknown is reported as unknown in both places, deliberately. Treating unknown
enablement as enabled shows a backlog on a workspace that has memory off; treating
it as disabled hides a real one. Likewise
skillsSyncedAtis null for "unknown",not "never" — the store is per-process, so a fresh session has not synced yet even
where the on-disk snapshot is current, and the line is hidden rather than rendered
as "never synced".
Supporting changes:
skill-sync.lastSuccessfulSyncAt(recentlySyncedanswers aboolean against the poll interval, which cannot say "6 minutes ago") and
memory-sync.memoryEnabledCached(the no-network read).How did you verify your code works?
8 tests in the manage suite, 486 across the workspace and plugin suites.
Typecheck clean.
Mutation-checked: ignoring
allowNetwork— the polling regression — and treatingunknown enablement as enabled each fail a test.
Screenshots / recordings
Text-only sidebar lines; the shape is in the code block above.
Checklist
Known gaps
existing harness in this repo, so coverage stops at the data
status()returns.name a link. Whichever lands second takes the rebase.
🤖 Generated with Claude Code
Summary by cubic
Shows memory and skill-sync drift in the workspace sidebar so
/workspace syncbecomes discoverable before state goes stale, and fixes four sidebar bugs and the sync toast to report what actually happened.Sidebar
Sync toast
/workspace synctoast reporting fully refused sweeps as "Everything is already in the workspace" —declinedblocks are now reported as refused, and transport failures are kept distinct from refusals.Written for commit 8638e34. Summary will update on new commits.