Start fresh when re-adding a reviewer

engine/app/controllers/coplan/api/v1/review_requests_controller.rb

@@ -34,10 +34,14 @@ def create
 
         def update
           if ActiveModel::Type::Boolean.new.cast(params[:request_again])
-            Reviews::Request.call(plan: @plan, reviewer: @review_request.reviewer, actor: current_user,
-              requirement: params[:requirement].presence || @review_request.requirement, api_token: @api_token)
+            Reviews::Request.again(request: @review_request, actor: current_user,
+              requirement: params[:requirement].presence, api_token: @api_token)
           else
-            @plan.with_lock { @review_request.update!(requirement: params.require(:requirement)) }
+            @plan.with_lock do
+              # The request may have been removed while waiting for the plan lock.
+              @review_request.reload
+              @review_request.update!(requirement: params.require(:requirement))
+            end
             Reviews::Changed.call(request: @review_request, event_type: "review.requested", actor_token_id: api_token_id,
               payload: { "action" => "requirement_changed", "requirement" => @review_request.requirement })
           end

engine/app/controllers/coplan/review_requests_controller.rb

@@ -1,7 +1,7 @@
 module CoPlan
   class ReviewRequestsController < ApplicationController
     before_action :set_plan
-    before_action :set_review_request, only: [ :update, :destroy, :respond, :compare ]
+    before_action :set_review_request, only: [ :update, :respond, :compare ]
     rescue_from Reviews::Respond::Conflict, with: :render_conflict
     rescue_from ActiveRecord::RecordInvalid, with: :render_invalid
 
@@ -17,21 +17,26 @@ def suggestions
 
     def create
       authorize!(@plan, :update?)
-      attributes = params.expect(review_request: [ :reviewer_id, :requirement ])
+      attributes = params.expect(review_request: [ :reviewer_id, :requirement, :replace_withdrawn ])
       reviewer = User.find(attributes[:reviewer_id])
       request = Reviews::Request.call(plan: @plan, reviewer: reviewer, actor: current_user,
-        requirement: attributes[:requirement].presence || "required", create_only: true)
+        requirement: attributes[:requirement].presence || "required", create_only: true,
+        replace_withdrawn: ActiveModel::Type::Boolean.new.cast(attributes[:replace_withdrawn]))
       render json: Reviews::Serialize.call(request), status: :created
     end
 
     def update
       authorize!(@plan, :update?)
       attributes = params.expect(review_request: [ :requirement, :request_again ])
       if ActiveModel::Type::Boolean.new.cast(attributes[:request_again])
-        Reviews::Request.call(plan: @plan, reviewer: @review_request.reviewer, actor: current_user,
-          requirement: attributes[:requirement].presence || @review_request.requirement)
+        Reviews::Request.again(request: @review_request, actor: current_user,
+          requirement: attributes[:requirement].presence)
       else
-        @plan.with_lock { @review_request.update!(requirement: attributes.fetch(:requirement)) }
+        @plan.with_lock do
+          # The request may have been removed while waiting for the plan lock.
+          @review_request.reload
+          @review_request.update!(requirement: attributes.fetch(:requirement))
+        end
         Reviews::Changed.call(request: @review_request, event_type: "review.requested",
           payload: { "action" => "requirement_changed", "requirement" => @review_request.requirement })
       end
@@ -40,7 +45,12 @@ def update
 
     def destroy
       authorize!(@plan, :update?)
-      Reviews::Withdraw.call(request: @review_request, actor: current_user)
+      if ActiveModel::Type::Boolean.new.cast(params.dig(:review_request, :remove))
+        Reviews::Remove.call(plan: @plan, request_id: params[:id])
+      else
+        set_review_request
+        Reviews::Withdraw.call(request: @review_request, actor: current_user)
+      end
       head :no_content
     end
 

engine/app/javascript/controllers/coplan/review_tray_controller.js

