fix: v10.6 Curation UX Fixes & Security#183
Merged
Conversation
…t verification - SUMMARY-v10.6.md: Full investigation results from code archaeology + live Playwright testing - REQUIREMENTS-v10.6.md: 5 requirements (R1-R5) with acceptance criteria and file targets - ROADMAP.md: Added phases 83-85 for v10.6 milestone - STATE.md: Updated with investigation results table and next steps
Phase 83: Status Creation Fix & Security - 1 plan in 1 wave - 1 parallel, 0 sequential - Ready for execution
- Move resetStatusForm() to start of showStatusModify() BEFORE data load - Remove resetStatusForm() from onModifyStatusModalShow() @show handler - Prevents entity_id deletion after loadStatusByEntity() populates it - Fixes HTTP 500 error on status change (missing entity_id in POST body)
- Update axios from 1.13.4 to 1.13.5 - Fixes CVE-2026-25639 (DoS via prototype pollution in mergeConfig) - No breaking changes, patch version bump
Tasks completed: 3/3 - Fix status form reset ordering in ModifyEntity.vue - Update axios to 1.13.5 (CVE-2026-25639 patch) - Verify "approve both" checkbox restoration SUMMARY: .planning/phases/83-status-creation-fix-security/83-01-SUMMARY.md
…sion
JSON null values from frontend become R NULL which tibble::as_tibble()
rejects ("All columns in a tibble must be vectors"). Use purrr::compact()
to strip NULLs before conversion. Discovered during Playwright E2E testing.
Phase 83 verified via E2E Playwright testing: - Status change returns HTTP 200 (was 500) - entity_id present in POST body - "Approve both" checkbox visible on ApproveReview - axios patched to 1.13.5 (CVE-2026-25639) - Backend NULL handling fix in status_create
Phase 84: Status Change Detection - Implementation decisions documented - Phase boundary established
Phase 84: Status Change Detection - Standard stack identified (Vue 3, TypeScript, Bootstrap-Vue-Next) - Architecture patterns documented (hasChanges computed, modal @hide events) - Pitfalls catalogued (modal timing, reactive proxy comparison) - Code examples from LlmPromptEditor, useReviewForm patterns - Missing review_change indicator identified in ApproveStatus
Phase 84: Status Change Detection - 3 plan(s) in 2 wave(s) - 2 parallel (01, 03), 1 sequential (02 depends on 01) - Ready for execution
- Split single task into Task 1 (status form via composable) and Task 2 (review form via local Options API tracking) for clearer isolation of the two different patterns - Document status_change indicator exclusion from ModifyEntity with architectural justification (entity endpoint lacks the data, single-entity view lacks scanning context, backend change out of scope) - Update must_haves to remove status_change indicator truth and add review form unsaved-changes truth - Add key_links for review form change detection wiring
- Add hasChanges computed property to useStatusForm tracking category_id, comment, problematic - Add hasChanges computed property to useReviewForm tracking synopsis, comment, phenotypes, variationOntology, publications, genereviews - Snapshot loaded data in loadedData ref after loadStatusData, loadStatusByEntity, loadReviewData - Clear loadedData on resetForm - Export hasChanges in both composables - Pattern follows LlmPromptEditor.vue hasChanges implementation
…forms - Add statusLoadedData and reviewLoadedData snapshots in data() - Add hasStatusChanges and hasReviewChanges computed properties - Snapshot loaded data in loadStatusInfo() and loadReviewInfo() - Silent skip submit when no changes detected - Add unsaved-changes confirmation on modal close - Reset loaded data in resetForm()
- Create useStatusForm.spec.ts with 8 test cases - Add 6 change detection tests to useReviewForm.spec.ts - Test hasChanges false when no data loaded - Test hasChanges false immediately after load - Test hasChanges true when fields change (category_id, comment, problematic, synopsis, publications) - Test hasChanges detects whitespace changes (exact comparison) - Test hasChanges false after resetForm - All 19 tests pass (14 new + 5 existing BUG-05 tests)
…view_change indicator - Add statusLoadedData snapshot in data() - Add hasStatusChanges computed property - Snapshot loaded data in loadStatusInfo() - Silent skip submit when no changes detected - Add unsaved-changes confirmation on modal close - Reset loaded data in resetForm() - Add review_change indicator overlay on status edit button - Add review_change to legend items
Tasks completed: 2/2 - Task 1: Add hasChanges to useStatusForm and useReviewForm - Task 2: Add unit tests for change detection SUMMARY: .planning/phases/84-status-change-detection/84-01-SUMMARY.md
…plan Tasks completed: 2/2 - Task 1: ApproveReview status and review forms change detection - Task 2: ApproveStatus change detection + review_change indicator SUMMARY: .planning/phases/84-status-change-detection/84-03-SUMMARY.md
- Expose hasStatusChanges from useStatusForm composable - Add silent skip guard in submitStatusChange() when no changes detected - Add @hide handler with unsaved changes confirmation dialog - Prevent unnecessary API calls when status unchanged
- Add reviewLoadedData state for tracking original values - Add hasReviewChanges computed property with array comparison - Snapshot loaded data in getReview() after API load - Add silent skip guard in submitReviewChange() - Add @hide handler with unsaved changes confirmation - Reset reviewLoadedData in resetForm() - Prevent unnecessary API calls when review unchanged
Tasks completed: 2/2 - Task 1: Wire hasChanges into ModifyEntity status form - Task 2: Add local change detection to review form SUMMARY: .planning/phases/84-status-change-detection/84-02-SUMMARY.md
3 plans executed across 2 waves: - 84-01: hasChanges composables + 14 unit tests - 84-02: ModifyEntity change detection wiring - 84-03: ApproveReview/ApproveStatus + review_change indicator Verification: 16/16 must-haves passed
Phase 85: Ghost Entity Cleanup & Prevention - Implementation decisions documented - Phase boundary established
Phase 85: Ghost Entity Cleanup & Prevention - Entity creation already uses atomic transactions (831ac85) - No API code changes needed for prevention - Cleanup via GitHub issue with SQL commands - Foreign key dependency on entity 1249 must be resolved first
Phase 85: Ghost Entity Cleanup & Prevention - 1 plan in 1 wave - 1 parallel, 0 sequential - Ready for execution
…_full - Add 4 new unit tests for error-handling contracts: * 409 response when duplicate entity detected * 400 response on validation error * 500 response on transaction error with rollback message * 500 response on unexpected error - Tests use local_mocked_bindings to mock dependencies (no database needed) - Verify each error path in entity-service.R lines 545-717 - Source db-helpers.R for db_with_transaction
Tasks completed: 2/2 - Task 1: Updated GitHub issue sysndd-administration#2 (prevention already implemented) - Task 2: Enhanced entity service tests with rollback contract verification SUMMARY: .planning/phases/85-ghost-entity-cleanup-prevention/85-01-SUMMARY.md
When approving a review or status, other pending reviews/statuses for the same entity are now automatically dismissed. Adds explicit dismiss buttons to ApproveReview and ApproveStatus tables. Filters dismissed items from pending lists. Adds ConfirmDiscardDialog component and integration tests for the dismiss/auto-dismiss workflow.
…rmatting Replace local_mocked_bindings() with mockery::stub() in rollback contract tests since the API is not a package (pkgload unavailable). Fix rlang::abort() condition signaling for proper class matching. Apply prettier formatting to useStatusForm.ts.
Add Phase 86 context, plan, and summary for the dismiss/auto-dismiss capability. Update roadmap (83-85→83-86), state, and requirements with R6 and decisions D86-01-01 through D86-01-03.
Archived: - milestones/v10.6-ROADMAP.md - milestones/v10.6-REQUIREMENTS.md Deleted (fresh for next milestone): - REQUIREMENTS-v10.6.md Updated: - MILESTONES.md (new entry) - PROJECT.md (requirements → Validated, context updated) - ROADMAP.md (v10.6 collapsed into history) - STATE.md (reset for next milestone)
CI starts with an empty MySQL database — ndd_entity_status and ndd_entity_review tables don't exist. Follows the same pattern as test-integration-rereview-sync.R which creates its table before tests.
There was a problem hiding this comment.
Pull request overview
This PR delivers SysNDD v10.6 curation workflow fixes and security hardening: preventing unnecessary status/review submissions via change detection, adding dismiss/auto-dismiss behavior for pending approval queues, and updating axios to address a reported CVE.
Changes:
- Add change detection (
hasChanges) to status/review forms and wire “silent skip” + discard-confirmation into curation modals. - Add dismiss + auto-dismiss-sibling behavior for pending statuses/reviews (frontend UX + backend filtering and repository updates).
- Update axios in lockfile and expand backend tests around entity creation rollback/error-handling contracts.
Reviewed changes
Copilot reviewed 42 out of 43 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| app/src/views/curate/composables/useStatusForm.ts | Adds loadedData snapshot + hasChanges computed for status change detection. |
| app/src/views/curate/composables/useReviewForm.ts | Adds loadedData snapshot + hasChanges computed for review change detection. |
| app/src/views/curate/composables/tests/useStatusForm.spec.ts | New unit tests validating status-form change detection behavior. |
| app/src/views/curate/composables/tests/useReviewForm.spec.ts | Extends existing tests to cover review-form change detection. |
| app/src/views/curate/ModifyEntity.vue | Wires silent-skip + discard confirmation and fixes status modal reset timing. |
| app/src/views/curate/ApproveStatus.vue | Adds dismiss UX + auto-dismiss messaging and status change detection + discard confirmation. |
| app/src/views/curate/ApproveReview.vue | Adds dismiss UX + auto-dismiss messaging and status/review change detection + discard confirmation. |
| app/src/components/ui/ConfirmDiscardDialog.vue | New reusable modal component for unsaved-change discard confirmation. |
| app/package-lock.json | Locks axios (and related deps) to patched versions. |
| api/tests/testthat/test-unit-entity-service.R | Adds svc_entity_create_full error-handling/rollback contract tests. |
| api/services/approval-service.R | Filters out dismissed items (approving_user_id IS NULL) for approve-all batches. |
| api/functions/status-repository.R | Compacts NULLs on create + auto-dismiss sibling pending statuses on approve. |
| api/functions/review-repository.R | Auto-dismiss sibling pending reviews on approve. |
| api/endpoints/status_endpoints.R | Filters dismissed statuses out of pending list responses. |
| api/endpoints/review_endpoints.R | Filters dismissed reviews out of pending list responses. |
| .planning/research/SUMMARY-v10.6.md | Research writeup for v10.6 root causes and validation. |
| .planning/phases/86-dismiss-autodismiss-pending/* | Phase plan/context/summary for dismiss + auto-dismiss feature. |
| .planning/phases/85-ghost-entity-cleanup-prevention/* | Phase plan/context/research/verification for ghost entity prevention + tests. |
| .planning/phases/84-status-change-detection/* | Phase plan/research/verification for change detection rollout. |
| .planning/phases/83-status-creation-fix-security/* | Phase plan/summary/verification for status creation fix + axios update. |
| .planning/milestones/v10.6-* | Milestone roadmap + requirements archive for v10.6. |
| .planning/STATE.md | Updates overall project state to reflect v10.6 completion. |
| .planning/ROADMAP.md | Adds v10.6 milestone to roadmap. |
| .planning/PROJECT.md | Updates project overview with v10.6 shipped scope. |
| .planning/MILESTONES.md | Adds v10.6 milestone entry. |
Files not reviewed (1)
- app/package-lock.json: Language not supported
Comments suppressed due to low confidence (2)
app/src/views/curate/ApproveStatus.vue:1219
submitStatusChange()setsthis.isBusy = truebefore the PUT, but on the error pathisBusyis never reset. That can leave the table/modal permanently busy and also disables the unsaved-changes confirmation gate (!this.isBusy). Add afinallythat restoresisBusy(and consider doing it before returning from errors).
app/src/views/curate/ApproveReview.vue:1836submitStatusChange()setsthis.isBusy = truefor the save, but neither the approved nor unapproved branch resets it when the request fails. This can leave the UI stuck busy and bypass unsaved-changes confirmation on subsequent closes. Add afinallythat restoresisBusy(or restore it in each catch path).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
2
to
14
| <BModal | ||
| :id="modalId" | ||
| ref="modal" | ||
| centered | ||
| size="sm" | ||
| header-bg-variant="warning" | ||
| header-text-variant="dark" | ||
| no-close-on-backdrop | ||
| no-close-on-esc | ||
| :title="title" | ||
| role="alertdialog" | ||
| :aria-label="title" | ||
| > |
There was a problem hiding this comment.
ConfirmDiscardDialog can currently be closed via the modal header “X” (default BModal behavior). If the user closes this dialog that way, the parent views keep pendingDiscardTarget set, so the next attempt to close the underlying edit modal will bypass confirmation and discard changes. Prevent header-close (e.g., hide-header-close) and/or treat any dialog close that isn’t an explicit Discard as “keep editing” (emit keep-editing on @hide/@hidden).
Comment on lines
1701
to
1760
| @@ -1621,6 +1759,13 @@ export default { | |||
| } | |||
| }, | |||
There was a problem hiding this comment.
submitReviewChange() sets this.isBusy = true, but if the PUT fails the catch block doesn’t restore it. Since isBusy is also used to suppress the unsaved-changes confirmation (!this.isBusy), a failed save can leave the view stuck in a state where closes won’t warn and the UI stays busy. Reset isBusy in a finally (and/or immediately in the catch).
- Add hide-header-close to ConfirmDiscardDialog to prevent header X
from orphaning pendingDiscardTarget state
- Add finally { isBusy = false } to submitReviewChange (ApproveReview)
- Add finally { isBusy = false } to submitStatusChange (ApproveReview)
- Add finally { isBusy = false } to submitStatusChange (ApproveStatus)
Without these, a failed API call would leave isBusy=true permanently,
which makes the table stuck in busy state and bypasses the unsaved
changes confirmation on modal close.
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
1 participant
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
resetStatusForm()destroyedentity_idafter data load; also fixed backend NULL→tibble crashsvc_entity_create_full()usesdb_with_transaction()preventing future orphans; enhanced rollback contract testsPhases
Stats
Test plan
npm run type-check— 0 errorsnpm run lint— 0 errors