feat(dockerMan): add Save button to save template without recreating container or requiring authoring mode#2589
Conversation
…ting container or authoring mode
WalkthroughAdded a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
wandercone
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php (1)
95-101: Minor UX consideration: no visual confirmation of save success.The implementation is functionally correct—the template is written before this block, and
done()navigates back. However, unlike the normal apply flow (which shows log output) or dry run (which shows XML/command), the save-only flow provides no feedback that the save succeeded.Consider whether a brief success toast or visual confirmation would improve user confidence. This is optional and doesn't block the feature.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php` around lines 95 - 101, When $save_only is true the current branch just echoes "<script>done();</script>" and immediately exits so users get no visual confirmation; modify the $save_only branch (referencing the $save_only variable and the done() call) to render a brief success confirmation before navigating back—e.g., output a small toast or a short HTML message (or include the existing log.htm content via readfile(".../log.htm") with a "Save successful" line) then call done(); ensure the UX is non-blocking and visually shows success prior to the existing navigation flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php`:
- Around line 95-101: When $save_only is true the current branch just echoes
"<script>done();</script>" and immediately exits so users get no visual
confirmation; modify the $save_only branch (referencing the $save_only variable
and the done() call) to render a brief success confirmation before navigating
back—e.g., output a small toast or a short HTML message (or include the existing
log.htm content via readfile(".../log.htm") with a "Save successful" line) then
call done(); ensure the UX is non-blocking and visually shows success prior to
the existing navigation flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f83510ec-eced-4fab-8876-86c03cf53b1d
📒 Files selected for processing (1)
emhttp/plugins/dynamix.docker.manager/include/CreateDocker.php
|
Curious why you want this. If anything, I'd switch the existing functionality save to actually save the template instead of just displaying it. The "dry run" was to simplify the creation of default templates for CA template repositories. |
As I understand authoring mode's primary intention is the creation of a new template and not modification to an existing one. That is really just a dry-run, add values and make sure they all work type of flow. Take my current situation; Seafile. The migration from v12 to v13 requires changes to a bunch of variables across a myriad of templates. Regardless of container state pre-edit, the apply button brings up the container. Well a service like Seafile needs to be brought up in a specific order as to not break dependencies. With the save button I can edit the templates as required and then manually bring up the services in the proper order ensuring an issue free upgrade of Seafile. |
|
I can get some minor template values to take effect on the drop down menu by just hitting save. They're minor ones though -> notably shell. I think that this should only show in authoring mode. An additional button that would display by default regardless of the authoring configured for specific use cases I think could lead to some confusion for users if they inadvertently hit save instead of Apply. But, this is a great addition and should've been done years ago. Thanks. |
|
With that, why is the Authoring Mode not only hidden behind Advanced View but also only visible when docker is stopped? |
|
With Authoring View enabled, you're in Advanced view by default whenever you edit a container. Why the setting can only be changed with the service stopped? My best answer is that I didn't write that code as there's no reason why the service needs to be stopped. |
2 participants
Adds a Save button (visible to all users) that writes the template XML to disk and navigates back, skipping image pull and container recreate/restart. The existing authoring-mode button is renamed from Save to Dry Run to reflect its actual behavior.
Summary by CodeRabbit