Repository navigation
fix(session): clear time.archived so archived sessions can be restored - #47848
alohaninja wants to merge 1 commit into
Conversation
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
The following comment was made by an LLM, it may be inaccurate: Potential Duplicate Found: PR #43919 - Why it's related: This PR appears to address the same core issues with session unarchiving/restoration functionality. Both PRs deal with fixing the archived timestamp handling and making the unarchive feature functional. The current PR (#47848) may be addressing the same defects that PR #43919 attempted to fix, or it could be a continuation/improvement of that work. I recommend checking PR #43919 to see if it's still open or merged, and whether the current PR supersedes it or if they address different aspects of the same issue. |
|
Superseding — #43919 was the original 76-file attempt and stalled without review. This PR is the backend fix split out of it: the two defects in #43919 will be rebased onto this and rescoped to the web UI only, or closed and reopened fresh. Nothing here depends on it. |
Clearing `time.archived` returned 200 and did nothing, so a session could be
archived but never restored. Two independent defects sat on that path, either one
of which alone makes it unreachable — which is why adding an unarchive button does
not fix it.
`packages/core/src/session/projector.ts` passed `info.time.archived` straight
through, so clearing sent `undefined`. Drizzle omits undefined keys from `.set()`,
meaning "leave this column alone" rather than "set NULL", and the row kept its old
value. Coalescing to null emits an explicit SQL NULL.
The update handler guarded on `ctx.payload.time?.archived !== undefined`. A client
clearing the field sends `{"time":{}}`, because JSON.stringify drops
undefined-valued properties, so the guard short-circuited and setArchived was
never called. Gating on `time !== undefined` treats presence of the object as the
intent signal and an absent `archived` as the clear.
`toRow` in session.ts carried the same undefined-vs-null shape. Not a live bug —
it only reaches `.values()` on the import path, where an insert applies the column
default — but it sits beside an onConflictDoUpdate that reuses the row, so
coalescing keeps the two row builders consistent.
Note `ArchivedTimestamp` is `Schema.Finite` and accepts 0 and negative values for
legacy compatibility, so presence must be tested rather than truthiness.
Two integration tests cover the exact client-sends-{"time":{}} scenario and verify
the cleared value survives a re-read, plus that a title-only PATCH leaves an
existing archived timestamp alone.
Scoped to the backend fix only. TUI archive/unarchive follows separately so this
carries no UX decision. Rebased on current dev. typecheck 30/30 uncached, httpapi
208 pass, httpapi-session 23 pass, core 1097 pass, import 6 pass.
7cf4df9 to
696c05e
Compare
alohaninja
changed the title
github-actions
Bot
added
needs:compliance
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
…Desktop
Two independent bugs each broke unarchiving on their own: the update
handler only acted when time.archived was explicitly set (an
unarchive request arrives as {"time":{}}, so it was skipped), and the
session projector wrote undefined instead of null when archive state
was cleared (Drizzle treats undefined as "leave unchanged" on a
column, so the old archived timestamp stuck).
Ported from upstream anomalyco#6680 (54 reactions): PR anomalyco#43919
(76 files, real documented root-cause engineering) applied to this
fork almost as-is -- excluding its packages/tui/** hunk, since Desktop
doesn't use the TUI -- closed by the automated PR-cleanup bot on
2026-09-21, never reviewed. One small hunk (the same null fix in a
second file) came from the author's own backend-only split-off, PR
anomalyco#47848, still open/unmerged.
Desktop: archived sessions now appear in palette search (grouped
under "Archived"), with a new session.unarchive command and an
Archive/Unarchive toggle in the session menu. Includes the upstream
author's translated locale strings across 62 packages/app/src/i18n/*.ts
files.
Known gaps: no dedicated "Archived" section in the sidebar/home page
(archived rows are filtered out at three separate points in the data
layer before the sidebar sees them -- a real follow-up); unarchive()
is a no-op against V2-protocol servers, matching archive()'s existing
limitation; GET /session still returns archived sessions, filtered
client-side rather than server-side; not manually tested in the
running Desktop app.
Verified against this fork's current dev (not just the implementing
agent's own stale worktree base): bun typecheck clean in app, core,
opencode, desktop. New integration test in
packages/opencode/test/server/httpapi-session.test.ts fails with
either backend fix reverted. New packages/app event-reducer test
covers the session count correctly returning. Full packages/core
suite: 1164 pass (only the documented pre-existing pty flake).
packages/app: 649 pass, 10 pre-existing failures confirmed identical
with or without this change (an unrelated solid-js/Bun SSR issue).
Co-Authored-By: Claude Sonnet 5 <[email protected]>
|
Automated PR Cleanup Thank you for contributing to opencode. Due to the high volume of PRs from users and AI agents, we periodically close older PRs using automated criteria so maintainers can focus review time on the most active and community-supported contributions. This PR was closed because it matched the following cleanup criteria:
PRs created within the last month are not affected by this cleanup. If you believe this PR was closed incorrectly, or if you are still actively working on it, please leave a comment explaining why it should be reopened. A maintainer can review and reopen it if appropriate. Thanks again for taking the time to contribute. |
Issue for this PR
Closes #47849
Type of change
What does this PR do?
Clearing
time.archivedreturned HTTP 200 and did nothing, so a session could be archived but never restored. Two independent defects sit on that path, and either one alone is enough to make it unreachable — which is why adding an "Unarchive" button doesn't fix it.1.
packages/core/src/session/projector.tspassedinfo.time.archivedstraight through, so clearing sentundefined. Drizzle omits undefined keys from.set(), which means "leave this column alone", not "set NULL" — the row kept its previous value. Coalescing tonullemits an explicit SQL NULL.2. The session update handler guarded on
ctx.payload.time?.archived !== undefined. A client clearing the field sends{"time":{}}, becauseJSON.stringifydrops undefined-valued properties, so the guard short-circuited andsetArchivedwas never called. Gating ontime !== undefinedtreats presence of the object as the intent signal and an absentarchivedas the clear.toRowinsession.tscarried the same shape. Not a live bug — it only reaches.values()on the import path, where an insert applies the column default — but it sits beside anonConflictDoUpdatethat reuses the row, so it is coalesced for consistency.Note
ArchivedTimestampisSchema.Finiteand accepts0and negative values for legacy compatibility, so presence must be tested rather than truthiness.Scope is backend only. TUI archive/unarchive follows in a separate PR so this one carries no UX decision, and the web UI is #43919.
How did you verify your code works?
Two integration tests in
httpapi-session.test.ts: archive, clear via{"time":{}}, then re-read to prove the cleared value persisted rather than being masked by the response object; and a title-only PATCH leaving an existing archived timestamp alone.Manually against a local
opencode serve:Also confirmed a session stored with
archived: 0reads back as archived rather than active.Fork PRs can't run the gated jobs without maintainer approval, so I ran their commands locally:
bun turbo typecheck --force— 30/30, 0 cachedpackages/opencodebun run test:httpapi— 208 passhttpapi-session.test.ts— 23 passpackages/core— 1097 passtest/cli/import.test.ts— 6 passScreenshots / recordings
Not a UI change — this PR is backend only.
Checklist