Code Review

This pull request implements session sorting and pagination by creation time across the frontend and backend. It updates session mapping in the layout and navigation components and adds API support for creation-time cursors. The PR also includes significant structural changes to the generated SDK, such as renaming core classes like Project to Workspace. Feedback was provided regarding the lack of error handling in the cursor decoding logic, which could lead to server-side exceptions if malformed input is provided.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f035f6e722

Actionable comments posted: 3

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/app/src/pages/layout/helpers.ts`:
- Around line 17-23: The sortSessions function no longer uses the now parameter,
so remove the unused argument from its signature and update any callers (notably
sortedRootSessions and latestRootSession) to stop accepting/forwarding now;
change sortSessions to function sortSessions() and adjust sortedRootSessions and
latestRootSession signatures and internal calls to call sortSessions() directly
and stop passing now through the chain, then run TypeScript checks to update any
remaining references to the removed parameter (Session type usages remain
unchanged).

In `@packages/opencode/src/server/instance/experimental.ts`:
- Around line 45-56: Both decoders treat 0 as absent and
decodeCreatedSessionCursor can throw on invalid base64/JSON; change the nil
check from if (!value) to explicit value === undefined || value === null so 0 is
preserved, and wrap the string decode/JSON parse and Zod validation in a
try/catch (or use z.safeParse) inside decodeCreatedSessionCursor to return
undefined on malformed input instead of throwing; in decodeUpdatedSessionCursor
accept numbers (including 0) or convert strings via Number(value) and only
return the numeric cursor if Number.isFinite(cursor), otherwise undefined.
Ensure references to decodeCreatedSessionCursor, decodeUpdatedSessionCursor, and
SessionID.zod are used when locating the code to update.

In `@packages/opencode/test/server/session-list.test.ts`:
- Around line 115-147: Convert the two timing-sensitive tests "keeps default
ordering by last update for existing clients" and "orders root sessions by
creation time when requested" from plain test(...) to the repo's Effect harness
by using testEffect(...) and mark them as live-time with it.live(...);
specifically update the test declarations to testEffect(it.live(...), async ()
=> { ... }) (preserving the existing body that uses tmpdir, Instance.provide,
svc.create, svc.touch and svc.list), and ensure the file imports testEffect and
it from test/lib/effect.ts (replacing the plain test import/usage) so these
tests run under the Effect runtime and are executed as live tests to avoid
flakiness.

Actionable comments posted: 1

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/opencode/test/server/global-session-list.test.ts`:
- Around line 110-130: Replace the timing-sensitive plain test "keeps default
global ordering by last update for existing clients" with the testEffect +
it.live pattern used elsewhere: change the test(...) declaration to
testEffect(() => it.live(async () => { ... })) (move the async body unchanged
inside the it.live callback), ensure you import/use the same testEffect and it
utilities as used in session-list.test.ts, and apply the same migration to the
sibling tests around lines 86-108 so all timing-dependent uses of
setTimeout/Instance.provide/ svc.create / svc.touch / svc.listGlobal run under
it.live.

---

Outside diff comments:
In `@packages/app/src/pages/layout.tsx`:
- Around line 1417-1432: The fallback fetch is using the API default ordering so
latestRootSession can pick the wrong session; update the Promise.all mapping
where globalSDK.client.session.list is called (inside the dirs.map that builds
fetched) to pass sort: "created" in the list call options so the list request
orders by creation time (i.e., change the call used when constructing fetched to
globalSDK.client.session.list({ directory: item, sort: "created" }) so
latestRootSession and openSession operate on sessions ordered by time.created).

Code Review

P1 - ID tiebreaker direction inconsistency

Multiple sorting functions use different ID ordering directions:

• compareSessionRecent: cmp(a.id, b.id) → ascending
• New sortSessions in helpers.ts: b.id.localeCompare(a.id) → descending
• compareSessionCreated: b.id.localeCompare(a.id) → descending

Sessions with equal creation timestamps will appear in reverse ID order compared to sessions with equal update timestamps. Consider standardizing to one direction.

Files affected:

• packages/app/src/pages/layout/helpers.ts:17
• packages/app/src/context/global-sync/session-trim.ts:19

P1 - Missing tiebreaker test coverage

The new sorting tests don't cover identical time.created values. The ID tiebreaker b.id.localeCompare(a.id) is never exercised and could regress.

File: packages/app/src/pages/layout/helpers.test.ts:161

P2 - Confusing test name

session-trim.test.ts test "selects base roots by creation time before returning id-sorted store order" - expected result is creation-time sorted, not ID sorted.

File: packages/app/src/context/global-sync/session-trim.test.ts:23

P2 - Cursor edge case

Numeric cursor with sort=created loses ID tiebreaker, potentially returning duplicates when sessions share creation timestamps.

File: packages/opencode/src/server/instance/experimental.ts:77

P2 - skipToken replacement

Replacing skipToken with noopQuery changes query behavior (suspend vs run). Should document safety rationale.

File: packages/app/src/context/global-sync.tsx:50

P2 - Mixed tiebreaker directions

pawwork-session-source.ts has three-level tiebreaker with inconsistent directions: creation (desc), projectLabel (asc), id (desc).

File: packages/app/src/pages/layout/pawwork-session-source.ts:42

Review Summary

This PR correctly implements creation-time ordering for sessions. The core logic is sound, and tests cover the main functionality well. However, I found several issues that should be addressed before merging:

P0 Issues (blocking)

