Phase 3: Rebuild FLASHApp viewers on OpenMS-Insight via frozen template

[t0mdavid-m]

Summary

This PR completes Phase 3 of the FLASHApp migration: rebuild the three viewer pages (FLASHDeconv, FLASHTnT, FLASHQuant) on top of OpenMS-Insight using a reusable, tool-agnostic grid module from a frozen streamlit-template package. The old bespoke Vue mega-component and index-based selection logic are replaced with value-based filters/interactivity and a shared StateManager for cross-linked panels.

Key Changes

New Modules

• src/view/grid.py — Tool-agnostic linked-grid renderer (render_linked_grid() + LayoutManager class). Distills the old render.py::render_grid() + both FLASH*LayoutManager classes into a single reusable template that can be frozen and vendored byte-for-byte unchanged into downstream apps.

• src/render/schema.py — Adapter layer that converts FLASHApp's wide, list-column, index-addressed FileManager caches into tidy parquet with stable value IDs (scan_id, mass_id, peak_id, protein_id, etc.). Enables OpenMS-Insight's value-based filters/interactivity model. Includes helpers for exploding nested signal/noise peaks, KDE distributions, and protein/tag data.

• src/render/render.py (repurposed) — Now a builder factory. make_builders(file_manager, dataset_id, tool, settings=None) returns a {comp_name: () -> BaseComponent} map. Each zero-arg factory closes over dataset context and uses file_manager.result_path() to feed data_path= to Insight components. Includes oracle Tabulator column definitions (titles, formatters, sorters) ported verbatim so migrated Tables show the same curated columns and formatting.

Removed Modules

• src/render/update.py — Index-based selection logic (scanIndex / massIndex / proteinIndex / xRange/yRange slicing). Replaced by value-based filters/interactivity in Insight components.
• src/render/initialize.py — Per-panel cache loading. Now handled by Insight's data_path= + subprocess preprocessing.
• src/render/StateTracker.py — Local state reconciler. Replaced 1:1 by openms_insight.StateManager.
• src/render/components.py — Vue component registration. No longer needed.
• src/render/util.py — Hash utility (unused).
• .gitmodules + openms-streamlit-vue-component/ — Old Vue submodule removed.

Page Updates

• content/FLASHDeconv/FLASHDeconvViewer.py, content/FLASHTnT/FLASHTnTViewer.py, content/FLASHQuant/FLASHQuantViewer.py — Simplified to call show_linked_grid() with a layout and builders. No more render_grid() / StateTracker / initialize_data / update_data / filter_data calls.

• src/common/common.py — Added show_linked_grid() convenience wrapper for rendering N-experiment grids with the frozen template.

Data Layer

