Fixed the no-JavaScript archive UI and stopped uncaught errors leaking detail - #11
Merged
Merged
Conversation
…lently ArchiveProjectsAction.execute skipped a projectIds value that would not parse or would not load, logged a warning and carried on, and the request still reported success. A researcher who selected ten projects and archived nine was told nothing. The 11 billed projects with no tblBilledProject row are exactly this case. Track a skippedAny flag alongside deniedAny, failedAny and hasFutureTimeAny, and add error.project.archiveskipped to the report, so a partial batch says so. Not tested. See TESTS in ai-uwpr-webapp. Co-Authored-By: Claude <noreply@anthropic.com>
…Script front.jsp hid the archived list in a div revealed only by the #toggle_archived handler, and started both bulk buttons disabled and enabled them only from updateArchiveButton. With JavaScript off the archived list, Unarchive Selected and Archive Selected never appeared or enabled. * The archived list is now a native <details>/<summary>, which expands and collapses with no script. The #toggle_archived handler is gone * Both submit buttons start enabled in the HTML. The script still disables each until a checkbox in its table is checked, as an enhancement. Submitting an empty selection is already handled by ArchiveProjectsAction Not tested. See TESTS in ai-uwpr-webapp. Co-Authored-By: Claude <noreply@anthropic.com>
A SQLException, or any uncaught exception, reached the browser through the container's default error page, which prints the exception message. For the project search that message was the SQL itself, with its table aliases and column list. The search injection is already bound (PR #9), but any other SQLException on any page leaks the same way. * Added WebRoot/error.jsp, a self-contained page that logs the detail with a reference and shows the user only a generic message and that reference * Registered it in web.xml for java.lang.Throwable and for status 500. DTD 2.3 requires the error-page entries after welcome-file-list and before the taglibs Not tested. See TESTS in ai-uwpr-webapp. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The public error endpoint permits fabricated error logs, references can collide, and one failure message is misleading.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Improves archive controls without JavaScript, reports skipped archive operations, and adds generic server-error handling.
Changes:
- Replaces JavaScript-only archive disclosure with native
<details>. - Reports projects skipped during bulk archive operations.
- Logs uncaught errors while showing users a reference ID.
File summaries
| File | Description |
|---|---|
WebRoot/WEB-INF/web.xml |
Registers generic exception and HTTP 500 handling. |
WebRoot/pages/internal/front.jsp |
Enables no-JavaScript archive controls. |
WebRoot/error.jsp |
Adds sanitized error output and server-side logging. |
src/PRMessageResources.properties |
Adds the skipped-project message. |
src/org/yeastrc/www/project/ArchiveProjectsAction.java |
Tracks and reports skipped archive selections. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
* Moved error.jsp under WEB-INF so only a container error dispatch reaches it. At the web root a direct request logged a spurious ErrorPage entry. * Used a UUID for the error reference instead of currentTimeMillis, which two failures in the same millisecond could share. * Reworded error.project.archiveskipped from "could not be found" to "could not be loaded", since the skip also covers a load failure or an invalid project type. Co-Authored-By: Claude <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
<details>andbuttons enabled in the HTML. With JavaScript on, the script still gates the buttons on selection.
success.
error.jsplogs an uncaught exception under a reference and shows the user only thatreference, so the project-search SQL no longer reaches the browser.
Test plan
error.jspshows a reference with no stack trace or SQL, detail logged server-sideCo-Authored-By: Claude noreply@anthropic.com