Skip to content

feat(structure-viewer): add ted domain coloring - #405

Open
FlorinSenoner wants to merge 6 commits into
mainfrom
feat/341-ted-domain-coloring
Open

feat(structure-viewer): add ted domain coloring#405
FlorinSenoner wants to merge 6 commits into
mainfrom
feat/341-ted-domain-coloring

Conversation

@FlorinSenoner

Copy link
Copy Markdown
Collaborator

Summary

  • fetch optional TED domain residue assignments alongside AlphaFold structures
  • add a reversible pLDDT / TED domains color control to the structure viewer
  • register a custom Mol* categorical theme with neutral unassigned residues

Root cause

The structure-loading path fetched only the AlphaFold mmCIF and the Mol* adapter exposed no representation-theme API. TED annotations live at a separate AlphaFold DB endpoint, so the viewer had neither residue ranges nor a control capable of applying domain colors.

Reproduction

  1. Open the Explore page with the default demo bundle.
  2. Search for A0A0B4U9L8 and press Enter.
  3. Observe that the structure loads with only pLDDT coloring and no TED domain option.

After this change, pLDDT remains the default, TED domains is enabled for the live three-domain response, selecting it colors all domain segments categorically, unassigned residues are gray, and switching back restores pLDDT without reloading the structure.

Tests

  • strict TDD regression tests for TED API parsing and graceful unavailability
  • Mol* adapter tests for deterministic/discontinuous domain coloring and reversible theme updates
  • Lit component tests for default, enabled, disabled, and switching states
  • live browser verification against A0A0B4U9L8
  • pnpm test:ci
  • pnpm precommit
  • openspec validate add-ted-domain-structure-coloring --strict

Closes #341

@FlorinSenoner
FlorinSenoner marked this pull request as ready for review August 1, 2026 20:44
type: isBinary ? 'application/octet-stream' : 'text/plain',
});
const blobUrl = URL.createObjectURL(blob);
const tedDomains = await tedDomainsPromise;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Bound the optional TED request

The TED promise is awaited unconditionally after the model file is ready. If the AlphaFold TED endpoint accepts the request but never settles, StructureService.loadStructure never returns, so the viewer remains on “Loading protein structure...” even though the prediction and CIF fetches completed. I reproduced this with a never-resolving response only for /api/domains/: after 250 ms the load was still pending while both primary fetches had completed.

Because TED is specified as optional and must not regress structure viewing, please give this sidecar request an AbortSignal/timeout and map timeout to an empty domain list, or otherwise stop gating the primary structure result on unbounded completion. A stalled-request regression test would protect this behavior.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 96f25e4. The optional TED request now races a five-second fallback to an empty domain list and receives an AbortSignal that is aborted on timeout, so StructureService.loadStructure returns the completed primary structure even if the sidecar fetch never settles. Added a stalled-request regression that advances virtual time, verifies the structure result resolves with tedDomains: [], and verifies the TED signal is aborted. Fresh focused tests, the full workspace suite, strict OpenSpec validation, browser QA, pnpm precommit, and the bundle contract all pass locally.

@FlorinSenoner
FlorinSenoner marked this pull request as draft August 1, 2026 21:11
@FlorinSenoner
FlorinSenoner marked this pull request as ready for review August 1, 2026 21:34
@tsenoner

tsenoner commented Aug 3, 2026

Copy link
Copy Markdown
Owner

Automated review

Does it solve #341? Yes — the web structure viewer now fetches AlphaFold DB's /api/domains sidecar (bounded at 5 s, failures degrade to an empty list), registers a categorical Mol* color theme keyed on TED domain number, and renders a "Color by" pLDDT / TED domains control that swaps themes on the loaded representations without reloading the structure. Unassigned residues fall back to neutral gray and the option is disabled when no domains exist. Deliberate scope limits (no domain legend or CATH labels; pLDDT re-selected on each newly loaded protein) are documented as non-goals in openspec/changes/add-ted-domain-structure-coloring/design.md, and the Dash viewer behind protspace serve is untouched — but the issue is about the AlphaFold panel in the web app, and that is fully delivered.

