From c3f05d48da725d4c028877cd816e6b3ac4ae8c73 Mon Sep 17 00:00:00 2001 From: Wyatt Childers Date: Tue, 3 Nov 2026 16:23:50 -0500 Subject: [PATCH 1/2] Add diagnostic popovers to AcknowlEDG (+ minor QoL fixes) [] --- dev_tools/pylibs/edgacknowledg/__init__.py | 6 +- .../pylibs/edgacknowledg/client/shared.py | 26 ++ .../edgacknowledg/client/test/__init__.py | 87 ++++--- .../tests/test_acknowledg_integration.py | 19 ++ .../acknowledg-forge/tests/test_symlinks.py | 91 +++++++ dev_tools/services/acknowledg/PROTOCOL.md | 33 ++- .../acknowledg/client/src/AnchoredPopover.vue | 237 ++++++++++++++++++ .../acknowledg/client/src/AppButton.vue | 22 +- .../acknowledg/client/src/AppSelect.vue | 3 + .../acknowledg/client/src/BenchReviewView.vue | 23 +- .../acknowledg/client/src/BenchStatus.vue | 19 +- .../acknowledg/client/src/DiffLine.vue | 31 ++- .../acknowledg/client/src/MarkerButton.vue | 63 +++++ .../acknowledg/client/src/NumberedSource.vue | 137 ++++++++++ .../acknowledg/client/src/ReviewShell.vue | 113 +++++---- .../acknowledg/client/src/ReviewsView.vue | 4 +- .../acknowledg/client/src/SourcePopover.vue | 63 +++++ .../acknowledg/client/src/TestReviewView.vue | 211 ++++++++++++++-- .../acknowledg/client/src/config-color.js | 5 +- .../acknowledg/client/src/diagnostics.js | 156 ++++++++++++ .../services/acknowledg/client/src/sockets.js | 8 +- .../server/acknowledg_server/__init__.py | 28 ++- .../acknowledg_server/reviewer_requests.py | 24 +- 23 files changed, 1257 insertions(+), 152 deletions(-) create mode 100644 dev_tools/services/acknowledg/client/src/AnchoredPopover.vue create mode 100644 dev_tools/services/acknowledg/client/src/MarkerButton.vue create mode 100644 dev_tools/services/acknowledg/client/src/NumberedSource.vue create mode 100644 dev_tools/services/acknowledg/client/src/SourcePopover.vue create mode 100644 dev_tools/services/acknowledg/client/src/diagnostics.js diff --git a/dev_tools/pylibs/edgacknowledg/__init__.py b/dev_tools/pylibs/edgacknowledg/__init__.py index 61697fc2f7..9b21f50672 100644 --- a/dev_tools/pylibs/edgacknowledg/__init__.py +++ b/dev_tools/pylibs/edgacknowledg/__init__.py @@ -121,13 +121,17 @@ def request_tags_with_prefix(prefix: str) -> FrozenSet[str]: ALLOWED_BENCH_EDIT_TAGS ) -PROTOCOL_REVISION = 19 +PROTOCOL_REVISION = 20 # 10MiB MAX_MESSAGE_SIZE = 10485760 # 8MiB MAX_DISPLAY_CONTENT_SIZE = 8388608 +# Most lines shown on each side of the requested line by a ranged +# ``test.src.show-src`` request. +MAX_SOURCE_RANGE_CONTEXT = 50 + WebSocketsMessageType = Union[str, bytes] ProtocolMessage = Dict[str, Any] DecodableMessage = Union[WebSocketsMessageType, Mapping[str, Any]] diff --git a/dev_tools/pylibs/edgacknowledg/client/shared.py b/dev_tools/pylibs/edgacknowledg/client/shared.py index 675f781cc1..9d47963490 100644 --- a/dev_tools/pylibs/edgacknowledg/client/shared.py +++ b/dev_tools/pylibs/edgacknowledg/client/shared.py @@ -19,6 +19,7 @@ Dict, List, IO, + Iterable, Iterator, NamedTuple, Optional, @@ -37,6 +38,7 @@ from edgacknowledg import ( MAX_DISPLAY_CONTENT_SIZE, + MAX_SOURCE_RANGE_CONTEXT, ProtocolMessage, RequestTags, RichName, @@ -149,6 +151,30 @@ async def announce_section(channel: Channel, )) +def source_range_window(line: int, context: int) -> Tuple[int, int]: + '''First and last (1-based, inclusive) lines of a ranged request.''' + context = max(0, min(context, MAX_SOURCE_RANGE_CONTEXT)) + return max(1, line - context), max(1, line) + context + +def slice_source_range(lines: Iterable[str], line: int, + context: int) -> Dict[str, Union[str, int]]: + '''Reply fields for the window around ``line`` of ``lines``. + + ``lines`` may be a lazy iterable (such as an open file). It is consumed + in full to count the lines, but only the window is kept in memory. + ''' + first, last = source_range_window(line, context) + shown: List[str] = [] + total = 0 + for total, text in enumerate(lines, start = 1): + if first <= total <= last: + shown.append(text.rstrip('\n')) + return { + 'content': '\n'.join(shown), + 'first_line': min(first, max(total, 1)), + 'total_lines': total + } + def trunc_display_content(content: str) -> Dict[str, Union[str, bool]]: '''Truncate ``content`` for UI display; may set ``truncated``.''' max_content_size = MAX_DISPLAY_CONTENT_SIZE diff --git a/dev_tools/pylibs/edgacknowledg/client/test/__init__.py b/dev_tools/pylibs/edgacknowledg/client/test/__init__.py index c6a40c1b55..caacc92693 100644 --- a/dev_tools/pylibs/edgacknowledg/client/test/__init__.py +++ b/dev_tools/pylibs/edgacknowledg/client/test/__init__.py @@ -41,6 +41,7 @@ IllegalFileAccessError, SandboxContext, reviewer_of, + slice_source_range, trunc_display_content, ) from edgutil import eprint @@ -491,19 +492,41 @@ async def form_curr_diff_text(config_name: str, file_list.sort() + # The name the primary file is compiled (and so shown in + # diagnostics) as: "Test_name.c" unless the test's //name: directive + # says otherwise. + if test_path.exists(): + primary_name = edgtest.runtest.load_single_description( + test_path.path + ).name + else: + primary_name = edgtest.runtest.TestDescription().name + await channel.send(message( RequestTags.TEST_LIST_ASSOC_FILES, value = { 'reviewer_id': reviewer_id, 'client_response': { 'id': test_id, - 'files': file_list + 'files': file_list, + 'primary_name': primary_name } } )) elif request_tag == RequestTags.TEST_SHOW_SOURCE: test_id = request_value['id'] test_path = test_id_to_path(test_id) + # Both present for a ranged request, otherwise neither. + range_line: Optional[int] = request_value.get('line') + range_context: Optional[int] = request_value.get('context') + is_ranged = range_line is not None and range_context is not None + reply_base: Dict[str, Any] = { + 'id': test_id, + 'assoc_file': request_value.get('assoc_file') + } + if is_ranged: + reply_base['line'] = range_line + reply_base['context'] = range_context path_to_show: Optional[ConfinedPath] = None if ('assoc_file' in request_value and @@ -515,18 +538,9 @@ async def form_curr_diff_text(config_name: str, else: path_to_show = test_path + reply_fields: Dict[str, Any] if path_to_show is None or not path_to_show.exists(): - await channel.send(message( - RequestTags.TEST_SHOW_SOURCE, - value = { - 'reviewer_id': reviewer_id, - 'client_response': { - 'id': test_id, - 'assoc_file': request_value.get('assoc_file'), - 'content': None - } - } - )) + reply_fields = { 'content': None } elif path_to_show.suffix == '.ifc': result = await context.run_command('ifc-dump', str(path_to_show)) if result.timed_out: @@ -536,34 +550,37 @@ async def form_curr_diff_text(config_name: str, ) else: ifc_dump_output = result.output.decode(errors = 'replace') - - await channel.send(message( - RequestTags.TEST_SHOW_SOURCE, - value = { - 'reviewer_id': reviewer_id, - 'client_response': { - 'id': test_id, - 'assoc_file': request_value.get('assoc_file'), - **trunc_display_content(ifc_dump_output) - } - } - )) + if is_ranged: + assert range_line is not None and range_context is not None + reply_fields = slice_source_range( + ifc_dump_output.splitlines(), + range_line, + range_context + ) + else: + reply_fields = trunc_display_content(ifc_dump_output) + elif is_ranged: + assert range_line is not None and range_context is not None + with path_to_show.open(errors = 'replace') as file_handle: + reply_fields = slice_source_range( + file_handle, + range_line, + range_context + ) else: # Only as much as can be shown is read (one character more marks # the content as truncated). with path_to_show.open(errors = 'replace') as file_handle: shown = file_handle.read(MAX_DISPLAY_CONTENT_SIZE + 1) - await channel.send(message( - RequestTags.TEST_SHOW_SOURCE, - value = { - 'reviewer_id': reviewer_id, - 'client_response': { - 'id': test_id, - 'assoc_file': request_value.get('assoc_file'), - **trunc_display_content(shown) - } - } - )) + reply_fields = trunc_display_content(shown) + + await channel.send(message( + RequestTags.TEST_SHOW_SOURCE, + value = { + 'reviewer_id': reviewer_id, + 'client_response': { **reply_base, **reply_fields } + } + )) else: await channel.send(message( RequestTags.INVALID_REQUEST, diff --git a/dev_tools/services/acknowledg-forge/tests/test_acknowledg_integration.py b/dev_tools/services/acknowledg-forge/tests/test_acknowledg_integration.py index 1674a88510..ed92fa4ce7 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_acknowledg_integration.py +++ b/dev_tools/services/acknowledg-forge/tests/test_acknowledg_integration.py @@ -247,6 +247,25 @@ async def test_publish_heal_and_survive_bad_requests( async with websockets.connect( f"ws://127.0.0.1:{server.port}/review/{review.review_id}" ) as reviewer: + # A malformed line range is rejected by the server (the connection and + # the section carry on). + for bad_range in ({'line': 3}, {'context': 3}, + {'line': 0, 'context': 3}, + {'line': 3, 'context': -1}, + {'line': 3, 'context': 51}, + {'line': True, 'context': 3}, + {'line': 3, 'context': True}): + await reviewer.send(encode_message(message( + 'test.src.show-src', + value = {'id': 'suite/x.sft.cpp', 'assoc_file': None, **bad_range}, + section = 'test-changes' + ))) + tag = None + while tag != 'invalid': + tag, _, _ = decode_response( + await asyncio.wait_for(reviewer.recv(), 10) + ) + assert tag != 'test.src.show-src', bad_range await reviewer.send(encode_message(message( 'test.src.list-statuses', section = 'test-changes' diff --git a/dev_tools/services/acknowledg-forge/tests/test_symlinks.py b/dev_tools/services/acknowledg-forge/tests/test_symlinks.py index efb70626b4..c10e0472f5 100644 --- a/dev_tools/services/acknowledg-forge/tests/test_symlinks.py +++ b/dev_tools/services/acknowledg-forge/tests/test_symlinks.py @@ -597,6 +597,97 @@ def read(size: int = -1) -> Any: assert reads == [65] +async def test_show_source_range(tmp_path: Path, secret: Path) -> None: + home, root = _test_tree(tmp_path, secret) + (home / 'tests' / 'suite' / 'ten.sft.cpp').write_text( + ''.join(f'line {n}\n' for n in range(1, 11)) + ) + context = SandboxContext(home, allow_symlinks = False) + handler = test_provider.TestSourceHandler( + 'Test', + 'test', + context, + context.subpath(home), + context.subpath(root) + ) + + async def ask(test_id: str, line: int, ctx: int) -> dict[str, Any]: + channel = FakeChannel(message( + RequestTags.TEST_SHOW_SOURCE, + value = {'id': test_id, 'assoc_file': None, 'line': line, + 'context': ctx, 'reviewer_id': 'r'} + )) + await handler.handle_request(channel) # type: ignore[arg-type] + [reply] = channel.sent + response: dict[str, Any] = reply['v']['client_response'] + return response + + # The window is returned, and the request echoed. + response = await ask('suite/ten.sft.cpp', 5, 2) + assert response['content'] == 'line 3\nline 4\nline 5\nline 6\nline 7' + assert response['first_line'] == 3 and response['total_lines'] == 10 + assert response['line'] == 5 and response['context'] == 2 + assert 'truncated' not in response + + # ...clamped at either end of the file. + response = await ask('suite/ten.sft.cpp', 1, 2) + assert response['content'] == 'line 1\nline 2\nline 3' + assert response['first_line'] == 1 + response = await ask('suite/ten.sft.cpp', 10, 3) + assert response['content'] == 'line 7\nline 8\nline 9\nline 10' + assert response['first_line'] == 7 + + # A missing file is reported as such. + response = await ask('suite/missing.sft.cpp', 5, 2) + assert response['content'] is None and response['line'] == 5 + + # Without a range the whole file comes back, as before. + channel = FakeChannel(message( + RequestTags.TEST_SHOW_SOURCE, + value = {'id': 'suite/ten.sft.cpp', 'assoc_file': None, + 'reviewer_id': 'r'} + )) + await handler.handle_request(channel) # type: ignore[arg-type] + [reply] = channel.sent + response = reply['v']['client_response'] + assert response['content'].count('\n') == 10 + assert 'first_line' not in response and 'line' not in response + + +async def test_list_assoc_files_reports_primary_name( + tmp_path: Path, + secret: Path) -> None: + home, root = _test_tree(tmp_path, secret) + suite = home / 'tests' / 'suite' + (suite / 'default.sft.cpp').write_text('int x;\n') + (suite / 'renamed.sft.cpp').write_text( + '//name:t\u00e9st.cpp\n//options:\nint x;\n' + ) + context = SandboxContext(home, allow_symlinks = False) + handler = test_provider.TestSourceHandler( + 'Test', + 'test', + context, + context.subpath(home), + context.subpath(root) + ) + + async def primary_name_of(test_id: str) -> Any: + channel = FakeChannel(message( + RequestTags.TEST_LIST_ASSOC_FILES, + value = {'id': test_id, 'reviewer_id': 'r'} + )) + await handler.handle_request(channel) # type: ignore[arg-type] + [reply] = channel.sent + return reply['v']['client_response']['primary_name'] + + # The //name: directive names the primary file in diagnostics... + assert await primary_name_of('suite/renamed.sft.cpp') == 't\u00e9st.cpp' + # ...otherwise it is the default. + assert await primary_name_of('suite/default.sft.cpp') == 'Test_name.c' + assert await primary_name_of('suite/missing.sft.cpp') == 'Test_name.c' + + async def test_sandbox_context_commands(tmp_path: Path) -> None: (tmp_path / 'home').mkdir() (tmp_path / 'home' / 'file').write_text('contents\n') diff --git a/dev_tools/services/acknowledg/PROTOCOL.md b/dev_tools/services/acknowledg/PROTOCOL.md index 07188c5904..b40dd98ddd 100644 --- a/dev_tools/services/acknowledg/PROTOCOL.md +++ b/dev_tools/services/acknowledg/PROTOCOL.md @@ -3,7 +3,7 @@ This document describes the WebSocket protocol used by the AcknowlEDG server, the Vue client, and the `edgy-review` / `edg-bench-review` / `edg-acknowledg-cli` providers. -The current protocol revision is **19**. +The current protocol revision is **20**. All frames are JSON text. Implementations live in `edgacknowledg` (`dev_tools/pylibs/edgacknowledg`) and must stay in sync with the JavaScript @@ -74,15 +74,15 @@ work can flow immediately. Providers may optionally send identity updates at any time (on dedicated sockets or nested in `batch.event`): ```json -{ "t": "meta.section.name", "r": 19, "v": "" } +{ "t": "meta.section.name", "r": 20, "v": "" } ``` ```json -{ "t": "meta.review.name", "r": 19, "v": [ ] } +{ "t": "meta.review.name", "r": 20, "v": [ ] } ``` ```json -{ "t": "meta.review.timestamp", "r": 19, "v": "" } +{ "t": "meta.review.timestamp", "r": 20, "v": "" } ``` - `meta.section.name` sets the display name for the section tab (shown in @@ -267,7 +267,7 @@ adding `reviewer_id` so the reply can be routed back to this reviewer. | `test.src.show-diff-curr` | `{ "id", "configs": [string] }` | test source | | `test.src.show-output` | string test id | test source | | `test.src.list-assoc-files` | string test id | test source | -| `test.src.show-src` | `{ "id", "assoc_file": string \| null }` | test source | +| `test.src.show-src` | `{ "id", "assoc_file": string \| null, "line"?: int, "context"?: int }` | test source | | `test.edit.update-diff` | `{ "id", "configs": [string] }` | test editor | | `bench.src.list-statuses` | empty | bench source | | `bench.src.show-annotate` | string bench id | bench source | @@ -329,7 +329,7 @@ content that isn't the provider user's own, one reached through a symlink), the provider doesn't answer the request. It reports the reviewer instead: ```json -{ "t": "illegal-access", "r": 19, "v": { "reviewer_id": "" } } +{ "t": "illegal-access", "r": 20, "v": { "reviewer_id": "" } } ``` The server then closes that reviewer's socket with code **3001**, without @@ -345,7 +345,7 @@ access), it doesn't answer the request either. It tells the reviewer it failed, through the server, and keeps serving the section: ```json -{ "t": "invalid", "r": 19, "v": { "reviewer_id": "" } } +{ "t": "invalid", "r": 20, "v": { "reviewer_id": "" } } ``` The server sends `invalid` to that reviewer alone, with `v` `null` and `s` @@ -402,9 +402,18 @@ match the representative. **`test.src.list-assoc-files`** ```json -{ "id": "", "files": ["relative/posix/path", "..."] } +{ + "id": "", + "files": ["relative/posix/path", "..."], + "primary_name": "" +} ``` +`primary_name` is the file name the primary source appears under in +diagnostics (`Test_name.c` unless the test's `//name:` directive overrides it; +any Unicode name is allowed). The primary file itself is not in `files`; it is +`assoc_file: null` in `test.src.show-src`. + **`test.src.show-src`** ```json @@ -416,6 +425,14 @@ match the representative. } ``` +The request may carry `line` (1-based, ≥ 1) and `context` (0 to 50), both or +neither (otherwise the server answers `invalid`). Then `content` holds only +lines `line - context` through `line + context` (clamped to the file, joined +with `\n`, no trailing newline), `line` and `context` are echoed back so the +reviewer can match the reply, and the reply adds `first_line` (the 1-based +number of the first returned line) and `total_lines`. `truncated` is not +sent. A missing file still yields `content: null`. + **`bench.src.list-statuses`** ```json diff --git a/dev_tools/services/acknowledg/client/src/AnchoredPopover.vue b/dev_tools/services/acknowledg/client/src/AnchoredPopover.vue new file mode 100644 index 0000000000..357db7ac75 --- /dev/null +++ b/dev_tools/services/acknowledg/client/src/AnchoredPopover.vue @@ -0,0 +1,237 @@ + + + + + + + diff --git a/dev_tools/services/acknowledg/client/src/AppButton.vue b/dev_tools/services/acknowledg/client/src/AppButton.vue index b2d481f3f3..6d2d1b2ec4 100644 --- a/dev_tools/services/acknowledg/client/src/AppButton.vue +++ b/dev_tools/services/acknowledg/client/src/AppButton.vue @@ -9,6 +9,11 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception import { computed } from 'vue' import { RouterLink } from 'vue-router' + // Enter on a focused button or link activates that control. The review + // views also bind Enter on the document (rerecord / update baseline), whose + // preventDefault() would otherwise suppress the native click, so every + // AppButton stops Enter from reaching it. Use `bare` for controls with + // their own look (e.g. icon buttons) that still need this behavior. defineOptions({ inheritAttrs: false }) const props = defineProps({ @@ -21,6 +26,11 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception type: Boolean, default: false }, + // Omit the shared button chrome; the caller styles the control. + bare: { + type: Boolean, + default: false + }, // Toggle / selected state for mode buttons (maps to aria-pressed). pressed: { type: Boolean, @@ -66,27 +76,27 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + @click="onClick" + @keydown.enter.stop> diff --git a/dev_tools/services/acknowledg/client/src/AppSelect.vue b/dev_tools/services/acknowledg/client/src/AppSelect.vue index b7b07ffab2..071c96c165 100644 --- a/dev_tools/services/acknowledg/client/src/AppSelect.vue +++ b/dev_tools/services/acknowledg/client/src/AppSelect.vue @@ -184,6 +184,8 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception if (event.key === 'ArrowDown' || event.key === 'ArrowUp' || event.key === 'Enter' || event.key === ' ') { event.preventDefault() + // Enter opens the list; keep it from the review views' shortcut. + event.stopPropagation() openList() } } @@ -217,6 +219,7 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception } if (event.key === 'Enter' || event.key === ' ') { event.preventDefault() + event.stopPropagation() if (activeIndex.value >= 0) { selectIndex(activeIndex.value) triggerRef.value?.focus() diff --git a/dev_tools/services/acknowledg/client/src/BenchReviewView.vue b/dev_tools/services/acknowledg/client/src/BenchReviewView.vue index 63484d1cd0..b414c930b4 100644 --- a/dev_tools/services/acknowledg/client/src/BenchReviewView.vue +++ b/dev_tools/services/acknowledg/client/src/BenchReviewView.vue @@ -177,7 +177,10 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception const sorted = benchmarks.value.slice() sorted.sort((left, right) => { const leftChange = computePercentChange(left.baseline_ops, left.run_ops) - const rightChange = computePercentChange(right.baseline_ops, right.run_ops) + const rightChange = computePercentChange( + right.baseline_ops, + right.run_ops + ) if (sortMode.value === 'greatest-speedup') { return rightChange - leftChange } @@ -353,7 +356,9 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
Average Change (OPs)
{{ formatSignedOps(Math.round(mean(sampleOpDeltas))) }} - σ {{ Math.round(populationStdev(sampleOpDeltas)).toLocaleString() }} + σ {{ + Math.round(populationStdev(sampleOpDeltas)).toLocaleString() + }}
@@ -412,7 +417,8 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception 
{{ displayedOutput }}
-
+
Select a benchmark to continue...
@@ -444,9 +450,14 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
- No benchmark source is connected for this review. - Start edg-bench-review with the same review UUID, or open the - Tests tab if a test source is available. + +
+ No benchmark source is connected for this review. + Start edg-bench-review with the same review UUID, or open + the Tests tab if a test source is available. +
diff --git a/dev_tools/services/acknowledg/client/src/BenchStatus.vue b/dev_tools/services/acknowledg/client/src/BenchStatus.vue index dbdc14b872..b55fe87065 100644 --- a/dev_tools/services/acknowledg/client/src/BenchStatus.vue +++ b/dev_tools/services/acknowledg/client/src/BenchStatus.vue @@ -6,8 +6,16 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception --> diff --git a/dev_tools/services/acknowledg/client/src/MarkerButton.vue b/dev_tools/services/acknowledg/client/src/MarkerButton.vue new file mode 100644 index 0000000000..556d66a9c4 --- /dev/null +++ b/dev_tools/services/acknowledg/client/src/MarkerButton.vue @@ -0,0 +1,63 @@ + + + + + + + diff --git a/dev_tools/services/acknowledg/client/src/NumberedSource.vue b/dev_tools/services/acknowledg/client/src/NumberedSource.vue new file mode 100644 index 0000000000..27a1dc9272 --- /dev/null +++ b/dev_tools/services/acknowledg/client/src/NumberedSource.vue @@ -0,0 +1,137 @@ + + + + + + + diff --git a/dev_tools/services/acknowledg/client/src/ReviewShell.vue b/dev_tools/services/acknowledg/client/src/ReviewShell.vue index 84ccc60afa..d91df086ea 100644 --- a/dev_tools/services/acknowledg/client/src/ReviewShell.vue +++ b/dev_tools/services/acknowledg/client/src/ReviewShell.vue @@ -9,7 +9,9 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception import { computed, ref, provide, onUnmounted, watch } from 'vue' import { useRoute, useRouter, RouterView } from 'vue-router' + import AnchoredPopover from './AnchoredPopover.vue' import AppButton from './AppButton.vue' + import LoadingSpinner from './LoadingSpinner.vue' import Notice from './Notice.vue' import RichName from './RichName.vue' import { acquireSocketForReview } from './sockets.js' @@ -21,12 +23,20 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception const socket = acquireSocketForReview(reviewId) const socketOpen = ref(true) + // Whether the server has told us which sources and editors are connected + // yet. Until then "no source is connected" would be a guess. + const connectionStateReceived = ref(false) const connectionState = ref({ name: [], sources: [], editors: [] }) const notices = ref([]) + + // The review-info popover is toggled by clicking the icon (it closes on + // Escape or a click elsewhere, see AnchoredPopover.vue). + const reviewInfoRef = ref(null) + const reviewInfoOpen = ref(false) const pendingTasks = ref({}) let notificationIdCounter = 0 @@ -151,6 +161,7 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception const message = JSON.parse(event.data) if (message.t === 'review.connection-state') { connectionState.value = message.v + connectionStateReceived.value = true } else if (message.t === 'test.edit.update-diff') { completePendingTask('test.recording-update') const sectionLabel = sectionNameForSlug(message.s, 'test') @@ -240,20 +251,27 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception @@ -319,7 +344,10 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception border-bottom: 1px solid rgba(127, 127, 127, 0.35); } .review-info { - position: relative; + padding: 0; + border: none; + background: none; + font: inherit; display: flex; align-items: center; margin-left: 4px; @@ -341,46 +369,14 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception text-align: center; color: white; background-color: darkgrey; - cursor: default; + cursor: pointer; transition: background-color .4s; } .review-info:hover .review-info-icon, - .review-info:focus-within .review-info-icon { + .review-info:focus-visible .review-info-icon, + .review-info[aria-expanded="true"] .review-info-icon { background-color: black; } - .review-info-popover { - position: absolute; - top: calc(100% + 6px); - left: 0; - z-index: 100; - width: max-content; - max-width: min(600px, 70vw); - padding: 6px 12px; - border: 1px solid rgba(127, 127, 127, 0.35); - border-radius: 15px; - box-shadow: 0 2px 8px rgba(0, 0, 0, 0.25); - font-size: 14px; - overflow-wrap: anywhere; - visibility: hidden; - opacity: 0; - transition: opacity .2s, visibility .2s; - } - /* Spans the gap to the icon, so the pointer can move onto the popover - (e.g. to follow a link) without it closing. */ - .review-info-popover::before { - content: ''; - position: absolute; - left: 0; - right: 0; - bottom: 100%; - height: 8px; - } - /* Kept open while focus is inside it, e.g. on a link. */ - .review-info:hover .review-info-popover, - .review-info:focus-within .review-info-popover { - visibility: visible; - opacity: 1; - } .tabs-end { margin-left: auto; display: flex; @@ -405,6 +401,13 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception flex: 1; min-height: 0; } + .initial-loader { + height: 70vh; + display: flex; + align-items: center; + justify-content: center; + font-size: 50px; + } .connection-loss { height: 100vh; display: flex; diff --git a/dev_tools/services/acknowledg/client/src/ReviewsView.vue b/dev_tools/services/acknowledg/client/src/ReviewsView.vue index 1dd119e6b0..308526d80a 100644 --- a/dev_tools/services/acknowledg/client/src/ReviewsView.vue +++ b/dev_tools/services/acknowledg/client/src/ReviewsView.vue @@ -220,7 +220,9 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception - {{ describeReviewtype(review) }} + + {{ describeReviewtype(review) }} + diff --git a/dev_tools/services/acknowledg/client/src/SourcePopover.vue b/dev_tools/services/acknowledg/client/src/SourcePopover.vue new file mode 100644 index 0000000000..b79780413a --- /dev/null +++ b/dev_tools/services/acknowledg/client/src/SourcePopover.vue @@ -0,0 +1,63 @@ + + + + + + + diff --git a/dev_tools/services/acknowledg/client/src/TestReviewView.vue b/dev_tools/services/acknowledg/client/src/TestReviewView.vue index ccc35b99a9..f4f3f05808 100644 --- a/dev_tools/services/acknowledg/client/src/TestReviewView.vue +++ b/dev_tools/services/acknowledg/client/src/TestReviewView.vue @@ -19,10 +19,18 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception import { protocolMessage } from './sockets.js' import { computeGradientStyleForConfigs } from './config-color.js' import { formatTestAnchor, parseTestAnchor } from './review-anchor.js' + import { + parseDiagnostics, + DEFAULT_PRIMARY_SOURCE_NAME, + parseDiffDiagnostics, + resolveDiagnosticFile + } from './diagnostics.js' import DiffLine from './DiffLine.vue' import LoadingSpinner from './LoadingSpinner.vue' + import NumberedSource from './NumberedSource.vue' import ScrollableContentArea from './ScrollableContentArea.vue' + import SourcePopover from './SourcePopover.vue' import TestStatus from './TestStatus.vue' import AppButton from './AppButton.vue' import AppSelect from './AppSelect.vue' @@ -42,6 +50,9 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception const displayedDiffHash = ref("") const displayedGroupConfigs = ref([]) const displayedTestAssocFiles = ref(null) + // How the primary source file is named in diagnostics (from the same reply + // as the associated files, so valid when those are non-null). + const displayedTestPrimaryName = ref(DEFAULT_PRIMARY_SOURCE_NAME) const displayedTestAssocFileName = ref(null) const displayTruncated = ref(false) const currDiffDivergenceNotice = ref(null) @@ -61,8 +72,139 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception const clearContentArea = () => { displayedOutput.value = null + closeSourcePopover() + } + + // Lines of context shown on each side of a diagnostic's line. + const POPOVER_CONTEXT_LINES = 10 + + // The source popover opened from a diagnostic marker in the output view. + // `source` is null while loading, see SourcePopover.vue. + const sourcePopover = ref({ + open: false, + anchor: null, + location: null, + request: null, + source: null + }) + // Fetched snippets, per displayed test. + const sourceSnippetCache = new Map() + + const closeSourcePopover = () => { + sourcePopover.value = { + open: false, + anchor: null, + location: null, + request: null, + source: null + } + } + + const snippetKey = (assocFile, line) => { + return JSON.stringify( + [displayedTestName.value, assocFile, line, POPOVER_CONTEXT_LINES] + ) + } + + // The diff views' markers, by zero-based line. + const markersByDiffLine = computed(() => { + return new Map( + isDiffMode.value ? + outputMarkers.value.map((marker) => [marker.line - 1, marker]) : [] + ) + }) + + const openSourcePopover = (marker, anchor) => { + if (marker.disabled) { + return + } + const request = { + assocFile: marker.assocFile, + line: marker.location.line + } + const cached = sourceSnippetCache.get( + snippetKey(request.assocFile, request.line) + ) + sourcePopover.value = { + open: true, + anchor, + location: marker.location, + request, + source: cached ?? null + } + if (cached === undefined) { + sendSectionMessage('test.src.show-src', { + 'id': displayedTestName.value, + 'assoc_file': request.assocFile, + 'line': request.line, + 'context': POPOVER_CONTEXT_LINES + }) + } } + // Applies a ranged `test.src.show-src` reply. Replies for a popover that + // has since been closed or moved on are cached but not shown. + const onSourceSnippetReply = (reply) => { + if (reply.id !== displayedTestName.value) { + return + } + const source = reply.content === null ? + { missing: true } : + { content: reply.content, firstLine: reply.first_line } + sourceSnippetCache.set(snippetKey(reply.assoc_file, reply.line), source) + const request = sourcePopover.value.request + if (sourcePopover.value.open && request !== null && + request.assocFile === reply.assoc_file && + request.line === reply.line) { + sourcePopover.value = { ...sourcePopover.value, source } + } + } + + const isDiffMode = computed(() => { + return outputViewModeTarget.value === 'test.src.show-diff' || + outputViewModeTarget.value === 'test.src.show-diff-curr' + }) + + // Diagnostic markers for the compiler output view, and for the added lines + // of the diff views. + const outputMarkers = computed(() => { + if (typeof displayedOutput.value !== 'string') { + return [] + } + let diagnostics + if (outputViewModeTarget.value === 'test.src.show-output') { + diagnostics = parseDiagnostics(displayedOutput.value) + } else if (isDiffMode.value) { + diagnostics = parseDiffDiagnostics(displayedOutput.value) + } else { + return [] + } + return diagnostics.map((diagnostic) => { + const resolved = resolveDiagnosticFile( + diagnostic.file, + displayedTestAssocFiles.value, + displayedTestPrimaryName.value + ) + const location = { + file: diagnostic.file, + line: diagnostic.line, + col: diagnostic.col + } + const what = `${diagnostic.file}, line ${diagnostic.line}` + // Until the file list arrives nothing but the primary file resolves. + const listPending = displayedTestAssocFiles.value === null + return { + line: diagnostic.outputLine + 1, + location, + assocFile: resolved === null ? null : resolved.assocFile, + disabled: resolved === null, + title: resolved !== null ? `Show source at ${what}` : + listPending ? `${what}: locating source files...` : + `${what}: source file not available` + } + }) + }) + const configuredIdForDisplayedTest = () => { return `(${displayedTestConfig.value}) ${displayedTestName.value}` } @@ -83,15 +225,16 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception const requestContentAreaUpdate = () => { clearContentArea() + if (displayedTestAssocFiles.value === null) { + // Every view needs the associated files: the source view lists them, + // and diagnostic markers locate sources among them. Fetched once per + // test. + sendSectionMessage( + 'test.src.list-assoc-files', + displayedTestName.value + ) + } if (outputViewModeTarget.value === 'test.src.show-src') { - if (displayedTestAssocFiles.value === null) { - // If this is the first time seeing the test source for this test, - // fetch the list of associated files. - sendSectionMessage( - 'test.src.list-assoc-files', - displayedTestName.value - ) - } sendSectionMessage('test.src.show-src', { 'id': displayedTestName.value, 'assoc_file': displayedTestAssocFileName.value @@ -115,7 +258,9 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception displayedDiffHash.value !== group.diff_hash) { // Wipe out the state for the associated files. displayedTestAssocFiles.value = null + displayedTestPrimaryName.value = DEFAULT_PRIMARY_SOURCE_NAME displayedTestAssocFileName.value = null + sourceSnippetCache.clear() // Update the displayed selection. displayedTestName.value = testName displayedDiffHash.value = group.diff_hash @@ -279,7 +424,8 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception isMultiConfig.value = message.v.configs.length > 1 testStatuses.value = message.v.statuses applyHashSelection() - } else if (message.t === "test.src.show-diff" || message.t === "test.src.show-diff-curr" || + } else if (message.t === "test.src.show-diff" || + message.t === "test.src.show-diff-curr" || message.t === "test.src.show-output") { if (message.v.id === configuredIdForDisplayedTest()) { displayTruncated.value = false @@ -303,7 +449,12 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception } else if (message.t === "test.src.list-assoc-files") { if (message.v.id === displayedTestName.value) { displayedTestAssocFiles.value = message.v.files + displayedTestPrimaryName.value = + message.v.primary_name ?? DEFAULT_PRIMARY_SOURCE_NAME } + } else if (message.t === "test.src.show-src" && + message.v.line !== undefined) { + onSourceSnippetReply(message.v) } else if (message.t === "test.src.show-src") { if (message.v.id === displayedTestName.value) { displayTruncated.value = false @@ -462,7 +613,8 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-
+
@@ -499,21 +651,33 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception style="overflow: visible; display: grid;"> - + +
+ -
+
Select a test status difference to continue...
-
+
- No test source is connected for this review. - Start edgy-review with the same review UUID, or open the - Benchmarks tab if a benchmark source is available. + +
+ No test source is connected for this review. + Start edgy-review with the same review UUID, or open the + Benchmarks tab if a benchmark source is available. +
diff --git a/dev_tools/services/acknowledg/client/src/config-color.js b/dev_tools/services/acknowledg/client/src/config-color.js index 577924ec43..7e0ca6a5d6 100644 --- a/dev_tools/services/acknowledg/client/src/config-color.js +++ b/dev_tools/services/acknowledg/client/src/config-color.js @@ -33,7 +33,10 @@ export const computeMutedHSLStyleForConfig = (configName) => { return mutedHslCache[configName] } -export const computeGradientStyleForConfigs = (configs, { muted = false } = {}) => { +export const computeGradientStyleForConfigs = ( + configs, + { muted = false } = {} +) => { if (!configs || configs.length === 0) { return muted ? 'hsl(0 0% 40% / .6)' diff --git a/dev_tools/services/acknowledg/client/src/diagnostics.js b/dev_tools/services/acknowledg/client/src/diagnostics.js new file mode 100644 index 0000000000..98aa1bf3c2 --- /dev/null +++ b/dev_tools/services/acknowledg/client/src/diagnostics.js @@ -0,0 +1,156 @@ +// Part of the EDG Compiler Project, under the Apache License v2.0 with LLVM +// Exceptions. +// See https://edgcpp.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +// Finds the source locations EDG diagnostics point at in front-end output. +// +// Recognized forms (EDG's own output only): +// +// "file.c", line 12: error: message +// "file.c", line 12 (col. 5): warning: message +// detected during instantiation of "..." at line 17 +// the other declaration is at line 3 of "tu2.cpp" +// +// A whole diagnostic looks like this: +// +// "Test_name.c", line 8: error: bad thing <- header (line i) +// int line8(void); <- source excerpt (i+1) +// ^ <- caret line (i+2) +// detected during instantiation of "f" at line 22 +// <- continuation (i+3...) +// +// Every header, and every continuation that names a location, becomes one +// result, so each can get its own marker in the UI. The header's own file +// and line are what a marker opens; the excerpt and caret are not parsed, +// since the source itself can be shown instead. + +// The header of a diagnostic. Groups: 1 file, 2 line, 3 column (optional), +// 4 severity ("error", "warning", "note", "remark", "catastrophic error", +// "internal error", ...). Starts with a quote, so most lines are rejected +// by the first character. +const HEADER = /^"([^"]+)", line (\d+)(?: \(col\. (\d+)\))?: ([a-z][a-z -]*): / +// A location named on an indented continuation line, in another file: +// the other declaration is at line 3 of "tu2.cpp" +const CONT_OF_FILE = /\bline (\d+) of "([^"]+)"/ +// ...or in the same file as the header, which is not repeated: +// detected during instantiation of "..." at line 17 +const CONT_SAME_FILE = /\bat line (\d+)\b/ +// The caret line under a source excerpt: nothing but whitespace and "^". +const CARET = /^\s*\^\s*$/ + +// Returns [{ outputLine, file, line, col, severity }], with `outputLine` +// zero-based (an index into `text.split('\n')`) and `col` possibly null. +// Continuation lines report severity 'context'. +export const parseDiagnostics = (text) => { + const lines = text.split('\n') + const found = [] + // The file named by the most recent header, while its diagnostic is still + // in progress (so "at line N" continuations can be attributed to it); null + // otherwise. + let headerFile = null + // The source excerpt and caret under a header are program text, not + // diagnostic text. A line of source could contain anything, including + // `line 3 of "x.h"`, which would be misread as a continuation; so, while + // `i <= skipUntil`, lines are ignored. + let skipUntil = -1 + for (let i = 0; i < lines.length; i++) { + const lineText = lines[i] + // A header: record it, then move on to the next line. + if (lineText.charCodeAt(0) === 34 /* " */) { + const m = HEADER.exec(lineText) + if (m !== null) { + headerFile = m[1] + found.push({ + outputLine: i, + file: m[1], + line: parseInt(m[2], 10), + col: m[3] === undefined ? null : parseInt(m[3], 10), + severity: m[4] + }) + // The excerpt (i + 1) and caret (i + 2) follow. Recognized by the + // caret, so that a header with no excerpt (brief output) does not + // swallow its continuations. + if (i + 2 < lines.length && CARET.test(lines[i + 2])) { + skipUntil = i + 2 + } + continue + } + } + // Skip excerpt/caret lines, and anything outside a diagnostic. + if (i <= skipUntil || headerFile === null) { + continue + } + const c = lineText.charCodeAt(0) + if (c !== 32 && c !== 9) { + // Continuations are indented; anything else ends the diagnostic. + headerFile = null + continue + } + // An indented line may name another location, either in another file... + let m = CONT_OF_FILE.exec(lineText) + if (m !== null) { + found.push({ + outputLine: i, + file: m[2], + line: parseInt(m[1], 10), + col: null, + severity: 'context' + }) + continue + } + // ...or in the header's file. + m = CONT_SAME_FILE.exec(lineText) + if (m !== null) { + found.push({ + outputLine: i, + file: headerFile, + line: parseInt(m[1], 10), + col: null, + severity: 'context' + }) + } + } + return found +} + +// As `parseDiagnostics`, for a diff of outputs: only the added ("+") lines +// are searched, and `outputLine` indexes the diff's lines. +export const parseDiffDiagnostics = (text) => { + // Other lines are blanked rather than dropped, keeping indices stable and + // ending any diagnostic in progress. + const added = text.split('\n').map( + (lineText) => lineText.charCodeAt(0) === 43 /* + */ ? + lineText.slice(1) : '' + ) + return parseDiagnostics(added.join('\n')) +} + +// The name the test harness compiles the primary source file as, unless the +// test's //name: directive says otherwise. The server reports the actual +// name (`primary_name` of `test.src.list-assoc-files`). +export const DEFAULT_PRIMARY_SOURCE_NAME = 'Test_name.c' + +// Maps a file named by a diagnostic to the value of `assoc_file` for a +// `test.src.show-src` request: `null` is the test's primary file. +// +// `assocFiles` and `primaryName` come from the `test.src.list-assoc-files` +// reply; until it arrives (`assocFiles` is null) nothing can be located. +// +// Returns `{ assocFile }` when the file can be located, or `null` when it +// cannot (not in `assocFiles`, or several candidates with that name). +export const resolveDiagnosticFile = (file, assocFiles, primaryName) => { + if (assocFiles === null) { + return null + } + if (file === primaryName) { + return { assocFile: null } + } + const wanted = file.replace(/^(\.\/)+/, '') + if (assocFiles.includes(wanted)) { + return { assocFile: wanted } + } + const baseOf = (path) => path.slice(path.lastIndexOf('/') + 1) + const matches = assocFiles.filter((f) => baseOf(f) === baseOf(wanted)) + return matches.length === 1 ? { assocFile: matches[0] } : null +} diff --git a/dev_tools/services/acknowledg/client/src/sockets.js b/dev_tools/services/acknowledg/client/src/sockets.js index cd57a09e9a..bea1823d0e 100644 --- a/dev_tools/services/acknowledg/client/src/sockets.js +++ b/dev_tools/services/acknowledg/client/src/sockets.js @@ -5,7 +5,7 @@ See https://edgcpp.org/LICENSE.txt for license information. SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception */ -const PROTOCOL_REVISION = 19 +const PROTOCOL_REVISION = 20 const getSocketHost = () => { if (process.env.NODE_ENV === "development") { @@ -26,7 +26,11 @@ export const acquireSocketForReview = (reviewId) => { ) } -export const protocolMessage = (tag, value = undefined, section = undefined) => { +export const protocolMessage = ( + tag, + value = undefined, + section = undefined +) => { const message = { t: tag, r: PROTOCOL_REVISION } if (value !== undefined) { message.v = value diff --git a/dev_tools/services/acknowledg/server/acknowledg_server/__init__.py b/dev_tools/services/acknowledg/server/acknowledg_server/__init__.py index 4fa04997c6..105524441c 100644 --- a/dev_tools/services/acknowledg/server/acknowledg_server/__init__.py +++ b/dev_tools/services/acknowledg/server/acknowledg_server/__init__.py @@ -449,15 +449,21 @@ async def send_list_of_assoc_files_to(self, slug: str, async def send_test_source_to(self, slug: str, reviewer_id: uuid.UUID, test_id: str, - assoc_file: Optional[str]) -> None: + assoc_file: Optional[str], + line: Optional[int] = None, + context: Optional[int] = None) -> None: + value: dict[str, Any] = { + 'id': test_id, + 'assoc_file': assoc_file, + 'reviewer_id': reviewer_id + } + if line is not None: + value['line'] = line + value['context'] = context await self.send_to_test_source( slug, RequestTags.TEST_SHOW_SOURCE, - { - 'id': test_id, - 'assoc_file': assoc_file, - 'reviewer_id': reviewer_id - } + value ) async def update_test_diff_for(self, slug: str, reviewer_id: uuid.UUID, @@ -813,12 +819,18 @@ async def _dispatch_reviewer_request( test_id ) case (RequestTags.TEST_SHOW_SOURCE, - ShowTestSourceRequest(id = test_id, assoc_file = assoc_file)): + ShowTestSourceRequest(id = test_id, assoc_file = assoc_file, + line = line, context = context) as request): + if not request.is_valid_range(): + await send(message(RequestTags.INVALID_REQUEST)) + return await run_descr.send_test_source_to( section, reviewer_id, test_id, - assoc_file + assoc_file, + line, + context ) case (RequestTags.TEST_UPDATE_DIFF, UpdateDiffRequest(id = test_id, configs = configs)): diff --git a/dev_tools/services/acknowledg/server/acknowledg_server/reviewer_requests.py b/dev_tools/services/acknowledg/server/acknowledg_server/reviewer_requests.py index 37f18dbc18..67b9c9ee48 100644 --- a/dev_tools/services/acknowledg/server/acknowledg_server/reviewer_requests.py +++ b/dev_tools/services/acknowledg/server/acknowledg_server/reviewer_requests.py @@ -14,7 +14,11 @@ from serde import serde # type: ignore[import-not-found] -from edgacknowledg import ALLOWED_REVIEWER_REQUEST_TAGS, RequestTags +from edgacknowledg import ( + ALLOWED_REVIEWER_REQUEST_TAGS, + MAX_SOURCE_RANGE_CONTEXT, + RequestTags +) @serde(deny_unknown_fields = True) class EmptyRequest: @@ -33,9 +37,25 @@ class BenchId: @serde(deny_unknown_fields = True) class ShowTestSourceRequest: - '''Request to display a test's primary source or an associated file.''' + '''Request to display a test's primary source or an associated file. + + ``line`` and ``context`` (both or neither) restrict the reply to the lines + ``line - context`` through ``line + context`` (``line`` is 1-based). + ''' id: str assoc_file: Optional[str] = None + line: Optional[int] = None + context: Optional[int] = None + + def is_valid_range(self) -> bool: + if self.line is None and self.context is None: + return True + # Exact type checks: ``bool`` is an ``int`` subclass, so pyserde's + # strict mode lets ``true`` through as 1. + return ( + type(self.line) is int and type(self.context) is int and + self.line >= 1 and 0 <= self.context <= MAX_SOURCE_RANGE_CONTEXT + ) @serde(deny_unknown_fields = True) class ShowCurrDiffRequest: From 8601ce159e899a54d5220bcc8d3759b84038533b Mon Sep 17 00:00:00 2001 From: Wyatt Childers Date: Wed, 4 Nov 2026 12:24:12 -0500 Subject: [PATCH 2/2] Hide the curr diff button when no test editor is available [] --- .../acknowledg/client/src/TestReviewView.vue | 24 +++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/dev_tools/services/acknowledg/client/src/TestReviewView.vue b/dev_tools/services/acknowledg/client/src/TestReviewView.vue index f4f3f05808..8a9bbe6e0d 100644 --- a/dev_tools/services/acknowledg/client/src/TestReviewView.vue +++ b/dev_tools/services/acknowledg/client/src/TestReviewView.vue @@ -323,11 +323,26 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // Recordings are updated by the section's editor; a section without one // (e.g. served by acknowledg-forge) is read-only. - const canRerecord = computed(() => { + const hasTestEditor = computed(() => { return (connectionState.value.editors ?? []).some((editor) => { return editor.kind === 'test' && editor.slug === reviewSectionSlug.value }) }) + const canRerecord = hasTestEditor + + // We approximate canShowCurrentDiff with hasTestEditor because, in the + // AcknowlEDG stack today, the only way to have a meaningful current diff + // is to have an editor. This may change in the future. + const canShowCurrentDiff = hasTestEditor + + // The view mode outlives the editor (and the section), so fall back to the + // plain diff when the current diff view is no longer available. + watch(canShowCurrentDiff, (available) => { + if (!available && + outputViewModeTarget.value === 'test.src.show-diff-curr') { + setOutputMode('test.src.show-diff') + } + }) const rerecord = () => { beginPendingTask('test.recording-update') @@ -526,7 +541,7 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception previous() } else if (event.key === "z") { setOutputMode("test.src.show-diff") - } else if (event.key === "x") { + } else if (event.key === "x" && canShowCurrentDiff.value) { setOutputMode("test.src.show-diff-curr") } else if (event.key === "c") { setOutputMode("test.src.show-output") @@ -586,11 +601,12 @@ SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception - Static Diff (z) + {{ canShowCurrentDiff ? 'Static Diff' : 'Diff' }} (z)