Skip to content

Add Confirmation Before Dev Tool Removal and Modification [AARD-2033] #1265

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 18 commits into
base: dev
Choose a base branch
from

Conversation

ryanzhangofficial
Copy link
Member

@ryanzhangofficial ryanzhangofficial commented Aug 8, 2025

Task

Add a confirmation dialog when removing a Dev Tools–placed scoring zone, allowing the user to either remove it temporarily (current behavior) or permanently by also deleting it from the Dev Tools cache.

Jira Ticket

Symptom

Currently if you remove a protected zone or scoring zone from the configure field panel that is placed by the dev tool it will remove it until the next refresh. Only the dev tool is able to cache scoring zone placement.

Solution

Popup a confirmation window when we remove a scoring zone that was placed by dev tools, and give us the option of removing it from dev tools as well (saving to cache).

Verification

  • Tested temporary removal + refresh
  • Tested permanent removal + refresh
Screenshot 2025-08-08 102909

Before merging, ensure the following criteria are met:

  • All acceptance criteria outlined in the ticket are met.
  • Necessary test cases have been added and updated.
  • A feature toggle or safe disable path has been added (if applicable).
  • User-facing polish:
    • Ask: "Is this ready-looking?"
  • Cross-linking between Jira and GitHub:
    • PR links to the relevant Jira issue.
    • Jira ticket has a comment referencing this PR.

@ryanzhangofficial ryanzhangofficial self-assigned this Aug 8, 2025
@ryanzhangofficial ryanzhangofficial added the ui/ux Relating to user interface, or in general, user experience label Aug 8, 2025
@ryanzhangofficial ryanzhangofficial marked this pull request as ready for review August 8, 2025 17:26
@ryanzhangofficial ryanzhangofficial requested review from a team as code owners August 8, 2025 17:26
Copy link
Member

@rutmanz rutmanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be better to say that the zones were defined in the field file/asset file/mirabuf file and avoid all reference to the developer tools. I think that, in theory, the developer tools are used by us to add this information to the cached files that people download, and so we shouldn't expect that they've used them / know they're there.

Also, this doesn't handle modifying the zones

@ryanzhangofficial ryanzhangofficial marked this pull request as draft August 12, 2025 18:33
@ryanzhangofficial ryanzhangofficial changed the title Add Confirmation Before Dev Tool Removal [AARD-2033] Add Confirmation Before Dev Tool Removal and Modification [AARD-2033] Aug 13, 2025
@ryanzhangofficial ryanzhangofficial marked this pull request as ready for review August 14, 2025 20:42
@ryanzhangofficial ryanzhangofficial force-pushed the ryan/2033/add-confirmation-before-dev-tool-removal branch from 6f24ff1 to 988f39a Compare August 14, 2025 20:44
actionType?: "removal" | "modification"
}

const DevtoolZoneRemovalModal: React.FC<DevtoolZoneRemovalModalProps> = ({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably be renamed if this is being generalized

@ryanzhangofficial ryanzhangofficial force-pushed the ryan/2033/add-confirmation-before-dev-tool-removal branch from 1f46d28 to 273982e Compare August 18, 2025 22:07
Copy link
Member

@rutmanz rutmanz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

otherwise lgtm

@AlexD717
Copy link
Member

Why are we only adding this confirmation for scoring zones?

Also Biome test fail

@ryanzhangofficial
Copy link
Member Author

Why are we only adding this confirmation for scoring zones?

Also Biome test fail

As noted in comments in code, protected zones are not supported yet.

@AlexD717
Copy link
Member

Why are we only adding this confirmation for scoring zones?

As noted in comments in code, protected zones are not supported yet.

Yeah, I can see that, I am just wondering why we are only adding this for scoring zones. Are we planning to add this for protected zones later on? From a user perspective it feels weird if they are only getting the popup for scoring zones, and nothing else.

@ryanzhangofficial
Copy link
Member Author

ryanzhangofficial commented Aug 19, 2025

Why are we only adding this confirmation for scoring zones?

As noted in comments in code, protected zones are not supported yet.

Yeah, I can see that, I am just wondering why we are only adding this for scoring zones. Are we planning to add this for protected zones later on? From a user perspective it feels weird if they are only getting the popup for scoring zones, and nothing else.

Yes that's the plan.

I think the order of merging should be this PR -> Zach's protected zone pr -> protected zone removal and modification

@AlexD717
Copy link
Member

Is it also intentional that you have to go into developer tools and press save to actually have scoring zones persist in cache? Currently spawning in a scoring zone and then refreshing doesn't have it persist. If this is the case, I am personally not a fan of this, and think that they should auto cache.

Screen.Recording.2025-08-19.at.9.29.14.AM.mov

@ryanzhangofficial ryanzhangofficial marked this pull request as draft August 19, 2025 18:48
@rutmanz
Copy link
Member

rutmanz commented Aug 19, 2025

I agree with @AlexD717, but this appears to be the current behavior on dev as well

@ryanzhangofficial
Copy link
Member Author

I agree with @AlexD717, but this appears to be the current behavior on dev as well

Yes I agree too, currently fixing this issue. It isn't a problem on prod so must've been an issue that came up throughout the summer.

@ryanzhangofficial ryanzhangofficial marked this pull request as ready for review August 20, 2025 17:41
@ryanzhangofficial
Copy link
Member Author

Would appreciate thoughts on this approach. I thought it would be appropriate because of the rewording of the modals, but at the same time it makes devtools seem less useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui/ux Relating to user interface, or in general, user experience
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants