Skip to content

Wrap the Verification panel at spaces and operators, one conjunct per line - #189

Merged
CatarinaGamboa merged 3 commits into
mainfrom
feat/187-panel-wrap
Nov 11, 2026
Merged

CatarinaGamboa merged 3 commits into
mainfrom
feat/187-panel-wrap

Conversation

@CatarinaGamboa

@CatarinaGamboa CatarinaGamboa commented Nov 10, 2026 •

Copy link
Copy Markdown
Collaborator

Closes #187

What changes

The Verification panel's Expected box and every Found line:

  • One line per top-level conjunct, with && at the end of each line except the last. An && inside parentheses or a string literal stays on its line. Nothing is split when a weaker operator (||, -->, ?:) sits outside the parentheses, so a line never looks like a conjunct of the whole expression.
  • A conditional outside parentheses shows its condition on one line and each branch on an indented line of its own (? …, : …). This is what makes A1_09 fit.
  • Wrapped lines have a hanging indent, so you can see where each line starts.
  • No breaks inside names. The VC cells used overflow-wrap: anywhere, which gave failures.siz|e(), compressionN|otExplicit and display(host|) ==. They now use break-word, so a line breaks at a space and splits a name only when the name alone is wider than the line.
  • Narrow panel (webview at most 420 px wide). Each binder (∀host) goes above its predicate, so the predicate gets the whole width. The card and the Found box get less horizontal padding, and the Found lines get line-height 1.4 instead of 1.6.

This is display only. The text of every expression is the same as before, only laid out on more lines. The hover and the CLI do not change.

Acceptance check: the six study tasks at the study width

Headless VS Code at 1600×900, Dark Modern, default side bar (300 px; the error card is about 240 px). Verifier 0.0.37, the released one, which is what this branch builds with. Each image shows main on the left and this PR on the right. The webview is 794 px tall. "Found ends at" is the bottom of the Found box, in webview px.

Task Found ends at (main → PR)
A2_10 503 → 453
A1_38 575 → 544
A1_01 562 → 521
A2_04 413 → 414
A1_56 498 → 473
A1_09 996 → 740: on main it ran below the fold

Expected and Found are fully visible in all six, with no name broken.

A1_09 A1_01
A2_10 A1_38
A2_04 A1_56

Verifier version

None needed. This works with 0.0.37 and with main.

Overlap with open PRs

