Merge Template

[t0mdavid-m]

Summary by CodeRabbit

Release Notes

• New Features

  • Added Matomo analytics integration with GDPR consent support.

• Bug Fixes

  • Improved error handling for file operations in tests.

• Chores

  • Updated CI/CD workflows for improved deployment configuration.
  • Enhanced test isolation and setup procedures.

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request integrates Matomo analytics support across the application, updates CI/CD workflows to use direct Python setup instead of conda, removes server configuration from local deployments, updates analytics settings, and improves test isolation and symlink handling.

Changes

Cohort / File(s)	Summary
CI/CD Workflow Updates .github/workflows/build-windows-executable-app.yaml, .github/workflows/test-win-exe-w-embed-py.yaml	Add steps to remove server address from local deployment config by filtering address lines from streamlit configuration.
Python Setup Migration .github/workflows/ci.yml	Replace conda/miniconda setup with actions/setup-python@v4, remove OpenMS conda installation, and update Python version testing comments.
Matomo Analytics Integration gdpr_consent/src/main.ts, hooks/hook-analytics.py, src/common/captcha_.py, src/common/common.py	Add Matomo analytics service configuration throughout consent flow: new matomo_head() function for script injection, conditional Matomo service addition to Klaro config, Matomo consent handling in GDPR gate and page setup.
Analytics Configuration .streamlit/config.toml, settings.json	Remove server address, enableCORS, and enableXsrfProtection from Streamlit config; add Matomo CDN URL and tag to analytics settings.
Test Improvements test_gui.py, tests/test_parameter_presets.py, tests/test_topp_workflow_parameter.py	Wrap file-copy operations to handle symlink errors; improve test isolation by mocking streamlit before ParameterManager import; remove pyopenms mock setup and its validation test.

Poem

🐰 Matomo hops into the fray,
With consent flows lighting the way,
CI pipelines swift, configs so clean,
Analytics flow like never seen!
Tests stand isolated and bright,
This PR makes everything right! 🌟

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The pull request title 'Merge Template' is vague and generic, providing no meaningful information about the actual changes made in the changeset.	Replace the title with a descriptive summary of the main changes, such as 'Add Matomo analytics integration and CI/CD workflow improvements' or similar, to clearly communicate the purpose of the pull request.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

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

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch merge_template

📝 Coding Plan

• Generate coding plan for human review comments

---

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

Tip

CodeRabbit can use OpenGrep to find security vulnerabilities and bugs across 17+ programming languages.

OpenGrep is compatible with Semgrep configurations. Add an opengrep.yml or semgrep.yml configuration file to your project to enable OpenGrep analysis.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

.github/workflows/build-windows-executable-app.yaml (1)

314-317: Harden the address filter to handle indented TOML keys.

At Line 316, -notmatch '^address' won’t catch lines like address = .... Use a whitespace-tolerant assignment pattern.

♻️ Suggested patch

-        (Get-Content streamlit_exe/.streamlit/config.toml) -notmatch '^address' | Set-Content streamlit_exe/.streamlit/config.toml
+        (Get-Content streamlit_exe/.streamlit/config.toml) -notmatch '^\s*address\s*=' | Set-Content streamlit_exe/.streamlit/config.toml

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/build-windows-executable-app.yaml around lines 314 - 317,
Update the PowerShell pipeline that removes the Streamlit server address by
changing the regex filter (currently using -notmatch '^address') to a
whitespace-tolerant assignment pattern such as '\s*address\s*=' so indented keys
like "  address = ..." are matched and removed; locate the Get-Content ... |
Set-Content pipeline and replace the '^address' pattern with the new
'\s*address\s*=' regex (keeping the -notmatch operator) so indented TOML address
lines are filtered out.

.github/workflows/ci.yml (1)

15-15: Consider upgrading to actions/setup-python@v5.

The current version (@v4) works with ubuntu-latest, but @v5 is the current recommended version per GitHub's Python workflow documentation. This is a maintenance best-practice update; no blocking issues exist.

🔧 Suggested patch

