Milab-6720: prerun hint and run disabling - #49
Open
erohinaelena wants to merge 28 commits into
Open
Conversation
Picking a file left the panel silent while every column of it was profiled, and the profile outputs are retentive, so the dropdowns kept answering with the previous file's headers as though nothing was happening. A mapping that had passed against that previous file also still satisfied bareSetValid, so Run stayed live over a file nobody had looked at and over headers it might not have. Prerun now states which file the profile came from, and the model pairs the two under retentive: getIsReadyOrError marks the read unstable, so the reported id stays the old file's until the new scan lands. The panel compares it with the loaded file, announces the wait, and withholds the mapping until the columns on offer are really this file's. Keyed to the file rather than to "prerun is busy" because prerun also re-runs on every mapping edit to re-check the identity column for collisions. Picking a different file drops the parts of the mapping that name columns, keeping the receptor declaration and the numbering scheme. That is what disables Run, and it is done on the gesture: args is a pure function of data and cannot consult prerun, and mirroring prerun's verdict back into data is the pattern this block is shedding. Re-picking the same file is not a swap. The import itself now shows the block's loader, which it never did, scoped to the main run so it cannot cover the settings panel during prerun.
PlAlert declares only a default slot and takes its heading from the `label` prop, so the four `<template #title>` headings in the panel had never rendered. The bodies showed with nothing above them.
The panel quoted bareSet.identity, which updates on the pick, against collisions produced for the previous column, so changing an offending column flashed the old verdict under the new column's name. Prerun now states which column it checked and the model reports both as one value, so the panel can tell a current verdict from a stale one. Also cuts the listed values from ten to three: the list buried the sentence saying what to do about them.
The record key is the identity's hash, so a value repeated on rows that are not identical merges two records into one. Prerun found these and the panel warned, but Run stayed live and the API imported the merged set without complaint. Run is now refused while the verdict for the selected column is outstanding and when it says the column repeats, and the panel says which. The platform enforces it too: invalid args leave no production to render. The args projection sees only data, and no gesture can capture this fact -- the scientist picks a column and only then does the check discover whether it is sound -- so the UI mirrors the verdict in. The rules that keep that safe are stated on BlockData.prerunChecks for the checks still to come.
Clearing it left the field reading "Value not available" in red. `identity` is a required string, so "nothing chosen" is stored as "", and a dropdown treats any non-undefined value as chosen -- "" is a chosen value that is not in the options. The field now reports nothing chosen. Also: clearing a mapping only reset it when the mapped chains were the IG pair, so a TCR mapping could never clear itself. The message shown while the checks are outstanding is now phrased for the checks in general rather than naming the id column, since more of them are coming.
Five separate entries for one PR's worth of related work read as five independent releases in the changelog. One entry, same package list.
MainPage had grown to 1037 lines, over half of it one subject: mapping a file of bare sequences onto records. That form, the checks that gate it and the numbering that follows from it now live in pages/components/BareSetForm.vue, leaving MainPage with the two doors, the dataset-door format mapping and the table (720 lines). The parent still decides whether the shape is being mapped at all -- a file is loaded and its columns are known -- and passes the header list it already computes for the other door. SCHEME_LABELS and forgetMappedColumns move to the model: both are shared with MainPage, and both are data-shape facts that sit naturally beside CHAIN_SLOT_LABELS and bareSetValid. Fixes a latent bug carried along in the move: propertyCandidates excluded only IGHeavy and IGLight from the property candidates, so a TCR mapping was offered its own sequence columns as record properties.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
A collision is not "the id column repeats" -- it is an id repeated on rows whose other mapped cells differ, so remapping a chain can turn a clean set into a colliding one and back. Keying the verdict on the id alone meant a chain remap left the previous verdict standing: a clean one enabled a mapping nothing had checked and merged distinct rows into one record, and a stale colliding one kept a valid mapping blocked until prerun caught up. Prerun now states the identity and the mapped sequence columns it checked, and one shared rule (collisionCheckKey) decides whether a verdict is about the current mapping. The panel, the run gate and the mirror all use it, so they cannot disagree about what "the same mapping" means. Exposed on the facade so the tests state the verdict with the same rule the gate applies rather than rebuilding it.
The dataset door had the same blind window as the file door: prerun infers the selected dataset's columns and nothing said so, while datasetColumns and validationResult are retentive and went on answering with the previous dataset -- so the panel could offer the last dataset's headers, and show its verdict, as though they were this one's. Prerun now states which dataset and format an inference was run for, paired with the headers the same way the file door pairs its profile. The panel announces the wait, withholds the custom mapping until the headers belong to the current selection, and no longer shows a format verdict reached for a different dataset. Not raised on the bare-set path, where prerun answers with collisions and never produces columns at all.
"The following columns will be imported:" appeared as soon as a dataset was picked, with nothing under it: the section was gated on the output existing, and an empty array exists. Before a format is chosen the inference has nothing to report, so the heading claimed the import would emit no columns. The list is also retentive, so it outlived a dataset switch and listed the previous dataset's columns; it now waits for the door, the format and the current inference. Also moves the two verdicts about the selected columns below the chain control. Sitting between the format and the chains, they pushed apart two controls that belong together and read as something to fill in rather than as a verdict on what was already filled.
The doc blocks this branch added were arguing their case at length where naming the fact was enough. Kept: what the code cannot say -- that getIsReadyOrError marks a read unstable and so pairs a retentive value with its subject, that a signal is keyed to the input rather than to prerun being busy, what makes the mirror in prerunChecks safe, and why the collision values are printed whole. Dropped: the restatements of what the code does, and the histories of what it used to do. Also removes a stale doc block stranded above the fileImports output by an earlier reorder: it documented identityCollisions, which carries its own doc further down, and its claim that the verdict is never mirrored into state from a UI watcher is now the opposite of what this branch deliberately does.
What an args projection is needs no explaining -- every block has one. Keeps the one fact it carried that the code cannot state: `chains` is passed through in the order the user picked, not sorted. Moved to the field it is about.
Nothing is reshaped, so there is nothing to explain -- the signature says which two buckets go in and what comes out. The one non-obvious fact about the upgrade, that V1's uiState fields are all optional on disk and so need explicit defaults, is already stated on viewStateDefaults. Its claim about V1's `loadFromFile` was also false. The file door, that flag, its removal and the V3 migration were all in PR #47 and released together (7023012), so no released V1 version ever had the flag and no project on disk can carry it.
For the seven non-custom formats Run was gated on five *ColumnsPresent booleans the UI mirrored back from the validationResult output. They were keyed on the format alone and never on the dataset, so switching between two datasets of the same format left the previous dataset's verdict in place: Run armed immediately while the panel was still scanning and still showing the old verdict. That breaks the rule the surviving prerunChecks hairpin follows — a verdict carries what it is about, and is dropped when the source changes. It cannot be fixed in place: the check needs the file's headers, those live in prerun, and a V3 args lambda receives data only (no ctx, and V3 has no argsValid). Removed: the presentByFormat check, the five BlockData fields, their five LegacyUiState counterparts, the viewStateDefaults entries, the formatFlags table and the mirroring watcher. Nothing in the workflow ever read them. What is lost is the refusal — a dataset lacking the format's columns can now be run and will fail in the workflow. What survives is the honest signal: the 'Invalid <format> dataset' alert reads validationResult directly, and is already suppressed while a new dataset is being scanned. Restoring the gate properly means moving the check onto prerunChecks, keyed on dataset and format. Old projects carry the removed keys on disk; upgradeLegacyData spreads uiState over the defaults, so they land as inert extra properties.
… a sample profiledSampleId (file door) and inferredFor (dataset door) answered the same question — what the prerun results on screen were computed for — and exactly one door is ever live, since projectArgs refuses both at once. They are now a single retentive output, prerunDatasetValidationInfo, carrying datasetId on the file door and datasetRef + format on the dataset door. It gates on whichever backing result the stamp describes, so stamp and result still cannot disagree. FileSource.sampleId becomes datasetId. One file is one dataset; it is only today that the dataset is also one sample, which is why the value mints the pl7.app/sampleId key. A file holding several samples would name those from its own contents while this stayed the identity of the file they came from. That rename needs a migration, not just a find-and-replace: the value mints an axis key and v1 shipped in block 1.8.1, so a saved project reaching v2 without it would key its records on undefined and lose every join a downstream block had made. Added as a v1 -> v2 step, preferring an existing datasetId over a stale sampleId beside it. Verified against four shapes: v1 project, already-migrated, both keys present, and the dataset door (no fileSource). pnpm check 12/12, model tests 4/4.
…dentity Two fixes in the same area. The stamp carried datasetId and datasetRef side by side, which reads as one id in two formats. It is not: the file door names a dataset that IS the file, by an id minted when it was picked; the dataset door names one already in the result pool, by a ref to its column. Now a union tagged by door — the block's own word for the distinction — so the mutual exclusivity projectArgs enforces is stated in the type rather than left as a convention. Separately, and worse: the file id was minted on every pick. Re-selecting the same file is how a file gets re-read, so a re-read handed it a fresh identity. That value mints the pl7.app/sampleId key, so the re-read orphaned every downstream join against the old key, silently. The field's doc already claimed the identity survived a re-read; the code did not. It is now minted only when the handle changes — the same test that already decided whether to drop the mapping. Two different files sharing a name are unaffected: different handles, so a swap, so a new id. They do share a label, which is the filename stem. pnpm check 12/12, model tests 4/4, both scanning predicates verified against every stamp shape including the wrong door and no stamp.
The prerunDatasetValidationInfo doc block goes from 14 lines to 8: what the stamp is, why it is tagged, and why it is keyed to the input. The parts restating what the type already shows are gone. Line numbers pointing into other packages are removed, keeping the file or block name so the pointer survives. Those targets version independently, so the numbers were wrong the moment anything upstream moved, and nothing here would notice: anarci.py:558-592, anarci.py:263 external tool sdk/workflow-tengo/src/strings.lib.tengo:4 SDK mixcr-clonotyping SettingsPanel.vue:288-301, process.tpl.tengo:695 peptide-extraction parse.tpl.tengo:28 immune-assay-data analysis.tpl.tengo:28-38 core/pl block_resolver_request.go:244 pl-tree/src/accessors.ts:347 Two references keep their line numbers: process-bulk.tpl.tengo:128 and infer-columns-common.lib.tengo:50-66 are in this package, so they move with the code and the same review sees both sides. pnpm check 12/12, model tests 4/4.
Removing the per-format flags fixed the stale verdict but dropped the wait with
it, so Run armed the instant a dataset was picked — earlier than before, not
later. The gate is back, keyed the way the collision check is keyed.
prerunDatasetCheck carries { dataset, columnsPresent }, where dataset is
datasetCheckKey(datasetRef, format). projectArgs compares it against the live
selection: a verdict for anything else reads as 'not judged yet'. Format is in the
key because the same dataset answers differently under a different format.
The UI writes it only once the prerun stamp says the results on hand were computed
for what is selected now. That check is load-bearing: validationResult is
retentive AND judges prerun's headers against the live format, so mid-switch it
can pair one dataset's headers with another's format. The clear-on-change watcher
now covers format and both verdicts.
Verified across the reported sequence:
just picked A, no verdict yet REFUSED: Validating the selected columns
verdict for A, valid pass
verdict for A, invalid REFUSED: missing columns
switched A->B, A's verdict stands REFUSED: Validating the selected columns
same dataset, format changed REFUSED: Validating the selected columns
file door pass (unchanged, has its own gate)
pnpm check 12/12, model tests 4/4.
… copy The previous commit's UI writer had to check prerun's stamp against the live selection before mirroring a verdict, because validationResult mixed sources: the headers came from prerun, the format from live block data. Mid-switch it could judge one dataset's headers under another's format, so the writer needed matching logic — and matching logic is exactly what the per-format flags got wrong. validationResult now reads the format from prerun's stamp too. Both halves of the question come from one staging context, so the mismatch is not guarded against, it is unrepresentable. The verdict also states the dataset it judged, so mirroring it is a copy: no comparison in the UI, nothing to get wrong. The args projection still does the comparing, against whatever is selected when it runs. Writer goes from 20 lines to 8 and no longer imports datasetCheckKey. Verified: with prerun still describing A/mixcr while the user has moved to B/airr, the verdict is stamped A/mixcr and the gate refuses; once prerun catches up to B/airr the same verdict passes. pnpm check 12/12, model tests 4/4.
MainPage.vue was 723 lines, 474 of them script. The ten declarations that do not depend on block state — eight option lists and two sentinels — move to ui/src/constants.ts, beside app.ts where the siblings keep util.ts. Anything derived from app.model stays a computed next to the code that reads it, so the new file never needs the app instance. MainPage.vue 723 -> 644. Typed on the way out, which the inline literals were not: formatOptions is ImportFormat[], receptorOptions is ChainSelection[], countTypeOptions is CountType[]. A value that drifts from the model's vocabulary is now a type error rather than a dropdown entry that selects nothing. Verified a pure move: every entry of all eight lists and both scalars compared against the pre-move file and identical, 58 entries in total. pnpm check 12/12.
Beside the only file that reads it, rather than at the ui/src root. The header comment goes with it — the file's contents say what it holds.
Five functions that turn plain data into user-facing text: emptySamplesMessage, missingColumnsMessage, identityCollisionMessage, propertyCollisionMessage and formatLabel. Pure — no app instance, no refs — so the components keep only the computed that feeds them and the template that shows them. Their helpers come too, since nothing else uses them. The two display caps (EMPTY_SAMPLES_SHOWN, COLLISIONS_SHOWN) move out of constants.ts into messages.ts as module-private values, so the functions no longer take a cap argument: a cap is meaningless apart from the message it truncates. formatOptions stays in constants.ts — the dropdown is built from it too. Collapsed one duplication on the way: three messages built 'first N, then and X more' by hand, twice with identical wording. That is now andMore(). Pinned with tests written from the pre-move strings, so the rendered text is held byte-identical: 7 cases covering both truncation paths, the empty cases, and formatLabel's fallback to a raw id. MainPage.vue 644 -> 622, BareSetForm.vue 334 -> 322. pnpm check 12/12. Unit suites 11/11. wf.test.ts needs a live backend and is unaffected — it fails identically before this change with 'can't resolve platform address'.
Every caller was doing the derivation before the call — Object.values over propertyCollisions, a key check against collisionCheckKey, a ?? [] unwrap. That is the calculation the components should not be carrying. The builders now take what the model actually holds and derive inside: buildPropertyCollisionMessage(app.model.data.bareSet?.properties) buildIdentityCollisionMessage(app.model.outputs.identityCollisions, bareSet) buildEmptySamplesMessage(app.model.outputs.emptyChainSamples) Each caller is now a single pass-through. BareSetForm loses its identityCollisions computed outright — the key check that drops a verdict reached for a different mapping was its whole body, and that rule belongs with the message that would otherwise print a stale accusation. Also collapsed the validationResult computed, which was behind a cast: both branches were identical, and reading app.model.data.format already established the dependency the comment claimed the ternary was for. The cast is unnecessary now that the output is typed. Tests follow the signatures and gained the case the moved logic introduced: a verdict whose key names a different mapping renders nothing. MainPage.vue 603 -> 592, BareSetForm.vue 303 -> 296. pnpm check 12/12, unit suites 12/12.
chainsOptions listed TCR-ɣ against TCRDelta and TCR-δ against TCRGamma. The values run alpha, beta, delta, gamma — MiXCR's diversity-first order, the D-recombining chain leading each pair — and the labels had been applied as though the order were alphabetical. Picking 'TCR-ɣ' therefore imported delta chains, and 'TCR-δ' imported gamma. The import succeeded either way, with the wrong locus and nothing to say so. All six entries now agree with CHAIN_SLOT_LABELS in model/src/types.ts, checked value by value.
block-extra.ts re-exported it so wf.test.ts could reach it through the facade. The reason was test-internal — the workflow tests fabricate the verdict the UI would normally mirror in, because projectArgs refuses without one, and a hand-rolled key would drift from the gate it means to exercise. That reason is sound; routing it through the facade was not. The facade is the contract a consumer sees, and a helper the run gate happens to use is not part of it. The test package already depends on the model directly, and messages.test.ts was already importing the same function that way. wf.test.ts now does too, so the two agree, and block-extra.ts is back to its comment header. The facade's surface is once more exactly what the structurer generates: platforma, BlockContract, BlockOutputs, BlockData, BlockPointer and the ImportVdjBlock* aliases. pnpm check 12/12, unit suites 12/12. wf.test.ts is unchanged apart from the import line and still needs a live backend to run.
The v1 -> v2 rename block goes from an eight-line doc plus an inline note to one line, keeping only the fact a reader cannot recover from the code: the value mints an axis key, so it has to carry over. FileSource.datasetId keeps a one-line description.
prerunChecks and prerunDatasetCheck were the same thing twice: a verdict plus the
subject it was reached for, mirrored in because a V3 args lambda sees only data.
They are now one field tagged by which check it is.
prerunCheck?:
| { check: 'columns'; subject; identityCollides }
| { check: 'dataset'; subject; columnsPresent }
Tagged by check rather than by door, because the columns verdict is consulted on
the file door AND on the dataset door under the custom format — the tag is about
which question was answered, not where the data came from. The two paths through
projectArgs that consult a verdict return before reaching each other, so only one
is ever live, and the union states that instead of leaving it to convention.
Keeping them apart was justified by 'different invalidation triggers', which was
never true: the clear-watcher already wiped both on any of the four inputs, and
the subject comparison is what actually protects the gate. The plural/singular
mismatch goes too, and the two idempotent writers now share one guard.
No migration. Neither field exists on origin/main — both were introduced on this
branch — so no saved project can carry either. The v1 -> v2 rename stays, because
FileSource.sampleId did ship, in block 1.8.1.
Verified both gates against all five shapes each, including the case the merge
newly makes possible: a verdict for the OTHER check now reads as 'not judged yet'
rather than being destructured for a field it does not have.
pnpm check 12/12, unit suites 12/12.
The test still asserted on profiledSampleId, which was folded into
prerunDatasetValidationInfo. The output no longer exists, so the read came back
undefined.
Mine to have caught: when merging the two stamps I swept model/src, ui/src and
workflow/src for the old name and never looked in test/src.
It now asserts the whole tagged object — { door: 'file', datasetId } — rather than
one field, so a stamp emitted under the wrong door fails here too.
Full suite green against a local backend: 19/19, wf.test.ts 7/7.
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.
Greptile Summary
This PR adds prerun progress feedback and gates bare-set imports on identity-collision checks, while extracting the bare-set mapping UI into its own component.
BlockDatafor argument validation.Confidence Score: 4/5
The stale collision-verdict path should be fixed before merging because it can allow an unchecked sequence mapping to import and merge distinct records.
Collision detection depends on the mapped sequence columns, but the persisted verdict survives sequence changes and the run gate validates it using only the unchanged identity-column name.
Files Needing Attention: ui/src/app.ts, model/src/index.ts
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Select file] --> B[Profile columns] B --> C[Profile tagged with sample ID] C --> D[Map identity and sequence columns] D --> E[Prerun collision check] E --> F[Mirror verdict into BlockData] F --> G{Verdict matches current mapping?} G -->|Pending or collision| H[Disable Run] G -->|Clear| I[Project workflow arguments] I --> J[Import bare-set records]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "MILAB-6720: split the bare-set form out ..." | Re-trigger Greptile
Context used (5)