Skip to content

fix: embeddings/inline inject ux#7963

Merged
qnixsynapse merged 3 commits intomainfrom
fix/first-message-attachment-dialog-and-embedding-ux
Apr 15, 2026
Merged

fix: embeddings/inline inject ux#7963
qnixsynapse merged 3 commits intomainfrom
fix/first-message-attachment-dialog-and-embedding-ux

Conversation

@qnixsynapse
Copy link
Copy Markdown
Contributor

@qnixsynapse qnixsynapse commented Apr 14, 2026

Describe Your Changes

  • When documents were attached in the first message of a thread, the
    embeddings-vs-inline dialog never appeared because
    processNewDocumentAttachments bailed out on missing currentThreadId.
    Additionally, attachments were cleared from the store before the thread
    detail page could transfer them, and embeddings started generating
    eagerly at attach time rather than at send time.

  • processNewDocumentAttachments now only collects the user's mode
    preference via the dialog and stores it as parseMode on the
    attachment. Actual ingestion is always deferred to send time.

  • handleSendMessage (home screen) no longer clears attachments,
    allowing the thread detail page to transfer and process them.

  • processAndSendMessage shows a preview user message immediately so
    the UI doesn't hang while embeddings are generated, with a shimmer
    indicator ("Processing embeddings...") visible during ingestion.

  • Removed the "Uploading attachments" progress bar from ChatInput as
    it was redundant with the new send-time processing flow.

Screencast.From.2026-04-14.12-35-24.mp4

Fixes Issues

  • Closes #
  • Closes #

Self Checklist

  • Added relevant comments, esp in complex areas
  • Updated docs (for bug fixes / features)
  • Created issues for follow-up changes or refactoring needed

When documents were attached in the first message of a thread, the
embeddings-vs-inline dialog never appeared because
processNewDocumentAttachments bailed out on missing currentThreadId.
Additionally, attachments were cleared from the store before the thread
detail page could transfer them, and embeddings started generating
eagerly at attach time rather than at send time.

- processNewDocumentAttachments now only collects the user's mode
  preference via the dialog and stores it as parseMode on the
  attachment. Actual ingestion is always deferred to send time.
- handleSendMessage (home screen) no longer clears attachments,
  allowing the thread detail page to transfer and process them.
- processAndSendMessage shows a preview user message immediately so
  the UI doesn't hang while embeddings are generated, with a shimmer
  indicator ("Processing embeddings...") visible during ingestion.
- Removed the "Uploading attachments" progress bar from ChatInput as
  it was redundant with the new send-time processing flow.
The shimmer was showing for inline documents too. Now it only appears
when at least one unprocessed document has a non-inline parseMode.
- useChatAttachments: 11 tests covering get/set/clear/transfer
  operations, parseMode preservation through transfer, and the
  NEW_THREAD_ATTACHMENT_KEY default.
- useAttachmentIngestionPrompt: 5 tests covering modal open/close,
  choose/cancel resolution, and sequential multi-document prompts.
- processAttachmentsForSend: 11 tests covering inline/embeddings
  routing, parseMode handling, project file override, per-file
  choices, fallback on parse failure, skip-if-processed, mixed
  attachment types, and callback invocations.
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 14, 2026

Barecheck - Code coverage report

Total: 25.36%

Your code coverage diff: 0.65% ▴

