Conversation
Summary
Testing
|
Summary
Testing
|
noa-lucent
left a comment
There was a problem hiding this comment.
Good work removing the Playwright mocks and wiring up real API adapters. The proto wire field mapping is well-structured and the e2e test setup via BrowserContext.request is a clean approach.
Requesting changes for two major issues:
-
Silent error suppression in
expectConversationsListVisible—try/catcharound a Playwright assertion is fragile and masks failures. Uselocator.or()instead. -
Near-duplicate test logic —
conversations-list.spec.tsandconversation-create.spec.tshave nearly identical "create conversation via UI" flows. Deduplicate or remove the redundant test. -
Test illusion on empty agents — Tests silently pass when no agents are available, hiding the fact that the core flow was never exercised. Use
test.skip()or fail explicitly.
Additional minor/nit feedback on naming (mapConversationResponse), duplicated wire types (AgentWire in 3 files), removed test coverage (redirect/navigation), and silent filtering in listAgents.
Summary
Testing
|
noa-lucent
left a comment
There was a problem hiding this comment.
All 7 prior review items are resolved and verified in code:
- ✅
try/catch→locator.or()inexpectConversationsListVisible - ✅ Duplicate "create via UI" test removed from
conversations-list.spec.ts - ✅
listAgentsnow throws on invalid payloads instead of silently filtering - ✅
mapConversationResponse→validateChatPayload - ✅
chat-api.tsadded to devspace sync readiness check - ✅
redirects root to /conversationsandnavigates to conversation detailtests restored - ✅
test.skip()for empty agents inconversation-create.spec.ts - ✅
AgentWireextracted tosrc/api/types/wire/agents.tsand shared across all three consumers
One nit left on dead code after test.skip() — non-blocking.
LGTM.
Summary
Testing
|
Summary
Testing
|
|
Summary:
Tests:
|
noa-lucent
left a comment
There was a problem hiding this comment.
The full codebase rename from conversation* → chat* is clean and thorough. Types now match proto exactly, all adapters are removed, and the API modules are clean pass-throughs. The socket boundary correctly preserves server-defined wire event names while translating to internal chat* naming. Mock plugin, mock data, components, hooks, pages, routes, storage keys, and test IDs are all consistently renamed. The UpdateChatStatus RPC was properly removed. Backward-compat route redirects are in place.
Requesting changes for one issue:
- [major]
chat-exchange.spec.ts— User B is not a participant in the chat.createChatuses a random UUID, not User B's identity. The test doesn't exercise a real two-participant exchange. If the server enforces access control, the test will fail.
Additionally:
- [minor]
multi-user-fixtures.ts— duplicate fixture setup should be extracted into a helper. - [nit]
chat-create.spec.ts— deadifblock aftertest.skip()(carried over from prior review).
|
Summary:
Tests:
|
noa-lucent
left a comment
There was a problem hiding this comment.
All three items verified in code:
- ✅ User B participant —
resolveUserIdreads the OIDCsubclaim from session storage with JWT decode and email fallbacks, then passes it tocreateChat. User B is now a proper participant. The resolution logic is well-layered and correctly defensive as boundary code. - ✅ Duplicate fixture logic —
createUserContext(browser, email)extracted; both fixtures are now clean three-liners. - ✅ Dead code —
ifblock aftertest.skip()removed.
LGTM — clean rename, proto-aligned types, proper boundary/internal separation, and solid e2e coverage.
|
Summary:
Tests:
|
Picks up fix(oidc): remove access token aud check (agynio/gateway#96). This unblocks real-API e2e tests in agynio/chat-app#38.
Picks up fix(oidc): remove access token aud check (agynio/gateway#96). This unblocks real-API e2e tests in agynio/chat-app#38.
|
Summary:
Tests:
|
Chat v0.1.0 required x-tenant-id gRPC metadata which the gateway never set and was a dead field (extracted but unused). Chat v0.2.0 (PR #17) removes this requirement, keeping only x-identity-id and x-identity-type. Unblocks e2e tests in agynio/chat-app#38.
|
Summary:
Tests:
|
This reverts commit 4880da1.
|
Summary:
Tests:
|
Ready for re-reviewChanges since last review:
The organizations gap (frontend has no org context → |
noa-lucent
left a comment
There was a problem hiding this comment.
The env-var revert, chat-create.spec.ts removal (blocked by org context gap), and signature change from BrowserContext to Page (to enable getAccessToken) are reasonable.
The CreateAgent/DeleteAgent additions in the vite mock plugin are clean.
Requesting changes for three issues:
- [major]
devspace.yamlstill references deletedchat-create.spec.ts— the file sync readiness check will always time out in CI. - [major] Duplicate session storage iteration —
readOidcSessionandgetAccessTokenboth iterate and parse the sameoidc.user:session storage blob independently. Consolidate into a single helper. - [major] Debug
console.logstatements — three log lines leak access tokens, session storage keys, and response bodies into test output. Remove them.
|
Summary:
Tests:
|
noa-lucent
left a comment
There was a problem hiding this comment.
All three items verified in code:
- ✅
devspace.yaml— bothchat-create.spec.tsreferences removed from readiness check - ✅
readOidcSessionconsolidated — single helper now returns{ profileSub, profileEmail, idToken, accessToken }; duplicategetAccessTokendeleted;postConnectconsumessession.accessTokendirectly - ✅ All debug
console.logstatements removed; error body included in thrown Error message instead
LGTM.
Chat chart v0.2.1 (agynio/chat#18) defaults threadsAddress to threads:50051, following the gateway chart convention where inter-service gRPC addresses use in-cluster DNS defaults. Remove the v0.1.0-specific service.port and threads.address overrides — the chart defaults handle both correctly. This also removes the x-tenant-id requirement (dropped in chat v0.2.0, agynio/chat#17), unblocking real-API e2e tests in agynio/chat-app#38.
* chore: bump chat chart version to 0.2.0 Chat v0.1.0 required x-tenant-id gRPC metadata which the gateway never set and was a dead field (extracted but unused). Chat v0.2.0 (PR #17) removes this requirement, keeping only x-identity-id and x-identity-type. Unblocks e2e tests in agynio/chat-app#38. * chore: bump chat chart version to 0.2.1 Chat chart v0.2.1 (agynio/chat#18) defaults threadsAddress to threads:50051, following the gateway chart convention where inter-service gRPC addresses use in-cluster DNS defaults. Remove the v0.1.0-specific service.port and threads.address overrides — the chart defaults handle both correctly. This also removes the x-tenant-id requirement (dropped in chat v0.2.0, agynio/chat#17), unblocking real-API e2e tests in agynio/chat-app#38. * ci: re-trigger after chat v0.2.1 chart published * fix: drop v prefix from chat image tag resolution The chat Docker workflow uses docker/metadata-action with semver pattern={{version}} which strips the v prefix from tags. Tag v0.2.1 produces image ghcr.io/agynio/chat:0.2.1 (no v). Align with gateway convention: use chart_version directly instead of format("v%s", chart_version). * chore: bump chat chart to 0.2.2 (numeric UID fix) * fix: revert accidental notifications_chart_version bump --------- Co-authored-by: Emerson Gray <emerson.gray@agyn.io>
Summary
Testing
|
Summary
Testing
|
Summary
Testing
|
Summary
Testing
|
Summary
Testing
|
CI Green — 6/6 e2e passing ✅Changes since last review approval:
Requesting re-review. |
noa-lucent
left a comment
There was a problem hiding this comment.
Changes since last approval look good:
resolveIdentityIdvia/api/me— cleaner and authoritative compared to OIDCsubparsing. Proper boundary validation on the response.- Proto3 zero-value normalization —
normalizeChat/normalizeMessageare correctly placed at the API module boundary, ensuring internal code always sees non-optional arrays. Clean pattern. createChatself-participant fallback — reasonable default behavior for single-user test flows.- Timeout increases — pragmatic for cold k3d clusters.
waitForResponseremoval fromchats-list— correct, since client-side navigation uses cached data.
Two minor items noted (non-blocking):
readOidcSessionstill extractsprofileSub,profileEmail,idToken— all dead sinceresolveUserIdwas deleted. Trim to justaccessToken.waitForResponseafterawait goto()is a race — start listening before navigation or remove if thetoBeVisibleassertion is the real gate.
LGTM.
| profileSub: string | null; | ||
| profileEmail: string | null; | ||
| idToken: string | null; | ||
| accessToken: string | null; |
There was a problem hiding this comment.
[minor] profileSub, profileEmail, and idToken are no longer consumed by any caller — the only consumer was the deleted resolveUserId. Now both postConnect and resolveIdentityId only read accessToken.
Trim the type and page.evaluate to only extract what's needed:
type OidcStorageSnapshot = {
accessToken: string | null;
};This removes ~10 lines of dead parsing logic. If these fields are needed later, they can be re-added.
There was a problem hiding this comment.
Addressed — trimmed readOidcSession to only extract accessToken. Dead fields removed.
| await page.waitForResponse( | ||
| (resp) => resp.url().includes('GetMessages') && resp.status() === 200, | ||
| { timeout: 15000 }, | ||
| ); |
There was a problem hiding this comment.
[minor] waitForResponse is called after await page.goto() returns (which waits for the load event by default). If the GetMessages response completes during navigation — before waitForResponse starts listening — it will be missed, and the call will either time out or race.
The safe pattern is to start listening before triggering navigation:
const messagesLoaded = page.waitForResponse(
(resp) => resp.url().includes('GetMessages') && resp.status() === 200,
{ timeout: 15000 },
);
await page.goto(`/chats/${chatId}`);
await messagesLoaded;Same applies in chat-exchange.spec.ts (4 occurrences). That said, the toBeVisible assertion with 15s timeout is the actual correctness gate here, so this is non-blocking — but the waitForResponse calls as written are misleading since they may silently no-op.
There was a problem hiding this comment.
Addressed — waitForResponse now starts before page.goto() in both chat-detail.spec.ts and chat-exchange.spec.ts.
Summary
Testing
|
Summary
Testing
Refs #36