Conversation
- Remove nested x-data component and merge copy functionality into main serverSetup() - Consolidate duplicate IP validation regex into validateIP() method - Remove unused 'copied' property - Remove redundant 'typeof toast !== undefined' checks (toast is always available) - Move global helper functions into serverSetup() as methods (createServerDoc, pingServer, verifyAndCreateServer) - Replace string concatenation with template literals - Remove unnecessary parentheses around string literals - Fix indentation throughout serverSetup() function for better readability This refactoring eliminates code duplication, removes dead code, prevents global namespace pollution, and improves overall maintainability without changing functionality.
- Remove redundant 'typeof toast !== undefined' checks (4 instances) - Fix indentation throughout script section after linter corruption - Improve code readability and consistency
- Remove redundant 'typeof toast !== undefined' check - Remove redundant local prevStep() method (use inherited from parent) - Move verify_domain() global function into domainSetup() as verifyDomain() method - Remove empty init() function and x-init directive from HTML - Replace hardcoded step 4 navigation with increment operator for flexibility
summary.html: - Remove redundant 'typeof toast !== undefined' checks (19 instances) - Remove unused buildingImage property and assignments - Remove redundant local prevStep() method (use inherited from parent) self-host.html: - Remove massive dead code from frappeInstaller() parent component - Delete unused properties: showAdvanced, generatedSSHKey, showCustomAppDialog, customAppForm, domain, domainVerified, domainStatusMessage, isLoading, deploying, deployComplete, deployStep, deploymentStatus, copied - Delete unused methods: copyToClipboard(), toggleApp(), getSelectedAppsNames(), startDeployment(), visitSite() - These were legacy code from old implementation before child components were properly isolated - Parent now only contains: stepNames, store getters, nextStep(), prevStep() This refactoring eliminates ~100 lines of legacy dead code that was creating confusion and making the codebase harder to maintain. All functionality has been properly moved to child components where it belongs.
apps.html: - Add x-cloak to custom app dialog to prevent flash on page load - Fixes FOUC (Flash of Unstyled Content) where dialog appeared briefly before Alpine.js initialized self_host.py: - Replace frappe.db.get_list() with frappe.get_all() in get_user_servers() and get_apps() - Replace frappe.db.get_all() with frappe.get_all() in get_frappe_versions() - All three functions now consistently use frappe.get_all() following Frappe ORM best practices
📝 Walkthrough🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nano_press/templates/components/core/server.html (1)
131-131: Malformed SVG path data.The path contains
0 7which appears to be a typo. The standard spinner path typically has0 0 7at this position for the arc command.🐛 Proposed fix
- d="M4 12a8 8 0 018-8V0C5.373 0 0 5.373 0 12h4zm2 5.291A7.962 7.962 0 714 12H0c0 3.042 1.135 5.824 3 7.938l3-2.647z"> + d="M4 12a8 8 0 018-8V0C5.373 0 0 5.373 0 12h4zm2 5.291A7.962 7.962 0 014 12H0c0 3.042 1.135 5.824 3 7.938l3-2.647z">
🤖 Fix all issues with AI agents
In `@nano_press/templates/components/core/apps.html`:
- Around line 277-299: The current removeCustomApp removes the app locally even
if the backend delete (frappe_call to
'/api/method/nano_press.api.delete_custom_app') fails; change the flow in
removeCustomApp so that for apps with app.app_reference_id you only splice
this.localCustomApps and call this.syncToStore() after a successful backend
response (i.e., move the splice/sync into the try block after verifying
response?.message?.success), and let the catch bail without altering
localCustomApps; for apps without app_reference_id keep the immediate local
removal path. Ensure references are to removeCustomApp, this.localCustomApps,
frappe_call('/api/method/nano_press.api.delete_custom_app'), and
this.syncToStore().
In `@nano_press/templates/components/core/stepper.html`:
- Line 4: The progress line was rendered as a static full-width div; restore its
dynamic width by binding its inline style to the step state (use the template
variable currentStep) so the width is computed as (currentStep - 1) * 33.33% (or
equivalent expression in the templating language) instead of being full
width—update the <div class="absolute top-4 left-0 h-px bg-gray-900 z-0
transition-all duration-500"> element to include a dynamic style attribute that
sets width using currentStep while preserving existing classes and transition
behavior.
In `@nano_press/templates/components/core/summary.html`:
- Line 1: The destroy() method in summarySetup() is never invoked by Alpine so
the polling interval leaks; modify summarySetup() to register a cleanup using
Alpine's $cleanup (e.g., in init() call $cleanup(() =>
clearInterval(pollingInterval))) or return a cleanup callback from the component
so the interval (the pollingInterval created inside summarySetup()/init()) is
cleared when the component is torn down; ensure you reference the polling
interval variable used in summarySetup() (and remove or keep the destroy()
method as needed).
🧹 Nitpick comments (7)
nano_press/templates/components/core/stepper.html (1)
7-7: Z-index reduced fromz-10toz-0.This change lowers the stacking context for stepper items. Verify this doesn't cause visual overlap issues with other positioned elements.
nano_press/www/self_host.py (1)
24-29: Consider addingorder_byfor consistent ordering.Unlike
get_user_servers()andget_frappe_versions(), this query lacks anorder_byclause. This may result in inconsistent app ordering across page loads.♻️ Proposed fix
def get_apps(): return frappe.get_all( "Apps", filters={"is_public": 1, "enabled": 1}, fields=["name", "branch", "repo_url", "scrubbed_name", "frappe", "app_logo"], + order_by="name asc", )nano_press/templates/components/core/domain.html (1)
11-14: Redundant domain update:x-modeland@inputboth modify the same value.The input has
x-model="$store.installer.domain"which already handles two-way binding. The@input="updateDomain($event.target.value)"handler then sets$store.installer.domain = valueagain at line 59. This is redundant.Consider having
updateDomainonly handle thedomainVerifiedreset logic:♻️ Proposed fix
- updateDomain(value) { - if (this.$store.installer.domainVerified && value !== this.$store.installer.domain) { + updateDomain() { + if (this.$store.installer.domainVerified) { this.$store.installer.domainVerified = false; } - this.$store.installer.domain = value; },And update the input:
- `@input`="updateDomain($event.target.value)" /> + `@input`="updateDomain()" />nano_press/templates/components/core/server.html (2)
221-245: Consider handling duplicate server creation.If a user manually enters an IP that already exists in
user_servers,createServerDocwill attempt to create a duplicate. Consider checking for existing servers with the same IP before creation, or handling the duplicate error gracefully.
273-276:verifyAndCreateServeralways creates a new server record.When a user selects an existing server from the dropdown, the flow correctly skips this via the
isVerifiedcheck innextStep(). However, if a user manually types an IP matching an existing server, a duplicate will be created. This ties into the previous comment about duplicate handling.nano_press/templates/components/core/summary.html (2)
328-371: Consider splitting document creation and submission.
createFrappeSiteDocboth creates and submits the document in one method. If the submission fails after creation, you may end up with an orphaned draft document. Consider handling this case or making the operations more atomic.
514-609: Async callback insetIntervalmay swallow errors.The interval callback is
asyncbut thetry/catchinside only logs errors without stopping the interval on unexpected failures. If the API consistently fails (e.g., network issues), the interval will keep polling indefinitely.Consider adding a retry limit or clearing the interval after repeated failures:
♻️ Proposed pattern for retry limiting
// Add to component state failedPollCount: 0, maxPollFailures: 10, // In the catch block of the polling callback: } catch (error) { console.error('Failed to poll deployment status:', error); this.failedPollCount++; if (this.failedPollCount >= this.maxPollFailures) { clearInterval(this.pollIntervalId); this.pollIntervalId = null; this.deploying = false; this.deploymentStatus = 'Lost connection to server. Please refresh to check status.'; toast('Polling stopped after repeated failures', { type: 'warning' }); } }
- Add --frappe-branch version-15 flag to bench init command
…letion summary.html: - Fix polling interval memory leak by registering cleanup with Alpine's $cleanup() - Previously destroy() method was never invoked, causing interval timers to persist - Cleanup callback now properly clears pollIntervalId when component unmounts stepper.html: - Restore dynamic progress bar width based on currentStep - Bar now animates to (currentStep - 1) * 33.33% instead of static full width - Preserves existing transition behavior for smooth step progression apps.html: - Fix removeCustomApp to only remove apps locally after successful backend deletion - For apps with app_reference_id: only splice/sync after verifying backend success - For apps without app_reference_id: keep immediate local removal - Prevents UI state desync when backend delete operations fail
2 participants
Summary
Major refactoring of the self-hosting installer UI components to eliminate legacy dead code, remove defensive coding patterns, and improve overall code maintainability. This PR cleans up ~300 lines of unnecessary code across 5 files without changing any functionality.
Changes by File
1. server.html (commit 204d376)
x-datacomponent and merge copy functionality into mainserverSetup()validateIP()methodcopiedpropertytypeof toast !== 'undefined'checks (toast always available)serverSetup()as methods:createServerDoc(),pingServer(),verifyAndCreateServer()2. apps.html (commit 3b774f3)
typeof toast !== 'undefined'checks (4 instances)3. domain.html (commit 9ad1abc)
typeof toast !== 'undefined'checkprevStep()method (use inherited from parent)verify_domain()global function intodomainSetup()asverifyDomain()methodinit()function andx-initdirective from HTML4. summary.html + self-host.html (commit a581c3a)
summary.html:
typeof toast !== 'undefined'checks (19 instances!)buildingImageproperty and assignmentsprevStep()method (use inherited from parent)self-host.html:
frappeInstaller()parent componentshowAdvanced,generatedSSHKey,showCustomAppDialog,customAppForm,domain,domainVerified,domainStatusMessage,isLoading,deploying,deployComplete,deployStep,deploymentStatus,copiedcopyToClipboard(),toggleApp(),getSelectedAppsNames(),startDeployment(),visitSite()stepNames, store getters,nextStep(),prevStep()Why These Changes Matter
Before this refactoring:
typeof toast !== 'undefined'checks littered throughout despite toast always being availableAfter this refactoring:
Testing
All functionality remains unchanged:
Impact
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.