Add SurfAnnotate: cortical surface ROI delineation - #1
Merged
Conversation
A browser-native tool for visualising cortical surfaces and drawing ROIs on them. Two workflows: closed-boundary regions, and point-and-click vertex selection. Everything runs locally; no data is uploaded. Nothing in packages/components handles meshes — ViewerController is volume-only and detectFiles classifies .gii/.mz3/.pial as unknown — so the surface layer is new code here rather than an extension of the shared library. Algorithms (framework-free, unit-tested under plain node --test): - CSR 1-ring adjacency, A* shortest paths along mesh edges - flood fill with Workbench's seed-outside-and-invert strategy, plus a seeded override, an escape guard, and interior-component reporting - uniform-grid nearest-vertex lookup, 163x faster than NiiVue's linear scan - FreeSurfer .label, GIfTI .label.gii / .shape.gii, and JSON/CSV point writers Boundary paths deliberately follow mesh edges rather than a smoother geodesic: consecutive vertices must be adjacent or the flood fill leaks through the barrier. FreeSurfer and Connectome Workbench make the same trade-off, and Workbench validates adjacency before filling for exactly this reason. Measured on a 163,842-vertex lh.pial: adjacency 50 ms once, A* segments 0.007-0.23 ms, a 20-click ROI traced in 0.5 ms, fill 24 ms. Pinned to @niivue/niivue 0.69.0 — npm latest, mesh code byte-identical to the 0.68.x used elsewhere in this monorepo, and unlike 1.0.0-rc.x it still has the nearest-vertex helper. All NiiVue access is isolated in src/niivue/meshAdapter.js so that migration stays a single-file change. Registered as an experimental vite-webgpu / imaging-workspace app. 55 unit tests and 15 Playwright tests (headless WebGL2 via SwiftShader). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Rename surfmark -> surfannotate throughout: registry id and public path, package, workspace directory, window globals, ROI layer name, and docs. Export files are now named <hemisphere>.<roi> rather than carrying the source filename, so an ROI drawn on lh.sphere.reg.surf.gii saves as lh.V1.label instead of lh.sphere.reg.surf.V1.label. Vertex indexing is shared across a subject's surfaces, so naming a label after the particular surface it was traced on misrepresents where it applies. The hemisphere is taken from GIfTI's AnatomicalStructurePrimary when present, otherwise from the filename (FreeSurfer lh./rh., BIDS hemi-L/R, or HCP .L./.R.); an unrecognised surface yields no prefix rather than a guessed one. Remove the .shape.gii export button — .label.gii covers the same ground and carries the ROI name and colour. The writer stays in io/gifti.js, still tested, as a general format function. 61 unit tests, 16 Playwright tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Delineating on an unfolded flat patch, an area often runs right up to the cut, so its border is partly the drawn line and partly the edge of the patch. Clicking along that edge to close the loop takes many clicks and puts the border wherever the clicks land rather than on the cut itself. "Close on surface edge" draws only the part of the border that crosses the patch: both ends are extended to the nearest edge vertex by a multi-source Dijkstra distance-to-cut field, and the edge closes the region. Two points are enough, where a loop needs three. Nothing is traced along the edge because nothing needs to be. Flood fill walks the 1-ring vertex graph and no edge of that graph crosses the cut, so the cut is already an impassable barrier; a border reaching it at both ends separates the patch on its own. That separation is verified by counting connected components rather than assumed — a line joining two *different* cuts turns an annulus into a disk without dividing it, and is refused. The smaller of the two sides is filled, since an area bounded by the cut is the smaller piece essentially every time; "Other side" swaps in one click. No 40%-of-surface refusal applies here, unlike the loop strategies: that guard catches fills that leaked through a gap, and a leak is impossible once the barrier is known to separate the graph. Also fix a layout bug this surfaced. #controls sets flex-direction: column while the shared .nd-imaging-controls class on the same element sets flex-wrap: wrap; together they wrap anything taller than the panel into a second column to the right of a 320px panel — invisible, unreachable, and silent, because wrapping absorbs the overflow so overflow-y never scrolls. Growing the tool section made every annotation button vanish as soon as a cut surface was loaded. Pinned to nowrap, with an e2e test asserting one column, and the [hidden] fix generalised from the drop hint to all elements. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Backspace and Delete undo the last border point, on a document-level listener that calls preventDefault. It fired regardless of focus, so it also swallowed every keystroke aimed at a text box: the ROI name and the overlay colour-range fields could only be changed by selecting all and overtyping. Guard on the event target. Sliders, checkboxes and buttons deliberately do not count as text entry — focus stays on the ROI opacity slider after you drag it, and undo should still work there. Escape now only reports "Cancelled" when a seed click was actually pending, rather than on every press. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The app held exactly one surface and one overlay, which caused three problems at once: a dropped surface rendered but left the panel looking empty, because a native file input shows nothing for a drag-and-drop and there was nowhere else for it to appear; a second surface silently replaced the first; and there was no way to keep more than one overlay. Surfaces and overlays are now lists. The lists, not the file inputs, are the record of what is loaded, so a drop and a pick look the same. One surface is shown at a time — not a preference, but because the depth picker returns a position rather than an identity, so a click over two overlapping meshes could not be attributed to either. Overlays belong to the surface they were loaded onto and each keeps its own visibility, colour map and range; hiding one rides on opacity, since NiiVue mesh layers have no visibility flag, so the chosen opacity is stored separately rather than being reset to opaque on re-show. ROI sessions are keyed by topology rather than by file. Surfaces sharing a vertex indexing — one subject's white, pial, inflated and sphere — share border points, so they can be placed on the inflated surface and seen on the folded one. RoiSession.rebind moves the session and discards the traced chain and the fill: the shortest path between two vertices genuinely runs differently over different geometry, so those must be rebuilt rather than carried across. Unrelated meshes keep independent ROIs, preserved while switching back and forth. Drop routing no longer depends on drop order, which could not express "add another surface". io/classify.js identifies each file from its magic number — FreeSurfer's 0xFFFFFE against curvature's 0xFFFFFF, GIfTI's intent codes, MZ3's attribute bitfield — falling back to extensions and FreeSurfer naming, and to the old positional rule only when nothing is conclusive. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Visual areas are delineated in order, and each one's border is mostly the border of the one before it: V2 shares the whole V1/V2 boundary with V1, V3 shares its outer boundary with V2. Re-clicking that shared border for every area is tedious and also wrong — the two areas end up with slightly different boundaries and a sliver of unassigned cortex between them. A filled region can now be saved to a list of completed ROIs, and ticked as an edge. That reuses the machinery already built for flat patches rather than adding a parallel one: a cut in a surface works as a barrier because the 1-ring graph has no edges crossing it, so cutting a finished ROI out of the graph gives its rim exactly the same standing. V1 becomes a hole, the rim of that hole is reported as an edge, and closeOnEdge anchors to it with nothing else changed. The pathfinder will not route through it and no fill can cross it, both of which fall out of the vertices being isolated — fill.js and isIsolated already skip vertices with no neighbours. Excluded vertices keep their indices, since every label, click and export refers to a vertex by index; only their edges go. This works on closed surfaces too, where there was no edge to begin with: once V1 is cut out, lh.pial is a sphere with a hole in it, and every edge closure applies. Completed ROIs are per topology like the sessions, painted from a palette, and solid rather than translucent while acting as an edge — they are part of the surface's structure at that point rather than an annotation over it. Selecting one makes the export buttons write it. Both exports now go through one function: the .label path read the session directly and would have written an empty file after a save cleared it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Saving was one-way: a border could only be corrected by removing the ROI and re-clicking it from scratch. The clicks were already stored in the record, so the pencil now puts them back on the canvas. Reopening is un-saving. The ROI leaves the list first, which matters because an ROI cannot be an edge for its own border — while it is cut out of the graph its own clicks are isolated and no path can reach them, so retracing would fail with an unexplained gap. The border is recomputed from the clicks rather than restored from the saved chain, for the same reason the clicks are authoritative everywhere else: the surface may have changed since, another ROI may have become an edge, and the border should respect it. The record now also carries how it was closed, so a loop is retraced as a loop and an edge closure as an edge closure, along with the region index and the include-boundary setting. A loop on a surface with an edge normally has to ask which side was meant. The saved region already answers, so the refill is seeded from a vertex inside it rather than re-running the geometric inside/outside guess. On the flat-patch fixture the two always agree wherever a region can be saved at all, so the e2e test covers the round trip rather than the difference; the seed is kept because recorded fact beats a heuristic. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reopening an ROI failed whenever a neighbour was also ticked as an edge: the border points came back but could not be retraced, and the region came back empty. The cause is that an ROI's border points routinely lie *inside* the ROI drawn next to it. The fill excludes the border row by default, so the row V1 was clicked along is unclaimed until V2 is drawn against V1's rim — at which point V2 owns it. Cutting V2 out of the graph isolates exactly the vertices V1's border runs through, so A* cannot reach them. Reopening now also unticks any edge ROI whose region covers part of the border being restored, and names it in the status line rather than doing it silently. Neighbours that do not touch the border keep their tick. Repro before the fix, on the flat patch: V1 from row 2, V2 from row 6, both ticked as edges; reopening V1 left both its clicks isolated and reported a gap. After: the border retraces and refills to its original 82 vertices. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Independent masks are the wrong model for adjacent areas. V1 and V2 share a boundary, so moving it changes both — but only the edited one changed, leaving the other claiming vertices that were no longer its own, or leaving a strip belonging to nobody. Editing V1 after completing V2 should hand V2 the vertices V1 gives up. An area is no longer a mask. It is a definition — its border points, how they were closed, which side was taken, and a vertex deep inside the region — and the masks are derived by resolving the whole list in order, each area cut out of the surface before the next is resolved. Disjointness holds by construction rather than by checking, and editing re-derives everything below: pull V1's border back and V2 grows into the space, because V2 was always defined as "my line, and whatever lies between it and the area above me". This is the rule the app already followed for clicks, one level up. Consequences: The per-area "edge" tick is gone. Every area above the one being drawn constrains it, which is what a parcellation means; the tick was expressing the same thing by hand and could be set inconsistently. Order is now meaningful and adjustable. An area can be squeezed out by one promoted above it, and moving it back up takes those vertices straight back. An area that no longer resolves is struck through, reports why, and claims nothing. Reopening keeps the area's position, which removes the neighbour-release special case added in the previous commit: an area is only ever blocked by one above it, and editing in place means the one below is simply re-derived. Filling by component size alone flips as neighbours grow and shrink, so each area stores an anchor — the vertex furthest from its border by hop count, the last one a neighbour would take — and the refill prefers the component containing it. Also fix the palette index, which came from the list length: re-saving an edited area recoloured it, and could give it its neighbour's colour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two things reported together. The area name lived in the Export panel, below the Save button that uses it, so naming an area meant scrolling past the button that saves it. Moved to the Areas panel, directly above Save. Export keeps the filename preview. A .label exported from here could not be dropped back in, while the .label.gii could. NiiVue's readLayer has no case for a FreeSurfer .label: the extension falls through to its curvature reader, which cannot parse ASCII and returns a layer with zero values. A .label is also sparse — a list of the vertices in the region — where every format NiiVue does read is one value per vertex. So it is expanded here instead. labelToValues scatters the vertex list into a dense array, using the fifth column when it carries a statistic and a binary mask when it does not, and refuses a label whose indices exceed the surface rather than truncating it. classify.js now routes .label to the overlay path, by extension and by its "#!ascii label" first line. The display window is set just under the smallest marked value rather than over the full range: a mask is mostly zeros, so a full-range window renders the whole surface in the colour map's low end and the region does not stand out. A hand-built layer must set nFrame4D: 1. NVMeshLayerDefaults leaves it 0 and NiiVue computes the frame as min(max(frame4D, 0), nFrame4D - 1) = -1, so it reads values[j - vertexCount], gets undefined, and every colour lookup lands on NaN — which paints the entire surface black. Found by looking at the render; the data assertions all passed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Matches the rest of the Neurodesk webapps: a short explanation of what the tool reads, writes and does, then a "Start annotating" button, rather than dropping a visitor straight into an empty viewer. Built the way calmar builds its own — a fixed-position section over the app rather than a second document — so there is still one bundle, one entry and one path in the composite site. The app is behind it the whole time, so the canvas is already sized when the overlay goes away. The shared shell always draws a one-glyph badge beside the title and has no option to omit it, so it is hidden in this app's stylesheet rather than changed in the component and every app with it. Two things found while wiring the e2e suite through the new button: Loads are now serialised. A file input fires `change` when the files are set, not when the async handler finishes, so picking two files in quick succession — or picking one while a drop was still parsing — started two loadSurface calls at once, which interleaved on state.surfaces and on the active-surface mirrors and could leave the app showing one surface while the tools pointed at another. Three e2e tests used window.__surfannotate immediately after setInputFiles, which resolves on the change event rather than on the load. That raced into a "session is null" failure in one test per full run, moving between tests. The suite now passes four consecutive times. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An SVG in public/, referenced with %BASE_URL% the way dicompare does, so it survives both the standalone build and the composite-site assembly whatever the base path is set to. The icon is the app's own picture of itself: the grey of an unlabelled mesh, the gold of a traced border, the red of a filled region. Two shapes and a line, because at 16px only the silhouette and the colour bands survive. Checked by rendering it at 16, 24, 32 and 96. The e2e test fetches it through the served path rather than just asserting the tag: the failure mode worth catching is %BASE_URL% going out unsubstituted, which 404s with no other symptom. Also fixes three .label tests that were never running. An earlier append landed at the repo root instead of the app directory, leaving a stray test/io.test.js with no imports — which broke the root test glob — while the app's own suite imported labelToValues without ever using it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Replaces the drawn placeholder with the mesh-and-pencil artwork. The file arrived as a PNG named favicon.svg and declared image/svg+xml, which would not have rendered at all. It was also 711x508 — a browser stretches a non-square icon rather than padding it — and 233 kB. So it is re-rendered: fitted centred into square tiles at 32, 48, 180 and 256, which is 1.9 kB for the one a tab actually uses. The artwork touches all four edges, so there is no transparent margin to trim and squaring has to letterbox; the alternative would be cropping the pencil. The test now walks every declared icon rather than one, checks each is served as a PNG, and measures it: square, and the size its tag claims. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The supplied SVG is now the source the PNGs are generated from, rather than the flaticon raster. Rendering each size from vector instead of downscaling a 711x508 bitmap is visibly cleaner at 32 and 48, and roughly halves the files: 256 goes from 53 kB to 47 kB and 32 from 1.9 kB to 1.8 kB, with the artwork filling more of the tile because the square is now fitted to the painted bounds rather than to the bitmap's frame. The SVG itself is not shipped. Measured, it is 202 kB raw and 87 kB gzipped — 46 kB after rounding Inkscape's seven decimal places to two, which is still a hundredth of a pixel at 256. Against 1.8 kB for the icon a tab actually draws, that is a poor trade for no visible gain, and Safari would need the PNG fallback anyway. It is kept in icon/ as the master, with icon/render.mjs to regenerate from it. Two things that had to be got right. The file arrived as an Inkscape A4 page, so the artwork occupied 40% of a tall portrait canvas and would have rendered tiny and letterboxed; the viewBox is retargeted to a square around the drawing. And those bounds come from rasterising and finding the painted alpha box rather than from getBBox(), which ignores stroke width and would have clipped the brain outline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
registry/apps.yml and the README both declared MIT, but the app carried no
LICENSE file and LICENSES.md did not list it — the NOASSERTION situation
that file exists to warn about, and one that would matter on the way
upstream. Every other MIT app here ships the text.
Also records the notices for what the bundle actually contains. A permissive
dependency does not dictate the licence of the work using it, so MIT here
against NiiVue's BSD-2-Clause is not a conflict. What BSD-2-Clause does
require is that its copyright notice and disclaimer accompany a binary
redistribution, and minification strips them: the built bundle carries
NiiVue's code with no notice left in it. THIRD-PARTY.md reproduces them.
Copyright line follows the house style used by qsmbly and seedseg
("<app> Contributors") rather than naming a person; change it if you would
rather it read otherwise.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The note said "original artwork", which claims more than is accurate: it was drawn in Inkscape from source images generated with ChatGPT. The conclusion is unchanged — no third-party licence attaches, because OpenAI assigns whatever rights it may hold in output to the user and asks for no attribution. That is the difference from a stock-library asset, whose terms would survive whoever assembled the file. Worth writing down all the same, since provenance is what the file is for, and because purely machine-generated material may not attract copyright everywhere: the protectable part is the drawing work, not the generated inputs. That bears on what could be enforced, not on what anyone may do with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Same shape as calmar's: a Cite link in the header opening a list of what should be cited, with the NiiVue entry worded exactly as calmar words it so the two agree. There is one entry, because there is one thing to cite — NiiVue does the rendering and the mesh parsing, and nothing else third-party is involved in producing a label. Sections are ready for more. Two small things. The button is a native <dialog> opened with showModal(), which puts it in the top layer, so the same dialog serves the start page and the app without the start page's fixed positioning getting in the way, and Escape closes it for free. And the shared shell builds its navigation with only the catalog link and takes no list of extra items, so the app's button is appended after mounting rather than by changing the component and every app with it — which also means it needs `font: inherit`, since the shell's link rule sets colour but not type and a bare <button> would otherwise sit noticeably smaller beside the anchor. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five agents exercised the app from different angles. Fixed here, each with a regression test: BLOCKERS The e2e suite depended on lh.realflat.surf.gii, which nothing in the repo produced — I generated it in a scratch directory. A maintainer checking out the PR got ENOENT from five tests. Its generator is now committed alongside make-flat-patch.mjs, and test:e2e fetches and builds every fixture it needs, so a clean checkout works. buildVertexIndex sized its grid by volume, so a strictly planar mesh — which is what mris_flatten writes — collapsed the cell size to nothing and exploded the grid across the two remaining axes. A 150k-vertex flat patch took 7.8 s (against 6 ms for the same size folded) or failed outright with an allocation error that loadSurface reported as a corrupt file. That is this app's headline use case. Sizing from the axes that actually have extent, plus a cap on total cells, takes it to 27 ms. SEVERE Saving an area selected it, and the export buttons prefer the selection while the filename comes from the name box — so the next export wrote the saved area's vertices under the new area's name, with a status line confirming the wrong count. Saving no longer selects. Reopening lifted an area out of the list into the session, and nothing put it back: switching surface, clearing, or reopening another lost it for good. restoreEdited() now returns it to its own position on every exit. Removing the last surface left every control enabled while the state behind them was null — eight uncaught TypeErrors. repaint() cannot fix this because it returns early without a session, so teardown resets the controls itself. FORMAT CORRECTNESS GZipBase64Binary wrote a gzip container. GIfTI 1.0 s5.0 says ZLIB and gifti_clib uses compress2(), so nibabel, wb_command and mris_convert all refuse the file; NiiVue's decompressor is gzip-tolerant and so agreed with the mistake, as did our own test, which gunzipped. Now a zlib stream, asserted by magic byte. An ROI name containing "]]>" closed the CDATA section early and produced XML no parser would read. hemispherePrefix took the first bare L or R it found, so MSM-L.R.midthickness exported a RIGHT hemisphere as lh.<roi>. Ambiguous names now yield no prefix. USABILITY onCanvasHover ran a full depth pick — two scene renders and a synchronous readPixels — on every pointermove, for a text readout. On a software renderer, which is a realistic Neurodesk deployment and not just CI, that is ~500 ms per mouse move and the app appears frozen. Throttled to one per frame. setMode left awaitingSeed set, so a pending "click inside the region" ate the first landmark click. recomputeParcellation now announces itself past a few areas, where it blocks long enough to look hung. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The header says `vox2ras=TkReg` and FreeSurfer's labelGetSurfaceRasCoords takes that verbatim, but the coordinates were scanner RAS: NiiVue adds the volume centre to every vertex on load — `cras` from a FreeSurfer footer, `VolGeomC_R/A/S` from GIfTI — so meshes line up with volumes. Measured on this repo's own lh.pial, vertex 0 was written 1.9991 mm off in x and z. Nothing rejected the file, which is what made it dangerous. freeview, mris_anatomical_stats and mris_label2annot all key on the vertex index and were unaffected; mri_label2vol and mri_label2label --regmethod coords read columns 1-3 and were silently wrong. io/geometryOffset.js recomputes the translation from the same bytes NiiVue parsed, and the writers subtract it. It mirrors NiiVue's quirks on purpose: `cras` is applied even when the footer says `valid = 0` (which is exactly the case in the shipped fixture), and GIfTI values are read only from a CDATA value. A correction that does not match what was applied would be worse than none. points.json made the same claim in its coordinateSpace field and gets the same correction. Not fixed, and now recorded in AGENT.md: the coordinates still come from whichever surface was drawn on, so an inflated or flat surface writes inflated or flattened positions under a tkreg header. freeview substitutes the white surface; we could too, since same-topology surfaces already load together. The README says plainly which numbers to trust meanwhile. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The coordinates in a .label are only meaningful if they came from a surface sitting in the subject's anatomy, and nothing in the file records which surface that was. freeview substitutes the white surface when the displayed one is inflated; this app does not, so it has to say so before writing. The export panel now names the surface the coordinates will come from, and turns into a warning when that surface is inflated, spherical or flat — pointing out that the vertex indices are still correct, since that is all freeview and mris_anatomical_stats read. When another loaded surface shares the vertex indexing and is anatomical, it names that one: same indexing means the same ROI, so the fix is one click rather than a reload. Detection is `naming.surfaceKind` on the filename plus a planarity check on the geometry, so a flat patch is caught whatever it is called. An unrecognised name counts as trustworthy on purpose — an oddly named anatomical surface is likelier than a flattened one, and warning about every unconventional filename would train the warning away. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fill.js measured both its safety guards against graph.V. excludeVertices keeps a completed area's vertices and their indices — every label and click depends on them — and only strips their edges, so graph.V stops being the size of the surface a flood can reach the moment any area is saved. Measured against it, the ">half the surface, so take the other side" rule fires spuriously. Reproduced on a 21x21 grid with an earlier area holding rows 8-20: a small 4x4 loop drawn in what was left filled 152 vertices — the exterior, including the far corner — and reported error: null. The same loop on the uncut graph fills the correct 4. The 40% escape guard fails the same way in reverse: a fill that leaked through a gap and swallowed 93% of the reachable surface passed, being only 36% of the nominal count. That is the end state a parcellation app is built to reach. Both now measure the walkable count, and the swap compares the two sides directly, which needs no denominator at all. Also from the review: activateSurface assigned entry.graph over what bindSession had just set to the cut graph, so every activation defeated bindSession's short-circuit and performed a second full excludeVertices plus pathfinder rebuild. restoreEdited put the area back on the list but left its clicks in the session, so a Save after switching surfaces appended the same area again with a new id and colour; the duplicate then resolved as unresolvable because the original already owned the territory. removeSurface also called it for surfaces that were not the active one. Dropped applyExclusion, dead since the parcellation rewrite, and cut window.__surfannotateUi from 22 members to the 7 the e2e suite drives — a shipping app should not export its internals wholesale. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The coordinate warning told everyone to load lh.white or lh.pial and switch to it. On an inflated or spherical surface that is right — the vertex indexing is the native surface's, so the areas come across. On a flat patch it is wrong twice over: a patch is a cut of the surface with its own numbering and fewer vertices (152,893 against 163,842 here), so switching shows none of the areas drawn on it, and the claim that "the vertex indices are still correct" is not true of the native surface either. The two cases now read differently, and the flat one says plainly that a flat surface has a different number of vertices from the native surface it was cut from, so its indices and its areas belong to the patch alone. Switching to a surface with a different vertex indexing also now says so: the areas are hidden rather than lost, and reappear on switching back, but nothing said that and it looked exactly like the work had been discarded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The review found a set of exports, options and fields that only tests or
nothing at all referenced. Removing anything with no production caller:
writeGiftiShape, maskToFloatArray the .shape.gii button went long ago
writePointsCsv, readPointsJson, a points import path never built
checkMeshIdentity
haloMask, hatchMask({cross}) from the abandoned halo experiment
patchVertexColors a VBO fast path with no caller
writeGiftiLabel({gzip}) GZipBase64Binary, never requested
state.layerIndex / entry.layerIndex written in six places, read in none
ResolvedArea.chain copied on every recompute, never read
__surfannotateFill in the e2e suite never assigned, so always undefined
three unreferenced ids in index.html, one class with no rule
AGENT.md's carve-out for writeGiftiShape goes with it: it was kept as "a
general format writer", which is the argument that keeps every unused
export alive. It is one revert away in the history.
Two corrections to the review while doing this. It reported that nothing
passes writeFreeSurferLabel's `stat` option — a test does, and the fifth
column is a real part of the format, so that one stays. And removing the
gzip path let a bug through: base64() takes bytes, so handing it an
Int32Array made Buffer.from() read it as an array of numbers and truncate
each to one byte. The round-trip test caught it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The parcellation rewrite renamed the concept but not the identifiers, so
main.js carried 127 `roi*` tokens against 96 `area*` — both in one
expression in places — and `<h2>Areas</h2>` contained `id="roiName"`,
`id="saveRoi"` and `id="roiList"`. The ids were always right; the prose and
the locals had drifted.
Mechanical throughout: identifiers take the code form (`roi`, `rois`),
comments and user-facing strings take the written form ("ROI", "ROIs").
AreaDefinition, ResolvedArea, resolveArea and AREA_ERRORS follow.
Two things deliberately left alone, both of which a blind sweep would have
broken: `.area` is a FreeSurfer per-vertex file extension in classify.js and
in the overlay error message, and "surface area" in meshAdapter.js is
ordinary English.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughSurfAnnotate adds a browser-local cortical surface viewer with overlay handling, topology-aware ROI editing, parcellation, landmark selection, annotation exports, documentation, configuration, and automated tests. ChangesSurfAnnotate application
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant SurfAnnotate
participant NiiVue
participant ROIEngine
participant ExportIO
User->>SurfAnnotate: Load surface and overlay files
SurfAnnotate->>NiiVue: Create meshes and display layers
User->>SurfAnnotate: Draw ROI or select landmarks
SurfAnnotate->>ROIEngine: Trace paths and resolve regions
ROIEngine-->>SurfAnnotate: Return masks and ROI state
SurfAnnotate->>ExportIO: Serialize selected annotation
ExportIO-->>User: Download label or points file
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
ci.shared_runtime was false, which meant the app's 56 browser tests never ran in automation — only `test` and `build` did. The flag reads as "does this app use the shared runtime store", but it does not track that: seedseg provides runtime assets and is false, while browserqc, vesselboost and spinalcordtoolbox provide none and are true. What it actually drives is whether ci.yml's shared-runtime-e2e job runs the app's test:e2e. It is also what scripts/new-app.mjs emits for a new app. Five of the six apps with a test:e2e script set it; browserqc is the closest analogue — vite-webgpu, imaging-workspace, no runtime assets — and sets it. Verified: app-plan now puts surfannotate in the shared-runtime-e2e matrix, and test:e2e passes from a clean fixture directory, fetching lh.pial and lh.curv and generating both flat patches before running. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (15)
apps/surfannotate/scripts/fetch-fixtures.mjs (1)
42-55: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a timeout to each fetch attempt.
fetchWithRetryhas no timeout on thefetch(url)call. A stalled connection (for example, a misconfigured proxy) can hang indefinitely instead of failing and retrying. AddAbortSignal.timeout(...)so a stalled attempt fails fast and the retry loop proceeds.♻️ Proposed fix to bound each fetch attempt
try { - const response = await fetch(url); + const response = await fetch(url, { signal: AbortSignal.timeout(30_000) }); if (!response.ok) throw new Error(`HTTP ${response.status}`); return Buffer.from(await response.arrayBuffer());🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/surfannotate/scripts/fetch-fixtures.mjs` around lines 42 - 55, Update fetchWithRetry so each fetch attempt passes an AbortSignal.timeout(...) option to fetch, using the script’s existing timeout configuration if available or an appropriate bounded duration. Ensure timeout failures are caught by the existing catch block so they trigger the current retry and final-error behavior.apps/surfannotate/e2e/smoke.spec.js (2)
1266-1268: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the empty
test.use({}).The call sets no fixture overrides, so it has no effect. The comment above it suggests an intent that the code does not implement; both tests call
page.reload()to get the initial state instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/surfannotate/e2e/smoke.spec.js` around lines 1266 - 1268, Remove the empty test.use({}) call from the start page test.describe block, leaving the existing tests and page.reload() behavior unchanged.
464-471: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReplace the fixed wait with a state-based wait.
waitForTimeout(1500)makes this render check timing-dependent. On a slow CI worker the screenshot can precede the first painted frame, and the 5,000-byte size assertion then fails intermittently. Poll a NiiVue render signal or a non-empty pixel sample instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/surfannotate/e2e/smoke.spec.js` around lines 464 - 471, Replace the fixed wait in the “the surface renders visibly” test with a state-based wait that polls a NiiVue render signal or verifies a non-empty pixel sample before taking the screenshot. Keep the existing `#gl` screenshot and size assertion, ensuring capture occurs only after the first rendered frame is detected.apps/surfannotate/test/vertexLookup.test.js (1)
26-30: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winThe probe generator loses the bits it then keeps.
seed * 1103515245reaches about 2^61 for a 31-bit seed. Doubles carry only 53 significant bits, so the low-order bits are discarded before& 0x7fffffffmasks the result. The mask keeps exactly those degraded bits, which can produce a short cycle and leave much of the volume unprobed. The sequence stays reproducible, so the weakness is silent.Use
Math.imulso the multiplication stays exact in 32-bit arithmetic.♻️ Proposed change
let seed = 12345; const random = () => { - seed = (seed * 1103515245 + 12345) & 0x7fffffff; + seed = (Math.imul(seed, 1103515245) + 12345) & 0x7fffffff; return seed / 0x7fffffff; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/surfannotate/test/vertexLookup.test.js` around lines 26 - 30, Update the random generator’s seed multiplication in the `random` function to use `Math.imul`, preserving exact 32-bit arithmetic before applying the existing mask and normalization. Keep the seed, constants, reproducibility, and returned range unchanged.apps/surfannotate/test/fixtures/make-real-patch.mjs (1)
30-57: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGenerate
lh.realflat.surf.giias a single disk with consistent winding.The radius cut keeps any face whose vertices fit inside the ball, so it can contain nonconnect components or interior holes. Keep only the largest connected component of the projected patch, then normalize all faces so their projected normals point toward -x before writing the fixture.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/surfannotate/test/fixtures/make-real-patch.mjs` around lines 30 - 57, Update the patch-generation flow after constructing the projected vertices and faces to retain only the largest connected face component, excluding disconnected islands and interior-hole artifacts. Then normalize every retained face’s winding using the projected coordinates so its normal points toward -x, and use the filtered, consistently oriented faces when building the output index array for lh.realflat.surf.gii.apps/surfannotate/test/surface.test.js (1)
97-105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHoist the duplicated coordinate literal into one constant.
Lines 99 and 102-104 repeat the same 18 coordinates.
buildAdjacencyandSurfacePathfindermust see identical geometry. If one copy is edited later, the graph weights and the pathfinder positions diverge, and the failure is hard to read.♻️ Proposed refactor
// Two disjoint triangles: no path between components. - const split = buildAdjacency( - Float32Array.from([0, 0, 0, 1, 0, 0, 0, 1, 0, 5, 5, 0, 6, 5, 0, 5, 6, 0]), - Uint32Array.from([0, 1, 2, 3, 4, 5]) - ); - const splitFinder = new SurfacePathfinder(split, Float32Array.from( - [0, 0, 0, 1, 0, 0, 0, 1, 0, 5, 5, 0, 6, 5, 0, 5, 6, 0] - )); + const splitVertices = Float32Array.from( + [0, 0, 0, 1, 0, 0, 0, 1, 0, 5, 5, 0, 6, 5, 0, 5, 6, 0] + ); + const split = buildAdjacency(splitVertices, Uint32Array.from([0, 1, 2, 3, 4, 5])); + const splitFinder = new SurfacePathfinder(split, splitVertices); assert.equal(splitFinder.path(0, 4), null);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/surfannotate/test/surface.test.js` around lines 97 - 105, Hoist the repeated 18-coordinate Float32Array data in the split-geometry test into a single named constant, then pass that same constant to both buildAdjacency and SurfacePathfinder so they always use identical geometry.apps/surfannotate/test/helpers.js (1)
33-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument that removed-block interior vertices remain in the mesh.
makeGridWithHoledrops faces but keeps every vertex. Forn = 9, a = 3, b = 5, vertex(4,4)ends up in no face, so it stays inverticesand becomes isolated in the adjacency graph. Tests that count vertices or components againstgraph.Vdepend on this. State it in the doc comment so a future change does not remove the orphan vertices silently.♻️ Proposed doc update
* The same grid with a square hole punched through it, so the patch is an * annulus rather than a disk. A line from the outer edge to the rim of the hole * does *not* separate an annulus — you can still walk round the other side — * which is the case edge closure has to detect and refuse. * + * Only faces are removed; every vertex is kept, so vertices strictly inside the + * removed block appear in no face and are isolated in the adjacency graph. + * `V` and `n` therefore stay the same as the intact grid. + * * `@param` {number} n vertices per side🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/surfannotate/test/helpers.js` around lines 33 - 55, Update the JSDoc for makeGridWithHole to explicitly state that removing the selected cell faces does not remove vertices, including interior vertices that become isolated or unused. Leave the mesh construction logic unchanged.apps/surfannotate/src/io/classify.js (1)
49-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the no-op ternary.
Both branches of
head instanceof Uint8Array ? head : headreturnhead, so the condition has no effect. The current code also copies an existingUint8Arrayinstead of reusing it.♻️ Proposed simplification
- const bytes = head ? new Uint8Array(head instanceof Uint8Array ? head : head) : null; + const bytes = head ? (head instanceof Uint8Array ? head : new Uint8Array(head)) : null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/surfannotate/src/io/classify.js` at line 49, Update the bytes initialization in the classify flow to remove the redundant ternary and reuse head directly when present, while preserving the null result when head is absent.apps/surfannotate/src/surface/exclude.js (1)
103-110: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDo not skip a mask whose length does not match
V.
unionMasksignores any mask of the wrong length without a signal. If a caller passes a mask built for another surface, that ROI is silently left out of the exclusion set, and a later ROI can then claim vertices that already belong to it. Every other entry point in this module throws on a length mismatch, so keep the contract consistent.🛡️ Proposed change
export function unionMasks(V, masks) { const union = new Uint8Array(V); for (const mask of masks) { - if (!mask || mask.length !== V) continue; + if (!mask) continue; + if (mask.length !== V) throw new Error('mask length must equal vertex count'); for (let v = 0; v < V; v++) if (mask[v]) union[v] = 1; } return union; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/surfannotate/src/surface/exclude.js` around lines 103 - 110, Update unionMasks to throw when any provided mask has a length different from V instead of silently skipping it; retain the existing handling for valid masks and align its validation behavior with the module’s other entry points.apps/surfannotate/src/surface/parcellation.js (1)
52-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the JSDoc names with the parameters and the return shape.
The parameter is
rois, notROIs, and the returned property isrois, asmain.jsshows withconst { rois: resolved } = resolveParcellation(...). Line 87 has the same mismatch withroi. Editors resolve these tags by name, so the current tags document nothing.📝 Proposed change
- * `@param` {RoiDefinition[]} ROIs - * `@returns` {{ROIs: Array<RoiDefinition & ResolvedRoi>, owner: Int32Array, + * `@param` {RoiDefinition[]} rois + * `@returns` {{rois: Array<RoiDefinition & ResolvedRoi>, owner: Int32Array, * assigned: number}} `owner` holds the id of the ROI owning each vertex, or * -1 where nothing does.- * `@param` {RoiDefinition} ROI + * `@param` {RoiDefinition} roiAlso applies to: 87-87
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/surfannotate/src/surface/parcellation.js` around lines 52 - 56, Update the JSDoc in the parcellation resolver to use the actual parameter name rois instead of ROIs, and document the returned property as rois to match resolveParcellation’s return shape and its consumers. Also correct the mismatched roi name in the JSDoc at the referenced later line, preserving the existing type descriptions.apps/surfannotate/src/surface/adjacency.js (1)
125-130: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReturned adjacency arrays retain their over-allocated buffers. Both graph builders allocate edge arrays sized for duplicate directed edges and then return
subarrayviews. A view keeps the whole buffer alive, so roughly twice the final edge storage stays resident for every graph the app holds. The app keeps topology per loaded surface and builds one cut graph per ROI duringresolveParcellation, so the waste multiplies.
apps/surfannotate/src/surface/adjacency.js#L125-L130: returnadjNeighbor.slice(0, k)andadjWeight.slice(0, k).apps/surfannotate/src/surface/exclude.js#L86-L91: returnnewNeighbor.slice(0, k)andnewWeight.slice(0, k).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/surfannotate/src/surface/adjacency.js` around lines 125 - 130, Replace the returned subarray views with copied slices so over-allocated adjacency buffers can be released: update adjacency.js lines 125-130 to use adjNeighbor.slice(0, k) and adjWeight.slice(0, k), and exclude.js lines 86-91 to use newNeighbor.slice(0, k) and newWeight.slice(0, k). Preserve the existing returned array contents and length.apps/surfannotate/src/niivue/meshAdapter.js (1)
131-163: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the
@returnscontract ofattachLabelLayer.The JSDoc promises the index of the new layer. The function returns
undefined. Return the index, or drop the@returnsline.♻️ Proposed fix
- * `@returns` {number} index of the new layer + * `@returns` {void}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/surfannotate/src/niivue/meshAdapter.js` around lines 131 - 163, Update attachLabelLayer to return the index of the layer it appends, preserving the existing `@returns` contract and calculating the index from mesh.layers after the push.apps/surfannotate/src/main.js (1)
1040-1044: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueParenthesize the
awaitin the conditional.
await isFreeSurferLabel(file) ? A : Bparses as(await isFreeSurferLabel(file)) ? A : B, which is the intent. The precedence is easy to misread, and one branch is synchronous while the other is awaited. Add parentheses.♻️ Proposed cleanup
- const layer = await isFreeSurferLabel(file) + const layer = (await isFreeSurferLabel(file)) ? attachValueLayer(state.nv, entry.mesh, labelToValues(await file.text(), entry.geometry.vertexCount).values, { ...display, name: file.name }) : await loadOverlay(state.nv, entry.mesh, file, display);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/surfannotate/src/main.js` around lines 1040 - 1044, In the layer assignment conditional, parenthesize the awaited `isFreeSurferLabel(file)` expression so the condition is visually unambiguous, while preserving the existing `attachValueLayer` and `loadOverlay` branches.apps/surfannotate/src/surface/hatch.js (1)
30-40: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the stale comment and the trailing comma.
Line 40 describes a perpendicular stripe family, but
hatchMaskcomputes only one direction. The comment misleads a later reader. Line 34 also ends the destructuring with,}.♻️ Proposed cleanup
- direction = [1, 0.6, 0.35],} = options; + direction = [1, 0.6, 0.35] + } = options; if (spacingMm <= 0) throw new Error('hatch spacing must be positive'); if (duty <= 0 || duty >= 1) throw new Error('hatch duty must be between 0 and 1'); const primary = normalize(direction); - // Any vector not parallel to `primary` gives a perpendicular family. -🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/surfannotate/src/surface/hatch.js` around lines 30 - 40, In hatchMask, remove the stale comment about a perpendicular stripe family and remove the trailing comma before the closing brace in the options destructuring. Preserve the existing option defaults, validation, and direction normalization.apps/surfannotate/src/io/points.js (1)
40-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the
offsetoption.Line 41 destructures
options.offset, but the JSDoc block does not list it. Add it so the export contract stays discoverable next tocoordinateSpace.♻️ Proposed JSDoc addition
* `@param` {string} [options.created] ISO timestamp; supplied by the caller so * the writer stays deterministic and testable + * `@param` {number[]} [options.offset] subtracted from every coordinate, undoing + * the volume-geometry translation the loader applied on the way in * `@returns` {string}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/surfannotate/src/io/points.js` around lines 40 - 67, Update the JSDoc for writePointsJson to document the options.offset parameter alongside coordinateSpace, including its purpose as the coordinate adjustment applied when writing xyz values. Keep the existing function behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/surfannotate/e2e/smoke.spec.js`:
- Around line 197-213: Update the smoke test around the evaluate callback so the
broken boundary is represented in session state or supplied through the fill
API, rather than mutating the temporary result of boundaryMask(). Ensure the
seeded fill tests escape through the intended gap, and remove the unused
geometry destructuring from window.__surfannotate.
In `@apps/surfannotate/src/io/geometryOffset.js`:
- Around line 25-35: Update niivueTranslation to handle gzip-compressed surface
buffers before magic-byte detection, decompressing them and passing the
uncompressed bytes to FreeSurfer/GIFTI detection; alternatively reject
compressed input with a clear error. Ensure meshAdapter’s offset calculation
cannot silently use [0,0,0] for supported .gz surfaces.
In `@apps/surfannotate/src/main.js`:
- Around line 1664-1672: Update the download function so URL.revokeObjectURL
runs in a later task after anchor.click(), rather than immediately in the same
task; preserve the existing blob URL creation and download behavior.
- Around line 836-908: Update loadSurface to retain the mesh returned by
loadMeshFromFile and, in the catch path, remove that mesh from the NiiVue scene
before reporting the error; ensure cleanup is safe when loading itself fails
before a mesh is available.
- Around line 1738-1746: Update exportPoints to pass writePointsJson an explicit
coordinateSpace derived from activeSurface(), rather than relying on its
tkreg-ras-white default; ensure inflated, spherical, flat, and white surfaces
are serialized with their corresponding coordinate-space value while preserving
the existing translation metadata and export behavior.
In `@apps/surfannotate/src/niivue/meshAdapter.js`:
- Around line 225-241: Update the validation around layer.values before calling
robustRange: reject missing values with the existing overlay error path, and
validate values.length against mesh.pts.length / 3 multiplied by (layer.nFrame4D
|| 1). Keep the robustRange and calibration assignments in the subsequent path,
ensuring overlays with valid single-frame or 4D data continue to register.
In `@apps/surfannotate/src/surface/parcellation.js`:
- Around line 97-100: Update the ROI validation around session.addClick so it
checks each roi.clicks vertex against the claimed/excluded vertex set directly
before or while adding clicks. Return the existing LOST_POINTS result when any
click was already claimed, while preserving session-based validation for invalid
or repeated vertices and the normal path for unclaimed clicks.
In `@apps/surfannotate/src/surface/roiSession.js`:
- Around line 259-268: Update the seeded fill path in the closure handling
around fillClosedRegion so CLOSURE_EDGE bypasses the DEFAULT_MAX_FRACTION 40%
guard, while preserving the guard for other closure types. Ensure edge fills
remain permitted only after the existing component-separation validation has
succeeded.
- Around line 366-374: Update the ROI session constructor and togglePoint so
default point names use a session-level monotonic counter rather than
points.length + 1. Increment the counter only when adding a point, ensuring
removals never allow generated names to repeat while preserving explicitly
supplied names.
In `@apps/surfannotate/test/fixtures/make-real-patch.mjs`:
- Around line 17-23: Update the lh.pial parsing flow to validate the 3-byte
FreeSurfer TRIANGLE_FILE magic before scanning the header, and bound the newline
search so p never advances beyond buf.length. Report an invalid or truncated
header instead of continuing indefinitely, while preserving the existing count
parsing for valid files.
In `@apps/surfannotate/test/geometryOffset.test.js`:
- Line 12: Update the geometryOffset test’s niivueTranslation input to use the
byte view returned by readFileSync directly, rather than its underlying .buffer,
so offsets and lengths match the fixture contents. Preserve the existing
translation assertion and fixture loading behavior.
In `@apps/surfannotate/test/surface.test.js`:
- Around line 240-243: Update the isolated-vertex test around fillClosedRegion
to explicitly assert both valid outcomes: verify result.inside[3] is 0 when
inside is returned, or assert the expected error when inside is null. Do not
leave the assertion conditional without validating the error path.
In `@apps/surfannotate/THIRD-PARTY.md`:
- Around line 38-41: Expand the `@neurodesk/webapp-components` entry in
THIRD-PARTY.md with the complete, exact MIT copyright and permission notice from
packages/components/LICENSE, replacing the repository-relative reference while
preserving the existing component description.
---
Nitpick comments:
In `@apps/surfannotate/e2e/smoke.spec.js`:
- Around line 1266-1268: Remove the empty test.use({}) call from the start page
test.describe block, leaving the existing tests and page.reload() behavior
unchanged.
- Around line 464-471: Replace the fixed wait in the “the surface renders
visibly” test with a state-based wait that polls a NiiVue render signal or
verifies a non-empty pixel sample before taking the screenshot. Keep the
existing `#gl` screenshot and size assertion, ensuring capture occurs only after
the first rendered frame is detected.
In `@apps/surfannotate/scripts/fetch-fixtures.mjs`:
- Around line 42-55: Update fetchWithRetry so each fetch attempt passes an
AbortSignal.timeout(...) option to fetch, using the script’s existing timeout
configuration if available or an appropriate bounded duration. Ensure timeout
failures are caught by the existing catch block so they trigger the current
retry and final-error behavior.
In `@apps/surfannotate/src/io/classify.js`:
- Line 49: Update the bytes initialization in the classify flow to remove the
redundant ternary and reuse head directly when present, while preserving the
null result when head is absent.
In `@apps/surfannotate/src/io/points.js`:
- Around line 40-67: Update the JSDoc for writePointsJson to document the
options.offset parameter alongside coordinateSpace, including its purpose as the
coordinate adjustment applied when writing xyz values. Keep the existing
function behavior unchanged.
In `@apps/surfannotate/src/main.js`:
- Around line 1040-1044: In the layer assignment conditional, parenthesize the
awaited `isFreeSurferLabel(file)` expression so the condition is visually
unambiguous, while preserving the existing `attachValueLayer` and `loadOverlay`
branches.
In `@apps/surfannotate/src/niivue/meshAdapter.js`:
- Around line 131-163: Update attachLabelLayer to return the index of the layer
it appends, preserving the existing `@returns` contract and calculating the index
from mesh.layers after the push.
In `@apps/surfannotate/src/surface/adjacency.js`:
- Around line 125-130: Replace the returned subarray views with copied slices so
over-allocated adjacency buffers can be released: update adjacency.js lines
125-130 to use adjNeighbor.slice(0, k) and adjWeight.slice(0, k), and exclude.js
lines 86-91 to use newNeighbor.slice(0, k) and newWeight.slice(0, k). Preserve
the existing returned array contents and length.
In `@apps/surfannotate/src/surface/exclude.js`:
- Around line 103-110: Update unionMasks to throw when any provided mask has a
length different from V instead of silently skipping it; retain the existing
handling for valid masks and align its validation behavior with the module’s
other entry points.
In `@apps/surfannotate/src/surface/hatch.js`:
- Around line 30-40: In hatchMask, remove the stale comment about a
perpendicular stripe family and remove the trailing comma before the closing
brace in the options destructuring. Preserve the existing option defaults,
validation, and direction normalization.
In `@apps/surfannotate/src/surface/parcellation.js`:
- Around line 52-56: Update the JSDoc in the parcellation resolver to use the
actual parameter name rois instead of ROIs, and document the returned property
as rois to match resolveParcellation’s return shape and its consumers. Also
correct the mismatched roi name in the JSDoc at the referenced later line,
preserving the existing type descriptions.
In `@apps/surfannotate/test/fixtures/make-real-patch.mjs`:
- Around line 30-57: Update the patch-generation flow after constructing the
projected vertices and faces to retain only the largest connected face
component, excluding disconnected islands and interior-hole artifacts. Then
normalize every retained face’s winding using the projected coordinates so its
normal points toward -x, and use the filtered, consistently oriented faces when
building the output index array for lh.realflat.surf.gii.
In `@apps/surfannotate/test/helpers.js`:
- Around line 33-55: Update the JSDoc for makeGridWithHole to explicitly state
that removing the selected cell faces does not remove vertices, including
interior vertices that become isolated or unused. Leave the mesh construction
logic unchanged.
In `@apps/surfannotate/test/surface.test.js`:
- Around line 97-105: Hoist the repeated 18-coordinate Float32Array data in the
split-geometry test into a single named constant, then pass that same constant
to both buildAdjacency and SurfacePathfinder so they always use identical
geometry.
In `@apps/surfannotate/test/vertexLookup.test.js`:
- Around line 26-30: Update the random generator’s seed multiplication in the
`random` function to use `Math.imul`, preserving exact 32-bit arithmetic before
applying the existing mask and normalization. Keep the seed, constants,
reproducibility, and returned range unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 036bfa81-161e-4876-a28f-a5369767ad3b
⛔ Files ignored due to path filters (6)
apps/surfannotate/icon/surfannotate.svgis excluded by!**/*.svgapps/surfannotate/public/favicon-180.pngis excluded by!**/*.pngapps/surfannotate/public/favicon-256.pngis excluded by!**/*.pngapps/surfannotate/public/favicon-32.pngis excluded by!**/*.pngapps/surfannotate/public/favicon-48.pngis excluded by!**/*.pngpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (49)
LICENSES.mdapps/surfannotate/.gitignoreapps/surfannotate/AGENT.mdapps/surfannotate/CLAUDE.mdapps/surfannotate/LICENSEapps/surfannotate/README.mdapps/surfannotate/THIRD-PARTY.mdapps/surfannotate/e2e/smoke.spec.jsapps/surfannotate/icon/render.mjsapps/surfannotate/index.htmlapps/surfannotate/package.jsonapps/surfannotate/playwright.config.jsapps/surfannotate/scripts/fetch-fixtures.mjsapps/surfannotate/src/io/classify.jsapps/surfannotate/src/io/freesurferLabel.jsapps/surfannotate/src/io/geometryOffset.jsapps/surfannotate/src/io/gifti.jsapps/surfannotate/src/io/naming.jsapps/surfannotate/src/io/points.jsapps/surfannotate/src/main.jsapps/surfannotate/src/niivue/colormaps.jsapps/surfannotate/src/niivue/meshAdapter.jsapps/surfannotate/src/styles.cssapps/surfannotate/src/surface/adjacency.jsapps/surfannotate/src/surface/edgeAnchor.jsapps/surfannotate/src/surface/exclude.jsapps/surfannotate/src/surface/fill.jsapps/surfannotate/src/surface/hatch.jsapps/surfannotate/src/surface/parcellation.jsapps/surfannotate/src/surface/pathfinder.jsapps/surfannotate/src/surface/roiSession.jsapps/surfannotate/src/surface/vertexLookup.jsapps/surfannotate/test/classify.test.jsapps/surfannotate/test/edgeAnchor.test.jsapps/surfannotate/test/edgeClosure.test.jsapps/surfannotate/test/exclude.test.jsapps/surfannotate/test/fixtures/make-flat-patch.mjsapps/surfannotate/test/fixtures/make-real-patch.mjsapps/surfannotate/test/geometryOffset.test.jsapps/surfannotate/test/hatch.test.jsapps/surfannotate/test/helpers.jsapps/surfannotate/test/io.test.jsapps/surfannotate/test/naming.test.jsapps/surfannotate/test/parcellation.test.jsapps/surfannotate/test/roiSession.test.jsapps/surfannotate/test/surface.test.jsapps/surfannotate/test/vertexLookup.test.jsapps/surfannotate/vite.config.jsregistry/apps.yml
Contributor
|
This is soooo cool :) |
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.
Adds
apps/surfannotate, a browser-native tool for delineating regions ofinterest on cortical surfaces and exporting them. Everything runs client-side —
no surface, overlay or label leaves the machine.
What it does
.surf.gii,.mz3and the other meshesNiiVue reads, plus per-vertex overlays (curvature, thickness,
.annot,.label.gii, CIFTI.dscalar.nii). Several surfaces at once, one shown.edges, so a border follows the surface rather than cutting through it.
it have left, so no vertex belongs to two, and each ROI's border becomes the
edge the next can be closed against. Editing one re-derives the ones below it,
so moving a shared boundary moves both sides.
you, so two clicks can be enough.
.label, GIfTI.label.gii, and vertex lists as JSON.Testing
109 unit tests (
node --test, no browser) covering the geometry and the fileformats, and 56 Playwright tests against headless WebGL2.
test:e2edownloadsand generates its own fixtures, so a clean checkout works.
Known limitations
.labelcoordinates come from the surface drawn on. The export panel warnswhen that surface is inflated, spherical or flat; the vertex indices are
correct regardless. freeview substitutes the white surface here — we warn
instead.
index, ~30 MB per 160k-vertex hemisphere.
Summary by CodeRabbit
New Features
Documentation
Tests