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

  • Viewer pages rebuilt to use a unified linked-grid with per-experiment layout editor, value-based selection, reusable Insight builders, and best-effort cache warming.

• Documentation

  • Added Phase 3 migration plan, review guidance, and convergence tooling docs.

• Tests

  • Large new suites validating builders, schema transforms, linked-grid wiring, and cache warming.

• Chores

  • Runtime now installs openms-insight; legacy local JS submodule removed.

[coderabbitai]

Warning

Review limit reached

@t0mdavid-m, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 35 minutes and 19 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more credits in the billing tab to continue.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 70be6150-9827-41e6-8fa9-77b63abf905c

📥 Commits

Reviewing files that changed from the base of the PR and between 988ff11 and 30d6f59.

📒 Files selected for processing (2)

• src/render/render.py
• tests/test_render_builders.py

📝 Walkthrough

Walkthrough

This PR migrates FLASHApp viewers to OpenMS‑Insight: adds tidy-parquet schema adapters, a builder factory, a reusable linked-grid + LayoutManager, viewer rewires for lazy per-dataset rendering, Docker/CI integration to build/install openms‑insight, migration governance scripts/config, and comprehensive tests/fixtures.

Changes

FLASHApp Phase 3 Migration

Layer / File(s)	Summary
Phase 3 governance, gates, and review harness migration/README.md, migration/REVIEW.md, migration/specs/PHASE3_PLAN.md, migration/units.yaml, migration/nondivergence.py, migration/run_review.py	Adds Phase 3 plan/specs, units registry, nondivergence comparator, and a review ledger CLI with record/gate/report.
Docker & CI: build and install openms-insight Dockerfile, Dockerfile.arm, requirements.txt, .github/workflows/unit-tests.yml	Adds Node 21 insight-build stage to build OpenMS‑Insight JS dist and pip-install the package during image build; removes legacy local js-build artifact copy; pins openms-insight==0.1.15; extends unit-tests workflow to build/install openms-insight before pytest.
FileManager: parquet path access for Insight src/workflow/FileManager.py	Adds as_path precedence to get_results(), guards missing DB rows, and adds result_path() helper returning on-disk parquet path for a single (dataset_id, name_tag).
Schema Adapter: build_insight_caches (tidy Parquet) src/render/schema.py	New Polars-based adapter with primitives to explode list/nested columns, comma-split traces, normalize KDEs, mint stable IDs, and tool-specific builders producing idempotent tidy Parquet outputs for flashdeconv/flashtnt/flashquant behind build_insight_caches().
Render Builder Factory: make_builders & warm_insight_caches src/render/render.py	Replaces index/state-tracking renderer with make_builders() returning zero-arg component factories, defines table column chrome and sequence-view wiring, and adds warm_insight_caches() for best-effort cache warm-up.
Frozen Grid & LayoutManager src/view/grid.py	Adds render_linked_grid() (shared StateManager for cross-panel interactivity), Store protocol, and LayoutManager UI for editing/validating/persisting layouts with JSON import/export; adds show_linked_grid helper in src/common/common.py.
Viewer rewires: FLASHDeconv / FLASHQuant / FLASHTnT content/FLASHDeconv/FLASHDeconvViewer.py, content/FLASHQuant/FLASHQuantViewer.py, content/FLASHTnT/FLASHTnTViewer.py	Rewrites viewers to use id-based per-experiment selectboxes, lazily call build_insight_caches() and make_builders(), render via show_linked_grid(), persist layout metadata, and add FLASHTnT best-per-spectrum/settings controls.
Workflow hooks: cache warming src/Workflow.py	Adds non-fatal warm_insight_caches() calls in TagWorkflow and DeconvWorkflow guarded by try/except.
Test infra: fixtures and end-to-end coverage tests/conftest.py, tests/test_render_builders.py, tests/test_render_schema.py	Adds Streamlit-mocking fixtures, synthetic oracle caches, and comprehensive tests for schema primitives, build_insight_caches() idempotency/edge-cases, make_builders() outputs, render_linked_grid wiring, table chrome, highlighting, and warm-insight behavior.
Legacy stack removals 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/initialize/update utilities/hash helper and selection-clear tests; removes submodule entry for the old Vue component and stops copying its dist artifact.

Possibly Related PRs

• OpenMS/FLASHApp#22: Also touches the openms-streamlit-vue-component submodule metadata.
• OpenMS/FLASHApp#57: Related to previous viewer selection UI changes this migration replaces.
• OpenMS/FLASHApp#21: Earlier viewer selection/validation work that overlaps with the refactor.

Poem

🐇 I hopped through parquet rows and frozen grid delight,
Swapped index keys for values, tuned the builder light.
Vue crumbs tucked away, Insight bakes the view,
Layouts freeze, caches warm — the panels hum anew.
Phase Three stitched, the rabbit claps tonight.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 59.21% 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 PR title clearly and concisely summarizes the main objective: rebuilding FLASHApp viewers on OpenMS-Insight using a frozen template from Phase 3 of the migration.
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 unit tests (beta)

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

[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.