Found 3 issues:

  1. _handleColorModeChange commits _colorMode only after the awaited theme swap and never checks the work is still current — a second click during the swap is silently dropped, and a protein change mid-swap leaves the toolbar disagreeing with the rendered colors. The early return on L297 compares against the committed mode, so while a TED swap is in flight a click on pLDDT no-ops; and if _loadStructure resets this._colorMode = 'plddt' and nulls _viewer meanwhile, the pending continuation still writes this._colorMode = mode, so a protein with no domains can render pLDDT while the TED button is both disabled and aria-pressed="true". Capturing the viewer at entry and bailing when this._viewer !== viewer fixes both.

private async _handleColorModeChange(mode: StructureColorMode) {
if (!this._viewer || this._colorMode === mode) return;
if (mode === 'ted-domains' && !this._structureData?.tedDomains.length) return;
try {
if (mode === 'ted-domains') {
await this._viewer.setColorTheme(mode, this._structureData!.tedDomains);
} else {
await this._viewer.setColorTheme(mode);
}
this._colorMode = mode;
} catch (error) {
console.warn('[StructureViewer] Failed to change structure color mode:', error);
}
}

  1. The "back to pLDDT" assertion is already satisfied by the load-time setColorTheme('plddt') call, so it passes even if the pLDDT button does nothing. _displayStructure records a ('plddt') call on every successful load after vi.clearAllMocks(), so toHaveBeenCalledWith('plddt') resolves on the first poll regardless of the click — leaving the spec's "User returns to pLDDT coloring" scenario unverified. toHaveBeenLastCalledWith plus aria-pressed / .color-description assertions (mirroring L113-L116) would make it real.

element.shadowRoot?.querySelector<HTMLButtonElement>('[data-color-mode="plddt"]')?.click();
await vi.waitFor(() => expect(mocks.setColorTheme).toHaveBeenCalledWith('plddt'));
});

  1. The user-facing structure docs still present pLDDT as the only coloring and omit the new control and endpoint. docs/explore/structures.md is unchanged: L21 states structures "are colored by pLDDT" unconditionally, the "How It Works" list omits alphafold.ebi.ac.uk/api/domains/{accession}, and the controls table has no row for the new toggle — while CONTRIBUTING.md asks for docs updated in the same PR when user-facing messaging changes.