Uncovered files and lines
FileLines
web-app/src/containers/ChatInput.tsx1-7, 12, 21-22, 35-41, 43-44, 46-48, 54-65, 71-76, 81, 86-90, 106-148, 152-155, 157-158, 160-162, 165-169, 171-172, 174-182, 184-189, 192-193, 195, 197-198, 200, 202-207, 209, 213-225, 227-229, 234-243, 246-249, 252-256, 258-260, 263-279, 281-284, 286, 289, 292-295, 297-302, 304, 306-314, 317-320, 322-332, 334-335, 338-339, 342-344, 346-357, 359-360, 363, 365-373, 375-376, 378-384, 386-389, 392-394, 397-403, 405-408, 410, 412-416, 418-426, 428, 431, 433-449, 453-455, 457, 459-467, 470-473, 476-479, 481-485, 487, 493-494, 496-501, 503-507, 509-510, 512-516, 519-523, 525-529, 532-536, 539-540, 542-546, 548-549, 551-559, 561, 563-565, 570-574, 576-587, 589, 591-600, 602-605, 609-625, 627-638, 640-650, 652-659, 661-663, 665-671, 673-676, 678-682, 684-689, 691-696, 698-699, 701-704, 706-713, 715-732, 734-744, 746-760, 762-768, 770-784, 787-807, 809-812, 814-824, 826-827, 829-834, 836-837, 839-845, 847-850, 852-856, 858-866, 868-869, 872-877, 879-893, 895-898, 900-911, 913-923, 925-932, 934-937, 939-940, 943, 945-948, 951-952, 955-958, 960-961, 964-967, 969-988, 991-995, 997-998, 1000-1002, 1004-1015, 1017-1033, 1035-1037, 1039-1050, 1052-1055, 1057-1059, 1061-1097, 1100-1104, 1106-1111, 1113-1117, 1119-1120, 1122-1128, 1130-1131, 1133-1134, 1137-1140, 1142-1145, 1148-1159, 1161-1163, 1165-1166, 1168-1169, 1172-1175, 1177-1186, 1188-1197, 1199-1205, 1207-1210, 1212-1214, 1216-1222, 1224-1226, 1228-1233, 1235, 1237-1243, 1245-1254, 1256-1266, 1268-1270, 1272-1275, 1277-1279, 1282-1286, 1288-1290, 1292-1295, 1297-1300, 1303-1305, 1308-1311, 1313-1314, 1316-1320, 1322-1324, 1326, 1328-1330, 1333-1336, 1338-1339, 1341-1342, 1344-1349, 1352-1358, 1360-1364, 1367, 1369-1373, 1376-1383, 1385-1388, 1390-1392, 1394-1405, 1407-1413, 1415-1421, 1424-1427, 1429, 1431, 1433-1439, 1441-1450, 1453-1463, 1465-1476, 1478-1484, 1487-1492, 1494-1499, 1501, 1503-1509, 1511-1525, 1528-1531, 1533-1537, 1539, 1541-1543, 1545-1551, 1553-1560, 1562-1571, 1573-1576, 1578-1581, 1584-1586, 1588, 1590-1625, 1627-1634, 1637-1644, 1646-1656, 1658-1660, 1662-1666, 1668-1671, 1673-1678, 1680-1691, 1694-1697, 1699-1700, 1702-1707, 1710-1718, 1720-1723, 1725-1730, 1732-1736, 1738-1751, 1753-1757, 1760-1763, 1773-1784, 1787-1791, 1793-1799, 1801-1812, 1815-1820, 1822-1831, 1834-1836, 1838-1842, 1844-1846, 1849-1851, 1853-1855, 1857-1863, 1865-1872, 1874-1879, 1881-1887, 1889-1896, 1900, 1930-1943, 1946-1959, 1961-1962, 1964-1985, 1988-2003, 2005-2011, 2013-2019, 2021-2022, 2024-2027, 2029-2036, 2038-2044, 2047-2067, 2070-2075, 2078-2083, 2085, 2087
web-app/src/routes/threads/$threadId.tsx1-3, 5-9, 11-19, 24, 26-27, 31-32, 38-39, 43-58, 60-63, 66-67, 79-86, 88-98, 100, 103-107, 110-111, 114, 117-118, 121-132, 135, 138-142, 146-149, 151-153, 157-165, 168-171, 174-192, 197-199, 202-207, 209-219, 221-229, 231, 236-237, 239-240, 245-256, 259-262, 264-270, 273-274, 277-278, 281-282, 284-286, 288-289, 292-296, 298, 300-307, 309, 312-325, 327-330, 332-346, 348-358, 361-363, 365-370, 376-386, 388, 390-394, 396-399, 401-402, 404-410, 412-426, 429-431, 434-437, 440-453, 455-458, 460-465, 467-474, 477-484, 486-490, 492-496, 498-499, 501-503, 505, 508, 510-517, 519-526, 528, 531-535, 537-542, 545, 548-552, 554, 556-560, 562, 565-569, 571-572, 575, 578-587, 590-593, 595-604, 609-619, 623, 626-640, 643-654, 656-665, 669-671, 674-680, 683, 686-691, 693-701, 703-721, 725-729, 731-738, 741, 743, 745, 747, 749, 751, 753-754, 757-759, 764-770, 773-781, 786, 788-789, 791, 794, 796-798, 800-801, 804-805, 807-813, 816, 819-825, 829-830, 833-838, 840, 842, 845-854, 857-866, 869, 872-875, 878-888, 891-893, 896-902, 905-906, 908-910, 912-915, 917, 920-923, 925-932, 934-938, 940-952, 954-955, 957-959, 961, 963-972, 975-976, 979-997, 999-1009, 1014, 1016-1018, 1020-1021, 1023-1031, 1034-1038, 1041-1045, 1047-1050, 1052-1059, 1061-1064, 1066-1085, 1087-1104, 1106-1109, 1111-1115, 1117-1118, 1120-1125, 1127-1131, 1133-1154, 1156, 1158, 1160-1164, 1166, 1168, 1170-1172, 1174-1177, 1180-1189, 1191

Copy link
Copy Markdown
Contributor

@dinhlongviolin1 dinhlongviolin1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@tokamak-pm
Copy link
Copy Markdown

tokamak-pm bot commented Apr 15, 2026

Code Review — first review

Summary