• src/workflow/FileManager.py — Added result_path(dataset_id, name_tag) method to expose parquet file paths for data_path= (Insight's subprocess preprocessing entry point).

Testing & Migration Harness

• tests/test_render_schema.py — Construct-smoke for build_insight_caches(): synthetic FileManager caches → tidy parquet with stable IDs and correct explode shapes.
• **`tests/test_render

https://claude.ai/code/session_017kD4FyAsNvW6VFTZwVvSne

Summary by CodeRabbit

• New Features
  • Unified linked-grid viewer with per-experiment layout editing and side-by-side rendering; value-based Insight builders and tidy-cache generation for all viewers; FileManager now exposes parquet path helpers.
• Documentation
  • Added Phase 3 migration plan and review guidance.
• Tests
  • Large new test suites exercising builders, schema transforms, and grid wiring.
• Chores
  • Runtime image build updated to install Insight package; dependency pinned to openms-insight; removed legacy local JS submodule.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR completes Phase 3 of the FLASHApp migration by replacing legacy Vue/component builds and index-based rendering with OpenMS-Insight integration and a frozen, reusable linked-grid template. It introduces tidy-parquet schema adapters, a builder factory, a layout manager/renderer, viewer rewrites, Docker/CI integration for openms-insight, migration gates/harness, and comprehensive tests.

Changes

FLASHApp Phase 3 Migration

Layer / File(s)	Summary
Docker: Build and Install OpenMS-Insight Package Dockerfile, Dockerfile.arm	Adds a node:21 insight-build stage that clones the OpenMS-Insight migration branch, runs the JS build, injects the Vue dist into the Python package tree, and pip-installs the built package during image build; removes legacy local JS build/artifact copy steps.
Requirements & CI build step requirements.txt, .github/workflows/unit-tests.yml	Pins openms-insight==0.1.15 and updates unit-tests workflow to set up Node 21, clone/build openms-insight, copy the built dist, install local package, then run pytest.
FileManager: Add Parquet Path Support for Insight src/workflow/FileManager.py	Adds as_path option to get_results() (priority over pyarrow/polars branches) and result_path() helper to return on-disk Parquet paths for Insight data_path usage; guards against missing DB rows.
Schema Adapter: Transform Oracle Caches to Insight-Ready Tidy Parquet src/render/schema.py	New adapter implementing Polars-based primitives to explode list/nested columns, mint stable IDs, normalize KDEs, and build idempotent tidy Parquet outputs per tool (flashdeconv, flashtnt, flashquant) behind build_insight_caches().
Render Builder Factory: Lazy Component Construction with Interactivity Wiring src/render/render.py	Repurposes render module into make_builders() returning zero-arg component factories with curated table column definitions, _sequence_view wiring, value-based filters/interactivity, and namespaced cache IDs routing to tidy Parquet frames.
Frozen Grid Module: Reusable Linked-Grid Renderer and Layout Editor src/view/grid.py	Adds render_linked_grid() to render nested component layouts using a shared StateManager for cross-panel interactivity and LayoutManager to edit/persist layouts with dependency validation and JSON import/export.
Streamlit Wrapper: show_linked_grid() for Multi-Experiment Rendering src/common/common.py	Introduces show_linked_grid() to wrap render_linked_grid() and handle multi-experiment state scoping and side-by-side vs stacked rendering.
FLASHDeconv Viewer: Lazy Experiment Selection with Insight Rendering content/FLASHDeconv/FLASHDeconvViewer.py	Rewrites viewer to use per-experiment selectbox values as stable dataset ids, lazily call build_insight_caches() and make_builders(), and render via show_linked_grid(); removes previous index/display-name mapping helpers.
FLASHQuant Viewer: Dataset Selection with Tidy Cache Building content/FLASHQuant/FLASHQuantViewer.py	Replaces name→index session-state flow with id-based selection, gates on quant_dfs, lazily builds tidy caches, constructs builders, and renders the linked grid.
FLASHTnT Viewer: Per-Experiment Rendering with Best-Per-Spectrum Control content/FLASHTnT/FLASHTnTViewer.py	Adds per-experiment _render_experiment() with "Best per spectrum" checkbox forwarded to make_builders(), lazily builds caches and uses show_linked_grid(); persists layout/side_by_side fields.
Migration Infrastructure: Phase 3 Planning, Verification, and Convergence migration/README.md, migration/REVIEW.md, migration/specs/PHASE3_PLAN.md, migration/units.yaml, migration/nondivergence.py, migration/run_review.py	Adds Phase 3 plans/specs, units registry, nondivergence gate that compares normalized file hashes, and a convergence harness (run_review.py) that records gates/reviews and reports convergence across rounds.
Test Infrastructure: Synthetic Caches, Component Smoke Tests, and Schema Validation tests/conftest.py, tests/test_render_builders.py, tests/test_render_schema.py	Adds Streamlit-mocking test fixtures and synthetic oracle caches; tests validate schema primitives, build_insight_caches idempotency/edge cases, make_builders() outputs, and render_linked_grid wiring and component payloads.
Legacy Stack Removal src/render/StateTracker.py, src/render/components.py, src/render/initialize.py, src/render/update.py, src/render/util.py, tests/test_selection_clear.py, .gitmodules, openms-streamlit-vue-component	Removes prior StateTracker, component wrappers, initialization/update utilities, hashing helper, selection-clear tests, and deletes .gitmodules submodule entry referencing the old Vue component.

Sequence Diagram(s)

sequenceDiagram
  participant Page as Streamlit Page
  participant Grid as render_linked_grid
  participant Builder as make_builders factory
  participant State as StateManager

  Page->>Grid: call render_linked_grid(layout, builders, state_key)
  Grid->>State: create StateManager(state_key)
  Grid->>Builder: invoke factory() for each cell
  Builder->>Grid: return component instance
  Builder->>State: register filters/interactivity
  Grid->>Builder: component.render() -> Vue payload

Loading

Possibly Related PRs

• OpenMS/FLASHApp#22: Related to the submodule reference for openms-streamlit-vue-component (both PRs touch that metadata).
• OpenMS/FLASHApp#57: Related changes to viewer selection/rendering flows in FLASHDeconv and FLASHTnT; both refactor selection behavior.
• OpenMS/FLASHApp#21: Prior work on viewer helpers that this PR ultimately removes or replaces in favor of Insight-linked-grid rendering.

Poem

🐰 I hopped through grids and tidy pails of data,
Swapped indices for values — now viewers cater.
Vue crumbs tucked away, Insight bakes the chart,
Layouts freeze, builders hum — the panels start.
Phase Three done, the rabbit nods with cheer.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 58.74% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: rebuilding FLASHApp viewers on OpenMS-Insight using a frozen template, which is the core objective of this Phase 3 migration PR.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch claude/kind-heisenberg-u6dVm

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 10

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/workflow/FileManager.py (1)

480-486: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle missing cache rows before iterating fetchone().

If dataset_id has no row in one of the cache tables, fetchone() returns None and both loops raise TypeError before partial/KeyError handling runs. That turns a normal cache miss into a hard crash and breaks the new result_path() contract.

Suggested fix

         result = self.cache_cursor.fetchone()
+        if result is None:
+            result = (None,) * len(file_columns)
         for c, r in zip(file_columns, result):
             if r is None:
                 if partial:
                     continue
                 else:
@@
         result = self.cache_cursor.fetchone()
+        if result is None:
+            result = (None,) * len(data_columns)
         for c, r in zip(data_columns, result):
             if r is None:
                 if partial:
                     continue
                 else:

Also applies to: 498-504

🤖 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 `@src/workflow/FileManager.py` around lines 480 - 486, The SELECT followed by
fetchone() can return None for a cache miss, so before zipping over file_columns
and result ensure you check "result is None" and treat it as a cache miss (do
not iterate). For both occurrences (the block using self.cache_cursor,
file_columns and dataset_id starting at the shown snippet and the similar block
at 498-504) either raise KeyError(dataset_id) so upstream partial/KeyError
handling runs, or return the missing-value expected by result_path(); do this
check immediately after result = self.cache_cursor.fetchone() to avoid the
TypeError.

🧹 Nitpick comments (1)

migration/nondivergence.py (1)

56-57: ⚡ Quick win

Resolve nondivergence pair paths from a fixed base, not process CWD.

Line 56-Line 57 currently makes relative paths caller-dependent. Resolve relative entries from repo/config root so checks are deterministic across runners.

🤖 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 `@migration/nondivergence.py` around lines 56 - 57, Current code creates
Path(pair[0]) and Path(pair[1]) which resolves relative paths against the
process CWD; change this to resolve against the repository/config root instead.
Modify the logic around a, b (and subsequent ea, eb) so you first compute a
stable base Path (e.g., repo_root or config_root) and then construct a =
base.joinpath(pair[0]) and b = base.joinpath(pair[1]) (or otherwise resolve pair
entries against that base) so existence checks ea, eb are deterministic across
runners; update any uses of a and b accordingly.

🤖 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 `@content/FLASHDeconv/FLASHDeconvViewer.py`:
- Around line 47-49: The current mapping using to_id =
{file_manager.get_display_name(r): r for r in results} is lossy when display
names are duplicated; update the lookup to preserve all datasets by either (a)
keying by a unique identifier (use the raw dataset id from each result instead
of file_manager.get_display_name) where the UI can still show display names, or
(b) build a multimap from display name to a list of result ids (e.g., to_id:
dict[str, list] and append each r) so duplicates are retained; apply the same
change to the other mapping logic around the block handling lines 57–64 to
ensure no dataset is overwritten or lost.
- Line 67: The tool key passed to show_linked_grid is not panel-scoped and can
collide when two panels use the same ds; update the call to generate a unique
panel-scoped tool string (e.g., include a panel identifier such as an explicit
panel index, a unique id derived from exp_layout or builders, or a UUID) and
pass that as the tool argument to show_linked_grid (replace the current
f"flashdeconv_{ds}" with something like f"flashdeconv_{panel_id}_{ds}" or
similar) so the linked-grid state is isolated per panel; change the code that
constructs the tool key where show_linked_grid([exp_layout], builders, tool=...)
is invoked.

In `@content/FLASHQuant/FLASHQuantViewer.py`:
- Around line 29-30: The current code builds names =
[file_manager.get_display_name(r) for r in results] and to_id =
{file_manager.get_display_name(r): r for r in results}, which loses datasets
when display names collide; update the mapping and selection logic to use a
unique key instead of the display name (or map a display name to a list of
results). For example, keep a list of tuples (display_name, result) or build
to_id using a unique identifier from each result (e.g., r.id or r.path) while
still presenting file_manager.get_display_name(r) in the UI, and change the
lookup code that currently uses the display name (the selection at Line 38 and
the surrounding block that spans the same section) to resolve selection via that
unique identifier or disambiguate by choosing the correct entry from the list of
results for that display name.

In `@content/FLASHTnT/FLASHTnTViewer.py`:
- Around line 42-43: The code builds names = [file_manager.get_display_name(r)
for r in results] and to_id = {file_manager.get_display_name(r): r for r in
results}, which will drop entries when multiple results share the same display
name; change the mapping so resolution is based on a unique identifier instead
of display name (e.g., use r.id or r.path) or map display names to lists of
results: create to_id_by_unique = {r.id: r for r in results} and keep names as
display labels, or build to_id = defaultdict(list) and append each r under
file_manager.get_display_name(r) so duplicates are preserved; then update any
lookup code that uses to_id (the lookup at or around the code that resolves
selections) to use the unique-id map or to handle list values accordingly (e.g.,
disambiguate by id or present multiple choices).
- Line 76: The linked-grid tool key is currently only namespaced by dataset id
via tool=f"flashtnt_{ds}", which allows two side-by-side panels selecting the
same dataset to collide; update the call to show_linked_grid to include the
panel index (e.g., tool=f"flashtnt_{ds}_{panel_idx}" or similar) so the key is
unique per panel (locate the show_linked_grid([exp_layout], builders, tool=...)
invocation and add the panel index variable used when rendering panels).

In `@migration/units.yaml`:
- Around line 8-14: Replace hard-coded absolute paths in migration/units.yaml
(oracle_root, template_repo, nondivergence_pairs) with repo-relative or
environment-parameterized tokens (e.g., use a ${REPO_ROOT} or ${TEMPLATE_REPO}
placeholder) and update the code that reads units.yaml to expand those tokens at
runtime (resolve via an env var or compute repo root with os.getcwd()/git
rev-parse and join paths). Specifically change the values for oracle_root and
template_repo and the paths inside nondivergence_pairs to use placeholders, and
update the loader/validator that reads units.yaml to perform
environment-variable expansion and path normalization so CI and other machines
can resolve the locations correctly.

In `@src/render/render.py`:
- Around line 624-626: The sequence_view factory is always registered and
immediately triggers result_path() for seq_deconv/seq_tnt even if
build_insight_caches skipped them, causing cache misses; update the registration
site to first check for the presence of the sequence cache(s) before adding the
"sequence_view" entry (e.g. query the cache mapping returned by
build_insight_caches or call file_manager.has_cache/exists for the
seq_deconv/seq_tnt result paths) and only assign "sequence_view": lambda:
_sequence_view(...) when that check succeeds so datasets without sequence data
won't construct the panel.

In `@src/render/schema.py`:
- Around line 551-555: The lambda passed to pl.col("Scan").map_elements() only
checks for s is not None, so NaN/pd.NA still reach int(s) and crash; update the
guard to treat all missing/null-like values as missing (return -1) by using a
null check that covers NaN/pd.NA (e.g., pd.isna(s) or math.isnan where
appropriate) before calling int(s), keeping the rest of the expression
(scan_to_deconv lookup, alias("scan_id"), return_dtype=pl.Int64) unchanged so
missing scans yield -1 and valid scans are int-cast and mapped via
scan_to_deconv.

In `@src/view/grid.py`:
- Around line 243-266: The upload flow needs to validate JSON and avoid index
errors in expand: wrap json.load(uploaded) in a try/except to catch
JSONDecodeError and return/raise a controlled validation error, verify the
decoded object is the expected list-of-lists structure before calling expand,
and inside Grid.expand change the name-lookup to a safe pattern (e.g., check
membership of col in self.component_names or use a dict mapping name->option and
skip unknown IDs) rather than calling self.component_names.index(col) which
raises ValueError; ensure expand returns a consistent validated structure (or
raises a clear validation error) instead of letting runtime exceptions
propagate.

---

Outside diff comments:
In `@src/workflow/FileManager.py`:
- Around line 480-486: The SELECT followed by fetchone() can return None for a
cache miss, so before zipping over file_columns and result ensure you check
"result is None" and treat it as a cache miss (do not iterate). For both
occurrences (the block using self.cache_cursor, file_columns and dataset_id
starting at the shown snippet and the similar block at 498-504) either raise
KeyError(dataset_id) so upstream partial/KeyError handling runs, or return the
missing-value expected by result_path(); do this check immediately after result
= self.cache_cursor.fetchone() to avoid the TypeError.

---

Nitpick comments:
In `@migration/nondivergence.py`:
- Around line 56-57: Current code creates Path(pair[0]) and Path(pair[1]) which
resolves relative paths against the process CWD; change this to resolve against
the repository/config root instead. Modify the logic around a, b (and subsequent
ea, eb) so you first compute a stable base Path (e.g., repo_root or config_root)
and then construct a = base.joinpath(pair[0]) and b = base.joinpath(pair[1]) (or
otherwise resolve pair entries against that base) so existence checks ea, eb are
deterministic across runners; update any uses of a and b accordingly.

🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9ef92ede-ee8d-47e5-83d9-a18e8557d9c0

📥 Commits

Reviewing files that changed from the base of the PR and between a89427d and de4b7d5.

📒 Files selected for processing (31)

• .gitmodules
• Dockerfile
• Dockerfile.arm
• content/FLASHDeconv/FLASHDeconvViewer.py
• content/FLASHQuant/FLASHQuantViewer.py
• content/FLASHTnT/FLASHTnTViewer.py
• migration/README.md
• migration/REVIEW.md
• migration/nondivergence.py
• migration/review-log/phase-3.jsonl
• migration/run_review.py
• migration/specs/.gitkeep
• migration/specs/PHASE3_PLAN.md
• migration/units.yaml
• openms-streamlit-vue-component
• requirements.txt
• src/common/common.py
• src/render/StateTracker.py
• src/render/components.py
• src/render/initialize.py
• src/render/render.py
• src/render/schema.py
• src/render/update.py
• src/render/util.py
• src/view/__init__.py
• src/view/grid.py
• src/workflow/FileManager.py
• tests/conftest.py
• tests/test_render_builders.py
• tests/test_render_schema.py
• tests/test_selection_clear.py

💤 Files with no reviewable changes (8)

• src/render/StateTracker.py
• .gitmodules
• tests/test_selection_clear.py
• src/render/util.py
• src/render/initialize.py
• src/render/components.py
• src/render/update.py
• openms-streamlit-vue-component

[coderabbitai]

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Hard-coded /home/user/... paths make the phase harness non-portable.

This config will fail on any machine/CI runner that doesn’t mirror that exact filesystem layout. Please switch targets/oracles/cwd entries to repo-relative or env-parameterized paths and resolve them in the scripts.

Also applies to: 22-50, 55-119

🤖 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 `@migration/units.yaml` around lines 8 - 14, Replace hard-coded absolute paths
in migration/units.yaml (oracle_root, template_repo, nondivergence_pairs) with
repo-relative or environment-parameterized tokens (e.g., use a ${REPO_ROOT} or
${TEMPLATE_REPO} placeholder) and update the code that reads units.yaml to
expand those tokens at runtime (resolve via an env var or compute repo root with
os.getcwd()/git rev-parse and join paths). Specifically change the values for
oracle_root and template_repo and the paths inside nondivergence_pairs to use
placeholders, and update the loader/validator that reads units.yaml to perform
environment-variable expansion and path normalization so CI and other machines
can resolve the locations correctly.

[coderabbitai]

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Make experiment rendering input/state explicitly per-experiment.

This helper currently applies one builders map to every experiment and uses state_key=f"{tool}__exp{idx}". That can leak stale selections across dataset switches and prevents clean per-dataset isolation when experiments differ.

💡 Suggested API shape

 def show_linked_grid(
     layout,
-    builders,
+    builders,
     *,
     tool,
+    state_keys=None,
     side_by_side=False,
@@
 ):
@@
-    def _one(exp_idx, exp_layout, container):
+    def _one(exp_idx, exp_layout, container):
+        exp_builders = (
+            builders[exp_idx] if isinstance(builders, (list, tuple)) else builders
+        )
+        exp_state_key = (
+            state_keys[exp_idx]
+            if state_keys is not None
+            else f"{tool}__exp{exp_idx}"
+        )
         with container:
             render_linked_grid(
                 exp_layout,
-                builders,
-                state_key=f"{tool}__exp{exp_idx}",
+                exp_builders,
+                state_key=exp_state_key,
                 grid_key=f"{grid_key}_{exp_idx}",
                 height=height,
                 column_heights=column_heights,
             )

[coderabbitai]

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Harden uploaded layout handling to avoid runtime crashes.

json.load(uploaded) and the expand() name lookup path can throw on invalid JSON or unknown/stale component IDs, which currently crashes the layout page instead of surfacing a validation error.

💡 Suggested fix

 def _handle_setting_buttons(self) -> None:
@@
-        if uploaded is not None:
-            uploaded_layout = json.load(uploaded)
+        if uploaded is not None:
+            try:
+                uploaded_layout = json.load(uploaded)
+            except json.JSONDecodeError:
+                st.session_state[self._k("component_error")] = "Invalid JSON layout file"
+                return
+            if not isinstance(uploaded_layout, list):
+                st.session_state[self._k("component_error")] = "Invalid layout format"
+                return
@@
-                st.session_state[self._k("layout")] = self.expand(
+                st.session_state[self._k("layout")] = self.expand(
                     uploaded_layout, drop_empty_experiments=False
                 )

 def expand(self, trimmed: list, drop_empty_experiments: bool = True) -> list:
@@
                 for col in row:
                     if col:
-                        cols.append(
-                            self.component_options[self.component_names.index(col)]
-                        )
+                        if col not in self.component_names:
+                            st.session_state[self._k("component_error")] = (
+                                f"Unknown component in saved layout: {col}"
+                            )
+                            continue
+                        cols.append(
+                            self.component_options[self.component_names.index(col)]
+                        )

Also applies to: 423-438

🤖 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 `@src/view/grid.py` around lines 243 - 266, The upload flow needs to validate
JSON and avoid index errors in expand: wrap json.load(uploaded) in a try/except
to catch JSONDecodeError and return/raise a controlled validation error, verify
the decoded object is the expected list-of-lists structure before calling
expand, and inside Grid.expand change the name-lookup to a safe pattern (e.g.,
check membership of col in self.component_names or use a dict mapping
name->option and skip unknown IDs) rather than calling
self.component_names.index(col) which raises ValueError; ensure expand returns a
consistent validated structure (or raises a clear validation error) instead of
letting runtime exceptions propagate.

[coderabbitai]

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

src/workflow/FileManager.py (1)

535-542: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

result_path() should enforce parquet-only contract.

result_path() is documented/typed to return a parquet path string, but it currently forwards whatever get_results(..., as_path=True) returns. For non-.pq tags, that can be a loaded Python object, which breaks the method contract and can cause downstream data_path= misuse.

Proposed fix

 def result_path(self, dataset_id: str, name_tag: str) -> str:
@@
-    return self.get_results(dataset_id, [name_tag], as_path=True)[name_tag]
+    value = self.get_results(dataset_id, [name_tag], as_path=True)[name_tag]
+    if not isinstance(value, str):
+        raise KeyError(
+            f"{name_tag} for {dataset_id} is not a parquet-backed result path"
+        )
+    return value

🤖 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 `@src/workflow/FileManager.py` around lines 535 - 542, The result_path method
currently returns whatever get_results(..., as_path=True)[name_tag] yields,
which can be a non-path object; change result_path to fetch value =
self.get_results(dataset_id, [name_tag], as_path=True)[name_tag], then validate
that value is a string path ending with ".pq" (and keep letting KeyError bubble
if the tag is missing); if the value is not a string or does not end with ".pq"
raise a TypeError with a clear message indicating result_path enforces a parquet
path contract. Ensure references inside the method remain to result_path and
get_results so the behavior is localized.

.github/workflows/unit-tests.yml (1)

44-49: ⚠️ Potential issue | 🟠 Major

Exclude openms-insight from the subsequent requirements.txt install.

requirements.txt includes openms-insight==0.1.15 (line 62), but the workflow already installs the built local /tmp/openms-insight. Leaving the pin in place can still make pip install -r requirements.txt try to resolve openms-insight again (despite it requiring the prebuilt/mirrored js-component/dist to be installable in this repo).

Suggested fix

-          pip install -r requirements.txt
+          grep -vi 'openms-insight' requirements.txt > /tmp/requirements-no-insight.txt
+          pip install -r /tmp/requirements-no-insight.txt

🤖 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 @.github/workflows/unit-tests.yml around lines 44 - 49, The workflow
currently installs the local wheel with "pip install /tmp/openms-insight" and
then re-runs "pip install -r requirements.txt" which contains a pinned
openms-insight entry; modify the job in .github/workflows/unit-tests.yml so the
requirements install does not attempt to reinstall openms-insight — e.g.,
generate or use a filtered requirements file before running "pip install -r
requirements.txt" or run pip install with a constraint/exclude to skip
"openms-insight==0.1.15"; ensure the change targets the sequence around the
existing "pip install /tmp/openms-insight" and "pip install -r requirements.txt"
commands so only the local package is installed.

Source: Learnings

🤖 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 @.github/workflows/unit-tests.yml:
- Around line 27-30: The workflow is using the mutable tag actions/setup-node@v4
which weakens CI supply-chain security; update the uses line in
.github/workflows/unit-tests.yml to pin to a specific commit SHA for
actions/setup-node (replace actions/setup-node@v4 with
actions/setup-node@<full-commit-sha>), keeping the existing node-version setting
intact and using the commit SHA from the official actions/setup-node repository
release you trust.
- Around line 38-39: The CI job currently clones the mutable branch reference
"claude/kind-heisenberg-u6dVm" via the git clone command and can drift; change
the workflow to fetch and checkout a specific immutable commit SHA instead of
relying on -b branch (or add an explicit git checkout <COMMIT_SHA> after clone),
and apply the same hardening to the Dockerfile/Dockerfile.arm flow that consumes
insight-ref.json by resolving and pinning the referenced commit SHA before
building; update references to the git clone invocation and the insight-ref.json
fetch so all checkouts use a concrete commit SHA rather than a branch name.

---

Outside diff comments:
In @.github/workflows/unit-tests.yml:
- Around line 44-49: The workflow currently installs the local wheel with "pip
install /tmp/openms-insight" and then re-runs "pip install -r requirements.txt"
which contains a pinned openms-insight entry; modify the job in
.github/workflows/unit-tests.yml so the requirements install does not attempt to
reinstall openms-insight — e.g., generate or use a filtered requirements file
before running "pip install -r requirements.txt" or run pip install with a
constraint/exclude to skip "openms-insight==0.1.15"; ensure the change targets
the sequence around the existing "pip install /tmp/openms-insight" and "pip
install -r requirements.txt" commands so only the local package is installed.

In `@src/workflow/FileManager.py`:
- Around line 535-542: The result_path method currently returns whatever
get_results(..., as_path=True)[name_tag] yields, which can be a non-path object;
change result_path to fetch value = self.get_results(dataset_id, [name_tag],
as_path=True)[name_tag], then validate that value is a string path ending with
".pq" (and keep letting KeyError bubble if the tag is missing); if the value is
not a string or does not end with ".pq" raise a TypeError with a clear message
indicating result_path enforces a parquet path contract. Ensure references
inside the method remain to result_path and get_results so the behavior is
localized.

🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5cf58efb-9598-46e6-8c10-83db94c2b574

📥 Commits

Reviewing files that changed from the base of the PR and between de4b7d5 and 94ed99a.

📒 Files selected for processing (7)

• .github/workflows/unit-tests.yml
• content/FLASHDeconv/FLASHDeconvViewer.py
• content/FLASHQuant/FLASHQuantViewer.py
• content/FLASHTnT/FLASHTnTViewer.py
• src/render/render.py
• src/render/schema.py
• src/workflow/FileManager.py

🚧 Files skipped from review as they are similar to previous changes (5)

• content/FLASHQuant/FLASHQuantViewer.py
• content/FLASHTnT/FLASHTnTViewer.py
• content/FLASHDeconv/FLASHDeconvViewer.py
• src/render/render.py
• src/render/schema.py

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Pin actions/setup-node to a full commit SHA.

.github/workflows/unit-tests.yml uses the mutable action tag actions/setup-node@v4, which weakens CI supply-chain integrity—pin it to a specific commit SHA instead of v4.

🧰 Tools

🪛 zizmor (1.25.2)

[error] 28-28: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)

(unpinned-uses)

🤖 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 @.github/workflows/unit-tests.yml around lines 27 - 30, The workflow is using
the mutable tag actions/setup-node@v4 which weakens CI supply-chain security;
update the uses line in .github/workflows/unit-tests.yml to pin to a specific
commit SHA for actions/setup-node (replace actions/setup-node@v4 with
actions/setup-node@<full-commit-sha>), keeping the existing node-version setting
intact and using the commit SHA from the official actions/setup-node repository
release you trust.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate and print the relevant section of the workflow around the cited lines
FILE=".github/workflows/unit-tests.yml"
if [ ! -f "$FILE" ]; then
  echo "Missing $FILE"
  exit 1
fi

nl -ba "$FILE" | sed -n '1,120p'
echo "----"
nl -ba "$FILE" | sed -n '20,80p'
echo "----"

# Also search for any other openms-insight clone/install logic in the repo
rg -n "openms-insight|/tmp/openms-insight|npm run build|js-component/dist|requirements\.txt" .github/workflows/unit-tests.yml .claude/hooks/session-start.sh Dockerfile* . || true

Repository: OpenMS/FLASHApp

Length of output: 103

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE=".github/workflows/unit-tests.yml"
if [ ! -f "$FILE" ]; then
  echo "Missing $FILE"
  exit 1
fi

echo "---- $FILE (lines ~20-80) ----"
awk 'NR>=20 && NR<=80 {printf "%5d %s\n", NR, $0}' "$FILE"

echo "---- matches for openms-insight related logic ----"
rg -n "openms-insight|/tmp/openms-insight|js-component/dist|npm run build|requirements\.txt" .github/workflows .claude/hooks Dockerfile* 2>/dev/null || true

echo "---- any checkout/pin patterns in the workflow ----"
rg -n "git clone|checkout .*sha|checkout --detach|fetch --depth|--depth 1" .github/workflows/unit-tests.yml 2>/dev/null || true

Repository: OpenMS/FLASHApp

Length of output: 5956

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

for f in Dockerfile Dockerfile.arm; do
  echo "===== $f (around insight clone) ====="
  [ -f "$f" ] || continue
  # print likely relevant region
  awk 'NR>=60 && NR<=160 {printf "%5d %s\n", NR, $0}' "$f"
  echo
done

echo "===== search for insight-ref.json usage ====="
rg -n "insight-ref\.json|INSIGHT_BRANCH|openms-insight" Dockerfile Dockerfile.arm

Repository: OpenMS/FLASHApp

Length of output: 12567

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n "insight-ref\.json|insight-ref|INSIGHT_BRANCH=.*|git.*checkout.*openms-insight" -S . \
  || true

# Specifically ensure there is no follow-up that extracts a SHA from insight-ref.json
rg -n "jq |\.object\.sha|\"sha\"|grep .*insight-ref\.json|cat .*insight-ref\.json" Dockerfile Dockerfile.arm .github/workflows .claude/hooks 2>/dev/null || true

# List any other workflows that clone openms-insight
rg -n "openms-insight\.git|/tmp/openms-insight|openms_insight/js-component/dist" .github/workflows .claude/hooks Dockerfile* 2>/dev/null || true

Repository: OpenMS/FLASHApp

Length of output: 2552

---

Use an immutable openms-insight revision in CI checkout.

.github/workflows/unit-tests.yml clones claude/kind-heisenberg-u6dVm via git clone -b ... --depth 1 with no commit SHA pinning, so CI results can silently drift as the branch moves. The same mutable-branch pattern is also used in Dockerfile/Dockerfile.arm (the insight-ref.json fetch is not followed by a SHA checkout).

Suggested hardening

-          git clone -b claude/kind-heisenberg-u6dVm --single-branch --depth 1 \
-            https://github.com/t0mdavid-m/openms-insight.git /tmp/openms-insight
+          git clone https://github.com/t0mdavid-m/openms-insight.git /tmp/openms-insight
+          git -C /tmp/openms-insight checkout <pinned-openms-insight-commit-sha>

🤖 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 @.github/workflows/unit-tests.yml around lines 38 - 39, The CI job currently
clones the mutable branch reference "claude/kind-heisenberg-u6dVm" via the git
clone command and can drift; change the workflow to fetch and checkout a
specific immutable commit SHA instead of relying on -b branch (or add an
explicit git checkout <COMMIT_SHA> after clone), and apply the same hardening to
the Dockerfile/Dockerfile.arm flow that consumes insight-ref.json by resolving
and pinning the referenced commit SHA before building; update references to the
git clone invocation and the insight-ref.json fetch so all checkouts use a
concrete commit SHA rather than a branch name.