Not changed

  • The Locations links (file names such as E_OSS_A1_01_SlideRecode|r.java) still break anywhere. They are in a <pre> with break-word, and the names are wider than the card. This was outside the issue.
  • Top-level || (A1_01's Expected) is not split into lines. The issue asks for conjuncts.

Checks

npm run test:unit (127 passed; 3 new tests, each fails on main), npm run test:typecheck, npm run lint, server mvn -B test (39 passed, unchanged).

🤖 Generated with Claude Code

@CatarinaGamboa

Copy link
Copy Markdown
Collaborator Author

Clean-code review

I audited this diff with the clean-code-principles checklist.

Fixed

  • highlighting.ts: splitConjuncts and getExpressionLines were exported but are only used inside the module, so they are module-private now (encapsulation, YAGNI).
  • renderHighlightedLines: the join("</span>") trick that closed spans is gone. Each line is now built explicitly, with the trailing arrow on the last line (KISS).

Declined

  • DRY with List Found as facts and show only readable simplification steps #185's splitConjuncts in vc-changes.ts. Both PRs are open, so the duplicate goes away when the second one lands, with one copy kept (noted in the body).
  • renderVCLine has two branches that both wrap in <span class="vc-node">. Merging them would move the arrow inside the animated diff highlight of changed lines, which would change how those look.
  • The 420px breakpoint is a literal, as the other breakpoints in styles.ts are. A CSS comment says what it is for.

@CatarinaGamboa

Copy link
Copy Markdown
Collaborator Author

Codex review, round 1

codex exec review --base origin/main:

  • [P2] Exclude character literals when locating conditional operators (highlighting.ts). In flag ? c == ':' : d, the colon inside ':' was taken for the conditional's separator.
  • [P2] Apply line formatting to changed VC predicates (vc-changes.ts). After stepping through the simplifications, a predicate that a step changed was rendered from its diff-highlighted HTML on one line, bypassing the line split.

Fixed (both)

  • topLevelIndices now skips single-quoted literals, with escapes, as well as strings. The existing line-split test covers it with c == ':'.
  • renderVCLine now takes a function that renders the text of each line, instead of finished HTML. A changed predicate is split into lines first, and each line is diffed against the old predicate. Whether the line counts as changed is decided on the whole predicate, as before. The two branches of renderVCLine are now one, which also clears the clean-code point I had declined. New test: keeps a conjunct per line in a predicate that a simplification changed.

Tests: 127 unit, typecheck, lint, and 39 server tests pass.

@CatarinaGamboa

Copy link
Copy Markdown
Collaborator Author

Codex review, round 2

  • [P2] Preserve change highlighting for repeated conjuncts (vc-changes.ts). Each line was diffed against the whole previous predicate. So for x > 0 becoming x > 0 && x > 0, the added x > 0 was not highlighted, because its tokens matched the old predicate.

Fixed. A changed predicate is now split into lines on both sides. The old and new lines are paired with the same alignChangedLines that pairs the lines of the chain. Each new line is diffed against the old line it replaces, and a new line that replaces none is highlighted whole. The test …marking a repeated conjunct it adds uses Codex's example and fails on round 1's code.

Tests: 127 unit, typecheck, lint, and 39 server tests pass.

@CatarinaGamboa

Copy link
Copy Markdown
Collaborator Author

Codex review, round 3

codex exec review --base origin/main: "I found no actionable regression in the changed code. The relevant unit tests pass."

The loop ends here: nothing to fix.

CatarinaGamboa and others added 3 commits November 11, 2026 14:22
… line

Expected and every Found line now show one top-level conjunct per line, and a
conditional's condition and branches each on a line of their own, with a
hanging indent for wrapped lines. The VC cells no longer break anywhere, so a
name is split only when it alone is wider than the line. A narrow panel puts
each binder above its predicate, with tighter padding and line height.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…erals

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@CatarinaGamboa
CatarinaGamboa merged commit c218968 into main Nov 11, 2026
2 of 3 checks passed
CatarinaGamboa added a commit that referenced this pull request Nov 11, 2026
Closes #188

## What changes
The editor diagnostic that `LJDiagnosticsHandler` publishes now carries:
1. the headline (`<title>: <message>`), as before;
2. then each of the verifier's **message notes**,
`LJDiagnostic.getMessageNotes()`, one per line;
3. then the verifier's **counterexample**, `getCounterexampleStr()`,
when it has one.

That text is what the hover and the Problems view show. **Hints are left
out**, since they stay collapsed in the panel by design.

The server reads the verifier's generic `LJDiagnostic` API, not a
per-error list. So any note the verifier adds later appears in the hover
with no extension change. That includes the planned From note
(liquid-java/liquidjava#522), and a counterexample for state errors once
the verifier gives one.

## Before → after (hover text, verifier 0.0.37)
**A2_04**
```diff
  Refinement Error: expected timeout >= 0 but could not prove it
+ Counterexample: timeout == -1
```
**A2_10**
```diff
  Refinement Error: failures.size() == size(this.failures) && failures.size() >= 0 is not a subtype of failures.size() > 0
+ Counterexample: failures.size() == 0
```
A1_38, A1_01, A1_56 and A1_09 are unchanged. Their errors have no
message notes and no counterexample in 0.0.37 or main.

| | Before | After |
|---|---|---|
| A2_04 | <img
src="https://raw.githubusercontent.com/liquid-java/vscode-liquidjava/pr-screenshots/190/A2_04-before.png"
width="440"> | <img
src="https://raw.githubusercontent.com/liquid-java/vscode-liquidjava/pr-screenshots/190/A2_04-after.png"
width="440"> |
| A2_10 | <img
src="https://raw.githubusercontent.com/liquid-java/vscode-liquidjava/pr-screenshots/190/A2_10-before.png"
width="440"> | <img
src="https://raw.githubusercontent.com/liquid-java/vscode-liquidjava/pr-screenshots/190/A2_10-after.png"
width="440"> |

(Headless VS Code, 1600×900, Dark Modern.)

## Verifier version
**0.0.37**, already released. `getMessageNotes()` and
`getCounterexampleStr()` are both in it, and the branch builds with the
current pom. Today the only message notes are the `where <alias> means
…` lines of refinement errors that use aliases, and no study task has
one. The From note will come with a later verifier release.

## Open question for the PM
The issue lists the `where … means …` gloss among the notes. In the
verifier, the gloss is a **declaration note** (`getDeclarationNotes()`,
liquid-java/liquidjava#517, needs 0.0.38). It explains the "declared
here" excerpt, which the hover does not show. So this PR shows only the
message notes, as specified. If the hover should also show the gloss,
that is one more `lines.addAll(diagnostic.getDeclarationNotes())`, but
it needs the 0.0.38 bump that #191 makes.

## Overlap with open PRs
Server only, in `LJDiagnosticsHandler`. No conflict with #185, #186,
#189 or #191.

## Checks
Server `mvn -B test` (40 passed; new `LJDiagnosticsHandlerTest`, which
fails on main), `npm run test:unit` (124 passed), `npm run
test:typecheck`, `npm run lint`.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
CatarinaGamboa added a commit that referenced this pull request Nov 11, 2026
Closes #181

## What changes
- **Server.** `RefinementErrorDTO` and `StateRefinementErrorDTO` now
send:
- `declarationPositions`, from `LJDiagnostic.getDeclarationPositions()`;
- `declarationNotes`, the `where … means …` lines, from
`getDeclarationNotes()`.

Both are read through the verifier's generic `LJDiagnostic` API. So when
the verifier adds notes or positions, they appear with no extension
change. `declarationPosition`, the first position, stays as it is for
the State Machine view.
- **Client.** The Locations section links to **every** declaration
position, then shows the notes under the links, muted and with a hanging
indent. A declaration in value form (`doorState(this) == closed`), or
one with unnamed sets, has no notes, so nothing changes for it.
- **Fallback to one position.** The verifier's default
`getDeclarationPositions()` returns the single
`getDeclarationPosition()`. Refinement errors and any other error with
one declaration therefore show one link, as before.

## Verifier version: needs 0.0.38 (not yet released)
| Part | Verifier change |
|---|---|
| `declarationNotes` (the gloss) | liquid-java/liquidjava#517 (merged in
main) |
| `declarationPositions` (every `from`, e.g. A1_01 lines 110 and 111) |
liquid-java/liquidjava#525 (**open**) |

The pom is bumped to `0.0.38`, so **CI's server build fails until 0.0.38
is on Maven Central**. Please merge this PR only after a verifier
release that has both #517 and #525. If #525 is not merged, drop the
`declarationPositions` lines from the two DTOs (one line each) and use
`declarationPosition` in `renderLocation` again. The gloss part only
needs #517.

I tested it locally against #525's head (`9d1bb1e`, which contains
main), installed as 0.0.38 in an isolated Maven repository.

## Before → after (Locations section, at the study width)
Before is main, with its server built on verifier main. After is this PR
on #525's head. Headless VS Code, 1600×900, Dark Modern, default side
bar.

**A1_01**: both `from`s, each with its gloss
```diff
  E_OSS_A1_01_SlideRecoder.java:136
  ImageWriteParamRefinements.java:110
+ ImageWriteParamRefinements.java:111
+ where compressionExplicit(this) means compression(this) == compressionExplicit
+ where compressionTypeSet(this) means compression(this) == compressionTypeSet
```
**A1_38**
```diff
  E_OSS_A1_38_FormatReaderManager.java:157
  BufferedInputStreamRefinements.java:81
+ where marked(this) means markState(this) == marked, open(this) means stream(this) == open
```
**A1_56**: `+ where redoable(this) means redoState(this) == redoable`
**A1_09**: `+ where notDisplayable(this) means display(this) ==
notDisplayable`
**A2_04**: unchanged. `@Refinement("_ >= 0")` has no gloss. The missing
Hint in the after shot comes from #525, not from this PR.

| A1_01 | A1_38 |
|---|---|
| <img
src="https://raw.githubusercontent.com/liquid-java/vscode-liquidjava/pr-screenshots/191/A1_01.png"
width="420"> | <img
src="https://raw.githubusercontent.com/liquid-java/vscode-liquidjava/pr-screenshots/191/A1_38.png"
width="420"> |

| A1_56 | A1_09 |
|---|---|
| <img
src="https://raw.githubusercontent.com/liquid-java/vscode-liquidjava/pr-screenshots/191/A1_56.png"
width="420"> | <img
src="https://raw.githubusercontent.com/liquid-java/vscode-liquidjava/pr-screenshots/191/A1_09.png"
width="420"> |

<details><summary>A2_04</summary>

<img
src="https://raw.githubusercontent.com/liquid-java/vscode-liquidjava/pr-screenshots/191/A2_04.png"
width="420">
</details>

The A1_01 and A1_09 shots are scrolled to Locations. The yellow outline
is the focus left on Hints by the screenshot script.

## Overlap with open PRs
- **#186 (Context tab)** may conflict in
`client/src/webview/views/sections.ts`. It changes
`renderVariableHighlightButton` and the imports, right after
`renderLocation`, which this PR edits.
- **#185 (Found as facts)** may conflict in
`client/src/types/diagnostics.ts`. Both PRs add fields to
`RefinementError` and `StateRefinementError`, in nearby hunks. No
conflict in logic.
- **#189 (panel wrapping)**: the notes are in the Locations `<pre>`,
which keeps `break-word`, so a long `compressionExplicit(this)` can
still wrap inside the name at 240 px (see A1_01). They do not share
files apart from `styles.ts`, in separate rules.

## Checks
`npm run test:unit` (125 passed; the new test fails on main and also
checks the copied text), `npm run test:typecheck`, `npm run lint`,
server `mvn -B test` (39 passed, against #525 as 0.0.38; the converter
test now also checks the notes and the positions).

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Verification panel: wrap at spaces and operators, one conjunct per line

1 participant