Preserve part ordering in conversations API#2866
Conversation
🦋 Changeset detectedLatest commit: f5ba887 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
TL;DR — Fixes Key changes
Summary ｜ 2 files ｜ 3 commits ｜ base: Inline text parts instead of hoisting
The root cause was a two-phase approach: extract-then-skip. By moving text handling into the loop body and guarding with
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, correct fix. The old code hoisted all text to position 0 via extractText() and left an empty if (kind === 'text') {} body in the parts loop, silently discarding text parts during iteration. The new code emits text parts inline during iteration, preserving interleaved order with data/file parts. The else if (text) fallback correctly handles legacy messages that only have content.text without a parts array.
One minor note: the openapi snapshot diff includes an unrelated fallbackModels addition because the branch is behind main — that will resolve on rebase/merge.
Claude Opus ｜ 𝕏
| "@inkeep/agents-api": patch | ||
| --- | ||
|
|
||
| Perserve Part Ordering in Conversations API |
There was a problem hiding this comment.
Typo: "Perserve" should be "Preserve".
| Perserve Part Ordering in Conversations API | |
| Preserve part ordering in conversations API |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Low
🟡 Minor (1) 🟡
🟡 1) conversations.ts:73-114 Consider adding regression test for interleaved part ordering
Issue: The bug fix correctly preserves interleaved ordering of text and data parts, but there's no dedicated test that verifies this specific behavior. The existing test (conversations.test.ts:475-534) tests text followed by a single data part but doesn't cover the regression scenario: multiple text parts interleaved with data parts maintaining their original order.
Why: Without a regression test, this bug could be reintroduced by future refactoring. The specific code path where text parts are emitted inline during iteration (lines 76-79) would benefit from explicit coverage. However, the fix is straightforward and the logic is clear, making accidental regression less likely.
Fix: Consider adding a test with pattern [text, data, text, data] or [data, text, data] to verify ordering is preserved:
it('should preserve interleaved order of text and data parts', async () => {
// Create message with parts: [text, data, text]
// Verify parts array maintains [text, data, text] order
});Refs:
💭 Consider (2) 💭
💭 1) conversations.ts:112-114 Test coverage for legacy messages without parts array
Issue: The else if (text) fallback handles legacy messages with only content.text (no parts array), but this path lacks explicit test coverage.
Why: Important for backward compatibility, though the code is simple and unlikely to break.
Fix: Add a test with a message that only has content: { text: '...' } (no parts array).
💭 2) conversations.ts:73-79 Test edge case: data part appearing before text
Issue: No test verifies ordering when a data/file part appears before any text part (e.g., [data, text]).
Why: Edge case where the old bug would have incorrectly placed text before data.
Fix: Add test with data-first part ordering pattern.
🕐 Pending Recommendations (1)
- 🟡
.changeset/happy-bobcats-open.md:5Typo: "Perserve" should be "Preserve"
💡 APPROVE WITH SUGGESTIONS
Summary: Clean, correct bugfix! The code change properly preserves interleaved part ordering by emitting text parts inline during iteration instead of hoisting them to position 0. The else if (text) fallback correctly handles legacy messages. Pullfrog's review and summary were accurate. The only actionable item is the typo fix already flagged. Test coverage suggestions are optional improvements — the fix itself is solid and ships well without blocking on tests. 🎉
Discarded (0)
No findings were discarded.
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
1 | 0 | 0 | 0 | 0 | 1 | 0 |
pr-review-tests |
3 | 1 | 2 | 0 | 0 | 0 | 0 |
| Total | 4 | 1 | 2 | 0 | 0 | 1 | 0 |
Bot commits pushed with the default GITHUB_TOKEN (github-actions[bot]) don't trigger workflow runs due to GitHub's infinite loop protection. This left PRs stuck waiting for required checks that never ran. Now uses the inkeep-internal-ci GitHub App token for OpenAPI snapshot commits (ci.yml) and auto-format commits (auto-format.yml), matching the pattern already used in release.yml. App token commits trigger downstream workflows. Fixes: PR #2866 stuck 30min waiting on CI after OpenAPI snapshot commit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Bot commits pushed with the default GITHUB_TOKEN (github-actions[bot]) don't trigger workflow runs due to GitHub's infinite loop protection. This left PRs stuck waiting for required checks that never ran. Now uses the inkeep-internal-ci GitHub App token for OpenAPI snapshot commits (ci.yml) and auto-format commits (auto-format.yml), matching the pattern already used in release.yml. App token commits trigger downstream workflows. Fixes: PR #2866 stuck 30min waiting on CI after OpenAPI snapshot commit. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Ito Test Report ❌18 test cases ran. 1 failed, 17 passed. Overall, 17 of 18 tests passed, confirming that conversation list/detail APIs are largely stable and secure: user scoping and cross-user isolation held, invalid/missing auth and unsupported format inputs were rejected without leakage, Vercel response envelopes and deterministic part ordering were preserved (including under concurrency, pagination extremes, UI/mobile replay), legacy/backward-compat mappings remained intact, malformed data/file parts were handled safely, and injection-like payloads stayed inert. The single significant issue was a medium-severity regression in Vercel message conversion where messages with content.text plus an empty content.parts array return no synthesized text part, creating a content/parts mismatch that can hide visible text in replay clients. ❌ Failed (1)
🟠 Empty parts array with populated content.text does not silently lose visible text
Relevant code:
function extractText(content: MessageContent): string {
if (content.text) return content.text;
if (content.parts) {
return content.parts
.filter((p) => getPartKind(p) === 'text' && p.text)
.map((p) => p.text as string)
.join('');
}
return '';
}
if (msg.content.parts) {
for (const p of msg.content.parts) {
const kind = getPartKind(p);
if (kind === 'text') {
if (p.text) {
parts.push({ type: 'text', text: p.text });
}
} else if (kind === 'data') {
let parsed = p.data;
if (typeof parsed === 'string') {
try {
parsed = JSON.parse(parsed);
} catch {
// keep as string
}
}
const isArtifact =
parsed &&
typeof parsed === 'object' &&
(parsed as Record<string, unknown>).artifactId &&
(parsed as Record<string, unknown>).toolCallId;
parts.push({ type: isArtifact ? 'data-artifact' : 'data-component', data: parsed });
} else if (kind === 'file') {
const url = typeof p.data === 'string' ? p.data : undefined;
if (!url) {
logger.warn({ part: p }, 'File part missing data, skipping');
continue;
}
const meta = p.metadata as Record<string, unknown> | undefined;
const mediaType = typeof meta?.mimeType === 'string' ? meta.mimeType : undefined;
const filename = typeof meta?.filename === 'string' ? meta.filename : undefined;
parts.push({
type: 'file',
url,
...(mediaType && { mediaType }),
...(filename && { filename }),
});
}
}
} else if (text) {
parts.push({ type: 'text', text });
}✅ Passed (17)
Commit: Tell us how we did: Give Ito Feedback |
2 participants
toVercelMessage() was hoisting all text to the front of the parts array and skipping text parts during iteration, breaking the interleaved order of text and data components. This caused conversations loaded from history to show components in a different order than what users saw during streaming. Now iterates content.parts in stored order, falling back to content.text only for legacy messages without parts.