-    - uses: actions/setup-python@v4
+    - uses: actions/setup-python@v5

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In @.github/workflows/ci.yml at line 15, Update the GitHub Actions step using
the Python setup action by replacing the current reference
"actions/setup-python@v4" with "actions/setup-python@v5"; locate the workflow
step that uses actions/setup-python@v4 and change the version tag to `@v5` (no
other behavior changes required unless you rely on new inputs, in which case
adjust the step inputs accordingly).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/common/common.py`:
- Around line 408-409: Replace direct dict indexing and equality comparisons for
tracking consent with truthy .get() checks: wherever you currently check
st.session_state.tracking_consent["matomo"] == True (and the similar checks for
other analytics at the nearby analytics checks), change them to use
st.session_state.tracking_consent.get("matomo") (or .get("<provider>") for the
other providers) and combine that with the existing analytics enabled condition
(e.g., keep st.session_state.settings["analytics"]["matomo"]["enabled"]) so the
condition becomes a safe truthy check that avoids KeyError and Ruff E712 issues.

In `@tests/test_parameter_presets.py`:
- Around line 30-38: The current test temporarily injects mock_streamlit into
sys.modules and then imports ParameterManager but does not guarantee restoration
if the import raises; wrap the import of ParameterManager (the code that uses
mock_streamlit) in a try/finally where you set
sys.modules['streamlit']=mock_streamlit before the try, perform from
src.workflow.ParameterManager import ParameterManager inside the try, and in the
finally restore sys.modules to _original_streamlit (or pop if None) using the
existing _original_streamlit and mock_streamlit identifiers so the mocked module
cannot leak into other tests.

---

Nitpick comments:
In @.github/workflows/build-windows-executable-app.yaml:
- Around line 314-317: Update the PowerShell pipeline that removes the Streamlit
server address by changing the regex filter (currently using -notmatch
'^address') to a whitespace-tolerant assignment pattern such as '\s*address\s*='
so indented keys like "  address = ..." are matched and removed; locate the
Get-Content ... | Set-Content pipeline and replace the '^address' pattern with
the new '\s*address\s*=' regex (keeping the -notmatch operator) so indented TOML
address lines are filtered out.

In @.github/workflows/ci.yml:
- Line 15: Update the GitHub Actions step using the Python setup action by
replacing the current reference "actions/setup-python@v4" with
"actions/setup-python@v5"; locate the workflow step that uses
actions/setup-python@v4 and change the version tag to `@v5` (no other behavior
changes required unless you rely on new inputs, in which case adjust the step
inputs accordingly).

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bf27e196-b2b3-40b1-8ab9-c7c9ace205cb

📥 Commits

Reviewing files that changed from the base of the PR and between 231a0fc and d00fcd3.

⛔ Files ignored due to path filters (1)

• gdpr_consent/dist/bundle.js is excluded by !**/dist/**

📒 Files selected for processing (12)

• .github/workflows/build-windows-executable-app.yaml
• .github/workflows/ci.yml
• .github/workflows/test-win-exe-w-embed-py.yaml
• .streamlit/config.toml
• gdpr_consent/src/main.ts
• hooks/hook-analytics.py
• settings.json
• src/common/captcha_.py
• src/common/common.py
• test_gui.py
• tests/test_parameter_presets.py
• tests/test_topp_workflow_parameter.py

💤 Files with no reviewable changes (2)

• .streamlit/config.toml
• tests/test_topp_workflow_parameter.py

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

cat -n src/common/common.py | sed -n '405,415p'

Repository: OpenMS/quantms-web

Length of output: 494

---

🏁 Script executed:

rg "tracking_consent" src/common/common.py -B 2 -A 2

Repository: OpenMS/quantms-web

Length of output: 935

---

🏁 Script executed:

rg "tracking_consent\s*=" src/common/common.py -B 3 -A 3 | head -50

Repository: OpenMS/quantms-web

Length of output: 409

---

🏁 Script executed:

rg "tracking_consent\[" --type py -B 1 -A 1

Repository: OpenMS/quantms-web

Length of output: 744

---

🏁 Script executed:

rg "tracking_consent" src/ --type py -B 2 -A 2 | grep -E "(matomo|google-analytics|piwik-pro)" | head -20

Repository: OpenMS/quantms-web

Length of output: 925

---

🏁 Script executed:

rg "tracking_consent" src/ --type py -g "!common.py" -B 3 -A 3

Repository: OpenMS/quantms-web

Length of output: 1178

---

🏁 Script executed:

find . -name "gdpr_consent*" -o -name "*gdpr*" | head -20

Repository: OpenMS/quantms-web

Length of output: 76

---

🏁 Script executed:

ls -la gdpr_consent/ && find gdpr_consent -type f -name "*.py" -o -name "*.js" -o -name "*.ts" -o -name "*.tsx" | head -20

Repository: OpenMS/quantms-web

Length of output: 763

---

🏁 Script executed:

cat gdpr_consent/src/main.ts

Repository: OpenMS/quantms-web

Length of output: 4341

---

Use a truthy .get() consent check instead of direct dict indexing with equality comparison.

At line 409, directly accessing ["matomo"] with == True is non-idiomatic (Ruff E712) and risks KeyError if the key is absent. The suggested .get("matomo") approach is safer and more Pythonic. This pattern should be applied consistently to the other analytics checks at lines 398 and 403 as well.

✅ Suggested patch

-        if (st.session_state.settings["analytics"]["matomo"]["enabled"]) and (
-            st.session_state.tracking_consent["matomo"] == True
-        ):
+        if st.session_state.settings["analytics"]["matomo"]["enabled"] and (
+            st.session_state.tracking_consent.get("matomo")
+        ):

🧰 Tools

🪛 Ruff (0.15.5)

[error] 409-409: Avoid equality comparisons to True; use st.session_state.tracking_consent["matomo"]: for truth checks

Replace with st.session_state.tracking_consent["matomo"]

(E712)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/common/common.py` around lines 408 - 409, Replace direct dict indexing
and equality comparisons for tracking consent with truthy .get() checks:
wherever you currently check st.session_state.tracking_consent["matomo"] == True
(and the similar checks for other analytics at the nearby analytics checks),
change them to use st.session_state.tracking_consent.get("matomo") (or
.get("<provider>") for the other providers) and combine that with the existing
analytics enabled condition (e.g., keep
st.session_state.settings["analytics"]["matomo"]["enabled"]) so the condition
becomes a safe truthy check that avoids KeyError and Ruff E712 issues.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Guard sys.modules restoration with try/finally to avoid cross-test contamination.