This PR reworks the document attachment flow so that:

  1. processNewDocumentAttachments only collects the user's inline-vs-embeddings preference (via dialog), storing it as parseMode on the attachment — actual ingestion is deferred to send time.
  2. Attachments are no longer cleared from the store on the home screen before the thread detail page can transfer them.
  3. processAndSendMessage in $threadId.tsx now shows a preview user message immediately with a shimmer indicator while embeddings are generated.
  4. The progress bar from ChatInput is removed as redundant.

This is a significant UX fix for the first-message-of-a-thread flow where documents were attached but the embeddings dialog never appeared.

Files Changed

  • web-app/src/containers/ChatInput.tsx — major simplification
  • web-app/src/routes/threads/$threadId.tsx — preview message + shimmer during embedding
  • 3 new test files for useAttachmentIngestionPrompt, useChatAttachments, and attachmentProcessing

Detailed Review

ChatInput.tsx — Simplification (positive)

  • Removed ~200 lines of eager ingestion logic (updateAttachmentProcessing, estimateTokens, model start/context estimation, processAttachmentsForSend call, and the progress bar UI). This is a significant cleanup that moves responsibility to the right place (send time).
  • The fileIngestProgress state is now unused but still declared as const [, setFileIngestProgress]. Since setFileIngestProgress is still passed somewhere or referenced in a callback, double-check whether it can be fully removed. If nothing references it anymore, clean it up.
  • The comment explaining why attachments are NOT cleared in handleSendMessage is helpful.

ChatInput.tsx — processNewDocumentAttachments simplification

  • The guard changed from if (!docs.length || !currentThreadId) return to just if (!docs.length) return. This is the key fix — previously, on first message, currentThreadId was null so the function bailed out and the dialog never showed. Good fix.
  • The docsNeedingPrompt filter now treats auto the same as prompt (always shows the dialog). Previously auto could skip the dialog if context estimation was available. This simplifies things but changes behavior: users will now always see the dialog for auto mode documents. This seems intentional per the PR description.

$threadId.tsx — Preview message pattern

  • The preview message is created with newUserThreadContent and added to setChatMessages immediately, then removed before the real message is added. This pattern works but has a subtle issue: if the user navigates away from the thread while embeddings are processing, the preview message cleanup (setChatMessages(prev => prev.filter(...))) might not fire correctly since the component could unmount. Consider whether this leaves ghost messages.
  • The messageId is generated earlier (before processing) and reused for both the preview and the final message. This is correct and ensures the same ID is used.
  • setProcessingEmbeddings(false) is correctly in the finally block.
  • The shimmer indicator <Shimmer duration={1}>Processing embeddings...</Shimmer> provides good user feedback.

Potential issue — attachment clearing timing

  • In ChatInput.tsx, clearAttachmentsForThread(attachmentsKey) was removed from the home screen send handler. The comment says "the thread detail page's processAndSendMessage already calls clearAttachmentsForThread after processing is complete." But what about non-document attachments (e.g., images only, no documents)? In the processAndSendMessage function in $threadId.tsx, clearAttachmentsForThread(attachmentsKey) is called unconditionally early in the function (line ~1013 in the diff). So this should be fine for all cases, but please verify this path for image-only messages on the home screen.

Variable shadowing

  • In processNewDocumentAttachments, the inner doc was renamed to d in the filter to avoid shadowing. Good.

Test coverage (positive)

  • Three new test files are added with good coverage:
    • useAttachmentIngestionPrompt.test.ts — tests showPrompt, choose, cancel, sequential prompts
    • useChatAttachments.test.ts — tests getAttachments, setAttachments, clearAttachments, transferAttachments
    • attachmentProcessing.test.ts — tests image/document processing, parseMode routing, fallback behavior, callbacks
  • Tests are well-structured and cover edge cases (cancellation, sequential prompts, mixed attachments, fallback on parse failure).

Dependency array changes

  • The processNewDocumentAttachments dependency array was trimmed significantly. The removed deps (autoInlineContextRatio, activeModels, currentThreadId, getProviderByName, selectedModel, selectedProvider, serviceHub, etc.) are all no longer referenced in the callback. This is correct.
  • In processAndSendMessage, setChatMessages was added to the dependency array. Correct since it is now used in the callback.

Risks

  • The preview-then-remove pattern for messages could lead to visual flicker if the embedding processing is very fast (add preview -> immediately remove -> add real message).
  • Removing the progress bar means users lose granular progress feedback for multi-file uploads. The shimmer is less informative than "2/5 files processed".

Minor

  • The ATTACHMENT_AUTO_INLINE_FALLBACK_BYTES is still in the dependency array of processNewDocumentAttachments but I don't see it used in the new simplified body. Verify if it can be removed.

Recommendation: improve needed

@qnixsynapse qnixsynapse merged commit ece9466 into main Apr 15, 2026
17 checks passed
@qnixsynapse qnixsynapse deleted the fix/first-message-attachment-dialog-and-embedding-ux branch April 15, 2026 03:00
@github-project-automation github-project-automation bot moved this to QA in Jan Apr 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: QA

Development

Successfully merging this pull request may close these issues.

2 participants