[obenno]

Summary

• migrate scatter rendering to a modular deck.gl pipeline, including lasso/tooltip/layout/view-state modules and tests
• integrate DuckDB-backed data paths and module updates across the Shiny app
• expand webR-based plotting/data transfer flow and update frontend dependencies/assets

Validation plan

• devtools::check()
• packer::bundle()
• targeted JS tests for scatter modules
• manual performance and UX checks with large datasets (100K/500K/1M+ cells)

Notes

• this branch includes substantial generated assets (inst/app/www/*) and lockfile updates (renv.lock, package-lock.json)
• please review large binary/generated changes with extra care

Closes #1

[obenno]

/opencode
Review this pull request:

• Check for code quality issues
• Look for potential bugs
• Suggest improvements

[opencode-agent]

Code Review Summary

I've reviewed this substantial PR that migrates the scatter rendering to deck.gl and integrates DuckDB/webR. Here are my findings:

---

🐛 Critical Bugs

1. Hardcoded assay name in fct_duckdb.R:57

   m <- LayerData(object, assay = "RNA", layer = l)  # BUG: Should be `assay`, not "RNA"

   This breaks when processing non-RNA assays (e.g., "ADT", "SCT").

2. Typo in documentation comment (fct_duckdb.R:207)

   • "Udpate" should be "Update"

---

⚠️ Code Quality Issues

1. Excessive console logging (53 instances)

   • Production code contains debug console.log statements in:
     • srcjs/index.js (40+ instances)
     • srcjs/modules/webr.js (11 instances)
     • srcjs/modules/featureSparkLine.js (2 instances)
   • Recommendation: Remove or wrap in process.env.NODE_ENV !== 'production' checks

2. Poor error handling in R

   • Multiple stopifnot() calls without descriptive error messages (e.g., fct_duckdb.R:48, mod_Convert2Duckdb.R:34)
   • Users will see unhelpful errors like "length(dbTables)==0 is not TRUE"

3. Commented-out code

   • Large blocks of commented code in deckScatter.js:216-234 should be removed

4. Inconsistent naming conventions

   • Some files use camelCase (scatterModel.js), others use snake_case conventions in R

---

🔧 Suggested Improvements

1. Add proper error messages

   # Instead of:
   stopifnot(length(dbTables)==0, overwrite)
   
   # Use:
   if (length(dbTables) > 0 && !overwrite) {
     stop("Database already contains tables. Set overwrite=TRUE to replace.")
   }

2. Add test coverage for edge cases

   • Tests exist for scatter modules but missing for:
     • DuckDB query functions with empty results
     • Large dataset handling (1M+ cells)
     • WebR memory exhaustion scenarios

3. Performance optimizations

   • Consider adding timeouts to long-running DuckDB queries
   • The scatterModel.js buildPlotData() function iterates through all cells - verify it handles 1M+ cells efficiently

4. Documentation

   • Several new exported functions in fct_duckdb.R have examples marked \dontrun{} - add runnable examples or vignettes

---

✅ Positive Aspects

1. Well-structured modular architecture - The scatter pipeline is cleanly separated into focused modules (model, renderer, layout, etc.)
2. Good test coverage for scatter modules with descriptive test cases
3. Performance-conscious - Uses TypedArrays, debouncing, and proper cleanup in clear() method
4. Excellent AGENTS.md documentation - Clear development guidelines and performance requirements
5. Proper resource cleanup - Uses on.exit() in R and disconnect()/purge() patterns in JS

---

Action Items Before Merge

• Fix hardcoded "RNA" in fct_duckdb.R:57
• Remove or guard debug console.log statements
• Add descriptive error messages to stopifnot calls
• Run devtools::check() and packer::bundle() as specified in validation plan

The code quality is generally high with good architecture, but the critical bug and excessive logging should be addressed before merging.

opencode session  |  github run

[obenno]

R codes will be refactored later.