Fix/Paste large text silently converted to @file#2172
Closed
idling11 wants to merge 2 commits into
Closed
Conversation
Contributor
There was a problem hiding this comment.
Code Review
This pull request defers the consolidation of large pasted text to submit time instead of doing it immediately upon pasting, allowing users to view and edit the pasted content in the composer. A review comment points out that setting a status message synchronously during consolidation is redundant and blocks the UI thread, potentially causing duplicate toast notifications.
Comment on lines
+4217
to
+4219
| self.status_message = Some("Consolidating large paste to file…".to_string()); | ||
| self.needs_redraw = true; | ||
|
|
Contributor
There was a problem hiding this comment.
Issue: Redundant Status Message & Synchronous UI Blocking
- UI Thread Blocking:
consolidate_large_inputruns synchronously on the main UI thread. Because the event loop is blocked during the execution of this function (specifically during the synchronousstd::fs::writecall), the TUI cannot perform a redraw to display the"Consolidating large paste to file…"status message while the consolidation is actually in progress. - Duplicate/Redundant Toasts: Once the function finishes and the event handler returns, the UI finally redraws. At this point, both the
"Consolidating..."status message (which gets synced to toasts viasync_status_message_to_toasts) and the"Large paste consolidated — sent as @mention"toast (pushed at the end of the function) will be displayed simultaneously. This results in redundant and confusing toast notifications for the user.
Recommendation
Remove the redundant self.status_message assignment. The final toast "Large paste consolidated — sent as @mention" is sufficient to inform the user of the completed action.
Owner
|
Merged manually — paste consolidation deferral landed on main. Thanks @idling11! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Defer paste consolidation to submit time so pasted text remains
visible and editable in the composer.
Closes: #2159
Problem
insert_paste_textcalledconsolidate_large_input_if_oversizedimmediately after paste, silently replacing the composer content
with an
@.deepseek/pastes/...file reference. Users pastinglarge text saw their content disappear with no chance to preview
or edit it.
Fix
Remove the paste-time consolidation call. The submit path (Enter)
still enforces the same safety cap, so oversized input is still
protected — but now the user sees their full text until they
choose to submit.
Also keep the "Consolidating large paste to file…" status message
in
consolidate_large_inputso the submit-time consolidation isvisible.
Files Changed
crates/tui/src/tui/app.rsTest
Updated
paste_consolidates_oversized_text_into_paste_file_visibly→paste_oversized_text_is_preserved_in_composer_not_consolidated.All 27 paste-related tests pass.
Greptile Summary
This PR defers large-paste consolidation from paste time to submit time, so pasted text stays visible and editable in the composer rather than being silently replaced with an
@filemention. A status message is now emitted insideconsolidate_large_inputto make the submit-time swap visible to users.insert_paste_textno longer callsconsolidate_large_input_if_oversized; the submit path insubmit_inputretains the safety-net call, so oversized input is still capped before being sent.paste_consolidates_oversized_text_into_paste_file_visiblyis replaced bypaste_oversized_text_is_preserved_in_composer_not_consolidated, correctly asserting the new behaviour; two inline comments that described the old dual-path flow were not updated and now describe behaviour that no longer exists.Confidence Score: 4/5
Safe to merge — the submit-time safety net remains intact and the test suite was updated to match the new paste behaviour.
The logic change is straightforward and well-tested. Two inline comments that described the old dual-path consolidation flow were not updated, leaving inaccurate descriptions of how
insert_paste_textandconsolidate_large_input_if_oversizedinteract — worth fixing before this becomes a maintenance trap, but not a correctness issue.The stale comments in
submit_input(around theconsolidate_large_input_if_oversizedcall) and the doc comment onconsolidate_large_input_if_oversizeditself both describe the removed paste-time path and should be updated.Important Files Changed
insert_paste_textand adds a status message toconsolidate_large_input; two inline comments that described the old dual-path behaviour were not updated and now mislead readers.Sequence Diagram
sequenceDiagram participant User participant insert_paste_text participant submit_input participant consolidate_large_input Note over User,consolidate_large_input: Before this PR (old behaviour) User->>insert_paste_text: paste large text insert_paste_text->>consolidate_large_input: consolidate_large_input_if_oversized() consolidate_large_input-->>insert_paste_text: "composer replaced with @mention" insert_paste_text-->>User: "sees @mention (content gone)" Note over User,consolidate_large_input: After this PR (new behaviour) User->>insert_paste_text: paste large text insert_paste_text-->>User: full text visible in composer (no consolidation) User->>submit_input: press Enter submit_input->>consolidate_large_input: consolidate_large_input_if_oversized() consolidate_large_input-->>submit_input: "status_message set, input replaced with @mention" submit_input-->>User: "@mention submitted, composer cleared"Comments Outside Diff (2)
crates/tui/src/tui/app.rs, line 4093-4098 (link)insert_paste_text, so "Bracketed pastes hit the consolidation ininsert_paste_textfirst" is now factually wrong and will mislead future readers.crates/tui/src/tui/app.rs, line 4200-4205 (link)consolidate_large_input_if_oversizedstill refers to "the paste-insert path (visible-before-submit)" routing through it. That path was removed by this PR, so the "enforced exactly once even when both paths fire" rationale no longer applies and will confuse future readers.Reviews (1): Last reviewed commit: "fix: defer paste consolidation to submit..." | Re-trigger Greptile