Address pr issues by caruizelembioinfo · Pull Request #12 · Elembio/MultiQC

caruizelembioinfo · 2025-12-18T04:02:23Z

Description of Changes

This PR addresses code review feedback for the bases2fastq module with the following improvements:

Bug Fixes

Fixed AnalysisID None handling: Added null checks before slicing AnalysisID[0:4] to prevent TypeError when AnalysisID is missing (extracted into _extract_run_analysis_name() helper)
Fixed color generation range: The range() for generating extra colors was already correct in the refactored code (range(len(self.palette), len(merged_groups)))

Code Quality Improvements

Reduced `init` Complexity

Extracted initialization logic into focused helper methods:
- _init_data_structures() - Initialize all data structures
- _parse_and_validate_data() - Parse input data and validate samples
- _determine_summary_path() - Determine run/project/combined level
- _select_data_by_summary_path() - Route to appropriate data sources
- _setup_colors() - Set up color schemes for groups and samples
- _generate_plots() - Create all report sections and plots

Eliminated Duplicate Code

Created _extract_run_analysis_name() helper to replace 6 repeated instances of RunName/AnalysisID extraction and validation

Improved Error Handling

Consolidated split log messages (multiple log.error() + log.debug() calls) into single multi-line error messages with all context
Added try/except blocks around file operations with json.JSONDecodeError and OSError handling

Performance Improvements

File caching: Added _read_json_file() method with caching to avoid reading RunStats.json and RunManifest.json multiple times
List concatenation: Replaced sum([...], []) with itertools.chain.from_iterable() for O(n) instead of O(n²) performance

Security Improvements

Path traversal validation: Added _validate_path() helper to ensure file paths don't escape expected directories
Refactored ../../RunManifest.json paths to use explicit Path(directory).parent.parent with validation

Code Style

Converted all string formatting to f-strings (removed .format() usage)
Added proper type hints to all functions (Dict[str, Any], Optional[str], Tuple[...], List[Callable], etc.)
Added Callable and Tuple to typing imports

Configuration

Made MIN_POLONIES configurable via config.bases2fastq_config["min_polonies"]

Documentation

Added comprehensive class docstring with:
- Data flow overview (run/project/sample levels)
- ASCII diagram of parsing flow
- Data structures explanation
- Sample naming convention
Added detailed docstrings to all parsing methods explaining data flow
Added inline comments to _init_data_structures() explaining each data structure's purpose

* Initial structural work to use SCSS for CSS compilation, with new Bootstrap 5.3 * Silence depreciation warnings in the CSS compilation so as not to pollute pre-commit See twbs/bootstrap#40962 - can hopefully remove these again in a future version of Bootstrap * Initial structural work for bootstrap5 JS - use Vite for build * First stab at converting BS3 -> BS5 for HTML in default template * Document config.development * Updates for other templates that use default as a base * Update CLI docs for --development now that vite minimises * Fix: Make callAfterDecompressed, notEmptyObj, and mqc_plots globally accessible The JavaScript bundler (Vite) encapsulates variables in module scope, making them inaccessible to external scripts like module-specific JavaScript files (e.g., multiqc_fastqc.js). This caused 'callAfterDecompressed is not defined' errors. Fixed by explicitly assigning these variables to the window object: - window.callAfterDecompressed (used by module JS files to register callbacks) - window.notEmptyObj (utility function used across modules) - window.mqc_plots (global plot data accessed by multiple files) This ensures backward compatibility with module JavaScript files that expect these as global variables. * Make common JS functions global scope * Add file for bootstrap variables. Overwrite table background colour. * Get the toolbox to show properly, use md icons instead of emoji * Start converting hardcoded hex values in custom.css to BS CSS variables * Minor cleanup * Smaller base font size. Outline buttons instead of secondary. * Update how SVG icons are handled, remove all traces of glyphicons * Fix scoping errors in JS for AI summaries * Fix buttons that launch modal windows * New icon for violin plots * Improve pre-commit build step * Fix old BS3 form-inline forms * Start work on toolbox: use offcanvas and BS list group * Refactor: Split toolbox javascript into multiple files * Prettier: also format jinja template files * Prettier formatting * Fix prettier error * Initial effort: dark mode toggle * Replace toast JS library with native Bootstrap toasts * Nav: make version a link, go to top of page * Fix 'scroll to top' button * Start work on fixing the toolbox * Working on fixing up the sidebar * Fix toolbox on mobile * Seqera SVG logos that work in dark mode * Fix up the footer styling and CSS * Start small-scale tweaking: buttons * Make plotly plots respond to theme toggle * Light-mode plotly python config for exports * Transparent backgrounds for HTML by default, white only for export * Work on custom themes by having 2 bootswatch examples * simplify vite now that the old toast library is gone * custom.scss - tidy up without changes * Directly use bootstrap SCSS vars, not var() * Custom CSS - remove nearly all hardcoded colour and replace with BS variables * Switch hardcoded border attributes * Back to css variables to handle dark mode switch * Don't hardcode fonts * Start cleaning custom css. Restyle toolbox: about * No hardcoded breakpoints. More cruft removal * Clean up some prefixes, some css * Nicer toolbox buttons in mobile nav * Update / fix the icon for the side nav toggle button * More cruft cleaning in custom.scss * Tidy up footer CSS * Cleanss * More tweaking till my brain melts * Finally figure out how to make child theming work properly * Bunch of work on getting themes to work properly, improvements to plotly dark mode and more * Nicer tooltips in dark mode * Violin plot axes coloured properly on first render * Dark mode improvements for the violin plot * Violin plot tooltips - dark mode * Fix toolbar, allow overflow scroll. Tidy assets. * Headings top-margin * Load Seqera AI icon from single file, prefix localstorage dark mode toggle with mqc * Start working through the toolbox HTML and CSS * Break toolbox into separate template files * Update toolbox: hide samples * Fix AI toolbox errors introduced in AI refactoring, improve layout * Move AI logos into asset files. Make them work in dark mode. * Clean up a lot of the AI summary styling * Update bootstrap tooltips, more AI summaries clean up * Fix / improve regex help modal. Clean up a load of old CSS * Fix / update export toolbox pane * Fix / update export toolbox pane * Update the citations toolbox tab * Final tidying on custom.scss * Start working on the fastqc status bars * Fix FastQC progress, popover and tooling. And DOIs and some other stuff. * Make FastQC statuses a generic MultiQC feature * Table scatter plots: fix for dark mode * Remove unused javascript variables * Small refactor theme watcher. Remove update plots call on first load * Fix bug for dark mode on page load * Fix plot export button functionality * Remove last reminants of glyphicons * Rename theme 'crazy' to 'disco' * Add back the old template, called 'original' * Proper spacing in welcome header * switch to custom plotly bundle in bootstrap-upgrade branch (MultiQC#3365) * Don't import unused bootstrap SCSS * Add description of plotly custom bundle build * Remove typo $ symbol * Several small dark mode fixes * Mixture of module-specific colour fixes for dark mode * Fix expand / collapse table button * Disco: remove SCSS imports for unused bootstrap components * AI summaries: format highlighted text even when not a sample ID * AI summary highlight colours that work in both themes * Fix 'scroll to top' button * Disco SCSS formatting * Add some custom CSS to 'original' to try to cater for Bootstrap 5 class names in main MultiQC * Revert changes in templates that inherit from 'original' * Add a .gitattributes to mark generated files * Run pre-commit / prettier on all files * Try to fix CI tests * Add prettier itself to the hook dependencies * Pin prettier dep version * Try fixing CI error with pre-commit jinja plugin See https://stackoverflow.com/questions/78708497/prettier-plugins-not-found-with-pre-commit * Docs tweaks * Ok fine maybe that does need single quotes after all? --------- Co-authored-by: Matthias Hörtenhuber <mashehu@users.noreply.github.com>

Fixes MultiQC#3355

Fixes MultiQC#3358

Fixes MultiQC#3339

Closes MultiQC#3336

