Signal data improvements #927

labkey-klum · 2025-09-11T18:27:31Z

Rationale

Address a few problems / inefficiencies with the signal data module that were discovered during client troubleshooting sessions:

No feedback given to the user during assay run creation if an error was reported by the server during the Experiment.saveBatch.api request.
Validation of the run data files and creation of the ExpData objects on the server was performed for each row in the run results. Refactor the getSignalDataResourceAction.api action to handle all files in the run.
Miscellaneous clean up.

…t once

labkey-klum · 2025-09-11T18:49:47Z

resultupdate.js contains code that basically implements the update view for a result row. There is no UI connection to this view and I'm wondering @labkey-nicka if you had any insight to it's history and whether it might be something we can delete?

labkey-nicka · 2025-09-11T19:29:50Z

signalData/resources/views/mockSignalDataWatch.html

+                        paths : paths,
+                        files : fileNames
+                    },
+                    success: function (response) {


nit: Consider using LABKEY.Utils.getCallbackWrapper() instead so this code isn't responsible for decoding the response. Same for changes in resultupdate.js.

good call, done.

labkey-nicka · 2025-09-11T21:28:04Z

signalData/src/org/labkey/signaldata/SignalDataController.java

+            int idx = 0;

-            if (null != resource)
+            for (String path : form.getPaths())


Should this be transacted?

yes, agreed

labkey-nicka · 2025-09-11T21:28:32Z

signalData/src/org/labkey/signaldata/SignalDataController.java

+            FileContentService svc = FileContentService.get();
+            TableInfo ti = ExpSchema.TableType.Data.createTable(new ExpSchema(getUser(), c), ExpSchema.TableType.Data.toString(), null);
+            QueryUpdateService qus = ti.getUpdateService();
+            int maxUrlSize = ExperimentService.get().getTinfoData().getColumn("DataFileURL").getScale();


Use ExpDataTable.Column.DataFileUrl.name() here

labkey-nicka · 2025-09-11T21:30:22Z

signalData/src/org/labkey/signaldata/SignalDataController.java

+            List<Map<String, String>> results = new ArrayList<>();
            Container c = getContainer();
+            FileContentService svc = FileContentService.get();
+            TableInfo ti = ExpSchema.TableType.Data.createTable(new ExpSchema(getUser(), c), ExpSchema.TableType.Data.toString(), null);


Can you just use the ExperimentService.get().getTinfoData() here instead? And get the update service from it?

No, that's the DbSchema table not the QuerySchema table, but ExpSchema(getUser(), getContainer()).getDatasTable() is more concise.

labkey-nicka · 2025-09-11T21:31:23Z

resultupdate.js contains code that basically implements the update view for a result row. There is no UI connection to this view and I'm wondering @labkey-nicka if you had any insight to it's history and whether it might be something we can delete?

If it isn't referenced, then I'd say remove it. Could be worth tracking down in version control where it went defunct.

signalData/resources/views/mockSignalDataWatch.html

Copilot

Pull Request Overview

This PR addresses inefficiencies in the signal data module by refactoring the API to handle multiple files in a single request, adding error handling for run creation failures, and performing miscellaneous cleanup.

Refactored API to process all files in a run at once instead of per-row processing
Added error feedback to users when run creation fails
Cleaned up imports and improved error handling throughout

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
SignalDataController.java	Refactored API to handle multiple files in one request, updated form structure and validation
uploadResultFiles.js	Added error handling for failed run save operations
UploadLog.js	Updated to use batch file processing and improved error handling
resultupdate.js	Modified to use new batch API format
mockSignalDataWatch.html	Updated to work with new batch API response format

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-17T22:24:57Z

signalData/src/org/labkey/signaldata/SignalDataController.java

+                                }
+                                else
+                                {
+                                    throw new ValidationException(String.format("The data file URL is too long to store in the database (max %d).", maxUrlSize));


The error message should provide more context about what failed. Consider including the actual file name or path that caused the issue to help users identify which specific file has the problem.

Suggested change

throw new ValidationException(String.format("The data file URL is too long to store in the database (max %d).", maxUrlSize));

throw new ValidationException(String.format("The data file URL for file '%s' (path: '%s') is too long to store in the database (max %d).", file.getName(), path, maxUrlSize));

Copilot · 2025-09-17T22:24:58Z

signalData/src/org/labkey/signaldata/SignalDataController.java

                                }
                            }
+                            else
+                                throw new RuntimeException(String.format("Unexpected number of rows returned for DataFileUrl '%s': %d", url, rows.size()));