If the import on Line 33 fails, Lines 35-38 never run, leaving the mocked streamlit globally installed for subsequent tests.

🛡️ Suggested patch

 _original_streamlit = sys.modules.get('streamlit')
-sys.modules['streamlit'] = mock_streamlit
-
-from src.workflow.ParameterManager import ParameterManager
-
-if _original_streamlit is not None:
-    sys.modules['streamlit'] = _original_streamlit
-else:
-    sys.modules.pop('streamlit', None)
+try:
+    sys.modules['streamlit'] = mock_streamlit
+    from src.workflow.ParameterManager import ParameterManager
+finally:
+    if _original_streamlit is not None:
+        sys.modules['streamlit'] = _original_streamlit
+    else:
+        sys.modules.pop('streamlit', None)

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	_original_streamlit = sys.modules.get('streamlit')
	sys.modules['streamlit'] = mock_streamlit
	# Now import after mocks are set up
	from src.workflow.ParameterManager import ParameterManager
	if _original_streamlit is not None:
	sys.modules['streamlit'] = _original_streamlit
	else:
	sys.modules.pop('streamlit', None)
	_original_streamlit = sys.modules.get('streamlit')
	try:
	sys.modules['streamlit'] = mock_streamlit
	from src.workflow.ParameterManager import ParameterManager
	finally:
	if _original_streamlit is not None:
	sys.modules['streamlit'] = _original_streamlit
	else:
	sys.modules.pop('streamlit', None)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/test_parameter_presets.py` around lines 30 - 38, The current test
temporarily injects mock_streamlit into sys.modules and then imports
ParameterManager but does not guarantee restoration if the import raises; wrap
the import of ParameterManager (the code that uses mock_streamlit) in a
try/finally where you set sys.modules['streamlit']=mock_streamlit before the
try, perform from src.workflow.ParameterManager import ParameterManager inside
the try, and in the finally restore sys.modules to _original_streamlit (or pop
if None) using the existing _original_streamlit and mock_streamlit identifiers
so the mocked module cannot leak into other tests.