## Confidence Coloring (pLDDT)
Structures are colored by **predicted Local Distance Difference Test (pLDDT)** confidence scores—the same scheme used on the [AlphaFold Database](https://alphafold.ebi.ac.uk/). Regions in **blue** are high-confidence, **yellow** moderate, and **red** low-confidence. This helps you quickly spot which parts of the model are more reliable.
## Viewer Controls
| Action | Effect |
| ---------------- | -------------------- |
| **Left drag** | Rotate the structure |
| **Right drag** | Pan the view |
| **Scroll** | Zoom in/out |
| **Double-click** | Reset the view |

🤖 Generated with Claude Code

Reviewed at 96f25e4 against issue #341.

@FlorinSenoner

Copy link
Copy Markdown
Collaborator Author

Independent triage at current head 96f25e44b3dcdad399c8358015722eacfadedb42 confirms all three items are actionable:

  1. The async color switch can commit stale state after a protein/viewer change, and a rapid TED → pLDDT click is dropped because _colorMode remains plddt until the TED promise resolves. Follow-up should guard viewer/load identity and preserve the latest requested mode (an identity check alone does not cover the rapid reverse-click case).
  2. The return-to-pLDDT test can pass from the load-time setColorTheme('plddt') call. Follow-up should assert post-click call order/count (for example, the last call) and the resulting pressed state/description.
  3. docs/explore/structures.md still describes pLDDT as the only coloring mode and omits the TED control/request, while CONTRIBUTING.md requires docs to accompany user-facing messaging changes. The docs should be updated in this PR.

No code or PR-state changes were made as part of this triage; follow-up implementation is needed for all three.

@FlorinSenoner

Copy link
Copy Markdown
Collaborator Author

Implemented all three actionable items in abedcff1:

  1. Color-mode updates are sequenced per viewer and guarded by viewer/request identity, so rapid TED → pLDDT ends in pLDDT and stale completions cannot update a replacement viewer.
  2. The component regression now requires pLDDT to be the final Mol* call and verifies both pressed states plus the pLDDT description; focused races cover rapid reversal and viewer replacement.
  3. docs/explore/structures.md now documents the optional TED endpoint, graceful availability, coloring behavior, and Color by control. The existing OpenSpec design/spec/tasks capture the concurrency contract.

Fresh verification: focused component tests 5/5; pnpm test:ci 1,882 passed with one unrelated skip; bundle contract 11/11; strict OpenSpec validation; live A0A0B4U9L8 browser TED → pLDDT interaction; and the staged/commit-hook pnpm precommit gates all passed.

- Drop the local `colorViewer` cast that re-declared `setColorTheme` as
  optional even though `MolstarViewer` already requires it, and call
  `viewer.setColorTheme(...)` directly without optional chaining.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016qoU16kDQxz6U3H2UWbbm2
@tsenoner

tsenoner commented Aug 6, 2026

Copy link
Copy Markdown
Owner

Adversarial review

Reviewed in an isolated worktree by three independent lenses (code quality, adversarial correctness, issue-resolution audit), with every finding then put through a refuter whose default position was that it is a false positive. 8 raised, 3 survived refutation.

Applied and pushed (d6b5fff7)

Behavior-preserving cleanups, verified green before pushing:

  • Fix 1 (simplification) in /Users/tsenoner/Documents/projects/protspace-suite/.protspace-wt-pr405/packages/core/src/components/structure-viewer/molstar-loader.test.ts: deleted the colorViewer alias that re-declared setColorTheme as an optional member via a cast, even though MolstarViewer (molstar-loader.ts:20) already declares it required. The three call sites now use the typed viewer directly — expect(viewer.setColorTheme).toBeTypeOf('function'), await viewer.setColorTheme('ted-domains', domains), await viewer.setColorTheme('plddt') — dropping the ?. optional-chaining noise. Per the authoritative revised fix, the toBeTypeOf('function') assertion was KEPT (the original proposal wanted it deleted) and the import type { TedDomain } on line 3 was left in place since the domains fixture on line 7 still uses it. Net: 3 insertions, 6 deletions; behavior-preserving (same calls, same assertions, same order).

Issue resolution — resolves the issue

The author's scoping is correct and I verified the declined items against the code rather than
accepting design.md's word.
What was scoped out, and whether it holds:

  • "No domain legend, selection filters, or new representations" — defensible against the literal
    issue text ("add a coloring by TED domains"), but it is the one omission a user will feel
    immediately, because categorical colors without a key are only half-useful. I'd land the PR and open
    a follow-up rather than expand scope here.
  • "No bundle/Python annotation-pipeline changes" — correct and well-reasoned. I confirmed
    ted_domains already exists as a bundle annotation (docs/guide/annotations.md:343,
    packages/utils/src/visualization/annotation-metadata.ts:327, ted_retriever.py), but it stores
    formatted cath_label|plddt strings with no residue ranges, so the viewer genuinely cannot reuse it
    and needs the live /api/domains call. Not duplicated work.
  • "No npm Mol* dependency, keep the CDN pin" — correct; adding the package would contradict the
    existing dynamic-CDN design, and I verified the untyped integration is fully contained in molstar- loader.ts.
  • "TED must be optional / must not gate structure loading" — implemented properly for the network
    path (AbortSignal + 5s race, catch {} → [], 404 → []), and I confirmed the 404 body is {} so
    the shape guards do the right thing. But the same principle is not applied to theme registration
    or the initial setColorTheme call, both of which sit unguarded on the critical load path. That is
    the one place the implementation falls short of the author's own stated rule.
    One structural nit: parseTedDomain rejects domainNumber < 1 and non-integers, then uses
    (domainNumber - 1) % palette.length — consistent, and the real API is 1-based, so no off-by-one.
    "Closes [FEATURE] Allow coloring by TED domains #341" is warranted. The issue is a single-sentence feature request with no comments, no
    additional acceptance criteria, and no sub-parts; the diff delivers a working, reversible,
    gracefully-degrading TED coloring mode with real-endpoint-accurate parsing, and the OpenSpec
    artifacts accurately describe what was built (I cross-checked each spec scenario against the
    corresponding code and test). The gaps above are enhancements and hardening, not unmet requirements
    of [FEATURE] Allow coloring by TED domains #341.
Gaps found by the issue audit (6)
  • Colors are unidentifiable: parseTedDomain (structure-service.ts:159-177) keeps only ted_domain_no and segment bounds, discarding cath_label, plddt, and nres_domain that the API already returns in the same response. There is no legend, hover label, or domain list anywhere in the viewer.
    • Why it matters: For a 7-domain protein like mTOR (P42345) the user sees seven colors and gray, with no way to learn which color is which domain or what CATH superfamily it corresponds to. The repo already resolves CATH codes to human names (apps/protspace/src/protspace/data/annotations/retrievers/cath_names.py) and ships ted_domains labels in the bundle, so the identifying information exists on both sides but is thrown away here. The 9-colour palette also cycles by domainNumber % 9, so a protein with >9 domains repeats colors with nothing to disambiguate them.
    • Suggested follow-up: Keep cath_label/plddt on the parsed TedDomain and render a compact domain legend (swatch + domain number + CATH label) under the color toolbar when TED mode is active. Track as a follow-up issue rather than blocking this PR, since the design explicitly scoped the legend out.
  • Theme registration and the initial theme application are unguarded, so a Mol API mismatch now breaks structure loading entirely. createMolstarViewer dereferences viewer.plugin.representation.structure.themes.colorThemeRegistry.add(...) (molstar-loader.ts:235-237) with no try/catch, and _displayStructure awaits setColorTheme('plddt') (structure-viewer.ts:229) inside the load path.*
    • Why it matters: The PR's own design.md states "a missing or malformed TED response must not prevent the underlying structure from loading." That principle is enforced for the fetch (empty array on any failure) but not for the theme plumbing: if the CDN Mol* shape ever drifts (a version bump, a CDN mishap), the throw propagates into _loadStructure's catch and the user gets "Failed to load structure. Please try again." for a structure that would otherwise render fine. Today MOLSTAR_VERSION is pinned to 3.44.0 and I confirmed the shape is correct, so this is latent, not live.
    • Suggested follow-up: Wrap the colorThemeRegistry.add call and the initial setColorTheme('plddt') in try/catch that logs and degrades to "TED unavailable" rather than failing the load; optionally have setColorTheme no-op when registration failed.
  • The two Mol-coupled functions — getResidueSequenceNumber and the provider's color() callback (molstar-loader.ts:106-145) — have zero test coverage. molstar-loader.test.ts:69 only asserts addTheme was called once; the captured provider is never invoked.*
    • Why it matters: These are the only pieces whose correctness depends on undocumented Mol* internals (Unit.Kind numeric 0, residueAtomSegments indirection, bond-location aUnit/aIndex). Everything else in the feature is covered. If residue mapping were wrong, every test in the PR would still pass and the button would appear to work while coloring the wrong residues — the failure mode is silent and visual-only.
    • Suggested follow-up: Capture the provider passed to the mocked colorThemeRegistry.add, call provider.factory(ctx, {}), and assert color() on hand-built element-location and bond-location fakes (atomic + coarse unit kinds) returns the expected domain color and TED_UNASSIGNED_COLOR.
  • No cross-check that the TED annotations describe the same model that was loaded. loadTedDomains is keyed on the bare accession while the structure comes from predictions[0], which is not always the canonical full-length model — e.g. GET /api/prediction/Q9Y6V0 returns AF-Q9Y6V0-3-F1 (isoform 3, 356 residues). Segments are also never clamped to the model's residue range.
    • Why it matters: If an accession ever yields an isoform or fragment model while TED annotates the canonical sequence, af_start/af_end would land on the wrong residues and color a chunk of the structure incorrectly with no error and no visual cue. I could not construct a live example (every long/isoform accession I probed 404s on the domains endpoint), so this is a latent correctness risk rather than an observed bug.
    • Suggested follow-up: Compare the prediction's uniprotSequence length (or entryId) against the largest af_end and drop the TED domains when they exceed the loaded model, so the button disables instead of mis-coloring.
  • Color mode is reset to pLDDT on every protein selection (structure-viewer.ts:162) and TED results are re-fetched per selection with no cache, unlike the 3D-Beacons model-page cache at structure-service.ts:30.
    • Why it matters: The stated motivation in proposal.md is "comparing domain organization with the embedding" — that is inherently a multi-protein workflow, and it currently costs one extra click plus a fresh network round-trip per point clicked. Re-selecting a protein you just viewed refetches its domains.
    • Suggested follow-up: Persist the chosen color mode across selections (falling back to pLDDT when the new protein has no TED domains), and add a small accession→domains Map cache mirroring alphaFoldModelPageCache.
  • Minor UI/robustness items: (a) the fixed-height host in apps/web/src/pages/Explore.tsx:82 (height="340px") now has a third flex row (color toolbar) competing with .viewer-container{height:100%} and a tips block that changed from one line to a two-line column, shrinking the 3D canvas; (b) .color-toolbar has no bottom border-radius, so with show-tips="false" it would be the last row with square corners against a rounded host; (c) forcing plddt-confidence on the format === 'pdb' fallback path overrides Mol's auto preset on a model with no ma_qa_metric, which would render in the theme's fallback color while the control still reads "pLDDT".*
    • Why it matters: (a) and (b) are cosmetic but land in the app's only structure surface; (c) is a real (if near-unreachable) case where the UI asserts a coloring the model cannot supply — AFDB always returns cifUrl, so the pdb branch at structure-service.ts:69-71 is effectively dead today.
    • Suggested follow-up: Eyeball the 340px sidebar with the toolbar present before merge; add border-radius: 0 0 6px 6px to .color-toolbar:last-child; and skip the explicit setColorTheme('plddt') (or fall back to Mol*'s auto theme) when structureData.format === 'pdb'.

Findings needing a decision (2)

These were left for you rather than auto-applied: each changes behavior, needs a product call, or reaches outside this diff.

1. _displayStructure re-reads this._viewer after an await, so the new initial setColorTheme('plddt') call can hit null or a different, newer viewer.

packages/core/src/components/structure-viewer/structure-viewer.ts:229 · medium · correctness

On origin/main _displayStructure dereferenced this._viewer exactly once, and that read
happened synchronously before the only await. This PR adds a second read after await this._viewer.loadStructureFromUrl(...) (line 229). loadStructureFromUrl awaits several times
internally, so click handlers and rAF callbacks can run in between, and every path that clears the
viewer (_cleanup() from close(), hide(), disconnectedCallback(), or a concurrent
_loadStructure() for a newly clicked protein) sets this._viewer = null synchronously.
Scenario A (null deref -> permanent false error over a working structure): user clicks protein A;
_viewer = V1; _displayStructure is parked in V1.loadStructureFromUrl(blobA) (Mol*
parse+render, hundreds of ms). User clicks protein B. rAF fires _loadStructure() for B, which
synchronously runs _isLoading=true; _error=null; _structureData=null; _colorMode='plddt' and then
_cleanup() -> this._viewer = null. When A's continuation resumes,
this._viewer.setColorTheme('plddt') throws TypeError: Cannot read properties of null (reading 'setColorTheme'). _loadStructure's catch does not match either known message, so it hits the
fallback branch: console.error(...), this._error = 'Failed to load structure. Please try again.', _isLoading = false, plus a spurious structure-error event. Load B already executed its
_error = null before this happened and never clears _error again, so protein B finishes
loading and renders in Mol* while the component paints the red error box on top and suppresses the
whole new .color-toolbar (its guard is !this._error).
Scenario B (viewer swap -> silent theme reset + lying UI): same setup, but A's
loadStructureFromUrl resolves after B has fully loaded (_viewer = V2) and the user has already
clicked "TED domains" (_colorMode='ted-domains', Mol* showing TED colors). A's continuation now
reads this._viewer === V2 and calls V2.setColorTheme('plddt') — outside the queue and outside
the requestId/viewer guards that _handleColorModeChange was carefully built with — so B's
structure silently reverts to pLDDT while the toolbar still shows "TED domains" pressed and the tip
still says "Colors distinguish TED domains".
Note that _handleColorModeChange (added in this same PR) does capture const viewer = this._viewer up front precisely to avoid this; _displayStructure was not given the same
treatment. Also worth noting: no test covers this added line at all — structure- viewer.coloring.test.ts never asserts the load-time setColorTheme('plddt') call — and Mol*'s
AlphaFold preset already selects plddt-confidence automatically for mmCIF models, so deleting the
line entirely is also a valid fix.

Suggested fix

In packages/core/src/components/structure-viewer/structure-viewer.ts, rewrite _displayStructure to
capture the viewer once and re-check identity across the await:
private async _displayStructure(structureData: StructureData): Promise {
const viewer = this._viewer;
if (!viewer) {
throw new Error('Viewer not initialized');
}
// Load structure based on source
switch (structureData.source) {
case 'alphafold':
if (structureData.url) {
await viewer.loadStructureFromUrl(
structureData.url,
structureData.format,
structureData.isBinary,
);
// A close()/hide()/new load may have disposed or replaced the viewer while
// Mol* was parsing; do not touch a viewer that is no longer ours.
if (this._viewer !== viewer) return;
await viewer.setColorTheme('plddt');
} else {
throw new Error('AlphaFold structure URL not available');
}
break;
default:
throw new Error(Unsupported structure source: ${structureData.source});
}
}
Then add a regression test to structure-viewer.coloring.test.ts: make mocks.loadStructureFromUrl
return a deferred promise, call element.close() (and separately: reassign element.proteinId)
while it is pending, resolve it, and assert no structure-error event fired and that the newly
loaded protein still renders its .color-toolbar.

2. No test ever invokes the registered TED color provider, leaving getResidueSequenceNumber — the change's highest-risk logic — completely uncovered.

packages/core/src/components/structure-viewer/molstar-loader.test.ts:69 · medium · test-gap

molstar-loader.test.ts captures the registered provider via addTheme but only asserts
expect(addTheme).toHaveBeenCalledOnce(); it never calls addTheme.mock.calls[0][0].factory(ctx, {}).color(location). The only other coverage is getTedDomainColor, a pure number->number
function. That means the entire Mol*-to-residue mapping in molstar-loader.ts:105-120 is untested,
even though design.md names "Residue numbering mismatch" as one of its two headline risks.
Concrete mutations that break the feature in the browser and yet leave all 22 tests in this PR
green:
(a) change unit.kind !== 0 to unit.kind !== 1 (line 113) — every atomic unit now returns null,
so a protein with TED domains renders 100% gray (0x9ca3af) in TED mode instead of colored domains;
(b) swap residues.label_seq_id for residues.auth_seq_id (line 118) — silently wrong for any
model whose auth numbering is offset from the UniProt numbering the TED af_start/af_end
coordinates use;
(c) drop the unit.elements[location.aIndex] translation on line 109 and use location.aIndex
directly — bond locations in ball-and-stick representations would look up an arbitrary unrelated
residue (unit-local index used as a model-global ElementIndex), producing scrambled bond colors.
The test file already builds a fake rawViewer and jsdom environment, so closing this gap is cheap.

Suggested fix

In packages/core/src/components/structure-viewer/molstar-loader.test.ts, import
TED_UNASSIGNED_COLOR alongside getTedDomainColor, and inside the existing 'registers TED
coloring and switches loaded representations without reloading' test, after await viewer.setColorTheme('ted-domains', domains) and BEFORE the final setColorTheme('plddt') call
(which resets the closure's domain list to []), drive the registered provider:
const provider = addTheme.mock.calls[0][0] as {
factory: (ctx: unknown, props: Record<string, never>) => { color: (loc: unknown) => number };
};
const theme = provider.factory({}, {});
const unit = {
kind: 0,
elements: [7],
model: {
atomicHierarchy: {
residueAtomSegments: { index: { 7: 3 } },
residues: { label_seq_id: { value: (i: number) => (i === 3 ? 35 : 200) } },
},
},
};
// element location -> residue 35 -> domain 1
expect(theme.color({ kind: 'element-location', unit, element: 7 })).toBe(
getTedDomainColor(35, domains),
);
// bond location resolves aIndex through unit.elements
expect(theme.color({ kind: 'bond-location', aUnit: unit, aIndex: 0 })).toBe(
getTedDomainColor(35, domains),
);
// coarse (non-atomic) units fall back to the neutral color
expect(theme.color({ kind: 'element-location', unit: { ...unit, kind: 1 }, element: 7 })).toBe(
TED_UNASSIGNED_COLOR,
);
(Prefer a separate it(...) block that re-runs the viewer setup if the reviewer wants the adapter
test kept single-purpose.)

5 further finding(s) were raised and refuted during verification.

@FlorinSenoner

Copy link
Copy Markdown
Collaborator Author

Addressed the substantive items from the adversarial review in 45df640a:

  • Already addressed — typed viewer cleanup: d6b5fff7 was the starting head and already contains the behavior-preserving colorViewer simplification. No further change was needed.
  • Implemented — stale post-load pLDDT write: removed the redundant setColorTheme('plddt') after loadStructureFromUrl. AlphaFold mmCIF loading already applies Mol*'s pLDDT preset, and removing the post-await write prevents a closed viewer from emitting a false structure-error and prevents an older load from resetting a replacement viewer from TED to pLDDT. Added regressions for both close-during-load and replacement-viewer theme preservation, and recorded the behavior in OpenSpec.
  • Implemented — registered provider coverage: the adapter test now invokes the registered provider and verifies atomic element locations, bond locations (including aIndex → unit.elements translation), and neutral fallback for coarse units. A deliberate Unit.Kind inversion made this test fail, confirming it protects the Mol* mapping boundary.
  • Deferred by existing scope — legend/CATH metadata: the current OpenSpec explicitly lists a domain legend as a non-goal. Adding CATH fields and a new legend is a separate user-facing feature rather than a review fix for [FEATURE] Allow coloring by TED domains #341.
  • Not changed — theme registry mismatch fallback: the concrete redundant initial theme application is removed. The registry shape is the pinned Mol* 3.44 adapter contract; swallowing a registry mismatch would require a new capability/error contract so the UI does not advertise a TED mode the adapter cannot support. The OpenSpec's graceful-degradation requirement is specifically for optional TED annotation data, not arbitrary Mol* integration failure.
  • Deferred — model/annotation length cross-check: no mismatched live TED/model response was reproduced, and defining equivalence for canonical accessions, isoforms, and fragments needs an explicit policy. No speculative filtering was added.
  • Rejected for this PR — persistent mode/cache: pLDDT on every newly loaded protein is an explicit requirement; persisting TED would conflict with it. Domain caching is a separate optimization with no demonstrated correctness issue.
  • No additional UI expansion: the layout/radius points were labeled cosmetic follow-ups and no break was reproduced. Removing the explicit post-load pLDDT write also avoids forcing plddt-confidence on the PDB fallback path.

Verification on the committed tree: focused structure-viewer tests 9/9, full pnpm test:ci 1,884 passed / 1 skipped, strict openspec validate add-ted-domain-structure-coloring --strict, and the staged pnpm precommit gate all passed. No Python files changed, so no uv checks were applicable.

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.

[FEATURE] Allow coloring by TED domains

2 participants