1. Unrelated refactoring mixed in (packages/app/src/context/global-sync.tsx, bootstrap.ts)

   • Removing import and replacing with plain objects is an unrelated refactor. This should be in a separate commit with its own justification. provides better TypeScript inference for TanStack Query.

2. Tiebreaker behavior change without documentation

   • The ID tiebreaker changed from ascending () to descending (). This affects session ordering when sessions have identical timestamps. This behavior change is not documented in the PR description.

P1 Issues (should fix)

1. Missing tiebreaker tests (helpers.test.ts, pawwork-session-source.test.ts)

   • No tests verify sorting behavior when timestamps are identical. The tiebreaker logic should be explicitly tested.

2. New function without direct tests (session-trim.ts)

   • is added but only indirectly tested through . A direct unit test would make the sorting contract explicit.

3. Legacy cursor edge case (session.ts)

   • When a legacy numeric cursor is passed to , sessions with identical timestamps but different IDs may be skipped. Consider documenting or handling this edge case.

Suggestions

• Add a test case for identical timestamps to document tiebreaker behavior
• Split the refactor into a separate commit
• Document the tiebreaker direction change in the PR description if intentional

Review Summary

This PR correctly implements creation-time ordering for sessions. The core logic is sound, and tests cover the main functionality well. However, I found several issues that should be addressed before merging:

P0 Issues (blocking)

1. Unrelated refactoring mixed in (packages/app/src/context/global-sync.tsx, bootstrap.ts)

   • Removing queryOptions import and replacing with plain objects is an unrelated refactor. This should be in a separate commit with its own justification. queryOptions provides better TypeScript inference for TanStack Query.

2. Tiebreaker behavior change without documentation

   • The ID tiebreaker changed from ascending (a.id.localeCompare(b.id)) to descending (b.id.localeCompare(a.id)). This affects session ordering when sessions have identical timestamps. This behavior change is not documented in the PR description.

P1 Issues (should fix)

1. Missing tiebreaker tests (helpers.test.ts, pawwork-session-source.test.ts)

   • No tests verify sorting behavior when created timestamps are identical. The tiebreaker logic should be explicitly tested.

2. New function without direct tests (session-trim.ts)

   • compareSessionCreated is added but only indirectly tested through trimSessions. A direct unit test would make the sorting contract explicit.

3. Legacy cursor edge case (session.ts)

   • When a legacy numeric cursor is passed to sort=created, sessions with identical timestamps but different IDs may be skipped. Consider documenting or handling this edge case.

Suggestions

• Add a test case for identical created timestamps to document tiebreaker behavior
• Split the queryOptions refactor into a separate commit
• Document the tiebreaker direction change in the PR description if intentional

Actionable comments posted: 2

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@packages/opencode/src/server/instance/experimental.ts`:
- Around line 374-381: The cursor schema currently uses z.coerce.number() which
turns an empty query string ("?cursor=") into 0 and causes
decodeUpdatedSessionCursor() to treat it as a valid timestamp; update the cursor
Zod schema (the cursor field in the sessions query schema) to preprocess empty
strings into undefined (using z.preprocess) before applying
z.coerce.number().or(z.string()).optional() so that empty cursor parameters
become undefined and pagination falls back to the first page; ensure the change
preserves existing handling of numeric and string cursors and still forwards
undefined to decodeUpdatedSessionCursor() when no cursor was provided.

In `@packages/opencode/src/session/session.ts`:
- Around line 776-780: The sort-to-order mapping used in list(...) and
listGlobal(...) is duplicated (the logic that sets sort, order based on
"created" vs default "updated"); extract this into a shared helper function
(e.g., getSessionOrder(sort: string) or computeSessionOrder) that returns the
array of ordering expressions, and replace the inline logic in list and
listGlobal with a call to that helper; ensure the helper references
SessionTable.time_created, SessionTable.time_updated and SessionTable.id so both
callers use the exact same ordering definition and future changes are
centralized.

Review Summary (Round 2)

Previous issues (P0: queryOptions refactor, tiebreaker direction) are now fixed. Tests for tiebreaker scenarios have been added. The implementation is solid.

P2 Issues (nitpicks, non-blocking)

1. Database vs frontend ID ordering inconsistency (session.ts:900-904)

   • Database uses desc(SessionTable.id) for tiebreaker (ID descending: z before a)
   • Frontend uses a.id.localeCompare(b.id) for tiebreaker (ID ascending: a before z)
   • Direct API consumers will see different ordering than frontend UI users
   • Suggestion: Align to asc(SessionTable.id) or document this difference

2. cmp vs localeCompare inconsistency (session-trim.ts)

   • compareSessionCreated uses cmp(a.id, b.id)
   • Other sorting functions use a.id.localeCompare(b.id)
   • Both work for ASCII IDs, but stylistic inconsistency
   • Suggestion: Use one consistently or add comment explaining choice

3. Test doesn't document API tiebreaker behavior (global-session-list.test.ts)

   • Test creates sessions with identical created time but doesn't verify which ID comes first
   • Adding explicit ID ordering assertion would document API behavior

4. Zod preprocess chain fragility (experimental.ts)

   • z.preprocess with z.coerce.number().or(z.string()).optional() works but type inference may be unclear
   • Consider using z.union([...]).optional() for clarity

Verdict

All blocking issues resolved. The above are minor improvements that can be addressed in a follow-up or accepted as-is since they don't affect correctness. Ready to merge after addressing P2 items if desired.