fix(findings): allow admins/owners to set ready_for_review status#2940
Merged
Conversation
Root cause: DescriptionEditor stored `draft` and `mode` as independent
state synced to the `value` prop via a chain of 4 useEffects. When
switching from a strategy with content to one without, mode flipped to
'edit' and the resync effect's edit-mode guard blocked reloading the
original plan when switching back.
Rewrote the component to use a single `draft: string | null` state —
null means preview mode (render value directly from props), string
means edit mode. This eliminates the 4 sync effects entirely and makes
the state model trivial: enter edit → copy value into draft, save →
push draft up and clear, cancel → clear.
Strategy changes remount the editor via key={strategy} so internal
state initializes fresh from the new value.
Also hides the "Generate treatment plan" button for non-Mitigate
strategies since regeneration always produces a mitigation plan.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…eview The ready_for_review status was gated behind a role-string check that blocked auditors — including dual-role users (admin+auditor) who need to signal readiness as clients. Remove the restriction entirely; the endpoint already requires finding:update via RBAC, and only needs_revision/closed remain auditor-restricted. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…-role Restore the business rule that pure auditors cannot set ready_for_review (it's a client action). The exception is users who also hold admin/owner — they have a client role and should be able to signal readiness. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Role-string checks (admin, owner, auditor) don't account for custom roles. The endpoint already requires finding:update via RBAC — that's sufficient authorization. Only needs_revision/closed remain gated behind the auditor role. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
cubic analysis
1 issue found across 6 files
Confidence score: 3/5
- There is a concrete user-impacting risk in
apps/app/src/components/risks/treatment-plan/DescriptionEditor.tsx: clicking Regenerate can immediately discard unsaved draft edits, and a failed trigger leaves the user with lost in-progress content. - Given the severity (6/10) and high confidence (8/10), this is more than a cosmetic issue and introduces meaningful regression risk around editing workflows.
- Pay close attention to
apps/app/src/components/risks/treatment-plan/DescriptionEditor.tsx- prevent unsaved draft loss when regenerate fails or does not complete.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/app/src/components/risks/treatment-plan/DescriptionEditor.tsx">
<violation number="1" location="apps/app/src/components/risks/treatment-plan/DescriptionEditor.tsx:92">
P2: Regenerate now discards unsaved draft content immediately, so a failed trigger can erase the user's in-progress edits.</violation>
</file>
Linked issue analysis
Linked issue: CS-402: [BUG] - Risk treatment plan UI bug
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Risk treatment plan description persists when switching strategies (does not disappear or flip to empty when toggling between strategies). | The PR makes the DescriptionEditor state resilient to strategy toggles and ensures the editor remounts per-strategy (key={strategy}). A new unit test exercises switching strategies and asserts the correct description is shown. |
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| const handleSave = async () => { | ||
| if (!isDirty) { | ||
| setMode('preview'); | ||
| setDraft(null); |
Contributor
There was a problem hiding this comment.
P2: Regenerate now discards unsaved draft content immediately, so a failed trigger can erase the user's in-progress edits.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/app/src/components/risks/treatment-plan/DescriptionEditor.tsx, line 92:
<comment>Regenerate now discards unsaved draft content immediately, so a failed trigger can erase the user's in-progress edits.</comment>
<file context>
@@ -82,121 +60,55 @@ export function DescriptionEditor({
const handleSave = async () => {
if (!isDirty) {
- setMode('preview');
+ setDraft(null);
return;
}
</file context>
…s gating
Replace all role-string checks (userRoles.includes('auditor')) with
RBAC permission checks (finding:create). The controller resolves
permissions via RolesService and passes canCreateFindings boolean to
the service. Frontend uses hasPermission('finding', 'create').
This handles custom roles correctly — any role with finding:create
gets auditor-level status access (needs_revision, closed), while
roles with only finding:update get client-level access (open,
ready_for_review).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
1 issue found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/app/src/components/risks/treatment-plan/DescriptionEditor.tsx">
<violation number="1" location="apps/app/src/components/risks/treatment-plan/DescriptionEditor.tsx:92">
P2: Regenerate now discards unsaved draft content immediately, so a failed trigger can erase the user's in-progress edits.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
The @RequirePermission guard already enforces finding:create and
finding:delete via RBAC. The manual userRoles.includes('auditor')
checks were redundant and didn't account for custom roles.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@cubic-dev-ai re-review this |
Contributor
@Marfuen I have started the AI code review. It will take a few minutes to complete. |
Contributor
There was a problem hiding this comment.
cubic analysis
No issues found across 6 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Linked issue analysis
Linked issue: CS-402: [BUG] - Risk treatment plan UI bug
| Status | Acceptance criteria | Notes |
|---|---|---|
| Treatment plan description persists when switching strategies (no disappearing text) | PR description and summary state DescriptionEditor was rewritten to a single draft state and remounted via key={strategy} and that tests were added for persistence. However, the provided diffs do not include the DescriptionEditor/component changes that implement this behavior, so the evidence in the diff is ambiguous. | |
| Form remains on the Treatment plan (does not switch unexpectedly to Rationale) when toggling strategies | The PR claims to address UI toggling behavior as part of the DescriptionEditor changes. The intent is stated in the PR summary, but the specific component changes that prevent the form flipping are not present in the supplied diffs, so verification from code is incomplete. | |
| Hide Generate/Regenerate buttons for non‑Mitigate strategies | The cubic summary explicitly says Generate/Regenerate are hidden for non‑Mitigate and that button visibility tests were added. The supplied diffs do not include the UI code that hides those buttons or the corresponding tests, so the claim is present but direct diff evidence is missing. |
Contributor
|
🎉 This PR is included in version 3.64.3 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
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
2 participants
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
userRoles.includes('auditor')) with RBAC permission checks (finding:createpermission) across the entire findings feature@RequirePermissionguards already enforce this via RBACfinding:creategets auditor-level status access, a role with onlyfinding:updategets client-level accessTest plan
findings.service.spec.tsfrom running🤖 Generated with Claude Code