Put the DeltaSignal UI behind a per-deployment flag (and update the branch) - #198
Open
adamjohnwright wants to merge 138 commits into
Open
Put the DeltaSignal UI behind a per-deployment flag (and update the branch)#198adamjohnwright wants to merge 138 commits into
adamjohnwright wants to merge 138 commits into
Conversation
The two formats the Java exporter still owned, and the two that most obviously looked like the old site. Both now come from the render page, so a downloaded figure is the diagram the site draws. GIF is the expression animation: one frame per sample, 1s each, looping, and a single frame when there is no analysis. Encoding happens inside the browser -- a frame is tens of megabytes of pixel data and there is one per sample, so shipping them out to be assembled costs more than the finished file. The palette is built from every frame rather than the first, because one frame's palette shifts colours on samples whose values land elsewhere on the scale; that is why frames are drawn twice and never accumulated. Verified per frame rather than by file size: PMAIP1 goes dark purple at 0.2 to bright green at 5.2 across the four samples of a posted dataset. Capped at 2000px on the longest side. A diagram's coordinate space is around 6000px wide and a GIF pays for that once per frame -- uncapped, four samples came to 3.1MB. Now 735KB. PPTX carries the SVG with a PNG fallback. PowerPoint draws the SVG and its Convert to Shape turns the diagram into editable shapes. The alternative is emitting DrawingML per glyph, as the Java exporter does via Aspose: editable on open, at the cost of a second renderer to keep in step with the first and a commercial licence. One click is worth that trade. Two bugs found in the illustration code this reuses: - EHLD raster download scaled twice, once on the context and again through scaled destination dimensions, so a downloaded PNG or JPEG showed the top-left ninth of the illustration blown up to fill the file. - showAnalysisInfo assumed the analysis-info group contains a text element. Every other lookup there is guarded; this one threw part-way through and left the region half-decorated. Rasterising an illustration now lives in EhldService, shared by the download and the render page, since both need the styles inlined first: an EHLD's styling comes from the page's stylesheets and does not travel with the markup. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Clicking GIF or PPTX in the download panel now gets a figure drawn by the site's own renderer instead of by the Java exporter's reimplementation of it. That was the last place a curator could download something that looked like the old site. The app builds its URLs from RENDER_SERVICE, which is the current origin plus /RenderService, exactly as CONTENT_SERVICE works -- so it follows whatever host the bundle is served from. proxy.conf.js maps that path to the service on loopback, and serve-prod.js reads the same table, so beta and the dev server both reach it with no Apache change. The analysis token travels with the request, which is the point of a GIF: one frame per sample rather than a still. So does the sub-pathway preference, so the checkbox means the same thing for a server-rendered file as for one the browser produces. Encoding the token needed care. The analysis service returns it already percent-encoded, and setSearchParam encoded it again -- "...%253D%253D", a different token to everything downstream and a separate cache entry. It happened to still render, because the render page passed the same mangled value back to the same service. Illustrations keep going to the content service for GIF and PPTX: it serves the same illustration file either way, so there is nothing to gain. Clamped every number that arrives in a query string, now that the path is publicly reachable. scale=50 asked for a 320-megapixel canvas and got it, which is worse than an error: one query string for a gigabyte of someone else's memory. Scale is capped at the default, and nothing has needed more. deploy/render-service/ carries the systemd unit and says plainly what is still missing before this fronts reactome.org: rate limiting at Apache, Apache serving the cache directly on a hit, and a cache key that changes per release. Docs use --token "$ANALYSIS_TOKEN" rather than an angle-bracket placeholder. GitGuardian read the placeholder as a CLI-option secret and raised an incident; there was no credential in it, and a variable reference reads as one to a scanner as well as to a person. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The animation was sharp and the words in it were gone. A pathway's coordinate space is around 6000px wide and its font sizes are chosen for 1:1, so fitting it into 2000px scaled 8pt type to under 3pt and softened every arrow. Zooming in then magnified a downscale, which is not the same as detail. It renders at the diagram's own size now, and that is affordable because frames are differenced. 255 palette colours carry the picture and one index is kept back to mean "unchanged": every pixel equal to the previous frame becomes that index, written with disposal 1 so the previous frame shows through. Between two samples of an expression analysis only the node fills differ -- compartments, edges and every label are identical -- and a long run of one repeated index is what LZW compresses best. R-HSA-109606 over four samples: 3.1 MB undifferenced at full size, 966 KB differenced, against 735 KB for the unreadable 2000px version. Three times the resolution for 30% more bytes, and each extra sample now costs what it changes rather than what it contains. --max-size and ?maxSize= still cap it for anyone who wants a smaller file. Deployment is a container rather than a systemd unit, so a wedged browser or an out-of-memory kill costs one request instead of the feature. node:22 plus Chromium rather than a Playwright image: those carry three browsers and land around 3 GB, this needs one, and node:22 is already here as the app image's base. tools/render/ has its own package.json for the same reason -- four packages instead of the site's whole tree -- and render-deps.spec.ts fails if its pins drift from the root's, which matters most for Playwright, whose browser download is version-locked to the library. The container publishes no port. It is reachable from the app container and nowhere else, which keeps the property that matters: a render can only be commissioned through whatever fronts the site. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…a figure The full-size GIF shipped and curators kept getting the 2000px one. Nothing was wrong with the renderer: the old file was in Cloudflare and in their browsers, and neither was ever going to ask again. Figures are served `public`, so Cloudflare stores them and keeps serving what it stored with the max-age it stored it under -- a day. Reloading the page does not touch it either, because a download link's URL is not a page subresource and is never revalidated. And Cloudflare's own Browser Cache TTL overrides what the service sends: 300s went out as 4h. So the address has to change, which is what RENDER_VERSION in the app does. The service ignores the parameter, so both versions of a figure share one entry in its disk cache, and everything downstream sees a new resource. Bump it with RENDER_CACHE_KEY whenever the renderer's output changes; the table in deploy/render-service/README.md says which invalidates what. Also an ETag, from the same key -- everything that determines the bytes is in it -- answered before any render happens, so a repeat download is a round trip rather than a browser holding a stale figure or the box drawing one twice. Verified by clicking the button rather than by reading the code: the download comes out 5775x2366, and cf-cache-status is a MISS on the versioned URL. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…xplicit CI has been failing since the GIF work landed, and every failure -- including the two dependabot pull requests -- was the same two items: gifenc and fflate looked like unused dependencies. They are imported from tools/render, which knip was not configured to look at. It scans tools/**/*.mjs now, with the CLI, the service and the SVG harness as entry points, so the count is back to the baseline of 147 with more of the repo covered than before rather than less. MAX_FRAMES and renderUrl stopped being exported; nothing outside their own modules used them, and exporting a constant only makes it look like API. Also the theme, which was implicit and machine-dependent. DarkService defaults from localStorage and then from the browser's prefers-color-scheme, and the render page inherited whatever that decided -- so the same request could produce a light figure here and a dark one elsewhere, with one cache entry for both. ?dark=true now asks for dark and nothing else gives it. The dark palette is plumbed through and cached separately, but the download panel does not offer it, deliberately: it is a screen theme. Standalone it reads as muddy -- pale mauve compartments, and sub-pathway labels whose dark halos exist to sit on a dark canvas. Frame capture takes the diagram's own background colour rather than a hardcoded white, which is what made the dark case wrong in a way that would have looked like a renderer bug: a dark palette on a white ground is neither theme. And a regression caught on the way: setting the GIF's MAX_SIZE to 0 for "the diagram's own size" left PowerPoint's fallback raster computing scale from MAX_SIZE / longest, so it asked for a PNG at scale 0. The zip would still have cleared the size floor, because the SVG in it is the real picture -- a blank fallback nobody would find until the one viewer that needs it opened the file. It has its own constant now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…kage.json A second manifest with its own pins was the obvious way to keep the image small and the wrong one. Dependabot only watches the root directory, so a bump there would have left the service's copy behind -- and the drift test I wrote to catch that would then have failed every dependabot pull request until someone edited a file dependabot cannot see. The express bump waiting in the queue would have hit it immediately. The Dockerfile now picks the four packages it needs out of the root manifest at build time, so there is one set of pins and nothing to keep in step. That matters most for Playwright, whose browser download is version-locked to the library: a mismatch there fails in a way that reads as a rendering bug. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…n stamp Two illustration bugs, both hidden by the fact that a diagram worked: A .png of any illustrated pathway was a 500 -- "this view cannot export PNG yet" -- because the export only knew how to ask a cytoscape instance. Illustrations are the top-level pathways, so it was the ones a report is most likely to want. They go through the same rasteriser the animation frames use now. A .pdf of one was a blank 674 bytes, caught only by the size floor. An EHLD declares width and height of 100% with no viewBox, so outside the page it has no intrinsic size at all, and its styling comes from the page's stylesheets rather than the markup. EhldService.svgMarkup writes both down, and the SVG export uses it too -- that one was shipping unstyled. And the version stamp goes away. Figures are `private, no-cache` now, so nothing reuses one without asking; the ETag makes a repeat download a 304 in ~1ms. It was introduced to defeat Cloudflare storing a figure for a day and serving it with the max-age it was stored under, which is real -- but it meant two constants that had to be bumped in step, and I bumped one and not the other within the hour, which produces the worst case: a new address answered from the old cache. One knob now, RENDER_CACHE_KEY, and nothing new gets stored anywhere. Bounded the cache while here. A figure averages a couple of megabytes and there are thousands of diagrams times five formats, on a host that also runs Tomcat, Neo4j and the site's builds with 4 GB free. Filling that disk takes the site down, which is far worse than paying for a render again -- everything in the cache is derived data. 2 GB by default, least-recently-used evicted first, and evictions are logged: a cache that silently discards what it was asked to keep looks exactly like one that is working. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The timeline could only be held still by keeping the pointer on the button, which is what the issue means by "instead of just over". All three controls it asks for now work, and the animation is CSS keyframes, so each needed a different answer: - Play/pause on click. The button carries both icons and a class on the root decides which shows, so it says what clicking will do rather than what the animation is doing. Pausing sets that class and also pauses each animation through the Web Animations API -- the class alone holds the picture still, so in a browser without the API play/pause still works and only seeking is lost. - Clicking a segment seeks the whole picture there, by setting currentTime on every animation at once. Verified against the timing rather than by eye: segments land on 1s, 3s, 5s, 7s for four samples at 2s each. - Hovering a segment names the sample, through a native <title> tooltip. That needs no script at all, which matters because a script in an SVG only runs when the file is opened as a document -- not inside an <img>. The hover-to-pause rule is kept for that case and scoped so the two never fight. Two things found on the way: A sample name containing "<" silently truncated its label. The markup is built as a string and parsed by innerHTML, which turns "<baseline>" into an element SVG does not know and therefore does not draw, taking the rest of the label with it. "&" was already being normalised by the serialiser, so only "<" bit. Names are escaped now, and a column called `Ctrl & <baseline>` round-trips exactly. And console.table of every frame's transition times, on every export. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Flagging used to repaint the flagged groups in the flag colour and, with no analysis running, wash every other group out to the surface colour. So a curator could see where a gene appears, or see their analysis result, but not both -- and the pathways where they most want the result are precisely the flagged ones. The flag is an outline now, drawn in groupContentDecorator by replaying polygonContext: that buffer holds the commands FoamTree used to trace the group's own polygon, so the stroke follows the real Voronoi shape rather than an approximating rectangle. The fill is left alone, so family colours and the analysis overlay both survive. Two strokes, a dark one under the flag colour, so it reads on a pale fill as well as a saturated one, and thinner at depth so a flagged child inside a flagged parent stays legible. The path is replayed once per stroke rather than stroked twice: on canvas either works, but the SVG export draws through svgcanvas, which records one path element per path and keeps only the last style set on it -- the halo was silently missing from every exported figure while looking right on screen. Checked in the exported SVG, not just on screen: both widths are there. Triggering moves to onSurfaceDirty, because flagging changes without the layout changing. The decorator returns immediately unless something is flagged, which is what keeps that affordable on a hierarchy this size. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The event hierarchy's auto-scroll was the good version of this behaviour, and it lived as a document.querySelector from the component after the tree finished building. Three other places had grown their own copy, each with slightly different behaviour and each reaching into a template it did not own by an id convention. RevealDirective inverts it: the element that knows it is selected reveals itself. Timing follows rendering rather than a guess about it, so a node that appears later -- as its branch expands, or when the tree is rebuilt for an analysis -- is revealed when it appears instead of racing whatever finished first. Applied to the hierarchy's nodes, the analysis result table's rows, and the pathway list under a selected search result. The hierarchy's fallback of scrolling to the pathway when the URL selects something that is not an event in the tree is kept, as `select() ?? pathwayId()`. `node.isSelected` was the obvious input and is the wrong one: it is also set on every ancestor of the selection, to draw the path. Two frames rather than one. One frame is enough for the element to exist and not enough for it to be where it ends up -- a table still expanding rows, or paging to a different page, moves after the first -- and revealing then left the row 28px past the edge of its container. Verified by measurement, not by eye: the hierarchy scrolls 469px to bring the selected event inside its container, the result row lands directly below the sticky header, and window.scrollY stays 0 in both, which is the page-drag that block:'nearest' exists to prevent. Also respects prefers-reduced-motion, which the copies did not. Left alone deliberately: found-table re-scrolls its parent row when its own content finishes loading, which is growth rather than selection and does not fit the directive; the object tree the issue mentions has no selection to bind to yet. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Six new things for them to look at, one of which we cannot check ourselves: the PowerPoint file is structurally valid, but "opens in PowerPoint and converts to editable shapes" needs PowerPoint, and there is none on the build machine. That one is also a decision they may want to push back on, so it says what we traded and why rather than only asking them to click. Also records what is deliberately not offered -- dark figures -- and what fails together: GIF and PPTX come from a service running alongside the site, so if those two buttons break it is almost certainly the service rather than the diagram, and saying so saves a round trip. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
"What does this protein have to do with Reactome?", ported from the Vue app on
idg.reactome.org, reachable from a tile on the homepage next to ReactomeFIViz.
The data did not have to move first, which is the reason this is a front-end
change and nothing else. idg.reactome.org/idgpairwise is public, answers with
Access-Control-Allow-Origin, and returns TANC1's 482 enriched pathways in 70ms.
IDG_SERVICE is the one line that changes when the data moves here.
The pathways it finds are ours, so the table links into our own Pathway Browser
rather than carrying the IDG portal's GWT diagram widgets across -- those widgets
are the thing this year's work replaced.
Two endpoint names are misspelled in the service ("realtionships", "Pathays").
They are spelled that way here too, because the service is what has to answer.
An empty result now says which kind of empty it is. checkTerm tells "we have never
heard of this symbol" apart from "this protein has no enriched pathway in the
datasets you picked", and those call for different next steps -- check the
spelling, or add datasets.
Handling the IDG server being down took two goes. Reading value() on a resource
that failed throws, and a throw inside a computed the template depends on takes
the render with it, so a 503 from IDG produced "nothing found for TANC1" -- the
page blaming the gene for the server being unreachable. Every resource read is
guarded by hasValue() now and failure is judged by status(), checked before the
loading and empty-term branches because it can happen before anyone searches.
Verified against a 503 and a refused connection, and that the healthy path still
returns 482 rows with no page errors.
Breadcrumbs title-cased the segment, so /idg read as "Idg". Acronym segments are
upper-cased now -- idg, api, doi, faq, orcid, toc -- which is right for all of
them and wrong for none.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`ng build --watch` empties and rewrites the output directory, so for the ten to twenty seconds a build takes there is no index.html to send. Requests landing in that window surfaced Express's ENOENT stack trace, which reads like the site is broken rather than busy, and gives whoever is looking at it no way to tell those apart. Adam hit exactly that while I was rebuilding beta. 503 with Retry-After is the honest answer -- the server is fine, the build is not there yet -- and the page refreshes itself so nobody sits reloading. Anything else that cannot be read is still a 500, because that is a real fault. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…bution The table had 482 rows for TANC1 because I passed a p-value where the service wanted a score. `prd` is the functional-interaction cutoff and defaults to 0.9; 0.01 meant no filtering at all. Two modes exist and the service picks by whether datasets are named, which is worth knowing: with datasets chosen `prd` is ignored entirely -- TANC1 returns the same 482 pathways at 0.01 and at 0.99 -- and only the no-datasets path actually filters. The threshold cannot have a fixed default. The service's own is 0.9, and TANC1's best predicted interactor scores 0.891, so 0.9 returns nothing whatsoever for it. It starts at the 90th percentile of the gene's own distribution instead, says how many interactors that keeps out of how many there are, and says what the best score is -- so a threshold that returns nothing is visibly the threshold's fault. Under it is the distribution itself, 28 buckets, filled where they are kept and faded where they are not. TANC1's scores sit between 0.24 and 0.89 with a median of 0.52, which no fixed number would have suited. The table defaults to FDR <= 0.05 and lowest-level only, both untickable. TANC1 goes from 482 rows to 71, and the top of it is now NMDA receptors and postsynaptic transmission, which is what TANC1 is for. And the overlay this was missing: the kept interactors can be run through Reactome's own analysis, which returns a token, which colours Reacfoam and every diagram through machinery that already exists. It is not IDG's enrichment recomputed -- it is Reactome's overrepresentation of the interactor list -- and the button says so rather than implying the numbers match the table. The token came back percent-encoded and the router encoded it again, which is the same double-encoding that bit the download links. Decoded at the service boundary this time, so every caller can encode once. Still missing IDG's own diagram: a pathway-similarity network, 412 nodes coloured by weighted Target Development Level and 22,509 edges. The edge count needs a filter of its own before it is worth drawing, so it is not in this pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The table had p-value and FDR, which any enrichment gives you. What makes this the *druggable* genome is Target Development Level, and the page was throwing it away. It comes from the network endpoint, which returns the same lowest-level pathways the table does -- 267 nodes against 267 lowest-level rows for TANC1 -- with a weighted mean TDL per pathway. The scale runs tDark to tClin and low is dark: TANC1's enriched pathways run 1.69 to 3.50, with FCGR3A-mediated phagocytosis at the dark end and acetylcholine binding at the drugged end. As a column, and as a plot of significance against how well studied, where the interesting corner is high and to the left: enriched for this protein's interactors, and full of proteins nobody has drugged. Lower-case symbols found nothing. The service's index holds upper-case human symbols only -- checkTerm says TANC1 exists and tanc1, Tanc1 and Trp53 do not -- and the enrichment endpoint answers a lower-case term with zero pathways rather than an error, so "tanc1" looked exactly like a gene with no enriched pathways. Terms are upper-cased on the way in, which cannot collide with a mixed-case symbol from another species because none is in there, and UniProt accessions are upper-case already. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
I had built one plot of my own choosing and left out all three of IDG's, which was the wrong way round. So I drove the real portal and recorded what its results page asks for, rather than guessing again: - **Interacting Pathway Plot** -- significance per pathway, coloured by top-level pathway. That is the "type of pathway" that was missing, and it is the thing a table of p-values cannot show: whether a gene's hits cluster in one part of biology or scatter across it. TANC1's cluster in Signal Transduction (18), Immune System (7) and Neuronal System (7). The mapping comes from getHierarchicalOrderedPathways -- 2,730 stId-to-top-level pairs, fetched once and shared. - **Genes vs Functional Interaction Score** -- how many predicted interactors survive each threshold, as a curve. More useful than my histogram for choosing a threshold, because it says what a move costs. - **Feature Summary** -- interactions per data source, coloured by kind of evidence. Its request wants gene *names* and dataset *id strings*, not the digital keys everything else takes; sending keys returns an empty list rather than an error, which is how I first concluded the endpoint was not the right one. Three defaults now match the portal, and the numbers agree exactly: threshold 0.8 rather than a percentile, so TANC1 has 11 interacting genes and 54 pathways where the portal shows 11 and 54; a Genes column, which the payload had as numGenes all along; and ten rows a page with a working pager rather than a hundred-row wall. The TDL plot I added stays. It is not in the portal's results page -- TDL appears there only inside the pathway overview -- but low TDL is what "druggable" means, and having it next to significance is the reason to look. Plots are positioned elements and one polyline rather than a charting library: every point is a link with a tooltip, the shapes are scatter and a line, and the repo has no charting dependency to justify adding one for that. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Left over from wiring the feature summary. My removal patch silently missed because prettier had already collapsed the import to one line, and I committed over the top of a lint gate that was one warning above the baseline -- exactly the failure I asked the asserts to catch, on the one edit where I did not check the gate afterwards. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
CI failed and it was right to: the homepage test asserts the exact set of shortcut cards, and I added a sixth without telling it. That test exists because these cards have been silently mis-wired before, so the answer is to declare the card, not to loosen the count. Also a smoke test for /idg, deliberately stopping short of the results table. That data comes from idg.reactome.org, a different server from everything else this suite touches, and a test that needs it to answer would fail here whenever it is down while saying nothing about our build. What is guarded is the part we own: the route renders, and searching a lower-case symbol puts the upper-case one in the URL -- the bug Adam found by typing "tanc1". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The last thing the portal's results page has that ours did not. Clicking a row opens the pathway's description and its place in the hierarchy, both from our own content service -- which is what the portal does too, so an expanded row costs nothing from a machine we do not control. One row open at a time, as it has. The ancestry comes back with the pathway first and the root last, which is the opposite of how a breadcrumb reads, so it is reversed: Neuronal System → Transmission across Chemical Synapses → Neurotransmitter receptors and postsynaptic signal transmission → Activation of NMDA receptors, which is what the portal shows for the same row. Summations carry markup -- `<br>` between paragraphs, the odd `<i>` -- and my first version rendered "...Traynelis 2018).<br>NMDA tetramers assemble..." with the tag visible. Converted to text with real line breaks rather than passed through innerHTML: nothing in a description needs to be clickable, and text cannot inject anything. Also settles where the network view belongs. IDG's "Open Pathway Overview" leaves the results page entirely -- it opens their GWT browser at #/FLG=TANC1&DSKEYS=0&SIGCUTOFF=0.8 -- so the pathway-similarity network is a feature of that browser, not of this page. Our equivalent is the genome-wide overlay button already here. Nothing on the results page is missing now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…aros The threshold note said "11 of 2541 predicted interactors score 0.80 or better" without ever saying which eleven. They are behind a collapsed summary now -- supporting evidence for the table rather than the point of the page -- strongest first, with their scores. For TANC1 they are SPTAN1, five tubulins, HOMER1, KRAS, SHANK1, DLG4 and CAMK2A: postsynaptic scaffolds, which is what TANC1 is. The CSV is built from what is already loaded rather than through the service's own download endpoint. The numbers are on the page to draw the list, so asking a second machine to derive the same file is a round trip and a failure mode for nothing. And a Pharos link per gene, which is where the Target Development Level in the table comes from -- so it is the right place to send someone who wants the evidence behind that number. Deliberately not included: the annotated-pathways tree. It is the one thing on the portal's results page we still do not have, and the one I would add next, but Adam's call was these three. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The file carries every row the filters kept, not the ten on screen -- a file of ten because that happens to be the page size would be a surprise. 54 rows for TANC1 at the default threshold, which is what the heading claims. It also carries the weighted TDL column, which the portal's own download does not, because building the file from what is already loaded means it can include anything the page knows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The container built, started, reported healthy, and failed every render: "Executable doesn't exist at /home/node/.cache/ms-playwright/...". `playwright install` had run as root and put Chromium in root's cache, while the service runs as node and looks in its own. PLAYWRIGHT_BROWSERS_PATH puts it somewhere neither owns, chowned to node. The headless shell is named explicitly too, since that is the binary chromium.launch() actually wants. Worth remembering what the health check did not catch: it proves the HTTP server answers, not that a browser can start. Healthy and useless at the same time. Compose also had to match this box, where the site runs on the host rather than in compose: the container renders host.docker.internal:4200 and publishes 4310 on loopback for serve-prod to proxy, and no longer depends_on app, which would have started a second app container to fight for :4200. A deployment where both are containers drops the published port and uses the service name. Verified through the container: R-HSA-109606 GIF at 5976x3350 in 22s, PPTX 1.1MB, an illustration GIF in 4.4s, and beta serving a cache-busted request end to end with cf-cache-status BYPASS. The cache is on a volume and a repeat is a 6ms hit. The restart policy is configured but not proven here, and the README says so: a crash is hard to simulate honestly. docker kill is a manual stop, which docker does not restart; kill -9 1 inside is ignored because the kernel shields a namespace's init from its own signals; and signalling from the host needs root. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adam's aim is that a green suite stands in for days of curator testing. This is the first slice of that, built from Appendix R3 rather than from what I happened to touch. RELEASE-TESTING.md is the new-UI successor to that document. Same ground -- front page, navigation, Pathway Browser, in-diagram search, details panel, context menus, the four analysis tools, downloads, post-release -- but every row says whether a test proves it, a human must do it and why, or it is a gap and therefore work. 53 rows, and honestly a minority are automated today. It should get shorter every release. Three specs, chosen because each covers something that has actually broken: nav-links crawls all 73 links in the rendered navigation and fails naming any that 404 or render an empty shell. This is the longest paragraph in the old document, and the curators' QA pass found Analyse Gene Expression 404ing and Species Comparison on a blank panel. The list comes from the rendered nav rather than nav-options.json, because the dropdowns are CMS content -- so a page an editor adds is covered without anyone updating a test. downloads checks what is *inside* each file. Every download bug this month passed "did something download" and fails this: a .jpeg holding PNG bytes, a blank 674-byte illustration PDF, an illustration PNG showing its own top-left ninth. GIF and PPTX come from the render service and skip with a reason when it is not running, since its absence says nothing about the build. in-diagram-search covers the USP10 case the document asks for by name, and was one of the four blockers that stopped the curators' pass entirely. Two traps are written down because I hit both: export URLs 403 without a Referer (protect-exporters.sh sheds crawler load that way, and it looks exactly like a broken feature under curl), and download buttons must be matched by their label's own text, since the icon ligature makes the button read "imageSVG". And the limit worth saying out loud: the suite runs against localhost:4200, so Apache rules, Cloudflare and TLS are outside it. Green means the application is right, not that beta is serving it correctly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The PPTX floor was 10KB, which was right when a package carried a picture of the diagram and wrong now: a reaction's figure is 5.6KB of shapes, and the check rejected it. Lowered to 4KB, which is the package boilerplate. A byte count was never the interesting question anyway. Both PPTX tests now read the slide: the diagram's has over a hundred shapes and no picture, and the reaction's has shapes on a slide a few inches across rather than the hundreds on the 56in slide a whole pathway needs -- which is the same claim the SVG case makes about being the reaction's own figure. Found by the full preflight; `--fast` skips these, so a real regression sat in the tree while the quick gate said all clear. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The smoke it skips is the only check that downloads a file and looks inside it. A change to an exporter can sit in the tree with fast mode reporting all clear, which is what happened here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An adversarial pass over yesterday's PowerPoint work, against the diagram it claims to reproduce. **Arrowheads carried no meaning.** The model had `arrow: boolean` and the exporter turned every true into a filled triangle line end. The diagram draws four heads and three of them say something a triangle does not: catalysis is a hollow circle, positive regulation a hollow green triangle, and negative regulation a bar across the line. So a slide drew inhibition as activation -- a figure asserting the opposite of the pathway. OOXML line ends cannot fix this: they are always filled, always the line's own colour, and there is no bar among them. Arrowheads are now geometry the page computes and the exporter merely spells, which is the same division everything else here follows. R-HSA-109606 exports 129 of them: 84 filled triangles, 17 filled circles, 11 hollow circles, 9 hollow triangles and 8 bars -- and the 8 bars and 20 hollow heads match, one for one, the 8 red and 20 green marks the site's own SVG draws. **Framed exports put shapes off the slide.** Keeping a glyph that crosses the frame is right; keeping the parts of it that fall outside is not. An SVG has a viewBox and never draws them; a slide has nothing of the sort, so a connector spanning the diagram arrived as an object 31 inches off the side of an 11 inch slide. Connectors are now clipped to the frame -- per segment, so one crossing the frame twice comes back as two runs rather than a line joining them through the middle -- glyph boxes are clipped with them, and an arrowhead is drawn only where the end it marks is in view. Both fixtures now assert the invariant that broke: every shape lies inside the extent the page declares. Also: a text run keeps its alpha, and a filled shape with no border no longer asks for a zero-width outline, which PowerPoint draws as a hairline -- on a 16px arrowhead that is most of the arrowhead. RENDER_CACHE_KEY goes to v5. v4 was published this morning and the output has changed again; the second time in one day that a correct build served a stale file, so the note about it now says to treat the key as part of the change. Not defects, though they looked like ones on the way past: the analysis overlay is carried in full -- the colours a run adds are all exported -- and the 69 greys the SVG "draws" and the slide does not carry every one have fill-opacity="0". The instrument that found them ignored opacity. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…ed (#170) Curators: "H2O and H+ disappear with zooming out." Separately, #150: a molecule's chemical structure drawn with no molecule underneath it. Same fault from two sides -- the node was hidden while a different handler carried on drawing its structure. Flagging pins trivial molecules visible by marking them `.always-visible`, a class rather than an inline style precisely so nothing can lose it. But the zoom handler writes an *inline* opacity across every `.trivial` element, and in cytoscape an inline style beats any stylesheet rule, so it painted straight over the class. Detaching it from the zoom event -- what the previous fix did -- was not enough: `triggerZoom()` calls it directly, on every restyle and whenever an interactor opens. The class is now the authority: the handler leaves anything wearing it alone. Measured on beta, with a flag active, opacity is 1 at every zoom and across a restyle; with no flag, molecules still fade out and back as before. Also, the reason this took two attempts to land: `build:libs` did not build reactome-cytoscape-style. CI compensated with a step of its own, so a fix to that library passed CI and never reached the dev server, which builds the app without building the library. `build:libs` now covers every workspace library and `build` depends on it, so any path that produces the app produces what it links against -- and CI's two special-case steps are gone, because it now runs the same command a developer does. The regression test is end-to-end rather than a unit test: the library cannot be constructed headlessly (its layers plugin refuses), and the bug lives in what a real renderer computes. It reads opacity through the registration cytoscape keeps on its own container, which beats sampling canvas pixels -- those cannot say which element was faded. Verified to fail without the fix: both flagging tests go red, the unflagged one stays green. Fixes #150 Fixes #143 Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…171) Curators, re-testing #154: "the components are shown, but an indication of which component is hit in the analysis is missing." It was not missing. Run against a real tissue analysis -- the HPA experiment through /ExperimentDigester, posted exactly as the Tissue form posts it -- every component row carries a marker: 8 popups, 15 rows, none unmarked, and the found/not-found split matches the data. So the report is about what the panel looks like, and on that it is right. Three things made a present indication unreadable: - **The marker was on its own line.** The row is a column flex, so a 7px dot sat above the name rather than beside it, unattached to anything. - **A hollow dot was the entire signal** for a component your data missed. On a row with no values beside it there was nothing else to read. - **The values were three bare numbers.** Which tissue each belonged to was in a tooltip, and nobody hovers a tooltip to discover a column exists. Now: the dot sits against the name it belongs to, with a ring that separates found from not-found at a glance; a missed row says "not in your data" in words; and each value is a chip carrying its sample's name. Also fixes the emphasis that was supposed to mark the sample the diagram is coloured by, and never once did. `state.sample` holds the column's *name* -- `samples()[index]` is what sets it -- so reading it as a number gave NaN for every real analysis. It only appeared to work when no sample was set, where the fallback was the first column anyway. Verified on beta across all four states: tissue expression (chips, current sample emphasised), overrepresentation (markers, no chips), a component the data missed, and no analysis at all (no marker, no text). Refs #154 Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
86 links in this site's own news archive open the pathway browser with
nothing in it. They are the old browser's fragment links, and they come in
two spellings because the old site produced both:
#/R-HSA-1430728 770 links, work
#R-HSA-202733 41 links, blank page
#R-HSA-8853659.1 45 links, blank page
FRAGMENT_PATTERN required the leading slash. Without one nothing matched,
so no id was extracted and the browser opened on no pathway -- from a link
in a release announcement, which is where someone arrives from a citation.
The slash is now optional and the id is required, which is what keeps a
fragment that is not a pathway -- `#introduction`, naming a section to
scroll to -- falling through untouched instead of being read as a stale
route. A trailing stIdVersion is consumed and dropped: the content service
does not want it, and it used to arrive as a query parameter called ".1".
Verified on beta: all three previously blank forms now open their pathway,
and the two that already worked are unchanged, parameters included. The
test cases are taken from what is actually in the content rather than
invented, and fail against the old pattern -- bare id and version go red,
the four that already worked stay green.
Not covered: 37 links spelling `#TOOL=AT`, the old analysis tool fragment.
They fall through as before rather than opening anything, and where they
should land is a product decision rather than a regex.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
#174) `CompareFormComponent` is the Angular CLI's generated placeholder -- eleven lines of TypeScript, an empty stylesheet, and a template reading `<p>compare-form works!</p>`. It has been in the tree since the initial commit, and it renders into every pathway browser page inside a panel that can never open: the panel is gated on `dropdown() === 'compare'`, and nothing anywhere sets that state. It cost me twenty minutes today. Chasing whether the toolbar's Compare button worked, I found a component named compare-form, a dropdown state called 'compare', and no way to reach either -- which reads like a broken feature rather than an unfinished one. Comparing against a disease variant is the Compare button in the toolbar, and it works: it opens a menu of the disease variants and navigates. That is the whole feature. So the state is narrowed to 'analysis' | null, the panel is gone, and the scaffold with it. Verified on beta: the Compare menu still lists "Defective Intrinsic Pathway for Apoptosis", Analyze still opens its form, no cr-compare-form left in the DOM, no console errors. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
I called this gap minor when I documented it. It was not: 65 of this diagram's 203 glyphs came out as empty outlined rectangles -- every complex, every set, both genes. Those types set `background-opacity: 0` and carry their whole body in a `background-image`, so reading `background-color` gave nothing to fill with. The images are SVG markup rather than rasters, so the real path is there: it is flattened with the browser's own geometry (`getPointAtLength` walks the arcs in a set's braces exactly), simplified, and handed over as a closed filled polygon -- which the exporter already knew how to write, from the arrowhead work. Nothing about Reactome's glyph vocabulary reaches it. A complex is an octagon with a white inner line; a set is a braced box with its drug marker in the corner. Three things had to be right and were not at first: - **Each image keeps its own position and size.** A set carries three -- its body twice and a 22x24 marker at a fixed offset -- and stretching every one over the node drew that marker across the whole glyph. - **`defs`, `clipPath` and `mask` hold definitions, not content.** Reading every element in the document drew a mask's white and black rectangles onto the slide and pulled the bare `<use>` inside a clipPath out as a shape with no paint at all: 40 of those on one diagram. - **A masked outline is clipped to what the mask reveals.** A set's braces are its whole outline stroked and masked down to the two ends; ignoring that drew a white line all the way round it. Simplification matters here. Walked at two units a step an octagon comes back as 161 points, and 229 glyph layers of that put 37,000 points and 2MB of XML on one slide; Ramer-Douglas-Peucker at 0.4 units keeps the arcs and gives back the eight corners -- 5,000 points, and the file went from 136KB to 54KB. RENDER_CACHE_KEY goes to v6, and the container is rebuilt: the output changed, so the key is part of the change. Verified: 973 shapes and no empty-outlined boxes in the payload, the curator-facing download through Apache carries 974, and SVG, PNG and GIF are untouched. The three failures above each have a test taken from the captured payload. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
An adversarial pass over #176, which had just landed. Framing was applied by each producer as it built its shapes, and that does not hold: every shape type added afterwards bypassed it silently. **Arrowheads protruded past the frame.** Two of five sampled reactions did it -- a head is drawn back from its tip and spread across the line, so a tip just inside the edge puts part of the head outside. The gate was on the tip alone. This is the defect #169 fixed for connectors, reintroduced by the next shape type to be added. **A glyph the frame cut was squashed rather than clipped.** Its body was scaled into the node's *already clipped* box, so `CASP3(1-277) dimer` -- 248 by 74 in the diagram -- arrived as a whole octagon crammed into a 20px sliver, every corner intact. `SEPT4 dimer` came out 5px wide instead of 176. Every partially visible complex on a framed slide was a distorted miniature of itself. Both have one cause, so there is now one clip. Geometry is built at its true size and position, and `clipToExtent` cuts the finished list: a node box is intersected, an open polyline becomes the runs that survive, and a closed polygon is clipped as a polygon -- Sutherland-Hodgman against the frame -- so a filled glyph keeps its fill and gains an edge along the frame rather than falling open and hollow. A shape type added next cannot forget to be clipped, because nothing asks it to remember. That also removed the per-producer clipping from the node and edge loops, which is why they read shorter than they did. Verified across five framed reactions: 0 shapes outside the extent, from 2. A cut glyph now has points on the frame edge, which is the difference between clipped and squashed, and the tests say so -- one fixture is captured from a selection that used to leak, because a single frame held the invariant by luck rather than by construction. RENDER_CACHE_KEY goes to v7; the unframed export is byte-identical, the framed one is not. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…#178) * feat(downloads): say the file is being made, and never save a failure A diagram's PowerPoint or GIF is rendered on demand and takes seconds. Handed to the browser as `<a href download>`, that wait is silent -- no sign anything is happening, no way to tell a slow render from a dead link. And a failure is saved. The render service answers an id it cannot resolve with `404 {"error":"no such pathway: R-HSA-000000"}`, and the browser writes those 41 bytes into `R-HSA-000000.pptx`, which PowerPoint then offers to repair. The reader is told nothing and left holding a broken file. So the page asks for the bytes itself: a spinner and "Preparing…" while the file is being made, a percentage once it is arriving, and the file saved only when the whole of it is here and the server said it was good. A failure shows the server's own sentence -- "Could not download: no such pathway: R-HSA-000000. Click to retry." -- and writes nothing. "Preparing…" is a state rather than 0% on purpose. Until the file exists the server is not sending, so a fraction would be a number we invented; only once bytes arrive is there an honest one, and it is shown only when Content-Length said what to expect. It stays a real anchor. A modified click -- ctrl, middle, save-link-as -- is the reader asking the browser to handle it, and those are left alone. Worth keeping in mind for the API rewrite: this only works because our render service builds the whole file before it responds, so a response that starts is one that finishes and its length is known. The Java exporters stream while they generate, which is why a mid-render failure there yields a truncated file that looks complete, with no size known in advance. Specs cover the phase machine, the failure messages and the filename parsing as pure functions -- no TestBed, no HTTP stack -- and one of them caught `filename=""` being read as a file literally called `""`. The end-to-end spec routes a 404 itself, so the case that matters runs in CI whether or not a render service is up. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(downloads): let a cross-origin deployment fetch its own downloads An adversarial pass over the download feedback, before it merged. It would have broken PowerPoint and GIF downloads outright on the published artefact. That build is served from one origin and points at beta for its backend, so its downloads are cross-origin. An `<a href download>` does not care. A fetch does: /RenderService sent no Access-Control-Allow-Origin, and a cross-origin fetch of a render is blocked by the browser -- proven from a page on another origin, where /ContentService answered 200 and the render service failed outright. Replacing the link with a fetch would have turned a working download into a button that does nothing. So the render service says who may read it, as /ContentService already does, and exposes Content-Disposition and X-Render-Cache -- without which a cross-origin page cannot see the name the server chose or whether the figure was already made, both of which it uses. Verified from another origin: 200, 54224 bytes, filename readable, cache hit readable. And the page no longer depends on that being true. A failure with a real status is the server's answer and is shown; status 0 is not an answer at all -- CORS, offline, an extension -- and the file is handed to the browser instead, which is how it behaved before any of this existed. A reader gets the download without a progress bar rather than a dead button. Four smaller things from the same pass: - The blob URL was revoked in the same task as the click. Chromium copes; other browsers cancel the save. It is revoked a minute later instead. - The button never stopped a download when it went away. A render takes seconds, and a reader who moves on should not have a file dropped on a page they have left. - `cached` was carried through the phase machine and never displayed. Removed rather than left as state nobody reads. - The live region only existed once it had something to say, which is exactly when a screen reader will not announce it. It is always present and empty until it is not. The last of those broke two of my own end-to-end tests, which had been matching `.button__state` unscoped -- there is now one on every button. The tests were reading somebody else's live region and are scoped to the working button. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
The download tab got this; the detail page's bar did not, and that is where
the waits are longest. Measured on beta:
reaction SBML 14 KB 0.1s no Content-Length
reaction PDF 78 KB 1.5s no Content-Length
pathway SBML 807 KB 8.6s no Content-Length
pathway PDF 2.1 MB 8.6s no Content-Length
Eight and a half seconds of nothing, and no length -- the content service's
exporters stream while they generate, so the browser cannot know how big
the file is, and a failure part-way through leaves a truncated file that
looks complete. Fetching in the page cannot stop the streaming, but a
stream that breaks throws, and nothing broken gets saved.
A directive rather than a component: these are plain text links in a dense
bar, so the state shows through an attribute and the stylesheet instead of
rebuilding markup around seven anchors. A spinning ring while it works, how
much has arrived beside the label, and a wavy red underline with the reason
in the title if it failed. What is shown is a size rather than a
percentage, because with no Content-Length a percentage would be invented.
Verified on beta: SBML saves 807,164 bytes of real SBML with "Preparing…"
shown throughout, and a 403 carrying an HTML error page saves *nothing* and
reads "Could not download: the server answered 403" -- the page of HTML is
deliberately not put in front of the reader.
One of the new tests would have skipped for ever. It gated on fetching an
exporter URL from the API request context, which beta's bot protection
answers with 403 -- so the gate said "not reachable" while the browser the
test runs in gets 200. It gates on the content service instead.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…is (#180) An adversarial pass over #178 and #179. **A request that was never answered spun for ever.** Thirty seconds into a route that never replies, the link still said "Preparing…", and there was nothing the reader could do: clicking again was ignored while busy, and there is no browser download UI to cancel from, because the page is doing the fetching. An `<a href download>` at least gave them the browser's own cancel. So there is a three minute ceiling -- past a genome-wide render at six seconds, a pathway PDF at nine, and the render service's own forty-five second limit -- and, more usefully, clicking a working download stops it. The title says so while it works: "Being made on the server… click to stop". **And there were two implementations of all of this.** The download tab's button had its own copy of the waiting, saving and failing, written before the directive existed. They had already diverged, and every edge case in this pass had to be considered twice. The button now uses the directive and holds only what it looks like -- a spinner in the icon's place, the state beside the label. Dropped the cancel-on-destroy that #178 added, which was the wrong instinct: a reader who clicked download asked for the file, and some of these links sit inside a menu that closes on the click. Cancelling then would have thrown the download away exactly when it was slow enough to matter. I checked that case before removing it -- a five second render from inside the PNG menu still saves, because the menu keeps its content. Verified on beta: the tab's PPTX still reads "Preparing…" then "30%" and saves 54,224 bytes; a hung export can be stopped by clicking it again. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
#181) Curators, re-testing #137: "no jumping in the search and the analysis table and none in the upper section of event hierarchy, but pathways near the bottom of the hierarchy (e.g. subevents) still jump." They were right, and the revealing was not the cause. Rebuilding the tree starts by emptying it -- a workaround for an Angular Material bug where nested children otherwise do not render -- and emptying it destroys every row, so the container collapses and the browser resets its scroll to the top. Clicking a sub-event that was already on screen measured 274px, then 0, then 6: a jump to the top and a scroll back. Expansion state was already carried across that rebuild. The scroll position now is too, restored again on the next frame because the height is not final until the restored branches have rendered and the browser clamps a scrollTop set against a container that is still short. Clicking a row you can already see now moves the tree 0px, from 274. A row you cannot see is still brought into view -- 274 to 130 for one above the fold -- because that is the half worth keeping, and both are tested. I tried removing the workaround instead, since the bug it cites is from 2018 and it carries a "check performance issue" note. It is still needed: without it the tree renders 29 rows instead of 41 and no branch opens. Left in place, with what it costs written down next to it. Verified to fail without the fix: the already-visible case goes red, the reveal case stays green. Fixes #137 Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…e id (#182) 278 links across the news open the pathway browser with nothing in it -- 21 of them in the current release's own announcement, which is the list of what is new in v97. They are written the way the old browser addressed a pathway: a bare dbId in the fragment, `/PathwayBrowser/#1280218`. #172 taught the pattern to read `#R-HSA-…`, and these are the other half of the same legacy. The browser already resolves a dbId given in the path, so only the fragment form was missing. **But it does not leave the reader on one.** A dbId is not stable across releases, so a URL carrying one is a URL not worth keeping or sharing, and an old link should hand over a good one. The dbId is resolved to its stable id and that is what is navigated to: `#1280218` now lands on `/PathwayBrowser/R-HSA-1280218`. If the lookup fails it falls back to the dbId, which loads -- a page on a worse URL beats a link that goes nowhere, and refusing to navigate would turn a working legacy link into a dead one. Four digits at least, so an ordinary page anchor cannot be taken for a dbId; the shortest one in the content is five, and there are no shorter numeric fragments in it. This also closes the last **gap** row in RELEASE-TESTING.md, which asked a person to check that "a newly added pathway, reaction and complex render" every release. The announcement names them, so `e2e/release/new-in-this-release.spec.ts` reads the newest release note in the repo and opens each link exactly as written -- 21 for v97, all of them now -- and asserts each lands on a stable id. That leaves 48 auto, 8 human, 2 missing and no gaps. Still written as dbIds in the content itself, which is a separate question: 13 curated files, and the announcements are authored through the CMS. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
An adversarial pass over #172 and #182. **Following a legacy link trapped the reader.** From the news archive, opening `/PathwayBrowser/#1280218` and pressing Back stayed on the pathway -- six times over. The rewrite is the cause: the app writes state into the URL while the fragment is still there, so `/PathwayBrowser#1280218` and `?tab=details#1280218` both became history entries carrying it, and going back to such an entry rewrites it forward again. Both PRs shipped this; the `#R-HSA-…` links have had it since #172. Two changes. A fragment being consumed is no longer carried forward by the navigations that write state into the URL, so no entry retains it. And a dbId is navigated on immediately rather than after its lookup -- going first means the fragment is consumed in the same turn as a stable-id one, before anything else can record it -- with the URL then corrected to the stable id by a replace, which adds no entry. Measured from /about/news: a dbId link now takes 3 Backs to leave, a stable-id link 4, and a direct load 3. It was never. **I had also written a resolver that already existed.** `dbIdToStId` has been on this service all along, unused, and it is the better one: it asks `/data/query/<dbId>/stId` and gets 13 bytes, where mine fetched the whole object for one field -- 12,682. Mine is gone and the existing one has its first caller. Not fixed, and not mine: a pathway load adds two history entries of its own before any of this, which is why leaving still takes three Backs rather than one. Every navigation in this service pushes, including the ones that are only normalising the URL. Worth a look, but making them replace would also stop Back working between pathways, so it needs deciding rather than just changing. The escape is now a test, because it broke twice without anything noticing. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Opening a pathway cost three presses of Back to get out of, and the first two did something nobody asked for. The app writes its own defaults into the URL, so `?tab=info` and then `?tab=details` each became a history entry: pressing Back stepped through tab changes the reader never made before it would leave the page. The URL still has to carry the tab -- a shared link has to open on the same thing -- so the fix is not to stop writing it. Being *given* a tab now replaces the entry; *choosing* one still adds to it. `details.component` already knew the difference, in `tabCameFromUrl`; there was just no way to tell the URL writer, so `settle()` is that way. From the news archive: one press of Back leaves, where it was three. Choosing the Molecule tab and pressing Back still returns to Details. Found while reviewing #183, where I called this pre-existing and left it. It was pre-existing, and it turned out to be one flag rather than the rework of navigation semantics I assumed -- the reason I gave for not doing it was wrong. Both halves are tested, because keeping the second is what stops the first turning into "Back does nothing in this app". Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…185) * fix(routing): decide "replace or add" from the URL, not from a flag #184 put a `settling` boolean on the service: settle() raised it, the writer effect read it and lowered it. That only works if the writer always runs, once, right after. It does not: - the writer returns early on the content and search routes, before it ever lowers the flag; - a default that already matches the current value changes no signal, so the writer never runs at all. Either way the flag outlives its turn, and the reader's next write -- a real choice -- silently replaces their history instead of adding to it. So the writer now decides from the values. settle() records the params the URL should say once its change has been applied; the writer replaces only if it is about to write exactly those, and clears the record either way. Both sides go through one currentQueryParams(), so they agree by construction rather than by a comment. The record is taken untracked. settle() is called from inside the details defaulting effect, so reading every param made that effect depend on every param: choosing the Molecule tab re-ran the default, which set the tab straight back to details. The URL never changed and the click did nothing -- caught by e2e/back-button.spec.ts, which now passes both ways again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(hierarchy): write down why the scroll restore takes one frame #181 restores the reader's scroll position on frame one; RevealDirective brings a selected row into view on frame two. That order is what makes `block: 'nearest'` compare against where the reader actually was, so a row already on screen is left alone. Reverse it and clicking a visible row scrolls twice again -- the bug #181 fixed. Neither side said so, and both comments read as if their frame count were a local choice. Noted in both, because either one being "simplified" breaks the other. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…ole URL (#186) Both effects that write pathway browser state into the URL stood down with `router.url.includes('content') || router.url.includes('query')`. The query string answers that as readily as the path does. `sample` holds a column name taken verbatim from the reader's own expression file, and analysis.service.ts sets it automatically to the first column. So an expression file whose first column is called "GC content" puts `?sample=GC__content` in the URL -- and from that moment nothing the reader did was written to the URL again. Not selecting a node, not flagging, not changing tab. None of it survived a reload, and none of it was in any link they shared. Now one exported predicate, testing whole segments of the path only. "query" is gone rather than fixed: the search page is `content/query`, already covered by the first test, and of the two words it is the likelier to turn up in somebody's data. Verified by putting the substring test back: the new e2e case fails on it and passes on this, so it is a regression test rather than a description. The three content pages keep their own addresses either way, which is what the guard is actually for and had no coverage at all. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…188) The allowlist was `md|json|yml`. Every one of the 253 authored content files is `.mdx`, because the Tina collections are format "mdx" -- so the regex matched none of them, and the guard rejected every content edit it exists to permit. No cms/* branch has ever merged, which is consistent with that: the first content PR to try it failed on all 17 files. Adds mdx. md and yml stay for content that may be authored that way later, json is what the generated index files are. content-dist/ is deliberately still excluded, and now says so: it is the compiled JSON the browser reads, and stage:content rebuilds it in both tests.yml and deploy.yml, so a content PR neither needs nor should carry it. Checked both directions against the 17 files of the content PR that caught this -- all allowed -- and against code, a workflow file, package.json, a generated content-dist JSON and a `.svg.ts` pretending to be an upload: all still rejected. An upload image still passes. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
The spec's header described the news archive as carrying 278 dbId links, and named one case "as the release announcements write it". Once those links are rewritten to stable ids, both statements are false and the test looks like coverage of something that no longer exists. It is not. Those links are a decade of citations, bookmarks and other people's pages, and none of that can be edited -- which is the whole reason the dbId path stays in the code. Said so, so nobody deletes the case for being obsolete. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Every pathway link in the news archive and the research spotlights was written the way the old browser addressed one: a bare dbId in the fragment, `/PathwayBrowser/#1280218`. #182 made those open the right pathway, so nothing is broken today -- but a dbId is not stable across releases, and these announcements are a decade of citations and bookmarks. A dbId that moves takes an old announcement's link with it, silently. 316 links across 17 files, 229 distinct ids. Every one was resolved against the ContentService first and every one answered; all 229 are pathways (205 Pathway, 14 CellLineagePath, 10 TopLevelPathway), so there is no id here that the fragment handler would open as something it is not. Nothing that failed to resolve would have been rewritten -- nothing did. Only the fragment of a PathwayBrowser link is touched. A number anywhere else in the prose is left as the curator wrote it, and each link keeps the spelling it had, with the slash or without. 38 changed line pairs, and none of them differs anywhere but the fragment. The dbId path stays in the code and stays tested: links in the wild cannot be edited, only ours. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Two findings from reviewing #187-#189. **The CMS guard could be walked around by a rename.** `git diff --name-only` prints only a rename's destination, so on a cms/* branch `git mv package.json projects/website-angular/content/sneaky.mdx` showed one allowed path and the deletion of package.json was invisible. --no-renames prints both, and the source is judged on its own; proved both ways on a scratch repo. Pre-existing, not something #188 introduced -- #188 only changed the extension list. **Numbers in two doc comments were wrong.** They said 770 links spelled `#/R-HSA-…` and 86 spelled `#R-HSA-…`. Measured against the content as it stood before the rewrite: 638 and 70. Repo-wide it is 1276 and 140, unique links 431 and 66; 770 and 86 reproduce at no scope. The dbId figure was right but mislabelled -- 278 is the no-slash spelling alone, and there were 285 in total. Corrected per spelling, and #189's comment now says what the old numbers were, so nobody re-derives them from the same bad source. The `24 in the current release's announcement` claim checks out exactly. Also notes in FRAGMENT_PATTERN that our own content no longer sends dbIds here, so the traffic that still arrives is external and the pattern has to stay. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
The CLI was already installed; the repo had never been initialised, so
none of the workflow was available. `specify init --here --integration
claude` adds the nine speckit skills and the templates, scripts and
workflow under .specify.
The constitution is filled in rather than left as placeholders, because
a placeholder constitution steers every later spec into nothing. Its
five principles are the ones this project has actually paid for:
I. verify the instrument -- a "186 broken links" crawl that was our
own rate limiting, a readiness probe that would have skipped its
test forever, a scroll measured on the wrong container
II. measure in the running app -- a legacy fragment mapping that
passed a typecheck and 19 unit tests and failed in a browser
twice, for two different reasons
III. prove a test fails first -- otherwise it describes the fix rather
than guarding it
IV. never leave a reader on a dbId
V. comments carry the failure, and any number in one has to be
measurable -- two of ours claimed counts that reproduce at no
scope
Plus the gates, the two ratcheted baselines, and the workflow facts that
have each cost a retraction this week: verify a commit by its merge
commit rather than its branch, `gh pr update-branch` because protection
is strict, and never run concurrent git operations in this tree.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* chore: adopt Spec Kit, with a constitution that says what it cost
The CLI was already installed; the repo had never been initialised, so
none of the workflow was available. `specify init --here --integration
claude` adds the nine speckit skills and the templates, scripts and
workflow under .specify.
The constitution is filled in rather than left as placeholders, because
a placeholder constitution steers every later spec into nothing. Its
five principles are the ones this project has actually paid for:
I. verify the instrument -- a "186 broken links" crawl that was our
own rate limiting, a readiness probe that would have skipped its
test forever, a scroll measured on the wrong container
II. measure in the running app -- a legacy fragment mapping that
passed a typecheck and 19 unit tests and failed in a browser
twice, for two different reasons
III. prove a test fails first -- otherwise it describes the fix rather
than guarding it
IV. never leave a reader on a dbId
V. comments carry the failure, and any number in one has to be
measurable -- two of ours claimed counts that reproduce at no
scope
Plus the gates, the two ratcheted baselines, and the workflow facts that
have each cost a retraction this week: verify a commit by its merge
commit rather than its branch, `gh pr update-branch` because protection
is strict, and never run concurrent git operations in this tree.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* spec(interactors): confidence filtering and download
The first spec written with Spec Kit, for the two rows of the curator
release checklist that cannot be signed off because the behaviour does
not exist here: RELEASE-TESTING.md:117 (raising the confidence threshold
reduces the interactors shown) and :118 (interactor download).
Three prioritised stories: filter the interactors on the diagram (P1,
the blocked row), keep and share the filtered view (P2), download the
filtered set (P3). Each is independently testable and independently
useful.
Two numbers in it are measured rather than estimated, and both are
re-measurable:
- scores run 0-1, sampled at 0.482-0.98 across the 33 interactions of
Q13158, from /interactors/static/molecule/{acc}/details
- the old browser's default threshold is 0.45, and thresholds are held
*per resource* -- DEFAULT_SCORE and Map<String, Double>
interactorsThreshold in reactome/pwp-diagram,
data/InteractorsContent.java
The second of those was the spec's only open question. Reading the GWT
source answered it and also corrected an assumption the first draft had
made: it assumed one global threshold, reasoning from there being a
single control beneath the diagram. It is per resource, so switching
resource applies that resource's own threshold -- a score from IntAct
does not mean what a score from another resource means. FR-004a and the
resource entity came out of that.
The default applies without being written into the address: a curator's
address should name the threshold they chose, not one nobody chose. That
is the same rule #185 established for the details tab.
Format of the download is left to /speckit-plan. It is constrained by
how the site's other tabular exports behave, and is not something a
curator would state as a requirement.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
47 links in this site's own content opened an empty pathway browser, and
always had. The fragment pattern required a pathway id, so a fragment
carrying only settings matched nothing and was read as a page anchor:
- `#TOOL=AT` (45 of them) is the "Analysis Tools" link in every
release announcement we have ever published, the current one
included. It now becomes `?analysisTab=qualitative`, which is the
tool's first tab and its fallback -- the gene-list upload the old
link led to.
- `#DIAGRAM=<dbId>&PATH=<dbId>` (2, in the v64 announcement) names the
pathway to open, so DIAGRAM is the route rather than a setting. It
is only ever a dbId, which the existing resolution then swaps for
the stable id, so the reader is not left holding one.
The id is now optional, but only when what follows is one of the old
browser's own keys, so `#introduction` still falls through untouched.
Both halves being optional means the pattern would otherwise match an
empty string -- and an absent fragment reads as '', which would have
made carriesLegacyPathway() answer true for every URL in the app. A
lookahead stops that, and a test holds it.
TOOL is rewritten into the URL rather than carried as a legacy token or
set on the signal, because neither survives: the writer replaces the
whole query string when any state settles, dropping `?TOOL=AT` before
the reader saw it, and the reader resets every param the URL does not
mention, so a signal write is undone in the same turn.
Also corrects `otherTransform`'s type. It was `(value: T) => T` and
receives a raw string off the query string; that was true of no caller
and went unnoticed only because `tab` is a `string | null`, so the lie
typechecked. A param with a narrower type could not have used it.
The spec asserted the old behaviour for TOOL=AT, reasoning that matching
it would make "a route to nowhere and a junk query parameter". True
while nothing mapped TOOL; analysisTab does now, so that case moved
rather than being deleted.
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
`says it is working during the wait` polled the DOM every 150ms against the real content service and asserted it had caught a transient label. It failed that way on main on 2026-09-09 -- run 34397628182, three attempts, each reporting `states seen:` with nothing after it -- and it has now blocked #191, which does not touch downloads. Note what that message means: the sampler collected nothing at all. The file still arrived and still contained `<sbml`, so the export worked. I cannot say from the log whether the export outran the sampling window or the busy state never appeared, and this change does not settle that question -- it removes the test's dependence on timing so the question stops mattering. The export is now served by the test and held for 2.5s, so the busy state is certain to exist while it is asserted. The assertion is on `data-download-state`, the label the reader actually sees, rather than a class that merely implies it, and it retries instead of sampling. It also now checks the label goes away once the file has arrived, which the sampling version could not express. Ran three times locally: 6 passed each time, and the case is now 7-8s rather than however long the real export happens to take. The content-service gate stays, because the detail page still needs it even though the export no longer does. `e2e/flagging-trivial.spec.ts:98` and `:102` failed in that same main run and are not touched here. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
`fade with zoom when nothing is flagged` zoomed once, waited 400ms and read once. It failed on main on 2026-09-09 -- run 34397628182 -- with `visible close up` receiving [0] where it expected [1]. The cause was reproduced rather than inferred. Immediately after zoomTo(1.5) the diagram reads zoom 1.5 and opacity [1]; forcing a fit() straight afterwards drops it to zoom 0.113 and opacity [0], which is exactly what CI reported. On a slow runner the diagram is still settling when the test zooms, its own fit() lands after, and the opacity read belongs to a zoom level nobody asked for. An opacity only means something alongside the zoom it was measured at, so the zoom is now checked in the same evaluate that reads the opacity, and a reading taken at the wrong zoom is discarded rather than asserted on. The assertion retries, so it converges as soon as the diagram stops re-fitting. Proved both directions by injecting a fit() 600ms into the test, which is what the slow runner does by accident: the old read returns [0] and fails, the new one passes. Only this test needed it. The two flagged cases expect [1] at every zoom, so a stray re-fit cannot falsify them -- they read directly, and now say why. Ran the spec three times: 3 passed each time. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
#195) 247 files under projects/website-angular/content-dist were committed, despite .gitignore:76 ignoring that directory -- the rule never applied because they were already tracked. tests.yml calls content-dist "a gitignored asset input", so the intent was always for it to be generated; the tracked copies were the accident. They stopped being harmless when #187 rewrote the content's links to stable ids without touching content-dist. From then on the repo held two disagreeing copies of every news announcement, and every `npm run build` dirtied 16 files. That is not theoretical: `git add -A` swept them into the commit for #194, lint-staged failed on the result, and the commit was lost. Untracking alone would have broken a cold local `npx playwright test`, because playwright.config.ts boots `npm run start:simple` and that was bare `ng serve` -- no content to serve. So start:simple now runs generate:indices and stage:content first, which is also what README already claims it does ("same as npm start, skipping TinaCMS"): it skipped the content generation too, so the documentation was describing a script that did not exist. start:simple:local had the same defect and gets the same fix. CI needed no change -- the e2e job already runs both steps, and so does the deploy workflow. Verified by deleting content-dist entirely and regenerating it: 253 pages to 256 JSON files, now carrying the stable-id links that the tracked copies still had as dbIds. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
* chore: point the docs at the scripts, and fix the last serve that skipped content Adversarial review of the untracking commit, which found three things it had missed. **README told people to run a command that will now break.** It called `ng serve -c curator` "the quickest way to check a clone works at all". That is a bare ng serve: it compiles no content, so on a fresh clone it serves nothing. `npm run start:curator` already does it properly, and the table now says so. **`start:local` compiled indices but not content.** It ran generate:indices and stopped, so it was already half-broken and untracking would have finished the job. Every start:* and serve:* script now compiles content; audited rather than eyeballed. **A bare `ng build` does not fail on the missing directory -- it succeeds.** The asset entry is a glob over content-dist, so an absent directory copies nothing and the build completes. My first draft of this note claimed it "fails on a missing asset". It does not, and the truth is worse: you get a site whose every content page is empty with nothing in the log to say why. Checked by moving the directory aside and running a production build, which finished normally -- and which is how I found it, having done exactly that to my own checkout. That last point is also worth stating plainly for anyone switching branches: git deletes these files when you move to a branch that no longer tracks them, because they were tracked in the branch you came from. Any start:* or build puts them back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(flagging): do not let an unmeasured diagram look like a faded one Review of the previous commit. The `faint far out` poll returned null when the zoom had not held, and asserted toBeLessThan(1) on it. In JavaScript `null < 1` is true, so a reading that could never be taken would have described an unmeasured diagram as a faded one -- the poll would stop and the test would pass having measured nothing. It does not, as it happens: Playwright's toBeLessThan throws on a null rather than comparing it, so the poll retries. Checked, not assumed. Returning Infinity anyway. The test should not depend on how a matcher treats a value it was never meant to receive, and the intent belongs at the point the value is produced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
The branch was 137 commits behind main and predates the SITE_PROFILES refactor, so its environments.ts was the old shape. A toggle written against that would have had to be written again, which is why the update comes first. Two conflicts, both in viewport, both the same shape: main removed the compare-form scaffold (#174) while this branch added the DeltaSignal panel beside it. Kept both decisions -- compare gone, DeltaSignal staying -- and folded the two dropdown states into one signal. **The flag.** The Perturb button and the panel rendered unconditionally, so DeltaSignal would have appeared on production, beta and the curator site alike. It is now a per-deployment profile field: SiteProfile.deltaSignal?: boolean -> SHOW_DELTASIGNAL -> viewport.showDeltaSignal -> @if around the button and panel Absent means off, so a new deployment opts in rather than inheriting a research feature. Only `development` has it on, because /api is routed by proxy.conf.js -- the dev server's proxy, which does not exist in a built artifact and which nothing in deploy.yml or the Apache config provides. Turning it on anywhere else would offer a button whose every call fails. A feature toggle, not a variant. The standing note in environments.ts says new deployments should compare SITE_VARIANT rather than grow a second boolean; that is about which UI a deployment presents, which is a different question from which features it enables, and the field says so. Verified in a browser rather than by reading the flag, using the __APP_ENV runtime override the config already reads, so one build proves both states: the button is absent under the default profile and present under `development`. e2e/deltasignal-toggle.spec.ts holds it. The first version of that test asserted /^Perturb$/ and found nothing, which made a working flag look broken -- the button renders a mat-icon first and the ligature is text, so its content is "conversion_path Perturb". The comment says so. Two unit tests as well, and the profile one was shown to fail: turning the flag on for beta reports `beta.deltaSignal: expected true to be false`. 210 unit tests, check:types, check:dead at baseline, lint 653, prettier clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
2 participants
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.
For #155 — targeted at
feat/deltasignal-uiso the merge stays yours, @Chryseisliu. The module itself is good work: signals throughout, explicitstatus/errorstate,HttpErrorResponsehandling, a typed model, its own spec, and it usesstableIdrather than dbIds without being told to.Two things it needs, and the first one forced the order of work.
The branch was 137 commits behind main
It predates the
SITE_PROFILESrefactor, so itsenvironments.tsis the old shape — a toggle written against that would have had to be written again. So main is merged in first.Two conflicts, both in viewport, both the same shape: main removed the compare-form scaffold (#174) while this branch added the DeltaSignal panel beside it. Both decisions kept — compare gone, DeltaSignal staying — and the two dropdown states folded into one signal.
The flag
The Perturb button and the panel rendered unconditionally, so DeltaSignal would have appeared on production, beta and the curator site alike. It is now a per-deployment profile field:
Absent means off, so a new deployment opts in rather than inheriting a research feature. Only
developmenthas it on, because/apiis routed byproxy.conf.js— the dev server's proxy, which does not exist in a built artifact and which nothing indeploy.ymlor the Apache config provides. Turning it on elsewhere would offer a button whose every call fails.A feature toggle, not a variant: the standing note in
environments.tssays new deployments should compareSITE_VARIANTrather than grow a second boolean, but that is about which UI a deployment presents, not which features it enables. The new field says so.Verified in a browser, not by reading the flag
e2e/deltasignal-toggle.spec.tsproves both states from one build, using thewindow.__APP_ENVruntime override the config already reads: the button is absent under the default profile and present underdevelopment.Worth admitting: the first version of that test asserted
/^Perturb$/and found nothing, which made a working flag look broken. The button renders a<mat-icon>first and the ligature is text, so its content is"conversion_path Perturb". There is a comment on the locator now.Two unit tests too, and the profile one was shown to fail rather than assumed to work — flipping the flag on for beta reports
beta.deltaSignal: expected true to be false.210 unit tests,
check:types,check:deadat baseline, lint 653, prettier clean.Still open, and not addressed here
/apionly works in dev. The service calls/api/pathways,/api/parse,/api/solveas relative paths with no environment indirection, and every other backend in this app goes through a constant (CONTENT_SERVICE,RENDER_SERVICE). Nothing configures/apifor a deployed environment, so on a built site those three calls hit the origin and getindex.htmlback. The flag makes that safe — the feature is simply not offered where it cannot work — but it is what stands between this and shipping anywhere but a dev machine.Two smaller notes:
/apiis a broad prefix for one feature where the house convention is named ones (/reactome,/RenderService,/GSAServer); andDELTASIGNAL_BACKEND || 'http://localhost:8080'defaults to the Tomcat running ContentService, which has no DeltaSignal endpoints, so an unconfigured dev gets 404s from a real server rather than a clear "not configured".🤖 Generated with Claude Code