This error message could be more helpful by explaining what the expected number of rows should be (which appears to be 1 based on the context). Consider changing to 'Expected 1 row but got %d rows for DataFileUrl...' to make the issue clearer.

Suggested change

throw new RuntimeException(String.format("Unexpected number of rows returned for DataFileUrl '%s': %d", url, rows.size()));

throw new RuntimeException(String.format("Expected 1 row but got %d rows for DataFileUrl '%s'", rows.size(), url));

Copilot · 2025-09-17T22:24:58Z

signalData/resources/web/signaldata/UploadView/uploadResultFiles.js

-                //TODO: Should probably do something here...
-            }
+            failure: LABKEY.Utils.getCallbackWrapper(function(json, response, opts) {
+                LABKEY.Utils.alert('Error', 'Unable to save run : ' + response.exception);


The error message concatenation could result in 'Unable to save run : undefined' if response.exception is undefined. Consider using optional chaining or a fallback message like 'Unable to save run: ' + (response.exception || 'Unknown error').

Suggested change

LABKEY.Utils.alert('Error', 'Unable to save run : ' + response.exception);

LABKEY.Utils.alert('Error', 'Unable to save run : ' + (response.exception || 'Unknown error'));

Copilot · 2025-09-17T22:24:58Z

signalData/resources/web/signaldata/UploadView/UploadLog.js

-            }
-            , scope: this
+            failure: LABKEY.Utils.getCallbackWrapper(function(json, response, opts) {
+                LABKEY.Utils.alert('Error', 'Unable to resolve file resources : ' + response.exception);


Similar to the previous error message, this could display 'undefined' if response.exception is not set. Consider adding a fallback: 'Unable to resolve file resources: ' + (response.exception || 'Unknown error').

Suggested change

LABKEY.Utils.alert('Error', 'Unable to resolve file resources : ' + response.exception);

LABKEY.Utils.alert('Error', 'Unable to resolve file resources : ' + (response.exception || 'Unknown error'));

labkey-klum added 3 commits September 9, 2025 10:46

do a better job of showing errors that might occur during run import

fbef3d3

refactor getSignalDataResourcesAction to handle multiple file paths a…

913c2f0

…t once

update comment

090f2e1

labkey-klum requested a review from labkey-nicka September 11, 2025 18:47

labkey-nicka approved these changes Sep 11, 2025

View reviewed changes

labkey-nicka assigned labkey-klum Sep 11, 2025

labkey-klum added 2 commits September 11, 2025 16:24

code review feedback

c18b8b8

Merge branch 'develop' into fb_issue_53786

f854a91

labkey-nicka reviewed Sep 16, 2025

View reviewed changes

signalData/resources/views/mockSignalDataWatch.html Outdated Show resolved Hide resolved

code review feedback

03cf08a

labkey-klum requested a review from Copilot September 17, 2025 22:24

Copilot AI reviewed Sep 17, 2025

View reviewed changes

labkey-danield added 2 commits September 19, 2025 13:49

Add error tests for Signal Data tests. Cover Issue 53786

bca56c1

Add some comments.

e36c4f0

labkey-klum merged commit 111622c into develop Sep 22, 2025
5 checks passed

labkey-klum deleted the fb_issue_53786 branch September 22, 2025 19:25

	throw new ValidationException(String.format("The data file URL is too long to store in the database (max %d).", maxUrlSize));
	throw new ValidationException(String.format("The data file URL for file '%s' (path: '%s') is too long to store in the database (max %d).", file.getName(), path, maxUrlSize));

	throw new RuntimeException(String.format("Unexpected number of rows returned for DataFileUrl '%s': %d", url, rows.size()));
	throw new RuntimeException(String.format("Expected 1 row but got %d rows for DataFileUrl '%s'", rows.size(), url));

	LABKEY.Utils.alert('Error', 'Unable to save run : ' + response.exception);
	LABKEY.Utils.alert('Error', 'Unable to save run : ' + (response.exception \|\| 'Unknown error'));

	LABKEY.Utils.alert('Error', 'Unable to resolve file resources : ' + response.exception);
	LABKEY.Utils.alert('Error', 'Unable to resolve file resources : ' + (response.exception \|\| 'Unknown error'));

Signal data improvements #927

Signal data improvements #927

Uh oh!

Conversation

labkey-klum commented Sep 11, 2025

Rationale

Uh oh!

labkey-klum commented Sep 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

labkey-nicka commented Sep 11, 2025

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants