Repository navigation
Conversation
|
The following comment was made by an LLM, it may be inaccurate: |
There was a problem hiding this comment.
The dock commit looks sound: it reads the goal read model, re-reads it on the four goal/todo events once a reader has loaded it, and clears the goal on session delete. The dock rendering follows the state table you describe. The client test and the packages/client and packages/app type checks pass. I could not run the Playwright component tests here (most likely no browser is installed) or open the web app, so before/after screenshots or a short recording of the dock in each phase would help.
Public API: this commit only adds data.session.goal.{list, sync, invalidate} to @opencode/client/solid. The stacked commits from #55744/#54799 add GET /api/session/:sessionID/goal (session.goal.get), POST /api/session/:sessionID/goal (session.goal.update), PUT /api/session/:sessionID/todo (session.todo.replace), the durable events session.goal.updated, session.goal.cleared, session.todo.updated and session.todo.verified, a built-in /goal command, the matching generated client methods and types, and a database migration. All of these are additions and backwards compatible. They should be judged on those PRs.
Concerns:
- The elapsed time keeps counting while a goal is paused, blocked or complete (see inline).
- The title says "todo dock", but nothing in this PR shows todos. The store only re-reads on todo events. Either render the todo list or retitle so it isn't mistaken for the todo UI.
| onCleanup(() => clearInterval(timer)) | ||
| }) | ||
|
|
||
| const elapsed = createMemo(() => { |
There was a problem hiding this comment.
The elapsed time is now - createdAt from a 1s interval, whatever the phase. A paused or complete goal keeps showing a growing time: a goal finished yesterday reads "1440m …". Only show it (or only tick) while the phase is active, or freeze it at the last update time. Also, after an hour it formats as minutes only (e.g. 95m 3s).
|
Both addressed in 1. Elapsed. It now ticks only while the phase is 2. Todos. The dock now renders the todo list under the objective: one row per item with a status icon ( Tests: the component spec is 10/10. The four new cases (freeze while paused, the hour format, the list, the five-row cap) fail against the previous |
89774bc to
df83fa2
Compare
There was a problem hiding this comment.
The elapsed time no longer ticks outside the active phase, and an hour or more now shows as hours and minutes, so that part of the earlier thread is addressed. The value a paused or complete goal starts from is still now - createdAt, though, so a reload still shows a stale, growing time (see inline). The todo list looks fine. The new commit doesn't change the public API. I could not run the component tests or open the web app here, so before/after screenshots of the dock (active with todos, paused, complete) would still help.
| timer = undefined | ||
| } | ||
|
|
||
| const tick = () => { |
There was a problem hiding this comment.
tick() runs once on mount and on each phase change, and always computes Date.now() - createdAt. The value only freezes while this component stays mounted. After a reload or reopening the session, a goal paused or completed yesterday shows ~24h, and more each time. To truly freeze it, the read model needs an end time (e.g. the goal row's last update or phase-change time) to measure to when the phase isn't active. Otherwise, hide the elapsed time outside active.
|
Browser-first user here. I upgraded to 2.1.3 and enabled This dock is the missing piece. Happy to test this branch or a preview build and report behavior across session switching, compaction, cleared lists, completed lists, and revert, with screenshots. Is there a build I can try, or should I run the branch locally? |
|
Thanks — this is the report that matters for this feature: the web session has no todo surface, so the dock is what a browser-first user is missing. Right now the branch is Two things from your report I want to make sure this PR actually covers:
Your test list — session switching, compaction, cleared, completed, revert — is exactly the set I could not exercise from here (no browser in the sandbox). Screenshots of those five would be genuinely useful, and I will fold whatever they show into the PR. |
df83fa2 to
013dc74
Compare
There was a problem hiding this comment.
The rebase broke the generated client: packages/client/src/promise/generated/types.ts was not regenerated against the new dev. It no longer exports McpClientTokenInput/McpClientTokenOutput and DebugHeapDumpInput/DebugHeapDumpOutput, which the generated client.ts still imports, so tsc fails in packages/client and packages/app. It also drops other types dev has, such as PtyTicketConnectToken and SessionMessageAgentSelected. Please regenerate the client from the rebased protocol instead of keeping the conflict-resolved file. This comes from the stacked commits, so #55744 needs the same fix.
The goal dock code is unchanged since the last review, so the elapsed-time threads are still open. The new push doesn't change the public API beyond what was noted before, apart from the accidental type removals above. The client goal test passes. I could not run the component tests or the web app here.
|
Both addressed in Elapsed. The value now freezes at the time the phase last changed, and that time survives a reload. I did not add Still-active hint. Shown when the goal is One caveat on this branch: disarm-on-human-message lives in the driver PR, so here "not armed" means "this process has not armed it" rather than "a human message disarmed it". The hint is accurate but will be rarer than it will be once both land; the Tests: |
There was a problem hiding this comment.
Thread 1 is addressed: the elapsed time only ticks while the goal is active, and long runs show hours and minutes. The reload case (thread 2) is only partly fixed: the frozen time comes from one browser-local entry, so it is right only for the last session this browser saw change phase live (see inline).
Also:
packages/appno longer type-checks: the stub goal model insrc/session/story-model.tsxlacks the newphaseChangedAtandinterruptedfields (TS2739).- The generated
types.tsregression from the last review is still there, sopackages/clientfails to type-check too. - The "still active" notice after an interruption is a new feature in a fix-up commit, and would be easier to review on its own.
The new commit doesn't change the public API: data.session.goal.phaseChange is an addition to the in-app solid data layer. The client goal test passes. I could not run the component tests or the web app here.
| api: ServerApi | ||
| }) { | ||
| const language = useLanguage() | ||
| const [phaseChange, setPhaseChange] = persisted(Persist.global("session-goal-phase"), PhaseChange, { |
There was a problem hiding this comment.
This is one global browser-storage entry (session-goal-phase), written only when this client sees a session.goal.updated event live. Opening another session that has a live record overwrites it. A goal paused in another browser or device, by /goal pause from the TUI, or while the app was closed has no record, and falls back to clock.ended = Date.now() at mount. That is the same reload drift as before. The phase-change time is durable server state: please add it to the goal row and SessionGoalState (e.g. phaseChangedAt, set in the projector when phase changes) and measure to it, instead of reconstructing it per browser.
6e314be to
8dd66dc
Compare
There was a problem hiding this comment.
The new commit only touches the core projection: a todo update without a goal row, or one that doesn't advance the goal revision, is now dropped. That matches how replaceTodo bumps the revision, and the core goal tests pass. It changes no event shape or endpoint.
The dock code is unchanged, so the elapsed-time threads stay open. The build is still broken:
- The generated
packages/client/src/promise/generated/types.tsis still missingMcpClientTokenInput/McpClientTokenOutputandDebugHeapDumpInput/DebugHeapDumpOutput, whichclient.tsimports, sopackages/clientandpackages/appfail to type-check. The commit message leaves regeneration to maintainers, but a PR that changes the protocol needs to ship a consistent generated client: please regenerate it from the rebased protocol (in #55744 too). - The stub goal model in
packages/app/src/session/story-model.tsxstill lacksphaseChangedAtandinterrupted(TS2739).
8dd66dc to
d21a252
Compare
There was a problem hiding this comment.
The generated client is consistent again: packages/client type-checks.
packages/app still doesn't. The stub goal model in src/session/story-model.tsx (around lines 205 and 239) lacks the new phaseChangedAt and interrupted fields, so tsc -b fails with TS2739 and a knock-on TS2322 at line 286. The dock code is unchanged, so the elapsed-time threads stay open. This push doesn't change the public API beyond the earlier review.
|
Fixed in the current head: the four declarations are back in
I could not run |
d21a252 to
9c8c525
Compare
|
The story stub is fixed:
The elapsed-time threads are addressed in the commit below the stub fix — the frozen value is keyed to the phase-change timestamp recorded from Screenshots: I can render the dock states locally but cannot attach files through the API. If you want them on this PR, say so and I will post the images. |
There was a problem hiding this comment.
The story stub now has phaseChangedAt and interrupted, and packages/app type-checks again. The rebuilt stack's session-event.ts matches dev apart from the goal/todo events. The schema manifest test and the client goal test pass. The public API is unchanged since the last review. The dock code is unchanged, so the elapsed-time threads stay open.
9c8c525 to
a90af7a
Compare
There was a problem hiding this comment.
Clean rebase onto the latest dev: the goal code is unchanged and the generated client stays consistent with dev. packages/client and packages/app type-check, and the client, core and server goal tests pass. The public API is unchanged since the last review. The dock code is unchanged, so the elapsed-time threads stay open.
There was a problem hiding this comment.
The elapsed time is now shown only while the goal is active, and the browser-local phase record is gone, so paused, blocked or complete goals no longer show a time that drifts across reloads. This addresses the remaining elapsed-time threads. packages/client and packages/app type-check and the client goal test passes. The public API is unchanged: the commit only removes an in-app data-layer addition from this PR.
|
Took your fallback rather than the The
Why not the durable field: The still-active hint is untouched, since it was a separate decision: Tests: |
838cddc to
335e36e
Compare
There was a problem hiding this comment.
This push only rewords a commit message; the code is identical to the previous review. The current code addresses all three elapsed-time threads: the elapsed time is shown and ticks only while the goal is active, uses hours and minutes past an hour, and no longer relies on a browser-local phase record. The public API is unchanged.
335e36e to
aee5104
Compare
There was a problem hiding this comment.
Rebase onto the latest dev: the dock code is unchanged, and the goal, migration and database tests pass with packages/core, packages/client and packages/app type-checking. The public API is unchanged from earlier reviews.
One cleanup in the stacked core commit: packages/core/schema.json was regenerated without the formatter, so all single-line arrays (["id"] and so on) are expanded, adding ~180 lines of unrelated churn. Please run the formatter on it. Optionally, also regenerate the migration with a fresh timestamp: 20261012112921_modern_red_wolf now chains after 20261024002554_session_allowance. It works because migrations are applied by id, but the order is confusing.
aee5104 to
9d1d8d6
Compare
|
Formatted. I verified the format rather than eyeballing it: the same serializer applied to the previous snapshot reproduces the committed On your optional point: I left
|
9d1d8d6 to
c8e4fb5
Compare
There was a problem hiding this comment.
The rebase brought back a stale column in the stacked core commit: packages/core/schema.json includes session_v2.allowance again, which dev dropped in 20261024175301_drop_session_allowance. As a result, the core test declared schema has no ungenerated migrations fails here. It passes on dev and on the previous head. Please regenerate the snapshot on top of current dev (bun script/migration.ts in packages/core) so it only adds the goal/todo tables (see inline).
Everything else checks out: the generated client and migration.gen.ts are additions only, packages/client and packages/app type-check, and the client, server and core goal tests pass. The public API is unchanged.
| "autoincrement": false, | ||
| "default": "0", | ||
| "generated": null, | ||
| "name": "allowance", |
There was a problem hiding this comment.
This re-adds session_v2.allowance, which dev dropped in 20261024175301_drop_session_allowance. Nothing in the schema code defines it anymore, so script/migration.ts --check reports ungenerated migrations, and test/database-migration.test.ts › "declared schema has no ungenerated migrations" fails. Regenerate the snapshot on top of current dev so only session_goal and session_todo are added.
The durable half of a session goal: a schema contract, session_goal / session_todo tables, four durable events, their projections and the read model. No endpoints, no UI, no round driver. The projections trust the event log's order: an update upserts the row wholesale, and a different goal id clears the previous todo list instead of being skipped. A clear deletes by (session_id, goal_id). The todo list is replaced wholesale in one transaction, and a verification applies verdicts by position, rejecting a verdict whose revision moved since it was taken. Round counting and the revision compare-and-set belong to the endpoints and the driver that use them.
A session.todo.updated event carries the goal revision a verification is judged against, so an event that does not advance it could rewind the goal onto a number an already-stale verdict carries; an event with no goal row wrote a list no verdict could ever match. Both are dropped now. Also drops the unrelated httpapi-codegen change (stripBrand and the Import artifact filter). The generated client stays as committed; regenerating from a clean install is left to the maintainer.
Add the session goal and todo endpoints, the /goal command, and the user's cancel authority. pause commits the durable phase and the process-local disarm separately, then interrupts the running turn with reason user. The round driver, verifier and model tools land in a follow-up.
c8e4fb5 to
4747075
Compare
There was a problem hiding this comment.
The schema snapshot is regenerated on current dev: it only adds the goal/todo tables, and the core migration check passes again. The rebase onto dev's new composer region looks right: the dock sits above the composer, and the composer-docks story has the goal stub. Core, server and client goal tests pass, and packages/client and packages/app type-check. The public API is unchanged.
The description's SVG is a mock-up rather than the real UI. Since I can't run the web app here, real screenshots or a short recording of the dock (active with todos, paused, blocked, complete) would still help.
|
Closing: #59437 landed a |
Issue for this PR
Implements the native session goal request in #27167.
Part of the goal/todo series. Stacked on #55744 (the endpoints) and #54799 (the durable contracts) — review those first; this PR's diff shrinks to the dock once they merge.
Type of change
What does this PR do?
Adds the goal indicator and the todo dock to the composer region.
session.goalstore in the client, fed by the four durable goal/todo events over SSE.SessionGoalDock(shell + state branch),SessionGoalProgress(objective + elapsed) andSessionGoalActions(buttons), rendered above the websearch dock.stoppedmarker whenarmedis false) + pause; paused → resume/edit/clear; blocked →blockedReason.message+ resume/clear; complete → clear.pause/playicons,session.goal.*English source strings, and theme tokens chosen by semantic role.The dock reads the
Goal.Stateread model only.armedis process-local, so a goal that is active but not armed in this process showsstoppedrather than pretending to run.Notes
editis an inline input in the dock (the endpoint needs an objective).expectedRevisionyet — updates are last-write-wins.Visual
How did you verify your code works?
bun typecheckinpackages/appandpackages/client: exit 0.bun run test:components -- component-tests/session-goal-dock.spec.ts: 6 passed.bun test ./test/solid-goal.test.tsinpackages/client: 2 pass.bun run test:unitinpackages/app: 760 pass.Screenshots / recordings
n/a
Checklist
Added by the opencode bot (AI-generated).
Related: #27167, #55744, #54799
Public API: client: adds data.session.goal list/sync/invalidate; the stacked commits add goal/todo HTTP endpoints, four durable events and a /goal command (all additions, backwards compatible)