Skip to content

Commit 742ea94

Browse files
krishhggclaude
andcommitted
Merge #70's fix for a diff read under way into the Timeline commit branch
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
2 parents b86ef2e + e965922 commit 742ea94

2 files changed

Lines changed: 71 additions & 12 deletions

File tree

‎docs/pull-request-panel.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -101,15 +101,15 @@ Where the detail's `capabilities.diff` is true, the details have a third tab, Co
101101
| --- | --- | --- |
102102
| Reading | The tab mounts the first time it shows and stays mounted while the panel shows the pull request (PullRequestDetailPanel.tsx:2784-2798). Its query reads the first slice, and reads it again when the tab mounts with an answer 60 seconds old. A sentinel 240 pixels under the files reads the next slice once it comes into view (PullRequestCodeTab.tsx:607-629). | The first slice is read the first time the Code shows after the details open, and read again then where it came a minute or more ago or failed. The next slice is read once a frame shows the end of the files within two pages of the view. One slice is read at a time. |
103103
| Reading again | `codeRefreshToken` (PullRequestDetailPanel.tsx:832) moves with each refresh number, a person's Refresh and a detail whose `updatedAt` changed (836-847), and the tab reads the first slice again | The same three read the first slice again. After `r`, the read waits until the read that sent `pullRequests.invalidate` is done, so T3 forgets the diff it held. The files read before show until the new ones come, and a file keeps its fold. A read again that fails keeps them under `May be out of date: t3term couldn't read the diff again.` and the reason. |
104-
| Commits | Where the change has commits, the toolbar's menu says `All commits` or the picked commit's headline. It lists `All commits`, then the commits newest first by when each was committed, 10 at a time with `Show more (13 left)`, each with its id's first seven characters (PullRequestCodeTab.tsx:683-692, 1063-1119). Picking one reads its own diff from its first slice, folds the files and closes the list off the diff (268-277), and places no thread on its lines, since a commit counts its lines apart from the change (454-458). A commit the change no longer lists goes back to All commits (1050-1062) | `› All commits` above the count, or `› Read the schema first · abc1234`. Enter or a click opens the list under it: `✓ All commits`, then each commit's headline cut to its row with the seven characters at the end, ✓ before the diff shown, and `Show more (13 left)`. ↑ and ↓ choose an entry, and Enter or a click shows its diff, reading it from its first slice, and closes the list with its row chosen. Commits at the same instant keep T3's order, and one whose time t3term can't read goes last. An empty headline reads `Untitled commit`, as on the Timeline. While a commit's diff shows, every thread is in the list off the diff. A read of the conversation that no longer lists the commit reads the whole change again. |
104+
| Commits | Where the change has commits, the toolbar's menu says `All commits` or the picked commit's headline. It lists `All commits`, then the commits newest first by when each was committed, 10 at a time with `Show more (13 left)`, each with its id's first seven characters (PullRequestCodeTab.tsx:683-692, 1063-1119). Picking one reads its own diff from its first slice, folds the files and closes the list off the diff (268-277), and places no thread on its lines, since a commit counts its lines apart from the change (454-458). A commit the change no longer lists goes back to All commits (1050-1062). The query of a diff picked before stays alive for 5 minutes once nothing shows it, so a read of it under way runs on and its answer is kept (packages/client-runtime/src/state/runtime.ts:534 and 590) | `› All commits` above the count, or `› Read the schema first · abc1234`. Enter or a click opens the list under it: `✓ All commits`, then each commit's headline cut to its row with the seven characters at the end, ✓ before the diff shown, and `Show more (13 left)`. ↑ and ↓ choose an entry, and Enter or a click shows its diff, reading it from its first slice, and closes the list with its row chosen. Commits at the same instant keep T3's order, and one whose time t3term can't read goes last. An empty headline reads `Untitled commit`, as on the Timeline. While a commit's diff shows, every thread is in the list off the diff. A read of the conversation that no longer lists the commit reads the whole change again. Picking another diff stops the read of the one before, whose answer t3term would drop. |
105105
| Count | `12 files`, with `+` while more slices come (PullRequestCodeTab.tsx:1124-1125), and a note when the host withheld part of the diff (1183-1195) | `12 files+`, then `The host withheld part of this diff, such as a binary file or a change too large to show.` in yellow where a slice says the host left something out or has text the parser found no file in. |
106106
| Files | Folded at first, as the `diffFilesCollapsed` setting is true until changed (pullRequestDiff.logic.ts:36-43, PullRequestCodeTab.tsx:230-232). The header has a chevron, an icon for how the file changed, the path, both paths of a rename, and `+3 -1` unless both are 0, from the host's own counts where the hunks it sent count none (822-823) | `› M src/lib.rs +3 -1`, with A, D, R or M in green, red, blue or yellow for an added, deleted, renamed or changed file. The path wraps under itself. Enter or a click unfolds it, `⌄`, and folds it again. A path one slice holds twice, as git writes a file whose type changed, shows once as its last part, as the desktop's `orderDiffFiles` keeps one file for each path (pullRequestFileOrder.logic.ts:152-153). |
107107
| Order | `orderDiffFiles` (pullRequestFileOrder.logic.ts:145-197) orders each slice on its own, so a later slice never moves the files above it (PullRequestCodeTab.tsx:404-411). Source comes first, each file after the changed files it imports. A quoted specifier after `from`, `import` or `require(` in any line of a file's hunks names a changed file by its path, with `./` and `../` resolved and a script's extension dropped, or by a name only one source file has. Where a cycle leaves no file ready, the lowest path goes next. Then the tests (`.test.`, `.spec.`, or a `test`, `tests` or `__tests__` directory), each after the source file whose name it has, then lockfiles, snapshots, minified files and files under `dist`, `build`, `vendor`, `__generated__` or `__snapshots__`, by path. Paths compare with `localeCompare` | The same order, worked out once when a slice comes. The diff library the desktop uses puts each unchanged line of a hunk on both sides, so an import in an unchanged line counts too (@pierre/diffs 1.5.2, dist/utils/parsePatchFiles.js:208-222), and t3term counts it. Paths compare as the root collation does for ASCII: punctuation, then digits, then letters with case ignored, then lowercase before uppercase. |
108108
| Lines | The stacked view: each line with its number, and `N unmodified lines` between hunks (apps/web/src/lib/diffRendering.ts:82) | Each line's number, the new one or the old for a removed line, then `+`, `-` or a space, in green, red or the text's color. A line that wraps leaves the number and the mark blank. Tabs stop every 4 columns, control characters go, and a line past 2,000 characters is cut with `…`. `⋯ 9 unmodified lines` comes before each hunk that doesn't start at the file's first line. A binary file, a file whose changes the host withheld and a rename that changed no line each say so. So does a file past t3term's 200,000 lines of a slice, or with lines t3term couldn't read, such as a hunk shorter or longer than its header counts. Only the files unfolded are laid out, each once for each width, so folding one or the minute changing doesn't lay out the others again. |
109109
| Review threads | Each thread on a file the tab shows, with a line inside one of its hunks, shows under that line (PullRequestCodeTab.tsx:452-472, pullRequestDiff.logic.ts:12-22). On a row with both numbers, a thread on the old side comes before one on the new (@pierre/diffs 1.5.2, dist/renderers/DiffHunksRenderer.js:1051-1075). `ReviewThreadCard` starts open unless resolved. Its header says `Open` or `Resolved`, the count of comments and `outdated`, and under it come the comments, `Load more comments` and buttons to resolve, reply, edit and react (PullRequestReviewAnnotation.tsx:89-358). `Load more comments` reads the next page through `pullRequests.threadComments`, reads `Loading...` until it comes, and adds its comments, keeping the first of each id (139-207, 305-313, pullRequestDetail.logic.ts:170-184). A failure shows a toast (PullRequestCodeTab.tsx:928-940) | `│ ⌄ Open · 3 comments · outdated` behind a bar, with `Resolved` in green. It starts open unless resolved, and Enter or a click on its first row opens and closes it. Open, it shows each comment as the Summary does, with a blank row between them, then `Load more comments` where the host has another page. Enter or a click reads that page, as [pull request reads](pull-request-reads.md#a-review-threads-remarks) says, and the row reads `Loading more comments…` until it comes. Its comments follow the thread's, each id once. A failed read shows `t3term couldn't read more comments.` and the reason above the button, which reads the same page again. Where the host has no page to offer but counts more, `2 more comments on the host.` shows instead. Only the threads of unfolded files show, and a thread stays open or closed across reads, by its id. t3term places a thread only on a line it read, so one on a line of a hunk shorter than its header counts goes to the list off the diff. The desktop checks the hunk's range alone. |
110110
| Off the diff | Above the code, closed at first: `Conversations not on the current diff`, or `on the diff loaded so far` while slices are left, and the count. Open, it lists the threads by path, each path once, with `Line 42` before each thread, at most 256 pixels high (1376-1440) | `› 2 conversations not on the current diff`, or `on the diff read so far`, above the files. Enter or a click opens it. Open, it lists each path once, then `Line 42` and the thread, for each thread on it. |
111111
| Waiting and failing | `Loading pull request diff...`, `Loading more files...` after the files, the first slice's error alone, and `The rest of this diff could not be loaded.` with Retry for a later slice (760-776) | `Loading pull request diff…` and `Loading more files…`. The first slice's failure shows alone, and `r` or showing the Code after the details open again reads it again. A later slice's failure shows `t3term couldn't read the rest of this diff.`, the reason and `↻ Retry`, which ↑ and ↓ choose. |
112-
| Empty | `This pull request has no file changes.`, or `This commit has no file changes.` for a commit's own diff (1366-1372) | The same, or `t3term couldn't split this patch into files.` where the patch has text but no file the parser could read. |
112+
| Empty | `This pull request has no file changes.`, or `This commit has no file changes.` for a commit's own diff (1366-1372), returned before the list off the diff (1376), so no review thread shows | The same, or `t3term couldn't split this patch into files.` where the patch has text but no file the parser could read. No review thread shows either, until a diff with files does. |
113113

114114
## Next slices
115115

‎src/tui/mod.rs‎

Lines changed: 69 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,8 @@ type WatchSetup = Box<dyn Fn(&str) -> mpsc::UnboundedReceiver<WatchEvent>>;
9393
/// as an action with the ask's ticket.
9494
type ReadPull = Box<dyn Fn(Reference, Ask)>;
9595
/// Reads a slice of a pull request's diff in the background as `DiffAsk` says, and sends the
96-
/// answer as an action with the ask's ticket.
96+
/// answer as an action with the ask's ticket. The details want only the answer to their newest
97+
/// ask, so a read stops the one under way before it.
9798
type ReadDiff = Box<dyn Fn(Reference, DiffAsk)>;
9899
/// Reads the next page of a review thread's remarks in the background as `CommentsAsk` says,
99100
/// and sends the answer as an action with the ask's ticket.
@@ -1023,15 +1024,23 @@ impl App {
10231024
read_pull_request(&client, &reference, &ask, send).await;
10241025
});
10251026
}),
1026-
read_diff: Box::new(move |reference, ask: DiffAsk| {
1027-
let (client, answers) = (differ.clone(), diffs.clone());
1028-
tokio::spawn(async move {
1029-
let (cursor, commit) = (ask.cursor.as_deref(), ask.commit.as_deref());
1030-
let answer = read_diff(&client, &reference, cursor, commit).await;
1031-
let ticket = ask.ticket;
1032-
let _ = answers.send(ActionResult::PullRequestDiff { ticket, answer });
1033-
});
1034-
}),
1027+
read_diff: {
1028+
let under_way = std::cell::RefCell::new(None::<tokio::task::AbortHandle>);
1029+
Box::new(move |reference, ask: DiffAsk| {
1030+
let (client, answers) = (differ.clone(), diffs.clone());
1031+
let read = tokio::spawn(async move {
1032+
let (cursor, commit) = (ask.cursor.as_deref(), ask.commit.as_deref());
1033+
let answer = read_diff(&client, &reference, cursor, commit).await;
1034+
let ticket = ask.ticket;
1035+
let _ = answers.send(ActionResult::PullRequestDiff { ticket, answer });
1036+
});
1037+
// An ask comes only once the details have the answer to the last one or no
1038+
// longer want it, as when another commit was picked, so that read stops.
1039+
if let Some(before) = under_way.replace(Some(read.abort_handle())) {
1040+
before.abort();
1041+
}
1042+
})
1043+
},
10351044
read_comments: Box::new(move |reference, ask: CommentsAsk| {
10361045
let (client, answers) = (pager.clone(), pages.clone());
10371046
tokio::spawn(async move {
@@ -15556,4 +15565,54 @@ mod tests {
1555615565
assert_eq!(sent["cursor"], "2");
1555715566
assert_eq!(sent["commit"], "abc1234def");
1555815567
}
15568+
15569+
#[tokio::test]
15570+
async fn the_apps_next_read_of_a_diff_stops_the_one_under_way() {
15571+
use crate::http::{Api, fake};
15572+
use std::sync::atomic::{AtomicUsize, Ordering};
15573+
15574+
// T3 answers one read at a time and holds the first back for a second, so the second
15575+
// read's answer comes after the first's unless the first stopped.
15576+
let body = json!({"patch": "", "truncated": false, "nextCursor": null});
15577+
let answer = fake::response("200 OK", &[], &body.to_string());
15578+
let reads = AtomicUsize::new(0);
15579+
let t3 = fake::Server::start(move |_| {
15580+
if reads.fetch_add(1, Ordering::SeqCst) == 0 {
15581+
std::thread::sleep(Duration::from_secs(1));
15582+
}
15583+
answer.clone()
15584+
});
15585+
let mut client = Client::offline();
15586+
client.api = Api::without_proxy(&t3.origin);
15587+
let (sender, mut results) = mpsc::unbounded_channel();
15588+
let app = App::new(
15589+
Arc::new(client),
15590+
Theme::new(Depth::TrueColor),
15591+
&Settings::default(),
15592+
sender,
15593+
);
15594+
let reference = Reference::new("p", Some("github.com"), "krishhgg/t3term", 31).unwrap();
15595+
let ask = |ticket, commit: &str| DiffAsk {
15596+
ticket,
15597+
host: "github.com".into(),
15598+
repository: "krishhgg/t3term".into(),
15599+
number: 31,
15600+
cursor: None,
15601+
commit: Some(commit.into()),
15602+
};
15603+
(app.read_diff)(reference.clone(), ask(7, "1111111aaaa"));
15604+
tokio::time::sleep(Duration::from_millis(200)).await;
15605+
(app.read_diff)(reference, ask(8, "2222222bbbb"));
15606+
let result = tokio::time::timeout(Duration::from_secs(5), results.recv())
15607+
.await
15608+
.expect("an answer within five seconds")
15609+
.expect("the answer");
15610+
let ActionResult::PullRequestDiff { ticket, .. } = result else {
15611+
panic!("an answer of another kind");
15612+
};
15613+
assert_eq!(ticket, 8);
15614+
assert!(results.try_recv().is_err());
15615+
let sent: Value = serde_json::from_str(&t3.bodies()[1]).unwrap();
15616+
assert_eq!(sent["commit"], "2222222bbbb");
15617+
}
1555915618
}

0 commit comments

Comments
 (0)