-
Notifications
You must be signed in to change notification settings - Fork 610
[feat] Sign-in parity, chat-package re-sync, and the mobile UX pass (12/12) #5691
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6a12c90
fcfcab5
e130e1f
0a690e4
0347952
056d982
95f224e
ba572f5
83f30ea
736be27
cd96de2
c399256
ab33771
e3311aa
c265a8a
e08fde2
ddf3f61
b122e55
6543e1c
94e2565
470aeec
67ddc2d
d152e32
3788be0
e6b4ff5
1185ee2
28d84c3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -89,8 +89,15 @@ scroll. `session_streams.updated_at` is heartbeat-fed last activity. | |
| """apply_windowing must support `updated_at` as the order/cursor attribute. | ||
|
|
||
| The sessions list is ordered by last activity (`updated_at` is heartbeat-fed on | ||
| session_streams). Both ORDER BY and the keyset cursor filters must ride updated_at — | ||
| ordering by updated_at while cursor-filtering on another column paginates incorrectly. | ||
| session_streams). Both ORDER BY and the keyset cursor filters must ride the SAME | ||
| expression — ordering by one column while cursor-filtering on another paginates | ||
| incorrectly. | ||
|
|
||
| That expression is `coalesce(updated_at, created_at)`, not bare `updated_at`: | ||
| `updated_at` is nullable, and a DESC sort puts NULLs first in Postgres, so a session | ||
| that never got a heartbeat would sit above every active one. The full statement | ||
| therefore mentions `created_at` by design — assert on the coalesced expression, not on | ||
| the absence of that column. | ||
|
Comment on lines
+92
to
+100
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Make the coalesced expression the implementation and test contract. The new text requires Also applies to: 149-156 |
||
| """ | ||
|
|
||
| from datetime import datetime, timezone | ||
|
|
@@ -196,10 +203,12 @@ the references *filter*; extend it to hydrate. | |
|
|
||
| **Files** | ||
| - Modify: `api/oss/src/core/sessions/service.py` (`query_sessions` — after fetching streams, | ||
| batch-fetch the latest turn per session via `SessionTurnsDAO` (one query, | ||
| `DISTINCT ON (session_id) ... ORDER BY session_id, turn_index DESC` or the DAO's existing | ||
| latest-turn helper from the turn-index fix `9613e7964e`) and attach `references` (+ | ||
| `trace_id` if cheap) to each row) | ||
| batch-fetch the latest turn per session via `SessionTurnsDAO` and attach `references` (+ | ||
| `trace_id` if cheap) to each row). This needs a NEW batch helper — | ||
| `latest_turn_per_session(session_ids)`, one `DISTINCT ON (session_id) ... ORDER BY | ||
| session_id, turn_index DESC` query. The existing latest-turn helper from the turn-index fix | ||
| `9613e7964e` takes a single session and would make `/sessions/query` an N+1 path; do not use | ||
| it here. Keep the one-call assertion in the service test. | ||
| - Modify: response model — either add `references`/`latest_turn` to the session row model the | ||
| root query returns, or wrap rows in an enriched envelope; follow whichever the track's | ||
| maintainer style suggests (read `SessionsResponse` in `api/oss/src/apis/fastapi/sessions/models.py` first) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Separate T8 from the WP5 dependency row.
Lines 114-118 correctly require WP2, WP4, and an enabled
AGENTA_MOBILE_GATE. Line 384 still includesretire NoMobilePageWrapperin WP5 with only WP1 as a dependency. A reader can apply T8 while the gate is off and remove the only mobile blocker. Update the table to list gate delivery and T8 retirement as separate milestones.