feat(vscode): checkmark and hover for image reference results - #37
Merged
Merged
Conversation
A values file whose images all resolve now reads as verified rather than merely quiet: every reference a registry confirmed gets a checkmark after its repository value, and hovering any checked reference explains what happened — which registry answered, or why the reference could not be answered for at all. The three surfaces (diagnostics, checkmarks, hovers) are projections of one `ReferenceCheck` record built once per document open, so they cannot drift apart about a reference. The checkmark names the answering registry only when it differs from the host the file itself names; nothing can differ until the registry override set lands, but the rule belongs with the decoration that implements it. An unverifiable verdict now has a second thing it must never render as. It already never produces a diagnostic; it never produces a checkmark either, because positive confirmation the tool did not actually obtain is the same lie as a phantom error. Closes #21
VS Code does not await event listeners, so anything escaping the async `onDidOpenTextDocument` callback becomes an unhandled rejection, and an unhandled rejection terminates the extension host. Closing the tab while the registry check is still in flight was enough to trigger it, because `setDecorations` throws on an editor that has since been disposed. The window is as long as the network call. The listener now catches at that boundary and reports to the output channel. `applyCheckmarks` additionally guards each editor separately, so one disposed editor no longer aborts the loop and costs every other visible editor its checkmarks. Driving the real bundle in a Node harness with a `vscode` fake reproduced the fatal rejection before the fix and shows it surviving after. Both new tests were mutation-checked against the guards they cover.
A reference with no mark is indistinguishable from an extension that never ran, which is exactly how this landed in practice: most real references are unverifiable today, so a values file rendered nothing at all and looked broken rather than unanswered. Every checked reference now carries one inline mark. A green check for verified, a red cross beside the existing squiggle for a reference that does not exist, and a muted question mark for one the tool could not answer. The unverifiable mark is deliberately a question mark and deliberately muted. It reports a fact about the developer's machine, not a defect in the file, and styling it as a failure would recreate the cry-wolf problem the unverifiable verdict exists to prevent. The invariant it protects is untouched: unverifiable still produces no diagnostic and never reaches the Problems panel. Glyph and colour ride on each decoration rather than on the decoration type, so all three marks share one type and one setDecorations call per editor still replaces the lot. Which mark an outcome renders as is an exhaustive switch over the verdict union, so a new verdict kind fails the build here. This contradicts two of #21's acceptance criteria as literally written, which said a not-found reference renders only its diagnostic and an unverifiable one renders nothing. Raised on the PR.
Collaborator
Author
Deliberate deviation from #21's acceptance criteriaTwo criteria as literally written no longer hold, and the change is intentional:
Every checked reference now carries exactly one inline mark: Why. #21's stated purpose is that "a correct file looks verified rather than merely quiet". In practice the opposite happened. Almost every real reference is unverifiable today, because anonymous manifest requests to Docker Hub, GHCR, ECR, and ACR all answer The invariant is untouched. #17's rule is that unverifiable never produces a diagnostic, and the reasoning behind it is about the Problems panel: a missing credential "is a fact about the developer's machine, not a defect in the file, and putting it in the Problems panel next to real errors trains people to ignore the panel." A muted inline glyph is a different surface. The If a reviewer disagrees, the narrower alternative is to mark verified and not-found but leave unverifiable bare. I'd argue against it: that leaves the most common case today invisible. |
A reference with no tag was filtered out before any verdict existed, so it rendered nothing at all. That is the same failure the marks were added to fix: nothing is indistinguishable from a tool that never ran, and a tagless image is normal in charts that let the tag default to appVersion. Tagless references now come through as a check with a `no-tag` reason, so they carry the muted question mark and a hover that says the reference names no tag to check. No registry request is made for them. Resolving the tag from the chart's appVersion stays a later ticket; this only stops the reference from being invisible until then. `UncheckedReason` widens the registry package's `UnverifiableReason` with the outcomes the extension settles before it asks anything, and `ReferenceVerdict` widens `ImageVerdict` the same way. The reason table is keyed on the wider type, so a new reason still fails the build rather than hovering with no explanation. `TaggedImageReference` had no callers left and is gone; the two places that assumed a tag now read it as optional rather than asserting it away. Digest-pinned references parse as tagless and so pick up the same question mark, where #17 called for no marker at all. Left as is deliberately: we don't use digests, and telling them apart would mean surfacing the digest key out of the helm package for no gain.
extension.ts had grown to 367 lines holding the check pipeline, all three render surfaces, and the activation wiring. It is now 78 lines of wiring only, with each surface in its own module: reference-check owns what a values file is and what checking one produces, diagnostics, marks, and hover each own one surface, and source-range holds the two range helpers they share. Comments are cut back to the ones carrying a why the code cannot show. The MARKS table explained at length what its three entries already say; that is gone, leaving only the reason `unchecked` is a muted question mark rather than a cross. The test file is deliberately untouched in this commit. An unchanged suite passing is the evidence that no behaviour moved with the code; splitting the tests to mirror the modules is the next commit.
481 lines in one file became 640 across seven, and 25 tests became 33. Each module's rules are now asserted against that module directly instead of through `activate` and a fetch fake, which is what made the single file long. `extension.test.ts` keeps only what it is the right seam for: that the surfaces are registered and that an opened values file drives all three. `createFakeDocument` and `fakeFetchResponse` move to `test/`, alongside the vscode stub, since four test files now need them. One test pins behaviour rather than intent. `extractImageReferences` uses the yaml package's `parseDocument`, which collects syntax errors on the document instead of throwing, so a malformed values file yields zero references rather than reaching the catch in `checkImageReferencesInDocument`. The test says so, with a comment explaining why the result is empty checks.
`extractImageReferences` cannot throw on malformed YAML. It uses the yaml
package's `parseDocument`, which collects syntax errors on the document
rather than throwing, unlike `parse`. Fifteen malformed inputs were probed
across two independent runs and none threw, so the catch never ran and the
comment above it described behaviour this feature does not have.
Removing it loses no safety. The open listener's own catch, added when a
disposed editor could kill the extension host, covers any genuine fault
from the extractor.
The gap it claimed to cover is real and is now visible rather than papered
over: a malformed values file yields whatever the parser salvaged, and
`image: {repository: x` salvages a reference that then gets checked and
marked. Making the extension stay quiet on a file the YAML language service
is already complaining about needs `packages/helm` to surface parse errors,
which is its own change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds the two positive-confirmation surfaces to the image reference check in
apps/vscode. A values file whose images all resolve now reads as verified rather than merely quiet.✓after its repository value, via aTextEditorDecorationTypecreated once at activation.Verified on \docker.io`.for a confirmed one,Not verified:for one that could not be answered. The reasons are aRecord<UnverifiableReason, string>` rather than a switch, so adding a reason to the union fails the build here instead of hovering with no explanation.ReferenceCheckrecord built once per document open, so they cannot drift apart about a reference.An unverifiable verdict now has a second thing it must never render as. It already never produces a diagnostic; it never produces a checkmark either, because positive confirmation the tool did not actually obtain is the same lie as a phantom error.
Two things worth a reviewer's attention:
onDidOpenTextDocumentcan fire before the editor is visible, and tab switches hand out editors carrying no decorations, so the stored checks are re-applied ononDidChangeVisibleTextEditors.Closes #21
Testing
apps/vscode: 20 tests, one per acceptance criterion in Checkmark decoration and hover for image reference results #21 plus the visibility-change re-apply, the stale-version guard on both surfaces, no hover outside a reference, and no hover on a not-found reference (its diagnostic already speaks, and its verdict names no registry to report).vscodestub gainedcreateTextEditorDecorationType,visibleTextEditors,onDidChangeVisibleTextEditors,registerHoverProvider, and theHover/MarkdownString/ThemeColorclasses, per the binding convention invitest.config.mtsagainst per-filevi.mockfactories.checksAsOffails exactly those two and nothing else.format,lint,type-check,build,test,packageviaturbopass.Known gaps, both deferred by #17
checksByDocumentnever drops entries, so a long session accumulates one per values file opened. There is noonDidCloseTextDocumentcleanup.packages/oci-registryyet, so those hovers are unreachable until the credentials ticket (Local Docker credentials end to end #22).