@@ -11,7 +11,9 @@ export default class extends Controller {
     // Shared streams carry an invalidation signal, never another viewer's
     // forms, tokens or personal task status.
     window.Turbo.StreamActions["coplan-review-changed"] ||= function () {
-      document.getElementById(this.getAttribute("target"))?.dispatchEvent(new CustomEvent("coplan:reviews-changed"))
+      document.getElementById(this.getAttribute("target"))?.dispatchEvent(new CustomEvent("coplan:reviews-changed", {
+        detail: { removedRequestId: this.getAttribute("data-removed-review-request-id") }
+      }))
     }
     const tray = this.element.querySelector(".review-tray")
     if (tray) tray.showPopover()
@@ -132,6 +134,8 @@ export default class extends Controller {
   confirmApproval(event) {
     this.responseUrl = event.currentTarget.dataset.url
     this.approvalRevision = this.viewedRevision()
+    this.approvalInvalidated = false
+    this.approveButtonTarget.disabled = false
     this.approvalErrorTarget.textContent = ""
     this.approveButtonTarget.textContent = `Approve v${this.approvalRevision}`
     this.approvalTitleTarget.textContent = `Approve v${this.approvalRevision}?`
@@ -142,6 +146,12 @@ export default class extends Controller {
     this.approvalTarget.close()
   }
 
+  invalidateApproval() {
+    this.approvalInvalidated = true
+    this.approveButtonTarget.disabled = true
+    this.approvalErrorTarget.textContent = "This review request changed. Close this dialog and reopen approval for the current request. Your note is preserved."
+  }
+
   responseIntent(url, revision, decision, note) {
     this.responseIntents ||= {}
     const fields = JSON.stringify([url, String(revision), note.trim()])
@@ -154,20 +164,36 @@ export default class extends Controller {
     if (this.responseIntents[decision] === intent) delete this.responseIntents[decision]
   }
 
+  responseIsCurrent(url) {
+    const parsed = new URL(url, document.baseURI)
+    return !this.reviewWithdrawn && this.hasCommentFormTarget && this.commentFormTarget.action === parsed.href &&
+      !this.removedRequestIds?.has(parsed.pathname.split("/").at(-2))
+  }
+
   async approve(event) {
     event.preventDefault()
+    if (this.approvalInvalidated) { this.invalidateApproval(); return }
     await this.withBusy(event.target, this.approvalErrorTarget, async () => {
-      const intent = this.responseIntent(this.responseUrl, this.approvalRevision, "approved", this.approvalNoteTarget.value)
-      const result = await this.write(this.responseUrl, "POST", { review_response: { revision: this.approvalRevision, decision: "approved", note: this.approvalNoteTarget.value, idempotency_key: intent.key } })
+      const url = this.responseUrl
+      const intent = this.responseIntent(url, this.approvalRevision, "approved", this.approvalNoteTarget.value)
+      const result = await this.write(url, "POST", { review_response: { revision: this.approvalRevision, decision: "approved", note: this.approvalNoteTarget.value, idempotency_key: intent.key } })
       this.confirmIntent("approved", intent)
-      const synced = !result.comment_thread_id || await this.refreshThreads(result.comment_thread_id)
+      const current = () => !this.approvalInvalidated && this.responseUrl === url && this.responseIsCurrent(url)
+      if (!current()) return
+      const synced = !result.comment_thread_id || await this.refreshThreads(result.comment_thread_id, current)
+      // Removal can also arrive after the server saved the old approval but
+      // before its result and comments arrive. Keep the dialog and its note.
+      if (!current()) return
       this.approvalTarget.close()
       this.approvalNoteTarget.value = ""
       if (this.hasNoticeTarget) this.noticeTarget.textContent = `Your approval of v${this.approvalRevision} is recorded.` + (synced ? "" : " Refresh the page to see your comment.")
       const tray = this.element.querySelector(".review-tray")
       if (tray) delete tray.dataset.reconsidering
       await this.refresh()
     }, "Recording…")
+    // A refresh can invalidate this dialog while the original POST is in
+    // flight. Keep it invalid after the busy-state cleanup restores buttons.
+    if (this.approvalInvalidated && this.approvalTarget.open) this.invalidateApproval()
   }
 
   async post(event) {
@@ -177,25 +203,34 @@ export default class extends Controller {
     if (form.closest('[data-dictation-busy="true"]') || form.querySelector('[data-dictation-busy="true"]')) return
     const error = form.querySelector('[role="alert"]')
     await this.withBusy(form, error, async () => {
-      const intent = this.responseIntent(form.action, this.changesRevision, "changes_requested", input.value)
-      const result = await this.write(form.action, "POST", { review_response: { revision: this.changesRevision, decision: "changes_requested", note: input.value, idempotency_key: intent.key } })
+      const url = form.action
+      const current = () => form.isConnected && this.responseIsCurrent(url)
+      const intent = this.responseIntent(url, this.changesRevision, "changes_requested", input.value)
+      const result = await this.write(url, "POST", { review_response: { revision: this.changesRevision, decision: "changes_requested", note: input.value, idempotency_key: intent.key } })
       this.confirmIntent("changes_requested", intent)
+      if (!current()) return
       // Feedback lives in the ordinary discussion collection.
-      const synced = await this.refreshThreads(result.comment_thread_id)
+      const synced = await this.refreshThreads(result.comment_thread_id, current)
+      if (!current()) return
       this.noticeTarget.textContent = "Changes requested. Your feedback is in the plan’s comments." + (synced ? "" : " Refresh the page to see your comment.")
       input.value = ""
       this.collapse()
       await this.refresh()
     }, "Posting…")
   }
 
-  async refreshThreads(threadId) {
+  async refreshThreads(threadId, current = () => true) {
+    if (!this.element.isConnected) return false
+    // Another reviewer's removal can also make this collection snapshot stale.
+    const removalSequence = this.removalSequence
     const editor = document.querySelector('[data-coplan--inline-editor-comments-url-value]')
     if (!editor) return false
     try {
       const response = await fetch(editor.getAttribute("data-coplan--inline-editor-comments-url-value"), { credentials: "same-origin", headers: { Accept: "text/html" } })
       if (response.ok) {
-        const incoming = new DOMParser().parseFromString(await response.text(), "text/html")
+        const html = await response.text()
+        if (!this.element.isConnected || !current() || removalSequence !== this.removalSequence) return false
+        const incoming = new DOMParser().parseFromString(html, "text/html")
         const id = `comment_thread_${threadId}`
         const thread = incoming.getElementById(id)
         if (thread && !document.getElementById(id)) document.getElementById("plan-threads")?.append(thread)
@@ -211,7 +246,14 @@ export default class extends Controller {
     return false
   }
 
-  async refresh() {
+  async refresh(event) {
+    const removedId = event?.detail?.removedRequestId
+    if (removedId) {
+      this.removalSequence = (this.removalSequence || 0) + 1
+      this.removedRequestIds ||= new Set()
+      this.removedRequestIds.add(removedId)
+      if (this.approvalTarget.open && this.responseUrl?.endsWith(`/${removedId}/respond`)) this.invalidateApproval()
+    }
     const sequence = ++this.refreshSequence
     try {
       const response = await fetch(this.urlValue, { credentials: "same-origin", headers: { Accept: "text/html" } })
@@ -220,6 +262,8 @@ export default class extends Controller {
       if (sequence !== this.refreshSequence || !this.element.isConnected) return
       const tray = incoming.querySelector(".review-tray")
       const existingTray = this.element.querySelector(".review-tray")
+      const responsePath = tray?.querySelector('[data-coplan--review-tray-target="commentForm"]')?.getAttribute("action")
+      if (this.approvalTarget.open && responsePath !== this.responseUrl) this.invalidateApproval()
       const wasWithdrawn = this.reviewWithdrawn
       this.reviewWithdrawn = !tray
       if (!tray && existingTray) {
@@ -234,6 +278,12 @@ export default class extends Controller {
         }
       }
       if (tray && existingTray) existingTray.dataset.completed = tray.dataset.completed
+      if (tray && existingTray) {
+        // Keep the draft and dictation state, but submit new feedback to the
+        // current request. An already-issued fetch retains its original URL.
+        const form = tray.querySelector('[data-coplan--review-tray-target="commentForm"]')
+        if (form && this.hasCommentFormTarget) this.commentFormTarget.action = form.action
+      }
       if (tray && !this.element.querySelector(".review-tray")) this.element.append(tray)
       const currentTray = this.element.querySelector(".review-tray")
       if (tray && currentTray) {
@@ -267,12 +317,13 @@ export default class extends Controller {
     event.preventDefault()
     if (!this.reviewerIdTarget.value) { this.assignmentErrorTarget.textContent = "Choose a person from the results first."; return }
     await this.withBusy(event.target, this.assignmentErrorTarget, async () => {
-      await this.write(this.urlValue, "POST", { review_request: { reviewer_id: this.reviewerIdTarget.value, requirement: event.target.querySelector('[type="checkbox"]').checked ? "required" : "optional" } })
+      await this.write(this.urlValue, "POST", { review_request: { reviewer_id: this.reviewerIdTarget.value, requirement: event.target.querySelector('[type="checkbox"]').checked ? "required" : "optional", replace_withdrawn: true } })
       this.searchTarget.value = ""; this.reviewerIdTarget.value = ""; this.resultsTarget.replaceChildren()
       this.closeAssignment()
       event.target.querySelector('[type="checkbox"]').checked = true
       this.addReviewerButtonTarget.focus({ preventScroll: true })
       await this.refresh()
+      await this.refreshThreads(null)
     }, "Requesting…")
     this.assignButtonTarget.disabled = !this.reviewerIdTarget.value
   }
@@ -354,7 +405,7 @@ export default class extends Controller {
 
   requestAgain(event) { this.manage(event, "PATCH", { review_request: { request_again: true } }) }
   changeRequirement(event) { this.manage(event, "PATCH", { review_request: { requirement: event.currentTarget.dataset.requirement } }) }
-  withdraw(event) { this.manage(event, "DELETE") }
+  withdraw(event) { this.manage(event, "DELETE", { review_request: { remove: true } }) }
 
   async manage(event, method, body) {
     if (this.managing) return
@@ -372,7 +423,9 @@ export default class extends Controller {
     try {
       await this.withBusy(button, this.managementErrorTarget, async () => {
         await this.write(button.dataset.url, method, body)
-        await this.refresh()
+        const removedId = method === "DELETE" && body?.review_request?.remove ? new URL(button.dataset.url, document.baseURI).pathname.split("/").at(-1) : null
+        await this.refresh(removedId ? { detail: { removedRequestId: removedId } } : undefined)
+        if (removedId) await this.refreshThreads(null)
       }, "Updating…")
     } finally {
       this.managing = false

engine/app/services/coplan/reviews/changed.rb

@@ -14,9 +14,17 @@ def call
         AgentEvents::Publish.call(plan: request.plan, event_type: event_type, actor_token_id: actor_token_id,
           payload: { "review_request_id" => request.id, "reviewer_id" => request.reviewer_id }.merge(payload))
         Notifications::BroadcastBadges.call(user_ids: [ request.reviewer_id, request.plan.created_by_user_id ])
-        # No viewer-relative HTML or CSRF tokens on a shared stream. Each
-        # open tab fetches its own controls, preserving its current draft.
-        Broadcaster.custom_action_to(request.plan, action: "coplan-review-changed", target: "plan-review-controls", html: "")
+        # No viewer-relative HTML or CSRF tokens on a shared stream. Announce
+        # removals before replacing comments so late HTTP results cannot put
+        # deleted decision labels back while the controls fetch is pending.
+        attrs = payload["removed"] ? { "data-removed-review-request-id" => request.id } : {}
+        Broadcaster.custom_action_to(request.plan, action: "coplan-review-changed", target: "plan-review-controls", html: "", attrs: attrs)
+        if payload["removed"]
+          # Public notes survive removal; their deleted review decisions must
+          # disappear from the discussion labels in open tabs as well.
+          Broadcaster.replace_to(request.plan, target: "plan-general-comment-list", partial: "coplan/plans/general_comment_list",
+            locals: { threads: request.plan.comment_threads.with_kept_comments.includes(:comments).order(:created_at) })
+        end
       end
     end
   end

engine/app/services/coplan/reviews/remove.rb

@@ -0,0 +1,22 @@
+module CoPlan
+  module Reviews
+    # A later assignment gets a fresh ID, so old response URLs stay invalid.
+    class Remove
+      def self.call(plan:, request_id:)
+        request = plan.with_lock do
+          request = plan.review_requests.find_by(id: request_id)
+          if request
+            unless plan.plan_events.where(event_type: "review_requested", after_value: request.id).exists?
+              RecordRequest.call(request: request)
+            end
+            request.destroy!
+          end
+          request
+        end
+        Changed.call(request: request, event_type: "review.requested",
+          payload: { "action" => "removed", "removed" => true }) if request
+        request
+      end
+    end
+  end
+end