…ltiQC#3374) * docs: Document Docker image variants with and without PDF support Add documentation for the two Docker image variants introduced in PR MultiQC#3349: - Standard image (multiqc/multiqc:latest) - smaller, core functionality - PDF-enabled image (multiqc/multiqc:pdf-latest) - includes Pandoc and LaTeX Updated: - installation.md: Added Docker image variants section with examples - running_multiqc.md: Added tip about using PDF-enabled Docker image Co-authored-by: Phil Ewels <ewels@users.noreply.github.com> * [automated] Fix code linting --------- Co-authored-by: claude[bot] <41898282+claude[bot]@users.noreply.github.com> Co-authored-by: Phil Ewels <ewels@users.noreply.github.com> Co-authored-by: MultiQC Bot <multiqc-bot@seqera.io>

…3375) Bumps [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite) from 7.1.10 to 7.1.11. - [Release notes](https://github.com/vitejs/vite/releases) - [Changelog](https://github.com/vitejs/vite/blob/main/packages/vite/CHANGELOG.md) - [Commits](https://github.com/vitejs/vite/commits/v7.1.11/packages/vite) --- updated-dependencies: - dependency-name: vite dependency-version: 7.1.11 dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

…3368) Bumps [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite) from 7.0.0 to 7.0.8. - [Release notes](https://github.com/vitejs/vite/releases) - [Changelog](https://github.com/vitejs/vite/blob/v7.0.8/packages/vite/CHANGELOG.md) - [Commits](https://github.com/vitejs/vite/commits/v7.0.8/packages/vite) --- updated-dependencies: - dependency-name: vite dependency-version: 7.0.8 dependency-type: direct:development ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Move most of xenium into a plugin * Fix for python 3.8 * Test up to Python 3.14 * Log message about availability of plugin * Remove unneeded dependencies, remove orphaned function * Tidying up and testing * Update the xenium docs * Move more code out to the plugin * Log the software version for xenium

* Generate new screengrabs with rich-codex * Move screenshot back to the right place --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

@ewels

* refactor: move module triage system to Claude Code infrastructure Refactored the module request triage system from PR MultiQC#3324 to use Claude Code's skills and commands instead of inline workflow prompts. Changes: - Created .claude/docs/module-triage-system.md - comprehensive user guide - Created .claude/docs/module-triage-project-setup.md - maintainer docs - Simplified .github/CONTRIBUTING.md - brief section with link to docs - Updated .github/ISSUE_TEMPLATE/module-request.yml - removed required checkboxes for gamification, added friendly guidance - Simplified .github/workflows/automated-module-generation.yml - now references /new-module command - Created .github/workflows/module-requests.yml - references existing triaging-module-requests skill instead of inline prompts Benefits: - Workflows are now thin orchestration layers - Skills/commands are reusable and maintainable - Better documentation separation (user vs implementation) - Claude Code can auto-discover and use skills 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * refactor(workflows): simplify module-requests.yml workflow Further refactored module-requests.yml to be cleaner and more maintainable by trusting the triaging-module-requests skill to handle the logic. Changes: - Reduced Claude Code prompt from 35 lines to 5 lines (86% reduction) - Simplified verification logic from 47 to 30 lines (36% reduction) - Converted bash mode determination to JavaScript for consistency - Removed verbose console logging and redundant checks - Overall file size: 191 lines → 154 lines (19% reduction) Benefits: - Workflow trusts the skill instead of repeating instructions - All logic now in JavaScript via github-script (more maintainable) - Cleaner, more focused code - Same functionality with less duplication The workflow is now a proper thin orchestration layer that delegates to the skill, exactly as intended. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * feat(skills): add module triage skill and fix workflow Create triaging-module-requests skill following best practices and address PR review feedback from @ewels. ## Skill Implementation Created comprehensive module triage skill with progressive disclosure: - SKILL.md: Concise entry point with operation modes - scoring-criteria.md: Detailed 0-100 point rubric (5 categories) - github-actions.md: GitHub CLI operations guide - analysis-templates.md: Comment templates for all priority levels - scripts/fetch-tool-metrics.js: Node.js script for API calls Scoring system: - Tool Popularity (25): GitHub stars, maintenance - Package Downloads (15): PyPI/Conda/Bioconda metrics - Community Engagement (35): Reactions, comments, duplicates - Request Quality (20): Completeness, example files - Technical Feasibility (15): Output format, metrics clarity ## Workflow Fixes 1. **Comment out cron schedule** until tested via workflow_dispatch - Prevents automatic weekly runs during testing 2. **Remove redundant verification step** (deleted ~40 lines) - Job-level `if` already filters for 'module: new' label - Issue template adds label automatically 3. **Clarify mode behavior** with inline comments - Explains triage-all loops over all open issues - Documents analyze-single, triage-all, dry-run modes 4. **Fix .gitignore** - Changed from `.claude` to `.claude/settings.local.json` - Allows skill files to be tracked 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com> * comment out trigger for now * Cut a lot out of analysis-templates.md * More manual fixes for claude skill * Delete a lot of fetch-tool-metrics.js * [automated] Fix code linting --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Phil Ewels <phil.ewels@seqera.io> Co-authored-by: MultiQC Bot <multiqc-bot@seqera.io>

…rientation (MultiQC#3385) Fixes a bug where using x_lines or y_lines configuration with horizontal bar graphs would cause a KeyError when accessing autorangeoptions during axis swapping. When creating horizontal bar graphs, the code swaps the x and y axes. Previously, it only preserved minallowed/maxallowed in autorangeoptions for the new xaxis, but didn't preserve clipmin/clipmax from the original yaxis, nor did it copy autorangeoptions to the new yaxis at all. This caused issues when reference lines were configured, as they rely on proper autorangeoptions being set. Changes: - Copy autorangeoptions from original xaxis to new yaxis when swapping - Safely extract clipmin/clipmax from original yaxis when creating new xaxis - Use .get() with defaults to avoid KeyError if autorangeoptions doesn't exist 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com>

…t template

… command (MultiQC#3418)

Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Josh Chorlton <josh@bugseq.com> Co-authored-by: Phil Ewels <phil.ewels@seqera.io>

Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Josh Chorlton <josh@bugseq.com> Co-authored-by: Phil Ewels <phil.ewels@seqera.io> Co-authored-by: MultiQC Bot <multiqc-bot@seqera.io>

… count (MultiQC#3343) (MultiQC#3419) Co-authored-by: Claude <noreply@anthropic.com>

…QC#3421)

…ltiQC#3404) Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Phil Ewels <phil.ewels@seqera.io>

Co-authored-by: Claude <noreply@anthropic.com>

Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Phil Ewels <phil.ewels@seqera.io>

…" control (MultiQC#3423) Co-authored-by: LouisLeNezet <louislenezet@gmaio.com> Co-authored-by: Phil Ewels <phil.ewels@seqera.io>

Co-authored-by: Claude <noreply@anthropic.com>

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…#3433) Co-authored-by: Claude <noreply@anthropic.com>

Co-authored-by: Claude <noreply@anthropic.com>

…s_pr_issues

ewels and others added 30 commits October 22, 2025 19:51

Update Pydantic config for show_hide_buttons

c530ae4

Fixes MultiQC#3355

Fix NotADirectoryError when rerunning with --zip-data-dir and --force

3e3f496

Fixes MultiQC#3358

custom_plot_config - check field names before attempting to set.

c14c00e

Fixes MultiQC#3339

Remove noisy log message when trying to format violin plot numbers.

7f7c2a7

Closes MultiQC#3336

Claude PR review: only run on open, not every new commit

f3af968

Attempt to get claude PR reviews on PRs from forks

4538ac5

Claude review trigger: Try using artefacts instead

21ec6c1

Claud PR review: check out repo before trying to fetch PR info

b5980fc

Try to fix paths for claude PR review artefacts

6e2e4d1

Update module docs

5dadf0a

Add warning mdi icon

b5f4aca

Prep v1.32 release

72cf4e1

Rich codex (MultiQC#3377)

741ebad

* Generate new screengrabs with rich-codex * Move screenshot back to the right place --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Bump version to v1.33dev

4f6b40e

Remove invalid 'projects' from GHA workflow

fa69635

Add id-token GHA permissions

83e0c68

Let claude do more stuff for the module-requests

bde530d

Try to fix Claude CLI flags from hallucinations

651e9a1

Show full output

02b1ff6

Make the prompt more specific about posting results as a comment.

9bd3065

Try to get module triage to use issue labels properly

08830c7

Claude tweaks

d62fe61

Remove leftover unused plotly 2.27 js asset files from the new defaul…

2637bde

…t template

ewels and others added 29 commits December 5, 2025 12:01

Avoid Python 3.14.1 (MultiQC#3414)

675837b

Bump pre commit versions (MultiQC#3417)

408eeea

fastp: Add support for naming samples after --report_title in Fastp…

f238f41

… command (MultiQC#3418)

Fix variable assignment order in homer/tagdirectory.py (MultiQC#3381)

724f0b5

Lima: split delimiter is a tab, not any whitespace. (MultiQC#3395)

4855f5f

Custom content: support passing help text (MultiQC#3338)

2b4deb5

Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Josh Chorlton <josh@bugseq.com> Co-authored-by: Phil Ewels <phil.ewels@seqera.io>

General StatS: Add configurable help text (MultiQC#3341)

097bf80

Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Josh Chorlton <josh@bugseq.com> Co-authored-by: Phil Ewels <phil.ewels@seqera.io> Co-authored-by: MultiQC Bot <multiqc-bot@seqera.io>

Samtools coverage: Fix crash when parsing lines with unexpected field…

be3f0fc

… count (MultiQC#3343) (MultiQC#3419) Co-authored-by: Claude <noreply@anthropic.com>

Custom content: strip whitespace around categories for tsv,csv (Multi…

8292d05

…QC#3421)

Add sample_groups configuration for visual grouping in bar graphs (Mu…

373da0a

…ltiQC#3404) Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Phil Ewels <phil.ewels@seqera.io>

Add uv install method detection to version check (MultiQC#3422)

267bc08

Co-authored-by: Claude <noreply@anthropic.com>

Add RiboTish module for Ribo-seq quality metrics (MultiQC#3384)

29ba13b

Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Phil Ewels <phil.ewels@seqera.io>

Add decimal format for "Glimpse" and add "axis_controlled_by_switches…

5ab9b20

…" control (MultiQC#3423) Co-authored-by: LouisLeNezet <louislenezet@gmaio.com> Co-authored-by: Phil Ewels <phil.ewels@seqera.io>

Address discussion in PR MultiQC#3345 for BISCUIT module (MultiQC#3426)

b88e365

Update Seqera AI chat URL (MultiQC#3415)

2ad29c0

Add version fetching for HiCUP, QoRTs, QualiMap, RNA-SeQC (MultiQC#3420)

7a10455

Co-authored-by: Claude <noreply@anthropic.com>

Changes for releasing v1.33 (MultiQC#3428)

5953b54

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Bump to v1.34dev

8ff6815

Bump default AI models for Anthropic (MultiQC#3430)

6c31244

export_plots_timeout: Add docs and increase default to 60s (MultiQC…

5bf1cbb

…#3433) Co-authored-by: Claude <noreply@anthropic.com>

fix table scroll styling with sticky headers (MultiQC#3437)

d53ee14

Fix SNPsplit module datetime serialization error (MultiQC#3444)

8a5d648

Co-authored-by: Claude <noreply@anthropic.com>

Fix incorrect plot title in umicollapse module (MultiQC#3446)

891ed30

Addressed pressing comments

480d2c8

Address performance comments

348c14f

Added documentation and better path handling

e2f347e

Linting

16cdc32

Merge branch 'MultiQC:main' into main

d058664

Merge branch 'main' of https://github.com/Elembio/MultiQC into addres…

d1bffdf

…s_pr_issues

caruizelembioinfo merged commit ff776bc into master Jan 8, 2026
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Address pr issues#12

Address pr issues#12
caruizelembioinfo merged 69 commits intomasterfrom
address_pr_issues

caruizelembioinfo commented Dec 18, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

Conversation

caruizelembioinfo commented Dec 18, 2025

Description of Changes

Bug Fixes

Code Quality Improvements

Reduced __init__ Complexity

Eliminated Duplicate Code

Improved Error Handling

Performance Improvements

Security Improvements

Code Style

Configuration

Documentation

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

Reduced `init` Complexity