diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 538e907e..2403a737 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -62,7 +62,7 @@ Summary: - **Swift unit tests** — cover ViewModel-level behavior in `shell/mac/Tests/JayJayTests/`. Run with `just test-app`. - **XCUITest scenes** — cover user-visible flows in `shell/mac/Tests/JayJayUITests/`. Run with `just test-ui`. -UI tests launch the app against deterministic fixtures at `/tmp/jayjay-test-fixtures/{simple,conflict}` built by `just shell::ui-test-setup`. Each scene subclasses `SceneBase` and asserts against accessibility identifiers declared in `shell/mac/Sources/JayJay/Shared/AccessibilityIdentifiers.swift`. When adding a new user-visible view or interaction: +UI tests launch the app against deterministic fixtures under `/tmp/jayjay-test-fixtures` built by `just shell::ui-test-setup`. Canonical fixtures cover simple, complex, structured-format, review-note, bookmark-diff, and conflict workflows. Repository-mutating scenes use workflow-named copies generated from those canonical fixtures so test order cannot leak state. Each scene subclasses `SceneBase` and asserts against accessibility identifiers declared in `shell/mac/Sources/JayJay/Shared/AccessibilityIdentifiers.swift`. When adding a new user-visible view or interaction: 1. Attach a stable `.accessibilityIdentifier(...)` to the view, keyed by the data that makes it unique (change-id prefix, file path, etc.). Add a constant/function to `AID` so tests and views share the same string. 2. Write a scene test under `Tests/JayJayUITests/Scenes/` that exercises the flow end-to-end. diff --git a/agents/testing.md b/agents/testing.md index f1f3c588..460797d5 100644 --- a/agents/testing.md +++ b/agents/testing.md @@ -22,11 +22,13 @@ Load this file before adding fixtures, reorganizing tests, or changing UI test b UI tests live in `shell/mac/Tests/JayJayUITests/`. -Each `SceneBase` subclass launches the app against a named fixture. The default fixture is `simple`; override `fixtureName` when a scene needs a different repo state. +Each `SceneBase` subclass launches a named fixture. The default fixture is `simple`; override `fixtureName` when a scene needs `complex`, `formats`, `review-notes`, `bookmark-diff`, or `conflict`. + +Use `complex` when the workflow depends on scale or mixed diff shapes. It has more than 30 changed paths and 1,000 changed lines across additions, rewrites, deletions, renames, binary content, deep paths, and a path containing spaces. Keep narrow scenes on purpose-built fixtures so their assertions stay legible. Use accessibility identifiers from `shell/mac/Sources/JayJay/Shared/AccessibilityIdentifiers.swift`. Add identifiers at the view body and key them by stable data such as change-id prefix or file path. -If a scene mutates repo state, give it its own fixture. Tests share a filesystem and run alphabetically, so mutations on `simple` leak into later tests. `ui-test-setup` already creates `simple-newchange` for `NewChangeScene`; add sibling copies for new mutating scenes. +The sandboxed XCUITest runner cannot create repositories where the launched app can open them. Mutating scenes therefore use dedicated copies generated from a canonical fixture by `ui-test-fixtures.sh`; name those copies for the workflow, not for their source fixture. Each scene gets an isolated review store. ## GPUI Tests diff --git a/scripts/ui-test-fixtures.sh b/scripts/ui-test-fixtures.sh index f1ea47f1..bb6b2214 100755 --- a/scripts/ui-test-fixtures.sh +++ b/scripts/ui-test-fixtures.sh @@ -21,15 +21,13 @@ setup_defaults() { defaults write "$bundle_id" jayjay.sideBySideDiff -bool NO defaults write "$bundle_id" jayjay.ignoreWhitespace -bool NO defaults write "$bundle_id" jayjay.treeFileList -bool NO - defaults write "$bundle_id" jayjay.recentRepos -array "$fixtures/simple-formats" + defaults write "$bundle_id" jayjay.recentRepos -array "$fixtures/formats" defaults delete "$bundle_id" jayjay.lastOpenedRepo 2>/dev/null || true # Start each run with the command palette at its default (centered) position. defaults delete "$bundle_id" commandPalette.frameOrigin 2>/dev/null || true } # Simple: three commits + an active working copy with two new files. -# Clone once, then copy per-class for tests that mutate repo state so leaks -# don't bleed across scenes (NewChangeScene creates a new @). fixture_simple() { jj git init --colocate "$fixtures/simple" ( @@ -45,24 +43,35 @@ fixture_simple() { echo "wip 1" > wip1.txt echo "wip 2" > wip2.txt ) - cp -R "$fixtures/simple" "$fixtures/simple-newchange" - cp -R "$fixtures/simple" "$fixtures/simple-commit" - cp -R "$fixtures/simple" "$fixtures/simple-editdesc" - cp -R "$fixtures/simple" "$fixtures/simple-save-description" - cp -R "$fixtures/simple" "$fixtures/simple-diffstats" - cp -R "$fixtures/simple" "$fixtures/simple-bookmark-diff" - cp -R "$fixtures/simple" "$fixtures/simple-review-notes" - cp -R "$fixtures/simple" "$fixtures/simple-formats" +} + +copy_fixture() { + local source="$1" + local destination="$2" + cp -R "$fixtures/$source" "$fixtures/$destination" +} + +fixture_mutating_scenes() { + copy_fixture simple commit + copy_fixture simple edit-description + copy_fixture simple save-description + copy_fixture simple diff-stats + copy_fixture simple new-change +} + +fixture_bookmark_diff() { + copy_fixture simple bookmark-diff ( - cd "$fixtures/simple-bookmark-diff" + cd "$fixtures/bookmark-diff" jj bookmark create bookmark-diff -r @ ) } # Simple plus structured files for projection/rendering checks. fixture_formats() { + copy_fixture simple formats ( - cd "$fixtures/simple-formats" + cd "$fixtures/formats" cp "$format_fixtures/analysis.ipynb" analysis.ipynb cp "$format_fixtures/notes.md" notes.md cp "$format_fixtures/release.html" release.html @@ -76,8 +85,9 @@ fixture_formats() { # A long function so ReviewNotesScene can verify the diff expands around embedded note rows. fixture_review_notes() { + copy_fixture simple review-notes ( - cd "$fixtures/simple-review-notes" + cd "$fixtures/review-notes" printf '%s\n' \ 'func fibonacciReport(limit: Int) -> String {' \ ' var values: [Int] = []' \ @@ -107,6 +117,93 @@ fixture_review_notes() { ) } +# Complex: a broad working-copy diff with enough files and changed lines to exercise Diff Edit's large-repository policy. +fixture_complex() { + jj git init --colocate "$fixtures/complex" + ( + cd "$fixtures/complex" + mkdir -p assets config/environments "docs/product specs" docs/guides src/modules + + for module_number in {1..12}; do + printf -v module '%02d' "$module_number" + { + printf 'public enum Module%s {\n' "$module" + for line in {1..36}; do + printf ' public static let value%s = "module-%s-base-%s"\n' "$line" "$module" "$line" + done + printf '}\n' + } > "src/modules/module-$module.swift" + done + + for guide_number in {1..10}; do + printf -v guide '%02d' "$guide_number" + { + printf '# Guide %s\n\n' "$guide" + for line in {1..24}; do + printf 'Baseline guide %s paragraph %s with stable documentation context.\n' "$guide" "$line" + done + } > "docs/guides/guide-$guide.md" + done + + for environment in development staging production test; do + printf '{\n "environment": "%s",\n "featureFlags": ["search", "sync", "review"]\n}\n' "$environment" > "config/environments/$environment.json" + done + + printf '# API overview\n\nBaseline API contract.\n' > "docs/product specs/api overview.md" + printf '\x00\x01jayjay-baseline-binary\x00' > assets/logo.bin + + jj describe -m "complex baseline" + jj bookmark create main -r @ + jj new + + for module_number in {1..8}; do + printf -v module '%02d' "$module_number" + { + printf 'public enum Module%s {\n' "$module" + for line in {1..48}; do + printf ' public static let revisedValue%s = "module-%s-working-copy-%s"\n' "$line" "$module" "$line" + done + printf '}\n' + } > "src/modules/module-$module.swift" + done + + printf '{\n "environment": "development",\n "featureFlags": ["search", "sync", "review", "debug-menu"],\n "apiBaseURL": "https://dev.example.test"\n}\n' > config/environments/development.json + printf '{\n "environment": "staging",\n "featureFlags": ["search", "sync", "review", "audit-log"],\n "apiBaseURL": "https://staging.example.test"\n}\n' > config/environments/staging.json + + for guide_number in {1..4}; do + printf -v guide '%02d' "$guide_number" + rm "docs/guides/guide-$guide.md" + done + mkdir -p docs/reference + mv docs/guides/guide-05.md docs/reference/guide-05-renamed.md + mv docs/guides/guide-06.md docs/reference/guide-06-renamed.md + + for feature_number in {1..24}; do + printf -v feature '%02d' "$feature_number" + mkdir -p "src/features/area-$feature" + { + printf 'public struct Feature%sComponent {\n' "$feature" + for line in {1..34}; do + printf ' public let field%s = "feature-%s-value-%s"\n' "$line" "$feature" "$line" + done + printf '}\n' + } > "src/features/area-$feature/component.swift" + done + + mkdir -p src/generated/tables + { + printf 'public let generatedRows = [\n' + for ((line = 1; line <= 420; line++)); do + printf ' "generated-row-%03d",\n' "$line" + done + printf ']\n' + } > src/generated/tables/large_table.swift + + printf '# API overview\n\nWorking-copy API contract with authentication, pagination, retries, and structured errors.\n' > "docs/product specs/api overview.md" + printf '\x00\x02jayjay-working-copy-binary\x00' > assets/logo.bin + ) +} + # Conflict: @ is a rebased change with one file containing multiple conflicts. fixture_conflict() { jj git init --colocate "$fixtures/conflict" @@ -122,12 +219,12 @@ fixture_conflict() { write_conflict_file feature jj rebase -r @ -d main ) - cp -R "$fixtures/conflict" "$fixtures/conflict-use-ours" + copy_fixture conflict conflict-use-ours } fixture_repository_stores() { printf '{"repositories":[]}\n' > "$fixtures/repositories-empty.json" - printf '{"repositories":["%s"]}\n' "$fixtures/simple-formats" > "$fixtures/repositories-pinned.json" + printf '{"repositories":["%s"]}\n' "$fixtures/formats" > "$fixtures/repositories-pinned.json" } # Three sections editing the same keys so every side of the rebase collides. @@ -154,7 +251,10 @@ setup_defaults rm -rf "$fixtures" mkdir -p "$fixtures" fixture_simple +fixture_mutating_scenes +fixture_bookmark_diff fixture_formats fixture_review_notes +fixture_complex fixture_conflict fixture_repository_stores diff --git a/shell/gpui/src/diff/diff_view/render.rs b/shell/gpui/src/diff/diff_view/render.rs index e822fe12..6c41a9c3 100644 --- a/shell/gpui/src/diff/diff_view/render.rs +++ b/shell/gpui/src/diff/diff_view/render.rs @@ -10,10 +10,10 @@ use super::header::{ }; use super::note_banner::stale_notes_banner; use super::placeholders::{placeholder, placeholder_card, placeholder_inner}; -use super::sbs_body::side_by_side_body; +use super::sbs_body::{SideBySideBodyState, side_by_side_body}; use super::sbs_note_banner::with_sbs_note_banner; use super::state::{DetailMode, DiffViewMode, DiffViewState, FindState}; -use super::unified_body::unified_body; +use super::unified_body::{UnifiedBodyState, unified_body}; use crate::app::theme::{Theme, theme, with_alpha}; use crate::diff::image_diff::{hunk_is_image, image_diff_view}; use crate::diff::markdown_diff::markdown_diff_view; @@ -153,24 +153,28 @@ pub fn diff_view( .into_any_element() } (Some(fd), DiffViewMode::Unified) => unified_body( - fd, - t.clone(), - query.clone(), - scroll.clone(), - state.unified_bounds.clone(), - &state.wrap_cache, - state.notes, + UnifiedBodyState { + file_diff: fd, + theme: t.clone(), + query: query.clone(), + scroll: scroll.clone(), + bounds: state.unified_bounds.clone(), + wrap_cache: &state.wrap_cache, + notes: state.notes, + }, cx, ), (Some(fd), DiffViewMode::SideBySide) => { let sbs = side_by_side_body( - fd, - t.clone(), - query.clone(), - scroll.clone(), - state.sbs_old_bounds.clone(), - state.sbs_new_bounds.clone(), - &state.wrap_cache, + SideBySideBodyState { + file_diff: fd, + theme: t.clone(), + query: query.clone(), + scroll: scroll.clone(), + old_bounds: state.sbs_old_bounds.clone(), + new_bounds: state.sbs_new_bounds.clone(), + wrap_cache: &state.wrap_cache, + }, cx, ); with_sbs_note_banner(sbs, state.notes, &t, cx) diff --git a/shell/gpui/src/diff/diff_view/sbs_body.rs b/shell/gpui/src/diff/diff_view/sbs_body.rs index 5c09a0b8..cfdd1532 100644 --- a/shell/gpui/src/diff/diff_view/sbs_body.rs +++ b/shell/gpui/src/diff/diff_view/sbs_body.rs @@ -19,23 +19,37 @@ use crate::repo::window::{DiffWrapCacheSlot, PanelBoundsSlot, RepoWindow}; use crate::ui::primitives::no_scrollbar_gutter; use crate::ui::scrollbar::vertical_uniform_scrollbar; -#[allow(clippy::too_many_arguments)] +pub(super) struct SideBySideBodyState<'a> { + pub(super) file_diff: &'a FileDiff, + pub(super) theme: Theme, + pub(super) query: Option, + pub(super) scroll: UniformListScrollHandle, + pub(super) old_bounds: PanelBoundsSlot, + pub(super) new_bounds: PanelBoundsSlot, + pub(super) wrap_cache: &'a DiffWrapCacheSlot, +} + pub(super) fn side_by_side_body( - fd: &FileDiff, - theme: Theme, - query: Option, - scroll: UniformListScrollHandle, - old_bounds: PanelBoundsSlot, - new_bounds: PanelBoundsSlot, - wrap_cache: &DiffWrapCacheSlot, + state: SideBySideBodyState<'_>, cx: &mut Context, ) -> AnyElement { + let SideBySideBodyState { + file_diff, + theme, + query, + scroll, + old_bounds, + new_bounds, + wrap_cache, + } = state; let theme = Arc::new(theme); let query = Arc::new(query); let advance = fonts::mono_advance(cx, px(12.)); let old_cols = wrap_cols_from_bounds(old_bounds.get(), advance); let new_cols = wrap_cols_from_bounds(new_bounds.get(), advance); - let rows = wrap_cache.borrow_mut().side_by_side(fd, old_cols, new_cols); + let rows = wrap_cache + .borrow_mut() + .side_by_side(file_diff, old_cols, new_cols); let count = rows.len(); let old_gutter = { @@ -68,7 +82,7 @@ pub(super) fn side_by_side_body( }; let old_content = sbs_content_list( - SbsContentArgs { + SbsContentState { id: "sbs-old-content", count, rows: rows.clone(), @@ -82,7 +96,7 @@ pub(super) fn side_by_side_body( cx, ); let new_content = sbs_content_list( - SbsContentArgs { + SbsContentState { id: "sbs-new-content", count, rows, @@ -165,7 +179,7 @@ pub(super) fn side_by_side_body( .into_any_element() } -struct SbsContentArgs { +struct SbsContentState { id: &'static str, count: usize, rows: Arc>, @@ -177,8 +191,8 @@ struct SbsContentArgs { advance: Pixels, } -fn sbs_content_list(args: SbsContentArgs, cx: &mut Context) -> UniformList { - let SbsContentArgs { +fn sbs_content_list(state: SbsContentState, cx: &mut Context) -> UniformList { + let SbsContentState { id, count, rows, @@ -188,7 +202,7 @@ fn sbs_content_list(args: SbsContentArgs, cx: &mut Context) -> Unifo side, bounds, advance, - } = args; + } = state; uniform_list( id, count, diff --git a/shell/gpui/src/diff/diff_view/unified_body.rs b/shell/gpui/src/diff/diff_view/unified_body.rs index db94c731..f8835369 100644 --- a/shell/gpui/src/diff/diff_view/unified_body.rs +++ b/shell/gpui/src/diff/diff_view/unified_body.rs @@ -23,45 +23,58 @@ use crate::repo::window::{DiffWrapCacheSlot, PanelBoundsSlot, RepoWindow}; use crate::ui::primitives::no_scrollbar_gutter; use crate::ui::scrollbar::vertical_uniform_scrollbar; -#[allow(clippy::too_many_arguments)] +pub(super) struct UnifiedBodyState<'a> { + pub(super) file_diff: &'a FileDiff, + pub(super) theme: Theme, + pub(super) query: Option, + pub(super) scroll: UniformListScrollHandle, + pub(super) bounds: PanelBoundsSlot, + pub(super) wrap_cache: &'a DiffWrapCacheSlot, + pub(super) notes: &'a [ReviewNoteStatus], +} + pub(super) fn unified_body( - fd: &FileDiff, - theme: Theme, - query: Option, - scroll: UniformListScrollHandle, - bounds_slot: PanelBoundsSlot, - wrap_cache: &DiffWrapCacheSlot, - notes: &[ReviewNoteStatus], + state: UnifiedBodyState<'_>, cx: &mut Context, ) -> AnyElement { + let UnifiedBodyState { + file_diff, + theme, + query, + scroll, + bounds, + wrap_cache, + notes, + } = state; let theme = Arc::new(theme); let query = Arc::new(query); let advance = fonts::mono_advance(cx, px(12.)); - let wrap_cols = wrap_cols_from_bounds(bounds_slot.get(), advance); - let lines = wrap_cache.borrow_mut().unified(fd, wrap_cols); + let wrap_cols = wrap_cols_from_bounds(bounds.get(), advance); + let lines = wrap_cache.borrow_mut().unified(file_diff, wrap_cols); // Both lists size off this shared, interleaved row list — never off lines.len() — so a NoteText row shifts gutter and content lists in lockstep. - let rendered = wrap_cache.borrow_mut().rows(fd, wrap_cols, notes); + let rendered = wrap_cache.borrow_mut().rows(file_diff, wrap_cols, notes); let count = rendered.rows.len(); let gutter_lines = lines.clone(); let gutter_rendered = rendered.clone(); let gutter_theme = theme.clone(); - let gutter_path = fd.path.clone(); + let gutter_path = file_diff.path.clone(); let gutter = uniform_list( "diff-gutter", count, cx.processor(move |view, range: std::ops::Range, _window, cx| { - let gutter_sel = view.diff.gutter_selection.clone(); + let selection = view.diff.gutter_selection.clone(); range .map(|ix| { gutter_row_at( - view, - ix, - &gutter_rendered, - &gutter_lines, - &gutter_path, - gutter_sel.as_ref(), - &gutter_theme, + GutterRowState { + ix, + rendered: &gutter_rendered, + lines: &gutter_lines, + path: &gutter_path, + selection: selection.as_ref(), + theme: &gutter_theme, + }, cx, ) }) @@ -74,8 +87,8 @@ pub(super) fn unified_body( let content_rendered = rendered; let content_theme = theme.clone(); let content_query = query; - let content_bounds = bounds_slot.clone(); - let content_path = fd.path.clone(); + let content_bounds = bounds.clone(); + let content_path = file_diff.path.clone(); let content = uniform_list( "diff-content", count, @@ -85,16 +98,18 @@ pub(super) fn unified_body( range .map(|ix| { content_row_at( - ix, - &content_rendered, - &content_lines, - &content_path, - sel, - gutter_sel.as_ref(), - &content_theme, - content_query.as_deref(), - advance, - &content_bounds, + ContentRowState { + ix, + rendered: &content_rendered, + lines: &content_lines, + path: &content_path, + selection: sel, + gutter_selection: gutter_sel.as_ref(), + theme: &content_theme, + query: content_query.as_deref(), + advance, + bounds: &content_bounds, + }, cx, ) }) @@ -119,7 +134,7 @@ pub(super) fn unified_body( .flex_1() .min_w_0() .h_full() - .child(bounds_capture(bounds_slot.clone())) + .child(bounds_capture(bounds.clone())) .on_mouse_up( MouseButton::Left, cx.listener(|v, _: &MouseUpEvent, _, cx| { @@ -129,7 +144,7 @@ pub(super) fn unified_body( .child(no_scrollbar_gutter(content).h_full()) .child(vertical_uniform_scrollbar( scroll, - bounds_slot, + bounds, px(count as f32 * ROW_HEIGHT), theme.as_ref(), cx, @@ -138,17 +153,24 @@ pub(super) fn unified_body( .into_any_element() } -#[allow(clippy::too_many_arguments)] -fn gutter_row_at( - _view: &mut RepoWindow, +struct GutterRowState<'a> { ix: usize, - rendered: &DiffRenderRows, - lines: &[WrappedDiffLine], - path: &str, - selection: Option<&GutterLineSelection>, - theme: &Theme, - cx: &mut Context, -) -> AnyElement { + rendered: &'a DiffRenderRows, + lines: &'a [WrappedDiffLine], + path: &'a str, + selection: Option<&'a GutterLineSelection>, + theme: &'a Theme, +} + +fn gutter_row_at(state: GutterRowState<'_>, cx: &mut Context) -> AnyElement { + let GutterRowState { + ix, + rendered, + lines, + path, + selection, + theme, + } = state; let DiffRenderRow::Line(w_ix) = &rendered.rows[ix] else { return note_gutter_row(theme).into_any_element(); }; @@ -178,20 +200,32 @@ fn gutter_row_at( attach_gutter_selection_handlers(row, path.to_owned(), line_ix, cx).into_any_element() } -#[allow(clippy::too_many_arguments)] -fn content_row_at( +struct ContentRowState<'a> { ix: usize, - rendered: &DiffRenderRows, - lines: &[WrappedDiffLine], - path: &str, + rendered: &'a DiffRenderRows, + lines: &'a [WrappedDiffLine], + path: &'a str, selection: Option, - gutter_selection: Option<&GutterLineSelection>, - theme: &Theme, - query: Option<&str>, + gutter_selection: Option<&'a GutterLineSelection>, + theme: &'a Theme, + query: Option<&'a str>, advance: Pixels, - bounds: &PanelBoundsSlot, - cx: &mut Context, -) -> AnyElement { + bounds: &'a PanelBoundsSlot, +} + +fn content_row_at(state: ContentRowState<'_>, cx: &mut Context) -> AnyElement { + let ContentRowState { + ix, + rendered, + lines, + path, + selection, + gutter_selection, + theme, + query, + advance, + bounds, + } = state; match &rendered.rows[ix] { DiffRenderRow::Line(w_ix) => { let line = &lines[*w_ix]; diff --git a/shell/gpui/src/diff/file_column/flat.rs b/shell/gpui/src/diff/file_column/flat.rs index bc118204..2d3af80d 100644 --- a/shell/gpui/src/diff/file_column/flat.rs +++ b/shell/gpui/src/diff/file_column/flat.rs @@ -8,7 +8,10 @@ use gpui::{ }; use jayjay_core::DiffHunk; -use super::row::{file_name_opacity, file_text_content, finish_file_row, review_checkbox, row_bg}; +use super::row::{ + FileRowHandlers, FileRowState, file_name_opacity, file_text_content, finish_file_row, + review_checkbox, row_bg, +}; use crate::app::theme::Theme; use crate::repo::window::RepoWindow; use crate::ui::primitives::no_scrollbar_gutter; @@ -29,20 +32,32 @@ pub(crate) fn middle_elide(s: &str, max_chars: usize) -> String { format!("{head}…{tail}") } -#[allow(clippy::too_many_arguments)] -pub(super) fn flat_body( - hunks: Arc>, - visible_indices: Arc>, - selected_ix: Option, - multi_selected: Arc>, - t: Theme, - scroll: UniformListScrollHandle, - change_id: Option, - show_review: bool, - note_counts: Arc>, - column_width: f32, - cx: &mut Context, -) -> AnyElement { +pub(super) struct FlatBodyState { + pub(super) hunks: Arc>, + pub(super) visible_indices: Arc>, + pub(super) selected_ix: Option, + pub(super) multi_selected: Arc>, + pub(super) theme: Theme, + pub(super) scroll: UniformListScrollHandle, + pub(super) change_id: Option, + pub(super) show_review: bool, + pub(super) note_counts: Arc>, + pub(super) column_width: f32, +} + +pub(super) fn flat_body(state: FlatBodyState, cx: &mut Context) -> AnyElement { + let FlatBodyState { + hunks, + visible_indices, + selected_ix, + multi_selected, + theme, + scroll, + change_id, + show_review, + note_counts, + column_width, + } = state; let count = visible_indices.len(); let fixed_chrome = if show_review { 80.0 } else { 56.0 }; let text_px = (column_width - fixed_chrome).max(80.0); @@ -52,7 +67,7 @@ pub(super) fn flat_body( "files-flat", count, cx.processor(move |this, range: std::ops::Range, _window, cx| { - let t = t.clone(); + let theme = theme.clone(); let change_id = change_id.clone(); let visible_indices = visible_indices.clone(); let note_counts = note_counts.clone(); @@ -74,31 +89,39 @@ pub(super) fn flat_body( }; let note_count = note_counts.get(&path).copied().unwrap_or(0); flat_file_row( - hunk, - is_selected, - reviewed, - show_review, - note_count, - hunk_ix, + FileRowState { + hunk, + is_selected, + reviewed, + show_review, + note_count, + ix: hunk_ix, + theme: &theme, + }, basename_chars, path_chars, - &t, - cx.listener(move |view, event: &ClickEvent, _window, cx| { - view.handle_file_row_click(hunk_ix, event.modifiers(), cx); - }), - cx.listener(move |view, ev: &MouseDownEvent, _w, cx| { - view.open_file_context_menu(&path, ev.position, cx); - }), - cx.listener(move |view, _event: &ClickEvent, _w, cx| { - if let Some(cid) = change_for_review.clone() { - view.toggle_reviewed( - cid, - path_for_review.clone(), - identity_for_review.clone(), - cx, - ); - } - }), + FileRowHandlers { + on_click: cx.listener(move |view, event: &ClickEvent, _window, cx| { + view.handle_file_row_click(hunk_ix, event.modifiers(), cx); + }), + on_right_click: cx.listener( + move |view, ev: &MouseDownEvent, _w, cx| { + view.open_file_context_menu(&path, ev.position, cx); + }, + ), + on_review_click: cx.listener( + move |view, _event: &ClickEvent, _w, cx| { + if let Some(cid) = change_for_review.clone() { + view.toggle_reviewed( + cid, + path_for_review.clone(), + identity_for_review.clone(), + cx, + ); + } + }, + ), + }, ) }) .collect() @@ -108,27 +131,32 @@ pub(super) fn flat_body( no_scrollbar_gutter(list).h_full().into_any_element() } -#[allow(clippy::too_many_arguments)] fn flat_file_row( - hunk: &DiffHunk, - is_selected: bool, - reviewed: bool, - show_review: bool, - note_count: usize, - ix: usize, + state: FileRowState<'_>, basename_chars: usize, path_chars: usize, - t: &Theme, - on_click: F, - on_right_click: FR, - on_review_click: FRev, + handlers: FileRowHandlers, ) -> AnyElement where F: Fn(&ClickEvent, &mut Window, &mut App) + 'static, FR: Fn(&MouseDownEvent, &mut Window, &mut App) + 'static, FRev: Fn(&ClickEvent, &mut Window, &mut App) + 'static, { - let bg_row = row_bg(is_selected, t); + let FileRowState { + hunk, + is_selected, + reviewed, + show_review, + note_count, + ix, + theme, + } = state; + let FileRowHandlers { + on_click, + on_right_click, + on_review_click, + } = handlers; + let bg_row = row_bg(is_selected, theme); let basename = middle_elide( hunk.path.rsplit('/').next().unwrap_or(&hunk.path), @@ -141,7 +169,7 @@ where SharedString::from(basename), SharedString::from(path_display), name_opacity, - t, + theme, ); let mut row = div() @@ -157,7 +185,7 @@ where .px(px(6.)) .rounded_md() .border_b_1() - .border_color(rgb(t.row_border)) + .border_color(rgb(theme.row_border)) .bg(rgb(bg_row)) .relative() .cursor_pointer() @@ -167,9 +195,9 @@ where row = row.child(review_checkbox( ("review-flat", ix), reviewed, - t, + theme, on_review_click, )); } - finish_file_row(row, hunk, content, note_count, t) + finish_file_row(row, hunk, content, note_count, theme) } diff --git a/shell/gpui/src/diff/file_column/mod.rs b/shell/gpui/src/diff/file_column/mod.rs index 591702bd..1cad23c9 100644 --- a/shell/gpui/src/diff/file_column/mod.rs +++ b/shell/gpui/src/diff/file_column/mod.rs @@ -28,9 +28,9 @@ pub(super) fn file_name_container(name: impl IntoElement) -> impl IntoElement { .child(name) } -use flat::flat_body; +use flat::{FlatBodyState, flat_body}; use header::{FileHeaderFilters, file_column_header}; -use tree::tree_body; +use tree::{TreeBodyState, tree_body}; pub(crate) use flat::middle_elide; @@ -132,33 +132,37 @@ pub fn file_column(state: FileColumnState<'_>, cx: &mut Context) -> .borrow_mut() .visible(&hunks, &visible_indices, collapsed_dirs); tree_body( - hunks.clone(), - visible_indices.clone(), - tree, - selected_ix, - multi_selected, - collapsed_dirs.clone(), - t.clone(), - tree_scroll, - change_id.clone(), - reviewed_files.clone(), - show_review, - note_counts.clone(), - column_width, + TreeBodyState { + hunks: hunks.clone(), + visible_indices: visible_indices.clone(), + tree, + selected_ix, + multi_selected, + collapsed: collapsed_dirs.clone(), + theme: t.clone(), + scroll: tree_scroll, + change_id: change_id.clone(), + reviewed_files: reviewed_files.clone(), + show_review, + note_counts: note_counts.clone(), + column_width, + }, cx, ) } else { flat_body( - hunks.clone(), - visible_indices.clone(), - selected_ix, - multi_selected, - t.clone(), - scroll, - change_id.clone(), - show_review, - note_counts.clone(), - column_width, + FlatBodyState { + hunks: hunks.clone(), + visible_indices: visible_indices.clone(), + selected_ix, + multi_selected, + theme: t.clone(), + scroll, + change_id: change_id.clone(), + show_review, + note_counts: note_counts.clone(), + column_width, + }, cx, ) }; diff --git a/shell/gpui/src/diff/file_column/row.rs b/shell/gpui/src/diff/file_column/row.rs index 022cc5ec..c01ca119 100644 --- a/shell/gpui/src/diff/file_column/row.rs +++ b/shell/gpui/src/diff/file_column/row.rs @@ -9,6 +9,22 @@ use crate::app::theme::{Theme, with_alpha}; use crate::diff::file_status; use crate::ui::primitives::{CheckCircleState, check_circle}; +pub(super) struct FileRowState<'a> { + pub(super) hunk: &'a DiffHunk, + pub(super) is_selected: bool, + pub(super) reviewed: bool, + pub(super) show_review: bool, + pub(super) note_count: usize, + pub(super) ix: usize, + pub(super) theme: &'a Theme, +} + +pub(super) struct FileRowHandlers { + pub(super) on_click: F, + pub(super) on_right_click: FR, + pub(super) on_review_click: FRev, +} + pub(super) fn row_bg(is_selected: bool, t: &Theme) -> u32 { if is_selected { t.selected_bg diff --git a/shell/gpui/src/diff/file_column/tree.rs b/shell/gpui/src/diff/file_column/tree.rs index 381146fc..5db457c5 100644 --- a/shell/gpui/src/diff/file_column/tree.rs +++ b/shell/gpui/src/diff/file_column/tree.rs @@ -8,7 +8,10 @@ use gpui::{ }; use jayjay_core::{DiffHunk, FileTreeEntry}; -use super::row::{file_name_opacity, file_text_content, finish_file_row, review_checkbox, row_bg}; +use super::row::{ + FileRowHandlers, FileRowState, file_name_opacity, file_text_content, finish_file_row, + review_checkbox, row_bg, +}; use crate::app::fonts; use crate::app::theme::Theme; use crate::repo::window::RepoWindow; @@ -33,23 +36,38 @@ pub(super) fn is_entry_visible( true } -#[allow(clippy::too_many_arguments)] -pub(super) fn tree_body( - hunks: Arc>, - visible_indices: Arc>, - tree: Arc>, - selected_ix: Option, - multi_selected: Arc>, - collapsed: std::collections::HashSet, - t: Theme, - scroll: ScrollHandle, - change_id: Option, - reviewed_files: Option>>, - show_review: bool, - note_counts: Arc>, - column_width: f32, - cx: &mut Context, -) -> AnyElement { +pub(super) struct TreeBodyState { + pub(super) hunks: Arc>, + pub(super) visible_indices: Arc>, + pub(super) tree: Arc>, + pub(super) selected_ix: Option, + pub(super) multi_selected: Arc>, + pub(super) collapsed: std::collections::HashSet, + pub(super) theme: Theme, + pub(super) scroll: ScrollHandle, + pub(super) change_id: Option, + pub(super) reviewed_files: Option>>, + pub(super) show_review: bool, + pub(super) note_counts: Arc>, + pub(super) column_width: f32, +} + +pub(super) fn tree_body(state: TreeBodyState, cx: &mut Context) -> AnyElement { + let TreeBodyState { + hunks, + visible_indices, + tree, + selected_ix, + multi_selected, + collapsed, + theme, + scroll, + change_id, + reviewed_files, + show_review, + note_counts, + column_width, + } = state; let collapsed = Arc::new(collapsed); let mut list = div() .id("files-tree") @@ -84,30 +102,40 @@ pub(super) fn tree_body( let note_count = note_counts.get(&path).copied().unwrap_or(0); tree_file_row( entry, - hunk, - is_selected, - reviewed, - show_review, - note_count, - ix, + FileRowState { + hunk, + is_selected, + reviewed, + show_review, + note_count, + ix, + theme: &theme, + }, row_width, - &t, - cx.listener(move |view, event: &ClickEvent, _window, cx| { - view.handle_file_row_click(hunk_ix, event.modifiers(), cx); - }), - cx.listener(move |view, ev: &MouseDownEvent, _w, cx| { - view.open_file_context_menu(&path, ev.position, cx); - }), - cx.listener(move |view, _event: &ClickEvent, _w, cx| { - if let Some(cid) = change_for_review.clone() { - view.toggle_reviewed( - cid, - path_for_review.clone(), - identity_for_review.clone(), - cx, - ); - } - }), + FileRowHandlers { + on_click: cx.listener( + move |view, event: &ClickEvent, _window, cx| { + view.handle_file_row_click(hunk_ix, event.modifiers(), cx); + }, + ), + on_right_click: cx.listener( + move |view, ev: &MouseDownEvent, _w, cx| { + view.open_file_context_menu(&path, ev.position, cx); + }, + ), + on_review_click: cx.listener( + move |view, _event: &ClickEvent, _w, cx| { + if let Some(cid) = change_for_review.clone() { + view.toggle_reviewed( + cid, + path_for_review.clone(), + identity_for_review.clone(), + cx, + ); + } + }, + ), + }, ) } else { div().into_any_element() @@ -122,7 +150,7 @@ pub(super) fn tree_body( entry, ix, is_collapsed, - &t, + &theme, cx.listener(move |view, _event, _window, cx| { view.toggle_dir(dir_path.clone(), cx); }), @@ -134,27 +162,32 @@ pub(super) fn tree_body( list.into_any_element() } -#[allow(clippy::too_many_arguments)] fn tree_file_row( entry: &FileTreeEntry, - hunk: &DiffHunk, - is_selected: bool, - reviewed: bool, - show_review: bool, - note_count: usize, - ix: usize, + state: FileRowState<'_>, column_width: f32, - t: &Theme, - on_click: F, - on_right_click: FR, - on_review_click: FRev, + handlers: FileRowHandlers, ) -> AnyElement where F: Fn(&ClickEvent, &mut Window, &mut App) + 'static, FR: Fn(&MouseDownEvent, &mut Window, &mut App) + 'static, FRev: Fn(&ClickEvent, &mut Window, &mut App) + 'static, { - let bg_row = row_bg(is_selected, t); + let FileRowState { + hunk, + is_selected, + reviewed, + show_review, + note_count, + ix, + theme, + } = state; + let FileRowHandlers { + on_click, + on_right_click, + on_review_click, + } = handlers; + let bg_row = row_bg(is_selected, theme); let indent = (entry.depth as f32) * 14.0; let name_opacity = file_name_opacity(show_review, reviewed); let fixed_chrome = if show_review { 80.0 } else { 56.0 }; @@ -167,7 +200,7 @@ where SharedString::from(name), SharedString::from(path_display), name_opacity, - t, + theme, ); let mut row = div() .id(("tree-file", ix)) @@ -182,7 +215,7 @@ where .h(px(TREE_FILE_ROW_HEIGHT)) .rounded_md() .border_b_1() - .border_color(rgb(t.row_border)) + .border_color(rgb(theme.row_border)) .bg(rgb(bg_row)) .relative() .cursor_pointer() @@ -192,11 +225,11 @@ where row = row.child(review_checkbox( ("review-tree", ix), reviewed, - t, + theme, on_review_click, )); } - finish_file_row(row, hunk, content, note_count, t) + finish_file_row(row, hunk, content, note_count, theme) } fn tree_dir_row( diff --git a/shell/gpui/src/repo/window/diff_edit/cards.rs b/shell/gpui/src/repo/window/diff_edit/cards.rs index af209c5e..ff3c976f 100644 --- a/shell/gpui/src/repo/window/diff_edit/cards.rs +++ b/shell/gpui/src/repo/window/diff_edit/cards.rs @@ -5,7 +5,7 @@ use gpui::{ rgb, rgba, uniform_list, }; -use super::gutter::diff_edit_line_row; +use super::gutter::{DiffEditLineRowState, diff_edit_line_row}; use super::header::{header_bg, header_row}; use super::rows::{DiffEditRow, DiffEditRowModel}; use crate::app::fonts; @@ -88,13 +88,15 @@ fn row_at( .is_some_and(|selected| selected.contains(&full)) }); diff_edit_line_row( - &card.path, - line, - *line_ix + 1, - full_line.is_some(), - checked, + DiffEditLineRowState { + path: &card.path, + line, + display_line: *line_ix + 1, + editable: full_line.is_some(), + checked, + advance, + }, t, - advance, cx, ) } diff --git a/shell/gpui/src/repo/window/diff_edit/gutter.rs b/shell/gpui/src/repo/window/diff_edit/gutter.rs index ecfcdb29..d7d1ce8e 100644 --- a/shell/gpui/src/repo/window/diff_edit/gutter.rs +++ b/shell/gpui/src/repo/window/diff_edit/gutter.rs @@ -11,17 +11,28 @@ use crate::repo::window::RepoWindow; const CHECKBOX_WIDTH: f32 = 18.; -#[allow(clippy::too_many_arguments)] +pub(super) struct DiffEditLineRowState<'a> { + pub(super) path: &'a str, + pub(super) line: &'a DiffLine, + pub(super) display_line: u32, + pub(super) editable: bool, + pub(super) checked: bool, + pub(super) advance: Pixels, +} + pub(super) fn diff_edit_line_row( - path: &str, - line: &DiffLine, - display_line: u32, - editable: bool, - checked: bool, + state: DiffEditLineRowState<'_>, t: &Theme, - advance: Pixels, cx: &mut Context, ) -> AnyElement { + let DiffEditLineRowState { + path, + line, + display_line, + editable, + checked, + advance, + } = state; let bg = line_bg_color(line.style, line.conflict_kind, t); let checkbox = if editable && line.is_changed() { checkbox_cell(path, display_line, checked, bg, t, cx) diff --git a/shell/gpui/src/repo/window/diff_edit/state.rs b/shell/gpui/src/repo/window/diff_edit/state.rs index 389103c7..fd81417f 100644 --- a/shell/gpui/src/repo/window/diff_edit/state.rs +++ b/shell/gpui/src/repo/window/diff_edit/state.rs @@ -30,6 +30,14 @@ pub(super) struct DiffEditLoadedFile { pub(super) changed: Arc>, } +struct DiffEditLoadResult { + path: String, + old_content: Arc, + new_content: Arc, + full: FileDiff, + collapsed: CollapsedDiff, +} + #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum DiffEditCheckboxState { None, @@ -188,24 +196,36 @@ impl RepoWindow { }) .await; let _ = this.update(cx, |view, cx| { - view.finish_diff_edit_load(session, path, old, new, full, collapsed, cx); + view.finish_diff_edit_load( + session, + DiffEditLoadResult { + path, + old_content: old, + new_content: new, + full, + collapsed, + }, + cx, + ); }); }) .detach(); } } - #[allow(clippy::too_many_arguments)] fn finish_diff_edit_load( &mut self, session: u64, - path: String, - old_content: Arc, - new_content: Arc, - full: FileDiff, - collapsed: CollapsedDiff, + result: DiffEditLoadResult, cx: &mut Context, ) { + let DiffEditLoadResult { + path, + old_content, + new_content, + full, + collapsed, + } = result; if !self.diff_edit.active || self.diff_edit.session != session { return; } diff --git a/shell/mac/Sources/JayJay/Detail/DetailDescription.swift b/shell/mac/Sources/JayJay/Detail/DetailDescription.swift index 25db6d53..41811c2e 100644 --- a/shell/mac/Sources/JayJay/Detail/DetailDescription.swift +++ b/shell/mac/Sources/JayJay/Detail/DetailDescription.swift @@ -131,6 +131,7 @@ private struct DetailDescriptionSection: View { Button("Edit Diff...") { onOpenDiffEdit() } .buttonStyle(.bordered) .controlSize(.small) + .accessibilityIdentifier(AID.DiffEdit.open) .help("Open dedicated diff edit mode") } } diff --git a/shell/mac/Sources/JayJay/DiffEdit/DiffEditFileSection.swift b/shell/mac/Sources/JayJay/DiffEdit/DiffEditFileSection.swift index 5b503f4c..8cec59d6 100644 --- a/shell/mac/Sources/JayJay/DiffEdit/DiffEditFileSection.swift +++ b/shell/mac/Sources/JayJay/DiffEdit/DiffEditFileSection.swift @@ -69,6 +69,7 @@ struct DiffEditFileSection: View, DiffGutterSelectionActions { } .buttonStyle(.plain) .accessibilityIdentifier(AID.DiffEdit.fileToggle(hunk.path)) + .accessibilityValue(isCollapsed ? "collapsed" : "expanded") if supportsDiffEdit { Button(action: onToggleFile) { Image(systemName: selection.state.systemImage) diff --git a/shell/mac/Sources/JayJay/DiffEdit/DiffEditView.swift b/shell/mac/Sources/JayJay/DiffEdit/DiffEditView.swift index f37b5750..5494f0eb 100644 --- a/shell/mac/Sources/JayJay/DiffEdit/DiffEditView.swift +++ b/shell/mac/Sources/JayJay/DiffEdit/DiffEditView.swift @@ -157,6 +157,7 @@ struct DiffEditView: View { .controlSize(.small) Button("Cancel", action: session.onDone) .keyboardShortcut(.cancelAction) + .accessibilityIdentifier(AID.DiffEdit.cancel) } .controlSize(.small) .padding(.horizontal, 18) diff --git a/shell/mac/Sources/JayJay/Shared/AccessibilityIdentifiers.swift b/shell/mac/Sources/JayJay/Shared/AccessibilityIdentifiers.swift index 9e8095ea..32398caf 100644 --- a/shell/mac/Sources/JayJay/Shared/AccessibilityIdentifiers.swift +++ b/shell/mac/Sources/JayJay/Shared/AccessibilityIdentifiers.swift @@ -80,8 +80,10 @@ enum AID { } enum DiffEdit { + static let open = "diffEdit.open" static let expandAll = "diffEdit.expandAll" static let collapseAll = "diffEdit.collapseAll" + static let cancel = "diffEdit.cancel" static func fileToggle(_ path: String) -> String { "diffEdit.fileToggle.\(path)" diff --git a/shell/mac/Tests/JayJayUITests/Scenes/BookmarkDiffScene.swift b/shell/mac/Tests/JayJayUITests/Scenes/BookmarkDiffScene.swift index b8e91aa6..28ee95e5 100644 --- a/shell/mac/Tests/JayJayUITests/Scenes/BookmarkDiffScene.swift +++ b/shell/mac/Tests/JayJayUITests/Scenes/BookmarkDiffScene.swift @@ -2,7 +2,7 @@ import XCTest final class BookmarkDiffScene: SceneBase { override class var fixtureName: String { - "simple-bookmark-diff" + "bookmark-diff" } func testDiffBookmarkFromDagContextMenu() throws { diff --git a/shell/mac/Tests/JayJayUITests/Scenes/CommitBoxEditScene.swift b/shell/mac/Tests/JayJayUITests/Scenes/CommitBoxEditScene.swift index 86f2adff..af87baf6 100644 --- a/shell/mac/Tests/JayJayUITests/Scenes/CommitBoxEditScene.swift +++ b/shell/mac/Tests/JayJayUITests/Scenes/CommitBoxEditScene.swift @@ -1,9 +1,8 @@ import XCTest final class CommitBoxEditScene: SceneBase { - /// Dedicated fixture: editing a non-@ change mutates the repo. override class var fixtureName: String { - "simple-editdesc" + "edit-description" } func testEditReplacesStaleDraftWithCommitDescription() throws { diff --git a/shell/mac/Tests/JayJayUITests/Scenes/CommitBoxSaveScene.swift b/shell/mac/Tests/JayJayUITests/Scenes/CommitBoxSaveScene.swift index ce449b44..c108e685 100644 --- a/shell/mac/Tests/JayJayUITests/Scenes/CommitBoxSaveScene.swift +++ b/shell/mac/Tests/JayJayUITests/Scenes/CommitBoxSaveScene.swift @@ -1,9 +1,8 @@ import XCTest final class CommitBoxSaveScene: SceneBase { - /// Dedicated fixture: saving @'s description rewrites the working-copy commit. override class var fixtureName: String { - "simple-save-description" + "save-description" } func testSaveDescriptionDoesNotAdvanceWorkingCopy() throws { diff --git a/shell/mac/Tests/JayJayUITests/Scenes/CommitBoxScene.swift b/shell/mac/Tests/JayJayUITests/Scenes/CommitBoxScene.swift index a1eb6592..f8ba8e86 100644 --- a/shell/mac/Tests/JayJayUITests/Scenes/CommitBoxScene.swift +++ b/shell/mac/Tests/JayJayUITests/Scenes/CommitBoxScene.swift @@ -1,8 +1,9 @@ import XCTest final class CommitBoxScene: SceneBase { - // Dedicated fixture: this scene runs `jj commit`, which mutates the repo. - override class var fixtureName: String { "simple-commit" } + override class var fixtureName: String { + "commit" + } func testCommitClearsDraft() throws { let app = try XCTUnwrap(app) diff --git a/shell/mac/Tests/JayJayUITests/Scenes/DiffEditScene.swift b/shell/mac/Tests/JayJayUITests/Scenes/DiffEditScene.swift new file mode 100644 index 00000000..d2d0f103 --- /dev/null +++ b/shell/mac/Tests/JayJayUITests/Scenes/DiffEditScene.swift @@ -0,0 +1,42 @@ +import XCTest + +final class DiffEditScene: SceneBase { + override class var fixtureName: String { + "complex" + } + + func testLargeRepositoryStartsCollapsedAndCanExpand() throws { + let app = try XCTUnwrap(app) + XCTAssertTrue(dagRows(of: app).element(boundBy: 0).waitForExistence(timeout: 10), "DAG never populated") + + let open = app.buttons[AID.DiffEdit.open] + XCTAssertTrue(open.waitForExistence(timeout: 5), "Edit Diff button did not appear") + open.click() + + let binaryCard = app.buttons[AID.DiffEdit.fileToggle("assets/logo.bin")] + let configCard = app.buttons[AID.DiffEdit.fileToggle("config/environments/development.json")] + XCTAssertTrue(binaryCard.waitForExistence(timeout: 5), "Binary diff-edit file card did not appear") + XCTAssertTrue(configCard.waitForExistence(timeout: 5), "Text diff-edit file card did not appear") + XCTAssertEqual(binaryCard.value as? String, "collapsed", "Large repository should start auto-collapsed") + XCTAssertEqual(configCard.value as? String, "collapsed", "Large repository should start auto-collapsed") + + let expandAll = app.buttons[AID.DiffEdit.expandAll] + XCTAssertTrue(expandAll.waitForExistence(timeout: 5), "Expand All button did not appear") + expandAll.click() + XCTAssertTrue( + app.textViews[AID.Diff.text].firstMatch.waitForExistence(timeout: 10), + "Expand All did not restore file previews" + ) + + let preview = app.textViews[AID.Diff.text].firstMatch + let collapseAll = app.buttons[AID.DiffEdit.collapseAll] + XCTAssertTrue(collapseAll.waitForExistence(timeout: 5), "Collapse All button did not appear") + collapseAll.click() + XCTAssertTrue(preview.waitForNonExistence(timeout: 5), "Collapse All did not hide file previews") + + let cancel = app.buttons[AID.DiffEdit.cancel] + XCTAssertTrue(cancel.waitForExistence(timeout: 5), "Cancel button did not appear") + cancel.click() + XCTAssertTrue(open.waitForExistence(timeout: 5), "Cancel did not leave diff-edit mode") + } +} diff --git a/shell/mac/Tests/JayJayUITests/Scenes/DiffStatsReloadScene.swift b/shell/mac/Tests/JayJayUITests/Scenes/DiffStatsReloadScene.swift index faa4c35b..a0550f8a 100644 --- a/shell/mac/Tests/JayJayUITests/Scenes/DiffStatsReloadScene.swift +++ b/shell/mac/Tests/JayJayUITests/Scenes/DiffStatsReloadScene.swift @@ -1,8 +1,9 @@ import XCTest final class DiffStatsReloadScene: SceneBase { - // Dedicated fixture: restoring a file mutates @, which must not leak into other scenes. - override class var fixtureName: String { "simple-diffstats" } + override class var fixtureName: String { + "diff-stats" + } func testDiffStatsReloadAfterAmend() throws { let app = try XCTUnwrap(app) diff --git a/shell/mac/Tests/JayJayUITests/Scenes/FileContextMenuScene.swift b/shell/mac/Tests/JayJayUITests/Scenes/FileContextMenuScene.swift index 102db608..c1bfa345 100644 --- a/shell/mac/Tests/JayJayUITests/Scenes/FileContextMenuScene.swift +++ b/shell/mac/Tests/JayJayUITests/Scenes/FileContextMenuScene.swift @@ -10,9 +10,7 @@ final class FileContextMenuScene: SceneBase { let app = try XCTUnwrap(app) XCTAssertTrue(dagRows(of: app).element(boundBy: 0).waitForExistence(timeout: 10), "DAG never populated") - let root = ProcessInfo.processInfo.environment["JAYJAY_FIXTURE_ROOT"] ?? "/tmp/jayjay-test-fixtures" - let expectedPath = URL(fileURLWithPath: root) - .appendingPathComponent(Self.fixtureName) + let expectedPath = try XCTUnwrap(fixtureURL) .appendingPathComponent("wip1.txt") .path NSPasteboard.general.clearContents() diff --git a/shell/mac/Tests/JayJayUITests/Scenes/NewChangeScene.swift b/shell/mac/Tests/JayJayUITests/Scenes/NewChangeScene.swift index 98a67799..1fce0e1e 100644 --- a/shell/mac/Tests/JayJayUITests/Scenes/NewChangeScene.swift +++ b/shell/mac/Tests/JayJayUITests/Scenes/NewChangeScene.swift @@ -1,8 +1,9 @@ import XCTest final class NewChangeScene: SceneBase { - // Dedicated fixture so the mutation doesn't leak into ReviewSplitScene etc. - override class var fixtureName: String { "simple-newchange" } + override class var fixtureName: String { + "new-change" + } func testContextMenuNewChangeClearsDraft() throws { let app = try XCTUnwrap(app) diff --git a/shell/mac/Tests/JayJayUITests/Scenes/RepoListInitialRepositoryScene.swift b/shell/mac/Tests/JayJayUITests/Scenes/RepoListInitialRepositoryScene.swift index abb0ca8c..61f09d53 100644 --- a/shell/mac/Tests/JayJayUITests/Scenes/RepoListInitialRepositoryScene.swift +++ b/shell/mac/Tests/JayJayUITests/Scenes/RepoListInitialRepositoryScene.swift @@ -84,7 +84,7 @@ final class RepoListInitialRepositoryScene: SceneBase { XCTAssertTrue(repoWindow.waitForExistence(timeout: 5), "Initial repository window missing") openRepositoryTitleMenu(in: repoWindow) - let pinnedRepos = app.menuItems.matching(identifier: "simple-formats") + let pinnedRepos = app.menuItems.matching(identifier: "formats") XCTAssertTrue(pinnedRepos.firstMatch.waitForExistence(timeout: 3), "Pinned repository menu item missing") let pinnedRepo = try XCTUnwrap( pinnedRepos.allElementsBoundByIndex.first(where: \.isHittable), @@ -93,9 +93,9 @@ final class RepoListInitialRepositoryScene: SceneBase { pinnedRepo.click() XCTAssertTrue( - app.windows["simple-formats"].waitForExistence(timeout: 10), + app.windows["formats"].waitForExistence(timeout: 10), "Pinned repository did not open after the title menu closed" ) - return (repoWindow, app.windows["simple-formats"]) + return (repoWindow, app.windows["formats"]) } } diff --git a/shell/mac/Tests/JayJayUITests/Scenes/RepoListWindowScene.swift b/shell/mac/Tests/JayJayUITests/Scenes/RepoListWindowScene.swift index c04b7d80..66c29d4d 100644 --- a/shell/mac/Tests/JayJayUITests/Scenes/RepoListWindowScene.swift +++ b/shell/mac/Tests/JayJayUITests/Scenes/RepoListWindowScene.swift @@ -59,12 +59,12 @@ final class RepoListWindowScene: SceneBase { private func openSeededRepo(in app: XCUIApplication) -> XCUIElement { let recentRepo = app.buttons - .matching(NSPredicate(format: "label BEGINSWITH %@", "simple-formats,")) + .matching(NSPredicate(format: "label BEGINSWITH %@", "formats,")) .firstMatch XCTAssertTrue(recentRepo.waitForExistence(timeout: 5), "Seeded repository did not appear") recentRepo.click() - let repoWindow = app.windows["simple-formats"] + let repoWindow = app.windows["formats"] XCTAssertTrue(repoWindow.waitForExistence(timeout: 10), "Repository window did not appear") return repoWindow } diff --git a/shell/mac/Tests/JayJayUITests/Scenes/ReviewNotesScene.swift b/shell/mac/Tests/JayJayUITests/Scenes/ReviewNotesScene.swift index f46c8ef7..925f1f48 100644 --- a/shell/mac/Tests/JayJayUITests/Scenes/ReviewNotesScene.swift +++ b/shell/mac/Tests/JayJayUITests/Scenes/ReviewNotesScene.swift @@ -2,9 +2,8 @@ import AppKit import XCTest final class ReviewNotesScene: SceneBase { - /// Dedicated fixture: adding and resolving review notes writes shared review state. override class var fixtureName: String { - "simple-review-notes" + "review-notes" } func testAddAndResolveReviewNoteFromGutterMarker() throws { @@ -43,7 +42,7 @@ final class ReviewNotesScene: SceneBase { [ "func fibonacciReport(limit: Int) -> String {", " var values: [Int] = []", - " var a = 0", + " var a = 0" ].joined(separator: "\n"), "Review note header did not select the full context" ) diff --git a/shell/mac/Tests/JayJayUITests/Support/SceneBase.swift b/shell/mac/Tests/JayJayUITests/Support/SceneBase.swift index 48643a92..5e259d04 100644 --- a/shell/mac/Tests/JayJayUITests/Support/SceneBase.swift +++ b/shell/mac/Tests/JayJayUITests/Support/SceneBase.swift @@ -1,8 +1,9 @@ import XCTest -/// Fixtures are built by `just shell::ui-test-setup` into /tmp/jayjay-test-fixtures; override `fixtureName` to pick one such as `simple`, `simple-formats`, or `conflict`. +/// Fixtures are built by `just shell::ui-test-setup` into /tmp/jayjay-test-fixtures; mutating scenes use dedicated generated copies. class SceneBase: XCTestCase { var app: XCUIApplication? + private(set) var fixtureURL: URL? class var fixtureName: String { "simple" @@ -22,19 +23,25 @@ class SceneBase: XCTestCase { override func setUpWithError() throws { continueAfterFailure = false - let root = ProcessInfo.processInfo.environment["JAYJAY_FIXTURE_ROOT"] ?? "/tmp/jayjay-test-fixtures" - let reviewStorePath = "\(root)/\(Self.fixtureName)-review-store.json" + let rootPath = ProcessInfo.processInfo.environment["JAYJAY_FIXTURE_ROOT"] ?? "/tmp/jayjay-test-fixtures" + let root = URL(fileURLWithPath: rootPath, isDirectory: true) + let reviewStorePath = root.appendingPathComponent("\(Self.fixtureName)-review-store.json").path try? FileManager.default.removeItem(atPath: reviewStorePath) + let app = XCUIApplication() // Ignore restored windows so each test controls its initial app state. app.launchArguments = ["-ApplePersistenceIgnoreState", "YES"] if Self.opensFixtureOnLaunch { - app.launchArguments += ["--repo", "\(root)/\(Self.fixtureName)"] + let fixture = root.appendingPathComponent(Self.fixtureName, isDirectory: true) + fixtureURL = fixture + app.launchArguments += ["--repo", fixture.path] } else { app.launchArguments += ["-jayjay.lastOpenedRepo", ""] } app.launchEnvironment["JAYJAY_REVIEW_STORE_PATH"] = reviewStorePath - app.launchEnvironment["JAYJAY_REPOSITORIES_PATH"] = "\(root)/\(Self.repositoryStoreFixtureName)" + app.launchEnvironment["JAYJAY_REPOSITORIES_PATH"] = root + .appendingPathComponent(Self.repositoryStoreFixtureName) + .path for (key, value) in Self.launchEnvironment { app.launchEnvironment[key] = value } @@ -45,6 +52,7 @@ class SceneBase: XCTestCase { override func tearDownWithError() throws { app?.terminate() + fixtureURL = nil } // MARK: - Query helpers